Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1097663003:
Fetch OAuth2 tokens prior to profile creation. (Closed)
|
Created:
5 years, 8 months ago by achuithb Modified:
5 years, 7 months ago CC:
chromium-reviews, dzhioev+watch_chromium.org, stevenjb+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFetch OAuth2 tokens prior to profile creation.
BUG=470984
TEST=
Committed: https://crrev.com/163a4a63af5e56801395eed067a69e501e7b51a5
Cr-Commit-Position: refs/heads/master@{#326691}
Committed: https://crrev.com/09f1fa1af73e355514a2eb1594c4ba1f2e76acbd
Cr-Commit-Position: refs/heads/master@{#326967}
Patch Set 1 #Patch Set 2 : remove logging #
Total comments: 18
Patch Set 3 : Xiyuan feedback #Patch Set 4 : RESTORE_FROM_PASSED_OAUTH2_ACCESS_TOKEN removed #Patch Set 5 : rebase #Patch Set 6 : set up default networks #
Total comments: 17
Patch Set 7 : Xiyuan/Nikita feedback #
Total comments: 10
Patch Set 8 : rebase #Patch Set 9 : OAuth2TokenInitializer for non-SAML only #Messages
Total messages: 69 (21 generated)
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1097663003/1
achuith@chromium.org changed reviewers: + xiyuan@chromium.org
Xiyuan, I'm fetching the tokens now before profile creation - let me know your thoughts?
Thank you for taking care of the problem. I think it is the right thing to do. :) https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.cc:1127: UserSessionManager::GetInstance()->FetchOAuth2Tokens( Prefer to put FetchOAuth2Tokens logic out of UserSessionManager because it is already too big. How about we have some helper similar to BootstrapUserContextInitializer that do the token fetching and put the fetched tokens in UserContext (O2RT as well as the oauthlogin_access_token). https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:426: LOG(WARNING) << "UserSessionManager::OnOAuth2TokensFetchFailed"; Should we still call login_callback_.Run()? Otherwise, we stuck because ExistingUserController's flow is not resumed. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:834: user_context_.SetAuthCode(std::string()); Move 831-834 to OnOAuth2TokensAvailable. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1250: OAuth2LoginManager::RESTORE_FROM_PASSED_OAUTH2_ACCESS_TOKEN; We probably don't need a new strategy. After we get refresh token, it can fold into RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.h:452: std::string oauthlogin_access_token_; nit: Consider add this to UserContext. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.h:453: std::string refresh_token_; nit: Use refresh_token field in user_context_. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/signin/oauth2_login_manager.cc (right): https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/oauth2_login_manager.cc:247: // SID/LSID cookies through OAuthLogin call. nit: DCHECK_EQ(RESTORE_FROM_COOKIE_JAR, restore_strategy_) https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/oauth2_login_manager.cc:265: StoreOAuth2Token(); GetTokenHandle() to keep existing behavior?
achuith@chromium.org changed reviewers: + nkostylev@chromium.org
Xiyuan, Nikita: PTAL. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/existing_user_controller.cc:1127: UserSessionManager::GetInstance()->FetchOAuth2Tokens( On 2015/04/20 22:02:41, xiyuan wrote: > Prefer to put FetchOAuth2Tokens logic out of UserSessionManager because it is > already too big. > > How about we have some helper similar to BootstrapUserContextInitializer that do > the token fetching and put the fetched tokens in UserContext (O2RT as well as > the oauthlogin_access_token). Done. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:426: LOG(WARNING) << "UserSessionManager::OnOAuth2TokensFetchFailed"; On 2015/04/20 22:02:41, xiyuan wrote: > Should we still call login_callback_.Run()? Otherwise, we stuck because > ExistingUserController's flow is not resumed. Done. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:834: user_context_.SetAuthCode(std::string()); On 2015/04/20 22:02:41, xiyuan wrote: > Move 831-834 to OnOAuth2TokensAvailable. Done. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1250: OAuth2LoginManager::RESTORE_FROM_PASSED_OAUTH2_ACCESS_TOKEN; On 2015/04/20 22:02:41, xiyuan wrote: > We probably don't need a new strategy. After we get refresh token, it can fold > into RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN. So RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN doesn't create TokenHandleUtil and call GetTokenHandle - should it? Also, currently the code paths for RESTORE_FROM_AUTH_CODE and RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN are quite different. Are you sure this is safe? RestoreSessionFromSavedTokens does a bunch of stuff that we were not doing before with the auth-code restore. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.h:452: std::string oauthlogin_access_token_; On 2015/04/20 22:02:41, xiyuan wrote: > nit: Consider add this to UserContext. Done. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.h:453: std::string refresh_token_; On 2015/04/20 22:02:41, xiyuan wrote: > nit: Use refresh_token field in user_context_. Done. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/signin/oauth2_login_manager.cc (right): https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/oauth2_login_manager.cc:247: // SID/LSID cookies through OAuthLogin call. On 2015/04/20 22:02:42, xiyuan wrote: > nit: DCHECK_EQ(RESTORE_FROM_COOKIE_JAR, restore_strategy_) Done. https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/signin/oauth2_login_manager.cc:265: StoreOAuth2Token(); On 2015/04/20 22:02:41, xiyuan wrote: > GetTokenHandle() to keep existing behavior? Done.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1097663003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I'm running into a problem with the PRE_NoSAML browsertest: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... This piece of code is the problem: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... During this browser test, there is no DefaultNetwork(), so we just wait for the network to start, forever. I'm not sure what the right way is to get around this. I'll poke around some more, but if you have any ideas on how to create a default network for tests, please let me know.
For the default network problem, I wonder we could use the trick in OobeBaseTest::SimulateNetworkOnline [1]. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1250: OAuth2LoginManager::RESTORE_FROM_PASSED_OAUTH2_ACCESS_TOKEN; On 2015/04/21 07:10:21, achuithb wrote: > So RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN doesn't create TokenHandleUtil and > call GetTokenHandle - should it? > Yes, it should as long as refresh token is involved. It is needed to figure out when the refresh token is revoked. > Also, currently the code paths for RESTORE_FROM_AUTH_CODE and > RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN are quite different. Are you sure this > is safe? > It should be safe. Since we do auth code -> O2RT now, the auth code restore flow essentially becomes the refresh token restore flow. The should be the same. > RestoreSessionFromSavedTokens does a bunch of stuff that we were not doing > before with the auth-code restore. RestoreSessionFromSavedTokens should not be executed for O2RT restore flow either.
Patchset #4 (id:60001) has been deleted
On 2015/04/22 00:37:17, xiyuan wrote: > For the default network problem, I wonder we could use the trick in > OobeBaseTest::SimulateNetworkOnline [1]. > I looked into SimulateNetworkOnline, and that doesn't help. I believe OAuth2TokenFetcher does an unnecessary network check - this is handled by GaiaAuthFetcher, and there's a retry loop in OAuth2TokenFetcher::RetryOnError that does the same retry logic as we have here. It's also consistent with the way PolicyOAuth2TokenFetcher does things. So I've removed this unnecessary network check.
PTAL, Xiyuan! https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1097663003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1250: OAuth2LoginManager::RESTORE_FROM_PASSED_OAUTH2_ACCESS_TOKEN; On 2015/04/22 00:37:17, xiyuan wrote: > On 2015/04/21 07:10:21, achuithb wrote: > > So RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN doesn't create TokenHandleUtil and > > call GetTokenHandle - should it? > > > > Yes, it should as long as refresh token is involved. It is needed to figure out > when the refresh token is revoked. > > > Also, currently the code paths for RESTORE_FROM_AUTH_CODE and > > RESTORE_FROM_PASSED_OAUTH2_REFRESH_TOKEN are quite different. Are you sure > this > > is safe? > > > > It should be safe. Since we do auth code -> O2RT now, the auth code restore flow > essentially becomes the refresh token restore flow. The should be the same. > > > RestoreSessionFromSavedTokens does a bunch of stuff that we were not doing > > before with the auth-code restore. > > RestoreSessionFromSavedTokens should not be executed for O2RT restore flow > either. Done.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1097663003/80001
On 2015/04/22 22:58:04, achuithb wrote: > On 2015/04/22 00:37:17, xiyuan wrote: > > For the default network problem, I wonder we could use the trick in > > OobeBaseTest::SimulateNetworkOnline [1]. > > > > I looked into SimulateNetworkOnline, and that doesn't help. > > I believe OAuth2TokenFetcher does an unnecessary network check - this is handled > by GaiaAuthFetcher, and there's a retry loop in OAuth2TokenFetcher::RetryOnError > that does the same retry logic as we have here. It's also consistent with the > way PolicyOAuth2TokenFetcher does things. So I've removed this unnecessary > network check. Most of the retry logics are moved to GaiaAuthFetcher except for the portal check which is ChromeOS only and stayed in OAuth2TokenFetcher. We probably should it. Let me patch your CL and see if there is a way to setup test stub code to pass the check.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
On 2015/04/22 23:06:36, xiyuan wrote: > On 2015/04/22 22:58:04, achuithb wrote: > > On 2015/04/22 00:37:17, xiyuan wrote: > > > For the default network problem, I wonder we could use the trick in > > > OobeBaseTest::SimulateNetworkOnline [1]. > > > > > > > I looked into SimulateNetworkOnline, and that doesn't help. > > > > I believe OAuth2TokenFetcher does an unnecessary network check - this is > handled > > by GaiaAuthFetcher, and there's a retry loop in > OAuth2TokenFetcher::RetryOnError > > that does the same retry logic as we have here. It's also consistent with the > > way PolicyOAuth2TokenFetcher does things. So I've removed this unnecessary > > network check. > > Most of the retry logics are moved to GaiaAuthFetcher except for the portal > check which is ChromeOS only and stayed in OAuth2TokenFetcher. We probably > should it. > > Let me patch your CL and see if there is a way to setup test stub code to pass > the check. Thank you! The failing test is SamlSuite/SAMLPolicyTest.PRE_NoSAML/1 src/out/Debug/browser_tests --allow-file-access --gtest_filter=SamlSuite/SAMLPolicyTest.PRE_NoSAML/1 --single_process --user-data-dir=UserDataDir Basically, DefaultNetwork() returns NULL when the browser test runs, even if you call SimulateNetworkOnline. I haven't found any other instances of this DefaultNetwork() check in the codebase, so I'm suspicious that this is the right way to check for connectivity. Kiosk mode uses a different mechanism (NetworkState) to track online/offline accessibility, and I think this probably works correctly with the fake network_portal_detector_ in SimulateNetworkOnline.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Please excuse the rebase.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1097663003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/04/22 23:21:04, achuithb wrote: > src/out/Debug/browser_tests --allow-file-access > --gtest_filter=SamlSuite/SAMLPolicyTest.PRE_NoSAML/1 --single_process > --user-data-dir=UserDataDir > > Basically, DefaultNetwork() returns NULL when the browser test runs, even if you > call SimulateNetworkOnline. > Thanks for the info. We probably did not set up fake network env properly in OobeBaseTest (base calls of SamlTest). > I haven't found any other instances of this DefaultNetwork() check in the > codebase, so I'm suspicious that this is the right way to check for > connectivity. > > Kiosk mode uses a different mechanism (NetworkState) to track online/offline > accessibility, and I think this probably works correctly with the fake > network_portal_detector_ in SimulateNetworkOnline. This is the poor man's connectivity check. It is written before the nice NetworkPortalDetector code refactored out of login screen. It could be replaced with NetworkPortalDetector now but we just don't have time to get to it.
Mystery resolved.
FakeShillManagerClient::SetupDefaultEnvironment is not called when
DBusThreadManager::GetSetterForTesting to explicitly replace a service.
SAMLPolicyTest uses that to set SessionManagerClient and we need to manually
setup the fake networks.
Essentially, we need the following code in SAMLPolicyTest's SetUpOnMainThread as
in DeviceDisablingTest [1]:
DBusThreadManager::Get()->GetShillManagerClient()->GetTestInterface()->
SetupDefaultEnvironment();
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
On 2015/04/23 01:25:04, xiyuan wrote: > Mystery resolved. > > FakeShillManagerClient::SetupDefaultEnvironment is not called when > DBusThreadManager::GetSetterForTesting to explicitly replace a service. > SAMLPolicyTest uses that to set SessionManagerClient and we need to manually > setup the fake networks. > > Essentially, we need the following code in SAMLPolicyTest's SetUpOnMainThread as > in DeviceDisablingTest [1]: > > DBusThreadManager::Get()->GetShillManagerClient()->GetTestInterface()-> > SetupDefaultEnvironment(); > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... Thank you! Done! PTAL :)
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1097663003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1127: user_context.GetAuthFlow() == UserContext::AUTH_FLOW_GAIA_WITHOUT_SAML) { What happens for AUTH_FLOW_GAIA_WITH_SAML case? Shouldn't we do the same i.e. exchange auth code received from GAIA to access token? https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1127: user_context.GetAuthFlow() == UserContext::AUTH_FLOW_GAIA_WITHOUT_SAML) { What about AUTH_FLOW_EASY_BOOTSTRAP? https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/signin/oauth2_login_manager.cc (left): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/signin/oauth2_login_manager.cc:262: SetSessionRestoreState(OAuth2LoginManager::SESSION_RESTORE_FAILED); I suggest keeping this code as a safety net when FetchOAuth2Tokens() is called with incorrect restore_strategy_
lgtm https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc:37: LOG(WARNING) << "UserSessionManager::OnOAuth2TokensFetchFailed"; nit: Please update comment.
LGTM with nits https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1127: user_context.GetAuthFlow() == UserContext::AUTH_FLOW_GAIA_WITHOUT_SAML) { > What about AUTH_FLOW_EASY_BOOTSTRAP? AUTH_FLOW_EASY_BOOTSTRAP uses the branch at line 1116-1123. Eventually, that would be merged with the code here when the testing frontend is merged into new Gaia. > What happens for AUTH_FLOW_GAIA_WITH_SAML case? > Shouldn't we do the same i.e. exchange auth code received from GAIA to access > token? Either way works at the moment. Cookies will be set for SAML flow. But I agree it might be better not to differentiate. So let's remove the AUTH_FLOW_GAIA_WITHOUT_SAML check here and only test auth code existence. https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/saml/saml_browsertest.cc:300: ->SetupDefaultEnvironment(); nit: Move this to SAMLPolicyTest::SetUpOnMainThread since DBusThreadManager::GetSetterForTesting is only called in SAMLPolicyTest https://codereview.chromium.org/1097663003/diff/120001/chromeos/login/auth/us... File chromeos/login/auth/user_context.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chromeos/login/auth/us... chromeos/login/auth/user_context.cc:62: context.public_session_input_method_ == public_session_input_method_; nit: Update this to include access_token_ and refresh_token_. I forgot this last time. :p https://codereview.chromium.org/1097663003/diff/120001/chromeos/login/auth/us... File chromeos/login/auth/user_context.h (right): https://codereview.chromium.org/1097663003/diff/120001/chromeos/login/auth/us... chromeos/login/auth/user_context.h:84: std::string access_token_; nit: // OAuthLogin scoped access token.
Xiyuan, could you please make another pass through this CL? There were quite a few nits addressed in the last patch. https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1127: user_context.GetAuthFlow() == UserContext::AUTH_FLOW_GAIA_WITHOUT_SAML) { On 2015/04/23 16:09:23, xiyuan wrote: > > What about AUTH_FLOW_EASY_BOOTSTRAP? > > AUTH_FLOW_EASY_BOOTSTRAP uses the branch at line 1116-1123. Eventually, that > would be merged with the code here when the testing frontend is merged into new > Gaia. > > > What happens for AUTH_FLOW_GAIA_WITH_SAML case? > > Shouldn't we do the same i.e. exchange auth code received from GAIA to access > > token? > > Either way works at the moment. Cookies will be set for SAML flow. But I agree > it might be better not to differentiate. So let's remove the > AUTH_FLOW_GAIA_WITHOUT_SAML check here and only test auth code existence. Done. https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1127: user_context.GetAuthFlow() == UserContext::AUTH_FLOW_GAIA_WITHOUT_SAML) { On 2015/04/23 10:59:25, Nikita Kostylev wrote: > What about AUTH_FLOW_EASY_BOOTSTRAP? Done. https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1127: user_context.GetAuthFlow() == UserContext::AUTH_FLOW_GAIA_WITHOUT_SAML) { On 2015/04/23 10:59:25, Nikita Kostylev wrote: > What happens for AUTH_FLOW_GAIA_WITH_SAML case? > Shouldn't we do the same i.e. exchange auth code received from GAIA to access > token? Done. https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/saml/saml_browsertest.cc:300: ->SetupDefaultEnvironment(); On 2015/04/23 16:09:23, xiyuan wrote: > nit: Move this to SAMLPolicyTest::SetUpOnMainThread since > DBusThreadManager::GetSetterForTesting is only called in SAMLPolicyTest Done. https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/signin/oauth2_login_manager.cc (left): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/signin/oauth2_login_manager.cc:262: SetSessionRestoreState(OAuth2LoginManager::SESSION_RESTORE_FAILED); On 2015/04/23 10:59:25, Nikita Kostylev wrote: > I suggest keeping this code as a safety net when FetchOAuth2Tokens() is called > with incorrect restore_strategy_ Done. https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/signin/oauth2_token_initializer.cc:37: LOG(WARNING) << "UserSessionManager::OnOAuth2TokensFetchFailed"; On 2015/04/23 11:14:24, Nikita Kostylev wrote: > nit: Please update comment. Done. https://codereview.chromium.org/1097663003/diff/120001/chromeos/login/auth/us... File chromeos/login/auth/user_context.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chromeos/login/auth/us... chromeos/login/auth/user_context.cc:62: context.public_session_input_method_ == public_session_input_method_; On 2015/04/23 16:09:23, xiyuan wrote: > nit: Update this to include access_token_ and refresh_token_. I forgot this last > time. :p Done. Sorry for the oversight!
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, nkostylev@chromium.org Link to the patchset: https://codereview.chromium.org/1097663003/#ps140001 (title: "Xiyuan/Nikita feedback")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1097663003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
SLGTM
On 2015/04/23 23:28:31, xiyuan wrote: > SLGTM Thanks for all the suggestions and feedback!
The CQ bit was checked by achuith@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1097663003/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/163a4a63af5e56801395eed067a69e501e7b51a5 Cr-Commit-Position: refs/heads/master@{#326691}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:140001) has been created in https://codereview.chromium.org/1102863002/ by horo@chromium.org. The reason for reverting is: Caused browser_tests failure. https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... .
Message was sent while issue was closed.
https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1127: user_context.GetAuthFlow() == UserContext::AUTH_FLOW_GAIA_WITHOUT_SAML) { Oops. What Achuith had before is actually correct. I forgot about the Saml IdP's cookies. Going down auth code path is okay for google but without the |has_auth_cookies| flag, Saml cookies are not transferred. And we should not be fetching tokens with |has_auth_cookies|. This is why we get the DCHECK failure for SAML tests. So let's restore this and update date comment to explain why. Something like: // Fetch OAuth2 tokens if we have an auth code and not using SAML. SAML // uses |has_auth_cookies| flag and use cookies to get tokens. https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1221: } else if (!user_context_.GetRefreshToken().empty()) { We need to include !user_context_.GetAuthCode().empty() here too so that auth code uses the restore via refresh token.
Message was sent while issue was closed.
dpolukhin@chromium.org changed reviewers: + dpolukhin@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1217: OnAuthFailure(AuthFailure(AuthFailure::NETWORK_AUTH_FAILED)); FYI, it causes DCHECK in https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/login/aut... For some test authentication failure is expected. Perhaps we have to remove that check if now it is possible in some cases.
Message was sent while issue was closed.
https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1217: OnAuthFailure(AuthFailure(AuthFailure::NETWORK_AUTH_FAILED)); On 2015/04/24 12:51:08, Dmitry Polukhin wrote: > FYI, it causes DCHECK in > https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/login/aut... > > For some test authentication failure is expected. Perhaps we have to remove that > check if now it is possible in some cases. That DCHECK is probably meant to prevent setting NETWORK_AUTH_FAILED without setting an error_. I don't know how important the check is. Maybe set a default error if NETWORK_AUTH_FAILED is used and remove the DCHECK? https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1221: } else if (!user_context_.GetRefreshToken().empty()) { On 2015/04/24 03:36:06, xiyuan wrote: > We need to include !user_context_.GetAuthCode().empty() here too so that auth > code uses the restore via refresh token. Ignore this. Forgot that token initializer would exchange auth code for RT and this would not be necessary. :p
Message was sent while issue was closed.
https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1217: OnAuthFailure(AuthFailure(AuthFailure::NETWORK_AUTH_FAILED)); On 2015/04/24 13:54:43, xiyuan wrote: > On 2015/04/24 12:51:08, Dmitry Polukhin wrote: > > FYI, it causes DCHECK in > > > https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/login/aut... > > > > For some test authentication failure is expected. Perhaps we have to remove > that > > check if now it is possible in some cases. > > That DCHECK is probably meant to prevent setting NETWORK_AUTH_FAILED without > setting an error_. I don't know how important the check is. Maybe set a default > error if NETWORK_AUTH_FAILED is used and remove the DCHECK? I'm not familiar with this AuthFailure code but my impression was the same. I tried to simple remove DCHECK but it won't help because code will crash later when will try to report error and LoginPerforme is missing. It seems that this CL needs a bit more to handle failure token fetch errors. I'm working BlockingLoginTest with this CL it almost passes it passes when all steps succeed.
Message was sent while issue was closed.
https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1217: OnAuthFailure(AuthFailure(AuthFailure::NETWORK_AUTH_FAILED)); On 2015/04/24 14:02:27, Dmitry Polukhin wrote: > On 2015/04/24 13:54:43, xiyuan wrote: > > On 2015/04/24 12:51:08, Dmitry Polukhin wrote: > > > FYI, it causes DCHECK in > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/login/aut... > > > > > > For some test authentication failure is expected. Perhaps we have to remove > > that > > > check if now it is possible in some cases. > > > > That DCHECK is probably meant to prevent setting NETWORK_AUTH_FAILED without > > setting an error_. I don't know how important the check is. Maybe set a > default > > error if NETWORK_AUTH_FAILED is used and remove the DCHECK? > > I'm not familiar with this AuthFailure code but my impression was the same. I > tried to simple remove DCHECK but it won't help because code will crash later > when will try to report error and LoginPerforme is missing. It seems that this > CL needs a bit more to handle failure token fetch errors. I'm working > BlockingLoginTest with this CL it almost passes it passes when all steps > succeed. Maybe we should create a new AuthFailure reason (e.g. FAILED_TO_INITIALIZE_TOKEN) instead of NETWORK_AUTH_FAILED? Would that still hit the NULL LoginPerformer problem?
PTAL, Xiyuan. Also, please advice on the best course for the token failure reporting issue. I'm happy to create a new auth failure enum if this helps matters. I'm unable to see any test failures, so I'm not sure how to reproduce this problem. https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1127: user_context.GetAuthFlow() == UserContext::AUTH_FLOW_GAIA_WITHOUT_SAML) { On 2015/04/24 03:36:06, xiyuan wrote: > Oops. What Achuith had before is actually correct. I forgot about the Saml IdP's > cookies. Going down auth code path is okay for google but without the > |has_auth_cookies| flag, Saml cookies are not transferred. And we should not be > fetching tokens with |has_auth_cookies|. This is why we get the DCHECK failure > for SAML tests. > > So let's restore this and update date comment to explain why. Something like: > > // Fetch OAuth2 tokens if we have an auth code and not using SAML. SAML > // uses |has_auth_cookies| flag and use cookies to get tokens. Done. https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1217: OnAuthFailure(AuthFailure(AuthFailure::NETWORK_AUTH_FAILED)); On 2015/04/24 15:49:38, xiyuan wrote: > On 2015/04/24 14:02:27, Dmitry Polukhin wrote: > > On 2015/04/24 13:54:43, xiyuan wrote: > > > On 2015/04/24 12:51:08, Dmitry Polukhin wrote: > > > > FYI, it causes DCHECK in > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/login/aut... > > > > > > > > For some test authentication failure is expected. Perhaps we have to > remove > > > that > > > > check if now it is possible in some cases. > > > > > > That DCHECK is probably meant to prevent setting NETWORK_AUTH_FAILED without > > > setting an error_. I don't know how important the check is. Maybe set a > > default > > > error if NETWORK_AUTH_FAILED is used and remove the DCHECK? > > > > I'm not familiar with this AuthFailure code but my impression was the same. I > > tried to simple remove DCHECK but it won't help because code will crash later > > when will try to report error and LoginPerforme is missing. It seems that this > > CL needs a bit more to handle failure token fetch errors. I'm working > > BlockingLoginTest with this CL it almost passes it passes when all steps > > succeed. > > Maybe we should create a new AuthFailure reason (e.g. > FAILED_TO_INITIALIZE_TOKEN) instead of NETWORK_AUTH_FAILED? Would that still hit > the NULL LoginPerformer problem? Is this a problem with a test in the current test suite (the tests are passing for me? Can we land this change and fix this in a follow-up? https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1221: } else if (!user_context_.GetRefreshToken().empty()) { On 2015/04/24 13:54:43, xiyuan wrote: > On 2015/04/24 03:36:06, xiyuan wrote: > > We need to include !user_context_.GetAuthCode().empty() here too so that auth > > code uses the restore via refresh token. > > Ignore this. Forgot that token initializer would exchange auth code for RT and > this would not be necessary. :p Yup, I got confused a bit. The auth code should be empty at this point for non-SAML since we clear it after fetching the tokens.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, nkostylev@chromium.org Link to the patchset: https://codereview.chromium.org/1097663003/#ps180001 (title: "OAuth2TokenInitializer for non-SAML only")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1097663003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM! https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1217: OnAuthFailure(AuthFailure(AuthFailure::NETWORK_AUTH_FAILED)); On 2015/04/25 00:53:32, achuithb wrote: > On 2015/04/24 15:49:38, xiyuan wrote: > > On 2015/04/24 14:02:27, Dmitry Polukhin wrote: > > > On 2015/04/24 13:54:43, xiyuan wrote: > > > > On 2015/04/24 12:51:08, Dmitry Polukhin wrote: > > > > > FYI, it causes DCHECK in > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/login/aut... > > > > > > > > > > For some test authentication failure is expected. Perhaps we have to > > remove > > > > that > > > > > check if now it is possible in some cases. > > > > > > > > That DCHECK is probably meant to prevent setting NETWORK_AUTH_FAILED > without > > > > setting an error_. I don't know how important the check is. Maybe set a > > > default > > > > error if NETWORK_AUTH_FAILED is used and remove the DCHECK? > > > > > > I'm not familiar with this AuthFailure code but my impression was the same. > I > > > tried to simple remove DCHECK but it won't help because code will crash > later > > > when will try to report error and LoginPerforme is missing. It seems that > this > > > CL needs a bit more to handle failure token fetch errors. I'm working > > > BlockingLoginTest with this CL it almost passes it passes when all steps > > > succeed. > > > > Maybe we should create a new AuthFailure reason (e.g. > > FAILED_TO_INITIALIZE_TOKEN) instead of NETWORK_AUTH_FAILED? Would that still > hit > > the NULL LoginPerformer problem? > > Is this a problem with a test in the current test suite (the tests are passing > for me? Can we land this change and fix this in a follow-up? I vote for landing this CL first and address the NETWORK_AUTH_FAILED issue in a separate CL.
On 2015/04/25 03:25:09, xiyuan wrote: > LGTM! Thank you!
The CQ bit was checked by achuith@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1097663003/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/09f1fa1af73e355514a2eb1594c4ba1f2e76acbd Cr-Commit-Position: refs/heads/master@{#326967}
Message was sent while issue was closed.
https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://codereview.chromium.org/1097663003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/existing_user_controller.cc:1217: OnAuthFailure(AuthFailure(AuthFailure::NETWORK_AUTH_FAILED)); On 2015/04/25 03:25:09, xiyuan wrote: > On 2015/04/25 00:53:32, achuithb wrote: > > On 2015/04/24 15:49:38, xiyuan wrote: > > > On 2015/04/24 14:02:27, Dmitry Polukhin wrote: > > > > On 2015/04/24 13:54:43, xiyuan wrote: > > > > > On 2015/04/24 12:51:08, Dmitry Polukhin wrote: > > > > > > FYI, it causes DCHECK in > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/login/aut... > > > > > > > > > > > > For some test authentication failure is expected. Perhaps we have to > > > remove > > > > > that > > > > > > check if now it is possible in some cases. > > > > > > > > > > That DCHECK is probably meant to prevent setting NETWORK_AUTH_FAILED > > without > > > > > setting an error_. I don't know how important the check is. Maybe set a > > > > default > > > > > error if NETWORK_AUTH_FAILED is used and remove the DCHECK? > > > > > > > > I'm not familiar with this AuthFailure code but my impression was the > same. > > I > > > > tried to simple remove DCHECK but it won't help because code will crash > > later > > > > when will try to report error and LoginPerforme is missing. It seems that > > this > > > > CL needs a bit more to handle failure token fetch errors. I'm working > > > > BlockingLoginTest with this CL it almost passes it passes when all steps > > > > succeed. > > > > > > Maybe we should create a new AuthFailure reason (e.g. > > > FAILED_TO_INITIALIZE_TOKEN) instead of NETWORK_AUTH_FAILED? Would that still > > hit > > > the NULL LoginPerformer problem? > > > > Is this a problem with a test in the current test suite (the tests are passing > > for me? Can we land this change and fix this in a follow-up? > > I vote for landing this CL first and address the NETWORK_AUTH_FAILED issue in a > separate CL. I'll take care of NETWORK_AUTH_FAILED. Tests that are failing are BlockingLoginTest with enabled webview (in trunk they are disabled for webview). |
