-
Notifications
You must be signed in to change notification settings - Fork 4.3k
ENT-3798 Multiple_SSO_Accounts_Association_to_SAML_User #26170
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| """ | ||
| Waffle flags and switches for third party auth . | ||
| """ | ||
|
|
||
|
|
||
| from edx_toggles.toggles import LegacyWaffleSwitch, LegacyWaffleSwitchNamespace | ||
|
|
||
| _WAFFLE_NAMESPACE = u'third_party_auth' | ||
| _WAFFLE_SWITCH_NAMESPACE = LegacyWaffleSwitchNamespace(name=_WAFFLE_NAMESPACE, log_prefix=u'ThirdPartyAuth: ') | ||
|
|
||
| # .. toggle_name: third_party_auth.enable_multiple_sso_accounts_association_to_saml_user | ||
| # .. toggle_implementation: WaffleSwitch | ||
| # .. toggle_default: False | ||
| # .. toggle_description: If enabled than learner should not be prompted for their edX password arriving via SAML | ||
| # and already linked to the enterprise customer linked to the same IdP." | ||
| # .. toggle_use_cases: temporary | ||
| # .. toggle_creation_date: 2021-01-29 | ||
| # .. toggle_target_removal_date: 2021-04-31 | ||
| # .. toggle_warnings: None. | ||
| # .. toggle_tickets: ENT-4034 | ||
| ENABLE_MULTIPLE_SSO_ACCOUNTS_ASSOCIATION_TO_SAML_USER = LegacyWaffleSwitch( | ||
| _WAFFLE_SWITCH_NAMESPACE, | ||
| 'enable_multiple_sso_accounts_association_to_saml_user', | ||
| __name__ | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |
|
|
||
| from uuid import UUID | ||
| from django.contrib.auth.models import User | ||
| from enterprise.models import EnterpriseCustomerUser, EnterpriseCustomerIdentityProvider | ||
| from . import provider | ||
|
|
||
|
|
||
| def user_exists(details): | ||
|
|
@@ -30,6 +32,23 @@ def user_exists(details): | |
| return False | ||
|
|
||
|
|
||
| def get_user_from_email(details): | ||
| """ | ||
| Return user with given details exist in the system.∂i | ||
|
|
||
| Arguments: | ||
| details (dict): dictionary containing user email. | ||
|
|
||
| Returns: | ||
| User: if user with given details exists, None otherwise. | ||
| """ | ||
| email = details.get('email') | ||
| if email: | ||
| return User.objects.filter(email=email).first() | ||
|
|
||
| return None | ||
|
|
||
|
|
||
| def convert_saml_slug_provider_id(provider): | ||
| """ | ||
| Provider id is stored with the backend type prefixed to it (ie "saml-") | ||
|
|
@@ -57,3 +76,19 @@ def validate_uuid4_string(uuid_string): | |
| except ValueError: | ||
| return False | ||
| return True | ||
|
|
||
|
|
||
| def is_saml_provider(backend, kwargs): | ||
| """ Verify that the third party provider uses SAML """ | ||
| current_provider = provider.Registry.get_from_pipeline({'backend': backend, 'kwargs': kwargs}) | ||
| saml_providers_list = list(provider.Registry.get_enabled_by_backend_name('tpa-saml')) | ||
| return (current_provider and | ||
| current_provider.slug in [saml_provider.slug for saml_provider in saml_providers_list]), current_provider | ||
|
|
||
|
|
||
| def is_enterprise_customer_user(provider_id, user): | ||
| """ 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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if no enterprise_idp is found?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| user_id=user.id).exists() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| from six.moves import range | ||
| from six.moves.urllib.parse import urlparse # pylint: disable=import-error | ||
|
|
||
| from common.djangoapps.third_party_auth.config.waffle import ENABLE_MULTIPLE_SSO_ACCOUNTS_ASSOCIATION_TO_SAML_USER | ||
| from openedx.core.djangoapps.site_configuration.models import SiteConfiguration | ||
| from openedx.core.djangoapps.theming.helpers import get_config_value_from_site_or_settings, get_current_site | ||
| from openedx.core.djangoapps.user_api.config.waffle import ( | ||
|
|
@@ -200,3 +201,13 @@ def is_multiple_user_enterprises_feature_enabled(): | |
| Boolean value representing switch status | ||
| """ | ||
| return user_api_waffle().is_enabled(ENABLE_MULTIPLE_USER_ENTERPRISES_FEATURE) | ||
|
|
||
|
|
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤷 -- will it be removed after the change has been validated?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
|
|
||
| Returns: | ||
| Boolean value representing switch status | ||
| """ | ||
| return ENABLE_MULTIPLE_SSO_ACCOUNTS_ASSOCIATION_TO_SAML_USER.is_enabled() | ||
Uh oh!
There was an error while loading. Please reload this page.