Feature: Desktop view - Implement Two-factor Authentication personal setting.

Co-authored-by: Benjamin Scharf <bs@zammad.com>
Co-authored-by: Dominik Klein <dk@zammad.com>
Co-authored-by: Dusan Vuckovic <dv@zammad.com>
Co-authored-by: Mantas Masalskis <mm@zammad.com>
Co-authored-by: Martin Gruner <mg@zammad.com>

app/assets/javascripts/app/controllers/_profile/password.coffee

@@ -1,5 +1,5 @@
 class ProfilePassword extends App.ControllerSubContent
-  @requiredPermission: 'user_preferences.password'
+  @requiredPermission: ['user_preferences.password', 'user_preferences.two_factor_authentication']
   header: __('Password & Authentication')
   events:
     'submit form':                       'update'
@@ -47,9 +47,13 @@ class ProfilePassword extends App.ControllerSubContent
     )
 
   allowsChangePassword: ->
+    return false if !@permissionCheck('user_preferences.password')
+
     App.Config.get('user_show_password_login') || @permissionCheck('admin.*')
 
   allowsTwoFactor: ->
+    return false if !@permissionCheck('user_preferences.two_factor_authentication')
+
     _.some(
       App.Config.all(),
       (state, setting) -> /^two_factor_authentication_method_/.test(setting) and state
@@ -245,9 +249,12 @@ App.Config.set('Password', {
   target: '#profile/password',
   controller: ProfilePassword,
   permission: (controller) ->
-    canChangePassword = App.Config.get('user_show_password_login') || controller.permissionCheck('admin.*')
-    twoFactorEnabled  = App.Config.get('two_factor_authentication_method_authenticator_app')
+    canChangePassword = App.Config.get('user_show_password_login') ||
+      controller.permissionCheck('admin.*')
+
+    twoFactorEnabled  = App.Config.get('two_factor_authentication_method_authenticator_app') &&
+      controller.permissionCheck('user_preferences.two_factor_authentication')
 
     return false if !canChangePassword && !twoFactorEnabled
-    return controller.permissionCheck('user_preferences.password')
+    return controller.permissionCheck('user_preferences.password') || twoFactorEnabled
 }, 'NavBarProfile')

app/assets/javascripts/app/controllers/after_auth/two_factor_configuration.coffee

@@ -31,7 +31,12 @@ class App.AfterAuthTwoFactorConfiguration extends App.ControllerAfterAuthModal
       type:    'GET'
       url:     "#{@apiPath}/users/#{App.User.current().id}/two_factor_enabled_authentication_methods"
       success: @renderAvailableMethods
-    )
+      error: (xhr, status, error) =>
+        return if xhr.status != 403
+
+        @message = __("Two-factor authentication is required, but you don't have sufficient permissions to set it up. Please contact your administrator.")
+        @update()
+      )
 
   renderAvailableMethods: (data, status, xhr) =>
     methodButtons = $(App.view('after_auth/two_factor_configuration/method_buttons')(

app/assets/javascripts/app/controllers/widget/two_factor_configuration/modal/security_keys.coffee

@@ -54,14 +54,14 @@ class App.TwoFactorConfigurationModalSecurityKeys extends App.TwoFactorConfigura
       ]
       objects: _.map(@credentials, (credential) ->
         _.extend(credential,
-          id: credential.external_id
+          id: credential.public_key
         )
       )
       pagerEnabled: false
     )
 
   confirmRemoval: (id) =>
-    credential = @credentials.find((credential) -> credential.external_id is id)
+    credential = @credentials.find((credential) -> credential.public_key is id)
 
     new App.ControllerConfirm(
       head:        __('Are you sure?')
@@ -74,21 +74,12 @@ class App.TwoFactorConfigurationModalSecurityKeys extends App.TwoFactorConfigura
     )
 
   removeSecurityKey: (id) =>
-    newConfiguration             = _.extend({}, @config)
-    newConfiguration.credentials = _.filter(@credentials, (credential) -> credential.external_id isnt id)
-
-    data =
-      configuration: _.extend({}, @config,
-        credentials: _.filter(@credentials, (credential) -> credential.external_id isnt id)
-      )
-
-    # Remove the complete configuration if it's the last key.
-    data.configuration = null if not data.configuration.credentials.length
+    data = { credential_id: id }
 
     @ajax(
       id:          'two_factor_authentication_method_configuration'
-      type:        'PUT'
-      url:         "#{@apiPath}/users/two_factor_authentication_method_configuration/security_keys"
+      type:        'DELETE'
+      url:         "#{@apiPath}/users/two_factor_authentication_remove_credentials/security_keys"
       data:        JSON.stringify(data)
       processData: true
       success: =>

app/controllers/user/two_factors_controller.rb

@@ -4,7 +4,9 @@ class User::TwoFactorsController < ApplicationController
   prepend_before_action :authenticate_and_authorize!
 
   def two_factor_remove_authentication_method
-    params_user.two_factor_destroy_authentication_method(params[:method])
+    Service::User::TwoFactor::RemoveMethod
+      .new(user: params_user, method_name: params[:method])
+      .execute
 
     render json: {}, status: :ok
   end
@@ -29,62 +31,60 @@ class User::TwoFactorsController < ApplicationController
   end
 
   def two_factor_verify_configuration
-    raise Exceptions::UnprocessableEntity, __('The required parameter "method" is missing.')  if !params[:method]
-    raise Exceptions::UnprocessableEntity, __('The required parameter "payload" is missing.') if !params[:payload]
+    raise Exceptions::UnprocessableEntity, __('The required parameter "method" is missing.')  if params[:method].blank?
+    raise Exceptions::UnprocessableEntity, __('The required parameter "payload" is missing.') if params[:payload].blank?
 
-    verified = two_factor_verify_configuration?
+    verify_method_configuration = Service::User::TwoFactor::VerifyMethodConfiguration.new(user: current_user, method_name: params[:method], payload: params[:payload], configuration: params[:configuration].permit!.to_h)
 
-    result = {
-      verified: verified,
-    }
-
-    if verified
-      result[:recovery_codes] = current_user.two_factor_recovery_codes_generate
+    begin
+      render json: verify_method_configuration.execute.merge({ verified: true }), status: :ok
+    rescue Service::User::TwoFactor::VerifyMethodConfiguration::Failed
+      render json: { verified: false }, status: :ok
     end
-
-    render json: result, status: :ok
   end
 
   def two_factor_authentication_method_initiate_configuration
     check_method!
-    check_two_factor_method!
 
-    render json: { configuration: @two_factor_method.initiate_configuration }, status: :ok
+    initiate_authentication_method_configuration = Service::User::TwoFactor::InitiateMethodConfiguration.new(user: current_user, method_name: @method_name)
+
+    render json: { configuration: initiate_authentication_method_configuration.execute }, status: :ok
   end
 
   def two_factor_recovery_codes_generate
-    render json: current_user.two_factor_recovery_codes_generate(force: true), status: :ok
+    codes = Service::User::TwoFactor::GenerateRecoveryCodes
+      .new(user: current_user, force: true)
+      .execute
+
+    render json: codes, status: :ok
   end
 
   def two_factor_default_authentication_method
     check_method!
-    check_two_factor_method!
 
-    current_user.two_factor_update_default_method(@method_name)
+    Service::User::TwoFactor::SetDefaultMethod
+      .new(user: current_user, method_name: @method_name)
+      .execute
 
     render json: {}, status: :ok
   end
 
   def two_factor_authentication_method_configuration
     check_method!
-    check_two_factor_method!
-    fetch_user_two_factor_preference!(raise_exception: false)
 
-    return render json: { configuration: {} }, status: :ok if @user_two_factor_preference.nil?
+    configuration = Service::User::TwoFactor::GetMethodConfiguration
+      .new(user: current_user, method_name: @method_name)
+      .execute
 
-    render json: { configuration: @user_two_factor_preference.configuration }, status: :ok
+    render json: { configuration: configuration || {} }, status: :ok
   end
 
-  def two_factor_authentication_method_configuration_save
+  def two_factor_authentication_remove_credentials
     check_method!
-    check_two_factor_method!
-    fetch_user_two_factor_preference!
 
-    if params[:configuration].nil?
-      current_user.two_factor_destroy_authentication_method(params[:method])
-    else
-      @user_two_factor_preference.update!(configuration: params[:configuration].permit!.to_h)
-    end
+    Service::User::TwoFactor::RemoveMethodCredentials
+      .new(user: current_user, method_name: @method_name, credential_id: params[:credential_id])
+      .execute
 
     render json: {}, status: :ok
   end
@@ -99,34 +99,7 @@ class User::TwoFactorsController < ApplicationController
     true
   end
 
-  def check_two_factor_method!
-    two_factor_method = current_user.auth_two_factor.authentication_method_object(@method_name)
-    raise Exceptions::UnprocessableEntity, __('The two-factor authentication method is not enabled.') if !two_factor_method&.enabled? || !two_factor_method&.available?
-
-    @two_factor_method ||= two_factor_method
-
-    true
-  end
-
-  def fetch_user_two_factor_preference!(raise_exception: true)
-    pref = @two_factor_method.user_two_factor_preference
-
-    if pref.blank? || pref.configuration.blank?
-      raise Exceptions::UnprocessableEntity, __('There is no stored configuration for this two-factor authentication method.') if raise_exception
-
-      return
-    end
-
-    @user_two_factor_preference ||= pref
-
-    true
-  end
-
   def params_user
     User.find(params[:id])
   end
-
-  def two_factor_verify_configuration?
-    current_user.two_factor_verify_configuration?(params[:method], params[:payload], params[:configuration].permit!.to_h)
-  end
 end

app/controllers/users_controller.rb

@@ -609,13 +609,9 @@ curl http://localhost/api/v1/users/password_change -v -u #{login}:#{password} -H
   def password_check
     raise Exceptions::UnprocessableEntity, __("The required parameter 'password' is missing.") if params[:password].blank?
 
-    begin
-      Auth.new(current_user.login, params[:password], only_verify_password: true).valid!
+    password_check = Service::User::PasswordCheck.new(user: current_user, password: params[:password])
 
-      render json: { success: true }, status: :ok
-    rescue Auth::Error::AuthenticationFailed
-      render json: { success: false }, status: :ok
-    end
+    render json: { success: password_check.execute }, status: :ok
   end
 
 =begin

app/frontend/apps/desktop/components/CommonActionMenu/CommonActionMenu.vue

@@ -25,6 +25,7 @@ interface Props {
   placement?: Placement
   orientation?: Orientation
   noSingleActionMode?: boolean
+  customMenuButtonLabel?: string
 }
 
 const props = withDefaults(defineProps<Props>(), {
@@ -44,6 +45,14 @@ const { filteredMenuItems, singleMenuItemPresent, singleMenuItem } =
 const entityId = computed(() => props.entity?.id || getUuid())
 const menuId = computed(() => `popover-${entityId.value}`)
 
+const singleActionAriaLabel = computed(() => {
+  if (typeof singleMenuItem.value?.ariaLabel === 'function') {
+    return singleMenuItem.value.ariaLabel(props.entity)
+  }
+
+  return singleMenuItem.value?.ariaLabel || singleMenuItem.value?.label
+})
+
 const singleActionMode = computed(() => {
   if (props.noSingleActionMode) return false
 
@@ -58,25 +67,22 @@ const buttonVariantClass = computed(() => {
 </script>
 
 <template>
-  <div
-    v-if="filteredMenuItems && filteredMenuItems.length > 0"
-    class="inline-block"
-  >
+  <div v-if="filteredMenuItems" class="inline-block">
     <CommonButton
       v-if="singleActionMode"
       :class="buttonVariantClass"
       :size="buttonSize"
-      :aria-label="$t(singleMenuItem?.label)"
+      :aria-label="$t(singleActionAriaLabel)"
       :icon="singleMenuItem?.icon"
       @click="singleMenuItem?.onClick?.(entity as ObjectLike)"
     />
     <CommonButton
       v-else
-      :id="entity?.id || entityId"
+      :id="`action-menu-${entityId}`"
       ref="popoverTarget"
-      :aria-label="$t('Action menu button')"
+      :aria-label="$t(customMenuButtonLabel || 'Action menu button')"
       aria-haspopup="true"
-      :aria-controls="menuId"
+      :aria-controls="popoverIsOpen ? menuId : undefined"
       class="text-stone-200 dark:text-neutral-500"
       :class="{
         'outline outline-1 outline-offset-1 outline-blue-800': popoverIsOpen,

app/frontend/apps/desktop/components/CommonActionMenu/__tests__/CommonActionMenu.spec.ts

@@ -2,6 +2,7 @@
 
 import CommonActionMenu from '#desktop/components/CommonActionMenu/CommonActionMenu.vue'
 import renderComponent from '#tests/support/components/renderComponent.ts'
+import type { ObjectLike } from '#shared/types/utils.ts'
 
 const fn = vi.fn()
 describe('CommonActionMenu', () => {
@@ -53,17 +54,24 @@ describe('CommonActionMenu', () => {
   })
 
   it('finds corresponding a11y controls', async () => {
+    await view.events.click(view.getByIconName('three-dots-vertical'))
     const id = view
       .getByLabelText('Action menu button')
       .getAttribute('aria-controls')
 
-    await view.events.click(view.getByIconName('three-dots-vertical'))
-
     const popover = document.getElementById(id as string)
 
     expect(popover?.getAttribute('id')).toEqual(id)
   })
 
+  it('sets a custom aria label on single action button', async () => {
+    await view.rerender({
+      customMenuButtonLabel: 'Custom Action Menu Label',
+    })
+
+    expect(view.getByLabelText('Custom Action Menu Label')).toBeInTheDocument()
+  })
+
   describe('single action mode', () => {
     beforeEach(async () => {
       await view.rerender({
@@ -100,5 +108,38 @@ describe('CommonActionMenu', () => {
 
       expect(view.getByIconName('trash3')).toBeInTheDocument()
     })
+
+    it('sets a custom aria label on single action', async () => {
+      await view.rerender({
+        actions: [
+          {
+            key: 'delete-foo',
+            label: 'Delete Foo',
+            ariaLabel: 'Custom Delete Foo',
+            icon: 'trash3',
+            onClick: ({ id }: { id: string }) => {
+              fn(id)
+            },
+          },
+        ],
+      })
+
+      expect(view.getByLabelText('Custom Delete Foo')).toBeInTheDocument()
+
+      await view.rerender({
+        actions: [
+          {
+            key: 'delete-foo',
+            label: 'Delete Foo',
+            ariaLabel: (entity: ObjectLike) => `label ${entity.id}`,
+            icon: 'trash3',
+            onClick: ({ id }: { id: string }) => {
+              fn(id)
+            },
+          },
+        ],
+      })
+      expect(view.getByLabelText('label foo-test-action')).toBeInTheDocument()
+    })
   })
 })

app/frontend/apps/desktop/components/CommonFlyout/CommonFlyout.vue

@@ -52,10 +52,11 @@ export interface Props {
   headerIcon?: string
   resizable?: boolean
   showBackdrop?: boolean
-  closeOnBackdropClick?: boolean
-  closeOnEscape?: boolean
+  noCloseOnBackdropClick?: boolean
+  noCloseOnEscape?: boolean
   hideFooter?: boolean
   footerActionOptions?: ActionFooterProps
+  noCloseOnAction?: boolean
   /**
    * @property noAutofocus
    * Don't focus the first element inside a Flyout after being mounted
@@ -67,8 +68,8 @@ export interface Props {
 const props = withDefaults(defineProps<Props>(), {
   resizable: true,
   showBackdrop: true,
-  closeOnBackdropClick: true,
-  closeOnEscape: true,
+  noCloseOnBackdropClick: true,
+  noCloseOnEscape: true,
 })
 
 defineOptions({
@@ -87,6 +88,9 @@ const close = async () => {
 
 const action = async () => {
   emit('action')
+
+  if (props.noCloseOnAction) return
+
   await closeFlyout(props.name)
 }
 
@@ -181,7 +185,7 @@ onMounted(async () => {
 
 // Keyboard
 onKeyUp('Escape', (e) => {
-  if (!props.closeOnEscape) return
+  if (props.noCloseOnEscape) return
   stopEvent(e)
   close()
 })
@@ -235,7 +239,7 @@ onMounted(() => {
     tag="aside"
     tabindex="-1"
     class="overflow-clip-x fixed bottom-0 top-0 z-40 flex max-h-dvh min-w-min flex-col border-y border-neutral-100 bg-white ltr:right-0 ltr:rounded-l-xl ltr:border-l rtl:left-0 rtl:rounded-r-xl rtl:border-r dark:border-gray-900 dark:bg-gray-500"
-    :close-on-backdrop-click="closeOnBackdropClick"
+    :no-close-on-backdrop-click="noCloseOnBackdropClick"
     :show-backdrop="showBackdrop"
     :style="{ width: `${flyoutContainerWidth}px` }"
     :class="{ 'transition-all': !isResizingHorizontal }"
@@ -261,9 +265,9 @@ onMounted(() => {
             size="small"
             :name="headerIcon"
           />
-          <h3 v-if="headerTitle" :id="`${flyoutId}-title`">
+          <h2 v-if="headerTitle" :id="`${flyoutId}-title`">
             {{ headerTitle }}
-          </h3>
+          </h2>
         </div>
       </slot>
       <CommonButton

app/frontend/apps/desktop/components/CommonFlyout/CommonFlyoutActionFooter.vue

@@ -15,6 +15,7 @@ export interface Props {
     ButtonProps,
     'prefixIcon' | 'variant' | 'type' | 'disabled'
   >
+  hideCancelButton?: boolean
   cancelLabel?: string
   cancelButton?: Pick<ButtonProps, 'prefixIcon' | 'variant' | 'disabled'>
   form?: FormRef
@@ -45,6 +46,7 @@ const execute = () => {
 <template>
   <div class="flex items-center justify-end gap-2">
     <CommonButton
+      v-if="!hideCancelButton"
       size="large"
       :disabled="isDisabled || cancelButton?.disabled"
       :prefix-icon="cancelButton?.prefixIcon"

app/frontend/apps/desktop/components/CommonFlyout/__tests__/CommonFlyout.spec.ts

@@ -134,7 +134,7 @@ describe('CommonFlyout', () => {
       })
 
       it('emits close event when escape key is pressed', async () => {
-        await flyout.rerender({ closeOnEscape: true })
+        await flyout.rerender({ noCloseOnEscape: false })
 
         await flyout.events.keyboard('{Escape}')