Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file.
25 changes: 25 additions & 0 deletions common/djangoapps/third_party_auth/config/waffle.py
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.
Comment thread
zamanafzal marked this conversation as resolved.
# .. 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__
)
94 changes: 93 additions & 1 deletion common/djangoapps/third_party_auth/pipeline.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,15 @@ def B(*args, **kwargs):
from lms.djangoapps.verify_student.utils import earliest_allowed_verification_date
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
from openedx.core.djangoapps.user_api import accounts
from openedx.core.djangoapps.user_api.accounts.utils import is_multiple_sso_accounts_association_to_saml_user_enabled
from openedx.core.djangoapps.user_authn import cookies as user_authn_cookies
from openedx.core.djangoapps.user_authn.utils import should_redirect_to_authn_microfrontend
from common.djangoapps.third_party_auth.utils import user_exists
from common.djangoapps.third_party_auth.utils import (
get_user_from_email,
is_enterprise_customer_user,
is_saml_provider,
user_exists,
)
from common.djangoapps.track import segment
from common.djangoapps.util.json_request import JsonResponse

Expand Down Expand Up @@ -740,6 +746,92 @@ def associate_by_email_if_login_api(auth_entry, backend, details, user, current_
return association_response


@partial.partial
def associate_by_email_if_saml(auth_entry, backend, details, user, strategy, *args, **kwargs):
"""
This pipeline step associates the current social auth with the user with the
same email address in the database. It defers to the social library's associate_by_email
implementation, which verifies that only a single database user is associated with the email.

This association is done ONLY if the user entered the pipeline belongs to SAML provider.
"""

def get_user():
"""
This is the helper method to get the user from system by matching email.
"""
user_details = {'email': details.get('email')} if details else None
return get_user_from_email(user_details or {})

def associate_by_email_if_enterprise_user():
"""
If the learner arriving via SAML is already linked to the enterprise customer linked to the same IdP,
they should not be prompted for their edX password.
"""
try:
enterprise_customer_user = is_enterprise_customer_user(current_provider.provider_id, current_user)
logger.info(
u'[Multiple_SSO_SAML_Accounts_Association_to_User] Enterprise user verification:'
u'Email: {email}, user_id: {user_id}, Provider: {provider},'
u' enterprise_customer_user: {enterprise_customer_user}'.format(
email=current_user.email,
user_id=current_user.id,
provider=current_provider.provider_id,
enterprise_customer_user=enterprise_customer_user,
)
)

if enterprise_customer_user:
# this is python social auth pipeline default method to automatically associate social accounts
# if the email already matches a user account.
association_response = associate_by_email(backend, details, user, *args, **kwargs)

if (
association_response and
association_response.get('user') and
association_response['user'].is_active
):
# Only return the user matched by email if their email has been activated.
# Otherwise, an illegitimate user can create an account with another user's
# email address and the legitimate user would now login to the illegitimate
# account.
return association_response
elif (
association_response and
association_response.get('user') and
not association_response['user'].is_active
):
logger.info(
u'[Multiple_SSO_SAML_Accounts_Association_to_User] User association account is not'
u' active: Email: {email}, user_id: {user_id}, Provider: {provider},'
u' enterprise_customer_user: {enterprise_customer_user}'.format(
email=current_user.email,
user_id=current_user.id,
provider=current_provider.provider_id,
enterprise_customer_user=enterprise_customer_user
)
)
return None

except Exception as ex: # pylint: disable=broad-except
logger.exception('[Multiple_SSO_SAML_Accounts_Association_to_User] Error in'
' saml multiple accounts association: %s:, %s:', current_user.id, ex)

# this is waffle switch to enable and disable this functionality from admin panel.
if is_multiple_sso_accounts_association_to_saml_user_enabled():

saml_provider, current_provider = is_saml_provider(strategy.request.backend.name, kwargs)

if saml_provider:
# get the user by matching email if the pipeline user is not available.
current_user = user if user else get_user()

# Verify that the user linked to enterprise customer of current identity provider and an active user
associate_response = associate_by_email_if_enterprise_user() if current_user else None
if associate_response:
return associate_response


def user_details_force_sync(auth_entry, strategy, details, user=None, *args, **kwargs):
"""
Update normally protected user details using data from provider.
Expand Down
1 change: 1 addition & 0 deletions common/djangoapps/third_party_auth/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ def apply_settings(django_settings):
'social_core.pipeline.social_auth.auth_allowed',
'social_core.pipeline.social_auth.social_user',
'common.djangoapps.third_party_auth.pipeline.associate_by_email_if_login_api',
'common.djangoapps.third_party_auth.pipeline.associate_by_email_if_saml',
'common.djangoapps.third_party_auth.pipeline.get_username',
'common.djangoapps.third_party_auth.pipeline.set_pipeline_timeout',
'common.djangoapps.third_party_auth.pipeline.ensure_user_information',
Expand Down
48 changes: 47 additions & 1 deletion common/djangoapps/third_party_auth/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,16 @@

from common.djangoapps.student.tests.factories import UserFactory
from common.djangoapps.third_party_auth.tests.testutil import TestCase
from common.djangoapps.third_party_auth.utils import user_exists, convert_saml_slug_provider_id
from common.djangoapps.third_party_auth.utils import (
get_user_from_email,
is_enterprise_customer_user,
user_exists,
convert_saml_slug_provider_id,
)
from openedx.features.enterprise_support.tests.factories import (
EnterpriseCustomerIdentityProviderFactory,
EnterpriseCustomerUserFactory,
)


@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
Expand Down Expand Up @@ -53,3 +62,40 @@ def test_convert_saml_slug_provider_id(self):
self.assertEqual(
convert_saml_slug_provider_id(provider_names[provider_id]), provider_id
)

def test_get_user(self):
"""
Match the email and return user if exists.
"""
# Create users from factory
UserFactory(username='test_user', email='test_user@example.com')
self.assertTrue(
get_user_from_email({'email': 'test_user@example.com'}),
)
self.assertFalse(
get_user_from_email({'email': 'invalid@example.com'}),
)

def test_is_enterprise_customer_user(self):
"""
Verify that if user is an enterprise learner.
"""
# Create users from factory

user = UserFactory(username='test_user', email='test_user@example.com')
other_user = UserFactory(username='other_user', email='other_user@example.com')
customer_idp = EnterpriseCustomerIdentityProviderFactory.create(
provider_id='the-provider',
)
customer = customer_idp.enterprise_customer
EnterpriseCustomerUserFactory.create(
enterprise_customer=customer,
user_id=user.id,
)

self.assertTrue(
is_enterprise_customer_user('the-provider', user),
)
self.assertFalse(
is_enterprise_customer_user('the-provider', other_user),
)
35 changes: 35 additions & 0 deletions common/djangoapps/third_party_auth/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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-")
Expand Down Expand Up @@ -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,

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.

What if no enterprise_idp is found?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.
https://github.com/edx/edx-platform/blob/aed3d8cbfcd81ab4320224743eeff0115f953d24/openedx/core/djangoapps/user_authn/views/login_form.py#L170

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 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 EnterpriseCustomerIdentityProvider matching query does not exist.: with all the other details like use email, provider id, and user id.

user_id=user.id).exists()
11 changes: 11 additions & 0 deletions openedx/core/djangoapps/user_api/accounts/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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

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.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

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.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

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.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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.

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.

@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()