refactor: add error management when getting scope subclasses

Author: mariajgrimaldi

openedx_authz/api/data.py

@@ -21,6 +21,7 @@
 ]
 
 AUTHZ_POLICY_ATTRIBUTES_SEPARATOR = "^"
+EXTERNAL_KEY_SEPARATOR = ":"
 
 
 class GroupingPolicyIndex(Enum):
@@ -162,12 +163,21 @@ def get_subclass_by_external_key(mcs, external_key: str) -> Type["ScopeData"]:
         # even 'course-v1:edX+DemoX+2021_T1'. This won't work for org scopes because they don't explicitly indicate
         # the namespace in the external key. TODO: We need to handle org scopes differently.
         # 2. The namespace is always the part before the first separator.
-        # 3. If the namespace is not recognized, we return the base ScopeData class
-        # 4. The subclass implements a validation method to validate the entire key
-        namespace = external_key.split(":", 1)[0]
+        # 3. If the namespace is not recognized, we raise an error.
+        # 4. The subclass implements a validation method to validate the entire key. E.g., ContentLibraryData
+        # validates that the external_key is a valid library ID.
+        if EXTERNAL_KEY_SEPARATOR not in external_key:
+            raise ValueError(f"Invalid external_key format: {external_key}")
+
+        namespace = external_key.split(EXTERNAL_KEY_SEPARATOR, 1)[0]
         scope_subclass = mcs.scope_registry.get(namespace)
-        if not scope_subclass or not scope_subclass.validate_external_key(external_key):
-            return ScopeData  # Fallback to base class if not found or invalid
+
+        if not scope_subclass:
+            raise ValueError(f"Unknown scope: {namespace} for external_key: {external_key}")
+
+        if not scope_subclass.validate_external_key(external_key):
+            raise ValueError(f"Invalid external_key format: {external_key}")
+
         return scope_subclass
 
     @classmethod
@@ -195,6 +205,22 @@ class ScopeData(AuthZData, metaclass=ScopeMeta):
 
     NAMESPACE: ClassVar[str] = "sc"
 
+    @classmethod
+    def validate_external_key(cls, external_key: str) -> bool:
+        """Validate the external_key format for ScopeData.
+
+        For the base ScopeData class, we accept any external_key works. This
+        is only implemented for the sake of completeness. Subclasses should
+        implement their own validation logic.
+
+        Args:
+            external_key: The external key to validate.
+
+        Returns:
+            bool: True if valid, False otherwise.
+        """
+        return True
+
 
 @define
 class ContentLibraryData(ScopeData):

openedx_authz/tests/api/test_data.py

@@ -238,7 +238,7 @@ def test_get_subclass_by_namespaced_key(self, namespaced_key, expected_class):
     @data(
         ("lib:DemoX:CSPROB", ContentLibraryData),
         ("lib:edX:Demo", ContentLibraryData),
-        ("unknown:something", ScopeData),
+        ("sc:generic_scope", ScopeData),
     )
     @unpack
     def test_get_subclass_by_external_key(self, external_key, expected_class):
@@ -247,7 +247,6 @@ def test_get_subclass_by_external_key(self, external_key, expected_class):
         Expected Result:
             - 'lib:...' returns ContentLibraryData
             - 'sc:...' returns ScopeData
-            - 'unknown:...' returns ScopeData (fallback)
         """
         subclass = ScopeMeta.get_subclass_by_external_key(external_key)
         self.assertIs(subclass, expected_class)
@@ -287,8 +286,8 @@ def test_base_scope_data_with_external_key(self):
             - ScopeData(external_key='...') creates ScopeData instance
             - No dynamic subclass selection occurs
         """
-        scope = ScopeData(external_key="generic_scope")
+        scope = ScopeData(external_key="sc:generic_scope")
         self.assertIsInstance(scope, ScopeData)
-        self.assertEqual(scope.external_key, "generic_scope")
-        expected_namespaced = f"{ScopeData.NAMESPACE}{ScopeData.SEPARATOR}generic_scope"
+        self.assertEqual(scope.external_key, "sc:generic_scope")
+        expected_namespaced = f"{ScopeData.NAMESPACE}{ScopeData.SEPARATOR}sc:generic_scope"
         self.assertEqual(scope.namespaced_key, expected_namespaced)

openedx_authz/tests/api/test_roles.py

@@ -786,9 +786,9 @@ def test_get_all_role_assignments_scopes(self, subject_name, expected_roles):
         ("library_author", "lib:Org6:project_beta", 1),
         ("library_collaborator", "lib:Org6:project_gamma", 1),
         ("library_user", "lib:Org6:project_delta", 1),
-        ("non_existent_role", "any_library", 0),
-        ("library_admin", "non_existent_scope", 0),
-        ("non_existent_role", "non_existent_scope", 0),
+        ("non_existent_role", "sc:any_library", 0),
+        ("library_admin", "sc:non_existent_scope", 0),
+        ("non_existent_role", "sc:non_existent_scope", 0),
     )
     @unpack
     def test_get_role_assignments_in_scope(self, role_name, scope_name, expected_count):
@@ -817,7 +817,7 @@ class TestRoleAssignmentAPI(RolesTestSetupMixin):
     """
 
     @ddt_data(
-        (["mary", "john"], "library_user", "batch_test", True),
+        (["mary", "john"], "library_user", "sc:batch_test", True),
         (
             ["paul", "diana", "lila"],
             "library_collaborator",
@@ -876,7 +876,7 @@ def test_batch_assign_role_to_subjects_in_scope(
             self.assertIn(role, role_names)
 
     @ddt_data(
-        (["mary", "john"], "library_user", "batch_test", True),
+        (["mary", "john"], "library_user", "sc:batch_test", True),
         (
             ["paul", "diana", "lila"],
             "library_collaborator",
@@ -1115,7 +1115,7 @@ def test_unassign_role_from_subject_in_scope(
                 )
             ],
         ),
-        ("non_existent_scope", []),
+        ("sc:non_existent_scope", []),
     )
     @unpack
     def test_get_all_role_assignments_in_scope(self, scope_name, expected_assignments):