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

Add dynamic requested attributes #332

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nsklikas
Copy link

@nsklikas nsklikas commented Jun 10, 2020

This MR introduces the feature of populating the requested attributes of the SAML backend's request based on the attributes that were requested from the frontend.

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?

@nsklikas nsklikas marked this pull request as draft June 12, 2020 08:36
@nsklikas
Copy link
Author

This PR has several problems:

For attributes requested and being mandatory
according to the eIDAS minimum data sets and [eIDAS-Attr-Profile] the attribute isRequired of
<eidas:RequestedAttribute> MUST be set to “true”. For all optional attributes according
to the eIDAS minimum data sets and [eIDAS-Attr-Profile] the attribute isRequired of<eidas:RequestedAttribute> MUST be set to “false”

So for each attribute we need to know not only the name but also whether it is required as well. We could require the operator to provide a list of attributes, in the format of the existing requested attributes, that will be if they were requested from the frontend. This behaviour will be useful in the case of eIDAS as we can't always request all the attributes with one call as sometimes we might want the natural person attributes and other times the legal person attributes.

  • If an internal attibute maps to more than one saml attributes the first one will be required. But I'm not sure this is actually a problem.

@nsklikas nsklikas force-pushed the master branch 2 times, most recently from b3a43da to 95d6a56 Compare July 1, 2020 12:17
@nsklikas
Copy link
Author

nsklikas commented Jul 1, 2020

I have worked the aforementioned problems, here are a couple of notes on the changes:

  • The operator will have to provide the attributes he wants to be requested, i.e.:
  dynamic_requested_attributes:
    - friendly_name: attr1
      required: True
    - friendly_name: attr2
      required: False

This is needed because eidas requires that we use the correct value for required (i.e. we can't have is_required=False for an attribute which is required and vice versa). The only other option would be to add a required field in the attribute mapping (?), but this doesn't sound very clean. Should required be optional?

  • Perhaps the _get_requested_attributes should be moved to the eIDAS backend. The SAML backend's function should be an empty function, until (and if) the saml extension is implemented in pysaml2.

@nsklikas nsklikas marked this pull request as ready for review July 3, 2020 07:35
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