Convert SigninTracker to use OAuth2TokenService notifications

Convert SigninTracker to use OAuth2TokenService notifications instead of TokenService notifications. 

By removing support for the kSyncService token, this breaks ClientLogin sign ins, but those are now deprecated on all platforms.

BUG=258107
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222709

[Roger Tawa OOO till Jul 10th, 2013-08-12 20:01:11]

Hi Drew,

Please take a look.  Thanks.

[Andrew T Wilson (Slow), 2013-08-19 15:23:06]

Mostly LG, but I had a few comments (and I think my new
FakeProfileOAuth2TokenService will make this code a bit simpler).

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (left):

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:54: HandleServiceStateChange();
I guess we no longer support initializing in an intermediate state and this was
obsolete?

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:23: Initialize();
I'm guessing it's an error to call this if the OAuth2TokenServiceFactory already
has a refresh token (since OnRefreshTokenAvailable() would never be fired) -
should we DCHECK() this?

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker_unittest.cc (right):

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker_unittest.cc:41:
MOCK_METHOD0(GetRefreshToken, std::string());
BTW, this is a misuse of GMock (using a mock instead of a fake here). By the
time you land this I should already have landed FakeProfileOAuth2TokenService -
any chance you could change this over to use that class instead?

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker_unittest.cc:68:
ProfileOAuth2TokenServiceFactory::GetInstance()->
After I land my CL, this is going to crash - you'll need to use
TestingProfile::Builder::SetTestingFactory() and create your TestingProfile at
build time.

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker_unittest.cc:141:
tracker_->OnRefreshTokenAvailable("username@gmail.com");
If you change to use FakeProfileOAuth2TokenService() you won't need to
explicitly make this OnRefreshTokenAvailable() call - the fake will do it for
you.

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker_unittest.cc:157:
tracker_->OnRefreshTokenRevoked("username@gmail.com", error);
Ditto for previous comment - you shouldn't have to call this directly -
FakeProfileOAuth2TokenService::IssueToken() will do it for you.

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:75: class
OAuth2TokenServiceMock : public ProfileOAuth2TokenService {
Technically this isn't a Mock, it's a Fake. Again, see if you can use
FakeProfileOAuth2TokenService instead of this.

[Roger Tawa OOO till Jul 10th, 2013-08-29 19:28:41]

Thanks Drew.  Addressed all your comments but the DCHECK, sent you an email
about it.  I'm also still working to fix the android compile error, but you can
take a look at the other changes.

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (left):

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:54: HandleServiceStateChange();
On 2013/08/19 15:23:06, Andrew T Wilson wrote:
> I guess we no longer support initializing in an intermediate state and this
was
> obsolete?

I noticed this was true while making this change.  Initialize() is called only
from ctor, and ctor always sets state_ to (WAITING_FOR_GAIA_VALIDATION.  So this
was dead code even before my CL.

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker_unittest.cc (right):

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker_unittest.cc:41:
MOCK_METHOD0(GetRefreshToken, std::string());
On 2013/08/19 15:23:06, Andrew T Wilson wrote:
> BTW, this is a misuse of GMock (using a mock instead of a fake here). By the
> time you land this I should already have landed FakeProfileOAuth2TokenService
-
> any chance you could change this over to use that class instead?

Changed to use new fake o2ts.

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker_unittest.cc:68:
ProfileOAuth2TokenServiceFactory::GetInstance()->
On 2013/08/19 15:23:06, Andrew T Wilson wrote:
> After I land my CL, this is going to crash - you'll need to use
> TestingProfile::Builder::SetTestingFactory() and create your TestingProfile at
> build time.

Done.

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker_unittest.cc:141:
tracker_->OnRefreshTokenAvailable("username@gmail.com");
On 2013/08/19 15:23:06, Andrew T Wilson wrote:
> If you change to use FakeProfileOAuth2TokenService() you won't need to
> explicitly make this OnRefreshTokenAvailable() call - the fake will do it for
> you.

Done.

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker_unittest.cc:157:
tracker_->OnRefreshTokenRevoked("username@gmail.com", error);
On 2013/08/19 15:23:06, Andrew T Wilson wrote:
> Ditto for previous comment - you shouldn't have to call this directly -
> FakeProfileOAuth2TokenService::IssueToken() will do it for you.

Done.

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/19567004/diff/123001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:75: class
OAuth2TokenServiceMock : public ProfileOAuth2TokenService {
On 2013/08/19 15:23:06, Andrew T Wilson wrote:
> Technically this isn't a Mock, it's a Fake. Again, see if you can use
> FakeProfileOAuth2TokenService instead of this.

Done.

[fgorski, 2013-08-29 22:31:12]

Looks good, with a couple of comments.

https://codereview.chromium.org/19567004/diff/173001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/19567004/diff/173001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.cc:132: }
You should call RevokeCredentials for the respective account_id on the PO2TS
(for now it can be GetAccountId(profile_)), as the TokenService related code
will be removed, and you still want to have the clean up performed.

https://codereview.chromium.org/19567004/diff/173001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/19567004/diff/173001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:108: RefreshTokenIsAvailable();
Add a comment or TODO that this is only tracking the primary account. I'll make
an update here as well.

[Roger Tawa OOO till Jul 10th, 2013-08-30 02:25:14]

Thanks Filip.  Comments addressed, changes uploaded.

https://codereview.chromium.org/19567004/diff/173001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/19567004/diff/173001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.cc:132: }
On 2013/08/29 22:31:12, Filip Gorski wrote:
> You should call RevokeCredentials for the respective account_id on the PO2TS
> (for now it can be GetAccountId(profile_)), as the TokenService related code
> will be removed, and you still want to have the clean up performed.

Yeah totally makes sense.  Done.

https://codereview.chromium.org/19567004/diff/173001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/19567004/diff/173001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:108: RefreshTokenIsAvailable();
On 2013/08/29 22:31:12, Filip Gorski wrote:
> Add a comment or TODO that this is only tracking the primary account. I'll
make
> an update here as well.

Done.

[Andrew T Wilson (Slow), 2013-08-30 09:03:33]

So, still really frightened about blowing away refresh tokens.

Also, I think SigninTracker needs to be made simpler - we don't need a state
machine, but instead just something that listens for SIGNIN_FAILED or
TokenAvailable notifications and maps then to SigninFailed()/SigninSuccessful()
callbacks on the listener. I don't think that class should listen to
GOOGLE_SIGNIN_SUCCESSFUL anymore. WDYT?

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.cc:128:
RevokeCredentials(GetAccountId(profile_));
Still extremely nervous about this. Are you sure we want to do this? I'm
officially not participating in any code reds from 100% of our users getting
their oauth tokens revoked due to some buggy server push :-P

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:51: case
chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL:
I wonder if we should not listen for this, but instead just listen for
OnRefreshTokenAvailable() (transition to SERVICES_INITIALIZING at that point).

In fact, I'm not even certain we need multiple states any more - seems like this
class basically should just sit around waiting for either
OnRefreshTokenAvailable() or GOOGLE_SIGNIN_FAILED. We don't need to track state
or anything - just listen to those two notifications and translate them into
SigninSuccess/SigninFailed?

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:710:
fake_oauth2_token_service_->IssueRefreshToken("refresh_token");
nit: is this the right ordering (i.e. should we fire OnRefreshTokenAvailable
*before* GOOGLE_SIGNIN_SUCCESSFUL, or should we move this after
GOOGLE_SIGNIN_SUCCESSFUL is fired below)?

[Roger Tawa OOO till Jul 10th, 2013-08-30 15:55:40]

Thanks Drew.  Please take another look.  I have changed the code to not revoke
the refresh token on error.  Instead, the check to see if the token is valid
takes into account the status of the last error.  This works well with all
normal sign ins as well as re-auth.

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.cc:128:
RevokeCredentials(GetAccountId(profile_));
On 2013/08/30 09:03:33, Andrew T Wilson wrote:
> Still extremely nervous about this. Are you sure we want to do this? I'm
> officially not participating in any code reds from 100% of our users getting
> their oauth tokens revoked due to some buggy server push :-P

Yeah, me too.  Changed to no longe revoke and check last error in
RefreshTokenIsAvailable().

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:51: case
chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL:
On 2013/08/30 09:03:33, Andrew T Wilson wrote:
> I wonder if we should not listen for this, but instead just listen for
> OnRefreshTokenAvailable() (transition to SERVICES_INITIALIZING at that point).
> 
> In fact, I'm not even certain we need multiple states any more - seems like
this
> class basically should just sit around waiting for either
> OnRefreshTokenAvailable() or GOOGLE_SIGNIN_FAILED. We don't need to track
state
> or anything - just listen to those two notifications and translate them into
> SigninSuccess/SigninFailed?

I think it makes sense to have the multiple states, as discussed off thread.

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/19567004/diff/154001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:710:
fake_oauth2_token_service_->IssueRefreshToken("refresh_token");
On 2013/08/30 09:03:33, Andrew T Wilson wrote:
> nit: is this the right ordering (i.e. should we fire OnRefreshTokenAvailable
> *before* GOOGLE_SIGNIN_SUCCESSFUL, or should we move this after
> GOOGLE_SIGNIN_SUCCESSFUL is fired below)?

It should not matter, SigninTracker does not depend on the order or notifs.

[Andrew T Wilson (Slow), 2013-08-31 14:04:10]

So, I think this would probably work, but I think we're missing out on an
opportunity to greatly simplify SigninTracker. We don't have a bunch of
states/tokens/services that we care about anymore - really all we care about is
waiting around until either we have an oauth token (TokenAvailable()), or we're
told that signin has failed (via a SIGNIN_FAILED/TokenRevoked() call?)

I just hate to see us do things like track state (SERVICES_INITIALIZING) that we
don't care about any more, and look at things (like auth errors) that probably
shouldn't matter.

https://codereview.chromium.org/19567004/diff/209001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/19567004/diff/209001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:75: if (state_ == SERVICES_INITIALIZING)
{
So, why do we only do this in SERVICES_INITIALIZING state - why would we want to
ignore some invocations of OnRefreshTokenRevoked()?

Also, why do we even care about OnRefreshTokenRevoked() - why isn't listening to
SIGNIN_FAILED sufficient?

https://codereview.chromium.org/19567004/diff/209001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.h (right):

https://codereview.chromium.org/19567004/diff/209001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.h:87: static bool
AreServiceTokensLoaded(Profile* profile);
Nobody outside SigninTracker calls this any more - can we make it just a utility
function inside the .cc file instead?

https://codereview.chromium.org/19567004/diff/209001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.h:93: static LoginState
GetSigninState(Profile* profile,
I think this is pretty much obsolete as public API too - it's used by
first_run.cc, but I actually think first_run.cc is using it incorrectly anyway
(it really only cares about whether GetAuthenticatedUsername() is empty - it
shouldn't make promo decisions based on whether we have tokens loaded/auth
errors.

[Roger Tawa OOO till Jul 10th, 2013-09-07 14:03:37]

Hi Drew, please take a look. Thanks.

https://codereview.chromium.org/19567004/diff/209001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/19567004/diff/209001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:75: if (state_ == SERVICES_INITIALIZING)
{
On 2013/08/31 14:04:10, Andrew T Wilson wrote:
> So, why do we only do this in SERVICES_INITIALIZING state - why would we want
to
> ignore some invocations of OnRefreshTokenRevoked()?
> 
> Also, why do we even care about OnRefreshTokenRevoked() - why isn't listening
to
> SIGNIN_FAILED sufficient?

Done.

https://codereview.chromium.org/19567004/diff/209001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.h (right):

https://codereview.chromium.org/19567004/diff/209001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.h:87: static bool
AreServiceTokensLoaded(Profile* profile);
On 2013/08/31 14:04:10, Andrew T Wilson wrote:
> Nobody outside SigninTracker calls this any more - can we make it just a
utility
> function inside the .cc file instead?

Done.

https://codereview.chromium.org/19567004/diff/209001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.h:93: static LoginState
GetSigninState(Profile* profile,
On 2013/08/31 14:04:10, Andrew T Wilson wrote:
> I think this is pretty much obsolete as public API too - it's used by
> first_run.cc, but I actually think first_run.cc is using it incorrectly anyway
> (it really only cares about whether GetAuthenticatedUsername() is empty - it
> shouldn't make promo decisions based on whether we have tokens loaded/auth
> errors.

Done.

[Andrew T Wilson (Slow), 2013-09-09 12:08:27]

lgtm with a couple of questions/nits.

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:74:
GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
Can we document under what circumstance OnRefreshTokenRevoked() is invoked
during a signin?

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker_unittest.cc (right):

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker_unittest.cc:87: EXPECT_CALL(observer_,
SigninSuccess()).Times(0);
Heh, good addition - we assumed that you wouldn't get a SigninSuccess() *and* a
SigninFailed() call here, but never validated it.

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:715:
chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL,
Why do we send out SIGNIN_SUCCESSFUL - does anyone listen to this in this test?

[Roger Tawa OOO till Jul 10th, 2013-09-10 14:29:23]

Thanks Drew.

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker.cc (right):

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker.cc:74:
GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED));
On 2013/09/09 12:08:27, Andrew T Wilson wrote:
> Can we document under what circumstance OnRefreshTokenRevoked() is invoked
> during a signin?

Good catch.  While writing the unit tests I realized this would not be called,
but forgot to change it.  Replaced with NOTREACHED.

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/signin/si...
File chrome/browser/signin/signin_tracker_unittest.cc (right):

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/signin/si...
chrome/browser/signin/signin_tracker_unittest.cc:87: EXPECT_CALL(observer_,
SigninSuccess()).Times(0);
On 2013/09/09 12:08:27, Andrew T Wilson wrote:
> Heh, good addition - we assumed that you wouldn't get a SigninSuccess() *and*
a
> SigninFailed() call here, but never validated it.

Done.

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/ui/sync/o...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/19567004/diff/150001/chrome/browser/ui/sync/o...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:715:
chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL,
On 2013/09/09 12:08:27, Andrew T Wilson wrote:
> Why do we send out SIGNIN_SUCCESSFUL - does anyone listen to this in this
test?

Done.  Turns out none of this is really needed.  I had written this code to make
sure that OneClickSigninSyncStarter would be deleted.  I removed it all, but
this required a bit of an hack with |do_not_start_sync_for_testing_|.  I did it
like this because otherwise I'd need to move |struct StartSyncArgs| and the
StartSync() declaration to the public .h file, which seems like an uglier hack. 
If you have a better suggestion though, let me know.

[commit-bot: I haz the power, 2013-09-11 14:54:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/19567004/280001

[commit-bot: I haz the power, 2013-09-11 15:08:36]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Roger Tawa OOO till Jul 10th, 2013-09-11 15:32:24]

Hi Gab,

Can you do an owner review for first_run.cc?  Thanks.

[gab, 2013-09-11 17:11:13]

lgtm stamp for first_run.cc (trusting you and other reviewers that
manager->AuthInProgress() results in the same behavior as what was there
before?)

[Roger Tawa OOO till Jul 10th, 2013-09-11 19:36:16]

Hi Gab,

The behaviour is the same.  Sign in used to happen in two phases: first the user
signs in to gaia, then a bunch of tokens are created.  Auth should be "in
progress" for both stages.

Only the first stage is now required, and this CL makes SigninTracker follow
this new state of the world.  The old code in first_run.cc was checking for the
second stage (not sure why it was not also checking for the first stage).  The
new code in first_run.cc now checks for the first stage, which is all that
really matters now.

[commit-bot: I haz the power, 2013-09-11 19:37:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/19567004/280001

[commit-bot: I haz the power, 2013-09-12 05:26:50]

Change committed as 222709