From 62bc8423a30343a91d63b24a80190c52bd6b82ec Mon Sep 17 00:00:00 2001 From: Amedeo Lepore Date: Tue, 23 Jan 2024 19:12:10 +0100 Subject: [PATCH] [AAE-19542] Fix infinite loop when code flow is enabled (#9193) * [AAE-12531] Bump angular-oauth-oidc to version 15, fixed a lot of critical bugs and ready for angular 15 * [AAE-12531] Remove commented code * [AAE-12531] Fix window.location.search is empty when loginCallback is called * [AAE-12521] Provide guard in root * [AAE-12521] move navigation to the guard to fix infinite loop issue with code flow auth * [AAE-12521] allow to set the preventClearHashAfterLogin value by forRoot method to choose if clear hash fragment after the lib read the token * [AAE-12531] Set angular-oauth-oidc version to 14, since version 15 doesn't work with angular v14 * Revert "[AAE-12531] Set angular-oauth-oidc version to 14, since version 15 doesn't work with angular v14" This reverts commit 4e2a39bf6a13954cd39e19a28eb089d22562e53a. * Revert "[AAE-12531] Bump angular-oauth-oidc to version 15, fixed a lot of critical bugs and ready for angular 15" This reverts commit 9ae308a7f8dd9d74226d91e0fe0dddb362dd1235. --- lib/core/src/lib/auth/oidc/auth-config.ts | 1 + .../src/lib/auth/oidc/auth-routing.module.ts | 3 +- lib/core/src/lib/auth/oidc/auth.module.ts | 4 +- lib/core/src/lib/auth/oidc/auth.service.ts | 4 +- .../src/lib/auth/oidc/oidc-auth.guard.spec.ts | 107 ++++++++++++------ lib/core/src/lib/auth/oidc/oidc-auth.guard.ts | 45 ++++---- .../lib/auth/oidc/redirect-auth.service.ts | 11 +- .../authentication-confirmation.component.ts | 17 +-- 8 files changed, 106 insertions(+), 86 deletions(-) diff --git a/lib/core/src/lib/auth/oidc/auth-config.ts b/lib/core/src/lib/auth/oidc/auth-config.ts index adfc5bf4c1..5adbb11f9a 100644 --- a/lib/core/src/lib/auth/oidc/auth-config.ts +++ b/lib/core/src/lib/auth/oidc/auth-config.ts @@ -19,6 +19,7 @@ import { InjectionToken } from '@angular/core'; export interface AuthModuleConfig { readonly useHash: boolean; + preventClearHashAfterLogin?: boolean; } export const AUTH_MODULE_CONFIG = new InjectionToken('AUTH_MODULE_CONFIG'); diff --git a/lib/core/src/lib/auth/oidc/auth-routing.module.ts b/lib/core/src/lib/auth/oidc/auth-routing.module.ts index 7308e3cde1..53406b208a 100644 --- a/lib/core/src/lib/auth/oidc/auth-routing.module.ts +++ b/lib/core/src/lib/auth/oidc/auth-routing.module.ts @@ -18,9 +18,10 @@ import { NgModule } from '@angular/core'; import { RouterModule, Routes } from '@angular/router'; import { AuthenticationConfirmationComponent } from './view/authentication-confirmation/authentication-confirmation.component'; +import { OidcAuthGuard } from './oidc-auth.guard'; const routes: Routes = [ - { path: 'view/authentication-confirmation', component: AuthenticationConfirmationComponent } + { path: 'view/authentication-confirmation', component: AuthenticationConfirmationComponent, canActivate: [OidcAuthGuard]} ]; @NgModule({ diff --git a/lib/core/src/lib/auth/oidc/auth.module.ts b/lib/core/src/lib/auth/oidc/auth.module.ts index 354edad04a..062e22013b 100644 --- a/lib/core/src/lib/auth/oidc/auth.module.ts +++ b/lib/core/src/lib/auth/oidc/auth.module.ts @@ -46,9 +46,6 @@ export function loginFactory(oAuthService: OAuthService, storage: OAuthStorage, imports: [AuthRoutingModule, OAuthModule.forRoot()], providers: [ { provide: OAuthStorage, useExisting: StorageService }, - // { provide: AuthGuard, useClass: OidcAuthGuard }, - // { provide: AuthGuardEcm, useClass: OidcAuthGuard }, - // { provide: AuthGuardBpm, useClass: OidcAuthGuard }, { provide: AuthenticationService}, { provide: AlfrescoApiService, useClass: AlfrescoApiNoAuthService }, { @@ -68,6 +65,7 @@ export function loginFactory(oAuthService: OAuthService, storage: OAuthStorage, }) export class AuthModule { static forRoot(config: AuthModuleConfig = { useHash: false }): ModuleWithProviders { + config.preventClearHashAfterLogin = config.preventClearHashAfterLogin ?? true; return { ngModule: AuthModule, providers: [{ provide: AUTH_MODULE_CONFIG, useValue: config }] diff --git a/lib/core/src/lib/auth/oidc/auth.service.ts b/lib/core/src/lib/auth/oidc/auth.service.ts index be027665ab..920d933402 100644 --- a/lib/core/src/lib/auth/oidc/auth.service.ts +++ b/lib/core/src/lib/auth/oidc/auth.service.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { TokenResponse } from 'angular-oauth2-oidc'; +import { LoginOptions, TokenResponse } from 'angular-oauth2-oidc'; import { Observable } from 'rxjs'; /** @@ -54,6 +54,6 @@ export abstract class AuthService { * * @returns Promise, resolve with stored state, reject if unable to reach IdP */ - abstract loginCallback(): Promise; + abstract loginCallback(loginOptions?: LoginOptions): Promise; abstract updateIDPConfiguration(...args: any[]): void; } diff --git a/lib/core/src/lib/auth/oidc/oidc-auth.guard.spec.ts b/lib/core/src/lib/auth/oidc/oidc-auth.guard.spec.ts index 2f5f8c50a8..58bf429869 100644 --- a/lib/core/src/lib/auth/oidc/oidc-auth.guard.spec.ts +++ b/lib/core/src/lib/auth/oidc/oidc-auth.guard.spec.ts @@ -16,57 +16,90 @@ */ import { TestBed } from '@angular/core/testing'; -import { ActivatedRouteSnapshot, RouterStateSnapshot } from '@angular/router'; import { RouterTestingModule } from '@angular/router/testing'; -import { AuthService } from './auth.service'; +import { Router } from '@angular/router'; import { OidcAuthGuard } from './oidc-auth.guard'; - -const state: RouterStateSnapshot = { - root: new ActivatedRouteSnapshot(), - url: 'http://example.com' -}; -const routeSnapshot = new ActivatedRouteSnapshot(); +import { AuthService } from './auth.service'; describe('OidcAuthGuard', () => { - beforeEach(() => { - TestBed.configureTestingModule({ - imports: [RouterTestingModule], - providers: [OidcAuthGuard] - }); - }); + let oidcAuthGuard: OidcAuthGuard; + let authServiceSpy: jasmine.SpyObj; + let routerSpy: jasmine.SpyObj; - describe('#canActivate', () => { - it('should return false if the user is not authenticated, and call login method', () => { - const authService = { authenticated: false, login: jasmine.createSpy() } as unknown as AuthService; - const authGuard = new OidcAuthGuard(authService); + beforeEach(() => { + const routerSpyObj = jasmine.createSpyObj('Router', ['navigateByUrl']); + const authSpy = jasmine.createSpyObj('AuthService', ['loginCallback']); - expect(authGuard.canActivate(routeSnapshot, state)).toEqual(false); - expect(authService.login).toHaveBeenCalled(); + TestBed.configureTestingModule({ + providers: [ + OidcAuthGuard, + { provide: AuthService, useValue: authSpy }, + { provide: Router, useValue: routerSpyObj } + ], + imports: [RouterTestingModule] + }); + + routerSpy = TestBed.inject(Router) as jasmine.SpyObj; + oidcAuthGuard = TestBed.inject(OidcAuthGuard); + authServiceSpy = TestBed.inject(AuthService) as jasmine.SpyObj; }); - it('should return true if the user is authenticated', () => { - const authService = { authenticated: true } as unknown as AuthService; - const authGuard = new OidcAuthGuard(authService); + describe('canActivate', () => { + it('should return true if is authenticated', () => { + authServiceSpy.authenticated = true; - expect(authGuard.canActivate(routeSnapshot, state)).toEqual(true); - }); - }); + const result = oidcAuthGuard.canActivate(); - describe('#canActivateChild', () => { - it('should return false if the user is not authenticated, and call login method', () => { - const authService = { authenticated: false, login: jasmine.createSpy() } as unknown as AuthService; - const authGuard = new OidcAuthGuard(authService); + expect(result).toBe(true); + }); - expect(authGuard.canActivateChild(routeSnapshot, state)).toEqual(false); - expect(authService.login).toHaveBeenCalled(); + it('should call isAuthenticated and return the result', () => { + const isAuthenticatedSpy = spyOn(oidcAuthGuard, '_isAuthenticated').and.returnValue(true); + + const result = oidcAuthGuard.canActivate(); + + expect(isAuthenticatedSpy).toHaveBeenCalled(); + expect(result).toBe(true); + }); }); - it('should return true if the user is authenticated', () => { - const authService = { authenticated: true } as unknown as AuthService; - const authGuard = new OidcAuthGuard(authService); + describe('canActivateChild', () => { + it('should call isAuthenticated and return its result', () => { + const isAuthenticatedSpy = spyOn(oidcAuthGuard, '_isAuthenticated').and.returnValue(true); - expect(authGuard.canActivateChild(routeSnapshot, state)).toEqual(true); + const result = oidcAuthGuard.canActivateChild(); + + expect(isAuthenticatedSpy).toHaveBeenCalled(); + expect(result).toBe(true); + }); }); - }); + describe('isAuthenticated', () => { + it('should return true if is authenticated', () => { + authServiceSpy.authenticated = true; + + const result = oidcAuthGuard['_isAuthenticated'](); + + expect(result).toBe(true); + }); + + it('should call loginCallback and navigateByUrl if not authenticated', async () => { + authServiceSpy.authenticated = false; + authServiceSpy.loginCallback.and.returnValue(Promise.resolve('/fake-route')); + + await oidcAuthGuard.canActivate(); + + expect(authServiceSpy.loginCallback).toHaveBeenCalled(); + expect(routerSpy.navigateByUrl).toHaveBeenCalledWith('/fake-route', { replaceUrl: true }); + }); + + it('should navigate to default route if loginCallback fails', async () => { + authServiceSpy.authenticated = false; + authServiceSpy.loginCallback.and.returnValue(Promise.reject(new Error())); + + await oidcAuthGuard.canActivate(); + + expect(routerSpy.navigateByUrl).toHaveBeenCalledWith('/', { replaceUrl: true }); + }); + }); }); diff --git a/lib/core/src/lib/auth/oidc/oidc-auth.guard.ts b/lib/core/src/lib/auth/oidc/oidc-auth.guard.ts index fa0d4e7738..436e41ad7b 100644 --- a/lib/core/src/lib/auth/oidc/oidc-auth.guard.ts +++ b/lib/core/src/lib/auth/oidc/oidc-auth.guard.ts @@ -16,37 +16,36 @@ */ import { Injectable } from '@angular/core'; -import { ActivatedRouteSnapshot, CanActivate, RouterStateSnapshot, UrlTree } from '@angular/router'; +import { CanActivate, Router, UrlTree } from '@angular/router'; import { Observable } from 'rxjs'; import { AuthService } from './auth.service'; -@Injectable() +const ROUTE_DEFAULT = '/'; + +@Injectable({ + providedIn: 'root' +}) export class OidcAuthGuard implements CanActivate { - constructor(private auth: AuthService) {} + constructor(private auth: AuthService, private _router: Router) { } - canActivate( - _route: ActivatedRouteSnapshot, - state: RouterStateSnapshot - ): Observable | Promise | boolean | UrlTree { - return this._isAuthenticated(state); - } - - canActivateChild(_route: ActivatedRouteSnapshot, state: RouterStateSnapshot) { - return this._isAuthenticated(state); - } - - private _isAuthenticated(state: RouterStateSnapshot) { - if (this.auth.authenticated) { - return true; + canActivate( + ): Observable | Promise | boolean | UrlTree { + return this._isAuthenticated(); } - const loginResult = this.auth.login(state.url); - - if (loginResult instanceof Promise) { - return loginResult.then(() => true).catch(() => false); + canActivateChild() { + return this._isAuthenticated(); } - return false; - } + private _isAuthenticated() { + if (this.auth.authenticated) { + return true; + } + + return this.auth.loginCallback({ customHashFragment: window.location.search }) + .then(route => this._router.navigateByUrl(route, { replaceUrl: true })) + .catch(() => this._router.navigateByUrl(ROUTE_DEFAULT, { replaceUrl: true })); + } } + diff --git a/lib/core/src/lib/auth/oidc/redirect-auth.service.ts b/lib/core/src/lib/auth/oidc/redirect-auth.service.ts index 5f7a3cc223..61d5975ec3 100644 --- a/lib/core/src/lib/auth/oidc/redirect-auth.service.ts +++ b/lib/core/src/lib/auth/oidc/redirect-auth.service.ts @@ -15,18 +15,21 @@ * limitations under the License. */ -import { Inject, Injectable } from '@angular/core'; -import { AuthConfig, AUTH_CONFIG, OAuthErrorEvent, OAuthService, OAuthStorage, TokenResponse } from 'angular-oauth2-oidc'; +import { Inject, Injectable, inject } from '@angular/core'; +import { AuthConfig, AUTH_CONFIG, OAuthErrorEvent, OAuthService, OAuthStorage, TokenResponse, LoginOptions } from 'angular-oauth2-oidc'; import { JwksValidationHandler } from 'angular-oauth2-oidc-jwks'; import { from, Observable } from 'rxjs'; import { distinctUntilChanged, filter, map, shareReplay } from 'rxjs/operators'; import { AuthService } from './auth.service'; +import { AUTH_MODULE_CONFIG, AuthModuleConfig } from './auth-config'; const isPromise = (value: T | Promise): value is Promise => value && typeof (value as Promise).then === 'function'; @Injectable() export class RedirectAuthService extends AuthService { + readonly authModuleConfig: AuthModuleConfig = inject(AUTH_MODULE_CONFIG); + onLogin: Observable; private _loadDiscoveryDocumentPromise = Promise.resolve(false); @@ -127,9 +130,9 @@ export class RedirectAuthService extends AuthService { ); } - async loginCallback(): Promise { + async loginCallback(loginOptions?: LoginOptions): Promise { return this.ensureDiscoveryDocument() - .then(() => this.oauthService.tryLogin({ preventClearHashAfterLogin: true })) + .then(() => this.oauthService.tryLogin({ ...loginOptions, preventClearHashAfterLogin: this.authModuleConfig.preventClearHashAfterLogin })) .then(() => this._getRedirectUrl()); } diff --git a/lib/core/src/lib/auth/oidc/view/authentication-confirmation/authentication-confirmation.component.ts b/lib/core/src/lib/auth/oidc/view/authentication-confirmation/authentication-confirmation.component.ts index 1bcf03eb95..e593b5e856 100644 --- a/lib/core/src/lib/auth/oidc/view/authentication-confirmation/authentication-confirmation.component.ts +++ b/lib/core/src/lib/auth/oidc/view/authentication-confirmation/authentication-confirmation.component.ts @@ -16,26 +16,11 @@ */ import { ChangeDetectionStrategy, Component } from '@angular/core'; -import { Router } from '@angular/router'; -import { from, of } from 'rxjs'; -import { catchError, first, map } from 'rxjs/operators'; -import { AuthService } from '../../auth.service'; - -const ROUTE_DEFAULT = '/'; @Component({ template: '
', changeDetection: ChangeDetectionStrategy.OnPush }) export class AuthenticationConfirmationComponent { - constructor(private auth: AuthService, private _router: Router) { - const routeStored$ = from(this.auth.loginCallback()).pipe( - map((route) => route || ROUTE_DEFAULT), - catchError(() => of(ROUTE_DEFAULT)) - ); - - routeStored$.pipe(first()).subscribe((route) => { - this._router.navigateByUrl(route, { replaceUrl: true }); - }); - } + constructor(){} }