Merge pull request #1871 from atom/aw/insufficient-token-loop

Cache token scope check failures

Author: smashwilson

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;
 
@@ -21,7 +21,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 +53,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 === UNAUTHORIZED) {
+            // Password is incorrect. Treat it as though you aren't authenticated at all.
+            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;
@@ -90,6 +102,7 @@ export default class GithubLoginModel {
     this.didUpdate();
   }
 
+  /* istanbul ignore next */
   async getScopes(host, token) {
     if (atom.inSpecMode()) {
       if (token === 'good-token') {
@@ -104,6 +117,10 @@ export default class GithubLoginModel {
       headers: {Authorization: `bearer ${token}`},
     });
 
+    if (response.status === 401) {
+      return UNAUTHORIZED;
+    }
+
     if (response.status !== 200) {
       throw new Error(`Unable to check token for OAuth scopes against ${host}: ${await response.text()}`);
     }

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,

test/models/github-login-model.test.js

@@ -5,6 +5,7 @@ import {
   InMemoryStrategy,
   UNAUTHENTICATED,
   INSUFFICIENT,
+  UNAUTHORIZED,
 } from '../../lib/shared/keytar-strategy';
 
 describe('GithubLoginModel', function() {
@@ -47,25 +48,55 @@ 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);
 
       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(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);
+    });
   });
 });