diff --git a/src/app/app.component.spec.ts b/src/app/app.component.spec.ts index f684dec..ccaa605 100644 --- a/src/app/app.component.spec.ts +++ b/src/app/app.component.spec.ts @@ -41,7 +41,7 @@ describe('AppComponent', () => { azureLoggedUser$ = new Subject(); natsLiveMessage$ = new Subject(); natsMessageCount$ = new Subject(); - mockAzureService = jasmine.createSpyObj('AzureService', ['initialize', 'login', 'logout', 'refreshToken'], { loggedUser$: azureLoggedUser$.asObservable() }); + mockAzureService = jasmine.createSpyObj('AzureService', ['initialize', 'login', 'logout', 'getAccessToken'], { loggedUser$: azureLoggedUser$.asObservable() }); mockNatsService = jasmine.createSpyObj('NatsService', ['initialize', 'initializeUser', 'readMessages', 'isValidMessage'], { liveMessage$: natsLiveMessage$.asObservable(), unreadMessagesCount$: natsMessageCount$.asObservable() }); mockToastService = jasmine.createSpyObj('AppShellToastService', ['showToast'], { toasts$: of([]) }); mockCatalogService = jasmine.createSpyObj( @@ -77,7 +77,7 @@ describe('AppComponent', () => { mockCatalogService.getSelectedCatalogSlug.and.returnValue(null); mockCatalogService.getSelectedCatalogDescriptor.and.returnValue({ slug: 'test-catalog', id: '1' }); mockAppConfigService.getConfig.and.returnValue({ natsUrl: 'nats://localhost:4222' }); - mockAzureService.refreshToken.and.returnValue(Promise.resolve({ accessToken: 'new-token' } as any)); + mockAzureService.getAccessToken.and.returnValue(Promise.resolve('new-token')); mockProjectService.getUserProjects.and.returnValue(of([])); mockDialogRef.afterClosed.and.returnValue(dialogSubject.asObservable()); mockMatDialog.open.and.returnValue(mockDialogRef); diff --git a/src/app/app.component.ts b/src/app/app.component.ts index 9926b21..dfcb6ea 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -139,8 +139,8 @@ export class AppComponent implements OnInit, OnDestroy { // Apply optimistic UI, start with it and later apply validations after fetching projects to avoid empty parts this.projectPicker = {...this.projectPicker, label: 'Project: ', selected: currentProjectForUi.projectKey}; } - this.azureService.refreshToken().then((azureData) => { - this.projectService.getUserProjects(azureData.accessToken).subscribe((projects: string[]) => { + this.azureService.getAccessToken().then((accessToken: string) => { + this.projectService.getUserProjects(accessToken).subscribe((projects: string[]) => { user.projects = projects; this.initializeNats(user); if (projects.length > 0) { diff --git a/src/app/app.config.ts b/src/app/app.config.ts index 094e7f4..9402c24 100644 --- a/src/app/app.config.ts +++ b/src/app/app.config.ts @@ -1,6 +1,8 @@ import { AppConfigService } from './services/app-config.service'; import { msalProviders } from './azure.config'; -import { ApplicationConfig, provideZoneChangeDetection, provideAppInitializer, inject } from '@angular/core'; +import { ApplicationConfig, provideZoneChangeDetection, provideAppInitializer, inject, Injector } from '@angular/core'; +import { MsalService } from '@azure/msal-angular'; +import { firstValueFrom } from 'rxjs'; import { provideRouter } from '@angular/router'; import { routes } from './app.routes'; import { provideMarkdown } from 'ngx-markdown'; @@ -30,9 +32,15 @@ export const appConfig: ApplicationConfig = { provideAppInitializer(() => { const appConfigService = inject(AppConfigService); const iconService = inject(IconRegistryService); - return appConfigService.loadConfig().then(() => - iconService.registerIconsFromManifest() - ); + const injector = inject(Injector); + return appConfigService.loadConfig() + .then(() => iconService.registerIconsFromManifest()) + .then(() => { + const msalService = injector.get(MsalService); + return firstValueFrom(msalService.initialize()).then(() => + firstValueFrom(msalService.handleRedirectObservable()) + ); + }); }), { provide: MAT_FORM_FIELD_DEFAULT_OPTIONS, diff --git a/src/app/azure.config.ts b/src/app/azure.config.ts index 9424c69..cbb24e5 100644 --- a/src/app/azure.config.ts +++ b/src/app/azure.config.ts @@ -61,7 +61,7 @@ export function MSALGuardConfigFactory(config: AppConfigService): MsalGuardConfi return { interactionType: InteractionType.Redirect, authRequest: { - scopes: [...config.getConfig()?.apiConfig?.scopes || []], + scopes: [...config.getConfig()?.apiConfig?.scopes || [], 'User.Read'], }, loginFailedRoute: '/login-failed', }; diff --git a/src/app/components/request-deletion-dialog/request-deletion-dialog.component.spec.ts b/src/app/components/request-deletion-dialog/request-deletion-dialog.component.spec.ts index b3b4e93..4ec2673 100644 --- a/src/app/components/request-deletion-dialog/request-deletion-dialog.component.spec.ts +++ b/src/app/components/request-deletion-dialog/request-deletion-dialog.component.spec.ts @@ -78,8 +78,7 @@ describe('RequestDeletionDialogComponent', () => { changeNumber: '-', reason: 'Test reason', projectKey: 'test-project', - componentName: 'test-component', - location: 'test-location' + componentName: 'test-component' }); }); @@ -95,8 +94,7 @@ describe('RequestDeletionDialogComponent', () => { changeNumber: 'CHG1234567', reason: 'Test reason', projectKey: 'test-project', - componentName: 'test-component', - location: 'test-location' + componentName: 'test-component' }); }); diff --git a/src/app/components/request-deletion-dialog/request-deletion-dialog.component.ts b/src/app/components/request-deletion-dialog/request-deletion-dialog.component.ts index e1d29c1..32f0548 100644 --- a/src/app/components/request-deletion-dialog/request-deletion-dialog.component.ts +++ b/src/app/components/request-deletion-dialog/request-deletion-dialog.component.ts @@ -60,8 +60,7 @@ export class RequestDeletionDialogComponent { changeNumber: this.deploymentStatus ? this.changeNumber : '-', reason: this.reason, projectKey: this.data.projectKey, - componentName: this.data.componentName, - location: this.data.location + componentName: this.data.componentName }; this.dialogRef.close(result); } diff --git a/src/app/models/request-deletion-dialog-data.ts b/src/app/models/request-deletion-dialog-data.ts index 99f5d37..c888180 100644 --- a/src/app/models/request-deletion-dialog-data.ts +++ b/src/app/models/request-deletion-dialog-data.ts @@ -10,5 +10,4 @@ export interface RequestDeletionDialogResult { deploymentStatus: boolean; changeNumber: string; reason: string; - location: string; } diff --git a/src/app/screens/product-action-screen/product-action-screen.component.spec.ts b/src/app/screens/product-action-screen/product-action-screen.component.spec.ts index 308fd04..788ee51 100644 --- a/src/app/screens/product-action-screen/product-action-screen.component.spec.ts +++ b/src/app/screens/product-action-screen/product-action-screen.component.spec.ts @@ -256,8 +256,7 @@ describe('ProductActionScreenComponent', () => { { name: 'param_4', type: 'string', value: '' }, { name: 'param_5', type: 'string', value: 'value_location_1' }, { name: 'param_6', type: 'singlelist', value: '' }, - { name: 'catalog_item_id', type: 'string', value: 'fakeId' }, - { name: 'access_token', type: 'string', value: 'fakeAccessToken' } + { name: 'catalog_item_id', type: 'string', value: 'fakeId' } ] }) req.flush({}); @@ -292,8 +291,7 @@ describe('ProductActionScreenComponent', () => { expect(req.request.body).toEqual({ id: 'fakeAction', parameters: [ - { name: 'catalog_item_id', type: 'string', value: 'fakeId' }, - { name: 'access_token', type: 'string', value: 'fakeAccessToken' } + { name: 'catalog_item_id', type: 'string', value: 'fakeId' } ], }) req.flush({}); @@ -325,8 +323,7 @@ describe('ProductActionScreenComponent', () => { expect(req.request.body).toEqual({ id: 'fakeAction', parameters: [ - { name: 'catalog_item_id', type: 'string', value: 'fakeId' }, - { name: 'access_token', type: 'string', value: 'fakeAccessToken' } + { name: 'catalog_item_id', type: 'string', value: 'fakeId' } ], }) req.flush({}); @@ -366,8 +363,7 @@ describe('ProductActionScreenComponent', () => { parameters: [ { name: 'param_1', type: 'string', value: 'value1' }, { name: 'param_2', type: 'string', value: 'value2' }, - { name: 'catalog_item_id', type: 'string', value: 'fakeId' }, - { name: 'access_token', type: 'string', value: 'fakeAccessToken' } + { name: 'catalog_item_id', type: 'string', value: 'fakeId' } ], }); @@ -413,8 +409,7 @@ describe('ProductActionScreenComponent', () => { id: 'fakeAction', parameters: [ { name: 'param_1', type: 'string', value: 'value1' }, - { name: 'catalog_item_id', type: 'string', value: 'fakeId' }, - { name: 'access_token', type: 'string', value: 'fakeAccessToken' }, + { name: 'catalog_item_id', type: 'string', value: 'fakeId' } ], }); diff --git a/src/app/screens/product-action-screen/product-action-screen.component.ts b/src/app/screens/product-action-screen/product-action-screen.component.ts index ada5ad2..dd83248 100644 --- a/src/app/screens/product-action-screen/product-action-screen.component.ts +++ b/src/app/screens/product-action-screen/product-action-screen.component.ts @@ -17,7 +17,7 @@ import { ProductActionParameterValidation } from '../../models/product-action-pa import { MatOptionModule } from '@angular/material/core'; import { MatSelectModule } from '@angular/material/select'; import { ProjectService } from '../../services/project.service'; -import { Subject, switchMap, takeUntil } from 'rxjs'; +import { Subject, takeUntil } from 'rxjs'; import { AppProject } from '../../models/project'; import { AzureService } from '../../services/azure.service'; import { AppUser } from '../../models/app-user'; @@ -358,32 +358,23 @@ export class ProductActionScreenComponent implements OnInit, OnDestroy { if (this.formGroup.valid && this.action.url && this.selectedProject) { this.isExecutingAction = true; const actionUrl = this.action.url; - this.azureService.getRefreshedAccessToken().pipe( - switchMap((refreshedAccessToken: string) => { - const actionBody = { - id: this.action.id, - parameters: this.actionParams.map(param => ({ - name: param.name, - type: param.type, - value: this.formGroup.getRawValue()[param.name] || this.getParamDefaultValue(param) || '' - })), - }; - actionBody.parameters.push( - { - name: 'catalog_item_id', - type: 'string', - value: this.product.id - }, - { - name: 'access_token', // ToDo: when using access token in headers, this parameter is not needed and the provisioner should retrieve it and add the param. - type: 'string', - value: refreshedAccessToken - } - ); - this.formGroup.disable(); - return this.http.post(actionUrl, actionBody); - }) - ).subscribe({ + const actionBody = { + id: this.action.id, + parameters: this.actionParams.map(param => ({ + name: param.name, + type: param.type, + value: this.formGroup.getRawValue()[param.name] || this.getParamDefaultValue(param) || '' + })), + }; + actionBody.parameters.push( + { + name: 'catalog_item_id', + type: 'string', + value: this.product.id + } + ); + this.formGroup.disable(); + this.http.post(actionUrl, actionBody).subscribe({ next: () => { if(this.action.triggerMessage && this.action.triggerMessage !== '') { this.toastService.showToast({ diff --git a/src/app/screens/project-components-screen/project-components-screen.component.spec.ts b/src/app/screens/project-components-screen/project-components-screen.component.spec.ts index 6d64a9e..be64c40 100644 --- a/src/app/screens/project-components-screen/project-components-screen.component.spec.ts +++ b/src/app/screens/project-components-screen/project-components-screen.component.spec.ts @@ -179,8 +179,7 @@ describe('ProjectComponentsScreenComponent', () => { autoFocus: false, data: { componentName: 'test-component', - projectKey: 'PROJECT_1', - location: 'LOC_1' + projectKey: 'PROJECT_1' } }); }); @@ -202,8 +201,7 @@ describe('ProjectComponentsScreenComponent', () => { autoFocus: false, data: { componentName: 'another-test-component', - projectKey: 'PROJECT_2', - location: 'LOC_2' + projectKey: 'PROJECT_2' } }); }); @@ -229,8 +227,7 @@ describe('ProjectComponentsScreenComponent', () => { changeNumber: 'CHG1234567', reason: 'Test reason', projectKey: 'PROJECT_1', - componentName: 'test-component', - location: 'LOC_1' + componentName: 'test-component' }; spyOn(component.dialog, 'open').and.returnValue({ afterClosed: () => of(mockResult) @@ -239,12 +236,9 @@ describe('ProjectComponentsScreenComponent', () => { component.onRequestDeletionClicked(testComponent); expect(component.projectComponents[0].status).toBe('DELETING'); const incidentParams: CreateIncidentParameter[] = [ - { name: 'cluster_location', type: 'string', value: 'LOC_1' as String }, - { name: 'caller', type: 'string', value: 'test-user' as String }, { name: 'is_deployed', type: 'boolean', value: true as Boolean }, { name: 'change_number', type: 'string', value: 'CHG1234567' as String }, - { name: 'reason', type: 'string', value: 'Test reason' as String }, - { name: 'access_token', type: 'string', value: 'test-access-token' as String } + { name: 'reason', type: 'string', value: 'Test reason' as String } ] expect(provisionerServiceSpy.requestComponentDeletion).toHaveBeenCalledWith( 'PROJECT_1', @@ -280,8 +274,7 @@ describe('ProjectComponentsScreenComponent', () => { changeNumber: 'CHG1234567', reason: 'Test reason', projectKey: 'PROJECT_1', - componentName: 'test-component', - location: 'LOC_1' + componentName: 'test-component' }; spyOn(component.dialog, 'open').and.returnValue({ afterClosed: () => of(mockResult) @@ -289,12 +282,9 @@ describe('ProjectComponentsScreenComponent', () => { provisionerServiceSpy.requestComponentDeletion.and.returnValue(throwError(() => new Error('Deletion failed'))); component.onRequestDeletionClicked(testComponent); const incidentParams: CreateIncidentParameter[] = [ - { name: 'cluster_location', type: 'string', value: 'LOC_1' as String }, - { name: 'caller', type: 'string', value: 'test-user' as String }, { name: 'is_deployed', type: 'boolean', value: true as Boolean }, { name: 'change_number', type: 'string', value: 'CHG1234567' as String }, - { name: 'reason', type: 'string', value: 'Test reason' as String }, - { name: 'access_token', type: 'string', value: 'test-access-token' as String } + { name: 'reason', type: 'string', value: 'Test reason' as String } ] expect(provisionerServiceSpy.requestComponentDeletion).toHaveBeenCalledWith( 'PROJECT_1', @@ -330,8 +320,7 @@ describe('ProjectComponentsScreenComponent', () => { changeNumber: 'CHG1234567', reason: 'Test reason', projectKey: 'PROJECT_1', - componentName: 'test-component', - location: 'LOC_1' + componentName: 'test-component' }; spyOn(component.dialog, 'open').and.returnValue({ afterClosed: () => of(mockResult) @@ -339,12 +328,9 @@ describe('ProjectComponentsScreenComponent', () => { component.loggedUser = null; component.onRequestDeletionClicked(testComponent); const incidentParams: CreateIncidentParameter[] = [ - { name: 'cluster_location', type: 'string', value: 'LOC_1' as String }, - { name: 'caller', type: 'string', value: 'unknown' as String }, { name: 'is_deployed', type: 'boolean', value: true as Boolean }, { name: 'change_number', type: 'string', value: 'CHG1234567' as String }, - { name: 'reason', type: 'string', value: 'Test reason' as String }, - { name: 'access_token', type: 'string', value: 'test-access-token' as String } + { name: 'reason', type: 'string', value: 'Test reason' as String } ] expect(provisionerServiceSpy.requestComponentDeletion).toHaveBeenCalledWith( 'PROJECT_1', diff --git a/src/app/screens/project-components-screen/project-components-screen.component.ts b/src/app/screens/project-components-screen/project-components-screen.component.ts index 7b5c4f8..d79d2fe 100644 --- a/src/app/screens/project-components-screen/project-components-screen.component.ts +++ b/src/app/screens/project-components-screen/project-components-screen.component.ts @@ -128,7 +128,6 @@ export class ProjectComponentsScreenComponent implements OnInit, OnDestroy { data: { componentName: component.name, projectKey: this.selectedProject.projectKey, - location: this.selectedProject.location } }); @@ -147,56 +146,37 @@ export class ProjectComponentsScreenComponent implements OnInit, OnDestroy { originalStatus = this.projectComponents[componentIndex].status; this.projectComponents[componentIndex].status = 'DELETING'; } - this.azureService.getRefreshedAccessToken().subscribe({ - next: (accessToken) => { - /* eslint-disable @typescript-eslint/no-wrapper-object-types */ - const incidentParams: CreateIncidentParameter[] = [ - { - name: 'cluster_location', - type: 'string', - value: result.location as String // NOSONAR - }, - { - name: 'caller', - type: 'string', - value: this.loggedUser?.username as String || 'unknown' // NOSONAR - }, - { - name: 'is_deployed', - type: 'boolean', - value: result.deploymentStatus as Boolean // NOSONAR - - }, - { - name: 'change_number', - type: 'string', - value: result.changeNumber as String // NOSONAR - }, - { - name: 'reason', - type: 'string', - value: result.reason as String // NOSONAR - }, - { - name: 'access_token', - type: 'string', - value: accessToken as String // NOSONAR - }, - ]; - /* eslint-enable @typescript-eslint/no-wrapper-object-types */ - this.provisionerService.requestComponentDeletion( - result.projectKey, - result.componentName, - incidentParams - ).subscribe({ - next: () => this.onDeletionRequestSuccess(), - error: (error) => { - this.onDeletionRequestError(error) - if (originalStatus && componentIndex !== -1) { - this.projectComponents[componentIndex].status = originalStatus; - } - } - }); + /* eslint-disable @typescript-eslint/no-wrapper-object-types */ + const incidentParams: CreateIncidentParameter[] = [ + { + name: 'is_deployed', + type: 'boolean', + value: result.deploymentStatus as Boolean // NOSONAR + + }, + { + name: 'change_number', + type: 'string', + value: result.changeNumber as String // NOSONAR + }, + { + name: 'reason', + type: 'string', + value: result.reason as String // NOSONAR + } + ]; + /* eslint-enable @typescript-eslint/no-wrapper-object-types */ + this.provisionerService.requestComponentDeletion( + result.projectKey, + result.componentName, + incidentParams + ).subscribe({ + next: () => this.onDeletionRequestSuccess(), + error: (error) => { + this.onDeletionRequestError(error) + if (originalStatus && componentIndex !== -1) { + this.projectComponents[componentIndex].status = originalStatus; + } } }); } diff --git a/src/app/services/azure.service.spec.ts b/src/app/services/azure.service.spec.ts index 1d77a0e..a8cee00 100644 --- a/src/app/services/azure.service.spec.ts +++ b/src/app/services/azure.service.spec.ts @@ -4,6 +4,7 @@ import { MSAL_GUARD_CONFIG, MsalBroadcastService, MsalGuardConfiguration, MsalSe import { BehaviorSubject, of, Subject } from 'rxjs'; import { AuthenticationResult, EventMessage, EventType, InteractionStatus, InteractionType, RedirectRequest } from '@azure/msal-browser'; import { Router } from '@angular/router'; +import { AppConfigService } from './app-config.service'; const destroyingMethodName = '_destroying$'; const fakeToken = 'test-token'; @@ -16,6 +17,7 @@ describe('AzureService', () => { let inProgress$: Subject; const msalInstanceSpy = jasmine.createSpyObj('instance', ['enableAccountStorageEvents', 'getAllAccounts', 'getActiveAccount', 'setActiveAccount', 'acquireTokenSilent']); const mockRouter: jasmine.SpyObj = jasmine.createSpyObj('Router', ['navigate']);; + const appConfigServiceSpy = jasmine.createSpyObj('AppConfigService', ['getConfig']); beforeEach(() => { msalSubject$ = new Subject(); @@ -26,6 +28,7 @@ describe('AzureService', () => { msalSubject$: msalSubject$.asObservable(), inProgress$: inProgress$.asObservable() }); + appConfigServiceSpy.getConfig.and.returnValue({ apiConfig: { scopes: ['User.Read'] } }); TestBed.configureTestingModule({ providers: [ @@ -33,7 +36,8 @@ describe('AzureService', () => { { provide: MSAL_GUARD_CONFIG, useValue: msalGuardConfig }, { provide: MsalService, useValue: msalServiceSpy }, { provide: MsalBroadcastService, useValue: msalBroadcastServiceSpy }, - { provide: Router, useValue: mockRouter } + { provide: Router, useValue: mockRouter }, + { provide: AppConfigService, useValue: appConfigServiceSpy } ] }); @@ -41,7 +45,11 @@ describe('AzureService', () => { msalService = TestBed.inject(MsalService) as jasmine.SpyObj; msalInstanceSpy.getAllAccounts.and.returnValue([{}]); msalInstanceSpy.getActiveAccount.and.returnValue(null); + msalInstanceSpy.acquireTokenSilent.calls.reset(); msalService.instance = msalInstanceSpy; + // Clear the token cache so each test starts fresh + (service as any).cachedAccessToken = null; + (service as any).tokenExpiresOn = null; msalGuardConfig.interactionType = InteractionType.Redirect; window.onbeforeunload = () => "Oh no!"; // Prevent page reloads during tests @@ -64,15 +72,13 @@ describe('AzureService', () => { }); it('initialize method works', () => { - msalService.handleRedirectObservable.and.returnValue(of({} as AuthenticationResult)); service.initialize(); msalSubject$.next({eventType: EventType.ACCOUNT_ADDED} as EventMessage); inProgress$.next(InteractionStatus.None); - expect(msalService.handleRedirectObservable).toHaveBeenCalled(); + expect(msalService.instance.enableAccountStorageEvents).toHaveBeenCalled(); }); it('initialize - should set window.location.pathname to "/" when all accounts are removed', fakeAsync(() => { - msalService.handleRedirectObservable.and.returnValue(of({} as AuthenticationResult)); msalInstanceSpy.getAllAccounts.and.returnValue([]); service.initialize(); msalSubject$.next({ eventType: EventType.ACCOUNT_REMOVED } as EventMessage); @@ -136,7 +142,7 @@ describe('AzureService', () => { msalInstanceSpy.getActiveAccount.and.returnValue(msalUser); msalInstanceSpy.acquireTokenSilent.and.returnValue(Promise.resolve({ accessToken: fakeToken })); - spyOn(window, 'fetch').and.returnValue(Promise.reject(new Error('Error fetching profile picture'))); + spyOn(window, 'fetch').and.callFake(() => Promise.reject(new Error('Error fetching profile picture'))); service.refreshLoggedUser(); @@ -180,9 +186,15 @@ describe('AzureService', () => { expect(msalService.loginRedirect).toHaveBeenCalledWith(); }); - it('logout - should call logout on msalService', () => { + 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); + service.logout(); + expect(msalService.logout).toHaveBeenCalled(); + expect((service as any).cachedAccessToken).toBeNull(); + expect((service as any).tokenExpiresOn).toBeNull(); }); it('checkAndSetActiveAccount - should set the first account as active if no active account is set', () => { @@ -244,56 +256,10 @@ describe('AzureService', () => { expect(service[destroyingMethodName].complete).toHaveBeenCalled(); }); - it('getIdToken - should return the idToken of the active account', () => { - const activeAccount = { idToken: 'test-id-token' }; - msalInstanceSpy.getActiveAccount.and.returnValue(activeAccount); - - const token = service.getIdToken(); - - expect(token).toBe('test-id-token'); - }); - - it('getIdToken - should return an empty string if no active account', () => { - msalInstanceSpy.getActiveAccount.and.returnValue(null); - - const token = service.getIdToken(); - - expect(token).toBe(''); - }); - - it('refreshToken - should call acquireTokenSilent with correct scopes', () => { - const acquireTokenSilentSpy = msalInstanceSpy.acquireTokenSilent.and.returnValue(Promise.resolve({ accessToken: fakeToken })); - - service.refreshToken(); - - expect(acquireTokenSilentSpy).toHaveBeenCalledWith({ scopes: ["User.Read"] }); - }); - - it('refreshToken - should return a promise that resolves with the access token', async () => { - const expectedToken = { accessToken: fakeToken } as AuthenticationResult; - msalInstanceSpy.acquireTokenSilent.and.returnValue(Promise.resolve(expectedToken)); - - const result = await service.refreshToken(); - - expect(result).toEqual(expectedToken); - }); - - it('refreshToken - should return a promise that rejects with an error', async () => { - const expectedError = new Error('Error acquiring token silently'); - msalInstanceSpy.acquireTokenSilent.and.returnValue(Promise.reject(expectedError)); - - try { - await service.refreshToken(); - fail('Expected promise to be rejected'); - } catch (error) { - expect(error).toEqual(expectedError); - } - }); - it('getRefreshedAccessToken - should return an observable that emits the access token', (done) => { const expectedToken = 'test-access-token'; - const authResult = { accessToken: expectedToken } as AuthenticationResult; - msalInstanceSpy.acquireTokenSilent.and.returnValue(Promise.resolve(authResult)); + const expiresOn = new Date(Date.now() + 10 * 60 * 1000); + msalInstanceSpy.acquireTokenSilent.and.returnValue(Promise.resolve({ accessToken: expectedToken, expiresOn })); service.getRefreshedAccessToken().subscribe((token) => { expect(token).toEqual(expectedToken); @@ -313,6 +279,85 @@ describe('AzureService', () => { } }); }); - -}); + describe('getAccessToken', () => { + it('should call acquireTokenSilent and return access token', async () => { + const expectedToken = 'test-access-token'; + const expiresOn = new Date(Date.now() + 10 * 60 * 1000); + msalInstanceSpy.acquireTokenSilent.and.returnValue(Promise.resolve({ accessToken: expectedToken, expiresOn })); + + const token = await service.getAccessToken(); + + expect(token).toBe(expectedToken); + expect(msalInstanceSpy.acquireTokenSilent).toHaveBeenCalledWith(jasmine.objectContaining({ scopes: ['User.Read'] })); + }); + + it('should return cached token if still valid', async () => { + const expectedToken = 'cached-token'; + const expiresOn = new Date(Date.now() + 10 * 60 * 1000); + msalInstanceSpy.acquireTokenSilent.and.returnValue(Promise.resolve({ accessToken: expectedToken, expiresOn })); + + await service.getAccessToken(); // Populate cache + msalInstanceSpy.acquireTokenSilent.calls.reset(); + + const token = await service.getAccessToken(); // Should use cache + + expect(token).toBe(expectedToken); + expect(msalInstanceSpy.acquireTokenSilent).not.toHaveBeenCalled(); + }); + + it('should call acquireTokenSilent when forceRefresh is true even if cached', async () => { + const initialToken = 'initial-token'; + const refreshedToken = 'refreshed-token'; + const expiresOn = new Date(Date.now() + 10 * 60 * 1000); + msalInstanceSpy.acquireTokenSilent.and.returnValues( + Promise.resolve({ accessToken: initialToken, expiresOn }), + Promise.resolve({ accessToken: refreshedToken, expiresOn }) + ); + + await service.getAccessToken(); // Populate cache + const token = await service.getAccessToken(true); // Force refresh + + expect(token).toBe(refreshedToken); + expect(msalInstanceSpy.acquireTokenSilent).toHaveBeenCalledTimes(2); + }); + + it('should call acquireTokenSilent when cached token is within expiry buffer', async () => { + const expiredToken = 'near-expiry-token'; + const newToken = 'new-token'; + // Expires in 4 minutes — within the 5-minute buffer, so should be treated as expired + const nearExpiryDate = new Date(Date.now() + 4 * 60 * 1000); + const validDate = new Date(Date.now() + 10 * 60 * 1000); + msalInstanceSpy.acquireTokenSilent.and.returnValues( + Promise.resolve({ accessToken: expiredToken, expiresOn: nearExpiryDate }), + Promise.resolve({ accessToken: newToken, expiresOn: validDate }) + ); + + await service.getAccessToken(); // Populate cache with near-expiry token + const token = await service.getAccessToken(); // Should fetch a new token + + expect(token).toBe(newToken); + expect(msalInstanceSpy.acquireTokenSilent).toHaveBeenCalledTimes(2); + }); + + it('should throw an error when there is no active account and no accounts are available', async () => { + msalInstanceSpy.getActiveAccount.and.returnValue(null); + msalInstanceSpy.getAllAccounts.and.returnValue([]); + + await expectAsync(service.getAccessToken()).toBeRejectedWithError('No accounts found. User must sign in first.'); + }); + + it('should fall back to empty scopes when getConfig returns null', async () => { + appConfigServiceSpy.getConfig.and.returnValue(null); + const expectedToken = 'test-token'; + const expiresOn = new Date(Date.now() + 10 * 60 * 1000); + msalInstanceSpy.acquireTokenSilent.and.returnValue(Promise.resolve({ accessToken: expectedToken, expiresOn })); + + const token = await service.getAccessToken(); + + expect(token).toBe(expectedToken); + expect(msalInstanceSpy.acquireTokenSilent).toHaveBeenCalledWith(jasmine.objectContaining({ scopes: [] })); + }); + }); + +}); diff --git a/src/app/services/azure.service.ts b/src/app/services/azure.service.ts index 38a6a3c..95ce42b 100644 --- a/src/app/services/azure.service.ts +++ b/src/app/services/azure.service.ts @@ -1,9 +1,10 @@ import { Inject, Injectable, OnDestroy } from "@angular/core"; import { MSAL_GUARD_CONFIG, MsalBroadcastService, MsalGuardConfiguration, MsalService } from "@azure/msal-angular"; -import { AuthenticationResult, EventMessage, EventType, InteractionStatus, RedirectRequest } from "@azure/msal-browser"; -import { BehaviorSubject, filter, from, map, Observable, Subject, takeUntil } from "rxjs"; +import { EventMessage, EventType, InteractionStatus, RedirectRequest } from "@azure/msal-browser"; +import { BehaviorSubject, filter, from, Observable, Subject, takeUntil } from "rxjs"; import { Router } from "@angular/router"; import { AppUser } from "../models/app-user"; +import { AppConfigService } from "./app-config.service"; @Injectable({ providedIn: 'root' @@ -12,6 +13,9 @@ export class AzureService implements OnDestroy { isIframe = false; loginDisplay = false; private readonly _destroying$ = new Subject(); + private cachedAccessToken: string | null = null; + private tokenExpiresOn: Date | null = null; + private static readonly TOKEN_EXPIRY_BUFFER_MS = 5 * 60 * 1000; // 5 minutes isFirstTime = true; loggedUser$ = new BehaviorSubject(null); @@ -20,11 +24,11 @@ export class AzureService implements OnDestroy { @Inject(MSAL_GUARD_CONFIG) private readonly msalGuardConfig: MsalGuardConfiguration, private readonly msalService: MsalService, private readonly msalBroadcastService: MsalBroadcastService, - private readonly router: Router + private readonly router: Router, + private readonly appConfigService: AppConfigService ) {} initialize() { - this.msalService.handleRedirectObservable().subscribe(); this.isIframe = window !== window.parent && !window.opener; // NOSONAR - Remove this line to use Angular Universal this.setLoginDisplay(); @@ -55,24 +59,37 @@ export class AzureService implements OnDestroy { this.setLoginDisplay(); this.checkAndSetActiveAccount(); }); + + this.checkAndSetActiveAccount(); } setLoginDisplay() { this.loginDisplay = this.msalService.instance.getAllAccounts().length > 0; } - getIdToken(): string { - return this.msalService.instance.getActiveAccount()?.idToken ?? ''; - } - - refreshToken(): Promise { - return this.msalService.instance.acquireTokenSilent({scopes: ["User.Read"]}); + async getAccessToken(forceRefresh = false): Promise { + const now = new Date(Date.now() + AzureService.TOKEN_EXPIRY_BUFFER_MS); + if (!forceRefresh && this.cachedAccessToken && this.tokenExpiresOn && this.tokenExpiresOn > now) { + return this.cachedAccessToken; + } + const scopes = this.appConfigService.getConfig()?.apiConfig?.scopes ?? []; + let account = this.msalService.instance.getActiveAccount(); + if (!account) { + const accounts = this.msalService.instance.getAllAccounts(); + if (accounts.length === 0) { + throw new Error('No accounts found. User must sign in first.'); + } + account = accounts[0]; + this.msalService.instance.setActiveAccount(account); + } + const result = await this.msalService.instance.acquireTokenSilent({ scopes, account }); + this.cachedAccessToken = result.accessToken; + this.tokenExpiresOn = result.expiresOn; + return this.cachedAccessToken; } getRefreshedAccessToken(): Observable { - return from(this.refreshToken()).pipe( - map((azureData: AuthenticationResult) => azureData.accessToken) - ); + return from(this.getAccessToken(true)); } refreshLoggedUser(): void { @@ -153,6 +170,8 @@ export class AzureService implements OnDestroy { } logout() { + this.cachedAccessToken = null; + this.tokenExpiresOn = null; this.msalService.logout(); } diff --git a/src/app/services/project.service.spec.ts b/src/app/services/project.service.spec.ts index be12769..aa5cd5a 100644 --- a/src/app/services/project.service.spec.ts +++ b/src/app/services/project.service.spec.ts @@ -4,7 +4,6 @@ import { provideHttpClient } from '@angular/common/http'; import { BASE_PATH, ProjectInfo, ProjectsService } from '../openapi/projects-info-service'; import { ProjectComponentsService } from '../openapi/component-catalog'; import { AzureService } from './azure.service'; -import { AuthenticationResult } from "@azure/msal-browser"; import { Observable, of, throwError } from 'rxjs'; import { CatalogService } from './catalog.service'; @@ -18,7 +17,7 @@ describe('ProjectService', () => { beforeEach(() => { localStorage.clear(); projectsServiceSpy = jasmine.createSpyObj('ProjectsService', ['getProjects']); - azureServiceSpy = jasmine.createSpyObj('AzureService', ['getAzureProjects', 'refreshToken']); + azureServiceSpy = jasmine.createSpyObj('AzureService', ['getAccessToken']); projectComponentsServiceSpy = jasmine.createSpyObj('ProjectComponentsService', ['listProjectComponents']); catalogServiceSpy = jasmine.createSpyObj('CatalogService', ['getProductImage']); @@ -33,7 +32,7 @@ describe('ProjectService', () => { ] }); - azureServiceSpy.refreshToken.and.returnValue(Promise.resolve({ accessToken: 'dummy-token' } as AuthenticationResult)); + azureServiceSpy.getAccessToken.and.returnValue(Promise.resolve('dummy-token')); catalogServiceSpy.getProductImage.and.returnValue(Promise.resolve('http://example.com/image.png')); service = TestBed.inject(ProjectService); @@ -115,7 +114,7 @@ describe('ProjectService', () => { const storedProject = JSON.parse(localStorage.getItem('selectedProject')!); expect(storedProject).toEqual({ projectKey: 'project1', location: 'cluster1' }); expect(service.getCurrentProject()).toEqual({ projectKey: 'project1', location: 'cluster1' }); - expect(azureServiceSpy.refreshToken).toHaveBeenCalled(); + expect(azureServiceSpy.getAccessToken).toHaveBeenCalled(); done(); }, 100); }); @@ -154,17 +153,17 @@ describe('ProjectService', () => { service.setCurrentProject('project1'); }); - it('ensureUserProjectsLoaded should return cached value and not call refreshToken', (done) => { + it('ensureUserProjectsLoaded should return cached value and not call getAccessToken', (done) => { const mockProjects: string[] = ['project1', 'project2']; projectsServiceSpy.getProjects.and.returnValue(of(mockProjects as any) as any); service.getUserProjects('test-token').subscribe(() => { projectsServiceSpy.getProjects.calls.reset(); - azureServiceSpy.refreshToken.calls.reset(); + azureServiceSpy.getAccessToken.calls.reset(); service.ensureUserProjectsLoaded().subscribe(projects => { expect(projects).toEqual(mockProjects); - expect(azureServiceSpy.refreshToken).not.toHaveBeenCalled(); + expect(azureServiceSpy.getAccessToken).not.toHaveBeenCalled(); expect(projectsServiceSpy.getProjects).not.toHaveBeenCalled(); done(); }); @@ -178,21 +177,21 @@ describe('ProjectService', () => { resolveToken = resolve; }); - azureServiceSpy.refreshToken.and.returnValue(tokenPromise as any); + azureServiceSpy.getAccessToken.and.returnValue(tokenPromise as any); projectsServiceSpy.getProjects.and.returnValue(of(mockProjects as any) as any); const request1$ = service.ensureUserProjectsLoaded(); const request2$ = service.ensureUserProjectsLoaded(); expect(request2$).toBe(request1$); - expect(azureServiceSpy.refreshToken).toHaveBeenCalledTimes(1); + expect(azureServiceSpy.getAccessToken).toHaveBeenCalledTimes(1); let result1: string[] | undefined; let result2: string[] | undefined; request1$.subscribe(r => (result1 = r)); request2$.subscribe(r => (result2 = r)); - resolveToken!({ accessToken: 'dummy-token' }); + resolveToken!('dummy-token'); flushMicrotasks(); expect(projectsServiceSpy.getProjects).toHaveBeenCalledTimes(1); @@ -204,7 +203,7 @@ describe('ProjectService', () => { it('ensureUserProjectsLoaded should load from backend when cache is empty and populate cache', fakeAsync(() => { const mockProjects: string[] = ['project1', 'project2']; - azureServiceSpy.refreshToken.and.returnValue(Promise.resolve({ accessToken: 'dummy-token' } as any)); + azureServiceSpy.getAccessToken.and.returnValue(Promise.resolve('dummy-token')); projectsServiceSpy.getProjects.and.returnValue(of(mockProjects as any) as any); let result: string[] | undefined; @@ -213,19 +212,19 @@ describe('ProjectService', () => { expect(result).toEqual(mockProjects); expect(service.getCachedUserProjects()).toEqual(mockProjects); - expect(azureServiceSpy.refreshToken).toHaveBeenCalledTimes(1); + expect(azureServiceSpy.getAccessToken).toHaveBeenCalledTimes(1); expect(projectsServiceSpy.getProjects).toHaveBeenCalledWith('dummy-token'); - azureServiceSpy.refreshToken.calls.reset(); + azureServiceSpy.getAccessToken.calls.reset(); projectsServiceSpy.getProjects.calls.reset(); service.ensureUserProjectsLoaded().subscribe(); - expect(azureServiceSpy.refreshToken).not.toHaveBeenCalled(); + expect(azureServiceSpy.getAccessToken).not.toHaveBeenCalled(); expect(projectsServiceSpy.getProjects).not.toHaveBeenCalled(); })); it('ensureUserProjectsLoaded should reset in-flight request on error (finalize) and allow retry', fakeAsync(() => { - azureServiceSpy.refreshToken.and.returnValue(Promise.resolve({ accessToken: 'dummy-token' } as any)); + azureServiceSpy.getAccessToken.and.returnValue(Promise.resolve('dummy-token')); projectsServiceSpy.getProjects.and.returnValue(throwError(() => new Error('backend error'))); let firstError: any; @@ -242,7 +241,7 @@ describe('ProjectService', () => { service.ensureUserProjectsLoaded().subscribe(r => (retryResult = r)); flushMicrotasks(); - expect(azureServiceSpy.refreshToken).toHaveBeenCalledTimes(2); + expect(azureServiceSpy.getAccessToken).toHaveBeenCalledTimes(2); expect(retryResult).toEqual(['project1']); expect(service.getCachedUserProjects()).toEqual(['project1']); })); @@ -258,9 +257,9 @@ describe('ProjectService', () => { }); }); - azureServiceSpy.refreshToken.and.returnValues( - Promise.resolve({ accessToken: 't1' } as any), - Promise.resolve({ accessToken: 't2' } as any) + azureServiceSpy.getAccessToken.and.returnValues( + Promise.resolve('t1'), + Promise.resolve('t2') ); service.setCurrentProject('project1'); @@ -285,7 +284,7 @@ describe('ProjectService', () => { expect(setItemValues.some(v => v.includes('"projectKey":"project2"'))).toBeTrue(); })); - it('should ignore stale refreshToken resolution when setCurrentProject is called again (requestId guard before getProjectClusters)', fakeAsync(() => { + it('should ignore stale getAccessToken resolution when setCurrentProject is called again (requestId guard before getProjectClusters)', fakeAsync(() => { spyOn(localStorage, 'setItem').and.callThrough(); let resolveToken1: ((value: any) => void) | undefined; @@ -294,7 +293,7 @@ describe('ProjectService', () => { const tokenPromise1 = new Promise(resolve => (resolveToken1 = resolve)); const tokenPromise2 = new Promise(resolve => (resolveToken2 = resolve)); - azureServiceSpy.refreshToken.and.returnValues(tokenPromise1 as any, tokenPromise2 as any); + azureServiceSpy.getAccessToken.and.returnValues(tokenPromise1 as any, tokenPromise2 as any); projectsServiceSpy.getProjectClusters = jasmine.createSpy().and.callFake((token: string, projectKey: string) => { return of({ projectKey, clusters: [`${projectKey}-cluster`] } as any); @@ -304,7 +303,7 @@ describe('ProjectService', () => { service.setCurrentProject('project2'); // Resolve the second token first; it should win. - resolveToken2!({ accessToken: 't2' }); + resolveToken2!('t2'); flushMicrotasks(); expect(projectsServiceSpy.getProjectClusters).toHaveBeenCalledTimes(1); @@ -317,7 +316,7 @@ describe('ProjectService', () => { }); // Now resolve the first token after it's stale; it should be ignored entirely (no backend call, no storage update). - resolveToken1!({ accessToken: 't1' }); + resolveToken1!('t1'); flushMicrotasks(); expect(projectsServiceSpy.getProjectClusters).toHaveBeenCalledTimes(1); @@ -346,7 +345,7 @@ describe('ProjectService', () => { } ]; - azureServiceSpy.refreshToken.and.returnValue(Promise.resolve({ accessToken: 'test-token' } as AuthenticationResult)); + azureServiceSpy.getAccessToken.and.returnValue(Promise.resolve('test-token')); projectComponentsServiceSpy.getProjectComponents = jasmine.createSpy().and.returnValue(of(mockComponents as any)); let result: any; @@ -356,7 +355,7 @@ describe('ProjectService', () => { flushMicrotasks(); - expect(azureServiceSpy.refreshToken).toHaveBeenCalled(); + expect(azureServiceSpy.getAccessToken).toHaveBeenCalled(); expect(projectComponentsServiceSpy.getProjectComponents).toHaveBeenCalledWith('PROJECT_1', 'test-token'); expect(result).toEqual([ { @@ -397,7 +396,7 @@ describe('ProjectService', () => { } ]; - azureServiceSpy.refreshToken.and.returnValue(Promise.resolve({ accessToken: 'test-token' } as AuthenticationResult)); + azureServiceSpy.getAccessToken.and.returnValue(Promise.resolve('test-token')); projectComponentsServiceSpy.getProjectComponents = jasmine.createSpy().and.returnValue(of(mockComponentsWithMissingData as any)); let result: any; @@ -407,7 +406,7 @@ describe('ProjectService', () => { flushMicrotasks(); - expect(azureServiceSpy.refreshToken).toHaveBeenCalled(); + expect(azureServiceSpy.getAccessToken).toHaveBeenCalled(); expect(projectComponentsServiceSpy.getProjectComponents).toHaveBeenCalledWith('PROJECT_1', 'test-token'); expect(result).toEqual([ { @@ -452,7 +451,7 @@ describe('ProjectService', () => { } ]; - azureServiceSpy.refreshToken.and.returnValue(Promise.resolve({ accessToken: 'test-token' } as AuthenticationResult)); + azureServiceSpy.getAccessToken.and.returnValue(Promise.resolve('test-token')); projectComponentsServiceSpy.getProjectComponents = jasmine.createSpy().and.returnValue(of(mockComponents as any)); catalogServiceSpy.getProductImage.and.returnValue(Promise.resolve(undefined)); diff --git a/src/app/services/project.service.ts b/src/app/services/project.service.ts index b442572..2daba67 100644 --- a/src/app/services/project.service.ts +++ b/src/app/services/project.service.ts @@ -7,7 +7,6 @@ import { ProjectComponentsService } from '../openapi/component-catalog'; import { ProjectComponent } from '../models/project-component'; import { ComponentStatus } from '../models/component-status'; import { CatalogService } from './catalog.service'; -import { AuthenticationResult } from "@azure/msal-browser"; @Injectable({ providedIn: 'root' @@ -59,8 +58,8 @@ export class ProjectService { return this.userProjectsRequest$; } - this.userProjectsRequest$ = from(this.azureService.refreshToken()).pipe( - switchMap((azureData: AuthenticationResult) => this.getUserProjects(azureData.accessToken)), + this.userProjectsRequest$ = from(this.azureService.getAccessToken()).pipe( + switchMap((accessToken: string) => this.getUserProjects(accessToken)), finalize(() => { this.userProjectsRequest$ = null; }), @@ -83,11 +82,11 @@ export class ProjectService { setCurrentProject(projectKey: string | null): void { const requestId = ++this.setProjectRequestId; if (projectKey) { - this.azureService.refreshToken().then((azureData: AuthenticationResult) => { + this.azureService.getAccessToken().then((accessToken: string) => { if (requestId !== this.setProjectRequestId) { return; } - this.getProjectCluster(projectKey, azureData.accessToken).subscribe(cluster => { + this.getProjectCluster(projectKey, accessToken).subscribe(cluster => { if (requestId !== this.setProjectRequestId) { return; } @@ -103,9 +102,9 @@ export class ProjectService { } getProjectComponents(projectKey: string): Observable { - return from(this.azureService.refreshToken()).pipe( - switchMap((azureData: AuthenticationResult) => - this.projectComponentsService.getProjectComponents(projectKey, azureData.accessToken).pipe( + return from(this.azureService.getAccessToken()).pipe( + switchMap((accessToken: string) => + this.projectComponentsService.getProjectComponents(projectKey, accessToken).pipe( switchMap(components => from(Promise.all(components.map(async component => ({ name: component.componentId || '', diff --git a/src/app/services/provisioner.service.spec.ts b/src/app/services/provisioner.service.spec.ts index 165ceb0..7fa85c0 100644 --- a/src/app/services/provisioner.service.spec.ts +++ b/src/app/services/provisioner.service.spec.ts @@ -47,15 +47,13 @@ describe('ProvisionerService', () => { const reason = 'No longer needed'; const deploymentStatus = false; const changeNumber = '-'; - const accessToken = 'test-access-token'; const incidentParams: CreateIncidentParameter[] = [ { name: 'cluster_location', type: 'string', value: location as String }, { name: 'caller', type: 'string', value: username as String }, { name: 'is_deployed', type: 'boolean', value: deploymentStatus as Boolean }, { name: 'change_number', type: 'string', value: changeNumber as String }, - { name: 'reason', type: 'string', value: reason as String }, - { name: 'access_token', type: 'string', value: accessToken as String } + { name: 'reason', type: 'string', value: reason as String } ] service.requestComponentDeletion(projectKey, componentName, incidentParams).subscribe(); diff --git a/src/app/token.interceptor.spec.ts b/src/app/token.interceptor.spec.ts index 619f968..069148f 100644 --- a/src/app/token.interceptor.spec.ts +++ b/src/app/token.interceptor.spec.ts @@ -4,7 +4,6 @@ import { HttpClient, provideHttpClient, withInterceptors } from '@angular/common import { Router } from '@angular/router'; import { AzureService } from './services/azure.service'; import { tokenInterceptor } from './token.interceptor'; -import { AuthenticationResult } from '@azure/msal-browser'; describe('TokenInterceptor', () => { @@ -13,7 +12,7 @@ describe('TokenInterceptor', () => { let mockAzureService: jasmine.SpyObj; beforeEach(() => { - mockAzureService = jasmine.createSpyObj('AzureService', ['getIdToken', 'refreshToken', 'login']); + mockAzureService = jasmine.createSpyObj('AzureService', ['getAccessToken', 'login']); TestBed.configureTestingModule({ providers: [ @@ -33,9 +32,10 @@ describe('TokenInterceptor', () => { }); it('should add an Authorization header', fakeAsync(() => { - mockAzureService.getIdToken.and.returnValue('test-token'); + mockAzureService.getAccessToken.and.returnValue(Promise.resolve('test-token')); http.get('/test').subscribe(() => {}); + tick(); const req = httpTestingController.expectOne( (r) => { @@ -50,11 +50,12 @@ describe('TokenInterceptor', () => { })); it('should refresh token on 401 or 403 error and retry the request', fakeAsync(() => { - mockAzureService.getIdToken.and.returnValue('expired-token'); - const newAuthResult: AuthenticationResult = { idToken: 'new-token' } as AuthenticationResult; - mockAzureService.refreshToken.and.returnValue(Promise.resolve(newAuthResult)); + mockAzureService.getAccessToken.and.callFake((forceRefresh?: boolean) => + Promise.resolve(forceRefresh ? 'new-token' : 'expired-token') + ); http.get('/test').subscribe(() => {}); + tick(); const req = httpTestingController.expectOne( (r) => { @@ -65,7 +66,7 @@ describe('TokenInterceptor', () => { req.flush(null, { status: 401, statusText: 'Unauthorized' }); - tick(); // Simulate the passage of time for the retry + tick(); // Resolve the force-refresh getAccessToken(true) promise const newReq = httpTestingController.expectOne( (r) => { @@ -80,12 +81,14 @@ describe('TokenInterceptor', () => { })); it('should do nothing on refresh token failure', fakeAsync(() => { - mockAzureService.getIdToken.and.returnValue('expired-token'); - mockAzureService.refreshToken.and.returnValue(Promise.reject(() => new Error('Refresh token failed'))); + mockAzureService.getAccessToken.and.callFake((forceRefresh?: boolean) => + forceRefresh ? Promise.reject(new Error('Refresh token failed')) : Promise.resolve('expired-token') + ); http.get('/test').subscribe({ error: () => {} }); + tick(); const req = httpTestingController.expectOne('/test'); req.flush(null, { status: 401, statusText: 'Unauthorized' }); @@ -93,14 +96,42 @@ describe('TokenInterceptor', () => { httpTestingController.verify(); })); + it('should navigate to /page-not-found when retry request fails with 403', fakeAsync(() => { + const mockRouter = TestBed.inject(Router) as jasmine.SpyObj; + mockAzureService.getAccessToken.and.callFake((forceRefresh?: boolean) => + Promise.resolve(forceRefresh ? 'new-token' : 'expired-token') + ); + + http.get('/test').subscribe({ error: () => {} }); + tick(); + + const req = httpTestingController.expectOne( + (r) => r.headers.get('Authorization') === 'Bearer expired-token' + ); + req.flush(null, { status: 401, statusText: 'Unauthorized' }); + + tick(); // Resolve the force-refresh getAccessToken(true) promise + + const retryReq = httpTestingController.expectOne( + (r) => r.headers.get('Authorization') === 'Bearer new-token' + ); + retryReq.flush(null, { status: 403, statusText: 'Forbidden' }); + + tick(); + + expect(mockRouter.navigate).toHaveBeenCalledWith(['/page-not-found']); + httpTestingController.verify(); + })); + it('should pass through non-401/403 errors', fakeAsync(() => { - mockAzureService.getIdToken.and.returnValue('test-token'); + mockAzureService.getAccessToken.and.returnValue(Promise.resolve('test-token')); http.get('/test').subscribe({ error: (error) => { expect(error.status).toBe(500); } }); + tick(); const req = httpTestingController.expectOne('/test'); req.flush(null, { status: 500, statusText: 'Server Error' }); diff --git a/src/app/token.interceptor.ts b/src/app/token.interceptor.ts index 78117e0..0c1c5ba 100644 --- a/src/app/token.interceptor.ts +++ b/src/app/token.interceptor.ts @@ -1,41 +1,63 @@ import { HttpErrorResponse, HttpEvent, HttpHandlerFn, HttpRequest } from "@angular/common/http"; import { AzureService } from "./services/azure.service"; import { catchError, from, Observable, switchMap, throwError } from "rxjs"; -import { AuthenticationResult } from "@azure/msal-browser"; import { inject } from "@angular/core"; - +import { Router } from "@angular/router"; export function tokenInterceptor(request: HttpRequest, next: HttpHandlerFn): Observable> { const authService = inject(AzureService); - - const token = authService.getIdToken(); - - if (token) { - request = request.clone({ - headers: request.headers.set('Authorization', `Bearer ${token}`), - }); - } + const router = inject(Router); - return next(request).pipe( - catchError((err) => { - if (err instanceof HttpErrorResponse && (err.status === 401 || err.status === 403)) { - // Token might be expired, try to refresh it - return from(authService.refreshToken()).pipe( - switchMap((newAuth: AuthenticationResult) => { - const newToken = newAuth.idToken; - // Clone the request with the new token - const newRequest = request.clone({ - headers: request.headers.set('Authorization', `Bearer ${newToken}`), - }); - return next(newRequest); - }), - catchError((refreshErr) => { - authService.login(); - return throwError(() => new Error(refreshErr)); - }) - ); + return from(authService.getAccessToken()).pipe( + switchMap((token) => { + if (token) { + request = request.clone({ + headers: request.headers.set('Authorization', `Bearer ${token}`), + }); } - return throwError(() => err); + return next(request).pipe( + catchError((err) => handleAuthError(err, request, next, authService, router)) + ); }) ); +} + +function handleRefreshError(refreshErr: unknown, authService: AzureService, router: Router): Observable { + if (refreshErr instanceof HttpErrorResponse && refreshErr.status === 403) { + router.navigate(['/page-not-found']); + } else { + authService.login(); + } + return throwError(() => refreshErr); +} + +function retryWithRefreshedToken( + request: HttpRequest, + next: HttpHandlerFn, + authService: AzureService, + router: Router +): Observable> { + // Token might be expired, force refresh and retry once + return from(authService.getAccessToken(true)).pipe( + switchMap((newToken) => { + const newRequest = request.clone({ + headers: request.headers.set('Authorization', `Bearer ${newToken}`), + }); + return next(newRequest); + }), + catchError((refreshErr) => handleRefreshError(refreshErr, authService, router)) + ); +} + +function handleAuthError( + err: unknown, + request: HttpRequest, + next: HttpHandlerFn, + authService: AzureService, + router: Router +): Observable> { + if (err instanceof HttpErrorResponse && (err.status === 401 || err.status === 403)) { + return retryWithRefreshedToken(request, next, authService, router); + } + return throwError(() => err); } \ No newline at end of file