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

Role / Group check #28

Closed
itsnagaraj opened this issue Jun 20, 2019 · 5 comments
Closed

Role / Group check #28

itsnagaraj opened this issue Jun 20, 2019 · 5 comments

Comments

@itsnagaraj
Copy link

With the existing implementation of SAML authenticator is it possible to restrict access by roles / groups that are returned in the SAML response? If not any plans to add in the near future?

@distortedsignal
Copy link
Contributor

@itsnagaraj - I think this is a pretty large feature. It involves adding one or more new configurable fields on the base object, and changing the documentation, and gating the login based on one (or more) new XPaths. There are also going to be a lot of tests with this feature.

That being said, I think this is an important feature to add. I think it's obviously an oversight on my part to not have included this in the first version of the project, and I think that it's a good thing that people are calling out for a better user experience when using this software.

I think because of the reasons listed above, this should probably go into some sort of 0.1.0 version - this is probably not a bugfix-type operation, but it's important.

Since I'm not getting a lot of time to work on this project at work, this might be slow, but I'll definitely add this to the v0.1.0 milestone.

@itsnagaraj
Copy link
Author

itsnagaraj commented Jun 24, 2019

Thanks for your response. I am thinking along these lines

2 configurable fields required to allow for role check

  • one to specify the xpath which will used to read the roles the user belongs to
  • one to specify the allowed roles
    xpath_role_location = Unicode(
        default_value='//saml:Role/text()',
        allow_none=True,
        config=True,
        help='''
        This is an XPath that specifies where the user's allowed roles is located in the
        SAML Assertion. This is to restrict users with certain roles granted by the administrator
        to have access to jupyterhub:

        {
            'ds'   : 'http://www.w3.org/2000/09/xmldsig#',
            'md'   : 'urn:oasis:names:tc:SAML:2.0:metadata',
            'saml' : 'urn:oasis:names:tc:SAML:2.0:assertion',
            'samlp': 'urn:oasis:names:tc:SAML:2.0:protocol'
        }
        '''
    )
    allowed_roles = List(
        default_value=None,
        allow_none=True,
        config=True,
        help='''
        This values specifies what roles are allowed to have access to jupyterhub.
        '''
    )    

Rules for check:

  • Only perform the check if allowed_roles are specified
  • If allowed_roles are specified then read the roles form SAML response using the xpath
  • if roles is empty then fail authentication
  • if roles is not empty and does not have any of the specified allowed_roles then fail authentication
            if self.allowed_roles and self.allowed_roles.len() > 0:
                user_roles = self._get_roles_from_saml_doc(signed_xml, saml_doc_etree)
                if user_roles.len() == 0:
                    return None
                if user_roles.len() > 0:
                    role_check = any(elem in self.allowed_roles for elem in user_roles)
                    if not role_check:
                        return None

@distortedsignal distortedsignal pinned this issue Jun 24, 2019
@0nebody
Copy link
Contributor

0nebody commented Jun 23, 2020

@distortedsignal I have added configurable role access but need advice on updating the test constants. It looks to be using responses from a cloud IAM provider. So changes can't be made that require the signature to be recalculated.

Any recommendations on the best way forward?

@distortedsignal
Copy link
Contributor

@0nebody if you want to open the PR, I'd be willing to work on getting the unit tests set up for you.

@distortedsignal
Copy link
Contributor

I think that after @0nebody's PR, this can be closed. I'll work on getting this out over the weekend.

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

No branches or pull requests

3 participants