From 33ba55aea327637f85d98c9c6ecac38ade3f960c Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Thu, 20 Aug 2026 10:26:46 +0200 Subject: [PATCH 1/3] Clarin9/Redirect back to originating page after DiscoJuice local login (#876) On the standalone login page the post-login redirect ignored the `redirectUrl` query param that the DiscoJuice local-auth flow (src/aai/aai.js) appends as the absolute page URL, so signing in from e.g. the search page always landed on the home page. `LogInPasswordComponent.submit()` now reads that query param, reduces it to an app-relative path (the same format `HardRedirectService.getCurrentRoute()` produces, which `reloadGuard` already consumes), and uses it as the redirect target; it prefers a nested `redirectUrl` so login is never the target, and keeps the previous `setRedirectUrlIfNotSet('/')` fallback when no param is present. Fixes the dspace-ui-tests LINDAT-013 scenario (loginPage.spec.ts "login from search page should redirect back to search page"). Mirrors the dtq-dev fix 9dff6af543, adapted to the refactored v9 component. Refs #876 Co-Authored-By: Claude Opus 4.8 --- .../log-in-password.component.spec.ts | 61 +++++++++++++++++++ .../password/log-in-password.component.ts | 54 +++++++++++++++- 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts b/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts index fe35ccebc77..1c7074b775a 100644 --- a/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts +++ b/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts @@ -158,6 +158,67 @@ describe('LogInPasswordComponent', () => { }); }); + // On the standalone login page (isStandalonePage === true) the redirect target comes from the + // `redirectUrl` query param that the DiscoJuice local-auth flow (aai.js) appends as the absolute + // page URL. Reduce it to an app-relative path so the user returns to where they logged in from. + describe('standalone login redirect (redirectUrl query param)', () => { + let authService: AuthServiceStub; + let setRedirectUrlSpy: jasmine.Spy; + let setRedirectUrlIfNotSetSpy: jasmine.Spy; + + const setQueryParams = (queryParams: Record) => { + (component as any).route = { snapshot: { queryParams } }; + }; + + beforeEach(() => { + authService = TestBed.inject(AuthService) as unknown as AuthServiceStub; + setRedirectUrlSpy = spyOn(authService, 'setRedirectUrl').and.callThrough(); + setRedirectUrlIfNotSetSpy = spyOn(authService, 'setRedirectUrlIfNotSet').and.callThrough(); + // Avoid scheduling the real DiscoJuice popup timer during ngOnInit. + spyOn(component as any, 'popUpDiscoJuiceLogin'); + + fixture.detectChanges(); + component.form.controls.email.setValue('user'); + component.form.controls.password.setValue('password'); + }); + + it('redirects back to the redirectUrl page, reduced to an app-relative path', () => { + setQueryParams({ redirectUrl: 'http://dev-6.pc:8603/repository/search' }); + + component.submit(); + + expect(setRedirectUrlSpy).toHaveBeenCalledWith('/repository/search'); + expect(setRedirectUrlIfNotSetSpy).not.toHaveBeenCalled(); + }); + + it('keeps the query string of the originating page', () => { + setQueryParams({ redirectUrl: 'http://dev-6.pc:8603/repository/search?query=test' }); + + component.submit(); + + expect(setRedirectUrlSpy).toHaveBeenCalledWith('/repository/search?query=test'); + }); + + it('prefers a nested redirectUrl so the login page is not the redirect target', () => { + setQueryParams({ + redirectUrl: 'http://dev-6.pc:8603/repository/login?redirectUrl=http://dev-6.pc:8603/repository/items/1', + }); + + component.submit(); + + expect(setRedirectUrlSpy).toHaveBeenCalledWith('/repository/items/1'); + }); + + it('falls back to setRedirectUrlIfNotSet("/") when no redirectUrl query param is present', () => { + setQueryParams({}); + + component.submit(); + + expect(setRedirectUrlIfNotSetSpy).toHaveBeenCalledWith('/'); + expect(setRedirectUrlSpy).not.toHaveBeenCalled(); + }); + }); + }); /** diff --git a/src/app/shared/log-in/methods/password/log-in-password.component.ts b/src/app/shared/log-in/methods/password/log-in-password.component.ts index 0b18adbac07..f7c2a9a42dd 100644 --- a/src/app/shared/log-in/methods/password/log-in-password.component.ts +++ b/src/app/shared/log-in/methods/password/log-in-password.component.ts @@ -17,7 +17,10 @@ import { UntypedFormGroup, Validators, } from '@angular/forms'; -import { RouterLink } from '@angular/router'; +import { + ActivatedRoute, + RouterLink, +} from '@angular/router'; import { select, Store, @@ -145,6 +148,7 @@ export class LogInPasswordComponent implements OnInit, OnDestroy { @Inject('isStandalonePage') public isStandalonePage: boolean, private authService: AuthService, private hardRedirectService: HardRedirectService, + private route: ActivatedRoute, private formBuilder: UntypedFormBuilder, protected store: Store, protected authorizationService: AuthorizationDataService, @@ -239,7 +243,15 @@ export class LogInPasswordComponent implements OnInit, OnDestroy { if (!this.isStandalonePage) { this.authService.setRedirectUrl(this.hardRedirectService.getCurrentRoute()); } else { - this.authService.setRedirectUrlIfNotSet('/'); + // On the standalone login page, honor the `redirectUrl` query param set by the DiscoJuice + // local-auth flow (src/aai/aai.js) so the user returns to the page they logged in from + // (e.g. the search page) instead of being sent to the home page. + const redirectUrl = this.getRedirectUrlFromQueryParams(); + if (isNotEmpty(redirectUrl)) { + this.authService.setRedirectUrl(redirectUrl); + } else { + this.authService.setRedirectUrlIfNotSet('/'); + } } // dispatch AuthenticationAction @@ -249,6 +261,44 @@ export class LogInPasswordComponent implements OnInit, OnDestroy { this.form.reset(); } + /** + * Resolve the post-login redirect target from the `redirectUrl` query param on the standalone + * login page. + * + * The DiscoJuice local-auth flow (src/aai/aai.js) sends the user to + * `/login?redirectUrl=` so that after signing in they return to the page the + * login was initiated from. The value is lost from the auth store while passing through + * DiscoJuice, so it has to be read back from the URL here. + * + * @returns an app-relative path (same format as {@link HardRedirectService#getCurrentRoute}), or + * `null` when there is no usable redirect target. + */ + private getRedirectUrlFromQueryParams(): string { + const rawRedirectUrl: string = this.route.snapshot.queryParams?.redirectUrl; + if (isEmpty(rawRedirectUrl)) { + return null; + } + + // When the value itself carries a nested `redirectUrl` (login initiated while already on the + // login page), prefer that inner target so we don't redirect back to the login page. + const nestedRedirectUrl = new URLSearchParams(rawRedirectUrl.split('?')[1] ?? '').get('redirectUrl'); + const redirectUrl = isNotEmpty(nestedRedirectUrl) ? nestedRedirectUrl : rawRedirectUrl; + + return this.toRelativePath(redirectUrl); + } + + /** + * Reduce a possibly-absolute URL to an app-relative path (`/path?query#hash`) by dropping the + * origin. Values that are already relative are returned unchanged. + */ + private toRelativePath(url: string): string { + if (/^https?:\/\//i.test(url)) { + const parsed = new URL(url); + return parsed.pathname + parsed.search + parsed.hash; + } + return url; + } + /** * Toggle Discojuice login. Show it every time except the case when the user click * on the `local` button in the discojuice box. From d81640d18eefe6452d0fe9f03997594bc6d7e1dd Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Thu, 20 Aug 2026 15:33:37 +0200 Subject: [PATCH 2/3] Clarin9/Trim inline comments in log-in-password redirect fix Shorten the multiline comments/JSDoc added for the redirect-from-search fix to one-liners; the fuller rationale now lives in the PR description. Co-Authored-By: Claude Opus 4.8 --- .../log-in-password.component.spec.ts | 4 +-- .../password/log-in-password.component.ts | 25 +++---------------- 2 files changed, 5 insertions(+), 24 deletions(-) diff --git a/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts b/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts index 1c7074b775a..999585ca9f7 100644 --- a/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts +++ b/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts @@ -158,9 +158,7 @@ describe('LogInPasswordComponent', () => { }); }); - // On the standalone login page (isStandalonePage === true) the redirect target comes from the - // `redirectUrl` query param that the DiscoJuice local-auth flow (aai.js) appends as the absolute - // page URL. Reduce it to an app-relative path so the user returns to where they logged in from. + // Standalone login reads the redirect target from the `redirectUrl` query param (set by aai.js). describe('standalone login redirect (redirectUrl query param)', () => { let authService: AuthServiceStub; let setRedirectUrlSpy: jasmine.Spy; diff --git a/src/app/shared/log-in/methods/password/log-in-password.component.ts b/src/app/shared/log-in/methods/password/log-in-password.component.ts index f7c2a9a42dd..0ee9a208014 100644 --- a/src/app/shared/log-in/methods/password/log-in-password.component.ts +++ b/src/app/shared/log-in/methods/password/log-in-password.component.ts @@ -243,9 +243,7 @@ export class LogInPasswordComponent implements OnInit, OnDestroy { if (!this.isStandalonePage) { this.authService.setRedirectUrl(this.hardRedirectService.getCurrentRoute()); } else { - // On the standalone login page, honor the `redirectUrl` query param set by the DiscoJuice - // local-auth flow (src/aai/aai.js) so the user returns to the page they logged in from - // (e.g. the search page) instead of being sent to the home page. + // Standalone login: return to the `redirectUrl` query param set by the aai.js local-auth flow. const redirectUrl = this.getRedirectUrlFromQueryParams(); if (isNotEmpty(redirectUrl)) { this.authService.setRedirectUrl(redirectUrl); @@ -261,36 +259,21 @@ export class LogInPasswordComponent implements OnInit, OnDestroy { this.form.reset(); } - /** - * Resolve the post-login redirect target from the `redirectUrl` query param on the standalone - * login page. - * - * The DiscoJuice local-auth flow (src/aai/aai.js) sends the user to - * `/login?redirectUrl=` so that after signing in they return to the page the - * login was initiated from. The value is lost from the auth store while passing through - * DiscoJuice, so it has to be read back from the URL here. - * - * @returns an app-relative path (same format as {@link HardRedirectService#getCurrentRoute}), or - * `null` when there is no usable redirect target. - */ + /** Post-login redirect target from the `redirectUrl` query param (aai.js), as an app-relative path or null. */ private getRedirectUrlFromQueryParams(): string { const rawRedirectUrl: string = this.route.snapshot.queryParams?.redirectUrl; if (isEmpty(rawRedirectUrl)) { return null; } - // When the value itself carries a nested `redirectUrl` (login initiated while already on the - // login page), prefer that inner target so we don't redirect back to the login page. + // Prefer a nested `redirectUrl` so login is never the redirect target. const nestedRedirectUrl = new URLSearchParams(rawRedirectUrl.split('?')[1] ?? '').get('redirectUrl'); const redirectUrl = isNotEmpty(nestedRedirectUrl) ? nestedRedirectUrl : rawRedirectUrl; return this.toRelativePath(redirectUrl); } - /** - * Reduce a possibly-absolute URL to an app-relative path (`/path?query#hash`) by dropping the - * origin. Values that are already relative are returned unchanged. - */ + /** Reduce a possibly-absolute URL to an app-relative path (drops origin); relative values pass through. */ private toRelativePath(url: string): string { if (/^https?:\/\//i.test(url)) { const parsed = new URL(url); From fe761ab69f96d4c8e9e0e0f55c2e712c8b6d428b Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Fri, 21 Aug 2026 09:12:02 +0200 Subject: [PATCH 3/3] Clarin9/Address Copilot review: harden redirectUrl parsing - getRedirectUrlFromQueryParams() now returns `string | null` and guards against non-string (e.g. repeated `string[]`) query params, so login submission falls back to the default redirect instead of throwing. - Normalize the redirect with a string `.replace(/^https?:\/\/[^/]+/i, '')` (v7-style, can't throw) instead of `new URL(...)`. - Add unit tests for an already-relative redirectUrl and a non-string value. Co-Authored-By: Claude Opus 4.8 --- .../log-in-password.component.spec.ts | 19 ++++++++++++++++++- .../password/log-in-password.component.ts | 15 ++++++--------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts b/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts index 999585ca9f7..6df4b577f2c 100644 --- a/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts +++ b/src/app/shared/log-in/methods/password/log-in-password.component.spec.ts @@ -164,7 +164,7 @@ describe('LogInPasswordComponent', () => { let setRedirectUrlSpy: jasmine.Spy; let setRedirectUrlIfNotSetSpy: jasmine.Spy; - const setQueryParams = (queryParams: Record) => { + const setQueryParams = (queryParams: Record) => { (component as any).route = { snapshot: { queryParams } }; }; @@ -207,6 +207,14 @@ describe('LogInPasswordComponent', () => { expect(setRedirectUrlSpy).toHaveBeenCalledWith('/repository/items/1'); }); + it('passes through an already-relative redirectUrl unchanged', () => { + setQueryParams({ redirectUrl: '/repository/search' }); + + component.submit(); + + expect(setRedirectUrlSpy).toHaveBeenCalledWith('/repository/search'); + }); + it('falls back to setRedirectUrlIfNotSet("/") when no redirectUrl query param is present', () => { setQueryParams({}); @@ -215,6 +223,15 @@ describe('LogInPasswordComponent', () => { expect(setRedirectUrlIfNotSetSpy).toHaveBeenCalledWith('/'); expect(setRedirectUrlSpy).not.toHaveBeenCalled(); }); + + it('falls back cleanly when redirectUrl is not a string (repeated query param)', () => { + setQueryParams({ redirectUrl: ['/repository/a', '/repository/b'] }); + + component.submit(); + + expect(setRedirectUrlIfNotSetSpy).toHaveBeenCalledWith('/'); + expect(setRedirectUrlSpy).not.toHaveBeenCalled(); + }); }); }); diff --git a/src/app/shared/log-in/methods/password/log-in-password.component.ts b/src/app/shared/log-in/methods/password/log-in-password.component.ts index 0ee9a208014..1177bf6301a 100644 --- a/src/app/shared/log-in/methods/password/log-in-password.component.ts +++ b/src/app/shared/log-in/methods/password/log-in-password.component.ts @@ -260,9 +260,10 @@ export class LogInPasswordComponent implements OnInit, OnDestroy { } /** Post-login redirect target from the `redirectUrl` query param (aai.js), as an app-relative path or null. */ - private getRedirectUrlFromQueryParams(): string { - const rawRedirectUrl: string = this.route.snapshot.queryParams?.redirectUrl; - if (isEmpty(rawRedirectUrl)) { + private getRedirectUrlFromQueryParams(): string | null { + // Query params are untyped (can be a string[]); only a non-empty string is usable here. + const rawRedirectUrl = this.route.snapshot.queryParams?.redirectUrl; + if (typeof rawRedirectUrl !== 'string' || isEmpty(rawRedirectUrl)) { return null; } @@ -273,13 +274,9 @@ export class LogInPasswordComponent implements OnInit, OnDestroy { return this.toRelativePath(redirectUrl); } - /** Reduce a possibly-absolute URL to an app-relative path (drops origin); relative values pass through. */ + /** Reduce a possibly-absolute URL to an app-relative path by dropping the scheme+host; relative values pass through. */ private toRelativePath(url: string): string { - if (/^https?:\/\//i.test(url)) { - const parsed = new URL(url); - return parsed.pathname + parsed.search + parsed.hash; - } - return url; + return url.replace(/^https?:\/\/[^/]+/i, ''); } /**