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
This commit is contained in:
Kane York
2020-07-07 12:19:30 -07:00
committed by GitHub
parent 81fe8a50d4
commit c86b1ee9d1
3 changed files with 25 additions and 2 deletions

View File

@ -361,9 +361,11 @@ class Admin::UsersController < Admin::AdminController
def disable_second_factor def disable_second_factor
guardian.ensure_can_disable_second_factor!(@user) guardian.ensure_can_disable_second_factor!(@user)
user_second_factor = @user.user_second_factors 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_second_factor.destroy_all
user_security_key.destroy_all
StaffActionLogger.new(current_user).log_disable_second_factor_auth(@user) StaffActionLogger.new(current_user).log_disable_second_factor_auth(@user)
Jobs.enqueue( Jobs.enqueue(

View File

@ -44,7 +44,7 @@ class AdminDetailedUserSerializer < AdminUserSerializer
end end
def can_disable_second_factor def can_disable_second_factor
object&.id != scope.user.id scope.is_admin? && (object&.id != scope.user.id)
end end
def can_revoke_admin def can_revoke_admin

View File

@ -878,12 +878,14 @@ RSpec.describe Admin::UsersController do
describe '#disable_second_factor' do describe '#disable_second_factor' do
let(:second_factor) { user.create_totp(enabled: true) } let(:second_factor) { user.create_totp(enabled: true) }
let(:second_factor_backup) { user.generate_backup_codes } let(:second_factor_backup) { user.generate_backup_codes }
let(:security_key) { Fabricate(:user_security_key, user: user) }
describe 'as an admin' do describe 'as an admin' do
before do before do
sign_in(admin) sign_in(admin)
second_factor second_factor
second_factor_backup second_factor_backup
security_key
expect(user.reload.user_second_factors.totps.first).to eq(second_factor) expect(user.reload.user_second_factors.totps.first).to eq(second_factor)
end end
@ -894,6 +896,7 @@ RSpec.describe Admin::UsersController do
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(user.reload.user_second_factors).to be_empty 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 job_args = Jobs::CriticalUserEmail.jobs.first["args"].first
@ -907,9 +910,27 @@ RSpec.describe Admin::UsersController do
expect(response.status).to eq(403) expect(response.status).to eq(403)
end 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 describe 'when user does not have second factor enabled' do
it 'should raise the right error' do it 'should raise the right error' do
user.user_second_factors.destroy_all user.user_second_factors.destroy_all
user.security_keys.destroy_all
put "/admin/users/#{user.id}/disable_second_factor.json" put "/admin/users/#{user.id}/disable_second_factor.json"