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 #129

Open
yshmarov opened this issue Nov 8, 2022 · 6 comments · May be fixed by #238
Open

require email presence in SIGN_IN #129

yshmarov opened this issue Nov 8, 2022 · 6 comments · May be fixed by #238

Comments

@yshmarov
Copy link
Contributor

yshmarov commented Nov 8, 2022

if the "send magic link" form is submitted without an email, there will be an SMTP error.
Screenshot 2022-11-08 at 13 23 35

this is because we do not guard against submitting a nil email.

@fchatterji
Copy link

Hey, i'd like to contribute on this issue if possible. Is it still ongoing? I see the first step has already been merged.

@mikker
Copy link
Owner

mikker commented Feb 7, 2023

Please go ahead 😊 PRs welcome

@fchatterji
Copy link

fchatterji commented Feb 8, 2023

@mikker Ok, I'm not really sure the second step is needed. Usually you would put the validation in the model, and the controller would render the error just fine. But here, the user of the gem creates the user model.

We could add a validation in the controller, render :new and use flash messages to add the error. But it seems overly complex. I think the user of the gem should be responsible for validating that he doesn´t allow empty emails on his own backend.

What do you think?

@yshmarov
Copy link
Contributor Author

yshmarov commented Feb 9, 2023

hey @fchatterji !
Having just frontend presence validation is not enough. This way, a user can input an invalid email, successfully submit the form, and get Net::SMTPSyntaxError in passwordless/sessions # create.

I think we can add some validation in find_authenticable to check if email satisfies URI::MailTo::EMAIL_REGEXP...

@fchatterji
Copy link

Hey @yshmarov, ok thanks, I see what you mean, here's my PR: #134

@wilburhimself wilburhimself linked a pull request Oct 8, 2024 that will close this issue
@wilburhimself
Copy link
Contributor

@yshmarov @fchatterji @mikker Created a PR to address this validation in #238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants