Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AAE-19542] Fix infinite loop when code flow is enabled #9193

Merged
merged 9 commits into from
Jan 23, 2024
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(){}
}
Loading