Rewrite and test Github auth logic, separating standard and search quota (#1205)

The end of an era.
This commit is contained in:
Paul Melnikow
2019-01-10 21:30:23 -05:00
committed by GitHub
parent a27bef5aa5
commit c4efdc8e66
13 changed files with 288 additions and 301 deletions

View File

@@ -1,7 +1,6 @@
'use strict'
const fsos = require('fsos')
const githubAuth = require('./github-auth')
const TokenPersistence = require('./token-persistence')
class FsTokenPersistence extends TokenPersistence {
@@ -23,21 +22,28 @@ class FsTokenPersistence extends TokenPersistence {
}
const tokens = JSON.parse(contents)
tokens.forEach(tokenString => {
githubAuth.addGithubToken(tokenString)
})
this._tokens = new Set(tokens)
return tokens
}
async save() {
const tokens = githubAuth.getAllTokenIds()
const tokens = Array.from(this._tokens)
await fsos.set(this.path, JSON.stringify(tokens))
}
async onTokenAdded(token) {
if (!this._tokens) {
throw Error('initialize() has not been called')
}
this._tokens.add(token)
await this.save()
}
async onTokenRemoved(token) {
if (!this._tokens) {
throw Error('initialize() has not been called')
}
this._tokens.delete(token)
await this.save()
}
}

View File

@@ -5,12 +5,8 @@ const tmp = require('tmp')
const readFile = require('fs-readfile-promise')
const { expect } = require('chai')
const FsTokenPersistence = require('./fs-token-persistence')
const githubAuth = require('./github-auth')
describe('File system token persistence', function() {
beforeEach(githubAuth.removeAllTokens)
afterEach(githubAuth.removeAllTokens)
let path, persistence
beforeEach(function() {
path = tmp.tmpNameSync()
@@ -19,8 +15,8 @@ describe('File system token persistence', function() {
context('when the file does not exist', function() {
it('does nothing', async function() {
await persistence.initialize()
expect(githubAuth.getAllTokenIds()).to.deep.equal([])
const tokens = await persistence.initialize()
expect(tokens).to.deep.equal([])
})
it('saving creates an empty file', async function() {
@@ -41,18 +37,17 @@ describe('File system token persistence', function() {
})
it('loads the contents', async function() {
await persistence.initialize()
expect(githubAuth.getAllTokenIds()).to.deep.equal(initialTokens)
const tokens = await persistence.initialize()
expect(tokens).to.deep.equal(initialTokens)
})
context('when tokens are added', function() {
it('saves the change', async function() {
const newToken = 'e'.repeat(40)
const expected = initialTokens.slice()
const expected = Array.from(initialTokens)
expected.push(newToken)
await persistence.initialize()
githubAuth.addGithubToken(newToken)
await persistence.noteTokenAdded(newToken)
const savedTokens = JSON.parse(await readFile(path))
@@ -67,7 +62,6 @@ describe('File system token persistence', function() {
await persistence.initialize()
githubAuth.rmGithubToken(toRemove)
await persistence.noteTokenRemoved(toRemove)
const savedTokens = JSON.parse(await readFile(path))

View File

@@ -1,207 +0,0 @@
'use strict'
const { EventEmitter } = require('events')
const queryString = require('query-string')
const mapKeys = require('lodash.mapkeys')
const serverSecrets = require('./server-secrets')
const { sanitizeToken } = require('./token-pool')
const userTokenRateLimit = 12500
let githubUserTokens = []
// Ideally, we would want priority queues here.
const reqRemaining = new Map() // From token to requests remaining.
const reqReset = new Map() // From token to timestamp.
const emitter = new EventEmitter()
// Personal tokens allow access to GitHub private repositories.
// You can manage your personal GitHub token at
// <https://github.com/settings/tokens>.
if (serverSecrets && serverSecrets.gh_token) {
addGithubToken(serverSecrets.gh_token)
}
// token: client token as a string.
// reqs: number of requests remaining.
// reset: timestamp when the number of remaining requests is reset.
function setReqRemaining(token, reqs, reset) {
reqRemaining.set(token, reqs)
reqReset.set(token, reset)
}
function rmReqRemaining(token) {
reqRemaining.delete(token)
reqReset.delete(token)
}
function utcEpochSeconds() {
return (Date.now() / 1000) >>> 0
}
// Return false if the token cannot reasonably be expected to perform
// a GitHub request.
function isTokenUsable(token, now) {
const reqs = reqRemaining.get(token)
const reset = reqReset.get(token)
// We don't want to empty more than 3/4 of a user's rate limit.
const hasRemainingReqs = reqs > userTokenRateLimit / 4
const isBeyondRateLimitReset = reset < now
return hasRemainingReqs || isBeyondRateLimitReset
}
// Return a list of tokens (as strings) which can be used for a GitHub request,
// with a reasonable chance that the request will succeed.
function usableTokens() {
const now = utcEpochSeconds()
return githubUserTokens.filter(token => isTokenUsable(token, now))
}
// Retrieve a user token if there is one for which we believe there are requests
// remaining. Return undefined if we could not find one.
function getReqRemainingToken() {
// Go through the user tokens.
// Among usable ones, use the one with the highest number of remaining
// requests.
const tokens = usableTokens()
let highestReq = -1
let highestToken
for (let i = 0; i < tokens.length; i++) {
const token = tokens[i]
const reqs = reqRemaining.get(token)
if (reqs > highestReq) {
highestReq = reqs
highestToken = token
}
}
return highestToken
}
function addGithubToken(token) {
// A reset date of 0 has to be in the past.
setReqRemaining(token, userTokenRateLimit, 0)
// Insert it only if it is not registered yet.
if (githubUserTokens.indexOf(token) === -1) {
githubUserTokens.push(token)
}
emitter.emit('token-added', token)
}
function rmGithubToken(token) {
rmReqRemaining(token)
// Remove it only if it is in there.
const idx = githubUserTokens.indexOf(token)
if (idx >= 0) {
githubUserTokens.splice(idx, 1)
}
}
// Convert an ES6 Map to an object.
function mapToObject(map) {
const result = {}
for (const [k, v] of map) {
result[k] = v
}
return result
}
function getAllTokenIds() {
return githubUserTokens.slice()
}
function removeAllTokens() {
githubUserTokens = []
}
function serializeDebugInfo(options) {
// Apply defaults.
const { sanitize } = Object.assign({ sanitize: true }, options)
const unsanitized = {
tokens: githubUserTokens,
reqRemaining: mapToObject(reqRemaining),
reqReset: mapToObject(reqReset),
utcEpochSeconds: utcEpochSeconds(),
sanitized: false,
}
if (sanitize) {
return {
tokens: unsanitized.tokens.map(k => sanitizeToken(k)),
reqRemaining: mapKeys(unsanitized.reqRemaining, (v, k) =>
sanitizeToken(k)
),
reqReset: mapKeys(unsanitized.reqReset, (v, k) => sanitizeToken(k)),
utcEpochSeconds: unsanitized.utcEpochSeconds,
sanitized: true,
}
} else {
return unsanitized
}
}
// When a global gh_token is configured, use that in place of our shields.io
// token-cycling logic. This produces more predictable behavior when a token
// is provided, and more predictable failures if that token is exhausted.
//
// You can manage your personal GitHub token at https://github.com/settings/tokens
const globalToken = (serverSecrets || {}).gh_token
// Act like request(), but tweak headers and query to avoid hitting a rate
// limit.
function githubRequest(request, url, query, cb) {
query = query || {}
// A special User-Agent is required:
// http://developer.github.com/v3/#user-agent-required
const headers = {
'User-Agent': 'Shields.io',
Accept: 'application/vnd.github.v3+json',
}
const githubToken =
globalToken === undefined ? getReqRemainingToken() : globalToken
if (githubToken != null) {
// Typically, GitHub user tokens grants us 12500 req/hour.
headers['Authorization'] = `token ${githubToken}`
} else if (serverSecrets && serverSecrets.gh_client_id) {
// Using our OAuth App secret grants us 5000 req/hour
// instead of the standard 60 req/hour.
query.client_id = serverSecrets.gh_client_id
query.client_secret = serverSecrets.gh_client_secret
}
const qs = queryString.stringify(query)
if (qs) {
url += `?${qs}`
}
request(url, { headers }, (err, res, buffer) => {
if (globalToken !== null && githubToken !== null && err === null) {
if (res.statusCode === 401) {
// Unauthorized.
rmGithubToken(githubToken)
emitter.emit('token-removed', githubToken)
} else {
const remaining = +res.headers['x-ratelimit-remaining']
// reset is in UTC epoch seconds.
const reset = +res.headers['x-ratelimit-reset']
setReqRemaining(githubToken, remaining, reset)
if (remaining === 0) {
return
} // Hope for the best in the cache.
}
}
cb(err, res, buffer)
})
}
module.exports = {
request: githubRequest,
serializeDebugInfo,
addGithubToken,
rmGithubToken,
getAllTokenIds,
removeAllTokens,
emitter,
}

View File

@@ -5,12 +5,8 @@ const RedisServer = require('redis-server')
const redis = require('redis')
const { expect } = require('chai')
const RedisTokenPersistence = require('./redis-token-persistence')
const githubAuth = require('./github-auth')
describe('Redis token persistence', function() {
beforeEach(githubAuth.removeAllTokens)
afterEach(githubAuth.removeAllTokens)
let server
// In CI, expect redis already to be running.
if (!process.env.CI) {
@@ -56,8 +52,8 @@ describe('Redis token persistence', function() {
context('when the key does not exist', function() {
it('does nothing', async function() {
await persistence.initialize()
expect(githubAuth.getAllTokenIds()).to.deep.equal([])
const tokens = await persistence.initialize()
expect(tokens).to.deep.equal([])
})
})
@@ -75,8 +71,8 @@ describe('Redis token persistence', function() {
})
it('loads the contents', async function() {
await persistence.initialize()
expect(githubAuth.getAllTokenIds()).to.deep.equal(initialTokens)
const tokens = await persistence.initialize()
expect(tokens).to.deep.equal(initialTokens)
})
context('when tokens are added', function() {
@@ -86,7 +82,6 @@ describe('Redis token persistence', function() {
expected.push(newToken)
await persistence.initialize()
githubAuth.addGithubToken(newToken)
await persistence.noteTokenAdded(newToken)
const savedTokens = await lrange(key, 0, -1)
@@ -101,7 +96,6 @@ describe('Redis token persistence', function() {
await persistence.initialize()
githubAuth.rmGithubToken(toRemove)
await persistence.noteTokenRemoved(toRemove)
const savedTokens = await lrange(key, 0, -1)

View File

@@ -3,7 +3,6 @@
const redis = require('redis')
const { promisify } = require('util')
const log = require('./log')
const githubAuth = require('./github-auth')
const TokenPersistence = require('./token-persistence')
class RedisTokenPersistence extends TokenPersistence {
@@ -21,16 +20,8 @@ class RedisTokenPersistence extends TokenPersistence {
const lrange = promisify(this.client.lrange).bind(this.client)
let tokens
try {
tokens = await lrange(this.key, 0, -1)
} catch (e) {
log.error(e)
}
tokens.forEach(tokenString => {
githubAuth.addGithubToken(tokenString)
})
const tokens = await lrange(this.key, 0, -1)
return tokens
}
async stop() {