From a30773f3a9bf595e85f08a30899d497d8a0eeae4 Mon Sep 17 00:00:00 2001 From: "Vila,Jordi (IT EDP)" Date: Mon, 8 Jun 2026 11:51:18 +0200 Subject: [PATCH 1/2] Fix azure loops --- src/app/app.routes.ts | 1 + src/app/services/azure.service.spec.ts | 45 ++++++++++++++- src/app/services/azure.service.ts | 26 ++++++++- src/app/token.interceptor.spec.ts | 79 +++++++++++++++++++++++--- src/app/token.interceptor.ts | 28 ++++++++- 5 files changed, 168 insertions(+), 11 deletions(-) diff --git a/src/app/app.routes.ts b/src/app/app.routes.ts index 998b666..0e4add9 100644 --- a/src/app/app.routes.ts +++ b/src/app/app.routes.ts @@ -12,6 +12,7 @@ import { PageNotFoundScreenComponent } from './screens/page-not-found-screen/pag export const routes: Routes = [ { path: '', component: ProductCatalogScreenComponent, canActivate: [ MsalGuard ], resolve: { catalogs: CatalogResolver } }, { path: 'notifications', component: NotificationsScreenComponent, canActivate: [ MsalGuard ] }, + { path: 'login-failed', component: PageNotFoundScreenComponent }, { path: 'page-not-found', component: PageNotFoundScreenComponent }, { path: ':projectKey/components', component: ProjectComponentsScreenComponent, canActivate: [ MsalGuard ] }, { path: ':catalogSlug', component: ProductCatalogScreenComponent, canActivate: [ MsalGuard ], resolve: { catalogs: CatalogResolver } }, diff --git a/src/app/services/azure.service.spec.ts b/src/app/services/azure.service.spec.ts index a8cee00..2ddc793 100644 --- a/src/app/services/azure.service.spec.ts +++ b/src/app/services/azure.service.spec.ts @@ -1,7 +1,7 @@ import { fakeAsync, TestBed, tick } from '@angular/core/testing'; import { AzureService } from './azure.service'; import { MSAL_GUARD_CONFIG, MsalBroadcastService, MsalGuardConfiguration, MsalService } from "@azure/msal-angular"; -import { BehaviorSubject, of, Subject } from 'rxjs'; +import { BehaviorSubject, of, Subject, throwError } from 'rxjs'; import { AuthenticationResult, EventMessage, EventType, InteractionStatus, InteractionType, RedirectRequest } from '@azure/msal-browser'; import { Router } from '@angular/router'; import { AppConfigService } from './app-config.service'; @@ -24,6 +24,7 @@ describe('AzureService', () => { inProgress$ = new Subject(); const msalServiceSpy = jasmine.createSpyObj('MsalService', ['handleRedirectObservable', 'instance', 'loginRedirect', 'logout']); + msalServiceSpy.loginRedirect.and.returnValue(of(void 0)); const msalBroadcastServiceSpy = jasmine.createSpyObj('MsalBroadcastService', [], { msalSubject$: msalSubject$.asObservable(), inProgress$: inProgress$.asObservable() @@ -170,6 +171,7 @@ describe('AzureService', () => { it('login - should call loginRedirect with authRequest if msalGuardConfig.authRequest is defined', () => { msalGuardConfig.authRequest = { scopes: ['User.Read'] } + msalInstanceSpy.getAllAccounts.and.returnValue([]); service.login(); @@ -180,12 +182,53 @@ describe('AzureService', () => { it('login - should call loginRedirect without parameters if msalGuardConfig.authRequest is not defined', () => { msalGuardConfig.authRequest = undefined; + msalInstanceSpy.getAllAccounts.and.returnValue([]); service.login(); expect(msalService.loginRedirect).toHaveBeenCalledWith(); }); + it('login - should reset loginInProgress when loginRedirect fails with non-interaction error', () => { + msalGuardConfig.authRequest = { scopes: ['User.Read'] }; + msalInstanceSpy.getAllAccounts.and.returnValue([]); + msalService.loginRedirect.and.returnValue(throwError(() => ({ errorCode: 'some_other_error' }))); + + service.login(); + + expect((service as any).loginInProgress).toBeFalse(); + }); + + it('login - should keep loginInProgress when loginRedirect fails with interaction_in_progress', () => { + msalGuardConfig.authRequest = { scopes: ['User.Read'] }; + msalInstanceSpy.getAllAccounts.and.returnValue([]); + msalService.loginRedirect.and.returnValue(throwError(() => ({ errorCode: 'interaction_in_progress' }))); + + service.login(); + + expect((service as any).loginInProgress).toBeTrue(); + }); + + it('login - should reset loginInProgress when loginRedirect without authRequest fails with non-interaction error', () => { + msalGuardConfig.authRequest = undefined; + msalInstanceSpy.getAllAccounts.and.returnValue([]); + msalService.loginRedirect.and.returnValue(throwError(() => ({ errorCode: 'some_other_error' }))); + + service.login(); + + expect((service as any).loginInProgress).toBeFalse(); + }); + + it('login - should keep loginInProgress when loginRedirect without authRequest fails with interaction_in_progress', () => { + msalGuardConfig.authRequest = undefined; + msalInstanceSpy.getAllAccounts.and.returnValue([]); + msalService.loginRedirect.and.returnValue(throwError(() => ({ errorCode: 'interaction_in_progress' }))); + + service.login(); + + expect((service as any).loginInProgress).toBeTrue(); + }); + it('logout - should call logout on msalService and clear the token cache', () => { (service as any).cachedAccessToken = 'cached-token'; (service as any).tokenExpiresOn = new Date(Date.now() + 10 * 60 * 1000); diff --git a/src/app/services/azure.service.ts b/src/app/services/azure.service.ts index 4918a9e..77f2fb3 100644 --- a/src/app/services/azure.service.ts +++ b/src/app/services/azure.service.ts @@ -15,6 +15,7 @@ export class AzureService implements OnDestroy { private readonly _destroying$ = new Subject(); private cachedAccessToken: string | null = null; private tokenExpiresOn: Date | null = null; + private loginInProgress = false; private static readonly TOKEN_EXPIRY_BUFFER_MS = 5 * 60 * 1000; // 5 minutes isFirstTime = true; @@ -56,6 +57,7 @@ export class AzureService implements OnDestroy { takeUntil(this._destroying$) ) .subscribe(() => { + this.loginInProgress = false; this.setLoginDisplay(); this.checkAndSetActiveAccount(); }); @@ -161,12 +163,32 @@ export class AzureService implements OnDestroy { } login() { + if (this.loginInProgress || this.msalService.instance.getAllAccounts().length > 0) { + return; + } + + this.loginInProgress = true; + if (this.msalGuardConfig.authRequest) { this.msalService.loginRedirect({ ...this.msalGuardConfig.authRequest, - } as RedirectRequest); + } as RedirectRequest).subscribe({ + error: (error: unknown) => { + if (!(error && typeof error === 'object' && 'errorCode' in error && (error as { errorCode?: string }).errorCode === 'interaction_in_progress')) { + this.loginInProgress = false; + console.error('Error during login redirect', error); + } + } + }); } else { - this.msalService.loginRedirect(); + this.msalService.loginRedirect().subscribe({ + error: (error: unknown) => { + if (!(error && typeof error === 'object' && 'errorCode' in error && (error as { errorCode?: string }).errorCode === 'interaction_in_progress')) { + this.loginInProgress = false; + console.error('Error during login redirect', error); + } + } + }); } } diff --git a/src/app/token.interceptor.spec.ts b/src/app/token.interceptor.spec.ts index 069148f..607010c 100644 --- a/src/app/token.interceptor.spec.ts +++ b/src/app/token.interceptor.spec.ts @@ -5,6 +5,8 @@ import { Router } from '@angular/router'; import { AzureService } from './services/azure.service'; import { tokenInterceptor } from './token.interceptor'; +type URLConstructor = typeof URL; + describe('TokenInterceptor', () => { let http: HttpClient; @@ -34,7 +36,7 @@ describe('TokenInterceptor', () => { it('should add an Authorization header', fakeAsync(() => { mockAzureService.getAccessToken.and.returnValue(Promise.resolve('test-token')); - http.get('/test').subscribe(() => {}); + http.get('/component-catalog/test').subscribe(() => {}); tick(); const req = httpTestingController.expectOne( @@ -54,7 +56,7 @@ describe('TokenInterceptor', () => { Promise.resolve(forceRefresh ? 'new-token' : 'expired-token') ); - http.get('/test').subscribe(() => {}); + http.get('/component-catalog/test').subscribe(() => {}); tick(); const req = httpTestingController.expectOne( @@ -85,12 +87,12 @@ describe('TokenInterceptor', () => { forceRefresh ? Promise.reject(new Error('Refresh token failed')) : Promise.resolve('expired-token') ); - http.get('/test').subscribe({ + http.get('/component-catalog/test').subscribe({ error: () => {} }); tick(); - const req = httpTestingController.expectOne('/test'); + const req = httpTestingController.expectOne('/component-catalog/test'); req.flush(null, { status: 401, statusText: 'Unauthorized' }); tick(); httpTestingController.verify(); @@ -102,7 +104,7 @@ describe('TokenInterceptor', () => { Promise.resolve(forceRefresh ? 'new-token' : 'expired-token') ); - http.get('/test').subscribe({ error: () => {} }); + http.get('/component-catalog/test').subscribe({ error: () => {} }); tick(); const req = httpTestingController.expectOne( @@ -126,17 +128,80 @@ describe('TokenInterceptor', () => { it('should pass through non-401/403 errors', fakeAsync(() => { mockAzureService.getAccessToken.and.returnValue(Promise.resolve('test-token')); - http.get('/test').subscribe({ + http.get('/component-catalog/test').subscribe({ error: (error) => { expect(error.status).toBe(500); } }); tick(); - const req = httpTestingController.expectOne('/test'); + const req = httpTestingController.expectOne('/component-catalog/test'); req.flush(null, { status: 500, statusText: 'Server Error' }); httpTestingController.verify(); })); + + it('should bypass token injection for non-protected requests', fakeAsync(() => { + http.get('/assets/logo.svg').subscribe(() => {}); + tick(); + + const req = httpTestingController.expectOne('/assets/logo.svg'); + expect(req.request.headers.has('Authorization')).toBeFalse(); + expect(mockAzureService.getAccessToken).not.toHaveBeenCalled(); + req.flush({}); + })); + + it('should bypass token injection when URL parsing throws', fakeAsync(() => { + const originalUrl = window.URL; + // Force the URL parsing branch in isProtectedApiRequest to hit catch. + (window as unknown as { URL: URLConstructor }).URL = function () { + throw new TypeError('Invalid URL'); + } as unknown as URLConstructor; + + try { + http.get('http://example.invalid/test').subscribe(() => {}); + tick(); + + const req = httpTestingController.expectOne('http://example.invalid/test'); + expect(req.request.headers.has('Authorization')).toBeFalse(); + expect(mockAzureService.getAccessToken).not.toHaveBeenCalled(); + req.flush({}); + } finally { + (window as unknown as { URL: URLConstructor }).URL = originalUrl; + } + })); + + it('should trigger login and cancel protected request when no account is available', fakeAsync(() => { + mockAzureService.getAccessToken.and.returnValue(Promise.reject(new Error('No accounts found. User must sign in first.'))); + + let completed = false; + http.get('/component-catalog/test').subscribe({ + complete: () => { + completed = true; + } + }); + tick(); + + httpTestingController.expectNone('/component-catalog/test'); + expect(mockAzureService.login).toHaveBeenCalled(); + expect(completed).toBeTrue(); + })); + + it('should rethrow token acquisition errors that are not no-account errors', fakeAsync(() => { + const expectedError = new Error('Unexpected token failure'); + mockAzureService.getAccessToken.and.returnValue(Promise.reject(expectedError)); + + let actualError: unknown; + http.get('/component-catalog/test').subscribe({ + error: (error) => { + actualError = error; + } + }); + tick(); + + httpTestingController.expectNone('/component-catalog/test'); + expect(mockAzureService.login).not.toHaveBeenCalled(); + expect(actualError).toBe(expectedError); + })); }); \ No newline at end of file diff --git a/src/app/token.interceptor.ts b/src/app/token.interceptor.ts index 0c1c5ba..026af0f 100644 --- a/src/app/token.interceptor.ts +++ b/src/app/token.interceptor.ts @@ -1,14 +1,40 @@ import { HttpErrorResponse, HttpEvent, HttpHandlerFn, HttpRequest } from "@angular/common/http"; import { AzureService } from "./services/azure.service"; -import { catchError, from, Observable, switchMap, throwError } from "rxjs"; +import { catchError, EMPTY, from, Observable, switchMap, throwError } from "rxjs"; import { inject } from "@angular/core"; import { Router } from "@angular/router"; +const PROTECTED_API_PREFIXES = ['/component-catalog', '/projects-api', '/component-provisioner']; + +function isProtectedApiRequest(url: string): boolean { + if (PROTECTED_API_PREFIXES.some((prefix) => url.startsWith(prefix))) { + return true; + } + + try { + const parsedUrl = new URL(url, window.location.origin); + return PROTECTED_API_PREFIXES.some((prefix) => parsedUrl.pathname.startsWith(prefix)); + } catch { + return false; + } +} + export function tokenInterceptor(request: HttpRequest, next: HttpHandlerFn): Observable> { const authService = inject(AzureService); const router = inject(Router); + if (!isProtectedApiRequest(request.url)) { + return next(request); + } + return from(authService.getAccessToken()).pipe( + catchError((tokenErr: unknown) => { + if (tokenErr instanceof Error && tokenErr.message.includes('No accounts found. User must sign in first.')) { + authService.login(); + return EMPTY; + } + return throwError(() => tokenErr); + }), switchMap((token) => { if (token) { request = request.clone({ From f302fae8e0c0849d043d41ad5001105a40ed6e79 Mon Sep 17 00:00:00 2001 From: "Vila,Jordi (IT EDP)" Date: Mon, 8 Jun 2026 12:23:34 +0200 Subject: [PATCH 2/2] Fix sonarqube smell --- src/app/token.interceptor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/token.interceptor.ts b/src/app/token.interceptor.ts index 026af0f..65e18d0 100644 --- a/src/app/token.interceptor.ts +++ b/src/app/token.interceptor.ts @@ -12,7 +12,7 @@ function isProtectedApiRequest(url: string): boolean { } try { - const parsedUrl = new URL(url, window.location.origin); + const parsedUrl = new URL(url, globalThis.location.origin); return PROTECTED_API_PREFIXES.some((prefix) => parsedUrl.pathname.startsWith(prefix)); } catch { return false;