Fix webauth device removal

- Remove backup codes when last two-factor auth method removed

 Changelog: fixed

app/controllers/profiles/webauthn_registrations_controller.rb

@@ -4,8 +4,7 @@ class Profiles::WebauthnRegistrationsController < Profiles::ApplicationControlle
   feature_category :system_access
 
   def destroy
-    webauthn_registration = current_user.webauthn_registrations.find(params[:id])
-    webauthn_registration.destroy
+    Webauthn::DestroyService.new(current_user, current_user, params[:id]).execute
 
     redirect_to profile_two_factor_auth_path, status: :found, notice: _("Successfully deleted WebAuthn device.")
   end

app/services/webauthn/destroy_service.rb

+# frozen_string_literal: true
+
+module Webauthn
+  class DestroyService < BaseService
+    attr_reader :webauthn_registration, :user, :current_user
+
+    def initialize(current_user, user, webauthn_registrations_id)
+      @current_user = current_user
+      @user = user
+      @webauthn_registration = user.webauthn_registrations.find(webauthn_registrations_id)
+    end
+
+    def execute
+      return error(_('You are not authorized to perform this action')) unless authorized?
+
+      webauthn_registration.destroy
+      user.reset_backup_codes! if last_two_factor_registration?
+    end
+
+    private
+
+    def last_two_factor_registration?
+      user.webauthn_registrations.empty? && !user.otp_required_for_login?
+    end
+
+    def authorized?
+      current_user.can?(:disable_two_factor, user)
+    end
+  end
+end

spec/controllers/profiles/webauthn_registrations_controller_spec.rb

@@ -10,11 +10,27 @@
   end
 
   describe '#destroy' do
-    it 'deletes the given webauthn registration' do
-      registration_to_delete = user.webauthn_registrations.first
+    let(:webauthn_id) { user.webauthn_registrations.first.id }
 
-      expect { delete :destroy, params: { id: registration_to_delete.id } }.to change { user.webauthn_registrations.count }.by(-1)
-      expect(response).to be_redirect
+    subject { delete :destroy, params: { id: webauthn_id } }
+
+    it 'redirects to the profile two factor authentication page' do
+      subject
+
+      expect(response).to redirect_to profile_two_factor_auth_path
+    end
+
+    it 'destroys the webauthn registration' do
+      expect { subject }.to change { user.webauthn_registrations.count }.by(-1)
+    end
+
+    it 'calls the Webauthn::DestroyService' do
+      service = double
+
+      expect(Webauthn::DestroyService).to receive(:new).with(user, user, webauthn_id.to_s).and_return(service)
+      expect(service).to receive(:execute)
+
+      subject
     end
   end
 end

spec/factories/users.rb

@@ -124,6 +124,8 @@
       transient { registrations_count { 5 } }
 
       after(:create) do |user, evaluator|
+        user.generate_otp_backup_codes!
+
         create_list(:webauthn_registration, evaluator.registrations_count, user: user)
       end
     end

spec/services/webauthn/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Webauthn::DestroyService, feature_category: :system_access do
+  let(:user) { create(:user, :two_factor_via_webauthn, registrations_count: 1) }
+  let(:current_user) { user }
+
+  describe '#execute' do
+    let(:webauthn_id) { user.webauthn_registrations.first.id }
+
+    subject { described_class.new(current_user, user, webauthn_id).execute }
+
+    context 'with only one webauthn method enabled' do
+      context 'when another user is calling the service' do
+        context 'for a user without permissions' do
+          let(:current_user) { create(:user) }
+
+          it 'does not destry the webauthn registration' do
+            expect { subject }.not_to change { user.webauthn_registrations.count }
+          end
+
+          it 'does not remove the user backup codes' do
+            expect { subject }.not_to change { user.otp_backup_codes }
+          end
+
+          it 'returns error' do
+            expect(subject[:status]).to eq(:error)
+          end
+        end
+
+        context 'for an admin' do
+          it 'destroys the webauthn registration' do
+            expect { subject }.to change { user.webauthn_registrations.count }.by(-1)
+          end
+
+          it 'removes the user backup codes' do
+            subject
+
+            expect(user.otp_backup_codes).to be_nil
+          end
+        end
+      end
+
+      context 'when current user is calling the service' do
+        context 'when there is also OTP enabled' do
+          before do
+            user.otp_required_for_login = true
+            user.otp_secret = User.generate_otp_secret(32)
+            user.otp_grace_period_started_at = Time.current
+            user.generate_otp_backup_codes!
+            user.save!
+          end
+
+          it 'removes the webauth registrations' do
+            expect { subject }.to change { user.webauthn_registrations.count }.by(-1)
+          end
+
+          it 'does not remove the user backup codes' do
+            expect { subject }.not_to change { user.otp_backup_codes }
+          end
+        end
+      end
+    end
+
+    context 'with multiple webauthn methods enabled' do
+      before do
+        create(:webauthn_registration, user: user)
+      end
+
+      it 'destroys the webauthn registration' do
+        expect { subject }.to change { user.webauthn_registrations.count }.by(-1)
+      end
+
+      it 'does not remove the user backup codes' do
+        expect { subject }.not_to change { user.otp_backup_codes }
+      end
+    end
+  end
+end