Refactor [github] token persistence, again

lib/github-auth.js

@@ -1,5 +1,6 @@
 'use strict'
 
+const { EventEmitter } = require('events')
 const crypto = require('crypto')
 const log = require('./log')
 const secretIsValid = require('./sys/secret-is-valid')
@@ -15,6 +16,8 @@ let githubUserTokens = []
 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>.
@@ -110,6 +113,7 @@ function setRoutes(server) {
       }, 10000)
     }
     addGithubToken(data.token)
+    emitter.emit('token-added', data.token)
     end('Thanks!')
   })
 }
@@ -312,6 +316,7 @@ function githubRequest(request, url, query, cb) {
       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.
@@ -331,6 +336,8 @@ module.exports = {
   setRoutes,
   serializeDebugInfo,
   addGithubToken,
+  rmGithubToken,
   getAllTokenIds,
   removeAllTokens,
+  emitter,
 }

lib/token-persistence.js

@@ -1,43 +1,55 @@
 'use strict'
 
-const autosave = require('json-autosave')
+const fsos = require('fsos')
 const githubAuth = require('./github-auth')
+const log = require('./log')
 
 // 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
+
+    this.noteTokenAdded = this.noteTokenAdded.bind(this)
+    this.noteTokenRemoved = this.noteTokenRemoved.bind(this)
   }
 
   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
+    let contents
+    try {
+      contents = await fsos.get(this.path)
+    } catch (e) {
+      if (e.code === 'ENOENT') {
+        contents = '[]'
+      } else {
+        throw e
+      }
+    }
 
-    save.data.forEach(tokenString => {
+    const tokens = JSON.parse(contents)
+    tokens.forEach(tokenString => {
       githubAuth.addGithubToken(tokenString)
     })
+  }
+
+  async save() {
+    const tokens = githubAuth.getAllTokenIds()
+    await fsos.set(this.path, JSON.stringify(tokens))
+  }
 
-    // Override the autosave handler to refresh the token data before
-    // saving.
-    save.autosave = () => {
-      save.data = githubAuth.getAllTokenIds()
-      return save.save()
+  async noteTokenAdded(token) {
+    try {
+      await this.save()
+    } catch (e) {
+      log.error(e)
     }
-    // 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
+  async noteTokenRemoved(token) {
+    try {
+      await this.save()
+    } catch (e) {
+      log.error(e)
     }
   }
 }

lib/token-persistence.spec.js

@@ -3,22 +3,11 @@
 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)
 
@@ -27,67 +16,63 @@ describe('Token persistence', 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() {
+    it('does nothing', async function() {
       await persistence.initialize()
-      const json = JSON.parse(await readFile(path))
+      expect(githubAuth.getAllTokenIds()).to.deep.equal([])
+    })
 
-      expect(json).to.deep.equal([])
+    it('saving creates an empty file', async function() {
+      await persistence.initialize()
+
+      await persistence.save()
+
+      const json = JSON.parse(await readFile(path))
+      expect(json).to.deep.deep.equal([])
     })
   })
 
   context('when the file exists', function() {
-    it('loads the contents', async function() {
-      const initialTokens = ['a', 'b', 'c'].map(char => char.repeat(40))
+    const initialTokens = ['a', 'b', 'c'].map(char => char.repeat(40))
+
+    beforeEach(async function() {
       fs.writeFileSync(path, JSON.stringify(initialTokens))
+    })
 
+    it('loads the contents', async function() {
       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)
+    context('when tokens are added', function() {
+      it('writes them to the file', async function() {
+        const newToken = 'e'.repeat(40)
+        const expected = initialTokens.slice()
+        expected.push(newToken)
 
-      await persistence.initialize()
-      githubAuth.addGithubToken(newToken)
-      await persistence.stop()
+        await persistence.initialize()
+        githubAuth.addGithubToken(newToken)
+        await persistence.noteTokenAdded(newToken)
 
-      const json = JSON.parse(await readFile(path))
-      expect(json).to.deep.equal(expected)
+        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))
+    context('when tokens are removed', function() {
+      it('writes them to the file', async function() {
+        const expected = Array.from(initialTokens)
+        const toRemove = expected.pop()
 
-      await persistence.initialize()
-
-      addedTokens.forEach(githubAuth.addGithubToken)
+        await persistence.initialize()
 
-      // 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)
+        githubAuth.rmGithubToken(toRemove)
+        await persistence.noteTokenAdded(toRemove)
 
-      const json = JSON.parse(await readFile(path))
-      expect(json).to.deep.equal(addedTokens)
+        const json = JSON.parse(await readFile(path))
+        expect(json).to.deep.equal(expected)
+      })
     })
   })
 })

package.json

@@ -28,12 +28,12 @@
     "dot": "~1.1.2",
     "emojic": "^1.1.14",
     "escape-string-regexp": "^1.0.5",
+    "fsos": "^1.1.3",
     "glob": "^7.1.1",
     "gm": "^1.23.0",
     "is-css-color": "^1.0.0",
     "joi": "^13.3.0",
     "js-yaml": "^3.11.0",
-    "json-autosave": "~1.1.2",
     "jsonpath": "~1.0.0",
     "lodash.countby": "^4.6.0",
     "lodash.mapkeys": "^4.6.0",

services/github/github-constellation.js

@@ -39,10 +39,12 @@ class GithubConstellation {
     try {
       await this.persistence.initialize()
     } catch (e) {
-      // TODO Send to sentry.
-      console.error(e)
+      log.error(e)
     }
 
+    githubAuth.emitter.on('token-added', this.persistence.noteTokenAdded)
+    githubAuth.emitter.on('token-removed', this.persistence.noteTokenRemoved)
+
     setAdminRoutes(server)
 
     if (serverSecrets && serverSecrets.gh_client_id) {
@@ -56,8 +58,14 @@ class GithubConstellation {
       this.debugInterval = undefined
     }
 
-    await this.persistence.stop()
-    this.persistence = undefined
+    githubAuth.emitter.removeListener(
+      'token-added',
+      this.persistence.noteTokenAdded
+    )
+    githubAuth.emitter.removeListener(
+      'token-removed',
+      this.persistence.noteTokenRemoved
+    )
   }
 }