Clarin9/Redirect to login when an identifier resolves to a restricted object (#876)

[milanmajchrak]

Problem description

Fixes A1 of dataquest-dev/dspace-customers#876 (analysed in dataquest-dev/dspace-customers#871).

Anonymous → http://dev-6.pc:8603/repository/handle/11234/1-f26b6363 stays on the handle URL and renders
"No item found for the identifier handle: 11234/1-f26b6363". No login redirect, no DiscoJuice — the user has no
way to authenticate and reach the item. The item exists and is only access-restricted (as admin the same handle
resolves; GET /server/api/pid/find?id=hdl:… returns 401 for anonymous).

It is inconsistent inside clarin9 itself: the same item opened by UUID does redirect anonymous users to
/repository/login. On 7.6.5 the handle path redirects too.

Analysis

lookupGuard collapsed every failure into one boolean:

map((response: RemoteData<DSpaceObject>) => response.hasFailed)

hasFailed is true for 401, 403, 404 and 500 alike, and lookup-by-id-routes.ts renders
ThemedObjectNotFoundComponent whenever the guard returns true — so a restricted object is indistinguishable from a
missing one. The HTTP status was available on the very same object (RemoteData.statusCode, populated by
remote-data-build.service.ts and passed through unchanged by DsoRedirectService); the guard just ignored it.

This is not an SSR-vs-CSR difference and not the auth interceptor — both are equivalent on the two branches.
What 7.6.5 has and the v9 branch does not is a CLARIN patch inside DsoRedirectService (commits 992125b697
"Login if restricted Item - accessing via ..handle/ url (#612)" and bbba91247a "…not login page but 403 (#920)")
that was never ported. Classic wiring-dropped: the guard, the routes, the component and the interceptor really are
identical — the behaviour lived in a fourth file.

Fix: branch on RemoteData.statusCode in the guard and delegate the 401/403 case to the existing shared operator
returnForbiddenUrlTreeOrLoginOnFalse(router, authService, state.url):

outcome	before	after
success	false (hard redirect already fired)	unchanged
401/403, anonymous	ObjectNotFound	UrlTree → /login, setRedirectUrl(state.url)
401/403, authenticated	ObjectNotFound	UrlTree → /403
404 / 501 / 5xx / network	ObjectNotFound	unchanged

The identifier endpoint (IdentifierRestRepository.getDSObyIdentifier) has exactly four outcomes: 302 + redirect
when resolved and visible, 401 when resolved but converter.toRest returns null (restricted), 404 for
null/IdentifierNotFoundException, and 501 SC_NOT_IMPLEMENTED for IdentifierNotResolvableException. It
never returns 422 - that status is handled by redirectOn4xx because that operator is shared with resolvers over
other endpoints. The 403 branch here is defence in depth: the endpoint itself never produces it, but the security
layer can.

Notes on the design:

• Not redirectOn4xx (what /items/:id uses). It implements its redirect as filter(… => false), i.e. it emits
  nothing on a 4xx; Angular resolves CanActivateFn with first(), so an empty completion throws EmptyError and
  produces a NavigationError. It also maps 404 → /404, which would delete the CLARIN-specific
  ObjectNotFoundComponent this route exists to render. returnForbiddenUrlTreeOrLoginOnFalse is the guard-shaped
  twin of the same logic, already used by every authorization guard in v9, and returns a UrlTree — the canonical
  Angular 20 way for a guard to redirect.
• Not a re-port of the 7.6.5 DsoRedirectService patch: it uses window.location.href (not SSR-safe), it makes a
  data service navigate as a side effect while the guard simultaneously returns true (ObjectNotFound briefly
  activates and races the navigation), and it changes the constructor of an otherwise vanilla-identical file.
• The post-login return URL is state.url (the requested identifier), not router.url — the latter is the
  pre-navigation URL and would send the user to / after login.
• No href, no window.location, no HardRedirectService: both the UrlTree and the stored state.url are
  router-space values, so the /repository base href is applied by the router.

/repository/id/<uuid> shares this guard and gets the same behaviour.

Accepted trade-off: an anonymous probe can now distinguish "exists but restricted" from "does not exist". That is
the same surface /items/:id and 7.6.5 already have, so this makes it consistent rather than larger.

Tests

lookup-guard.spec.ts — the three existing argument tests keep passing (now passing state/authService/router
positionally) plus six new behavioural specs: success → false; 404 → true with no redirect; 500 → true; 401
anonymous → login UrlTree + setRedirectUrl; 403 anonymous → login UrlTree; 403 authenticated → forbidden
UrlTree and no setRedirectUrl. 9/9 green locally, full ng lint clean.

Problems

One thing that can only be confirmed on the live stack: does the backend return 404 (not 401) for a handle that does
not exist at all? If /server/api/pid/find answered 401 for unknown handles too, a typo'd handle would now bounce
anonymous users to the login page instead of ObjectNotFound. If that turns out to be the case the fix belongs in the
backend (return 404 for unknown handles) — do not paper over it by treating 401 as 404, that would restore the reported
bug.

Manual Testing (if applicable)

• Anonymous → /repository/handle/11234/1-f26b6363 → lands on /repository/login with DiscoJuice; after logging
  in with an authorised account you end up on the item.
• Anonymous → a handle that does not exist → still "No item found for the identifier …".
• Logged in as a user without READ → /403.
• Same three checks through /repository/id/<uuid>.

Copilot review

• Requested review from Copilot

[Copilot]

Pull request overview

This PR fixes identifier-based navigation for restricted objects by distinguishing “not found” from “access restricted” in lookupGuard, ensuring anonymous users are redirected to login (and authenticated users to /403) instead of seeing the ObjectNotFound page.

Changes:

• Update lookupGuard to branch on RemoteData.statusCode and delegate 401/403 handling to returnForbiddenUrlTreeOrLoginOnFalse.
• Extend unit tests for lookupGuard to cover success, 404/500 failures, and 401/403 behavior for anonymous vs authenticated users.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/app/lookup-by-id/lookup-guard.ts	Implements 401/403 redirect behavior via returnForbiddenUrlTreeOrLoginOnFalse while keeping existing “failed lookup” behavior for other statuses.
src/app/lookup-by-id/lookup-guard.spec.ts	Adds behavioral specs validating the new guard outcomes for success, 404/500, and 401/403 (anonymous vs authenticated).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/app/lookup-by-id/lookup-guard.ts:62

• The comment mentions a 501 “identifier not resolvable” status, but elsewhere in the codebase 4xx handling explicitly accounts for 422 (see redirectOn4xx in core/shared/authorized.operators.ts). To avoid misleading future maintenance, reword this comment to mention 422 (or generalize to “non-401/403 failures”) rather than calling out 501 specifically.

      // Any other failure - 404, 501 (identifier not resolvable), 5xx, or no response at all -
      // activates the route so ObjectNotFoundComponent renders, exactly as before. On success
      // DsoRedirectService has already triggered a hard redirect to the object's own page, so the
      // route must not be activated.
      return of(response.hasFailed);

src/app/lookup-by-id/lookup-guard.spec.ts:216

• This authenticated-401 branch now relies on ServerResponseService.setStatus(401) to keep SSR responses honest, but the spec doesn’t assert that behavior. Adding the assertion here makes the SSR/caching requirement harder to regress.

    it('should return a UrlTree to the forbidden page', (done) => {
      guard(handleRoute, state).subscribe((result) => {
        expect(authService.setRedirectUrl).not.toHaveBeenCalled();
        expect(router.parseUrl).toHaveBeenCalledWith('/403');
        expect(result).toBe(forbiddenUrlTree);
        done();
      });
    });

src/app/lookup-by-id/lookup-guard.spec.ts:232

• The guard sets the SSR status code for both 401 and 403 restricted lookups; this 403+anonymous case isn’t currently asserted. Adding an expectation here will better cover the new SSR-status behavior.

    it('should store the requested url and return a UrlTree to the login page', (done) => {
      guard(handleRoute, state).subscribe((result) => {
        expect(authService.setRedirectUrl).toHaveBeenCalledWith(state.url);
        expect(router.parseUrl).toHaveBeenCalledWith('login');
        expect(result).toBe(loginUrlTree);
        done();
      });
    });

[milanmajchrak]

Follow-up on the three suppressed comments from the last Copilot pass.

Applied (2/3). The setStatus assertions for 401 + authenticated and 403 + anonymous were genuinely missing — both cases go through the same serverResponseService.setStatus(response.statusCode) call but only the other two branches asserted it. Added in d6e9ac9. Verified they are load-bearing by temporarily removing the setStatus call: TOTAL: 4 FAILED, 13 SUCCESS (all four SSR-status specs), restored, TOTAL: 17 SUCCESS.

Not applied (1/3): the 501 -> 422 rewording. 422 does not occur on this endpoint. IdentifierRestRepository.getDSObyIdentifier (dtq-dev-9-base) has exactly four outcomes:

outcome	status
resolved, visible	302 + redirect
resolved, but converter.toRest returns null (restricted)	401
null / IdentifierNotFoundException	404
IdentifierNotResolvableException	501 SC_NOT_IMPLEMENTED

redirectOn4xx handles 422 because it is shared with resolvers over other endpoints; this guard only ever sees the four above. The comment has since been shortened to // Any other failure (404, 501, 5xx) activates the route so ObjectNotFoundComponent renders, which is accurate for this endpoint.

(The 403 branch is kept as defence in depth — the endpoint itself never returns it, but the security layer can.)

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/app/lookup-by-id/lookup-guard.spec.ts:138

• The guard’s fallback branch treats any non-401/403 failure the same, but the specs only cover 404/500/undefined. Since redirectOn4xx and the PR description mention 422 as a possible outcome for unresolvable identifiers, it would be good to add a 422 spec to lock in the intended behavior (still show ObjectNotFound, no redirects).

  describe('when the lookup fails with a 404', () => {
    beforeEach(() => {
      dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Not found', 404)));

src/app/lookup-by-id/lookup-guard.ts:53

• The comment mentions a 501 status, but elsewhere in the codebase (e.g. redirectOn4xx in core/shared/authorized.operators.ts) treats identifier resolution failures as 422. This comment is likely misleading for future maintenance; consider updating it to reflect the actual status codes you expect here (e.g. 404/422/5xx/network).

      // Any other failure (404, 501, 5xx) activates the route so ObjectNotFoundComponent renders

[milanmajchrak]

Both remaining suppressed comments addressed in a052bc4.

The 422 came from my own PR description, which listed 404 / 422 / 500 / network — that was wrong and is the reason the point kept coming back. Corrected to 404 / 501 / 5xx / network, with the endpoint contract now spelled out in the description.

On the spec: the fallback describe is now parameterised over 501, 422 and 500, so the "any non-401/403 status activates the route, no redirect, no SSR status" contract is locked in for all of them — including 422, even though this endpoint cannot produce it. TOTAL: 19 SUCCESS.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.