Skip to content

Commit

Permalink
[AAE-19542] Fix infinite loop when code flow is enabled (#9193)
Browse files Browse the repository at this point in the history
* [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 4e2a39b.

* Revert "[AAE-12531] Bump angular-oauth-oidc to version 15, fixed a lot of critical bugs and ready for angular 15"

This reverts commit 9ae308a.
  • Loading branch information
alep85 committed Jan 23, 2024
1 parent b9b89fd commit 62bc842
Show file tree
Hide file tree
Showing 8 changed files with 107 additions and 87 deletions.
1 change: 1 addition & 0 deletions lib/core/src/lib/auth/oidc/auth-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { InjectionToken } from '@angular/core';

export interface AuthModuleConfig {
readonly useHash: boolean;
preventClearHashAfterLogin?: boolean;
}

export const AUTH_MODULE_CONFIG = new InjectionToken<AuthModuleConfig>('AUTH_MODULE_CONFIG');
3 changes: 2 additions & 1 deletion lib/core/src/lib/auth/oidc/auth-routing.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
4 changes: 1 addition & 3 deletions lib/core/src/lib/auth/oidc/auth.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
{
Expand All @@ -68,6 +65,7 @@ export function loginFactory(oAuthService: OAuthService, storage: OAuthStorage,
})
export class AuthModule {
static forRoot(config: AuthModuleConfig = { useHash: false }): ModuleWithProviders<AuthModule> {
config.preventClearHashAfterLogin = config.preventClearHashAfterLogin ?? true;
return {
ngModule: AuthModule,
providers: [{ provide: AUTH_MODULE_CONFIG, useValue: config }]
Expand Down
4 changes: 2 additions & 2 deletions lib/core/src/lib/auth/oidc/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -54,6 +54,6 @@ export abstract class AuthService {
*
* @returns Promise, resolve with stored state, reject if unable to reach IdP
*/
abstract loginCallback(): Promise<string | undefined>;
abstract loginCallback(loginOptions?: LoginOptions): Promise<string | undefined>;
abstract updateIDPConfiguration(...args: any[]): void;
}
107 changes: 70 additions & 37 deletions lib/core/src/lib/auth/oidc/oidc-auth.guard.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<AuthService>;
let routerSpy: jasmine.SpyObj<Router>;

beforeEach(() => {
const routerSpyObj = jasmine.createSpyObj('Router', ['navigateByUrl']);
const authSpy = jasmine.createSpyObj('AuthService', ['loginCallback']);

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);
TestBed.configureTestingModule({
providers: [
OidcAuthGuard,
{ provide: AuthService, useValue: authSpy },
{ provide: Router, useValue: routerSpyObj }
],
imports: [RouterTestingModule]
});

expect(authGuard.canActivate(routeSnapshot, state)).toEqual(false);
expect(authService.login).toHaveBeenCalled();
routerSpy = TestBed.inject(Router) as jasmine.SpyObj<Router>;
oidcAuthGuard = TestBed.inject(OidcAuthGuard);
authServiceSpy = TestBed.inject(AuthService) as jasmine.SpyObj<AuthService>;
});

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();

expect(result).toBe(true);
});

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);
it('should call isAuthenticated and return the result', () => {
const isAuthenticatedSpy = spyOn<any>(oidcAuthGuard, '_isAuthenticated').and.returnValue(true);

expect(authGuard.canActivateChild(routeSnapshot, state)).toEqual(false);
expect(authService.login).toHaveBeenCalled();
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<any>(oidcAuthGuard, '_isAuthenticated').and.returnValue(true);

const result = oidcAuthGuard.canActivateChild();

expect(authGuard.canActivateChild(routeSnapshot, state)).toEqual(true);
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 });
});
});
});
47 changes: 23 additions & 24 deletions lib/core/src/lib/auth/oidc/oidc-auth.guard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}

canActivate(
_route: ActivatedRouteSnapshot,
state: RouterStateSnapshot
): Observable<boolean | UrlTree> | Promise<boolean | UrlTree> | 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;
}
constructor(private auth: AuthService, private _router: Router) { }

const loginResult = this.auth.login(state.url);
canActivate(
): Observable<boolean | UrlTree> | Promise<boolean | UrlTree> | boolean | UrlTree {
return this._isAuthenticated();
}

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 }));
}

}

11 changes: 7 additions & 4 deletions lib/core/src/lib/auth/oidc/redirect-auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = <T>(value: T | Promise<T>): value is Promise<T> => value && typeof (value as Promise<T>).then === 'function';

@Injectable()
export class RedirectAuthService extends AuthService {

readonly authModuleConfig: AuthModuleConfig = inject(AUTH_MODULE_CONFIG);

onLogin: Observable<any>;

private _loadDiscoveryDocumentPromise = Promise.resolve(false);
Expand Down Expand Up @@ -127,9 +130,9 @@ export class RedirectAuthService extends AuthService {
);
}

async loginCallback(): Promise<string | undefined> {
async loginCallback(loginOptions?: LoginOptions): Promise<string | undefined> {
return this.ensureDiscoveryDocument()
.then(() => this.oauthService.tryLogin({ preventClearHashAfterLogin: true }))
.then(() => this.oauthService.tryLogin({ ...loginOptions, preventClearHashAfterLogin: this.authModuleConfig.preventClearHashAfterLogin }))
.then(() => this._getRedirectUrl());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: '<div data-automation-id="auth-confirmation"></div>',
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(){}
}

0 comments on commit 62bc842

Please sign in to comment.