[Autofill Assistant] Remove intent validation for password scripts

As password scripts have a confirmation step, we don't need to validate
an intent to change a password.
We still need to check whether a running script is a password script (if
yes, the password manager should be notified that a script is running).
For that, we will check whether |TriggerContext| has
PASSWORD_CHANGE_USERNAME param. (For convenience, this CL moves that
parameter's name and also kOverlayColorsParameterName to
trigger_context.cc).

This CL also changes what origin is used for password store request. If
a password script is started from Settings, GetLastCommittedURL is not
set yet. So, we have to use the deep url provided.

Bug: 1086109
Change-Id: I7c38388b06f433dc51bce1a2b54a7d48bb322bfc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2332594
Commit-Queue: Maxim Kolosovskiy  <kolos@chromium.org>
Reviewed-by: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Milica Selakovic <selakovic@google.com>
Cr-Commit-Position: refs/heads/master@{#795844}

chrome/browser/android/autofill_assistant/client_android.cc

@@ -81,19 +81,6 @@ std::unique_ptr<TriggerContextImpl> CreateTriggerContext(
       base::android::ConvertJavaStringToUTF8(env, jexperiment_ids));
 }
 
-// Notifies Chrome's Password Manager that Autofill Assistant is running or
-// not. No-op if the script is not a password change script.
-void NotifyPasswordManagerIfApplicable(
-    ClientAndroid* client,
-    password_manager::AutofillAssistantMode mode) {
-  auto* password_manager_client = client->GetPasswordManagerClient();
-  if (password_manager_client &&
-      password_manager_client->WasCredentialLeakDialogShown()) {
-    password_manager_client->GetPasswordManager()->SetAutofillAssistantMode(
-        mode);
-  }
-}
-
 }  // namespace
 
 static base::android::ScopedJavaLocalRef<jobject>
@@ -197,9 +184,8 @@ void ClientAndroid::TransferUITo(
   auto ui_ptr = std::move(ui_controller_android_);
   // From this point on, the UIController, in ui_ptr, is either transferred or
   // deleted.
-
   NotifyPasswordManagerIfApplicable(
-      this, password_manager::AutofillAssistantMode::kNotRunning);
+      password_manager::AutofillAssistantMode::kNotRunning);
 
   if (!jother_web_contents)
     return;
@@ -351,6 +337,18 @@ void ClientAndroid::OnFetchWebsiteActions(
       env, java_object_, jcallback, controller_ != nullptr);
 }
 
+void ClientAndroid::NotifyPasswordManagerIfApplicable(
+    password_manager::AutofillAssistantMode mode) {
+  if (!controller_->GetTriggerContext()->GetPasswordChangeUsername())
+    return;
+
+  auto* password_manager_client = GetPasswordManagerClient();
+  if (password_manager_client) {
+    password_manager_client->GetPasswordManager()->SetAutofillAssistantMode(
+        mode);
+  }
+}
+
 bool ClientAndroid::PerformDirectAction(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& jcaller,
@@ -438,7 +436,7 @@ void ClientAndroid::AttachUI(
     // Suppress password manager's prompts while running a password change
     // script.
     NotifyPasswordManagerIfApplicable(
-        this, password_manager::AutofillAssistantMode::kRunning);
+        password_manager::AutofillAssistantMode::kRunning);
   }
 }
 
@@ -552,11 +550,12 @@ void ClientAndroid::Shutdown(Metrics::DropOutReason reason) {
   if (!controller_)
     return;
 
+  if (ui_controller_android_ && ui_controller_android_->IsAttached()) {
+    // Notify the password manager only if ui is not transferred to another tab.
     NotifyPasswordManagerIfApplicable(
-      this, password_manager::AutofillAssistantMode::kNotRunning);
-
-  if (ui_controller_android_ && ui_controller_android_->IsAttached())
+        password_manager::AutofillAssistantMode::kNotRunning);
     DestroyUI();
+  }
 
   if (started_)
     Metrics::RecordDropOut(reason);

chrome/browser/android/autofill_assistant/client_android.h

@@ -22,6 +22,10 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_user_data.h"
 
+namespace password_manager {
+enum class AutofillAssistantMode;
+}
+
 namespace autofill_assistant {
 
 // Creates a Autofill Assistant client associated with a WebContents.
@@ -129,6 +133,10 @@ class ClientAndroid : public Client,
       const base::android::JavaParamRef<jobject>& jonboarding_coordinator);
   bool NeedsUI();
   void OnFetchWebsiteActions(const base::android::JavaRef<jobject>& jcallback);
+  // Notifies Chrome's Password Manager that Autofill Assistant is running or
+  // not. No-op if the script is not a password change script.
+  void NotifyPasswordManagerIfApplicable(
+      password_manager::AutofillAssistantMode mode);
 
   base::android::ScopedJavaLocalRef<jobjectArray>
   GetDirectActionsAsJavaArrayOfStrings(JNIEnv* env) const;

components/autofill_assistant/browser/controller.cc

@@ -42,15 +42,6 @@ namespace {
 // message.
 static constexpr int kAutostartInitialProgress = 5;
 
-// Parameter that allows setting the color of the overlay.
-const char kOverlayColorParameterName[] = "OVERLAY_COLORS";
-
-// Parameter that contains the current session username. Should be synced with
-// |SESSION_USERNAME_PARAMETER| from
-// .../password_manager/PasswordChangeLauncher.java
-// TODO(b/151401974): Eliminate duplicate parameter definitions.
-const char kPasswordChangeUsernameParameterName[] = "PASSWORD_CHANGE_USERNAME";
-
 // Experiment for toggling the new progress bar.
 const char kProgressBarExperiment[] = "4400697";
 
@@ -1027,7 +1018,7 @@ void Controller::InitFromParameters() {
     SetDetails(std::move(details));
 
   const base::Optional<std::string> overlay_color =
-      trigger_context_->GetParameter(kOverlayColorParameterName);
+      trigger_context_->GetOverlayColors();
   if (overlay_color) {
     std::unique_ptr<OverlayColors> colors = std::make_unique<OverlayColors>();
     std::vector<std::string> color_strings =
@@ -1045,9 +1036,11 @@ void Controller::InitFromParameters() {
     SetOverlayColors(std::move(colors));
   }
   const base::Optional<std::string> password_change_username =
-      trigger_context_->GetParameter(kPasswordChangeUsernameParameterName);
+      trigger_context_->GetPasswordChangeUsername();
   if (password_change_username) {
-    user_data_->selected_login_.emplace(web_contents()->GetLastCommittedURL(),
+    DCHECK(
+        GetCurrentURL().is_valid());  // At least |deeplink_url_| must be set.
+    user_data_->selected_login_.emplace(GetCurrentURL().GetOrigin(),
                                         *password_change_username);
   }
 
@@ -1087,20 +1080,10 @@ bool Controller::Start(const GURL& deeplink_url,
       state_ != AutofillAssistantState::TRACKING) {
     return false;
   }
-  // Verify a password change intent before running.
-  // TODO(b/151391231): Remove when intent signing is implemented.
-  if (trigger_context->GetParameter(kPasswordChangeUsernameParameterName)) {
-    auto* password_manager_client = client_->GetPasswordManagerClient();
-    if (!password_manager_client ||
-        !password_manager_client->WasCredentialLeakDialogShown()) {
-      VLOG(1) << "Failed to start a password change flow.";
-      return false;
-    }
-  }
 
   trigger_context_ = std::move(trigger_context);
-  InitFromParameters();
   deeplink_url_ = deeplink_url;
+  InitFromParameters();
 
   // Force a re-evaluation of the script, to get a chance to autostart.
   if (state_ == AutofillAssistantState::TRACKING)

components/autofill_assistant/browser/controller_unittest.cc

@@ -24,7 +24,6 @@
 #include "components/autofill_assistant/browser/service.h"
 #include "components/autofill_assistant/browser/trigger_context.h"
 #include "components/autofill_assistant/browser/web/mock_web_controller.h"
-#include "components/password_manager/core/browser/stub_password_manager_client.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_renderer_host.h"
@@ -62,12 +61,6 @@ using ::testing::UnorderedElementsAre;
 
 namespace {
 
-class MockPasswordManagerClient
-    : public password_manager::StubPasswordManagerClient {
- public:
-  MOCK_CONST_METHOD0(WasCredentialLeakDialogShown, bool());
-};
-
 // Same as non-mock, but provides default mock callbacks.
 struct MockCollectUserDataOptions : public CollectUserDataOptions {
   MockCollectUserDataOptions() {
@@ -104,8 +97,6 @@ class ControllerTest : public content::RenderViewHostTestHarness {
     mock_service_ = service.get();
 
     ON_CALL(mock_client_, GetWebContents).WillByDefault(Return(web_contents()));
-    ON_CALL(mock_client_, GetPasswordManagerClient)
-        .WillByDefault(Return(&mock_password_manager_client_));
     ON_CALL(mock_client_, HasHadUI()).WillByDefault(Return(true));
 
     controller_ = std::make_unique<Controller>(
@@ -228,7 +219,6 @@ class ControllerTest : public content::RenderViewHostTestHarness {
   MockWebController* mock_web_controller_;
   NiceMock<MockClient> mock_client_;
   NiceMock<MockControllerObserver> mock_observer_;
-  MockPasswordManagerClient mock_password_manager_client_;
   std::unique_ptr<Controller> controller_;
 };
 
@@ -2398,9 +2388,6 @@ TEST_F(ControllerTest, SetGenericUi) {
 }
 
 TEST_F(ControllerTest, StartPasswordChangeFlow) {
-  EXPECT_CALL(mock_password_manager_client_, WasCredentialLeakDialogShown())
-      .WillOnce(Return(true));
-
   GURL initialUrl("http://example.com/password");
   EXPECT_CALL(*mock_service_, OnGetScriptsForUrl(Eq(initialUrl), _, _))
       .WillOnce(RunOnceCallback<2>(true, ""));
@@ -2413,24 +2400,6 @@ TEST_F(ControllerTest, StartPasswordChangeFlow) {
   EXPECT_EQ(GetUserData()->selected_login_->username, username);
 }
 
-TEST_F(ControllerTest, BlockPasswordChangeFlow) {
-  // If the password manager doesn't confirm that a leak dialog was shown, the
-  // flow should not start.
-  EXPECT_CALL(mock_password_manager_client_, WasCredentialLeakDialogShown())
-      .WillOnce(Return(false));
-
-  GURL initialUrl("http://example.com/password");
-  EXPECT_CALL(*mock_service_, OnGetScriptsForUrl(Eq(initialUrl), _, _))
-      .Times(0);
-  std::map<std::string, std::string> parameters;
-  std::string username = "test_username";
-  parameters["PASSWORD_CHANGE_USERNAME"] = username;
-
-  EXPECT_FALSE(
-      controller_->Start(initialUrl, TriggerContext::Create(parameters, "")));
-  EXPECT_FALSE(GetUserData()->selected_login_);
-}
-
 TEST_F(ControllerTest, EndPromptWithOnEndNavigation) {
   // A single script, with a prompt action and on_end_navigation enabled.
   SupportsScriptResponseProto script_response;

components/autofill_assistant/browser/trigger_context.cc

@@ -5,6 +5,17 @@
 #include "components/autofill_assistant/browser/trigger_context.h"
 #include "base/strings/string_split.h"
 
+namespace {
+// Parameter that allows setting the color of the overlay.
+const char kOverlayColorParameterName[] = "OVERLAY_COLORS";
+
+// Parameter that contains the current session username. Should be synced with
+// |SESSION_USERNAME_PARAMETER| from
+// .../password_manager/PasswordChangeLauncher.java
+// TODO(b/151401974): Eliminate duplicate parameter definitions.
+const char kPasswordChangeUsernameParameterName[] = "PASSWORD_CHANGE_USERNAME";
+}  // namespace
+
 namespace autofill_assistant {
 
 // static
@@ -28,6 +39,14 @@ std::unique_ptr<TriggerContext> TriggerContext::Merge(
 TriggerContext::TriggerContext() {}
 TriggerContext::~TriggerContext() {}
 
+base::Optional<std::string> TriggerContext::GetOverlayColors() const {
+  return GetParameter(kOverlayColorParameterName);
+}
+
+base::Optional<std::string> TriggerContext::GetPasswordChangeUsername() const {
+  return GetParameter(kPasswordChangeUsernameParameterName);
+}
+
 TriggerContextImpl::TriggerContextImpl() {}
 
 TriggerContextImpl::TriggerContextImpl(

components/autofill_assistant/browser/trigger_context.h

@@ -44,6 +44,10 @@ class TriggerContext {
   virtual base::Optional<std::string> GetParameter(
       const std::string& name) const = 0;
 
+  // Getters for specific parameters.
+  base::Optional<std::string> GetOverlayColors() const;
+  base::Optional<std::string> GetPasswordChangeUsername() const;
+
   // Returns a comma-separated set of experiment ids.
   virtual std::string experiment_ids() const = 0;
 

components/password_manager/core/browser/password_manager_client.h

@@ -272,8 +272,9 @@ class PasswordManagerClient {
   virtual bool WasLastNavigationHTTPError() const;
 
   // Returns true if a credential leak dialog was shown. Used by Autofill
-  // Assistance to verify a password change intent. TODO(b/151391231): Remove
-  // when proper intent signing is implemented.
+  // Assistance to verify a password change intent. TODO(b/151391231): At the
+  // moment, password change scripts don't need validation, but it may change.
+  // If it doesn't change, remove this method and related code.
   virtual bool WasCredentialLeakDialogShown() const;
 
   // Obtains the cert status for the main frame.