Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Documentation] Optional multiple user_id_from_attrs #222

Conversation

peppelinux
Copy link
Member

@peppelinux peppelinux commented Apr 17, 2019

Hi,
I'm using SaToSa with two Saml Backends, through this additional feature. These two Saml2 backends gets different attributes from their IDPs (target endpoint). If one of them do not return any 'eduPersonTargetedID' but another id with a different name, then SaToSa goes in Exception Unknow Error because it cannot find eduPersonTargetedID as attribute name.

Using this patch I can consider multiple uid, if anyone of them could be unavailable SaToSa will try another One, following this internal_attributes example:

user_id_from_attrs: ["edupersontargetedid", "eppn", "thatname"]

I share as it came, if you will like to spend some more word on this...
thank you in advance

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

@peppelinux peppelinux changed the title Optional user_id_from_attrs Optional multiple user_id_from_attrs Apr 17, 2019
@peppelinux
Copy link
Member Author

peppelinux commented Sep 13, 2019

As previously discussed during an idpy call - we can close this PR with an example about getting the same result with the current asset

@c00kiemon5ter
Copy link
Member

Using this patch I can consider multiple uid, if anyone of them could be unavailable SaToSa will try another One, following this internal_attributes example:

user_id_from_attrs: ["edupersontargetedid", "eppn", "thatname"]

Instead of doing this here, you can set a new internal attribute that gets a value from all those attributes. Then this internal attribute will map to the user_id.

attributes:
  the_user_id:
    saml:
      - eduPersonTargetedID
      - eduPersonPrincipalName
      - mail

user_id_from_attrs:
  - the_user_id

Even more flexible would be to have a micro-service to set the internal_data.user_id to whatever you need, with the logic you need.

@c00kiemon5ter
Copy link
Member

Having said this, I'm not opposed to what you suggest here. I am not sure whether the semantics should be all those attributes need to have a value (what currently happens) or all those attributes may have a value (what you suggest). There is no way to indicate if it's one or the other, while it should be clear.

I am thinking that maybe user_id_from_attrs and user_id_to_attr should be removed entirely and internal_attributes should just be about the mapping.

These changes will probably come along the switch from friendly names to canonical names.

I will leave this open for now, but we should come back to it at some point. I have some ideas on how to generalize many of these decisions so that they can be configurable, but this needs to be tried out first with something small.

@peppelinux
Copy link
Member Author

Great, thank you for the example, I think that's fine. If this thread could improve in any way the general asset I'll follow you

@peppelinux
Copy link
Member Author

Got it to work as you explained.
We could close this issue

@peppelinux
Copy link
Member Author

Todo: convert this PR to Documentation, to help users to handle this use case.

@peppelinux peppelinux changed the title Optional multiple user_id_from_attrs [Documentation] Optional multiple user_id_from_attrs Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants