Skip to content

Commit d513553

Browse files
milanmajchrakclaude
milanmajchrak
and
claude
authored
Clarin9/Redirect to login when an identifier resolves to a restricted object (#876) (#1429)
* 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * Shorten the lookup guard comments Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * Cover the whole non-401/403 fallback, 501 and 422 included Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 0e9772a commit d513553

2 files changed

Lines changed: 267 additions & 12 deletions

File tree

‎src/app/lookup-by-id/lookup-guard.spec.ts‎

Lines changed: 235 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,60 @@
1-
import { of } from 'rxjs';
1+
import { TestBed } from '@angular/core/testing';
2+
import {
3+
Router,
4+
UrlTree,
5+
} from '@angular/router';
6+
import {
7+
BehaviorSubject,
8+
Observable,
9+
of,
10+
} from 'rxjs';
11+
import { take } from 'rxjs/operators';
212

13+
import { AuthService } from '../core/auth/auth.service';
14+
import { DsoRedirectService } from '../core/data/dso-redirect.service';
315
import { IdentifierType } from '../core/data/request.models';
16+
import { ServerResponseService } from '../core/services/server-response.service';
17+
import {
18+
createFailedRemoteDataObject,
19+
createSuccessfulRemoteDataObject,
20+
} from '../shared/remote-data.utils';
421
import { lookupGuard } from './lookup-guard';
522

623
describe('lookupGuard', () => {
724
let dsoService: any;
25+
let authService: any;
26+
let router: any;
27+
let serverResponseService: any;
28+
// the guard is typed as CanActivateFn, so its injected parameters can only be passed positionally through `any`
829
let guard: any;
30+
let forbiddenUrlTree: UrlTree;
31+
let loginUrlTree: UrlTree;
32+
33+
const state: any = { url: '/handle/123456789/1234' };
34+
const handleRoute: any = {
35+
params: {
36+
id: '1234',
37+
idType: '123456789',
38+
},
39+
};
940

1041
beforeEach(() => {
1142
dsoService = {
12-
findByIdAndIDType: jasmine.createSpy('findByIdAndIDType').and.returnValue(of({ hasFailed: false,
13-
hasSucceeded: true })),
43+
findByIdAndIDType: jasmine.createSpy('findByIdAndIDType')
44+
.and.returnValue(of(createSuccessfulRemoteDataObject(undefined))),
1445
};
15-
guard = lookupGuard;
46+
authService = jasmine.createSpyObj('authService', {
47+
// the real AuthService returns a store selector, which never completes
48+
isAuthenticated: new BehaviorSubject(false),
49+
setRedirectUrl: {},
50+
});
51+
forbiddenUrlTree = new UrlTree();
52+
loginUrlTree = new UrlTree();
53+
router = jasmine.createSpyObj('router', ['parseUrl']);
54+
router.parseUrl.and.callFake((url: string) => url === '/403' ? forbiddenUrlTree : loginUrlTree);
55+
serverResponseService = jasmine.createSpyObj('serverResponseService', ['setStatus']);
56+
guard = (route: any, routerState: any): Observable<boolean | UrlTree> =>
57+
(lookupGuard as any)(route, routerState, dsoService, authService, router, serverResponseService);
1658
});
1759

1860
it('should call findByIdAndIDType with handle params', () => {
@@ -22,18 +64,18 @@ describe('lookupGuard', () => {
2264
idType: '123456789',
2365
},
2466
};
25-
guard(scopedRoute as any, undefined, dsoService);
67+
guard(scopedRoute, state);
2668
expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('hdl:123456789/1234', IdentifierType.HANDLE);
2769
});
2870

29-
it('should call findByIdAndIDType with handle params', () => {
71+
it('should call findByIdAndIDType with encoded handle params', () => {
3072
const scopedRoute = {
3173
params: {
3274
id: '123456789%2F1234',
3375
idType: 'handle',
3476
},
3577
};
36-
guard(scopedRoute as any, undefined, dsoService);
78+
guard(scopedRoute, state);
3779
expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('hdl:123456789%2F1234', IdentifierType.HANDLE);
3880
});
3981

@@ -44,8 +86,193 @@ describe('lookupGuard', () => {
4486
idType: 'uuid',
4587
},
4688
};
47-
guard(scopedRoute as any, undefined, dsoService);
89+
guard(scopedRoute, state);
4890
expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('34cfed7c-f597-49ef-9cbe-ea351f0023c2', IdentifierType.UUID);
4991
});
5092

93+
it('should resolve its dependencies from the injector when they are not passed in', (done) => {
94+
TestBed.configureTestingModule({
95+
providers: [
96+
{ provide: DsoRedirectService, useValue: dsoService },
97+
{ provide: AuthService, useValue: authService },
98+
{ provide: Router, useValue: router },
99+
{ provide: ServerResponseService, useValue: serverResponseService },
100+
],
101+
});
102+
103+
const result = TestBed.runInInjectionContext(() => lookupGuard(handleRoute, state)) as Observable<boolean | UrlTree>;
104+
105+
expect(dsoService.findByIdAndIDType).toHaveBeenCalledWith('hdl:123456789/1234', IdentifierType.HANDLE);
106+
result.subscribe((activate) => {
107+
expect(activate).toBeFalse();
108+
done();
109+
});
110+
});
111+
112+
describe('when the object was found', () => {
113+
it('should return false so the ObjectNotFound page is not shown', (done) => {
114+
guard(handleRoute, state).subscribe((result) => {
115+
expect(result).toBeFalse();
116+
expect(serverResponseService.setStatus).not.toHaveBeenCalled();
117+
done();
118+
});
119+
});
120+
});
121+
122+
describe('when the lookup fails with a 404', () => {
123+
beforeEach(() => {
124+
dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Not found', 404)));
125+
});
126+
127+
it('should return true so the ObjectNotFound page is shown', (done) => {
128+
guard(handleRoute, state).subscribe((result) => {
129+
expect(result).toBeTrue();
130+
expect(authService.setRedirectUrl).not.toHaveBeenCalled();
131+
expect(router.parseUrl).not.toHaveBeenCalled();
132+
expect(serverResponseService.setStatus).not.toHaveBeenCalled();
133+
done();
134+
});
135+
});
136+
});
137+
138+
// 501 is what the identifier endpoint answers for an unresolvable identifier type; 422 never
139+
// reaches this guard, but the fallback must treat every non-401/403 status the same way
140+
[501, 422, 500].forEach((statusCode: number) => {
141+
describe(`when the lookup fails with a ${statusCode}`, () => {
142+
beforeEach(() => {
143+
dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Failed', statusCode)));
144+
});
145+
146+
it('should return true so the ObjectNotFound page is shown', (done) => {
147+
guard(handleRoute, state).subscribe((result) => {
148+
expect(result).toBeTrue();
149+
expect(authService.setRedirectUrl).not.toHaveBeenCalled();
150+
expect(router.parseUrl).not.toHaveBeenCalled();
151+
expect(serverResponseService.setStatus).not.toHaveBeenCalled();
152+
done();
153+
});
154+
});
155+
});
156+
});
157+
158+
describe('when the lookup fails without a status code', () => {
159+
beforeEach(() => {
160+
dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Network error', undefined)));
161+
});
162+
163+
it('should return true so the ObjectNotFound page is shown', (done) => {
164+
guard(handleRoute, state).subscribe((result) => {
165+
expect(result).toBeTrue();
166+
expect(router.parseUrl).not.toHaveBeenCalled();
167+
done();
168+
});
169+
});
170+
});
171+
172+
describe('when the lookup fails with a 401 and the user is not authenticated', () => {
173+
beforeEach(() => {
174+
dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Unauthorized', 401)));
175+
authService.isAuthenticated.and.returnValue(new BehaviorSubject(false));
176+
});
177+
178+
it('should store the requested url and return a UrlTree to the login page', (done) => {
179+
guard(handleRoute, state).subscribe((result) => {
180+
expect(authService.setRedirectUrl).toHaveBeenCalledWith(state.url);
181+
expect(router.parseUrl).toHaveBeenCalledWith('login');
182+
expect(result).toBe(loginUrlTree);
183+
done();
184+
});
185+
});
186+
187+
it('should set the server response status so the page is not cached as a 200', (done) => {
188+
guard(handleRoute, state).subscribe(() => {
189+
expect(serverResponseService.setStatus).toHaveBeenCalledWith(401);
190+
done();
191+
});
192+
});
193+
194+
it('should emit exactly once and complete even though isAuthenticated() never completes', (done) => {
195+
let emissions = 0;
196+
guard(handleRoute, state).pipe(take(2)).subscribe({
197+
next: (result) => {
198+
emissions++;
199+
expect(result).toBe(loginUrlTree);
200+
},
201+
complete: () => {
202+
expect(emissions).toBe(1);
203+
done();
204+
},
205+
});
206+
});
207+
});
208+
209+
describe('when the lookup fails with a 401 and the user is authenticated', () => {
210+
beforeEach(() => {
211+
dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Unauthorized', 401)));
212+
authService.isAuthenticated.and.returnValue(new BehaviorSubject(true));
213+
});
214+
215+
it('should return a UrlTree to the forbidden page', (done) => {
216+
guard(handleRoute, state).subscribe((result) => {
217+
expect(authService.setRedirectUrl).not.toHaveBeenCalled();
218+
expect(router.parseUrl).toHaveBeenCalledWith('/403');
219+
expect(result).toBe(forbiddenUrlTree);
220+
done();
221+
});
222+
});
223+
224+
it('should set the server response status so the page is not cached as a 200', (done) => {
225+
guard(handleRoute, state).subscribe(() => {
226+
expect(serverResponseService.setStatus).toHaveBeenCalledWith(401);
227+
done();
228+
});
229+
});
230+
});
231+
232+
describe('when the lookup fails with a 403 and the user is not authenticated', () => {
233+
beforeEach(() => {
234+
dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Forbidden', 403)));
235+
authService.isAuthenticated.and.returnValue(new BehaviorSubject(false));
236+
});
237+
238+
it('should store the requested url and return a UrlTree to the login page', (done) => {
239+
guard(handleRoute, state).subscribe((result) => {
240+
expect(authService.setRedirectUrl).toHaveBeenCalledWith(state.url);
241+
expect(router.parseUrl).toHaveBeenCalledWith('login');
242+
expect(result).toBe(loginUrlTree);
243+
done();
244+
});
245+
});
246+
247+
it('should set the server response status so the page is not cached as a 200', (done) => {
248+
guard(handleRoute, state).subscribe(() => {
249+
expect(serverResponseService.setStatus).toHaveBeenCalledWith(403);
250+
done();
251+
});
252+
});
253+
});
254+
255+
describe('when the lookup fails with a 403 and the user is authenticated', () => {
256+
beforeEach(() => {
257+
dsoService.findByIdAndIDType.and.returnValue(of(createFailedRemoteDataObject('Forbidden', 403)));
258+
authService.isAuthenticated.and.returnValue(new BehaviorSubject(true));
259+
});
260+
261+
it('should return a UrlTree to the forbidden page and not touch the redirect url', (done) => {
262+
guard(handleRoute, state).subscribe((result) => {
263+
expect(authService.setRedirectUrl).not.toHaveBeenCalled();
264+
expect(router.parseUrl).toHaveBeenCalledWith('/403');
265+
expect(result).toBe(forbiddenUrlTree);
266+
done();
267+
});
268+
});
269+
270+
it('should set the server response status so the page is not cached as a 200', (done) => {
271+
guard(handleRoute, state).subscribe(() => {
272+
expect(serverResponseService.setStatus).toHaveBeenCalledWith(403);
273+
done();
274+
});
275+
});
276+
});
277+
51278
});

‎src/app/lookup-by-id/lookup-guard.ts‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,25 @@ import { inject } from '@angular/core';
22
import {
33
ActivatedRouteSnapshot,
44
CanActivateFn,
5+
Router,
56
RouterStateSnapshot,
7+
UrlTree,
68
} from '@angular/router';
7-
import { Observable } from 'rxjs';
8-
import { map } from 'rxjs/operators';
9+
import {
10+
Observable,
11+
of,
12+
} from 'rxjs';
13+
import {
14+
switchMap,
15+
take,
16+
} from 'rxjs/operators';
917

18+
import { AuthService } from '../core/auth/auth.service';
1019
import { DsoRedirectService } from '../core/data/dso-redirect.service';
1120
import { RemoteData } from '../core/data/remote-data';
1221
import { IdentifierType } from '../core/data/request.models';
22+
import { ServerResponseService } from '../core/services/server-response.service';
23+
import { returnForbiddenUrlTreeOrLoginOnFalse } from '../core/shared/authorized.operators';
1324
import { DSpaceObject } from '../core/shared/dspace-object.model';
1425

1526
interface LookupParams {
@@ -21,10 +32,27 @@ export const lookupGuard: CanActivateFn = (
2132
route: ActivatedRouteSnapshot,
2233
state: RouterStateSnapshot,
2334
dsoService: DsoRedirectService = inject(DsoRedirectService),
24-
): Observable<boolean> => {
35+
authService: AuthService = inject(AuthService),
36+
router: Router = inject(Router),
37+
serverResponseService: ServerResponseService = inject(ServerResponseService),
38+
): Observable<boolean | UrlTree> => {
2539
const params = getLookupParams(route);
2640
return dsoService.findByIdAndIDType(params.id, params.type).pipe(
27-
map((response: RemoteData<DSpaceObject>) => response.hasFailed),
41+
switchMap((response: RemoteData<DSpaceObject>) => {
42+
// A restricted object, which REST reports as 401/403 rather than 404
43+
if (response.hasFailed && (response.statusCode === 401 || response.statusCode === 403)) {
44+
// or SSR would cache the login page as HTTP 200 under the identifier's URL. No-op in the browser.
45+
serverResponseService.setStatus(response.statusCode);
46+
// `false` = not authorized: login page for anonymous users, /403 for authenticated ones, the
47+
// same split /items/:id makes. take(1) because isAuthenticated() never completes.
48+
return of(false).pipe(
49+
returnForbiddenUrlTreeOrLoginOnFalse(router, authService, state.url),
50+
take(1),
51+
);
52+
}
53+
// Any other failure (404, 501, 5xx) activates the route so ObjectNotFoundComponent renders
54+
return of(response.hasFailed);
55+
}),
2856
);
2957
};
3058

0 commit comments

Comments
 (0)