fix: harden CLARIN shared surface + dtq owningCollection reauth

dspace_rest_client/client.py

@@ -1165,12 +1165,19 @@ 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)
-            r_json = parse_json(response=r)
-            return Collection(r_json)
-        except ValueError:
-            _logger.error(f'Invalid owningCollection for UUID: {item_uuid}')
+        except requests.RequestException as e:
+            # one timeout must not abort a caller's prefetch loop; there is no
+            # response to record, so last_err stays None
+            _logger.error(f'Error fetching owningCollection [item:{item_uuid}]: {e}')
             return None
+        # Return None (not an empty, truthy Collection) with last_err set, so the
+        # caller's `owning_col is None and last_err.status_code == 401` reauth
+        # path (src/repo/_audit.py) fires. verify_response records last_err only
+        # for a non-200, so an unparseable 200 is recorded here.
+        if not self.verify_response(r, f"item:{item_uuid}", True):
+            self._last_err = r
+            return None
+        return Collection(parse_json(response=r))
 
     def create_item(self, parent, item):
         """
@@ -1278,7 +1285,11 @@ def create_user(self, user, token=None):
         params = None
         if token is not None:
             params = {'token': token}
-        return User(api_resource=parse_json(self.create_dso(url, params=params, data=data)))
+        r_json = parse_json(self.create_dso(url, params=params, data=data))
+        # User(None) / User(error body) would be truthy and uuid-less
+        if not r_json or not r_json.get('uuid'):
+            return None
+        return User(api_resource=r_json)
 
     def delete_user(self, user):
         if not isinstance(user, User):
@@ -1317,7 +1328,11 @@ def create_group(self, group):
             data = group.as_dict()
             # TODO: Validation. Note, at least here I will just allow a dict instead of the pointless cast<->cast
             # that you see for other DSO types - still figuring out the best way
-        return Group(api_resource=parse_json(self.create_dso(url, params=None, data=data)))
+        r_json = parse_json(self.create_dso(url, params=None, data=data))
+        # Group(None) / Group(error body) would be truthy and uuid-less
+        if not r_json or not r_json.get('uuid'):
+            return None
+        return Group(api_resource=r_json)
 
     def create_submit_group(self, collection):
         """
@@ -1326,7 +1341,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 and j.get('uuid'):
+                return Group(j)
         return None
 
     def add_member(self, group, eperson):
@@ -1349,7 +1369,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; DSpace resolves the uri-list by its trailing
+        # UUID only, so the former bare /epersons/{uuid} worked too
+        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
@@ -1417,8 +1439,11 @@ def get_items_from_collection(self, collection_id, page=0, size=1000):
         url = f'{self.API_ENDPOINT}/discover/search/objects?sort=dc.date.accessioned,DESC&page={page}&size={size}&scope={collection_id}&dsoType=ITEM&embed=thumbnail'
 
         items = list()
-        r = self.api_get(url)
-        r_json = parse_json(r)
+        r_json = self.fetch_resource(url)
+        # [] is the callers' end-of-collection signal: a failed page must be
+        # None (with last_err on a non-200) or their paging loop silently stops
+        if r_json is None:
+            return None
         if '_embedded' in r_json:
             if 'searchResult' in r_json['_embedded']:
                 if '_embedded' in r_json['_embedded']['searchResult']:
@@ -1436,7 +1461,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:
@@ -1449,10 +1476,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):
         """
@@ -1542,10 +1579,18 @@ def get_user_by_email(self, email):
         """
         url = f'{self.API_ENDPOINT}/eperson/epersons/search/byEmail'
         params = {'email': email}
-        try:
-            response = self.api_get(url, params=params)
-            user_data = parse_json(response)
-            return User(user_data)
-        except Exception as e:
-            _logger.error(f"Error retrieving user by email {email}: {e}")
+        # a transport error (eg. a timeout) propagates: returning None would
+        # make it indistinguishable from "no such user"
+        response = self.api_get(url, params=params)
+        # DSpace answers an unknown email with 204 No Content: a clean miss
+        if response.status_code == 204:
+            return None
+        # anything else that is not a user (404 = misconfigured endpoint,
+        # 401/403/5xx, an unparseable or uuid-less body) is an error: None, but
+        # logged and kept in last_err. The raw email is never logged.
+        user_data = parse_json(response) if response.status_code == 200 else None
+        if not user_data or not user_data.get('uuid'):
+            self._last_err = response
+            _logger.error(f"Error retrieving user by email: HTTP {response.status_code}")
             return None
+        return User(user_data)

dspace_rest_client/models.py

@@ -412,6 +412,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']
@@ -447,6 +448,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

@@ -14,11 +14,12 @@
 import unittest
 
 import pytest
+import requests
 import requests_mock
 
 import _helpers  # noqa: F401
 from _helpers import (
-    make_client, sent_params, embedded, item_json, bundle_json, raw_policy_json,
+    make_client, sent_params, embedded, no_warnings_logged, item_json, bundle_json, raw_policy_json,
     user_json, clarin_allowance_json, search_envelope,
     API, ITEM_UUID, COLLECTION_UUID, BUNDLE_UUID, EPERSON_UUID)
 from dspace_rest_client.models import Bundle, Item, Collection, User
@@ -52,6 +53,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 +95,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 +131,17 @@ 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_none_and_records_last_err(self):
+        """D6: a failed page must not look like an empty one - [] is the
+        consumers' end-of-collection signal, so an error would silently
+        truncate their paging loop. None + last_err tells the two apart."""
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(self.URL, status_code=500, text="boom")
+            self.assertIsNone(c.get_items_from_collection(COLLECTION_UUID))
+            self.assertEqual(c.last_err.status_code, 500)
+
 
 class TestGetItemByHandle(unittest.TestCase):
     """Mirrors dspace-rest-test - get_item_by_handle(handle)."""
@@ -150,6 +190,40 @@ 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_204_no_content_is_a_clean_miss(self):
+        """D4: DSpace answers an unknown email with 204 No Content. That is a
+        clean miss - None, nothing logged, no last_err - not a truthy
+        uuid-less User that slips past the consumer's `if user:` guard."""
+        c = make_client()
+        with requests_mock.Mocker() as m, no_warnings_logged(self, "dspace.client"):
+            m.get(self.URL, status_code=204)
+            self.assertIsNone(c.get_user_by_email("nobody@nowhere"))
+        self.assertIsNone(c.last_err)
+
+    @pytest.mark.dtq_only
+    def test_error_status_returns_none_logs_error_and_records_last_err(self):
+        """A 404 means a misconfigured endpoint, 401/403/5xx a real failure:
+        None, but logged at ERROR (without the raw email) and kept in last_err
+        so it is distinguishable from the clean 204 miss."""
+        for status in (404, 401, 403, 500):
+            with self.subTest(status=status):
+                c = make_client()
+                with requests_mock.Mocker() as m, self.assertLogs("dspace.client", "ERROR") as logs:
+                    m.get(self.URL, status_code=status, json={"timestamp": "now"})
+                    self.assertIsNone(c.get_user_by_email("nobody@nowhere"))
+                self.assertEqual(c.last_err.status_code, status)
+                self.assertNotIn("nobody@nowhere", " ".join(logs.output))
+
+    @pytest.mark.dtq_only
+    def test_transport_error_is_not_a_clean_miss(self):
+        """A timeout must not return the clean-miss None: it propagates."""
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(self.URL, exc=requests.exceptions.ConnectTimeout)
+            with self.assertRaises(requests.exceptions.ConnectTimeout):
+                c.get_user_by_email("nobody@nowhere")
+
 
 class TestGetClarinAllowances(unittest.TestCase):
     """Mirrors dspace-rest-test - get_clarinlruallowances[_by_bitstream_and_user]."""
@@ -197,6 +271,31 @@ 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)
+
+    def test_unparseable_200_returns_none_and_sets_last_err(self):
+        c = make_client()
+        with requests_mock.Mocker() as m:
+            m.get(self.URL, status_code=200, text="<html>proxy page</html>")
+            self.assertIsNone(c.get_owningCollection(ITEM_UUID))
+            self.assertEqual(c.last_err.status_code, 200)
+
+    def test_transport_error_returns_none_without_raising(self):
+        """One timeout must not abort the consumer's prefetch loop."""
+        c = make_client()
+        with requests_mock.Mocker() as m, self.assertLogs("dspace.client", "ERROR"):
+            m.get(self.URL, exc=requests.exceptions.ReadTimeout)
+            self.assertIsNone(c.get_owningCollection(ITEM_UUID))
+
 
 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:
+            # DSpace answers an unknown email with 204 No Content
+            m.get(f"{API}/eperson/epersons/search/byEmail", status_code=204)
+
+            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