fix(auth): Improve concurrency logic in oauth controller code

lib/package.json

@@ -67,6 +67,7 @@
     "watch": "(trap 'kill 0' SIGINT; npm run build && (npm run build:watch & npm run test -- --watch))"
   },
   "dependencies": {
+    "async-mutex": "^0.5.0",
     "axios": "^1.6.1",
     "axios-retry": "^3.9.0",
     "base64-js": "^1.5.1",

lib/src/auth/oidc.ts

@@ -1,4 +1,5 @@
 import { default as dpopFn } from 'dpop';
+import { Mutex } from 'async-mutex';
 import { HttpRequest, withHeaders } from './auth.js';
 import { base64 } from '../encodings/index.js';
 import { ConfigurationError, TdfError } from '../errors.js';
@@ -57,6 +58,7 @@ const qstringify = (obj: Record<string, string>) => new URLSearchParams(obj).toS
 export type AccessTokenResponse = {
   access_token: string;
   refresh_token?: string;
+  timestamp?: number;
 };
 
 /**
@@ -96,6 +98,8 @@ export class AccessToken {
 
   currentAccessToken?: string;
 
+  mutex: Mutex = new Mutex();
+
   constructor(cfg: OIDCCredentials, request?: typeof fetch) {
     if (!cfg.clientId) {
       throw new ConfigurationError(
@@ -205,38 +209,47 @@ export class AccessToken {
         `token/code exchange fail: POST [${url}] => ${response.status} ${response.statusText}`
       );
     }
-    return response.json();
+    const r = await response.json();
+    r.timestamp = Date.now();
+    return r;
   }
 
   /**
    * Gets an access token; operates lazily/cached, with an optional check for freshness.
    * @param validate if we should run a inline check against the OIDC 'userinfo' endpoint to make sure any cached access token is still valid
    * @returns
    */
-  async get(validate = true): Promise<string> {
-    if (this.data?.access_token) {
-      try {
-        if (validate) {
-          await this.info(this.data.access_token);
-        }
-        return this.data.access_token;
-      } catch (e) {
-        console.log('access_token fails on user_info endpoint; attempting to renew', e);
-        if (this.data.refresh_token) {
-          // Prefer the latest refresh_token if present over creds passed in
-          // to constructor
-          this.config = {
-            ...this.config,
-            exchange: 'refresh',
-            refreshToken: this.data.refresh_token,
-          };
+  async get(validate?: boolean): Promise<string> {
+    const now = Date.now();
+    const release = await this.mutex.acquire();
+    try {
+      if (this.data?.access_token) {
+        try {
+          // Validation was explicitly requested OR the token is older than 5 minutes.
+          if (validate || (this.data.timestamp && this.data.timestamp + 1000 * 60 * 5 < now)) {
+            await this.info(this.data.access_token);
+          }
+          return this.data.access_token;
+        } catch (e) {
+          console.log('access_token fails on user_info endpoint; attempting to renew', e);
+          if (this.data.refresh_token) {
+            // Prefer the latest refresh_token if present over creds passed in
+            // to constructor
+            this.config = {
+              ...this.config,
+              exchange: 'refresh',
+              refreshToken: this.data.refresh_token,
+            };
+          }
+          delete this.data;
         }
-        delete this.data;
       }
-    }
 
-    const tokenResponse = (this.data = await this.accessTokenLookup(this.config));
-    return tokenResponse.access_token;
+      const tokenResponse = (this.data = await this.accessTokenLookup(this.config));
+      return tokenResponse.access_token;
+    } finally {
+      release();
+    }
   }
 
   /**
@@ -261,27 +274,31 @@ export class AccessToken {
    * Converts included refresh token or external JWT for a new one.
    */
   async exchangeForRefreshToken(): Promise<string> {
-    const cfg = this.config;
-    if (cfg.exchange != 'external' && cfg.exchange != 'refresh') {
-      throw new ConfigurationError('no refresh token provided!');
-    }
-    const tokenResponse = (this.data = await this.accessTokenLookup(this.config));
-    if (!tokenResponse.refresh_token) {
-      console.log('No refresh_token returned');
-      return (
-        (cfg.exchange == 'refresh' && cfg.refreshToken) ||
-        (cfg.exchange == 'external' && cfg.externalJwt) ||
-        ''
-      );
+    const release = await this.mutex.acquire();
+    try {
+      const cfg = this.config;
+      if (cfg.exchange != 'external' && cfg.exchange != 'refresh') {
+        throw new ConfigurationError('no refresh token provided!');
+      }
+      const tokenResponse = (this.data = await this.accessTokenLookup(this.config));
+      if (!tokenResponse.refresh_token) {
+        return (
+          (cfg.exchange == 'refresh' && cfg.refreshToken) ||
+          (cfg.exchange == 'external' && cfg.externalJwt) ||
+          ''
+        );
+      }
+      // Prefer the latest refresh_token if present over creds passed in
+      // to constructor
+      this.config = {
+        ...this.config,
+        exchange: 'refresh',
+        refreshToken: tokenResponse.refresh_token,
+      };
+      return tokenResponse.access_token;
+    } finally {
+      release();
     }
-    // Prefer the latest refresh_token if present over creds passed in
-    // to constructor
-    this.config = {
-      ...this.config,
-      exchange: 'refresh',
-      refreshToken: tokenResponse.refresh_token,
-    };
-    return tokenResponse.access_token;
   }
 
   async withCreds(httpReq: HttpRequest): Promise<HttpRequest> {

lib/tests/web/auth/auth.test.ts

@@ -298,8 +298,8 @@ describe('AccessToken', () => {
         mf
       );
       accessTokenClient.data = {
-        refresh_token: 'r',
         access_token: 'a',
+        refresh_token: 'r',
       };
       // Do a refresh to cache tokenset
       const res = await accessTokenClient.get();
@@ -309,15 +309,30 @@ describe('AccessToken', () => {
     });
     it('should attempt to refresh token if userinfo call throws error', async () => {
       const signingKey = await crypto.subtle.generateKey(algorithmSigner, true, ['sign']);
-      const json = fake.resolves({ access_token: 'a' });
+      const json = fake.resolves({ access_token: 'A' });
       const mf = fake((url: RequestInfo | URL, init?: RequestInit): Promise<Response> => {
         if (!init) {
           return Promise.reject('No init found');
         }
-        if (init.method === 'POST') {
+        if (typeof (url as unknown) == 'string') {
+          url = new URL(url as string);
+        } else if (!(url instanceof URL)) {
+          return Promise.reject('url is not a string or URL');
+        }
+        if (init.method === 'POST' && url.pathname.endsWith('token')) {
           return Promise.resolve({ ...ok, json });
         }
-        return Promise.reject(`yee [${url}] [${JSON.stringify(init.headers)}]`);
+        if (url.pathname.endsWith('userinfo')) {
+          if (
+            !init.headers ||
+            !('Authorization' in init.headers) ||
+            init.headers.Authorization !== 'Bearer A'
+          ) {
+            return Promise.reject('yee');
+          }
+          return Promise.resolve({ ...ok, json: fake.resolves({ email: 'user@some.where' }) });
+        }
+        return Promise.reject('zee');
       });
       const accessTokenClient = new AccessToken(
         {
@@ -330,12 +345,19 @@ describe('AccessToken', () => {
         mf
       );
       accessTokenClient.data = {
-        refresh_token: 'r',
         access_token: 'a',
+        refresh_token: 'r',
+        timestamp: Date.now() - 1000 * 60 * 60,
       };
       // Do a refresh to cache tokenset
-      const res = await accessTokenClient.get();
-      expect(res).to.eql('a');
+      const res = await Promise.all([
+        accessTokenClient.get(),
+        accessTokenClient.get(),
+        accessTokenClient.get(),
+        accessTokenClient.get(),
+        accessTokenClient.get(),
+      ]);
+      expect(res).to.include('A');
       expect(mf.callCount).to.eql(2);
     });
   });