feat: associates user by email for oauth when tpa is required

[nizarmah]

Description

Currently, in the edX platform, if you try to login with an SSO account using the login form, a username is generated for you based on the email that is used with the SSO provider.

The username is generated using the pipeline third_party_auth.pipeline.get_username.

If the username already exists in the platform, you run into a small conflict which sends you back to the login form and requests the following:

It basically requests that you link your edX account and your SSO account.

However, sometimes we would want the edX account and SSO account to be associated by email. There's already a special pipeline available in the platform for that, third_party_auth.pipeline.associate_by_email_if_login_api. Sadly, however, it is only available for a certain entry method of authentication, called login_api.

So in order to start associating the edX account and SSO account by email, this pull request adds a waffle switch.

Update: After a lot of reading through the code and investigation, it seems like the login_api auth entry is no longer used. Accordingly, I took the liberty of removing said pipeline and replacing it with something that seemed useful. Now the pipeline runs whenever third party authentication is required.

Update: After @waheedahmed's findings, which is that login_api is in fact still used, and @zamanafzal's request, which is to make the functionality specific to Oauth providers, a new pipeline was added to associate the user by email only when the following conditions are met:

• The social auth provider is an Oauth2 provider
• The ENABLE_REQUIRE_THIRD_PARTY_AUTH is enabled

Supporting information

• JIRA tickets: SE-3816, OSPR-5312
• Sandbox URL:
  • Default Behavior:
    • LMS: https://pr25935-default.sandbox.opencraft.hosting/
    • Studio: https://studio.pr25935-default.sandbox.opencraft.hosting/
  • New Behavior:
    • LMS: https://pr25935.sandbox.opencraft.hosting/
    • Studio: https://studio.pr25935.sandbox.opencraft.hosting/

Testing instructions

Preparation

Please send an email to nizar at opencraft.com, requesting to be added as a test user to the Google SSO used for this sandbox.

You need a Google Account, because you'll be using the same email address when registering through the form and through the SSO.

Reproducing the problem

Logging in with the SSO

Login with the Google SSO over at the sandbox with default behavior

Removing Social Auth Record

Sign in as staff:edx into the django admin and delete the user social auth for your user.

Logging in with the SSO, again...

Login, again, with the Google SSO over at the sandbox with default behavior. After doing that, you should receive a similar screenshot

Testing the fix

Logging in with the SSO

Login with the Google SSO over at the sandbox with default behavior

Removing Social Auth Record

Sign in as staff:edx into the django admin and delete the user social auth for your user.

Logging in with the SSO, again...

Login, again, with the Google SSO over at the sandbox with default behavior. After that, you should be able to directly login 😃

Settings

EDXAPP_LMS_ENV_EXTRA:
  ENABLE_REQUIRE_THIRD_PARTY_AUTH: true

[openedx-webhooks]

Thanks for the pull request, @nizarmah! I've created OSPR-5312 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

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

• supporting documentation
• Open edX discussion forum 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 be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[nizarmah]

Sorry for all the messy force pushes! I just wanted to clean the history before any reviewer took a look. I have been testing on the sandbox, which made pushing to the branch just more convenient.

[gabor-boros]

👍 🎉

• I tested what's in the test instructions
• I read through the code
• NA I checked for accessibility issues
• NA Includes documentation
• NA I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

[nizarmah]

@natabene, I hope you and the members on the edX team are having/have had a nice holiday!
This is ready for edX's review 👍🏼

[natabene]

@nizarmah Thanks, lining this up for our review.

[bradenmacdonald]

This will likely need product review but I would be inclined to suggest making this the default behavior and deprecating the previous behavior with the yellow message ("You have successfully signed in but there's no linked account"). Everyone I've ever talked to about the current flow has said it's confusing.

The most common case is that the user's SSO email matches their existing account's email, and in that case it should automatically link (if we trust the SSO provider), which I believe is what's happening here.

The "edge case" is that the user's SSO email is different from their existing account's email; in that case if they want to "link" instead of creating a new account, they can always sign in to their existing account, go to the account settings page, and link the accounts from there. That's how most other websites work anyways.

It may also be necessary to enable this per provider, because we don't necessarily trust every provider's "email" field (e.g. user logs in via their company provider and company provider was hacked to say that they own the email of an edX employee, we don't want to log them in to the edX employee's account).

[natabene]

@nasthagiri Are you ok with this going to core committers?

[nasthagiri]

@natabene Yes, I am.

@bradenmacdonald +1 to your suggestion of changing the default. @wajeeha-khalid can provide Product input on this.

[waheedahmed]

Just one concern otherwise looks good now.

[waheedahmed]

It will enable it by default for each auth entry in the oauth2 provider's case and I believe the purpose of associate_by_email_if_login_api is failed here. Don’t you think we need to put it under the feature flag ENABLE_REQUIRE_THIRD_PARTY_AUTH?

[waheedahmed]

And if we are making it the default behavior in the oauth2 providers case, then I think we can change this check if auth_entry == AUTH_ENTRY_LOGIN_API: to something if is_oauth_provider(backend.name, **kwargs) and auth_entry in ALLOWED_AUTH_ENTRIES: instead of this new pipeline function.

ALLOWED_AUTH_ENTRIES will be a list of auth entries you want to allow here. e.g. login_api, login, register.

[nizarmah]

@waheedahmed Yes, I do think we need to put it under the feature flag ENABLE_REQUIRE_THIRD_PARTY_AUTH, that's a great point.

Your recommendation about the ALLOWED_AUTH_ENTRIES is great, but I'm hesitant to make such a change because it can be considered "breaking".

[nizarmah]

@waheedahmed may you please check the latest changes? I added the setting check. I also added a test to verify that it is working 😃

I thought of adding another test, when the flag is disabled, but I believe test_new_account_registration_assigns_distinct_username_on_collision already covers the default case. Let me know though, please, if you don't consider that to be enough 👍🏼

[waheedahmed]

This test is passing even with ENABLE_REQUIRE_THIRD_PARTY_AUTH=False, shouldn’t it fail without this setting enabled?

[nizarmah]

😯 it should indeed fail without this setting enabled... I'll look into it. Sorry about that!

[nizarmah]

@waheedahmed sorry for this taking so much longer than it should 😞

Anyway, I updated the tests, and I also added some additional tests for the utils I added 👍🏼
I made sure to test everything extensively this time, instead of rushing it.

I went also with a different approach for the test, which is just the opposite of test_signin_fails_if_no_account_associated.
I just had to mock certain stuff because the pipeline made things pretty difficult.

This should be ready for your review again 😁

cc @natabene

[robrap]

For historical purposes, would you mind updating the PR description? Thank you.

[nizarmah]

@robrap thanks for the reminder 👍🏼 The description has been updated, and I made sure to link to two important messages in the pull request. 😄

[waheedahmed]

A small nit otherwise looks good now 👍, please squash your commits and let me know when it's ready to merge.

Thanks!

[waheedahmed]

Since you have already defined the return value within the mock call, I guess you can remove this unnecessary as statement _mock_get_associated_user_by_email_response.

[nizarmah]

Sure! 👍🏼

I also removed _mock_associate_by_email since it was also not used 😃

This should be addressed by now. I also squashed by the way.

[nizarmah]

@waheedahmed 🥳 awesome!

Thank you a lot for the review and for being patient with me through this change 😅

I addressed your nit and squashed. This is ready to be merged 👍🏼 (once the builds pass)

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[openedx-webhooks]

@nizarmah 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.