ambient: Do not allow lock screen token refresh

Bug: b/162880032
Test: added new AmbientControllerTest
Change-Id: I9e1104b6d3a8999b2799efa29fc656545b77289a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2339642
Commit-Queue: Tao Wu <wutao@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799945}

ash/ambient/ambient_access_token_controller.cc

@@ -4,6 +4,7 @@
 
 #include "ash/ambient/ambient_access_token_controller.h"
 
+#include "ash/login/ui/lock_screen.h"
 #include "ash/public/cpp/ambient/ambient_client.h"
 #include "base/logging.h"
 #include "base/rand_util.h"
@@ -18,10 +19,6 @@ constexpr base::TimeDelta kMinTokenRefreshDelay =
 constexpr base::TimeDelta kMaxTokenRefreshDelay =
     base::TimeDelta::FromMilliseconds(60 * 1000);
 
-// The buffer time to use the access token.
-constexpr base::TimeDelta kTokenExpirationTimeBuffer =
-    base::TimeDelta::FromMinutes(10);
-
 }  // namespace
 
 AmbientAccessTokenController::AmbientAccessTokenController() = default;
@@ -29,7 +26,8 @@ AmbientAccessTokenController::AmbientAccessTokenController() = default;
 AmbientAccessTokenController::~AmbientAccessTokenController() = default;
 
 void AmbientAccessTokenController::RequestAccessToken(
-    AccessTokenCallback callback) {
+    AccessTokenCallback callback,
+    bool may_refresh_token_on_lock) {
   // |token_refresh_timer_| may become stale during sleeping.
   if (token_refresh_timer_.IsRunning())
     token_refresh_timer_.AbandonAndStop();
@@ -39,7 +37,7 @@ void AmbientAccessTokenController::RequestAccessToken(
 
     // Return the token if there is enough time to use the access token when
     // requested.
-    if (expiration_time_ - base::Time::Now() > kTokenExpirationTimeBuffer) {
+    if (expiration_time_ - base::Time::Now() > token_usage_time_buffer_) {
       RunCallback(std::move(callback));
       return;
     }
@@ -48,6 +46,11 @@ void AmbientAccessTokenController::RequestAccessToken(
     expiration_time_ = base::Time::Now();
   }
 
+  if (!may_refresh_token_on_lock && LockScreen::HasInstance()) {
+    RunCallback(std::move(callback));
+    return;
+  }
+
   callbacks_.emplace_back(std::move(callback));
 
   if (has_pending_request_)
@@ -107,9 +110,13 @@ void AmbientAccessTokenController::NotifyAccessTokenRefreshed() {
 }
 
 void AmbientAccessTokenController::RunCallback(AccessTokenCallback callback) {
-  DCHECK(!gaia_id_.empty());
-  DCHECK(!access_token_.empty());
+  LOG(ERROR) << " DW run calback " << access_token_;
   std::move(callback).Run(gaia_id_, access_token_);
 }
 
+void AmbientAccessTokenController::SetTokenUsageBufferForTesting(
+    base::TimeDelta time) {
+  token_usage_time_buffer_ = time;
+}
+
 }  // namespace ash

ash/ambient/ambient_access_token_controller.h

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "ash/ambient/ambient_constants.h"
 #include "ash/ash_export.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
@@ -30,7 +31,15 @@ class ASH_EXPORT AmbientAccessTokenController {
       delete;
   ~AmbientAccessTokenController();
 
-  void RequestAccessToken(AccessTokenCallback callback);
+  // The caller will pass in a preference |may_refresh_token_on_lock| whether
+  // to refresh token on lock screen when it expires. In current implementation,
+  // the AmbientController will request token once the screen is locked. This is
+  // allowed (We could make the request before screen is locked, then the logic
+  // in AmbientAccessTokenController could be simpler, i.e. just check if the
+  // lock screen is on or not). Future requests on lock screen can not refresh
+  // token if it expires.
+  void RequestAccessToken(AccessTokenCallback callback,
+                          bool may_refresh_token_on_lock = false);
 
  private:
   friend class AmbientAshTestBase;
@@ -43,6 +52,8 @@ class ASH_EXPORT AmbientAccessTokenController {
   void NotifyAccessTokenRefreshed();
   void RunCallback(AccessTokenCallback callback);
 
+  void SetTokenUsageBufferForTesting(base::TimeDelta time);
+
   std::string gaia_id_;
   std::string access_token_;
 
@@ -52,6 +63,9 @@ class ASH_EXPORT AmbientAccessTokenController {
   // True if has already sent access token request and waiting for result.
   bool has_pending_request_ = false;
 
+  // The buffer time to use the access token.
+  base::TimeDelta token_usage_time_buffer_ = kTokenUsageTimeBuffer;
+
   base::OneShotTimer token_refresh_timer_;
   int token_refresh_error_backoff_factor = 1;
 

ash/ambient/ambient_constants.h

@@ -44,6 +44,10 @@ constexpr char kPhotoDetailsFileExt[] = ".txt";
 // Directory name of ambient mode.
 constexpr char kAmbientModeDirectoryName[] = "ambient-mode";
 
+// The buffer time to use the access token.
+constexpr base::TimeDelta kTokenUsageTimeBuffer =
+    base::TimeDelta::FromMinutes(10);
+
 }  // namespace ash
 
 #endif  // ASH_AMBIENT_AMBIENT_CONSTANTS_H_

ash/ambient/ambient_controller.cc

@@ -275,7 +275,7 @@ void AmbientController::OnLockStateChanged(bool locked) {
     //        locking screen. We can add the refresh token into it. If the token
     //        has already been fetched, then there is not additional time to
     //        wait.
-    RequestAccessToken(base::DoNothing());
+    RequestAccessToken(base::DoNothing(), /*may_refresh_token_on_lock=*/true);
 
     ShowUi(AmbientUiMode::kLockScreenUi);
   } else {
@@ -463,8 +463,10 @@ void AmbientController::CloseWidget(bool immediately) {
 }
 
 void AmbientController::RequestAccessToken(
-    AmbientAccessTokenController::AccessTokenCallback callback) {
-  access_token_controller_.RequestAccessToken(std::move(callback));
+    AmbientAccessTokenController::AccessTokenCallback callback,
+    bool may_refresh_token_on_lock) {
+  access_token_controller_.RequestAccessToken(std::move(callback),
+                                              may_refresh_token_on_lock);
 }
 
 AmbientBackendModel* AmbientController::GetAmbientBackendModel() {

ash/ambient/ambient_controller.h

@@ -86,7 +86,8 @@ class ASH_EXPORT AmbientController
   void UpdateUiMode(AmbientUiMode ui_mode);
 
   void RequestAccessToken(
-      AmbientAccessTokenController::AccessTokenCallback callback);
+      AmbientAccessTokenController::AccessTokenCallback callback,
+      bool may_refresh_token_on_lock = false);
 
   AmbientBackendModel* GetAmbientBackendModel();
 
@@ -139,6 +140,10 @@ class ASH_EXPORT AmbientController
     return container_view_;
   }
 
+  AmbientAccessTokenController* access_token_controller_for_testing() {
+    return &access_token_controller_;
+  }
+
   // Owned by |RootView| of its parent widget.
   AmbientContainerView* container_view_ = nullptr;
 

ash/ambient/ambient_controller_unittest.cc

@@ -136,6 +136,50 @@ TEST_F(AmbientControllerTest, ShouldReturnCachedAccessToken) {
   CloseAmbientScreen();
 }
 
+TEST_F(AmbientControllerTest, ShouldReturnEmptyAccessToken) {
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  // Lock the screen will request a token.
+  LockScreen();
+  EXPECT_TRUE(IsAccessTokenRequestPending());
+  std::string access_token = "access_token";
+  IssueAccessToken(access_token, /*with_error=*/false);
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+
+  // Another token request will return cached token.
+  base::OnceClosure closure = base::MakeExpectedRunClosure(FROM_HERE);
+  base::RunLoop run_loop_1;
+  ambient_controller()->RequestAccessToken(base::BindLambdaForTesting(
+      [&](const std::string& gaia_id, const std::string& access_token_fetched) {
+        EXPECT_EQ(access_token_fetched, access_token);
+
+        std::move(closure).Run();
+        run_loop_1.Quit();
+      }));
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+  run_loop_1.Run();
+
+  base::RunLoop run_loop_2;
+  // When token expired, another token request will get empty token.
+  constexpr base::TimeDelta kTokenRefreshDelay =
+      base::TimeDelta::FromSeconds(60);
+  task_environment()->FastForwardBy(kTokenRefreshDelay);
+
+  closure = base::MakeExpectedRunClosure(FROM_HERE);
+  ambient_controller()->RequestAccessToken(base::BindLambdaForTesting(
+      [&](const std::string& gaia_id, const std::string& access_token_fetched) {
+        EXPECT_TRUE(access_token_fetched.empty());
+
+        std::move(closure).Run();
+        run_loop_2.Quit();
+      }));
+  EXPECT_FALSE(IsAccessTokenRequestPending());
+  run_loop_2.Run();
+
+  // Clean up.
+  CloseAmbientScreen();
+}
+
 TEST_F(AmbientControllerTest, ShouldRefreshAccessTokenAfterFailure) {
   EXPECT_FALSE(IsAccessTokenRequestPending());
 

ash/ambient/test/ambient_ash_test_base.cc

@@ -8,6 +8,7 @@
 #include <utility>
 #include <vector>
 
+#include "ash/ambient/ambient_access_token_controller.h"
 #include "ash/ambient/ambient_constants.h"
 #include "ash/ambient/ambient_photo_controller.h"
 #include "ash/ambient/ui/ambient_background_image_view.h"
@@ -115,6 +116,8 @@ void AmbientAshTestBase::SetUp() {
       std::make_unique<TestAmbientURLLoaderImpl>());
   photo_controller()->set_image_decoder_for_testing(
       std::make_unique<TestAmbientImageDecoderImpl>());
+  token_controller()->SetTokenUsageBufferForTesting(
+      base::TimeDelta::FromSeconds(30));
   base::RunLoop().RunUntilIdle();
 }
 
@@ -263,6 +266,10 @@ AmbientContainerView* AmbientAshTestBase::container_view() {
   return ambient_controller()->get_container_view_for_testing();
 }
 
+AmbientAccessTokenController* AmbientAshTestBase::token_controller() {
+  return ambient_controller()->access_token_controller_for_testing();
+}
+
 void AmbientAshTestBase::FetchTopics() {
   photo_controller()->FetchTopicsForTesting();
 }

ash/ambient/test/ambient_ash_test_base.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 
+#include "ash/ambient/ambient_access_token_controller.h"
 #include "ash/ambient/ambient_controller.h"
 #include "ash/ambient/ui/ambient_background_image_view.h"
 #include "ash/public/cpp/test/test_ambient_client.h"
@@ -20,6 +21,7 @@
 
 namespace ash {
 
+class AmbientAccessTokenController;
 class AmbientContainerView;
 class AmbientPhotoController;
 class MediaStringView;
@@ -103,6 +105,8 @@ class AmbientAshTestBase : public AshTestBase {
   // Returns the top-level view which contains all the ambient components.
   AmbientContainerView* container_view();
 
+  AmbientAccessTokenController* token_controller();
+
   void FetchTopics();
 
   void FetchImage();

ash/public/cpp/test/test_ambient_client.cc

@@ -16,7 +16,7 @@ namespace {
 const char* kTestGaiaId = "0123456789";
 
 constexpr base::TimeDelta kDefaultTokenExpirationDelay =
-    base::TimeDelta::FromHours(1);
+    base::TimeDelta::FromSeconds(60);
 
 }  // namespace