diff --git a/.circleci/config.yml b/.circleci/config.yml index 0169195899b6d..12ef00e299fb7 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -26,6 +26,7 @@ jobs: main: docker: - image: shieldsio/shields-ci-node-8:0.0.3 + - image: redis working_directory: ~/repo steps: - checkout @@ -62,6 +63,7 @@ jobs: main@node-latest: docker: - image: shieldsio/shields-ci-node-latest:0.0.3 + - image: redis working_directory: ~/repo steps: - checkout diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 363dbbc0cca89..8c881a2b683b0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -130,6 +130,10 @@ e.g. **[Travis] Fix timeout issues** When changing other code, please add unit tests. +To run the integration tests, you must have redis installed and in your PATH. +Use `brew install redis`, `yum install redis`, etc. The test runner will +start the server automatically. + [service-tests]: https://github.com/badges/shields/blob/master/doc/service-tests.md ### Code organization diff --git a/Makefile b/Makefile index 9a9c0b938df92..7925feae240a8 100644 --- a/Makefile +++ b/Makefile @@ -69,17 +69,6 @@ deploy-heroku: git push -f origin gh-pages:gh-pages) || git checkout master git checkout master -setup: - curl http://download.redis.io/releases/redis-2.8.8.tar.gz >redis.tar.gz \ - && tar xf redis.tar.gz \ - && rm redis.tar.gz \ - && mv redis-2.8.8 redis \ - && cd redis \ - && make - -redis: - ./redis/src/redis-server - test: npm test diff --git a/doc/self-hosting.md b/doc/self-hosting.md index 9f08ee261fc94..83649d3274d3b 100644 --- a/doc/self-hosting.md +++ b/doc/self-hosting.md @@ -116,6 +116,10 @@ npm run build # Not sure why, but this needs to be run before deploying. now ``` +## Persistence + +To enable Redis-backed GitHub token persistence, point `REDIS_URL` to your +Redis installation. Server secrets -------------- diff --git a/lib/fs-token-persistence.js b/lib/fs-token-persistence.js new file mode 100644 index 0000000000000..91b20f2016ab2 --- /dev/null +++ b/lib/fs-token-persistence.js @@ -0,0 +1,45 @@ +'use strict' + +const fsos = require('fsos') +const githubAuth = require('./github-auth') +const TokenPersistence = require('./token-persistence') + +class FsTokenPersistence extends TokenPersistence { + constructor({ path }) { + super() + this.path = path + } + + async initialize() { + let contents + try { + contents = await fsos.get(this.path) + } catch (e) { + if (e.code === 'ENOENT') { + contents = '[]' + } else { + throw e + } + } + + 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)) + } + + async onTokenAdded(token) { + await this.save() + } + + async onTokenRemoved(token) { + await this.save() + } +} + +module.exports = FsTokenPersistence diff --git a/lib/token-persistence.spec.js b/lib/fs-token-persistence.spec.js similarity index 78% rename from lib/token-persistence.spec.js rename to lib/fs-token-persistence.spec.js index 6f2a3c77006b0..6c561743c63a6 100644 --- a/lib/token-persistence.spec.js +++ b/lib/fs-token-persistence.spec.js @@ -4,17 +4,17 @@ const fs = require('fs') const tmp = require('tmp') const readFile = require('fs-readfile-promise') const { expect } = require('chai') -const TokenPersistence = require('./token-persistence') +const FsTokenPersistence = require('./fs-token-persistence') const githubAuth = require('./github-auth') -describe('Token persistence', function() { +describe('File system token persistence', function() { beforeEach(githubAuth.removeAllTokens) afterEach(githubAuth.removeAllTokens) let path, persistence beforeEach(function() { path = tmp.tmpNameSync() - persistence = new TokenPersistence({ path }) + persistence = new FsTokenPersistence({ path }) }) context('when the file does not exist', function() { @@ -46,7 +46,7 @@ describe('Token persistence', function() { }) context('when tokens are added', function() { - it('writes them to the file', async function() { + it('saves the change', async function() { const newToken = 'e'.repeat(40) const expected = initialTokens.slice() expected.push(newToken) @@ -55,13 +55,13 @@ describe('Token persistence', function() { githubAuth.addGithubToken(newToken) await persistence.noteTokenAdded(newToken) - const json = JSON.parse(await readFile(path)) - expect(json).to.deep.equal(expected) + const savedTokens = JSON.parse(await readFile(path)) + expect(savedTokens).to.deep.equal(expected) }) }) context('when tokens are removed', function() { - it('writes them to the file', async function() { + it('saves the change', async function() { const expected = Array.from(initialTokens) const toRemove = expected.pop() @@ -70,8 +70,8 @@ describe('Token persistence', function() { githubAuth.rmGithubToken(toRemove) await persistence.noteTokenRemoved(toRemove) - const json = JSON.parse(await readFile(path)) - expect(json).to.deep.equal(expected) + const savedTokens = JSON.parse(await readFile(path)) + expect(savedTokens).to.deep.equal(expected) }) }) }) diff --git a/lib/redis-token-persistence.integration.js b/lib/redis-token-persistence.integration.js new file mode 100644 index 0000000000000..750c3915c0673 --- /dev/null +++ b/lib/redis-token-persistence.integration.js @@ -0,0 +1,112 @@ +'use strict' + +const { promisify } = require('util') +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) { + beforeEach(async function() { + server = new RedisServer({ config: { host: 'localhost' } }) + await server.open() + }) + } + + const key = 'tokenPersistenceIntegrationTest' + + let client + beforeEach(async function() { + client = redis.createClient() + const del = promisify(client.del).bind(client) + await del(key) + }) + afterEach(async function() { + if (client) { + const quit = promisify(client.quit).bind(client) + await quit() + client = undefined + } + }) + + if (!process.env.CI) { + afterEach(async function() { + await server.close() + server = undefined + }) + } + + let persistence + beforeEach(function() { + persistence = new RedisTokenPersistence({ key }) + }) + afterEach(async function() { + if (persistence) { + await persistence.stop() + persistence = undefined + } + }) + + context('when the key does not exist', function() { + it('does nothing', async function() { + await persistence.initialize() + expect(githubAuth.getAllTokenIds()).to.deep.equal([]) + }) + }) + + context('when the key exists', function() { + const initialTokens = ['a', 'b', 'c'].map(char => char.repeat(40)) + + beforeEach(async function() { + const rpush = promisify(client.rpush).bind(client) + await rpush(key, initialTokens) + }) + + let lrange + beforeEach(function() { + lrange = promisify(client.lrange).bind(client) + }) + + it('loads the contents', async function() { + await persistence.initialize() + expect(githubAuth.getAllTokenIds()).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() + expected.push(newToken) + + await persistence.initialize() + githubAuth.addGithubToken(newToken) + await persistence.noteTokenAdded(newToken) + + const savedTokens = await lrange(key, 0, -1) + expect(savedTokens).to.deep.equal(expected) + }) + }) + + context('when tokens are removed', function() { + it('saves the change', async function() { + const expected = Array.from(initialTokens) + const toRemove = expected.pop() + + await persistence.initialize() + + githubAuth.rmGithubToken(toRemove) + await persistence.noteTokenRemoved(toRemove) + + const savedTokens = await lrange(key, 0, -1) + expect(savedTokens).to.deep.equal(expected) + }) + }) + }) +}) diff --git a/lib/redis-token-persistence.js b/lib/redis-token-persistence.js new file mode 100644 index 0000000000000..31a96844949c3 --- /dev/null +++ b/lib/redis-token-persistence.js @@ -0,0 +1,52 @@ +'use strict' + +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 { + constructor({ url, key }) { + super() + this.url = url + this.key = key + } + + async initialize() { + this.client = redis.createClient(this.url) + this.client.on('error', e => { + log.error(e) + }) + + 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) + }) + } + + async stop() { + const quit = promisify(this.client.quit).bind(this.client) + await quit() + } + + async onTokenAdded(token) { + const rpush = promisify(this.client.rpush).bind(this.client) + await rpush(this.key, token) + } + + async onTokenRemoved(token) { + const lrem = promisify(this.client.lrem).bind(this.client) + await lrem(this.key, 0, token) + } +} + +module.exports = RedisTokenPersistence diff --git a/lib/server-config.js b/lib/server-config.js index 838b6004d0460..1a5ce8dabcc93 100644 --- a/lib/server-config.js +++ b/lib/server-config.js @@ -51,6 +51,7 @@ const config = { }, persistence: { dir: process.env.PERSISTENCE_DIR || './private', + redisUrl: process.env.REDIS_URL || process.env.REDISTOGO_URL, }, services: { github: { diff --git a/lib/token-persistence.js b/lib/token-persistence.js index 488d0b0c09440..1ad2256566a5f 100644 --- a/lib/token-persistence.js +++ b/lib/token-persistence.js @@ -1,53 +1,40 @@ 'use strict' -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 - + constructor() { this.noteTokenAdded = this.noteTokenAdded.bind(this) this.noteTokenRemoved = this.noteTokenRemoved.bind(this) } async initialize() { - let contents - try { - contents = await fsos.get(this.path) - } catch (e) { - if (e.code === 'ENOENT') { - contents = '[]' - } else { - throw e - } - } - - const tokens = JSON.parse(contents) - tokens.forEach(tokenString => { - githubAuth.addGithubToken(tokenString) - }) + throw Error('initialize() is not implemented') } - async save() { - const tokens = githubAuth.getAllTokenIds() - await fsos.set(this.path, JSON.stringify(tokens)) + async stop() {} + + async onTokenAdded(token) { + throw Error('onTokenAdded() is not implemented') } async noteTokenAdded(token) { try { - await this.save() + await this.onTokenAdded(token) } catch (e) { log.error(e) } } + async onTokenRemoved(token) { + throw Error('onTokenRemoved() is not implemented') + } + async noteTokenRemoved(token) { try { - await this.save() + await this.onTokenRemoved(token) } catch (e) { log.error(e) } diff --git a/package-lock.json b/package-lock.json index 3104d204bb0e4..185ea6a048fac 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11641,6 +11641,12 @@ "integrity": "sha1-2chtPcTcLfkBboiUbe/Wm0m0EWI=", "dev": true }, + "promise-queue": { + "version": "2.2.5", + "resolved": "https://registry.npmjs.org/promise-queue/-/promise-queue-2.2.5.tgz", + "integrity": "sha1-L29ffA9tCBCelnZZx5uIqe1ek7Q=", + "dev": true + }, "prop-types": { "version": "15.6.2", "resolved": "https://registry.npmjs.org/prop-types/-/prop-types-15.6.2.tgz", @@ -12147,6 +12153,15 @@ "resolved": "https://registry.npmjs.org/redis-parser/-/redis-parser-2.6.0.tgz", "integrity": "sha1-Uu0J2srBCPGmMcB+m2mUHnoZUEs=" }, + "redis-server": { + "version": "1.2.2", + "resolved": "https://registry.npmjs.org/redis-server/-/redis-server-1.2.2.tgz", + "integrity": "sha512-pOaSIeSMVFkEFIuaMtpQ3TOr3uI4sUmEHm4ofGks5vTPRseHUszxyIlC70IFjUR9qSeH8o/ARZEM8dqcJmgGJw==", + "dev": true, + "requires": { + "promise-queue": "^2.2.5" + } + }, "regenerate": { "version": "1.3.3", "resolved": "https://registry.npmjs.org/regenerate/-/regenerate-1.3.3.tgz", diff --git a/package.json b/package.json index 02206f8b003d7..549dd5a52b218 100644 --- a/package.json +++ b/package.json @@ -69,7 +69,7 @@ "danger": "danger", "test:js:frontend": "NODE_ENV=mocha mocha --require babel-polyfill --require babel-register \"frontend/**/*.spec.js\"", "test:js:server": "HANDLE_INTERNAL_ERRORS=false mocha \"*.spec.js\" \"lib/**/*.spec.js\" \"services/**/*.spec.js\"", - "test:integration": "mocha \"services/**/*.integration.js\"", + "test:integration": "mocha \"lib/**/*.integration.js\" \"services/**/*.integration.js\"", "test:services": "HANDLE_INTERNAL_ERRORS=false mocha --delay lib/service-test-runner/cli.js", "test:services:trace": "TRACE_SERVICES=true npm run test:services -- $*", "test:services:pr:prepare": "node lib/service-test-runner/pull-request-services-cli.js > pull-request-services.log", @@ -160,6 +160,7 @@ "react-modal": "^3.5.1", "react-router-dom": "^4.3.1", "read-all-stdin-sync": "^1.0.5", + "redis-server": "^1.2.2", "rimraf": "^2.6.2", "sazerac": "^0.4.2", "semver-regex": "^2.0.0", diff --git a/services/github/github-constellation.js b/services/github/github-constellation.js index 872fe2719746d..658975b31cd8d 100644 --- a/services/github/github-constellation.js +++ b/services/github/github-constellation.js @@ -4,7 +4,8 @@ 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 RedisTokenPersistence = require('../../lib/redis-token-persistence') +const FsTokenPersistence = require('../../lib/fs-token-persistence') const GithubApiProvider = require('./github-api-provider') const { setRoutes: setAdminRoutes } = require('./auth/admin') @@ -15,11 +16,21 @@ class GithubConstellation { this._debugEnabled = config.service.debug.enabled this._debugIntervalSeconds = config.service.debug.intervalSeconds - const userTokensPath = path.resolve( - config.persistence.dir, - 'github-user-tokens.json' - ) - this.persistence = new TokenPersistence({ path: userTokensPath }) + const { redisUrl, dir: persistenceDir } = config.persistence + if (config.persistence.redisUrl) { + log(`RedisTokenPersistence configured with ${redisUrl}`) + this.persistence = new RedisTokenPersistence({ + url: redisUrl, + key: 'githubUserTokens', + }) + } else { + const userTokensPath = path.resolve( + persistenceDir, + 'github-user-tokens.json' + ) + log(`FsTokenPersistence configured with ${userTokensPath}`) + this.persistence = new FsTokenPersistence({ path: userTokensPath }) + } const baseUrl = process.env.GITHUB_URL || 'https://api.github.com' this.apiProvider = new GithubApiProvider({ baseUrl }) @@ -66,6 +77,12 @@ class GithubConstellation { 'token-removed', this.persistence.noteTokenRemoved ) + + try { + await this.persistence.stop() + } catch (e) { + log.error(e) + } } }