ENT-3798 Multiple_SSO_Accounts_Association_to_SAML_User by zamanafzal · Pull Request #26170 · openedx/openedx-platform

zamanafzal · 2021-01-26T10:45:01Z

No description provided.

moconnell1453

Looks good, though we may benefit from log statements in the short term to help us diagnose any situations where we might end up deviating from the expected flow.

mattdrayer · 2021-01-27T15:12:46Z

+                user_details = {'email': details.get('email')} if details else None
+                current_user = get_user(user_details or {})
+
+            if current_user:


Is there a possible scenario where the if current user check returns false, and if so, how should that be handled?

Yes, that's possible in case when the user is coming for the very first time and doesn't have an account on edX. In that case, everything is working fine already.

mattdrayer · 2021-01-27T15:25:24Z

+
+def is_multiple_sso_accounts_association_to_saml_user_enabled():
+    """
+    Checks to see if the django-waffle switch for enabling the multiple sso accounts association to saml user is active


The purpose of this switch isn't really clear to me -- are we adding a control to determine if it is possible for a single user to be associated with more than one third party identity (ie, UserSocialAuth record)? This is already the case -- lots of users have more than one USA record -- are we altering the default behavior?

For the safe side, I have added this waffle switch. As, we need some time to test these changes on stage and meanwhile, changes will go on PROD. So, they have been kept behind a switch, which will be turned on once the changes have been tested on Stage.

I guess I still don't see what this has to do with multiple SSO accounts. We're simply checking to see if the learner is already linked to an enterprise customer, and if the IdP is also linked to that customer.

This is just a flag which will be controled from admin panel and will be switched on on PROD once we are done with the testing of this change on stage. It doesn't have to do anything with functionality.

🤷 -- will it be removed after the change has been validated?

Yes, this can be removed once the changes are validated. Until then we can plug in and out the code by just clicking on a flag in the admin panel which is the safest way to take these changes.

@mattdrayer Zaman has added this flag at my request, due to my concern for affecting third party auth in prod while we are testing the changes in stage. We'll remove the flag when we go to prod.

mattdrayer · 2021-01-27T15:28:39Z

+    """ Verify that the user linked to enterprise customer of current identity provider"""
+    enterprise_idp = EnterpriseCustomerIdentityProvider.objects.get(provider_id=provider_id)
+
+    return EnterpriseCustomerUser.objects.filter(enterprise_customer=enterprise_idp.enterprise_customer,


What if no enterprise_idp is found?

That is not possible as the user will be coming from an IDP and we redirect the user to IDP only if the provider id is found. Here is the reference code where we redirect the user.
https://github.com/edx/edx-platform/blob/aed3d8cbfcd81ab4320224743eeff0115f953d24/openedx/core/djangoapps/user_authn/views/login_form.py#L170

it seems to me that we're putting a lot of faith in the workflow by assuming that this code only gets executed in that context. I think it would be worthwhile to handle for a null enterprise_idp condition and include logging, accordingly.

We have also put this code in a try block and catching and logging the exception if something like this will happen. I have tested this locally and it will log the exception EnterpriseCustomerIdentityProvider matching query does not exist.: with all the other details like use email, provider id, and user id.

zamanafzal · 2021-01-28T15:36:11Z

@mattdrayer Thank you for the valuable feedback. I have incorporated it. Can you please review PR again?

nasthagiri · 2021-01-28T16:40:06Z

FYI @edx/arch-bom - as technically being technical owners of third_party_auth.

edx-pipeline-bot · 2021-02-01T07:50:25Z

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

edx-pipeline-bot · 2021-02-01T08:20:06Z

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

edx-pipeline-bot · 2021-02-01T08:34:48Z

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

edx-pipeline-bot · 2021-02-01T16:02:31Z

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

moconnell1453 approved these changes Jan 26, 2021

View reviewed changes

zamanafzal force-pushed the zafzal/ENT3798-attach-multiple-sso-saml-user-accounts-to-email branch 2 times, most recently from eaeefe7 to 20b5c75 Compare January 26, 2021 20:48

zamanafzal requested review from HammadAhmadWaqas, irfanuddinahmad, mattdrayer, muhammad-ammar and saleem-latif January 27, 2021 08:04

mattdrayer reviewed Jan 27, 2021

View reviewed changes

zamanafzal force-pushed the zafzal/ENT3798-attach-multiple-sso-saml-user-accounts-to-email branch from 20b5c75 to 64a8fb9 Compare January 28, 2021 15:25

zamanafzal requested a review from nasthagiri January 28, 2021 15:25

zamanafzal force-pushed the zafzal/ENT3798-attach-multiple-sso-saml-user-accounts-to-email branch from 64a8fb9 to 14666e3 Compare January 28, 2021 15:34

mattdrayer approved these changes Jan 28, 2021

View reviewed changes

zamanafzal force-pushed the zafzal/ENT3798-attach-multiple-sso-saml-user-accounts-to-email branch from 14666e3 to 45bcf2e Compare January 28, 2021 16:04

openedx deleted a comment from edx-status-bot Jan 28, 2021

robrap reviewed Jan 28, 2021

View reviewed changes

Comment thread openedx/core/djangoapps/user_api/config/waffle.py Outdated

moconnell1453 approved these changes Jan 28, 2021

View reviewed changes

saleem-latif approved these changes Jan 29, 2021

View reviewed changes

zamanafzal force-pushed the zafzal/ENT3798-attach-multiple-sso-saml-user-accounts-to-email branch from 45bcf2e to 7a4d7dc Compare January 29, 2021 10:28

ENT-3798 Multiple_SSO_Accounts_Association_to_SAML_User

a3f5169

zamanafzal force-pushed the zafzal/ENT3798-attach-multiple-sso-saml-user-accounts-to-email branch from 7a4d7dc to a3f5169 Compare January 29, 2021 12:00

openedx deleted a comment from edx-status-bot Jan 29, 2021

zamanafzal merged commit b99a64c into master Feb 1, 2021

zamanafzal deleted the zafzal/ENT3798-attach-multiple-sso-saml-user-accounts-to-email branch February 1, 2021 06:44

zamanafzal mentioned this pull request Feb 1, 2021

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

Merged

robrap reviewed Feb 1, 2021

View reviewed changes

Comment thread common/djangoapps/third_party_auth/config/waffle.py

nizarmah mentioned this pull request Mar 19, 2021

fix: updates get provider from pipeline wrong parameter #27079

Closed

Uh oh!

Conversation

zamanafzal commented Jan 26, 2021

Uh oh!

moconnell1453 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zamanafzal commented Jan 28, 2021

Uh oh!

nasthagiri commented Jan 28, 2021

Uh oh!

Uh oh!

edx-pipeline-bot commented Feb 1, 2021

Uh oh!

edx-pipeline-bot commented Feb 1, 2021

Uh oh!

edx-pipeline-bot commented Feb 1, 2021

Uh oh!

edx-pipeline-bot commented Feb 1, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants