Skip to content
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

fix validatable with password_required? #32

Closed
wants to merge 2 commits into from

Conversation

SethHorsley
Copy link
Contributor

@SethHorsley SethHorsley commented Aug 13, 2023

password_required? now uses the same logic devise uses to check if password should be required which also does the validation checks.

Before the current_user.update_with_password(password_params) would not validate if the confirm was different from the password etc...

Also fixes:

App #⁠1: All users have a saved password; any user may login via email OR password (i.e. accept both methods)
App #⁠2: Only some users have a saved password; those users must login via password; everyone else logs in via email

@SethHorsley SethHorsley changed the title fix validatable if user wants to use passwords too fix validatable with password_required? Aug 13, 2023
abevoelker

This comment was marked as outdated.

@@ -6,6 +6,7 @@ class Mailer < Devise::Mailer
def magic_link(record, token, remember_me, opts = {})
@token = token
@remember_me = remember_me
@opts = opts
Copy link
Owner

Choose a reason for hiding this comment

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

Can you help me understand the need for this? opts is currently used as a passthrough to Devise for things like setting email headers. I take it you are sending a magic link and need to render some data in the view template that you can't get through the user/resource model? In that case, if it were easier to attach extra metadata to the @token itself would that solve your use case?

@abevoelker
Copy link
Owner

I rebased this patch on master (https://github.com/abevoelker/devise-passwordless/tree/iseth/master) but it doesn't work with users that only use the :magic_link_authenticatable strategy. Here's a CI test run example: https://github.com/abevoelker/devise-passwordless/actions/runs/6177687801

You can test the CI/integration specs locally using https://github.com/nektos/act with:

act -j test

@abevoelker
Copy link
Owner

I decided to take a different approach in resolving the original issue, but thank you for your suggestion! Closing now as resolved on master but feel free to re-open if any concerns linger.

@abevoelker abevoelker closed this Sep 14, 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.

2 participants