Cleanup of third-party login and auto-enrollment

[bradenmacdonald]

Description
This is a fix for CRI-9 ("Third party auth association fails to link on login/registration") and ECOM-1503. In order to fix that issue in a clean way, I did some refactoring of parts of the login and auto-enrollment flow.

There were three different parts of the code handling user login and optional auto-enrollment: the old login/register views, the third party auth pipeline, and the new combined "logistration" page. Each of these three had a completely separate implementation for auto-enrolling users into a course following login/registration. Also, none of them correctly completed the third party auth pipeline under all circumstances, meaning that users who do a "social" login often find that their social account never gets linked to their edX account.

Current Situation vs. New Situation

Detailed Description
First, it's important to understand the many different ways that users can login to or register for the LMS:

• Can be a potential user (not yet registered), a newly registered user (email is unverified, won't be able to login again until verifying), or a normal user
• Can be authenticated via first party auth (username/password), third_party_auth (Facebook/Google/etc.), Legacy Shibboleth (work is underway to consolidate this with third_party_auth), or the edX API (used by the mobile app)
• Can use the new combined login/registration ("logistration") page (à la edx.org) or the old/deprecated login/registration pages (à la Edge). Each of which uses different APIs to do the login/registration.
• The login/register page[s] will sometimes be passed a "course_id" (and other) parameters from the edX marketing site. If present, the code must try to enroll the user in the specified course once the user has logged in or created an account. If the course is paid, the user should be redirected to the checkout flow.

The code has two significant issues that are addressed by this PR:

• Third party authentication is not handled correctly in many cases
  • As you can see in the flow chart, there are several ways to login/register (vertical columns) where the third_party_auth ("TPA") pipeline does not get completed. This goes against the design principles of the pipeline and means that user's social accounts do not get linked to their edX account during the login or registration process.
• There are three redundant implementations of auto-enrollment (see scribbled red circles on the flow chart):
  • third_party_auth.pipeline.change_enrollment sometimes handles auto-enrollment if third party auth is being used for login. It updates the CourseEnrollment table directly.
  • student.views.[create_account/login_user] handle auto-enrollment for the old login/register screens (using try_change_enrollment and change_enrollment in that same file)
  • the new login form has client-side code (AccessView.js) containing the auto-enrollment logic, and it uses the commerce API which in turn uses the enrollment API

Changes in this PR:

• python-social-auth version is bumped to 0.2.7 which gives us more fine-grained control over the pipeline when unverified users are involved. This change is confined to its own commit.
• The three different flows for auto-enrollment and all the redundant logic is combined into a new page. The login views and the third party auth views no longer need to know anything about auto-enrollment; they simply know to send the user to the "auto-enrollment" page following login/registration. The new page has the same auto-enrollment logic that was previously used by the combined logistration form. I just moved it from AccessView.js to FinishAuthView.js.
  • See New Design flow chart
• Adds a new "Dummy" third party auth provider that can be used for tests and development.

Benefits of this PR:

• Social auth accounts now get linked correctly during login/registration. There is no need for users to go to their "Account Settings" page and manually link their accounts.
• The social auth pipeline now always gets completed, as required by its design assumptions.
• No API changes were necessary
• Redundant auto-enrollment logic has been reduced substantially
• The auto-enrollment process now displays status updates to the user, which is nicer than the login form simply taking an unusually long time to submit.
• If desired, the marketing site could be changed to simply enroll users in a course by sending them to /account/finish_auth?course_id=x&enrollment_action=enroll regardless of whether they are logged in or not. Currently the marketing site tracks the user's logged in status and uses different APIs and different logic depending on whether or not the user is logged in. If it did this, there is even more code that could be removed from edX platform, since the login/register pages would no longer need to know about any query string arguments other than next.

Sandbox

• http://sandbox4.opencraft.com/login - updated June 18

Upgrade Issue
When merged, this PR does cause a problem for existing installations: python-social-auth at one point converted from syncdb to south/django migrations, and bumping the version of python-social-auth that we use will result in this error next time paver update_db is run:

django.db.utils.DatabaseError: table "social_auth_usersocialauth" already exists

To fix this (on devstack), the user must manually run this command:

./manage.py lms migrate default 0001_initial --fake --settings=devstack

Unfortunately, I'm not aware of any way to avoid this issue other than this manual fix. New installs are not affected.

Screenshots

• New auto-enrollment page. (Only ever seen if auto-enrollment is happening, and is not really meant to be seen at all. If the server's responding quickly, the user will barely see it and will be immediately redirected.)

Testing Instructions

1. Checkout this branch, get it running, and configure ~/lms.auth.json so that a social auth provider (e.g. "Google" or "Dummy") is enabled. Dummy requires no configuration and is the easiest, but it always returns the same user information/id.
2. Note the setting of ENABLE_COMBINED_LOGIN_REGISTRATION in the FEATURES of ~/lms.env.json (this change should work whether it's true or false, but each uses different code paths)
3. Open an incognito window for the remainder of this test.
4. On your LMS, go /login . Click on a third party auth provider.
5. Once back at the login page ("You authenticated but this account is not associated with an edX account"), click "Register Now" and finish creating the account.
6. Go to Account Settings and verify that the third party account is linked. (This indicates the fix worked).
7. In your devstack console output, copy the link from the activation email, and paste it into the browser in order to activate this account. (Or run these commands in the django shell: from django.contrib.auth.models import User; u=User.objects.get(username="Galactica1"); u.is_active=True; u.save())
8. From Account Settings, unlink the account and also create a password for the user.
9. Go back to /login and use the third party auth to login again. Because we unlinked, the user won't be recognized ("You authenticated but this account is not associated with an edX account"). Enter the password from step 8 and login.
10. Go to Account Settings and verify that the third party account is linked again because of this new login. (This indicates the fix worked).
11. Paste the URL /account/finish_auth?course_id=course-v1%3Aorg%2Bcourse%2Brun&enrollment_action=enroll&email_opt_in=false but replace org course and run with the values for an actual course on this instance.
12. Verify that the user gets auto-enrolled in that course.
13. Close and re-open the incognito window.
14. Paste the URL /login?course_id=course-v1%3Aorg%2Bcourse%2Brun&enrollment_action=enroll&email_opt_in=false but replace org course and run with the values for a different course on this instance.
15. Login using third party auth, and verify that the user gets auto-enrolled in that new course.
16. (In a normal browser window - not incognito:) As an admin-capable user, go to /admin/auth/user/ and delete the user you have been testing with. It's important that you delete it from that page by checking the checkbox next to that user then selecting "Delete" from the dropdown menu. If you are not on a debug machine, this may not work - in that case, use the shell to delete the UserPreference entries for the user, then delete the user using any means.
17. (Back in incognito:) Go to /login?course_id=course-v1%3Aorg%2Bcourse%2Brun&enrollment_action=enroll&email_opt_in=false but replace org course and run with the values for a course on this instance.
18. Click "Register". The URL may change, but that's ok.
19. Click on a third party provider, finish creating the account, then make sure that the user has been enrolled in the course and that the third party account has been linked.
20. Repeat step 16
21. Change the setting of ENABLE_COMBINED_LOGIN_REGISTRATION, restart the server, and repeat the process to test the other views.

TODO:

• A JIRA task should be created to remove/simplify student.views.change_enrollment in the future (the LMS should use the enrollment API instead for consistency with the login process)

Reviewers:
Code: Destination team
Product: Griff, Beth
DevOps: (TBD)

[openedx-webhooks]

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?number=8262&repo=edx%2Fedx-platform

[smarnach]

@bradenmacdonald I had a short look at the code, and did a bit of testing. I'll do a full pass through the code tomorrow.

Unfortunately, the fix doesn't seem to work for me. I went to the sandbox, clicked on "Sign in", then on "Dummy", then on "Create an account", entered some data and clicked "Create your account", went to the account settings and scrolled to the bottom. The "Connected Accounts" list shows Dummy with the link "Link" behind it, indicating that the account is not linked yet. Finishing the registration by clicking the activation link in the email doesn't seem to change anything.

I also got a few 500s when trying to remove the accounts I added again, but they are likely caused by me creating the superuser the wrong way (using ./manage.py lms createsuperuser --settings=aws), so they don't seem to be related to this PR.

[bradenmacdonald]

@smarnach Hmm, I'll look into it. Regarding the 500 errors when deleting users: The behaviour on our sandbox seems different than on devstack. It turns out that you have to delete all the associated UserPreference objects, and then you can delete the user without issue.

[bradenmacdonald]

@smarnach Ah, turns out a last-minute change I'd made had broken this if you started with the login page rather than the register page I believe I have fixed it now by rewriting the ensure_user_information pipeline step.

[smarnach]

@bradenmacdonald Thanks, the linking works fine now. After the steps described in my previous comment, "Dummy" is listed with an "Unlink" link after it in the user settings, indicating that linking has worked. However, if I then immediately log out, click "Sign in" and "Dummy" again, nothing happens (well, the page gets reloaded, but that's hardly noticable). After account activation, logging in with the Dummy button works fine.

[smarnach]

With ENABLE_COMBINED_LOGIN_REGISTRATION set to False, I get a 500 on loading the login page. I commented on the offending line.

[smarnach]

I updated the code on the sandbox and tested the latest fix – works fine. The issue that logging in with Dummy again before activating the account does nothing without giving an error message to the user still remains (but might be considered a different bug outside the scope of this fix).

Let me know when this is ready for a final round of testing.

[bradenmacdonald]

@smarnach This is ready for you to review again. All the tests should be passing now.

Regarding the lack of a visible error message for inactive accounts: I have a fix for that on my other PR, #8155 (3c62a6b). Tomorrow I can try cherry-picking that into this PR.

[bradenmacdonald]

@smarnach Ok, error messages should now be working too. You can test them locally if you disable django debug toolbar.

[smarnach]

@bradenmacdonald I tested it again with various settings and login flows, and couldn't find any issues with the latest version. 👍

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald! I've created OSPR-631 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.

[rlucioni]

@sarina we still need an accessibility review and agreement on an upgrade strategy from DevOps.

[feanil]

@rlucioni I believe we have already talked about doing the fake migration when it comes time to deploy this code to edx production environments. Is there something more, that I'm not aware of?

[rlucioni]

Great, thanks @feanil. I was making sure we were all set on that front.

@bradenmacdonald and @sarina, all we need before merging this is an accessibility review from @cptvitamin. I'll do my best to help him walk through the changes being made today.

[cptvitamin]

@bradenmacdonald, @rlucioni just walked me through this process and it appears as though the only event that would have an accessibility impact would be the interstitial page that asks a user to "Please wait" Do you agree with that? If so, I think this should be fine. The page's <title> element, the first thing read to a screen reader user, indicates as much, and if the page was onscreen long enough for the screen reader user to explore it, they first Heading says the same thing. I do not think anything like a role=alert is necessary here and would just complicate things. That is my long winded way of saying 👍

Unless there is some other UI you think I should take a look at.

[rlucioni]

@bradenmacdonald I'm ready to merge this once you respond to @cptvitamin's comment.

[bradenmacdonald]

@cptvitamin Yep, I agree that that's the only accessibility impact. Thanks!

@rlucioni Please merge away :) Thanks a lot for all your help on this. Do you want [me] to squash this first though?

[sarina]

@rlucioni I confirmed with Nimisha that Mobile is good with this PR. We've got UX and a11y sign off as well. Can you respond to Braden above, and merge when it's looking good to you? Thanks!

[rlucioni]

Thanks for the follow-up @sarina. @bradenmacdonald yes, please squash your commits. As I said earlier, since your changes require an upgraded version of python-social-auth (which may not be backward-compatible with the old code), I think the version change should be bundled together with your other work.

[bradenmacdonald]

@rlucioni I fixed the JS issues: The fixes I made are in cbdb8526161b6cf5edcdca5a0403409b3014308d but I subsequently squashed them into the main commit. Please take a quick look at that commit, and then this should be ready to merge.

[rlucioni]

@bradenmacdonald why is this being removed? It makes the browser's back button compatible with the logistration page.

[bradenmacdonald]

As discussed via chat: JSHint was complaining about this code and it appears to not be doing anything. Let me know if you find out otherwise, and I can revert this since it's mostly an incidental change.

[rlucioni]

I did a little more digging into the event handler, and I agree that it's not necessary. getState() doesn't appear to have any side effects.

[sarina]

Thanks Renzo!!!

Braden - could you share with me any SSO project plan? (fine to do in
email). I want to understand where the SSO project is at and what other PRs
may be coming in for it.

On Fri, Jun 19, 2015 at 9:12 AM, Renzo Lucioni notifications@github.com
wrote:

Merged #8262 https://github.com/edx/edx-platform/pull/8262.

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/8262#event-335222760.

[rlucioni]

@bradenmacdonald could you explain why this was removed? FYI @chrisndodge.

[rlucioni]

@bradenmacdonald I see that the functionality moved to the auth completion view, which makes sense given your refactor. The question now is why the shoppingcart addition isn't working for @chrisndodge, even though the new code is tested.

[rlucioni]

@bradenmacdonald I'm not familiar with the reproduction steps for this issue, but @chrisndodge should have more details.

[bradenmacdonald]

@rlucioni EDIT: What I said before didn't really make sense. But yeah, the functionality is available in the auth completion view instead. It says right in the comment that "this elif clause is only used in the "auto-add after user reg/login" case, i.e. it's always wrapped in try_change_enrollment.", and I removed try_change_enrollment.

Can you give me a link to an example on staging or instructions for reproducing the issue?

[chrisndodge]

Just closing the loop on the added discussion on this PR, post merge. The issue is that there was a change to the JavaScript on the registration page. Since the JavaScript is in the .html template file (rather than a separate .js file), the JS changes have to be also applied to our customers customized registration (it is customized since they have additional signup fields to capture). This has been applied and verified on staging.

As follow-up work, it'd be nice to get the JavaScript out of the the registration.html template and into it's own file at some point.