fix: harden CLARIN shared surface + dtq owningCollection reauth

dspace_rest_client/client.py

@@ -1096,7 +1096,12 @@ def get_owningCollection(self, item_uuid):
         url = f'{self.API_ENDPOINT}/core/items/{item_uuid}/owningCollection'
         try:
             r = self.api_get(url, None, None)
-            self.verify_response(r, f"item:{item_uuid}",  True)
+            # On a non-200, verify_response records self._last_err and returns
+            # False - return None here (not an empty, truthy Collection) so the
+            # caller's `owning_col is None and last_err.status_code == 401`
+            # reauth path (src/repo/_audit.py) actually fires.
+            if not self.verify_response(r, f"item:{item_uuid}", True):
+                return None
             r_json = parse_json(response=r)
             return Collection(r_json)
         except ValueError:
@@ -1257,7 +1262,12 @@ def create_submit_group(self, collection):
         url = f'{self.API_ENDPOINT}/core/collections/{collection.uuid}/submittersGroup'
         r = self.api_post(url, json={}, params=None)
         if r.status_code == 201:
-            return Group(parse_json(r))
+            # a 201 with an empty/invalid body would make Group(None) here; return
+            # None instead so a caller's `if not group` guard fires cleanly rather
+            # than passing a uuid-less Group into add_member()
+            j = parse_json(r)
+            if j:
+                return Group(j)
         return None
 
     def add_member(self, group, eperson):
@@ -1280,7 +1290,9 @@ def add_member(self, group, eperson):
             return False
 
         url = f'{self.API_ENDPOINT}/eperson/groups/{group.uuid}/epersons'
-        eperson_uri = f'{self.API_ENDPOINT}/epersons/{eperson.uuid}'
+        # canonical eperson href is /eperson/epersons/{uuid}; a bare /epersons/
+        # path does not resolve and DSpace rejects the uri-list with a 422
+        eperson_uri = f'{self.API_ENDPOINT}/eperson/epersons/{eperson.uuid}'
         r = self.api_post_uri(url, params=None, uri_list=eperson_uri)
         if r.status_code == 204:
             return True
@@ -1350,7 +1362,9 @@ def get_items_from_collection(self, collection_id, page=0, size=1000):
         items = list()
         r = self.api_get(url)
         r_json = parse_json(r)
-        if '_embedded' in r_json:
+        # a failed request parses to None; return the empty list rather than
+        # crashing on `'_embedded' in None`
+        if r_json and '_embedded' in r_json:
             if 'searchResult' in r_json['_embedded']:
                 if '_embedded' in r_json['_embedded']['searchResult']:
                     for item_resource in r_json['_embedded']['searchResult']['_embedded']['objects']:
@@ -1367,7 +1381,9 @@ def get_bundle_by_name(self, name, item_uuid):
         """
         url = f'{self.API_ENDPOINT}/core/items/{item_uuid}/bundles'
         r_json = self.fetch_resource(url, params=None)
-        if '_embedded' in r_json:
+        # fetch_resource returns None on any non-200 (records self._last_err);
+        # guard so a failed lookup is a clean None, not a NoneType subscript crash
+        if r_json and '_embedded' in r_json:
             if 'bundles' in r_json['_embedded']:
                 for bundle in r_json['_embedded']['bundles']:
                     if bundle['name'] == name:
@@ -1380,10 +1396,20 @@ def get_resource_policy(self, bundle_uuid):
         """
         url = f'{self.API_ENDPOINT}/authz/resourcepolicies/search/resource?uuid={bundle_uuid}&embed=eperson&embed=group'
         r = self.api_get(url)
+        # record the failing response so a caller can tell an HTTP error apart
+        # from a genuine "no policy" (empty list) result - both return None
+        if r.status_code != 200:
+            self._last_err = r
+            _logger.error(f'Error fetching resource policy [{bundle_uuid}]: {r.status_code}')
+            return None
         r_json = parse_json(r)
-        if '_embedded' in r_json:
-            if 'resourcepolicies' in r_json['_embedded']:
-                return r_json['_embedded']['resourcepolicies'][0]
+        # guard against an unparseable body and against an empty policy list -
+        # both are a clean None, not a TypeError/IndexError
+        if r_json and '_embedded' in r_json and 'resourcepolicies' in r_json['_embedded']:
+            policies = r_json['_embedded']['resourcepolicies']
+            if policies:
+                return policies[0]
+        return None
 
     def create_resource_policy(self, resource_uuid, data, group_uuid=None, eperson_uuid=None):
         """
@@ -1475,7 +1501,19 @@ def get_user_by_email(self, email):
         params = {'email': email}
         try:
             response = self.api_get(url, params=params)
+            # a miss returns 404 (with a JSON error body): building a User from
+            # that yields a truthy, uuid-less object that passes `if user:` and
+            # fails much later. Treat any non-200 as "no such user" -> None, but
+            # record last_err so callers keep the HTTP-error diagnostics, and
+            # don't log a plain 404 miss at error level (nor the raw email).
+            if response.status_code != 200:
+                self._last_err = response
+                if response.status_code != 404:
+                    _logger.error(f"Error retrieving user by email: HTTP {response.status_code}")
+                return None
             user_data = parse_json(response)
+            if not user_data:
+                return None
             return User(user_data)
         except Exception as e:
             _logger.error(f"Error retrieving user by email {email}: {e}")

dspace_rest_client/models.py

@@ -407,6 +407,7 @@ def __init__(self, api_resource=None):
         @param api_resource: API result object to use as initial data
         """
         super().__init__(api_resource)
+        api_resource = api_resource or {}
         self.type = 'group'
         if 'name' in api_resource:
             self.name = api_resource['name']
@@ -442,6 +443,7 @@ def __init__(self, api_resource=None):
         @param api_resource: API result object to use as initial data
         """
         super().__init__(api_resource)
+        api_resource = api_resource or {}
         self.type = 'user'
         if 'name' in api_resource:
             self.name = api_resource['name']

tests/test_clarin_read.py

@@ -52,6 +52,26 @@ def test_sends_uuid_and_both_embeds(self):
             self.assertEqual(qs["uuid"], [BUNDLE_UUID])
             self.assertEqual(sorted(qs["embed"]), ["eperson", "group"])
 
+    @pytest.mark.dtq_only
+    def test_empty_list_returns_none(self):
+        """D2: an empty policy list must be a clean None, not an IndexError."""
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(self.URL, json=embedded("resourcepolicies", []))
+            self.assertIsNone(c.get_resource_policy(BUNDLE_UUID))
+
+    @pytest.mark.dtq_only
+    def test_non_200_returns_none_and_records_last_err(self):
+        """D3: a failed request must be None, not a NoneType subscript crash -
+        and last_err is recorded so a caller can tell an HTTP error apart from a
+        genuine empty result (both return None)."""
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(self.URL, status_code=500, text="upstream boom")
+            self.assertIsNone(c.get_resource_policy(BUNDLE_UUID))
+            self.assertIsNotNone(c.last_err)
+            self.assertEqual(c.last_err.status_code, 500)
+
 
 class TestGetBundleByName(unittest.TestCase):
     """Mirrors dspace-import-clarin - get_bundle_by_name('ORIGINAL', item)."""
@@ -74,6 +94,14 @@ def test_no_match_returns_none(self):
             m.get(self.URL, json=embedded("bundles", [bundle_json("b1", "LICENSE")]))
             self.assertIsNone(c.get_bundle_by_name("ORIGINAL", ITEM_UUID))
 
+    @pytest.mark.dtq_only
+    def test_non_200_returns_none(self):
+        """D1: a failed lookup must be None, not a NoneType subscript crash."""
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(self.URL, status_code=500, text="boom")
+            self.assertIsNone(c.get_bundle_by_name("ORIGINAL", ITEM_UUID))
+
 
 class TestGetItemsFromCollection(unittest.TestCase):
     """Mirrors dspace-import-clarin - get_items_from_collection(collection)."""
@@ -102,6 +130,14 @@ def test_sends_scope_dsotype_sort_embed(self):
             self.assertEqual(qs["sort"], ["dc.date.accessioned,DESC"])
             self.assertEqual(qs["embed"], ["thumbnail"])
 
+    @pytest.mark.dtq_only
+    def test_non_200_returns_empty(self):
+        """D6: a failed request must yield [], not a NoneType subscript crash."""
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(self.URL, status_code=500, text="boom")
+            self.assertEqual(c.get_items_from_collection(COLLECTION_UUID), [])
+
 
 class TestGetItemByHandle(unittest.TestCase):
     """Mirrors dspace-rest-test - get_item_by_handle(handle)."""
@@ -150,6 +186,20 @@ def test_returns_user_with_uuid_and_email(self):
             self.assertEqual((u.uuid, u.email), (EPERSON_UUID, "a@b.c"))
             self.assertEqual(sent_params(m.last_request)["email"], ["a@b.c"])
 
+    @pytest.mark.dtq_only
+    def test_404_returns_none(self):
+        """D4: a miss (404) must be falsy, not a truthy uuid-less User that
+        slips past the consumer's `if user:` guard."""
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(self.URL, status_code=404, json={"timestamp": "now"})
+            u = c.get_user_by_email("nobody@nowhere")
+            self.assertIsNone(u)
+            self.assertFalse(bool(u))
+            # the failing response is retained for callers, even for a 404 miss
+            self.assertIsNotNone(c.last_err)
+            self.assertEqual(c.last_err.status_code, 404)
+
 
 class TestGetClarinAllowances(unittest.TestCase):
     """Mirrors dspace-rest-test - get_clarinlruallowances[_by_bitstream_and_user]."""
@@ -197,6 +247,17 @@ def test_returns_typed_collection(self):
             self.assertIsInstance(col, Collection)
             self.assertEqual(col.uuid, COLLECTION_UUID)
 
+    def test_non_200_returns_none_and_sets_last_err(self):
+        """D8: on 401 the method must return None (not an empty truthy
+        Collection) and expose last_err, or _audit.py's reauth branch is dead."""
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(self.URL, status_code=401, json={"message": "Unauthorized"})
+            col = c.get_owningCollection(ITEM_UUID)
+            self.assertIsNone(col)
+            self.assertIsNotNone(c.last_err)
+            self.assertEqual(c.last_err.status_code, 401)
+
 
 if __name__ == "__main__":
     unittest.main()

tests/test_clarin_usage_contract.py

@@ -53,6 +53,16 @@ def test_bundle_then_policy_dict_then_group_update(self):
             self.assertEqual(r.status_code, 200)
             self.assertEqual(m.last_request.method, "PUT")
 
+    @pytest.mark.dtq_only
+    def test_missing_bundle_aborts_cleanly(self):
+        """D1: when the bundle lookup fails, get_bundle_by_name is None and the
+        chain stops - it must not crash on a NoneType subscript."""
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(f"{API}/core/items/{ITEM_UUID}/bundles",
+                  status_code=500, text="boom")
+            self.assertIsNone(c.get_bundle_by_name("ORIGINAL", ITEM_UUID))
+
 
 class TestImportClarinLicenseIngest(unittest.TestCase):
     """Mirrors dspace-import-clarin - read a collection's items, then build the
@@ -119,6 +129,21 @@ def test_email_group_member(self):
 
             self.assertTrue(c.add_member(group, user))
 
+    @pytest.mark.dtq_only
+    def test_unknown_email_stops_before_add_member(self):
+        """D4: an unknown email must resolve to None so the consumer's
+        `if user:` guard skips group creation - not proceed with a uuid-less
+        User and fail deep inside add_member."""
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(f"{API}/eperson/epersons/search/byEmail",
+                  status_code=404, json={"timestamp": "now"})
+
+            user = c.get_user_by_email("ghost@nowhere")
+            self.assertIsNone(user)
+            # consumer guard: nothing past the lookup should have been requested
+            self.assertEqual(len(m.request_history), 1)
+
 
 class TestRestTestBitstreamPolicyFlow(unittest.TestCase):
     """Mirrors dspace-rest-test - resolve an item by handle, find its ORIGINAL

tests/test_clarin_write.py

@@ -145,6 +145,16 @@ def test_non_201_returns_none(self):
             m.post(url, status_code=500, text="boom")
             self.assertIsNone(c.create_submit_group(self._collection()))
 
+    @pytest.mark.dtq_only
+    def test_empty_body_returns_none(self):
+        """D5: a 201 with an empty body must be None, not a Group(None) crash /
+        a uuid-less Group that would poison a following add_member call."""
+        c = make_client()
+        url = f"{API}/core/collections/{COLLECTION_UUID}/submittersGroup"
+        with requests_mock.Mocker() as m:
+            m.post(url, status_code=201, text="")
+            self.assertIsNone(c.create_submit_group(self._collection()))
+
 
 class TestAddMember(unittest.TestCase):
     """Mirrors dspace-rest-test - add_member(group, eperson)."""
@@ -155,6 +165,21 @@ def _group(self):
     def _user(self):
         return User({"uuid": EPERSON_UUID, "email": "a@b.c"})
 
+    @pytest.mark.dtq_only
+    def test_posts_eperson_uri_and_returns_true_on_204(self):
+        """The eperson uri-list body must be the canonical /eperson/epersons/
+        href. dtq_only: the fix (dropping the bare /epersons/ path) lands on
+        dtq; main still emits the malformed URI, so this is deselected on the
+        main leg of the differential-contract job."""
+        c = make_client()
+        url = f"{API}/eperson/groups/{GROUP_UUID}/epersons"
+        with requests_mock.Mocker() as m:
+            m.post(url, status_code=204)
+            self.assertTrue(c.add_member(self._group(), self._user()))
+            # canonical eperson href - /eperson/epersons/, not a bare /epersons/
+            self.assertEqual(m.last_request.text,
+                             f"{API}/eperson/epersons/{EPERSON_UUID}")
+
     def test_non_204_returns_false(self):
         c = make_client()
         url = f"{API}/eperson/groups/{GROUP_UUID}/epersons"

tests/test_models_clarin.py

@@ -79,6 +79,13 @@ def test_fields_and_as_dict(self):
         self.assertEqual((g.uuid, g.name, g.permanent), (GROUP_UUID, "submitters", True))
         self.assertEqual(g.as_dict()["name"], "submitters")
 
+    @pytest.mark.dtq_only
+    def test_from_none_does_not_crash(self):
+        """D7: Group(None) must yield an empty object, not a TypeError - the
+        create_submit_group / parse-failure paths can hand it None."""
+        g = Group(None)
+        self.assertIsNone(g.name)
+
 
 class TestUser(unittest.TestCase):
     """User is built by get_user_by_email / consumed by add_member."""
@@ -89,6 +96,12 @@ def test_fields_and_as_dict(self):
         self.assertTrue(u.canLogIn)
         self.assertEqual(u.as_dict()["email"], "a@b.c")
 
+    @pytest.mark.dtq_only
+    def test_from_none_does_not_crash(self):
+        """D7: User(None) must yield an empty object, not a TypeError."""
+        u = User(None)
+        self.assertIsNone(u.email)
+
 
 if __name__ == "__main__":
     unittest.main()

tests/test_repo_usage_contract.py

@@ -169,5 +169,29 @@ def test_group_search_shape(self):
             self.assertEqual(groups[0]["uuid"], "anon-uuid")
 
 
+class TestOwningCollectionReauthContract(unittest.TestCase):
+    """Mirrors src/repo/_audit.py:105-111 - get_owningCollection, then the
+    `owning_col is None and last_err.status_code == 401` reauth decision. If the
+    method ever returns a truthy empty Collection on failure, that branch dies."""
+
+    def test_success_returns_collection(self):
+        c = make_client()
+        url = f"{API}/core/items/{ITEM_UUID}/owningCollection"
+        with requests_mock.Mocker() as m:
+            m.get(url, json={"uuid": "col-1", "name": "Coll", "type": "collection"})
+            col = c.get_owningCollection(ITEM_UUID)
+            self.assertEqual(col.uuid, "col-1")
+
+    def test_401_yields_none_and_last_err_drives_reauth(self):
+        c = make_client()
+        url = f"{API}/core/items/{ITEM_UUID}/owningCollection"
+        with requests_mock.Mocker() as m:
+            m.get(url, status_code=401, json={"message": "Unauthorized"})
+            col = c.get_owningCollection(ITEM_UUID)
+            # the exact predicate _audit.py evaluates
+            self.assertTrue(col is None and c.last_err is not None
+                            and c.last_err.status_code == 401)
+
+
 if __name__ == "__main__":
     unittest.main()