New issue
Advanced search Search tips

Issue 836212 link

Starred by 4 users

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Refactor authentication error handling

Project Member Reported by droger@chromium.org, Apr 24 2018

Issue description

Currently the SigninErrorController is centralizing the authentication errors.
However, this is not practical, because SigninErrorController is not really multi-account.

Proposed change:
* make the token service the default API for managing authentication errors.
* change SigninErrorController to observe the token service
* switch clients of SigninErrorController to use TokenService instead when relevant

 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/944adf7fb9b949344c605e107e27bdc8c1643d08

commit 944adf7fb9b949344c605e107e27bdc8c1643d08
Author: David Roger <droger@chromium.org>
Date: Wed Apr 25 11:05:05 2018

[signin] Remove usage of AuthErrorProvider from tests

Going forward the default API for interacting with authentication error
should be the token service rather than the SigninErrorController.
This CL updates tests to use the token service instead of
AuthErrorProvider to set authentication errors.

TBR=ellyjones

Bug: 836212
Change-Id: I694068a1d037b1f2314d389328b9ccbea5f2bcf3
Reviewed-on: https://chromium-review.googlesource.com/1016642
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553509}
[modify] https://crrev.com/944adf7fb9b949344c605e107e27bdc8c1643d08/chrome/browser/app_controller_mac_unittest.mm
[modify] https://crrev.com/944adf7fb9b949344c605e107e27bdc8c1643d08/chrome/browser/signin/signin_global_error.h
[modify] https://crrev.com/944adf7fb9b949344c605e107e27bdc8c1643d08/chrome/browser/signin/signin_global_error_unittest.cc
[modify] https://crrev.com/944adf7fb9b949344c605e107e27bdc8c1643d08/chrome/browser/signin/signin_tracker_unittest.cc
[modify] https://crrev.com/944adf7fb9b949344c605e107e27bdc8c1643d08/chrome/browser/sync/sync_ui_util_unittest.cc
[modify] https://crrev.com/944adf7fb9b949344c605e107e27bdc8c1643d08/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc
[modify] https://crrev.com/944adf7fb9b949344c605e107e27bdc8c1643d08/chrome/browser/ui/webui/signin/signin_create_profile_handler_unittest.cc
[modify] https://crrev.com/944adf7fb9b949344c605e107e27bdc8c1643d08/chrome/browser/ui/webui/signin/signin_supervised_user_import_handler_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, May 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82e2a47ab797a6435d3d29714559b451210436f4

commit 82e2a47ab797a6435d3d29714559b451210436f4
Author: David Roger <droger@chromium.org>
Date: Fri May 25 11:32:44 2018

[signin] Remove usage of AuthErrorProvider from tests (2)

Going forward the default API for interacting with authentication error
should be the token service rather than the SigninErrorController.
This CL updates tests to use the token service instead of
AuthErrorProvider to set authentication errors.

Bug: 836212
Change-Id: I9b5629bad7b17b6cab70978cc23bbc828bf62d61
Reviewed-on: https://chromium-review.googlesource.com/1016904
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561830}
[modify] https://crrev.com/82e2a47ab797a6435d3d29714559b451210436f4/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc
[modify] https://crrev.com/82e2a47ab797a6435d3d29714559b451210436f4/chrome/browser/signin/signin_error_notifier_ash_unittest.cc
[modify] https://crrev.com/82e2a47ab797a6435d3d29714559b451210436f4/chrome/browser/ui/webui/settings/people_handler_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, May 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1569d2b963d9e13bfdf72e4aeee75e962bc40c04

commit 1569d2b963d9e13bfdf72e4aeee75e962bc40c04
Author: Olga Sharonova <olka@chromium.org>
Date: Fri May 25 14:33:35 2018

Revert "[signin] Remove usage of AuthErrorProvider from tests (2)"

This reverts commit 82e2a47ab797a6435d3d29714559b451210436f4.

Reason for revert: Suspected of causing OAuth2Test.SetInvalidTokenStatus failure in https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/5838

Original change's description:
> [signin] Remove usage of AuthErrorProvider from tests (2)
> 
> Going forward the default API for interacting with authentication error
> should be the token service rather than the SigninErrorController.
> This CL updates tests to use the token service instead of
> AuthErrorProvider to set authentication errors.
> 
> Bug: 836212
> Change-Id: I9b5629bad7b17b6cab70978cc23bbc828bf62d61
> Reviewed-on: https://chromium-review.googlesource.com/1016904
> Commit-Queue: David Roger <droger@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561830}

TBR=stevenjb@chromium.org,droger@chromium.org,msarda@chromium.org

Change-Id: I5fe286175add827a626413ea1bee442fbfd4f258
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 836212
Reviewed-on: https://chromium-review.googlesource.com/1073457
Reviewed-by: Olga Sharonova <olka@chromium.org>
Commit-Queue: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561868}
[modify] https://crrev.com/1569d2b963d9e13bfdf72e4aeee75e962bc40c04/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc
[modify] https://crrev.com/1569d2b963d9e13bfdf72e4aeee75e962bc40c04/chrome/browser/signin/signin_error_notifier_ash_unittest.cc
[modify] https://crrev.com/1569d2b963d9e13bfdf72e4aeee75e962bc40c04/chrome/browser/ui/webui/settings/people_handler_unittest.cc

Comment 4 by tapted@chromium.org, May 28 2018

 Issue 846776  has been merged into this issue.
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2d35fa2e6d7bf7d69d8631c29787e8d84d47549f

commit 2d35fa2e6d7bf7d69d8631c29787e8d84d47549f
Author: David Roger <droger@chromium.org>
Date: Wed Nov 21 12:35:38 2018

[signin] Remove usage of AuthErrorProvider from tests (2)

Going forward the default API for interacting with authentication error
should be the token service rather than the SigninErrorController.
This CL updates tests to use the token service instead of
AuthErrorProvider to set authentication errors.

Note: this is a reland of
https://chromium-review.googlesource.com/c/chromium/src/+/1016904
Relanding notes:
The CL was reverted because OAuth2Test.SetInvalidTokenStatus was flaky.
This was happening because the error was set through UpdateAuthError,
which does not cancel requests that are in flight, and thus requests
could randomly succeed later on and remove the error.
To fix this, the test now uses UpdateCredentials() which cancels all
in-flight requests, and prevent the error from disappearing later.

TBR=msarda, stevenjb

Bug: 836212
Change-Id: I2d9cde22e6fc1933de0b47f39aebdd495ab2dd95
Reviewed-on: https://chromium-review.googlesource.com/c/1075088
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610012}
[modify] https://crrev.com/2d35fa2e6d7bf7d69d8631c29787e8d84d47549f/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc
[modify] https://crrev.com/2d35fa2e6d7bf7d69d8631c29787e8d84d47549f/chrome/browser/signin/signin_error_notifier_ash_unittest.cc
[modify] https://crrev.com/2d35fa2e6d7bf7d69d8631c29787e8d84d47549f/chrome/browser/ui/webui/settings/people_handler_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d70ca123951d4f512f0516b72d043581ec96e7cb

commit d70ca123951d4f512f0516b72d043581ec96e7cb
Author: David Roger <droger@chromium.org>
Date: Tue Nov 27 13:50:53 2018

[signin] Remove dependency of IOSWebViewSigninClient on SigninErrorController

SigninClient implementations should not depend on SigninErrorController.
The dependency should be the other way around.
This CL fixes the WebView implementation of SigninClient.

NOTRY=true

Change-Id: I0592028bd988036bc01a2a48d375337f077bfbd7
Bug: 836212
Reviewed-on: https://chromium-review.googlesource.com/c/1348035
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611090}
[modify] https://crrev.com/d70ca123951d4f512f0516b72d043581ec96e7cb/ios/web_view/internal/cwv_web_view_configuration.mm
[modify] https://crrev.com/d70ca123951d4f512f0516b72d043581ec96e7cb/ios/web_view/internal/signin/ios_web_view_signin_client.h
[modify] https://crrev.com/d70ca123951d4f512f0516b72d043581ec96e7cb/ios/web_view/internal/signin/ios_web_view_signin_client.mm
[modify] https://crrev.com/d70ca123951d4f512f0516b72d043581ec96e7cb/ios/web_view/internal/signin/web_view_signin_client_factory.mm
[modify] https://crrev.com/d70ca123951d4f512f0516b72d043581ec96e7cb/ios/web_view/internal/sync/cwv_sync_controller.mm
[modify] https://crrev.com/d70ca123951d4f512f0516b72d043581ec96e7cb/ios/web_view/internal/sync/cwv_sync_controller_internal.h
[modify] https://crrev.com/d70ca123951d4f512f0516b72d043581ec96e7cb/ios/web_view/internal/sync/cwv_sync_controller_unittest.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9

commit cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9
Author: David Roger <droger@chromium.org>
Date: Thu Nov 29 13:15:56 2018

[signin] Cleanup the SigninClient implementations

- SigninClient duplicates API that is already in
  SigninManagerBase::Observer. This CL removes these functions from the
  SigninClient and uses SigninManagerBase::Observer instead.
- Several SigninClient implementations depend on SigninErrorController,
  but this dependency is conceptually wrong. This CL moves the
  corresponding code out of SigninClient implementations.

Change-Id: I4a09e40a204713ba3cb8ea76e65c46228fc3923d
Bug: 836212
Reviewed-on: https://chromium-review.googlesource.com/c/1346950
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612170}
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/chrome/browser/BUILD.gn
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/chrome/browser/signin/chrome_signin_client.cc
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/chrome/browser/signin/chrome_signin_client.h
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/chrome/browser/signin/chrome_signin_client_factory.cc
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/chrome/browser/signin/chrome_signin_client_unittest.cc
[add] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/chrome/browser/signin/signin_profile_attributes_updater.cc
[add] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/chrome/browser/signin/signin_profile_attributes_updater.h
[add] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/chrome/browser/signin/signin_profile_attributes_updater_factory.cc
[add] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/chrome/browser/signin/signin_profile_attributes_updater_factory.h
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/components/signin/core/browser/signin_client.h
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/components/signin/core/browser/signin_manager.cc
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/components/signin/core/browser/test_signin_client.cc
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/components/signin/core/browser/test_signin_client.h
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/browsing_data/cache_counter_unittest.cc
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/signin/BUILD.gn
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/signin/authentication_service_unittest.mm
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/signin/ios_chrome_signin_client.h
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/signin/ios_chrome_signin_client.mm
[add] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/signin/signin_browser_state_info_updater.h
[add] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/signin/signin_browser_state_info_updater.mm
[add] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/signin/signin_browser_state_info_updater_factory.h
[add] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/signin/signin_browser_state_info_updater_factory.mm
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/signin/signin_client_factory.cc
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/chrome/browser/ui/settings/cells/clear_browsing_data_item_unittest.mm
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/web_view/internal/signin/ios_web_view_signin_client.h
[modify] https://crrev.com/cc13e7ebdd1c65894fc16f3bdacdf36aef19d9f9/ios/web_view/internal/signin/ios_web_view_signin_client.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0c09aaf82f1e73fe4d119490a8a9fa595e50677

commit d0c09aaf82f1e73fe4d119490a8a9fa595e50677
Author: David Roger <droger@chromium.org>
Date: Fri Nov 30 10:43:56 2018

[signin] SigninErrorController observes the TokenService

To simplify authentication error handling, this CL updates the
SigninErrorController to get its error state from the token service by
observing it.
This removes some coupling between the classes, and removes the
AuthStatusProvider mechanism, leading to an overall simplification of the code.

TBR=eugenebut

Bug: 836212
Change-Id: Ie9b849fcd804d4fe6b4787b277b27fb848cdf2dc
Reviewed-on: https://chromium-review.googlesource.com/c/1070154
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612610}
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/chrome/browser/chromeos/oauth2_token_service_delegate.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/chrome/browser/chromeos/oauth2_token_service_delegate.h
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/chrome/browser/chromeos/oauth2_token_service_delegate_unittest.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/chrome/browser/signin/chrome_signin_client_unittest.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/chrome/browser/signin/dice_response_handler_unittest.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/chrome/browser/signin/profile_oauth2_token_service_factory.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/chrome/browser/signin/signin_error_controller_factory.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/components/signin/core/browser/BUILD.gn
[delete] https://crrev.com/3f9e02a91564e3bd3a7a026e5982697cacefa886/components/signin/core/browser/fake_auth_status_provider.cc
[delete] https://crrev.com/3f9e02a91564e3bd3a7a026e5982697cacefa886/components/signin/core/browser/fake_auth_status_provider.h
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/components/signin/core/browser/signin_error_controller.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/components/signin/core/browser/signin_error_controller.h
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/components/signin/core/browser/signin_error_controller_unittest.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/components/signin/ios/browser/profile_oauth2_token_service_ios_delegate.h
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/components/signin/ios/browser/profile_oauth2_token_service_ios_delegate.mm
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/components/signin/ios/browser/profile_oauth2_token_service_ios_delegate_unittest.mm
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/google_apis/gaia/fake_oauth2_token_service_delegate.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/ios/chrome/browser/signin/authentication_service_unittest.mm
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/ios/chrome/browser/signin/fake_oauth2_token_service_builder.mm
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/ios/chrome/browser/signin/profile_oauth2_token_service_factory.mm
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/ios/chrome/browser/signin/signin_client_factory.h
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/ios/chrome/browser/signin/signin_error_controller_factory.cc
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/ios/web_view/internal/signin/web_view_oauth2_token_service_factory.mm
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/ios/web_view/internal/signin/web_view_signin_error_controller_factory.mm
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/ios/web_view/internal/sync/cwv_sync_controller.mm
[modify] https://crrev.com/d0c09aaf82f1e73fe4d119490a8a9fa595e50677/ios/web_view/internal/sync/cwv_sync_controller_unittest.mm

Sign in to add a comment