# ADR: `PaperService.findByIdForCaller` is the single gateway for `PaperDetailResponse` - **Status:** Accepted - **Date:** 2026-04-12 - **Deciders:** Shodh Sanchayan engineering - **Related:** `docs/reviewer-paper-access/phase-1-discovery.md`, `docs/reviewer-paper-access/phase-2a-core-design.md` --- ## Context Phase 1 discovery confirmed that `GET /papers/{id}` leaks coauthor PII to any authenticated caller. The root cause is structural, not a missing check in a single place: 1. `PaperServiceImpl` exposed `findById(UUID)` as a public service method returning a fully-populated `PaperDetailResponse` with coauthors, author contact info, filenames, admin notes, plagiarism fields, and publication metadata. 2. `PaperServiceImpl` also had a private `toDetailResponse(Paper)` helper that `submit`, `uploadRevision`, and `updatePaper` called directly to produce their return values. That meant there were already **two** in-service paths producing a `PaperDetailResponse` — and neither one did role-based shaping. 3. Phase 2A needs to add a third caller shape (assigned reviewer → reviewer-safe detail). Naively adding a sibling `findByIdForCaller` would give us **three** paths — `findById`, the private helper, and the new gateway — and nothing would stop a future controller from reintroducing the leak by calling the older, unshaped method. The review of the Phase 2A draft made this explicit. The user's R1 change request was that *as long as `findById(UUID)` exists, the leak is one refactor away from returning*. The fix has to be structural: there must be exactly one public service method that resolves a paper id to a `PaperDetailResponse`, and it must know the caller. ## Decision **`PaperService.findByIdForCaller(UUID paperId, UUID callerId)` is the only public service method that resolves a paper id to a `PaperDetailResponse`. Every other path is removed.** Concretely: 1. `PaperService.findById(UUID)` is **removed** from the interface. Not deprecated — removed. 2. `PaperServiceImpl` gets a new implementation of `findByIdForCaller`, which: - Loads the paper. - Determines the caller's relationship to the paper (admin / author / assigned reviewer / stranger). - Returns `PaperMapper.toDetail(paper)` for admin or author. - Returns `PaperMapper.toReviewerSafeDetail(paper)` for an assigned reviewer (active statuses only: `PENDING`, `IN_PROGRESS`, `COMPLETED`). - Throws `ForbiddenException` otherwise. 3. The private `toDetailResponse(Paper)` helper in `PaperServiceImpl` is deleted. `submit`, `uploadRevision`, and `updatePaper` now return via `findByIdForCaller(paper.getId(), authorId)` — they go through the same gateway as the read endpoint. This guarantees that the shaping logic cannot diverge between read and write paths. 4. `PaperController.getById` is annotated `@PreAuthorize("isAuthenticated()")` and its body is reduced to `return paperService.findByIdForCaller(id, authenticatedUserResolver.currentUserId())`. There is no in-controller DTO construction and no direct repository access. 5. The single-gateway rule is documented in the Javadoc on `PaperService.findByIdForCaller` so that a future developer reading the interface cannot miss it, and in this ADR so the reasoning survives refactors that touch the Javadoc. `AdminPaperController` is intentionally out of scope: it uses its own admin-scoped service method for admin-only flows and is not a DTO gateway for `PaperDetailResponse` from an id. If a future change adds an admin-only detail endpoint keyed by paper id, it must route through `findByIdForCaller` as well, because ADMIN is already one of the supported caller relationships. ## Consequences ### Positive - **One place to change the rule.** If the reviewer-safe field list changes, the authorization model changes, or a new caller role is introduced, there is exactly one method to modify and exactly one set of tests to update. - **No drift between read and write paths.** Because `submit`/`uploadRevision`/`updatePaper` return via the same gateway, the shape a reviewer sees after a resubmission is guaranteed to match the shape they see on `GET /papers/{id}`. No one can forget to update one of two helpers. - **Greppable enforcement.** `findByIdForCaller` is a unique identifier. `git grep findByIdForCaller` returns every place in the codebase that produces a `PaperDetailResponse` from an id. Reviewers can audit this in seconds. - **The wire shape is stable.** Existing author and admin frontends see no change. Only the values of sensitive fields differ for reviewers, not the JSON keys. ### Negative - **Two-parameter call signature everywhere.** Callers must always supply the caller id. This is slightly more verbose than the old `findById(UUID)`, but it's also the point — the signature makes it impossible to produce a `PaperDetailResponse` without declaring who is asking. - **One extra user lookup inside the gateway** to resolve caller roles. This is a hot path only on the paper-detail endpoint, and the lookup is trivially cacheable if profiling ever demands it. - **Tests must cover all four caller relationships** (admin, author, assigned reviewer, stranger) for the read endpoint, and all three author-write paths (submit, uploadRevision, updatePaper). This is a cost paid once; the test matrix is small and finite. ### Neutral - **`ReviewServiceImpl.submitReview` is refactored in the same phase** to call `ReviewAuthorizationService.assertReviewerCanAccess(...)` instead of its inline check. This is a separate change but is motivated by the same principle — eliminate duplicate enforcement sites — so it is captured in the same phase design document. ## Enforcement This rule is enforced by a combination of code structure, documentation, and tests. It is **not** enforced by a custom static-analysis rule or an ArchUnit test, because the codebase does not currently use ArchUnit and introducing it for a single rule would be disproportionate. 1. **Interface-level:** `PaperService` has no other public method returning `PaperDetailResponse` from a paper id. `findById(UUID)` is removed, not deprecated. There is literally nothing else to call. 2. **Javadoc:** `PaperService.findByIdForCaller`'s Javadoc states explicitly: > This is the ONLY public service method that resolves a paper id to a `PaperDetailResponse`. Controllers MUST NOT call any other path (see `docs/reviewer-paper-access/adr-paper-detail-authorization.md`). 3. **ADR (this document):** The reasoning and the rule are documented here so that future refactors that touch the Javadoc still leave an authoritative reference. 4. **Tests (Phase 2B):** Integration tests cover all four caller relationships on the read endpoint, plus the three author-write paths, asserting that: - Admin sees all fields populated. - Author sees all fields populated. - Assigned reviewer sees reviewer-safe fields nulled (coauthors, submittedByName, manuscriptName, doi, volume, issue, pageStart, pageEnd, acceptedAt, publishedAt, adminNotes, plagiarismScore, plagiarismReport, citationCount, downloadCount). - Stranger gets `403 FORBIDDEN`. 5. **Code review:** New controllers or services that need a `PaperDetailResponse` from a paper id go through review, and reviewers reject any call-site that doesn't route through `findByIdForCaller`. ## Alternatives considered ### A. Keep `findById(UUID)`, add `findByIdForCaller` as a sibling Rejected. Two public methods producing the same DTO from the same id is exactly the shape we are trying to escape. The older method would stay as an attractive nuisance — every new controller is one `paperService.findById(...)` call away from reintroducing the leak, and code review is the only thing stopping it. The discovery that caused this ADR is proof that code review alone is insufficient. ### B. Use Jackson `@JsonView` to role-shape the response at serialization time Rejected. The codebase does not use `@JsonView` anywhere today. Introducing it here would mean every contributor has to learn a new pattern for one endpoint, and the view selection still has to be driven by the caller's role — which puts us back to needing the same caller-aware service method. It adds a mechanism without removing the one we actually need. ### C. Custom SpEL expression `@PreAuthorize("@paperAccess.canSee(#id)")` on the controller, keep the service naive Rejected for two reasons: 1. No custom SpEL method security expressions exist in the codebase today. Adding one here introduces a pattern nobody else follows, making the authorization model harder to reason about, not easier. 2. SpEL at the controller only gates entry. It does nothing about `PaperServiceImpl.submit`, `uploadRevision`, or `updatePaper` producing a full-detail response for a reviewer who just resubmitted. The leak would still be reachable through the write endpoints. The structural fix — one gateway — covers both. ### D. Separate DTOs: `PaperDetailAdminResponse`, `PaperDetailReviewerResponse` Rejected. The wire shape (JSON keys) would diverge, forcing the frontend to maintain two TypeScript types and two render paths for what is visually the same page. The user explicitly wanted the wire shape preserved so existing author and admin UIs keep working without changes. Nulling values in a single DTO type is the minimum-friction way to do that. ## Status notes Phase 2A is documentation only. The actual deletion of `findById(UUID)` and the addition of `findByIdForCaller` happen in Phase 3 implementation, gated on the test plan in Phase 2B. This ADR is the contract Phase 3 must implement against; it is not a retrospective.