[OP-19885] User cannot update own profile because password confirmation autosaves

https://community.openproject.org/wp/OP-19885

Author: oliverguenther

frontend/src/app/shared/components/editor/components/ckeditor-augmented-textarea/ckeditor-augmented-textarea.component.spec.ts

@@ -73,4 +73,36 @@ describe('CkeditorAugmentedTextareaComponent', () => {
 
     expect(sync).toHaveBeenCalledTimes(1);
   });
+
+  describe('form submit interception', () => {
+    let form:HTMLFormElement;
+    let sync:ReturnType<typeof vi.spyOn>;
+    let saveForm:ReturnType<typeof vi.spyOn>;
+
+    beforeEach(() => {
+      form = document.createElement('form');
+      component.formElement = form;
+      sync = vi.spyOn(component, 'syncToTextarea').mockImplementation(() => undefined);
+      saveForm = vi.spyOn(component, 'saveForm').mockResolvedValue(undefined);
+      (component as unknown as { registerFormSubmitListener():void }).registerFormSubmitListener();
+    });
+
+    it('delegates to saveForm when submit is not already prevented', () => {
+      const event = new SubmitEvent('submit', { cancelable: true, bubbles: true });
+      form.dispatchEvent(event);
+
+      expect(event.defaultPrevented).toBe(true);
+      expect(saveForm).toHaveBeenCalledWith(event);
+      expect(sync).not.toHaveBeenCalled();
+    });
+
+    it('only syncs when another handler already prevented default', () => {
+      const event = new SubmitEvent('submit', { cancelable: true, bubbles: true });
+      event.preventDefault();
+      form.dispatchEvent(event);
+
+      expect(saveForm).not.toHaveBeenCalled();
+      expect(sync).toHaveBeenCalledTimes(1);
+    });
+  });
 });

frontend/src/app/shared/components/editor/components/ckeditor-augmented-textarea/ckeditor-augmented-textarea.component.ts

@@ -178,6 +178,15 @@ export class CkeditorAugmentedTextareaComponent extends UntilDestroyedMixin impl
         this.untilDestroyed(),
       )
       .subscribe((evt:SubmitEvent) => {
+        // Another handler (e.g. require-password-confirmation) already owns this
+        // submit. Still flush editor → textarea so a later confirmed submit has
+        // the latest content, but do not re-submit — Turbo's navigator.submitForm
+        // would bypass that other handler and POST without confirmation.
+        if (evt.defaultPrevented) {
+          this.syncToTextarea();
+          return;
+        }
+
         evt.preventDefault();
         void this.saveForm(evt);
       });
@@ -214,7 +223,11 @@ export class CkeditorAugmentedTextareaComponent extends UntilDestroyedMixin impl
         (evt.submitter as HTMLInputElement).disabled = false;
       }
 
-      if (this.turboMode && !this.formElement.dataset.action) {
+      // Honor the form's data-turbo="false" even when this component was created
+      // with turboMode (Primer rich_text_area default). Turbo's submitForm skips
+      // the submit event and would bypass other submit interceptors.
+      const turboDisabled = this.formElement.dataset.turbo === 'false';
+      if (this.turboMode && !turboDisabled && !this.formElement.dataset.action) {
         navigator.submitForm(this.formElement, evt?.submitter ?? undefined);
       } else {
         this.formElement.requestSubmit(evt?.submitter);

frontend/src/stimulus/controllers/require-password-confirmation.controller.spec.ts

@@ -102,6 +102,21 @@ describe('Require password confirmation controller', () => {
     });
   });
 
+  it('intercepts submit in the capture phase before bubble listeners', async () => {
+    const form = await renderForm();
+    const bubbleOrder:string[] = [];
+
+    form.addEventListener('submit', (event) => {
+      bubbleOrder.push(`bubble:prevented=${event.defaultPrevented}`);
+    });
+
+    const event = new SubmitEvent('submit', { cancelable: true, bubbles: true });
+    form.dispatchEvent(event);
+
+    expect(event.defaultPrevented).toBe(true);
+    expect(bubbleOrder).toEqual(['bubble:prevented=true']);
+  });
+
   it('appends the confirmed password and resubmits the form', async () => {
     const form = await renderForm();
     const requestSubmit = vi.fn();

frontend/src/stimulus/controllers/require-password-confirmation.controller.ts

@@ -54,7 +54,10 @@ export default class RequirePasswordConfirmationController extends ApplicationCo
   connect() {
     super.connect();
 
-    this.element.addEventListener('submit', this.formListener);
+    // Capture phase so we run before other submit interceptors on the same form
+    // (notably CKEditor-augmented textareas), which can otherwise re-submit via
+    // Turbo and bypass the confirmation dialog.
+    this.element.addEventListener('submit', this.formListener, { capture: true });
     document.addEventListener('password-confirmation-dialog:close', this.dialogCloseListener);
     document.addEventListener('password-confirmation-dialog:submit', this.dialogSubmitListener);
 
@@ -66,7 +69,7 @@ export default class RequirePasswordConfirmationController extends ApplicationCo
   disconnect() {
     super.disconnect();
 
-    this.element.removeEventListener('submit', this.formListener);
+    this.element.removeEventListener('submit', this.formListener, { capture: true });
     document.removeEventListener('password-confirmation-dialog:close', this.dialogCloseListener);
     document.removeEventListener('password-confirmation-dialog:submit', this.dialogSubmitListener);
   }

spec/features/users/my_spec.rb

@@ -143,44 +143,77 @@ def expect_changed!
     describe "#account" do
       let(:dialog) { Components::PasswordConfirmationDialog.new }
 
-      before do
-        visit my_account_path
+      context "when updating profile fields" do
+        before do
+          visit my_account_path
+
+          fill_in "user[mail]", with: "foo@mail.com"
+          fill_in "user[firstname]", with: "Foo"
+          fill_in "user[lastname]", with: "Bar"
+          click_on "Update profile"
+        end
 
-        fill_in "user[mail]", with: "foo@mail.com"
-        fill_in "user[firstname]", with: "Foo"
-        fill_in "user[lastname]", with: "Bar"
-        click_on "Update profile"
-      end
+        context "when confirmation disabled",
+                with_config: { internal_password_confirmation: false } do
+          it "does not request confirmation" do
+            expect_changed!
+          end
+        end
 
-      context "when confirmation disabled",
-              with_config: { internal_password_confirmation: false } do
-        it "does not request confirmation" do
-          expect_changed!
+        context "when confirmation required",
+                with_config: { internal_password_confirmation: true } do
+          it "requires the password for a regular user" do
+            dialog.confirm_flow_with(user_password)
+            expect_changed!
+          end
+
+          it "declines the change when invalid password is given" do
+            dialog.confirm_flow_with("#{user_password}INVALID", should_fail: true)
+
+            user.reload
+            expect(user.mail).to eq("old@mail.com")
+          end
+
+          context "as admin" do
+            shared_let(:admin) { create(:admin) }
+            let(:user) { admin }
+
+            it "requires the password" do
+              dialog.confirm_flow_with("adminADMIN!")
+              expect_changed!
+            end
+          end
         end
       end
 
-      context "when confirmation required",
+      # CKEditor-augmented text custom fields also intercept submit (to flush
+      # editor → textarea). With turboMode they used to call Turbo's
+      # navigator.submitForm, which bypassed password confirmation and POSTed
+      # immediately — flashing notice_password_confirmation_failed.
+      context "with a long text custom field",
               with_config: { internal_password_confirmation: true } do
-        it "requires the password for a regular user" do
-          dialog.confirm_flow_with(user_password)
-          expect_changed!
-        end
+        let!(:text_cf) { create(:user_custom_field, :text, name: "Biography") }
+        let(:editor) { Components::WysiwygEditor.new("[data-test-selector='#{text_cf.attribute_name(:kebab_case)}']") }
 
-        it "declines the change when invalid password is given" do
-          dialog.confirm_flow_with(user_password + "INVALID", should_fail: true)
+        it "still requires password confirmation and does not submit without it" do
+          visit my_account_path
 
-          user.reload
-          expect(user.mail).to eq("old@mail.com")
-        end
+          editor.expect_value("")
+          editor.set_markdown("Loves hiking")
 
-        context "as admin" do
-          shared_let(:admin) { create(:admin) }
-          let(:user) { admin }
+          fill_in "user[mail]", with: "foo@mail.com"
+          fill_in "user[firstname]", with: "Foo"
+          fill_in "user[lastname]", with: "Bar"
+          click_on "Update profile"
 
-          it "requires the password" do
-            dialog.confirm_flow_with("adminADMIN!")
-            expect_changed!
-          end
+          dialog.expect_open
+          expect(page).to have_no_text(I18n.t(:notice_password_confirmation_failed))
+
+          dialog.confirm_flow_with(user_password)
+          expect_changed!
+
+          user.reload
+          expect(user.typed_custom_value_for(text_cf)).to include("Loves hiking")
         end
       end
     end