-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
SAML support #616
SAML support #616
Conversation
This is a great PR, I'll take a look to it shortly. Many thanks! |
8493545
to
02ab628
Compare
|
||
# Helpful constants: | ||
OID_COMMON_NAME = "urn:oid:2.5.4.3" | ||
OID_EDU_PERSON_PRINCIPAL_NAME = "urn:oid:1.3.6.1.4.1.5923.1.1.1.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constants are specific to the education market - given the generic nature of SAML, should they default to more generic properties with the ability for a developer to either provide the mapping via settings and then use the mapping in get_user_details()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what I've done here. OID_EDU_PERSON_PRINCIPAL_NAME
and OID_EDU_PERSON_ENTITLEMENT
are defined as a convenience but neither are used by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there more generic defaults that could be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defaults I'm using here the ones listed below in get_user_details
on line 51. (e.g. OID_COMMON_NAME
for the user's name) I think they're the most generic, but please let me know if there are any better defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what I am confused about is that "urn:oid:1.3.6.1.4.1.5923.1.1.1.6" for example. I may be naive but isn't urn:oid:1.3.6.1.4.1.5923.1.1.1.6
defined specifically as an attribute for Principle Name for the purposes of University/Higher Education LDAP systems? If it was an LDAP for a corporation, it would use a different attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looking at this document which I think defines the spec as per SAML2.0.
https://www.oasis-open.org/committees/download.php/28042/sstc-saml-attribute-x500-cs-01.pdf
Are the additional attributes defined by different industries - in this case Universities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that one (eduPersonPrincipalName) is education-specific, but it's not actually used in this implementation at all. It's just defined as a string at the top here as a convenience for people who want to override the class and use that instead of the defaults, which are more generic values. You could delete that line above and this code would work unchanged.
@omab, just wondering if you'll be able to look at this soon? |
@bradenmacdonald, I've checked the code and it looks good, thanks for working on it. I think that leaving
Thanks |
Awesome, thanks for the review, @omab. I have added some documentation now - sorry for not including that originally. I'm not sure of the python-saml pull request status. I have pinged the guy who manages that repository to ask, but I haven't heard back yet. |
Thanks for those docs, I've merged the work, thanks! |
Awesome, thanks! When the python-saml PRs get merged upstream, I'll open a new PR to use the vanilla upstream version. |
Sounds great. |
@omab Would you mind releasing a versioned PyPi release now that this has merged? That way the edX project can require an official release rather than a development build in our project's next major release. |
@omab, @bradenmacdonald sorry for the delay. I will do a new release of the python-saml today. |
@bradenmacdonald, @pitbulk thanks for taking a look to it! |
This is a SAML2 backend for python-social-auth. See discussion in the feature request issue #388
Multiple SAML Provider (IdP) Support
Unlike all of the other backends, this one can be configured to work with many identity providers (IdPs). For example, a University that belongs to a Shibboleth federation may support authentication via ~100 partner universities, making it impractical to include a separate
.py
file and backend class for each IdP.Instead, the IdP configuration is done via the new
SOCIAL_AUTH_SAML_ENABLED_IDPS
setting.It should also work perfectly well with a single provider, and can be subclassed, e.g. for customization or if we wanted to add a separate class for something like OneLogin.
Dependency: python-saml
This new backend depends on a patched version of https://github.com/onelogin/python-saml . The patches are currently undergoing review in four separate PRs. One significant issue is that python-saml is currently compatible with python 2.7 only, but that will be changing in the near future (see discussion in #388).
This PR does add python-saml to the test requirement so that Travis CI can cover the new backend, but it does not add any new dependencies for just general use of python-social-auth, as I don't want users to have to install python-saml if they're not going to use this backend.
Notes
This is a contribution from edX, developed by OpenCraft as part of bringing integrated SAML support to the edX platform, via a new python-social-auth SAML backend.