Skip to content

Commit

Permalink
[ios/sign-in sheet] Re-write account cookie waiting
Browse files Browse the repository at this point in the history
No behavior change for most practical cases. The CL might also be a
bug fix for some speculative scenarios, more details below.

Context: When accounts.google.com is opened, a bottom sheet is shown
to offer browser sign in. If the user taps its primary button, the
sheet waits until the account cookie is populated, then closes itself.
It might also show an error dialog instead, in case of an auth error or
timeout.

Before this CL, this was implemented as follows:
1. ConsistencyPromoSigninMediator observed the IdentityManager for
  its entire lifetime.
2. Upon getting an OnAccountsInCookieUpdated() notification, it
  checked whether a) there was no auth error, b) there was a primary
  account, and c) the cookie jar was non-empty. If all 3 were
  satisfied, it closed the sheet.

This approach has some problems, at least theoretically.
- In 1, more than one notification might arrive. Or a notification
  might arrive before the user taps the button.
- In 2, the check is still satisfied if the cookie jar is filled with
  an account different from the one selected by the user in the UI.

This CL:
- Extracts the waiting logic to a helper class.
- Checks that cookie jar for the correct account.

Bug: 1471140
Change-Id: Ie54430662a01098a8126c30f17993023e9c6ecf9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4766696
Reviewed-by: Jérôme Lebel <jlebel@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/main@{#1188514}
  • Loading branch information
Victor Hugo Vianna Silva authored and Chromium LUCI CQ committed Aug 25, 2023
1 parent 9a5b329 commit f667894
Show file tree
Hide file tree
Showing 7 changed files with 211 additions and 131 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import("//build/config/chrome_build.gni")

source_set("consistency_promo_signin") {
sources = [
"account_cookie_waiter.h",
"account_cookie_waiter.mm",
"consistency_promo_signin_coordinator.h",
"consistency_promo_signin_coordinator.mm",
"consistency_promo_signin_mediator.h",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef IOS_CHROME_BROWSER_UI_AUTHENTICATION_SIGNIN_CONSISTENCY_PROMO_SIGNIN_ACCOUNT_COOKIE_WAITER_H_
#define IOS_CHROME_BROWSER_UI_AUTHENTICATION_SIGNIN_CONSISTENCY_PROMO_SIGNIN_ACCOUNT_COOKIE_WAITER_H_

#import "base/functional/callback_forward.h"
#import "base/memory/raw_ptr.h"
#import "base/timer/timer.h"
#import "components/signin/public/identity_manager/identity_manager.h"
#import "google_apis/gaia/core_account_id.h"

// Helper to wait for a Gaia cookie to be populated.
class AccountCookieWaiter : public signin::IdentityManager::Observer {
public:
enum class Result {
kSuccess,
kAuthError,
kTimeout,
};

explicit AccountCookieWaiter(signin::IdentityManager* identity_manager);

AccountCookieWaiter(const AccountCookieWaiter&) = delete;
AccountCookieWaiter& operator=(const AccountCookieWaiter&) = delete;

// If `Wait()` is ongoing, aborts without invoking the callback.
~AccountCookieWaiter() override;

// Waits for `account_id` to be present in the cookie jar and invokes
// `callback` with the outcome. Must not be called again before the previous
// wait finishes, i.e. before `callback` is invoked.
void Wait(CoreAccountId account_id,
base::OnceCallback<void(Result)> callback);

// signin::IdentityManager::Observer implementation.
void OnAccountsInCookieUpdated(
const signin::AccountsInCookieJarInfo& accounts_in_cookie_jar_info,
const GoogleServiceAuthError& error) override;

private:
void OnTimeout();

const raw_ptr<signin::IdentityManager> identity_manager_;

base::OneShotTimer timer_;
// Set on every Wait().
CoreAccountId account_id_;
// Set on every Wait().
base::OnceCallback<void(Result)> callback_;
};

#endif // IOS_CHROME_BROWSER_UI_AUTHENTICATION_SIGNIN_CONSISTENCY_PROMO_SIGNIN_ACCOUNT_COOKIE_WAITER_H_
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#import "ios/chrome/browser/ui/authentication/signin/consistency_promo_signin/account_cookie_waiter.h"

#import "base/check.h"
#import "base/containers/contains.h"
#import "base/functional/callback.h"
#import "base/location.h"
#import "base/time/time.h"
#import "base/timer/timer.h"
#import "components/signin/public/identity_manager/accounts_in_cookie_jar_info.h"
#import "google_apis/gaia/gaia_auth_util.h"
#import "google_apis/gaia/google_service_auth_error.h"

namespace {

// Sign-in time out duration.
constexpr base::TimeDelta kSigninTimeout = base::Seconds(10);

} // namespace

AccountCookieWaiter::AccountCookieWaiter(
signin::IdentityManager* identity_manager)
: identity_manager_(identity_manager) {
identity_manager_->AddObserver(this);
}

AccountCookieWaiter::~AccountCookieWaiter() {
identity_manager_->RemoveObserver(this);
}

void AccountCookieWaiter::Wait(CoreAccountId account_id,
base::OnceCallback<void(Result)> callback) {
CHECK(!timer_.IsRunning())
<< "Wait() musn't be called before previous call finished";

account_id_ = account_id;
callback_ = std::move(callback);

// Unretained() is safe because `this` outlives `timer_`.
timer_.Start(
FROM_HERE, kSigninTimeout,
base::BindOnce(&AccountCookieWaiter::OnTimeout, base::Unretained(this)));
}

void AccountCookieWaiter::OnAccountsInCookieUpdated(
const signin::AccountsInCookieJarInfo& accounts_in_cookie_jar_info,
const GoogleServiceAuthError& error) {
if (!timer_.IsRunning()) {
// No waiting ongoing.
return;
}

if (error.state() != GoogleServiceAuthError::State::NONE) {
timer_.Stop();
std::move(callback_).Run(Result::kAuthError);
} else if (base::Contains(accounts_in_cookie_jar_info.signed_in_accounts,
account_id_, &gaia::ListedAccount::id)) {
timer_.Stop();
std::move(callback_).Run(Result::kSuccess);
} else {
// Keep waiting.
}
}

void AccountCookieWaiter::OnTimeout() {
std::move(callback_).Run(Result::kTimeout);
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#import "ios/chrome/browser/ui/authentication/signin/consistency_promo_signin/consistency_promo_signin_coordinator.h"

#import <memory>

#import "base/metrics/user_metrics.h"
#import "components/prefs/pref_service.h"
#import "components/signin/public/base/signin_metrics.h"
Expand All @@ -19,6 +21,7 @@
#import "ios/chrome/browser/signin/identity_manager_factory.h"
#import "ios/chrome/browser/signin/system_identity.h"
#import "ios/chrome/browser/ui/authentication/authentication_flow.h"
#import "ios/chrome/browser/ui/authentication/signin/consistency_promo_signin/account_cookie_waiter.h"
#import "ios/chrome/browser/ui/authentication/signin/consistency_promo_signin/consistency_account_chooser/consistency_account_chooser_coordinator.h"
#import "ios/chrome/browser/ui/authentication/signin/consistency_promo_signin/consistency_default_account/consistency_default_account_coordinator.h"
#import "ios/chrome/browser/ui/authentication/signin/consistency_promo_signin/consistency_layout_delegate.h"
Expand Down Expand Up @@ -109,7 +112,8 @@ - (void)start {
self.consistencyPromoSigninMediator = [[ConsistencyPromoSigninMediator alloc]
initWithAccountManagerService:accountManagerService
authenticationService:authenticationService
identityManager:identityManager
accountCookieWaiter:std::make_unique<AccountCookieWaiter>(
identityManager)
userPrefService:browserState->GetPrefs()
accessPoint:self.accessPoint];
self.consistencyPromoSigninMediator.delegate = self;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@
#define IOS_CHROME_BROWSER_UI_AUTHENTICATION_SIGNIN_CONSISTENCY_PROMO_SIGNIN_CONSISTENCY_PROMO_SIGNIN_MEDIATOR_H_

#import <Foundation/Foundation.h>
#import <memory>

#import "base/ios/block_types.h"
#import "ios/chrome/browser/ui/authentication/signin/signin_constants.h"

class AccountCookieWaiter;
@class AuthenticationFlow;
class AuthenticationService;
class ChromeAccountManagerService;
Expand All @@ -18,10 +20,6 @@ class PrefService;
@class SigninCompletionInfo;
@protocol SystemIdentity;

namespace signin {
class IdentityManager;
} // signin

namespace signin_metrics {
enum class AccessPoint : int;
}
Expand Down Expand Up @@ -69,7 +67,8 @@ typedef NS_ENUM(NSInteger, ConsistencyPromoSigninMediatorError) {
initWithAccountManagerService:
(ChromeAccountManagerService*)accountManagerService
authenticationService:(AuthenticationService*)authenticationService
identityManager:(signin::IdentityManager*)identityManager
accountCookieWaiter:
(std::unique_ptr<AccountCookieWaiter>)accountCookieWaiter
userPrefService:(PrefService*)userPrefService
accessPoint:(signin_metrics::AccessPoint)accessPoint;

Expand Down

0 comments on commit f667894

Please sign in to comment.