-
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
86ee69d
add copy for feature
jmdembe dfc39aa
modify list element
jmdembe 73b4bdc
update strings and yml keys
jmdembe 87c2808
Merge branch 'main' into jd-LG-11758-edit-auth-apps
jmdembe 158b63e
create controller and form
jmdembe b41928a
create controller
jmdembe d27848c
add destroy for app auth deletion
jmdembe 2a58a90
create page to edit auth app nickname
jmdembe 338ae43
create routes
jmdembe 2fcd33a
Merge branch 'main' into jd-LG-11758-edit-auth-apps
jmdembe 93ac353
enhance edit functionality
jmdembe b2bc727
add tests for update nickname
jmdembe 31ab114
create test for app auth update form
jmdembe 19056aa
add test for app update form
jmdembe d564848
fix functionality for deleting auth app
jmdembe b6f8c55
remove binding.pry, add test for renaming auth app
jmdembe 0e96216
remove binding.pry, update validate_unique_name validator
jmdembe dc3f78f
create test for app auth deletion
jmdembe 34d3d17
create test for auth app controller api
jmdembe a6340f2
fix test for unique name error
jmdembe 8d12282
lintfix
jmdembe f8818df
update deletion test
jmdembe 374103c
fix test for update totp
jmdembe c12e6cf
Merge branch 'main' into jd-LG-11758-edit-auth-apps
jmdembe a5ed70b
removed unused keys
jmdembe 775966a
add test for auth apps show
jmdembe 187e910
add missing keys
jmdembe 8cf5e54
Something is not right
jmdembe 8ce9a81
Merge branch 'main' into jd-LG-11758-edit-auth-apps
jmdembe 1b78914
update text for change nickname
jmdembe a51b436
fix lint
jmdembe bd946a0
WIP: delete spec from account shows
jmdembe 4ffb39f
remove test since show/hide feature is being done elsewhere
jmdembe 41fdd48
Merge branch 'main' into jd-LG-11758-edit-auth-apps
jmdembe 5f8eb06
Merge branch 'main' into jd-LG-11758-edit-auth-apps
jmdembe f1684ac
Merge branch 'main' into jd-LG-11758-edit-auth-apps
jmdembe edd3a10
Merge branch 'main' into jd-LG-11758-edit-auth-apps
jmdembe 094177d
Merge branch 'main' into jd-LG-11758-edit-auth-apps
jmdembe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
56 changes: 56 additions & 0 deletions
56
app/controllers/api/internal/two_factor_authentication/auth_app_controller.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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(:authenticator_disabled) | ||
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| 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 | ||
| 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.auth_app.deleted') | ||
| create_user_event(:authenticator_disabled) | ||
| 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 | ||
|
|
||
| def validate_configuration_exists | ||
| render_not_found if form.configuration.blank? | ||
| end | ||
| end | ||
| end | ||
57 changes: 57 additions & 0 deletions
57
app/forms/two_factor_authentication/auth_app_delete_form.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
68 changes: 68 additions & 0 deletions
68
app/forms/two_factor_authentication/auth_app_update_form.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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.auth_app_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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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.auth_app.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 %> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
set_formalias and before_action.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
form_class, we use that method to define the form in each controller and thevalidate_configuration_existsbefore_action. Wouldn't droppingset_formbreak the validation before_action?There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
render_not_found. The existing controller redirects, and I think that might be preferable (perhaps with an error message?).There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.