From 84fb79a907fba92f41c34db02108e530bfb889f7 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 2 Jan 2019 15:17:00 -0500 Subject: [PATCH 1/6] Sinon has a `.resolves()` helper to save some typing --- test/models/github-login-model.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/models/github-login-model.test.js b/test/models/github-login-model.test.js index 816c9ce428..bfc2240263 100644 --- a/test/models/github-login-model.test.js +++ b/test/models/github-login-model.test.js @@ -47,19 +47,19 @@ describe('GithubLoginModel', function() { }); it('returns INSUFFICIENT if scopes are present', async function() { - sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'read:org'])); + sinon.stub(loginModel, 'getScopes').resolves(['repo', 'read:org']); assert.strictEqual(await loginModel.getToken('https://api.github.com'), INSUFFICIENT); }); it('returns the token if at least the required scopes are present', async function() { - sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'read:org', 'user:email', 'extra'])); + sinon.stub(loginModel, 'getScopes').resolves(['repo', 'read:org', 'user:email', 'extra']); assert.strictEqual(await loginModel.getToken('https://api.github.com'), '1234'); }); it('caches checked tokens', async function() { - sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'read:org', 'user:email'])); + sinon.stub(loginModel, 'getScopes').resolves(['repo', 'read:org', 'user:email']); assert.strictEqual(await loginModel.getToken('https://api.github.com'), '1234'); assert.strictEqual(loginModel.getScopes.callCount, 1); From cf36d8a6c0b25cccbb6738aa642378694f7cc3f6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 2 Jan 2019 15:17:12 -0500 Subject: [PATCH 2/6] Tests for failure caching --- test/models/github-login-model.test.js | 30 ++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/models/github-login-model.test.js b/test/models/github-login-model.test.js index bfc2240263..d770ac317f 100644 --- a/test/models/github-login-model.test.js +++ b/test/models/github-login-model.test.js @@ -67,5 +67,35 @@ describe('GithubLoginModel', function() { assert.strictEqual(await loginModel.getToken('https://api.github.com'), '1234'); assert.strictEqual(loginModel.getScopes.callCount, 1); }); + + it('caches tokens that failed to authenticate correctly', async function() { + sinon.stub(loginModel, 'getScopes').resolves(GithubLoginModel.UNAUTHORIZED); + + assert.strictEqual(await loginModel.getToken('https://api.github.com'), UNAUTHENTICATED); + assert.strictEqual(loginModel.getScopes.callCount, 1); + + assert.strictEqual(await loginModel.getToken('https://api.github.com'), UNAUTHENTICATED); + assert.strictEqual(loginModel.getScopes.callCount, 1); + }); + + it('caches tokens that had insufficient scopes', async function() { + sinon.stub(loginModel, 'getScopes').resolves(['repo', 'read:org']); + + assert.strictEqual(await loginModel.getToken('https://api.github.com'), INSUFFICIENT); + assert.strictEqual(loginModel.getScopes.callCount, 1); + + assert.strictEqual(await loginModel.getToken('https://api.github.com'), INSUFFICIENT); + assert.strictEqual(loginModel.getScopes.callCount, 1); + }); + + it('does not cache network errors', async function() { + sinon.stub(loginModel, 'getScopes').rejects(new Error('You unplugged your ethernet cable')); + + assert.strictEqual(await loginModel.getToken('https://api.github.com'), UNAUTHENTICATED); + assert.strictEqual(loginModel.getScopes.callCount, 1); + + assert.strictEqual(await loginModel.getToken('https://api.github.com'), UNAUTHENTICATED); + assert.strictEqual(loginModel.getScopes.callCount, 2); + }); }); }); From 92849a97068d7c131cca7a6b5514fe70018e9e1a Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 2 Jan 2019 15:17:49 -0500 Subject: [PATCH 3/6] Cache the outcome of the scope check regardless of success or failure --- lib/models/github-login-model.js | 34 +++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/lib/models/github-login-model.js b/lib/models/github-login-model.js index 99554e64bd..04e02d3dd6 100644 --- a/lib/models/github-login-model.js +++ b/lib/models/github-login-model.js @@ -10,6 +10,9 @@ export default class GithubLoginModel { // give everyone a really frustrating experience ;-) static REQUIRED_SCOPES = ['repo', 'read:org', 'user:email'] + // Returned from getScopes if the HEAD request fails with a 401 + static UNAUTHORIZED = Symbol('unauthorized') + static get() { if (!instance) { instance = new GithubLoginModel(); @@ -21,7 +24,7 @@ export default class GithubLoginModel { this._Strategy = Strategy; this._strategy = null; this.emitter = new Emitter(); - this.checked = new Set(); + this.checked = new Map(); } async getStrategy() { @@ -53,21 +56,33 @@ export default class GithubLoginModel { hash.update(password); const fingerprint = hash.digest('base64'); - if (!this.checked.has(fingerprint)) { + const outcome = this.checked.get(fingerprint); + if (outcome === UNAUTHENTICATED || outcome === INSUFFICIENT) { + // Cached failure + return outcome; + } else if (!outcome) { + // No cached outcome. Query for scopes. try { - const scopes = new Set(await this.getScopes(account, password)); + const scopes = await this.getScopes(account, password); + if (scopes === this.constructor.UNAUTHORIZED) { + // password is incorrect + this.checked.set(fingerprint, UNAUTHENTICATED); + return UNAUTHENTICATED; + } + const scopeSet = new Set(scopes); for (const scope of this.constructor.REQUIRED_SCOPES) { - if (!scopes.has(scope)) { + if (!scopeSet.has(scope)) { // Token doesn't have enough OAuth scopes, need to reauthenticate + this.checked.set(fingerprint, INSUFFICIENT); return INSUFFICIENT; } } - // We're good - this.checked.add(fingerprint); + // Successfully authenticated and had all required scopes. + this.checked.set(fingerprint, true); } catch (e) { - // Bad credential most likely + // Most likely a network error. Do not cache the failure. // eslint-disable-next-line no-console console.error(`Unable to validate token scopes against ${account}`, e); return UNAUTHENTICATED; @@ -104,6 +119,11 @@ export default class GithubLoginModel { headers: {Authorization: `bearer ${token}`}, }); + if (response.status === 401) { + // Unauthorized + return this.constructor.UNAUTHORIZED; + } + if (response.status !== 200) { throw new Error(`Unable to check token for OAuth scopes against ${host}: ${await response.text()}`); } From ea391fb3425ef864e30999eedf6c2810d22949e7 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 2 Jan 2019 15:44:06 -0500 Subject: [PATCH 4/6] getScopes() exists to do a `fetch`, skip it for coverage --- lib/models/github-login-model.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/models/github-login-model.js b/lib/models/github-login-model.js index 04e02d3dd6..394dbfd71f 100644 --- a/lib/models/github-login-model.js +++ b/lib/models/github-login-model.js @@ -105,6 +105,7 @@ export default class GithubLoginModel { this.didUpdate(); } + /* istanbul ignore next */ async getScopes(host, token) { if (atom.inSpecMode()) { if (token === 'good-token') { From 9a81c9a1434c07a37caca80fd1884bcc66be13d7 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 3 Jan 2019 08:45:20 -0500 Subject: [PATCH 5/6] Move UNAUTHORIZED to keytar-strategy with the rest of the token states --- lib/models/github-login-model.js | 12 ++++-------- lib/shared/keytar-strategy.js | 6 ++++++ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/models/github-login-model.js b/lib/models/github-login-model.js index 394dbfd71f..ab95551915 100644 --- a/lib/models/github-login-model.js +++ b/lib/models/github-login-model.js @@ -1,7 +1,7 @@ import crypto from 'crypto'; import {Emitter} from 'event-kit'; -import {UNAUTHENTICATED, INSUFFICIENT, createStrategy} from '../shared/keytar-strategy'; +import {UNAUTHENTICATED, INSUFFICIENT, UNAUTHORIZED, createStrategy} from '../shared/keytar-strategy'; let instance = null; @@ -10,9 +10,6 @@ export default class GithubLoginModel { // give everyone a really frustrating experience ;-) static REQUIRED_SCOPES = ['repo', 'read:org', 'user:email'] - // Returned from getScopes if the HEAD request fails with a 401 - static UNAUTHORIZED = Symbol('unauthorized') - static get() { if (!instance) { instance = new GithubLoginModel(); @@ -64,8 +61,8 @@ export default class GithubLoginModel { // No cached outcome. Query for scopes. try { const scopes = await this.getScopes(account, password); - if (scopes === this.constructor.UNAUTHORIZED) { - // password is incorrect + if (scopes === UNAUTHORIZED) { + // Password is incorrect. Treat it as though you aren't authenticated at all. this.checked.set(fingerprint, UNAUTHENTICATED); return UNAUTHENTICATED; } @@ -121,8 +118,7 @@ export default class GithubLoginModel { }); if (response.status === 401) { - // Unauthorized - return this.constructor.UNAUTHORIZED; + return UNAUTHORIZED; } if (response.status !== 200) { diff --git a/lib/shared/keytar-strategy.js b/lib/shared/keytar-strategy.js index 86dcea24e2..51aa16c3d3 100644 --- a/lib/shared/keytar-strategy.js +++ b/lib/shared/keytar-strategy.js @@ -16,10 +16,15 @@ if (typeof atom === 'undefined') { }; } +// No token available in your OS keychain. const UNAUTHENTICATED = Symbol('UNAUTHENTICATED'); +// The token in your keychain isn't granted all of the required OAuth scopes. const INSUFFICIENT = Symbol('INSUFFICIENT'); +// The token in your keychain is not accepted by GitHub. +const UNAUTHORIZED = Symbol('UNAUTHORIZED'); + class KeytarStrategy { static get keytar() { return require('keytar'); @@ -248,6 +253,7 @@ async function createStrategy() { module.exports = { UNAUTHENTICATED, INSUFFICIENT, + UNAUTHORIZED, KeytarStrategy, SecurityBinaryStrategy, InMemoryStrategy, From e8f41f1c240161576cd74921f3fae339ba7954d6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 3 Jan 2019 08:55:58 -0500 Subject: [PATCH 6/6] Use UNAUTHORIZED constant in GithubLoginModel tests --- test/models/github-login-model.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/models/github-login-model.test.js b/test/models/github-login-model.test.js index d770ac317f..e4d2f754be 100644 --- a/test/models/github-login-model.test.js +++ b/test/models/github-login-model.test.js @@ -5,6 +5,7 @@ import { InMemoryStrategy, UNAUTHENTICATED, INSUFFICIENT, + UNAUTHORIZED, } from '../../lib/shared/keytar-strategy'; describe('GithubLoginModel', function() { @@ -69,7 +70,7 @@ describe('GithubLoginModel', function() { }); it('caches tokens that failed to authenticate correctly', async function() { - sinon.stub(loginModel, 'getScopes').resolves(GithubLoginModel.UNAUTHORIZED); + sinon.stub(loginModel, 'getScopes').resolves(UNAUTHORIZED); assert.strictEqual(await loginModel.getToken('https://api.github.com'), UNAUTHENTICATED); assert.strictEqual(loginModel.getScopes.callCount, 1);