Skip to content

LG-11454: Restore successful WebAuthn delete handling user events#9780

Merged
aduth merged 1 commit intomainfrom
aduth-lg-11454-webauthn-delete-events
Dec 18, 2023
Merged

LG-11454: Restore successful WebAuthn delete handling user events#9780
aduth merged 1 commit intomainfrom
aduth-lg-11454-webauthn-delete-events

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Dec 18, 2023

🎫 Ticket

Relates to LG-11454 (originally #9674)

🛠 Summary of changes

Restores additional behaviors expected to occur after the successful deletion of a WebAuthn credential.

I noticed these after starting to put together the clean-up pull request to remove the old deletion routes, which implement these behaviors:

create_user_event(:webauthn_key_removed)
webauthn.destroy
revoke_remember_device(current_user)
event = PushNotification::RecoveryInformationChangedEvent.new(user: current_user)
PushNotification::HttpPush.deliver(event)

📜 Testing Plan

Verify specs pass:

  1. rspec spec/controllers/api/internal/two_factor_authentication/webauthn_controller_spec.rb spec/controllers/users/webauthn_controller_spec.rb

You can also verify that behaviors actually happen when deleting Face or Touch Unlock:

  1. (Prerequisite) Have an account with Face or Touch Unlock configured
  2. Go to http://localhost:3000
  3. Sign in
  4. From account dashboard, click "Manage" for your Face or Touch Unlock
  5. Click "Delete"
  6. Confirm prompt
  7. In Rails console, verify user event: User.find_with_email('email@example.com').events.webauthn_key_removed
  8. TBD how to verify RISC event (see Slack thread)
  9. Sign out
  10. Sign in
  11. Observe that you're prompted to MFA

changelog: User-Facing Improvements, Face or Touch Unlock, Add option to rename face or touch unlock in account dashboard
Comment on lines +38 to +41
create_user_event(:webauthn_key_removed)
revoke_remember_device(current_user)
event = PushNotification::RecoveryInformationChangedEvent.new(user: current_user)
PushNotification::HttpPush.deliver(event)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately, it might be worth thinking about extracting this behavior, since we're doing several things here, and we have the same code across basically all of MFAs... or at least I expect we intend to, but the RISC events aren't consistently implemented (even more justification to extract a common helper).

e.g.

module TwoFactorAuthenticatableMethods
  # ...
  def handle_valid_auth_method_removed(user_event:)
    create_user_event(user_event)
    revoke_remember_device(current_user)
    event = PushNotification::RecoveryInformationChangedEvent.new(user: current_user)
    PushNotification::HttpPush.deliver(event)
  end
end

@aduth aduth merged commit bc9da12 into main Dec 18, 2023
@aduth aduth deleted the aduth-lg-11454-webauthn-delete-events branch December 18, 2023 17:29
@jmdembe jmdembe mentioned this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants