diff --git a/core/token-pooling/sql-token-persistence.integration.js b/core/token-pooling/sql-token-persistence.integration.js index 9658d9d2b0b7f..fdcba25073de0 100644 --- a/core/token-pooling/sql-token-persistence.integration.js +++ b/core/token-pooling/sql-token-persistence.integration.js @@ -66,7 +66,28 @@ describe('SQL token persistence', function () { it('loads the contents', async function () { const tokens = await persistence.initialize(pool) - expect(tokens.sort()).to.deep.equal(initialTokens) + expect(tokens.map(({ token }) => token).sort()).to.deep.equal( + initialTokens, + ) + expect(tokens.map(({ scopes }) => scopes)).to.deep.equal([ + null, + null, + null, + ]) + }) + + it('loads token scopes', async function () { + const scopedToken = 'd'.repeat(40) + await pool.query( + `INSERT INTO pg_temp.${tableName} (token, scopes) VALUES ($1::text, $2::text[]);`, + [scopedToken, ['read:packages', 'read:user']], + ) + + const tokens = await persistence.initialize(pool) + expect(tokens).to.deep.include({ + token: scopedToken, + scopes: ['read:packages', 'read:user'], + }) }) context('when tokens are added', function () { @@ -84,6 +105,40 @@ describe('SQL token persistence', function () { const savedTokens = result.rows.map(row => row.token) expect(savedTokens.sort()).to.deep.equal(expected) }) + + it('saves token scopes', async function () { + const newToken = 'e'.repeat(40) + + await persistence.initialize(pool) + await persistence.noteTokenAdded(newToken, { + scopes: ['read:packages'], + }) + + const result = await pool.query( + `SELECT token, scopes FROM pg_temp.${tableName} WHERE token=$1::text;`, + [newToken], + ) + expect(result.rows).to.deep.equal([ + { token: newToken, scopes: ['read:packages'] }, + ]) + }) + + it('updates token scopes when a known token is re-added with scopes', async function () { + const knownToken = initialTokens[0] + + await persistence.initialize(pool) + await persistence.noteTokenAdded(knownToken, { + scopes: ['read:packages'], + }) + + const result = await pool.query( + `SELECT token, scopes FROM pg_temp.${tableName} WHERE token=$1::text;`, + [knownToken], + ) + expect(result.rows).to.deep.equal([ + { token: knownToken, scopes: ['read:packages'] }, + ]) + }) }) context('when tokens are removed', function () { diff --git a/core/token-pooling/sql-token-persistence.js b/core/token-pooling/sql-token-persistence.js index 610df1b973c09..2bd4e196262ba 100644 --- a/core/token-pooling/sql-token-persistence.js +++ b/core/token-pooling/sql-token-persistence.js @@ -16,9 +16,9 @@ export default class SqlTokenPersistence { this.pool = new pg.Pool({ connectionString: this.url }) } const result = await this.pool.query( - `SELECT token FROM ${this.table} ORDER BY RANDOM();`, + `SELECT token, scopes FROM ${this.table} ORDER BY RANDOM();`, ) - return result.rows.map(row => row.token) + return result.rows.map(({ token, scopes }) => ({ token, scopes })) } async stop() { @@ -27,10 +27,16 @@ export default class SqlTokenPersistence { } } - async onTokenAdded(token) { + async onTokenAdded(token, { scopes } = {}) { return await this.pool.query( - `INSERT INTO ${this.table} (token) VALUES ($1::text) ON CONFLICT (token) DO NOTHING;`, - [token], + ` + INSERT INTO ${this.table} (token, scopes) + VALUES ($1::text, $2::text[]) + ON CONFLICT (token) DO UPDATE + SET scopes = EXCLUDED.scopes + WHERE EXCLUDED.scopes IS NOT NULL; + `, + [token, scopes], ) } @@ -41,9 +47,9 @@ export default class SqlTokenPersistence { ) } - async noteTokenAdded(token) { + async noteTokenAdded(token, data) { try { - await this.onTokenAdded(token) + await this.onTokenAdded(token, data) } catch (e) { log.error(e) } diff --git a/core/token-pooling/sql-token-persistence.spec.js b/core/token-pooling/sql-token-persistence.spec.js new file mode 100644 index 0000000000000..d22b55d54f36b --- /dev/null +++ b/core/token-pooling/sql-token-persistence.spec.js @@ -0,0 +1,50 @@ +import { expect } from 'chai' +import sinon from 'sinon' +import SqlTokenPersistence from './sql-token-persistence.js' + +describe('SQL token persistence unit', function () { + const tableName = 'github_user_tokens' + + let pool + let persistence + + beforeEach(function () { + pool = { query: sinon.stub() } + persistence = new SqlTokenPersistence({ + url: 'postgres://example.test/shields', + table: tableName, + }) + }) + + it('loads token scopes', async function () { + pool.query.resolves({ + rows: [ + { token: 'unscoped-token', scopes: null }, + { token: 'scoped-token', scopes: ['read:packages'] }, + ], + }) + + const tokens = await persistence.initialize(pool) + + expect(tokens).to.deep.equal([ + { token: 'unscoped-token', scopes: null }, + { token: 'scoped-token', scopes: ['read:packages'] }, + ]) + sinon.assert.calledOnceWithExactly( + pool.query, + `SELECT token, scopes FROM ${tableName} ORDER BY RANDOM();`, + ) + }) + + it('saves token scopes', async function () { + pool.query.resolves({ rows: [] }) + await persistence.initialize(pool) + + await persistence.onTokenAdded('scoped-token', { + scopes: ['read:packages'], + }) + + const [, params] = pool.query.secondCall.args + expect(params).to.deep.equal(['scoped-token', ['read:packages']]) + }) +}) diff --git a/core/token-pooling/token-pool.js b/core/token-pooling/token-pool.js index 441daedb88a8c..8c56c4a5d6a1e 100644 --- a/core/token-pooling/token-pool.js +++ b/core/token-pooling/token-pool.js @@ -84,6 +84,17 @@ class Token { return this._usesRemaining - 1 } + /** + * Update user-provided data associated with the token. + * + * Callers should only invoke this when they have fresh data to store. + * + * @param {*} data reserved for future use + */ + updateData(data) { + this._data = data + } + /** * Update the uses remaining and next reset time for a token. * @@ -128,6 +139,13 @@ class Token { this._isValid = false } + /** + * Indicate that the token should be used again. + */ + validate() { + this._isValid = true + } + /** * Freeze the uses remaining and next reset values. Helpful for keeping * stable ordering for a valid priority queue. @@ -184,8 +202,8 @@ class TokenPool { this.currentBatch = { currentToken: null, remaining: 0 } - // A set of IDs used for deduplication. - this.tokenIds = new Set() + // A map of IDs to tokens, used for deduplication and metadata updates. + this.tokensById = new Map() // See discussion on the FIFO and priority queues in `next()`. this.fifoQueue = [] @@ -203,9 +221,23 @@ class TokenPool { return second.nextReset - first.nextReset } + _removeFromPriorityQueue(token) { + const priorityQueue = new PriorityQueue(this.constructor.compareTokens) + this.priorityQueue.forEach(queuedToken => { + if (queuedToken !== token) { + priorityQueue.enq(queuedToken) + } + }) + this.priorityQueue = priorityQueue + } + /** * Add a token with user-provided ID and data. * + * Adding an existing token updates its data only when `data` is provided. + * This keeps calls which only revalidate or deduplicate a token from + * clearing previously stored metadata. + * * @param {string} id token string * @param {*} data reserved for future use * @param {number} usesRemaining @@ -216,15 +248,32 @@ class TokenPool { * @returns {boolean} Was the token added to the pool? */ add(id, data, usesRemaining, nextReset) { - if (this.tokenIds.has(id)) { + const existingToken = this.tokensById.get(id) + if (existingToken) { + const wasInvalid = !existingToken.isValid + if (data !== undefined) { + existingToken.updateData(data) + } + if (wasInvalid) { + this._removeFromPriorityQueue(existingToken) + existingToken.unfreeze() + } + existingToken.validate() + const isQueued = this.fifoQueue.includes(existingToken) + const isCurrent = + this.currentBatch.token === existingToken && + this.currentBatch.remaining > 0 + if (wasInvalid && !isQueued && !isCurrent) { + this.fifoQueue.push(existingToken) + } return false } - this.tokenIds.add(id) usesRemaining = usesRemaining === undefined ? this.batchSize : usesRemaining nextReset = nextReset === undefined ? Token.nextResetNever : nextReset const token = new Token(id, data, usesRemaining, nextReset) + this.tokensById.set(id, token) this.fifoQueue.push(token) return true diff --git a/core/token-pooling/token-pool.spec.js b/core/token-pooling/token-pool.spec.js index 95208feaa046f..8016e817e8735 100644 --- a/core/token-pooling/token-pool.spec.js +++ b/core/token-pooling/token-pool.spec.js @@ -35,6 +35,83 @@ describe('The token pool', function () { ) }) + it('updates data when an existing token is added again', function () { + const tokenPool = new TokenPool() + + expect(tokenPool.add('token', { scopes: [] })).to.equal(true) + expect(tokenPool.add('token', { scopes: ['read:packages'] })).to.equal( + false, + ) + + expect(tokenPool.next().data).to.deep.equal({ + scopes: ['read:packages'], + }) + }) + + it('preserves existing data when an existing token is added again without data', function () { + const tokenPool = new TokenPool() + + expect(tokenPool.add('token', { scopes: ['read:packages'] })).to.equal(true) + expect(tokenPool.add('token')).to.equal(false) + + expect(tokenPool.next().data).to.deep.equal({ + scopes: ['read:packages'], + }) + }) + + it('revalidates invalid tokens when they are added again', function () { + const tokenPool = new TokenPool() + expect(tokenPool.add('token', { scopes: [] })).to.equal(true) + + tokenPool.next().invalidate() + expectPoolToBeExhausted(tokenPool) + + expect(tokenPool.add('token', { scopes: ['read:packages'] })).to.equal( + false, + ) + + const token = tokenPool.next() + expect(token.id).to.equal('token') + expect(token.data).to.deep.equal({ scopes: ['read:packages'] }) + }) + + it('does not duplicate queued tokens when they are revalidated', function () { + const tokenPool = new TokenPool({ batchSize: 2 }) + tokenPool.add('first') + tokenPool.add('second') + + tokenPool.next().invalidate() + tokenPool.add('first') + + expect(tokenPool.next().id).to.equal('first') + expect(tokenPool.next().id).to.equal('second') + expect(tokenPool.next().id).to.equal('second') + expect(tokenPool.next().id).to.equal('first') + }) + + it('does not duplicate exhausted tokens when they are revalidated', function () { + const tokenPool = new TokenPool() + tokenPool.add('first') + tokenPool.add('second') + + const first = tokenPool.next() + first.update(0, Token.nextResetNever) + tokenPool.next() + + first.invalidate() + tokenPool.add('first') + + const { allTokenDebugInfo } = tokenPool.serializeDebugInfo({ + sanitize: false, + }) + const tokenIds = allTokenDebugInfo.map(({ id }) => id) + + expect(tokenIds).to.have.members(['first', 'second']) + expect( + allTokenDebugInfo.find(({ id }) => id === 'first').isFrozen, + ).to.equal(false) + }) + context('tokens are marked exhausted immediately', function () { it('should be exhausted', function () { ids.forEach(() => { diff --git a/migrations/1780134030603_add-token-scopes.cjs b/migrations/1780134030603_add-token-scopes.cjs new file mode 100644 index 0000000000000..9a820b2133850 --- /dev/null +++ b/migrations/1780134030603_add-token-scopes.cjs @@ -0,0 +1,11 @@ +exports.shorthands = undefined + +exports.up = pgm => { + pgm.addColumn('github_user_tokens', { + scopes: { type: 'text[]' }, + }) +} + +exports.down = pgm => { + pgm.dropColumn('github_user_tokens', 'scopes') +} diff --git a/services/github/auth/acceptor.js b/services/github/auth/acceptor.js index ac7ec88e949e7..f74adaffc549d 100644 --- a/services/github/auth/acceptor.js +++ b/services/github/auth/acceptor.js @@ -14,6 +14,9 @@ function setRoutes({ server, authHelper, onTokenAccepted }) { // it's not setting a bad example. client_id: authHelper._user, redirect_uri: `${baseUrl}/github-auth/done`, + // TODO: Request the OAuth scopes needed for scoped GitHub API features. + // Remove this TODO once this authorize URL includes a `scope` parameter. + // See https://github.com/badges/shields/issues/4169. }) ask.res.setHeader( 'Location', @@ -58,10 +61,14 @@ function setRoutes({ server, authHelper, onTokenAccepted }) { return end('The GitHub OAuth token could not be parsed.') } - const { access_token: token } = content + const { access_token: token, scope = '' } = content if (!token) { return end('The GitHub OAuth process did not return a user token.') } + const scopes = scope + .split(',') + .map(scope => scope.trim()) + .filter(Boolean) ask.res.setHeader('Content-Type', 'text/html') end( @@ -76,7 +83,7 @@ function setRoutes({ server, authHelper, onTokenAccepted }) { '

Back to the website

', ) - onTokenAccepted(token) + onTokenAccepted(token, { scopes }) }) } diff --git a/services/github/auth/acceptor.spec.js b/services/github/auth/acceptor.spec.js index c8357c93767ad..03b5c4511b3e6 100644 --- a/services/github/auth/acceptor.spec.js +++ b/services/github/auth/acceptor.spec.js @@ -4,6 +4,7 @@ import sinon from 'sinon' import portfinder from 'portfinder' import qs from 'qs' import nock from 'nock' +import '../../../core/register-chai-plugins.spec.js' import got from '../../../core/got-test-client.js' import GithubConstellation from '../github-constellation.js' import { setRoutes } from './acceptor.js' @@ -72,8 +73,10 @@ describe('Github token acceptor', function () { context('a code is provided', function () { let scope + let tokenResponse beforeEach(function () { nock.enableNetConnect(/127\.0\.0\.1/) + tokenResponse = { access_token: fakeAccessToken } scope = nock('https://github.com') .post('/login/oauth/access_token') @@ -82,7 +85,7 @@ describe('Github token acceptor', function () { expect(parsedBody.client_id).to.equal(fakeClientId) expect(parsedBody.client_secret).to.equal(fakeClientSecret) expect(parsedBody.code).to.equal(fakeCode) - return [200, qs.stringify({ access_token: fakeAccessToken })] + return [200, qs.stringify(tokenResponse)] }) }) @@ -117,6 +120,21 @@ describe('Github token acceptor', function () { ).to.be.true expect(onTokenAccepted).to.have.been.calledWith(fakeAccessToken) }) + + it('should pass token scopes from the OAuth response', async function () { + tokenResponse.scope = 'read:packages, read:user' + + const form = new FormData() + form.append('code', fakeCode) + + await got.post(`${baseUrl}/github-auth/done`, { + body: form, + }) + + expect(onTokenAccepted).to.have.been.calledWith(fakeAccessToken, { + scopes: ['read:packages', 'read:user'], + }) + }) }) }) }) diff --git a/services/github/github-api-provider.js b/services/github/github-api-provider.js index eceb743ea44bb..f44339fd098a3 100644 --- a/services/github/github-api-provider.js +++ b/services/github/github-api-provider.js @@ -65,11 +65,11 @@ class GithubApiProvider { this.restApiVersion = restApiVersion } - addToken(tokenString) { + addToken(tokenString, data) { if (this.authType === this.constructor.AUTH_TYPES.TOKEN_POOL) { - this.standardTokens.add(tokenString) - this.searchTokens.add(tokenString) - this.graphqlTokens.add(tokenString) + this.standardTokens.add(tokenString, data) + this.searchTokens.add(tokenString, data) + this.graphqlTokens.add(tokenString, data) } else { throw Error('When not using a token pool, do not provide tokens') } diff --git a/services/github/github-constellation.js b/services/github/github-constellation.js index bfc887b1f82d5..20303b1572650 100644 --- a/services/github/github-constellation.js +++ b/services/github/github-constellation.js @@ -92,27 +92,28 @@ class GithubConstellation { log.error(e) } - tokens.forEach(tokenString => { - this.apiProvider.addToken(tokenString) + tokens.forEach(({ token, scopes }) => { + this.apiProvider.addToken(token, { scopes }) }) if (this.oauthHelper.isConfigured) { setAcceptorRoutes({ server, authHelper: this.oauthHelper, - onTokenAccepted: tokenString => this.onTokenAdded(tokenString), + onTokenAccepted: (tokenString, data) => + this.onTokenAdded(tokenString, data), }) } } - onTokenAdded(tokenString) { + onTokenAdded(tokenString, data) { if (!this.persistence) { throw Error('Token persistence is not configured') } - this.apiProvider.addToken(tokenString) + this.apiProvider.addToken(tokenString, data) process.nextTick(async () => { try { - await this.persistence.noteTokenAdded(tokenString) + await this.persistence.noteTokenAdded(tokenString, data) } catch (e) { log.error(e) }