From eb35085720eb874ce33a5cc2e22425fb9691c63e Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 11:34:25 +0200 Subject: [PATCH 1/6] Clarin9/Redirect to login when an identifier resolves to a restricted object (#876) `lookupGuard` collapsed every failed lookup into a single boolean, so a 401/403 from `/server/api/pid/find` was indistinguishable from a genuine 404: an anonymous user opening a restricted item by handle got "No item found for the identifier ..." and had no way to authenticate. Branch on `RemoteData.statusCode` and delegate the 401/403 case to the shared `returnForbiddenUrlTreeOrLoginOnFalse` operator, which returns a UrlTree to the login page for anonymous users (remembering `state.url` so they come back to the identifier after logging in) and to the forbidden page for authenticated ones. Any other failure - 404 included - still activates the route so ObjectNotFoundComponent keeps rendering. This makes `/handle/...` and `/id/...` behave like `/items/:id`, which already gets this through `itemPageResolver` -> `redirectOn4xx`. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/lookup-by-id/lookup-guard.spec.ts | 123 ++++++++++++++++++++-- src/app/lookup-by-id/lookup-guard.ts | 33 +++++- 2 files changed, 146 insertions(+), 10 deletions(-) diff --git a/src/app/lookup-by-id/lookup-guard.spec.ts b/src/app/lookup-by-id/lookup-guard.spec.ts index b3a5ad6ccdc..30adbf1344c 100644 --- a/src/app/lookup-by-id/lookup-guard.spec.ts +++ b/src/app/lookup-by-id/lookup-guard.spec.ts @@ -1,17 +1,42 @@ +import { UrlTree } from '@angular/router'; import { of } from 'rxjs'; import { IdentifierType } from '../core/data/request.models'; +import { + createFailedRemoteDataObject, + createSuccessfulRemoteDataObject, +} from '../shared/remote-data.utils'; import { lookupGuard } from './lookup-guard'; describe('lookupGuard', () => { let dsoService: any; + let authService: any; + let router: any; let guard: any; + let forbiddenUrlTree: UrlTree; + let loginUrlTree: UrlTree; + + const state: any = { url: '/handle/123456789/1234' }; + const handleRoute: any = { + params: { + id: '1234', + idType: '123456789', + }, + }; beforeEach(() => { dsoService = { - findByIdAndIDType: jasmine.createSpy('findByIdAndIDType').and.returnValue(of({ hasFailed: false, - hasSucceeded: true })), + findByIdAndIDType: jasmine.createSpy('findByIdAndIDType') + .and.returnValue(of(createSuccessfulRemoteDataObject(undefined))), }; + authService = jasmine.createSpyObj('authService', { + isAuthenticated: of(false), + setRedirectUrl: {}, + }); + forbiddenUrlTree = new UrlTree(); + loginUrlTree = new UrlTree(); + router = jasmine.createSpyObj('router', ['parseUrl']); + router.parseUrl.and.callFake((url: string) => url === '/403' ? forbiddenUrlTree : loginUrlTree); guard = lookupGuard; }); @@ -22,18 +47,18 @@ describe('lookupGuard', () => { idType: '123456789', }, }; - guard(scopedRoute as any, undefined, dsoService); + guard(scopedRoute as any, state, dsoService, authService, router); expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('hdl:123456789/1234', IdentifierType.HANDLE); }); - it('should call findByIdAndIDType with handle params', () => { + it('should call findByIdAndIDType with encoded handle params', () => { const scopedRoute = { params: { id: '123456789%2F1234', idType: 'handle', }, }; - guard(scopedRoute as any, undefined, dsoService); + guard(scopedRoute as any, state, dsoService, authService, router); expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('hdl:123456789%2F1234', IdentifierType.HANDLE); }); @@ -44,8 +69,94 @@ describe('lookupGuard', () => { idType: 'uuid', }, }; - guard(scopedRoute as any, undefined, dsoService); + guard(scopedRoute as any, state, dsoService, authService, router); expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('34cfed7c-f597-49ef-9cbe-ea351f0023c2', IdentifierType.UUID); }); + describe('when the object was found', () => { + it('should return false so the ObjectNotFound page is not shown', (done) => { + guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + expect(result).toBeFalse(); + done(); + }); + }); + }); + + describe('when the lookup fails with a 404', () => { + beforeEach(() => { + dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Not found', 404))); + }); + + it('should return true so the ObjectNotFound page is shown', (done) => { + guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + expect(result).toBeTrue(); + expect(authService.setRedirectUrl).not.toHaveBeenCalled(); + expect(router.parseUrl).not.toHaveBeenCalled(); + done(); + }); + }); + }); + + describe('when the lookup fails with a 500', () => { + beforeEach(() => { + dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Server error', 500))); + }); + + it('should return true so the ObjectNotFound page is shown', (done) => { + guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + expect(result).toBeTrue(); + expect(router.parseUrl).not.toHaveBeenCalled(); + done(); + }); + }); + }); + + describe('when the lookup fails with a 401 and the user is not authenticated', () => { + beforeEach(() => { + dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Unauthorized', 401))); + authService.isAuthenticated.and.returnValue(of(false)); + }); + + it('should store the requested url and return a UrlTree to the login page', (done) => { + guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + expect(authService.setRedirectUrl).toHaveBeenCalledWith(state.url); + expect(router.parseUrl).toHaveBeenCalledWith('login'); + expect(result).toBe(loginUrlTree); + done(); + }); + }); + }); + + describe('when the lookup fails with a 403 and the user is not authenticated', () => { + beforeEach(() => { + dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Forbidden', 403))); + authService.isAuthenticated.and.returnValue(of(false)); + }); + + it('should store the requested url and return a UrlTree to the login page', (done) => { + guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + expect(authService.setRedirectUrl).toHaveBeenCalledWith(state.url); + expect(router.parseUrl).toHaveBeenCalledWith('login'); + expect(result).toBe(loginUrlTree); + done(); + }); + }); + }); + + describe('when the lookup fails with a 403 and the user is authenticated', () => { + beforeEach(() => { + dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Forbidden', 403))); + authService.isAuthenticated.and.returnValue(of(true)); + }); + + it('should return a UrlTree to the forbidden page and not touch the redirect url', (done) => { + guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + expect(authService.setRedirectUrl).not.toHaveBeenCalled(); + expect(router.parseUrl).toHaveBeenCalledWith('/403'); + expect(result).toBe(forbiddenUrlTree); + done(); + }); + }); + }); + }); diff --git a/src/app/lookup-by-id/lookup-guard.ts b/src/app/lookup-by-id/lookup-guard.ts index 25813d28ff5..39c99e2b4d8 100644 --- a/src/app/lookup-by-id/lookup-guard.ts +++ b/src/app/lookup-by-id/lookup-guard.ts @@ -2,14 +2,21 @@ import { inject } from '@angular/core'; import { ActivatedRouteSnapshot, CanActivateFn, + Router, RouterStateSnapshot, + UrlTree, } from '@angular/router'; -import { Observable } from 'rxjs'; -import { map } from 'rxjs/operators'; +import { + Observable, + of, +} from 'rxjs'; +import { switchMap } from 'rxjs/operators'; +import { AuthService } from '../core/auth/auth.service'; import { DsoRedirectService } from '../core/data/dso-redirect.service'; import { RemoteData } from '../core/data/remote-data'; import { IdentifierType } from '../core/data/request.models'; +import { returnForbiddenUrlTreeOrLoginOnFalse } from '../core/shared/authorized.operators'; import { DSpaceObject } from '../core/shared/dspace-object.model'; interface LookupParams { @@ -21,10 +28,28 @@ export const lookupGuard: CanActivateFn = ( route: ActivatedRouteSnapshot, state: RouterStateSnapshot, dsoService: DsoRedirectService = inject(DsoRedirectService), -): Observable => { + authService: AuthService = inject(AuthService), + router: Router = inject(Router), +): Observable => { const params = getLookupParams(route); return dsoService.findByIdAndIDType(params.id, params.type).pipe( - map((response: RemoteData) => response.hasFailed), + switchMap((response: RemoteData) => { + if (response.hasFailed && (response.statusCode === 401 || response.statusCode === 403)) { + // The identifier resolves to an object the current user isn't allowed to see, which the + // REST API reports as 401/403 rather than 404. Emitting `false` means "not authorized": + // the shared operator turns that into a UrlTree to the login page for an anonymous user + // (remembering state.url so they come back to this identifier afterwards), or to the + // forbidden page for an authenticated one. This mirrors what /items/:id already does via + // itemPageResolver -> redirectOn4xx, so both routes behave identically. + return of(false).pipe( + returnForbiddenUrlTreeOrLoginOnFalse(router, authService, state.url), + ); + } + // Activate the route - and therefore render ObjectNotFoundComponent - only when the lookup + // genuinely failed to find anything (404). 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); + }), ); }; From 392414a8111b5b1bf3bc62de2bd58e3484a26d57 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 12:21:48 +0200 Subject: [PATCH 2/6] Review feedback: keep the SSR response honest and harden the guard spec - Set the 401/403 status on the server response before redirecting. Without it SSR would serve the login page with HTTP 200 under the identifier's own URL, and server.ts would store that in the bot cache for a day. No-op in the browser. - Add take(1): returnForbiddenUrlTreeOrLoginOnFalse combines with isAuthenticated(), a store selector that never completes, so the guard's observable relied on the router's own first() to terminate. - Reword the fallback comment - it is the catch-all for 404, 501, 5xx and status-less failures, not 404 only (Copilot) - and drop the claim that the authenticated branch behaves identically to /items/:id (a UrlTree rewrites the address bar, redirectOn4xx uses skipLocationChange). - Spec: use a non-completing isAuthenticated stub, assert the guard emits exactly once and completes, cover a failure with no status code and 401-while- authenticated, assert the server response status, and add a TestBed.runInInjectionContext case so the injected defaults are exercised. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/lookup-by-id/lookup-guard.spec.ts | 127 +++++++++++++++++++--- src/app/lookup-by-id/lookup-guard.ts | 23 +++- 2 files changed, 127 insertions(+), 23 deletions(-) diff --git a/src/app/lookup-by-id/lookup-guard.spec.ts b/src/app/lookup-by-id/lookup-guard.spec.ts index 30adbf1344c..afd8adc92b9 100644 --- a/src/app/lookup-by-id/lookup-guard.spec.ts +++ b/src/app/lookup-by-id/lookup-guard.spec.ts @@ -1,7 +1,19 @@ -import { UrlTree } from '@angular/router'; -import { of } from 'rxjs'; - +import { TestBed } from '@angular/core/testing'; +import { + Router, + UrlTree, +} from '@angular/router'; +import { + BehaviorSubject, + Observable, + of, +} from 'rxjs'; +import { take } from 'rxjs/operators'; + +import { AuthService } from '../core/auth/auth.service'; +import { DsoRedirectService } from '../core/data/dso-redirect.service'; import { IdentifierType } from '../core/data/request.models'; +import { ServerResponseService } from '../core/services/server-response.service'; import { createFailedRemoteDataObject, createSuccessfulRemoteDataObject, @@ -12,6 +24,8 @@ describe('lookupGuard', () => { let dsoService: any; let authService: any; let router: any; + let serverResponseService: any; + // the guard is typed as CanActivateFn, so its injected parameters can only be passed positionally through `any` let guard: any; let forbiddenUrlTree: UrlTree; let loginUrlTree: UrlTree; @@ -30,14 +44,17 @@ describe('lookupGuard', () => { .and.returnValue(of(createSuccessfulRemoteDataObject(undefined))), }; authService = jasmine.createSpyObj('authService', { - isAuthenticated: of(false), + // the real AuthService returns a store selector, which never completes + isAuthenticated: new BehaviorSubject(false), setRedirectUrl: {}, }); forbiddenUrlTree = new UrlTree(); loginUrlTree = new UrlTree(); router = jasmine.createSpyObj('router', ['parseUrl']); router.parseUrl.and.callFake((url: string) => url === '/403' ? forbiddenUrlTree : loginUrlTree); - guard = lookupGuard; + serverResponseService = jasmine.createSpyObj('serverResponseService', ['setStatus']); + guard = (route: any, routerState: any): Observable => + (lookupGuard as any)(route, routerState, dsoService, authService, router, serverResponseService); }); it('should call findByIdAndIDType with handle params', () => { @@ -47,7 +64,7 @@ describe('lookupGuard', () => { idType: '123456789', }, }; - guard(scopedRoute as any, state, dsoService, authService, router); + guard(scopedRoute, state); expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('hdl:123456789/1234', IdentifierType.HANDLE); }); @@ -58,7 +75,7 @@ describe('lookupGuard', () => { idType: 'handle', }, }; - guard(scopedRoute as any, state, dsoService, authService, router); + guard(scopedRoute, state); expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('hdl:123456789%2F1234', IdentifierType.HANDLE); }); @@ -69,14 +86,31 @@ describe('lookupGuard', () => { idType: 'uuid', }, }; - guard(scopedRoute as any, state, dsoService, authService, router); + guard(scopedRoute, state); expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('34cfed7c-f597-49ef-9cbe-ea351f0023c2', IdentifierType.UUID); }); + it('should resolve its dependencies from the injector when they are not passed in', () => { + TestBed.configureTestingModule({ + providers: [ + { provide: DsoRedirectService, useValue: dsoService }, + { provide: AuthService, useValue: authService }, + { provide: Router, useValue: router }, + { provide: ServerResponseService, useValue: serverResponseService }, + ], + }); + + const result = TestBed.runInInjectionContext(() => lookupGuard(handleRoute, state)) as Observable; + + expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('hdl:123456789/1234', IdentifierType.HANDLE); + result.subscribe((activate) => expect(activate).toBeFalse()); + }); + describe('when the object was found', () => { it('should return false so the ObjectNotFound page is not shown', (done) => { - guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + guard(handleRoute, state).subscribe((result) => { expect(result).toBeFalse(); + expect(serverResponseService.setStatus).not.toHaveBeenCalled(); done(); }); }); @@ -88,10 +122,11 @@ describe('lookupGuard', () => { }); it('should return true so the ObjectNotFound page is shown', (done) => { - guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + guard(handleRoute, state).subscribe((result) => { expect(result).toBeTrue(); expect(authService.setRedirectUrl).not.toHaveBeenCalled(); expect(router.parseUrl).not.toHaveBeenCalled(); + expect(serverResponseService.setStatus).not.toHaveBeenCalled(); done(); }); }); @@ -103,7 +138,21 @@ describe('lookupGuard', () => { }); it('should return true so the ObjectNotFound page is shown', (done) => { - guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + guard(handleRoute, state).subscribe((result) => { + expect(result).toBeTrue(); + expect(router.parseUrl).not.toHaveBeenCalled(); + done(); + }); + }); + }); + + describe('when the lookup fails without a status code', () => { + beforeEach(() => { + dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Network error', undefined))); + }); + + it('should return true so the ObjectNotFound page is shown', (done) => { + guard(handleRoute, state).subscribe((result) => { expect(result).toBeTrue(); expect(router.parseUrl).not.toHaveBeenCalled(); done(); @@ -114,27 +163,64 @@ describe('lookupGuard', () => { describe('when the lookup fails with a 401 and the user is not authenticated', () => { beforeEach(() => { dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Unauthorized', 401))); - authService.isAuthenticated.and.returnValue(of(false)); + authService.isAuthenticated.and.returnValue(new BehaviorSubject(false)); }); it('should store the requested url and return a UrlTree to the login page', (done) => { - guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + guard(handleRoute, state).subscribe((result) => { expect(authService.setRedirectUrl).toHaveBeenCalledWith(state.url); expect(router.parseUrl).toHaveBeenCalledWith('login'); expect(result).toBe(loginUrlTree); done(); }); }); + + it('should set the server response status so the page is not cached as a 200', (done) => { + guard(handleRoute, state).subscribe(() => { + expect(serverResponseService.setStatus).toHaveBeenCalledWith(401); + done(); + }); + }); + + it('should emit exactly once and complete even though isAuthenticated() never completes', (done) => { + let emissions = 0; + guard(handleRoute, state).pipe(take(2)).subscribe({ + next: (result) => { + emissions++; + expect(result).toBe(loginUrlTree); + }, + complete: () => { + expect(emissions).toBe(1); + done(); + }, + }); + }); + }); + + describe('when the lookup fails with a 401 and the user is authenticated', () => { + beforeEach(() => { + dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Unauthorized', 401))); + authService.isAuthenticated.and.returnValue(new BehaviorSubject(true)); + }); + + 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(); + }); + }); }); describe('when the lookup fails with a 403 and the user is not authenticated', () => { beforeEach(() => { dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Forbidden', 403))); - authService.isAuthenticated.and.returnValue(of(false)); + authService.isAuthenticated.and.returnValue(new BehaviorSubject(false)); }); it('should store the requested url and return a UrlTree to the login page', (done) => { - guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + guard(handleRoute, state).subscribe((result) => { expect(authService.setRedirectUrl).toHaveBeenCalledWith(state.url); expect(router.parseUrl).toHaveBeenCalledWith('login'); expect(result).toBe(loginUrlTree); @@ -146,17 +232,24 @@ describe('lookupGuard', () => { describe('when the lookup fails with a 403 and the user is authenticated', () => { beforeEach(() => { dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Forbidden', 403))); - authService.isAuthenticated.and.returnValue(of(true)); + authService.isAuthenticated.and.returnValue(new BehaviorSubject(true)); }); it('should return a UrlTree to the forbidden page and not touch the redirect url', (done) => { - guard(handleRoute, state, dsoService, authService, router).subscribe((result) => { + guard(handleRoute, state).subscribe((result) => { expect(authService.setRedirectUrl).not.toHaveBeenCalled(); expect(router.parseUrl).toHaveBeenCalledWith('/403'); expect(result).toBe(forbiddenUrlTree); done(); }); }); + + it('should set the server response status so the page is not cached as a 200', (done) => { + guard(handleRoute, state).subscribe(() => { + expect(serverResponseService.setStatus).toHaveBeenCalledWith(403); + done(); + }); + }); }); }); diff --git a/src/app/lookup-by-id/lookup-guard.ts b/src/app/lookup-by-id/lookup-guard.ts index 39c99e2b4d8..5fa238dc820 100644 --- a/src/app/lookup-by-id/lookup-guard.ts +++ b/src/app/lookup-by-id/lookup-guard.ts @@ -10,12 +10,16 @@ import { Observable, of, } from 'rxjs'; -import { switchMap } from 'rxjs/operators'; +import { + switchMap, + take, +} from 'rxjs/operators'; import { AuthService } from '../core/auth/auth.service'; import { DsoRedirectService } from '../core/data/dso-redirect.service'; import { RemoteData } from '../core/data/remote-data'; import { IdentifierType } from '../core/data/request.models'; +import { ServerResponseService } from '../core/services/server-response.service'; import { returnForbiddenUrlTreeOrLoginOnFalse } from '../core/shared/authorized.operators'; import { DSpaceObject } from '../core/shared/dspace-object.model'; @@ -30,24 +34,31 @@ export const lookupGuard: CanActivateFn = ( dsoService: DsoRedirectService = inject(DsoRedirectService), authService: AuthService = inject(AuthService), router: Router = inject(Router), + serverResponseService: ServerResponseService = inject(ServerResponseService), ): Observable => { const params = getLookupParams(route); return dsoService.findByIdAndIDType(params.id, params.type).pipe( switchMap((response: RemoteData) => { if (response.hasFailed && (response.statusCode === 401 || response.statusCode === 403)) { + // Keep the server-rendered response honest: without this the login page would be sent with + // HTTP 200 under the identifier's own URL and stored in the SSR bot cache. No-op in the browser. + serverResponseService.setStatus(response.statusCode); // The identifier resolves to an object the current user isn't allowed to see, which the // REST API reports as 401/403 rather than 404. Emitting `false` means "not authorized": // the shared operator turns that into a UrlTree to the login page for an anonymous user // (remembering state.url so they come back to this identifier afterwards), or to the - // forbidden page for an authenticated one. This mirrors what /items/:id already does via - // itemPageResolver -> redirectOn4xx, so both routes behave identically. + // forbidden page for an authenticated one - the same split /items/:id makes through + // itemPageResolver -> redirectOn4xx. `take(1)` is needed because the operator combines with + // `authService.isAuthenticated()`, a store selector that never completes. return of(false).pipe( returnForbiddenUrlTreeOrLoginOnFalse(router, authService, state.url), + take(1), ); } - // Activate the route - and therefore render ObjectNotFoundComponent - only when the lookup - // genuinely failed to find anything (404). On success DsoRedirectService has already - // triggered a hard redirect to the object's own page, so the route must not be activated. + // 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); }), ); From 73d37900c666577fc61eed9098207a496c14f241 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Wed, 5 Aug 2026 14:11:12 +0200 Subject: [PATCH 3/6] Make the injection-context spec fail if the guard stops emitting It asserted inside a subscribe callback with no done(), so it would have passed silently had the observable never emitted. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/lookup-by-id/lookup-guard.spec.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/app/lookup-by-id/lookup-guard.spec.ts b/src/app/lookup-by-id/lookup-guard.spec.ts index afd8adc92b9..17a54ce6827 100644 --- a/src/app/lookup-by-id/lookup-guard.spec.ts +++ b/src/app/lookup-by-id/lookup-guard.spec.ts @@ -90,7 +90,7 @@ describe('lookupGuard', () => { expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('34cfed7c-f597-49ef-9cbe-ea351f0023c2', IdentifierType.UUID); }); - it('should resolve its dependencies from the injector when they are not passed in', () => { + it('should resolve its dependencies from the injector when they are not passed in', (done) => { TestBed.configureTestingModule({ providers: [ { provide: DsoRedirectService, useValue: dsoService }, @@ -103,7 +103,10 @@ describe('lookupGuard', () => { const result = TestBed.runInInjectionContext(() => lookupGuard(handleRoute, state)) as Observable; expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('hdl:123456789/1234', IdentifierType.HANDLE); - result.subscribe((activate) => expect(activate).toBeFalse()); + result.subscribe((activate) => { + expect(activate).toBeFalse(); + done(); + }); }); describe('when the object was found', () => { From 393e4517c0b087d50fa41d3290f91d7641aaa72a Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 6 Aug 2026 09:06:06 +0200 Subject: [PATCH 4/6] Shorten the lookup guard comments Co-Authored-By: Claude Opus 5 (1M context) --- src/app/lookup-by-id/lookup-guard.ts | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/app/lookup-by-id/lookup-guard.ts b/src/app/lookup-by-id/lookup-guard.ts index 5fa238dc820..6ea42567540 100644 --- a/src/app/lookup-by-id/lookup-guard.ts +++ b/src/app/lookup-by-id/lookup-guard.ts @@ -39,26 +39,18 @@ export const lookupGuard: CanActivateFn = ( const params = getLookupParams(route); return dsoService.findByIdAndIDType(params.id, params.type).pipe( switchMap((response: RemoteData) => { + // A restricted object, which REST reports as 401/403 rather than 404 if (response.hasFailed && (response.statusCode === 401 || response.statusCode === 403)) { - // Keep the server-rendered response honest: without this the login page would be sent with - // HTTP 200 under the identifier's own URL and stored in the SSR bot cache. No-op in the browser. + // or SSR would cache the login page as HTTP 200 under the identifier's URL. No-op in the browser. serverResponseService.setStatus(response.statusCode); - // The identifier resolves to an object the current user isn't allowed to see, which the - // REST API reports as 401/403 rather than 404. Emitting `false` means "not authorized": - // the shared operator turns that into a UrlTree to the login page for an anonymous user - // (remembering state.url so they come back to this identifier afterwards), or to the - // forbidden page for an authenticated one - the same split /items/:id makes through - // itemPageResolver -> redirectOn4xx. `take(1)` is needed because the operator combines with - // `authService.isAuthenticated()`, a store selector that never completes. + // `false` = not authorized: login page for anonymous users, /403 for authenticated ones, the + // same split /items/:id makes. take(1) because isAuthenticated() never completes. return of(false).pipe( returnForbiddenUrlTreeOrLoginOnFalse(router, authService, state.url), take(1), ); } - // 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. + // Any other failure (404, 501, 5xx) activates the route so ObjectNotFoundComponent renders return of(response.hasFailed); }), ); From d6e9ac98ed8cf0ba32485846af665c04e71cb02a Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 6 Aug 2026 09:58:47 +0200 Subject: [PATCH 5/6] Assert the SSR status for the two remaining restricted-lookup cases 401+authenticated and 403+anonymous went through the same setStatus call but were not covered. Verified: all four assertions fail without it. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/lookup-by-id/lookup-guard.spec.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/app/lookup-by-id/lookup-guard.spec.ts b/src/app/lookup-by-id/lookup-guard.spec.ts index 17a54ce6827..701dc38ab3f 100644 --- a/src/app/lookup-by-id/lookup-guard.spec.ts +++ b/src/app/lookup-by-id/lookup-guard.spec.ts @@ -214,6 +214,13 @@ describe('lookupGuard', () => { done(); }); }); + + it('should set the server response status so the page is not cached as a 200', (done) => { + guard(handleRoute, state).subscribe(() => { + expect(serverResponseService.setStatus).toHaveBeenCalledWith(401); + done(); + }); + }); }); describe('when the lookup fails with a 403 and the user is not authenticated', () => { @@ -230,6 +237,13 @@ describe('lookupGuard', () => { done(); }); }); + + it('should set the server response status so the page is not cached as a 200', (done) => { + guard(handleRoute, state).subscribe(() => { + expect(serverResponseService.setStatus).toHaveBeenCalledWith(403); + done(); + }); + }); }); describe('when the lookup fails with a 403 and the user is authenticated', () => { From a052bc4130bd8a3702b33acf3952d976cf90421f Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 6 Aug 2026 10:48:52 +0200 Subject: [PATCH 6/6] Cover the whole non-401/403 fallback, 501 and 422 included Co-Authored-By: Claude Opus 5 (1M context) --- src/app/lookup-by-id/lookup-guard.spec.ts | 24 ++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/app/lookup-by-id/lookup-guard.spec.ts b/src/app/lookup-by-id/lookup-guard.spec.ts index 701dc38ab3f..d4b6d5509cb 100644 --- a/src/app/lookup-by-id/lookup-guard.spec.ts +++ b/src/app/lookup-by-id/lookup-guard.spec.ts @@ -135,16 +135,22 @@ describe('lookupGuard', () => { }); }); - describe('when the lookup fails with a 500', () => { - beforeEach(() => { - dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Server error', 500))); - }); + // 501 is what the identifier endpoint answers for an unresolvable identifier type; 422 never + // reaches this guard, but the fallback must treat every non-401/403 status the same way + [501, 422, 500].forEach((statusCode: number) => { + describe(`when the lookup fails with a ${statusCode}`, () => { + beforeEach(() => { + dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Failed', statusCode))); + }); - it('should return true so the ObjectNotFound page is shown', (done) => { - guard(handleRoute, state).subscribe((result) => { - expect(result).toBeTrue(); - expect(router.parseUrl).not.toHaveBeenCalled(); - done(); + it('should return true so the ObjectNotFound page is shown', (done) => { + guard(handleRoute, state).subscribe((result) => { + expect(result).toBeTrue(); + expect(authService.setRedirectUrl).not.toHaveBeenCalled(); + expect(router.parseUrl).not.toHaveBeenCalled(); + expect(serverResponseService.setStatus).not.toHaveBeenCalled(); + done(); + }); }); }); });