Featured Items API can now retrieve images for creator - #12381
Featured Items API can now retrieve images for creator#12381stevenwinship wants to merge 1 commit into
Conversation
This comment has been minimized.
This comment has been minimized.
6f1095d to
17d5660
Compare
This comment has been minimized.
This comment has been minimized.
17d5660 to
c7865f3
Compare
This comment has been minimized.
This comment has been minimized.
c7865f3 to
45f1a22
Compare
This comment has been minimized.
This comment has been minimized.
45f1a22 to
5a224a9
Compare
This comment has been minimized.
This comment has been minimized.
5a224a9 to
7269a84
Compare
This comment has been minimized.
This comment has been minimized.
7269a84 to
49ee2af
Compare
This comment has been minimized.
This comment has been minimized.
49ee2af to
5dafcc2
Compare
This comment has been minimized.
This comment has been minimized.
5dafcc2 to
925c3a8
Compare
This comment has been minimized.
This comment has been minimized.
1464223 to
cebec54
Compare
This comment has been minimized.
This comment has been minimized.
cebec54 to
52f8f11
Compare
This comment has been minimized.
This comment has been minimized.
52f8f11 to
2e15bcd
Compare
This comment has been minimized.
This comment has been minimized.
2e15bcd to
49b5bbd
Compare
This comment has been minimized.
This comment has been minimized.
49b5bbd to
a290572
Compare
This comment has been minimized.
This comment has been minimized.
a290572 to
727260f
Compare
This comment has been minimized.
This comment has been minimized.
727260f to
fdd9788
Compare
This comment has been minimized.
This comment has been minimized.
fdd9788 to
a620402
Compare
This comment has been minimized.
This comment has been minimized.
a620402 to
fb805ec
Compare
This comment has been minimized.
This comment has been minimized.
fb805ec to
d988c97
Compare
This comment has been minimized.
This comment has been minimized.
d988c97 to
d45be96
Compare
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
rtreacy
left a comment
There was a problem hiding this comment.
Claude
PR #12381 Review: Featured Items API can now retrieve images for creator
What it does
Fixes #11537: the creator of an unpublished dataverse couldn't view/download their own featured-item images, because GetDataverseFeaturedItemCommand.getRequiredPermissions() unconditionally required Permission.ViewUnpublishedDataverse whenever the dataverse wasn't released — with no allowance for the dataverse's own creator. The fix adds an explicit carve-out: if the requesting user equals dataverseFeaturedItem.getDataverse().getCreator(), no permission is required, same as the published case.
Correctness
- getRequest().getUser().equals(...) is safe against the two null-ish edge cases that matter: an anonymous/guest user (GuestUser.equals falls back to reference equality against an AuthenticatedUser → false, correctly not treated as creator) and a dataverse with no recorded creator (getCreator() returns null; AuthenticatedUser.equals(null) returns false via the instanceof guard, so the code correctly falls back to requiring ViewUnpublishedDataverse rather than NPEing or accidentally granting access).
- AuthenticatedUser.equals() compares by database ID (DvObject.java/AuthenticatedUser.java:439), not reference identity, so comparing the request's live user object against the JPA-loaded creator relationship works correctly across different object instances.
- The fix is narrowly scoped — it changes behavior only for the literal creator of the dataverse; every other user (including dataverse admins/curators without the creator flag) still goes through the unchanged ViewUnpublishedDataverse check, so there's no broadened attack surface beyond the intended case.
- @AuthRequired on the Access.getDataverseFeatureItemImage endpoint guarantees an authenticated user reaches this command, so there's no unauthenticated-guest path to worry about here.
Test coverage
- The new assertions in testListFeaturedItems exercise the actual bug: the dataverse in that test is never published (unlike the sibling testUpdateFeaturedItems, which explicitly publishes), so isReleased() is false and the fixed branch is genuinely exercised — this isn't a test that would've passed before the fix, which is exactly what you want for a regression test.
- Confirms both sides: creator's token → 200, unrelated random user's token → 204 (matching this endpoint's existing convention of returning null/204 rather than 403 on permission failure — pre-existing behavior in Access.java, unaffected by this PR).
- No test for a non-creator user who does have ViewUnpublishedDataverse (e.g., a curator role-assignee) still succeeding — that path is unchanged by this diff and was presumably already covered elsewhere, so it's a minor gap rather than a real risk.
Style (minor, non-blocking)
- Collections.singletonMap("",Collections.emptySet()) is missing a space after the comma — trivial whitespace nit.
- The double-negative condition (!isReleased() && !user.equals(creator)) reads a little awkwardly; something like a single boolean (isCreator) combined with isReleased() || isCreator in a ternary would be marginally more readable, but this is a matter of taste, not a defect.
Verdict
Small, well-targeted bug fix with a test that actually proves the fix works and would have failed before it. No correctness, security, or regression concerns. Good to merge as-is; the style nits aren't worth blocking on.
What this PR does / why we need it: Creator of a featured item of a non published dataverse could not see the images.
Which issue(s) this PR closes:#11537
Special notes for your reviewer:
Suggestions on how to test this: See DataversesIT testListFeaturedItems.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?: included
Additional documentation: