fix: NPE (HTTP 500) in CanManageMappingsFeature for item with null owning collection#1348
fix: NPE (HTTP 500) in CanManageMappingsFeature for item with null owning collection#1348milanmajchrak wants to merge 1 commit into
Conversation
CanManageMappingsFeature.isAuthorized dereferenced item.getOwningCollection().getID() with no null check, so evaluating the canManageMappings feature for an item whose owning_collection is NULL threw a NullPointerException (HTTP 500). This surfaced on the item edit page via GET /api/authz/authorizations/search/object for users with WRITE (admins). Resolve the owning collection once and treat null as "no collection to exclude" in the filter, mirroring the defensive pattern already used by DeleteFeature. Also guard against itemService.find returning null. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIn CanManageMappingsFeature Authorization Fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes a NullPointerException (HTTP 500) in the REST authorization feature canManageMappings when evaluating permissions for an Item whose owningCollection is null (or when the Item cannot be found), preventing admin UI flows like Edit Item from crashing on orphaned items.
Changes:
- Add a null guard for
itemService.find(...)returningnullbefore running authorization checks. - Resolve
owningCollectiononce and update the stream filter to safely handlenullowning collections (avoid dereferencingitem.getOwningCollection().getID()).
| if (item == null || !authorizeService.authorizeActionBoolean(context, item, Constants.WRITE)) { | ||
| return false; | ||
| } | ||
| // An orphaned item (e.g. one whose owning collection was lost) has no owning collection. | ||
| // Guard against it so the feature evaluation does not throw a NullPointerException. |
Problem description
CanManageMappingsFeature.isAuthorized()throws aNullPointerException(HTTP 500) for any item whoseowning_collectionis NULL. In production this made an item completely unmanageable from the UI: opening Edit Item as an administrator returned 500.Observed in production (
data-dspace.zcu.cz) for item62133a8a-fbd1-474f-9b55-fa92d318aa03(handle20.500.14592/107). The page/items/<uuid>/edit/metadatacallsGET /api/authz/authorizations/search/object, which evaluates thecanManageMappingsfeature and crashes:Analysis
Exact cause
The stream filter dereferenced the item's owning collection without a null check:
When
item.getOwningCollection()isnull,.getID()is called onnull→NullPointerException. The exception only reaches users who pass the precedingWRITEauthorization check (i.e. administrators), which is why anonymous/non-write users saw the item but admins got a 500 when editing it.This is a latent upstream bug: the same unguarded
item.getOwningCollection().getID()is still present in upstream DSpace (dspace-7_x,main,dspace-8_x). It only manifests for an item that is in an inconsistent state with no owning collection.How an item reaches that state (real incident)
The affected item was created by a normal UI submission and its workflow deposit committed successfully (handle assigned,
collection2itemmapping created, Solr-indexed, workspace item deleted). However the item row ended up withowning_collection = NULLandin_archive = falsewhile thecollection2itemmapping remained — an inconsistent half-reverted state. Withowning_collection = NULL, this feature crashes. (Repairing that specific item's data — re-settingowning_collection+in_archiveand reindexing — is a separate operational fix and is not part of this PR. This PR makes the code robust so the feature never 500s regardless of how an item lost its owning collection.)Fix
nullas "there is no owning collection to exclude" in the filter — mirroring the defensive pattern already used byDeleteFeature.itemService.find()returningnull.After this change, evaluating
canManageMappingsfor an orphaned item returns a normal authorization result instead of throwing, so Edit Item no longer returns 500.Problems
None.
Manual Testing (if applicable)
Manual repro: with an item whose
owning_collection IS NULL, open Edit Item as admin → before: HTTP 500 (NPE); after: page loads, no exception.Copilot review
Summary by CodeRabbit