Skip to content

New configuration for third_party_auth & shibboleth/SAML SSO#8155

Merged
bradenmacdonald merged 13 commits intoopenedx:feature/shibboleth-tpafrom
open-craft:shibboleth-2
Jun 12, 2015
Merged

New configuration for third_party_auth & shibboleth/SAML SSO#8155
bradenmacdonald merged 13 commits intoopenedx:feature/shibboleth-tpafrom
open-craft:shibboleth-2

Conversation

@bradenmacdonald
Copy link
Copy Markdown
Contributor

Description
This PR contains three major changes:

  1. This PR refactors third_party_auth to use keyed ConfigurationModels for most configuration. This has the following benefits:
    • Auth provider configuration can be changed at runtime (no need to reboot server, fast and easy to make changes)
    • Auth provider configuration is edited with the Django admin UI, which is much friendlier than editing lms.(auth|env).json files.
    • Now administrators can easily configure their instances to use any of the python-social-auth OAuth2 backends, without writing a single line of code. Before, only Google/Facebook/LinkedIn were available, and adding more required writing new Python classes within a Django app.
    • The third_party_auth code is considerably simplified (lots of code removed from settings.py and provider.py)
  2. Shibboleth/SAML provider metadata is now fetched, parsed, and stored in the database by a manage.py command.
  3. Shibboleth/SAML providers can be configured to require an eduPersonEntitlement. This can be used, for example, to only allow a third party University's students to authenticate with SSO but not guests or alumni (who still have valid Shibboleth credentials from that University).

This PR is against the Shibboleth/SAML SSO feature branch, feature/shibboleth-tpa.

Notes:

  • Configuring Google/Facebook/LinkedIn has changed: you must enter the name ("Google"), icon ("fa-google-plus"), key, and secret into the Django admin UI. You also must choose the backend ("google-oauth2") from a dropdown menu.
  • When upgrading an existing installation, the data migration will copy the third party auth settings over from ~/lms.auth.json to the new ConfigurationModel.
  • All features should work whether the FEATURES flag ENABLE_COMBINED_LOGIN_REGISTRATION is true or false.

Coming in the future:

  • I'd like to get this approach reviewed now before I get too carried away writing tests, so new tests will come later, either as a separate PR or part of this PR.
  • Set up the manage.py saml pull task to run via celery beat

Screenshots:

New SAML Provider configuration UI:
New SAML Provider configuration UI

Sandbox (Updated May 26):
There is a demo at http://sandbox5.opencraft.com/admin/third_party_auth/ - user name 'admin', ping Braden via HipChat or IRC or email for the password.

Reviewers:
Code: @smarnach, @cpennington

Test Instructions:
Provider Configuration:

  1. Go to http://sandbox5.opencraft.com/admin/third_party_auth/oauth2providerconfig/
  2. Click "Add"
  3. Click "Enabled", set icon to "fa-google-plus", Name to "Google" backend to "google-oauth2", client ID and secret to "test", and hit save.
  4. A "Sign in with Google" button should now be visible at http://sandbox5.opencraft.com/login if opened in an incognito window
  5. Back on the admin change list for OAuth provider configuration, click the "Update" link next to the Google entry, then uncheck "Enabled" and save changes.
  6. Refresh the login page seen in an incognito window. The Google button should immediately be gone.

Provider configuration migration:

  1. On a devstack or other instance that is currently configured to use Google/Facebook/LinkedIn as a social auth provider, check out this branch and run paver update_db.
  2. Go to http://localhost:8000/admin/third_party_auth/oauth2providerconfig/
  3. The old provider[s] configuration should now be seen in the new admin screen
  4. Run the LMS and verify that third party auth via that provider[s] is working

SAML Configuration:

  1. (If on a local instance only - skip this step for the sandbox): Go to /third_party_auth/samlconfiguration/ and click "Add SAML Configuration". Check "Enabled". Under the "Private Key" field is an openssl command. Run that command, then paste the contents of the resulting "saml.key" into "Private Key" and "saml.crt" into "Public Key". Set "Entity ID" to a unique URL like "http://my-devstack.example.com". Edit "Organization Info" if desired.
  2. Go to /auth/saml/metadata.xml and verify that the metadata is working. Save this file to your desktop.
  3. Go to https://www.testshib.org/register.html and upload the metadata file from step 2.
  4. Go to /admin/third_party_auth/samlproviderconfig/
  5. Click "Add Provider Configuration".
  6. Check "Enabled", set "Icon Class" to "fa-university", set "Name" to "TestShib [something]" (e.g. "TestShib 1"), set "Backend" to the only option available, set "IdP Slug" to "testshib-1"
  7. Set "Entity ID" to "https://idp.testshib.org/idp/shibboleth" and "Metadata source" to "https://www.testshib.org/metadata/testshib-providers.xml"
  8. Set "Email attribute" to "urn:oid:1.3.6.1.4.1.5923.1.1.1.6"
  9. Save
  10. (If on a local instance): Run the command ./manage.py lms saml pull --settings=devstack and verify that it worked by checking /admin/third_party_auth/samlproviderdata/
  11. Open the LMS in an incognito window and try logging in using the new "TestShib 1" button.
  12. Edit the new SAML provider configuration and change the entity ID to something different/invalid.
  13. Go to the login page and click the "TestShib 1" button. Verify that a friendly error message is seen. (Note: friendly error will only be seen on the sandbox or if DEBUG is False).

@openedx-webhooks
Copy link
Copy Markdown

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=8155

@bradenmacdonald bradenmacdonald mentioned this pull request May 21, 2015
14 tasks
@bradenmacdonald bradenmacdonald force-pushed the shibboleth-2 branch 9 times, most recently from d19f8b9 to 9a46db9 Compare May 22, 2015 23:43
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "Does" → "Do"

@smarnach
Copy link
Copy Markdown
Contributor

@bradenmacdonald I've looked through the code once without finding any big issues. I don't have time left today, but I hope I will get to testing this tomorrow before the meeting. Is there anything in particular I could do to test this? Something like configuring a provider in the old version, migrating, and then configuring another one in the new version and trying whether they both work would be a go test.

@bradenmacdonald
Copy link
Copy Markdown
Contributor Author

@smarnach Yes, exactly - what you described would be a great test. And then try getting TestShib working again by configuring it via the admin interface - (the Shibboleth configuration won't automatically migrate - only Google, Facebook, or LinkedIn will).

@smarnach
Copy link
Copy Markdown
Contributor

@bradenmacdonald I can't make Google authentication work on the edx:feature/shibboleth-tpa branch. I followed the configuration instructions at http://johnmcox.blogspot.de/2014/05/getting-started-with-edx-third-party.html, but I get a 403 when trying to authenticate with Google. I then wanted to try whether this works on master, but going back to master results in this error:

south.exceptions.NoMigrations: Application '<module 'social.apps.django_app.default' from '/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/social/apps/django_app/default/__init__.pyc'>' has no migrations.

This is most likely related to going back to the older python-social-auth version that doesn't use migrations, but I don't know how to recover from it. Any ideas?

@bradenmacdonald
Copy link
Copy Markdown
Contributor Author

@smarnach Hmm. Try checking out shibboleth-2, then ./manage.py lms migrate default zero --settings=devstack, then checkout master and try again. If it's still not working, drop into a python REPL and type import social; print social.__file__, then go to that folder plus apps/django/default/ and delete any vestigial migrations folder found there. (The version of python-social-auth on master has no migrations). Ping me on IRC if still stuck.

@smarnach
Copy link
Copy Markdown
Contributor

@bradenmacdonald Thanks, reverting the migration while on shibboleth-2 worked. I already tried the same command while on master, which didn't work. (After switching to the master branch, I also had to do git clean -fx to remove the .pyc files of migrations not existing on master.)

On master, logging in via Google works, but it doesn't work on the edx:feature/shibboleth-tpa branch, and I get a 403 deep in the python-social-auth code instead. (I can provide a traceback if desired.)

Should I try a different branch as well?

@smarnach
Copy link
Copy Markdown
Contributor

@bradenmacdonald I also tried the shibboleth-2 branch, and I get the same traceback, available at https://gist.github.com/smarnach/2ebfbe947128e2432b67. It occurs after the Google page that asks for permission for offline access.

@smarnach
Copy link
Copy Markdown
Contributor

As discussed in IRC, the problem turned out to be that I had to enable the Google+ API in the Google developer console. The new version of python-social-auth used in the feature branch uses that API to authenticate, while the old version used some other API.

@smarnach
Copy link
Copy Markdown
Contributor

With exception of the Google+ API problem mentioned above, all tests went fine. I think it would be good if you could give more detailed testing instructions for the upstream reviewers. 👍 for the code from me.

@antoviaque
Copy link
Copy Markdown
Contributor

As discussed in IRC, the problem turned out to be that I had to enable the Google+ API in the Google developer console. The new version of python-social-auth used in the feature branch uses that API to authenticate, while the old version used some other API.

Will that be required during deployment to edge & prod? It would be useful to prepare a checklist of things devops will need to do upon deployment, along with the rest of the django configuration setup.

@bradenmacdonald
Copy link
Copy Markdown
Contributor Author

@antoviaque Yes, I'm tracking all of those issues in the feature branch PR: https://github.com/edx/edx-platform/pull/8140

@antoviaque
Copy link
Copy Markdown
Contributor

@bradenmacdonald Perfect - thanks!

@bradenmacdonald
Copy link
Copy Markdown
Contributor Author

@cpennington This is also now ready for your review.

@wmono
Copy link
Copy Markdown
Contributor

wmono commented Jun 5, 2015

Following an IRC conversation with @bradenmacdonald, it turns out that the TPA backends must appear first in AUTHENTICATION_BACKENDS (or, at least, prior to RateLimitModelBackend / django.contrib.auth.backends.ModelBackend) in order to work correctly. The expected way to do this is by setting THIRD_PARTY_AUTH_BACKENDS in aws.py, not by appending to AUTHENTICATION_BACKENDS. Otherwise, RateLimitModelBackend will raise a KeyError due to lack of a username kwarg.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a bit confusing that this looked like a ConfigurationModel, but then was actually just implemented as a raw model. Would it be easier to write it as a ConfigurationModel, but then simply not give users write permissions (so that only the pull command is actually writing to it)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did actually code it that way at first (using ConfigurationModel). But of the three columns that ConfigurationModel defines, I didn't really want any of them for this model. change_date is ambiguous and has auto_now_add=True, whereas in the model I wrote here we have a fetch_date which is more clear and which gets updated to the current time during every fetch where there are no changes to the data. changed_by doesn't apply, and enabled isn't necessary since providers should be enabled/disabled via the SAMLProviderConfig model instead.

So it seemed like ConfigurationModel would add things we didn't need, and didn't provide that much in the way of functionality code for this. It is also arguably confusing to use ConfigurationModel for something that isn't "configured" by the user.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

@cpennington
Copy link
Copy Markdown
Contributor

Some small notes, but once those are resolved, 👍

@bradenmacdonald
Copy link
Copy Markdown
Contributor Author

Thanks @cpennington ! If you think I should still change SAMLProviderData to be based on ConfigurationModel after reading my thoughts above, let me know. Otherwise I think I've addressed everything and I'll wait for the build to pass and then merge this to the feature branch.

@cpennington
Copy link
Copy Markdown
Contributor

@bradenmacdonald: Merge away!

bradenmacdonald added a commit that referenced this pull request Jun 12, 2015
New configuration for third_party_auth & shibboleth/SAML SSO
@bradenmacdonald bradenmacdonald merged commit 5325eaf into openedx:feature/shibboleth-tpa Jun 12, 2015
@bradenmacdonald bradenmacdonald deleted the shibboleth-2 branch June 12, 2015 16:33
@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Nov 10, 2015
@sarina sarina removed needs triage open-source-contribution PR author is not from Axim or 2U labels Nov 13, 2015
@openedx-webhooks
Copy link
Copy Markdown

Thanks for the pull request, @bradenmacdonald! I've created OSPR-2182 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jan 30, 2018
@smarnach
Copy link
Copy Markdown
Contributor

@edx-webhook Thanks, though I don't think this would have been strictly necessary for a PR that was merged more than two years ago. :)

@nedbat
Copy link
Copy Markdown
Contributor

nedbat commented Feb 2, 2018

Buh? ¯\_(ツ)_/¯

@nedbat nedbat removed needs triage open-source-contribution PR author is not from Axim or 2U labels Feb 2, 2018
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.

8 participants