From 9007658fd007fa8a4f5bb30f93b459f2a69332ba Mon Sep 17 00:00:00 2001
From: Paul Melnikow <github@paulmelnikow.com>
Date: Sat, 11 Aug 2018 20:13:40 -0400
Subject: [PATCH] Refactor and test [github] token persistence (#1863)
Ref #1848 #1205
---
lib/github-auth.js | 74 ++++++++------------
lib/in-process-server-test-helpers.js | 4 +-
lib/service-test-runner/cli.js | 4 +-
lib/text-formatters.spec.js | 6 +-
lib/token-persistence.js | 45 ++++++++++++
lib/token-persistence.spec.js | 93 +++++++++++++++++++++++++
package-lock.json | 15 ++++
package.json | 5 +-
server.js | 8 ++-
services/github/github-constellation.js | 19 +++--
10 files changed, 213 insertions(+), 60 deletions(-)
create mode 100644 lib/token-persistence.js
create mode 100644 lib/token-persistence.spec.js
diff --git a/lib/github-auth.js b/lib/github-auth.js
index 8a6f2b55ef..2c4b456c92 100644
--- a/lib/github-auth.js
+++ b/lib/github-auth.js
@@ -5,40 +5,21 @@ const log = require('./log')
const secretIsValid = require('./sys/secret-is-valid')
const queryString = require('query-string')
const request = require('request')
-const autosave = require('json-autosave')
const serverSecrets = require('./server-secrets')
const mapKeys = require('lodash.mapkeys')
-// This is an initial value which makes the code work while the initial data
-// is loaded. In the then() callback of scheduleAutosaving(), it's reassigned
-// with a JsonSave object.
-let githubUserTokens = { data: [] }
-
-function scheduleAutosaving(githubUserTokensFile) {
- autosave(githubUserTokensFile, { data: [] })
- .then(save => {
- githubUserTokens = save
- for (let i = 0; i < githubUserTokens.data.length; i++) {
- addGithubToken(githubUserTokens.data[i])
- }
- // 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)
- }
- })
- .catch(e => {
- console.error('Could not create ' + githubUserTokensFile)
- })
-}
+const userTokenRateLimit = 12500
-function cancelAutosaving() {
- if (githubUserTokens.stop) {
- githubUserTokens.stop()
- githubUserTokens.save()
- githubUserTokens = { data: [] }
- }
+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.
+
+// 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)
}
function setRoutes(server) {
@@ -166,12 +147,6 @@ function sendTokenToAllServers(token) {
)
}
-// Track rate limit requests remaining.
-
-// Ideally, we would want priority queues here.
-const reqRemaining = new Map() // From token to requests remaining.
-const reqReset = new Map() // From token to timestamp.
-
// token: client token as a string.
// reqs: number of requests remaining.
// reset: timestamp when the number of remaining requests is reset.
@@ -189,8 +164,6 @@ function utcEpochSeconds() {
return (Date.now() / 1000) >>> 0
}
-const userTokenRateLimit = 12500
-
// Return false if the token cannot reasonably be expected to perform
// a GitHub request.
function isTokenUsable(token, now) {
@@ -206,7 +179,7 @@ function isTokenUsable(token, now) {
// with a reasonable chance that the request will succeed.
function usableTokens() {
const now = utcEpochSeconds()
- return githubUserTokens.data.filter(function(token) {
+ return githubUserTokens.filter(function(token) {
return isTokenUsable(token, now)
})
}
@@ -235,17 +208,17 @@ 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.data.indexOf(token) === -1) {
- githubUserTokens.data.push(token)
+ if (githubUserTokens.indexOf(token) === -1) {
+ githubUserTokens.push(token)
}
}
function rmGithubToken(token) {
rmReqRemaining(token)
// Remove it only if it is in there.
- const idx = githubUserTokens.data.indexOf(token)
+ const idx = githubUserTokens.indexOf(token)
if (idx >= 0) {
- githubUserTokens.data.splice(idx, 1)
+ githubUserTokens.splice(idx, 1)
}
}
@@ -266,12 +239,20 @@ function sha(str) {
.digest('hex')
}
+function getAllTokenIds() {
+ return githubUserTokens.slice()
+}
+
+function removeAllTokens() {
+ githubUserTokens = []
+}
+
function serializeDebugInfo(options) {
// Apply defaults.
const { sanitize } = Object.assign({ sanitize: true }, options)
const unsanitized = {
- tokens: githubUserTokens.data,
+ tokens: githubUserTokens,
reqRemaining: mapToObject(reqRemaining),
reqReset: mapToObject(reqReset),
utcEpochSeconds: utcEpochSeconds(),
@@ -347,9 +328,10 @@ function githubRequest(request, url, query, cb) {
}
module.exports = {
- scheduleAutosaving,
- cancelAutosaving,
request: githubRequest,
setRoutes,
serializeDebugInfo,
+ addGithubToken,
+ getAllTokenIds,
+ removeAllTokens,
}
diff --git a/lib/in-process-server-test-helpers.js b/lib/in-process-server-test-helpers.js
index 2187427362..1b2f90bd17 100644
--- a/lib/in-process-server-test-helpers.js
+++ b/lib/in-process-server-test-helpers.js
@@ -56,9 +56,9 @@ function reset(server) {
*
* @param {Object} server instance
*/
-function stop(server) {
+async function stop(server) {
if (server) {
- server.stop()
+ await server.stop()
}
}
diff --git a/lib/service-test-runner/cli.js b/lib/service-test-runner/cli.js
index 9336f1b0d9..8a219e0d9c 100644
--- a/lib/service-test-runner/cli.js
+++ b/lib/service-test-runner/cli.js
@@ -57,8 +57,8 @@ before('Start running the server', function() {
this.timeout(5000)
server = serverHelpers.start()
})
-after('Shut down the server', function() {
- serverHelpers.stop(server)
+after('Shut down the server', async function() {
+ await serverHelpers.stop(server)
})
const runner = new Runner()
diff --git a/lib/text-formatters.spec.js b/lib/text-formatters.spec.js
index a4885dd658..6ea7793efc 100644
--- a/lib/text-formatters.spec.js
+++ b/lib/text-formatters.spec.js
@@ -83,8 +83,12 @@ describe('Text formatters', function() {
})
context('in october', function() {
+ let clock
beforeEach(function() {
- sinon.useFakeTimers(new Date(2017, 9, 15).getTime())
+ clock = sinon.useFakeTimers(new Date(2017, 9, 15).getTime())
+ })
+ afterEach(function() {
+ clock.restore()
})
test(formatDate, () => {
diff --git a/lib/token-persistence.js b/lib/token-persistence.js
new file mode 100644
index 0000000000..11356a16fc
--- /dev/null
+++ b/lib/token-persistence.js
@@ -0,0 +1,45 @@
+'use strict'
+
+const autosave = require('json-autosave')
+const githubAuth = require('./github-auth')
+
+// This is currently bound to the legacy github auth code. That will be
+// replaced with a dependency-injected token provider.
+class TokenPersistence {
+ constructor({ path }) {
+ this.path = path
+ this.save = undefined
+ }
+
+ async initialize() {
+ // This code is a bit difficult to understand, in part because it's
+ // working against the interface of `json-autosave` which wants to own the
+ // data structure.
+ const save = await autosave(this.path, { data: [] })
+ this.save = save
+
+ save.data.forEach(tokenString => {
+ githubAuth.addGithubToken(tokenString)
+ })
+
+ // Override the autosave handler to refresh the token data before
+ // saving.
+ save.autosave = () => {
+ save.data = githubAuth.getAllTokenIds()
+ return save.save()
+ }
+ // Put the change in autosave handler into effect.
+ save.stop()
+ save.start()
+ }
+
+ async stop() {
+ if (this.save) {
+ this.save.stop()
+ await this.save.autosave()
+ this.save = undefined
+ }
+ }
+}
+
+module.exports = TokenPersistence
diff --git a/lib/token-persistence.spec.js b/lib/token-persistence.spec.js
new file mode 100644
index 0000000000..6f44b65566
--- /dev/null
+++ b/lib/token-persistence.spec.js
@@ -0,0 +1,93 @@
+'use strict'
+
+const fs = require('fs')
+const tmp = require('tmp')
+const readFile = require('fs-readfile-promise')
+const sinon = require('sinon')
+const { sleep } = require('wait-promise')
+const { expect } = require('chai')
+const TokenPersistence = require('./token-persistence')
+const githubAuth = require('./github-auth')
+
+describe('Token persistence', function() {
+ // Fake timers must be set up before any timers are scheduled.
+ let clock
+ beforeEach(function() {
+ clock = sinon.useFakeTimers()
+ })
+ afterEach(function() {
+ clock.restore()
+ })
+
+ beforeEach(githubAuth.removeAllTokens)
+ afterEach(githubAuth.removeAllTokens)
+
+ let path, persistence
+ beforeEach(function() {
+ path = tmp.tmpNameSync()
+ persistence = new TokenPersistence({ path })
+ })
+ afterEach(function() {
+ if (persistence) {
+ persistence.stop()
+ persistence = null
+ }
+ })
+
+ context('when the file does not exist', function() {
+ it('creates it with an empty array', async function() {
+ await persistence.initialize()
+ const json = JSON.parse(await readFile(path))
+
+ expect(json).to.deep.equal([])
+ })
+ })
+
+ context('when the file exists', function() {
+ it('loads the contents', async function() {
+ const initialTokens = ['a', 'b', 'c'].map(char => char.repeat(40))
+ fs.writeFileSync(path, JSON.stringify(initialTokens))
+
+ await persistence.initialize()
+
+ expect(githubAuth.getAllTokenIds()).to.deep.equal(initialTokens)
+ })
+ })
+
+ context('when shutting down', function() {
+ it('writes added tokens to the file', async function() {
+ const initialTokens = ['a', 'b', 'c'].map(char => char.repeat(40))
+ fs.writeFileSync(path, JSON.stringify(initialTokens))
+
+ const newToken = 'e'.repeat(40)
+ const expected = initialTokens.slice()
+ expected.push(newToken)
+
+ await persistence.initialize()
+ githubAuth.addGithubToken(newToken)
+ await persistence.stop()
+
+ const json = JSON.parse(await readFile(path))
+ expect(json).to.deep.equal(expected)
+ })
+ })
+
+ context('time has elapsed', function() {
+ it('writes added tokens to the file', async function() {
+ const addedTokens = ['d', 'e'].map(char => char.repeat(40))
+
+ await persistence.initialize()
+
+ addedTokens.forEach(githubAuth.addGithubToken)
+
+ // Fake time passing to trigger autosaving, then give the save a brief
+ // moment of real time to complete before reading.
+ clock.tick(5000)
+ clock.restore()
+ await sleep(200)
+
+ const json = JSON.parse(await readFile(path))
+ expect(json).to.deep.equal(addedTokens)
+ })
+ })
+})
diff --git a/package-lock.json b/package-lock.json
index 205fc06573..8cfefb7e1b 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -5762,6 +5762,15 @@
"integrity": "sha1-mC1ok6+RjnLQjeyehnP/K1qNat0=",
"dev": true
},
+ "fs-readfile-promise": {
+ "version": "3.0.1",
+ "resolved": "https://registry.npmjs.org/fs-readfile-promise/-/fs-readfile-promise-3.0.1.tgz",
+ "integrity": "sha512-LsSxMeaJdYH27XrW7Dmq0Gx63mioULCRel63B5VeELYLavi1wF5s0XfsIdKDFdCL9hsfQ2qBvXJszQtQJ9h17A==",
+ "dev": true,
+ "requires": {
+ "graceful-fs": "^4.1.11"
+ }
+ },
"fs-write-stream-atomic": {
"version": "1.0.10",
"resolved": "https://registry.npmjs.org/fs-write-stream-atomic/-/fs-write-stream-atomic-1.0.10.tgz",
@@ -14648,6 +14657,12 @@
"integrity": "sha512-8Xz4H3vhYRGbFupLtl6dHwMx0ojUcjt0HYkqZ9oBCfipd/5mD7Md58m2/dq7uPuZU/0T3Gb1m66KS9jn+I+14Q==",
"dev": true
},
+ "wait-promise": {
+ "version": "0.4.1",
+ "resolved": "https://registry.npmjs.org/wait-promise/-/wait-promise-0.4.1.tgz",
+ "integrity": "sha1-Hq2PgtKRolAQwLHu+NRxEyPMTas=",
+ "dev": true
+ },
"walk": {
"version": "2.3.9",
"resolved": "https://registry.npmjs.org/walk/-/walk-2.3.9.tgz",
diff --git a/package.json b/package.json
index fa1c9c91d4..b4fdd18536 100644
--- a/package.json
+++ b/package.json
@@ -133,6 +133,7 @@
"eslint-plugin-react": "^7.6.1",
"eslint-plugin-standard": "^3.0.1",
"fetch-ponyfill": "^6.0.0",
+ "fs-readfile-promise": "^3.0.1",
"icedfrisby": "2.0.0-alpha.2",
"icedfrisby-nock": "^1.0.0",
"is-png": "^1.1.0",
@@ -163,7 +164,9 @@
"sinon": "^6.0.0",
"sinon-chai": "^3.0.0",
"snap-shot-it": "^5.0.1",
- "url": "^0.11.0"
+ "tmp": "0.0.33",
+ "url": "^0.11.0",
+ "wait-promise": "^0.4.1"
},
"engines": {
"node": ">= 8.x",
diff --git a/server.js b/server.js
index 238ab78f96..918ffccfe4 100644
--- a/server.js
+++ b/server.js
@@ -132,10 +132,12 @@ function reset() {
clearRegularUpdateCache();
}
-function stop(callback) {
- githubConstellation.stop();
+async function stop() {
+ await githubConstellation.stop();
analytics.cancelAutosaving();
- camp.close(callback);
+ return new Promise(resolve => {
+ camp.close(resolve);
+ });
}
module.exports = {
diff --git a/services/github/github-constellation.js b/services/github/github-constellation.js
index 7ea11b7bbd..e5bec5f26f 100644
--- a/services/github/github-constellation.js
+++ b/services/github/github-constellation.js
@@ -4,6 +4,7 @@ const path = require('path')
const githubAuth = require('../../lib/github-auth')
const serverSecrets = require('../../lib/server-secrets')
const log = require('../../lib/log')
+const TokenPersistence = require('../../lib/token-persistence')
const GithubApiProvider = require('./github-api-provider')
const { setRoutes: setAdminRoutes } = require('./auth/admin')
@@ -13,10 +14,12 @@ class GithubConstellation {
constructor(config) {
this._debugEnabled = config.service.debug.enabled
this._debugIntervalSeconds = config.service.debug.intervalSeconds
- this._userTokensPath = path.resolve(
+
+ const userTokensPath = path.resolve(
config.persistence.dir,
'github-user-tokens.json'
)
+ this.persistence = new TokenPersistence({ path: userTokensPath })
const baseUrl = process.env.GITHUB_URL || 'https://api.github.com'
this.apiProvider = new GithubApiProvider({ baseUrl })
@@ -33,8 +36,12 @@ class GithubConstellation {
async initialize(server) {
this.scheduleDebugLogging()
- githubAuth.scheduleAutosaving(this._userTokensPath)
- // TODO Catch errors and send them to Sentry.
+ try {
+ await this.persistence.initialize()
+ } catch (e) {
+ // TODO Send to sentry.
+ console.error(e)
+ }
setAdminRoutes(server)
@@ -43,12 +50,14 @@ class GithubConstellation {
}
}
- stop() {
- githubAuth.cancelAutosaving()
+ async stop() {
if (this.debugInterval) {
clearInterval(this.debugInterval)
this.debugInterval = undefined
}
+
+ await this.persistence.stop()
+ this.persistence = undefined
}
}
--
GitLab