Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12929014:
Identity API: Pop-up a sign-in dialog if gaia credentials are bad (Closed)
|
Created:
7 years, 9 months ago by Michael Courage Modified:
7 years, 6 months ago Reviewers:
Roger Tawa OOO till Jul 10th, Andrew T Wilson (Slow), Evan Stade, miket_OOO, Pete Williamson, jhawkins CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org, dcheng, Pete Williamson, Andrew T Wilson (Slow), Munjal (Google), saroop Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Visibility:
Public. |
DescriptionIdentity API: Pop-up a sign-in dialog if gaia credentials are bad
TBR=jhawkins@chromium.org
(for files added to chrome/chrome_browser_extensions.gypi)
BUG=222774
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192471
Patch Set 1 #
Total comments: 2
Patch Set 2 : improve ui flow for web-based sign-in #Patch Set 3 : Use SigninGlobalError and chrome::ShowBrowserSignin #Patch Set 4 : launch sign-in through LoginUIService #
Total comments: 12
Patch Set 5 : address code review comments #Patch Set 6 : improve interactive flag clarity #
Total comments: 14
Patch Set 7 : address code review comments #
Total comments: 9
Patch Set 8 : address code review comments #Patch Set 9 : rebase #Patch Set 10 : windows build fix #
Total comments: 2
Patch Set 11 : fix signin browser test for chromeos #Messages
Total messages: 28 (0 generated)
Putting this out to get comments on the approach. Recommendation in the bug was to use an AuthStatusProvider with SigninGlobalError to indicate the problem to the user, or integrate with TokenService, which already does so. I think the change here may be better for getAuthToken for two reasons: 1. It's small. 2. The user may not have any browser windows open, and it doesn't look to me like SigninGlobalError will do anything helpful in that case.
https://codereview.chromium.org/12929014/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/12929014/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/identity_api.cc:114: How will the user know *why* they are being asked to signin? If I were the user, and I saw this dialog when I thought chrome was signed in, I'd be confused.
Refactored sign-in into its own flow and made it work better with the web-based dialogs. https://codereview.chromium.org/12929014/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/12929014/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/identity/identity_api.cc:114: On 2013/03/22 16:42:34, Pete Williamson wrote: > How will the user know *why* they are being asked to signin? If I were the > user, and I saw this dialog when I thought chrome was signed in, I'd be > confused. There is nothing on the sign in screen that explains. This only happens when the interactive flag is set though, so typically the dialog will pop up right after the user clicked a button labeled "Sign In" or something similar.
https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:297: content::Details<TokenService::TokenAvailableDetails>(details).ptr(); It seems odd that we aren't doing much with the token_details. Why do we need to observe both here and in IdentitySigninFlow? https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_api.h (right): https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.h:85: scoped_ptr<OAuth2MintTokenFlow> flow_; Since we have multiple flows, it might be nice to rename this one to oauth_flow_ https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_apitest.cc (right): https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_apitest.cc:233: EXPECT_TRUE(StartsWithASCII(error, errors::kAuthFailure, false)); This seems like we are just testing that the returned error starts with a letter A-Za-z. If I have understood that correctly, can we instead match against the exact error we expect? That would make the test more robust. https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.cc (right): https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:5: #include "chrome/browser/extensions/api/identity/identity_signin_flow.h" Should there be a unit test for this file? https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:50: } This implies to me that the signin flow object should only be used once, we should never call Start a second time. Let's add that limitation to the .h file description. https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.h (right): https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.h:41: // SigninTracker::Observer implementation. If we aren't using the signin tracker observer yet, let's wait until the signin tracker is working properly, and add it then, so we aren't adding dead code.
https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:297: content::Details<TokenService::TokenAvailableDetails>(details).ptr(); On 2013/03/27 18:03:55, Pete Williamson wrote: > It seems odd that we aren't doing much with the token_details. Why do we need > to observe both here and in IdentitySigninFlow? This class has an interest in the token independent of IdentitySigninFlow. In a non-interactive getAuthToken call, the API call will return immediately, but the error state persists until a token is added. The flow is waiting for the TokenService to provide a token, so it makes sense for that class to listen to the TokenService event directly rather than proxying the event through IdentityAPI, which is acting as an error status service. https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_api.h (right): https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.h:85: scoped_ptr<OAuth2MintTokenFlow> flow_; On 2013/03/27 18:03:55, Pete Williamson wrote: > Since we have multiple flows, it might be nice to rename this one to oauth_flow_ Done. https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_apitest.cc (right): https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_apitest.cc:233: EXPECT_TRUE(StartsWithASCII(error, errors::kAuthFailure, false)); On 2013/03/27 18:03:55, Pete Williamson wrote: > This seems like we are just testing that the returned error starts with a letter > A-Za-z. If I have understood that correctly, can we instead match against the > exact error we expect? That would make the test more robust. StartsWithASCII verifies that error starts with errors::kAuthFailure. The remainder of the string is provided by GoogleServiceAuthError, which should have its own tests. https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.cc (right): https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:5: #include "chrome/browser/extensions/api/identity/identity_signin_flow.h" On 2013/03/27 18:03:55, Pete Williamson wrote: > Should there be a unit test for this file? Pretty much everything in this file would have to be stubbed out for a test, so I think the answer is no. https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:50: } On 2013/03/27 18:03:55, Pete Williamson wrote: > This implies to me that the signin flow object should only be used once, we > should never call Start a second time. Let's add that limitation to the .h file > description. Done. https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.h (right): https://codereview.chromium.org/12929014/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.h:41: // SigninTracker::Observer implementation. On 2013/03/27 18:03:55, Pete Williamson wrote: > If we aren't using the signin tracker observer yet, let's wait until the signin > tracker is working properly, and add it then, so we aren't adding dead code. Done.
lgtm
Hi reviewers, please have a look at this change. atwilson: check out the SigninGlobalError implementation in chrome/browser/extensions/api/identity/identity_api.* benwells: owner for chrome/browser/extensions jhawkins: owner for chrome/chrome_browser_extensions.gypi chrome/browser/ui/webui
Ping. Bug is an M27-stable release blocker. PTAL or let me know if I should find another reviewer.
Ping again. Also, dcheng pointed out that the relationship between interactive_ and should_retry_with_signin_ was not very clear, so I changed the names and added some comments to make it more clear that one controls the signin prompt and the other is for the scope prompt.
lgtm https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:300: const content::NotificationSource& source, indentation got weird here, maybe a rename gone wild https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:311: NOTREACHED(); Not a huge deal, but this seems like an odd way to structure this block. Is it better than a CHECK(type)? Are you envisioning a future second type? If so, why not a switch? https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_apitest.cc (right): https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_apitest.cc:459: scoped_refptr<IdentityLaunchWebAuthFlowFunction> function( Was there a specific test for the ChromeOS case? Or is it already encompassed in one of these tests? I can't really tell. If the ChromeOS case is not tested, it'd be nice to add that. https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.cc (right): https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:23: delegate_ = NULL; This is a naked pointer. It's OK to just let it die. Alternatively, if there used to be an ordering constraint in the destructor that got refactored out, please clean this up. https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:46: const content::NotificationSource& source, indent messed up https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:53: } else { same comment as earlier about unusual block structure (and as before, OK if you have a reason for it) https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:61: delegate_ = NULL; These nulls make me nervous. Have you made a promise somewhere that the delegate methods will be called only once? I worry that if these turned out to be necessary to prevent multiple calls, maybe they're symptomatic of a deeper bug, like maybe the token service is not well. Alternatively, if you're just doing it for cleanliness, then the downside is that they might mask buggy behavior like what I've described above.
https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:300: const content::NotificationSource& source, On 2013/03/29 22:06:22, miket wrote: > indentation got weird here, maybe a rename gone wild Done. https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:311: NOTREACHED(); Done. I prefer the CHECK(type) approach, but I've had other reviewers request that I do it this way. https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_apitest.cc (right): https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_apitest.cc:459: scoped_refptr<IdentityLaunchWebAuthFlowFunction> function( There was actually a missing case for ChromeOS. Added InteractiveMintBadCredentialsLoginCanceled. (note, the actual platform specific code is stubbed out in all of these tests, so there are no #ifdefs in this file). https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.cc (right): https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:23: delegate_ = NULL; Done. Forgot to clean this up after removing the reentrancy issue that originally necessitated it. https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:46: const content::NotificationSource& source, On 2013/03/29 22:06:22, miket wrote: > indent messed up Done. https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:53: } else { On 2013/03/29 22:06:22, miket wrote: > same comment as earlier about unusual block structure (and as before, OK if you > have a reason for it) Done. https://codereview.chromium.org/12929014/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:61: delegate_ = NULL; On 2013/03/29 22:06:22, miket wrote: > These nulls make me nervous. Have you made a promise somewhere that the delegate > methods will be called only once? I worry that if these turned out to be > necessary to prevent multiple calls, maybe they're symptomatic of a deeper bug, > like maybe the token service is not well. > > Alternatively, if you're just doing it for cleanliness, then the downside is > that they might mask buggy behavior like what I've described above. Done.
+rogerta, Roger could you look at this change also given Drew's absence?
lgtm with some nits below. https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:63: // Check that the necessary information is present in the manfist. manfist --> manifest? https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:150: error_ = identity_constants::kUserNotSignedIn; clear |refresh_token_| ? https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.cc (right): https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:51: } What happens if this notification is never sent after calling ShowLoginPopup()? https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.h (right): https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.h:18: class IdentitySigninFlow : public content::NotificationObserver { Please add a comment describing what this class does in more details.
https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_api.cc (right): https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:63: // Check that the necessary information is present in the manfist. On 2013/04/02 14:41:07, Roger Tawa wrote: > manfist --> manifest? Done. https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_api.cc:150: error_ = identity_constants::kUserNotSignedIn; On 2013/04/02 14:41:07, Roger Tawa wrote: > clear |refresh_token_| ? Once we trigger the response, the whole object is going to be deleted, so clearing out state doesn't matter. https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.cc (right): https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:51: } On 2013/04/02 14:41:07, Roger Tawa wrote: > What happens if this notification is never sent after calling ShowLoginPopup()? The (async) getAuthToken call will not complete until a token becomes available. I'd like to be able to watch for an aborted sign-in flow through a SigninTracker or LoginUIService, but that's a larger change that I didn't want to hold up this fix for in M27. I'll file a separate issue for handling aborted sign-ins. https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.h (right): https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.h:18: class IdentitySigninFlow : public content::NotificationObserver { On 2013/04/02 14:41:07, Roger Tawa wrote: > Please add a comment describing what this class does in more details. Done.
https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... File chrome/browser/extensions/api/identity/identity_signin_flow.cc (right): https://codereview.chromium.org/12929014/diff/33001/chrome/browser/extensions... chrome/browser/extensions/api/identity/identity_signin_flow.cc:51: } On 2013/04/02 17:17:49, Michael Courage wrote: > On 2013/04/02 14:41:07, Roger Tawa wrote: > > What happens if this notification is never sent after calling > ShowLoginPopup()? > > The (async) getAuthToken call will not complete until a token becomes available. > I'd like to be able to watch for an aborted sign-in flow through a SigninTracker > or LoginUIService, but that's a larger change that I didn't want to hold up this > fix for in M27. I'll file a separate issue for handling aborted sign-ins. Is the plan to also resolve this separate issue in M27? If the getAuthToken() call never completes, now does the caller deal with this situation? Can they cancel the call?
> On 2013/04/02 17:17:49, Michael Courage wrote: > > I'll file a separate issue for handling aborted sign-ins. On 2013/04/02 18:00:24, Roger Tawa wrote: > Is the plan to also resolve this separate issue in M27? If the getAuthToken() > call never completes, now does the caller deal with this situation? Can they > cancel the call? No, I was not planning to handle this case in M27. Calls can't be cancelled, the caller just has to wait. The same would be true if the user ignored the sign-in tab without closing it. I don't believe aborted sign-ins will be a blocking issue for the apps using the (still experimental) API, while the issues being addressed in the CL definitely are blockers.
OK. Thanks, Roger - On Tue, Apr 2, 2013 at 2:13 PM, <courage@chromium.org> wrote: >> On 2013/04/02 17:17:49, Michael Courage wrote: >> > I'll file a separate issue for handling aborted sign-ins. > > > On 2013/04/02 18:00:24, Roger Tawa wrote: >> >> Is the plan to also resolve this separate issue in M27? If the >> getAuthToken() >> call never completes, now does the caller deal with this situation? Can >> they >> cancel the call? > > > No, I was not planning to handle this case in M27. Calls can't be cancelled, > the > caller just has to wait. The same would be true if the user ignored the > sign-in > tab without closing it. > > I don't believe aborted sign-ins will be a blocking issue for the apps using > the > (still experimental) API, while the issues being addressed in the CL > definitely > are blockers. > > https://codereview.chromium.org/12929014/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/courage@chromium.org/12929014/41001
Failed to apply patch for
chrome/browser/extensions/api/identity/identity_api.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/extensions/api/identity/identity_api.cc
Hunk #1 succeeded at 12 (offset 1 line).
Hunk #2 succeeded at 24 (offset 1 line).
Hunk #3 succeeded at 49 (offset 5 lines).
Hunk #4 succeeded at 77 (offset 5 lines).
Hunk #5 succeeded at 108 (offset 5 lines).
Hunk #6 succeeded at 133 (offset 5 lines).
Hunk #7 succeeded at 146 (offset 5 lines).
Hunk #8 succeeded at 170 (offset 5 lines).
Hunk #9 FAILED at 184.
Hunk #10 succeeded at 270 (offset 18 lines).
Hunk #11 succeeded at 311 (offset 18 lines).
1 out of 11 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/identity/identity_api.cc.rej
Patch: chrome/browser/extensions/api/identity/identity_api.cc
Index: chrome/browser/extensions/api/identity/identity_api.cc
diff --git a/chrome/browser/extensions/api/identity/identity_api.cc
b/chrome/browser/extensions/api/identity/identity_api.cc
index
72658a33fcf26119ac4088fe958bfa6e838e4acc..928c83193cdf4d52c1dd29d505f6fdaa828c41f7
100644
--- a/chrome/browser/extensions/api/identity/identity_api.cc
+++ b/chrome/browser/extensions/api/identity/identity_api.cc
@@ -11,13 +11,11 @@
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/extensions/permissions_updater.h"
#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/signin/signin_manager.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
#include "chrome/browser/signin/token_service.h"
#include "chrome/browser/signin/token_service_factory.h"
#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_navigator.h"
-#include "chrome/browser/ui/webui/signin/login_ui_service.h"
-#include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
-#include "chrome/browser/ui/webui/sync_promo/sync_promo_ui.h"
#include "chrome/common/extensions/api/experimental_identity.h"
#include "chrome/common/extensions/api/identity/oauth2_manifest_handler.h"
#include "chrome/common/extensions/extension.h"
@@ -25,6 +23,7 @@
#include "chrome/common/extensions/manifest_handler.h"
#include "chrome/common/url_constants.h"
#include "content/public/common/page_transition_types.h"
+#include "google_apis/gaia/gaia_constants.h"
#include "googleurl/src/gurl.h"
#include "ui/base/window_open_disposition.h"
@@ -45,18 +44,23 @@ namespace LaunchWebAuthFlow =
api::experimental_identity::LaunchWebAuthFlow;
namespace identity = api::experimental_identity;
IdentityGetAuthTokenFunction::IdentityGetAuthTokenFunction()
- : interactive_(false) {}
+ : should_prompt_for_scopes_(false),
+ should_prompt_for_signin_(false) {}
IdentityGetAuthTokenFunction::~IdentityGetAuthTokenFunction() {}
bool IdentityGetAuthTokenFunction::RunImpl() {
scoped_ptr<GetAuthToken::Params>
params(GetAuthToken::Params::Create(*args_));
EXTENSION_FUNCTION_VALIDATE(params.get());
- if (params->details.get() && params->details->interactive.get())
- interactive_ = *params->details->interactive;
+ bool interactive = params->details.get() &&
+ params->details->interactive.get() &&
+ *params->details->interactive;
+
+ should_prompt_for_scopes_ = interactive;
+ should_prompt_for_signin_ = interactive;
const OAuth2Info& oauth2_info = OAuth2Info::GetOAuth2Info(GetExtension());
- // Check that the necessary information is present in the manfist.
+ // Check that the necessary information is present in the manifest.
if (oauth2_info.client_id.empty()) {
error_ = identity_constants::kInvalidClientId;
return false;
@@ -68,24 +72,26 @@ bool IdentityGetAuthTokenFunction::RunImpl() {
}
// Balanced in OnIssueAdviceSuccess|OnMintTokenSuccess|OnMintTokenFailure|
- // InstallUIAbort|OnLoginUIClosed.
+ // InstallUIAbort|SigninFailed.
AddRef();
if (!HasLoginToken()) {
- if (StartLogin()) {
- return true;
- } else {
+ if (!should_prompt_for_signin_) {
+ error_ = identity_constants::kUserNotSignedIn;
Release();
return false;
}
- }
-
- if (StartFlow(OAuth2MintTokenFlow::MODE_MINT_TOKEN_NO_FORCE)) {
- return true;
+ // Display a login prompt. If the subsequent mint fails, don't display the
+ // prompt again.
+ should_prompt_for_signin_ = false;
+ ShowLoginPopup();
} else {
- Release();
- return false;
+ TokenService* token_service =
TokenServiceFactory::GetForProfile(profile());
+ refresh_token_ = token_service->GetOAuth2LoginRefreshToken();
+ StartFlow(OAuth2MintTokenFlow::MODE_MINT_TOKEN_NO_FORCE);
}
+
+ return true;
}
void IdentityGetAuthTokenFunction::OnMintTokenSuccess(
@@ -97,6 +103,24 @@ void IdentityGetAuthTokenFunction::OnMintTokenSuccess(
void IdentityGetAuthTokenFunction::OnMintTokenFailure(
const GoogleServiceAuthError& error) {
+ switch (error.state()) {
+ case GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS:
+ case GoogleServiceAuthError::ACCOUNT_DELETED:
+ case GoogleServiceAuthError::ACCOUNT_DISABLED:
+ extensions::IdentityAPI::GetFactoryInstance()->GetForProfile(
+ profile())->ReportAuthError(error);
+ if (should_prompt_for_signin_) {
+ // Display a login prompt and try again (once).
+ should_prompt_for_signin_ = false;
+ ShowLoginPopup();
+ return;
+ }
+ break;
+ default:
+ // Return error to caller.
+ break;
+ }
+
error_ = std::string(identity_constants::kAuthFailure) + error.ToString();
SendResponse(false);
Release(); // Balanced in RunImpl.
@@ -104,9 +128,10 @@ void IdentityGetAuthTokenFunction::OnMintTokenFailure(
void IdentityGetAuthTokenFunction::OnIssueAdviceSuccess(
const IssueAdviceInfo& issue_advice) {
+ should_prompt_for_signin_ = false;
// Existing grant was revoked and we used NO_FORCE, so we got info back
// instead.
- if (interactive_) {
+ if (should_prompt_for_scopes_) {
install_ui_.reset(new ExtensionInstallPrompt(GetAssociatedWebContents()));
ShowOAuthApprovalDialog(issue_advice);
} else {
@@ -116,21 +141,22 @@ void IdentityGetAuthTokenFunction::OnIssueAdviceSuccess(
}
}
-void IdentityGetAuthTokenFunction::OnLoginUIClosed(
- LoginUIService::LoginUI* ui) {
- StopObservingLoginService();
- if (!StartFlow(OAuth2MintTokenFlow::MODE_MINT_TOKEN_NO_FORCE)) {
- SendResponse(false);
- Release();
- }
+void IdentityGetAuthTokenFunction::SigninSuccess(const std::string& token) {
+ refresh_token_ = token;
+ StartFlow(OAuth2MintTokenFlow::MODE_MINT_TOKEN_NO_FORCE);
+}
+
+void IdentityGetAuthTokenFunction::SigninFailed() {
+ error_ = identity_constants::kUserNotSignedIn;
+ SendResponse(false);
+ Release();
}
void IdentityGetAuthTokenFunction::InstallUIProceed() {
DCHECK(install_ui_->record_oauth2_grant());
// The user has accepted the scopes, so we may now force (recording a grant
// and receiving a token).
- bool success = StartFlow(OAuth2MintTokenFlow::MODE_MINT_TOKEN_FORCE);
- DCHECK(success);
+ StartFlow(OAuth2MintTokenFlow::MODE_MINT_TOKEN_FORCE);
}
void IdentityGetAuthTokenFunction::InstallUIAbort(bool user_initiated) {
@@ -139,45 +165,15 @@ void IdentityGetAuthTokenFunction::InstallUIAbort(bool
user_initiated) {
Release(); // Balanced in RunImpl.
}
-bool IdentityGetAuthTokenFunction::StartFlow(OAuth2MintTokenFlow::Mode mode) {
- if (!HasLoginToken()) {
- error_ = identity_constants::kUserNotSignedIn;
- return false;
- }
-
- flow_.reset(CreateMintTokenFlow(mode));
- flow_->Start();
- return true;
-}
-
-bool IdentityGetAuthTokenFunction::StartLogin() {
- if (!interactive_) {
- error_ = identity_constants::kUserNotSignedIn;
- return false;
- }
-
- ShowLoginPopup();
- return true;
-}
-
-void IdentityGetAuthTokenFunction::StartObservingLoginService() {
- LoginUIService* login_ui_service =
- LoginUIServiceFactory::GetForProfile(profile());
- login_ui_service->AddObserver(this);
-}
-
-void IdentityGetAuthTokenFunction::StopObservingLoginService() {
- LoginUIService* login_ui_service =
- LoginUIServiceFactory::GetForProfile(profile());
- login_ui_service->RemoveObserver(this);
+void IdentityGetAuthTokenFunction::StartFlow(OAuth2MintTokenFlow::Mode mode) {
+ signin_flow_.reset(NULL);
+ mint_token_flow_.reset(CreateMintTokenFlow(mode));
+ mint_token_flow_->Start();
}
void IdentityGetAuthTokenFunction::ShowLoginPopup() {
- StartObservingLoginService();
-
- LoginUIService* login_ui_service =
- LoginUIServiceFactory::GetForProfile(profile());
- login_ui_service->ShowLoginPopup();
+ signin_flow_.reset(new IdentitySigninFlow(this, profile()));
+ signin_flow_->Start();
}
void IdentityGetAuthTokenFunction::ShowOAuthApprovalDialog(
@@ -188,12 +184,11 @@ void
IdentityGetAuthTokenFunction::ShowOAuthApprovalDialog(
OAuth2MintTokenFlow* IdentityGetAuthTokenFunction::CreateMintTokenFlow(
OAuth2MintTokenFlow::Mode mode) {
const OAuth2Info& oauth2_info = OAuth2Info::GetOAuth2Info(GetExtension());
- TokenService* token_service = TokenServiceFactory::GetForProfile(profile());
return new OAuth2MintTokenFlow(
profile()->GetRequestContext(),
this,
OAuth2MintTokenFlow::Parameters(
- token_service->GetOAuth2LoginRefreshToken(),
+ refresh_token_,
GetExtension()->id(),
oauth2_info.client_id,
oauth2_info.scopes,
@@ -256,13 +251,39 @@ void
IdentityLaunchWebAuthFlowFunction::OnAuthFlowFailure() {
Release(); // Balanced in RunImpl.
}
-IdentityAPI::IdentityAPI(Profile* profile) {
+IdentityAPI::IdentityAPI(Profile* profile)
+ : profile_(profile),
+ signin_manager_(NULL),
+ error_(GoogleServiceAuthError::NONE) {
(new OAuth2ManifestHandler)->Register();
}
IdentityAPI::~IdentityAPI() {
}
+void IdentityAPI::Initialize() {
+ signin_manager_ = SigninManagerFa…
(message too large)
+estade, Evan, can you do an OWNERS review for chrome/browser/ui/webui/signin/login_ui_service.cc please?
lgtm https://codereview.chromium.org/12929014/diff/53001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/login_ui_service.cc (right): https://codereview.chromium.org/12929014/diff/53001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/login_ui_service.cc:46: Browser* browser = FindOrCreateTabbedBrowser(profile_, this isn't implemented on Android. If Android started trying to use this function, I think it would fail to link.
https://codereview.chromium.org/12929014/diff/53001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/signin/login_ui_service.cc (right): https://codereview.chromium.org/12929014/diff/53001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/signin/login_ui_service.cc:46: Browser* browser = FindOrCreateTabbedBrowser(profile_, On 2013/04/04 01:25:19, Evan Stade wrote: > this isn't implemented on Android. If Android started trying to use this > function, I think it would fail to link. Good to know. This function is mostly used by app/extension APIs. There's some upcoming work to improve sign-in for apps, so we should probably refactor this code into an app/extension specific location as part of that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/courage@chromium.org/12929014/59001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/courage@chromium.org/12929014/59001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/courage@chromium.org/12929014/59001
Message was sent while issue was closed.
Change committed as 192471 |
