Skip to content

Commit f78da0e

Browse files
KasinhouMatus Kasakclaude
authored
DSpace9/Redirect back to originating page after DiscoJuice local login (#1472)
* 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 9dff6af, adapted to the refactored v9 component. Refs #876 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Matus Kasak <matus.kasak@dataquest.sk> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent dd506ab commit f78da0e

2 files changed

Lines changed: 108 additions & 2 deletions

File tree

src/app/shared/log-in/methods/password/log-in-password.component.spec.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,82 @@ describe('LogInPasswordComponent', () => {
158158
});
159159
});
160160

161+
// Standalone login reads the redirect target from the `redirectUrl` query param (set by aai.js).
162+
describe('standalone login redirect (redirectUrl query param)', () => {
163+
let authService: AuthServiceStub;
164+
let setRedirectUrlSpy: jasmine.Spy;
165+
let setRedirectUrlIfNotSetSpy: jasmine.Spy;
166+
167+
const setQueryParams = (queryParams: Record<string, unknown>) => {
168+
(component as any).route = { snapshot: { queryParams } };
169+
};
170+
171+
beforeEach(() => {
172+
authService = TestBed.inject(AuthService) as unknown as AuthServiceStub;
173+
setRedirectUrlSpy = spyOn(authService, 'setRedirectUrl').and.callThrough();
174+
setRedirectUrlIfNotSetSpy = spyOn(authService, 'setRedirectUrlIfNotSet').and.callThrough();
175+
// Avoid scheduling the real DiscoJuice popup timer during ngOnInit.
176+
spyOn(component as any, 'popUpDiscoJuiceLogin');
177+
178+
fixture.detectChanges();
179+
component.form.controls.email.setValue('user');
180+
component.form.controls.password.setValue('password');
181+
});
182+
183+
it('redirects back to the redirectUrl page, reduced to an app-relative path', () => {
184+
setQueryParams({ redirectUrl: 'http://dev-6.pc:8603/repository/search' });
185+
186+
component.submit();
187+
188+
expect(setRedirectUrlSpy).toHaveBeenCalledWith('/repository/search');
189+
expect(setRedirectUrlIfNotSetSpy).not.toHaveBeenCalled();
190+
});
191+
192+
it('keeps the query string of the originating page', () => {
193+
setQueryParams({ redirectUrl: 'http://dev-6.pc:8603/repository/search?query=test' });
194+
195+
component.submit();
196+
197+
expect(setRedirectUrlSpy).toHaveBeenCalledWith('/repository/search?query=test');
198+
});
199+
200+
it('prefers a nested redirectUrl so the login page is not the redirect target', () => {
201+
setQueryParams({
202+
redirectUrl: 'http://dev-6.pc:8603/repository/login?redirectUrl=http://dev-6.pc:8603/repository/items/1',
203+
});
204+
205+
component.submit();
206+
207+
expect(setRedirectUrlSpy).toHaveBeenCalledWith('/repository/items/1');
208+
});
209+
210+
it('passes through an already-relative redirectUrl unchanged', () => {
211+
setQueryParams({ redirectUrl: '/repository/search' });
212+
213+
component.submit();
214+
215+
expect(setRedirectUrlSpy).toHaveBeenCalledWith('/repository/search');
216+
});
217+
218+
it('falls back to setRedirectUrlIfNotSet("/") when no redirectUrl query param is present', () => {
219+
setQueryParams({});
220+
221+
component.submit();
222+
223+
expect(setRedirectUrlIfNotSetSpy).toHaveBeenCalledWith('/');
224+
expect(setRedirectUrlSpy).not.toHaveBeenCalled();
225+
});
226+
227+
it('falls back cleanly when redirectUrl is not a string (repeated query param)', () => {
228+
setQueryParams({ redirectUrl: ['/repository/a', '/repository/b'] });
229+
230+
component.submit();
231+
232+
expect(setRedirectUrlIfNotSetSpy).toHaveBeenCalledWith('/');
233+
expect(setRedirectUrlSpy).not.toHaveBeenCalled();
234+
});
235+
});
236+
161237
});
162238

163239
/**

src/app/shared/log-in/methods/password/log-in-password.component.ts

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,10 @@ import {
1717
UntypedFormGroup,
1818
Validators,
1919
} from '@angular/forms';
20-
import { RouterLink } from '@angular/router';
20+
import {
21+
ActivatedRoute,
22+
RouterLink,
23+
} from '@angular/router';
2124
import {
2225
select,
2326
Store,
@@ -145,6 +148,7 @@ export class LogInPasswordComponent implements OnInit, OnDestroy {
145148
@Inject('isStandalonePage') public isStandalonePage: boolean,
146149
private authService: AuthService,
147150
private hardRedirectService: HardRedirectService,
151+
private route: ActivatedRoute,
148152
private formBuilder: UntypedFormBuilder,
149153
protected store: Store<CoreState>,
150154
protected authorizationService: AuthorizationDataService,
@@ -239,7 +243,13 @@ export class LogInPasswordComponent implements OnInit, OnDestroy {
239243
if (!this.isStandalonePage) {
240244
this.authService.setRedirectUrl(this.hardRedirectService.getCurrentRoute());
241245
} else {
242-
this.authService.setRedirectUrlIfNotSet('/');
246+
// Standalone login: return to the `redirectUrl` query param set by the aai.js local-auth flow.
247+
const redirectUrl = this.getRedirectUrlFromQueryParams();
248+
if (isNotEmpty(redirectUrl)) {
249+
this.authService.setRedirectUrl(redirectUrl);
250+
} else {
251+
this.authService.setRedirectUrlIfNotSet('/');
252+
}
243253
}
244254

245255
// dispatch AuthenticationAction
@@ -249,6 +259,26 @@ export class LogInPasswordComponent implements OnInit, OnDestroy {
249259
this.form.reset();
250260
}
251261

262+
/** Post-login redirect target from the `redirectUrl` query param (aai.js), as an app-relative path or null. */
263+
private getRedirectUrlFromQueryParams(): string | null {
264+
// Query params are untyped (can be a string[]); only a non-empty string is usable here.
265+
const rawRedirectUrl = this.route.snapshot.queryParams?.redirectUrl;
266+
if (typeof rawRedirectUrl !== 'string' || isEmpty(rawRedirectUrl)) {
267+
return null;
268+
}
269+
270+
// Prefer a nested `redirectUrl` so login is never the redirect target.
271+
const nestedRedirectUrl = new URLSearchParams(rawRedirectUrl.split('?')[1] ?? '').get('redirectUrl');
272+
const redirectUrl = isNotEmpty(nestedRedirectUrl) ? nestedRedirectUrl : rawRedirectUrl;
273+
274+
return this.toRelativePath(redirectUrl);
275+
}
276+
277+
/** Reduce a possibly-absolute URL to an app-relative path by dropping the scheme+host; relative values pass through. */
278+
private toRelativePath(url: string): string {
279+
return url.replace(/^https?:\/\/[^/]+/i, '');
280+
}
281+
252282
/**
253283
* Toggle Discojuice login. Show it every time except the case when the user click
254284
* on the `local` button in the discojuice box.

0 commit comments

Comments
 (0)