From 219a4423d8371fc89f122f3ef4874e9121b423f7 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Tue, 10 Apr 2018 09:16:06 +0200 Subject: [PATCH] Feature: Allow staff to change user emails (#7074) * Admin: Show unconfirmed email address on account page * Admin: Allow staff to change user email addresses * ActionLog: On change_email, log current email address and new unconfirmed email address --- .../admin/change_emails_controller.rb | 49 +++++++++++++++++++ app/helpers/admin/action_logs_helper.rb | 4 +- app/models/account.rb | 1 + app/models/admin/action_log.rb | 5 ++ app/policies/user_policy.rb | 4 ++ app/views/admin/accounts/show.html.haml | 6 ++- app/views/admin/change_emails/show.html.haml | 7 +++ config/locales/en.yml | 9 ++++ config/routes.rb | 1 + .../admin/change_email_controller_spec.rb | 47 ++++++++++++++++++ 10 files changed, 131 insertions(+), 2 deletions(-) create mode 100644 app/controllers/admin/change_emails_controller.rb create mode 100644 app/views/admin/change_emails/show.html.haml create mode 100644 spec/controllers/admin/change_email_controller_spec.rb diff --git a/app/controllers/admin/change_emails_controller.rb b/app/controllers/admin/change_emails_controller.rb new file mode 100644 index 000000000..a689d3a53 --- /dev/null +++ b/app/controllers/admin/change_emails_controller.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module Admin + class ChangeEmailsController < BaseController + before_action :set_account + before_action :require_local_account! + + def show + authorize @user, :change_email? + end + + def update + authorize @user, :change_email? + + new_email = resource_params.fetch(:unconfirmed_email) + + if new_email != @user.email + @user.update!( + unconfirmed_email: new_email, + # Regenerate the confirmation token: + confirmation_token: nil + ) + + log_action :change_email, @user + + @user.send_confirmation_instructions + end + + redirect_to admin_account_path(@account.id), notice: I18n.t('admin.accounts.change_email.changed_msg') + end + + private + + def set_account + @account = Account.find(params[:account_id]) + @user = @account.user + end + + def require_local_account! + redirect_to admin_account_path(@account.id) unless @account.local? && @account.user.present? + end + + def resource_params + params.require(:user).permit( + :unconfirmed_email + ) + end + end +end diff --git a/app/helpers/admin/action_logs_helper.rb b/app/helpers/admin/action_logs_helper.rb index 7c26c0b05..4c663211e 100644 --- a/app/helpers/admin/action_logs_helper.rb +++ b/app/helpers/admin/action_logs_helper.rb @@ -45,6 +45,8 @@ module Admin::ActionLogsHelper log.recorded_changes.slice('domain', 'visible_in_picker') elsif log.target_type == 'User' && [:promote, :demote].include?(log.action) log.recorded_changes.slice('moderator', 'admin') + elsif log.target_type == 'User' && [:change_email].include?(log.action) + log.recorded_changes.slice('email', 'unconfirmed_email') elsif log.target_type == 'DomainBlock' log.recorded_changes.slice('severity', 'reject_media') elsif log.target_type == 'Status' && log.action == :update @@ -84,7 +86,7 @@ module Admin::ActionLogsHelper 'positive' when :create opposite_verbs?(log) ? 'negative' : 'positive' - when :update, :reset_password, :disable_2fa, :memorialize + when :update, :reset_password, :disable_2fa, :memorialize, :change_email 'neutral' when :demote, :silence, :disable, :suspend, :remove_avatar, :reopen 'negative' diff --git a/app/models/account.rb b/app/models/account.rb index 446144a3e..51304fc18 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -124,6 +124,7 @@ class Account < ApplicationRecord scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) } delegate :email, + :unconfirmed_email, :current_sign_in_ip, :current_sign_in_at, :confirmed?, diff --git a/app/models/admin/action_log.rb b/app/models/admin/action_log.rb index c437c8ee8..81f278e07 100644 --- a/app/models/admin/action_log.rb +++ b/app/models/admin/action_log.rb @@ -35,6 +35,11 @@ class Admin::ActionLog < ApplicationRecord self.recorded_changes = target.attributes when :update, :promote, :demote self.recorded_changes = target.previous_changes + when :change_email + self.recorded_changes = ActiveSupport::HashWithIndifferentAccess.new( + email: [target.email, nil], + unconfirmed_email: [nil, target.unconfirmed_email] + ) end end end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index aae207d06..dabdf707a 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -5,6 +5,10 @@ class UserPolicy < ApplicationPolicy staff? && !record.staff? end + def change_email? + staff? && !record.staff? + end + def disable_2fa? admin? && !record.staff? end diff --git a/app/views/admin/accounts/show.html.haml b/app/views/admin/accounts/show.html.haml index fecfd6cc8..7312618ee 100644 --- a/app/views/admin/accounts/show.html.haml +++ b/app/views/admin/accounts/show.html.haml @@ -36,9 +36,13 @@ %th= t('admin.accounts.email') %td = @account.user_email - - if @account.user_confirmed? = fa_icon('check') + = table_link_to 'edit', t('admin.accounts.change_email.label'), admin_account_change_email_path(@account.id) if can?(:change_email, @account.user) + - if @account.user_unconfirmed_email.present? + %th= t('admin.accounts.unconfirmed_email') + %td + = @account.user_unconfirmed_email %tr %th= t('admin.accounts.login_status') %td diff --git a/app/views/admin/change_emails/show.html.haml b/app/views/admin/change_emails/show.html.haml new file mode 100644 index 000000000..a661b1ad6 --- /dev/null +++ b/app/views/admin/change_emails/show.html.haml @@ -0,0 +1,7 @@ +- content_for :page_title do + = t('admin.accounts.change_email.title', username: @account.acct) + += simple_form_for @user, url: admin_account_change_email_path(@account.id) do |f| + = f.input :email, wrapper: :with_label, disabled: true, label: t('admin.accounts.change_email.current_email') + = f.input :unconfirmed_email, wrapper: :with_label, label: t('admin.accounts.change_email.new_email') + = f.button :submit, class: "button", value: t('admin.accounts.change_email.submit') diff --git a/config/locales/en.yml b/config/locales/en.yml index 70af9530c..11f3fb924 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -63,6 +63,13 @@ en: are_you_sure: Are you sure? avatar: Avatar by_domain: Domain + change_email: + changed_msg: Account email successfully changed! + current_email: Current Email + label: Change Email + new_email: New Email + submit: Change Email + title: Change Email for %{username} confirm: Confirm confirmed: Confirmed demote: Demote @@ -131,6 +138,7 @@ en: statuses: Statuses subscribe: Subscribe title: Accounts + unconfirmed_email: Unconfirmed E-mail undo_silenced: Undo silence undo_suspension: Undo suspension unsubscribe: Unsubscribe @@ -139,6 +147,7 @@ en: action_logs: actions: assigned_to_self_report: "%{name} assigned report %{target} to themselves" + change_email_user: "%{name} changed the e-mail address of user %{target}" confirm_user: "%{name} confirmed e-mail address of user %{target}" create_custom_emoji: "%{name} uploaded new emoji %{target}" create_domain_block: "%{name} blocked domain %{target}" diff --git a/config/routes.rb b/config/routes.rb index 7187fd743..2776898ac 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -151,6 +151,7 @@ Rails.application.routes.draw do post :memorialize end + resource :change_email, only: [:show, :update] resource :reset, only: [:create] resource :silence, only: [:create, :destroy] resource :suspension, only: [:create, :destroy] diff --git a/spec/controllers/admin/change_email_controller_spec.rb b/spec/controllers/admin/change_email_controller_spec.rb new file mode 100644 index 000000000..50f94f835 --- /dev/null +++ b/spec/controllers/admin/change_email_controller_spec.rb @@ -0,0 +1,47 @@ +require 'rails_helper' + +RSpec.describe Admin::ChangeEmailsController, type: :controller do + render_views + + let(:admin) { Fabricate(:user, admin: true) } + + before do + sign_in admin + end + + describe "GET #show" do + it "returns http success" do + account = Fabricate(:account) + user = Fabricate(:user, account: account) + + get :show, params: { account_id: account.id } + + expect(response).to have_http_status(:success) + end + end + + describe "GET #update" do + before do + allow(UserMailer).to receive(:confirmation_instructions).and_return(double('email', deliver_later: nil)) + end + + it "returns http success" do + account = Fabricate(:account) + user = Fabricate(:user, account: account) + + previous_email = user.email + + post :update, params: { account_id: account.id, user: { unconfirmed_email: 'test@example.com' } } + + user.reload + + expect(user.email).to eq previous_email + expect(user.unconfirmed_email).to eq 'test@example.com' + expect(user.confirmation_token).not_to be_nil + + expect(UserMailer).to have_received(:confirmation_instructions).with(user, user.confirmation_token, { to: 'test@example.com' }) + + expect(response).to redirect_to(admin_account_path(account.id)) + end + end +end