[GitHub] Store accepted token scopes

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 () {

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)
     }

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']])
+  })
+})

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

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(() => {

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')
+}

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 }) {
         '<p><a href="/">Back to the website</a></p>',
     )
 
-    onTokenAccepted(token)
+    onTokenAccepted(token, { scopes })
   })
 }