diff --git a/app/assets/javascripts/discourse/app/components/dialog-messages/confirm-session.gjs b/app/assets/javascripts/discourse/app/components/dialog-messages/confirm-session.gjs index 27806d7dba0..172552a86b5 100644 --- a/app/assets/javascripts/discourse/app/components/dialog-messages/confirm-session.gjs +++ b/app/assets/javascripts/discourse/app/components/dialog-messages/confirm-session.gjs @@ -6,11 +6,17 @@ import { inject as service } from "@ember/service"; import DButton from "discourse/components/d-button"; import UserLink from "discourse/components/user-link"; import { ajax } from "discourse/lib/ajax"; +import { popupAjaxError } from "discourse/lib/ajax-error"; +import { + getPasskeyCredential, + isWebauthnSupported, +} from "discourse/lib/webauthn"; import I18n from "discourse-i18n"; export default class ConfirmSession extends Component { @service dialog; @service currentUser; + @service siteSettings; @tracked errorMessage; @@ -19,8 +25,48 @@ export default class ConfirmSession extends Component { loggedInAs = I18n.t("user.confirm_access.logged_in_as"); finePrint = I18n.t("user.confirm_access.fine_print"); + get canUsePasskeys() { + return ( + this.siteSettings.enable_local_logins && + this.siteSettings.enable_passkeys && + this.currentUser.user_passkeys?.length > 0 && + isWebauthnSupported() + ); + } + + @action + async confirmWithPasskey() { + try { + const publicKeyCredential = await getPasskeyCredential((e) => + this.dialog.alert(e) + ); + + if (!publicKeyCredential) { + return; + } + + const result = await ajax("/u/confirm-session", { + type: "POST", + data: { publicKeyCredential }, + }); + + if (result.success) { + this.errorMessage = null; + this.dialog.didConfirmWrapped(); + } else { + this.errorMessage = I18n.t("user.confirm_access.incorrect_passkey"); + } + } catch (e) { + popupAjaxError(e); + } + } + @action async submit() { + this.errorMessage = this.password + ? null + : I18n.t("user.confirm_access.incorrect_password"); + const result = await ajax("/u/confirm-session", { type: "POST", data: { @@ -73,6 +119,15 @@ export default class ConfirmSession extends Component { @label="user.password.confirm" /> + {{#if this.canUsePasskeys}} +
+ +
+ {{/if}} diff --git a/app/assets/javascripts/discourse/app/components/modal/login.js b/app/assets/javascripts/discourse/app/components/modal/login.js index f745ca60315..4c003e547d3 100644 --- a/app/assets/javascripts/discourse/app/components/modal/login.js +++ b/app/assets/javascripts/discourse/app/components/modal/login.js @@ -118,28 +118,10 @@ export default class Login extends Component { @action async passkeyLogin(mediation = "optional") { try { - // we need to check isConditionalMediationAvailable for Firefox - // without it, Firefox will throw console errors - // We cannot do a general check because iOS Safari and Chrome in Selenium quietly support the feature - // but they do not support the PublicKeyCredential.isConditionalMediationAvailable() method - if ( - mediation === "conditional" && - this.capabilities.isFirefox && - window.PublicKeyCredential - ) { - const isCMA = - // eslint-disable-next-line no-undef - await PublicKeyCredential.isConditionalMediationAvailable(); - if (!isCMA) { - return; - } - } - const response = await ajax("/session/passkey/challenge.json"); - const publicKeyCredential = await getPasskeyCredential( - response.challenge, - (errorMessage) => this.dialog.alert(errorMessage), - mediation + (e) => this.dialog.alert(e), + mediation, + this.capabilities.isFirefox ); if (publicKeyCredential) { diff --git a/app/assets/javascripts/discourse/app/components/user-preferences/user-passkeys.gjs b/app/assets/javascripts/discourse/app/components/user-preferences/user-passkeys.gjs index ef941f1fdd1..93392353e91 100644 --- a/app/assets/javascripts/discourse/app/components/user-preferences/user-passkeys.gjs +++ b/app/assets/javascripts/discourse/app/components/user-preferences/user-passkeys.gjs @@ -9,7 +9,11 @@ import PasskeyOptionsDropdown from "discourse/components/user-preferences/passke import RenamePasskey from "discourse/components/user-preferences/rename-passkey"; import formatDate from "discourse/helpers/format-date"; import { popupAjaxError } from "discourse/lib/ajax-error"; -import { bufferToBase64, stringToBuffer } from "discourse/lib/webauthn"; +import { + bufferToBase64, + stringToBuffer, + WebauthnAbortHandler, +} from "discourse/lib/webauthn"; import I18n from "discourse-i18n"; export default class UserPasskeys extends Component { @@ -71,6 +75,7 @@ export default class UserPasskeys extends Component { const credential = await navigator.credentials.create({ publicKey: publicKeyCredentialCreationOptions, + signal: WebauthnAbortHandler.signal(), }); let credentialParam = { diff --git a/app/assets/javascripts/discourse/app/lib/webauthn.js b/app/assets/javascripts/discourse/app/lib/webauthn.js index f6ed1613be1..8b453c19027 100644 --- a/app/assets/javascripts/discourse/app/lib/webauthn.js +++ b/app/assets/javascripts/discourse/app/lib/webauthn.js @@ -1,3 +1,6 @@ +/* global PublicKeyCredential */ + +import { ajax } from "discourse/lib/ajax"; import I18n from "discourse-i18n"; export function stringToBuffer(str) { @@ -36,7 +39,7 @@ class WebauthnAbortService { // Need to use a singleton here to reset the active webauthn ceremony // Inspired by the BaseWebAuthnAbortService in https://github.com/MasterKale/SimpleWebAuthn -const WebauthnAbortHandler = new WebauthnAbortService(); +export const WebauthnAbortHandler = new WebauthnAbortService(); export function getWebauthnCredential( challenge, @@ -117,18 +120,31 @@ export function getWebauthnCredential( } export async function getPasskeyCredential( - challenge, errorCallback, - mediation = "optional" + mediation = "optional", + isFirefox = false ) { if (!isWebauthnSupported()) { return errorCallback(I18n.t("login.security_key_support_missing_error")); } + // we need to check isConditionalMediationAvailable for Firefox + // without it, Firefox will throw console errors + // We cannot do a general check because iOS Safari and Chrome in Selenium quietly support the feature + // but they do not support the PublicKeyCredential.isConditionalMediationAvailable() method + if (mediation === "conditional" && isFirefox) { + const isCMA = await PublicKeyCredential.isConditionalMediationAvailable(); + if (!isCMA) { + return; + } + } + try { + const resp = await ajax("/session/passkey/challenge.json"); + const credential = await navigator.credentials.get({ publicKey: { - challenge: stringToBuffer(challenge), + challenge: stringToBuffer(resp.challenge), // https://www.w3.org/TR/webauthn-2/#user-verification // for passkeys (first factor), user verification should be marked as required // it ensures browser requests PIN or biometrics before authenticating diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-security-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-security-test.js index 6d9587e9265..e256796121e 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-security-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-security-test.js @@ -136,6 +136,14 @@ acceptance("User Preferences - Security", function (needs) { "displays a dialog to confirm the user's identity before adding a passkey" ); + assert + .dom(".dialog-body #password") + .exists("dialog includes a password field"); + + assert + .dom(".dialog-body .confirm-session__passkey") + .exists("dialog includes a passkey button"); + await click(".dialog-close"); const dropdown = selectKit(".passkey-options-dropdown"); @@ -176,6 +184,12 @@ acceptance("User Preferences - Security", function (needs) { .exists( "displays a dialog to confirm the user's identity before adding a passkey" ); + + assert.dom(".dialog-body #password").exists("includes a password field"); + + assert + .dom(".dialog-body .confirm-session__passkey") + .doesNotExist("does not include a passkey button"); }); test("Viewing Passkeys - another user has a key", async function (assert) { diff --git a/app/assets/stylesheets/common/base/modal.scss b/app/assets/stylesheets/common/base/modal.scss index 6113d9260e6..adaf8e0a52f 100644 --- a/app/assets/stylesheets/common/base/modal.scss +++ b/app/assets/stylesheets/common/base/modal.scss @@ -803,6 +803,11 @@ margin: 1.5em 0; } + &__passkey .btn { + padding-left: 0.25em; + padding-right: 0.25em; + } + &__fine-print { font-size: var(--font-down-1); color: var(--primary-medium); diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 9125572906b..099943fcc1a 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1510,9 +1510,6 @@ class UsersController < ApplicationController end def confirm_session - # TODO(pmusaraj): add support for confirming via passkey, 2FA - params.require(:password) - if SiteSetting.enable_discourse_connect || !SiteSetting.enable_local_logins raise Discourse::NotFound end @@ -1520,8 +1517,10 @@ class UsersController < ApplicationController if confirm_secure_session render json: success_json else - render json: failed_json.merge(error: I18n.t("login.incorrect_password")) + render json: failed_json.merge(error: I18n.t("login.incorrect_password_or_passkey")) end + rescue ::DiscourseWebauthn::SecurityKeyError => err + render_json_error(err.message, status: 401) end def trusted_session @@ -2183,13 +2182,32 @@ class UsersController < ApplicationController SiteSetting.max_logins_per_ip_per_minute, 1.minute, ).performed! - return false if !current_user.confirm_password?(params[:password]) - secure_session["confirmed-password-#{current_user.id}"] = "true" + if !params[:password].present? && !params[:publicKeyCredential].present? + raise Discourse::InvalidParameters.new "Missing password or passkey" + end + + if params[:password].present? + return false if !current_user.confirm_password?(params[:password]) + end + + if params[:publicKeyCredential].present? + passkey = + ::DiscourseWebauthn::AuthenticationService.new( + current_user, + params[:publicKeyCredential], + session: secure_session, + factor_type: UserSecurityKey.factor_types[:first_factor], + ).authenticate_security_key + + return false if !passkey + end + + secure_session["confirmed-session-#{current_user.id}"] = "true" end def secure_session_confirmed? - secure_session["confirmed-password-#{current_user.id}"] == "true" + secure_session["confirmed-session-#{current_user.id}"] == "true" end def summary_cache_key(user) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index d18a140be92..b0cf155adb7 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1526,6 +1526,7 @@ en: never_used: "Never Used" not_allowed_error: "The passkey registration process either timed out, was cancelled or is not allowed." already_added_error: "You have already registered this passkey. You don’t have to register it again." + confirm_button: "or use a passkey" change_about: title: "Change About Me" @@ -1845,6 +1846,7 @@ en: confirm_access: title: "Confirm access" incorrect_password: "The entered password is incorrect." + incorrect_passkey: "That passkey is incorrect." logged_in_as: "You are logged in as: " instructions: "Please confirm your identity in order to complete this action." fine_print: "We are asking you to confirm your identity because this is a potentially sensitive action. Once authenticated, you will only be asked to re-authenticate again after a few hours of inactivity." diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 39d75b910a7..25165f10d00 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2676,6 +2676,7 @@ en: not_approved: "Your account hasn't been approved yet. You will be notified by email when you are ready to log in." incorrect_username_email_or_password: "Incorrect username, email or password" incorrect_password: "Incorrect password" + incorrect_password_or_passkey: "Incorrect password or passkey" wait_approval: "Thanks for signing up. We will notify you when your account has been approved." active: "Your account is activated and ready to use." activate_email: "

You’re almost done! We sent an activation mail to %{email}. Please follow the instructions in the mail to activate your account.

If it doesn’t arrive, check your spam folder.

" diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 6417855581d..8275ddd95a8 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5447,7 +5447,7 @@ RSpec.describe UsersController do ApplicationController .any_instance .expects(:secure_session) - .returns("confirmed-password-#{user1.id}" => "false") + .returns("confirmed-session-#{user1.id}" => "false") post "/users/create_second_factor_totp.json" expect(response.status).to eq(403) @@ -5478,7 +5478,7 @@ RSpec.describe UsersController do ApplicationController .any_instance .stubs(:secure_session) - .returns("confirmed-password-#{user1.id}" => "true") + .returns("confirmed-session-#{user1.id}" => "true") post "/users/create_second_factor_totp.json" expect(response.status).to eq(200) @@ -5704,7 +5704,7 @@ RSpec.describe UsersController do ApplicationController .any_instance .stubs(:secure_session) - .returns("confirmed-password-#{user1.id}" => "true") + .returns("confirmed-session-#{user1.id}" => "true") end it "should allow second factor backup for the user to be disabled" do put "/users/second_factor.json", @@ -5744,7 +5744,7 @@ RSpec.describe UsersController do ApplicationController .any_instance .expects(:secure_session) - .returns("confirmed-password-#{user1.id}" => "false") + .returns("confirmed-session-#{user1.id}" => "false") put "/users/second_factors_backup.json" expect(response.status).to eq(403) @@ -5775,7 +5775,7 @@ RSpec.describe UsersController do ApplicationController .any_instance .expects(:secure_session) - .returns("confirmed-password-#{user1.id}" => "true") + .returns("confirmed-session-#{user1.id}" => "true") put "/users/second_factors_backup.json" @@ -6388,18 +6388,25 @@ RSpec.describe UsersController do end end - context "when the site settings allow second factors" do + context "when the site settings allow local logins" do before do SiteSetting.enable_local_logins = true SiteSetting.enable_discourse_connect = false end - context "when the password is wrong" do - it "returns incorrect password response" do + context "when params are incorrect" do + it "returns 400 response if no password or passkey is supplied" do + post "/u/confirm-session.json" + + expect(response.status).to eq(400) + expect(response.parsed_body["errors"][0]).to include("Missing password or passkey") + end + + it "returns incorrect response on a wrong password" do post "/u/confirm-session.json", params: { password: password } expect(response.status).to eq(200) - expect(response.parsed_body["error"]).to eq("Incorrect password") + expect(response.parsed_body["error"]).to eq("Incorrect password or passkey") end end @@ -6413,6 +6420,69 @@ RSpec.describe UsersController do expect(response.parsed_body["error"]).to eq(nil) end end + + context "with an invalid passkey" do + it "returns invalid response" do + post "/u/confirm-session.json", params: { publicKeyCredential: "someboringstring" } + + expect(response.status).to eq(401) + + json = response.parsed_body + expect(json["errors"][0]).to eq( + I18n.t("webauthn.validation.malformed_public_key_credential_error"), + ) + end + end + + context "with a valid passkey" do + fab!(:user2) { Fabricate(:user) } + let!(:passkey) do + Fabricate( + :user_security_key, + credential_id: valid_passkey_data[:credential_id], + public_key: valid_passkey_data[:public_key], + user: user1, + factor_type: UserSecurityKey.factor_types[:first_factor], + last_used: nil, + name: "passkey", + ) + end + + it "returns a successful response for the correct user" do + simulate_localhost_passkey_challenge + user1.create_or_fetch_secure_identifier + + post "/u/confirm-session.json", + params: { + publicKeyCredential: + valid_passkey_auth_data.merge( + { userHandle: Base64.strict_encode64(user1.secure_identifier) }, + ), + } + + expect(response.status).to eq(200) + expect(response.parsed_body["error"]).to eq(nil) + end + + it "returns invalid response when key belongs to a different user" do + sign_in(user2) + simulate_localhost_passkey_challenge + user2.create_or_fetch_secure_identifier + + post "/u/confirm-session.json", + params: { + publicKeyCredential: + valid_passkey_auth_data.merge( + { userHandle: Base64.strict_encode64(user2.secure_identifier) }, + ), + } + + expect(response.status).to eq(401) + + json = response.parsed_body + expect(json["errors"][0]).to eq(I18n.t("webauthn.validation.ownership_error")) + end + end end end