Skip to content

Form helpers should not be the ones storing the challenge in the session#73

Merged
nicolastemciuc merged 22 commits intomasterfrom
temciuc--not-store-the-challenge-in-helper
Jan 22, 2026
Merged

Form helpers should not be the ones storing the challenge in the session#73
nicolastemciuc merged 22 commits intomasterfrom
temciuc--not-store-the-challenge-in-helper

Conversation

@nicolastemciuc
Copy link
Copy Markdown
Member

Closes #53

@nicolastemciuc nicolastemciuc added the enhancement New feature or request label Dec 10, 2025
@nicolastemciuc nicolastemciuc force-pushed the temciuc--not-store-the-challenge-in-helper branch from 2b0ce3c to 686ebc9 Compare December 10, 2025 17:58
@nicolastemciuc nicolastemciuc force-pushed the temciuc--not-store-the-challenge-in-helper branch 2 times, most recently from 0350783 to da88872 Compare January 15, 2026 18:17
Comment thread app/controllers/devise/passkeys_controller.rb Outdated
@nicolastemciuc nicolastemciuc force-pushed the temciuc--not-store-the-challenge-in-helper branch from da88872 to bec8cbd Compare January 15, 2026 18:43
@nicolastemciuc nicolastemciuc force-pushed the temciuc--not-store-the-challenge-in-helper branch from bec8cbd to 07b1d33 Compare January 15, 2026 18:47
@nicolastemciuc nicolastemciuc force-pushed the temciuc--not-store-the-challenge-in-helper branch from 07b1d33 to d5dd6a6 Compare January 15, 2026 18:49
@nicolastemciuc nicolastemciuc force-pushed the temciuc--not-store-the-challenge-in-helper branch 2 times, most recently from 4445c4d to 13cf467 Compare January 19, 2026 12:55
@nicolastemciuc nicolastemciuc force-pushed the temciuc--not-store-the-challenge-in-helper branch 3 times, most recently from e8e2643 to ec11bd4 Compare January 19, 2026 13:48
@nicolastemciuc nicolastemciuc force-pushed the temciuc--not-store-the-challenge-in-helper branch from ec11bd4 to 5523100 Compare January 19, 2026 14:04
Comment thread app/controllers/devise/passkey_registration_options_controller.rb
Copy link
Copy Markdown
Contributor

@RenzoMinelli RenzoMinelli left a comment

Choose a reason for hiding this comment

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

Have you considered grouping these controllers under different modules/folders, something like this:

passkey 
- registration_options_controller
- authentication_options_controller

security_key
- registration_options_controller
- authentication_options_controller

@nicolastemciuc
Copy link
Copy Markdown
Member Author

Have you considered grouping these controllers under different modules/folders, something like this:

@RenzoMinelli Done in chore: scope options controllers under passkey and security_key

Comment thread spec/system/sign_in_with_webauthn_spec.rb
Comment thread spec/requests/devise/two_factor_authentication_spec.rb
RenzoMinelli
RenzoMinelli previously approved these changes Jan 20, 2026
@RenzoMinelli RenzoMinelli dismissed their stale review January 20, 2026 19:30

Talked offline and the controller generation from templates doesn't work

@nicolastemciuc
Copy link
Copy Markdown
Member Author

nicolastemciuc commented Jan 20, 2026

Talked offline and the controller generation from templates doesn't work

Fixed in 8c5ede6

I've tested it using our demo app :)

@nicolastemciuc
Copy link
Copy Markdown
Member Author

After try this out on our demo app.

I noticed that overriding the options controller routes would require something like this:

Rails.application.routes.draw do
  devise_for :users, controllers: {
    "passkey/authentication_options": 'users/passkey_authentication_options'
  }
end

Which, IMO, isn’t very user friendly. To avoid forcing users into this setup, I reverted the changes and kept the controllers as they were before.

cc @RenzoMinelli

Copy link
Copy Markdown
Collaborator

@santiagorodriguez96 santiagorodriguez96 left a comment

Choose a reason for hiding this comment

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

LGTM

@nicolastemciuc nicolastemciuc merged commit cc107b4 into master Jan 22, 2026
72 of 75 checks passed
@nicolastemciuc nicolastemciuc deleted the temciuc--not-store-the-challenge-in-helper branch January 22, 2026 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Form helpers should not be the ones storing the challenge in the session

4 participants