Skip to content

Commit

Permalink
NAS-132648 / 25.04 / query param handling for auth tokens (#11080)
Browse files Browse the repository at this point in the history
* NAS-132648: adds query param handling to signin.store and auth.service. forward query params on redirect in auth-guard.service

* NAS-132648: patch signin.store tests for now

* NAS-132648: flag in websocket-handler.service to determine if a socket connection to the NAS has opened at least once. adds queryToken to signin.store state.

* NAS-132648: remove router debugging

* NAS-132648: add method for handling query token login. remove queryToken from signin.store state.

* NAS-132648: update resetUi method to preserve query params during redirect to /signin

* NAS-132648: unit test coverage for AuthService.setQueryToken

* NAS-132648: test coverage for AuthGuardService
  • Loading branch information
aervin authored Dec 5, 2024
1 parent 373844d commit 357cc9f
Show file tree
Hide file tree
Showing 9 changed files with 142 additions and 24 deletions.
8 changes: 6 additions & 2 deletions src/app/core/guards/websocket-connection.guard.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { Injectable } from '@angular/core';
import { Inject, Injectable } from '@angular/core';
import { MatDialog } from '@angular/material/dialog';
import { Router } from '@angular/router';
import { UntilDestroy, untilDestroyed } from '@ngneat/until-destroy';
import { TranslateService } from '@ngx-translate/core';
import { WINDOW } from 'app/helpers/window.helper';
import { DialogService } from 'app/modules/dialog/dialog.service';
import { WebSocketHandlerService } from 'app/services/websocket/websocket-handler.service';

Expand All @@ -16,6 +17,7 @@ export class WebSocketConnectionGuard {
private matDialog: MatDialog,
private dialogService: DialogService,
private translate: TranslateService,
@Inject(WINDOW) private window: Window,
) {
this.wsManager.isClosed$.pipe(untilDestroyed(this)).subscribe((isClosed) => {
if (isClosed) {
Expand All @@ -37,7 +39,9 @@ export class WebSocketConnectionGuard {
private resetUi(): void {
this.closeAllDialogs();
if (!this.wsManager.isSystemShuttingDown) {
this.router.navigate(['/signin']);
// manually preserve query params
const params = new URLSearchParams(this.window.location.search);
this.router.navigate(['/signin'], { queryParams: Object.fromEntries(params) });
}
}

Expand Down
1 change: 1 addition & 0 deletions src/app/core/testing/utils/empty-auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ export class EmptyAuthService {
logout = getMissingInjectionErrorFactory(AuthService.name);
refreshUser = getMissingInjectionErrorFactory(AuthService.name);
loginWithToken = getMissingInjectionErrorFactory(AuthService.name);
setQueryToken = getMissingInjectionErrorFactory(AuthService.name);
}
18 changes: 15 additions & 3 deletions src/app/pages/signin/store/signin.store.spec.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { Router } from '@angular/router';
import { ActivatedRoute, Router } from '@angular/router';
import { createServiceFactory, SpectatorService } from '@ngneat/spectator';
import { mockProvider } from '@ngneat/spectator/jest';
import { BehaviorSubject, firstValueFrom, of } from 'rxjs';
import { MockApiService } from 'app/core/testing/classes/mock-api.service';
import { getTestScheduler } from 'app/core/testing/utils/get-test-scheduler.utils';
import { mockCall, mockApi } from 'app/core/testing/utils/mock-api.utils';
import { mockApi, mockCall } from 'app/core/testing/utils/mock-api.utils';
import { FailoverDisabledReason } from 'app/enums/failover-disabled-reason.enum';
import { FailoverStatus } from 'app/enums/failover-status.enum';
import { LoginResult } from 'app/enums/login-result.enum';
Expand Down Expand Up @@ -66,6 +66,7 @@ describe('SigninStore', () => {
},
},
},
mockProvider(ActivatedRoute, { snapshot: { queryParamMap: { get: jest.fn(() => null) } } }),
],
});

Expand All @@ -82,6 +83,7 @@ describe('SigninStore', () => {
});
jest.spyOn(authService, 'loginWithToken').mockReturnValue(of(LoginResult.Success));
jest.spyOn(authService, 'clearAuthToken').mockReturnValue(null);
jest.spyOn(authService, 'setQueryToken').mockReturnValue(undefined);
});

describe('selectors', () => {
Expand Down Expand Up @@ -187,14 +189,24 @@ describe('SigninStore', () => {
expect(spectator.inject(Router).navigateByUrl).toHaveBeenCalledWith('/dashboard');
});

it('should not call "loginWithToken" if token is not within timeline and clear auth token', () => {
it('should not call "loginWithToken" if token is not within timeline and clear auth token and queryToken is null', () => {
isTokenWithinTimeline$.next(false);
spectator.service.init();

expect(authService.clearAuthToken).toHaveBeenCalled();
expect(authService.loginWithToken).not.toHaveBeenCalled();
expect(spectator.inject(Router).navigateByUrl).not.toHaveBeenCalled();
});

it('should call "loginWithToken" if queryToken is not null', () => {
isTokenWithinTimeline$.next(false);
const token = 'token';
const activatedRoute = spectator.inject(ActivatedRoute);
jest.spyOn(activatedRoute.snapshot.queryParamMap, 'get').mockImplementationOnce(() => token);
spectator.service.init();
expect(authService.setQueryToken).toHaveBeenCalledWith(token);
expect(authService.loginWithToken).toHaveBeenCalled();
});
});

describe('init - failover subscriptions', () => {
Expand Down
45 changes: 35 additions & 10 deletions src/app/pages/signin/store/signin.store.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Inject, Injectable } from '@angular/core';
import { MatSnackBar } from '@angular/material/snack-bar';
import { Router } from '@angular/router';
import { ActivatedRoute, Router } from '@angular/router';
import { UntilDestroy, untilDestroyed } from '@ngneat/until-destroy';
import { ComponentStore } from '@ngrx/component-store';
import { Actions, ofType } from '@ngrx/effects';
Expand Down Expand Up @@ -68,6 +68,14 @@ export class SigninStore extends ComponentStore<SigninState> {
private failoverStatusSubscription: Subscription;
private disabledReasonsSubscription: Subscription;

private handleLoginResult = (loginResult: LoginResult): void => {
if (loginResult !== LoginResult.Success) {
this.authService.clearAuthToken();
} else {
this.handleSuccessfulLogin();
}
};

constructor(
private api: ApiService,
private translate: TranslateService,
Expand All @@ -81,6 +89,7 @@ export class SigninStore extends ComponentStore<SigninState> {
private authService: AuthService,
private updateService: UpdateService,
private actions$: Actions,
private activatedRoute: ActivatedRoute,
@Inject(WINDOW) private window: Window,
) {
super(initialState);
Expand All @@ -97,7 +106,14 @@ export class SigninStore extends ComponentStore<SigninState> {
this.updateService.hardRefreshIfNeeded(),
])),
tap(() => this.setLoadingState(false)),
switchMap(() => this.handleLoginWithToken()),
switchMap(() => {
const queryToken = this.activatedRoute.snapshot.queryParamMap.get('token');
if (queryToken) {
return this.handleLoginWithQueryToken(queryToken);
}

return this.handleLoginWithToken();
}),
));

handleSuccessfulLogin = this.effect((trigger$: Observable<void>) => trigger$.pipe(
Expand Down Expand Up @@ -239,8 +255,23 @@ export class SigninStore extends ComponentStore<SigninState> {
.subscribe((event) => this.setFailoverDisabledReasons(event.disabled_reasons));
}

private handleLoginWithQueryToken(token: string): Observable<LoginResult> {
this.authService.setQueryToken(token);

return this.authService.loginWithToken().pipe(
tap(this.handleLoginResult.bind(this)),
tapResponse(
() => {},
(error: unknown) => {
this.dialogService.error(this.errorHandler.parseError(error));
},
),
);
}

private handleLoginWithToken(): Observable<LoginResult> {
return this.tokenLastUsedService.isTokenWithinTimeline$.pipe(take(1)).pipe(
return this.tokenLastUsedService.isTokenWithinTimeline$.pipe(
take(1),
filter((isTokenWithinTimeline) => {
if (!isTokenWithinTimeline) {
this.authService.clearAuthToken();
Expand All @@ -249,13 +280,7 @@ export class SigninStore extends ComponentStore<SigninState> {
return isTokenWithinTimeline;
}),
switchMap(() => this.authService.loginWithToken()),
tap((loginResult) => {
if (loginResult !== LoginResult.Success) {
this.authService.clearAuthToken();
} else {
this.handleSuccessfulLogin();
}
}),
tap(this.handleLoginResult.bind(this)),
tapResponse(
() => {},
(error: unknown) => {
Expand Down
47 changes: 47 additions & 0 deletions src/app/services/auth/auth-guard.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { ActivatedRouteSnapshot, Router, RouterStateSnapshot } from '@angular/router';
import {
createServiceFactory,
mockProvider,
SpectatorService,
} from '@ngneat/spectator/jest';
import { BehaviorSubject } from 'rxjs';
import { AuthGuardService } from 'app/services/auth/auth-guard.service';
import { AuthService } from 'app/services/auth/auth.service';

describe('AuthGuardService', () => {
const redirectUrl = 'storage/disks';
const isAuthenticated$ = new BehaviorSubject(false);
const arSnapshot = { queryParams: {} } as ActivatedRouteSnapshot;
const state = { url: redirectUrl } as RouterStateSnapshot;

let spectator: SpectatorService<AuthGuardService>;
const createService = createServiceFactory({
service: AuthGuardService,
providers: [mockProvider(AuthService, { isAuthenticated$ })],
});

beforeEach(() => {
spectator = createService();
isAuthenticated$.next(false);
});

describe('canActivate', () => {
it('allows activation if the user is logged in', () => {
expect(spectator.service.canActivate(arSnapshot, state)).toBe(false);
isAuthenticated$.next(true);
expect(spectator.service.canActivate(arSnapshot, state)).toBe(true);
});

it('if the user is not logged in, the redirect URL is saved to sessionStorage', () => {
spectator.service.canActivate(arSnapshot, state);
expect(sessionStorage.getItem('redirectUrl')).toEqual(redirectUrl);
});

it('if the user is not logged in, the user is redirected to signin', () => {
const router = spectator.inject(Router);
const navigateSpy = jest.spyOn(router, 'navigate');
spectator.service.canActivate(arSnapshot, state);
expect(navigateSpy).toHaveBeenCalledWith(['/signin'], { queryParams: {} });
});
});
});
8 changes: 4 additions & 4 deletions src/app/services/auth/auth-guard.service.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Inject, Injectable } from '@angular/core';
import { ActivatedRouteSnapshot, RouterStateSnapshot, Router } from '@angular/router';
import { ActivatedRouteSnapshot, Router, RouterStateSnapshot } from '@angular/router';
import { UntilDestroy, untilDestroyed } from '@ngneat/until-destroy';
import { WINDOW } from 'app/helpers/window.helper';
import { AuthService } from 'app/services/auth/auth.service';
Expand All @@ -20,13 +20,13 @@ export class AuthGuardService {
});
}

canActivate(_: ActivatedRouteSnapshot, state: RouterStateSnapshot): boolean {
canActivate({ queryParams }: ActivatedRouteSnapshot, state: RouterStateSnapshot): boolean {
if (this.isAuthenticated) {
return true;
}

this.window.sessionStorage.setItem('redirectUrl', state.url);
this.router.navigate(['/signin']);
this.window.sessionStorage.setItem('redirectUrl', state.url.split('?')[0]);
this.router.navigate(['/signin'], { queryParams });

return false;
}
Expand Down
26 changes: 25 additions & 1 deletion src/app/services/auth/auth.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ import { mockCall, mockApi } from 'app/core/testing/utils/mock-api.utils';
import { mockAuth } from 'app/core/testing/utils/mock-auth.utils';
import { LoginResult } from 'app/enums/login-result.enum';
import { Role } from 'app/enums/role.enum';
import { LoginExResponse, LoginExResponseType } from 'app/interfaces/auth.interface';
import { WINDOW } from 'app/helpers/window.helper';
import { LoginExMechanism, LoginExResponse, LoginExResponseType } from 'app/interfaces/auth.interface';
import { DashConfigItem } from 'app/interfaces/dash-config-item.interface';
import { LoggedInUser } from 'app/interfaces/ds-cache.interface';
import { Preferences } from 'app/interfaces/preferences.interface';
Expand Down Expand Up @@ -194,4 +195,27 @@ describe('AuthService', () => {
expect(await firstValueFrom(spectator.service.hasRole([Role.AlertListRead]))).toBe(true);
});
});

describe('setQueryToken', () => {
it('does not set the token if the token is null or the protocol is not secure', async () => {
spectator.service.setQueryToken(null);
let result = await firstValueFrom(spectator.service.loginWithToken());
expect(result).toEqual(LoginResult.NoToken);

const window = spectator.inject<Window>(WINDOW);
Object.defineProperty(window, 'location', { value: { protocol: 'http:' } });
spectator.service.setQueryToken('token');
result = await firstValueFrom(spectator.service.loginWithToken());
expect(result).toEqual(LoginResult.NoToken);

const token = 'token';
window.location.protocol = 'https:';
spectator.service.setQueryToken(token);
await firstValueFrom(spectator.service.loginWithToken());
expect(spectator.inject(ApiService).call).toHaveBeenCalledWith(
'auth.login_ex',
[{ mechanism: LoginExMechanism.TokenPlain, token }],
);
});
});
});
8 changes: 8 additions & 0 deletions src/app/services/auth/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,14 @@ export class AuthService {
);
}

setQueryToken(token: string | null): void {
if (!token || this.window.location.protocol !== 'https:') {
return;
}

this.token = token;
}

loginWithToken(): Observable<LoginResult> {
if (!this.token) {
return of(LoginResult.NoToken);
Expand Down
5 changes: 1 addition & 4 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,7 @@ import { MAT_SNACK_BAR_DEFAULT_OPTIONS, MatSnackBarConfig } from '@angular/mater
import { BrowserModule, bootstrapApplication } from '@angular/platform-browser';
import { provideAnimations } from '@angular/platform-browser/animations';
import {
withPreloading,
provideRouter,
PreloadAllModules,
withComponentInputBinding,
withPreloading, provideRouter, PreloadAllModules, withComponentInputBinding,
} from '@angular/router';
import { provideEffects } from '@ngrx/effects';
import { provideRouterStore } from '@ngrx/router-store';
Expand Down

0 comments on commit 357cc9f

Please sign in to comment.