[sync] Force re-auth during sync setup if Oauth2 token is unavailable

[sync] Force re-auth during sync setup if Oauth2 token is unavailable

If an enterprise user disables sync via a dashboard clear, and goes on
to re-enable sync at a later time, set up can fail due to an expired or
missing Oauth2 token.

This patch updates SyncSetupHandler::OpenSyncSetup to first check if an
auth token is available for sync, and if not, force a re-auth.

R=atwilson
TBR=fgorski,rogerta
BUG=276650
TEST=Disable sync for an enterprise user, exit chrome, delete "Web
Data*" from the profile dir, restart chrome, and set up sync. Re-auth
must be forced in this case.

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

[Raghu Simha, 2013-08-20 22:15:31]

Drew, please review. Thanks.

[Andrew T Wilson (Slow), 2013-08-21 08:30:46]

https://codereview.chromium.org/23005016/diff/1/chrome/browser/ui/webui/sync_...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/23005016/diff/1/chrome/browser/ui/webui/sync_...
chrome/browser/ui/webui/sync_setup_handler.cc:860: (service &&
!service->IsOAuthRefreshTokenAvailable())) {
I think this probably isn't the right way to do this. If you are signed in but
don't have an OAuth token, the ProfileOAuth2TokenService (or some maybe some
other service) should be generating an auth error on startup (so, you'd end up
with HasMenuItem() returning true).

Maybe you can ping fgorski about the best way to add this to
ProfileOAuth2TokenService? It's just not ideal that Chrome doesn't have an oauth
token, but the only way the user would find out about this is if they happen to
try to setup sync.

[Raghu Simha, 2013-08-22 00:05:12]

+fgorski, rogerta.

Drew: Overall review again, please. Thanks!
Filip: PTAL at profile_oauth2_token_service.cc.
Roger: PTAL at one_click_signin_helper.cc.

I've updated the CL with a new approach. The latest patch contains 2 changes:

1) If OAuth2TokenService loads an empty OAuth2 refresh token during startup, it
surfaces the error by badging the menu.
2) In OneClickSigninHelper, when the user signs in in order to set up sync, in
the presence of an auth error, we neglected to show the config dialog. This CL
makes sure we now show the advanced config dialog after sign in when sync has
been disabled.

https://codereview.chromium.org/23005016/diff/1/chrome/browser/ui/webui/sync_...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/23005016/diff/1/chrome/browser/ui/webui/sync_...
chrome/browser/ui/webui/sync_setup_handler.cc:860: (service &&
!service->IsOAuthRefreshTokenAvailable())) {
On 2013/08/21 08:30:46, Andrew T Wilson wrote:
> I think this probably isn't the right way to do this. If you are signed in but
> don't have an OAuth token, the ProfileOAuth2TokenService (or some maybe some
> other service) should be generating an auth error on startup (so, you'd end up
> with HasMenuItem() returning true).
> 
> Maybe you can ping fgorski about the best way to add this to
> ProfileOAuth2TokenService? It's just not ideal that Chrome doesn't have an
oauth
> token, but the only way the user would find out about this is if they happen
to
> try to setup sync.

Agreed. I've sent out a new CL with a different approach, and added fgorski as a
reviewer.

[Andrew T Wilson (Slow), 2013-08-22 10:29:09]

LGTM with one suggestion.

https://codereview.chromium.org/23005016/diff/24001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23005016/diff/24001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.cc:159: if
(GetRefreshToken().empty()) {
I think we probably only want to do this if the user is signed in (if
GetAccountId() returns a non-empty string)?

[Raghu Simha, 2013-08-22 17:48:50]

Ping.

fgorski, rogerta: Please let me know if you have any objections to this being
landed. Thanks.

https://codereview.chromium.org/23005016/diff/24001/chrome/browser/signin/pro...
File chrome/browser/signin/profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/23005016/diff/24001/chrome/browser/signin/pro...
chrome/browser/signin/profile_oauth2_token_service.cc:159: if
(GetRefreshToken().empty()) {
On 2013/08/22 10:29:09, Andrew T Wilson wrote:
> I think we probably only want to do this if the user is signed in (if
> GetAccountId() returns a non-empty string)?

From what I've seen, we enter this observer only when the user is signed in.
Having said that, I agree that it's good to check if the user is signed in
before badging the menu. Done.

[Raghu Simha, 2013-08-22 18:10:11]

TBR Filip and Roger, since they're both on vacation this week.

[commit-bot: I haz the power, 2013-08-22 18:15:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsimha@chromium.org/23005016/29001

[commit-bot: I haz the power, 2013-08-22 23:39:46]

Change committed as 219146