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

Require email presence in SIGN_IN #238

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wilburhimself
Copy link
Contributor

@wilburhimself wilburhimself commented Oct 8, 2024

fixes #129

Our current implementation lacks robust server-side email validation, potentially allowing invalid or empty email addresses to be processed. This can lead to errors like Net::SMTPSyntaxError and a poor user experience.

Solution

This PR introduces enhanced email validation in the Passwordless::SessionsController, specifically in the find_authenticatable method. It adds checks for:

  1. Empty email addresses
  2. Email format validation using URI::MailTo::EMAIL_REGEXP

Changes

  • Updated find_authenticatable method to explicitly check for blank emails and validate email format
  • Added error handling in the create action to catch and display appropriate error messages
  • Introduced new test cases to cover invalid email formats and empty email submissions

Test Coverage

Added the following test cases:

  • Submission of an invalid email format
  • Submission of an empty email string
  • Submission of a whitespace-only email string

@wilburhimself
Copy link
Contributor Author

@mikker There's a failure about Rails 8.0.0.beta1 and the Ruby version ... I'm not sure how to fix it. Should create a Gemfile for rails 8.0?

https://github.com/mikker/passwordless/actions/runs/11238743396/job/31244211533?pr=238

@mikker
Copy link
Owner

mikker commented Oct 8, 2024

Thank you for yet another contribution!

There's no requirement currently that the passwordless_for field is an email address. So we'll at least need to put this behind an config option and default it to off to not be a breaking change.

Coming back to this issue, I'm also not entirely convinced this is something we need. We don't control how users are created and I think this is more of a thing to validate on creation. What exactly is Passwordless' worry if someone inputs something that's not an email address?

@yshmarov do you have any thoughts on this?

@wilburhimself
Copy link
Contributor Author

Thanks for the feedback @mikker You've raised some excellent points I hadn't fully considered. I agree that adding strict email validation would be too restrictive and potentially breaking for many cases.

So I could:

  1. Remove the email validation I added.
  2. Focus on improving error handling in the create method to gracefully handle cases where invalid data cause issues during the authentication process.
  3. Add a configuration option for users who do want email validation and default to off for backward compatibility.

For users who do want email validation, we could add an optional configuration:

Passwordless.configure do |config|
  config.validate_email = false # default to false for backward compatibility
end

This approach would address the original concern of validating emails while maitaining the gem's flexibility and avoid breaking changes.

What are your thoughts?

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.

require email presence in SIGN_IN
2 participants