From a5adae888893782e03e934f0362803d04e6eb481 Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Thu, 14 Apr 2022 12:28:53 -0400 Subject: [PATCH] fix: Revert "Revert "fix: override get_assignments() so that active enterprise uuids come first."" Ensure that EnterpriseSystemWideUserRoleAssignment.get_assignments() can handle null values that might be returned by get_context(). This reverts commit 6927277aad53f2497a336730e8af52372ee316cf. ENT-5700 --- CHANGELOG.rst | 14 ++++++ enterprise/models.py | 111 +++++++++++++++++++++++++++++++++++++++++-- tests/test_models.py | 88 ++++++++++++++++++++++++++++++++-- 3 files changed, 206 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 31dc97271f..298d955ced 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -22,11 +22,25 @@ None - feat: configure django admin for degreed2 audit records - fix: Moodle client should accept treat duplicate course id on create as a success +[3.44.2] +-------- +fix: Undoes revert of 3.44.0, while also ensuring that +``SystemWideEnterpriseUserRoleAssignment.get_assignments()`` can handle and respect any null values returned +from ``get_context()``. + [3.44.1] -------- fix: no-op version bump (skipping 3.44.0) to account for a revert: https://github.com/openedx/edx-enterprise/pull/1534 +[3.44.0] +-------- +fix: [REVERTED] override get_assignments() so that active enterprise uuids come first. + +Overrides the SystemWideEnterpriseUserRoleAssignment.get_assignments() method to return +a list of (role, context) assignments, where the first item in the list corresponds +to the currently active enterprise for the user. + [3.43.1] --------- chore: replace enterprise customer drop-downs in django admin diff --git a/enterprise/models.py b/enterprise/models.py index f1db240000..2b457fa183 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -3,6 +3,7 @@ """ import collections +import itertools import json import os from decimal import Decimal @@ -1268,12 +1269,28 @@ def create_order_for_enrollment(self, course_run_id, discount_percentage, mode, @classmethod def inactivate_other_customers(cls, user_id, enterprise_customer): """ - Inactive all the enterprise customers of given user except the given enterprise_customer. + Mark as inactive all the enterprise customers of given user except the given enterprise_customer. """ EnterpriseCustomerUser.objects.filter( user_id=user_id ).exclude(enterprise_customer=enterprise_customer).update(active=False) + @classmethod + def get_active_enterprise_users(cls, user_id, enterprise_customer_uuids=None): + """ + Return a queryset of all active enterprise users to which the given user is related. + Or, if ``enterprise_customer_uuids`` is non-null, only the enterprise users + related to the list of given ``enterprise_customer_uuids``. + """ + kwargs = { + 'user_id': user_id, + 'active': True, + } + if enterprise_customer_uuids: + kwargs['enterprise_customer__in'] = enterprise_customer_uuids + + return EnterpriseCustomerUser.objects.filter(**kwargs) + class PendingEnterpriseCustomerUser(TimeStampedModel): """ @@ -2795,7 +2812,9 @@ def enterprise_customer_uuids(self): def get_context(self): """ - Return the context for this role assignment class. A list is returned in case of user with multiple contexts. + Returns a non-empty list of enterprise customer uuid strings to which + ``self.user`` is linked, or ``None`` if the user is not linked + to any EnterpriseCustomer. """ return self.enterprise_customer_uuids @@ -2860,7 +2879,8 @@ def has_access_to_all_contexts(self): def get_context(self): """ - Return the context for this role assignment class. + Return a non-empty list of contexts for which ``self.user`` is assigned ``self.role``, + or ``None`` if the user is assigned a role with no corresponding context. """ if self.has_access_to_all_contexts(): return [ALL_ACCESS_CONTEXT] @@ -2870,6 +2890,91 @@ def get_context(self): return super().get_context() + @classmethod + def get_distinct_assignments_by_role_name(cls, user, role_names=None): + """ + Returns a mapping of role names to sets of enterprise customer uuids + for which the user is assigned that role. + """ + # super().get_assignments() returns pairs of (role name, contexts), where + # contexts is a list of 0 or more enterprise uuids (or the ALL_ACCESS_CONTEXT token) + # as returned from super().get_context(). + # To make matters worse, get_context() could return null, meaning the role + # applies to any context. So we should still include it in the list of "customers" + # for a given role. + # See https://openedx.atlassian.net/browse/ENT-4346 for outstanding technical debt + # related to this issue. + assigned_customers_by_role = collections.defaultdict(set) + for role_name, customer_uuids in super().get_assignments(user, role_names): + if customer_uuids is not None: + assigned_customers_by_role[role_name].update(customer_uuids) + else: + assigned_customers_by_role[role_name].add(None) + return assigned_customers_by_role + + @classmethod + def get_assignments(cls, user, role_names=None): + """ + Return an iterator of (rolename, [enterprise customer uuids]) for the given + user (and maybe role_names). + + Differs from super().get_assignments(...) in that it yields (role name, customer uuid list) pairs + such that the first item in the customer uuid list for each role + corresponds to the currently *active* EnterpriseCustomerUser for the user. + + The resulting generated pairs are sorted by role name, and within role_name, by (active, customer uuid). + For example: + + ('enterprise_admin', ['active-enterprise-uuid', 'inactive-enterprise-uuid', 'other-inactive-enterprise-uuid']) + ('enterprise_learner', ['active-enterprise-uuid', 'inactive-enterprise-uuid']), + ('enterprise_openedx_operator', ['*']) + """ + customers_by_role = cls.get_distinct_assignments_by_role_name(user, role_names) + if not customers_by_role: + return + + # Filter for a set of only the *active* enterprise uuids for which the user is assigned a role. + # A user should typically only have one active enterprise user at a time, but we'll + # use sets to cover edge cases. + all_customer_uuids_for_user = set(itertools.chain(*customers_by_role.values())) + + # ALL_ACCESS_CONTEXT is not a value UUID on which to filter enterprise customer uuids. + all_customer_uuids_for_user.discard(ALL_ACCESS_CONTEXT) + + active_enterprise_uuids_for_user = set( + str(customer_uuid) for customer_uuid in + EnterpriseCustomerUser.get_active_enterprise_users( + user.id, + enterprise_customer_uuids=all_customer_uuids_for_user, + ).values_list('enterprise_customer', flat=True) + ) + + for role_name in sorted(customers_by_role): + customer_uuids_for_role = customers_by_role[role_name] + + # Determine the *active* enterprise uuids assigned for this role. + active_enterprises_for_role = sorted( + customer_uuids_for_role.intersection(active_enterprise_uuids_for_user) + ) + # Determine the *inactive* enterprise uuids assigned for this role, + # could include the ALL_ACCESS_CONTEXT token. + inactive_enterprises_for_role = sorted( + customer_uuids_for_role.difference(active_enterprise_uuids_for_user) + ) + ordered_enterprises = active_enterprises_for_role + inactive_enterprises_for_role + + # Sometimes get_context() returns ``None``, and ``None`` is a meaningful downstream value + # to the consumers of get_assignments(), either + # when constructing JWT roles or when checking for explicit or implicit access to some context. + # So if the only unique thing returned by get_context() for this role was ``None``, + # we should unpack it from the list before yielding. + # See https://openedx.atlassian.net/browse/ENT-4346 for outstanding technical debt + # related to this issue. + if ordered_enterprises == [None]: + yield (role_name, None) + else: + yield (role_name, ordered_enterprises) + def __str__(self): """ Return human-readable string representation. diff --git a/tests/test_models.py b/tests/test_models.py index 9cae00d771..c0f39b5221 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -9,6 +9,7 @@ import unittest from datetime import timedelta from unittest import mock +from uuid import UUID import ddt from edx_rest_api_client.exceptions import HttpClientError @@ -31,7 +32,12 @@ from consent.helpers import get_data_sharing_consent from consent.models import DataSharingConsent, ProxyDataSharingConsent from enterprise import roles_api -from enterprise.constants import ENTERPRISE_ADMIN_ROLE, ENTERPRISE_LEARNER_ROLE, ENTERPRISE_OPERATOR_ROLE +from enterprise.constants import ( + ALL_ACCESS_CONTEXT, + ENTERPRISE_ADMIN_ROLE, + ENTERPRISE_LEARNER_ROLE, + ENTERPRISE_OPERATOR_ROLE, +) from enterprise.errors import LinkUserToEnterpriseError from enterprise.models import ( EnrollmentNotificationEmailTemplate, @@ -2184,7 +2190,7 @@ class TestSystemWideEnterpriseUserRoleAssignment(unittest.TestCase): Tests SystemWideEnterpriseUserRoleAssignment. """ - def _create_and_link_user(self, user_email, *enterprise_customers): + def _create_and_link_user(self, user_email, *enterprise_customers, **kwargs): """ Helper that creates a User with the given email, then links that user to each of the given EnterpriseCustomers. @@ -2192,10 +2198,16 @@ def _create_and_link_user(self, user_email, *enterprise_customers): should also create some role assignments. """ user = factories.UserFactory(email=user_email) - for enterprise_customer in enterprise_customers: - EnterpriseCustomerUser.objects.link_user(enterprise_customer, user_email) + self._link_user(user_email, *enterprise_customers, **kwargs) return user + def _link_user(self, user_email, *enterprise_customers, **kwargs): + """ + Helper to link a given test user to 1 or more enterprises. + """ + for enterprise_customer in enterprise_customers: + EnterpriseCustomerUser.objects.link_user(enterprise_customer, user_email, **kwargs) + @ddt.data( { 'role_name': ENTERPRISE_LEARNER_ROLE, @@ -2287,6 +2299,74 @@ def test_get_context_explicit_enterprise_customer(self, role_name, expected_cont assert expected_context == enterprise_role_assignment.get_context() + def test_get_assignments_many_assignments(self): + """ + Tests that get_assignments orders context such that active enterprise uuids + are listed first for a given role. + """ + alpha_customer = factories.EnterpriseCustomerFactory(uuid=UUID('aaaaaaaa-0000-0000-0000-000000000000')) + beta_customer = factories.EnterpriseCustomerFactory(uuid=UUID('bbbbbbbb-0000-0000-0000-000000000000')) + delta_customer = factories.EnterpriseCustomerFactory(uuid=UUID('dddddddd-0000-0000-0000-000000000000')) + + test_user = factories.UserFactory(email='test@example.com') + + # The order matters: because delta is most recently linked (and active), all other linked + # enterprise users will get active=False, leaving only delta as the active, linked enterprise. + # Remember that link_user() creates or updates an ECU record, which will automatically + # assign the enterprise_learner role via a Django signal. + self._link_user(test_user.email, alpha_customer) + self._link_user(test_user.email, beta_customer) + self._link_user(test_user.email, delta_customer) + + # Create admin role assignments explicitly. + for customer in (alpha_customer, delta_customer): + SystemWideEnterpriseUserRoleAssignment.objects.get_or_create( + user=test_user, role=roles_api.admin_role(), + enterprise_customer=customer, applies_to_all_contexts=False, + ) + + SystemWideEnterpriseUserRoleAssignment.objects.get_or_create( + user=test_user, + role=roles_api.openedx_operator_role(), + applies_to_all_contexts=True, + ) + + expected_assignments = [ + ( + ENTERPRISE_ADMIN_ROLE, [ + str(delta_customer.uuid), str(alpha_customer.uuid), + ], + ), + ( + ENTERPRISE_LEARNER_ROLE, [ + str(delta_customer.uuid), str(alpha_customer.uuid), str(beta_customer.uuid) + ], + ), + (ENTERPRISE_OPERATOR_ROLE, [ALL_ACCESS_CONTEXT]), + ] + + actual_assignments = list(SystemWideEnterpriseUserRoleAssignment.get_assignments(test_user)) + self.assertEqual(expected_assignments, actual_assignments) + + def test_get_assignments_no_context_or_link(self): + """ + Test the scenario where a user has an assignment record, + but the record has no enterprise_customer_id, applies_to_all_contexts is False, + and the user is not linked to any enterprise. + """ + user_with_no_link = factories.UserFactory(email='zelda@example.com') + SystemWideEnterpriseUserRoleAssignment.objects.get_or_create( + user=user_with_no_link, + role=roles_api.learner_role(), + enterprise_customer=None, + applies_to_all_contexts=False, + ) + + self.assertEqual( + [('enterprise_learner', None)], + list(SystemWideEnterpriseUserRoleAssignment.get_assignments(user_with_no_link)), + ) + def test_unique_together_constraint(self): """ Verify that duplicate combination of records with same user, role and enterprise cannot be created and