diff --git a/Dockerfile b/Dockerfile index 70d1fb3..0f01100 100644 --- a/Dockerfile +++ b/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 diff --git a/src/authcache.ts b/src/authcache.ts index dcdf7fc..9653dec 100644 --- a/src/authcache.ts +++ b/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); } } diff --git a/src/gitlab.ts b/src/gitlab.ts index b1a15f8..4b193d8 100644 --- a/src/gitlab.ts +++ b/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 { private options: PluginOptions; 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> = new Map(); public constructor(config: VerdaccioGitlabConfig, options: PluginOptions) { this.logger = options.logger; @@ -84,15 +94,45 @@ export default class VerdaccioGitLab implements IPluginAuth { + 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 { 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]; @@ -100,7 +140,7 @@ export default class VerdaccioGitLab implements IPluginAuth 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) { diff --git a/test/__mocks__/gitlab.js b/test/__mocks__/gitlab.js index 2227175..e3f495e 100644 --- a/test/__mocks__/gitlab.js +++ b/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; diff --git a/test/unit/gitlab-concurrency.spec.ts b/test/unit/gitlab-concurrency.spec.ts new file mode 100644 index 0000000..a3b89fc --- /dev/null +++ b/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 { + 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); + }); + }); +});