From fa8cd629f1ad4c64308e3e5c1d93f3ae78b2815a Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Thu, 25 Nov 2021 09:34:39 +0200 Subject: [PATCH] DEV: Hash tokens stored from email_tokens (#14493) This commit adds token_hash and scopes columns to email_tokens table. token_hash is a replacement for the token column to avoid storing email tokens in plaintext as it can pose a security risk. The new scope column ensures that email tokens cannot be used to perform a different action than the one intended. To sum up, this commit: * Adds token_hash and scope to email_tokens * Reuses code that schedules critical_user_email * Refactors EmailToken.confirm and EmailToken.atomic_confirm methods * Periodically cleans old, unconfirmed or expired email tokens --- app/controllers/admin/users_controller.rb | 2 +- .../finish_installation_controller.rb | 11 +- app/controllers/invites_controller.rb | 15 +- app/controllers/session_controller.rb | 8 +- .../users/omniauth_callbacks_controller.rb | 5 +- app/controllers/users_controller.rb | 46 ++--- app/controllers/users_email_controller.rb | 10 +- .../scheduled/activation_reminder_emails.rb | 12 +- app/jobs/scheduled/clean_up_email_tokens.rb | 14 ++ app/mailers/invite_mailer.rb | 2 +- app/models/email_change_request.rb | 15 +- app/models/email_token.rb | 165 ++++++++------- app/models/invite_redeemer.rb | 2 +- app/models/user.rb | 10 +- app/services/user_activator.rb | 11 +- app/services/user_authenticator.rb | 5 +- .../show_confirm_new_email.html.erb | 10 +- .../show_confirm_old_email.html.erb | 2 +- ...929215543_add_token_hash_to_email_token.rb | 29 +++ ...20211005163152_add_scope_to_email_token.rb | 11 + lib/email_updater.rb | 87 ++++---- lib/tasks/admin.rake | 2 +- spec/components/email_updater_spec.rb | 122 +++++------ spec/fabricators/email_token_fabricator.rb | 1 + spec/jobs/automatic_group_membership_spec.rb | 4 +- spec/mailers/user_notifications_spec.rb | 2 +- spec/models/email_token_spec.rb | 22 +- spec/models/user_spec.rb | 12 +- spec/models/web_hook_spec.rb | 2 +- spec/requests/api/users_spec.rb | 12 +- spec/requests/session_controller_spec.rb | 26 ++- spec/requests/users_controller_spec.rb | 191 ++++++------------ spec/requests/users_email_controller_spec.rb | 174 ++++++---------- spec/services/user_activator_spec.rb | 39 +--- 34 files changed, 482 insertions(+), 599 deletions(-) create mode 100644 app/jobs/scheduled/clean_up_email_tokens.rb create mode 100644 db/migrate/20210929215543_add_token_hash_to_email_token.rb create mode 100644 db/migrate/20211005163152_add_scope_to_email_token.rb diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 637172fa8a0..1c411fae985 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -328,7 +328,7 @@ class Admin::UsersController < Admin::AdminController def activate guardian.ensure_can_activate!(@user) # ensure there is an active email token - @user.email_tokens.create(email: @user.email) unless @user.email_tokens.active.exists? + @user.email_tokens.create!(email: @user.email, scope: EmailToken.scopes[:signup]) if !@user.email_tokens.active.exists? @user.activate StaffActionLogger.new(current_user).log_user_activate(@user, I18n.t('user.activated_by_staff')) render json: success_json diff --git a/app/controllers/finish_installation_controller.rb b/app/controllers/finish_installation_controller.rb index 13b7b64f5df..e84252c65ac 100644 --- a/app/controllers/finish_installation_controller.rb +++ b/app/controllers/finish_installation_controller.rb @@ -33,7 +33,6 @@ class FinishInstallationController < ApplicationController send_signup_email redirect_confirm(@user.email) end - end end @@ -50,14 +49,10 @@ class FinishInstallationController < ApplicationController protected def send_signup_email - email_token = @user.email_tokens.unconfirmed.active.first + return if @user.active && @user.email_confirmed? - if email_token.present? - Jobs.enqueue(:critical_user_email, - type: :signup, - user_id: @user.id, - email_token: email_token.token) - end + email_token = @user.email_tokens.create!(email: @user.email, scope: EmailToken.scopes[:signup]) + EmailToken.enqueue_signup_email(email_token) end def redirect_confirm(email) diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 85c17e4b4de..3cc4ecc455f 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -415,7 +415,10 @@ class InvitesController < ApplicationController Group.refresh_automatic_groups!(:admins, :moderators, :staff) if user.staff? if user.has_password? - send_activation_email(user) unless user.active + if !user.active + email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:signup]) + EmailToken.enqueue_signup_email(email_token) + end elsif !SiteSetting.enable_discourse_connect && SiteSetting.enable_local_logins Jobs.enqueue(:invite_password_instructions_email, username: user.username) end @@ -440,14 +443,4 @@ class InvitesController < ApplicationController end end end - - def send_activation_email(user) - email_token = user.email_tokens.create!(email: user.email) - - Jobs.enqueue(:critical_user_email, - type: :signup, - user_id: user.id, - email_token: email_token.token - ) - end end diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 1f099228bb3..76ca11fd395 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -339,7 +339,7 @@ class SessionController < ApplicationController def email_login_info token = params[:token] - matched_token = EmailToken.confirmable(token) + matched_token = EmailToken.confirmable(token, scope: EmailToken.scopes[:email_login]) user = matched_token&.user check_local_login_allowed(user: user, check_login_via_email: true) @@ -377,7 +377,7 @@ class SessionController < ApplicationController def email_login token = params[:token] - matched_token = EmailToken.confirmable(token) + matched_token = EmailToken.confirmable(token, scope: EmailToken.scopes[:email_login]) user = matched_token&.user check_local_login_allowed(user: user, check_login_via_email: true) @@ -388,7 +388,7 @@ class SessionController < ApplicationController return render(json: @second_factor_failure_payload) end - if user = EmailToken.confirm(token) + if user = EmailToken.confirm(token, scope: EmailToken.scopes[:email_login]) if login_not_approved_for?(user) return render json: login_not_approved elsif payload = login_error_check(user) @@ -444,7 +444,7 @@ class SessionController < ApplicationController user_presence = user.present? && user.human? && !user.staged if user_presence - email_token = user.email_tokens.create(email: user.email) + email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:password_reset]) Jobs.enqueue(:critical_user_email, type: :forgot_password, user_id: user.id, email_token: email_token.token) end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 7850cf2c67a..57c794f9cc2 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -155,9 +155,8 @@ class Users::OmniauthCallbacksController < ApplicationController user.update!(password: SecureRandom.hex) # Ensure there is an active email token - unless EmailToken.where(email: user.email, confirmed: true).exists? || - user.email_tokens.active.where(email: user.email).exists? - user.email_tokens.create!(email: user.email) + if !EmailToken.where(email: user.email, confirmed: true).exists? && !user.email_tokens.active.where(email: user.email).exists? + user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:signup]) end user.activate diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b4919f5c9d9..3e2a815949d 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -783,7 +783,6 @@ class UsersController < ApplicationController # no point doing anything else if we can't even find # a user from the token if @user - if !secure_session["second-factor-#{token}"] second_factor_authentication_result = @user.authenticate_second_factor(params, secure_session) if !second_factor_authentication_result.ok @@ -869,7 +868,7 @@ class UsersController < ApplicationController def confirm_email_token expires_now - EmailToken.confirm(params[:token]) + EmailToken.confirm(params[:token], scope: EmailToken.scopes[:signup]) render json: success_json end @@ -895,7 +894,7 @@ class UsersController < ApplicationController RateLimiter.new(nil, "admin-login-min-#{request.remote_ip}", 3, 1.minute).performed! if user = User.with_email(params[:email]).admins.human_users.first - email_token = user.email_tokens.create(email: user.email) + email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:email_login]) Jobs.enqueue(:critical_user_email, type: :admin_login, user_id: user.id, email_token: email_token.token) @message = I18n.t("admin_login.success") else @@ -926,7 +925,7 @@ class UsersController < ApplicationController RateLimiter.new(nil, "email-login-min-#{user.id}", 3, 1.minute).performed! if user_presence - email_token = user.email_tokens.create!(email: user.email) + email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:email_login]) Jobs.enqueue(:critical_user_email, type: :email_login, @@ -996,7 +995,7 @@ class UsersController < ApplicationController def perform_account_activation raise Discourse::InvalidAccess.new if honeypot_or_challenge_fails?(params) - if @user = EmailToken.confirm(params[:token]) + if @user = EmailToken.confirm(params[:token], scope: EmailToken.scopes[:signup]) # Log in the user unless they need to be approved if Guardian.new(@user).can_access_forum? @user.enqueue_welcome_message('welcome_user') if @user.send_welcome_message @@ -1041,8 +1040,8 @@ class UsersController < ApplicationController primary_email.skip_validate_email = false if primary_email.save - @user.email_tokens.create!(email: @user.email) - enqueue_activation_email + @email_token = @user.email_tokens.create!(email: @user.email, scope: EmailToken.scopes[:signup]) + EmailToken.enqueue_signup_email(@email_token, to_address: @user.email) render json: success_json else render_json_error(primary_email) @@ -1061,11 +1060,10 @@ class UsersController < ApplicationController if params[:username].present? @user = User.find_by_username_or_email(params[:username].to_s) end + raise Discourse::NotFound unless @user - if !current_user&.staff? && - @user.id != session[SessionController::ACTIVATE_USER_KEY] - + if !current_user&.staff? && @user.id != session[SessionController::ACTIVATE_USER_KEY] raise Discourse::InvalidAccess.new end @@ -1074,17 +1072,12 @@ class UsersController < ApplicationController if @user.active && @user.email_confirmed? render_json_error(I18n.t('activation.activated'), status: 409) else - @email_token = @user.email_tokens.unconfirmed.active.first - enqueue_activation_email + @email_token = @user.email_tokens.create!(email: @user.email, scope: EmailToken.scopes[:signup]) + EmailToken.enqueue_signup_email(@email_token, to_address: @user.email) render body: nil end end - def enqueue_activation_email - @email_token ||= @user.email_tokens.create!(email: @user.email) - Jobs.enqueue(:critical_user_email, type: :signup, user_id: @user.id, email_token: @email_token.token, to_address: @user.email) - end - def search_users term = params[:term].to_s.strip @@ -1635,14 +1628,17 @@ class UsersController < ApplicationController end def password_reset_find_user(token, committing_change:) - if EmailToken.valid_token_format?(token) - @user = committing_change ? EmailToken.confirm(token) : EmailToken.confirmable(token)&.user - if @user - secure_session["password-#{token}"] = @user.id - else - user_id = secure_session["password-#{token}"].to_i - @user = User.find(user_id) if user_id > 0 - end + @user = if committing_change + EmailToken.confirm(token, scope: EmailToken.scopes[:password_reset]) + else + EmailToken.confirmable(token, scope: EmailToken.scopes[:password_reset])&.user + end + + if @user + secure_session["password-#{token}"] = @user.id + else + user_id = secure_session["password-#{token}"].to_i + @user = User.find(user_id) if user_id > 0 end @error = I18n.t('password_reset.no_token') if !@user diff --git a/app/controllers/users_email_controller.rb b/app/controllers/users_email_controller.rb index a4af506c1da..f3f30a47dd5 100644 --- a/app/controllers/users_email_controller.rb +++ b/app/controllers/users_email_controller.rb @@ -200,17 +200,17 @@ class UsersEmailController < ApplicationController def load_change_request(type) expires_now - @token = EmailToken.confirmable(params[:token]) + token = EmailToken.confirmable(params[:token], scope: EmailToken.scopes[:email_update]) - if @token + if token if type == :old - @change_request = @token.user&.email_change_requests.where(old_email_token_id: @token.id).first + @change_request = token.user&.email_change_requests.where(old_email_token_id: token.id).first elsif type == :new - @change_request = @token.user&.email_change_requests.where(new_email_token_id: @token.id).first + @change_request = token.user&.email_change_requests.where(new_email_token_id: token.id).first end end - @user = @token&.user + @user = token&.user if (!@user || !@change_request) @error = I18n.t("change_email.already_done") diff --git a/app/jobs/scheduled/activation_reminder_emails.rb b/app/jobs/scheduled/activation_reminder_emails.rb index 171b5d139cf..c3203a273db 100644 --- a/app/jobs/scheduled/activation_reminder_emails.rb +++ b/app/jobs/scheduled/activation_reminder_emails.rb @@ -13,11 +13,13 @@ module Jobs user.custom_fields['activation_reminder'] = true user.save_custom_fields - email_token = user.email_tokens.create!(email: user.email) - ::Jobs.enqueue(:user_email, - type: :activation_reminder, - user_id: user.id, - email_token: email_token.token) + email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:signup]) + ::Jobs.enqueue( + :user_email, + type: :activation_reminder, + user_id: user.id, + email_token: email_token.token + ) end end end diff --git a/app/jobs/scheduled/clean_up_email_tokens.rb b/app/jobs/scheduled/clean_up_email_tokens.rb new file mode 100644 index 00000000000..006c5dbf026 --- /dev/null +++ b/app/jobs/scheduled/clean_up_email_tokens.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module Jobs + class CleanUpEmailTokens < ::Jobs::Scheduled + every 1.day + + def execute(args) + EmailToken + .where('NOT confirmed OR expired') + .where('created_at < ?', 1.month.ago) + .delete_all + end + end +end diff --git a/app/mailers/invite_mailer.rb b/app/mailers/invite_mailer.rb index 6f5f284422f..6f7fee63468 100644 --- a/app/mailers/invite_mailer.rb +++ b/app/mailers/invite_mailer.rb @@ -56,7 +56,7 @@ class InviteMailer < ActionMailer::Base def send_password_instructions(user) if user.present? - email_token = user.email_tokens.create(email: user.email) + email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:password_reset]) build_email(user.email, template: 'invite_password_instructions', email_token: email_token.token) diff --git a/app/models/email_change_request.rb b/app/models/email_change_request.rb index 74bb307bea3..5282d5d2fd5 100644 --- a/app/models/email_change_request.rb +++ b/app/models/email_change_request.rb @@ -1,9 +1,9 @@ # frozen_string_literal: true class EmailChangeRequest < ActiveRecord::Base + belongs_to :user belongs_to :old_email_token, class_name: 'EmailToken', dependent: :destroy belongs_to :new_email_token, class_name: 'EmailToken', dependent: :destroy - belongs_to :user belongs_to :requested_by, class_name: "User", foreign_key: :requested_by_user_id validates :new_email, presence: true, format: { with: EmailValidator.email_regex } @@ -12,6 +12,13 @@ class EmailChangeRequest < ActiveRecord::Base @states ||= Enum.new(authorizing_old: 1, authorizing_new: 2, complete: 3) end + def self.find_by_new_token(token) + EmailChangeRequest + .joins("INNER JOIN email_tokens ON email_tokens.id = email_change_requests.new_email_token_id") + .where("email_tokens.token_hash = ?", EmailToken.hash_token(token)) + .last + end + def requested_by_admin? self.requested_by&.admin? && !self.requested_by_self? end @@ -19,12 +26,6 @@ class EmailChangeRequest < ActiveRecord::Base def requested_by_self? self.requested_by_user_id == self.user_id end - - def self.find_by_new_token(token) - joins( - "INNER JOIN email_tokens ON email_tokens.id = email_change_requests.new_email_token_id" - ).where("email_tokens.token = ?", token).last - end end # == Schema Information diff --git a/app/models/email_token.rb b/app/models/email_token.rb index 8472881b8b6..020a0112841 100644 --- a/app/models/email_token.rb +++ b/app/models/email_token.rb @@ -1,97 +1,106 @@ # frozen_string_literal: true class EmailToken < ActiveRecord::Base + class TokenAccessError < StandardError; end + belongs_to :user - validates :token, :user_id, :email, presence: true + validates :user_id, :email, :token_hash, presence: true - before_validation(on: :create) do - self.token = EmailToken.generate_token - self.email = self.email.downcase if self.email - end + scope :unconfirmed, -> { where(confirmed: false) } + scope :active, -> { where(expired: false).where('created_at >= ?', SiteSetting.email_token_valid_hours.hours.ago) } - after_create do - # Expire the previous tokens - EmailToken.where(user_id: self.user_id) - .where("id != ?", self.id) - .update_all(expired: true) - end - - def self.token_length - 16 - end - - def self.valid_after - SiteSetting.email_token_valid_hours.hours.ago - end - - def self.unconfirmed - where(confirmed: false) - end - - def self.active - where(expired: false).where('created_at > ?', valid_after) - end - - def self.generate_token - SecureRandom.hex(EmailToken.token_length) - end - - def self.valid_token_format?(token) - token.present? && token =~ /\h{#{token.length / 2}}/i - end - - def self.atomic_confirm(token) - failure = { success: false } - return failure unless valid_token_format?(token) - - email_token = confirmable(token) - return failure if email_token.blank? - - user = email_token.user - failure[:user] = user - row_count = EmailToken.where(confirmed: false, id: email_token.id, expired: false).update_all 'confirmed = true' - - if row_count == 1 - { success: true, user: user, email_token: email_token } - else - failure + after_initialize do + if self.token_hash.blank? + @token ||= SecureRandom.hex + self.token = @token + self.token_hash = self.class.hash_token(@token) end end - def self.confirm(token, skip_reviewable: false) - User.transaction do - result = atomic_confirm(token) - user = result[:user] - if result[:success] - # If we are activating the user, send the welcome message - user.send_welcome_message = !user.active? - user.email = result[:email_token].email - user.active = true - user.custom_fields.delete('activation_reminder') - user.save! - user.create_reviewable unless skip_reviewable - user.set_automatic_groups - DiscourseEvent.trigger(:user_confirmed_email, user) - end + after_create do + EmailToken + .where(user_id: self.user_id) + .where(scope: [nil, self.scope]) + .where.not(id: self.id) + .update_all(expired: true) + end - if user - if Invite.redeem_from_email(user.email).present? - user.reload - end - user - end + before_validation do + self.email = self.email.downcase if self.email + end + + before_save do + if self.scope.blank? + Discourse.deprecate("EmailToken#scope cannot be empty.", output_in_test: true) + end + end + + def self.scopes + @scopes ||= Enum.new( + signup: 1, + password_reset: 2, + email_login: 3, + email_update: 4, + ) + end + + def token + raise TokenAccessError.new if @token.blank? + self[:token] + end + + def self.confirm(token, scope: nil, skip_reviewable: false) + User.transaction do + email_token = confirmable(token, scope: scope) + return if email_token.blank? + + email_token.update!(confirmed: true) + + user = email_token.user + user.send_welcome_message = !user.active? + user.email = email_token.email + user.active = true + user.custom_fields.delete('activation_reminder') + user.save! + user.create_reviewable if !skip_reviewable + user.set_automatic_groups + DiscourseEvent.trigger(:user_confirmed_email, user) + Invite.redeem_from_email(user.email) + + user.reload end rescue ActiveRecord::RecordInvalid # If the user's email is already taken, just return nil (failure) end - def self.confirmable(token) - EmailToken.where(token: token) - .where(expired: false, confirmed: false) - .where("created_at >= ?", EmailToken.valid_after) + def self.confirmable(token, scope: nil) + return nil if token.blank? + + relation = unconfirmed.active .includes(:user) - .first + .where(token_hash: hash_token(token)) + + # TODO(2022-01-01): All email tokens should have scopes by now + if !scope + relation.first + else + relation.where(scope: scope).first || relation.where(scope: nil).first + end + end + + def self.enqueue_signup_email(email_token, to_address: nil) + Jobs.enqueue( + :critical_user_email, + type: :signup, + user_id: email_token.user_id, + email_token: email_token.token, + to_address: to_address + ) + end + + def self.hash_token(token) + Digest::SHA256.hexdigest(token) end end @@ -107,6 +116,8 @@ end # expired :boolean default(FALSE), not null # created_at :datetime not null # updated_at :datetime not null +# token_hash :string not null +# scope :integer # # Indexes # diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb index 9dfc743cbd3..2ba2a6771b0 100644 --- a/app/models/invite_redeemer.rb +++ b/app/models/invite_redeemer.rb @@ -74,7 +74,7 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_ authenticator.finish if invite.emailed_status != Invite.emailed_status_types[:not_required] && email == invite.email && invite.email_token.present? && email_token == invite.email_token - user.email_tokens.create!(email: user.email) + user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:signup]) user.activate end diff --git a/app/models/user.rb b/app/models/user.rb index 40c48bd1505..5df57647b93 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1049,11 +1049,9 @@ class User < ActiveRecord::Base end def activate - if email_token = self.email_tokens.active.where(email: self.email).first - EmailToken.confirm(email_token.token, skip_reviewable: true) - end - self.update!(active: true) - create_reviewable + email_token = self.email_tokens.create!(email: self.email, scope: EmailToken.scopes[:signup]) + EmailToken.confirm(email_token.token, scope: EmailToken.scopes[:signup]) + reload end def deactivate(performed_by) @@ -1495,7 +1493,7 @@ class User < ActiveRecord::Base end def create_email_token - email_tokens.create!(email: email) + email_tokens.create!(email: email, scope: EmailToken.scopes[:signup]) end def ensure_password_is_hashed diff --git a/app/services/user_activator.rb b/app/services/user_activator.rb index 6a7d8ad1b05..4c0d14ab623 100644 --- a/app/services/user_activator.rb +++ b/app/services/user_activator.rb @@ -54,15 +54,8 @@ end class EmailActivator < UserActivator def activate - email_token = user.email_tokens.unconfirmed.active.first - email_token = user.email_tokens.create(email: user.email) if email_token.nil? - - Jobs.enqueue(:critical_user_email, - type: :signup, - user_id: user.id, - email_token: email_token.token - ) - + email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:signup]) + EmailToken.enqueue_signup_email(email_token) success_message end diff --git a/app/services/user_authenticator.rb b/app/services/user_authenticator.rb index 48d0b8038a9..c5cd920cf1c 100644 --- a/app/services/user_authenticator.rb +++ b/app/services/user_authenticator.rb @@ -49,10 +49,7 @@ class UserAuthenticator private def confirm_email - if authenticated? - EmailToken.confirm(@user.email_tokens.first.token) - @user.set_automatic_groups - end + @user.activate if authenticated? end def authenticator diff --git a/app/views/users_email/show_confirm_new_email.html.erb b/app/views/users_email/show_confirm_new_email.html.erb index d5a49bc3cb7..b820ba60391 100644 --- a/app/views/users_email/show_confirm_new_email.html.erb +++ b/app/views/users_email/show_confirm_new_email.html.erb @@ -24,7 +24,7 @@

<%=form_tag(u_confirm_new_email_path, method: :put) do %> - <%= hidden_field_tag 'token', @token.token %> + <%= hidden_field_tag 'token', params[:token] %> <%= hidden_field_tag 'second_factor_token', nil, id: 'security-key-credential' %>
@@ -34,7 +34,7 @@ <% if @show_backup_codes %>
- <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:backup_code] %> + <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:backup_code] %>

<%= t('login.second_factor_backup_title') %>

<%= label_tag(:second_factor_token, t("login.second_factor_backup_description")) %>
<%= render 'common/second_factor_backup_input' %>
@@ -44,8 +44,8 @@ <%= link_to t("login.second_factor_toggle.totp"), show_backup: "false" %>
<% elsif @show_security_key %> - <%= hidden_field_tag 'security_key_challenge', @security_key_challenge, id: 'security-key-challenge' %> - <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:security_key] %> + <%= hidden_field_tag 'security_key_challenge', @security_key_challenge, id: 'security-key-challenge' %> + <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:security_key] %> <%= hidden_field_tag 'security_key_allowed_credential_ids', @security_key_allowed_credential_ids.join(","), id: 'security-key-allowed-credential-ids' %>

<%= t('login.security_key_authenticate') %>

@@ -61,7 +61,7 @@ <% end %> <% elsif @show_second_factor %>
- <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:totp] %> + <%= hidden_field_tag 'second_factor_method', UserSecondFactor.methods[:totp] %>

<%= t('login.second_factor_title') %>

<%= label_tag(:second_factor_token, t('login.second_factor_description')) %>
<%= render 'common/second_factor_text_field' %>
diff --git a/app/views/users_email/show_confirm_old_email.html.erb b/app/views/users_email/show_confirm_old_email.html.erb index 9fc65856cb6..403103e8b26 100644 --- a/app/views/users_email/show_confirm_old_email.html.erb +++ b/app/views/users_email/show_confirm_old_email.html.erb @@ -27,7 +27,7 @@

<%=form_tag(u_confirm_old_email_path, method: :put) do %> - <%= hidden_field_tag 'token', @token.token %> + <%= hidden_field_tag 'token', params[:token] %> <%= submit_tag t('change_email.confirm'), class: "btn btn-primary" %> <% end %> <% end %> diff --git a/db/migrate/20210929215543_add_token_hash_to_email_token.rb b/db/migrate/20210929215543_add_token_hash_to_email_token.rb new file mode 100644 index 00000000000..b691b3cedf7 --- /dev/null +++ b/db/migrate/20210929215543_add_token_hash_to_email_token.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class AddTokenHashToEmailToken < ActiveRecord::Migration[6.1] + def up + add_column :email_tokens, :token_hash, :string + + loop do + rows = DB + .query("SELECT id, token FROM email_tokens WHERE token_hash IS NULL LIMIT 500") + .map { |row| { id: row.id, token_hash: Digest::SHA256.hexdigest(row.token) } } + + break if rows.size == 0 + + data_string = rows.map { |r| "(#{r[:id]}, '#{r[:token_hash]}')" }.join(",") + execute <<~SQL + UPDATE email_tokens + SET token_hash = data.token_hash + FROM (VALUES #{data_string}) AS data(id, token_hash) + WHERE email_tokens.id = data.id + SQL + end + + change_column_null :email_tokens, :token_hash, false + end + + def down + drop_column :email_tokens, :token_hash, :string + end +end diff --git a/db/migrate/20211005163152_add_scope_to_email_token.rb b/db/migrate/20211005163152_add_scope_to_email_token.rb new file mode 100644 index 00000000000..e6ef09cac9e --- /dev/null +++ b/db/migrate/20211005163152_add_scope_to_email_token.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddScopeToEmailToken < ActiveRecord::Migration[6.1] + def up + add_column :email_tokens, :scope, :integer + end + + def down + drop_column :email_tokens, :scope, :integer + end +end diff --git a/lib/email_updater.rb b/lib/email_updater.rb index 5c99219b544..f5e41fc8127 100644 --- a/lib/email_updater.rb +++ b/lib/email_updater.rb @@ -4,6 +4,7 @@ class EmailUpdater include HasErrors attr_reader :user + attr_reader :change_req def self.human_attribute_name(name, options = {}) User.human_attribute_name(name, options) @@ -48,16 +49,16 @@ class EmailUpdater UserHistory.create!(action: UserHistory.actions[:add_email], acting_user_id: @user.id) end - change_req = EmailChangeRequest.find_or_initialize_by(user_id: @user.id, new_email: email) + @change_req = EmailChangeRequest.find_or_initialize_by(user_id: @user.id, new_email: email) - if change_req.new_record? - change_req.requested_by = @guardian.user - change_req.old_email = old_email - change_req.new_email = email + if @change_req.new_record? + @change_req.requested_by = @guardian.user + @change_req.old_email = old_email + @change_req.new_email = email end - if change_req.change_state.blank? || change_req.change_state == EmailChangeRequest.states[:complete] - change_req.change_state = if @user.staff? + if @change_req.change_state.blank? || @change_req.change_state == EmailChangeRequest.states[:complete] + @change_req.change_state = if @user.staff? # Staff users must confirm their old email address first. EmailChangeRequest.states[:authorizing_old] else @@ -65,51 +66,53 @@ class EmailUpdater end end - if change_req.change_state == EmailChangeRequest.states[:authorizing_old] - change_req.old_email_token = @user.email_tokens.create!(email: @user.email) - send_email(add ? :confirm_old_email_add : :confirm_old_email, change_req.old_email_token) - elsif change_req.change_state == EmailChangeRequest.states[:authorizing_new] - change_req.new_email_token = @user.email_tokens.create!(email: email) - send_email(:confirm_new_email, change_req.new_email_token) + if @change_req.change_state == EmailChangeRequest.states[:authorizing_old] + @change_req.old_email_token = @user.email_tokens.create!(email: @user.email, scope: EmailToken.scopes[:email_update]) + send_email(add ? :confirm_old_email_add : :confirm_old_email, @change_req.old_email_token) + elsif @change_req.change_state == EmailChangeRequest.states[:authorizing_new] + @change_req.new_email_token = @user.email_tokens.create!(email: email, scope: EmailToken.scopes[:email_update]) + send_email(:confirm_new_email, @change_req.new_email_token) end - change_req.save! + @change_req.save! + @change_req end def confirm(token) confirm_result = nil User.transaction do - result = EmailToken.atomic_confirm(token) - if result[:success] - token = result[:email_token] - @user = token.user - - change_req = @user.email_change_requests - .where('old_email_token_id = :token_id OR new_email_token_id = :token_id', token_id: token.id) - .first - - case change_req.try(:change_state) - when EmailChangeRequest.states[:authorizing_old] - change_req.update!( - change_state: EmailChangeRequest.states[:authorizing_new], - new_email_token: @user.email_tokens.create(email: change_req.new_email) - ) - send_email(:confirm_new_email, change_req.new_email_token) - confirm_result = :authorizing_new - when EmailChangeRequest.states[:authorizing_new] - change_req.update!(change_state: EmailChangeRequest.states[:complete]) - if !@user.staff? - # Send an email notification only to users who did not confirm old - # email. - send_email_notification(change_req.old_email, change_req.new_email) - end - update_user_email(change_req.old_email, change_req.new_email) - confirm_result = :complete - end - else + email_token = EmailToken.confirmable(token, scope: EmailToken.scopes[:email_update]) + if email_token.blank? errors.add(:base, I18n.t('change_email.already_done')) confirm_result = :error + next + end + + email_token.update!(confirmed: true) + + @user = email_token.user + @change_req = @user.email_change_requests + .where('old_email_token_id = :token_id OR new_email_token_id = :token_id', token_id: email_token.id) + .first + + case @change_req.try(:change_state) + when EmailChangeRequest.states[:authorizing_old] + @change_req.update!( + change_state: EmailChangeRequest.states[:authorizing_new], + new_email_token: @user.email_tokens.create!(email: @change_req.new_email, scope: EmailToken.scopes[:email_update]) + ) + send_email(:confirm_new_email, @change_req.new_email_token) + confirm_result = :authorizing_new + when EmailChangeRequest.states[:authorizing_new] + @change_req.update!(change_state: EmailChangeRequest.states[:complete]) + if !@user.staff? + # Send an email notification only to users who did not confirm old + # email. + send_email_notification(@change_req.old_email, @change_req.new_email) + end + update_user_email(@change_req.old_email, @change_req.new_email) + confirm_result = :complete end end diff --git a/lib/tasks/admin.rake b/lib/tasks/admin.rake index d730ed66fe3..0c95b07001a 100644 --- a/lib/tasks/admin.rake +++ b/lib/tasks/admin.rake @@ -27,7 +27,7 @@ task "admin:invite", [:email] => [:environment] do |_, args| user.email_tokens.update_all confirmed: true puts "Sending email!" - email_token = user.email_tokens.create(email: user.email) + email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:signup]) Jobs.enqueue(:user_email, type: :account_created, user_id: user.id, email_token: email_token.token) end diff --git a/spec/components/email_updater_spec.rb b/spec/components/email_updater_spec.rb index f70c009b507..e3294c9c279 100644 --- a/spec/components/email_updater_spec.rb +++ b/spec/components/email_updater_spec.rb @@ -47,22 +47,20 @@ describe EmailUpdater do it "logs the admin user as the requester" do updater.change_to(new_email) - @change_req = user.email_change_requests.first - expect(@change_req.requested_by).to eq(admin) + expect(updater.change_req.requested_by).to eq(admin) end it "starts the new confirmation process" do updater.change_to(new_email) - @change_req = user.email_change_requests.first expect(updater.errors).to be_blank - expect(@change_req).to be_present - expect(@change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_new]) + expect(updater.change_req).to be_present + expect(updater.change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_new]) - expect(@change_req.old_email).to eq(old_email) - expect(@change_req.new_email).to eq(new_email) - expect(@change_req.old_email_token).to be_blank - expect(@change_req.new_email_token.email).to eq(new_email) + expect(updater.change_req.old_email).to eq(old_email) + expect(updater.change_req.new_email).to eq(new_email) + expect(updater.change_req.old_email_token).to be_blank + expect(updater.change_req.new_email_token.email).to eq(new_email) end end @@ -73,24 +71,22 @@ describe EmailUpdater do expect_enqueued_with(job: :critical_user_email, args: { type: :confirm_old_email, to_address: old_email }) do updater.change_to(new_email) end - - @change_req = user.email_change_requests.first end it "starts the old confirmation process" do expect(updater.errors).to be_blank - expect(@change_req.old_email).to eq(old_email) - expect(@change_req.new_email).to eq(new_email) - expect(@change_req).to be_present - expect(@change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_old]) + expect(updater.change_req.old_email).to eq(old_email) + expect(updater.change_req.new_email).to eq(new_email) + expect(updater.change_req).to be_present + expect(updater.change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_old]) - expect(@change_req.old_email_token.email).to eq(old_email) - expect(@change_req.new_email_token).to be_blank + expect(updater.change_req.old_email_token.email).to eq(old_email) + expect(updater.change_req.new_email_token).to be_blank end it "does not immediately confirm the request" do - expect(@change_req.change_state).not_to eq(EmailChangeRequest.states[:complete]) + expect(updater.change_req.change_state).not_to eq(EmailChangeRequest.states[:complete]) end end @@ -103,30 +99,27 @@ describe EmailUpdater do expect_enqueued_with(job: :critical_user_email, args: { type: :confirm_old_email, to_address: old_email }) do updater.change_to(new_email) end - - @change_req = user.email_change_requests.first end it "logs the user as the requester" do updater.change_to(new_email) - @change_req = user.email_change_requests.first - expect(@change_req.requested_by).to eq(user) + expect(updater.change_req.requested_by).to eq(user) end it "starts the old confirmation process" do expect(updater.errors).to be_blank - expect(@change_req.old_email).to eq(old_email) - expect(@change_req.new_email).to eq(new_email) - expect(@change_req).to be_present - expect(@change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_old]) + expect(updater.change_req.old_email).to eq(old_email) + expect(updater.change_req.new_email).to eq(new_email) + expect(updater.change_req).to be_present + expect(updater.change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_old]) - expect(@change_req.old_email_token.email).to eq(old_email) - expect(@change_req.new_email_token).to be_blank + expect(updater.change_req.old_email_token.email).to eq(old_email) + expect(updater.change_req.new_email_token).to be_blank end it "does not immediately confirm the request" do - expect(@change_req.change_state).not_to eq(EmailChangeRequest.states[:complete]) + expect(updater.change_req.change_state).not_to eq(EmailChangeRequest.states[:complete]) end end end @@ -140,20 +133,18 @@ describe EmailUpdater do expect_enqueued_with(job: :critical_user_email, args: { type: :confirm_new_email, to_address: new_email }) do updater.change_to(new_email) end - - @change_req = user.email_change_requests.first end it "starts the new confirmation process" do expect(updater.errors).to be_blank - expect(@change_req).to be_present - expect(@change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_new]) + expect(updater.change_req).to be_present + expect(updater.change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_new]) - expect(@change_req.old_email).to eq(old_email) - expect(@change_req.new_email).to eq(new_email) - expect(@change_req.old_email_token).to be_blank - expect(@change_req.new_email_token.email).to eq(new_email) + expect(updater.change_req.old_email).to eq(old_email) + expect(updater.change_req.new_email).to eq(new_email) + expect(updater.change_req.old_email_token).to be_blank + expect(updater.change_req.new_email_token.email).to eq(new_email) end context 'confirming an invalid token' do @@ -168,7 +159,7 @@ describe EmailUpdater do it "updates the user's email" do event = DiscourseEvent.track_events { expect_enqueued_with(job: :critical_user_email, args: { type: :notify_old_email, to_address: old_email }) do - updater.confirm(@change_req.new_email_token.token) + updater.confirm(updater.change_req.new_email_token.token) end }.last @@ -178,8 +169,8 @@ describe EmailUpdater do expect(event[:event_name]).to eq(:user_updated) expect(event[:params].first).to eq(user) - @change_req.reload - expect(@change_req.change_state).to eq(EmailChangeRequest.states[:complete]) + updater.change_req.reload + expect(updater.change_req.change_state).to eq(EmailChangeRequest.states[:complete]) end end end @@ -189,8 +180,6 @@ describe EmailUpdater do expect_enqueued_with(job: :critical_user_email, args: { type: :confirm_new_email, to_address: new_email }) do updater.change_to(new_email, add: true) end - - @change_req = user.email_change_requests.first end context 'confirming a valid token' do @@ -199,7 +188,7 @@ describe EmailUpdater do event = DiscourseEvent.track_events { expect_enqueued_with(job: :critical_user_email, args: { type: :notify_old_email_add, to_address: old_email }) do - updater.confirm(@change_req.new_email_token.token) + updater.confirm(updater.change_req.new_email_token.token) end }.last @@ -209,15 +198,15 @@ describe EmailUpdater do expect(event[:event_name]).to eq(:user_updated) expect(event[:params].first).to eq(user) - @change_req.reload - expect(@change_req.change_state).to eq(EmailChangeRequest.states[:complete]) + updater.change_req.reload + expect(updater.change_req.change_state).to eq(EmailChangeRequest.states[:complete]) end end context 'that was deleted before' do it 'works' do expect_enqueued_with(job: :critical_user_email, args: { type: :notify_old_email_add, to_address: old_email }) do - updater.confirm(@change_req.new_email_token.token) + updater.confirm(updater.change_req.new_email_token.token) end expect(user.reload.user_emails.pluck(:email)).to contain_exactly(old_email, new_email) @@ -229,10 +218,8 @@ describe EmailUpdater do updater.change_to(new_email, add: true) end - @change_req = user.email_change_requests.first - expect_enqueued_with(job: :critical_user_email, args: { type: :notify_old_email_add, to_address: old_email }) do - updater.confirm(@change_req.new_email_token.token) + updater.confirm(updater.change_req.new_email_token.token) end expect(user.reload.user_emails.pluck(:email)).to contain_exactly(old_email, new_email) @@ -266,20 +253,18 @@ describe EmailUpdater do expect_enqueued_with(job: :critical_user_email, args: { type: :confirm_old_email, to_address: old_email }) do updater.change_to(new_email) end - - @change_req = user.email_change_requests.first end it "starts the old confirmation process" do expect(updater.errors).to be_blank - expect(@change_req.old_email).to eq(old_email) - expect(@change_req.new_email).to eq(new_email) - expect(@change_req).to be_present - expect(@change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_old]) + expect(updater.change_req.old_email).to eq(old_email) + expect(updater.change_req.new_email).to eq(new_email) + expect(updater.change_req).to be_present + expect(updater.change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_old]) - expect(@change_req.old_email_token.email).to eq(old_email) - expect(@change_req.new_email_token).to be_blank + expect(updater.change_req.old_email_token.email).to eq(old_email) + expect(updater.change_req.new_email_token).to be_blank end context 'confirming an invalid token' do @@ -293,34 +278,33 @@ describe EmailUpdater do context 'confirming a valid token' do before do expect_enqueued_with(job: :critical_user_email, args: { type: :confirm_new_email, to_address: new_email }) do - updater.confirm(@change_req.old_email_token.token) + @old_token = updater.change_req.old_email_token.token + updater.confirm(@old_token) end - - @change_req.reload end it "starts the new update process" do expect(updater.errors).to be_blank expect(user.reload.email).to eq(old_email) - expect(@change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_new]) - expect(@change_req.new_email_token).to be_present + expect(updater.change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_new]) + expect(updater.change_req.new_email_token).to be_present end it "cannot be confirmed twice" do - updater.confirm(@change_req.old_email_token.token) + updater.confirm(@old_token) expect(updater.errors).to be_present expect(user.reload.email).to eq(old_email) - @change_req.reload - expect(@change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_new]) - expect(@change_req.new_email_token.email).to eq(new_email) + updater.change_req.reload + expect(updater.change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_new]) + expect(updater.change_req.new_email_token.email).to eq(new_email) end context "completing the new update process" do before do expect_not_enqueued_with(job: :critical_user_email, args: { type: :notify_old_email, to_address: old_email }) do - updater.confirm(@change_req.new_email_token.token) + updater.confirm(updater.change_req.new_email_token.token) end end @@ -328,8 +312,8 @@ describe EmailUpdater do expect(updater.errors).to be_blank expect(user.reload.email).to eq(new_email) - @change_req.reload - expect(@change_req.change_state).to eq(EmailChangeRequest.states[:complete]) + updater.change_req.reload + expect(updater.change_req.change_state).to eq(EmailChangeRequest.states[:complete]) end end end diff --git a/spec/fabricators/email_token_fabricator.rb b/spec/fabricators/email_token_fabricator.rb index a1ae1cbd9d5..c2ec17d3b55 100644 --- a/spec/fabricators/email_token_fabricator.rb +++ b/spec/fabricators/email_token_fabricator.rb @@ -3,4 +3,5 @@ Fabricator(:email_token) do user email { |attrs| attrs[:user].email } + scope EmailToken.scopes[:signup] end diff --git a/spec/jobs/automatic_group_membership_spec.rb b/spec/jobs/automatic_group_membership_spec.rb index 304b43c2502..250e12b1371 100644 --- a/spec/jobs/automatic_group_membership_spec.rb +++ b/spec/jobs/automatic_group_membership_spec.rb @@ -12,9 +12,9 @@ describe Jobs::AutomaticGroupMembership do user1 = Fabricate(:user, email: "no@bar.com") user2 = Fabricate(:user, email: "no@wat.com") user3 = Fabricate(:user, email: "noo@wat.com", staged: true) - EmailToken.confirm(user3.email_tokens.last.token) + EmailToken.confirm(Fabricate(:email_token, user: user3).token) user4 = Fabricate(:user, email: "yes@wat.com") - EmailToken.confirm(user4.email_tokens.last.token) + EmailToken.confirm(Fabricate(:email_token, user: user4).token) user5 = Fabricate(:user, email: "sso@wat.com") user5.create_single_sign_on_record(external_id: 123, external_email: "hacker@wat.com", last_payload: "") user6 = Fabricate(:user, email: "sso2@wat.com") diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index e186b8d6d23..907b356cabe 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -121,7 +121,7 @@ describe UserNotifications do end describe '.email_login' do - let(:email_token) { user.email_tokens.create!(email: user.email).token } + let(:email_token) { Fabricate(:email_token, user: user, scope: EmailToken.scopes[:email_login]).token } subject { UserNotifications.email_login(user, email_token: email_token) } it "generates the right email" do diff --git a/spec/models/email_token_spec.rb b/spec/models/email_token_spec.rb index aa6dfac6d70..92ad1e55c46 100644 --- a/spec/models/email_token_spec.rb +++ b/spec/models/email_token_spec.rb @@ -3,7 +3,6 @@ require 'rails_helper' describe EmailToken do - it { is_expected.to validate_presence_of :user_id } it { is_expected.to validate_presence_of :email } it { is_expected.to belong_to :user } @@ -11,14 +10,14 @@ describe EmailToken do context '#create' do fab!(:user) { Fabricate(:user, active: false) } let!(:original_token) { user.email_tokens.first } - let!(:email_token) { user.email_tokens.create(email: 'bubblegum@adventuretime.ooo') } + let!(:email_token) { Fabricate(:email_token, user: user, email: 'bubblegum@adventuretime.ooo') } it 'should create the email token' do expect(email_token).to be_present end it 'should downcase the email' do - token = user.email_tokens.create(email: "UpperCaseSoWoW@GMail.com") + token = Fabricate(:email_token, user: user, email: "UpperCaseSoWoW@GMail.com") expect(token.email).to eq "uppercasesowow@gmail.com" end @@ -45,20 +44,15 @@ describe EmailToken do end context '#confirm' do - fab!(:user) { Fabricate(:user, active: false) } - let(:email_token) { user.email_tokens.first } + let!(:email_token) { Fabricate(:email_token, user: user) } it 'returns nil with a nil token' do expect(EmailToken.confirm(nil)).to be_blank end - it 'returns nil with a made up token' do - expect(EmailToken.confirm(EmailToken.generate_token)).to be_blank - end - - it 'returns nil unless the token is the right length' do - expect(EmailToken.confirm('a')).to be_blank + it 'returns nil with an invalid token' do + expect(EmailToken.confirm("random token")).to be_blank end it 'returns nil when a token is expired' do @@ -73,7 +67,6 @@ describe EmailToken do end context 'taken email address' do - before do @other_user = Fabricate(:coding_horror) email_token.update_attribute :email, @other_user.email @@ -82,7 +75,6 @@ describe EmailToken do it 'returns nil when the email has been taken since the token has been generated' do expect(EmailToken.confirm(email_token.token)).to be_blank end - end context 'welcome message' do @@ -94,7 +86,6 @@ describe EmailToken do end context 'success' do - let!(:confirmed_user) { EmailToken.confirm(email_token.token) } it "returns the correct user" do @@ -124,7 +115,7 @@ describe EmailToken do fab!(:invite) { Fabricate(:invite, email: 'test@example.com') } fab!(:invited_user) { Fabricate(:user, active: false, email: invite.email) } - let(:user_email_token) { invited_user.email_tokens.first } + let!(:user_email_token) { Fabricate(:email_token, user: invited_user) } let!(:confirmed_invited_user) { EmailToken.confirm(user_email_token.token) } it "returns the correct user" do @@ -151,5 +142,4 @@ describe EmailToken do end end end - end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 1a6a22588a0..7113318e6ea 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -234,7 +234,7 @@ describe User do reviewable = ReviewableUser.find_by(target: user) expect(reviewable).to be_blank - EmailToken.confirm(user.email_tokens.first.token) + EmailToken.confirm(Fabricate(:email_token, user: user).token) expect(user.reload.active).to eq(true) reviewable = ReviewableUser.find_by(target: user) expect(reviewable).to be_present @@ -876,7 +876,7 @@ describe User do expect(@user.active).to eq(false) expect(@user.confirm_password?("ilovepasta")).to eq(true) - email_token = @user.email_tokens.create(email: 'pasta@delicious.com') + email_token = Fabricate(:email_token, user: @user, email: 'pasta@delicious.com') UserAuthToken.generate!(user_id: @user.id) @@ -1073,7 +1073,7 @@ describe User do context 'when email has been confirmed' do it 'should return true' do - token = user.email_tokens.find_by(email: user.email) + token = Fabricate(:email_token, user: user) EmailToken.confirm(token.token) expect(user.email_confirmed?).to eq(true) end @@ -1549,14 +1549,14 @@ describe User do it "doesn't automatically add staged users" do staged_user = Fabricate(:user, active: true, staged: true, email: "wat@wat.com") - EmailToken.confirm(staged_user.email_tokens.last.token) + EmailToken.confirm(Fabricate(:email_token, user: staged_user).token) group.reload expect(group.users.include?(staged_user)).to eq(false) end it "is automatically added to a group when the email matches" do user = Fabricate(:user, active: true, email: "foo@bar.com") - EmailToken.confirm(user.email_tokens.last.token) + EmailToken.confirm(Fabricate(:email_token, user: user).token) group.reload expect(group.users.include?(user)).to eq(true) @@ -1585,7 +1585,7 @@ describe User do user.password_required! user.save! - EmailToken.confirm(user.email_tokens.last.token) + EmailToken.confirm(Fabricate(:email_token, user: user).token) user.reload expect(user.title).to eq("bars and wats") diff --git a/spec/models/web_hook_spec.rb b/spec/models/web_hook_spec.rb index fd8ed079667..73d7c62f690 100644 --- a/spec/models/web_hook_spec.rb +++ b/spec/models/web_hook_spec.rb @@ -333,7 +333,7 @@ describe WebHook do payload = JSON.parse(job_args["payload"]) expect(payload["id"]).to eq(user.id) - email_token = user.email_tokens.create(email: user.email) + email_token = Fabricate(:email_token, user: user) EmailToken.confirm(email_token.token) job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 2489d79f77f..e651df4c1f6 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -567,11 +567,13 @@ describe 'users' do expected_response_schema = nil let(:user) { Fabricate(:user) } - let(:token) { user.email_tokens.create(email: user.email).token } - let(:params) { { - 'username' => user.username, - 'password' => 'NH8QYbxYS5Zv5qEFzA4jULvM' - } } + let(:token) { Fabricate(:email_token, user: user, scope: EmailToken.scopes[:password_reset]).token } + let(:params) do + { + 'username' => user.username, + 'password' => 'NH8QYbxYS5Zv5qEFzA4jULvM' + } + end it_behaves_like "a JSON endpoint", 200 do let(:expected_response_schema) { expected_response_schema } diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index c4684a182e0..073ef1a88f0 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -3,10 +3,9 @@ require 'rails_helper' require 'rotp' -RSpec.describe SessionController do - let(:email_token) { Fabricate(:email_token) } - let(:user) { email_token.user } - let(:logo_fixture) { "http://#{Discourse.current_hostname}/uploads/logo.png" } +describe SessionController do + let(:user) { Fabricate(:user) } + let(:email_token) { Fabricate(:email_token, user: user) } shared_examples 'failed to continue local login' do it 'should return the right response' do @@ -16,6 +15,8 @@ RSpec.describe SessionController do end describe '#email_login_info' do + let(:email_token) { Fabricate(:email_token, user: user, scope: EmailToken.scopes[:email_login]) } + before do SiteSetting.enable_local_logins_via_email = true end @@ -118,6 +119,8 @@ RSpec.describe SessionController do end describe '#email_login' do + let(:email_token) { Fabricate(:email_token, user: user, scope: EmailToken.scopes[:email_login]) } + before do SiteSetting.enable_local_logins_via_email = true end @@ -200,7 +203,6 @@ RSpec.describe SessionController do post "/session/email-login/#{email_token.token}.json" expect(response.status).to eq(200) - expect(response.parsed_body["error"]).to eq(I18n.t("login.not_approved")) expect(session[:current_user_id]).to eq(nil) end @@ -1112,6 +1114,8 @@ RSpec.describe SessionController do let(:headers) { { host: Discourse.current_hostname } } describe 'can act as an SSO provider' do + let(:logo_fixture) { "http://#{Discourse.current_hostname}/uploads/logo.png" } + before do stub_request(:any, /#{Discourse.current_hostname}\/uploads/).to_return( status: 200, @@ -1315,8 +1319,6 @@ RSpec.describe SessionController do end describe '#create' do - let(:user) { Fabricate(:user) } - context 'local login is disabled' do before do SiteSetting.enable_local_logins = false @@ -1354,8 +1356,7 @@ RSpec.describe SessionController do context 'when email is confirmed' do before do - token = user.email_tokens.find_by(email: user.email) - EmailToken.confirm(token.token) + EmailToken.confirm(email_token.token) end it "raises an error when the login isn't present" do @@ -1508,6 +1509,7 @@ RSpec.describe SessionController do )) end end + context "when the security key params are invalid" do it "shows an error message and denies login" do @@ -1532,9 +1534,9 @@ RSpec.describe SessionController do )) end end + context "when the security key params are valid" do it "logs the user in" do - post "/session.json", params: { login: user.username, password: 'myawesomepassword', @@ -1549,6 +1551,7 @@ RSpec.describe SessionController do expect(user.user_auth_tokens.count).to eq(1) end end + context "when the security key is disabled in the background by the user and TOTP is enabled" do before do user_security_key.destroy! @@ -1556,7 +1559,6 @@ RSpec.describe SessionController do end it "shows an error message and denies login" do - post "/session.json", params: { login: user.username, password: 'myawesomepassword', @@ -1609,6 +1611,7 @@ RSpec.describe SessionController do )) end end + context 'when using backup code method' do it 'should return the right response' do post "/session.json", params: { @@ -1646,6 +1649,7 @@ RSpec.describe SessionController do .to eq(user.user_auth_tokens.first.auth_token) end end + context 'when using backup code method' do it 'should log the user in' do post "/session.json", params: { diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index e436e9f2b1a..bbe67e69ad0 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -42,12 +42,7 @@ describe UsersController do end describe '#perform_account_activation' do - let(:token) do - return @token if @token.present? - email_token = EmailToken.create!(expired: false, confirmed: false, user: user, email: user.email) - @token = email_token.token - @token - end + let(:email_token) { Fabricate(:email_token, user: user) } before do UsersController.any_instance.stubs(:honeypot_or_challenge_fails?).returns(false) @@ -66,7 +61,7 @@ describe UsersController do it 'enqueues a welcome message if the user object indicates so' do SiteSetting.send_welcome_message = true user.update(active: false) - put "/u/activate-account/#{token}" + put "/u/activate-account/#{email_token.token}" expect(response.status).to eq(200) expect(Jobs::SendSystemMessage.jobs.size).to eq(1) expect(Jobs::SendSystemMessage.jobs.first["args"].first["message_type"]).to eq("welcome_user") @@ -74,7 +69,7 @@ describe UsersController do it "doesn't enqueue the welcome message if the object returns false" do user.update(active: true) - put "/u/activate-account/#{token}" + put "/u/activate-account/#{email_token.token}" expect(response.status).to eq(200) expect(Jobs::SendSystemMessage.jobs.size).to eq(0) end @@ -83,24 +78,21 @@ describe UsersController do context "honeypot" do it "raises an error if the honeypot is invalid" do UsersController.any_instance.stubs(:honeypot_or_challenge_fails?).returns(true) - put "/u/activate-account/#{token}" + put "/u/activate-account/#{email_token.token}" expect(response.status).to eq(403) end end context 'response' do - before do - Guardian.any_instance.expects(:can_access_forum?).returns(true) - EmailToken.expects(:confirm).with("#{token}").returns(user) - end - it 'correctly logs on user' do + email_token + events = DiscourseEvent.track_events do - put "/u/activate-account/#{token}" + put "/u/activate-account/#{email_token.token}" end expect(events.map { |event| event[:event_name] }).to contain_exactly( - :user_logged_in, :user_first_logged_in + :user_confirmed_email, :user_first_logged_in, :user_logged_in ) expect(response.status).to eq(200) @@ -115,11 +107,10 @@ describe UsersController do context 'user is not approved' do before do SiteSetting.must_approve_users = true - EmailToken.expects(:confirm).with("#{token}").returns(user) - put "/u/activate-account/#{token}" end it 'should return the right response' do + put "/u/activate-account/#{email_token.token}" expect(response.status).to eq(200) expect(CGI.unescapeHTML(response.body)) @@ -140,7 +131,7 @@ describe UsersController do destination_url = 'http://thisisasite.com/somepath' cookies[:destination_url] = destination_url - put "/u/activate-account/#{token}" + put "/u/activate-account/#{email_token.token}" expect(response).to redirect_to(destination_url) end @@ -211,23 +202,21 @@ describe UsersController do end context 'valid token' do + let!(:user) { Fabricate(:user) } + let!(:user_auth_token) { UserAuthToken.generate!(user_id: user.id) } + let!(:email_token) { Fabricate(:email_token, user: user, scope: EmailToken.scopes[:password_reset]) } + context 'when rendered' do it 'renders referrer never on get requests' do - user = Fabricate(:user) - token = user.email_tokens.create(email: user.email).token - get "/u/password-reset/#{token}" + get "/u/password-reset/#{email_token.token}" expect(response.status).to eq(200) expect(response.body).to include('') end end it 'returns success' do - user = Fabricate(:user) - user_auth_token = UserAuthToken.generate!(user_id: user.id) - token = user.email_tokens.create(email: user.email).token - events = DiscourseEvent.track_events do - put "/u/password-reset/#{token}", params: { password: 'hg9ow8yhg98o' } + put "/u/password-reset/#{email_token.token}", params: { password: 'hg9ow8yhg98o' } end expect(events.map { |event| event[:event_name] }).to contain_exactly( @@ -240,87 +229,57 @@ describe UsersController do expect(json['password_reset']).to include('{"is_developer":false,"admin":false,"second_factor_required":false,"security_key_required":false,"backup_enabled":false,"multiple_second_factor_methods":false}') end - expect(session["password-#{token}"]).to be_blank + expect(session["password-#{email_token.token}"]).to be_blank expect(UserAuthToken.where(id: user_auth_token.id).count).to eq(0) end it 'disallows double password reset' do - user = Fabricate(:user) - token = user.email_tokens.create(email: user.email).token - - put "/u/password-reset/#{token}", params: { password: 'hg9ow8yHG32O' } - - put "/u/password-reset/#{token}", params: { password: 'test123987AsdfXYZ' } - - user.reload - expect(user.confirm_password?('hg9ow8yHG32O')).to eq(true) - - # logged in now + put "/u/password-reset/#{email_token.token}", params: { password: 'hg9ow8yHG32O' } + put "/u/password-reset/#{email_token.token}", params: { password: 'test123987AsdfXYZ' } + expect(user.reload.confirm_password?('hg9ow8yHG32O')).to eq(true) expect(user.user_auth_tokens.count).to eq(1) end it "doesn't redirect to wizard on get" do - user = Fabricate(:admin) - UserAuthToken.generate!(user_id: user.id) + user.update!(admin: true) - token = user.email_tokens.create(email: user.email).token - get "/u/password-reset/#{token}.json" + get "/u/password-reset/#{email_token.token}.json" expect(response).not_to redirect_to(wizard_path) end it "redirects to the wizard if you're the first admin" do - user = Fabricate(:admin) - UserAuthToken.generate!(user_id: user.id) - - token = user.email_tokens.create(email: user.email).token - get "/u/password-reset/#{token}" - - put "/u/password-reset/#{token}", params: { password: 'hg9ow8yhg98oadminlonger' } + user.update!(admin: true) + get "/u/password-reset/#{email_token.token}" + put "/u/password-reset/#{email_token.token}", params: { password: 'hg9ow8yhg98oadminlonger' } expect(response).to redirect_to(wizard_path) end it "sets the users timezone if the param is present" do - user = Fabricate(:admin) - UserAuthToken.generate!(user_id: user.id) - - token = user.email_tokens.create(email: user.email).token - get "/u/password-reset/#{token}" - + get "/u/password-reset/#{email_token.token}" expect(user.user_option.timezone).to eq(nil) - put "/u/password-reset/#{token}", params: { password: 'hg9ow8yhg98oadminlonger', timezone: "America/Chicago" } + + put "/u/password-reset/#{email_token.token}", params: { password: 'hg9ow8yhg98oadminlonger', timezone: "America/Chicago" } expect(user.user_option.reload.timezone).to eq("America/Chicago") end it "logs the password change" do - user = Fabricate(:admin) - UserAuthToken.generate!(user_id: user.id) - token = user.email_tokens.create(email: user.email).token - get "/u/password-reset/#{token}" + get "/u/password-reset/#{email_token.token}" expect do - put "/u/password-reset/#{token}", params: { password: 'hg9ow8yhg98oadminlonger' } + put "/u/password-reset/#{email_token.token}", params: { password: 'hg9ow8yhg98oadminlonger' } end.to change { UserHistory.count }.by (1) - entry = UserHistory.last - - expect(entry.target_user_id).to eq(user.id) - expect(entry.action).to eq(UserHistory.actions[:change_password]) + user_history = UserHistory.last + expect(user_history.target_user_id).to eq(user.id) + expect(user_history.action).to eq(UserHistory.actions[:change_password]) end it "doesn't invalidate the token when loading the page" do - user = Fabricate(:user) - user_token = UserAuthToken.generate!(user_id: user.id) - - email_token = user.email_tokens.create(email: user.email) - get "/u/password-reset/#{email_token.token}.json" expect(response.status).to eq(200) - - email_token.reload - - expect(email_token.confirmed).to eq(false) - expect(UserAuthToken.where(id: user_token.id).count).to eq(1) + expect(email_token.reload.confirmed).to eq(false) + expect(UserAuthToken.where(id: user_auth_token.id).count).to eq(1) end context "rate limiting" do @@ -329,10 +288,8 @@ describe UsersController do it "rate limits reset passwords" do freeze_time - token = user.email_tokens.create!(email: user.email).token - 6.times do - put "/u/password-reset/#{token}", params: { + put "/u/password-reset/#{email_token.token}", params: { second_factor_token: 123456, second_factor_method: 1 } @@ -340,7 +297,7 @@ describe UsersController do expect(response.status).to eq(200) end - put "/u/password-reset/#{token}", params: { + put "/u/password-reset/#{email_token.token}", params: { second_factor_token: 123456, second_factor_method: 1 } @@ -351,10 +308,8 @@ describe UsersController do it "rate limits reset passwords by username" do freeze_time - token = user.email_tokens.create!(email: user.email).token - 6.times do |x| - put "/u/password-reset/#{token}", params: { + put "/u/password-reset/#{email_token.token}", params: { second_factor_token: 123456, second_factor_method: 1 }, env: { "REMOTE_ADDR": "1.2.3.#{x}" } @@ -362,7 +317,7 @@ describe UsersController do expect(response.status).to eq(200) end - put "/u/password-reset/#{token}", params: { + put "/u/password-reset/#{email_token.token}", params: { second_factor_token: 123456, second_factor_method: 1 }, env: { "REMOTE_ADDR": "1.2.3.4" } @@ -375,16 +330,16 @@ describe UsersController do let!(:second_factor) { Fabricate(:user_second_factor_totp, user: user) } it 'does not change with an invalid token' do - token = user.email_tokens.create!(email: user.email).token + user.user_auth_tokens.destroy_all - get "/u/password-reset/#{token}" + get "/u/password-reset/#{email_token.token}" expect(response.body).to have_tag("div#data-preloaded") do |element| json = JSON.parse(element.current_scope.attribute('data-preloaded').value) expect(json['password_reset']).to include('{"is_developer":false,"admin":false,"second_factor_required":true,"security_key_required":false,"backup_enabled":false,"multiple_second_factor_methods":false}') end - put "/u/password-reset/#{token}", params: { + put "/u/password-reset/#{email_token.token}", params: { password: 'hg9ow8yHG32O', second_factor_token: '000000', second_factor_method: UserSecondFactor.methods[:totp] @@ -398,11 +353,9 @@ describe UsersController do end it 'changes password with valid 2-factor tokens' do - token = user.email_tokens.create(email: user.email).token + get "/u/password-reset/#{email_token.token}" - get "/u/password-reset/#{token}" - - put "/u/password-reset/#{token}", params: { + put "/u/password-reset/#{email_token.token}", params: { password: 'hg9ow8yHG32O', second_factor_token: ROTP::TOTP.new(second_factor.data).now, second_factor_method: UserSecondFactor.methods[:totp] @@ -424,13 +377,12 @@ describe UsersController do public_key: valid_security_key_data[:public_key] ) end - let(:token) { user.email_tokens.create!(email: user.email).token } before do simulate_localhost_webauthn_challenge # store challenge in secure session by visiting the email login page - get "/u/password-reset/#{token}" + get "/u/password-reset/#{email_token.token}" end it 'preloads with a security key challenge and allowed credential ids' do @@ -450,34 +402,32 @@ describe UsersController do end it 'changes password with valid security key challenge and authentication' do - put "/u/password-reset/#{token}.json", params: { + put "/u/password-reset/#{email_token.token}.json", params: { password: 'hg9ow8yHG32O', second_factor_token: valid_security_key_auth_post_data, second_factor_method: UserSecondFactor.methods[:security_key] } - user.reload expect(response.status).to eq(200) - + user.reload expect(user.confirm_password?('hg9ow8yHG32O')).to eq(true) expect(user.user_auth_tokens.count).to eq(1) end it "does not change a password if a fake TOTP token is provided" do - put "/u/password-reset/#{token}.json", params: { + put "/u/password-reset/#{email_token.token}.json", params: { password: 'hg9ow8yHG32O', second_factor_token: 'blah', second_factor_method: UserSecondFactor.methods[:security_key] } - user.reload expect(response.status).to eq(200) - expect(user.confirm_password?('hg9ow8yHG32O')).to eq(false) + expect(user.reload.confirm_password?('hg9ow8yHG32O')).to eq(false) end context "when security key authentication fails" do it 'shows an error message and does not change password' do - put "/u/password-reset/#{token}", params: { + put "/u/password-reset/#{email_token.token}", params: { password: 'hg9ow8yHG32O', second_factor_token: { signature: 'bad', @@ -488,24 +438,19 @@ describe UsersController do second_factor_method: UserSecondFactor.methods[:security_key] } - user.reload - expect(user.confirm_password?('hg9ow8yHG32O')).to eq(false) expect(response.status).to eq(200) expect(response.body).to include(I18n.t("webauthn.validation.not_found_error")) + expect(user.reload.confirm_password?('hg9ow8yHG32O')).to eq(false) end end end end context 'submit change' do - let(:token) { EmailToken.generate_token } - - before do - EmailToken.expects(:confirm).with(token).returns(user) - end + let(:email_token) { Fabricate(:email_token, user: user, scope: EmailToken.scopes[:password_reset]) } it "fails when the password is blank" do - put "/u/password-reset/#{token}.json", params: { password: '' } + put "/u/password-reset/#{email_token.token}.json", params: { password: '' } expect(response.status).to eq(200) expect(response.parsed_body["errors"]).to be_present @@ -513,7 +458,7 @@ describe UsersController do end it "fails when the password is too long" do - put "/u/password-reset/#{token}.json", params: { password: ('x' * (User.max_password_length + 1)) } + put "/u/password-reset/#{email_token.token}.json", params: { password: ('x' * (User.max_password_length + 1)) } expect(response.status).to eq(200) expect(response.parsed_body["errors"]).to be_present @@ -521,7 +466,7 @@ describe UsersController do end it "logs in the user" do - put "/u/password-reset/#{token}.json", params: { password: 'ksjafh928r' } + put "/u/password-reset/#{email_token.token}.json", params: { password: 'ksjafh928r' } expect(response.status).to eq(200) expect(response.parsed_body["errors"]).to be_blank @@ -530,8 +475,9 @@ describe UsersController do it "doesn't log in the user when not approved" do SiteSetting.must_approve_users = true + user.update!(approved: false) - put "/u/password-reset/#{token}.json", params: { password: 'ksjafh928r' } + put "/u/password-reset/#{email_token.token}.json", params: { password: 'ksjafh928r' } expect(response.parsed_body["errors"]).to be_blank expect(session[:current_user_id]).to be_blank end @@ -540,16 +486,15 @@ describe UsersController do describe '#confirm_email_token' do fab!(:user) { Fabricate(:user) } + let!(:email_token) { Fabricate(:email_token, user: user) } it "token doesn't match any records" do - email_token = user.email_tokens.create(email: user.email) get "/u/confirm-email-token/#{SecureRandom.hex}.json" expect(response.status).to eq(200) expect(email_token.reload.confirmed).to eq(false) end it "token matches" do - email_token = user.email_tokens.create(email: user.email) get "/u/confirm-email-token/#{email_token.token}.json" expect(response.status).to eq(200) expect(email_token.reload.confirmed).to eq(true) @@ -833,18 +778,13 @@ describe UsersController do expect(Jobs::SendSystemMessage.jobs.size).to eq(0) expect(response.status).to eq(200) - json = response.parsed_body - - new_user = User.find(json["user_id"]) - email_token = new_user.email_tokens.active.where(email: new_user.email).first - - expect(json['active']).to be_truthy - + expect(response.parsed_body['active']).to be_truthy + new_user = User.find(response.parsed_body["user_id"]) expect(new_user.active).to eq(true) expect(new_user.approved).to eq(true) expect(new_user.approved_by_id).to eq(admin.id) expect(new_user.approved_at).to_not eq(nil) - expect(email_token.confirmed?).to eq(true) + expect(new_user.email_tokens.where(confirmed: true, email: new_user.email)).to exist end it "will create a reviewable when a user is created as active but not approved" do @@ -2358,7 +2298,7 @@ describe UsersController do context 'for an activated account with email confirmed' do it 'fails' do user = post_user - email_token = user.email_tokens.create(email: user.email).token + email_token = Fabricate(:email_token, user: user).token EmailToken.confirm(email_token) post "/u/action/send_activation_email.json", params: { username: user.username } @@ -2375,7 +2315,7 @@ describe UsersController do it 'should send an email' do user = post_user user.update!(active: true) - user.email_tokens.create!(email: user.email) + Fabricate(:email_token, user: user) expect_enqueued_with(job: :critical_user_email, args: { type: :signup, to_address: user.email }) do post "/u/action/send_activation_email.json", params: { @@ -2398,7 +2338,7 @@ describe UsersController do user = post_user user.update(active: true) user.save! - user.email_tokens.create(email: user.email) + Fabricate(:email_token, user: user) post "/u/action/send_activation_email.json", params: { username: user.username } @@ -4289,10 +4229,9 @@ describe UsersController do expect(response.parsed_body['user_found']).to eq(true) job_args = Jobs::CriticalUserEmail.jobs.last["args"].first - expect(job_args["user_id"]).to eq(user.id) expect(job_args["type"]).to eq("email_login") - expect(job_args["email_token"]).to eq(user.email_tokens.last.token) + expect(EmailToken.hash_token(job_args["email_token"])).to eq(user.email_tokens.last.token_hash) end describe 'when enable_local_logins_via_email is disabled' do diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb index 046e6188573..0b65943f7c5 100644 --- a/spec/requests/users_email_controller_spec.rb +++ b/spec/requests/users_email_controller_spec.rb @@ -6,18 +6,19 @@ require 'rotp' describe UsersEmailController do fab!(:user) { Fabricate(:user) } + let!(:email_token) { Fabricate(:email_token, user: user) } fab!(:moderator) { Fabricate(:moderator) } describe "#confirm-new-email" do it 'does not redirect to login for signed out accounts, this route works fine as anon user' do - get "/u/confirm-new-email/asdfasdf" + get "/u/confirm-new-email/invalidtoken" expect(response.status).to eq(200) end it 'does not redirect to login for signed out accounts on login_required sites, this route works fine as anon user' do SiteSetting.login_required = true - get "/u/confirm-new-email/asdfasdf" + get "/u/confirm-new-email/invalidtoken" expect(response.status).to eq(200) end @@ -25,7 +26,7 @@ describe UsersEmailController do it 'errors out for invalid tokens' do sign_in(user) - get "/u/confirm-new-email/asdfasdf" + get "/u/confirm-new-email/invalidtoken" expect(response.status).to eq(200) expect(response.body).to include(I18n.t('change_email.already_done')) @@ -33,18 +34,14 @@ describe UsersEmailController do it 'does not change email if accounts mismatch for a signed in user' do updater = EmailUpdater.new(guardian: user.guardian, user: user) - updater.change_to('new.n.cool@example.com') + updater.change_to('bubblegum@adventuretime.ooo') old_email = user.email sign_in(moderator) - put "/u/confirm-new-email", params: { - token: "#{user.email_tokens.last.token}" - } - - user.reload - expect(user.email).to eq(old_email) + put "/u/confirm-new-email", params: { token: "#{email_token.token}" } + expect(user.reload.email).to eq(old_email) end context "with a valid user" do @@ -52,14 +49,14 @@ describe UsersEmailController do before do sign_in(user) - updater.change_to('new.n.cool@example.com') + updater.change_to('bubblegum@adventuretime.ooo') end it 'includes security_key_allowed_credential_ids in a hidden field' do key1 = Fabricate(:user_security_key_with_random_credential, user: user) key2 = Fabricate(:user_security_key_with_random_credential, user: user) - get "/u/confirm-new-email/#{user.email_tokens.last.token}" + get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}" doc = Nokogiri::HTML5(response.body) credential_ids = doc.css("#security-key-allowed-credential-ids").first["value"].split(",") @@ -70,17 +67,15 @@ describe UsersEmailController do user.user_stat.update_columns(bounce_score: 42, reset_bounce_score_after: 1.week.from_now) put "/u/confirm-new-email", params: { - token: "#{user.email_tokens.last.token}" + token: "#{updater.change_req.new_email_token.token}" } expect(response.status).to eq(302) expect(response.redirect_url).to include("done") - user.reload - expect(user.user_stat.bounce_score).to eq(0) expect(user.user_stat.reset_bounce_score_after).to eq(nil) - expect(user.email).to eq("new.n.cool@example.com") + expect(user.email).to eq('bubblegum@adventuretime.ooo') end context 'second factor required' do @@ -88,29 +83,23 @@ describe UsersEmailController do fab!(:backup_code) { Fabricate(:user_second_factor_backup, user: user) } it 'requires a second factor token' do - get "/u/confirm-new-email/#{user.email_tokens.last.token}" + get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}" expect(response.status).to eq(200) - - response_body = response.body - - expect(response_body).to include(I18n.t("login.second_factor_title")) - expect(response_body).not_to include(I18n.t("login.invalid_second_factor_code")) + expect(response.body).to include(I18n.t("login.second_factor_title")) + expect(response.body).not_to include(I18n.t("login.invalid_second_factor_code")) end it 'requires a backup token' do - get "/u/confirm-new-email/#{user.email_tokens.last.token}?show_backup=true" + get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}?show_backup=true" expect(response.status).to eq(200) - - response_body = response.body - - expect(response_body).to include(I18n.t("login.second_factor_backup_title")) + expect(response.body).to include(I18n.t("login.second_factor_backup_title")) end it 'adds an error on a second factor attempt' do put "/u/confirm-new-email", params: { - token: user.email_tokens.last.token, + token: updater.change_req.new_email_token.token, second_factor_token: "000000", second_factor_method: UserSecondFactor.methods[:totp] } @@ -123,13 +112,11 @@ describe UsersEmailController do put "/u/confirm-new-email", params: { second_factor_token: ROTP::TOTP.new(second_factor.data).now, second_factor_method: UserSecondFactor.methods[:totp], - token: user.email_tokens.last.token + token: updater.change_req.new_email_token.token } expect(response.status).to eq(302) - - user.reload - expect(user.email).to eq("new.n.cool@example.com") + expect(user.reload.email).to eq('bubblegum@adventuretime.ooo') end context "rate limiting" do @@ -163,7 +150,7 @@ describe UsersEmailController do 6.times do |x| user.email_change_requests.last.update(change_state: EmailChangeRequest.states[:complete]) put "/u/confirm-new-email", params: { - token: user.email_tokens.last.token, + token: updater.change_req.new_email_token.token, second_factor_token: "000000", second_factor_method: UserSecondFactor.methods[:totp] }, env: { "REMOTE_ADDR": "1.2.3.#{x}" } @@ -173,7 +160,7 @@ describe UsersEmailController do user.email_change_requests.last.update(change_state: EmailChangeRequest.states[:authorizing_new]) put "/u/confirm-new-email", params: { - token: user.email_tokens.last.token, + token: updater.change_req.new_email_token.token, second_factor_token: "000000", second_factor_method: UserSecondFactor.methods[:totp] }, env: { "REMOTE_ADDR": "1.2.3.4" } @@ -198,14 +185,11 @@ describe UsersEmailController do end it 'requires a security key' do - get "/u/confirm-new-email/#{user.email_tokens.last.token}" + get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}" expect(response.status).to eq(200) - - response_body = response.body - - expect(response_body).to include(I18n.t("login.security_key_authenticate")) - expect(response_body).to include(I18n.t("login.security_key_description")) + expect(response.body).to include(I18n.t("login.security_key_authenticate")) + expect(response.body).to include(I18n.t("login.security_key_description")) end context "if the user has a TOTP enabled and wants to use that instead" do @@ -214,21 +198,18 @@ describe UsersEmailController do end it 'allows entering the totp code instead' do - get "/u/confirm-new-email/#{user.email_tokens.last.token}?show_totp=true" + get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}?show_totp=true" expect(response.status).to eq(200) - - response_body = response.body - - expect(response_body).to include(I18n.t("login.second_factor_title")) - expect(response_body).not_to include(I18n.t("login.security_key_authenticate")) + expect(response.body).to include(I18n.t("login.second_factor_title")) + expect(response.body).not_to include(I18n.t("login.security_key_authenticate")) end end it 'adds an error on a security key attempt' do - get "/u/confirm-new-email/#{user.email_tokens.last.token}" + get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}" put "/u/confirm-new-email", params: { - token: user.email_tokens.last.token, + token: updater.change_req.new_email_token.token, second_factor_token: "{}", second_factor_method: UserSecondFactor.methods[:security_key] } @@ -238,26 +219,24 @@ describe UsersEmailController do end it 'confirms with a correct security key token' do - get "/u/confirm-new-email/#{user.email_tokens.last.token}" + get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}" put "/u/confirm-new-email", params: { + token: updater.change_req.new_email_token.token, second_factor_token: valid_security_key_auth_post_data.to_json, - second_factor_method: UserSecondFactor.methods[:security_key], - token: user.email_tokens.last.token + second_factor_method: UserSecondFactor.methods[:security_key] } expect(response.status).to eq(302) - - user.reload - expect(user.email).to eq("new.n.cool@example.com") + expect(user.reload.email).to eq('bubblegum@adventuretime.ooo') end context "if the security key data JSON is garbled" do it "raises an invalid parameters error" do - get "/u/confirm-new-email/#{user.email_tokens.last.token}" + get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}" put "/u/confirm-new-email", params: { + token: updater.change_req.new_email_token.token, second_factor_token: "{someweird: 8notjson}", - second_factor_method: UserSecondFactor.methods[:security_key], - token: user.email_tokens.last.token + second_factor_method: UserSecondFactor.methods[:security_key] } expect(response.status).to eq(400) @@ -268,9 +247,8 @@ describe UsersEmailController do end describe '#confirm-old-email' do - it 'redirects to login for signed out accounts' do - get "/u/confirm-old-email/asdfasdf" + get "/u/confirm-old-email/invalidtoken" expect(response.status).to eq(302) expect(response.redirect_url).to eq("http://test.localhost/login") @@ -279,75 +257,62 @@ describe UsersEmailController do it 'errors out for invalid tokens' do sign_in(user) - get "/u/confirm-old-email/asdfasdf" + get "/u/confirm-old-email/invalidtoken" expect(response.status).to eq(200) expect(response.body).to include(I18n.t('change_email.already_done')) end it 'bans change when accounts do not match' do - sign_in(user) updater = EmailUpdater.new(guardian: moderator.guardian, user: moderator) - updater.change_to('new.n.cool@example.com') + email_change_request = updater.change_to('bubblegum@adventuretime.ooo') - get "/u/confirm-old-email/#{moderator.email_tokens.last.token}" + get "/u/confirm-old-email/#{email_change_request.old_email_token.token}" expect(response.status).to eq(200) expect(body).to include("alert-error") end - context 'valid old address token' do - + context 'valid old token' do it 'confirms with a correct token' do - # NOTE: only moderators need to confirm both old and new sign_in(moderator) updater = EmailUpdater.new(guardian: moderator.guardian, user: moderator) - updater.change_to('new.n.cool@example.com') + email_change_request = updater.change_to('bubblegum@adventuretime.ooo') - get "/u/confirm-old-email/#{moderator.email_tokens.last.token}" + get "/u/confirm-old-email/#{email_change_request.old_email_token.token}" expect(response.status).to eq(200) - body = CGI.unescapeHTML(response.body) - - expect(body) - .to include(I18n.t('change_email.authorizing_old.title')) - - expect(body) - .to include(I18n.t('change_email.authorizing_old.description')) + expect(body).to include(I18n.t('change_email.authorizing_old.title')) + expect(body).to include(I18n.t('change_email.authorizing_old.description')) put "/u/confirm-old-email", params: { - token: moderator.email_tokens.last.token + token: email_change_request.old_email_token.token } expect(response.status).to eq(302) expect(response.redirect_url).to include("done=true") - end end end describe '#create' do - let(:new_email) { 'bubblegum@adventuretime.ooo' } - it 'has an email token' do sign_in(user) - expect { post "/u/#{user.username}/preferences/email.json", params: { email: new_email } } + expect { post "/u/#{user.username}/preferences/email.json", params: { email: 'bubblegum@adventuretime.ooo' } } .to change(EmailChangeRequest, :count) emailChangeRequest = EmailChangeRequest.last expect(emailChangeRequest.old_email).to eq(nil) - expect(emailChangeRequest.new_email).to eq(new_email) + expect(emailChangeRequest.new_email).to eq('bubblegum@adventuretime.ooo') end end describe '#update' do - let(:new_email) { 'bubblegum@adventuretime.ooo' } - it "requires you to be logged in" do - put "/u/#{user.username}/preferences/email.json", params: { email: new_email } + put "/u/#{user.username}/preferences/email.json", params: { email: 'bubblegum@adventuretime.ooo' } expect(response.status).to eq(403) end @@ -368,10 +333,9 @@ describe UsersEmailController do end it "raises an error if you can't edit the user's email" do - Guardian.any_instance.expects(:can_edit_email?).with(user).returns(false) - - put "/u/#{user.username}/preferences/email.json", params: { email: new_email } + SiteSetting.email_editable = false + put "/u/#{user.username}/preferences/email.json", params: { email: 'bubblegum@adventuretime.ooo' } expect(response).to be_forbidden end @@ -384,18 +348,12 @@ describe UsersEmailController do end it 'raises an error' do - put "/u/#{user.username}/preferences/email.json", params: { - email: other_user.email - } - + put "/u/#{user.username}/preferences/email.json", params: { email: other_user.email } expect(response).to_not be_successful end it 'raises an error if there is whitespace too' do - put "/u/#{user.username}/preferences/email.json", params: { - email: "#{other_user.email} " - } - + put "/u/#{user.username}/preferences/email.json", params: { email: "#{other_user.email} " } expect(response).to_not be_successful end end @@ -406,10 +364,7 @@ describe UsersEmailController do end it 'responds with success' do - put "/u/#{user.username}/preferences/email.json", params: { - email: other_user.email - } - + put "/u/#{user.username}/preferences/email.json", params: { email: other_user.email } expect(response.status).to eq(200) end end @@ -419,10 +374,7 @@ describe UsersEmailController do fab!(:other_user) { Fabricate(:user, email: 'case.insensitive@gmail.com') } it 'raises an error' do - put "/u/#{user.username}/preferences/email.json", params: { - email: other_user.email.upcase - } - + put "/u/#{user.username}/preferences/email.json", params: { email: other_user.email.upcase } expect(response).to_not be_successful end end @@ -430,34 +382,24 @@ describe UsersEmailController do it 'raises an error when new email domain is present in blocked_email_domains site setting' do SiteSetting.blocked_email_domains = "mailinator.com" - put "/u/#{user.username}/preferences/email.json", params: { - email: "not_good@mailinator.com" - } - + put "/u/#{user.username}/preferences/email.json", params: { email: "not_good@mailinator.com" } expect(response).to_not be_successful end it 'raises an error when new email domain is not present in allowed_email_domains site setting' do SiteSetting.allowed_email_domains = "discourse.org" - put "/u/#{user.username}/preferences/email.json", params: { - email: new_email - } - + put "/u/#{user.username}/preferences/email.json", params: { email: 'bubblegum@adventuretime.ooo' } expect(response).to_not be_successful end context 'success' do it 'has an email token' do expect do - put "/u/#{user.username}/preferences/email.json", params: { - email: new_email - } + put "/u/#{user.username}/preferences/email.json", params: { email: 'bubblegum@adventuretime.ooo' } end.to change(EmailChangeRequest, :count) end end end - end - end diff --git a/spec/services/user_activator_spec.rb b/spec/services/user_activator_spec.rb index adf3191bab9..11e1775da5d 100644 --- a/spec/services/user_activator_spec.rb +++ b/spec/services/user_activator_spec.rb @@ -3,42 +3,21 @@ require 'rails_helper' describe UserActivator do + fab!(:user) { Fabricate(:user) } + let!(:email_token) { Fabricate(:email_token, user: user) } describe 'email_activator' do + let(:activator) { EmailActivator.new(user, nil, nil, nil) } - it 'does not create new email token unless required' do - SiteSetting.email_token_valid_hours = 24 - user = Fabricate(:user) - activator = EmailActivator.new(user, nil, nil, nil) - - expect_enqueued_with(job: :critical_user_email, args: { type: :signup, email_token: user.email_tokens.first.token }) do - activator.activate - end - end - - it 'creates and send new email token if the existing token expired' do + it 'create email token and enqueues user email' do now = freeze_time - - SiteSetting.email_token_valid_hours = 24 - user = Fabricate(:user) - email_token = user.email_tokens.first - email_token.update_column(:created_at, 48.hours.ago) - activator = EmailActivator.new(user, nil, nil, nil) - - expect_not_enqueued_with(job: :critical_user_email, args: { type: :signup, user_id: user.id, email_token: email_token.token }) do - activator.activate - end - + activator.activate email_token = user.reload.email_tokens.last - - expect(job_enqueued?(job: :critical_user_email, args: { - type: :signup, - user_id: user.id, - email_token: email_token.token - })).to eq(true) - expect(email_token.created_at).to eq_time(now) + job_args = Jobs::CriticalUserEmail.jobs.last["args"].first + expect(job_args["user_id"]).to eq(user.id) + expect(job_args["type"]).to eq("signup") + expect(EmailToken.hash_token(job_args["email_token"])).to eq(email_token.token_hash) end - end end