Alteration of validation and submit callbacks can cause conflicts

Yup, hardcoding [0] is conflicting with username_enumeration_preventation module. Will see if I can submit a patch for this.

Patch attached with fixes. array_search for concerned validation is added which leaves the door open for other module developers to override the callbacks.

A new patch has been added which checks if array_search does not return FALSE.

I wrote a rather large comment about why, got so big (with lots of history on the why) i won't post it here.

Install user_registrationpassword and username_enumeration_prevention, apply this patch and step-debug with phpstorm to find the issues i have with this. (cause user_registrationpassword callbacks are never ever executed with this patch, cause they are always replaced now). And i've visited the username_enumeration_prevention issues about this, not resolved for years (and i know why).

The modules are just not compatible, and i believe the functionality provided for flood, privacy, etc for the pass reset form, should be provided by 1 module that contains it all. (not 3 or 4 modules that won't work together, like we have currently).

Sorry for being a bit short, get on IRC, then i'll explain with lots of details.

"but it does improve it by not hard-coding the offsets"

Until we have that framework, i don't see how we can change it, cause we need to run, or we need to create some setting.

"If you have a particular combination of modules in use, it's probable you will need a bridging module that orchestrates the final set of callbacks."

Kind of agree, but that would require some effort. And see below.

user_registrationpassword fakes a new state for users (when not logged in / never authenticated and not activated). (access 0 login 0 status 0)
username_enumeration_prevention does not know about this and searches for enabled users, and thus, always sets the wrong message. (the fake one). With or without user_registrationpassword.

So i rather create that module that contains it all with some hooks for other modules to change things, but not a bridge module, cause thats not required if we have 1 dedicated module, something like this, but with a couple hooks. (yes i did do some work in this direction some time ago).

This is my very large post.

(Ok apologies up front to anyone, this looks more like a blog post now, but there's some history involved.)

We expect the callbacks on position 0. This is kinda because a couple of modules should not (just can't) be used together, like logintoboggan, as no decent framework exists for these situations (and some others) AND because the whole enumeration / flood control-bit for the password reset form was kinda not figured out in the opensource drupal world back in the day (at least, the priority / impact wasn't there at that time) (in short: privacy was an issue, but we had bigger fish to fry).

logintoboggan (for example) does something similar like what you propose. See here.

They unset the core submit handler on the 'user_register_form' by checking the key and then they add their callback to the start of the array.

When i visit the code of username_enumeration_prevention here i see that this module does something very similar to what we do.

And when i apply your patch, enable both modules, this is what i experience:

• user_registrationpassword uses HOOK_form_FORM_ID_alter() and username_enumeration_prevention uses HOOK_form_alter() to make their changes.
• Due to the default module weights, user_registrationpassword is initiated before username_enumeration_prevention.

1. When the user_registrationpassword_form_user_pass_alter() visits the array, the original is stil 0 (but only if no other module that runs before user_registrationpassword changes the form and adds something before the user_registrationpassword callback!).
2. Then user_registrationpassword replaces the original by it's callbacks (by key) ( 0 ).
3. Then username_enumeration_prevention just replaces key 0, also expecting the same callback we replace.
4. But 0 now contains the callbacks provided by user_registrationpassword, so now they are replaced and never executed, breaking the functionality provided by user_registrationpassword (only sending password reset mails to activated users and activation mails for never used accounts).

And when i make a simple change, so the callbacks provided by username_enumeration_prevention are executed before or after the callbacks provided by user_registrationpassword, i see another (obvious) issue. The "Further instructions have been sent to your e-mail address." message is now set multiple times and 2 password reset mails are sent, one by core and one by user_registationpassword. So i guess these modules are just not compatible.

I tried to add a (forgive me) B***S*** mode to user_registrationpassword for the password reset form in the past, but i changed the goal to let it become a separate module to prevent over-complicating the code for user_registrationpassword - and because other modules already implemented parts of it, but then my life changed due to required surgery (that was not diagnosed for way to long, still recovering, but all ok now) and thus i didn't completed this module, while i did revert the commit for user_registrationpassword, because of other compatibility/complication issues.

See the code that used to be implemented at some point: here.

Meanwhile username_enumeration_prevention was getting more popular, while containing the same 'glitch', and no debate sprung up to address these issues for D7 (and never will i guess). So contrib now needs to fix this, by creating even better modules, that combine some changes we see modules make to cover these issues, but now let's make them work together. I (for example) would (love to) chop up logintoboggan in a heartbeat, so we have more control over individual items we implement in our projects and not let these things get way to complicated / incompatible (also with lots of new insights we all have gathered in the last couple years), but that would lead to even more individual modules we need to install and lots of corporate sites already run with a boatload of modules, adding more and more just isn't possible.

(Years ago, i also spoke with YesCT and lots of other people about a refactoring the account workflow, so we get to something of a framework for the whole authentication system that would facilitate a structure for these modules/features in D8, but due to my health i never got the time/energy to create a concept for this that actually worked, did had some fun with the inner parts of core, but D8 surely has similar issues and user 'party hats' and lots of other things (pluggable features for your authentication workflow) are still on hold. Read more about this all here and here.)

I'm not so sure how we'r really going to 'fix' this one, nor if we should implement the change here, without making username_enumeration_prevention not execute it's callback if user_registrationpassword is enabled (optionally provide some setting to change this behaviour).

(it might actually be a neat idea to create a setting in user_registrationpassword to enable/disable the provided feature anyway)

And you might also want to start a related issue in the username_enumeration_prevention issue queue and related it to this issue, so we have some reference. I've visited #2172705: username_enumeration_prevention_form_alter can break other contributed modules., but that does not fix the problem, cause when applied, it is runs after user_registrationpassword, and thus, the code provided by username_enumeration_prevention is never executed and if #2589661: Change hook_form_alter to hook_form_FORM_ID_alter lands, we have the same issue i previously described. I rather pursue #1526166: Split enemuration for autocomplete into seperate module. and break it up, so we can create one specific module i figured was required years ago. One that only deals with the password reset form, with flood control / privacy issues covered / etc.

Hope you made it trough this post, and hope this helps a bit.

Thanks, indeed i have and understood.

We could simply wrap the stuff in the form alter in an if statement that checks a variable and expose that on admin/config/people/accounts.
Then add a cache clear to the submit of our form alter that alters admin/config/people/accounts and voila.

Result: if username_enumeration_prevention is enabled together with user_registrationpassword you are able to disable the feature provided by user_registrationpassword. (This does lead to the situation where new users that didn't activate their accounts yet won't be able to 'abuse' the password reset form to maintain a new activation mail, but now it's the site builders choice.)

Let me think about this for a couple days, maybe other people have more suggestions.

Ok, this patch implements the variable and config. Add it to your drush makefile or patch it by hand. I'm not sure if we should commit this as-is, but at least now we have a temporary fix.

As Rob kindly noticed in #2700341: Password reset executed twice / emails sent twice, that issue is caused by the same reason, hardcoding [0]. Here's the pach that makes the module always replace right callbacks, not the first ones. Please let me know if it doesn't resolve any issue mentioned in this thread.

Confirmed, the patch in #14 fixes the lost handler.

@rjjakes what modules did you use to test this?

Would love to have some feedback from people using the username_enumeration_prevention module. I see similar changes when i look at their code.

This patch fixes a minor whitespace issue in #14.

I have tested this with username_enumeration_prevention. Request a password using either a valid username or email address sends the appropriate email. Invalid usernames or email addresses do not send an email, and display the same message, ie. the intended behavior of username_enumeration_prevention.

RTBC or was there some other issue?

@mstrelan sounds like it.

Thanks for testing and for the updated patch.

What do you consider invalid? Unknown to the system? Blocked?

Because blocked and never ever used should sent out an email via user registrationpassword. If thats the case with username enumeration also enabled we can commit this.

Ill review / test / commit later today.

I didn't consider the blocked case, only unknown to the system. Good point.

I'm rethinking / proposing a new approach on how to fix this (at least for username enumeration prevention). What if we check for module_exist(username enumeration prevention) in both validate and submit handlers and call the validate and submit within the module_exist() after the user_registrationpassword code didnt return an account. (Like we already do for core). Then both these modules are compatible.

Other exotic cases are more difficult (like logintoboggan). I'm almost sure user_registrationpassword and logintoboggan will not become compatible ever, so let's focus on compatibility with highly used modules and not this overcomplex case.

I'm almost sure user_registrationpassword and logintoboggan will not become compatible ever

Adding related issues from logintoboggan and username_enumeration_prevention and updating issue description, adding issue template.

I hope logintoboggan commits #1773424 at some point, i do not think username_enumeration_prevention will ever be compatible on a level i'm comfortable with.

So i think we need a new battleplan. I had some time to work on the Drupal 8 port of user_registrationpassword and meanwhile also found some time to work on *this* issue, so here it goes.

• Attempt to get logintoboggan issues fixed (at least #1773424).
• Revive the code that used to exist in user_registrationpassword for securing the password reset form (deprecating the need for username_enumeration_prevention once user_registrationpassword 1.6 is released - and this prevents overcomplex code in user_registrationpassword to fix this case).
• Implement hook_module_implements_alter() and make sure we run last (or at least, try to) and replace the core handler, not key 0 - and (re-)setting it as the first in the array.
• Once this has all landed, implement fixes for other modules that we want user_registrationpassword to be compatible with.

Moving to D8.

Adding patches for both the D7 and D8 version.

Created #2928768: username_enumeration_prevention compatiblity as a followup for these patches, also both D7+D8.

I'll create another issue for logintobbogan when i'm ready, working on a patch that makes them semi-work together, but needs more time. (also lt needs some critical other fixes, so i tend to provide the same fix as for username_enumeration_prevention: just replace the callbacks again).

Once the 8.x patch is committed, the 7.x patch should follow soon.

I couldn't apply the last patch with composer because the test file already existed, on Drupal 8. So i rerolled the patch; hoppefully it will work.

I patched the module with #24 on Drupal 8, but it didn't work because a test was missing. I updated it with #25 but failed because some test didn't of class imports. So corrected it on #27.

Here is an updated patch. It works for me in conjunction with https://www.drupal.org/project/simple_pass_reset and some custom code that adds a validation function.

Hello,

Just a quick one to confirm that the patch in #29 solved the conflict issue I had between this module and the Username Enumeration Prevention module.

Thanks a lot!

Arno2Mars thanks for testing and the feedback!

Fixed 1 issue: D9 requires more params. No other changes.

Adding #3167137: Report incompatible modules on status report page as a related issue. This can use some testing / feedback.

Hey Rob C,

Thanks for this last patch in #32.
I confirm it works again perfectly for me.

One point though (hope it is not too silly):
Applying this patch makes my site work great again with both User Registration Password and User Enumeration Prevention installed:

• When I enter a non existing user in the password reset form, I have no more error.
• When I enter an existing user in the password reset form, the user receives the email with the one-time login link in his email box and can connect to change his password. The sending of the email is also correctly recorded in dblog.
• In both cases (user exists or not), the Drupal message displayed after the password reset form submit is the one from the User Enumeration Prevention module (--> 'If the username or email address exists and is active, further instructions have been sent to your email address.'), which is the intended behavior and indicates that this modules works.

I understand that the fix provided cannot be a robust long term solution until some deeper changes are made in core, and I understand also the need to warn the user that installing both modules may result in incompatibility (error and/or wrong behavior).
But it works at least in the configuration I have, which is using the latest version of both modules + your patch, so I guess a configuration used by most users. Therefore having this red error in the Status Report appears maybe a bit too strong.

May I suggest the following instead:

• Replace the error by a warning in the status report, saying something like: The following modules may not be 100% compatible with user_registrationpassword: @modules', ['@modules' => $modules_found_string]. Installing both on a same site may result in errors or unwanted behavior. Please test password functionalities carefully to prevent any issue before releasing in production.
• Commit the patch (maybe after more reviews by other users to ensure it works well for everybody and not just for me) and release a new module version, so that users installing this module would get this fix automatically, with a chance to not have any problem.

Otherwise keeping a red error would be I think detrimental for User Registration Password. This would fear the users and incite them to uninstall of the modules. And users may prefer to keep User Enumeration Prevention which is addressing Security, despite User Registration Password improves greatly the registration workflow.

Another suggestion (hope it is also not silly) would be maybe to merge both modules into one? If you integrate the functionalities of the User Enumeration Prevention in User Registration Password, doesn't it allow you to fully control how the password is hooked?

Hoping it helps.
May you need further testing from me, please continue to post on this issue that I am following, and I will help as much as I can.

Cheers,

Adding related issue.

Ignore #32, i added the code from #3167137, that should have been a separate patch. But we also have #2928768: username_enumeration_prevention compatiblity (totally forgot about that one, i already dove into this some time ago).

This is the same patch as #32 with the hook removed (so only fixes the missing parameters for the test).

#36 also helps with Shared Email Password Reset (if you install module in the right order or tweak their weights..)

Thanks everyone for contributing.
Committed to 2.0.x