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 invited user registration without SMTP #5712

Merged
merged 1 commit into from
Apr 4, 2025

Conversation

Timshel
Copy link
Contributor

@Timshel Timshel commented Mar 19, 2025

Perform the user check only if verification email is used.

It was blocking the registration of invited user when email was disabled.

Edit: I wonder if it would not make sense to just always send the email ?
It does create an unintuitive flow if the user already exists since it will fail later on, but currently it means that if the user has deleted the email he has no way to verify his email anymore.

Copy link
Owner

@dani-garcia dani-garcia left a comment

Choose a reason for hiding this comment

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

LGTM, just one duplicate if with the move.

We can consider changing the email sending from user.is_none() to user.is_none() || user.password_hash.is_none(), which should allow resending registration emails to users that are invited but not registered yet. Is that what you're thinking about?

@Timshel Timshel force-pushed the fix/registration branch from 3c9507b to e54a7af Compare April 4, 2025 11:33
@Timshel
Copy link
Contributor Author

Timshel commented Apr 4, 2025

just one duplicate if with the move.

Sorry about that :(.

For the resending was thinking of sending it in all cases but your suggestion is way better since it will prevent abuse.

Hope you don't mind used the private_key as an indicator that the user is initialized (A leftover of when I made a VW version which was not storing the MP when using OIDC).

Copy link
Owner

@dani-garcia dani-garcia left a comment

Choose a reason for hiding this comment

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

LGTM, yeah checking for private key works as well

@dani-garcia dani-garcia merged commit f960bf5 into dani-garcia:main Apr 4, 2025
5 checks passed
@Timshel Timshel deleted the fix/registration branch April 4, 2025 11:56
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