[Signin] Add Scope error to GoogleServiceAuthError

This CL adds a new error to `GoogleServiceAuthError`. This error will be used in a future CL to handle authentication errors that are persistent to a scope and can't be solved by user action e.g. Invalid scope, Restricted scope. This error is not persisted as it does not imply the account is in an error state that requires reauth but it indicates an error that is relevant only to the scope set in the access token request. The consumer of the access token should not retry if they receive a scope limited error. Bug: 1336627 Change-Id: I69d3248f24657fc9e5316acdde1f1bab0bf0bc14 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3769752 Reviewed-by: Joe Mason <joenotcharles@google.com> Reviewed-by: David Roger <droger@chromium.org> Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com> Reviewed-by: Anastasiia N <anastasiian@chromium.org> Commit-Queue: Monica Basta <msalama@chromium.org> Cr-Commit-Position: refs/heads/main@{#1026661}
Denger-Network · Jul 21, 2022 · b70b093 · b70b093
1 parent 9812239
commit b70b093
Show file tree

Hide file tree

Showing 14 changed files with 98 additions and 30 deletions.
diff --git a/chrome/browser/ash/login/enrollment/enterprise_enrollment_helper_impl.cc b/chrome/browser/ash/login/enrollment/enterprise_enrollment_helper_impl.cc
@@ -337,6 +337,7 @@ void EnterpriseEnrollmentHelperImpl::ReportAuthStatus(
     case GoogleServiceAuthError::REQUEST_CANCELED:
     case GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE:
     case GoogleServiceAuthError::SERVICE_ERROR:
+    case GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR:
       UMA(policy::kMetricEnrollmentLoginFailed);
       LOG(ERROR) << "Auth error " << error.state();
       break;

diff --git a/chrome/browser/ash/login/signin/signin_error_notifier_unittest.cc b/chrome/browser/ash/login/signin/signin_error_notifier_unittest.cc
@@ -213,20 +213,16 @@ TEST_F(SigninErrorNotifierTest, ErrorTransitionForPrimaryAccount) {
 
 // Verify that SigninErrorNotifier ignores certain errors.
 TEST_F(SigninErrorNotifierTest, AuthStatusEnumerateAllErrors) {
-  typedef struct {
-    GoogleServiceAuthError::State error_state;
-    bool is_error;
-  } ErrorTableEntry;
-
-  ErrorTableEntry table[] = {
-      {GoogleServiceAuthError::NONE, false},
-      {GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS, true},
-      {GoogleServiceAuthError::USER_NOT_SIGNED_UP, true},
-      {GoogleServiceAuthError::CONNECTION_FAILED, false},
-      {GoogleServiceAuthError::SERVICE_UNAVAILABLE, false},
-      {GoogleServiceAuthError::REQUEST_CANCELED, false},
-      {GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE, true},
-      {GoogleServiceAuthError::SERVICE_ERROR, true},
+  GoogleServiceAuthError::State table[] = {
+      GoogleServiceAuthError::NONE,
+      GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS,
+      GoogleServiceAuthError::USER_NOT_SIGNED_UP,
+      GoogleServiceAuthError::CONNECTION_FAILED,
+      GoogleServiceAuthError::SERVICE_UNAVAILABLE,
+      GoogleServiceAuthError::REQUEST_CANCELED,
+      GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE,
+      GoogleServiceAuthError::SERVICE_ERROR,
+      GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR,
   };
   static_assert(
       std::size(table) == GoogleServiceAuthError::NUM_STATES -
@@ -238,15 +234,22 @@ TEST_F(SigninErrorNotifierTest, AuthStatusEnumerateAllErrors) {
           .account_id;
 
   for (size_t i = 0; i < std::size(table); ++i) {
-    SetAuthError(account_id, GoogleServiceAuthError(table[i].error_state));
+    GoogleServiceAuthError error(table[i]);
+    SetAuthError(account_id, error);
     absl::optional<message_center::Notification> notification =
         display_service_->GetNotification(kPrimaryAccountErrorNotificationId);
-    ASSERT_EQ(table[i].is_error, !!notification) << "Failed case #" << i;
-    if (table[i].is_error) {
-      EXPECT_FALSE(notification->title().empty());
-      EXPECT_FALSE(notification->message().empty());
-      EXPECT_EQ((size_t)1, notification->buttons().size());
-    }
+
+    // Only non scope persistent errors are reported.
+    bool expect_notification =
+        error.IsPersistentError() && !error.IsScopePersistentError();
+    ASSERT_EQ(expect_notification, !!notification) << "Failed case #" << i;
+    if (!expect_notification)
+      continue;
+
+    ASSERT_TRUE(notification.has_value()) << "Failed case #" << i;
+    EXPECT_FALSE(notification->title().empty());
+    EXPECT_FALSE(notification->message().empty());
+    EXPECT_EQ((size_t)1, notification->buttons().size());
     SetAuthError(account_id, GoogleServiceAuthError::AuthErrorNone());
   }
 }

diff --git a/chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc b/chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
@@ -443,6 +443,7 @@ void EnrollmentScreenHandler::ShowAuthError(
     case GoogleServiceAuthError::REQUEST_CANCELED:
     case GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE:
     case GoogleServiceAuthError::SERVICE_ERROR:
+    case GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR:
       ShowError(IDS_ENTERPRISE_ENROLLMENT_AUTH_FATAL_ERROR, false);
       return;
     case GoogleServiceAuthError::USER_NOT_SIGNED_UP:

diff --git a/chromeos/crosapi/mojom/account_manager.mojom b/chromeos/crosapi/mojom/account_manager.mojom
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Next MinVersion: 10
+// Next MinVersion: 11
 
 module crosapi.mojom;
 
@@ -42,7 +42,7 @@ struct Account {
 
 // See google_apis/gaia/google_service_auth_error.h
 // Currently sent from ash to lacros.
-// Next value: 14.
+// Next value: 15.
 [Stable]
 struct GoogleServiceAuthError {
   [Stable, Extensible]
@@ -61,6 +61,7 @@ struct GoogleServiceAuthError {
     kUnexpectedServiceResponse = 11,
     kServiceError = 12,
     // DEPRECATED kWebLoginRequired = 13,
+    [MinVersion=10] kScopeLimitedUnrecoverableError = 14,
   };
 
   // Next value: 4.

diff --git a/components/account_manager_core/account_manager_util.cc b/components/account_manager_core/account_manager_util.cc
@@ -87,6 +87,8 @@ crosapi::mojom::GoogleServiceAuthError::State ToMojoGoogleServiceAuthErrorState(
       return cm::GoogleServiceAuthError::State::kUnexpectedServiceResponse;
     case GoogleServiceAuthError::State::SERVICE_ERROR:
       return cm::GoogleServiceAuthError::State::kServiceError;
+    case GoogleServiceAuthError::State::SCOPE_LIMITED_UNRECOVERABLE_ERROR:
+      return cm::GoogleServiceAuthError::State::kScopeLimitedUnrecoverableError;
     case GoogleServiceAuthError::State::NUM_STATES:
       NOTREACHED();
       return cm::GoogleServiceAuthError::State::kNone;
@@ -239,6 +241,9 @@ absl::optional<GoogleServiceAuthError> FromMojoGoogleServiceAuthError(
     case cm::GoogleServiceAuthError::State::kRequestCanceled:
       return GoogleServiceAuthError(
           GoogleServiceAuthError::State::REQUEST_CANCELED);
+    case cm::GoogleServiceAuthError::State::kScopeLimitedUnrecoverableError:
+      return GoogleServiceAuthError(
+          GoogleServiceAuthError::State::SCOPE_LIMITED_UNRECOVERABLE_ERROR);
     default:
       LOG(WARNING) << "Unknown crosapi::mojom::GoogleServiceAuthError::State: "
                    << mojo_error->state;

diff --git a/components/signin/core/browser/signin_error_controller_unittest.cc b/components/signin/core/browser/signin_error_controller_unittest.cc
@@ -157,7 +157,9 @@ TEST(SigninErrorControllerTest, AuthStatusEnumerateAllErrors) {
       GoogleServiceAuthError::SERVICE_UNAVAILABLE,
       GoogleServiceAuthError::REQUEST_CANCELED,
       GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE,
-      GoogleServiceAuthError::SERVICE_ERROR};
+      GoogleServiceAuthError::SERVICE_ERROR,
+      GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR,
+  };
   static_assert(
       std::size(table) == GoogleServiceAuthError::NUM_STATES -
                               GoogleServiceAuthError::kDeprecatedStateCount,
@@ -166,8 +168,8 @@ TEST(SigninErrorControllerTest, AuthStatusEnumerateAllErrors) {
   for (GoogleServiceAuthError::State state : table) {
     GoogleServiceAuthError error(state);
 
-    if (error.IsTransientError())
-      continue;  // Only persistent errors or non-errors are reported.
+    if (error.IsTransientError() || error.IsScopePersistentError())
+      continue;  // Only non scope persistent errors or non-errors are reported.
 
     identity_test_env.UpdatePersistentErrorOfRefreshTokenForAccount(
         test_account_id, error);

diff --git a/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.cc b/components/signin/internal/identity_manager/profile_oauth2_token_service_delegate.cc
@@ -195,6 +195,11 @@ void ProfileOAuth2TokenServiceDelegate::UpdateAuthError(
   if (error.IsTransientError())
     return;
 
+  // Scope errors are only relevant to the scope set of the request and it does
+  // not imply that the account is in an error state.
+  if (error.IsScopePersistentError())
+    return;
+
   auto it = errors_.find(account_id);
   if (error.state() == GoogleServiceAuthError::NONE) {
     if (it == errors_.end())

diff --git a/...onents/signin/internal/identity_manager/profile_oauth2_token_service_delegate_unittest.cc b/...onents/signin/internal/identity_manager/profile_oauth2_token_service_delegate_unittest.cc
@@ -73,6 +73,7 @@ const GoogleServiceAuthError::State table[] = {
     GoogleServiceAuthError::REQUEST_CANCELED,
     GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE,
     GoogleServiceAuthError::SERVICE_ERROR,
+    GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR,
 };
 
 TEST_F(ProfileOAuth2TokenServiceDelegateTest, UpdateAuthError_PersistenErrors) {
@@ -86,7 +87,7 @@ TEST_F(ProfileOAuth2TokenServiceDelegateTest, UpdateAuthError_PersistenErrors) {
 
   for (GoogleServiceAuthError::State state : table) {
     GoogleServiceAuthError error(state);
-    if (!error.IsPersistentError())
+    if (!error.IsPersistentError() || error.IsScopePersistentError())
       continue;
 
     EXPECT_CALL(observer_, OnAuthErrorChanged(account_id, error)).Times(1);
@@ -130,6 +131,26 @@ TEST_F(ProfileOAuth2TokenServiceDelegateTest, UpdateAuthError_TransientErrors) {
   }
 }
 
+TEST_F(ProfileOAuth2TokenServiceDelegateTest,
+       UpdateAuthError_ScopePersistenErrors) {
+  const CoreAccountId account_id("account_id");
+  delegate_.UpdateCredentials(account_id, "refresh_token");
+  GoogleServiceAuthError error(
+      GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR);
+
+  // Scope persistent errors are not persisted or notified as it does not imply
+  // that the account is in an error state but the error is only relevant to
+  // the scope set requested in the access token request.
+  EXPECT_CALL(observer_, OnAuthErrorChanged(account_id, error)).Times(0);
+
+  delegate_.UpdateAuthError(account_id, error);
+  EXPECT_EQ(delegate_.GetAuthError(account_id),
+            GoogleServiceAuthError::AuthErrorNone());
+  // Backoff only used for transient errors.
+  EXPECT_EQ(delegate_.BackoffEntry()->failure_count(), 0);
+  testing::Mock::VerifyAndClearExpectations(&observer_);
+}
+
 TEST_F(ProfileOAuth2TokenServiceDelegateTest,
        UpdateAuthError_RefreshTokenNotAvailable) {
   const CoreAccountId account_id("account_id");

diff --git a/components/sync/driver/sync_auth_manager.cc b/components/sync/driver/sync_auth_manager.cc
@@ -544,6 +544,7 @@ void SyncAuthManager::AccessTokenFetched(
       break;
     case GoogleServiceAuthError::USER_NOT_SIGNED_UP:
     case GoogleServiceAuthError::UNEXPECTED_SERVICE_RESPONSE:
+    case GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR:
       DLOG(ERROR) << "Unexpected persistent error: " << error.ToString();
       SetLastAuthError(error);
       break;

diff --git a/google_apis/gaia/google_service_auth_error.cc b/google_apis/gaia/google_service_auth_error.cc
@@ -109,6 +109,7 @@ bool GoogleServiceAuthError::IsValid(State state) {
     case REQUEST_CANCELED:
     case UNEXPECTED_SERVICE_RESPONSE:
     case SERVICE_ERROR:
+    case SCOPE_LIMITED_UNRECOVERABLE_ERROR:
       return true;
     case NUM_STATES:
       return false;
@@ -157,6 +158,9 @@ std::string GoogleServiceAuthError::ToString() const {
     case SERVICE_ERROR:
       return base::StringPrintf("Service responded with error: '%s'",
                                 error_message_.c_str());
+    case SCOPE_LIMITED_UNRECOVERABLE_ERROR:
+      return base::StringPrintf("Service responded with error: '%s'",
+                                error_message_.c_str());
     case NUM_STATES:
       NOTREACHED();
       return std::string();
@@ -168,6 +172,10 @@ bool GoogleServiceAuthError::IsPersistentError() const {
   return !IsTransientError();
 }
 
+bool GoogleServiceAuthError::IsScopePersistentError() const {
+  return state_ == GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR;
+}
+
 bool GoogleServiceAuthError::IsTransientError() const {
   switch (state_) {
   // These are failures that are likely to succeed if tried again.

diff --git a/google_apis/gaia/google_service_auth_error.h b/google_apis/gaia/google_service_auth_error.h
@@ -78,8 +78,12 @@ class GoogleServiceAuthError {
     // The password is valid but web login is required to get a token.
     // WEB_LOGIN_REQUIRED = 13,
 
+    // Indicates the service responded with an error that is bound to the scopes
+    // that are in the request.
+    SCOPE_LIMITED_UNRECOVERABLE_ERROR = 14,
+
     // The number of known error states.
-    NUM_STATES = 14,
+    NUM_STATES = 15,
   };
 
   static constexpr size_t kDeprecatedStateCount = 6;
@@ -149,9 +153,22 @@ class GoogleServiceAuthError {
   // Returns a message describing the error.
   std::string ToString() const;
 
-  // Check if this is error may go away simply by trying again.  Except for the
-  // NONE case, these are mutually exclusive.
+  // In contrast to transient errors, errors in this category imply that
+  // authentication shouldn't simply be retried. The error can be:
+  // - User recoverable: persistent error that can be fixed by user action
+  // (e.g. Sign in).
+  // - Scope error: persistent error that is bound to the scopes in the access
+  // token request and can't be fixed by user action.
   bool IsPersistentError() const;
+
+  // Persistent error that is bound to the scopes in the access
+  // token request and can't be fixed by user action. Authentication should not
+  // be retried.
+  bool IsScopePersistentError() const;
+
+  // Check if this is error may go away simply by trying again.
+  // Except for the NONE case, errors are either transient or persistent but not
+  // both.
   bool IsTransientError() const;
 
  private:

diff --git a/ios/chrome/browser/sync/sync_setup_service.cc b/ios/chrome/browser/sync/sync_setup_service.cc
@@ -149,6 +149,7 @@ SyncSetupService::SyncServiceState SyncSetupService::GetSyncServiceState() {
       break;
     // The following errors are unexpected on iOS.
     case GoogleServiceAuthError::SERVICE_ERROR:
+    case GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR:
     // Conventional value for counting the states, never used.
     case GoogleServiceAuthError::NUM_STATES:
       NOTREACHED() << "Unexpected Auth error ("

diff --git a/ios/web_view/internal/sync/cwv_sync_controller.mm b/ios/web_view/internal/sync/cwv_sync_controller.mm
@@ -54,6 +54,7 @@ CWVSyncError CWVConvertGoogleServiceAuthErrorStateToCWVSyncError(
       return CWVSyncErrorUnexpectedServiceResponse;
     // The following errors are unexpected on iOS.
     case GoogleServiceAuthError::SERVICE_ERROR:
+    case GoogleServiceAuthError::SCOPE_LIMITED_UNRECOVERABLE_ERROR:
     case GoogleServiceAuthError::NUM_STATES:
       NOTREACHED();
       return CWVSyncErrorNone;

diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
@@ -46266,6 +46266,7 @@ Called by update_permissions_policy_enum.py.-->
   <int value="11" label="UNEXPECTED_SERVICE_RESPONSE"/>
   <int value="12" label="SERVICE_ERROR"/>
   <int value="13" label="WEB_LOGIN_REQUIRED (deprecated)"/>
+  <int value="14" label="SCOPE_LIMITED_UNRECOVERABLE_ERROR"/>
 </enum>
 
 <enum name="GoogleUpdateAfterItemClickedActions">