From 2a95b6ff835b2c613d0fd1736d7a74af296e770d Mon Sep 17 00:00:00 2001 From: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Date: Thu, 20 Aug 2026 10:35:26 +0200 Subject: [PATCH] fix: harden CLARIN shared surface + dtq owningCollection reauth Characterization-tested fixes (test written to the intended behaviour first, then the method corrected) to methods the main-lineage consumers call: get_bundle_by_name / get_items_from_collection / get_resource_policy non-200 or empty body -> clean None/[] instead of a NoneType subscript crash; get_resource_policy also records last_err so a caller can tell an HTTP error from a genuine empty result. get_user_by_email 404/any non-200 -> None (+ last_err), not a truthy uuid-less User that slips past a consumer `if user:` guard. create_submit_group 201 with an empty body -> None, not Group(None). add_member post the canonical /eperson/epersons/{uuid} href; the bare /epersons/ path 404s and DSpace rejects the uri-list. Group(None) / User(None) empty object, not a TypeError. get_owningCollection (dtq-only) non-200 -> None + last_err, so ISstag's src/repo/_audit.py `owning_col is None and last_err.status_code == 401` reauth retry stops being dead code. Stacked on the test/CI PR, which supplies the _helpers builders, pytest.ini markers and the differential-contract job these tests run under. Merge that PR first. The failure-mode assertions are marked dtq_only (deselected on the main leg): main carries the same bugs, but propagating these fixes to main is deliberately out of scope here. Co-Authored-By: Claude Opus 4.8 --- dspace_rest_client/client.py | 54 +++++++++++++++++++++---- dspace_rest_client/models.py | 2 + tests/test_clarin_read.py | 61 +++++++++++++++++++++++++++++ tests/test_clarin_usage_contract.py | 25 ++++++++++++ tests/test_clarin_write.py | 25 ++++++++++++ tests/test_models_clarin.py | 13 ++++++ tests/test_repo_usage_contract.py | 24 ++++++++++++ 7 files changed, 196 insertions(+), 8 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 9cacb08..793d400 100644 --- a/dspace_rest_client/client.py +++ b/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}") diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index 4427512..dff8b78 100644 --- a/dspace_rest_client/models.py +++ b/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'] diff --git a/tests/test_clarin_read.py b/tests/test_clarin_read.py index 78663c4..d7fe79d 100644 --- a/tests/test_clarin_read.py +++ b/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() diff --git a/tests/test_clarin_usage_contract.py b/tests/test_clarin_usage_contract.py index 7dbd04e..9de1442 100644 --- a/tests/test_clarin_usage_contract.py +++ b/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 diff --git a/tests/test_clarin_write.py b/tests/test_clarin_write.py index a4d4853..0fd2761 100644 --- a/tests/test_clarin_write.py +++ b/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" diff --git a/tests/test_models_clarin.py b/tests/test_models_clarin.py index 89f3d4c..04052e0 100644 --- a/tests/test_models_clarin.py +++ b/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() diff --git a/tests/test_repo_usage_contract.py b/tests/test_repo_usage_contract.py index 55b57f6..ff26023 100644 --- a/tests/test_repo_usage_contract.py +++ b/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()