fix(gitlab): stop re-walking every group and project per npm request

Dockerfile

@@ -1,4 +1,4 @@
-FROM node:12.16.3-alpine3.11 as builder
+FROM node:22-alpine AS builder
 
 WORKDIR /opt/verdaccio-gitlab-build
 COPY . .
@@ -14,7 +14,7 @@ RUN yarn config set registry $VERDACCIO_BUILD_REGISTRY && \
 
 
 
-FROM verdaccio/verdaccio:4
+FROM verdaccio/verdaccio:6
 LABEL maintainer="https://github.com/bufferoverflow/verdaccio-gitlab"
 
 # Go back to root to be able to install the plugin

src/authcache.ts

@@ -15,7 +15,9 @@ export class AuthCache {
     return 300;
   }
 
-  private static _generateKeyHash(username: string, password: string) {
+  // Public so the plugin can key its in-flight request map the same way the
+  // cache keys entries, without duplicating the hashing rule.
+  public static generateKeyHash(username: string, password: string) {
     const sha = Crypto.createHash('sha256');
     sha.update(JSON.stringify({ username: username, password: password }));
     return sha.digest('hex');
@@ -35,11 +37,11 @@ export class AuthCache {
   }
 
   public findUser(username: string, password: string): UserData {
-    return this.storage.get(AuthCache._generateKeyHash(username, password)) as UserData;
+    return this.storage.get(AuthCache.generateKeyHash(username, password)) as UserData;
   }
 
   public storeUser(username: string, password: string, userData: UserData): boolean {
-    return this.storage.set(AuthCache._generateKeyHash(username, password), userData);
+    return this.storage.set(AuthCache.generateKeyHash(username, password), userData);
   }
 }
 

src/gitlab.ts

@@ -38,12 +38,22 @@ const BUILTIN_ACCESS_LEVEL_ANONYMOUS = ['$anonymous', '$all'];
 // Level to apply on 'allow_access' calls when a package definition does not define one
 const DEFAULT_ALLOW_ACCESS_LEVEL = ['$all'];
 
+// Page size for the group and project queries. The GitLab API defaults to 20
+// and caps at 100, and the library passes this straight through to the query
+// string. On an instance with a few hundred projects this is the difference
+// between roughly 32 requests per login and roughly 7.
+const GITLAB_PAGE_SIZE = 100;
+
 export default class VerdaccioGitLab implements IPluginAuth<VerdaccioGitlabConfig> {
   private options: PluginOptions<VerdaccioGitlabConfig>;
   private config: VerdaccioGitlabConfig;
   private authCache?: AuthCache;
   private logger: Logger;
   private publishLevel: VerdaccioGitlabAccessLevel;
+  // Walks that are currently running, keyed by credentials. The auth cache is
+  // only written once a walk finishes, so without this every request that
+  // arrives while one is in flight starts its own walk against gitlab.
+  private inflightWalks: Map<string, Promise<string[]>> = new Map();
 
   public constructor(config: VerdaccioGitlabConfig, options: PluginOptions<VerdaccioGitlabConfig>) {
     this.logger = options.logger;
@@ -84,23 +94,53 @@ export default class VerdaccioGitLab implements IPluginAuth<VerdaccioGitlabConfi
     // Not found in cache, query gitlab
     this.logger.trace(`[gitlab] user: ${user} not found in cache`);
 
+    // A burst of npm requests arrives faster than one walk completes, so join
+    // an already running walk for the same credentials instead of starting
+    // another. Without this the cache never gets the chance to help.
+    const walkKey = AuthCache.generateKeyHash(user, password);
+    let walk = this.inflightWalks.get(walkKey);
+
+    if (walk) {
+      this.logger.trace(`[gitlab] user: ${user} joining gitlab query already in flight`);
+    } else {
+      walk = this._queryUserGroups(user, password);
+      this.inflightWalks.set(walkKey, walk);
+
+      // Release the slot on success and on failure alike, so a rejected walk
+      // is never replayed to later logins.
+      const release = () => {
+        this.inflightWalks.delete(walkKey);
+      };
+      walk.then(release, release);
+    }
+
+    walk.then(
+      realGroups => cb(null, realGroups),
+      error => cb(error)
+    );
+  }
+
+  // Resolves to the groups the user may publish to, or rejects with the
+  // verdaccio error to hand back. Split out of authenticate() so that a single
+  // walk can be shared by concurrent logins.
+  private _queryUserGroups(user: string, password: string): Promise<string[]> {
     const GitlabAPI = new Gitlab({
       url: this.config.url,
       token: password,
     });
 
-    GitlabAPI.Users.current()
-      .then(response => {
+    return GitlabAPI.Users.current().then(
+      response => {
         if (user.toLowerCase() !== response.username.toLowerCase()) {
-          return cb(getUnauthorized('wrong gitlab username'));
+          return Promise.reject(getUnauthorized('wrong gitlab username'));
         }
 
         const publishLevelId = ACCESS_LEVEL_MAPPING[this.publishLevel];
 
         // Set the groups of an authenticated user, in normal mode:
         // - for access, depending on the package settings in verdaccio
         // - for publish, the logged in user id and all the groups they can reach as configured with access level `$auth.gitlab.publish`
-        const gitlabPublishQueryParams = { min_access_level: publishLevelId };
+        const gitlabPublishQueryParams = { min_access_level: publishLevelId, per_page: GITLAB_PAGE_SIZE };
 
         this.logger.trace('[gitlab] querying gitlab user groups with params:', gitlabPublishQueryParams.toString());
 
@@ -112,25 +152,27 @@ export default class VerdaccioGitLab implements IPluginAuth<VerdaccioGitlabConfi
           return projects.map(project => project.path_with_namespace);
         });
 
-        Promise.all([groupsPromise, projectsPromise])
-          .then(([groups, projectGroups]) => {
+        return Promise.all([groupsPromise, projectsPromise]).then(
+          ([groups, projectGroups]) => {
             const realGroups = [user, ...groups, ...projectGroups];
             this._setCachedUserGroups(user, password, { publish: realGroups });
 
             this.logger.info(`[gitlab] user: ${user} successfully authenticated`);
             this.logger.debug(`[gitlab] user: ${user}, with groups:`, realGroups.toString());
 
-            return cb(null, realGroups);
-          })
-          .catch(error => {
+            return realGroups;
+          },
+          error => {
             this.logger.error(`[gitlab] user: ${user} error querying gitlab: ${error}`);
-            return cb(getUnauthorized('error authenticating user'));
-          });
-      })
-      .catch(error => {
+            return Promise.reject(getUnauthorized('error authenticating user'));
+          }
+        );
+      },
+      error => {
         this.logger.error(`[gitlab] user: ${user} error querying gitlab user data: ${error.message || {}}`);
-        return cb(getUnauthorized('error authenticating user'));
-      });
+        return Promise.reject(getUnauthorized('error authenticating user'));
+      }
+    );
   }
 
   public adduser(user: string, password: string, cb: Callback) {

test/__mocks__/gitlab.js

@@ -1,34 +1,58 @@
-const mock = jest.fn().mockImplementation(() => {
+// Manual mock for the `gitlab` library.
+//
+// The API calls are shared jest.fn() instances rather than fresh closures per
+// `new Gitlab()`, so tests can assert how many requests a single
+// authenticate() actually makes. They are exposed as `Gitlab.__calls`.
+//
+// `Users.current` honours the token: previously it resolved unconditionally,
+// which meant a wrong password authenticated successfully. The two "should
+// fail authentication" tests only passed because the assertion threw inside
+// the plugin's promise chain and its own .catch re-invoked the callback with
+// an error.
+const VALID_USER = 'myUser';
+const VALID_TOKEN = 'myPass';
+
+const usersCurrent = jest.fn(token => {
+  if (token !== VALID_TOKEN) {
+    return Promise.reject(new Error('401 - {"message":"401 Unauthorized"}'));
+  }
+
+  return Promise.resolve({ username: VALID_USER });
+});
+
+const groupsAll = jest.fn(() =>
+  Promise.resolve([
+    {
+      path: 'myGroup',
+      full_path: 'myGroup',
+    },
+  ])
+);
+
+const projectsAll = jest.fn(() =>
+  Promise.resolve([
+    {
+      path_with_namespace: 'anotherGroup/myProject',
+    },
+  ])
+);
+
+const mock = jest.fn().mockImplementation((options) => {
+  const token = (options || {}).token;
+
   return {
     Users: {
-      current: () => {
-        return Promise.resolve({
-          username: 'myUser',
-        });
-      },
+      current: () => usersCurrent(token),
     },
     Groups: {
-      all: params => {
-        // eslint-disable-line no-unused-vars
-        return Promise.resolve([
-          {
-            path: 'myGroup',
-            full_path: 'myGroup',
-          },
-        ]);
-      },
+      all: params => groupsAll(params),
     },
     Projects: {
-      all: params => {
-        // eslint-disable-line no-unused-vars
-        return Promise.resolve([
-          {
-            path_with_namespace: 'anotherGroup/myProject',
-          },
-        ]);
-      },
+      all: params => projectsAll(params),
     },
   };
 });
 
+mock.__calls = { usersCurrent, groupsAll, projectsAll };
+
 export default mock;

test/unit/gitlab-concurrency.spec.ts

@@ -0,0 +1,79 @@
+import Gitlab from 'gitlab';
+
+import VerdaccioGitlab from '../../src/gitlab';
+
+import config from './partials/config';
+
+const calls = (Gitlab as any).__calls;
+
+function authenticate(plugin: VerdaccioGitlab, user: string, pass: string): Promise<string[]> {
+  return new Promise((resolve, reject) => {
+    plugin.authenticate(user, pass, (err: any, data: any) => {
+      if (err) {
+        reject(err);
+      } else {
+        resolve(data);
+      }
+    });
+  });
+}
+
+describe('Gitlab Auth Plugin concurrency', () => {
+  beforeEach(() => {
+    calls.usersCurrent.mockClear();
+    calls.groupsAll.mockClear();
+    calls.projectsAll.mockClear();
+  });
+
+  test('concurrent logins with the same credentials share a single gitlab walk', () => {
+    const plugin: VerdaccioGitlab = new VerdaccioGitlab(config.verdaccioGitlabConfig, config.options);
+
+    // Fire them before any of them can resolve, which is exactly what a burst
+    // of npm requests does. The auth cache cannot help here: it is only
+    // written once the walk finishes.
+    const logins = [1, 2, 3, 4, 5].map(() => authenticate(plugin, config.user, config.pass));
+
+    return Promise.all(logins).then(results => {
+      results.forEach(groups => {
+        expect(groups.sort()).toEqual(['myGroup', 'anotherGroup/myProject', 'myUser'].sort());
+      });
+
+      expect(calls.groupsAll).toHaveBeenCalledTimes(1);
+      expect(calls.projectsAll).toHaveBeenCalledTimes(1);
+      expect(calls.usersCurrent).toHaveBeenCalledTimes(1);
+    });
+  });
+
+  test('a failed walk is not cached, so the next login retries', () => {
+    const plugin: VerdaccioGitlab = new VerdaccioGitlab(config.verdaccioGitlabConfig, config.options);
+    const wrongPass = config.pass + '_wrong';
+
+    return authenticate(plugin, config.user, wrongPass)
+      .then(
+        () => {
+          throw new Error('authentication should have failed');
+        },
+        () => authenticate(plugin, config.user, wrongPass).then(
+          () => {
+            throw new Error('authentication should have failed');
+          },
+          () => {
+            // Two attempts must reach gitlab: a rejected walk must not leave a
+            // stale entry that makes later logins fail without asking.
+            expect(calls.usersCurrent).toHaveBeenCalledTimes(2);
+          }
+        )
+      );
+  });
+
+  test('group and project queries ask for the maximum page size', () => {
+    const plugin: VerdaccioGitlab = new VerdaccioGitlab(config.verdaccioGitlabConfig, config.options);
+
+    return authenticate(plugin, config.user, config.pass).then(() => {
+      const expected = { min_access_level: 40, per_page: 100 };
+
+      expect(calls.groupsAll).toHaveBeenCalledWith(expected);
+      expect(calls.projectsAll).toHaveBeenCalledWith(expected);
+    });
+  });
+});