From c86b1ee9d11ccd1efb76a0ef33589c69dd88cbc0 Mon Sep 17 00:00:00 2001
From: Kane York <kanepyork@gmail.com>
Date: Tue, 7 Jul 2020 12:19:30 -0700
Subject: [PATCH] FIX: Disable security keys at same time as TOTP 2FA (#10144)

Previously, the "Remove 2FA" button could result in an error. This syncs button visibility with behavior.

* FIX: Only offer disabling 2FA to admins
---
 app/controllers/admin/users_controller.rb     |  4 +++-
 .../admin_detailed_user_serializer.rb         |  2 +-
 spec/requests/admin/users_controller_spec.rb  | 21 +++++++++++++++++++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index c4923ec2c6a..921c589865d 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -361,9 +361,11 @@ class Admin::UsersController < Admin::AdminController
   def disable_second_factor
     guardian.ensure_can_disable_second_factor!(@user)
     user_second_factor = @user.user_second_factors
-    raise Discourse::InvalidParameters unless !user_second_factor.empty?
+    user_security_key = @user.security_keys
+    raise Discourse::InvalidParameters if user_second_factor.empty? && user_security_key.empty?
 
     user_second_factor.destroy_all
+    user_security_key.destroy_all
     StaffActionLogger.new(current_user).log_disable_second_factor_auth(@user)
 
     Jobs.enqueue(
diff --git a/app/serializers/admin_detailed_user_serializer.rb b/app/serializers/admin_detailed_user_serializer.rb
index c0b18e3df57..0f3840ed8ee 100644
--- a/app/serializers/admin_detailed_user_serializer.rb
+++ b/app/serializers/admin_detailed_user_serializer.rb
@@ -44,7 +44,7 @@ class AdminDetailedUserSerializer < AdminUserSerializer
   end
 
   def can_disable_second_factor
-    object&.id != scope.user.id
+    scope.is_admin? && (object&.id != scope.user.id)
   end
 
   def can_revoke_admin
diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb
index 099372cad00..3cbdf5a7d85 100644
--- a/spec/requests/admin/users_controller_spec.rb
+++ b/spec/requests/admin/users_controller_spec.rb
@@ -878,12 +878,14 @@ RSpec.describe Admin::UsersController do
   describe '#disable_second_factor' do
     let(:second_factor) { user.create_totp(enabled: true) }
     let(:second_factor_backup) { user.generate_backup_codes }
+    let(:security_key) { Fabricate(:user_security_key, user: user) }
 
     describe 'as an admin' do
       before do
         sign_in(admin)
         second_factor
         second_factor_backup
+        security_key
         expect(user.reload.user_second_factors.totps.first).to eq(second_factor)
       end
 
@@ -894,6 +896,7 @@ RSpec.describe Admin::UsersController do
 
         expect(response.status).to eq(200)
         expect(user.reload.user_second_factors).to be_empty
+        expect(user.reload.security_keys).to be_empty
 
         job_args = Jobs::CriticalUserEmail.jobs.first["args"].first
 
@@ -907,9 +910,27 @@ RSpec.describe Admin::UsersController do
         expect(response.status).to eq(403)
       end
 
+      describe 'when user has only one second factor type enabled' do
+        it 'should succeed with security keys' do
+          user.user_second_factors.destroy_all
+
+          put "/admin/users/#{user.id}/disable_second_factor.json"
+
+          expect(response.status).to eq(200)
+        end
+        it 'should succeed with totp' do
+          user.security_keys.destroy_all
+
+          put "/admin/users/#{user.id}/disable_second_factor.json"
+
+          expect(response.status).to eq(200)
+        end
+      end
+
       describe 'when user does not have second factor enabled' do
         it 'should raise the right error' do
           user.user_second_factors.destroy_all
+          user.security_keys.destroy_all
 
           put "/admin/users/#{user.id}/disable_second_factor.json"