From e08d6b18c5baba89259a801e0b41569c01e5ae32 Mon Sep 17 00:00:00 2001 From: Omkar Zade <103944670+omkar-ethz@users.noreply.github.com> Date: Wed, 26 Feb 2025 15:13:21 +0100 Subject: [PATCH] feat: pass the last visited route on OIDC login (#1753) ## Description This PR adds a service to track the last visited route, and passes it to /oidc backend endpoint as a query parameter, so that after successful login (i.e. when browser calls /oidc/callback), the user is redirected to the page he started from. Related backend PR: https://github.com/SciCatProject/scicat-backend-next/pull/1714 ## Motivation Previously, after OIDC login, the user would always land on "/" (or "/datasets" or some hardcoded route configured in backend). A better user experience is to return the user to the page she was on (e.g. if the user clicks login from /datasets/pid123/datafiles, she should come back to this page after logging in). This PR utilizes the backend support added in https://github.com/SciCatProject/scicat-backend-next/pull/1714 to enable this behavior. ## Fixes: * None ## Changes: - Add RotueTrackerService, to keep track of last navigated route, and start it on Application startup. - Modify redirectOIDC in login component to pass returnUrl (retrieved from above service) to the backend as query param - Don't pass redirectUrl in AuthGuard to /login component, as this is already handled by RouteTrackerService ## Tests included - [x] Included for each change/fix? - [x] Passing? (Merge will not be approved unless this is checked) ## Documentation - [ ] swagger documentation updated \[required\] - [ ] official documentation updated \[nice-to-have\] ### official documentation info If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included ## Backend version - [x] Does it require a specific version of the backend - which version of the backend is required: v4 ## Summary by Sourcery This pull request enhances the user experience by redirecting users to their last visited page after OIDC login. It introduces a RouteTrackerService to keep track of the last route and passes it to the backend as a query parameter during the OIDC authentication process. It also adds basic exception handling in /auth-callback to ensure returnUrl is a valid path. New Features: - Implements a RouteTrackerService to track the last visited route before OIDC login. - Passes the last visited route to the backend's /oidc endpoint as a query parameter. Enhancements: - After successful OIDC login, users are now redirected to their last visited page instead of a hardcoded route. Tests: - Added a unit test for the RouteTrackerService to verify that it saves the last route navigated from. --- cypress/e2e/datasets/datasets-general.cy.js | 4 ++ .../app-header/app-header.component.ts | 3 +- src/app/app-routing/auth.guard.ts | 4 +- src/app/app.module.ts | 7 +++ .../services/route-tracker.service.spec.ts | 46 +++++++++++++++++++ .../shared/services/route-tracker.service.ts | 24 ++++++++++ .../auth-callback/auth-callback.component.ts | 3 +- src/app/users/login/login.component.ts | 9 +++- 8 files changed, 93 insertions(+), 7 deletions(-) create mode 100644 src/app/shared/services/route-tracker.service.spec.ts create mode 100644 src/app/shared/services/route-tracker.service.ts diff --git a/cypress/e2e/datasets/datasets-general.cy.js b/cypress/e2e/datasets/datasets-general.cy.js index 9dcc9764c..ada1e91a5 100644 --- a/cypress/e2e/datasets/datasets-general.cy.js +++ b/cypress/e2e/datasets/datasets-general.cy.js @@ -38,6 +38,10 @@ describe("Datasets general", () => { cy.finishedLoading(); + cy.reload(); + // Without reloading, the user will land on last visited page before logout + // i.e. the dataset detail page, because the login page "remembers" the previousRoute. + cy.url().should("include", "/login"); cy.get('mat-tab-group [role="tab"]').contains("Local").click(); diff --git a/src/app/_layout/app-header/app-header.component.ts b/src/app/_layout/app-header/app-header.component.ts index 8d012c982..347524b81 100644 --- a/src/app/_layout/app-header/app-header.component.ts +++ b/src/app/_layout/app-header/app-header.component.ts @@ -46,8 +46,9 @@ export class AppHeaderComponent implements OnInit { login(): void { if (this.config.skipSciCatLoginPageEnabled) { + const returnURL = encodeURIComponent(this.router.url); for (const endpoint of this.oAuth2Endpoints) { - this.document.location.href = `${this.config.lbBaseURL}/${endpoint.authURL}`; + this.document.location.href = `${this.config.lbBaseURL}/${endpoint.authURL}?returnURL=${returnURL}`; } } else { this.router.navigateByUrl("/login"); diff --git a/src/app/app-routing/auth.guard.ts b/src/app/app-routing/auth.guard.ts index 019212b97..d3df76126 100644 --- a/src/app/app-routing/auth.guard.ts +++ b/src/app/app-routing/auth.guard.ts @@ -34,9 +34,7 @@ export class AuthGuard implements CanActivate { .usersControllerGetMyUser() .toPromise() .catch(() => { - this.router.navigate(["/login"], { - queryParams: { returnUrl: state.url }, - }); + this.router.navigate(["/login"]); return false; }) .then(() => true); diff --git a/src/app/app.module.ts b/src/app/app.module.ts index 5a6cf2996..596e13037 100644 --- a/src/app/app.module.ts +++ b/src/app/app.module.ts @@ -31,6 +31,7 @@ import { CookieService } from "ngx-cookie-service"; import { TranslateLoader, TranslateModule } from "@ngx-translate/core"; import { CustomTranslateLoader } from "shared/loaders/custom-translate.loader"; import { DATE_PIPE_DEFAULT_OPTIONS } from "@angular/common"; +import { RouteTrackerService } from "shared/services/route-tracker.service"; const appConfigInitializerFn = (appConfig: AppConfigService) => { return () => appConfig.loadAppConfig(); @@ -103,6 +104,12 @@ const apiConfigurationFn = ( multi: true, deps: [AppThemeService], }, + { + provide: APP_INITIALIZER, + useFactory: () => () => {}, + multi: true, + deps: [RouteTrackerService], + }, { provide: HTTP_INTERCEPTORS, useClass: SnackbarInterceptor, diff --git a/src/app/shared/services/route-tracker.service.spec.ts b/src/app/shared/services/route-tracker.service.spec.ts new file mode 100644 index 000000000..db490341c --- /dev/null +++ b/src/app/shared/services/route-tracker.service.spec.ts @@ -0,0 +1,46 @@ +import { TestBed } from "@angular/core/testing"; + +import { RouteTrackerService } from "./route-tracker.service"; +import { provideRouter, Router } from "@angular/router"; +import { Component } from "@angular/core"; + +@Component({}) +class DummyComponent {} + +describe("RouteTrackerService", () => { + let service: RouteTrackerService; + let router: Router; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [ + provideRouter([ + { path: "datasets/:pid", component: DummyComponent }, + { path: "login", component: DummyComponent }, + ]), + ], + }); + service = TestBed.inject(RouteTrackerService); + router = TestBed.inject(Router); + }); + + it("should be created", () => { + expect(service).toBeTruthy(); + }); + + it("should save the last route navigated from", async () => { + expect(router).toBeTruthy(); + await router.navigateByUrl("/datasets/123"); + await router.navigateByUrl("/login"); + const previousRoute = service.getPreviousRoute(); + expect(previousRoute).toBe("/datasets/123"); + }); + + it("should preserve query parameters in the previous route", async () => { + expect(router).toBeTruthy(); + await router.navigateByUrl("/datasets/123?query=value"); + await router.navigateByUrl("/login"); + const previousRoute = service.getPreviousRoute(); + expect(previousRoute).toBe("/datasets/123?query=value"); + }); +}); diff --git a/src/app/shared/services/route-tracker.service.ts b/src/app/shared/services/route-tracker.service.ts new file mode 100644 index 000000000..313f047be --- /dev/null +++ b/src/app/shared/services/route-tracker.service.ts @@ -0,0 +1,24 @@ +import { Injectable } from "@angular/core"; +import { Router, NavigationStart } from "@angular/router"; +import { filter } from "rxjs"; + +@Injectable({ + providedIn: "root", +}) +export class RouteTrackerService { + private currentRoute: string | null = null; + private previousRoute: string | null = null; + + constructor(private router: Router) { + this.router.events + .pipe(filter((event) => event instanceof NavigationStart)) + .subscribe((event) => { + this.previousRoute = this.currentRoute; + this.currentRoute = (event as NavigationStart).url; + }); + } + + getPreviousRoute(): string | null { + return this.previousRoute; + } +} diff --git a/src/app/users/auth-callback/auth-callback.component.ts b/src/app/users/auth-callback/auth-callback.component.ts index 7d2c1ad77..76014bfff 100644 --- a/src/app/users/auth-callback/auth-callback.component.ts +++ b/src/app/users/auth-callback/auth-callback.component.ts @@ -63,7 +63,8 @@ export class AuthCallbackComponent implements OnInit { ); // After the user is authenticated, we will redirect to the home page - const returnUrl = params["returnUrl"]; + // or the value of returnUrl query param + const returnUrl: string = params["returnUrl"]; this.router.navigateByUrl(returnUrl || "/"); } }); diff --git a/src/app/users/login/login.component.ts b/src/app/users/login/login.component.ts index 00a10a119..8fbcb0aab 100644 --- a/src/app/users/login/login.component.ts +++ b/src/app/users/login/login.component.ts @@ -20,6 +20,7 @@ import { AppConfigService, OAuth2Endpoint, } from "app-config.service"; +import { RouteTrackerService } from "shared/services/route-tracker.service"; interface LoginForm { username: string; @@ -74,13 +75,17 @@ export class LoginComponent implements OnInit, OnDestroy { private router: Router, private route: ActivatedRoute, private store: Store, + private routeTrackerService: RouteTrackerService, @Inject(DOCUMENT) public document: Document, ) { - this.returnUrl = this.route.snapshot.queryParams["returnUrl"] || ""; + this.returnUrl = this.routeTrackerService.getPreviousRoute() || ""; } redirectOIDC(authURL: string) { - this.document.location.href = `${this.appConfig.lbBaseURL}/${authURL}`; + const returnURL = this.returnUrl + ? encodeURIComponent(this.returnUrl) + : "/datasets"; + this.document.location.href = `${this.appConfig.lbBaseURL}/${authURL}?returnURL=${returnURL}`; } openPrivacyDialog() {