refactor: address PR reviews

Author: mariajgrimaldi

openedx_authz/api/data.py

@@ -89,9 +89,7 @@ def __attrs_post_init__(self):
 
         # Case 3: Neither provided, raise error
         if not self.external_key and not self.namespaced_key:
-            raise ValueError(
-                "Either external_key or namespaced_key must be provided."
-            )
+            raise ValueError("Either external_key or namespaced_key must be provided.")
 
 
 class ScopeMeta(type):
@@ -329,7 +327,6 @@ class ActionData(AuthZData):
     """
 
     NAMESPACE: ClassVar[str] = "act"
-    name: str = ""
 
     @property
     def name(self) -> str:
@@ -356,36 +353,17 @@ class PermissionData(AuthZData):
     effect: Literal["allow", "deny"] = "allow"
 
 
-@define
-class RoleMetadataData(AuthZData):
-    """Metadata for a role.
-
-    Attributes:
-        description: A description of the role.
-        created_at: The date and time the role was created.
-        created_by: The ID of the subject who created the role.
-    """
-
-    description: str = None
-    created_at: str = None
-    created_by: str = None
-
-
 @define
 class RoleData(AuthZData):
     """A role is a named group of permissions.
 
     Attributes:
         name: The name of the role. Must have 'role@' namespace prefix.
-        role_id: The role identifier namespaced (e.g., 'role@instructor').
         permissions: A list of permissions assigned to the role.
-        metadata: A dictionary of metadata assigned to the role. This can include
-            information such as the description of the role, creation date, etc.
     """
 
     NAMESPACE: ClassVar[str] = "role"
-    permissions: list[PermissionData] = None
-    metadata: RoleMetadataData = None
+    permissions: list[PermissionData] = list()
 
     @property
     def name(self) -> str:
@@ -405,10 +383,9 @@ class RoleAssignmentData(AuthZData):
     """A role assignment is the assignment of a role to a subject in a specific scope.
 
     Attributes:
-        subject: The ID of the user namespaced (e.g., 'user@john_doe').
-        email: The email of the user.
-        role_name: The name of the role.
-        scope: The scope in which the role is assigned.
+        subject: The subject to whom the role is assigned (e.g., user or service).
+        role: The role being assigned.
+        scope: The scope in which the role is assigned (e.g., organization, course).
     """
 
     subject: SubjectData = None  # Needs defaults to avoid value error from attrs

openedx_authz/api/permissions.py

@@ -5,7 +5,13 @@
 are not explicitly defined, but are inferred from the policy rules.
 """
 
-from openedx_authz.api.data import ActionData, PermissionData, PolicyIndex, ScopeData, SubjectData
+from openedx_authz.api.data import (
+    ActionData,
+    PermissionData,
+    PolicyIndex,
+    ScopeData,
+    SubjectData,
+)
 from openedx_authz.engine.enforcer import enforcer
 
 __all__ = [

openedx_authz/api/roles.py

@@ -23,6 +23,7 @@
 from openedx_authz.engine.enforcer import enforcer
 
 __all__ = [
+    "get_permissions_for_single_role",
     "get_permissions_for_roles",
     "get_all_roles_names",
     "get_all_roles_in_scope",
@@ -47,8 +48,23 @@
 # in this case, ALL the policies, but that might not be the case
 
 
+def get_permissions_for_single_role(
+    role: RoleData,
+) -> list[PermissionData]:
+    """Get the permissions (actions) for a single role.
+
+    Args:
+        role: A RoleData object representing the role.
+
+    Returns:
+        list[PermissionData]: A list of PermissionData objects associated with the given role.
+    """
+    policies = enforcer.get_implicit_permissions_for_user(role.namespaced_key)
+    return [get_permission_from_policy(policy) for policy in policies]
+
+
 def get_permissions_for_roles(
-    roles: list[RoleData] | RoleData,
+    roles: list[RoleData],
 ) -> dict[str, dict[str, list[PermissionData | str]]]:
     """Get the permissions (actions) for a list of roles.
 
@@ -59,22 +75,11 @@ def get_permissions_for_roles(
         dict[str, list[PermissionData]]: A dictionary mapping role names to their permissions and scopes.
     """
     permissions_by_role = {}
-    if not roles:
-        return permissions_by_role
-
-    if isinstance(roles, RoleData):
-        roles = [roles]
 
     for role in roles:
-        policies = enforcer.get_implicit_permissions_for_user(role.namespaced_key)
-
-        permissions_by_role[role.external_key] = (
-            {  # Index by role external_key for easy lookup
-                "permissions": [
-                    get_permission_from_policy(policy) for policy in policies
-                ],
-            }
-        )
+        permissions_by_role[role.external_key] = {
+            "permissions": get_permissions_for_single_role(role)
+        }
 
     return permissions_by_role
 
@@ -252,7 +257,7 @@ def get_subject_role_assignments(subject: SubjectData) -> list[RoleAssignmentDat
     """Get all the roles for a subject across all scopes.
 
     Args:
-        subject: The ID of the subject namespaced (e.g., 'subject:john_doe').
+        subject: The ID of the subject namespaced (e.g., 'subject^john_doe').
 
     Returns:
         list[Role]: A list of role names and all their metadata assigned to the subject.
@@ -262,11 +267,7 @@ def get_subject_role_assignments(subject: SubjectData) -> list[RoleAssignmentDat
         GroupingPolicyIndex.SUBJECT.value, subject.namespaced_key
     ):
         role = RoleData(namespaced_key=policy[GroupingPolicyIndex.ROLE.value])
-        role.permissions = get_permissions_for_roles(role)[
-            role.external_key
-        ][  # Index by role external_key for readability
-            "permissions"
-        ]
+        role.permissions = get_permissions_for_single_role(role)
 
         role_assignments.append(
             RoleAssignmentData(
@@ -284,7 +285,7 @@ def get_subject_role_assignments_in_scope(
     """Get the roles for a subject in a specific scope.
 
     Args:
-        subject: The ID of the subject namespaced (e.g., 'subject:john_doe').
+        subject: The ID of the subject namespaced (e.g., 'subject^john_doe').
         scope: The scope to filter roles (e.g., 'library:123').
 
     Returns:
@@ -301,9 +302,7 @@ def get_subject_role_assignments_in_scope(
                 subject=subject,
                 role=RoleData(
                     namespaced_key=namespaced_key,
-                    permissions=get_permissions_for_roles(role)[role.external_key][
-                        "permissions"
-                    ],
+                    permissions=get_permissions_for_single_role(role),
                 ),
                 scope=scope,
             )
@@ -332,14 +331,10 @@ def get_subjects_role_assignments_for_role_in_scope(
             continue
         role_assignments.append(
             RoleAssignmentData(
-                subject=SubjectData(
-                    namespaced_key=subject
-                ),
+                subject=SubjectData(namespaced_key=subject),
                 role=RoleData(
                     external_key=role.external_key,
-                    permissions=get_permissions_for_roles(role)[role.external_key][
-                        "permissions"
-                    ],
+                    permissions=get_permissions_for_single_role(role),
                 ),
                 scope=scope,
             )
@@ -364,9 +359,7 @@ def get_all_subject_role_assignments_in_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_roles(role)[role.external_key][
-            "permissions"
-        ]  # Index by role external_key for easy lookup
+        role.permissions = get_permissions_for_single_role(role)
 
         role_assignments.append(
             RoleAssignmentData(

openedx_authz/api/users.py

@@ -9,7 +9,13 @@
 (e.g., 'user@john_doe').
 """
 
-from openedx_authz.api.data import ActionData, RoleAssignmentData, RoleData, ScopeData, UserData
+from openedx_authz.api.data import (
+    ActionData,
+    RoleAssignmentData,
+    RoleData,
+    ScopeData,
+    UserData,
+)
 from openedx_authz.api.permissions import has_permission
 from openedx_authz.api.roles import (
     assign_role_to_subject_in_scope,
@@ -24,7 +30,7 @@
 
 __all__ = [
     "assign_role_to_user_in_scope",
-    "batch_assign_role_to_users",
+    "batch_assign_role_to_users_in_scope",
     "unassign_role_from_user",
     "batch_unassign_role_from_users",
     "get_user_role_assignments",
@@ -52,9 +58,7 @@ def assign_role_to_user_in_scope(
     )
 
 
-def batch_assign_role_to_users(
-    users: list[str], role_external_key: str, scope_external_key: str
-) -> dict[str, bool]:
+def batch_assign_role_to_users_in_scope(users: list[str], role_external_key: str, scope_external_key: str):
     """Assign a role to multiple users in a specific scope.
 
     Args:
@@ -70,9 +74,7 @@ def batch_assign_role_to_users(
     )
 
 
-def unassign_role_from_user(
-    user_external_key: str, role_external_key: str, scope_external_key: str
-) -> bool:
+def unassign_role_from_user(user_external_key: str, role_external_key: str, scope_external_key: str):
     """Unassign a role from a user in a specific scope.
 
     Args:
@@ -87,9 +89,7 @@ def unassign_role_from_user(
     )
 
 
-def batch_unassign_role_from_users(
-    users: list[str], role_external_key: str, scope_external_key: str
-) -> dict[str, bool]:
+def batch_unassign_role_from_users(users: list[str], role_external_key: str, scope_external_key: str):
     """Unassign a role from multiple users in a specific scope.
 
     Args:
@@ -112,7 +112,7 @@ def get_user_role_assignments(user_external_key: str) -> list[RoleAssignmentData
         user_external_key (str): ID of the user (e.g., 'john_doe').
 
     Returns:
-        list[dict]: A list of role assignments and all their metadata assigned to the user.
+        list[RoleAssignmentData]: A list of role assignments and all their metadata assigned to the user.
     """
     return get_subject_role_assignments(UserData(external_key=user_external_key))
 
@@ -127,7 +127,7 @@ def get_user_role_assignments_in_scope(
         scope (str): Scope in which to retrieve the roles.
 
     Returns:
-        list: A list of role assignments assigned to the user in the specified scope.
+        list[RoleAssignmentData]: A list of role assignments assigned to the user in the specified scope.
     """
     return get_subject_role_assignments_in_scope(
         UserData(external_key=user_external_key),
@@ -145,7 +145,7 @@ def get_user_role_assignments_for_role_in_scope(
         scope (str): Scope in which to retrieve the role assignments.
 
     Returns:
-        list[dict]: A list of user names and all their metadata assigned to the role.
+        list[RoleAssignmentData]: A list of user names and all their metadata assigned to the role.
     """
     # 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
@@ -164,7 +164,7 @@ def get_all_user_role_assignments_in_scope(
         scope (str): Scope in which to retrieve the user role assignments.
 
     Returns:
-        list[dict]: A list of user role assignments and all their metadata in the specified scope.
+        list[RoleAssignmentData]: A list of user role assignments and all their metadata in the specified scope.
     """
     return get_all_subject_role_assignments_in_scope(
         ScopeData(external_key=scope_external_key)

openedx_authz/engine/utils.py

@@ -13,7 +13,7 @@
 GROUPING_POLICY_PTYPES = ["g", "g2", "g3", "g4", "g5", "g6"]
 
 
-def migrate_policy_from_file_to_db(
+def migrate_policy_between_enforcers(
     source_enforcer: Enforcer,
     target_enforcer: Enforcer,
 ) -> None:

openedx_authz/management/commands/load_policies.py

@@ -13,7 +13,7 @@
 from django.core.management.base import BaseCommand
 
 from openedx_authz.engine.enforcer import enforcer as global_enforcer
-from openedx_authz.engine.utils import migrate_policy_from_file_to_db
+from openedx_authz.engine.utils import migrate_policy_between_enforcers
 
 
 class Command(BaseCommand):
@@ -49,11 +49,6 @@ def add_arguments(self, parser) -> None:
             default="openedx_authz/engine/config/model.conf",
             help="Path to the Casbin model configuration file",
         )
-        parser.add_argument(
-            "--clear-existing",
-            action="store_true",
-            help="Clear existing policies in the database before loading new ones",
-        )
 
     def handle(self, *args, **options):
         """Execute the policy loading command.
@@ -87,4 +82,4 @@ def migrate_policies(self, source_enforcer, target_enforcer):
             source_enforcer: The Casbin enforcer instance to migrate policies from.
             target_enforcer: The Casbin enforcer instance to migrate policies to.
         """
-        migrate_policy_from_file_to_db(source_enforcer, target_enforcer)
+        migrate_policy_between_enforcers(source_enforcer, target_enforcer)

openedx_authz/tests/api/test_data.py

@@ -74,7 +74,7 @@ def test_scope_content_lib_data_namespace(self, external_key):
 
 
 @ddt
-class TestPolymorphismLowLevelAPIs(TestCase):
+class TestPolymorphicData(TestCase):
     """Test polymorphic factory pattern for SubjectData and ScopeData."""
 
     @data(