refactor: group role assignments for all subjects

Author: mariajgrimaldi

docs/conf.py

@@ -47,7 +47,8 @@ def get_version(*file_paths):
 # If extensions (or modules to document with autodoc) are in another directory,
 # add these directories to sys.path here. If the directory is relative to the
 # documentation root, use os.path.abspath to make it absolute, like shown here.
-#
+
+
 # import os
 # import sys
 # sys.path.insert(0, os.path.abspath('.'))

openedx_authz/api/data.py

@@ -389,5 +389,5 @@ class RoleAssignmentData(AuthZData):
     """
 
     subject: SubjectData = None  # Needs defaults to avoid value error from attrs
-    role: RoleData = None
+    roles: list[RoleData] = list()
     scope: ScopeData = None

openedx_authz/api/roles.py

@@ -34,7 +34,7 @@
     "unassign_role_from_subject_in_scope",
     "batch_unassign_role_from_subjects_in_scope",
     "get_subject_role_assignments_in_scope",
-    "get_subjects_role_assignments_for_role_in_scope",
+    "get_subject_role_assignments_for_role_in_scope",
     "get_all_subject_role_assignments_in_scope",
     "get_subject_role_assignments",
 ]
@@ -182,7 +182,7 @@ def get_all_roles_names() -> list[str]:
 
 
 def get_all_roles_in_scope(scope: ScopeData) -> list[list[str]]:
-    """Get all the available roles names in a specific scope.
+    """Get all the available role grouping policies in a specific scope.
 
     Args:
         scope: The scope to filter roles (e.g., 'lib@*' or '*' for global).
@@ -272,7 +272,7 @@ def get_subject_role_assignments(subject: SubjectData) -> list[RoleAssignmentDat
         role_assignments.append(
             RoleAssignmentData(
                 subject=subject,
-                role=role,
+                roles=[role],
                 scope=ScopeData(namespaced_key=policy[GroupingPolicyIndex.SCOPE.value]),
             )
         )
@@ -300,17 +300,17 @@ def get_subject_role_assignments_in_scope(
         role_assignments.append(
             RoleAssignmentData(
                 subject=subject,
-                role=RoleData(
+                roles=[RoleData(
                     namespaced_key=namespaced_key,
                     permissions=get_permissions_for_single_role(role),
-                ),
+                )],
                 scope=scope,
             )
         )
     return role_assignments
 
 
-def get_subjects_role_assignments_for_role_in_scope(
+def get_subject_role_assignments_for_role_in_scope(
     role: RoleData, scope: ScopeData
 ) -> list[RoleAssignmentData]:
     """Get the subjects assigned to a specific role in a specific scope.
@@ -329,43 +329,46 @@ def get_subjects_role_assignments_for_role_in_scope(
         if subject.startswith(f"{RoleData.NAMESPACE}{RoleData.SEPARATOR}"):
             # Skip roles that are also subjects
             continue
+
         role_assignments.append(
             RoleAssignmentData(
                 subject=SubjectData(namespaced_key=subject),
-                role=RoleData(
-                    external_key=role.external_key,
+                roles=[RoleData(
+                    namespaced_key=role.namespaced_key,
                     permissions=get_permissions_for_single_role(role),
-                ),
+                )],
                 scope=scope,
             )
         )
+
     return role_assignments
 
 
-def get_all_subject_role_assignments_in_scope(
-    scope: ScopeData,
-) -> list[RoleAssignmentData]:
+def get_all_subject_role_assignments_in_scope(scope: ScopeData) -> list[RoleAssignmentData]:
     """Get all the subjects assigned to any role in a specific scope.
 
     Args:
         scope: The scope to filter subjects (e.g., 'library:123' or '*' for global).
 
     Returns:
-        list[RoleAssignment]: A list of subjects assigned to roles in the specified scope.
+        list[RoleAssignment]: A list of role assignments for all subjects in the specified scope.
     """
-    role_assignments = []
+    role_assignments_per_subject = {}
     roles_in_scope = get_all_roles_in_scope(scope)
 
     for policy in roles_in_scope:
         subject = SubjectData(namespaced_key=policy[GroupingPolicyIndex.SUBJECT.value])
         role = RoleData(namespaced_key=policy[GroupingPolicyIndex.ROLE.value])
         role.permissions = get_permissions_for_single_role(role)
 
-        role_assignments.append(
-            RoleAssignmentData(
-                subject=subject,
-                role=role,
-                scope=scope,
-            )
+        if subject.external_key in role_assignments_per_subject:
+            role_assignments_per_subject[subject.external_key].roles.append(role)
+            continue
+
+        role_assignments_per_subject[subject.external_key] = RoleAssignmentData(
+            subject=subject,
+            roles=[role],
+            scope=scope,
         )
-    return role_assignments
+
+    return list(role_assignments_per_subject.values())

openedx_authz/api/users.py

@@ -24,7 +24,7 @@
     get_all_subject_role_assignments_in_scope,
     get_subject_role_assignments,
     get_subject_role_assignments_in_scope,
-    get_subjects_role_assignments_for_role_in_scope,
+    get_subject_role_assignments_for_role_in_scope,
     unassign_role_from_subject_in_scope,
 )
 
@@ -145,11 +145,9 @@ def get_user_role_assignments_for_role_in_scope(
         scope (str): Scope in which to retrieve the role assignments.
 
     Returns:
-        list[RoleAssignmentData]: A list of user names and all their metadata assigned to the role.
+        list[RoleAssignmentData]: List of users assigned to the specified role in the given scope.
     """
-    # TODO: this SHOULD definitely be managed in a better way by using class inheritance and factories
-    # But for now we'll keep it simple and explicit
-    return get_subjects_role_assignments_for_role_in_scope(
+    return get_subject_role_assignments_for_role_in_scope(
         RoleData(external_key=role_external_key),
         ScopeData(external_key=scope_external_key),
     )

openedx_authz/tests/api/test_roles.py

@@ -29,7 +29,7 @@
     get_role_definitions_in_scope,
     get_subject_role_assignments,
     get_subject_role_assignments_in_scope,
-    get_subjects_role_assignments_for_role_in_scope,
+    get_subject_role_assignments_for_role_in_scope,
     unassign_role_from_subject_in_scope,
 )
 from openedx_authz.engine.enforcer import enforcer as global_enforcer
@@ -572,7 +572,7 @@ def test_get_subject_role_assignments_in_scope(
             SubjectData(external_key=subject_name), ScopeData(external_key=scope_name)
         )
 
-        role_names = {assignment.role.external_key for assignment in role_assignments}
+        role_names = {r.external_key for assignment in role_assignments for r in assignment.roles}
         self.assertEqual(role_names, expected_roles)
 
     @ddt_data(
@@ -758,7 +758,7 @@ def test_get_all_role_assignments_scopes(self, subject_name, expected_roles):
         for expected_role in expected_roles:
             # Compare the role part of the assignment
             found = any(
-                assignment.role == expected_role for assignment in role_assignments
+                expected_role in assignment.roles for assignment in role_assignments
             )
             self.assertTrue(
                 found, f"Expected role {expected_role} not found in assignments"
@@ -797,7 +797,7 @@ def test_get_role_assignments_in_scope(self, role_name, scope_name, expected_cou
         Expected result:
             - The number of role assignments in the given scope is correctly retrieved.
         """
-        role_assignments = get_subjects_role_assignments_for_role_in_scope(
+        role_assignments = get_subject_role_assignments_for_role_in_scope(
             RoleData(external_key=role_name), ScopeData(external_key=scope_name)
         )
 
@@ -860,7 +860,7 @@ def test_batch_assign_role_to_subjects_in_scope(
                 user_roles = get_subject_role_assignments_in_scope(
                     SubjectData(external_key=subject_name), ScopeData(external_key=scope_name)
                 )
-                role_names = {assignment.role.external_key for assignment in user_roles}
+                role_names = {r.external_key for assignment in user_roles for r in assignment.roles}
                 self.assertIn(role, role_names)
         else:
             assign_role_to_subject_in_scope(
@@ -872,7 +872,7 @@ def test_batch_assign_role_to_subjects_in_scope(
                 SubjectData(external_key=subject_names),
                 ScopeData(external_key=scope_name),
             )
-            role_names = {assignment.role.external_key for assignment in user_roles}
+            role_names = {r.external_key for assignment in user_roles for r in assignment.roles}
             self.assertIn(role, role_names)
 
     @ddt_data(
@@ -917,7 +917,7 @@ def test_unassign_role_from_subject_in_scope(
                     SubjectData(external_key=subject),
                     ScopeData(external_key=scope_name),
                 )
-                role_names = {assignment.role.external_key for assignment in user_roles}
+                role_names = {r.external_key for assignment in user_roles for r in assignment.roles}
                 self.assertNotIn(role, role_names)
         else:
             unassign_role_from_subject_in_scope(
@@ -929,7 +929,7 @@ def test_unassign_role_from_subject_in_scope(
                 SubjectData(external_key=subject_names),
                 ScopeData(external_key=scope_name),
             )
-            role_names = {assignment.role.external_key for assignment in user_roles}
+            role_names = {r.external_key for assignment in user_roles for r in assignment.roles}
             self.assertNotIn(role, role_names)
 
     @ddt_data(
@@ -938,7 +938,7 @@ def test_unassign_role_from_subject_in_scope(
             [
                 RoleAssignmentData(
                     subject=SubjectData(external_key="alice"),
-                    role=RoleData(
+                    roles=[RoleData(
                         external_key="library_admin",
                         permissions=[
                             PermissionData(
@@ -986,7 +986,7 @@ def test_unassign_role_from_subject_in_scope(
                                 effect="allow",
                             ),
                         ],
-                    ),
+                    )],
                     scope=ScopeData(external_key="lib:Org1:math_101"),
                 )
             ],
@@ -996,7 +996,7 @@ def test_unassign_role_from_subject_in_scope(
             [
                 RoleAssignmentData(
                     subject=SubjectData(external_key="bob"),
-                    role=RoleData(
+                    roles=[RoleData(
                         external_key="library_author",
                         permissions=[
                             PermissionData(
@@ -1038,7 +1038,7 @@ def test_unassign_role_from_subject_in_scope(
                                 effect="allow",
                             ),
                         ],
-                    ),
+                    )],
                     scope=ScopeData(external_key="lib:Org1:history_201"),
                 )
             ],
@@ -1048,7 +1048,7 @@ def test_unassign_role_from_subject_in_scope(
             [
                 RoleAssignmentData(
                     subject=SubjectData(external_key="carol"),
-                    role=RoleData(
+                    roles=[RoleData(
                         external_key="library_collaborator",
                         permissions=[
                             PermissionData(
@@ -1084,7 +1084,7 @@ def test_unassign_role_from_subject_in_scope(
                                 effect="allow",
                             ),
                         ],
-                    ),
+                    )],
                     scope=ScopeData(external_key="lib:Org1:science_301"),
                 )
             ],
@@ -1094,7 +1094,7 @@ def test_unassign_role_from_subject_in_scope(
             [
                 RoleAssignmentData(
                     subject=SubjectData(external_key="dave"),
-                    role=RoleData(
+                    roles=[RoleData(
                         external_key="library_user",
                         permissions=[
                             PermissionData(
@@ -1110,7 +1110,7 @@ def test_unassign_role_from_subject_in_scope(
                                 effect="allow",
                             ),
                         ],
-                    ),
+                    )],
                     scope=ScopeData(external_key="lib:Org1:english_101"),
                 )
             ],