fix: make sure http only retry one time on 401

okta · May 25, 2020 · 471d5be · 471d5be
1 parent fcaa560
commit 471d5be
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 72 deletions.
diff --git a/src/http.js b/src/http.js
@@ -69,45 +69,44 @@ class Http {
     request.headers = Object.assign(this.defaultHeaders, request.headers);
     request.method = request.method || 'get';
 
-    const promise = this.prepareRequest(request)
-      .then(() => this.requestExecutor.fetch(request))
-      .then(Http.errorFilter)
-      .catch(error => {
-        // Handle oauth access token expiration
-        if (this.oauth && error && error.status === 401) {
-          this.oauth.clearCachedAccessToken();
-          return this.oauth.introspectAccessToken()
-            .then(({ active }) => {
-              if (active === false) {
-                return this.http(uri, request, context);
-              }
-              // Access token is still active or other error happen, re-throw
-              throw error;
-            });
-        }
+    let retriedOnAuthError = false;
+    const execute = () => {
+      const promise = this.prepareRequest(request)
+        .then(() => this.requestExecutor.fetch(request))
+        .then(Http.errorFilter)
+        .catch(error => {
+          // Clear cached token then retry request one more time
+          if (this.oauth && error && error.status === 401 && !retriedOnAuthError) {
+            retriedOnAuthError = true;
+            this.oauth.clearCachedAccessToken();
+            return execute();
+          }
 
-        throw error;
-      });
+          throw error;
+        });
 
-    if (!this.cacheMiddleware) {
-      return promise;
-    }
+      if (!this.cacheMiddleware) {
+        return promise;
+      }
+
+      const ctx = {
+        uri, // TODO: remove unused property. req.url should be the key.
+        isCollection: context.isCollection,
+        resources: context.resources,
+        req: request,
+        cacheStore: this.cacheStore
+      };
+      return this.cacheMiddleware(ctx, () => {
+        if (ctx.res) {
+          return;
+        }
 
-    const ctx = {
-      uri, // TODO: remove unused property. req.url should be the key.
-      isCollection: context.isCollection,
-      resources: context.resources,
-      req: request,
-      cacheStore: this.cacheStore
+        return promise.then(res => ctx.res = res);
+      })
+      .then(() => ctx.res);
     };
-    return this.cacheMiddleware(ctx, () => {
-      if (ctx.res) {
-        return;
-      }
 
-      return promise.then(res => ctx.res = res);
-    })
-    .then(() => ctx.res);
+    return execute();
   }
 
   delete(uri, request, context) {

diff --git a/src/oauth.js b/src/oauth.js
@@ -57,34 +57,6 @@ class OAuth {
       });
   }
 
-  introspectAccessToken() {
-    if (!this.accessToken) {
-      return Promise.reject(new Error('No accessToken in cache to be introspected.'));
-    }
-
-    const endpoint = '/oauth2/v1/introspect';
-    return this.getJwt(endpoint)
-      .then(jwt => {
-        const params = formatParams({
-          token: this.accessToken,
-          token_type_hint: 'access_token',
-          client_assertion_type: 'urn:ietf:params:oauth:client-assertion-type:jwt-bearer',
-          client_assertion: jwt
-        });
-        return this.client.requestExecutor.fetch({
-          url: `${this.client.baseUrl}${endpoint}`,
-          method: 'POST',
-          body: params,
-          headers: {
-            'Accept': 'application/json',
-            'Content-Type': 'application/x-www-form-urlencoded'
-          }
-        });
-      })
-      .then(Http.errorFilter)
-      .then(res => res.json());
-  }
-
   clearCachedAccessToken() {
     this.accessToken = null;
   }

diff --git a/test/jest/http.test.js b/test/jest/http.test.js
@@ -402,8 +402,8 @@ describe('Http class', () => {
     });
 
     describe('Oauth', () => {
-      it('should get new token and retry request when accessToken is not active any more', () => {
-        expect.assertions(5);
+      it('should retry to get new token when response staus is 401', () => {
+        expect.assertions(4);
         requestExecutor = {
           fetch: jest.fn().mockImplementation((request) => {
             if (request.headers.Authorization === 'Bearer expired_token') {
@@ -424,31 +424,55 @@ describe('Http class', () => {
           }),
           clearCachedAccessToken: jest.fn().mockImplementation(() => {
             oauth.accessToken = null;
-          }),
-          introspectAccessToken: jest.fn().mockResolvedValueOnce({ active: false })
+          })
         };
         const http = new Http({ requestExecutor, oauth });
         jest.spyOn(http, 'http');
         return http.http('http://fakey.local')
           .then(res => {
-            expect(http.http).toHaveBeenCalledTimes(2);
+            expect(http.http).toHaveBeenCalledTimes(1);
             expect(oauth.getAccessToken).toHaveBeenCalledTimes(2);
-            expect(oauth.introspectAccessToken).toHaveBeenCalledTimes(1);
             expect(oauth.clearCachedAccessToken).toHaveBeenCalledTimes(1);
             expect(res.status).toEqual(200);
           });
       });
-      it('should throw error if status is 401, but access token is still active', () => {
+      it('should retry only one time when response staus is 401', () => {
+        expect.assertions(5);
+        requestExecutor = {
+          fetch: jest.fn().mockImplementation((request) => {
+            if (request.headers.Authorization === 'Bearer invalid_token') {
+              response.status = 401;
+            } else if (request.headers.Authorization === 'Bearer valid_token') {
+              response.status = 200;
+            }
+            return Promise.resolve(response);
+          })
+        };
+        const oauth = {
+          getAccessToken: jest.fn().mockResolvedValue({ access_token: 'invalid_token' }),
+          clearCachedAccessToken: jest.fn()
+        };
+        const http = new Http({ requestExecutor, oauth });
+        jest.spyOn(http, 'http');
+        return http.http('http://fakey.local')
+          .catch(error => {
+            expect(http.http).toHaveBeenCalledTimes(1);
+            expect(oauth.getAccessToken).toHaveBeenCalledTimes(2);
+            expect(oauth.clearCachedAccessToken).toHaveBeenCalledTimes(1);
+            expect(error).toBeInstanceOf(OktaApiError);
+            expect(error.status).toEqual(401);
+          });
+      });
+      it('should throw error from oauth.getAccessToken', () => {
         expect.assertions(1);
         response.status = 401;
         const oauth = {
-          getAccessToken: jest.fn().mockResolvedValueOnce({ access_token: 'fake token' }),
-          introspectAccessToken: jest.fn().mockResolvedValueOnce({ active: true })
+          getAccessToken: jest.fn().mockRejectedValueOnce(new Error('bad jwk'))
         };
         const http = new Http({ requestExecutor, oauth });
         return http.http('http://fakey.local')
           .catch(err => {
-            expect(err).toBeInstanceOf(OktaApiError);
+            expect(err.message).toEqual('bad jwk');
           });
       });
     });