3
\$\begingroup\$

I have a project, for which I use Tokenauthentication with JWT Tokens. I am relatively new to Angular Development and rxjs in particular, so there are a lot of concepts I am likely not yet familiar with or can't apply properly. My Backend is Django 3, using the Django Rest Framework and rest_framework_simplejwt.

What the interceptor is doing is check any outgoing HTTP request on if it's a call to my API. If it is, attach the JWT Token. If it is and the Access Token is expired, refresh the Access Token first, then send the call to the API. I haven't yet coded in the scenario on what to do if the Refresh Token expires/is close to expiring but I'm doing this step by step and that's next on the list.

I don't like my code here. It's hard for me to grasp to the point I need comments to make it easier. An even bigger problem is that I don't fully understand the code and thus am struggling to split the intercept function into smaller chunks to move into their own functions.

What could I be doing to make it more comprehensible? What could I "split off" into some nicely named function?

Here the code:

//jwt-interceptor.ts
@Injectable()
export class JWTInterceptor implements HttpInterceptor{
    private tokenRefreshInProgress: boolean = false;
    private refreshAccessTokenSubject: Subject<any> = new BehaviorSubject<any>(null);

    constructor(private userService: UserService){}

    intercept(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>>{
        if (this.isApiUrl(request.url)){
            const accessToken = this.userService.getAccessToken();

            //Below: If Access Token Expired and no refresh of it currently running
            if(this.userService.isTokenExpired(accessToken) && !this.tokenRefreshInProgress){
                this.tokenRefreshInProgress = true;
                this.refreshAccessTokenSubject.next(null);

                return this.userService.refreshToken().pipe(
                    switchMap(authResponse => {
                        this.userService.setAccessToken(authResponse.access);

                        this.tokenRefreshInProgress = false;
                        this.refreshAccessTokenSubject.next(authResponse.access);
                        request = this.addTokenToRequest(authResponse.access, request);
                        return next.handle(request);
                    })
                )

            //Below: If Access Token is expired and a refresh of it already running
            } else if(this.userService.isTokenExpired(accessToken) && this.tokenRefreshInProgress){
                return this.refreshAccessTokenSubject.pipe(
                    filter(result => result !== null),
                    first(),
                    switchMap(response => {
                        request = this.addTokenToRequest(this.userService.getAccessToken(), request);
                        return next.handle(request);
                    })
                )
            
             //Below: If Access Token Valid
            } else {
                request = this.addTokenToRequest(accessToken, request);      
            }      
        } 

        return next.handle(request);
    }

    isApiUrl(url: string): boolean{
        const isApiUrl: boolean = url.startsWith(Constants.wikiApiUrl);
        const isTokenLoginUrl: boolean = url.endsWith('/token');
        const isTokenRefreshUrl: boolean = url.endsWith('/token/refresh');
        return isApiUrl && !isTokenLoginUrl && !isTokenRefreshUrl;
    }

    addTokenToRequest(token: string, request: HttpRequest<any>): HttpRequest<any>{
        const httpHeaders = new HttpHeaders().set("Authorization", `Bearer ${token}`);
        request = request.clone({headers: httpHeaders});
        return request;   
    }
}

//methods from UserService class in user.service.ts
  isTokenExpired(token: string): boolean{
    const [encodedHeader, encodedPayload, encodedSignature] = token.split('.');
    const payload = JSON.parse(atob(encodedPayload));
    const expiryTimestamp = payload.exp;
    const currentTimestamp = Math.floor((new Date).getTime()/1000);
    return currentTimestamp >= expiryTimestamp;
  }

  getRefreshToken(): string{
    return localStorage.getItem('refresh_token');
  }

  getAccessToken(): string{
    return localStorage.getItem('access_token');
  }

  setAccessToken(accessToken: string): void{
    localStorage.setItem('access_token', accessToken);
  }

  refreshToken(): Observable<{access: string, refresh: string}>{
    const refreshToken = this.getRefreshToken();
    return this.http.post<{access, refresh}>(Constants.wikiTokenRefreshUrl, {refresh: refreshToken});
  }
```
\$\endgroup\$
4
\$\begingroup\$

Short explanation of the code

If the token is expired and not yet requested, the process is quite straight:

  • Change tokenRefreshInProgress status to true so that other interceptions will know that and do not trigger the refresh also
  • The refreshAccessTokenSubject BehaviorSubject gets set to null
  • Refresh the token and as soon as we get a result
    • set the token
    • change the tokenRefreshInProgress to false
    • store the token in our BehaviorSubject.
    • add the token to the current request
    • and now finally execute the current request and we return that observable of the request

If the token is expired but already requested

  • Listen to the refreshAccessTokenSubject and wait until it sends an event
    • The first event is the current value in the BehaviorSubject, most likely a "null" (because the token refresh is still in progress), that event gets filtered out by filter .
    • The second event is the refreshed token, that will pass the filter
    • We are only interested in that token, so with ''first'' we take the first that passed the filter (the event reached the first pipe) and after processing that we stop listening to the BehaviorSubject
    • Now we switch from the stream of the BehaviorSubject to a new Stream
      • We add the token to the current request
      • and now we finally handle the current request and return the observable of that request

Things I would change

Danger of multiple instances

Be aware that there may be multiple instances of your interceptor if you import HTTPCLientModule multiple times (see HTTPInterceptor documentation).

There are two ways of handling that.

  • Hope that nobody will add the HTTPClientModule ever somewhere else.
  • Extract the logic into a class and make it a singleton (having a service with Injectable( {providedIn: 'root'}) makes it a singleton the easy way)

Access Modifier

I would always use access modifiers (public / private). Yes, if none is mentioned it's public by default. But the reader does not know if its public by intention or if the developer has missed the private modifier.

Add Types whereever possible

I love typed variables, therefore, I would take the extra step to create custom types if none are provided

RxJs side effects

There are some side effects (like changing the status or changing the Behavior Subject. I would move those into a tap to make it more obvious that those are wished side effects and that we are aware of that.

Omit else parts

I personally omit the else part, when the if part clearly returns out of the method (return). With "clearly" I mean that when I read the if that I already see the return, that means the if block may only be 1-3 lines long.
In this case, the blocks are a bit longer because of the pipe and the switchMap, but the return is on the first line after the if statement, so the approach is still okay for me.

Changed Code

I would move each exported element into its own file, to separate them more clearly

export interface AccessToken{
    ...
}

Injectable()
export class JWTInterceptor implements HttpInterceptor {

    constructor(private refreshToken: RefreshTokenService) {
    }

    public intercept(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
        if (this.isApiUrl(request.url)) {
            return this.refreshToken.handleRequest(request, next);
        }
        return next.handle(request);
    }

    private isApiUrl(url: string): boolean{
        const isApiUrl: boolean = url.startsWith(Constants.wikiApiUrl);
        const isTokenLoginUrl: boolean = url.endsWith('/token');
        const isTokenRefreshUrl: boolean = url.endsWith('/token/refresh');
        return isApiUrl && !isTokenLoginUrl && !isTokenRefreshUrl;
    }
}

Injectable({
    providedIn: 'root'
})
export class RefreshTokenService{
    private tokenRefreshInProgress: boolean = false;
    private refreshAccessTokenSubject: Subject<AccessToken> = new BehaviorSubject<AccessToken>(null);

    constructor(private userService: UserService){}

    public handleRequest(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
        const accessToken: AccessToken = this.userService.getAccessToken();

        if(this.tokenNeedsRefresh(accessToken)){
            return this.refreshToken().pipe(
                switchMap((token:AccessToken) => {
                    request = this.addTokenToRequest(token, request);
                    return next.handle(request);
                })
            )
        }
        if(this.hasToWaitForRefresh(accessToken)){
            return this.waitForRefreshToken.pipe(
                switchMap((token:AccessToken) => {
                    request = this.addTokenToRequest(token, request);
                    return next.handle(request);
                })
            )
          } 

          request = this.addTokenToRequest(accessToken, request);
          return next.handle(request);
    }

    private tokenNeedsRefresh(accessToken: AccessToken):boolean{
        return this.userService.isTokenExpired(accessToken) && !this.tokenRefreshInProgress
    }

    private hasToWaitForRefresh(accessToken: AccessToken):boolean{
        return this.userService.isTokenExpired(accessToken) && this.tokenRefreshInProgress
    }

    // Completes after first event
    private refreshToken():Observable<AccessToken>{
        return this.userService.refreshToken().pipe(
            map((authRespose):AccessToken => authRespose.access),
            tap((token:AccessToken) => {
                this.userService.setAccessToken(token);
                this.tokenRefreshInProgress = false;
                this.refreshAccessTokenSubject.next(token);
            })
        );
    }

    // Completes after first event
    private waitForRefreshToken():Observable<AccessToken>{
        return this.refreshAccessTokenSubject.pipe(
            filter(result => result !== null),
            first()
        )
    }

    private  addTokenToRequest(token: string, request: HttpRequest<any>): HttpRequest<any>{
        const httpHeaders = new HttpHeaders().set("Authorization", `Bearer ${token}`);
        request = request.clone({headers: httpHeaders});
        return request;
    }
}

At least that would be my approach. Three developers -> four approaches. And all are kind of valid :-)
Pick the parts you like and ignore the rest :-)

\$\endgroup\$
6
  • \$\begingroup\$ Thank you very much! This is helping (and teaching) me tons! Would have never thought of the potential stumbling block of the multiple interceptor instances as e.g. I'm importing the HTTPClientModule in root, but it's a valid point ! I'll accept this answer and likely add a reply later attempting to apply the suggestions from you that I can! I particularly liked how you managed to split off the event handling from the logic to decide when to do the event handling. \$\endgroup\$ – Philipp Doerner Nov 17 '20 at 12:39
  • \$\begingroup\$ Sidenote though. Previously (because it wasn't that much code) I had just thrown in the logic around Refreshtokens into my service for fetching and manipulating user data (for now I'm only at the stage that I'm fetching it, if at all). Since I am now moving this into its own service, wouldn't it also make sense to move the entire logic of setting and retrieving tokens into the refresh-token-service? \$\endgroup\$ – Philipp Doerner Nov 17 '20 at 12:47
  • \$\begingroup\$ Oh also, is it syntactically possible to not have the switchmap end on "return next handle" but instead just add the token to the request and be done in the switchmap at that point? This way, the logic would always flow to the one "return next.handle(request)" at the end, avoiding that I repeat that line. I'll be experimenting with this either way, I just don't know if that's somehow complicated... I really don't have a solid grasp on rxjs yet. \$\endgroup\$ – Philipp Doerner Nov 17 '20 at 12:57
  • \$\begingroup\$ When a file gets bigger, then i evaluate the "complexity" of the code and if its not VERY simple (i accept a 400 row switch statement as simple), i try to split it. Therefor i would seperate the logic for retrieving the token from the refreshing. I would move both services near each other, but still keep them in seperate files. \$\endgroup\$ – JanRecker Nov 17 '20 at 12:58
  • \$\begingroup\$ In theory you could try to have multiple event sources, combining them into one stream and handle the request manipulation there. Then, depending on the context, trigger the sources. But i would assume its much more complicated. For learning purpose, take a look at "rxjs-dev.firebaseapp.com/operator-decision-tree". It provides solutions for different situations \$\endgroup\$ – JanRecker Nov 17 '20 at 13:24
1
\$\begingroup\$

Thanks to the efforts of Jan Recker I gained a much better understanding of what was happening there. I followed all of his suggestions, but arrived at a different final code structure. My main difference was, that I also wanted jwt-interceptor.ts to contain all logic about handling the request and refresh-token.service.ts to only care about logic about refreshing tokens.

Thus I gained 4 files:

  1. jwttoken.ts - Contains the models/interfaces
  2. jwt-interceptor.ts - Contains the Interceptor and all logic to handle requests
  3. refresh-token.service.ts - Contains all logic specific to refreshing tokens
  4. token.service.ts - Contains logic to generally handle tokens

jwttoken.ts

export interface EncodedJWTToken{
    access: string,
    refresh: string,
}

export interface DecodedTokenPayload{
    exp: number,
    jti: string,
    token_type: string,
    user_id: number,
}

jwt-interceptor.ts

@Injectable()
export class JWTInterceptor implements HttpInterceptor{
    constructor(
        private refreshTokenService: RefreshTokenService,
        private tokenService: TokenService
    ){}

    public intercept(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>>{
        if (this.isApiUrl(request.url)){
            const accessToken = this.userService.getAccessToken();

            if (this.refreshTokenService.tokenNeedsRefresh(accessToken)){
                return this.handleByRefreshingAccessToken(request, next);
            }

            if (this.refreshTokenService.hasToWaitForRefresh(accessToken)){
                return this.handleByWaitingForRefresh(request, next);
            }

            request = this.addTokenToRequest(accessToken, request);
            return next.handle(request);
        } 

        return next.handle(request);
    }

    private handleByRefreshingAccessToken(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>>{
        return this.refreshTokenService.refreshAccessToken().pipe(
          switchMap((newAccessToken: string) => {
            request = this.addTokenToRequest(newAccessToken, request);
            return next.handle(request);
          }),
        )
      }
    
    private handleByWaitingForRefresh(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>>{
        return this.refreshTokenService.waitForAccessTokenRefresh().pipe(
            switchMap((newAccessToken: string) => {
            request = this.addTokenToRequest(newAccessToken, request);
            return next.handle(request);
            })
        )
    }

    private isApiUrl(url: string): boolean{
        const isApiUrl: boolean = url.startsWith(Constants.wikiApiUrl);
        const isTokenLoginUrl: boolean = url.endsWith('/token');
        const isTokenRefreshUrl: boolean = url.endsWith('/token/refresh');
        return isApiUrl && !isTokenLoginUrl && !isTokenRefreshUrl;
    }

    private addTokenToRequest(token: string, request: HttpRequest<any>): HttpRequest<any>{
        const httpHeaders = new HttpHeaders().set("Authorization", `Bearer ${token}`);
        request = request.clone({headers: httpHeaders});
        return request;   
    }
}

refresh-token.service.ts

@Injectable({
  providedIn: 'root'
})
export class RefreshTokenService {
  private tokenRefreshInProgress: boolean = false;
  private refreshAccessTokenSubject: Subject<string> = new BehaviorSubject<string>(null);

  constructor(private tokenService: TokenService) { }

  public refreshAccessToken(): Observable<string>{
    return this.tokenService.refreshToken().pipe(
      map((tokenResponse: EncodedJWTToken) => tokenResponse.access),
      tap((accessToken: string) => {
        this.tokenService.setAccessToken(accessToken);
        this.tokenRefreshInProgress = false;
        this.refreshAccessTokenSubject.next(accessToken);
      })
    )
  }

  public waitForAccessTokenRefresh(): Observable<string>{
    return this.refreshAccessTokenSubject.pipe(
      filter(result => result !== null),
      first()
    )
  }

  public tokenNeedsRefresh(accessToken: string): boolean{
    return this.tokenService.isTokenExpired(accessToken) && !this.tokenRefreshInProgress;
  }

  public hasToWaitForRefresh(accessToken: string): boolean{
    return this.tokenService.isTokenExpired(accessToken) && this.tokenRefreshInProgress;
  }
}

token.service.ts

@Injectable({
  providedIn: 'root'
})
export class TokenService {
  private jwtTokenUrl: string = Constants.wikiTokenUrl;
  private refreshTokenUrl: string = Constants.wikiTokenRefreshUrl;

  constructor(private http: HttpClient, private router: Router) { }

  public getAccessToken(): string{
    return localStorage.getItem(Constants.accessTokenKey);
  }

  public refreshToken(): Observable<EncodedJWTToken>{
    const refreshToken = this.getRefreshToken();
    return this.http.post<EncodedJWTToken>(this.refreshTokenUrl, {refresh: refreshToken});
  }

  public setAccessToken(token: string): void{
    if(this.decodeTokenPayload(token).token_type !== Constants.accessTokenType){
      throw "The Token you are trying to set as an Access Token is not an Access Token. Something is incorrectly handled about JWT token storage!"
    }
    localStorage.setItem(Constants.accessTokenKey, token);
  }

  public isTokenExpired(token: string): boolean{
    const payload: DecodedTokenPayload = this.decodeTokenPayload(token);
    const expiryTimestamp = payload.exp;
    const currentTimestamp = Math.floor((new Date).getTime()/1000);
    return currentTimestamp >= expiryTimestamp;
  }

  public decodeTokenPayload(token: string): DecodedTokenPayload{
    const [encodedHeader, encodedPayload, encodedSignature]: string[] = token.split('.');
    return JSON.parse(atob(encodedPayload));
  }
}
```
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.