DEV: Standardize session confirmation prompt (#24212)

Switches to using a dialog to confirm a session (i.e. sudo mode for account changes where we want to be extra sure the current user is who they say they are) to match what we do with passkeys.
2025-03-28 15:25:36 +08:00 · 2023-11-07 08:26:10 -08:00 · 2023-11-07 08:26:10 -08:00 · a1c1f7ce75
commit a1c1f7ce75
parent dcaa719363
14 changed files with 249 additions and 358 deletions
--- a/app/assets/javascripts/discourse/app/controllers/preferences/second-factor.js
+++ b/app/assets/javascripts/discourse/app/controllers/preferences/second-factor.js
@ -21,17 +21,12 @@ export default Controller.extend(CanCheckEmails, {
  modal: service(),
  loading: false,
  dirty: false,
-  resetPasswordLoading: false,
-  resetPasswordProgress: "",
-  password: null,
  errorMessage: null,
  newUsername: null,
  backupEnabled: alias("model.second_factor_backup_enabled"),
  secondFactorMethod: SECOND_FACTOR_METHODS.TOTP,
  totps: null,

-  loaded: false,
-
  init() {
    this._super(...arguments);
    this.set("totps", []);
@ -47,6 +42,11 @@ export default Controller.extend(CanCheckEmails, {
    return user && user.enforcedSecondFactor;
  },

+  @discourseComputed("totps", "security_keys")
+  hasSecondFactors(totps, security_keys) {
+    return totps.length > 0 || security_keys.length > 0;
+  },
+
  @action
  handleError(error) {
    if (error.jqXHR) {
@ -81,7 +81,7 @@ export default Controller.extend(CanCheckEmails, {
    this.set("loading", true);

    this.model
-      .loadSecondFactorCodes(this.password)
+      .loadSecondFactorCodes()
      .then((response) => {
        if (response.error) {
          this.set("errorMessage", response.error);
@ -90,17 +90,10 @@ export default Controller.extend(CanCheckEmails, {

        this.setProperties({
          errorMessage: null,
-          loaded: true,
          totps: response.totps,
          security_keys: response.security_keys,
-          password: null,
          dirty: false,
        });
-        this.set(
-          "model.second_factor_enabled",
-          (response.totps && response.totps.length > 0) ||
-            (response.security_keys && response.security_keys.length > 0)
-        );
      })
      .catch((e) => this.handleError(e))
      .finally(() => this.set("loading", false));
@ -111,37 +104,7 @@ export default Controller.extend(CanCheckEmails, {
    this.set("dirty", true);
  },

-  @action
-  resetPassword(event) {
-    event?.preventDefault();
-
-    this.setProperties({
-      resetPasswordLoading: true,
-      resetPasswordProgress: "",
-    });
-
-    return this.model
-      .changePassword()
-      .then(() => {
-        this.set(
-          "resetPasswordProgress",
-          I18n.t("user.change_password.success")
-        );
-      })
-      .catch(popupAjaxError)
-      .finally(() => this.set("resetPasswordLoading", false));
-  },
-
  actions: {
-    confirmPassword() {
-      if (!this.password) {
-        return;
-      }
-      this.markDirty();
-      this.loadSecondFactors();
-      this.set("password", null);
-    },
-
    disableAllSecondFactors() {
      if (this.loading) {
        return;
--- a/app/assets/javascripts/discourse/app/controllers/preferences/security.js
+++ b/app/assets/javascripts/discourse/app/controllers/preferences/security.js
@ -2,6 +2,7 @@ import Controller from "@ember/controller";
 import { action } from "@ember/object";
 import { gt } from "@ember/object/computed";
 import { inject as service } from "@ember/service";
+import ConfirmSession from "discourse/components/dialog-messages/confirm-session";
 import AuthTokenModal from "discourse/components/modal/auth-token";
 import { ajax } from "discourse/lib/ajax";
 import { popupAjaxError } from "discourse/lib/ajax-error";
@ -17,6 +18,8 @@ const DEFAULT_AUTH_TOKENS_COUNT = 2;

 export default Controller.extend(CanCheckEmails, {
  modal: service(),
+  dialog: service(),
+  router: service(),
  passwordProgress: null,
  subpageTitle: I18n.t("user.preferences_nav.security"),
  showAllAuthTokens: false,
@ -114,6 +117,27 @@ export default Controller.extend(CanCheckEmails, {
      .catch(popupAjaxError);
  },

+  @action
+  async manage2FA() {
+    try {
+      const trustedSession = await this.model.trustedSession();
+
+      if (!trustedSession.success) {
+        this.dialog.dialog({
+          title: I18n.t("user.confirm_access.title"),
+          type: "notice",
+          bodyComponent: ConfirmSession,
+          didConfirm: () =>
+            this.router.transitionTo("preferences.second-factor"),
+        });
+      } else {
+        await this.router.transitionTo("preferences.second-factor");
+      }
+    } catch (error) {
+      popupAjaxError(error);
+    }
+  },
+
  actions: {
    save() {
      this.set("saved", false);
--- a/app/assets/javascripts/discourse/app/models/user.js
+++ b/app/assets/javascripts/discourse/app/models/user.js
@ -540,9 +540,8 @@ const User = RestModel.extend({
    });
  },

-  loadSecondFactorCodes(password) {
+  loadSecondFactorCodes() {
    return ajax("/u/second_factors.json", {
-      data: { password },
      type: "POST",
    });
  },
--- a/app/assets/javascripts/discourse/app/routes/preferences-second-factor.js
+++ b/app/assets/javascripts/discourse/app/routes/preferences-second-factor.js
@ -5,6 +5,7 @@ import RestrictedUserRoute from "discourse/routes/restricted-user";
 export default RestrictedUserRoute.extend({
  currentUser: service(),
  siteSettings: service(),
+  router: service(),

  model() {
    return this.modelFor("user");
@ -15,15 +16,15 @@ export default RestrictedUserRoute.extend({
    controller.set("loading", true);

    model
-      .loadSecondFactorCodes("")
+      .loadSecondFactorCodes()
      .then((response) => {
        if (response.error) {
          controller.set("errorMessage", response.error);
+        } else if (response.unconfirmed_session) {
+          this.router.transitionTo("preferences.security");
        } else {
          controller.setProperties({
            errorMessage: null,
-            loaded: !response.password_required,
-            dirty: !!response.password_required,
            totps: response.totps,
            security_keys: response.security_keys,
          });
--- a/app/assets/javascripts/discourse/app/templates/preferences-second-factor.hbs
+++ b/app/assets/javascripts/discourse/app/templates/preferences-second-factor.hbs
@ -19,7 +19,6 @@
        <div class="alert alert-error">{{this.errorMessage}}</div>
      {{/if}}

-      {{#if this.loaded}}
      <div class="control-group totp">
        <div class="controls">
          <h2>{{i18n "user.second_factor.totp.title"}}</h2>
@ -116,16 +115,12 @@
              </div>

              {{#if
-                  (and
-                    this.model.second_factor_backup_enabled this.isCurrentUser
-                  )
+                (and this.model.second_factor_backup_enabled this.isCurrentUser)
              }}
                <div class="actions">
                  <TwoFactorBackupDropdown
                    @secondFactorBackupEnabled={{this.model.second_factor_backup_enabled}}
-                      @editSecondFactorBackup={{action
-                        "editSecondFactorBackup"
-                      }}
+                    @editSecondFactorBackup={{action "editSecondFactorBackup"}}
                    @disableSecondFactorBackup={{action
                      "disableSecondFactorBackup"
                    }}
@ -140,7 +135,7 @@
        </div>
      </div>

-        {{#if this.model.second_factor_enabled}}
+      {{#if this.hasSecondFactors}}
        {{#unless this.showEnforcedNotice}}
          <div class="control-group pref-second-factor-disable-all">
            <div class="controls -actions">
@ -159,55 +154,6 @@
          </div>
        {{/unless}}
      {{/if}}
-      {{else}}
-        <div class="control-group">
-          <label class="control-label">{{i18n "user.password.title"}}</label>
-
-          <div class="controls">
-            <div>
-              <TextField
-                @value={{this.password}}
-                @id="password"
-                @type="password"
-                @classNames="input-large"
-                @autofocus="autofocus"
-              />
-            </div>
-            <div class="instructions">
-              {{i18n "user.second_factor.confirm_password_description"}}
-            </div>
-          </div>
-        </div>
-
-        <div class="control-group">
-          <div class="controls -actions">
-            <DButton
-              @action={{action "confirmPassword"}}
-              @disabled={{this.loading}}
-              @label="continue"
-              type="submit"
-              class="btn-primary"
-            />
-
-            {{#unless this.showEnforcedNotice}}
-              <CancelLink
-                @route="preferences.security"
-                @args={{this.model.username}}
-              />
-            {{/unless}}
-          </div>
-          <div class="controls" style="margin-top: 5px">
-            {{this.resetPasswordProgress}}
-            {{#unless this.resetPasswordLoading}}
-              <a
-                href
-                class="instructions"
-                {{on "click" this.resetPassword}}
-              >{{i18n "user.second_factor.forgot_password"}}</a>
-            {{/unless}}
-          </div>
-        </div>
-      {{/if}}
    </form>
  </ConditionalLoadingSpinner>
 </section>
--- a/app/assets/javascripts/discourse/app/templates/preferences/security.hbs
+++ b/app/assets/javascripts/discourse/app/templates/preferences/security.hbs
@ -19,28 +19,26 @@
    <UserPreferences::UserPasskeys @model={{@model}} />
  {{/if}}

+  {{#if this.isCurrentUser}}
    <div
      class="control-group pref-second-factor"
      data-setting-name="user-second-factor"
    >
      <label class="control-label">{{i18n "user.second_factor.title"}}</label>
-    {{#unless this.model.second_factor_enabled}}
      <div class="instructions">
        {{i18n "user.second_factor.short_description"}}
      </div>
-    {{/unless}}
+
      <div class="controls pref-second-factor">
-      {{#if this.isCurrentUser}}
-        <LinkTo
-          @route="preferences.second-factor"
-          class="btn btn-default btn-second-factor"
-        >
-          {{d-icon "lock"}}
-          <span>{{i18n "user.second_factor.enable"}}</span>
-        </LinkTo>
+        <DButton
+          @action={{this.manage2FA}}
+          @icon="lock"
+          @label="user.second_factor.enable"
+          class="btn-second-factor"
+        />
+      </div>
+    </div>
  {{/if}}
-    </div>
-  </div>
 {{/if}}

 {{#if this.canCheckEmails}}
--- a/app/assets/javascripts/discourse/tests/acceptance/enforce-second-factor-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/enforce-second-factor-test.js
@ -15,13 +15,13 @@ async function catchAbortedTransition() {
  }
 }

-acceptance("Enforce Second Factor", function (needs) {
+acceptance("Enforce Second Factor for unconfirmed session", function (needs) {
  needs.user();
  needs.pretender((server, helper) => {
    server.post("/u/second_factors.json", () => {
      return helper.response({
        success: "OK",
-        password_required: "true",
+        unconfirmed_session: "true",
      });
    });
  });
@ -30,22 +30,10 @@ acceptance("Enforce Second Factor", function (needs) {
    await visit("/u/eviltrout/preferences/second-factor");
    this.siteSettings.enforce_second_factor = "staff";

-    await catchAbortedTransition();
-
    assert.strictEqual(
      currentRouteName(),
-      "preferences.second-factor",
-      "it will not transition from second-factor preferences"
-    );
-
-    await click(
-      ".sidebar-section[data-section-name='community'] .sidebar-section-link[data-link-name='admin']"
-    );
-
-    assert.strictEqual(
-      currentRouteName(),
-      "preferences.second-factor",
-      "it stays at second-factor preferences"
+      "preferences.security",
+      "it transitions to security preferences"
    );
  });

@ -55,26 +43,10 @@ acceptance("Enforce Second Factor", function (needs) {
    await visit("/u/eviltrout/preferences/second-factor");
    this.siteSettings.enforce_second_factor = "all";

-    await catchAbortedTransition();
-
    assert.strictEqual(
      currentRouteName(),
-      "preferences.second-factor",
-      "it will not transition from second-factor preferences"
-    );
-
-    await click(
-      ".sidebar-section[data-section-name='community'] .sidebar-more-section-links-details-summary"
-    );
-
-    await click(
-      ".sidebar-section[data-section-name='community'] .sidebar-section-link[data-link-name='about']"
-    );
-
-    assert.strictEqual(
-      currentRouteName(),
-      "preferences.second-factor",
-      "it stays at second-factor preferences"
+      "preferences.security",
+      "it will transition to security preferences"
    );
  });

--- a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-second-factor-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-second-factor-test.js
@ -1,4 +1,4 @@
-import { click, fillIn, visit } from "@ember/test-helpers";
+import { click, currentRouteName, fillIn, visit } from "@ember/test-helpers";
 import { test } from "qunit";
 import {
  acceptance,
@ -14,7 +14,6 @@ acceptance("User Preferences - Second Factor", function (needs) {
    server.post("/u/second_factors.json", () => {
      return helper.response({
        success: "OK",
-        password_required: "true",
        totps: [{ id: 1, name: "one of them" }],
        security_keys: [{ id: 2, name: "key" }],
      });
@ -57,12 +56,6 @@ acceptance("User Preferences - Second Factor", function (needs) {
  test("second factor totp", async function (assert) {
    await visit("/u/eviltrout/preferences/second-factor");

-    assert.ok(exists("#password"), "it has a password input");
-
-    await fillIn("#password", "secrets");
-    await click(".user-preferences .btn-primary");
-    assert.notOk(exists("#password"), "it hides the password input");
-
    await click(".new-totp");
    assert.ok(exists(".qr-code img"), "shows qr code image");

@ -82,12 +75,6 @@ acceptance("User Preferences - Second Factor", function (needs) {
  test("second factor security keys", async function (assert) {
    await visit("/u/eviltrout/preferences/second-factor");

-    assert.ok(exists("#password"), "it has a password input");
-
-    await fillIn("#password", "secrets");
-    await click(".user-preferences .btn-primary");
-    assert.notOk(exists("#password"), "it hides the password input");
-
    await click(".new-security-key");
    assert.ok(exists("#security-key-name"), "shows security key name input");

@ -109,10 +96,6 @@ acceptance("User Preferences - Second Factor", function (needs) {
    updateCurrentUser({ moderator: false, admin: false, trust_level: 1 });
    await visit("/u/eviltrout/preferences/second-factor");

-    assert.ok(exists("#password"), "it has a password input");
-
-    await fillIn("#password", "secrets");
-    await click(".user-preferences .btn-primary");
    await click(".token-based-auth-dropdown .select-kit-header");
    await click("li[data-name='Disable']");

@ -147,3 +130,29 @@ acceptance("User Preferences - Second Factor", function (needs) {
    );
  });
 });
+
+acceptance(
+  "User Preferences - Second Factor - Unconfirmed Session",
+  function (needs) {
+    needs.user();
+
+    needs.pretender((server, helper) => {
+      server.post("/u/second_factors.json", () => {
+        return helper.response({
+          success: "OK",
+          unconfirmed_session: "true",
+        });
+      });
+    });
+
+    test("redirects to security preferences", async function (assert) {
+      await visit("/u/eviltrout/preferences/second-factor");
+
+      assert.strictEqual(
+        currentRouteName(),
+        "preferences.security",
+        "it transitions to security preferences"
+      );
+    });
+  }
+);
--- a/app/assets/stylesheets/common/base/user.scss
+++ b/app/assets/stylesheets/common/base/user.scss
@ -518,6 +518,23 @@
 }

 .user-preferences {
+  .form-vertical {
+    width: 500px;
+    max-width: 100%;
+
+    .control-group {
+      margin-bottom: 2em;
+    }
+  }
+
+  .category-selector,
+  .tag-chooser,
+  textarea,
+  input.user-selector,
+  .user-chooser {
+    width: 100%;
+  }
+
  textarea {
    height: 100px;
  }
@ -735,6 +752,10 @@
      }
    }
  }
+
+  .pref-second-factor {
+    margin-top: 0.5em;
+  }
 }

 .paginated-topics-list {
--- a/app/assets/stylesheets/desktop/user.scss
+++ b/app/assets/stylesheets/desktop/user.scss
@ -107,10 +107,6 @@
  }
 }

-.pref-second-factor {
-  margin-top: 10px;
-}
-
 .invite-controls .btn {
  margin-right: 0px;
 }
@ -252,21 +248,6 @@ table.user-invite-list {
  margin-right: 0.2em;
 }

-.user-preferences {
-  .form-vertical {
-    width: 500px;
-    max-width: 100%;
-  }
-
-  .category-selector,
-  .tag-chooser,
-  textarea,
-  input.user-selector,
-  .user-chooser {
-    width: 100%;
-  }
-}
-
 .user-crawler {
  .username {
    margin-left: 5px;
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@ -1533,12 +1533,6 @@ class UsersController < ApplicationController
      raise Discourse::NotFound
    end

-    if params[:password].present?
-      if !confirm_secure_session
-        return render json: failed_json.merge(error: I18n.t("login.incorrect_password"))
-      end
-    end
-
    if secure_session_confirmed?
      totp_second_factors =
        current_user
@ -1555,7 +1549,7 @@ class UsersController < ApplicationController

      render json: success_json.merge(totps: totp_second_factors, security_keys: security_keys)
    else
-      render json: success_json.merge(password_required: true)
+      render json: success_json.merge(unconfirmed_session: true)
    end
  end

--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@ -1463,8 +1463,6 @@ en:
        title: "Two-Factor Authentication"
        enable: "Manage Two-Factor Authentication"
        disable_all: "Disable All"
-        forgot_password: "Forgot password?"
-        confirm_password_description: "Please confirm your password to continue"
        name: "Name"
        label: "Code"
        rate_limit: "Please wait before trying another authentication code."
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@ -6322,23 +6322,18 @@ RSpec.describe UsersController do
        SiteSetting.enable_discourse_connect = false
      end

-      context "when the password parameter is not provided" do
-        let(:password) { "" }
+      context "when the session is unconfirmed" do
+        it "returns unconfirmed session response" do
+          post "/u/second_factors.json"

-        before { post "/u/second_factors.json", params: { password: password } }
-
-        it "returns password required response" do
          expect(response.status).to eq(200)
          response_body = response.parsed_body
-          expect(response_body["password_required"]).to eq(true)
+          expect(response_body["unconfirmed_session"]).to eq(true)
        end
      end

-      context "when the password is provided" do
-        fab!(:user) { Fabricate(:user, password: "8555039dd212cc66ec68") }
-
-        context "when the password is correct" do
-          let(:password) { "8555039dd212cc66ec68" }
+      context "when the session is confirmed" do
+        fab!(:user) { Fabricate(:user, password: "acoolpassword") }

        it "returns a list of enabled totps and security_key second factors" do
          totp_second_factor = Fabricate(:user_second_factor_totp, user: user)
@ -6349,7 +6344,9 @@ RSpec.describe UsersController do
              factor_type: UserSecurityKey.factor_types[:second_factor],
            )

-            post "/u/second_factors.json", params: { password: password }
+          post "/u/confirm-session.json", params: { password: "acoolpassword" }
+
+          post "/u/second_factors.json"

          expect(response.status).to eq(200)
          response_body = response.parsed_body
@ -6361,18 +6358,6 @@ RSpec.describe UsersController do
          ).to include(security_key_second_factor.id)
        end
      end
-
-        context "when the password is not correct" do
-          let(:password) { "wrongpassword" }
-
-          it "returns the incorrect password response" do
-            post "/u/second_factors.json", params: { password: password }
-
-            response_body = response.parsed_body
-            expect(response_body["error"]).to eq(I18n.t("login.incorrect_password"))
-          end
-        end
-      end
    end
  end

--- a/spec/system/page_objects/pages/user_preferences_security.rb
+++ b/spec/system/page_objects/pages/user_preferences_security.rb
@ -9,10 +9,10 @@ module PageObjects
      end

      def visit_second_factor(password)
-        click_link(class: "btn-second-factor")
+        click_button "Manage Two-Factor Authentication"

-        find(".second-factor input#password").fill_in(with: password)
-        find(".second-factor .btn-primary").click
+        find(".dialog-body input#password").fill_in(with: password)
+        find(".dialog-body .btn-primary").click
      end
    end
  end