Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8698001:
sync: change semantics (and name) of SigninManager::GetUsername (Closed)
|
Created:
9 years ago by tim (not reviewing) Modified:
9 years ago CC:
chromium-reviews, Raghu Simha, ncarter (slow), akalin, tim (not reviewing), Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionsync: change semantics (and name) of SigninManager::GetUsername
This is an incremental change rolled out of a larger one to remove cros_user, and make
the ProfileSyncService<Foo>Test framework easier to configure / decouple from the cros case.
In the upcoming patch I'll remove cros_user and in several cases rely on SigninManager's new GetAuthenticatedUsername.
Also makes it so we always initialize the signin manager, regardless of platform.
Also fix a case in ProfileSyncService that calls DisableForUser when a UI error seems more appropriate.
This effectively re-implements the fix for bug 103861.
BUG=88109
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113412
Patch Set 1 : first #
Total comments: 14
Patch Set 2 : remove check #
Total comments: 6
Patch Set 3 : update auth #Patch Set 4 : rebase #
Total comments: 1
Messages
Total messages: 20 (0 generated)
Ready for a peek. I may wind up waiting until I return to Seattle to actually land this series of patches since NX isn't working for me and playing with Chrome over ssh is painfully slow. Lingesh: Please review ClearInMemoryData/ClearTransientSigninData changes. Rick: Please review OAuth path in signin_manager. Roger: Mostly FYI for change in auto_login Drew: You know the drill ;) Please review it all!
http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service.cc (right): http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:1439: NotifyObservers(); I'm not entirely sure why it's now OK to NotifyObservers() here and not actually force a sign out? This seems to make sense (don't sign out the user unnecessarily), but I wonder why we were doing this before? http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... File chrome/browser/sync/signin_manager.cc (right): http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.cc:49: if (!authenticated_username_.empty() && Would the token service ever actually have a token for the user prior to Initialize() being called here? I'm curious about what this new if clause is for. http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... File chrome/browser/sync/signin_manager.h (right): http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.h:62: // If a user has previously established a username and SignOut has not been To be clear, "established a username" == "has had a successful invocation of StartSignIn OR is running CrOS which calls SetAuthenticatedUsername()"? http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.h:163: std::string authenticated_username_; My big question with this CL is: why does SigninManager track the username, given that we already take pains to ensure that authenticated_username_ always matches what's stored under kGoogleServicesUsername in prefstore? Is this due to cros, perhaps? There are just so many different ways the username is abstracted now - through SyncPrefs, through GetAuthenticatedUsername(), through setting the pref directly, and there's no clear ownership of the pref (SigninManager clears it on SignOut, but there are other places that set it). I'd like to see us either get rid of all APIs that provide an interface for getting/setting the username (in favor of having people just directly access the pref) or else have a single interface that everyone uses (and so nobody touches the pref directly) - I wonder if this CL would be a reasonable vehicle for a change like that? What's your take on the situation?
http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... File chrome/browser/sync/signin_manager.h (right): http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.h:62: // If a user has previously established a username and SignOut has not been On 2011/11/28 04:54:02, Andrew T Wilson wrote: > To be clear, "established a username" == "has had a successful invocation of > StartSignIn OR is running CrOS which calls SetAuthenticatedUsername()"? Established means validated / authenticated. It can be either of the cases you mention, though I'd like to get rid of SetAuthenticatedUsername (see TODO) and pass it on construction, if it fits the paradigm better for platforms where the valid username is known a priori. http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.h:163: std::string authenticated_username_; On 2011/11/28 04:54:02, Andrew T Wilson wrote: > My big question with this CL is: why does SigninManager track the username, > given that we already take pains to ensure that authenticated_username_ always > matches what's stored under kGoogleServicesUsername in prefstore? Is this due to > cros, perhaps? > > There are just so many different ways the username is abstracted now - through > SyncPrefs, through GetAuthenticatedUsername(), through setting the pref > directly, and there's no clear ownership of the pref (SigninManager clears it on > SignOut, but there are other places that set it). I'd like to see us either get > rid of all APIs that provide an interface for getting/setting the username (in > favor of having people just directly access the pref) or else have a single > interface that everyone uses (and so nobody touches the pref directly) - I > wonder if this CL would be a reasonable vehicle for a change like that? What's > your take on the situation? My motivation for this change (and the one before it) was to reduce the number of different ways the username is abstracted. I removed GetAuthenticatedUsername from the PSS, and am about to remove cros user, and the signin manager seems like the best central / injectable thing to manage it. If you notice my TODO in SetAuthenticatedUsername, now that it's more obvious that this username maps to the pref, we can probably remove this in the future and just use the pref. i.e. the todo is pretty much the same as your question above, I'm just trying to do it piecewise. Since the SigninManager will wind up setting the value as it orchestrates the signin process, it seems like a reasonable place to build the interface to the pref (either directly or wrapped in SyncPrefs, which can make it easier to test than building a pref-service equipped profile).
http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... File chrome/browser/sync/signin_manager.h (right): http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.h:62: // If a user has previously established a username and SignOut has not been Is there a reason why cros can't just also use the pref instead of passing in on construction? http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.h:163: std::string authenticated_username_; OK, sounds like we both agree on the long-term vision here, and I agree this is a good intermediate step.
http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service.cc (right): http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:1439: NotifyObservers(); On 2011/11/28 04:54:02, Andrew T Wilson wrote: > I'm not entirely sure why it's now OK to NotifyObservers() here and not actually > force a sign out? This seems to make sense (don't sign out the user > unnecessarily), but I wonder why we were doing this before? I'm in the same boat here, honestly. In fact I'm not sure why we need to listen to TOKEN_LOADING_FINISHED. This was added ages ago though and seems to be directly at odds with / not considered by Lingesh's recent fix (http://codereview.chromium.org/8515030). Hence why I was looking for his comment. We can leave it, but I just feel like if we don't understand it I'd rather show an error and kill sync and leave the user in the "what the, sync just disabled itself" state. http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... File chrome/browser/sync/signin_manager.cc (right): http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.cc:49: if (!authenticated_username_.empty() && On 2011/11/28 04:54:02, Andrew T Wilson wrote: > Would the token service ever actually have a token for the user prior to > Initialize() being called here? I'm curious about what this new if clause is > for. Some tests were catching this since I think they add tokens before calling setup to satisfy some AreCredentialsAvaialable checks. I'll see if that makes sense, if they should change in this CL or I should file a bug here.
http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/profile_... File chrome/browser/sync/profile_sync_service.cc (right): http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/profile_... chrome/browser/sync/profile_sync_service.cc:1439: NotifyObservers(); I assumed this meant the following: If we have an incomplete tokens cache then on restart simply disable sync. Although it is pretty unlikely we have the user name set but missing the tokens(only thing that i could think of is the tokens db has different flushing to disk behavior from the preferences db. in which case one might have made it to disk and another still in memory. but this is far fetched). Regardless of that in the unlikely event this happens we need to handle it. Thats why it looked fine for me to leave it as such. Also NotifyObservers here likely wont do any thing. Because last_auth_error would be set to none so would not display the UI. Am I missing something here? On 2011/12/05 21:24:50, timsteele wrote: > On 2011/11/28 04:54:02, Andrew T Wilson wrote: > > I'm not entirely sure why it's now OK to NotifyObservers() here and not > actually > > force a sign out? This seems to make sense (don't sign out the user > > unnecessarily), but I wonder why we were doing this before? > > I'm in the same boat here, honestly. In fact I'm not sure why we need to listen > to TOKEN_LOADING_FINISHED. This was added ages ago though and seems to be > directly at odds with / not considered by Lingesh's recent fix > (http://codereview.chromium.org/8515030). Hence why I was looking for his > comment. We can leave it, but I just feel like if we don't understand it I'd > rather show an error and kill sync and leave the user in the "what the, sync > just disabled itself" state.
Agree it's very unlikely. The case you mention Lingesh doesn't seem outside of the realm of possibilities since writing to WebDatabase and PrefService are independent. I guess what I'm most concerned about is what I mentioned in the last part of my last email - we've on occasion (very rarely) seen reports of sync mysteriously (and gracefully!) disabling itself. This is the only case I'm aware of where this could happen. So I'm in general happier knowing we don't pull the big red lever. On Mon, Dec 5, 2011 at 1:40 PM, <lipalani@chromium.org> wrote: > > http://codereview.chromium.**org/8698001/diff/2001/chrome/** > browser/sync/profile_sync_**service.cc<http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/profile_sync_service.cc> > File chrome/browser/sync/profile_**sync_service.cc (right): > > http://codereview.chromium.**org/8698001/diff/2001/chrome/** > browser/sync/profile_sync_**service.cc#newcode1439<http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/profile_sync_service.cc#newcode1439> > chrome/browser/sync/profile_**sync_service.cc:1439: NotifyObservers(); > I assumed this meant the following: > If we have an incomplete tokens cache then on restart simply disable > sync. > > Although it is pretty unlikely we have the user name set but missing the > tokens(only thing that i could think of is the tokens db has different > flushing to disk behavior from the preferences db. in which case one > might have made it to disk and another still in memory. but this is far > fetched). > > Regardless of that in the unlikely event this happens we need to handle > it. Thats why it looked fine for me to leave it as such. > > Also NotifyObservers here likely wont do any thing. Because > last_auth_error would be set to none so would not display the UI. Am I > missing something here? > > > On 2011/12/05 21:24:50, timsteele wrote: > >> On 2011/11/28 04:54:02, Andrew T Wilson wrote: >> > I'm not entirely sure why it's now OK to NotifyObservers() here and >> > not > >> actually >> > force a sign out? This seems to make sense (don't sign out the user >> > unnecessarily), but I wonder why we were doing this before? >> > > I'm in the same boat here, honestly. In fact I'm not sure why we need >> > to listen > >> to TOKEN_LOADING_FINISHED. This was added ages ago though and seems to >> > be > >> directly at odds with / not considered by Lingesh's recent fix >> (http://codereview.chromium.**org/8515030<http://codereview.chromium.org/8515030>). >> Hence why I was looking for >> > his > >> comment. We can leave it, but I just feel like if we don't understand >> > it I'd > >> rather show an error and kill sync and leave the user in the "what >> > the, sync > >> just disabled itself" state. >> > > http://codereview.chromium.**org/8698001/<http://codereview.chromium.org/8698... >
I reviewed my portion ! http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... File chrome/browser/sync/signin_manager.cc (right): http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.cc:50: !profile_->GetTokenService()->HasTokenForService( I heard other services also depend on the tokens obtained from here. So if sync is eventually disabled by the user the other tokens will not load. We probably dont want that. http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.cc:176: possibly_invalid_username_.clear(); Without clearing the AuthenticateduserName the PSS' AreCredentialsAvailable will return true even if there is an auth error. Which is probably fine but worth pointing out to make sure you have done the due diligence it is fine. Also may be we can add a comment the method AreCredentialsAvailable to say this does not mean the credentials are current.
Agree on that. However NotifyObservers is not going to make the UI show there is an auth error. We probably have to set the last_auth_error_ variable as well. On 2011/12/05 21:54:50, timsteele wrote: > Agree it's very unlikely. The case you mention Lingesh doesn't seem > outside of the realm of possibilities since writing to WebDatabase and > PrefService are independent. > > I guess what I'm most concerned about is what I mentioned in the last part > of my last email - we've on occasion (very rarely) seen reports of sync > mysteriously (and gracefully!) disabling itself. This is the only case I'm > aware of where this could happen. So I'm in general happier knowing we > don't pull the big red lever. > > On Mon, Dec 5, 2011 at 1:40 PM, <mailto:lipalani@chromium.org> wrote: > > > > > http://codereview.chromium.**org/8698001/diff/2001/chrome/** > > > browser/sync/profile_sync_**service.cc<http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/profile_sync_service.cc> > > File chrome/browser/sync/profile_**sync_service.cc (right): > > > > http://codereview.chromium.**org/8698001/diff/2001/chrome/** > > > browser/sync/profile_sync_**service.cc#newcode1439<http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/profile_sync_service.cc#newcode1439> > > chrome/browser/sync/profile_**sync_service.cc:1439: NotifyObservers(); > > I assumed this meant the following: > > If we have an incomplete tokens cache then on restart simply disable > > sync. > > > > Although it is pretty unlikely we have the user name set but missing the > > tokens(only thing that i could think of is the tokens db has different > > flushing to disk behavior from the preferences db. in which case one > > might have made it to disk and another still in memory. but this is far > > fetched). > > > > Regardless of that in the unlikely event this happens we need to handle > > it. Thats why it looked fine for me to leave it as such. > > > > Also NotifyObservers here likely wont do any thing. Because > > last_auth_error would be set to none so would not display the UI. Am I > > missing something here? > > > > > > On 2011/12/05 21:24:50, timsteele wrote: > > > >> On 2011/11/28 04:54:02, Andrew T Wilson wrote: > >> > I'm not entirely sure why it's now OK to NotifyObservers() here and > >> > > not > > > >> actually > >> > force a sign out? This seems to make sense (don't sign out the user > >> > unnecessarily), but I wonder why we were doing this before? > >> > > > > I'm in the same boat here, honestly. In fact I'm not sure why we need > >> > > to listen > > > >> to TOKEN_LOADING_FINISHED. This was added ages ago though and seems to > >> > > be > > > >> directly at odds with / not considered by Lingesh's recent fix > >> > (http://codereview.chromium.**org/8515030%3Chttp://codereview.chromium.org/851...>). > >> Hence why I was looking for > >> > > his > > > >> comment. We can leave it, but I just feel like if we don't understand > >> > > it I'd > > > >> rather show an error and kill sync and leave the user in the "what > >> > > the, sync > > > >> just disabled itself" state. > >> > > > > > http://codereview.chromium.**org/8698001/%3Chttp://codereview.chromium.org/86...> > >
http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... File chrome/browser/sync/signin_manager.cc (right): http://codereview.chromium.org/8698001/diff/2001/chrome/browser/sync/signin_m... chrome/browser/sync/signin_manager.cc:50: !profile_->GetTokenService()->HasTokenForService( On 2011/12/05 21:56:57, lipalani1 wrote: > I heard other services also depend on the tokens obtained from here. So if sync > is eventually disabled by the user the other tokens will not load. We probably > dont want that. Those services shouldn't depend on sync calling LoadTokens though, right? It should probably self-check internally, I believe what happens is some tests that use a WebDatabase (e.g. PSSAutofill) wind up seeing tasks get posted that aren't expected and hence run after the test exits, which causes a crash. Looking to see if I can avoid that another way
PTAL Removed the HasTokensForService check in SigninManager (although my last comment @ Lingesh still stands). It required some tomfoolery to make ProfileSyncServiceAutofillTest work, added a comment in that file explaining.
LGTM with a couple of questions. http://codereview.chromium.org/8698001/diff/15001/chrome/browser/sync/profile... File chrome/browser/sync/profile_sync_service.cc (right): http://codereview.chromium.org/8698001/diff/15001/chrome/browser/sync/profile... chrome/browser/sync/profile_sync_service.cc:272: credentials.email = cros_user_.empty() ? BTW, is the plan to eventually get rid of cros_user_ and instead move the username for cros into SigninManager? I ask, because as we move the signin code out of sync it would be nice for there to be one place that can authoritatively say whether there's a signed-in user or not. http://codereview.chromium.org/8698001/diff/15001/chrome/browser/sync/profile... chrome/browser/sync/profile_sync_service.cc:1447: NotifyObservers(); Agreed with Lingesh - can you explain why the UI is going to display an error in this case? Does the UI someplace check for AreCredentialsAvailable() rather than just last_auth_error? http://codereview.chromium.org/8698001/diff/15001/chrome/browser/sync/signin_... File chrome/browser/sync/signin_manager.h (right): http://codereview.chromium.org/8698001/diff/15001/chrome/browser/sync/signin_... chrome/browser/sync/signin_manager.h:136: void ClearTransientSigninData(); I'm not clear why this doesn't clear authenticated_username_? What's the use case where we want to keep it around but blow away other cached data?
http://codereview.chromium.org/8698001/diff/15001/chrome/browser/sync/profile... File chrome/browser/sync/profile_sync_service.cc (right): http://codereview.chromium.org/8698001/diff/15001/chrome/browser/sync/profile... chrome/browser/sync/profile_sync_service.cc:272: credentials.email = cros_user_.empty() ? On 2011/12/06 21:09:15, Andrew T Wilson wrote: > BTW, is the plan to eventually get rid of cros_user_ and instead move the > username for cros into SigninManager? I ask, because as we move the signin code > out of sync it would be nice for there to be one place that can authoritatively > say whether there's a signed-in user or not. As I mention in the CL description and in my first comment response (and there's a blurb in the notes from yday's meeting too), I'm working towards removing cros_user :) That (and having one single place store username info) is the sole purpose for this CL. I have a patch that I haven't sent up for review (because it's based on this and my earlier removal of GetAuthenticatedUsername from PSS so the diffs are messy) that removes cros_user, and distinguishes more explicitly between auto-starting and manual-starting the service. http://codereview.chromium.org/8698001/diff/15001/chrome/browser/sync/profile... chrome/browser/sync/profile_sync_service.cc:1447: NotifyObservers(); On 2011/12/06 21:09:15, Andrew T Wilson wrote: > Agreed with Lingesh - can you explain why the UI is going to display an error in > this case? Does the UI someplace check for AreCredentialsAvailable() rather than > just last_auth_error? Woops, I musn't have uploaded the right patchset. I addressed this, will reupload shortly. http://codereview.chromium.org/8698001/diff/15001/chrome/browser/sync/signin_... File chrome/browser/sync/signin_manager.h (right): http://codereview.chromium.org/8698001/diff/15001/chrome/browser/sync/signin_... chrome/browser/sync/signin_manager.h:136: void ClearTransientSigninData(); On 2011/12/06 21:09:15, Andrew T Wilson wrote: > I'm not clear why this doesn't clear authenticated_username_? What's the use > case where we want to keep it around but blow away other cached data? See the comment at the top of signin_manager.h for the semantics of authenticated username. The use case is any time we re-authenticate (refresh credentials, for example). Changing the username is not permitted unless you SignOut.
LGTM on my part pending fixing the last_auth_error in PSS.
Fixed auth error. Landing once try runs succeed.
http://codereview.chromium.org/8698001/diff/16009/chrome/browser/sync/profile... File chrome/browser/sync/profile_sync_service.cc (right): http://codereview.chromium.org/8698001/diff/16009/chrome/browser/sync/profile... chrome/browser/sync/profile_sync_service.cc:1497: // to trigger errors in the UI that will allow the user to re-login. Is this comment referring to NotifyObservers still accurate?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tim@chromium.org/8698001/16009
Try job failure for 8698001-16009 (retry) on win_rel for step "unit_tests". It's a second try, previously, steps "safe_browsing_tests, unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tim@chromium.org/8698001/16009
Change committed as 113412 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
