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

Email Verification Problems #1613

Closed
FatihKoz opened this issue Sep 17, 2023 · 10 comments · Fixed by #1629
Closed

Email Verification Problems #1613

FatihKoz opened this issue Sep 17, 2023 · 10 comments · Fixed by #1629

Comments

@FatihKoz
Copy link
Contributor

Currently email verification works in below logic;

  1. User registers
  2. Pending mail is dispatched
  3. Either automatically or by admin/staff user gets accepted
  4. Accepted mail is dispatched
  5. User tries to log in
  6. Gets a warning about email verification

Here the problem starts (base of this issue)

  1. Email verification mail is not dispatched automatically, user needs to click "send a new one" link
  2. Verification mail is dispatched
  3. User clicks the link (or the button)
  4. Gets 403 Not Authorized error (if he/she is using different browsers for this action, imagine like login is done from Opera, but the mail is opened from phone or Chrome etc.)

Version
beta-5 and up

To Reproduce
Steps to reproduce the behavior:

  1. Register as a new user to your own VA with a different email
  2. Follow the steps as usual and use a different browser to click the verification link
  3. See the error

Expected behavior
Technically speaking, I think the verification should happen just after registering the VA, not after being accepted. Admins should be sure that the pending application is valid and ready to be accepted.

So in theory;
Register > Verification Mail > E-Mail Verification > Pending Mail > Accept or Reject (auto or by admin/staff) > Result Mail
should be the order of things.

And users should not be forced to click "send verification link" after waiting it to arrive, this process should be automated fully. Something like `We have send your verification link at 18:59 to your ...@.... address, if you do not receive it, please check spam folder and request a new one after ... minutes" would be nice in that blade.

Additional context
Email verification should be tied to an admin setting (like auto accept), people may want to disable it at all. This should not be a forced feature.

Also we may consider adding a second field to double check the mail entered during registration, like the password... Password + Confirm Password , Email + Confirm Email fields to be sure it is entered properly twice.

@FatihKoz
Copy link
Contributor Author

@ArthurHetem , @nabeelio any thoughts or ideas about this ?

@ProAviaAZ
Copy link

ProAviaAZ commented Oct 15, 2023

Any chance of an update for this?
@ArthurHetem
@nabeelio

Email verification email is not automatically sent. User must click the "click here to request another" link.
Happens when using same browser.
image

@ariel035
Copy link

ariel035 commented Oct 15, 2023 via email

@FatihKoz
Copy link
Contributor Author

This needs lots of changes (or improvements) like;

  • Admins should be able to enable/disable verification easily via settings (not via editing a config file)
  • Admins should be able to remove a verification entry manually (as all current users are verified by migration, they may want a user to re-verify)
  • Verification should be removed when a user changes his/her verified email

And as I wrote in the first post, order should be different for new users, they should verify their email before being accepted to the va, not after.

@arthurpar06
Copy link
Contributor

arthurpar06 commented Oct 15, 2023

We can easily send the verification email when the user registers (at the same time as the pending email etc). I think that the event that is listened to automatically send the verficiation email when the user registers is not the right one, if we change it we fix it.

However, blocking the pending flow (sending email to admins, offering the ability of accepting it, etc...) as long as the email has not been verfied seems more complicated.

@FatihKoz
Copy link
Contributor Author

Sure, but why should an admin gets an email about a non verified user?

It may be hard to change things but if we are introducing something critical like this we need to consider all options and provide the best solution. Currently this is not something running fine (considering device differences and 503 errors) :(

Anyway, maybe dev needs to add a new event like UserEmailVerified to trigger new user registered actions to fix the order, another one like UserMailChanged to trigger an automated verification etc. etc.

@arthurpar06
Copy link
Contributor

I agree.

I would suggest correcting the EventServiceProvider to automatically send the verification email upon registration. Then when verified we send the pending email/the email to admins/ we display the user as pending...

As for device changes, this is probably a Laravel security system and I don't know how we could disable it.

Finally adding a button which allows admin to request a new email verification / request a new one if the email is changed should be possible since we just have to edit a field in the db.

I can try todo all this tomorrow if I find some time otherwise Tuesday maybe

@FatihKoz
Copy link
Contributor Author

It may be us (I mean v7) forcing auth for that verification route... Just thinking :) If I can find some time, I can inspect this too (probably tomorrow afternoon in the office as I will be working on another laravel project using email verification)

@ArthurHetem
Copy link
Contributor

I'm taking a look at this right now, the event of registration apparently is the wrong, i've added the verification email on the NotificationEventsHandler before the acceptance e-mail.

For the error when the user tries on another device, i can't undestand exactly why this is happening, as in the RouteServiceProvider its group don't have the auth middleware

Admins should be able to enable/disable verification easily via settings (not via editing a config file)

I and @nabeelio talked about it on the PR, and we concluded that it will not be fine to make db calls when the application is starting (the email verification needs a middleware on the RouteServiceProvider)

@ArthurHetem
Copy link
Contributor

Actually, found the 503 error, on the laravel-ui VerifiesEmails, it uses the authenticated user to get some data
image

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 a pull request may close this issue.

5 participants