SECURITY: Confirm new administrator accounts via email

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);
   },
 

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)

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

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

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
+

app/views/users/confirm_admin.html.erb

@@ -0,0 +1,20 @@
+<div id='simple-container'>
+  <h2><%= t 'activation.admin_confirm.title' %></h2>
+  <br/>
+
+  <%- unless @confirmed %>
+    <%=raw (t('activation.admin_confirm.description', target_username: @confirmation.target_user.username)) %>
+    <br/>
+    <br/>
+
+    <%= 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)) %>
+    <br/>
+    <br/>
+    <%= link_to t("activation.admin_confirm.back_to", title: SiteSetting.title), "/", class: 'btn btn-primary' %>
+  <% end %>
+
+</div>

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'

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 <b>%{target_username}</b> to be an administrator?"
+      grant: "Grant Admin Access"
+      complete: "<b>%{target_username}</b> 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"

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}

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

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
+

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

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