Refactor and test [github] token persistence (#1863)

Ref #1848 #1205

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

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

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

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

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

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

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",

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