Skip to content

Commit

Permalink
fix: httpInterceptor behavioural bug (#3193)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

---------

Co-authored-by: Madhusudan Shekhawat <[email protected]>
  • Loading branch information
@Madhusudan092001
Madhusudan092001 and Madhusudan Shekhawat authored Sep 4, 2024
1 parent dc6e780 commit e799237
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 143 deletions.
99 changes: 49 additions & 50 deletions src/app/core/interceptors/httpInterceptor.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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():', () => {
Expand All @@ -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():', () => {
Expand Down Expand Up @@ -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) => {
Expand All @@ -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);
},
});
Expand Down
165 changes: 73 additions & 92 deletions src/app/core/interceptors/httpInterceptor.ts
View file
Original file line number Diff line number Diff line change
@@ -1,33 +1,29 @@
/* 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';
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<string>(null);
public accessTokenSubject = new BehaviorSubject<string | null>(null);

constructor(
private jwtHelperService: JwtHelperService,
Expand Down Expand Up @@ -55,130 +51,115 @@ 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) {
return true;
}
}

refreshAccessToken(): Observable<string> {
refreshAccessToken(): Observable<string | null> {
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<never> {
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<string> {
getAccessToken(): Observable<string | null> {
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<string>, next: HttpHandler): Observable<HttpEvent<string>> {
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<any>, next: HttpHandler, accessToken: string): Observable<HttpEvent<any>> {
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);
Expand Down
Loading

0 comments on commit e799237

Please sign in to comment.