[signin] Cleanup GoogleServiceAuthError and add error reason

Bug: 822728
Change-Id: Iaf1de0917be3533d03da0e2aac3b9d33fb03d477
Reviewed-on: https://chromium-review.googlesource.com/966068
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545057}

components/signin/core/browser/account_fetcher_service.h

@@ -23,6 +23,10 @@ class ChildAccountInfoFetcher;
 class OAuth2TokenService;
 class SigninClient;
 
+namespace base {
+class DictionaryValue;
+}
+
 namespace image_fetcher {
 struct RequestMetadata;
 class ImageDecoder;

google_apis/gaia/google_service_auth_error.cc

@@ -8,11 +8,8 @@
 #include <string>
 #include <utility>
 
-#include "base/json/json_reader.h"
 #include "base/logging.h"
-#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
-#include "base/values.h"
 #include "net/base/net_errors.h"
 
 GoogleServiceAuthError::Captcha::Captcha() : image_width(0), image_height(0) {
@@ -77,23 +74,17 @@ bool GoogleServiceAuthError::operator!=(
 }
 
 GoogleServiceAuthError::GoogleServiceAuthError()
-    : state_(NONE), network_error_(0) {}
+    : GoogleServiceAuthError(NONE) {}
 
 GoogleServiceAuthError::GoogleServiceAuthError(State s)
-    : state_(s),
-      network_error_(0) {
-  // If the caller has no idea, then we just set it to a generic failure.
-  if (s == CONNECTION_FAILED) {
-    network_error_ = net::ERR_FAILED;
-  }
-}
+    : GoogleServiceAuthError(s, std::string()) {}
 
-GoogleServiceAuthError::GoogleServiceAuthError(
-    State state,
+GoogleServiceAuthError::GoogleServiceAuthError(State state,
                                                const std::string& error_message)
-    : state_(state),
-      network_error_(0),
-      error_message_(error_message) {
+    : GoogleServiceAuthError(
+          state,
+          (state == CONNECTION_FAILED) ? net::ERR_FAILED : 0) {
+  error_message_ = error_message;
 }
 
 GoogleServiceAuthError::GoogleServiceAuthError(
@@ -105,6 +96,14 @@ GoogleServiceAuthError
   return GoogleServiceAuthError(CONNECTION_FAILED, error);
 }
 
+// static
+GoogleServiceAuthError GoogleServiceAuthError::FromInvalidGaiaCredentialsReason(
+    InvalidGaiaCredentialsReason reason) {
+  GoogleServiceAuthError error(INVALID_GAIA_CREDENTIALS);
+  error.invalid_gaia_credentials_reason_ = reason;
+  return error;
+}
+
 // static
 GoogleServiceAuthError GoogleServiceAuthError::FromClientLoginCaptchaChallenge(
     const std::string& captcha_token,
@@ -171,53 +170,10 @@ const std::string& GoogleServiceAuthError::error_message() const {
   return error_message_;
 }
 
-base::DictionaryValue* GoogleServiceAuthError::ToValue() const {
-  base::DictionaryValue* value = new base::DictionaryValue();
-  std::string state_str;
-  switch (state_) {
-#define STATE_CASE(x) case x: state_str = #x; break
-    STATE_CASE(NONE);
-    STATE_CASE(INVALID_GAIA_CREDENTIALS);
-    STATE_CASE(USER_NOT_SIGNED_UP);
-    STATE_CASE(CONNECTION_FAILED);
-    STATE_CASE(CAPTCHA_REQUIRED);
-    STATE_CASE(ACCOUNT_DELETED);
-    STATE_CASE(ACCOUNT_DISABLED);
-    STATE_CASE(SERVICE_UNAVAILABLE);
-    STATE_CASE(TWO_FACTOR);
-    STATE_CASE(REQUEST_CANCELED);
-    STATE_CASE(UNEXPECTED_SERVICE_RESPONSE);
-    STATE_CASE(SERVICE_ERROR);
-    STATE_CASE(WEB_LOGIN_REQUIRED);
-#undef STATE_CASE
-    default:
-      NOTREACHED();
-      break;
-  }
-  value->SetString("state", state_str);
-  if (!error_message_.empty()) {
-    value->SetString("errorMessage", error_message_);
-  }
-  if (state_ == CAPTCHA_REQUIRED) {
-    auto captcha_value = std::make_unique<base::DictionaryValue>();
-    captcha_value->SetString("token", captcha_.token);
-    captcha_value->SetString("audioUrl", captcha_.audio_url.spec());
-    captcha_value->SetString("imageUrl", captcha_.image_url.spec());
-    captcha_value->SetString("unlockUrl", captcha_.unlock_url.spec());
-    captcha_value->SetInteger("imageWidth", captcha_.image_width);
-    captcha_value->SetInteger("imageHeight", captcha_.image_height);
-    value->Set("captcha", std::move(captcha_value));
-  } else if (state_ == CONNECTION_FAILED) {
-    value->SetString("networkError", net::ErrorToString(network_error_));
-  } else if (state_ == TWO_FACTOR) {
-    auto two_factor_value = std::make_unique<base::DictionaryValue>();
-    two_factor_value->SetString("token", second_factor_.token);
-    two_factor_value->SetString("promptText", second_factor_.prompt_text);
-    two_factor_value->SetString("alternateText", second_factor_.alternate_text);
-    two_factor_value->SetInteger("fieldLength", second_factor_.field_length);
-    value->Set("two_factor", std::move(two_factor_value));
-  }
-  return value;
+GoogleServiceAuthError::InvalidGaiaCredentialsReason
+GoogleServiceAuthError::GetInvalidGaiaCredentialsReason() const {
+  DCHECK_EQ(INVALID_GAIA_CREDENTIALS, state());
+  return invalid_gaia_credentials_reason_;
 }
 
 std::string GoogleServiceAuthError::ToString() const {
@@ -225,7 +181,9 @@ std::string GoogleServiceAuthError::ToString() const {
     case NONE:
       return std::string();
     case INVALID_GAIA_CREDENTIALS:
-      return "Invalid credentials.";
+      return base::StringPrintf(
+          "Invalid credentials (%d).",
+          static_cast<int>(invalid_gaia_credentials_reason_));
     case USER_NOT_SIGNED_UP:
       return "Not authorized.";
     case CONNECTION_FAILED:
@@ -280,11 +238,10 @@ bool GoogleServiceAuthError::IsTransientError() const {
 
 GoogleServiceAuthError::GoogleServiceAuthError(State s, int error)
     : state_(s),
-      network_error_(error) {
-}
+      network_error_(error),
+      invalid_gaia_credentials_reason_(InvalidGaiaCredentialsReason::UNKNOWN) {}
 
-GoogleServiceAuthError::GoogleServiceAuthError(
-    State s,
+GoogleServiceAuthError::GoogleServiceAuthError(State s,
                                                const std::string& captcha_token,
                                                const GURL& captcha_audio_url,
                                                const GURL& captcha_image_url,
@@ -292,7 +249,11 @@ GoogleServiceAuthError::GoogleServiceAuthError(
                                                int image_width,
                                                int image_height)
     : state_(s),
-      captcha_(captcha_token, captcha_audio_url, captcha_image_url,
-               captcha_unlock_url, image_width, image_height),
-      network_error_(0) {
-}
+      captcha_(captcha_token,
+               captcha_audio_url,
+               captcha_image_url,
+               captcha_unlock_url,
+               image_width,
+               image_height),
+      network_error_((state_ == CONNECTION_FAILED) ? net::ERR_FAILED : net::OK),
+      invalid_gaia_credentials_reason_(InvalidGaiaCredentialsReason::UNKNOWN) {}

google_apis/gaia/google_service_auth_error.h

@@ -8,17 +8,6 @@
 // Accounts (e.g expired credentials).  It may contain additional data such as
 // captcha or OTP challenges.
 
-// A GoogleServiceAuthError without additional data is just a State, defined
-// below. A case could be made to have this relation implicit, to allow raising
-// error events concisely by doing OnAuthError(GoogleServiceAuthError::NONE),
-// for example. But the truth is this class is ever so slightly more than a
-// transparent wrapper around 'State' due to additional Captcha data
-// (e.g consider operator=), and this would violate the style guide. Thus,
-// you must explicitly use the constructor when all you have is a State.
-// The good news is the implementation nests the enum inside a class, so you
-// may forward declare and typedef GoogleServiceAuthError to something shorter
-// in the comfort of your own translation unit.
-
 #ifndef GOOGLE_APIS_GAIA_GOOGLE_SERVICE_AUTH_ERROR_H_
 #define GOOGLE_APIS_GAIA_GOOGLE_SERVICE_AUTH_ERROR_H_
 
@@ -34,10 +23,6 @@ class GoogleServiceAuthErrorDataView;
 }
 }
 
-namespace base {
-class DictionaryValue;
-}
-
 class GoogleServiceAuthError {
  public:
   //
@@ -101,6 +86,23 @@ class GoogleServiceAuthError {
     NUM_STATES = 14,
   };
 
+  // Error reason for invalid credentials. Only used when the error is
+  // INVALID_GAIA_CREDENTIALS.
+  // Used by UMA histograms: do not remove or reorder values, add new values at
+  // the end.
+  enum class InvalidGaiaCredentialsReason {
+    // The error was not specified.
+    UNKNOWN = 0,
+    // Credentials were rejectedby the Gaia server.
+    CREDENTIALS_REJECTED_BY_SERVER,
+    // Credentials were invalidated locally by Chrome.
+    CREDENTIALS_REJECTED_BY_CLIENT,
+    // Credentials are missing (e.g. could not be loaded from disk).
+    CREDENTIALS_MISSING,
+
+    NUM_REASONS
+  };
+
   // Additional data for CAPTCHA_REQUIRED errors.
   struct Captcha {
     Captcha();
@@ -164,6 +166,9 @@ class GoogleServiceAuthError {
   // It will be created with CONNECTION_FAILED set.
   static GoogleServiceAuthError FromConnectionError(int error);
 
+  static GoogleServiceAuthError FromInvalidGaiaCredentialsReason(
+      InvalidGaiaCredentialsReason reason);
+
   // Construct a CAPTCHA_REQUIRED error with CAPTCHA challenge data from the
   // the ClientLogin endpoint.
   // TODO(rogerta): once ClientLogin is no longer used, may be able to get
@@ -198,9 +203,8 @@ class GoogleServiceAuthError {
   const std::string& token() const;
   const std::string& error_message() const;
 
-  // Returns info about this object in a dictionary.  Caller takes
-  // ownership of returned dictionary.
-  base::DictionaryValue* ToValue() const;
+  // Should only be used when the error state is INVALID_GAIA_CREDENTIALS.
+  InvalidGaiaCredentialsReason GetInvalidGaiaCredentialsReason() const;
 
   // Returns a message describing the error.
   std::string ToString() const;
@@ -234,6 +238,7 @@ class GoogleServiceAuthError {
   SecondFactor second_factor_;
   int network_error_;
   std::string error_message_;
+  InvalidGaiaCredentialsReason invalid_gaia_credentials_reason_;
 };
 
 #endif  // GOOGLE_APIS_GAIA_GOOGLE_SERVICE_AUTH_ERROR_H_

google_apis/gaia/google_service_auth_error_unittest.cc

@@ -7,49 +7,73 @@
 #include <memory>
 #include <string>
 
-#include "base/test/values_test_util.h"
-#include "base/values.h"
 #include "net/base/net_errors.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
-using base::ExpectDictStringValue;
+TEST(GoogleServiceAuthErrorTest, State) {
+  for (GoogleServiceAuthError::State i = GoogleServiceAuthError::NONE;
+       i < GoogleServiceAuthError::NUM_STATES;
+       i = GoogleServiceAuthError::State(i + 1)) {
+    GoogleServiceAuthError error(i);
+    EXPECT_EQ(i, error.state());
+    EXPECT_TRUE(error.error_message().empty());
 
-class GoogleServiceAuthErrorTest : public testing::Test {};
+    if (i == GoogleServiceAuthError::CONNECTION_FAILED)
+      EXPECT_EQ(net::ERR_FAILED, error.network_error());
+    else
+      EXPECT_EQ(net::OK, error.network_error());
 
-void TestSimpleState(GoogleServiceAuthError::State state) {
-  GoogleServiceAuthError error(state);
-  std::unique_ptr<base::DictionaryValue> value(error.ToValue());
-  EXPECT_EQ(1u, value->size());
-  std::string state_str;
-  EXPECT_TRUE(value->GetString("state", &state_str));
-  EXPECT_FALSE(state_str.empty());
-  EXPECT_NE("CONNECTION_FAILED", state_str);
-  EXPECT_NE("CAPTCHA_REQUIRED", state_str);
-}
+    if (i == GoogleServiceAuthError::NONE) {
+      EXPECT_FALSE(error.IsTransientError());
+      EXPECT_FALSE(error.IsPersistentError());
+    } else if ((i == GoogleServiceAuthError::CONNECTION_FAILED) ||
+               (i == GoogleServiceAuthError::SERVICE_UNAVAILABLE) ||
+               (i == GoogleServiceAuthError::REQUEST_CANCELED)) {
+      EXPECT_TRUE(error.IsTransientError());
+      EXPECT_FALSE(error.IsPersistentError());
+    } else {
+      EXPECT_FALSE(error.IsTransientError());
+      EXPECT_TRUE(error.IsPersistentError());
+    }
 
-TEST_F(GoogleServiceAuthErrorTest, SimpleToValue) {
-  for (int i = GoogleServiceAuthError::NONE;
-       i <= GoogleServiceAuthError::USER_NOT_SIGNED_UP; ++i) {
-    TestSimpleState(static_cast<GoogleServiceAuthError::State>(i));
+    if (i == GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS) {
+      EXPECT_EQ(GoogleServiceAuthError::InvalidGaiaCredentialsReason::UNKNOWN,
+                error.GetInvalidGaiaCredentialsReason());
     }
+  }
+}
+
+TEST(GoogleServiceAuthErrorTest, FromConnectionError) {
+  GoogleServiceAuthError error =
+      GoogleServiceAuthError::FromConnectionError(net::ERR_TIMED_OUT);
+  EXPECT_EQ(GoogleServiceAuthError::CONNECTION_FAILED, error.state());
+  EXPECT_EQ(net::ERR_TIMED_OUT, error.network_error());
+}
+
+TEST(GoogleServiceAuthErrorTest, FromServiceError) {
+  GoogleServiceAuthError error =
+      GoogleServiceAuthError::FromServiceError("Foo");
+  EXPECT_EQ(GoogleServiceAuthError::SERVICE_ERROR, error.state());
+  EXPECT_EQ("Foo", error.error_message());
 }
 
-TEST_F(GoogleServiceAuthErrorTest, None) {
-  GoogleServiceAuthError error(GoogleServiceAuthError::AuthErrorNone());
-  std::unique_ptr<base::DictionaryValue> value(error.ToValue());
-  EXPECT_EQ(1u, value->size());
-  ExpectDictStringValue("NONE", *value, "state");
+TEST(GoogleServiceAuthErrorTest, FromInvalidGaiaCredentialsReason) {
+  GoogleServiceAuthError error =
+      GoogleServiceAuthError::FromInvalidGaiaCredentialsReason(
+          GoogleServiceAuthError::InvalidGaiaCredentialsReason::
+              CREDENTIALS_REJECTED_BY_SERVER);
+  EXPECT_EQ(GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS, error.state());
+  EXPECT_EQ(GoogleServiceAuthError::InvalidGaiaCredentialsReason::
+                CREDENTIALS_REJECTED_BY_SERVER,
+            error.GetInvalidGaiaCredentialsReason());
+  EXPECT_EQ("Invalid credentials (1).", error.ToString());
 }
 
-TEST_F(GoogleServiceAuthErrorTest, ConnectionFailed) {
-  GoogleServiceAuthError error(
-      GoogleServiceAuthError::FromConnectionError(net::OK));
-  std::unique_ptr<base::DictionaryValue> value(error.ToValue());
-  EXPECT_EQ(2u, value->size());
-  ExpectDictStringValue("CONNECTION_FAILED", *value, "state");
-  ExpectDictStringValue("net::OK", *value, "networkError");
+TEST(GoogleServiceAuthErrorTest, AuthErrorNone) {
+  EXPECT_EQ(GoogleServiceAuthError(GoogleServiceAuthError::NONE),
+            GoogleServiceAuthError::AuthErrorNone());
 }
 
 }  // namespace