Authentication Manager for Google Now

chrome/browser/resources/google_now/utility.js

Index: chrome/browser/resources/google_now/utility.js
diff --git a/chrome/browser/resources/google_now/utility.js b/chrome/browser/resources/google_now/utility.js
index 309ee57e0f2daf1c5c30b3c4f013cdb291d15886..4d9019e80a91d058e48742f0f1fcdf851274f568 100644
--- a/chrome/browser/resources/google_now/utility.js
+++ b/chrome/browser/resources/google_now/utility.js
@@ -283,6 +283,8 @@ function buildTaskManager(areConflicting) {
   }
 
   instrumentApiFunction(chrome.alarms.onAlarm, 'addListener', 0);
+  instrumentApiFunction(chrome.identity, 'getAuthToken', 1);

[vadimt, 2013/07/30 23:27:23]

We instrument only functions with callbacks that are directly called by Chrome.
For these ones, the callbacks will be invoked in the context of already
instrumented Chrome callbacks.

[robliao, 2013/07/30 23:58:03]

Done.

+  instrumentApiFunction(chrome.identity, 'removeCachedAuthToken', 1);
   instrumentApiFunction(chrome.runtime.onSuspend, 'addListener', 0);
 
   chrome.runtime.onSuspend.addListener(function() {
@@ -413,3 +415,94 @@ function buildAttemptManager(
   };
 }
 
+/**
+ * Wraps chrome.identity to provide limited listening support for
+ * the sign in state by polling periodically for the auth token.
+ * @return {object} The Authentication Manager interface.

[vadimt, 2013/07/30 23:27:23]

{Object}

[robliao, 2013/07/30 23:58:03]

Done.

+ */
+function buildAuthenticationManager() {
+  var alarmName = 'sign-in-alarm';
+
+  // Holds callbacks for calls to isSignedIn.
+  // Multiple calls to isSignedIn are possible while getAuthToken is
+  // returning back to us. Queue up the callbacks so that only
+  // one call to getAuthToken is required.
+  var isSignedInRequests = [];

[vadimt, 2013/07/30 23:27:23]

Do we queue them for perf reasons? If so, no need to do this. I checked with
identity guys: this API has great performance, and no caching of grouping is
necessary.

[robliao, 2013/07/30 23:58:03]

Sounds good then.

On 2013/07/30 23:27:23, vadimt wrote:

+
+  /**
+   * Determines if the user is signed in and provides a token if signed in.
+   * @param {function(boolean, string)} onSuccess Called on completion.

[vadimt, 2013/07/30 23:27:23]

It's called even when getAuthToken fails, therefore, it cannot be called
onSuccess.

[robliao, 2013/07/30 23:58:03]

In our case, getAuthToken can fail and we treat the answer as not signed in.
This is a success in our case.
On 2013/07/30 23:27:23, vadimt wrote:

[vadimt, 2013/07/31 02:29:49]

But onSuccess implies there is onFailure. If it's alone, whoever reads this code
will be confused.
If there is one callback, and it's called in all cases, the more appropriate
name for it is neutral: 'callback'. See, for example, this:
https://developer.chrome.com/extensions/alarms.html#method-getAll

[robliao, 2013/07/31 14:46:24]

sgtm
On 2013/07/31 02:29:49, vadimt wrote:

+   *     The boolean is true if the user is signed in, false otherwise.
+   *     If the user is signed in, the string contains the token.
+   */
+  function isSignedIn(onSuccess) {
+    isSignedInRequests.push(onSuccess);
+    if (isSignedInRequests.length == 1) {
+      chrome.identity.getAuthToken({interactive: false}, function(token) {
+        var signedIn = !chrome.runtime.lastError && !!token;
+        isSignedInRequests.forEach(function(queuedCallback) {
+          queuedCallback(signedIn, token);

[vadimt, 2013/07/30 23:27:23]

It would be enough to simply pass token. If it's undefined, we are not signed
in.

[robliao, 2013/07/30 23:58:03]

This was done to remove all of the !!'s that were around for the token.
On 2013/07/30 23:27:23, vadimt wrote:

[vadimt, 2013/07/31 02:29:49]

But what you essentially do is passing a value and !!value.
It's like passing a, b and a+b.
We are ending up confusing the reader by such excessiveness. It's better to
calculate !!value every time.

[robliao, 2013/07/31 14:46:24]

Done.

+        });
+        isSignedInRequests = [];
+        checkAndNotifyListeners(signedIn);
+      });
+    }
+  }
+
+  /**
+   * Removes the specified cached token.
+   * @param {string} token Authentication Token to remove from the cache.
+   * @param {function} onSuccess Called on completion.
+   */
+  function removeToken(token, onSuccess) {
+    chrome.identity.removeCachedAuthToken({token: token}, function() {
+      // Removing the token from the cache will change the sign in state.
+      // Repoll now to check the state and notify listeners.
+      // This also lets Chrome now about a possible problem with the token.
+      isSignedIn(function() {});
+      onSuccess();
+    });
+  }
+
+  var listeners = [];

[vadimt, 2013/07/30 23:27:23]

Please add a TODO item to get rid of this and implement a chrome API instead.

[robliao, 2013/07/30 23:58:03]

Done.

+
+  /**
+   * Registers a listener that gets called back when the signed in state
+   * is found to be changed.
+   * @param {function} callback Called when the answer to isSignedIn changes.
+   */
+  function addListener(callback) {
+    listeners.push(callback);
+  }
+
+  // Tracks the last answer of isSignedIn and is intentionally initialized
+  // to undefined. checkAndNotifyListeners will not notify the listeners if
+  // this is undefined because technically, no sign in state change occurred.
+  var lastReturnedSignedInState;

[vadimt, 2013/07/30 23:27:23]

Please explicitly assign null. No reasons to use undefined.

[robliao, 2013/07/30 23:58:03]

Done.

+
+  function checkAndNotifyListeners(currentSignedInState) {
+    if ((lastReturnedSignedInState != currentSignedInState) &&
+        (lastReturnedSignedInState != undefined)) {

[vadimt, 2013/07/30 23:27:23]

Please use !== to not deal with JS strangeness.

[robliao, 2013/07/30 23:58:03]

Done.

+      for (var listenerIndex in listeners) {
+        listeners[listenerIndex]();
+      }
+    }
+    lastReturnedSignedInState = currentSignedInState;
+  }
+
+  chrome.alarms.onAlarm.addListener(function(alarm) {
+    if (alarm.name == alarmName) {
+      isSignedIn(function() {});
+    }
+  });
+
+  // Poll for the sign in state every hour.
+  // One hour is just an arbitrary amount of time chosen.
+  chrome.alarms.create(alarmName, {periodInMinutes: 60});
+
+  return {
+    addListener: addListener,
+    isSignedIn: isSignedIn,
+    removeToken: removeToken
+  };
+}