diff --git a/src/proxy/chain.ts b/src/proxy/chain.ts index cab32f5c4..18fa0db47 100644 --- a/src/proxy/chain.ts +++ b/src/proxy/chain.ts @@ -43,6 +43,7 @@ const branchPushChain: ProcessorExec[] = [ ]; const tagPushChain: ProcessorExec[] = [ + proc.push.resolveUserFromToken, proc.push.checkRepoInAuthorisedList, proc.push.checkUserPushPermission, proc.push.checkIfWaitingAuth, diff --git a/src/proxy/processors/push-action/resolveUserFromToken.ts b/src/proxy/processors/push-action/resolveUserFromToken.ts index 54c8e2099..2476fe1b1 100644 --- a/src/proxy/processors/push-action/resolveUserFromToken.ts +++ b/src/proxy/processors/push-action/resolveUserFromToken.ts @@ -73,8 +73,11 @@ async function exec(req: Request, action: Action): Promise { const cached = scmTokenCache.lookup(provider.name, token); if (cached) { - step.log(`${provider.name}: resolved push identity from cache: ${cached}`); - action.user = cached; + step.log(`${provider.name}: resolved push identity from cache: ${cached.username}`); + action.user = cached.username; + if (cached.email) { + action.userEmail = cached.email; + } action.addStep(step); return action; } @@ -97,7 +100,7 @@ async function exec(req: Request, action: Action): Promise { ); action.user = user.username; action.userEmail = user.email; - scmTokenCache.store(provider.name, token, user.username); + scmTokenCache.store(provider.name, token, user.username, user.email); } else { step.log( `No git-proxy user has gitAccount '${identity.login}' — ` + diff --git a/src/proxy/processors/push-action/tokenIdentity.ts b/src/proxy/processors/push-action/tokenIdentity.ts index 993ee6287..7690fbc9e 100644 --- a/src/proxy/processors/push-action/tokenIdentity.ts +++ b/src/proxy/processors/push-action/tokenIdentity.ts @@ -20,7 +20,9 @@ export type ScmUserInfo = { login: string; }; -type CacheEntry = { username: string; provider: string; cachedAt: number }; +export type CachedIdentity = { username: string; email: string | null }; + +type CacheEntry = CachedIdentity & { provider: string; cachedAt: number }; // 7 days — PATs are rarely rotated more frequently than this in practice; the cache is a // rate-limit optimization only (keys are one-way SHA-512 hashes, not recoverable tokens). const DEFAULT_TTL_MS = 7 * 24 * 60 * 60 * 1000; @@ -37,7 +39,7 @@ export class ScmTokenCache { return crypto.createHash('sha512').update(`${provider}:${token}`).digest('hex'); } - lookup(provider: string, token: string): string | null { + lookup(provider: string, token: string): CachedIdentity | null { const k = this.key(provider, token); const entry = this.cache.get(k); if (!entry) return null; @@ -46,11 +48,16 @@ export class ScmTokenCache { return null; } entry.cachedAt = Date.now(); - return entry.username; + return { username: entry.username, email: entry.email ?? null }; } - store(provider: string, token: string, username: string): void { - this.cache.set(this.key(provider, token), { username, provider, cachedAt: Date.now() }); + store(provider: string, token: string, username: string, email: string | null = null): void { + this.cache.set(this.key(provider, token), { + username, + email, + provider, + cachedAt: Date.now(), + }); } evictByUsername(provider: string, username: string): void { diff --git a/test/chain.test.ts b/test/chain.test.ts index afc6d7e1b..bdb4880e5 100644 --- a/test/chain.test.ts +++ b/test/chain.test.ts @@ -564,6 +564,14 @@ describe('proxy chain', function () { expect(pullChain).toEqual(chain.pullActionChain); }); + it('runs resolveUserFromToken before checkUserPushPermission on tag pushes', () => { + const tag = chain.tagPushChain; + const resolveIdx = tag.indexOf(processors.push.resolveUserFromToken); + const permIdx = tag.indexOf(processors.push.checkUserPushPermission); + expect(resolveIdx).toBeGreaterThanOrEqual(0); + expect(permIdx).toBeGreaterThan(resolveIdx); + }); + it('returns tagPushChain when action.type is push and action.actionType is TAG', async () => { const action = new Action( '2', diff --git a/test/processors/resolveUserFromToken.test.ts b/test/processors/resolveUserFromToken.test.ts index 14eec0c72..015d34bac 100644 --- a/test/processors/resolveUserFromToken.test.ts +++ b/test/processors/resolveUserFromToken.test.ts @@ -131,7 +131,16 @@ describe('ScmTokenCache', () => { it('should return username on cache hit', () => { const cache = new ScmTokenCache(); cache.store('github', 'sometoken', 'octocat'); - expect(cache.lookup('github', 'sometoken')).toBe('octocat'); + expect(cache.lookup('github', 'sometoken')).toEqual({ username: 'octocat', email: null }); + }); + + it('should return stored email on cache hit', () => { + const cache = new ScmTokenCache(); + cache.store('github', 'sometoken', 'octocat', 'octocat@github.com'); + expect(cache.lookup('github', 'sometoken')).toEqual({ + username: 'octocat', + email: 'octocat@github.com', + }); }); it('should return null after TTL expires', () => { @@ -153,10 +162,10 @@ describe('ScmTokenCache', () => { provider: 'github', cachedAt: Date.now() - 90, }); - expect(cache.lookup('github', 'sometoken')).toBe('octocat'); // hit resets cachedAt + expect(cache.lookup('github', 'sometoken')).toEqual({ username: 'octocat', email: null }); // hit resets cachedAt // backdate again to 90ms — if TTL had not been reset, this would be 180ms total (expired) (cache as any).cache.get(key).cachedAt = Date.now() - 90; - expect(cache.lookup('github', 'sometoken')).toBe('octocat'); // still valid because TTL was reset + expect(cache.lookup('github', 'sometoken')).toEqual({ username: 'octocat', email: null }); // still valid because TTL was reset }); it('should not share entries across providers', () => { @@ -173,14 +182,14 @@ describe('ScmTokenCache', () => { cache.evictByUsername('github', 'alice'); expect(cache.lookup('github', 'token1')).toBeNull(); expect(cache.lookup('github', 'token2')).toBeNull(); - expect(cache.lookup('github', 'token3')).toBe('bob'); + expect(cache.lookup('github', 'token3')).toEqual({ username: 'bob', email: null }); }); it('should not evict across providers', () => { const cache = new ScmTokenCache(); cache.store('github', 'sometoken', 'alice'); cache.evictByUsername('gitlab', 'alice'); - expect(cache.lookup('github', 'sometoken')).toBe('alice'); + expect(cache.lookup('github', 'sometoken')).toEqual({ username: 'alice', email: null }); }); }); @@ -399,4 +408,29 @@ describe('resolveUserFromToken cache integration', () => { expect(result.user).toBe('cached-user'); expect(fetchSpy).not.toHaveBeenCalled(); }); + + it('should set userEmail from cache so permission checks the pusher not the last committer', async () => { + vi.doMock('../../src/db', () => ({ + findUserByGitAccount: vi.fn(), + })); + vi.doMock('../../src/proxy/processors/push-action/tokenIdentity', async () => { + const real = await vi.importActual< + typeof import('../../src/proxy/processors/push-action/tokenIdentity') + >('../../src/proxy/processors/push-action/tokenIdentity'); + const cache = new real.ScmTokenCache(); + cache.store('github', 'ghp_testtoken123', 'bob', 'bob@corp.example'); + return { ...real, scmTokenCache: cache }; + }); + const mod = await import('../../src/proxy/processors/push-action/resolveUserFromToken'); + const req = makeRequest(); + const action = makeAction('https://github.com/finos/git-proxy.git'); + action.user = 'eve'; + action.userEmail = 'eve@corp.example'; + + const result = await mod.exec(req, action); + + expect(result.user).toBe('bob'); + expect(result.userEmail).toBe('bob@corp.example'); + expect(fetchSpy).not.toHaveBeenCalled(); + }); });