Commit 282f20f8 authored by Sam McNally's avatar Sam McNally Committed by Commit Bot

Wait until the primary account is ready before minting DriveFS tokens.

Returning transient errors when the refresh token isn't loaded yet was
better than persistent errors, but it could result in quite a few errors
being returned before a success, sometimes causing DriveFS to give up
anyway. Avoid this by waiting for the account to be ready before
servicing any access token requests.

Bug: 855002
Change-Id: I1578f8f9116592e1ee2864a804269a4a49ba8c92
Reviewed-on: https://chromium-review.googlesource.com/1125551Reviewed-by: default avatarStuart Langley <slangley@chromium.org>
Commit-Queue: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572482}
parent 1aba972d
...@@ -260,7 +260,6 @@ class DriveIntegrationService::DriveFsHolder ...@@ -260,7 +260,6 @@ class DriveIntegrationService::DriveFsHolder
void OnMounted(const base::FilePath& path) override; void OnMounted(const base::FilePath& path) override;
std::unique_ptr<drivefs::DriveFsHost::MojoConnectionDelegate> std::unique_ptr<drivefs::DriveFsHost::MojoConnectionDelegate>
CreateMojoConnectionDelegate() override; CreateMojoConnectionDelegate() override;
bool AreRefreshTokensReady() override;
Profile* const profile_; Profile* const profile_;
...@@ -309,14 +308,6 @@ DriveIntegrationService::DriveFsHolder::CreateMojoConnectionDelegate() { ...@@ -309,14 +308,6 @@ DriveIntegrationService::DriveFsHolder::CreateMojoConnectionDelegate() {
return Delegate::CreateMojoConnectionDelegate(); return Delegate::CreateMojoConnectionDelegate();
} }
bool DriveIntegrationService::DriveFsHolder::AreRefreshTokensReady() {
const auto state = ProfileOAuth2TokenServiceFactory::GetForProfile(profile_)
->GetDelegate()
->GetLoadCredentialsState();
return state != OAuth2TokenServiceDelegate::LOAD_CREDENTIALS_IN_PROGRESS &&
state != OAuth2TokenServiceDelegate::LOAD_CREDENTIALS_NOT_STARTED;
}
void DriveIntegrationService::DriveFsHolder::OnMounted( void DriveIntegrationService::DriveFsHolder::OnMounted(
const base::FilePath& path) { const base::FilePath& path) {
on_drivefs_mounted_.Run(); on_drivefs_mounted_.Run();
......
...@@ -165,8 +165,7 @@ class DriveFsHost::MountState : public mojom::DriveFsDelegate, ...@@ -165,8 +165,7 @@ class DriveFsHost::MountState : public mojom::DriveFsDelegate,
const std::vector<std::string>& scopes, const std::vector<std::string>& scopes,
GetAccessTokenCallback callback) override { GetAccessTokenCallback callback) override {
DCHECK_CALLED_ON_VALID_SEQUENCE(host_->sequence_checker_); DCHECK_CALLED_ON_VALID_SEQUENCE(host_->sequence_checker_);
if (get_access_token_callback_ || if (get_access_token_callback_) {
!host_->delegate_->AreRefreshTokensReady()) {
std::move(callback).Run(mojom::AccessTokenStatus::kTransientError, ""); std::move(callback).Run(mojom::AccessTokenStatus::kTransientError, "");
return; return;
} }
...@@ -175,11 +174,8 @@ class DriveFsHost::MountState : public mojom::DriveFsDelegate, ...@@ -175,11 +174,8 @@ class DriveFsHost::MountState : public mojom::DriveFsDelegate,
mint_token_flow_ = mint_token_flow_ =
host_->delegate_->CreateMintTokenFlow(this, client_id, app_id, scopes); host_->delegate_->CreateMintTokenFlow(this, client_id, app_id, scopes);
DCHECK(mint_token_flow_); DCHECK(mint_token_flow_);
host_->GetIdentityManager().GetAccessToken( host_->GetIdentityManager().GetPrimaryAccountWhenAvailable(base::BindOnce(
host_->delegate_->GetAccountId().GetUserEmail(), {}, &DriveFsHost::MountState::AccountReady, base::Unretained(this)));
kIdentityConsumerId,
base::BindOnce(&DriveFsHost::MountState::GotChromeAccessToken,
base::Unretained(this)));
} }
void OnMounted() override { void OnMounted() override {
...@@ -198,6 +194,15 @@ class DriveFsHost::MountState : public mojom::DriveFsDelegate, ...@@ -198,6 +194,15 @@ class DriveFsHost::MountState : public mojom::DriveFsDelegate,
void NotifyDelegateOnMounted() { host_->delegate_->OnMounted(mount_path()); } void NotifyDelegateOnMounted() { host_->delegate_->OnMounted(mount_path()); }
void AccountReady(const AccountInfo& info,
const identity::AccountState& state) {
host_->GetIdentityManager().GetAccessToken(
host_->delegate_->GetAccountId().GetUserEmail(), {},
kIdentityConsumerId,
base::BindOnce(&DriveFsHost::MountState::GotChromeAccessToken,
base::Unretained(this)));
}
void GotChromeAccessToken(const base::Optional<std::string>& access_token, void GotChromeAccessToken(const base::Optional<std::string>& access_token,
base::Time expiration_time, base::Time expiration_time,
const GoogleServiceAuthError& error) { const GoogleServiceAuthError& error) {
......
...@@ -59,7 +59,6 @@ class COMPONENT_EXPORT(DRIVEFS) DriveFsHost ...@@ -59,7 +59,6 @@ class COMPONENT_EXPORT(DRIVEFS) DriveFsHost
virtual net::URLRequestContextGetter* GetRequestContext() = 0; virtual net::URLRequestContextGetter* GetRequestContext() = 0;
virtual service_manager::Connector* GetConnector() = 0; virtual service_manager::Connector* GetConnector() = 0;
virtual const AccountId& GetAccountId() = 0; virtual const AccountId& GetAccountId() = 0;
virtual bool AreRefreshTokensReady() = 0;
virtual std::unique_ptr<OAuth2MintTokenFlow> CreateMintTokenFlow( virtual std::unique_ptr<OAuth2MintTokenFlow> CreateMintTokenFlow(
OAuth2MintTokenFlow::Delegate* delegate, OAuth2MintTokenFlow::Delegate* delegate,
const std::string& client_id, const std::string& client_id,
......
...@@ -109,10 +109,7 @@ class TestingDriveFsHostDelegate : public DriveFsHost::Delegate { ...@@ -109,10 +109,7 @@ class TestingDriveFsHostDelegate : public DriveFsHost::Delegate {
TestingDriveFsHostDelegate( TestingDriveFsHostDelegate(
std::unique_ptr<service_manager::Connector> connector, std::unique_ptr<service_manager::Connector> connector,
const AccountId& account_id) const AccountId& account_id)
: connector_(std::move(connector)), account_id_(account_id) { : connector_(std::move(connector)), account_id_(account_id) {}
ON_CALL(*this, AreRefreshTokensReady())
.WillByDefault(testing::Return(true));
}
MockOAuth2MintTokenFlow& mock_flow() { return mock_flow_; } MockOAuth2MintTokenFlow& mock_flow() { return mock_flow_; }
...@@ -122,7 +119,6 @@ class TestingDriveFsHostDelegate : public DriveFsHost::Delegate { ...@@ -122,7 +119,6 @@ class TestingDriveFsHostDelegate : public DriveFsHost::Delegate {
// DriveFsHost::Delegate: // DriveFsHost::Delegate:
MOCK_METHOD1(OnMounted, void(const base::FilePath&)); MOCK_METHOD1(OnMounted, void(const base::FilePath&));
MOCK_METHOD0(AreRefreshTokensReady, bool());
private: private:
// DriveFsHost::Delegate: // DriveFsHost::Delegate:
...@@ -192,6 +188,16 @@ class FakeIdentityService ...@@ -192,6 +188,16 @@ class FakeIdentityService
} }
// identity::mojom::IdentityManagerInterceptorForTesting overrides: // identity::mojom::IdentityManagerInterceptorForTesting overrides:
void GetPrimaryAccountWhenAvailable(
GetPrimaryAccountWhenAvailableCallback callback) override {
auto account_id = AccountId::FromUserEmailGaiaId("test@example.com", "ID");
AccountInfo account_info;
account_info.email = account_id.GetUserEmail();
account_info.gaia = account_id.GetGaiaId();
account_info.account_id = account_id.GetAccountIdKey();
std::move(callback).Run(account_info, {});
}
void GetAccessToken(const std::string& account_id, void GetAccessToken(const std::string& account_id,
const ::identity::ScopeSet& scopes, const ::identity::ScopeSet& scopes,
const std::string& consumer_id, const std::string& consumer_id,
...@@ -571,29 +577,6 @@ TEST_F(DriveFsHostTest, GetAccessToken_SequentialRequests) { ...@@ -571,29 +577,6 @@ TEST_F(DriveFsHostTest, GetAccessToken_SequentialRequests) {
} }
} }
TEST_F(DriveFsHostTest, GetAccessToken_RefreshTokensNotReadyYet) {
ASSERT_NO_FATAL_FAILURE(DoMount());
EXPECT_CALL(*host_delegate_, AreRefreshTokensReady())
.WillOnce(testing::Return(false));
EXPECT_CALL(mock_identity_manager_,
GetAccessToken("test@example.com", _, "drivefs"))
.Times(0);
host_delegate_->mock_flow().ExpectNoStartCalls();
base::RunLoop run_loop;
auto quit_closure = run_loop.QuitClosure();
delegate_ptr_->GetAccessToken(
"client ID", "app ID", {"scope1", "scope2"},
base::BindLambdaForTesting(
[&](mojom::AccessTokenStatus status, const std::string& token) {
EXPECT_EQ(mojom::AccessTokenStatus::kTransientError, status);
EXPECT_TRUE(token.empty());
std::move(quit_closure).Run();
}));
run_loop.Run();
}
TEST_F(DriveFsHostTest, GetAccessToken_GetAccessTokenFailure_Permanent) { TEST_F(DriveFsHostTest, GetAccessToken_GetAccessTokenFailure_Permanent) {
ASSERT_NO_FATAL_FAILURE(DoMount()); ASSERT_NO_FATAL_FAILURE(DoMount());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment