Add OAuth2 authentication for some voice transcription requests.

Add OAuth2 authentication for speech recognition requests from the launcher, when audio-history is opted-into.

Audio history allows a user to see the speech recognition result at https://history.google.com/history/audio. This change will attach an auth (OAuth2) token to the recognition request, which is used to associate the request with the user. This only happens when the user opts-in to audio history, and only on speech recognition from the app launcher.

This change adds a helper class that handles retrieving and refreshing the auth token, and plumbs that token into the speech recognition request.

BUG=397019
Committed: https://crrev.com/7e93806b149d4378f5032bcd3a6c65b4e64f7774
Cr-Commit-Position: refs/heads/master@{#308270}

[Anand Mistry (off Chromium), 2014-12-08 04:32:05]

amistry@chromium.org changed reviewers:
+ gshires@chromium.org, hans@chromium.org, mgiuca@chromium.org,
rlp@chromium.org, sievers@chromium.org, thestig@chromium.org

[Anand Mistry (off Chromium), 2014-12-08 04:32:05]

mgiuca@chromium.org: Please review changes in app_list

hans@chromium.org: Please review changes in content/browser/speech

thestig@chromium.org: Please review changes in *.gypi

sievers@chromium.org: Please review changes in content/public/browser

gshires: FYI.

[Anand Mistry (off Chromium), 2014-12-08 04:32:06]

amistry@chromium.org changed reviewers:
+ gshires@chromium.org, hans@chromium.org, mgiuca@chromium.org,
rlp@chromium.org, sievers@chromium.org, thestig@chromium.org

[Anand Mistry (off Chromium), 2014-12-08 04:32:06]

mgiuca@chromium.org: Please review changes in app_list

hans@chromium.org: Please review changes in content/browser/speech

thestig@chromium.org: Please review changes in *.gypi

sievers@chromium.org: Please review changes in content/public/browser

gshires: FYI.

[Anand Mistry (off Chromium), 2014-12-08 04:32:46]

amistry@chromium.org changed reviewers:
- gshires@chromium.org

[Anand Mistry (off Chromium), 2014-12-08 04:32:46]

mgiuca@chromium.org: Please review changes in app_list

hans@chromium.org: Please review changes in content/browser/speech

thestig@chromium.org: Please review changes in *.gypi

sievers@chromium.org: Please review changes in content/public/browser

rlp@: In general.

gshires: FYI.

[Anand Mistry (off Chromium), 2014-12-08 04:48:38]

amistry@chromium.org changed reviewers:
+ calamity@chromium.org
- mgiuca@chromium.org

[Anand Mistry (off Chromium), 2014-12-08 04:48:38]

-Matt, +Chris

[hans, 2014-12-08 17:55:36]

The change description doesn't explain what's going on. I guess it has the
"what" but not the "why". And the bug is restricted.


What OAuth token is this, and why should it be sent to the server?

[calamity, 2014-12-09 02:21:17]

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_auth_helper.cc (right):

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:33:
ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
Hmm. Can we factor token_service into a member of the class so we don't have to
ask the factory every time you want to check its existence?

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:34: if (token_service &&
!token_service->RefreshTokenIsAvailable(account_id)) {
Split this into

if (token_service)
  return;

if (!token_service->Refresh...

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:37:
token_service->AddObserver(this);
Does this mean this is only added as an observer sometimes?

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:48:
token_service->RemoveObserver(this);
It doesn't seem to match up with the RemoveObserver() policy here?

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:59: base::Time now =
base::Time::Now();
It's not great to use a base::Time::Now() in code.. But you _are_ using a
base::Time::Max(). I'll allow it.

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:103: if (token_service) {
Is it possible for this to be null when called from ScheduleTokenFetch()? Should
the task be rescheduled in that case? If not, consider adding a comment in
ScheduleTokenFetch() about why we don't care about failure/failure can't happen.

[calamity, 2014-12-09 07:11:21]

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_auth_helper.cc (right):

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:59: base::Time now =
base::Time::Now();
On 2014/12/09 02:21:16, calamity wrote:
> It's not great to use a base::Time::Now() in code.. But you _are_ using a
> base::Time::Max(). I'll allow it.

Another option is to take a base::Clock() in your constructor and hold onto it
as a scoped_ptr field. You can then use DefaultClock and SimpleTestClock for
normal and test use respectively.

[Anand Mistry (off Chromium), 2014-12-10 02:13:25]

hans: I updated the description with a bit more info. As for why the bug is
restricted, I'll direct you to chat with somast@ about that.

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_auth_helper.cc (right):

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:33:
ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
On 2014/12/09 02:21:16, calamity wrote:
> Hmm. Can we factor token_service into a member of the class so we don't have
to
> ask the factory every time you want to check its existence?

Done.

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:34: if (token_service &&
!token_service->RefreshTokenIsAvailable(account_id)) {
On 2014/12/09 02:21:16, calamity wrote:
> Split this into
> 
> if (token_service)
>   return;
> 
> if (!token_service->Refresh...

Done.

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:37:
token_service->AddObserver(this);
On 2014/12/09 02:21:16, calamity wrote:
> Does this mean this is only added as an observer sometimes?

Yes. It only needs to be when the oauth2 refresh token is not yet available.

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:48:
token_service->RemoveObserver(this);
On 2014/12/09 02:21:16, calamity wrote:
> It doesn't seem to match up with the RemoveObserver() policy here?

What policy?

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:59: base::Time now =
base::Time::Now();
On 2014/12/09 07:11:20, calamity wrote:
> On 2014/12/09 02:21:16, calamity wrote:
> > It's not great to use a base::Time::Now() in code.. But you _are_ using a
> > base::Time::Max(). I'll allow it.
> 
> Another option is to take a base::Clock() in your constructor and hold onto it
> as a scoped_ptr field. You can then use DefaultClock and SimpleTestClock for
> normal and test use respectively.

The problem is that this doesn't buy any extra testability since you can't
advance time on the MessageLoop.

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:103: if (token_service) {
On 2014/12/09 02:21:16, calamity wrote:
> Is it possible for this to be null when called from ScheduleTokenFetch()?
Should
> the task be rescheduled in that case? If not, consider adding a comment in
> ScheduleTokenFetch() about why we don't care about failure/failure can't
happen.

Added comment here.

[calamity, 2014-12-10 02:26:13]

lgtm.

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_auth_helper.cc (right):

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:59: base::Time now =
base::Time::Now();
On 2014/12/10 02:13:25, Anand Mistry wrote:
> On 2014/12/09 07:11:20, calamity wrote:
> > On 2014/12/09 02:21:16, calamity wrote:
> > > It's not great to use a base::Time::Now() in code.. But you _are_ using a
> > > base::Time::Max(). I'll allow it.
> > 
> > Another option is to take a base::Clock() in your constructor and hold onto
it
> > as a scoped_ptr field. You can then use DefaultClock and SimpleTestClock for
> > normal and test use respectively.
> 
> The problem is that this doesn't buy any extra testability since you can't
> advance time on the MessageLoop.

The objective was to add more determinism since base::Time::Now() will return
different values across test runs.

https://codereview.chromium.org/778393002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_auth_helper.cc (right):

https://codereview.chromium.org/778393002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:55:
token_service_->RemoveObserver(this);
If the refresh token is available, I would expect this to DCHECK that you're
removing an observer that was never added.

[Lei Zhang, 2014-12-10 02:32:46]

chrome gypi change lgtm

[Anand Mistry (off Chromium), 2014-12-10 03:52:15]

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_auth_helper.cc (right):

https://codereview.chromium.org/778393002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:59: base::Time now =
base::Time::Now();
On 2014/12/10 02:26:13, calamity wrote:
> On 2014/12/10 02:13:25, Anand Mistry wrote:
> > On 2014/12/09 07:11:20, calamity wrote:
> > > On 2014/12/09 02:21:16, calamity wrote:
> > > > It's not great to use a base::Time::Now() in code.. But you _are_ using
a
> > > > base::Time::Max(). I'll allow it.
> > > 
> > > Another option is to take a base::Clock() in your constructor and hold
onto
> it
> > > as a scoped_ptr field. You can then use DefaultClock and SimpleTestClock
for
> > > normal and test use respectively.
> > 
> > The problem is that this doesn't buy any extra testability since you can't
> > advance time on the MessageLoop.
> 
> The objective was to add more determinism since base::Time::Now() will return
> different values across test runs.

Ok, gotcha. Done.

https://codereview.chromium.org/778393002/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_auth_helper.cc (right):

https://codereview.chromium.org/778393002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_auth_helper.cc:55:
token_service_->RemoveObserver(this);
On 2014/12/10 02:26:13, calamity wrote:
> If the refresh token is available, I would expect this to DCHECK that you're
> removing an observer that was never added.

It doesn't. RemoveObserver() gracefully handles NULL and the observer not being
in the list.

[hans, 2014-12-10 17:45:37]

> hans: I updated the description with a bit more info.

Thanks! Much appreciated.

https://codereview.chromium.org/778393002/diff/140001/content/browser/speech/...
File content/browser/speech/google_streaming_remote_engine.cc (right):

https://codereview.chromium.org/778393002/diff/140001/content/browser/speech/...
content/browser/speech/google_streaming_remote_engine.cc:7: #include <algorithm>
Is this used for anything?

https://codereview.chromium.org/778393002/diff/140001/content/browser/speech/...
content/browser/speech/google_streaming_remote_engine.cc:348:
upstream_args.push_back("authToken=" + config_.auth_token);
I assume these strings are safe to send without escaping? Is there any code that
makes sure that's the case (e.g. when creating the config), or should we maybe
escape them anyway to be sure?

[Anand Mistry (off Chromium), 2014-12-10 22:43:21]

https://codereview.chromium.org/778393002/diff/140001/content/browser/speech/...
File content/browser/speech/google_streaming_remote_engine.cc (right):

https://codereview.chromium.org/778393002/diff/140001/content/browser/speech/...
content/browser/speech/google_streaming_remote_engine.cc:7: #include <algorithm>
On 2014/12/10 17:45:37, hans wrote:
> Is this used for anything?

Yes, for std::min. I added it because the linter complained.

https://codereview.chromium.org/778393002/diff/140001/content/browser/speech/...
content/browser/speech/google_streaming_remote_engine.cc:348:
upstream_args.push_back("authToken=" + config_.auth_token);
On 2014/12/10 17:45:37, hans wrote:
> I assume these strings are safe to send without escaping? Is there any code
that
> makes sure that's the case (e.g. when creating the config), or should we maybe
> escape them anyway to be sure?

Good point. According to the OAuth2 spec, these can be any non-control ASCII
character.

[hans, 2014-12-10 22:46:16]

content/browser/speech lgtm

[Anand Mistry (off Chromium), 2014-12-12 05:25:11]

rlp, sievers: Ping! I'd like to get this in before next week's dev channel
build.

[rpetterson, 2014-12-12 05:35:23]

Didn't realize you were waiting on me. Overall LGTM.
On Dec 11, 2014 9:25 PM, <amistry@chromium.org> wrote:

> rlp, sievers: Ping! I'd like to get this in before next week's dev channel
> build.
>
> https://codereview.chromium.org/778393002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rpetterson, 2014-12-12 19:34:00]

Trying the whole LGTM thing again (since it apparently didn't work via email :P
)

[no sievers, 2014-12-12 20:39:37]

lgtm

[Anand Mistry (off Chromium), 2014-12-13 00:59:25]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-12-13 01:01:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/778393002/180001

[commit-bot: I haz the power, 2014-12-13 07:03:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-13 07:03:09]

Exceeded time limit waiting for builds to trigger.

[Anand Mistry (off Chromium), 2014-12-13 12:12:44]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-12-13 12:13:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/778393002/180001

[commit-bot: I haz the power, 2014-12-13 12:44:26]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-12-13 12:46:15]

Patchset 10 (id:??) landed as
https://crrev.com/7e93806b149d4378f5032bcd3a6c65b4e64f7774
Cr-Commit-Position: refs/heads/master@{#308270}