diff --git a/tests/frontend/confirm_controller_test.js b/tests/frontend/confirm_controller_test.js index 21552dbdfb29..3f2b9b294705 100644 --- a/tests/frontend/confirm_controller_test.js +++ b/tests/frontend/confirm_controller_test.js @@ -22,23 +22,18 @@ describe("Confirm controller", () => { document.body.innerHTML = ` `; @@ -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" } }); diff --git a/tests/frontend/modal_close_controller_test.js b/tests/frontend/modal_close_controller_test.js new file mode 100644 index 000000000000..3ca8f1994631 --- /dev/null +++ b/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 = ` + + `; + + 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"); + }); + }); +}); diff --git a/tests/unit/manage/test_forms.py b/tests/unit/manage/test_forms.py index 7297b8fb4f83..7b2c38dd5d68 100644 --- a/tests/unit/manage/test_forms.py +++ b/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() diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index e0f72622944d..80c1b5392b13 100644 --- a/tests/unit/manage/test_views.py +++ b/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,7 +1786,7 @@ 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, @@ -1776,6 +1794,8 @@ def test_delete_macaroon_invalid_form(self, monkeypatch): }[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,13 +1811,16 @@ 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, @@ -1805,6 +1828,8 @@ def test_delete_macaroon_dangerous_redirect(self, monkeypatch): }[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, diff --git a/warehouse/manage/forms.py b/warehouse/manage/forms.py index 64dcf92ee79e..be196aebc3bc 100644 --- a/warehouse/manage/forms.py +++ b/warehouse/manage/forms.py @@ -87,15 +87,20 @@ def __init__(self, *args, user_service, **kwargs): self.user_service = user_service -class DeleteTOTPForm(UsernameMixin, forms.Form): +class ConfirmPasswordForm(UsernameMixin, PasswordMixin, forms.Form): - __params__ = ["confirm_username"] + __params__ = ["confirm_password"] def __init__(self, *args, user_service, **kwargs): super().__init__(*args, **kwargs) self.user_service = user_service +class DeleteTOTPForm(ConfirmPasswordForm): + # TODO: delete? + pass + + class ProvisionTOTPForm(TOTPValueMixin, forms.Form): __params__ = ["totp_value"] @@ -246,15 +251,16 @@ def validate_token_scope(self, field): self.validated_scope = {"projects": [scope_value]} -class DeleteMacaroonForm(forms.Form): - __params__ = ["macaroon_id"] +class DeleteMacaroonForm(UsernameMixin, PasswordMixin, forms.Form): + __params__ = ["confirm_password", "macaroon_id"] macaroon_id = wtforms.StringField( validators=[wtforms.validators.DataRequired(message="Identifier required")] ) - def __init__(self, *args, macaroon_service, **kwargs): + def __init__(self, *args, macaroon_service, user_service, **kwargs): super().__init__(*args, **kwargs) + self.user_service = user_service self.macaroon_service = macaroon_service def validate_macaroon_id(self, field): diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index f25e0ce60921..f98d2a46ee8c 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -45,6 +45,7 @@ AddEmailForm, ChangePasswordForm, ChangeRoleForm, + ConfirmPasswordForm, CreateMacaroonForm, CreateRoleForm, DeleteMacaroonForm, @@ -309,18 +310,22 @@ def change_password(self): return {**self.default_response, "change_password_form": form} - @view_config(request_method="POST", request_param=["confirm_username"]) + @view_config(request_method="POST", request_param=DeleteTOTPForm.__params__) def delete_account(self): - username = self.request.params.get("confirm_username") - - if not username: + confirm_password = self.request.params.get("confirm_password") + if not confirm_password: self.request.session.flash("Confirm the request", queue="error") return self.default_response - if username != self.request.user.username: + form = ConfirmPasswordForm( + password=confirm_password, + username=self.request.user.username, + user_service=self.user_service, + ) + + if not form.validate(): self.request.session.flash( - f"Could not delete account - {username!r} is not the same as " - f"{self.request.user.username!r}", + f"Could not delete account - Invalid credentials. Please try again.", queue="error", ) return self.default_response @@ -475,7 +480,7 @@ def delete_totp(self): return HTTPSeeOther(self.request.route_path("manage.account")) form = DeleteTOTPForm( - **self.request.POST, + password=self.request.POST["confirm_password"], username=self.request.user.username, user_service=self.user_service, ) @@ -494,7 +499,7 @@ def delete_totp(self): queue="success", ) else: - self.request.session.flash("Invalid credentials", queue="error") + self.request.session.flash("Invalid credentials. Try again", queue="error") return HTTPSeeOther(self.request.route_path("manage.account")) @@ -636,7 +641,9 @@ def default_response(self): project_names=self.project_names, ), "delete_macaroon_form": DeleteMacaroonForm( - macaroon_service=self.macaroon_service + username=self.request.user.username, + user_service=self.user_service, + macaroon_service=self.macaroon_service, ), } @@ -704,7 +711,11 @@ def create_macaroon(self): @view_config(request_method="POST", request_param=DeleteMacaroonForm.__params__) def delete_macaroon(self): form = DeleteMacaroonForm( - **self.request.POST, macaroon_service=self.macaroon_service + password=self.request.POST["confirm_password"], + macaroon_id=self.request.POST["macaroon_id"], + macaroon_service=self.macaroon_service, + username=self.request.user.username, + user_service=self.user_service, ) if form.validate(): @@ -735,6 +746,8 @@ def delete_macaroon(self): self.request.session.flash( f"Deleted API token '{macaroon.description}'.", queue="success" ) + else: + self.request.session.flash("Invalid credentials. Try again", queue="error") redirect_to = self.request.referer if not is_safe_url(redirect_to, host=self.request.host): diff --git a/warehouse/static/js/warehouse/controllers/confirm_controller.js b/warehouse/static/js/warehouse/controllers/confirm_controller.js index a008a558ca8a..9c18c9349f37 100644 --- a/warehouse/static/js/warehouse/controllers/confirm_controller.js +++ b/warehouse/static/js/warehouse/controllers/confirm_controller.js @@ -22,14 +22,6 @@ export default class extends Controller { this.buttonTarget.disabled = true; } - cancel() { - // Cancel button is a button (not an `a`) so we need to do close the - // modal manually - window.location.href = "#modal-close"; - this.inputTarget.value = ""; - this.buttonTarget.disabled = true; - } - check() { if (this.inputTarget.value == this.buttonTarget.dataset.expected) { this.buttonTarget.disabled = false; diff --git a/warehouse/static/js/warehouse/controllers/confirm_password_controller.js b/warehouse/static/js/warehouse/controllers/confirm_password_controller.js new file mode 100644 index 000000000000..518989a25c96 --- /dev/null +++ b/warehouse/static/js/warehouse/controllers/confirm_password_controller.js @@ -0,0 +1,33 @@ +/** + * 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. + */ + + +import { Controller } from "stimulus"; + +export default class extends Controller { + static targets = [ "button", "password", "showPassword" ] + + connect() { + this.buttonTarget.disabled = true; + this.setPasswordVisibility(); + } + + setPasswordVisibility() { + this.passwordTarget.type = this.showPasswordTarget.checked ? "text" : "password"; + } + + check() { + this.buttonTarget.disabled = this.passwordTarget.value.trim() === ""; + } +} diff --git a/warehouse/static/js/warehouse/controllers/modal_close_controller.js b/warehouse/static/js/warehouse/controllers/modal_close_controller.js new file mode 100644 index 000000000000..e8e4832d2b5d --- /dev/null +++ b/warehouse/static/js/warehouse/controllers/modal_close_controller.js @@ -0,0 +1,28 @@ +/** + * 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. + */ + + +import { Controller } from "stimulus"; + +export default class extends Controller { + static targets = [ "input", "button" ] + + cancel() { + // Cancel button is a button (not an `a`) so we need to do close the + // modal manually + window.location.href = "#modal-close"; + this.inputTarget.value = ""; + this.buttonTarget.disabled = true; + } +} diff --git a/warehouse/static/sass/blocks/_modal.scss b/warehouse/static/sass/blocks/_modal.scss index 70962bce24d9..44e1309fcee9 100644 --- a/warehouse/static/sass/blocks/_modal.scss +++ b/warehouse/static/sass/blocks/_modal.scss @@ -100,6 +100,11 @@ &__form { label { font-weight: bold; + + input { + width: auto; + min-width: auto; + } } input { diff --git a/warehouse/templates/manage/account.html b/warehouse/templates/manage/account.html index 5651e500ffc3..192b628e3084 100644 --- a/warehouse/templates/manage/account.html +++ b/warehouse/templates/manage/account.html @@ -213,9 +213,7 @@ {% set token_warning_text %}

{% trans %}Applications or scripts using this token will no longer have access to PyPI.{% endtrans %}

{% endset %} - - {{ confirm_modal(title=title, confirm_name=gettext("Username"), confirm_string=user.username, confirm_button_label=confirm_button_label, slug=slug, extra_fields=extra_fields, action=action, custom_warning_text=token_warning_text, confirm_string_in_title=False) }} - + {{ confirm_password_modal(title=title, confirm_button_label=confirm_button_label, slug=slug, extra_fields=extra_fields, action=action, custom_warning_text=token_warning_text) }} {# modal to view token ID #}