Problem/Motivation

When needing to verify email address of existing users, some users might be logged in when they are instructed to open the form.

verifyEmailForm::buildForm is directly redirecting anonymous users, preventing any way to intervene.

  if ($this->currentUser()->isAuthenticated()) {
      return new RedirectResponse($entity->getRedirectPath());
    }

The easiest way to overcome this would be to extend this form and tweak it. but it's a final class.

It would also be very nice if the class Verifier.php wasn't final.
It isn't advantageous in my case that it creates users.

Proposed resolution

Is there a reason that the classes are final?
Should we make them not final?
Should we use permissions instead of authentication status (will need dynamic permission creation per form)?
Maybe move the redirect to a controller or event subscriber because it's anyway not best practice coming from buildForm function (more complexity that's not justified for the time being).

Let me know what you think. thanks.

Issue fork verify_email-3554870

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

yoa created an issue. See original summary.

lostcarpark’s picture

lostcarpark commented

Some excellent ideas here.

I think the reason for making classes final might have been to stop PHPStan warning about "unsafe use of new static()" in the create function. I'm aiming to move to autowiring, which should allow most create functions to be deleted. But preventing subclassing is clearly not desired, so let's get the final removed.

Permissions sound like a good way of dealing with redirecting. I could see value in having "allow email verification", and "bypass verification" permissions for each verification form. If you don't want to allow user creation, just set "allow email verification" to "authenticated users". If you want to skip verification for logged in users, then set "bypass verification" to "authenticated users". Otherwise, "bypass verification" could be granted to a role for users who won't have to complete verification.

Another useful option would be to add an option to the verify email form settings to allow a user to be added to a role on successful verification. That way, users could be added to a "verified users" role once verified, and that role could be granted "bypass verification" so they wouldn't be challenged again.

Moving the redirect out of the form does make sense. I have a few thoughts on the best way to do it.

As there are a few different pieces, I'll create some smaller issues to tackle them individually.

lostcarpark’s picture

lostcarpark commented

Hi @yoa,

We have now removed the "final" class declarations, so subclassing should be possible.

We have also added two permissions (per verify form), "Access verification form" and "Skip verification form".

You can grant "Skip verification" permission to roles that will be directed to bypass the form, so it will be shown to logged in users who don't have that role.

The "Access verification" permission doesn't do anything yet, but that will be the next issue. That will let you determine who can access the form. I think you only want logged in users to be able to verify, so if you only grant this permission to "Authenticated users", anonymous users won't see it.

These are currently only in the Dev release, but when we have a few more issues completed, we'll look at moving to a Beta release.

aayush7 made their first commit to this issue’s fork.

aayushpathak’s picture

aayushpathak commented

working on it !

aayush7 opened merge request !56

aayushpathak’s picture

aayushpathak commented
Status: Active » Needs review
lostcarpark’s picture

lostcarpark commented
Status: Needs review » Needs work

I'm not sure about some of the changes on the MR. It would be really helpful to post a comment summarising the changes and the rationale behind them.

Also there seem to be some changes unrelated to this issue, such as updating the ReadMe. As this issue is dealing with a fundamental part of the module's logic, I feel that we should keep the changes in this issue entirely focused on that. Any unrelated changes should be moved into separate issues.

At present there are several code quality errors, as well as test failures. These will need to be fixed before merging could be considered.

lostcarpark’s picture

lostcarpark commented

I can see several useful parts of MR !56, but it's a bit vague what issue it's solving, and there seem to be parts that aren't related to what's been requested in this issue.

I will open separate issues and refer to parts of this MR, but I'm not keen on this MR in its current form.

lostcarpark’s picture

lostcarpark commented
Status: Needs work » Postponed

Setting this issue to "postponed" as I don't want to merge a change directly under it. I have created several child issues, the latest of these is #3592953: Use "email" field type and better validation of email.