diff --git a/app/assets/javascripts/discourse/app/components/modal/second-factor-add-security-key.hbs b/app/assets/javascripts/discourse/app/components/modal/second-factor-add-security-key.hbs index bde62e2e656..e1ce954901e 100644 --- a/app/assets/javascripts/discourse/app/components/modal/second-factor-add-security-key.hbs +++ b/app/assets/javascripts/discourse/app/components/modal/second-factor-add-security-key.hbs @@ -27,6 +27,7 @@ @value={{this.securityKeyName}} id="security-key-name" placeholder="security key name" + maxlength={{this.maxSecondFactorNameLength}} /> diff --git a/app/assets/javascripts/discourse/app/components/modal/second-factor-add-security-key.js b/app/assets/javascripts/discourse/app/components/modal/second-factor-add-security-key.js index 624330fd73e..ced82ae4878 100644 --- a/app/assets/javascripts/discourse/app/components/modal/second-factor-add-security-key.js +++ b/app/assets/javascripts/discourse/app/components/modal/second-factor-add-security-key.js @@ -8,6 +8,7 @@ import I18n from "I18n"; import { inject as service } from "@ember/service"; import { action } from "@ember/object"; import { tracked } from "@glimmer/tracking"; +import { MAX_SECOND_FACTOR_NAME_LENGTH } from "discourse/models/user"; export default class SecondFactorAddSecurityKey extends Component { @service capabilities; @@ -16,6 +17,8 @@ export default class SecondFactorAddSecurityKey extends Component { @tracked errorMessage = null; @tracked securityKeyName; + maxSecondFactorNameLength = MAX_SECOND_FACTOR_NAME_LENGTH; + get webauthnUnsupported() { return !isWebauthnSupported(); } @@ -30,7 +33,7 @@ export default class SecondFactorAddSecurityKey extends Component { } else { key = "user.second_factor.security_key.default_name"; } - this.securityKeyName = key; + this.securityKeyName = I18n.t(key); this.loading = true; this.args.model.secondFactor diff --git a/app/assets/javascripts/discourse/app/components/modal/second-factor-add-totp.hbs b/app/assets/javascripts/discourse/app/components/modal/second-factor-add-totp.hbs index 9e709d1ed2d..e13d19e938a 100644 --- a/app/assets/javascripts/discourse/app/components/modal/second-factor-add-totp.hbs +++ b/app/assets/javascripts/discourse/app/components/modal/second-factor-add-totp.hbs @@ -46,6 +46,7 @@ }}
diff --git a/app/assets/javascripts/discourse/app/components/modal/second-factor-edit-security-key.js b/app/assets/javascripts/discourse/app/components/modal/second-factor-edit-security-key.js index 441b72ed4a4..9600c229d15 100644 --- a/app/assets/javascripts/discourse/app/components/modal/second-factor-edit-security-key.js +++ b/app/assets/javascripts/discourse/app/components/modal/second-factor-edit-security-key.js @@ -1,10 +1,13 @@ import Component from "@glimmer/component"; import { action } from "@ember/object"; import { tracked } from "@glimmer/tracking"; +import { MAX_SECOND_FACTOR_NAME_LENGTH } from "discourse/models/user"; export default class SecondFactorEditSecurityKey extends Component { @tracked loading = false; + maxSecondFactorNameLength = MAX_SECOND_FACTOR_NAME_LENGTH; + @action editSecurityKey() { this.loading = true; diff --git a/app/assets/javascripts/discourse/app/components/modal/second-factor-edit.hbs b/app/assets/javascripts/discourse/app/components/modal/second-factor-edit.hbs index c3d9f5ad6f1..99038d08162 100644 --- a/app/assets/javascripts/discourse/app/components/modal/second-factor-edit.hbs +++ b/app/assets/javascripts/discourse/app/components/modal/second-factor-edit.hbs @@ -9,6 +9,7 @@ }} diff --git a/app/assets/javascripts/discourse/app/components/modal/second-factor-edit.js b/app/assets/javascripts/discourse/app/components/modal/second-factor-edit.js index be3ec5c5cce..8f5ffc6af26 100644 --- a/app/assets/javascripts/discourse/app/components/modal/second-factor-edit.js +++ b/app/assets/javascripts/discourse/app/components/modal/second-factor-edit.js @@ -1,10 +1,13 @@ import Component from "@glimmer/component"; import { action } from "@ember/object"; import { tracked } from "@glimmer/tracking"; +import { MAX_SECOND_FACTOR_NAME_LENGTH } from "discourse/models/user"; export default class SecondFactorEdit extends Component { @tracked loading = false; + maxSecondFactorNameLength = MAX_SECOND_FACTOR_NAME_LENGTH; + @action editSecondFactor() { this.loading = true; diff --git a/app/assets/javascripts/discourse/app/components/second-factor-input.hbs b/app/assets/javascripts/discourse/app/components/second-factor-input.hbs index a8015110206..5fe396418a6 100644 --- a/app/assets/javascripts/discourse/app/components/second-factor-input.hbs +++ b/app/assets/javascripts/discourse/app/components/second-factor-input.hbs @@ -11,4 +11,5 @@ autocapitalize="off" autocorrect="off" autofocus="autofocus" + ...attributes /> \ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/models/user.js b/app/assets/javascripts/discourse/app/models/user.js index defe5e8437d..8909ec0cd77 100644 --- a/app/assets/javascripts/discourse/app/models/user.js +++ b/app/assets/javascripts/discourse/app/models/user.js @@ -45,6 +45,8 @@ export const SECOND_FACTOR_METHODS = { SECURITY_KEY: 3, }; +export const MAX_SECOND_FACTOR_NAME_LENGTH = 300; + const TEXT_SIZE_COOKIE_NAME = "text_size"; const COOKIE_EXPIRY_DAYS = 365; diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index d1c6b6ca010..2dde4aae935 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1551,6 +1551,11 @@ class UsersController < ApplicationController end def create_second_factor_security_key + if current_user.all_security_keys.count >= UserSecurityKey::MAX_KEYS_PER_USER + render_json_error(I18n.t("login.too_many_security_keys"), status: 422) + return + end + challenge_session = DiscourseWebauthn.stage_challenge(current_user, secure_session) render json: success_json.merge( diff --git a/app/models/user.rb b/app/models/user.rb index 42abded842d..3f4c19b68bc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -96,6 +96,7 @@ class User < ActiveRecord::Base has_many :directory_items has_many :email_logs has_many :security_keys, -> { where(enabled: true) }, class_name: "UserSecurityKey" + has_many :all_security_keys, class_name: "UserSecurityKey" has_many :badges, through: :user_badges has_many :default_featured_user_badges, diff --git a/app/models/user_second_factor.rb b/app/models/user_second_factor.rb index 3c71c6b65e7..01048a6f16f 100644 --- a/app/models/user_second_factor.rb +++ b/app/models/user_second_factor.rb @@ -2,6 +2,10 @@ class UserSecondFactor < ActiveRecord::Base include SecondFactorManager + + MAX_TOTPS_PER_USER = 50 + MAX_NAME_LENGTH = 300 + belongs_to :user scope :backup_codes, -> { where(method: UserSecondFactor.methods[:backup_codes], enabled: true) } @@ -10,6 +14,10 @@ class UserSecondFactor < ActiveRecord::Base scope :all_totps, -> { where(method: UserSecondFactor.methods[:totp]) } + validates :name, length: { maximum: MAX_NAME_LENGTH }, if: :name_changed? + + validate :count_per_user_does_not_exceed_limit, on: :create + def self.methods @methods ||= Enum.new(totp: 1, backup_codes: 2, security_key: 3) end @@ -21,6 +29,16 @@ class UserSecondFactor < ActiveRecord::Base def totp_provisioning_uri totp_object.provisioning_uri(user.email) end + + private + + def count_per_user_does_not_exceed_limit + if self.method == UserSecondFactor.methods[:totp] + if self.class.where(method: self.method, user_id: self.user_id).count >= MAX_TOTPS_PER_USER + errors.add(:base, I18n.t("login.too_many_authenticators")) + end + end + end end # == Schema Information @@ -35,7 +53,7 @@ end # last_used :datetime # created_at :datetime not null # updated_at :datetime not null -# name :string +# name :string(300) # # Indexes # diff --git a/app/models/user_security_key.rb b/app/models/user_security_key.rb index 5447ee23adc..4caa0261128 100644 --- a/app/models/user_security_key.rb +++ b/app/models/user_security_key.rb @@ -2,13 +2,26 @@ class UserSecurityKey < ActiveRecord::Base belongs_to :user + MAX_KEYS_PER_USER = 50 + MAX_NAME_LENGTH = 300 scope :second_factors, -> { where(factor_type: UserSecurityKey.factor_types[:second_factor], enabled: true) } + validates :name, length: { maximum: MAX_NAME_LENGTH }, if: :name_changed? + validate :count_per_user_does_not_exceed_limit, on: :create + def self.factor_types @factor_types ||= Enum.new(second_factor: 0, first_factor: 1, multi_factor: 2) end + + private + + def count_per_user_does_not_exceed_limit + if UserSecurityKey.where(user_id: self.user_id).count >= MAX_KEYS_PER_USER + errors.add(:base, I18n.t("login.too_many_security_keys")) + end + end end # == Schema Information @@ -21,7 +34,7 @@ end # public_key :string not null # factor_type :integer default(0), not null # enabled :boolean default(TRUE), not null -# name :string not null +# name :string(300) not null # last_used :datetime # created_at :datetime not null # updated_at :datetime not null diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 7b0dc2c2549..eb8a53d131e 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2674,6 +2674,8 @@ en: invalid_security_key: "Invalid security key." missing_second_factor_name: "Please provide a name." missing_second_factor_code: "Please provide a code." + too_many_authenticators: "Sorry, you can't have more than 50 authenticators. Please remove an existing one and try again." + too_many_security_keys: "Sorry, you can't have more than 50 security keys. Please remove an existing one and try again." second_factor_toggle: totp: "Use an authenticator app or security key instead" backup_code: "Use a backup code instead" diff --git a/db/post_migrate/20230823095931_add_limit_to_user_second_factor_name.rb b/db/post_migrate/20230823095931_add_limit_to_user_second_factor_name.rb new file mode 100644 index 00000000000..8e2fcdffd2b --- /dev/null +++ b/db/post_migrate/20230823095931_add_limit_to_user_second_factor_name.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddLimitToUserSecondFactorName < ActiveRecord::Migration[7.0] + def up + execute(<<~SQL) + UPDATE user_second_factors + SET name = LEFT(name, 300) + WHERE name IS NOT NULL AND LENGTH(name) > 300 + SQL + change_column :user_second_factors, :name, :string, limit: 300 + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/post_migrate/20230823100627_add_limit_to_user_security_key_name.rb b/db/post_migrate/20230823100627_add_limit_to_user_security_key_name.rb new file mode 100644 index 00000000000..6f4240c2a29 --- /dev/null +++ b/db/post_migrate/20230823100627_add_limit_to_user_security_key_name.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddLimitToUserSecurityKeyName < ActiveRecord::Migration[7.0] + def up + execute(<<~SQL) + UPDATE user_security_keys + SET name = LEFT(name, 300) + WHERE name IS NOT NULL AND LENGTH(name) > 300 + SQL + change_column :user_security_keys, :name, :string, limit: 300 + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/webauthn/security_key_registration_service.rb b/lib/webauthn/security_key_registration_service.rb index 3a8ccd3deb2..0cdd4950acc 100644 --- a/lib/webauthn/security_key_registration_service.rb +++ b/lib/webauthn/security_key_registration_service.rb @@ -109,7 +109,7 @@ module DiscourseWebauthn # then register the new credential with the account that was denoted in options.user, by # associating it with the credentialId and credentialPublicKey in the attestedCredentialData # in authData, as appropriate for the Relying Party's system. - UserSecurityKey.create( + UserSecurityKey.create!( user: @current_user, credential_id: encoded_credential_id, public_key: endcoded_public_key, diff --git a/spec/models/user_second_factor_spec.rb b/spec/models/user_second_factor_spec.rb index 1dc16116dfb..560c9b3c23e 100644 --- a/spec/models/user_second_factor_spec.rb +++ b/spec/models/user_second_factor_spec.rb @@ -1,10 +1,83 @@ # frozen_string_literal: true RSpec.describe UserSecondFactor do + fab!(:user) { Fabricate(:user) } + describe ".methods" do it "should retain the right order" do expect(described_class.methods[:totp]).to eq(1) expect(described_class.methods[:backup_codes]).to eq(2) end end + + describe "name length validation" do + it "allows the name to be nil" do + Fabricate(:user_second_factor_totp, user: user, name: nil) + end + + it "doesn't allow the name to be longer than the limit" do + expect do + Fabricate( + :user_second_factor_totp, + user: user, + name: "a" * (described_class::MAX_NAME_LENGTH + 1), + ) + end.to raise_error(ActiveRecord::RecordInvalid) do |error| + expect(error.message).to include( + I18n.t("activerecord.errors.messages.too_long", count: described_class::MAX_NAME_LENGTH), + ) + end + end + + it "allows a name that is equal to or less than the limit" do + expect do + Fabricate( + :user_second_factor_totp, + user: user, + name: "a" * described_class::MAX_NAME_LENGTH, + ) + end.not_to raise_error + end + end + + describe "per-user count validation" do + it "doesn't allow a user to have more authenticators than the limit allows" do + stub_const(UserSecondFactor, "MAX_TOTPS_PER_USER", 1) do + Fabricate(:user_second_factor_totp, user: user) + expect do Fabricate(:user_second_factor_totp, user: user) end.to raise_error( + ActiveRecord::RecordInvalid, + ) do |error| + expect(error.message).to include(I18n.t("login.too_many_authenticators")) + end + end + end + + it "doesn't count backup codes in the authenticators limit" do + user.generate_backup_codes + expect(user.user_second_factors.backup_codes.count).to eq(10) + + stub_const(UserSecondFactor, "MAX_TOTPS_PER_USER", 1) do + Fabricate(:user_second_factor_totp, user: user) + expect do Fabricate(:user_second_factor_totp, user: user) end.to raise_error( + ActiveRecord::RecordInvalid, + ) do |error| + expect(error.message).to include(I18n.t("login.too_many_authenticators")) + end + end + end + + it "doesn't count authenticators from other users" do + another_user = Fabricate(:user) + Fabricate(:user_second_factor_totp, user: another_user) + + stub_const(UserSecondFactor, "MAX_TOTPS_PER_USER", 1) do + Fabricate(:user_second_factor_totp, user: user) + expect do Fabricate(:user_second_factor_totp, user: user) end.to raise_error( + ActiveRecord::RecordInvalid, + ) do |error| + expect(error.message).to include(I18n.t("login.too_many_authenticators")) + end + end + end + end end diff --git a/spec/models/user_security_key_spec.rb b/spec/models/user_security_key_spec.rb new file mode 100644 index 00000000000..44c49e8dc3a --- /dev/null +++ b/spec/models/user_security_key_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +RSpec.describe UserSecurityKey do + fab!(:user) { Fabricate(:user) } + + describe "name length validation" do + it "doesn't allow the name to be longer than the limit" do + expect do + Fabricate( + :user_security_key_with_random_credential, + user: user, + name: "b" * (described_class::MAX_NAME_LENGTH + 1), + ) + end.to raise_error(ActiveRecord::RecordInvalid) do |error| + expect(error.message).to include( + I18n.t("activerecord.errors.messages.too_long", count: described_class::MAX_NAME_LENGTH), + ) + end + end + + it "allows a name that's under the limit" do + expect do + Fabricate( + :user_security_key_with_random_credential, + user: user, + name: "b" * described_class::MAX_NAME_LENGTH, + ) + end.not_to raise_error + end + end + + describe "per-user count validation" do + it "doesn't allow a user to have more security keys than the limit allows" do + stub_const(UserSecurityKey, "MAX_KEYS_PER_USER", 1) do + Fabricate(:user_security_key_with_random_credential, user: user) + expect do + Fabricate(:user_security_key_with_random_credential, user: user) + end.to raise_error(ActiveRecord::RecordInvalid) do |error| + expect(error.message).to include(I18n.t("login.too_many_security_keys")) + end + end + end + + it "doesn't count security keys from other users" do + another_user = Fabricate(:user) + Fabricate(:user_security_key_with_random_credential, user: another_user) + + stub_const(UserSecurityKey, "MAX_KEYS_PER_USER", 1) do + Fabricate(:user_security_key_with_random_credential, user: user) + expect do + Fabricate(:user_security_key_with_random_credential, user: user) + end.to raise_error(ActiveRecord::RecordInvalid) do |error| + expect(error.message).to include(I18n.t("login.too_many_security_keys")) + end + end + end + end +end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 4ddad06890c..b9bb71453d1 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5539,6 +5539,46 @@ RSpec.describe UsersController do expect(response.parsed_body["error"]).to eq(I18n.t("login.missing_second_factor_code")) end end + + it "doesn't allow creating too many TOTPs" do + Fabricate(:user_second_factor_totp, user: user1) + + create_totp + staged_totp_key = read_secure_session["staged-totp-#{user1.id}"] + token = ROTP::TOTP.new(staged_totp_key).now + + stub_const(UserSecondFactor, "MAX_TOTPS_PER_USER", 1) do + post "/users/enable_second_factor_totp.json", + params: { + name: "test", + second_factor_token: token, + } + end + + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to include(I18n.t("login.too_many_authenticators")) + + expect(user1.user_second_factors.count).to eq(1) + end + + it "doesn't allow the TOTP name to exceed the limit" do + create_totp + staged_totp_key = read_secure_session["staged-totp-#{user1.id}"] + token = ROTP::TOTP.new(staged_totp_key).now + + post "/users/enable_second_factor_totp.json", + params: { + name: "a" * (UserSecondFactor::MAX_NAME_LENGTH + 1), + second_factor_token: token, + } + + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to include( + "Name is too long (maximum is 300 characters)", + ) + + expect(user1.user_second_factors.count).to eq(0) + end end describe "#update_second_factor" do @@ -5716,6 +5756,13 @@ RSpec.describe UsersController do ) end + it "doesn't create a challenge if the user has the maximum number allowed of security keys" do + Fabricate(:user_security_key_with_random_credential, user: user1) + stub_const(UserSecurityKey, "MAX_KEYS_PER_USER", 1) { create_second_factor_security_key } + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to include(I18n.t("login.too_many_security_keys")) + end + context "if the user has security key credentials already" do fab!(:user_security_key) { Fabricate(:user_security_key_with_random_credential, user: user1) } @@ -5745,6 +5792,43 @@ RSpec.describe UsersController do ) expect(user1.security_keys.last.name).to eq(valid_security_key_create_post_data[:name]) end + + it "doesn't allow creating too many security keys" do + simulate_localhost_webauthn_challenge + create_second_factor_security_key + _response_parsed = response.parsed_body + + Fabricate(:user_security_key_with_random_credential, user: user1) + + stub_const(UserSecurityKey, "MAX_KEYS_PER_USER", 1) do + post "/u/register_second_factor_security_key.json", + params: valid_security_key_create_post_data + end + + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to include(I18n.t("login.too_many_security_keys")) + + expect(user1.security_keys.count).to eq(1) + end + + it "doesn't allow the security key name to exceed the limit" do + simulate_localhost_webauthn_challenge + create_second_factor_security_key + _response_parsed = response.parsed_body + + post "/u/register_second_factor_security_key.json", + params: + valid_security_key_create_post_data.merge( + name: "a" * (UserSecurityKey::MAX_NAME_LENGTH + 1), + ) + + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to include( + "Name is too long (maximum is 300 characters)", + ) + + expect(user1.security_keys.count).to eq(0) + end end context "when the creation parameters are invalid" do