2fa disabling ask password -- Take 2 (#6527)

* Extract modal close to its own controller

Remove unused `index` parameter

* Add `confirm_password_modal` macro

* Add confirm_password_controller and hook up to confirm_password modal

* Have DeleteTOTPForm check user's password

* Pass username and password to form as expected by PasswordMixin

Allow `username` to be set as an instance variable

* Fix tests and set password visibility on connect

* Add test for username set as instance variable in form

* Rename form field for consistency

* Amend copy on incorrect credentials

* Pass parameters to confirm_password_modal

* Add confirm password to DeleteMacaroonForm

* Fix tests

* Fix linting

* Add UsernameMixin to DeleteMacaroonForm and DeleteTOTPForm

Avoid passing the username to constructor simplifying base class

* Fix linting

* Fix tests after merge

* Remove `index` parameter from call to `confirm_modal`

* Reinstate id and fix missing name on modal input

* Require user password to delete account

Add confirm_password_button macro

* Fix linting

* Fix sorting imports

* Reinstate index kwargs to confirm_password_button

* Add translation to modal text

* Remove index parameter from confirm_modal macros

* Fix call to gettext

* Add tests for ModalClose controller

Amend tests for Confirm controller

tests/frontend/confirm_controller_test.js

@@ -22,23 +22,18 @@ describe("Confirm controller", () => {
     document.body.innerHTML = `
   <div class="modal" data-controller="confirm">
     <div class="modal__content" role="dialog">
-        <a id="cancel" href="#modal-close" data-action="click->confirm#cancel" title="Close" class="modal__close">
-            <i class="fa fa-times" aria-hidden="true"></i>
-            <span class="sr-only">close</span>
-        </a>
-        <div class="modal__body">
-            <h3 class="modal__title">Delete package?</h3>
+      <div class="modal__body">
+        <h3 class="modal__title">Delete package?</h3>
         <p>Confirm to continue.</p>
         <label for="package">Delete</label>
         <input id="input-target" name="package" data-action="input->confirm#check" data-target="confirm.input" type="text" autocomplete="off" autocorrect="off" autocapitalize="off">
         </div>
         <div class="modal__footer">
-            <button type="reset" data-action="click->confirm#cancel">Cancel</button>
-            <button id="button-target" data-target="confirm.button" data-expected="package" type="submit">
-                Confirm
-            </button>
-        </div>
-        </form>
+          <button id="button-target" data-target="confirm.button" data-expected="package" type="submit">
+              Confirm
+          </button>
+      </div>
+      </form>
     </div>
   </div>
     `;
@@ -57,18 +52,6 @@ describe("Confirm controller", () => {
   });
 
   describe("functionality", function() {
-    describe("clicking cancel", function() {
-      it("sets the window location, resets the input target and disables the button", function() {
-        document.getElementById("cancel").click();
-
-        expect(window.location.href).toContain("#modal-close");
-        const inputTarget = document.getElementById("input-target");
-        expect(inputTarget.value).toEqual("");
-        const buttonTarget = document.getElementById("button-target");
-        expect(buttonTarget).toHaveAttribute("disabled");
-      });
-    });
-
     describe("entering expected text", function() {
       it("enables the button", function() {
         fireEvent.input(document.getElementById("input-target"), { target: { value: "package" } });

tests/frontend/modal_close_controller_test.js

@@ -0,0 +1,56 @@
+/* Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/* global expect, beforeEach, describe, it */
+
+import { Application } from "stimulus";
+import ModalCloseController from "../../warehouse/static/js/warehouse/controllers/modal_close_controller";
+
+describe("Modal close controller", () => {
+  beforeEach(() => {
+    document.body.innerHTML = `
+  <div class="modal" data-controller="modal-close">
+    <div class="modal__content" role="dialog">
+      <a id="cancel" href="#modal-close" data-action="click->modal-close#cancel" title="Close" class="modal__close">
+        <i class="fa fa-times" aria-hidden="true"></i>
+        <span class="sr-only">close</span>
+      </a>
+      <div class="modal__body">
+        <h3 class="modal__title">Modal Title</h3>
+        <input id="input-target" name="package" data-target="modal-close.input" type="text" autocomplete="off" autocorrect="off" autocapitalize="off">
+        <div class="modal__footer">
+          <button id="button-target" data-target="modal-close.button" type="submit">
+              Confirm
+          </button>
+        </div>
+      </div>
+    </div>
+  </div>
+    `;
+
+    const application = Application.start();
+    application.register("modal-close", ModalCloseController);
+  });
+
+  describe("clicking cancel", function() {
+    it("sets the window location, resets the input target and disables the button", function() {
+      document.getElementById("cancel").click();
+
+      expect(window.location.href).toContain("#modal-close");
+      const inputTarget = document.getElementById("input-target");
+      expect(inputTarget.value).toEqual("");
+      const buttonTarget = document.getElementById("button-target");
+      expect(buttonTarget).toHaveAttribute("disabled");
+    });
+  });
+});

tests/unit/manage/test_forms.py

@@ -167,6 +167,19 @@ def test_creation(self):
 
         assert form.user_service is user_service
 
+    def test_validate_confirm_password(self):
+        user_service = pretend.stub(
+            find_userid=pretend.call_recorder(lambda userid: 1),
+            check_password=pretend.call_recorder(
+                lambda userid, password, tags=None: True
+            ),
+        )
+        form = forms.DeleteTOTPForm(
+            username="username", user_service=user_service, password="password"
+        )
+
+        assert form.validate()
+
 
 class TestProvisionWebAuthnForm:
     def test_creation(self):
@@ -433,16 +446,26 @@ def test_validate_token_scope_valid_project(self):
 class TestDeleteMacaroonForm:
     def test_creation(self):
         macaroon_service = pretend.stub()
-        form = forms.DeleteMacaroonForm(macaroon_service=macaroon_service)
+        user_service = pretend.stub()
+        form = forms.DeleteMacaroonForm(
+            macaroon_service=macaroon_service, user_service=user_service
+        )
 
         assert form.macaroon_service is macaroon_service
+        assert form.user_service is user_service
 
     def test_validate_macaroon_id_invalid(self):
         macaroon_service = pretend.stub(
             find_macaroon=pretend.call_recorder(lambda id: None)
         )
+        user_service = pretend.stub(
+            find_userid=lambda *a, **kw: 1, check_password=lambda *a, **kw: True
+        )
         form = forms.DeleteMacaroonForm(
-            data={"macaroon_id": pretend.stub()}, macaroon_service=macaroon_service
+            data={"macaroon_id": pretend.stub(), "password": "password"},
+            macaroon_service=macaroon_service,
+            user_service=user_service,
+            username="username",
         )
 
         assert not form.validate()
@@ -452,8 +475,14 @@ def test_validate_macaroon_id(self):
         macaroon_service = pretend.stub(
             find_macaroon=pretend.call_recorder(lambda id: pretend.stub())
         )
+        user_service = pretend.stub(
+            find_userid=lambda *a, **kw: 1, check_password=lambda *a, **kw: True
+        )
         form = forms.DeleteMacaroonForm(
-            data={"macaroon_id": pretend.stub()}, macaroon_service=macaroon_service
+            data={"macaroon_id": pretend.stub(), "password": "password"},
+            macaroon_service=macaroon_service,
+            username="username",
+            user_service=user_service,
         )
 
         assert form.validate()

tests/unit/manage/test_views.py

@@ -667,9 +667,15 @@ def test_delete_account(self, monkeypatch, db_request):
         jid = JournalEntryFactory.create(submitted_by=user).id
 
         db_request.user = user
-        db_request.params = {"confirm_username": user.username}
+        db_request.params = {"confirm_password": user.password}
         db_request.find_service = lambda *a, **kw: pretend.stub()
 
+        confirm_password_obj = pretend.stub(validate=lambda: True)
+        confirm_password_cls = pretend.call_recorder(
+            lambda *a, **kw: confirm_password_obj
+        )
+        monkeypatch.setattr(views, "ConfirmPasswordForm", confirm_password_cls)
+
         monkeypatch.setattr(
             views.ManageAccountViews, "default_response", pretend.stub()
         )
@@ -698,7 +704,7 @@ def test_delete_account(self, monkeypatch, db_request):
 
     def test_delete_account_no_confirm(self, monkeypatch):
         request = pretend.stub(
-            params={"confirm_username": ""},
+            params={"confirm_password": ""},
             session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)),
             find_service=lambda *a, **kw: pretend.stub(),
         )
@@ -716,12 +722,18 @@ def test_delete_account_no_confirm(self, monkeypatch):
 
     def test_delete_account_wrong_confirm(self, monkeypatch):
         request = pretend.stub(
-            params={"confirm_username": "invalid"},
+            params={"confirm_password": "invalid"},
             user=pretend.stub(username="username"),
             session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)),
             find_service=lambda *a, **kw: pretend.stub(),
         )
 
+        confirm_password_obj = pretend.stub(validate=lambda: False)
+        confirm_password_cls = pretend.call_recorder(
+            lambda *a, **kw: confirm_password_obj
+        )
+        monkeypatch.setattr(views, "ConfirmPasswordForm", confirm_password_cls)
+
         monkeypatch.setattr(
             views.ManageAccountViews, "default_response", pretend.stub()
         )
@@ -731,19 +743,25 @@ def test_delete_account_wrong_confirm(self, monkeypatch):
         assert view.delete_account() == view.default_response
         assert request.session.flash.calls == [
             pretend.call(
-                "Could not delete account - 'invalid' is not the same as " "'username'",
+                "Could not delete account - Invalid credentials. Please try again.",
                 queue="error",
             )
         ]
 
     def test_delete_account_has_active_projects(self, monkeypatch):
         request = pretend.stub(
-            params={"confirm_username": "username"},
+            params={"confirm_password": "password"},
             user=pretend.stub(username="username"),
             session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)),
             find_service=lambda *a, **kw: pretend.stub(),
         )
 
+        confirm_password_obj = pretend.stub(validate=lambda: True)
+        confirm_password_cls = pretend.call_recorder(
+            lambda *a, **kw: confirm_password_obj
+        )
+        monkeypatch.setattr(views, "ConfirmPasswordForm", confirm_password_cls)
+
         monkeypatch.setattr(
             views.ManageAccountViews, "default_response", pretend.stub()
         )
@@ -1081,7 +1099,7 @@ def test_delete_totp(self, monkeypatch, db_request):
             record_event=pretend.call_recorder(lambda *a, **kw: None),
         )
         request = pretend.stub(
-            POST={"confirm_username": pretend.stub()},
+            POST={"confirm_password": pretend.stub()},
             session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)),
             find_service=lambda *a, **kw: user_service,
             user=pretend.stub(
@@ -1124,13 +1142,13 @@ def test_delete_totp(self, monkeypatch, db_request):
             )
         ]
 
-    def test_delete_totp_bad_username(self, monkeypatch, db_request):
+    def test_delete_totp_bad_password(self, monkeypatch, db_request):
         user_service = pretend.stub(
             get_totp_secret=lambda id: b"secret",
             update_user=pretend.call_recorder(lambda *a, **kw: None),
         )
         request = pretend.stub(
-            POST={"confirm_username": pretend.stub()},
+            POST={"confirm_password": pretend.stub()},
             session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)),
             find_service=lambda *a, **kw: user_service,
             user=pretend.stub(
@@ -1152,7 +1170,7 @@ def test_delete_totp_bad_username(self, monkeypatch, db_request):
 
         assert user_service.update_user.calls == []
         assert request.session.flash.calls == [
-            pretend.call("Invalid credentials", queue="error")
+            pretend.call("Invalid credentials. Try again", queue="error")
         ]
         assert isinstance(result, HTTPSeeOther)
         assert result.headers["Location"] == "/foo/bar/"
@@ -1163,7 +1181,7 @@ def test_delete_totp_not_provisioned(self, monkeypatch, db_request):
             update_user=pretend.call_recorder(lambda *a, **kw: None),
         )
         request = pretend.stub(
-            POST={"confirm_username": pretend.stub()},
+            POST={"confirm_password": pretend.stub()},
             session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)),
             find_service=lambda *a, **kw: user_service,
             user=pretend.stub(
@@ -1470,7 +1488,7 @@ def test_default_response(self, monkeypatch):
         )
 
         request = pretend.stub(
-            user=pretend.stub(id=pretend.stub()),
+            user=pretend.stub(id=pretend.stub(), username=pretend.stub()),
             find_service=lambda interface, **kw: {
                 IMacaroonService: pretend.stub(),
                 IUserService: pretend.stub(),
@@ -1768,14 +1786,16 @@ def test_delete_macaroon_invalid_form(self, monkeypatch):
             delete_macaroon=pretend.call_recorder(lambda id: pretend.stub())
         )
         request = pretend.stub(
-            POST={},
+            POST={"confirm_password": "password", "macaroon_id": "macaroon_id"},
             route_path=pretend.call_recorder(lambda x: pretend.stub()),
             find_service=lambda interface, **kw: {
                 IMacaroonService: macaroon_service,
                 IUserService: pretend.stub(),
             }[interface],
             referer="/fake/safe/route",
             host=None,
+            user=pretend.stub(username=pretend.stub()),
+            session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)),
         )
 
         delete_macaroon_obj = pretend.stub(validate=lambda: False)
@@ -1791,20 +1811,25 @@ def test_delete_macaroon_invalid_form(self, monkeypatch):
         assert isinstance(result, HTTPSeeOther)
         assert result.location == "/fake/safe/route"
         assert macaroon_service.delete_macaroon.calls == []
+        assert request.session.flash.calls == [
+            pretend.call("Invalid credentials. Try again", queue="error")
+        ]
 
     def test_delete_macaroon_dangerous_redirect(self, monkeypatch):
         macaroon_service = pretend.stub(
             delete_macaroon=pretend.call_recorder(lambda id: pretend.stub())
         )
         request = pretend.stub(
-            POST={},
+            POST={"confirm_password": "password", "macaroon_id": "macaroon_id"},
             route_path=pretend.call_recorder(lambda x: "/safe/route"),
             find_service=lambda interface, **kw: {
                 IMacaroonService: macaroon_service,
                 IUserService: pretend.stub(),
             }[interface],
             referer="http://google.com/",
             host=None,
+            user=pretend.stub(username=pretend.stub()),
+            session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)),
         )
 
         delete_macaroon_obj = pretend.stub(validate=lambda: False)
@@ -1834,7 +1859,7 @@ def test_delete_macaroon(self, monkeypatch):
         )
         user_service = pretend.stub(record_event=record_event)
         request = pretend.stub(
-            POST={},
+            POST={"confirm_password": "password", "macaroon_id": "macaroon_id"},
             route_path=pretend.call_recorder(lambda x: pretend.stub()),
             find_service=lambda interface, **kw: {
                 IMacaroonService: macaroon_service,
@@ -1843,7 +1868,7 @@ def test_delete_macaroon(self, monkeypatch):
             session=pretend.stub(flash=pretend.call_recorder(lambda *a, **kw: None)),
             referer="/fake/safe/route",
             host=None,
-            user=pretend.stub(id=pretend.stub()),
+            user=pretend.stub(id=pretend.stub(), username=pretend.stub()),
             remote_addr="0.0.0.0",
         )
 
@@ -1893,7 +1918,7 @@ def test_delete_macaroon_records_events_for_each_project(self, monkeypatch):
         )
         user_service = pretend.stub(record_event=record_event)
         request = pretend.stub(
-            POST={},
+            POST={"confirm_password": pretend.stub(), "macaroon_id": pretend.stub()},
             route_path=pretend.call_recorder(lambda x: pretend.stub()),
             find_service=lambda interface, **kw: {
                 IMacaroonService: macaroon_service,