Skip to content
This repository has been archived by the owner on May 7, 2021. It is now read-only.

feat(rpt-interceptor): add RPT refresh logic to auth interceptor & cleanup code #129

Merged
merged 1 commit into from
Sep 29, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 6 additions & 14 deletions src/app/auth/authentication.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ import { AuthInterceptor } from '../shared/auth.interceptor';
import { SSO_API_URL } from '../shared/sso-api';
import { AUTH_API_URL } from '../shared/auth-api';
import { REALM } from '../shared/realm-token';
import { WIT_API_URL } from '../shared/wit-api';

describe('Service: Authentication service', () => {

const authUrl: string = 'http://example.com/';
const authUrl: string = 'http://auth.example.com/';
let authenticationService: AuthenticationService;
let broadcaster: Broadcaster;
let httpClient: HttpClient;
let httpClientTestingModule: HttpClientTestingModule;
let httpTestingController: HttpTestingController;

beforeEach(() => {
Expand All @@ -31,18 +31,10 @@ describe('Service: Authentication service', () => {
useClass: AuthInterceptor,
multi: true
},
{
provide: AUTH_API_URL,
useValue: 'http://example.com/'
},
{
provide: REALM,
useValue: 'fabric8'
},
{
provide: SSO_API_URL,
useValue: 'http://example.com/auth'
},
{ provide: REALM, useValue: 'fabric8' },
{ provide: AUTH_API_URL, useValue: 'http://auth.example.com/' },
{ provide: SSO_API_URL, useValue: 'http://sso.example.com/auth' },
{ provide: WIT_API_URL, useValue: 'http://wit.example.com'},
Broadcaster
]
});
Expand Down
4 changes: 1 addition & 3 deletions src/app/auth/authentication.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { AUTH_API_URL } from '../shared/auth-api';
import { SSO_API_URL } from '../shared/sso-api';
import { REALM } from '../shared/realm-token';
import { Token } from '../user/token';
import { isAuthenticationError } from '../shared/isAuthenticationError';
import { ConnectableObservable } from 'rxjs/internal/observable/ConnectableObservable';

export interface ProcessTokenResponse {
Expand Down Expand Up @@ -100,8 +99,7 @@ export class AuthenticationService {
let tokenUrl = this.apiUrl + `token?force_pull=true&for=` + encodeURIComponent(cluster);
const httpOptions = {
headers: new HttpHeaders({
'Content-Type': 'application/json',
'Authorization': `Bearer ${this.getToken()}`
'Content-Type': 'application/json'
})
};
return this.http.get(tokenUrl, httpOptions)
Expand Down
45 changes: 41 additions & 4 deletions src/app/shared/auth.interceptor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,15 @@ import {
import { TestBed } from '@angular/core/testing';
import { Broadcaster } from 'ngx-base';
import { AuthInterceptor } from './auth.interceptor';
import { WIT_API_URL } from './wit-api';
import { AUTH_API_URL } from './auth-api';
import { SSO_API_URL } from './sso-api';

describe(`AuthHttpInterceptor`, () => {
const testUrl: string = 'http://localhost/test';
const testUrl: string = 'http://auth.example.com/test';
const otherUrl: string = 'http://other.example.com/test';
const testToken: string = 'test_token';
const rptToken = 'new_token_with_rpt_data';

let httpMock: HttpTestingController;
let httpClient: HttpClient;
Expand All @@ -24,16 +29,17 @@ describe(`AuthHttpInterceptor`, () => {
useClass: AuthInterceptor,
multi: true
},
{ provide: WIT_API_URL, useValue: 'http://wit.example.com'},
{ provide: AUTH_API_URL, useValue: 'http://auth.example.com'},
{ provide: SSO_API_URL, useValue: 'http://sso.example.com'},
Broadcaster
]
});
httpMock = TestBed.get(HttpTestingController);
httpClient = TestBed.get(HttpClient);
broadcaster = TestBed.get(Broadcaster);

spyOn(localStorage, 'getItem').and.callFake( (key: string): string => {
return key === 'auth_token' ? testToken : null;
});
localStorage.setItem('auth_token', testToken);
});
afterEach(() => {
httpMock.verify();
Expand All @@ -44,8 +50,39 @@ describe(`AuthHttpInterceptor`, () => {

const req = httpMock.expectOne(testUrl);

expect(req.request.headers.has('Authorization')).toBeTruthy();
expect(req.request.headers.get('Authorization')).toBe(`Bearer ${testToken}`);
});

it('should not intercept request if the URL is not valid auth endpoint', () => {
httpClient.get(otherUrl).subscribe(() => {});

const req = httpMock.expectOne(otherUrl);

// should not add authorization header
expect(req.request.headers.has('Authorization')).toBeFalsy();
});

it('should update auth_token if there is a new RPT token in response header', () => {
httpClient.get(testUrl, { observe: 'response' }).subscribe(res => {
expect(res.headers.has('Authorization')).toBeTruthy();
expect(res.headers.get('Authorization')).toBe(`Bearer ${rptToken}`);
});

const req = httpMock.expectOne(testUrl);

// mock response
req.flush(
{ data: 'mock-data' },
{ headers: { 'Authorization': `Bearer ${rptToken}` } }
);

// check if request sends proper auth headers
expect(req.request.headers.has('Authorization'));
expect(req.request.headers.get('Authorization')).toBe(`Bearer ${testToken}`);

// check if localStorage is updated with new RPT token
expect(localStorage.getItem('auth_token')).toBe(rptToken);
});

it('should broadcast authenticationError event on 401 with code jwt_security_error', () => {
Expand Down
66 changes: 50 additions & 16 deletions src/app/shared/auth.interceptor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,26 @@ import { tap } from 'rxjs/operators';

import { Broadcaster } from 'ngx-base';

import { isAuthenticationError } from './isAuthenticationError';
import { WIT_API_URL } from './wit-api';
import { AUTH_API_URL } from './auth-api';
import { SSO_API_URL } from './sso-api';

@Injectable()
export class AuthInterceptor implements HttpInterceptor {

constructor(@Inject(forwardRef(() => Broadcaster)) private broadcaster: Broadcaster) {
}
constructor(
@Inject(forwardRef(() => Broadcaster)) private broadcaster: Broadcaster,
@Inject(WIT_API_URL) private witApiUrl: string,
@Inject(AUTH_API_URL) private authApiUrl: string,
@Inject(SSO_API_URL) private ssoUrl: string
) {}

intercept(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
if (!this.isAuthUrl(request.url)) {
return next.handle(request);
}

let token = localStorage.getItem('auth_token');
let ok: string;
if (token !== null) {
request = request.clone({
setHeaders: {
Expand All @@ -35,20 +44,45 @@ export class AuthInterceptor implements HttpInterceptor {
.pipe(
tap(
// Succeeds when there is a response; ignore other events
event => ok = event instanceof HttpResponse ? 'succeeded' : '',
event => event instanceof HttpResponse ? this.refreshRPT(event) : '',
// Operation failed; error is an HttpErrorResponse
error => {
if (error instanceof HttpErrorResponse) {
const res: HttpErrorResponse = error;
if (res.status === 403 || isAuthenticationError(res)) {
this.broadcaster.broadcast('authenticationError', res);
} else if (res.status === 500) {
this.broadcaster.broadcast('communicationError', res);
}
}
return throwError(error);
}
error => error instanceof HttpErrorResponse ? this.catchError(error) : throwError(error)
)
);
}

private refreshRPT(res: HttpResponse<any>) {
if (res.headers.has('Authorization')) {
const token = localStorage.getItem('auth_token');
const newToken = res.headers.get('Authorization').replace('Bearer ', '');
if (token !== newToken) {
localStorage.setItem('auth_token', newToken);
}
}
}

private catchError(res: HttpErrorResponse) {
if (res.status === 403 || this.isAuthenticationError(res)) {
this.broadcaster.broadcast('authenticationError', res);
} else if (res.status === 500) {
this.broadcaster.broadcast('communicationError', res);
}
}

private isAuthenticationError(res: HttpErrorResponse): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't need isAuthenticationError then why was it moved in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do use isAuthenticationError but only inside the interceptor and that is why i moved it here. Earlier it was being used by other services as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not understanding why make the change. If it is external it keeps the code clean, it can be tested and it can be shared. It is more functional to keep it external.

What is the benefit of moving it inside?

Copy link
Contributor Author

@rohitkrai03 rohitkrai03 Sep 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will not be used anywhere else since interceptor does that already for every response. So, it made more sense to keep it tightly coupled as a bussiness logic to auth interceptor rather than keeping it external. Its just separation of concern for me.

Before moving to interceptors we had a shared http.service which had this logic but we moved it outside because we needed other services to explicitly check for the same errors. Since, we have interceptors now we do not need any other service to use this explicitly.

if (res.status === 401) {
const json: any = res.error;
const hasErrors: boolean = json && Array.isArray(json.errors);
const isJwtError: boolean = hasErrors &&
json.errors.filter((e: any) => e.code === 'jwt_security_error').length >= 1;
const authHeader = res.headers.get('www-authenticate');
const isLoginHeader = authHeader && authHeader.toLowerCase().includes('login');
return isJwtError || isLoginHeader;
}
return false;
}

private isAuthUrl(url: string) {
return url.startsWith(this.witApiUrl) || url.startsWith(this.authApiUrl) || url.startsWith(this.ssoUrl);
}
}
14 changes: 0 additions & 14 deletions src/app/shared/check-auth-error.ts

This file was deleted.

14 changes: 0 additions & 14 deletions src/app/shared/isAuthenticationError.ts

This file was deleted.

3 changes: 3 additions & 0 deletions src/app/shared/wit-api.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { InjectionToken } from '@angular/core';

export let WIT_API_URL = new InjectionToken<string>('fabric8.wit.api.url');
13 changes: 10 additions & 3 deletions src/app/user/user.service.spec.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import { HttpHeaders } from '@angular/common/http';
import { HttpHeaders, HTTP_INTERCEPTORS } from '@angular/common/http';
import { HttpClientTestingModule, HttpTestingController} from '@angular/common/http/testing';
import { TestBed } from '@angular/core/testing';

import { Broadcaster, Logger } from 'ngx-base';

import { AUTH_API_URL } from '../shared/auth-api';
import { UserService } from './user.service';
import { AuthInterceptor } from '../shared/auth.interceptor';
import { SSO_API_URL } from '../shared/sso-api';
import { WIT_API_URL } from '../shared/wit-api';


describe('Service: User service', () => {
Expand All @@ -20,9 +23,13 @@ describe('Service: User service', () => {
providers: [
UserService,
{
provide: AUTH_API_URL,
useValue: 'http://example.com/'
provide: HTTP_INTERCEPTORS,
useClass: AuthInterceptor,
multi: true
},
{ provide: AUTH_API_URL, useValue: 'http://auth.example.com/' },
{ provide: SSO_API_URL, useValue: 'http://sso.example.com/auth' },
{ provide: WIT_API_URL, useValue: 'http://wit.example.com'},
Broadcaster,
Logger
]
Expand Down
Loading