From 22565ee9e0d09a56b333342c889060fb1c901f63 Mon Sep 17 00:00:00 2001 From: Michael Rentmeister Date: Thu, 1 May 2025 23:48:24 -0500 Subject: [PATCH 1/3] fix: ensure isAuthenticated$ has fresh value after handleRedirectCallback and logout --- .../auth0-angular/src/lib/auth.service.ts | 44 ++++++++++++++----- projects/auth0-angular/src/lib/auth.state.ts | 20 ++++++--- .../auth0-angular/src/lib/refresh-state.ts | 8 ++++ 3 files changed, 54 insertions(+), 18 deletions(-) create mode 100644 projects/auth0-angular/src/lib/refresh-state.ts diff --git a/projects/auth0-angular/src/lib/auth.service.ts b/projects/auth0-angular/src/lib/auth.service.ts index a2b61f5e..94ed94d8 100644 --- a/projects/auth0-angular/src/lib/auth.service.ts +++ b/projects/auth0-angular/src/lib/auth.service.ts @@ -29,6 +29,8 @@ import { catchError, switchMap, withLatestFrom, + filter, + take, } from 'rxjs/operators'; import { Auth0ClientService } from './auth.client'; @@ -36,7 +38,7 @@ import { AbstractNavigator } from './abstract-navigator'; import { AuthClientConfig, AppState } from './auth.config'; import { AuthState } from './auth.state'; import { LogoutOptions, RedirectLoginOptions } from './interfaces'; - +import { RefreshState } from './refresh-state'; @Injectable({ providedIn: 'root', }) @@ -182,11 +184,18 @@ export class AuthService * @param options The logout options */ logout(options?: LogoutOptions): Observable { - return from( - this.auth0Client.logout(options).then(() => { - if (options?.openUrl === false || options?.openUrl) { - this.authState.refresh(); + return from(this.auth0Client.logout(options)).pipe( + concatMap(() => { + if (options?.openUrl === undefined) { + return of(undefined); } + + this.authState.refresh(); + return this.authState.refresh$.pipe( + filter((state) => state === RefreshState.Complete), + take(1), + map(() => undefined) + ); }) ); } @@ -309,10 +318,7 @@ export class AuthService this.auth0Client.handleRedirectCallback(url) ).pipe( withLatestFrom(this.authState.isLoading$), - tap(([result, isLoading]) => { - if (!isLoading) { - this.authState.refresh(); - } + concatMap(([result, isLoading]) => { const appState = result?.appState; const target = appState?.target ?? '/'; @@ -320,9 +326,23 @@ export class AuthService this.appStateSubject$.next(appState); } - this.navigator.navigateByUrl(target); - }), - map(([result]) => result) + if (isLoading) { + this.navigator.navigateByUrl(target); + return of(result); + } + + this.authState.refresh(); + + // Wait for the refresh to complete before navigating + return this.authState.refresh$.pipe( + filter((state) => state === RefreshState.Complete), + take(1), + // Wait for the refresh to complete before navigating, + // otherwise an auth.guard could have a stale isAuthenticated value + tap(() => this.navigator.navigateByUrl(target)), + map(() => result) + ); + }) ); } diff --git a/projects/auth0-angular/src/lib/auth.state.ts b/projects/auth0-angular/src/lib/auth.state.ts index 7fe85559..00c88256 100644 --- a/projects/auth0-angular/src/lib/auth.state.ts +++ b/projects/auth0-angular/src/lib/auth.state.ts @@ -16,16 +16,15 @@ import { scan, shareReplay, switchMap, + tap, } from 'rxjs/operators'; import { Auth0ClientService } from './auth.client'; +import { RefreshState } from './refresh-state'; -/** - * Tracks the Authentication State for the SDK - */ @Injectable({ providedIn: 'root' }) export class AuthState { private isLoadingSubject$ = new BehaviorSubject(true); - private refresh$ = new Subject(); + private refreshSubject$ = new Subject(); private accessToken$ = new ReplaySubject(1); private errorSubject$ = new ReplaySubject(1); @@ -34,6 +33,11 @@ export class AuthState { */ public readonly isLoading$ = this.isLoadingSubject$.asObservable(); + /** + * Emits the current state of refresh operations after a change in authentication state is made. + */ + public readonly refresh$ = this.refreshSubject$.asObservable(); + /** * Trigger used to pull User information from the Auth0Client. * Triggers when the access token has changed. @@ -71,7 +75,11 @@ export class AuthState { this.accessTokenTrigger$.pipe( mergeMap(() => this.auth0Client.isAuthenticated()) ), - this.refresh$.pipe(mergeMap(() => this.auth0Client.isAuthenticated())) + this.refreshSubject$.pipe( + filter((state) => state === RefreshState.Refreshing), + mergeMap(() => this.auth0Client.isAuthenticated()), + tap(() => this.refreshSubject$.next(RefreshState.Complete)) + ) ) ) ); @@ -125,7 +133,7 @@ export class AuthState { * reflect the most up-to-date values from Auth0Client. */ public refresh(): void { - this.refresh$.next(); + this.refreshSubject$.next(RefreshState.Refreshing); } /** diff --git a/projects/auth0-angular/src/lib/refresh-state.ts b/projects/auth0-angular/src/lib/refresh-state.ts new file mode 100644 index 00000000..4f4a1742 --- /dev/null +++ b/projects/auth0-angular/src/lib/refresh-state.ts @@ -0,0 +1,8 @@ +/** + * Tracks the state of refresh operations + */ +export const RefreshState = { + Refreshing: 'Refreshing', + Complete: 'Complete', +} as const; +export type RefreshState = (typeof RefreshState)[keyof typeof RefreshState]; From ab321e7da86d56469d839066ba4281def6535dee Mon Sep 17 00:00:00 2001 From: Michael Rentmeister Date: Mon, 5 May 2025 07:22:23 -0500 Subject: [PATCH 2/3] 668 - Added unit tests --- .../src/lib/auth.service.spec.ts | 418 +++++++++++------- 1 file changed, 266 insertions(+), 152 deletions(-) diff --git a/projects/auth0-angular/src/lib/auth.service.spec.ts b/projects/auth0-angular/src/lib/auth.service.spec.ts index 83a433b6..28e16a3a 100644 --- a/projects/auth0-angular/src/lib/auth.service.spec.ts +++ b/projects/auth0-angular/src/lib/auth.service.spec.ts @@ -126,7 +126,9 @@ describe('AuthService', () => { }); it('should not set isLoading when service destroyed before checkSession finished', (done) => { - ((auth0Client.checkSession as unknown) as jest.SpyInstance).mockImplementation( + ( + auth0Client.checkSession as unknown as jest.SpyInstance + ).mockImplementation( () => new Promise((resolve) => setTimeout(resolve, 5000)) ); const localService = createService(); @@ -151,9 +153,9 @@ describe('AuthService', () => { }); it('should return `true` when the client is authenticated', (done) => { - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); const service = createService(); loaded(service).subscribe(() => { @@ -165,9 +167,9 @@ describe('AuthService', () => { }); it('should return true after successfully getting a new token', (done) => { - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - false - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(false); const service = createService(); service.isAuthenticated$.pipe(bufferCount(2)).subscribe((values) => { @@ -179,12 +181,12 @@ describe('AuthService', () => { // Add a small delay before triggering a new emit to the isAuthenticated$. // This ensures we can capture both emits using the above bufferCount(2) setTimeout(() => { - ((auth0Client.getTokenSilently as unknown) as jest.SpyInstance).mockResolvedValue( - {} - ); - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); + ( + auth0Client.getTokenSilently as unknown as jest.SpyInstance + ).mockResolvedValue({}); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); service.getAccessTokenSilently().subscribe(); }, 0); @@ -194,9 +196,9 @@ describe('AuthService', () => { done: any ) => { authState.setIsLoading(false); - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); const service = createService(); @@ -207,9 +209,9 @@ describe('AuthService', () => { // When the token is expired, auth0Client.isAuthenticated is resolving to false. // This is unexpected but known behavior in Auth0-SPA-JS, so we shouldnt rely on it apart from initially. // Once this is resolved, we should be able to rely on `auth0Client.isAuthenticated`, even when the Access Token is expired. - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - false - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(false); service.isAuthenticated$.pipe(take(1)).subscribe((value) => { expect(value).toBe(true); @@ -223,10 +225,10 @@ describe('AuthService', () => { name: 'Test User', }; - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); - ((auth0Client.getUser as unknown) as jest.SpyInstance).mockResolvedValue( + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); + (auth0Client.getUser as unknown as jest.SpyInstance).mockResolvedValue( user ); @@ -246,10 +248,10 @@ describe('AuthService', () => { name: 'Another User', }; - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); - ((auth0Client.getUser as unknown) as jest.SpyInstance).mockResolvedValue( + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); + (auth0Client.getUser as unknown as jest.SpyInstance).mockResolvedValue( user ); @@ -263,10 +265,10 @@ describe('AuthService', () => { // Add a small delay before triggering a new emit to the user$. // This ensures we can capture both emits using the above bufferCount(2) setTimeout(() => { - ((auth0Client.getTokenSilently as unknown) as jest.SpyInstance).mockResolvedValue( - {} - ); - ((auth0Client.getUser as unknown) as jest.SpyInstance).mockResolvedValue( + ( + auth0Client.getTokenSilently as unknown as jest.SpyInstance + ).mockResolvedValue({}); + (auth0Client.getUser as unknown as jest.SpyInstance).mockResolvedValue( user2 ); @@ -279,10 +281,10 @@ describe('AuthService', () => { name: 'Test User', }; - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); - ((auth0Client.getUser as unknown) as jest.SpyInstance).mockResolvedValue( + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); + (auth0Client.getUser as unknown as jest.SpyInstance).mockResolvedValue( user ); @@ -294,9 +296,9 @@ describe('AuthService', () => { }); service.isAuthenticated$.pipe(filter(Boolean)).subscribe(() => { - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - false - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(false); service.logout({ openUrl: false, }); @@ -308,13 +310,13 @@ describe('AuthService', () => { name: 'Test User', }; - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); - ((auth0Client.getTokenSilently as unknown) as jest.SpyInstance).mockResolvedValue( - 'AT1' - ); - ((auth0Client.getUser as unknown) as jest.SpyInstance).mockResolvedValue( + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); + ( + auth0Client.getTokenSilently as unknown as jest.SpyInstance + ).mockResolvedValue('AT1'); + (auth0Client.getUser as unknown as jest.SpyInstance).mockResolvedValue( user ); @@ -326,15 +328,15 @@ describe('AuthService', () => { .getAccessTokenSilently() .pipe( tap(() => - ((auth0Client.getTokenSilently as unknown) as jest.SpyInstance).mockResolvedValue( - 'AT2' - ) + ( + auth0Client.getTokenSilently as unknown as jest.SpyInstance + ).mockResolvedValue('AT2') ), mergeMap(() => service.getAccessTokenSilently()), tap(() => - ((auth0Client.getTokenSilently as unknown) as jest.SpyInstance).mockResolvedValue( - 'AT3' - ) + ( + auth0Client.getTokenSilently as unknown as jest.SpyInstance + ).mockResolvedValue('AT3') ), mergeMap(() => service.getAccessTokenSilently()), // Allow user emissions to come through @@ -356,12 +358,12 @@ describe('AuthService', () => { iss: 'https://example.eu.auth0.com/', }; - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); - ((auth0Client.getIdTokenClaims as unknown) as jest.SpyInstance).mockResolvedValue( - claims - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); + ( + auth0Client.getIdTokenClaims as unknown as jest.SpyInstance + ).mockResolvedValue(claims); const service = createService(); service.idTokenClaims$.subscribe((value) => { @@ -385,12 +387,12 @@ describe('AuthService', () => { iss: 'https://example.eu.auth0.com/', }; - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); - ((auth0Client.getIdTokenClaims as unknown) as jest.SpyInstance).mockResolvedValue( - claims - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); + ( + auth0Client.getIdTokenClaims as unknown as jest.SpyInstance + ).mockResolvedValue(claims); const service = createService(); service.idTokenClaims$.pipe(bufferCount(2)).subscribe((values) => { @@ -402,12 +404,12 @@ describe('AuthService', () => { // Add a small delay before triggering a new emit to the idTokenClaims$. // This ensures we can capture both emits using the above bufferCount(2) setTimeout(() => { - ((auth0Client.getTokenSilently as unknown) as jest.SpyInstance).mockResolvedValue( - {} - ); - ((auth0Client.getIdTokenClaims as unknown) as jest.SpyInstance).mockResolvedValue( - claims2 - ); + ( + auth0Client.getTokenSilently as unknown as jest.SpyInstance + ).mockResolvedValue({}); + ( + auth0Client.getIdTokenClaims as unknown as jest.SpyInstance + ).mockResolvedValue(claims2); service.getAccessTokenSilently().subscribe(); }, 0); @@ -421,12 +423,12 @@ describe('AuthService', () => { iss: 'https://example.eu.auth0.com/', }; - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); - ((auth0Client.getIdTokenClaims as unknown) as jest.SpyInstance).mockResolvedValue( - claims - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); + ( + auth0Client.getIdTokenClaims as unknown as jest.SpyInstance + ).mockResolvedValue(claims); const service = createService(); service.idTokenClaims$.pipe(bufferCount(2)).subscribe((values) => { @@ -436,9 +438,9 @@ describe('AuthService', () => { }); service.isAuthenticated$.pipe(filter(Boolean)).subscribe(() => { - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - false - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(false); service.logout({ openUrl: false, }); @@ -509,13 +511,13 @@ describe('AuthService', () => { }); it('should redirect to the route specified in appState', (done) => { - ((auth0Client.handleRedirectCallback as unknown) as jest.SpyInstance).mockResolvedValue( - { - appState: { - target: '/test-route', - }, - } - ); + ( + auth0Client.handleRedirectCallback as unknown as jest.SpyInstance + ).mockResolvedValue({ + appState: { + target: '/test-route', + }, + }); const localService = createService(); @@ -526,9 +528,9 @@ describe('AuthService', () => { }); it('should fallback to `/` when missing appState', (done) => { - ((auth0Client.handleRedirectCallback as unknown) as jest.SpyInstance).mockResolvedValue( - {} - ); + ( + auth0Client.handleRedirectCallback as unknown as jest.SpyInstance + ).mockResolvedValue({}); const localService = createService(); @@ -539,9 +541,9 @@ describe('AuthService', () => { }); it('should fallback to `/` when handleRedirectCallback returns undefined', (done) => { - ((auth0Client.handleRedirectCallback as unknown) as jest.SpyInstance).mockResolvedValue( - undefined - ); + ( + auth0Client.handleRedirectCallback as unknown as jest.SpyInstance + ).mockResolvedValue(undefined); const localService = createService(); @@ -556,11 +558,11 @@ describe('AuthService', () => { myValue: 'State to Preserve', }; - ((auth0Client.handleRedirectCallback as unknown) as jest.SpyInstance).mockResolvedValue( - { - appState, - } - ); + ( + auth0Client.handleRedirectCallback as unknown as jest.SpyInstance + ).mockResolvedValue({ + appState, + }); const localService = createService(); @@ -573,11 +575,11 @@ describe('AuthService', () => { it('should record errors in the error$ observable', (done) => { const errorObj = new Error('An error has occured'); - ((auth0Client.handleRedirectCallback as unknown) as jest.SpyInstance).mockImplementation( - () => { - throw errorObj; - } - ); + ( + auth0Client.handleRedirectCallback as unknown as jest.SpyInstance + ).mockImplementation(() => { + throw errorObj; + }); const localService = createService(); @@ -594,11 +596,11 @@ describe('AuthService', () => { const errorObj = new Error('An error has occured'); authConfig.errorPath = '/error'; - ((auth0Client.handleRedirectCallback as unknown) as jest.SpyInstance).mockImplementation( - () => { - throw errorObj; - } - ); + ( + auth0Client.handleRedirectCallback as unknown as jest.SpyInstance + ).mockImplementation(() => { + throw errorObj; + }); const localService = createService(); @@ -654,10 +656,10 @@ describe('AuthService', () => { it('should call `loginWithPopup`', (done) => { const service = createService(); loaded(service).subscribe(() => { - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockReset(); - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); + (auth0Client.isAuthenticated as unknown as jest.SpyInstance).mockReset(); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); service.loginWithPopup(); @@ -679,10 +681,10 @@ describe('AuthService', () => { const service = createService(); loaded(service).subscribe(() => { - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockReset(); - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); + (auth0Client.isAuthenticated as unknown as jest.SpyInstance).mockReset(); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); service.loginWithPopup(options, config); @@ -714,15 +716,15 @@ describe('AuthService', () => { it('should reset the authentication state when passing `localOnly` to logout', (done) => { const options = { openUrl: async () => { - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - false - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(false); }, }; - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ); + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true); const service = createService(); @@ -763,9 +765,9 @@ describe('AuthService', () => { id_token: '456', expires_in: 2, }; - ((auth0Client.getTokenSilently as unknown) as jest.SpyInstance).mockResolvedValue( - tokenResponse - ); + ( + auth0Client.getTokenSilently as unknown as jest.SpyInstance + ).mockResolvedValue(tokenResponse); const service = createService(); service @@ -777,25 +779,23 @@ describe('AuthService', () => { }); it('should null when nothing in cache', (done) => { - ((auth0Client.getTokenSilently as unknown) as jest.SpyInstance).mockResolvedValue( - null - ); + ( + auth0Client.getTokenSilently as unknown as jest.SpyInstance + ).mockResolvedValue(null); const service = createService(); - service - .getAccessTokenSilently() - .subscribe((token) => { - expect(token).toBeNull(); - done(); - }); + service.getAccessTokenSilently().subscribe((token) => { + expect(token).toBeNull(); + done(); + }); }); it('should record errors in the error$ observable', (done) => { const errorObj = new Error('An error has occured'); - ((auth0Client.getTokenSilently as unknown) as jest.SpyInstance).mockRejectedValue( - errorObj - ); + ( + auth0Client.getTokenSilently as unknown as jest.SpyInstance + ).mockRejectedValue(errorObj); const service = createService(); service.getAccessTokenSilently().subscribe({ @@ -811,9 +811,9 @@ describe('AuthService', () => { it('should bubble errors', (done) => { const errorObj = new Error('An error has occured'); - ((auth0Client.getTokenSilently as unknown) as jest.SpyInstance).mockRejectedValue( - errorObj - ); + ( + auth0Client.getTokenSilently as unknown as jest.SpyInstance + ).mockRejectedValue(errorObj); const service = createService(); service.getAccessTokenSilently().subscribe({ @@ -848,9 +848,9 @@ describe('AuthService', () => { it('should record errors in the error$ observable', (done) => { const errorObj = new Error('An error has occured'); - ((auth0Client.getTokenWithPopup as unknown) as jest.SpyInstance).mockRejectedValue( - errorObj - ); + ( + auth0Client.getTokenWithPopup as unknown as jest.SpyInstance + ).mockRejectedValue(errorObj); const service = createService(); service.getAccessTokenWithPopup().subscribe({ @@ -866,9 +866,9 @@ describe('AuthService', () => { it('should bubble errors', (done) => { const errorObj = new Error('An error has occured'); - ((auth0Client.getTokenWithPopup as unknown) as jest.SpyInstance).mockRejectedValue( - errorObj - ); + ( + auth0Client.getTokenWithPopup as unknown as jest.SpyInstance + ).mockRejectedValue(errorObj); const service = createService(); service.getAccessTokenWithPopup().subscribe({ @@ -948,9 +948,9 @@ describe('AuthService', () => { .pipe( filter((isLoading) => !isLoading), tap(() => - ((auth0Client.isAuthenticated as unknown) as jest.SpyInstance).mockResolvedValue( - true - ) + ( + auth0Client.isAuthenticated as unknown as jest.SpyInstance + ).mockResolvedValue(true) ), mergeMap(() => localService.handleRedirectCallback()) ) @@ -962,11 +962,11 @@ describe('AuthService', () => { myValue: 'State to Preserve', }; - ((auth0Client.handleRedirectCallback as unknown) as jest.SpyInstance).mockResolvedValue( - { - appState, - } - ); + ( + auth0Client.handleRedirectCallback as unknown as jest.SpyInstance + ).mockResolvedValue({ + appState, + }); const localService = createService(); localService.handleRedirectCallback().subscribe(() => { @@ -976,5 +976,119 @@ describe('AuthService', () => { }); }); }); + + it('should ensure isAuthenticated$ has correct value after refresh', (done) => { + // Simulate the authentication state changing + (auth0Client.isAuthenticated as unknown as jest.SpyInstance) + .mockResolvedValueOnce(false) + .mockResolvedValueOnce(true); + + const service = createService(); + authState.setIsLoading(false); + const authStates: boolean[] = []; + + // Subscribe to isAuthenticated$ before triggering the refresh + service.isAuthenticated$ + .pipe( + // Use bufferTime to collect all emissions within a small window + bufferTime(100), + take(1) + ) + .subscribe((states) => { + authStates.push(...states); + expect(authStates).toEqual([false, true]); + done(); + }); + + service.handleRedirectCallback().subscribe(); + }); + + it('should not wait for refresh when already loading', (done) => { + const service = createService(); + + // Track isAuthenticated$ values + const authStates: boolean[] = []; + service.isAuthenticated$ + .pipe(bufferTime(100), take(1)) + .subscribe((states) => { + authStates.push(...states); + + // Should only have one state change since we're not waiting for refresh + expect(authStates).toEqual([false]); + expect(callbackComplete).toBe(true); + done(); + }); + + let callbackComplete = false; + service.handleRedirectCallback().subscribe(() => { + callbackComplete = true; + }); + }); + }); + + describe('logout', () => { + it('should return not call refresh when openUrl is undefined', (done) => { + const service = createService(); + const refreshSpy = jest.spyOn(authState, 'refresh'); + + service.logout({ openUrl: undefined }).subscribe(() => { + expect(refreshSpy).not.toHaveBeenCalled(); + done(); + }); + }); + + it('should wait for refresh to complete when openUrl is false', (done) => { + const service = createService(); + authState.setIsLoading(false); + + // Simulate the authentication state changing + (auth0Client.isAuthenticated as unknown as jest.SpyInstance) + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(false); + + const authStates: boolean[] = []; + + // Subscribe to isAuthenticated$ before triggering the refresh + service.isAuthenticated$ + .pipe( + // Use bufferTime to collect all emissions within a small window + bufferTime(100), + take(1) + ) + .subscribe((states) => { + authStates.push(...states); + expect(authStates).toEqual([true, false]); + done(); + }); + + service.logout({ openUrl: false }).subscribe(); + }); + + it('should wait for refresh to complete when openUrl is a function', (done) => { + const service = createService(); + authState.setIsLoading(false); + + // Simulate the authentication state changing + (auth0Client.isAuthenticated as unknown as jest.SpyInstance) + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(false); + + const authStates: boolean[] = []; + + // Subscribe to isAuthenticated$ before triggering the refresh + service.isAuthenticated$ + .pipe( + // Use bufferTime to collect all emissions within a small window + bufferTime(100), + take(1) + ) + .subscribe((states) => { + authStates.push(...states); + expect(authStates).toEqual([true, false]); + done(); + }); + + service.logout({ openUrl: () => {} }).subscribe(); + }); }); }); From d4592f74a842a11627c3f7a4b5b4d8fce9bdbb05 Mon Sep 17 00:00:00 2001 From: Michael Rentmeister Date: Tue, 6 May 2025 12:40:19 -0500 Subject: [PATCH 3/3] 668 - Fix unit tests --- .../src/lib/auth.service.spec.ts | 108 +++++++++--------- projects/auth0-angular/src/lib/auth.state.ts | 36 ++++-- 2 files changed, 85 insertions(+), 59 deletions(-) diff --git a/projects/auth0-angular/src/lib/auth.service.spec.ts b/projects/auth0-angular/src/lib/auth.service.spec.ts index 28e16a3a..73387deb 100644 --- a/projects/auth0-angular/src/lib/auth.service.spec.ts +++ b/projects/auth0-angular/src/lib/auth.service.spec.ts @@ -126,20 +126,35 @@ describe('AuthService', () => { }); it('should not set isLoading when service destroyed before checkSession finished', (done) => { + // Mock checkSession to never resolve ( auth0Client.checkSession as unknown as jest.SpyInstance ).mockImplementation( - () => new Promise((resolve) => setTimeout(resolve, 5000)) + () => new Promise(() => {}) // Never resolves ); + const localService = createService(); + const loadingStates: boolean[] = []; - localService.isLoading$.pipe(bufferTime(500)).subscribe((loading) => { - expect(loading.length).toEqual(1); - expect(loading).toEqual([true]); - done(); - }); + // Subscribe to isLoading$ and collect states + localService.isLoading$ + .pipe( + // Use a longer buffer time to ensure we catch all emissions + bufferTime(1000), + // Take only the first buffer + take(1) + ) + .subscribe((states) => { + loadingStates.push(...states); + // Should only see the initial true state + expect(loadingStates).toEqual([true]); + done(); + }); - localService.ngOnDestroy(); + // Destroy the service after a short delay to ensure subscription is set up + setTimeout(() => { + localService.ngOnDestroy(); + }, 100); }); }); @@ -299,9 +314,11 @@ describe('AuthService', () => { ( auth0Client.isAuthenticated as unknown as jest.SpyInstance ).mockResolvedValue(false); - service.logout({ - openUrl: false, - }); + service + .logout({ + openUrl: false, + }) + .subscribe(); }); }); @@ -441,9 +458,11 @@ describe('AuthService', () => { ( auth0Client.isAuthenticated as unknown as jest.SpyInstance ).mockResolvedValue(false); - service.logout({ - openUrl: false, - }); + service + .logout({ + openUrl: false, + }) + .subscribe(); }); }); }); @@ -735,7 +754,7 @@ describe('AuthService', () => { done(); }); - service.logout(options); + service.logout(options).subscribe(); }); }); @@ -978,27 +997,23 @@ describe('AuthService', () => { }); it('should ensure isAuthenticated$ has correct value after refresh', (done) => { - // Simulate the authentication state changing + // Simulate the authentication state changing. isAuthenticatedTrigger$ calls isAuthenticated() + // twice at the start of the stream, and then once after the refresh completes. (auth0Client.isAuthenticated as unknown as jest.SpyInstance) .mockResolvedValueOnce(false) - .mockResolvedValueOnce(true); + .mockResolvedValueOnce(false) + .mockResolvedValue(true); const service = createService(); authState.setIsLoading(false); const authStates: boolean[] = []; // Subscribe to isAuthenticated$ before triggering the refresh - service.isAuthenticated$ - .pipe( - // Use bufferTime to collect all emissions within a small window - bufferTime(100), - take(1) - ) - .subscribe((states) => { - authStates.push(...states); - expect(authStates).toEqual([false, true]); - done(); - }); + service.isAuthenticated$.pipe(bufferCount(2)).subscribe((states) => { + authStates.push(...states); + expect(authStates).toEqual([false, true]); + done(); + }); service.handleRedirectCallback().subscribe(); }); @@ -1041,25 +1056,21 @@ describe('AuthService', () => { const service = createService(); authState.setIsLoading(false); - // Simulate the authentication state changing + // Simulate the authentication state changing. isAuthenticatedTrigger$ calls isAuthenticated() + // twice at the start of the stream, and then once after the refresh completes. (auth0Client.isAuthenticated as unknown as jest.SpyInstance) .mockResolvedValueOnce(true) - .mockResolvedValueOnce(false); + .mockResolvedValueOnce(true) + .mockResolvedValue(false); const authStates: boolean[] = []; // Subscribe to isAuthenticated$ before triggering the refresh - service.isAuthenticated$ - .pipe( - // Use bufferTime to collect all emissions within a small window - bufferTime(100), - take(1) - ) - .subscribe((states) => { - authStates.push(...states); - expect(authStates).toEqual([true, false]); - done(); - }); + service.isAuthenticated$.pipe(bufferCount(2)).subscribe((states) => { + authStates.push(...states); + expect(authStates).toEqual([true, false]); + done(); + }); service.logout({ openUrl: false }).subscribe(); }); @@ -1070,23 +1081,18 @@ describe('AuthService', () => { // Simulate the authentication state changing (auth0Client.isAuthenticated as unknown as jest.SpyInstance) + .mockResolvedValueOnce(true) .mockResolvedValueOnce(true) .mockResolvedValueOnce(false); const authStates: boolean[] = []; // Subscribe to isAuthenticated$ before triggering the refresh - service.isAuthenticated$ - .pipe( - // Use bufferTime to collect all emissions within a small window - bufferTime(100), - take(1) - ) - .subscribe((states) => { - authStates.push(...states); - expect(authStates).toEqual([true, false]); - done(); - }); + service.isAuthenticated$.pipe(bufferCount(2)).subscribe((states) => { + authStates.push(...states); + expect(authStates).toEqual([true, false]); + done(); + }); service.logout({ openUrl: () => {} }).subscribe(); }); diff --git a/projects/auth0-angular/src/lib/auth.state.ts b/projects/auth0-angular/src/lib/auth.state.ts index 00c88256..ce915fdf 100644 --- a/projects/auth0-angular/src/lib/auth.state.ts +++ b/projects/auth0-angular/src/lib/auth.state.ts @@ -1,4 +1,4 @@ -import { Inject, Injectable } from '@angular/core'; +import { Inject, Injectable, OnDestroy } from '@angular/core'; import { Auth0Client } from '@auth0/auth0-spa-js'; import { BehaviorSubject, @@ -16,13 +16,16 @@ import { scan, shareReplay, switchMap, + takeUntil, tap, } from 'rxjs/operators'; import { Auth0ClientService } from './auth.client'; import { RefreshState } from './refresh-state'; @Injectable({ providedIn: 'root' }) -export class AuthState { +export class AuthState implements OnDestroy { + // https://stackoverflow.com/a/41177163 + private ngUnsubscribe$ = new Subject(); private isLoadingSubject$ = new BehaviorSubject(true); private refreshSubject$ = new Subject(); private accessToken$ = new ReplaySubject(1); @@ -56,6 +59,15 @@ export class AuthState { filter(({ previous, current }) => previous !== current) ); + /** + * Stream that handles refresh state completion independently of isAuthenticated$ subscriptions + */ + private readonly refreshCompletion$ = this.refreshSubject$.pipe( + filter((state) => state === RefreshState.Refreshing), + mergeMap(() => this.auth0Client.isAuthenticated()), + tap(() => this.refreshSubject$.next(RefreshState.Complete)) + ); + /** * Trigger used to pull User information from the Auth0Client. * Triggers when an event occurs that needs to retrigger the User Profile information. @@ -75,11 +87,7 @@ export class AuthState { this.accessTokenTrigger$.pipe( mergeMap(() => this.auth0Client.isAuthenticated()) ), - this.refreshSubject$.pipe( - filter((state) => state === RefreshState.Refreshing), - mergeMap(() => this.auth0Client.isAuthenticated()), - tap(() => this.refreshSubject$.next(RefreshState.Complete)) - ) + this.refreshCompletion$ ) ) ); @@ -117,7 +125,10 @@ export class AuthState { */ public readonly error$ = this.errorSubject$.asObservable(); - constructor(@Inject(Auth0ClientService) private auth0Client: Auth0Client) {} + constructor(@Inject(Auth0ClientService) private auth0Client: Auth0Client) { + // Subscribe to refreshCompletion$ to ensure refresh state completes + this.refreshCompletion$.pipe(takeUntil(this.ngUnsubscribe$)).subscribe(); + } /** * Update the isLoading state using the provided value @@ -153,4 +164,13 @@ export class AuthState { public setError(error: any): void { this.errorSubject$.next(error); } + + ngOnDestroy(): void { + this.ngUnsubscribe$.next(); + this.ngUnsubscribe$.complete(); + this.isLoadingSubject$.complete(); + this.refreshSubject$.complete(); + this.accessToken$.complete(); + this.errorSubject$.complete(); + } }