From e799237279914bce8838fae7ce1cbb260ca77631 Mon Sep 17 00:00:00 2001 From: Madhusudan092001 Date: Wed, 4 Sep 2024 12:42:38 +0530 Subject: [PATCH] fix: httpInterceptor behavioural bug (#3193) * fix hhtpInterceptor behavioural bug * some updates * changes in .spec.ts file for testing * some fixes * Modifications in testing file for increase code coverage of httpInterceptor (#3197) Co-authored-by: Madhusudan Shekhawat --------- Co-authored-by: Madhusudan Shekhawat --- .../core/interceptors/httpInterceptor.spec.ts | 99 ++++++----- src/app/core/interceptors/httpInterceptor.ts | 165 ++++++++---------- .../personal-cards/personal-cards.page.ts | 3 +- 3 files changed, 124 insertions(+), 143 deletions(-) diff --git a/src/app/core/interceptors/httpInterceptor.spec.ts b/src/app/core/interceptors/httpInterceptor.spec.ts index 274d8a2cb2..5097559952 100644 --- a/src/app/core/interceptors/httpInterceptor.spec.ts +++ b/src/app/core/interceptors/httpInterceptor.spec.ts @@ -139,11 +139,20 @@ describe('HttpConfigInterceptor', () => { }); }); - it('expiringSoon(): should check if token is expiring soon', () => { - jwtHelperService.getExpirationDate.and.returnValue(new Date('2023-03-03T06:50:11.644Z')); + describe('expiringSoon():', () => { + it('should return true if token is expiring soon', () => { + jwtHelperService.getExpirationDate.and.returnValue(new Date('2023-03-03T06:50:11.644Z')); - const result = httpInterceptor.expiringSoon(authResData2.accessToken); - expect(result).toBeTrue(); + const result = httpInterceptor.expiringSoon(authResData2.accessToken); + expect(result).toBeTrue(); + }); + + it('should return true if an error occurs while checking token expiration', () => { + jwtHelperService.getExpirationDate.and.throwError('Error in getting expiration date'); + + const result = httpInterceptor.expiringSoon(authResData2.accessToken); + expect(result).toBeTrue(); + }); }); describe('refreshAccessToken():', () => { @@ -170,13 +179,26 @@ describe('HttpConfigInterceptor', () => { httpInterceptor.refreshAccessToken().subscribe({ error: (err) => { expect(err).toBeTruthy(); - expect(userEventService.logout).toHaveBeenCalledTimes(1); - expect(secureStorageService.clearAll).toHaveBeenCalledTimes(1); - expect(storageService.clearAll).toHaveBeenCalledTimes(1); + expect(userEventService.logout).not.toHaveBeenCalled(); + expect(secureStorageService.clearAll).not.toHaveBeenCalled(); + expect(storageService.clearAll).not.toHaveBeenCalled(); done(); }, }); }); + + it('should return null if refresh token is null or undefined', (done) => { + tokenService.getRefreshToken.and.resolveTo(null); // Simulate a null refresh token + + httpInterceptor.refreshAccessToken().subscribe((res) => { + expect(res).toBeNull(); + expect(tokenService.getRefreshToken).toHaveBeenCalledTimes(1); + expect(routerAuthService.fetchAccessToken).not.toHaveBeenCalled(); + expect(routerAuthService.newAccessToken).not.toHaveBeenCalled(); + expect(tokenService.getAccessToken).not.toHaveBeenCalled(); + done(); + }); + }); }); describe('getAccessToken():', () => { @@ -234,67 +256,44 @@ describe('HttpConfigInterceptor', () => { .intercept(new HttpRequest('GET', 'https://app.fylehq.com/'), { handle: () => of(null) }) .subscribe((res) => { expect(res).toBeNull(); - expect(httpInterceptor.secureUrl).toHaveBeenCalledTimes(2); + expect(httpInterceptor.secureUrl).toHaveBeenCalledTimes(1); expect(httpInterceptor.getAccessToken).toHaveBeenCalledTimes(1); expect(deviceService.getDeviceInfo).toHaveBeenCalledTimes(1); done(); }); }); - it('should refresh token if the next handler errors out', (done) => { - spyOn(httpInterceptor, 'expiringSoon').and.returnValue(true); - spyOn(httpInterceptor, 'refreshAccessToken').and.returnValue(of(authResData2.refresh_token)); - spyOn(httpInterceptor, 'getAccessToken').and.returnValue(of(authResData2.accessToken)); - deviceService.getDeviceInfo.and.returnValue(of(extendedDeviceInfoMockData)); + it('should handle unauthorized error if no access token is available', (done) => { + spyOn(httpInterceptor, 'secureUrl').and.returnValue(true); + spyOn(httpInterceptor, 'getAccessToken').and.returnValue(of(null)); // Simulate no access token httpInterceptor - .intercept(new HttpRequest('GET', 'https://app.fylehq.com/'), { - handle: () => - throwError( - () => - new HttpErrorResponse({ - status: 200, - }) - ), - }) + .intercept(new HttpRequest('GET', 'https://app.fylehq.com/'), { handle: () => of(null) }) .subscribe({ + next: () => fail('Expected an error, but got a success response'), error: (err) => { - expect(err).toBeTruthy(); - expect(httpInterceptor.expiringSoon).toHaveBeenCalledTimes(1); - expect(httpInterceptor.refreshAccessToken).toHaveBeenCalledTimes(1); + expect(err.status).toBe(401); + expect(err.error).toBe('Unauthorized'); + expect(httpInterceptor.secureUrl).toHaveBeenCalledTimes(1); expect(httpInterceptor.getAccessToken).toHaveBeenCalledTimes(1); - expect(deviceService.getDeviceInfo).toHaveBeenCalledTimes(1); done(); }, }); }); - it('should clear cache if the next handler error out but the token is not expiring soon', (done) => { - spyOn(httpInterceptor, 'expiringSoon').and.returnValue(false); - spyOn(httpInterceptor, 'getAccessToken').and.returnValue(of(authResData2.accessToken)); - deviceService.getDeviceInfo.and.returnValue(of(extendedDeviceInfoMockData)); + it('should pass through requests for non-secure URLs', (done) => { + spyOn(httpInterceptor, 'secureUrl').and.returnValue(false); + + const nextHandleSpy = jasmine.createSpy().and.returnValue(of(null)); httpInterceptor - .intercept(new HttpRequest('GET', 'https://app.fylehq.com/'), { - handle: () => - throwError( - () => - new HttpErrorResponse({ - status: 401, - }) - ), - }) - .subscribe({ - error: (err) => { - expect(err).toBeTruthy(); - expect(httpInterceptor.expiringSoon).toHaveBeenCalledTimes(1); - expect(httpInterceptor.getAccessToken).toHaveBeenCalledTimes(1); - expect(userEventService.logout).toHaveBeenCalledTimes(1); - expect(secureStorageService.clearAll).toHaveBeenCalledTimes(1); - expect(storageService.clearAll).toHaveBeenCalledTimes(1); - }, + .intercept(new HttpRequest('GET', 'http://example.com'), { handle: nextHandleSpy }) + .subscribe((res) => { + expect(res).toBeNull(); + expect(httpInterceptor.secureUrl).toHaveBeenCalledTimes(1); + expect(nextHandleSpy).toHaveBeenCalled(); + done(); }); - done(); }); it('should throw an error if the next handler returns a 404 and device information could be retrived', (done) => { @@ -319,7 +318,7 @@ describe('HttpConfigInterceptor', () => { .subscribe({ error: (err) => { expect(err).toBeTruthy(); - expect(httpInterceptor.expiringSoon).toHaveBeenCalledTimes(1); + expect(httpInterceptor.expiringSoon).not.toHaveBeenCalled(); expect(httpInterceptor.getAccessToken).toHaveBeenCalledTimes(1); }, }); diff --git a/src/app/core/interceptors/httpInterceptor.ts b/src/app/core/interceptors/httpInterceptor.ts index a857e76d8c..953bc5bb71 100644 --- a/src/app/core/interceptors/httpInterceptor.ts +++ b/src/app/core/interceptors/httpInterceptor.ts @@ -1,20 +1,17 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ import { HttpErrorResponse, HttpEvent, HttpHandler, HttpInterceptor, HttpParameterCodec, - HttpParams, HttpRequest, } from '@angular/common/http'; import { Injectable } from '@angular/core'; - -import { BehaviorSubject, Observable, forkJoin, from, iif, of, throwError } from 'rxjs'; -import { catchError, concatMap, filter, mergeMap, take } from 'rxjs/operators'; - -import { JwtHelperService } from '../services/jwt-helper.service'; - +import { BehaviorSubject, Observable, from, of, throwError } from 'rxjs'; +import { catchError, filter, switchMap, take } from 'rxjs/operators'; import * as dayjs from 'dayjs'; +import { JwtHelperService } from '../services/jwt-helper.service'; import { globalCacheBusterNotifier } from 'ts-cacheable'; import { DeviceService } from '../services/device.service'; import { RouterAuthService } from '../services/router-auth.service'; @@ -22,12 +19,11 @@ import { SecureStorageService } from '../services/secure-storage.service'; import { StorageService } from '../services/storage.service'; import { TokenService } from '../services/token.service'; import { UserEventService } from '../services/user-event.service'; - @Injectable() export class HttpConfigInterceptor implements HttpInterceptor { public accessTokenCallInProgress = false; - public accessTokenSubject = new BehaviorSubject(null); + public accessTokenSubject = new BehaviorSubject(null); constructor( private jwtHelperService: JwtHelperService, @@ -55,8 +51,8 @@ export class HttpConfigInterceptor implements HttpInterceptor { expiringSoon(accessToken: string): boolean { try { const expiryDate = dayjs(this.jwtHelperService.getExpirationDate(accessToken)); - const now = dayjs(new Date()); - const differenceSeconds = expiryDate.diff(now, 'second'); + const now = dayjs(); + const differenceSeconds = expiryDate.diff(now, 'seconds'); const maxRefreshDifferenceSeconds = 2 * 60; return differenceSeconds < maxRefreshDifferenceSeconds; } catch (err) { @@ -64,121 +60,106 @@ export class HttpConfigInterceptor implements HttpInterceptor { } } - refreshAccessToken(): Observable { + refreshAccessToken(): Observable { return from(this.tokenService.getRefreshToken()).pipe( - concatMap((refreshToken) => this.routerAuthService.fetchAccessToken(refreshToken)), - catchError((error) => { - this.userEventService.logout(); - this.secureStorageService.clearAll(); - this.storageService.clearAll(); - globalCacheBusterNotifier.next(); - return throwError(error); - }), - concatMap((authResponse) => this.routerAuthService.newAccessToken(authResponse.access_token)), - concatMap(() => from(this.tokenService.getAccessToken())) + switchMap((refreshToken) => { + if (refreshToken) { + return from(this.routerAuthService.fetchAccessToken(refreshToken)).pipe( + switchMap((authResponse) => from(this.routerAuthService.newAccessToken(authResponse.access_token))), + switchMap(() => from(this.tokenService.getAccessToken())), + catchError((error: HttpErrorResponse) => this.handleError(error)) // Handle refresh errors + ); + } else { + return of(null); + } + }) ); } + handleError(error: HttpErrorResponse): Observable { + if (error.status === 401) { + this.userEventService.logout(); + this.secureStorageService.clearAll(); + this.storageService.clearAll(); + globalCacheBusterNotifier.next(); + } + return throwError(error); // Rethrow the error to the caller + } + /** * This method get current accessToken from Storage, check if this token is expiring or not. * If the token is expiring it will get another accessToken from API and return the new accessToken * If multiple API call initiated then `this.accessTokenCallInProgress` will block multiple access_token call * Reference: https://stackoverflow.com/a/57638101 */ - getAccessToken(): Observable { + getAccessToken(): Observable { return from(this.tokenService.getAccessToken()).pipe( - concatMap((accessToken) => { - if (this.expiringSoon(accessToken)) { - if (!this.accessTokenCallInProgress) { - this.accessTokenCallInProgress = true; - this.accessTokenSubject.next(null); - return this.refreshAccessToken().pipe( - concatMap((newAccessToken) => { - this.accessTokenCallInProgress = false; - this.accessTokenSubject.next(newAccessToken); - return of(newAccessToken); - }) - ); - } else { - return this.accessTokenSubject.pipe( - filter((result) => result !== null), - take(1), - concatMap(() => from(this.tokenService.getAccessToken())) - ); - } - } else { + switchMap((accessToken) => { + if (accessToken && !this.expiringSoon(accessToken)) { return of(accessToken); } + if (!this.accessTokenCallInProgress) { + this.accessTokenCallInProgress = true; + this.accessTokenSubject.next(null); + return this.refreshAccessToken().pipe( + switchMap((newAccessToken) => { + this.accessTokenCallInProgress = false; + this.accessTokenSubject.next(newAccessToken); + return of(newAccessToken); + }) + ); + } else { + // If a refresh is already in progress, wait for it to complete + return this.accessTokenSubject.pipe( + filter((result) => result !== null), + take(1), + switchMap(() => from(this.tokenService.getAccessToken())) + ); + } }) ); } getUrlWithoutQueryParam(url: string): string { - const queryIndex = Math.min( - url.indexOf('?') !== -1 ? url.indexOf('?') : url.length, - url.indexOf(';') !== -1 ? url.indexOf(';') : url.length - ); - if (queryIndex !== url.length) { - url = url.substring(0, queryIndex); - } - if (url.length > 200) { - url = url.substring(0, 200); - } - return url; + // Remove query parameters from the URL + return url.split('?')[0].split(';')[0].substring(0, 200); } intercept(request: HttpRequest, next: HttpHandler): Observable> { - return forkJoin({ - token: iif(() => this.secureUrl(request.url), this.getAccessToken(), of(null)), - deviceInfo: from(this.deviceService.getDeviceInfo()), - }).pipe( - concatMap(({ token, deviceInfo }) => { - if (token && this.secureUrl(request.url)) { - request = request.clone({ headers: request.headers.set('Authorization', 'Bearer ' + token) }); - const params = new HttpParams({ encoder: new CustomEncoder(), fromString: request.params.toString() }); - request = request.clone({ params }); - } + if (this.secureUrl(request.url)) { + return this.getAccessToken().pipe( + switchMap((accessToken) => { + if (!accessToken) { + return this.handleError({ status: 401, error: 'Unauthorized' } as HttpErrorResponse); + } + return this.executeHttpRequest(request, next, accessToken); + }) + ); + } else { + return next.handle(request); + } + } + + executeHttpRequest(request: HttpRequest, next: HttpHandler, accessToken: string): Observable> { + return from(this.deviceService.getDeviceInfo()).pipe( + switchMap((deviceInfo) => { const appVersion = deviceInfo.appVersion || '0.0.0'; const osVersion = deviceInfo.osVersion; const operatingSystem = deviceInfo.operatingSystem; - const mobileModifiedappVersion = `fyle-mobile::${appVersion}::${operatingSystem}::${osVersion}`; + const mobileModifiedAppVersion = `fyle-mobile::${appVersion}::${operatingSystem}::${osVersion}`; request = request.clone({ + headers: request.headers.set('Authorization', `Bearer ${accessToken}`), setHeaders: { - 'X-App-Version': mobileModifiedappVersion, + 'X-App-Version': mobileModifiedAppVersion, 'X-Page-Url': this.getUrlWithoutQueryParam(window.location.href), 'X-Source-Identifier': 'mobile_app', }, }); - - return next.handle(request).pipe( - catchError((error) => { - if (error instanceof HttpErrorResponse) { - if (this.expiringSoon(token)) { - return from(this.refreshAccessToken()).pipe( - mergeMap((newToken) => { - request = request.clone({ headers: request.headers.set('Authorization', 'Bearer ' + newToken) }); - return next.handle(request); - }) - ); - } else if ( - (error.status === 404 && error.headers.get('X-Mobile-App-Blocked') === 'true') || - error.status === 401 - ) { - this.userEventService.logout(); - this.secureStorageService.clearAll(); - this.storageService.clearAll(); - globalCacheBusterNotifier.next(); - return throwError(error); - } - } - return throwError(error); - }) - ); + return next.handle(request).pipe(catchError((error: HttpErrorResponse) => this.handleError(error))); }) ); } } - export class CustomEncoder implements HttpParameterCodec { encodeKey(key: string): string { return encodeURIComponent(key); diff --git a/src/app/fyle/personal-cards/personal-cards.page.ts b/src/app/fyle/personal-cards/personal-cards.page.ts index b6d47516dd..5befa73b2e 100644 --- a/src/app/fyle/personal-cards/personal-cards.page.ts +++ b/src/app/fyle/personal-cards/personal-cards.page.ts @@ -55,6 +55,7 @@ import { SelectedFilters } from 'src/app/shared/components/fy-filters/selected-f import { Expense } from 'src/app/core/models/expense.model'; import { ToastMessageComponent } from 'src/app/shared/components/toast-message/toast-message.component'; +// eslint-disable-next-line custom-rules/prefer-semantic-extension-name type Filters = Partial<{ amount: number; createdOn: Partial<{ @@ -471,7 +472,7 @@ export class PersonalCardsPage implements OnInit, AfterViewInit { if (this.selectionMode) { this.switchSelectionMode(); } - this.selectedTrasactionType = event.detail.value as string; + this.selectedTrasactionType = event.detail.value; this.acc = []; const params = this.loadData$.getValue(); const queryParams = params.queryParams || {};