From 17f2974d0a99a6c38b1aa90abe68c9f8b69dc567 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 4 Apr 2017 13:59:22 -0400 Subject: [PATCH] SECURITY: Confirm new administrator accounts via email --- .../admin/models/admin-user.js.es6 | 18 ++--- app/controllers/admin/users_controller.rb | 8 +-- app/controllers/users_controller.rb | 21 +++++- app/jobs/regular/admin_confirmation_email.rb | 21 ++++++ app/mailers/admin_confirmation_mailer.rb | 15 +++++ app/views/users/confirm_admin.html.erb | 20 ++++++ config/locales/client.en.yml | 1 + config/locales/server.en.yml | 14 ++++ config/routes.rb | 5 ++ lib/admin_confirmation.rb | 59 +++++++++++++++++ spec/components/admin_confirmation_spec.rb | 54 +++++++++++++++ .../admin/users_controller_spec.rb | 12 +++- spec/controllers/users_controller_spec.rb | 65 +++++++++++++++++++ 13 files changed, 293 insertions(+), 20 deletions(-) create mode 100644 app/jobs/regular/admin_confirmation_email.rb create mode 100644 app/mailers/admin_confirmation_mailer.rb create mode 100644 app/views/users/confirm_admin.html.erb create mode 100644 lib/admin_confirmation.rb create mode 100644 spec/components/admin_confirmation_spec.rb diff --git a/app/assets/javascripts/admin/models/admin-user.js.es6 b/app/assets/javascripts/admin/models/admin-user.js.es6 index 4c952d953f8..b1e1202a906 100644 --- a/app/assets/javascripts/admin/models/admin-user.js.es6 +++ b/app/assets/javascripts/admin/models/admin-user.js.es6 @@ -115,11 +115,10 @@ const AdminUser = Discourse.User.extend({ }, revokeAdmin() { - const self = this; - return ajax("/admin/users/" + this.get('id') + "/revoke_admin", { + return ajax(`/admin/users/${this.get('id')}/revoke_admin`, { type: 'PUT' - }).then(function() { - self.setProperties({ + }).then(() => { + this.setProperties({ admin: false, can_grant_admin: true, can_revoke_admin: false @@ -128,15 +127,10 @@ const AdminUser = Discourse.User.extend({ }, grantAdmin() { - const self = this; - return ajax("/admin/users/" + this.get('id') + "/grant_admin", { + return ajax(`/admin/users/${this.get('id')}/grant_admin`, { type: 'PUT' - }).then(function() { - self.setProperties({ - admin: true, - can_grant_admin: false, - can_revoke_admin: true - }); + }).then(() => { + bootbox.alert(I18n.t("admin.user.grant_admin_confirm")); }).catch(popupAjaxError); }, diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index c0e97911169..3f007d48c51 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -1,5 +1,6 @@ require_dependency 'user_destroyer' require_dependency 'admin_user_index_query' +require_dependency 'admin_confirmation' class Admin::UsersController < Admin::AdminController @@ -103,10 +104,8 @@ class Admin::UsersController < Admin::AdminController end def grant_admin - guardian.ensure_can_grant_admin!(@user) - @user.grant_admin! - StaffActionLogger.new(current_user).log_grant_admin(@user) - render_serialized(@user, AdminUserSerializer) + AdminConfirmation.new(@user, current_user).create_confirmation + render json: success_json end def revoke_moderation @@ -321,6 +320,7 @@ class Admin::UsersController < Admin::AdminController end def invite_admin + raise Discourse::InvalidAccess.new unless is_api? email = params[:email] unless user = User.find_by_email(email) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 28f53334e13..7ca6f0d3801 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -3,11 +3,12 @@ require_dependency 'user_name_suggester' require_dependency 'rate_limiter' require_dependency 'wizard' require_dependency 'wizard/builder' +require_dependency 'admin_confirmation' class UsersController < ApplicationController skip_before_filter :authorize_mini_profiler, only: [:avatar] - skip_before_filter :check_xhr, only: [:show, :password_reset, :update, :account_created, :activate_account, :perform_account_activation, :user_preferences_redirect, :avatar, :my_redirect, :toggle_anon, :admin_login] + skip_before_filter :check_xhr, only: [:show, :password_reset, :update, :account_created, :activate_account, :perform_account_activation, :user_preferences_redirect, :avatar, :my_redirect, :toggle_anon, :admin_login, :confirm_admin] before_filter :ensure_logged_in, only: [:username, :update, :user_preferences_redirect, :upload_user_image, :pick_avatar, :destroy_user_image, :destroy, :check_emails, :topic_tracking_state] @@ -27,7 +28,8 @@ class UsersController < ApplicationController :send_activation_email, :password_reset, :confirm_email_token, - :admin_login] + :admin_login, + :confirm_admin] def index end @@ -726,6 +728,21 @@ class UsersController < ApplicationController render json: result end + def confirm_admin + @confirmation = AdminConfirmation.find_by_code(params[:token]) + + raise Discourse::NotFound unless @confirmation + raise Discourse::InvalidAccess.new unless + @confirmation.performed_by.id == (current_user&.id || @confirmation.performed_by.id) + + if request.post? + @confirmation.email_confirmed! + @confirmed = true + end + + render layout: 'no_ember' + end + private def honeypot_value diff --git a/app/jobs/regular/admin_confirmation_email.rb b/app/jobs/regular/admin_confirmation_email.rb new file mode 100644 index 00000000000..dbebf609629 --- /dev/null +++ b/app/jobs/regular/admin_confirmation_email.rb @@ -0,0 +1,21 @@ +require_dependency 'email/sender' + +module Jobs + class AdminConfirmationEmail < Jobs::Base + sidekiq_options queue: 'critical' + + def execute(args) + to_address = args[:to_address] + token = args[:token] + target_username = args[:target_username] + + raise Discourse::InvalidParameters.new(:to_address) if to_address.blank? + raise Discourse::InvalidParameters.new(:token) if token.blank? + raise Discourse::InvalidParameters.new(:target_username) if target_username.blank? + + message = AdminConfirmationMailer.send_email(to_address, target_username, token) + Email::Sender.new(message, :admin_confirmation_message).send + end + + end +end diff --git a/app/mailers/admin_confirmation_mailer.rb b/app/mailers/admin_confirmation_mailer.rb new file mode 100644 index 00000000000..ea4d444ad8c --- /dev/null +++ b/app/mailers/admin_confirmation_mailer.rb @@ -0,0 +1,15 @@ +require_dependency 'email/message_builder' + +class AdminConfirmationMailer < ActionMailer::Base + include Email::BuildEmailHelper + + def send_email(to_address, target_username, token) + build_email( + to_address, + template: 'admin_confirmation_mailer', + target_username: target_username, + admin_confirm_url: confirm_admin_url(token: token, host: Discourse.base_url_no_prefix) + ) + end +end + diff --git a/app/views/users/confirm_admin.html.erb b/app/views/users/confirm_admin.html.erb new file mode 100644 index 00000000000..4e996da74a7 --- /dev/null +++ b/app/views/users/confirm_admin.html.erb @@ -0,0 +1,20 @@ +
+

<%= t 'activation.admin_confirm.title' %>

+
+ + <%- unless @confirmed %> + <%=raw (t('activation.admin_confirm.description', target_username: @confirmation.target_user.username)) %> +
+
+ + <%= form_tag do %> + <%= submit_tag t('activation.admin_confirm.grant'), class: 'btn btn-primary' %> + <% end %> + <% else %> + <%=raw (t('activation.admin_confirm.complete', target_username: @confirmation.target_user.username)) %> +
+
+ <%= link_to t("activation.admin_confirm.back_to", title: SiteSetting.title), "/", class: 'btn btn-primary' %> + <% end %> + +
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 9850258dcc0..ab0caede55c 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3111,6 +3111,7 @@ en: logged_out: "User was logged out on all devices" revoke_admin: 'Revoke Admin' grant_admin: 'Grant Admin' + grant_admin_confirm: "We've sent you an email to verify the new administrator. Please open it and follow the instructions." revoke_moderation: 'Revoke Moderation' grant_moderation: 'Grant Moderation' unblock: 'Unblock' diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index d83e26658a8..e952de369d0 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -624,6 +624,12 @@ en: approval_required: "A moderator must manually approve your new account before you can access this forum. You'll get an email when your account is approved!" missing_session: "We cannot detect if your account was created, please ensure you have cookies enabled." activated: "Sorry, this account has already been activated." + admin_confirm: + title: "Confirm Admin Account" + description: "Are you sure you want %{target_username} to be an administrator?" + grant: "Grant Admin Access" + complete: "%{target_username} is now an administrator." + back_to: "Return to %{title}" post_action_types: off_topic: title: 'Off-Topic' @@ -1811,6 +1817,14 @@ en: no_token: | Sorry, this backup download link has already been used or has expired. + admin_confirmation_mailer: + title: "Admin Confirmation" + subject_template: "[%{email_prefix}] Confirm new Admin Account" + text_body_template: | + Please confirm that you'd like to add **%{target_username}** as an administrator for your forum. + + [Confirm Administrator Account](%{admin_confirm_url}) + test_mailer: title: "Test Mailer" subject_template: "[%{email_prefix}] Email Deliverability Test" diff --git a/config/routes.rb b/config/routes.rb index b33b21830cb..8456bbb50b6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -322,6 +322,11 @@ Discourse::Application.routes.draw do get "#{root_path}/activate-account/:token" => "users#activate_account" put({ "#{root_path}/activate-account/:token" => "users#perform_account_activation" }.merge(index == 1 ? { as: 'perform_activate_account' } : {})) get "#{root_path}/authorize-email/:token" => "users_email#confirm" + get({ + "#{root_path}/confirm-admin/:token" => "users#confirm_admin", + constraints: { token: /[0-9a-f]+/ } + }.merge(index == 1 ? { as: 'confirm_admin' } : {})) + post "#{root_path}/confirm-admin/:token" => "users#confirm_admin", constraints: { token: /[0-9a-f]+/ } get "#{root_path}/:username/private-messages" => "user_actions#private_messages", constraints: {username: USERNAME_ROUTE_FORMAT} get "#{root_path}/:username/private-messages/:filter" => "user_actions#private_messages", constraints: {username: USERNAME_ROUTE_FORMAT} get "#{root_path}/:username/messages" => "user_actions#private_messages", constraints: {username: USERNAME_ROUTE_FORMAT} diff --git a/lib/admin_confirmation.rb b/lib/admin_confirmation.rb new file mode 100644 index 00000000000..cb756750525 --- /dev/null +++ b/lib/admin_confirmation.rb @@ -0,0 +1,59 @@ +class AdminConfirmation + attr_accessor :token + attr_reader :performed_by + attr_reader :target_user + + def initialize(target_user, performed_by) + @target_user = target_user + @performed_by = performed_by + end + + def create_confirmation + guardian = Guardian.new(@performed_by) + guardian.ensure_can_grant_admin!(@target_user) + + @token = SecureRandom.hex + $redis.setex("admin-confirmation:#{@target_user.id}", 3.hours.to_i, @token) + + payload = { + target_user_id: @target_user.id, + performed_by: @performed_by.id + } + $redis.setex("admin-confirmation-token:#{@token}", 3.hours.to_i, payload.to_json) + + Jobs.enqueue( + :admin_confirmation_email, + to_address: @performed_by.email, + target_username: @target_user.username, + token: @token + ) + end + + def email_confirmed! + guardian = Guardian.new(@performed_by) + guardian.ensure_can_grant_admin!(@target_user) + + @target_user.grant_admin! + StaffActionLogger.new(@performed_by).log_grant_admin(@target_user) + $redis.del "admin-confirmation:#{@target_user.id}" + $redis.del "admin-confirmation-token:#{@token}" + end + + def self.exists_for?(user_id) + $redis.exists "admin-confirmation:#{user_id}" + end + + def self.find_by_code(token) + json = $redis.get("admin-confirmation-token:#{token}") + return nil unless json + + parsed = JSON.parse(json) + target_user = User.find(parsed['target_user_id'].to_i) + performed_by = User.find(parsed['performed_by'].to_i) + + ac = AdminConfirmation.new(target_user, performed_by) + ac.token = token + ac + end + +end diff --git a/spec/components/admin_confirmation_spec.rb b/spec/components/admin_confirmation_spec.rb new file mode 100644 index 00000000000..20d74fec96b --- /dev/null +++ b/spec/components/admin_confirmation_spec.rb @@ -0,0 +1,54 @@ +require 'admin_confirmation' +require 'rails_helper' + +describe AdminConfirmation do + + let(:admin) { Fabricate(:admin) } + let(:user) { Fabricate(:user) } + + describe "create_confirmation" do + it "raises an error for non-admins" do + ac = AdminConfirmation.new(user, Fabricate(:moderator)) + expect { ac.create_confirmation }.to raise_error(Discourse::InvalidAccess) + end + end + + describe "email_confirmed!" do + before do + ac = AdminConfirmation.new(user, admin) + ac.create_confirmation + @token = ac.token + end + + it "cannot confirm if the user loses admin access" do + ac = AdminConfirmation.find_by_code(@token) + ac.performed_by.update_column(:admin, false) + expect { ac.email_confirmed! }.to raise_error(Discourse::InvalidAccess) + end + + it "can confirm admin accounts" do + ac = AdminConfirmation.find_by_code(@token) + expect(ac.performed_by).to eq(admin) + expect(ac.target_user).to eq(user) + expect(ac.token).to eq(@token) + ac.email_confirmed! + + user.reload + expect(user.admin?).to eq(true) + + # It creates a staff log + logs = UserHistory.where( + action: UserHistory.actions[:grant_admin], + target_user_id: user.id + ) + expect(logs).to be_present + + # It removes the redis keys for another user + expect(AdminConfirmation.find_by_code(ac.token)).to eq(nil) + expect(AdminConfirmation.exists_for?(user.id)).to eq(false) + end + + end + +end + diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 912338f262c..23bb1ebe8c7 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -166,9 +166,9 @@ describe Admin::UsersController do end it 'updates the admin flag' do + expect(AdminConfirmation.exists_for?(@another_user.id)).to eq(false) xhr :put, :grant_admin, user_id: @another_user.id - @another_user.reload - expect(@another_user).to be_admin + expect(AdminConfirmation.exists_for?(@another_user.id)).to eq(true) end end @@ -491,7 +491,14 @@ describe Admin::UsersController do end context ".invite_admin" do + it "doesn't work when not via API" do + controller.stubs(:is_api?).returns(false) + xhr :post, :invite_admin, name: 'Bill', username: 'bill22', email: 'bill@bill.com' + expect(response).not_to be_success + end + it 'should invite admin' do + controller.stubs(:is_api?).returns(true) Jobs.expects(:enqueue).with(:critical_user_email, anything).returns(true) xhr :post, :invite_admin, name: 'Bill', username: 'bill22', email: 'bill@bill.com' expect(response).to be_success @@ -503,6 +510,7 @@ describe Admin::UsersController do end it "doesn't send the email with send_email falsy" do + controller.stubs(:is_api?).returns(true) Jobs.expects(:enqueue).with(:user_email, anything).never xhr :post, :invite_admin, name: 'Bill', username: 'bill22', email: 'bill@bill.com', send_email: '0' expect(response).to be_success diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 119859d3f5e..5d6ed231056 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -1807,4 +1807,69 @@ describe UsersController do end end + + describe ".confirm_admin" do + it "fails without a valid token" do + expect { + get :confirm_admin, token: 'invalid-token' + }.to raise_error(ActionController::UrlGenerationError) + end + + it "fails with a missing token" do + get :confirm_admin, token: 'a0a0a0a0a0' + expect(response).to_not be_success + end + + it "succeeds with a valid code as anonymous" do + user = Fabricate(:user) + ac = AdminConfirmation.new(user, Fabricate(:admin)) + ac.create_confirmation + get :confirm_admin, token: ac.token + expect(response).to be_success + + user.reload + expect(user.admin?).to eq(false) + end + + it "succeeds with a valid code when logged in as that user" do + admin = log_in(:admin) + user = Fabricate(:user) + + ac = AdminConfirmation.new(user, admin) + ac.create_confirmation + get :confirm_admin, token: ac.token + expect(response).to be_success + + user.reload + expect(user.admin?).to eq(false) + end + + it "fails if you're logged in as a different account" do + log_in(:admin) + user = Fabricate(:user) + + ac = AdminConfirmation.new(user, Fabricate(:admin)) + ac.create_confirmation + get :confirm_admin, token: ac.token + expect(response).to_not be_success + + user.reload + expect(user.admin?).to eq(false) + end + + describe "post" do + it "gives the user admin access when POSTed" do + user = Fabricate(:user) + ac = AdminConfirmation.new(user, Fabricate(:admin)) + ac.create_confirmation + post :confirm_admin, token: ac.token + expect(response).to be_success + + user.reload + expect(user.admin?).to eq(true) + end + end + + end + end