Obfuscated Gaia ID fetcher

chrome/browser/extensions/api/push_messaging/push_messaging_api.cc

Index: chrome/browser/extensions/api/push_messaging/push_messaging_api.cc
diff --git a/chrome/browser/extensions/api/push_messaging/push_messaging_api.cc b/chrome/browser/extensions/api/push_messaging/push_messaging_api.cc
index cc92f87bf20c337da9829d319bb89c3c14820560..316f49786fc3cfaa6e9201fcb98215aec3b7069a 100644
--- a/chrome/browser/extensions/api/push_messaging/push_messaging_api.cc
+++ b/chrome/browser/extensions/api/push_messaging/push_messaging_api.cc
@@ -9,9 +9,16 @@
 #include "chrome/browser/extensions/event_names.h"
 #include "chrome/browser/extensions/event_router.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"

[dcheng, 2012/08/16 19:12:43]

Remove this include since it's not used.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+#include "chrome/browser/signin/token_service.h"
+#include "chrome/browser/signin/token_service_factory.h"
 #include "chrome/common/extensions/api/experimental_push_messaging.h"
+#include "chrome/common/net/gaia/google_service_auth_error.h"

[dcheng, 2012/08/16 19:12:43]

Remove this include as it's not used.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

 #include "content/public/browser/browser_thread.h"
 #include "googleurl/src/gurl.h"
+#include "chrome/browser/extensions/api/push_messaging/obfuscated_gaia_id_fetcher.h"
+
+using content::BrowserThread;
 
 namespace extensions {
 
@@ -43,4 +50,82 @@ void PushMessagingEventRouter::OnMessage(const std::string& extension_id,
       GURL());
 }
 
+PushMessagingGetChannelIdFunction::PushMessagingGetChannelIdFunction() {}
+
+PushMessagingGetChannelIdFunction::~PushMessagingGetChannelIdFunction() {}
+
+bool PushMessagingGetChannelIdFunction::RunImpl() {
+

[Munjal (Google), 2012/08/15 23:04:19]

Nit: remove unnecessary empty lines. Here and every where.

[Pete Williamson, 2012/08/19 22:05:14]

Done, but you and I have different ideas of where they are unnecessary.  Please
let me know if I missed any next time around, since I am still internalizing the
coding standards.

+  // Start the async fetch of the GAIA ID.
+  StartGaiaIdFetch(profile());
+
+  // Will finish asynchronously.
+  return true;
+}
+
+void PushMessagingGetChannelIdFunction::RespondOnUIThread(

[Munjal (Google), 2012/08/15 23:04:19]

Isn't the ObfuscatedGaiaIdFetcher already running on UI thread (and it takes
care of doing the fetch on IO thread)?

In fact, IIRC, all fetcher / flow classes call back the delegate on the original
thread the fetchers were constructed. So I don't think we need to post task on
UI thread to report a result. We should already be caleld back on the UI thread.

[Pete Williamson, 2012/08/20 18:38:42]

Done.

+    const std::string& gaia_id, const long status) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // Unpack the status and GaiaId parameters, and use it to build the
+  // channel ID here.
+  std::string channel_id = gaia_id + "." + extension_id();

[Munjal (Google), 2012/08/15 23:04:19]

NIt: Use StringPrintf?

[dcheng, 2012/08/16 19:12:43]

Shouldn't channel ID be empty if gaia_id is empty?

[Pete Williamson, 2012/08/19 22:05:14]

Good catch!  Added check for empty

[Pete Williamson, 2012/08/19 22:05:14]

I apologize for the pushback, but why?  (While I'm definitely willing to do it,
I want to understand the reason first.)  The code as is is clear and simple, and
it seems that StringPrintf is in a separate library, which complicates code
construction.

[dcheng, 2012/08/19 22:38:35]

Chaining + results in multiple temporaries being created. This results in the
contents of strings being copied multiple times between temporaries, and a
simple chain of concatenations can be surprisingly expensive.

It probably doesn't matter here, but because of the behavior of +, it's
generally avoided in favor of alternatives for consistency. If you don't want to
use StringPrintf, then use +=.

[Pete Williamson, 2012/08/20 18:38:42]

Done.

+
+  // TODO: (after initial checkin) It may be a good idea to further
+  // obfuscate the channel ID to prevent the user's obfuscated GAIA ID
+  // from being readily obtained.  Security review will tell us if we need to.
+
+  // Create a status object and set the fields.
+  DictionaryValue* result = new DictionaryValue();

[dcheng, 2012/08/16 19:12:43]

scoped_ptr

[Pete Williamson, 2012/08/19 22:05:14]

I tried originally, and that failed to properly transfer ownership.  Reading up
on scoped pointers, they are supposed to be used only if the lifetime of the
object matches the lifetime of the function - in this case, the result must
outlast the lifetime of the function.  Perhaps I could use some unique_ptr and a
 swap idiom, but the ExtensionFunction is not using that so I'd have to change
the contract of ExtensionFunction::SetResult everywhere to use it..  I could use
auto_ptr with release instead, but auto_ptr seems to be a no-no in our code
base, and not a recommended practice any more.  I'm definitely open to more
suggestions...

[dcheng, 2012/08/19 22:38:35]

Use scoped_ptr<T>::release() to transfer ownership to SetResult(). It makes the
code self-documenting.

In this instance, however, we should be using the glue provided by the IDL
generator instead of rolling our own serialization (for example, how OnMessage
uses glue::Message instead of using DictionaryValue* directly).

[dcheng, 2012/08/19 22:45:15]

Some examples of using the IDL glue:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

Search for "Results"

[Pete Williamson, 2012/08/20 18:38:42]

Done.  Ah, I see that we are using chrome's scoped_ptr, not std::scoped_ptr,
which has no release method, which is why I thought it wouldn't work.
In any case, this has now been replaced with the IDL glue method.

+  result->SetString("channelId", channel_id);
+  result->SetDouble("status", status);
+
+  // SetResult() takes ownership of result.

[dcheng, 2012/08/16 19:12:43]

If you use scoped_ptr, this comment won't be needed.

[Pete Williamson, 2012/08/19 22:05:14]

scoped_ptr doesn't work here because the object needs to outlast the lifetime of
this function.  I could use uniqe_ptr instead if ExtensionFunction::SetResult
used it, but they don't.  I could use auto_ptr and do a release, but I've heard
that auto_ptr is a no-no in our code base.  I clarified the comment instead of
changing the code.

+  SetResult(result);
+  SendResponse(true);
+}
+
+// Member function to start the fetch operation.
+// This must be called on the UI thread.
+void PushMessagingGetChannelIdFunction::StartGaiaIdFetch(
+    Profile* profile) {

[Munjal (Google), 2012/08/15 23:04:19]

You don't need to pass the profile here since it is an instance method of an
extension function. IT can get the profile itself.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+
+  TokenService* token_service = TokenServiceFactory::GetForProfile(profile);
+  net::URLRequestContextGetter* context = profile->GetRequestContext();
+  const std::string  refresh_token =

[Munjal (Google), 2012/08/15 23:04:19]

const std::string& refresh_token

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+      token_service->GetOAuth2LoginRefreshToken();
+
+  // The fetcher deletes itself when the operation completes.
+  // It holds an addref to this class, which it releases when it deletes itself.
+  fetcher_ = new  ObfuscatedGaiaIdFetcher(context, this, refresh_token);
+  fetcher_->Start();
+}
+
+void PushMessagingGetChannelIdFunction::OnObfuscatedGaiaIdFetchSuccess(
+      const std::string& gaia_id) {

[Munjal (Google), 2012/08/15 23:04:19]

Indentation: 4 spaces

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+

[dcheng, 2012/08/15 22:20:45]

Extra new line. With 80 chars, vertical space is a premium. =)

[Pete Williamson, 2012/08/19 22:05:14]

Done, but I'm not sure that I agree in principle.  Sure, it is nice to see more
code vertically, but if extra whitespace makes the code more readable, I see it
as an overall maintanability win, even if you can fit fewer lines onscreen.

[dcheng, 2012/08/19 22:38:35]

It's OK to have newlines to break up logical blocks of code, but it's
unnecessary at the beginning/end of a function.

+  // Post the task over to the UI thread to return to JS.
+  // This also holds a reference on this class to keep it alive for a callback.

[dcheng, 2012/08/15 22:20:45]

These two lines (and several other places) just describe what the code does.
Instead of describing what code does, it is preferable to describe why (if
needed).

[Pete Williamson, 2012/08/19 22:05:14]

Comment changed, see if the new one is better.

[dcheng, 2012/08/19 22:38:35]

You can remove the comment on line 104 entirely. base::Bind() is designed to be
safe in the way it handles "this" pointers. If you don't want base::Bind() to
take a reference, you must use a WeakPtr<T> or base::Unretained() or
base::Owned()). That's just the way it works, and it's not typically commented
upon.

[Pete Williamson, 2012/08/20 18:38:42]

Done.

+  bool rv = BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(
+        &PushMessagingGetChannelIdFunction::RespondOnUIThread,
+        this, gaia_id, 0));
+  DCHECK(rv);
+

[Munjal (Google), 2012/08/15 23:04:19]

nit: remove empty line. Same below.

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+}
+
+void PushMessagingGetChannelIdFunction::OnObfuscatedGaiaIdFetchFailure(
+      const GoogleServiceAuthError& error) {
+  // Post the task over to the UI thread to return to JS.
+  // This also holds a reference on this class to keep it alive for a callback.
+  bool rv = BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+        &PushMessagingGetChannelIdFunction::RespondOnUIThread,
+        this, "", error.state()));

[dcheng, 2012/08/16 19:12:43]

std::string() instead of ""

[Pete Williamson, 2012/08/19 22:05:14]

Done.

+  DCHECK(rv);
+
+}
+
 }  // namespace extensions