fix: Revert "Revert "fix: override get_assignments() so that active e…

…nterprise uuids come first."" Ensure that EnterpriseSystemWideUserRoleAssignment.get_assignments() can handle null values that might be returned by get_context(). This reverts commit 6927277. ENT-5700
openedx · Apr 19, 2022 · a5adae8 · a5adae8
1 parent 260f48b
commit a5adae8
Show file tree

Hide file tree

Showing 3 changed files with 206 additions and 7 deletions.
diff --git 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
@@ -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
@@ -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,18 +2190,24 @@ 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.
         Since we're saving an `EnterpriseCustomerUser` instance(s), this
         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