-
Notifications
You must be signed in to change notification settings - Fork 166
LG-11758: Add inline device nickname editing for authenticator apps #9845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 15 commits
86ee69d
dfc39aa
73b4bdc
87c2808
158b63e
b41928a
d27848c
2a58a90
338ae43
2fcd33a
93ac353
b2bc727
31ab114
19056aa
d564848
b6f8c55
0e96216
dc3f78f
34d3d17
a6340f2
8d12282
f8818df
374103c
c12e6cf
a5ed70b
775966a
187e910
8cf5e54
8ce9a81
1b78914
a51b436
bd946a0
4ffb39f
41fdd48
5f8eb06
f1684ac
edd3a10
094177d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| module Api | ||
| module Internal | ||
| module TwoFactorAuthentication | ||
| class AuthAppController < ApplicationController | ||
| include CsrfTokenConcern | ||
| include ReauthenticationRequiredConcern | ||
|
|
||
| before_action :render_unauthorized, unless: :recently_authenticated_2fa? | ||
|
|
||
| after_action :add_csrf_token_header_to_response | ||
|
|
||
| respond_to :json | ||
|
|
||
| def update | ||
| result = ::TwoFactorAuthentication::AuthAppUpdateForm.new( | ||
| user: current_user, | ||
| configuration_id: params[:id], | ||
| ).submit(name: params[:name]) | ||
|
|
||
| analytics.auth_app_update_name_submitted(**result.to_h) | ||
|
|
||
| if result.success? | ||
| render json: { success: true } | ||
| else | ||
| render json: { success: false, error: result.first_error_message }, status: :bad_request | ||
| end | ||
| end | ||
|
|
||
| def destroy | ||
| result = ::TwoFactorAuthentication::AuthAppDeleteForm.new( | ||
| user: current_user, | ||
| configuration_id: params[:id], | ||
| ).submit | ||
|
|
||
| analytics.auth_app_delete_submitted(**result.to_h) | ||
|
|
||
| if result.success? | ||
| create_user_event(:webauthn_key_removed) | ||
| revoke_remember_device(current_user) | ||
| event = PushNotification::RecoveryInformationChangedEvent.new(user: current_user) | ||
| PushNotification::HttpPush.deliver(event) | ||
| render json: { success: true } | ||
| else | ||
| render json: { success: false, error: result.first_error_message }, status: :bad_request | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def render_unauthorized | ||
| render json: { error: 'Unauthorized' }, status: :unauthorized | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| module Users | ||
| class AuthAppController < ApplicationController | ||
| include ReauthenticationRequiredConcern | ||
|
|
||
| before_action :confirm_two_factor_authenticated | ||
| before_action :confirm_recently_authenticated_2fa | ||
| before_action :set_form | ||
| before_action :validate_configuration_exists | ||
|
|
||
| def edit; end | ||
|
|
||
| def update | ||
| binding.pry | ||
| result = form.submit(name: params.dig(:form, :name)) | ||
|
|
||
| analytics.auth_app_update_name_submitted(**result.to_h) | ||
|
|
||
| if result.success? | ||
| flash[:success] = t('two_factor_authentication.auth_app.renamed') | ||
| redirect_to account_path | ||
| else | ||
| flash.now[:error] = result.first_error_message | ||
| render :edit | ||
| end | ||
| end | ||
|
|
||
| def destroy | ||
| result = form.submit | ||
|
|
||
| analytics.auth_app_delete_submitted(**result.to_h) | ||
|
|
||
| if result.success? | ||
| flash[:success] = t('two_factor_authentication.webauthn_platform.deleted') | ||
| create_user_event(:webauthn_key_removed) | ||
| revoke_remember_device(current_user) | ||
| event = PushNotification::RecoveryInformationChangedEvent.new(user: current_user) | ||
| PushNotification::HttpPush.deliver(event) | ||
| redirect_to account_path | ||
| else | ||
| flash[:error] = result.first_error_message | ||
| redirect_to edit_auth_app_path(id: params[:id]) | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def form | ||
| @form ||= form_class.new(user: current_user, configuration_id: params[:id]) | ||
| end | ||
|
|
||
| alias_method :set_form, :form | ||
|
|
||
| def form_class | ||
| case action_name | ||
| when 'edit', 'update' | ||
| TwoFactorAuthentication::AuthAppUpdateForm | ||
| when 'destroy' | ||
| TwoFactorAuthentication::AuthAppDeleteForm | ||
| end | ||
| end | ||
|
Comment on lines
+52
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this abstraction is more difficult to follow and it would be preferable to explicitly instantiate the form in each of the controllers. It would also allow dropping the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see what you mean. It would break that, but I think it may be worth splitting it up still and being explicit about that too since this makes it difficult to respond with something other than
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After some thought, I will stick to what I am proposing--we are using one view to rename and delete an app method and the form that we need in the view is toggled based on the desired action. |
||
|
|
||
| def validate_configuration_exists | ||
| render_not_found if form.configuration.blank? | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| module TwoFactorAuthentication | ||
| class AuthAppDeleteForm | ||
| include ActiveModel::Model | ||
| include ActionView::Helpers::TranslationHelper | ||
|
|
||
| attr_reader :user, :configuration_id | ||
|
|
||
| validate :validate_configuration_exists | ||
| validate :validate_has_multiple_mfa | ||
|
|
||
| def initialize(user:, configuration_id:) | ||
| @user = user | ||
| @configuration_id = configuration_id | ||
| end | ||
|
|
||
| def submit | ||
| success = valid? | ||
|
|
||
| configuration.destroy if success | ||
|
|
||
| FormResponse.new( | ||
| success:, | ||
| errors:, | ||
| extra: extra_analytics_attributes, | ||
| serialize_error_details_only: true, | ||
| ) | ||
| end | ||
|
|
||
| def configuration | ||
| @configuration ||= user.auth_app_configurations.find_by(id: configuration_id) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def validate_configuration_exists | ||
| return if configuration.present? | ||
| errors.add( | ||
| :configuration_id, | ||
| :configuration_not_found, | ||
| message: t('errors.manage_authenticator.internal_error'), | ||
| ) | ||
| end | ||
|
|
||
| def validate_has_multiple_mfa | ||
| return if !configuration || MfaPolicy.new(user).multiple_factors_enabled? | ||
| errors.add( | ||
| :configuration_id, | ||
| :only_method, | ||
| message: t('errors.manage_authenticator.remove_only_method_error'), | ||
| ) | ||
| end | ||
|
|
||
| def extra_analytics_attributes | ||
| { configuration_id: } | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| module TwoFactorAuthentication | ||
| class AuthAppUpdateForm | ||
| include ActiveModel::Model | ||
| include ActionView::Helpers::TranslationHelper | ||
|
|
||
| attr_reader :user, :configuration_id | ||
|
|
||
| validate :validate_configuration_exists | ||
| validate :validate_unique_name | ||
|
|
||
| def initialize(user:, configuration_id:) | ||
| @user = user | ||
| @configuration_id = configuration_id | ||
| end | ||
|
|
||
| def submit(name:) | ||
| @name = name | ||
|
|
||
| success = valid? | ||
| if valid? | ||
| configuration.name = name | ||
| success = configuration.valid? | ||
| errors.merge!(configuration.errors) | ||
| configuration.save if success | ||
| end | ||
|
|
||
| FormResponse.new( | ||
| success:, | ||
| errors:, | ||
| extra: extra_analytics_attributes, | ||
| serialize_error_details_only: true, | ||
| ) | ||
| end | ||
|
|
||
| def name | ||
| return @name if defined?(@name) | ||
| @name = configuration&.name | ||
| end | ||
|
|
||
| def configuration | ||
| @configuration ||= user.auth_app_configurations.find_by(id: configuration_id) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def validate_configuration_exists | ||
| return if configuration.present? | ||
| errors.add( | ||
| :configuration_id, | ||
| :configuration_not_found, | ||
| message: t('errors.manage_authenticator.internal_error'), | ||
| ) | ||
| end | ||
|
|
||
| def validate_unique_name | ||
| return unless user.webauthn_configurations.where.not(id: configuration_id).find_by(name:) | ||
| errors.add( | ||
| :name, | ||
| :duplicate, | ||
| message: t('errors.manage_authenticator.unique_name_error'), | ||
| ) | ||
| end | ||
|
|
||
| def extra_analytics_attributes | ||
| { configuration_id: } | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| <% self.title = t('two_factor_authentication.auth_app.edit_heading') %> | ||
|
|
||
| <%= render PageHeadingComponent.new.with_content(t('two_factor_authentication.auth_app.edit_heading')) %> | ||
|
|
||
| <%= simple_form_for( | ||
| @form, | ||
| as: :form, | ||
| method: :put, | ||
| html: { autocomplete: 'off' }, | ||
| url: auth_app_path(id: @form.configuration.id), | ||
| ) do |f| %> | ||
| <%= render ValidatedFieldComponent.new( | ||
| form: f, | ||
| name: :name, | ||
| label: t('two_factor_authentication.auth_app.nickname'), | ||
| ) %> | ||
|
|
||
| <%= f.submit( | ||
| t('two_factor_authentication.webauthn_platform.change_nickname'), | ||
| class: 'display-block margin-top-5', | ||
| ) %> | ||
| <% end %> | ||
|
|
||
| <%= render ButtonComponent.new( | ||
| action: ->(**tag_options, &block) do | ||
| button_to( | ||
| auth_app_path(id: @form.configuration.id), | ||
| form: { aria: { label: t('two_factor_authentication.auth_app.delete') } }, | ||
| **tag_options, | ||
| &block | ||
| ) | ||
| end, | ||
| method: :delete, | ||
| big: true, | ||
| wide: true, | ||
| danger: true, | ||
| class: 'display-block margin-top-2', | ||
| ).with_content(t('two_factor_authentication.auth_app.delete')) %> | ||
|
|
||
| <%= render 'shared/cancel', link: account_path %> |
Uh oh!
There was an error while loading. Please reload this page.