Problem/Motivation
Currently, when a new user registers for a Drupal account the details are sent to the user's supplied email address. This provides a basic mechanism that confirms the user is at that email address. However, once registered, users are permitted to change their email address without further confirming that the user is in fact at that email address.
Possible implications
- A user can change their email address to be that of an unsuspecting third party as no confirmation of change is required. Using a second Drupal account (with it's email address also faked using the same method) the first user is then able to send anonymous malicious messages to the unsuspecting third party
- A slow method for sending spam but exploitable none the less
Proposed resolution
Add a mechanism (similar to reset password) that:
- Sends an E-mail to the new address requiring the verification of the new address (similar to register confirmation).
- Sends a notification E-mail to the old address.
- Allow the site builder to customise both messages at
admin/config/people/accounts
- Provides an update path to set the default behaviour and messages content.
- Write tests.
This new mechanism is bypassed if the e-mail address is changed by an administrator.
Remaining tasks
- #279.2:
Seems to be fixed as well as possible right now given limitations of other issues. Raise a follow-on issue to tidy up once other issues have been fixed. #279.6: Add a test - see further explanation in #284Release note snippet for IS.#345 test #2 outstanding- Not easy to test as involves email send failure - code seems OK#347 unaddressed- Addressed around #356, with new test included- Security review.
Follow-up questions from #270: (Potentially follow up change as there are other potential improvements in #358 that would resolve a lot of these and go further to improve UX)-
Should there be some indication that additional form fields are available once checked? (NOTE: Same issue in Account Canceled email form)See #360Should the suggestions exist prior to checking the box? (NOTE: Same issue in Account Canceled email form)See #360Should we make the request visible as pending to user and/or admin until completion?See #360Should there be visible history (related question, are users revisionable)?See #360
Follow-up questions from #272:See #360-
Should there be an exception for admin users changing their own email?- In #312 noted admin users are generally able to do a lot of things without checks, including remove their own rights, so already trustworthy to set a new email correctly without needing to verify it
Check that the unresolved comments on MR 437 are fixed on MR 5774- #334 Create a followup for JSONAPI
- Don't re-use user_mail_notify or mutate the email on a cloned version of the user in case someone calls ::save() on it
- Add flood control to the change email controller
See #360
User interface changes
New UI additions to admin/config/people/accounts
:
New confirmation message (warning) when user changes e-mail address:
Default text of the generated e-mail (some elements will vary):
alice,
A request to change your email address has been made at Drupal Usability. You
need to verify the change by clicking on the link below or copying and
pasting it in your browser:http://drupalux.lndo.site/user/mail-change/2/alice%40example.org/1542687...
This is a one-time URL, so it can be used only once. It expires after one
day. If not used, your email address at Drupal Usability will not change.
After using the one-time link, the user is redirected to the site's home page, with the message (info)
Your email address has been changed to alice@example.org.
API changes
New controller used for mail changing: \Drupal\user\Controller\MailChangeController
Data model changes
New schema for configs user.settings
and user.mail
.
Release notes snippet
When users wish to change their email, they must now verify the email belongs to them using a link sent to that address. This behavior is enabled by default on new installations but disabled by default on existing installations. Review the change record for more information.
Comment | File | Size | Author |
---|---|---|---|
#452 | 85494-451-drupal9511.patch | 24.02 KB | lawxen |
#447 | 85494_447_10_2_3.patch | 43.11 KB | InaW |
Issue fork drupal-85494
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:
Comments
Comment #1
beginner CreditAttribution: beginner commentedLOL! Why single me out :) I'm innocent, I didn't do anything, I promise!
Can the code be extended for deal with another situation where the user's email is not confirmed: when new user accounts must be reviewed by the admin before being set active. I have had this happen on a Drupal site. If the settings is that the admin must authorized new accounts, the admin may waste time reviewing new accounts created by robots or by a human spammer with fake emails. It is not always obvious which account is bona-fide, which is fake. The admin shouldn't have to make a decision on an account with an invalid email (user error is also possible).
The workflow should be:
1) user creates account
2) user confirms email is valid (where part of your code is relevant)
3) admin authorizes (or not) the account.
The following ancient proposal is not necessarily good, but it may give some ideas for a better systematic approach: http://drupal.org/node/64861.
My point is that your new function user_change_mail() should be user_confirm_mail() so that it is general enough, and reusable in the different scenario I've exposed.
I hope you don't regret that I DID follow up with "the not so easy bit" ;)
Comment #2
AjK CreditAttribution: AjK commentedNot at all, your proposal sounds good. However, as drumm has pointed out, let's keep core patches nice, simple and byte sized ;)
This issue addresses one simple potential exploit which I would class as a bug. What you want is, imho, is a feature. A good one, but none the less, a feature. Introducing that on this issue will hamper it's chances of a commit.
The current issue is plain, simple and hopefully committable (review will tell).
I'd be happy to look at what you want to do, just lets get it on another issue and thrash it out there so this cuurent issue doesn't get bogged down.
Sound reasonable?
Thanks for the feedback.
regards,
-AjK
(p.s. I didn't single you in particular, just a gentle prod to get a review! How sneaky of me ;)
Comment #3
beginner CreditAttribution: beginner commentedYour 'sneakiness' worked (and I don't mind it, so don't worry) :)
I know different issues must be kept separate, so I perfectly agree with your point.
My question still stand, though: is there a way to make your new function more general so it can be used for this bug report and reused for other separate issues later?
How difficult would it be to reuse existing code and make it more general to reuse in every situation when we want a mail check?
Comment #4
AjK CreditAttribution: AjK commentedThe answer to your question is "probably fairly easy". The only reason I'm holding back on this is that we are in code freeze right now.
I believe this issue is regards a "critical bug" and labelled as such stands a chance of being committed.
I believe what you want is a "feature" and if this issue thread goes down that road I'm 100% certain this issue won't be addressed in the code freeze before Drupal5 is released.
I would suggest you start a new issue for what you want and in link back to this issue and say "could have gone in that issue but missed the boat".
I'm all up for doing what you want, it's a good solution, just not on this thread (we can develope what you want on a new thread right now, it doesn't have to be postponed, just I'd like it on it's own issue, pref as a patch after this patch gets commited, to modify the function in core, not modify a patch with a patch).
seem reasonable? (if/when you start a new issue, contact me via my contact form so I can get subscribed to it)
regards,
--AjK
Comment #5
drummComment #6
AjK CreditAttribution: AjK commentedFound a bug in this patch.
When user updates their "my account" but leaves the password fields empty (i.e. they don't want to change their password but are changing something else) the email still gets sent informing them of their "intention to change their password".
Obviously, if the password fields are empty the "change email" workflow should not kick in and the email should not get sent.
I'll sort this out shortly but for now changing status to "code needs work"
regards,
--AjK
Comment #7
AjK CreditAttribution: AjK commentedCorrected bug in #6
Comment #8
webchickgoing to test this later.
Comment #9
Heine CreditAttribution: Heine commentedSome t calls in #7 need work:
Afaik, % calls theme_placeholder, additional em-tags are not necessary. In the mail t with %-variables will get you em-tags in the plaintext mail.
Comment #10
hunmonk CreditAttribution: hunmonk commentedupdated patch attached. this addresses many issues:
patch has been pretty thoroughly tested on a clean HEAD site, and seems to work perfectly.
Comment #11
chx CreditAttribution: chx commentedExcellent work, Chad!
Comment #12
Dries CreditAttribution: Dries commentedWe shouldn't set 'X-Mailer' => 'Drupal'. AFAIK, drupal_mail already does that ...
I'll provide a more in-depth review shortly.
Comment #13
webchickadding to queue...
Comment #14
hunmonk CreditAttribution: hunmonk commented@Dries: looks to me like we don't need that header array at all there.
attached patch removes it. anything else we need to do here?
Comment #15
Dries CreditAttribution: Dries commentedThis patch needs work. Punctuation is not consistent; some sentences end with a point, other don't. Also, the URL scheme is awkard.
Restting a password happens at ?q=user/reset. Restting an e-mail adress happens at ?q=user/change/mail. Maybe that should be ?q=user/reset-password and q?=user/reset-mail.
I haven't tested the patch yet but it is not clear what happens as long the new e-mail adress hasn't been confirmed. What are the implications?
Comment #16
AjK CreditAttribution: AjK commentedThe "reset email" that is sent out is time limited (24hours). If the reset link is not activated within this time, the email address is not altered and remains the same. In fact, it doesn't change to the new address until activation by the one time, time limited email link.
As for the implications, it means that if the Drupal generated link in the email sent out isn't activated within it's given timeout value, the email address is not altered.
Did that make sense? It's late!
Comment #17
AjK CreditAttribution: AjK commentedPatch attached addresses / corrects this.
Hmm, I may be English but languages is "low" on my "best of" list. So, I failed to spot the missing full stop!
Comment #18
hunmonk CreditAttribution: hunmonk commentedokay, i believe i've fixed up the last of the problems:
tested the mail change and password reset functionalities on a clean HEAD site, and both are working perfectly AFAICT. it would be great if we could get one more person to test them both...
Comment #19
edmund.kwok CreditAttribution: edmund.kwok commentedTested on fresh head:
1) Email changed only when the activation link is clicked. Did not wait to see if the link is disabled after the timeout though
2) Reset password works as it normally does
I noticed that in the confirmation mail that was sent when the email is changed, the From field is the user's old email address. Is this intentional? Or should the mail come from the site's email address?
Comment #20
hunmonk CreditAttribution: hunmonk commentedper chx's consult, changed $from to the site mail.
Comment #21
chx CreditAttribution: chx commentedJust before the beta. To sum up: what's the point in the initial email confirmation if I can change it to foo whenever?
Comment #22
hunmonk CreditAttribution: hunmonk commentedpatch broken by the capitalization commit. updated patch attached. no changes in code functionality whatsoever--just fixed up the conflict and updated the capitalization.
Comment #23
Dries CreditAttribution: Dries commentedchx: note that we store the initial e-mail address, even if you chose to change it later. $user->init should always be valid, and we should have some sort of audit trail.
(We've never had this kind of functionality, and all of a sudden it is critical? The critical status is debatable, IMO.)
Comment #24
chx CreditAttribution: chx commentedNote that this is the child of the contact spam issue which is indeed a very serious one -- hence the critical.
Comment #25
Dries CreditAttribution: Dries commentedI think there is room for improvement:
1. I wonder what my mother will think when she reads: "The account identification numbers did not match.
2. "There was a problem with this one time link. Please try again.'" Try what again? And what difference could it make?
Comment #26
hunmonk CreditAttribution: hunmonk commentedper Dries' request, i have reworked the user messages in this patch.
Comment #27
hunmonk CreditAttribution: hunmonk commentedComment #28
drummIs there any particular need to change the password reset url? I think it is fine at the old place until a separate patch decided it should happen.
"If it is not used, your e-mail address change will not be processed." might want to say what will happen instead of what won't happen- "If not used, your email address at [site] will not change."
Many sites send a non-essential email to the old email address as a notification of the change and what to do if the change wasn't intended. Should we have that?
Comment #29
AjK CreditAttribution: AjK commentedsee #15
Good point, I would say so (if we're copying the functionality often found on other sites, might as well make it a deep copy ;)
Comment #30
hunmonk CreditAttribution: hunmonk commentedper dries' comment in #15 i'm leaving this change in.
fixed.
yeah, seems like a good idea. added.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedi question the need for this. it is trivially easy to forge the from address of any email, not just ones sent from drupal. if proper authentication is our goal, i suggest we add a header like X-Drupal-UID and send the uid there. Or send the $user->init email address. Those are both authoritative/verified.
I don't really object to this going in, but it doesn't buy a whole lot and the critical status is shaky at best.
Comment #32
moshe weitzman CreditAttribution: moshe weitzman commentedactually, contact.module already states the UID of the sender so we are covered there. thanks for illustrating that, boggieman.
Comment #33
moshe weitzman CreditAttribution: moshe weitzman commentedComment #34
hunmonk CreditAttribution: hunmonk commentedpatch updated. functionality is the same.
Comment #35
AjK CreditAttribution: AjK commentedComment #36
drummComment #37
Zen CreditAttribution: Zen commentedPatch no longer applies; re-rolled with updates to hook_menu; untested.
-K
Comment #38
hunmonk CreditAttribution: hunmonk commentedno time to put into this anymore. any takers?
Comment #39
hickory CreditAttribution: hickory commentedI'd say this is critical. We need to be sure that a user owns the email address registered for their account, otherwise what's the point confirming it in the first place?
Comment #40
hickory CreditAttribution: hickory commentedAlso, without this confirmation, a user could change their registered email address to someone else's, causing all kinds of problems (content associated with a person, unable to create a new account, etc).
Comment #41
igorzh CreditAttribution: igorzh commentedHello everyone, has this problem been solved in drupal 5.x versions?
Comment #42
hickory CreditAttribution: hickory commentedChanging back to 6.x-dev. I doubt this will be changed for 5.x.
Comment #43
chx CreditAttribution: chx commentedThis can happily live in a contrib. Viva la form_alter!
Comment #44
birdmanx35 CreditAttribution: birdmanx35 commentedNo surprise, but this fails against HEAD:
$ patch -p0 < user-mail-change.patch
(Stripping trailing CRs from patch.)
patching file modules/user/user.module
Hunk #1 FAILED at 801.
Hunk #2 FAILED at 1256.
Hunk #3 succeeded at 1381 (offset 106 lines).
Hunk #4 succeeded at 1536 with fuzz 1 (offset -14 lines).
Hunk #5 FAILED at 1763.
3 out of 5 hunks FAILED -- saving rejects to file modules/user/user.module.rej
Comment #45
sunRe: #40: A user is not able to change her email address to one that is already used by another user.
Comment #46
jaydub CreditAttribution: jaydub commentedAs chx said: "This can happily live in a contrib" and here is the proof:
http://drupal.org/project/email_confirm
feedback is welcome
Comment #47
moshe weitzman CreditAttribution: moshe weitzman commentedI withdraw my objection earlier. This reallty does make perfect sense. Considering where we are in the release cycle, critical is warranted here.
Comment #48
moshe weitzman CreditAttribution: moshe weitzman commentedI should elaborate on my use case. My client is is borrowing an idea from Facebook about authorization. Specifically, if you have a verified @harvard.edu email address then you belong to harvard group and are allowed to see certain premium nodes and so on. Naturally, we need to verify email address changes so that people don't just switch into harvard group.
So, I hope the need has been proven and someone can move on to polishing and committing the patch.
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribing. Currently I'm drawn toward leaving this feature as a module add-on. Workflow is different for different sites and what one might do another will do something else. If the feature is added to core it needs to be an option that the admin can use or not use. So, this might as well as just be a contrib package.
Comment #50
mikeryanShould this patch get re-rolled for D7, please have it call hook_user('update') - for Moshe's use case, other modules may need to know when the email address has been changed and take action based on the new address.
Comment #51
Taxoman CreditAttribution: Taxoman commentedFYI - Decscription / Feature request for the email_confirm module mentioned in #46:
"Account hijack prevention management (prerequisites)"
http://drupal.org/node/475264
Comment #52
robby.smith CreditAttribution: robby.smith commented+1 subscribing
Comment #53
jrabeemer CreditAttribution: jrabeemer commentedThis seems like a good core feature to have for D7. I hope it doesn't die here.
+1 subscribe
Comment #54
AaronBauman"feature request" and "critical" are mutually exclusive.
maybe priority-major is warranted.
Comment #55
Damien Tournoud CreditAttribution: Damien Tournoud commented7.x is feature frozen anyway.
Comment #56
marcingy CreditAttribution: marcingy commentedChanging to major as per tag.
Comment #57
MustangGB CreditAttribution: MustangGB commentedTag update
Comment #58
amc CreditAttribution: amc commentedsubscribing
Comment #59
geerlingguy CreditAttribution: geerlingguy commentedSubscribing. Some of this functionality should, IMHO, live in core. I've form altered the registration and account forms too many times to get this/similar functionality :-/
Comment #60
webchickUnless I'm totally mistaken, this feature is already in D7, so this is fixed.
Comment #61
geerlingguy CreditAttribution: geerlingguy commentedWait... I thought the issue was about actually confirming the email address itself, not the user's *intention* of changing the address. E.g. If a user changes his email, then the user would get a confirmation email sent to the new email address, and the new email address wouldn't be used for $user->mail until its confirmed.
Right? That's kind of what I read in most of the posts here... I think.
Comment #62
webchickOops! You are right, sorry. :)
Comment #63
shadysamir CreditAttribution: shadysamir commentedTotally the wrong place to post this. That's what you get when you post while asleep. Sorry folks
I use LT along with http://drupal.org/project/email_confirm which deals with this issue.My problem is when a user changes their email address BEFORE verifying the old one. This function is needed as a second chance for users to use a different email address and activate their account. But even though email_confirm is one form of email confirmation, LT is nor aware of it or that it already verified the address. I think I will write something for email_confirm to integrate with LT.How do force account verification from PHP?Comment #64
ralf.strobel CreditAttribution: ralf.strobel commentedI support this request, as not having a valid user email address at all times is also a potential legal issue.
Users could post illegal content and there would be no ways of persecuting them.
It's not hugely problematic, as Drupal retains the original, validated address in the "init" field. But that address might be outdated and no longer functional.
Comment #65
andypostThis functionality should stay in contrib, email could be changed by custom auth provider and it's not really useful for now. Let's close this issue
Comment #66
Taxoman CreditAttribution: Taxoman commentedIMO, this is important to have in core. Absolutely needed.
Comment #67
amc CreditAttribution: amc commentedAgreed. Some even thought to make it critical -- while that's debatable, it's definitely a hole that needs to be fixed and should be part of core's default system.
Comment #68
rogical CreditAttribution: rogical commentedThere's a potential security issue here, user password may somehow happened exposed -- simple password, same password with other low secure sites etc.
Then people get the password can have full control of the account -- change password, email etc. And you'll never can get back your account!
So, we should at least let the hackers not able to change email easily. User needs to have email change confirm in the old email address and this features is very common in websites.
This feature should be able to port to D7, as it's a potential security issue!
Comment #69
arnoldbird CreditAttribution: arnoldbird commentedIs there a contrib module that provides this functionality for Drupal 7? Thanks
Comment #70
jaydub CreditAttribution: jaydub commentedhttp://drupal.org/project/email_confirm - have a beta release for testing on d7.
Comment #71
greggles@rogical - since Drupal doesn't store the password in plaintext I don't see how a password change could expose the user's current password. Further, I don't see how someone can be tricked into setting their email to that of an attacker.
Tagging to increase awareness.
Comment #72
ralf.strobel CreditAttribution: ralf.strobel commentedI rather see a legal problem here and not a security problem. As a web site owner, you usually wish to maintain a valid email address for each of your users, in case they do anything illegal. So users shouldn't be allowed to change their address to an unconfirmed one.
You could also defend it from a usability point, since the verification protects the users from misspelled addresses which would prevent them from receiving updates.
Comment #73
jibranThis is the re-roll against 8.x from #37 and a working patch.
Comment #75
Homotechsual CreditAttribution: Homotechsual commentedThis bug has been in existence since D5.x (2006) it's managed to exist before, during and after code freeze on D6, D7 and D8. Whilst the need for debate is understood this is a pretty basic and low level feature. The use case for it has been iterated and re-iterated many times on this issue queue and whilst a contrib module does exist the case is STILL there for this to appear in core.
There are, from what I can see, no blockers to this becoming a core feature so the time has come to ask why, after 7 years, this is not already a feature?
Is this ever likely to appear in core?
Comment #76
kifuzzy CreditAttribution: kifuzzy commentedHi, may out of topic but may more around questioned by you one of my bookmarks
knowledge-base/contributed-modules-securing-your-drupal-site
(Submitted by Ben Jeavons on Thu, 09/08/2011 - 15:53)
[Among the thousands of modules on drupal.org there are over 100 in the security category. Unfortunately some of those are abandoned or inaccurately tagged. We've looked at every module and compiled this resource to help you understand the security-related community modules available. Not all modules provide security exactly, some are about hardening your site against weaknesses and others are about monitoring and reporting abuses (....)]
.. or link drupal-7-user-login-and-registration
Comment #77
Homotechsual CreditAttribution: Homotechsual commentedThere is, at least in part. The module is: https://drupal.org/project/email_confirm
Comment #78
dawehnerRelated to #2014185: Check usernames that are email addresses more rigidly, only allow if matches email
This sounds like a feature request contrib should provide and probably can.
Comment #79
joachim CreditAttribution: joachim commentedI think this is an omission in core, for the reasons outlined in the issue summary. I don't think it should be left to contrib.
Comment #80
kaareDon't see why this is a feature request. Smells like a bug to me. Today, at least. Maybe not 9 years ago when Drupal was a hobbyist project.
In addition to be a slow-spam method, it has UX-implications and/or potential security risk: If you by mistake enter a non-functional email, you won't receive reset password info if you forget it. Your account is effectively lost and you need to contact support. And who is support to verify that whoever calls is the rightful owner? It's just a random email nobody owns.
Comment #81
andypostComment #82
kaareThe email address is more and more used as the primary ID of user accounts. The concept of user name is a fading thing. That Drupal 8 will come out with a ID-mechanism this flawed is, uhm, not good. Yes, contrib can do this, but having this functionality in core will assert its quality and assure that the user is who it claims to be.
Comment #83
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedI'm working on this. I wasn't able to rebase the patch in #73, so I started rewriting it. I finished the first part for now. This patch contains the WIP.
Comment #84
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedI rewrote the rest of the patch, but didn't test this manually yet. I'm just curious if we have failing tests now, so triggering testbot.
Also this will need tests, so tagging accordingly.
Comment #86
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedComment #87
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedComment #89
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedComment #90
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAdded some minor changes, and added some coverage.
Some remarks/questions:
- I think we might need a follow up for migration from D6/D7.
- The current permissions don't specify changing an email address, only 'cancel own user account', 'change own username' and 'view user information'. Should we add one specifically for this use case, e.g. 'change own email address'?
- I can't find any unit test coverage for UserController. Shouldn't we do that here? Specifically for UserController::changeEmail().
Comment #91
donutdan4114 CreditAttribution: donutdan4114 commented+1 this should be fixed. It is clearly a security concern, especially for sites that integrate with 3rd party services that rely on a verified email address.
No idea why Drupal would verify a new user email address, but not reverify when they change it.
Comment #92
deanflory CreditAttribution: deanflory commentedJust a reminder to other users that the patch in #90 is for D8 and not D7.
Comment #93
ptmkenny CreditAttribution: ptmkenny commented@pjonckiere: Regarding the need for a follow-up, it would be great to get this fixed in D7/D6 as well. I have actually had a couple users change their emails to harass people on my site, so it would be awesome to fix this.
Comment #94
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedRe #92: Indeed. The policy is to first fix this in D8 and then backport it to previous versions.
Re #93: With a follow-up I meant a new issue to tackle migration issue that might occur due to this fix.
Comment #95
AnybodyPatch works great for me.
+1 and absolutely important to also fix in D7 :)
Comment #96
legolasboshouldn't "one day" be "24 hours" to be more precise?
Also shouldn't the mail_change_notification contain the new e-mail address? If it was a legitimate change, the user already knows their own e-mail address. Else they have some clue as to who tried to change their email address.
How about:
Require email verification when a user changes their email address
Let's just do \Drupal::currentUser()->hasPermission();
Edit the e-mail message sent to a user's old e-mail address when the e-mail address is changed.
Edit the e-mail message sent to a user's new e-mail address when the e-mail address is changed.
Returns
expired -- your
Let's inform the user a little more about the actual problem.
Your e-mail address has been changed to %email.
The user's email address.
Comment #97
StryKaizerUpdated patch from #90 to current head.
Remarks from #96:
1. To stay consistent with the other user emails, (being cancel_confirm and password_reset), I thinks its better to keep “one day” instead of 24 hours.
2. Ok
3. Ok
4. Changed it to "Edit the email messages sent to users' old email address when the email address is changed.” to stay consistent with the other descriptions.
5. Changed to "Edit the email messages sent to users' new email address when the email address is changed.” for consistency.
6. Ok
7. Ok
8. Ok
9. Ok
10. Ok
I added an extra warning message when an email is changed with validation, to ensure the user knows an action is required.
Bugs which still needs to be addressed:
- Provide a token to show the new email address in the mail_change_notification email. Since we are not storing the old emailadres at this moment, there's no quick fix
- When a user is not logged in while clicking the email validation change link, an error occurs.
Comment #98
StryKaizerCorrect patch now, remarks see #97
Comment #99
borisson_Setting to needs review so the testbot can have a look at #98.
Comment #101
StryKaizerBugs which still needs to be addressed:
- Provide a token to show the new email address in the mail_change_notification email. Since we are not storing the old emailadres at this moment, there's no quick fix
- When a user is not logged in while clicking the email validation change link, an error occurs.
Comment #102
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedOne thing to be careful of here is that if Drupal actually verified the email addresses of all accounts, the privacy problems discussed in the issues I'm linking to would become significantly more severe.
Comment #103
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedI rerolled the patch. It is my first rerroll. So, if I did something wrong, please let me know. I will try to fix asap. Thank you!
Comment #104
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #105
cilefen CreditAttribution: cilefen commented@LOBsTerr Well done! Achievement unlocked! I noticed only one thing, looking at this side-by-side with the prior patch:
This should not be in the patch (should have taken HEAD's version). Generally, when resolving conflicts, HEAD is "correct" and you have to carefully consider what the prior patch was doing in that place. Dreditor helps. You can easily sort out the thing above with a commit on your reroll branch then submit a new patch.
Comment #106
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@cilefen - Thank you for information. I will fix it.
Comment #107
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@cilefen - I have created an interdiff. I hope now it will be ok! I'm sorry the next time I will be more careful!
Comment #108
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #110
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedRenamed interdiff to be ignored by the Testbot
Comment #111
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #112
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #114
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedRename interdiff one more. I missed "do-not-test" part :(
Comment #115
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #117
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #118
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #119
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #121
valthebaldI dare mark it RTBC
Comment #123
cilefen CreditAttribution: cilefen commentedThere is a random, unrelated newline here.
Comment #124
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@cilefen - You are right. Nice catch. Missed. Fixed it in a new patch
Comment #125
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #127
cilefen CreditAttribution: cilefen commentedA hook_update_N() is needed to set the body and subject for these message for existing sites.
There does not seem to be a form element for a site admin to set these booleans, as there is with the "Account blocked" notification, for example.
Comment #128
k4v CreditAttribution: k4v as a volunteer commentedWorking on this @Drupalcamp Vienna
Comment #129
k4v CreditAttribution: k4v as a volunteer commentedFixed a few minor things, works for me now. I also added the update hook.
Comment #130
k4v CreditAttribution: k4v as a volunteer commentedComment #132
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedFor me everything looks good: Updated logic to get user, to set an e-mail and update it only when it is updated, return result of redirect function.
Checked the update hook everything was set properly.
Comment #134
catchSince this contains form and string changes, moving to 8.1.x
I didn't do a full review, so leaving at RTBC isn't an indication either way of whether I think it's actually RTBC.
Comment #135
claudiu.cristeaWe currently use "E-mail", not "e-mail", "email", or "mail", when referring the E-mail in human-readable strings.
Move
$old_email
before if() and use it in if() condition. Also consider to name it $old_mail, $new_mail (we typically use "mail" as machine-readable name).Also you can replace
\Drupal::currentUser()
with$this->currentUser()
For all
t(...)
inside this function consider replacing them with$this->t(...)
.Maybe "Provides a callback for user E-mail change page"?
Missed the '$' prefix.
Use
$this->t(...)
instead oft()
for all translated strings inside this function.You don't need
$user
variable. Just use$this->currentUser
or, better,$this->currentUser()
.Lines should wrap at 80 characters.
We are not using directly this variable. Use the
REQUEST_TIME
constant instead.Space after "if".
We are using the compact form "elseif" w/o space between.
"Updates", maybe?
This I don't like. Is it possible to read to parse directly the YAML file and get the values instead hardcoding them?
Use modern
[]
instead ofarray()
.I wonder if we cannot place this as static method in an external class. It's used rarely, why loading user.module?
Comment #136
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #137
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedNope, "email" is correct - see https://www.drupal.org/drupalorg/style-guide/content#relatedwords
Comment #138
claudiu.cristeaOuch, sorry. I missed that.
Comment #139
k4v CreditAttribution: k4v as a volunteer commentedThe test could actually call the URL from the notification mail and make shure the email address is the new one afterwards.
Comment #140
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #141
claudiu.cristeaPer #137, let's revert to "email", "Email" form. Sorry again for confusing.
Maybe swap "change" with "email"? For me "Returns the user email change page" sounds better, but I'm not a native English speaker.
Now, let's drop
$current
and use directly the constant.s/t()/$this->t().
Also, use
[]
instead ofarray()
.Drop $timestamp, use REQUEST_TIME constant.
Comment #142
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #143
claudiu.cristeaI wonder if we cannot create a new Drupal\user\Controller\ChangeEmailController. Then we put there the changeEmail() (maybe as changeEmailPage()?) page and also we move there the user_change_mail_url() as a static method called changeEmailUrl(). I don't like adding new procedural functions to *.module files that are parsed on every request.
EDIT: Also we can open a follow-up to do the same with reset password, in the future.
Comment #144
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #146
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #148
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #149
claudiu.cristeaBecause the interdiff is against #140, I cannot see the cause of test failures in #146, #148. And I'm so curious :)
Comment #150
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@claudiu.cristea.
1) in comment 144 - As, you asked I change the new_email to new_mail
2) in comment 146. I used the wrong namespace
instead of
If you need I can create interdiffs. Was a little bit in hurry didn't check it properly :(
Comment #151
claudiu.cristeaAh, OK. Thank you for explanations. Minor changes. However, It's a recommended practice to create interdiffs on each patch agains the last patch, so that the reviewer understands exactly what was changed since the last patch.
Some more reviews:
s/users'/user's
ChangeEmailController?
Missed:
Entity manager is deprecated but the parent ControllerBase already provides
$this->entityTypeManager()
. Let's use that. Also consider to a add a type hint comment above, telling to IDE that $account is UserInterface.The wrapping is still incorrect. "which" can go up 1 line. Also words from the 3rd line can go up.
Forgot to convert array() to [] :)
We need to test this.
This is wrong. It should be type-hinted to \Drupal\user\UserInterface, not object.
This line indent is wrong "URLs" should left-align with "langcode".
Missed the type-hint.
$account & $options must be type-hinted.
Let's use the modern [] instead of array(). Also we have the service already in, let's use it: replace \Drupal::languageManager() with $this->languageManager().
\Drupal::url() is deprecated. Use
Url::fromRoute()
instead.All changes to UserController are unrelated. Remove them from this patch.
Comment #152
claudiu.cristeaComment #153
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedI'm sorry, I didn't about the patches. I thought, the last successful patch must have been used.
Comment #154
claudiu.cristeaNo problem :)
And more...
There's no $account->name but only $account->getAccountName(). Also, there's no
$user
object.array() -> [].
We need to test each of these cases.
Comment #155
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@claudiu.cristea
ChangeEmailController? - what should be done here ?
Comment #156
claudiu.cristeaThat is about the line before
You forgot the class name when copied the file :)
Comment #157
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@claudiu.cristea Thanks for review, back to work :)
Comment #158
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedI have added simple tests to test the change email functionality.
Comment #159
claudiu.cristeaThank you for the great work.
This method takes no argument. Just remove $form_state. Also it would be nice to type-hint the $account var with /** @var ... */ above, like you did in other place.
I would wrap
$old_mail !== $new_mail
in parenthesis.Isn't it "...sent to user's old email..."? I'm not a native English speaker, I might be wrong.
s/users'/user's
Let's change it to "The ID of the user requesting reset."
I don't like that we are playing here with the $account object by changing its email 2 times. Also if the 1st notification fail, the $account will have the $new_mail as email and that's a lie. I know that this seems to not affect the current flow, but we cannot predict how contrib or custom modules will add their submit callbacks and they will not find there the correct email. I would prefer to use a fake cloned account just to send that email.
Maybe $cloned_account is not a very beautiful name?
Maybe "Your new email address..."?
Let's use !$this->currentUser()->isAnonymous() instead of the 1st condition. Also let's wrap the 2nd condition in parenthesis.
Well, this is very problematic. I don't think we can rely on user_pass_rehash() to check the integrity of the $hash. I see that hash generated by that function vary on: $timestamp, $account->getLastLoginTime(), $account->id(), $account->getEmail() and $account->getPassword(). Let's take this scenario:
I see this solved in other way. Let's add a new public method in this controller called changeEmailAccess. Also add the access check to the route. Add a new protected static method in the controller called getHash(). We will use that method to generate the hash in both places — url and validation. gatHash() might be similar with user_pass_rehash() but depend only on non-alterable variables: $timestamp, $account->id(), $account->getEmail(), $account->getCreatedTime(), $account->getInitialEmail(). In this way we can remove this check because the URL will not be accessible for wrong hashes.
The 2nd condition is redundant, we tested that in the wrapping if (). I don't see the reason of the first condition. I think we can replace the elseif with else.
Well, you added the ->isActive() condition. I think is correct, but we should reflect this also in this comment.
Move the namespaced name to
user Drupal\user\UserInterface;
. use UserInterface instead of User for type-hinting.Why we need standard? If 'user' module is not installed just add it to $modules. But I think is installed by default.
Remove it.
In tests, the container is available $this->container->get('token'); You can also add a type-hint declaration for $token_service.
Why that permission?
Also I see that we are doing this in each test. Then we need to reuse that by implementing setUp() method. We declare a
protected $account;
in class and in setUp() we assign the account and login. Then in each test, we only use $this->account if case.No. We use the Drupal config system to retrieve configurations. YAML files are only a way to import configs into Drupal when a module gets enabled.
$mail_settings = $this->config('user.mail');
"... the user has sent..." ?
and generally we use a comment for assertions like:
"Check that the user...." — let's standardise all assertion comments like that.
We can omit assertion messages.
Missing @param and @return.
protected? Are we using it outside?
"Generates a random..."?
protected. Are we using the function outside the test?
Maybe it would be nicer to have only lowercase email user. Then Unicode::strtolower($this->randomMachineName()).
If a parameter is optional you should tell that and also provide the default. See https://www.drupal.org/coding-standards/docs#param
Based on our coding standards we need to use lowerCamel naming for methods. See https://www.drupal.org/node/608152#naming. So, getChangeEmailUrl(). Also use
use Drupal\user\UserInterface;
to avoid the full qualified name in method signature.Hmmm. Why not using the already existing controller url generator?
We need to test that. See how other modules are testing upgrades.
I need to review more on tests but it's difficult right now because we need to implement first the changes.
Comment #160
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@claudiu.cristea - as usual, thank you very much for you great review!
Comment #161
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@claudiu.cristea - It is not necessary, but if you don't mind I have fixed the codding standards for AccountForm.php, AccountSettingsForm.php, because there are only minors changes.
I forgot to add the test case for update hook :( I will do it tomorrow.
Comment #162
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@claudiu.cristea Unfortunately, I cannot find examples in the core modules. I'm not sure how to properly check the update hook, I just need to check the current configuration with values in YML files?
Comment #163
claudiu.cristeaThis is only a brief, bird-eye, formal review. More to come. I didn't had time to review the test scenarios and to think more on hashing and security.
An update test: \Drupal\field\Tests\Update\FieldUpdateTest. Create a similar one in core/modules/user/src/Tests/Update
These changes are unrelated. Please remove them.
I think $account_copy sounds better. What you think?
Don't send NULL, that argument always defaults to NULL.
Now that we have a dedicated controller only for E-mail change, we can simplify the name changing. Let's:
We should add other per-user variables? I mentioned earlier the getCreatedTime() and getInitEmail().
I'm not sure. What if the user changes its pass after issuing the E-mail change but before verifying? Anyway this is open for discussion. I don't see very clear right now how to deal with this. I agree that the passwords hash is a good value of randomness. Leave it as it is for now.
Remove extra-line.
Comment #164
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@claudiu.cristea - I took an example from other core modules. They use this naming $account_cloned and not $account_copy. About the first section, I know it is not related to this task, but I just wanted to fix them, because these changes are minor and it is easy to figure out what has been changed. If you insist I will remove them and will not touch other parts of the code anymore.
Comment #165
claudiu.cristeaOK, let's go in this way.
Reviewing a patch is not easy. Unrelated changes make patches hard to review. also, each issue should fix only one thing. Yes, please remove them from patch.
EDIT: There are exceptions. For example you edit a line in the scope of the issue but the above comment, referring to the line you've just edited, doesn't respect the coding standards. Of course you can correct that comment. But it has to be in the area you already touched. But here is not the case.
Comment #166
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedI have created this test, but I cannot run it. There are some issues with update tests. I tested other simple tests which use UpdatePathTestBase. All their breaks with the same errors. So, I will try to find an issue for that and upload this test later.
Comment #168
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedComment #170
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@claudiu.cristea - It is strange, I tried to run UserChangeEmailTest before I uploaded the patch.
51 passes, 0 fails, 0 exceptions, 14 debug messages
Could it be some glitches with Drupal testing system?
Comment #171
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAfaik, it should work. I ran the test several times locally and it always passes.
I did fix a typo while I was waiting for the test to run.
Comment #173
claudiu.cristea@LOBsTerr, probably the HEAD is ahead and you need to update your codebase from repo.
Comment #174
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commentedI always try to pull the latest changes. Anyway tried to pull changes again and added some minor fixes.
Comment #175
claudiu.cristeaGreen, thank you. Then let's add the Update test and I will review the whole patch. Also we need to figure out what to make with the hash.
Comment #176
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@claudiu.cristea - I think it is ok to remove password as you suggested above, because we enough unique information to generate a secure hash.
Comment #177
LOBsTerr CreditAttribution: LOBsTerr as a volunteer commented@claudiu.cristea - I have been struggled with UpdateTest and I cannot make it work. Actually, I cannot make work any update test in the core. Usually, I have about 150 fails, but it happens UpdatePathTestBase. It is strange, because the same tests obviously run without any issue on the Drupal.org. Can you try to run any UpdateTest locally? Maybe I need to install something to make it work. I can send the patch with my UserUpdateTest class, but I cannot test it locally:(
As you see it is quite simple, but it fails, because something breaks in $this->runUpdates();
Thank you for helping me.
Comment #178
claudiu.cristeaI don't see the test in the patch. Please add the patch with the test to see the failures.
Comment #180
claudiu.cristeaHere we go.
Comment #181
attiks CreditAttribution: attiks at Attiks commentedCode is looking good, same phrasing sounds a bit weird to me, so best that somebody else can review it as well
Didn't had a chance to test the patch, will try later today
Strange sentence
'If missed' => 'When not provided'
Comment #182
claudiu.cristeaThank you @attiks.
Comment #184
swentel CreditAttribution: swentel commentedThe logic seems off here. If you don't enable mail change verification, you will still see the message and your old mail will be stored, no ?
'for another account' instead of 'for other account'
Comment #185
claudiu.cristea@swentel, thank you for looking at this issue.
#184.1: (EDIT)
mail_change_verification is not a flag but a pair of E-mail subject + body texts. So_user_mail_notify('mail_change_verification', $account_cloned)
is sending out an E-mail. The fact that the mail was sent cannot be guaranteed. The logic there would beBut note that we store the old E-mail because we want the account to be saved with the old E-mail. We don't allow the new E-mail to be stored till the user will confirm the new E-mail. We effectively change the E-mail when the user clicks the verification link, in.\Drupal\user\Controller\MailChangeController::page()
#184.2: Fixed Thanks.
I also dropped
MailChangeController::getHash()
because a more simple way was to changeuser_pass_rehash()
to accept a new optional parameter.Comment #186
claudiu.cristea@swentel, you are right in #184. Forgot that we have also checkboxes for that notification.
Comment #187
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedLooks good to me now imo. Wondering how to trigger usability and security reviews on this one .. maybe just put to RTBC, that will at least set more eyes on this one.
Comment #190
claudiu.cristea@swentel, yes probably it's good to have it RTBC. This issue is 10 years old and has 72 followers :)
Fixed the test and converted the test to BrowserTestBase test. Also I added a new test
testMailChangeNoVerification()
that tests the case when mail change validation is switched off. Fixed also other nits.Comment #191
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedNit in the test.
Hmm, should be something like '.. and verify that the no email was sent'.
Moving to RTBC, let's see what the core committers think of this.
Comment #192
claudiu.cristeaFixed the comments.
Comment #193
claudiu.cristeaChanged the status by mistake
Comment #194
xjmComment #195
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedSo I've been thinking more about this update which enables the email change verification. While this update was handy in my case (because we require it for a project), I'm not sure if we should really run this by default when users are going to upgrade.
It's an extremely useful feature of course, which on new installations warrant to be enabled by default, but I'm not sure about upgrades, because maybe users don't necessarily see this as a bug ?
Not setting to needs work for this, because I'm fine if it gets committed as is as well, just wanted to express my thoughts.
Comment #196
dawehnerPersonally I'm a huge fan of changing default values, but not update to those default values, but rather keep existing behaviours. You know, it just avoids unnecessary problems.
Comment #197
claudiu.cristeaI agree with #195 and #196. More clarifications:
I think this should remain as it is, so new site installations will get advantage of this fix.
Here we need these values to FALSE, to disable this feature for existing sites.
This should remain even the feature will be disabled for the existing sites because here we set the default mail subject & body. Will not have effect but will be set in the case the admin manually enable them.
Are we OK with this plan?
Comment #198
claudiu.cristea@swentel,
Implemented #195, #196, #197. I'm setting the issue to "Needs review" to get your feedback.
Comment #199
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedSounds perfect to me.
Extreme nit, should be FALSE :)
Moving back to RTBC, if anyone wants to fix that nit, leave the status on RTBC :)
Comment #200
claudiu.cristeaOh, yeah. Missed that :)
Comment #201
alexpottInitially looking at this change I wondered if this was a security issue - ie it is possible to generate a hash without an email address - but looking at the code it appears not.
Comment #202
catchThis should probably be 'updated' rather than new. The e-mail address is new to the site, but probably not to the user.
Is there a UI for this? If not why not put it in $settings?
Nit, should be 'gets expired' or 'is expired'.
Should 'registered a new login' bit 'logged in'?
if the user is user is
Shouldn't this duplicate the value from the configuration instead of reading it in? Otherwise the config might change again, and this update will have different results depending on which version is updated to.
Also this is still tagged usability review. I didn't review the whole issue to see whether that had been done, would be good to update the issue summary and remove the tag if it has, or ask someone to review it if not. Tagging for product review as well.
Comment #203
claudiu.cristeaFixed #202.
Also removed
t()
from test according to https://www.drupal.org/node/2783189#t and replaced the deprecated REQUEST_TIME with the new service.Comment #204
claudiu.cristeaRemoved unused use statement
Comment #205
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedThis isn't really consistent. Password reset timeout doesn't have a UI either and it's in config. So I'd leave mail_change_timeout in config as well and create a follow up to expose both password and mail change timeout in the UI (if wanted).
Looks good to me other than that.
Comment #206
claudiu.cristeaReverted
mail_change_timeout
to a config in order to keep the symmetry withpassword_reset_timeout
. Opened #2823877: [PP-1] Expose user.settings:password_reset_timeout and mail_change_timeout in UI to provide UI for both, as a follow-up.Comment #207
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedOk, looks good to me. Let's give committers an eye on this again.
Comment #208
catchre #205 I think really the password reset timeout should move to settings as well - this looks like something that got converted from variables to config all in one go.
Comment #209
claudiu.cristea@catch, If somebody has overwritten that value in a custom module he will lose the setting as we cannot provide an update path. Or I'm missing something.
EDIT: Or a custom/contrib module just uses the config for some reasons. After conversion, this will crash:
\Drupal::config('user.settings')->get('password_reset_timeout')
Comment #210
alexpott.
Lack of UI does not mean something should be in settings. I think this belongs in config because it is precisely the sort of thing you want the same in all environments and to be able to be able to deploy via a config change.
Also this still needs a product manager review. It definitely needs a change record too.
Doing this in the submitForm is wrong - it needs to be done in User::postSave() so the changes via the API have the same logic.
Not used.
Will this actually work? Ah yes the mail is only updated after the verification is successful.
Given that everything redirects I don't think we have the issue with hash exposure that we had with one time links. Nice! However I think there should be a similar comment in the function docblock...
Are we sure about this?
Comment #211
alexpottRe #210.2 It actually might need to be done in preSave actually because you're resetting the value back to the old value.
Comment #212
claudiu.cristea@alexpott, sending Email notifications from preSave()? Brrrr
Comment #213
claudiu.cristeaAs an effect of #210.2, #211, we need to test the REST part too. Test in \Drupal\user\Tests\RestRegisterUserTest
Comment #214
k4v CreditAttribution: k4v as a volunteer commentedI just took a quick look at User::preSave, where should we get the old value from? The new address is already set at this point.
Comment #215
claudiu.cristea@k4v, I already started to work on this. Assigned.
Comment #216
webchickRemoved the "8.3.0 release notes" for now, since this isn't actually committed to 8.3.0.
Comment #218
rootworkUnassigned given the length of time since the last code contribution. @claudiu.cristea if you have updates, please let us know!
Comment #219
manuel.adanI've recently publish the email_confirmer module, that makes the hard work for any other module that needs email address confirmation. It provides a service for email confirmation as well as a content entity type for the confirmations control. This approach might be convenient to be in core, since confirming email addresses is a basic feature for any website that captures emails in some way.
Comment #220
shrop CreditAttribution: shrop at Mediacurrent commentedComment #222
webchickSounds like this more needs usability review than product manager review; changing the tag so we can review on a future UX meeting with a larger crew.
Comment #223
manuel.adanFor those who need email change confirmation in Drupal 8, email_confirmer now provides it. It has been added as a functionality of a new user-related submodule.
Comment #226
claudiu.cristeaAssigning to fix #210.
Comment #227
claudiu.cristeaComment #228
claudiu.cristeaStraight reroll of #206. Plus a WebTestBase was moved as Functional. More fixes to follow...
Comment #229
claudiu.cristeaComment #231
claudiu.cristeaFixing test failures. #210 not yet addressed.
Comment #233
claudiu.cristeaMore failures to fix.
Comment #234
claudiu.cristeaFixed #210.3, 5, 6.
Now, regarding #210.2:
@alexpott,
After investigating the request from #210.2 and trying to implement that, I found that moving the verification & notification logic from the form submit into the API (
User::preSave()
) will definitely not work. Here are the reasons:User::load($uid)->setEmail('new@example.com')->save()
. This is because inUser::preSave()
, the old E-mail will be set back, waiting for the user verification.User::load($uid)->setEmail('new@example.com')->save()
. I read this snippet straight: after this, the account should have new@example.com as E-mail, not the old one. And the API should not depend on user interaction.User::preSave()
the E-mail change will never occur because when the user clicks the E-mail link to verify the new E-mail, this code is invoked$user->setEmail($new_mail)->setLastLoginTime($request_time)->save();
, in MailChangeController::page() and, again, User::preSave() is called, which is switching back to the old E-mail. Of, course this could be handled in some way, but it would not be a nice solution.\Drupal\user\RegisterForm::save()
). This totally makes sense because it allows you to programatically create an unblocked user in this wayUser::create([...])->activate()->save()
. If the registration E-mail verification logic would have been implemented inUser::preSave()
(or somewhere in the save API chain), that snippet would not have been created an active user but a blocked one, contrary on how this code reads.Proposal
entity:user
by extending\Drupal\rest\Plugin\rest\resource\EntityResource::patch()
and implementing the same logic. But, I think, due to the fact that this patch is already complex, we should do it in a follow-up.Comment #235
mcdruidComment #236
jonathanshawWe can't RTBC as it still needs a change record
Comment #237
claudiu.cristeaDraft change record added https://www.drupal.org/node/2994918
Comment #238
mcdruidMade a couple of tweaks to the CR: https://www.drupal.org/node/2994918/revisions/view/11089451/11089464
Comment #239
jonathanshawRTBC per #199 as #210 is addressed.
Let's see if comitter thinks security or usability reviews are still needed.
Comment #241
claudiu.cristeaLuckily, the last failure from #234 has uncovered a bug in the patch. True, that it's a very, very edge case. I was running locally the
UserMailChangeTest::testMailChange()
test and I managed to get a failure rate of 50% (!!!) The problem occurs when testing the verification link expiry when accessing for the 2nd time the link. If the test runs the second request within 1 second (in the same server request timestamp) then the test fails because the link is not considered expired. I cannot explain why the high failure rate didn't make old patches to fail, I only guess that the infrastructure is now improved, we run now tests faster, on PHP7, and that made possible the edge case (making 2 GET requests within the same second).The fix is simple. When comparing the link timestamp with the server request time, use the server request time as float, which adds milliseconds precision and is always greater than the timestamp send via the link.
Comment #242
claudiu.cristeaAn improved version that honours the
setLastLoginTime()
contract. Also has small improvements on docs.Comment #243
jonathanshawComment #245
claudiu.cristeaWeird, the patch never fails when on posting but only when is RTBC :)
Comment #246
claudiu.cristeaTesting this is tricky. What exactly is happening with the random failures? Follow
UserMailChangeTest
andMailChangeController
code to understandUserMailChangeTest
, line 49.UserMailChangeTest
, line 67.UserMailChangeTest
, line 90.UserMailChangeTest
, line 94.MailChangeController
, line 82, to understand why.This cannot happen in the real world for a human. So, I added an one second sleep before the user changes its account E-mail just to ensure that the test is acting as a human. Note that if the user last login time were stored as a timestamp with millisecond precision this trick wouldn't had been needed.
Comment #247
jonathanshawSeems like you need to walk back #241/242, or at least modify the comments? At the moment it seems like we've got different bits of code claiming they fix the same problem.
Comment #248
claudiu.cristeaIndeed. And #246 makes #241/#242 undeeded. I reverted #241/#242 and improved a little bit the docs in test. Unassigning.
Comment #249
jonathanshawComment #251
jonathanshawBot results show no failure I can see.
Comment #252
claudiu.cristeaLuckily, we've been tested this patch also in production and we managed to hit an edge case: Drupal accepts accounts without E-mail when the account is created by an admin. When such a user is edited, on save, will throw an exception when building the user tokens.
We need also to prevent sending a notification to the old address, if the old address is empty. The other way around, changing an non-empty E-mail to an empty one, is not allowed, so we don't need to deal with that case. Provide test for this edge case.
Assigning to work on it.
Comment #253
sandzel CreditAttribution: sandzel commentedThat's great idea to have email verification functionality,
I have a suggestion for the step when user already passed all validation and email is changed. Will be great to have a hook that permits developers apply some custom functionality. For example add/remove a role, permissions, notify admins, etc.
I updated the patch 85494-248.patch and I added the hook_user_email_change($account).
By the way, I noticed that when user is not logged on the site and click on mail-change-url the email is changed but user is not logged automatically.
Is it an intentional behavior?
Comment #254
sandzel CreditAttribution: sandzel commentedComment #255
jonathanshawRe #253 I suggest opening a follow-up issue for that and postpone it on this issue, it doesn't need to block this issue.
Comment #256
claudiu.cristeaTest proving the bug described in #252.
Comment #257
claudiu.cristeaFix the bug. The interdiff is against #248.
@sandzel, I don't think there's need for such a new hook. This can be easily handled in a normal hook (presave, update) by checking the mail against the original E-mail. However, while this is debatable, please open a followup, as @jonathanshaw suggested, if you still think that core needs such an API addition.
Comment #259
jonathanshawComment #260
claudiu.cristeaUnassigning
Comment #262
AaronMcHaleThanks for all the work on this, looking forward to seeing it in core :)
Also file clean-up, cause I'm OCD like that
Comment #263
AaronMcHaleComment #264
claudiu.cristeaLet's take an extra precaution for the case when 3rd party code computes the tokens and the account has no E-mail.
Comment #265
andypostComment #266
AaronMcHaleFound another one, thanks to the issue @andypost linked
Comment #267
jonathanshawComment #268
larowlanPinged security team and UX team for reviews here
Comment #269
benjifisherI plan to bring this up at the weekly Usability meeting in about 4 hours (3:30 PM EST). Join us on the #ux channel in the Drupal Slack account if you would like to be part of the discussion.
Comment #270
worldlinemine CreditAttribution: worldlinemine at Acquia commentedAt the Drupal Usability meeting we reviewed the suggested patches (participating Benji Fisher and Thomas Howell).
Change examined at "Configuration" -> "People" -> "Account email changing"
Note to testers: must check field to enable form for subject and body of potential emails.
Question:
Follow up questions:
Testing patch results:
Inconclusive technically because we were unable to test locally with outgoing emails. However, the page updated by patch matches the screenshots provided in this issue which is sufficient for the usability review.
Conclusion: We concur that this change is an excellent idea though if possible the follow up questions should be addressed. This was a usability review so the lack of confirmation on outgoing emails by our team should not be viewed as a blocker.
Comment #271
worldlinemine CreditAttribution: worldlinemine at Acquia commentedComment #272
benjifisherAs @worldlinemine said, we discussed this at the Drupal Usability Meeting 2018-11-13 (recording on Youtube).
Since then, I have made two changes before re-testing:
I notice that the e-mail verification is not sent when I change the e-mail address of user 1 (admin). From the comments above, I guess that any user with the "administer users" permission can change an e-mail address without triggering the e-mail. I wonder if there should be an exception to the exception: when an admin user changes their own password, maybe we should still use the new system.
I added a note to the issue summary about such admin users. I also added another screenshot and the text of a confirmation e-mail.
Comment #273
claudiu.cristea@benjifisher, @worldlinemine, thank you for the UX review.
They can always set back or enter a correct E-mail address if they used a wrong one. No need for verification.
Comment #274
benjifisherAccording to Murphy's Law, if I ever make a mistake changing my e-mail address, that will be the time that my password manager crashes. Then I will not be able to log in and fix it.
Comment #275
plachPer #268 tagging as needing UX review as well (the IS has all the relevant info).
Comment #276
benjifisherThe usability review was in #270. I gave a link to the recording of the meeting in #272.
Comment #277
plachOops, missed that, sorry for the noise.
Comment #278
alexpottThere's been a process change whilst this is RTBC - we need to populate the issue summary with a Release note section. This is important for this issue as there is change record and this does affect site operation.
Not changing the status of the issue - going to review soon.
Comment #279
alexpottWe've done quite a bit of work recently to make it easier to have users with a permission other than
administer users
be able to administer users because that permission allows you to change everything. See #2854252: User forms broken for admin without 'administer users' I think we need to account for that somehow.We need to enforce the logic of the form on the config level. I.e. if
user.settings:notify.mail_change_verification
is set to FALSE there is no world whereuser.settings:notify.mail_change_notifcation
. I.e. we need a user version of \Drupal\system\SystemConfigSubscriber to enforce this logic.It might not be you doing the updating. See comment above. I think we should consider this like admin approval - ie. we should only do this if the user is themselves.
I think it is worth commenting that we expect the account to have the new email at this point.
This functionality appears to be only for testing. Given this method is being used as API I'm not sure about this. I think tests can generate their own URLs without this extra complexity.
Nice that this test is here. It would be great if there is also a test to ensure that if a user has a correct hash - but hacks the email address on the URL it still does not work. Afaics it won't but this all relies on the User object being stateful and passed to functions in the expected state - ie. having the new email but not being saved.
$data .= $mail ?: $account->getEmail()
will suffice. The additional local variable makes me wonder what it is for.Comment #280
claudiu.cristeaAssigning to fix requests from #279.
Comment #281
claudiu.cristea@alexpott, I need some clarifications on your request from #279:
#279.1:
By evaluating
$account->access('update')
and checking if the account is the current user we cannot determine if the user is a sub-admin editing its account. In that case we don't want to verify the email. I see no way to avoid the verification in such case by not checking theadminister users
permission. This is because$account->access('update')
returns TRUE also when a regular user is updating his own account. Or probably I don't get correctly your request, which is a little bit vague.#279.2:
This is tricky. Unfortunately there's no way to alter the config (
user.settings
) being saved, The actualConfigEvents::SAVE
event is fired after the config has been saved to the storage. For this reason the subscriber must save again the config that was just saved and this leads to a recursion problem (as the subscriber is recursively called). Of course, we can hack around by using a recurse flag. But that would look so ugly. I created an issue to add a pre-save event just before the config is stored in the backend: #3022725: Add ConfigEvents::PRE_SAVE event.There is also a UX/UI aspect. We should do the same in UI by using FAPI
#states
so that the user visually understands this dependency. I.e. in UI, when he unchecks thenotify.mail_change_verification
, thenotify.mail_change_notifcation
should be automatically unchecked. But, unfortunately, #states are now buggy when it comes to, at least, the following aspects:'unchecked' => ['input[name="mail_change_verification[enabled]"]' => ['unchecked' => TRUE]]
is not the same as'checked' => ['input[name="mail_change_verification[enabled]"]' => ['checked' => TRUE]]
. But the actual implementation works in this way. Even I use 'unchecked' as host/remote condition is triggered also on check.notify.mail_change_verification
is unchecked, thenotify.mail_change_notifcation
gets unchecked but it doesn't propagate the event by closing the details section (see the other implemented #states).#279.3:
There's no way to land there w/o being a user editing its own account. We don't trigger verification for (sub)admins editing the account of other user. At least this is the initial plan. Am I missing something?
EDIT (added later):
#279.6:
Sorry, I don't understand the scenario you are proposing.
Comment #282
claudiu.cristeaI'm posting also the incomplete progress I made while analysing #279. It covers #279.4, 5, 7 and a try for #279.2.
Comment #284
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedBackground: I was the main author of #2854252: User forms broken for admin without 'administer users'
Re #279.1: We could write the test something like this
Or if we want admins to have to verify their own change (as #274) then instead
#279.3:
I think @AlexPott is saying that in the previous patch #264 it could be another "sub-admin user" as per comment #279.1. However you will be right it must be own account if you change the code as per #279.1. Hence solve #279.1 and this problem goes away too.
#279.6 I would interpret this comment as a request to test this scenario:
Comment #285
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedComment #287
claudiu.cristeaReroll for 8.8.x.
Comment #288
AaronMcHaleThe backport to D7 tag doesn't really feel relevant anymore.
Could we also get an issue summary update listing the remaining tasks to move this from NW to NR/RTBC?
Comment #289
AkashKumar07 CreditAttribution: AkashKumar07 at OpenSense Labs commented1. The remaining failure is now due to a $defaultTheme property required on your test class.
Example :-
/**
* {@inheritdoc}
*/
protected $defaultTheme = 'stark';
2. Drupal\Component\Utility\Crypt::hashEquals() is deprecated in drupal:8.8.0.
Use PHP's built-in hash_equals() function instead.
Example :-
hashEquals($hash, user_pass_rehash($user, $timestamp, $new_mail)) instead of
use Drupal\Component\Utility\Crypt;
Crypt::hashEquals($hash, user_pass_rehash($user, $timestamp, $new_mail))
Hope this will help.
Thanks
Comment #290
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedI updated the IS as far as I can understand it based on reading the recent comments.
#289 accidentally reverted the tag change of #287 - fixing that.
Comment #291
AaronMcHaleGreat, sounds like we're near the home stretch for this issue, yay!
Tagging this for usability review, looking back it looks like one might have been done a few years ago but it would be good to get an up to date one, I'll demo this at a future UX Team meeting.
Comment #292
rwohlebIdeally we'd have tokens available in the email templates where we have access to both the old and new email addresses. Right now [user:email] is either the old or new email address depending on if it's the notification or validation template. This isn't exactly intuitive nor explained in the help text.
The token module adds support for [user:original], but unfortunately the source of that ($entity->original) is only set in EntityStorageBase::doPreSave(). We could set $account->original in AccountForm::submitForm() which would be nice when that module is enabled, but we obviously aren't going to add that as a dependency. That seems to leave us with the current situation. I suppose we could at least set $account_cloned->original so that'd be available in the verification template.
Thoughts?
Comment #294
pfrenssenRerolled for 8.9.x and 9.1.x.
Comment #295
jonathanshaw#289 explains clearly how to fix the remaining test failures.
Comment #296
Lal_interdiff wrt #294 added the changes mentioned in #289
Comment #297
jonathanshawThanks @Lal_
Comment #298
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedFound one more deprecation still exits in #296 (Declaring ::setUp without a void return typehint) that I have updated here in the patch, please review.
Comment #301
narendra.rajwar27Comment #302
narendra.rajwar27Comment #303
narendra.rajwar27Comment #304
AaronMcHaleI just want to be sure that the IS is up to date, specifically the remaining tasks? As they seem to reference comment numbers which might be old now, so I suspect some of the remaining tasks are now done. If it is already up to date then please feel free to remove the tag.
Also, is this at the stage where we can do a usability review (as per the remaining tasks) or are there still likely to be UI/UX changes?
Thanks
Comment #305
jonathanshawNothing related to IS has happened since @AdamPS updated the IS 6 months ago; he's a careful guy so the IS is likely good. The more recent work fixed deprecations only.
The UI is final, the slight implementation question mark around #279.2 only involves a detail of the admin UI, is believed to be fixed as much as @claudiu and @Adam can see, and the end user UX is likely far more important to review anyway.
Comment #306
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThe tests now pass, good work, so I remove that item from the remaining tasks list. However unfortunately this has to go back to "needs work" as there are still some remaining tasks..
1.
This is definitely not fixed - the hard-coded permission is still present at line 87 of patch 301.
3.
From a quick glance I don't think this has been done either.
I propose that these need to be done before a security review. The usability review could perhaps go ahead as the UI seems fixed.
Comment #307
AaronMcHale@jonathanshaw @AdamPS
Great, thanks for confirming :)
Comment #308
sharma.amitt16 CreditAttribution: sharma.amitt16 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFix for test failure.
Comment #309
sharma.amitt16 CreditAttribution: sharma.amitt16 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedApologies, I checked the first page only. Please ignore the previous patch.
REverting back to needs work.
Comment #310
sharma.amitt16 CreditAttribution: sharma.amitt16 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedUploading a patch for change raised in #306.1
Working for #306.3, it will take some time. I will upload that after completion.
Please review.
Comment #311
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedGreat thanks @sharma.amitt16
Here are my comments based on looking at the interdiff.
1.The latest patch appears to have resorted the use statements at the top of AccountForm.php. I understand that this is the preference of the Drupal coding standards, but I don't think we should fix it here - it's not the job of this issue to make unrelated minor changes.
2. In #284 I offered two options for the code to use. You have chosen the one that means that admins have to verify their own change. Is this definitely the consensus of the community? - I'm not sure but I think maybe not - we should get confirmation from an appropriate expert. At very least it doesn't match the issue summary which says:
Comment #312
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedI thought some more and it seems there is a clear precedent that the Admin role is trustworthy and competent. The could easily remove the admin role from their account with no way to get back. They could delete all site content. They could assign the admin role to an untrusted user. Drupal makes no attempt to prevent any of these. Therefore I propose that a user with 'administer users' permission should not need to perform verification. Here is a new patch that does that and fixes the comments from #311.
The usability review was done in #270 and #272 so removing that tag.
Comment #313
jonathanshaw#312 makes sense to me.
NW per #306
Comment #314
benjifisherAfter the usability review in #270 and #272, a new one was requested in #291. So I am adding back the tag. Please make sure that the usability issues are clearly described in the issue summary.
I see that this issue still needs a release notes snippet. It may also need updates to the proposed resolution and remaining tasks, so I am adding a tag for that.
Comment #315
nimoatwoodwayTested patch #312 with Drupal 9.0.3.
Tested
- NOTIFICATION OF OLD EMAIL
- VERIFICATION OF NEW EMAIL
Works like a charm! Thanks for your efforts!
Comment #319
volegerReplaced deprecated method call in the test. Added review comments.
Comment #320
ptmkenny CreditAttribution: ptmkenny commentedComment #321
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks @voleger, the new patch looks good. However it is still "needs work" for adding a test requested by AlexPott - see the issue summary for an explanation. I updated the issue summary so I removed that tag.
Comment #325
darvanenWanted a rebased version but wasn't sure of the etiquette around rebasing a MR someone else created.... so I made a new one. Easier to pull my work into the first MR than to extract it in the case it was unwanted :)
Sorry if that was the wrong thing to do ¯\_(ツ)_/¯
New MR
Comment #326
jonathanshawThe remaining tasks from the IS are now complete.
A usability review has been conducted; a new one was offered but never happened so I'm unsure it's needed.
A security review would probably be a good idea.
I'm tagging this RTBC to get a committer verdict on what further reviews are needed.
Comment #327
AaronMcHaleThanks @jonathanshaw.
Yeah that was me and I completely forgot to follow-up on this, since its now RTBC I'm queuing this for review at #3218550: Drupal Usability Meeting 2021-06-18, thanks.
Comment #328
AaronMcHaleHi everyone.
At #3218550: Drupal Usability Meeting 2021-06-18 we reviewed this issue based on the screenshots in the issue summary.
First of all, we wanted to highlight how much effort and work has been put into this so far, thanks to everyone involved; This is a really important issue and getting this in will be a really valuable change for Drupal.
The bulk of our review focused on the settings screen, specifically the "old email" template and the tokens.
Relating to the "old email", the overall question that we had is, why are we sending the old email if it's not actionable?
We considered how sites like GitHub would deal with this and what their standard messaging might be; It was recommended that the "old email" template for we should provide some kind of call to action, in the event this change was made maliciously or in error.
For example: If the old email address is still correct, but someone managed to compromise the user's account, chances are they will finish the process (clicking confirm, updating account information, change password, etc.) before the user has a chance to undo it. At that point any cancel/one-time URLs we provide will have been invalidated, so contacting site administrator would be the right course of action here. If a site has more specific business logic they can of course then update the template to meet their needs, but we felt that starting from a sensible default is important for account security.
With that in mind, for the "old email" template, we recommend discouraging the use of cancel-url token. The one-time-login-url token being available in that email template was also a concern; If the old email was compromised, we definitely don't want to provide a one-time login URL in it.
To summarise that, more work is needed on the "old email" template, providing something actionable. We specifically recommended providing the site email address. For example, this could say "If you did not intend to make this change contact [site-email]."
We also looked more generally at the list of tokens: It was not clear what some of them are, for example the [user:cancel-url] token is ambiguous; We assumed it was the URL to cancel the change, but it coudd also be the actual account cancelation URL. We recomended "mail-cancel-url" as an alternative name. We also noted that the tokens could use better descriptions in general, we did note that this should be done in a follow-up as it is probably a wider issue covering the other templates on that form, and it is a minor thing so shouldn't block this getting in.
Following on from that, and as a more general recommendation, we wanted to highlight that there should be as little user information as possible in the "old email". So for example, in the event that the user was changing their email address because their old email address was compromised, we wouldn't want to give the malicious actor a direct route to stop the change, so that's why providing a contact email address felt most appropriate.
We also looked at the subject line of the email notifications, the word "information" seemed redundant, "Email change ..." seemed enough or "Email change notification ..."
We then moved over to the user account screen and look at the status message wording. The first sentence in the message "Your updated email address needs to be validated" didn't work for us, we advise using an active voice and changing it to "You must confirm your email address."
So to summarise:
Most of these are minor tweaks, and once these are addressed this could go back to RTBC.
Again, thanks everyone for all the hard work on this, it's looking great overall.
-Aaron and everyone on the UX Call (Benji, Thomas, Ofer, and Cristina).
Comment #329
skyriter CreditAttribution: skyriter commentedComment #330
rootworkApologies for pushing changes twice. This is actually my first time working in the new MR system (I know, behind) so I'm sorry if I did anything wrong.
The commits include a rebase, and address:
#328.1: Adds the line "If you did not intend to make this change, contact [site-email]."
#328.3: Changes the subject to "'Email change for [user:display-name] at [site:name]"
#328.4: Changes the first line of the status message to "You must confirm your email address."
I also added to the descriptions of the new parameters in
_user_mail_notify()
to clarify thatmail_change_notification
goes to the old address, andmail_change_verification
goes to the new one, because at first I wasn't sure which was which.As for #328.2, I didn't actually see any use of a cancel/one-time URL in the notification email (just in the verification email) but maybe that was changed in the meantime.
Leaving #328.5 for someone who has more history on this issue/area to articulate.
Out of scope for this issue, but I noticed that many of the email templates reference "clicking" a link rather than "using" or "following" (or some other term that would encompass both a click and a tap). Maybe also worth a follow-up?
Comment #331
DiDebruThanks for the patch it works like a charm.
Maybe some improvement is an event or hook to react on after the user has verified,
to also update e.g. NL providers.
Potential follow-up force setting:
After the user changes his email address he will be logged out and blocked.
After he verfies himself he will be unblocked and logged in with updated email.
Comment #332
jonathanshawI think this looks ready now.
#330 seems good.
It's not always easy to follow #328, it's possible they were using an outdated version of the patch or misunderstanding some things. The main point is that found no significant usability issues that are relevant to the latest patch.
Regarding tokens, the default notification email to the old address uses no tokens of questionable safety, but they are available if a sitebuilder wanted to use them. It's not clear whether #328 is for or against them being available, but I believe it's a good compromise and very much an edge case.
For improving the token UI we have #514990: Add a UI for browsing tokens.
Comment #333
AaronMcHaleCasting my mind back to the UX meeting, I think it was just a concern around them being available and the potential for a site builder to unknowingly use them in a way that might result in a security issue for an account being compromised. As in, site builder provides the cancel URL as the only way to take action (not providing contact information) and how quickly that could become invalidated. It wasn't a particularly strong concern from memory, and if the default "old email" template has the site contact email address as the call to action then I think that's broadly fine.
Strictly speaking though it is not in the remit of the UX group to form opinions/recommendations on matters relating to site security, so we would tend to defer to the security team on this specific matter. Therefor, if we could get input on this point from the security team as to whether there is any potential real-world risks relating to these tokens being available in the "old email" template, then I think that would satisfy.
Thanks,
-Aaron
Comment #334
larowlanHiding patches because there's an MR here
Left some comments in the MR.
It looks like all of @alexpott's points from #279 are yet to be resolved.
Do we have a follow-up to add this functionality for non form-based interactions (e.g. JSONAPI etc)
Comment #335
darvanenMight take a crack at this tomorrow.
Comment #336
darvanenConsolidated merge requests, favouring !437 as the original.
Cherry-picked work by @rootwork and @darvanen and pushed.
That should now address #319 within the MR.
What should I do with MR!776? Delete it?
Comment #337
larowlan@Darvanen @quietone and I discussed the need for
MailChangeController::getUrl
at DrupalSouth.We decided that instead of introducing new APIs it would be best inline those 7 lines into user.module and file a follow up to create a user url generator service.
This service could be the home of user_pass_reset_url and user_cancel_url too.
We'd then move the method from the controller to one of the two test classes (the one that uses it the most) and use that in tests, with a todo to remove it in favour of the service from the above follow up when that is done.
Comment #338
darvanenFollow-up created: #3229038: Consolidate public APIs for generating user action URLs into a service
Assigning while I attempt to address feedback.
Comment #339
darvanenRe #279:
Additional question:
The contributed Email Change Verification requires a user to log in to confirm their email change, are we sure we want to allow anonymous users to confirm the change?
Comment #340
darvanenRe #279.6 I tested this flow manually and I've since realised that this issue patches user_pass_rehash to allow injection of the new email address in order to verify the requested email address. I'm thinking we should inline that API change and address it in #3229038: Consolidate public APIs for generating user action URLs into a service.
Comment #341
shalini_jha CreditAttribution: shalini_jha commentedComment #342
darvanenFeedback on #340 in slack from @larowlan:
Comment #343
darvanenRe #279.2, the account settings form allows
mail_change_verification
andmail_change_notification
to be set separately, which makes more sense to me than making the notification dependent upon verification:Suggest instead of enforcing the logic in AccountForm we alter the submit function to send the emails individually instead of the current dependent behaviour?
Comment #344
darvanenComment #345
darvanenGot some direction from @alexpott on Slack: "343 makes sense - if the system obeys the flags then I think we’re good. The code I highlighted in 279.2 does not. This will all need testing."
I think the reason the notification email was nested in a check on the verification email was to make sure it wasn't sent if the verification email was attempted and failed. So I've adjusted the code logic to keep that check in place, while allowing the notification email to still happen if we're skipping verification.
Flagging needs tests for the various email send scenarios as I understand them:
Comment #346
darvanenComment #347
andyrigby CreditAttribution: andyrigby at Ixis - UK Drupal Support, Maintenance, Hosting and Development commentedHi, we're having success with this patch, but unfortunately have hit an issue. If the user attempts to change their password and email address at the same time, they cannot verify their email address using the verify email link.
I believe it's down to the password hash generation, as this occurs only on the User entity save.
e.g.
I had some success calling parent::submitForm() earlier, and calling User->save() in a couple of places in AccountForm, but it wasn't pretty.
We have a fair amount of custom code on this site - I was wondering if anyone was seeing the same issue? #10 alludes to the issue, so it might have been addressed and we're just seeing a conflict from custom / other contrib code.
Comment #348
darvanenYeah.... the password is used to form the comparison hash, and the hash is being generated and sent to the user before the new password is saved, just as you've described.
I've got some thoughts on that tangle in a follow-up #3229038: Consolidate public APIs for generating user action URLs into a service though they were based on loginTime changing rather than the password. I'll add a link to your comment there for discussion.
Not sure what to do about it for this issue. My first instinct is to pull that follow-up into this issue to make it available here but as @larowlan has pointed out, that will delay this issue considerably. But if we change the contents of the inlined code to use a different hash we're going to generate a heap more debate (rightly so) on this thread anyway.
I hate to say it but perhaps we need to flip the dependency and postpone this issue on #3229038: Consolidate public APIs for generating user action URLs into a service.
Unless... is there any way we can ensure Drupal only ever generates the email *after* all account saving activity in a request has been completed? I doubt it.
Custom code is one thing, pretty or not (I've made some pretty ugly class overrides myself) but I wouldn't want to ship this code with this obvious flaw. So thank you for trying it out!
Comment #349
darvanenAhhhhhhahahahaha oh dear.
The checks for whether the email should be sent or not based on the checkboxes shown in #343 are done *inside* _user_mail_notify. So that function returns null if either:
Hi, welcome to the party, my name's Max, yes I just got here too... /facepalm
I think we need to repeat the test for the setting in the submitForm method so that a null result from _user_mail_notify('mail_change_verification', ...) definitely means failure. I'll give that a try.
Comment #350
darvanenObviously this took me a few tries, and in the end I didn't change much. Here's an interdiff against #344 which is just before I started trying to alter the logic regarding the notification.
In short: I consolidated all of the tests required to determine if verification is required into a more readable format (happy to be told that's not how we should do it), and I moved the notification email outside of the verification logic with a flag to disable it if the verification email is attempted and fails.
I've taken a stab at the tests outlined in #345. I have no clue how to cover the event of an email failing to send (test #2 on that comment).
#347 remains outstanding, I've suggested an approach in #348 but I'm not convinced that's the right approach.
Leaving as NW on the above but this needs a few eyeballs other than mine now.
Comment #351
darvanenNow with 100% more attachment.
Comment #352
Driskell CreditAttribution: Driskell as a volunteer and at Other Media commented@darvanen Regarding #347, I think this is due to the parent::submitForm happening after sending the email? As it's parent::submitForm that will copy the new values to the entity and save.
Perhaps the fix here is to only send email once parent::submitForm is called. So it does mean keeping some logic at top to work out, "Do we need to verify?" and if that's the case we need to restore original email, then call parent::submitForm, then trigger the necessary emails.
I haven't tried this yet but hope to allocate some time on Monday (9am BST) to try it. But on the face of things it looks like it would ensure the emails use the latest password hash, with very minor changes. I'll check some of the other remaining tasks too it'll be good if this made it to a security review and eventual release :)
Comment #353
Driskell CreditAttribution: Driskell as a volunteer and at Other Media commented@darvanen I added one commit to add the test for the issue in #347. Sorry if I was meant to push to a separate MR it seemed to me the best approach.
I also added a potential fix for #347. It moves the email send to a separate submit handler. Which makes sense to me to keep the submit processing, save processing, and eventual notifications separate. It also means when the notifications are sent the account is fully saved so any new password is now hashed (which happens in the field plugin on preSave), fixing #347.
Probably one for later but I did wonder about the addition of $mail to user_pass_rehash. I wonder if in a separate issue it should be recommended to instead improve _user_mail_notify. If someone adds a $to parameter to override who receives the email it'll mean the user_pass_rehash does not need modifying. It also has the benefit that inside the "verify change email" notification and the "notify change email" the "[user:mail]" tokens are the same and consistent. It wasn't expected to me that they'd be different as the account wasn't updated yet... and perhaps at the same time a new "$params" would be worthwhile to provide more tokens to the emails, such as the new email in the case of the email change ones.
Furthermore, and this is probably an edge case, if the emails contain any other URLs added by a module using user_pass_rehash, they currently break - using one-time-login-url as a really bad example the URL will fail in the verification notification sent to the new email due to it using the new email for the hash that isn't saved against the account yet.
Comment #354
darvanenOf course! I was too deep in the problem to see we could solve it by running the parent submit first. Thanks @Driskell for giving it a go, and yes putting it on the same MR is fine, we can always rollback if need be (I wasn't sure about this either) - it's better than opening a new one, unfortunately you've revived the very MR I made that mistake with, I should delete that one.
Can you try your changes again on branch
85494-use-email-verification
instead of85494-use-email-verification-9.3.x
? The latter has fallen well behind the current work which left a bunch of logic errors in the code you submitted.I didn't realise all of this until I was through reviewing the logic, I'm going to submit that just so you can see where to watch out.
Comment #355
darvanenAs for the arguments around the emails and how they're used, I think that's valuable commentary for the follow-up: #3229038: Consolidate public APIs for generating user action URLs into a service
Comment #356
Driskell CreditAttribution: Driskell as a volunteer and at Other Media commented@darvanen I've pushed to the correct branch now, sorry about that!
I did find that after my change the testMailChangeNoVerification was not working. It seemed before my changed it was sending a notification for any email change even if verification was disabled. This seemed a usability issue to me - especially as the default notification text contains "In order to complete the change you will need to follow the instructions sent to your new email address". I also found it was sending it even if the email DID NOT change.
So I added a test to ensure if email is not changed it does not send anything. I also adjusted the test for testMailChangeNoVerification to not expect any emails.
If we want to send a notification to old email when old email is changed I would suggest that would be a different template so a default can be provided without the "in order to complete the change" text. This template is then only used if the verification is disabled. Something I would suggest occurs in a different issue as it is unrelated to verification of email addresses.
Comment #357
darvanenI agree the template could be changed, I didn't notice that. See #279.2 and #345 for a bit of backstory on that one... what's the default setting though? If a site admin deliberately turns off verification and leaves notification on, I say it's on them to ensure their email fits the bill.
I personally think since the notification email can be turned on completely independently of the verification email that if it's on, it should be sent regardless of whether the verification is required/requested - EXCEPT if the verification email is attempted and fails.
You're right that I left a hole in the logic where the notification gets sent even if the email doesn't change.
Comment #358
Driskell CreditAttribution: Driskell as a volunteer and at Other Media commented@darvanen I'm on Slack (Jason.woods) if it helps to chat things over to push through.
Thinking about the email change notifications I am thinking the following:
* The mail_change_notification is labeled "Notify user when email changes" but actually we're not doing that - we're emailing them to let them know a REQUEST was made to change it (when verification is enabled). When the email finally is verified and the change happens there is no notification it was done. Possibly this is a further argument for separate "requested" notifications and "email changed" notification - where the "requested" ones are only used is verification is enabled. This to me is extremely useful from a user perspective as it both notifies them someone is trying to change the email, and notifies them if it was successful. It would also mean admins don't need to think about changing email templates when adjusting configuration and ending up getting an email forwarded from a confused person down the line. Further it improves on usability review in #328 as the benefit of this notification is made available even if verification is not desired.
* I also am unsure about the checkbox for verification being inside the emails. All current checkboxes in there are about enabling and disabling notifications. The checkbox for enabling notifying of old email a change happened is fine as that's just on/off. However, the enable of verification email does more than just enable the email - it enables verification preventing email change until action - so it's more wide reaching. The configuration is also called "notify.mail_change_verification" - I think it might be unexpected to some developers that turning off the notification turns off verification. For example, turning off "notify.cancel_confirm" won't stop the process of confirming it would just stop the email. Perhaps it should be top level like the registration approval settings? I appreciate this isn't something raised in the usability reviews though.
* Regarding verification email send failing - I don't think this can be acted upon. So my last push removed some code on that as it was unreachable except in case of failure to send email. Just because the verification failed to send doesn't mean we shouldn't send the other email (which might fail too?). Core only seems to check this in order to adjust messaging to the user to say whether sent or not, or to raise an error that send failed. Not sure either of those cases apply except perhaps the error message as you wouldn't be able to confirm the email change.
What are your thoughts? I've already done some changes locally to split the template and move the verification option to top level and happy to contribute bits of it if we think it's a good approach and improves upon what's here already.
Comment #359
Driskell CreditAttribution: Driskell as a volunteer and at Other Media commented@darvanen Just a thought - does the REST API need to be taken into account? It's an area I'm not really familiar with so unsure if a regular user could update their account via REST API.
Comment #360
darvanen@Driskell considering the age of the issue and the fact that it has gone to the UX review team at least once I wasn't game to change up the form. However, if I was designing this from scratch I would:
All of the settings would be independent of each other, e.g. one could turn on notifications to the old address and no other emails in the case that they don't want verification but do want to let the old address know what has happened and how to deal with it if it's an issue (seems a fairly common pattern in SaaS these days).
I doubt I would ever use all of the options but Drupal's strength is its flexibility, so I would want site builders to be able to create the flow they want out of the options available to them there.
Some of these things mirror what you've written.
As we discussed on Slack, this is opening a rather large can of worms. I'd rather not take this line of thought much further without some direction from the UX team. I think we can arrange that by requesting to present the issue at the weekly meeting.
There's a module
Just a note to anyone despairing that this will ever get off the ground... I've found https://www.drupal.org/project/email_change_verification does a good job, if in a slightly different fashion to how this patch currently aims to operate. I'm honestly surprised it has such a low uptake.
Comment #361
AaronMcHaleRe discussion in #358-360: This issue is getting quite long and hard to follow, so I'm inclined to recommend that we try and get it over the line with the UI as is and then make incremental improvements in one or more follow-up issues, especially given that the current UI has already been signed off. Splitting the improvements in this way will be much easier to manage and much easier to review. An example of this approach is #3199972: Improve user interface text on the account cancellation screen and its various follow-up issues.
It is important to keep in mind that the email templates we provide are just what we consider sensible defaults, the responsibility is left up to the site builder to ensure they make sense for their specific configuration, it would be impossible for us to cover all the various use cases out of the box. We had a lengthy discussion to this effect during the last UX review of this issue.
Overall this is a very positive improvement to Drupal, even if it's not perfect, it's still better than the current state.
Comment #362
Driskell CreditAttribution: Driskell as a volunteer and at Other Media commented@AaronMcHale @darvanen Thank you both. I agree entirely. I've now moved to Needs review. It is entirely a huge benefit as is!
I'll hold on raising additional bits so I can save time to help push this forwards first.
Comment #363
Driskell CreditAttribution: Driskell as a volunteer and at Other Media commentedComment #364
volegerRelated contrib module list:
- https://www.drupal.org/project/email_confirm - D7 only
- https://www.drupal.org/project/email_field_confirm - D7 only
- https://www.drupal.org/project/email_confirmer - D9
- https://www.drupal.org/project/email_change_verification - D9
It is worth tracking such modules as this change will require follow-up feature support issues for the related version of the modules.
Comment #365
jonathanshawIt would really help if someone could document what points are still outstanding for work or discussion ... it's not easy to follow the recent discussions.
Comment #366
darvanen@jonathanshaw I feel you, my apologies for my role in that.
#350 was my attempt at a summary at the last point that I did any work. If your request is still outstanding tomorrow (it's late here) I'll review @Driskell's work against the status at #350.
Comment #367
Driskell CreditAttribution: Driskell as a volunteer and at Other Media commentedThe remaining tasks in the OP are the same with exception that #347 is fixed now, and #342.2 wasn't implemented as it's not something easily tested, and is a rare edge case which I have checked and behaves as described (if fail to send verification email to new email, don't send the notice to the old email either).
Everything else I believe is addressed so just needs a fresh code review, and a security review, and some testing feedback. The usability review was completed and changes requested done so I don't believe needs that again. I will remove needs tests flag as those are all done.
Latest discussions involved me raising a few things that can be improved but @AaronMcHale, @darvanen and I believe that although there are things that could be improved, after 15 years and with the positive improvements this will make to Drupal Core it would not make sense to revisit everything here, so I'll raise separate issues later on if needed to tweak. The code, as is, provides significant benefit and does as described and UX review has already concluded it would be OK with the final requested copy changes made. Of course, maintainers needs to make that final judgement.
Hope this helps move things! I have updated OP with what I think is the current status.
Comment #368
Driskell CreditAttribution: Driskell as a volunteer and at Other Media commentedJust added another note to OP for the #270 follow up questions - as the first two are generally apply also to Account cancel email form thus why I am thinking follow up issue for that.
The last 2 questions seem like separate issues too to improve things and will need a lot of thought (history of changes / revisioning, making the request visible in the admin / storing the new email which is currently not stored we just generate a link containing hashed token against new email and the new email)
Comment #369
darvanenConsidering this, and the age of this issue, may I suggest we take the path of least resistance (notify the old email address *every time*) and open a follow-up to make it configurable whether a notification is sent based on verification?
Comment #370
larowlanThat seems like a reasonable approach - can we get a follow-up issue opened (this issue is already at 370 comments) to add this as a configurable feature?
Comment #374
darvanen@voleger could you change the target branch to 9.4.x again? I can do the rebase if you like.
Comment #375
larowlanI've changed the target to 9.4.x @Darvanen
Comment #376
pfrenssenNow that we have moved to 9.4.x the patch doesn't apply any more to 9.3.x. Here is a backport to 9.3.x for whoever needs it.
This failure is reported by the test bot, but the underlying failure that is logged is this:
error An unexpected error occurred: "https://registry.yarnpkg.com/postcss-discard-comments/-/postcss-discard-comments-4.0.2.tgz: Request failed \"503 Service Unavailable\"".
I wasn't able to find any drupal.org issues related to this, but according to #7996: registry.yarnpkg.com responds with 503 error code quite frequently it looks like it could be a temporary availability issue.
Comment #377
darvanenThe error will likely be because I wasn't able to complete the rebase yet, that patch will help a lot, thanks. Late here, will do tomorrow
Comment #379
dwwThis issue was mentioned in the #ux channel. Seemed blocked on a trivial phpcs fix, so I just re-rebased to the
9.4.x
branch and fixedAccountForm.php
(actually,phpcbf
fixed it, I just committed and pushed). 😉 Haven't looked at any code yet or read the issue, but hopefully the bot will have valid results now. I'll try to more closely review "soon", but if there's anything else that needs fixing in the meanwhile, lemme know.Thanks,
-Derek
Comment #380
jonathanshawNeeds followups creating per #370 and the IS.
Needs security review for #333.
Comment #381
dwwI believe the test failures are from the
user.mail.yml
changes still being 1 giant newline-tastic string, not the nice multiline config that the rest of core uses now that #2844452: Export configuration YAML strings as multiline is done. At leastcore/tests/Drupal/KernelTests/Config/DefaultConfigTest.php
is passing for me again locally. Also re-rebased. Let's see what the bot says now.Comment #382
benjifisherI am adding some related issues based on earlier comments:
I am not sure I described #3265076 well. I plan to do some manual testing of this issue, and I will update the issue description then.
Comment #383
benjifisherI did some manual testing. I enabled both e-mails, then tried to change the address for a non-admin account. There are two problems, but they should both be easy to fix.
First, the e-mail sent to my old address contains the literal text "[site-email]". The correct token is "[site:mail]". Make sure to fix it in both the config file and the update function.
The second problem is that the one-time link in the verification e-mail does not work. The problem is that
mail_change_timeout
is not set inuser.settings
. We have to update the config in the Standard profile with the new settings, and we also have to fix other profiles:I am adding the tag for a change record update, since that same change will have to be made in non-core profiles, if they override
user.settings
.Comment #384
dwwThanks for the manual testing, @benjifisher, great finds! Pushed a few commits to address both points. The relevant tests are all still passing locally, I assume the bot will be happy.
But does this point to a gap in our test coverage? How come we didn't notice that the link wasn't valid until manual testing? I still haven't gotten through the whole diff yet, so I haven't looked closely at the tests, but
core/modules/user/tests/src/Functional/UserMailChangeTest.php
sure looks pretty thorough on a brief skim. However, do we need to add coverage for this before we can merge?Comment #385
benjifisherI think "need to" is too strong, but it is good practice to add test coverage for any bug fix.
I care more about updating the change record. I already added "Distribution developers" to the audience of the change record, but I did not add any text.
Comment #386
dwwThanks to #2852557: Config export key order is not predictable, use config schema to order keys for maps we can get the ordering right in
user.schema.yml
and have deterministic results for exported config. 🎉Drupal\KernelTests\Config\DefaultConfigTest
passes locally once again, so I'm hoping the bot is happy, too. Sorry for all the noise in here.Also, very minor tweak to the release note snippet.
I still haven't finished reading the full diff relative to 9.4.x. I also notice there are some unresolved threads in the MR, but I haven't looked into those closely yet, either.
Agreed on the need for CR updates, but I won't be working on that until I'm at least done with reviewing the diff and keeping the bot happy. 😉
Thanks,
-Derek
p.s. I also just noticed that the 9.4.x version of this has an upgrade path using drupal-8.8.0.bare.standard.php.gz. Sadly, that fixture is already gone from 10.0.x. So when a core committer wants to commit this to 10.0.x first, we're going to need a separate patch. Trying to figure out the right way to handle that from an MR / patch perspective. My thought is to wait on this until the 9.4.x patch is RTBCed and then push a new branch to the issue fork, rebase to 10.0.x, change the path to the fixture only in that branch, and open a 2nd MR here targeting 10.0.x. But to avoid complication, let's not do that until we have to.
Comment #387
benjifisherUsability review
We discussed this issue at #3263897: Drupal Usability Meeting 2022-02-18. That issue will have a link to a recording of the meeting.
Unlike the last time we discussed this issue, we applied the patch and tested functionality instead of relying on screenshots.
Here are some suggestions:
There are a lot of improvements that should be made to the help text. Earlier comments on this issue mention some, and those changes are out of scope for this issue. I want to add one more: the closed tabs should have an indication of whether the email is enabled or disabled.
For the record, the meeting attendees were
benjifisher, Antoniya, beatrizrodrigues, victoria-marina, shaal, rkoller, tmaiochi, worldlinemine, Johnny Santos, kimberlly_amaral
Comment #388
dwwRe: #387: Thanks for the review!
Re: #386.p.s.: Yay, we landed #3263886: Copy drupal-9.3.0.bare.standard.php.gz and drupal-9.3.0.filled.standard.php.gz from the Drupal 10 branch so now we've got a
drupal-9.3.0.bare.standard.php.gz
fixture we can use in all relevant branches. 🎉 Rebased and updatedUpdateMailChangeTest
to use that. Passes locally. So that mostly helps. However, sadly the patch won't apply to 10.0.x user.post_update.php. 😢 So we're still gonna need separate patches / branches once this is actually RTBC...Comment #389
benjifisher@dww, thanks for the updates!
I just added another child issue: #3265346: [PP-1] Manage user account timeouts in the admin UI. I will edit my earlier comment, since this was suggested at today's meeting.
We might get some push-back from postponing the status indication (#387.1) to a follow-up issue. I think we will get less push-back if we change the default from 24 hours (86400s) to something shorter (part of #387.4).
Comment #390
AaronMcHaleCatching up on the recording from #3263897: Drupal Usability Meeting 2022-02-18, unfortunately I was unable to attend the meeting.
The first part of the meeting looked at some of the remaining tasks:
Potentially, but that should be looked at in a follow-up issue, because it impacts the other fields on the form, and what we do here should behave consistent with the other fields. So for this issue, no that's not something we should worry about, but in a follow-up issue it should be discussed for all of the email templates.
One concern I have is that, by using progressive discovery, in order to change e.g. one of the body fields, if you have one of the checkboxes unchecked, you have to check the box, change the field, and then uncheck the box again before saving. If you don't know that the field exists you wouldn't know where to go to change this. Why this is a problem is because it creates inequality between changing something via configuration and changing something via the UI, when you change the same field via configuration, it doesn't matter whether the box is checked or not, you can still find and change the key. Now, that may or may not be a problem, but I think a separate issue to discuss this is appropriate.
Tagging needs follow-up for that.
Would be nice to have, happy for this to be dealt with in a follow-up, as it would be good to have the space to refine what that looks like. Glad to see #3265315: [PP-1] Indicate on the user-edit form when an address change is pending has been created.
I would add that there should also be a link in that message to resend the verification email, the verification link in the email might have expired when the user finally gets to it or maybe the email didn't arrive. That is also quite a common pattern with online services these days. I will also add this to the follow-up issue.
The next part of the meeting looked at more general improvements/recommendations:
Agree with this, and good to see the follow-up issue has been created #3265316: [PP-1] Add a warning message to the site status report if the e-mail to the old address is not enabled..
I would also like to see this warning appear on the account settings form itself in the same part of the form as the email settings. I will also add this comment to the follow-up issue.
I think this was missed from the review comment #387, but I think this would be a nice improvement to add, so we also need a follow-up for this.
So, 2 more follow-up issues required.
Comment #391
AaronMcHaleFiled #3265988: Should there be some indication that additional form fields are available for the email templates on the Account Settings Form and #3265993: Provide at a glance information for which email templates are enabled on the Account Settings form from comment #390.
Comment #392
AaronMcHaleComment #394
darvanenRerolled MR437 as a patch for 9.5.x
As far as I can tell, all of the feedback so far has been addressed.
Comment #395
darvanenOops, here's one without the patch rejections inside the patch file....
Comment #396
smustgrave CreditAttribution: smustgrave at Mobomo commentedTested by applying patch
Running drush updb and enabling the 2 new forms under account settings
Creating a content editor user with email something@test.com
Verified in mailhog I got an email about a new account being created
Changed the email with my admin user account and received no email
Change the email with the logged in content editor I just created and received no email.
Please advise if I missed a step
Comment #397
darvanenThat's odd, I just followed the same steps and it worked fine. Just checking these are the two options you turned on?
I believe it's by design that no email is sent when the admin user changes an email (see IS) in case you were indicating that as a failure.
Comment #398
smustgrave CreditAttribution: smustgrave at Mobomo commentedOkay I tested against this morning and it appears to be working. My test account may not have been fully "verified" first after creation.
If anyone feels it shouldn't be marked RTBC please move back.
Comment #399
quietone CreditAttribution: quietone at PreviousNext commentedI have not read the 398 comments in this issue. :-) But I see that there are quite a few remaining tasks still open. Are they complete or is there work to do?
Can someone familiar with this issue check the open remaining tasks and update the Issue Summary accordingly? Thanks.
Also to do is to get a security review and the change record needs updates.
Thanks.
Comment #400
brad.bulger CreditAttribution: brad.bulger commentedI imagine there has been some back and forth about this, but "require confirmation of changes to email" and related settings seem like a fairly major config setting, that belongs up with "require email verification when a visitor creates an account", not hidden down in the template. I couldn't figure out why the patch was having no effect...
Comment #401
brad.bulger CreditAttribution: brad.bulger commentedI tried applying the 9.4 diff of the fork as a patch before installing, and mail_change_timeout did not end up being defined.
If I installed Drupal on the normal core code first, and then applied the patch, then running updatedb ran user_post_update_mail_change and mail_change_timeout was defined.
Also, it seems like there are no log entries caused by any of the steps of changing an email address. Probably there should be.
Comment #403
brad.bulger CreditAttribution: brad.bulger commentedIt ought to at least be a configurable choice to require being logged in to the account that requested the change for the confirmation link to work. Allowing an anonymous user to immediately confirm the change by using the link is not something we would prefer, at least, I imagine we're not alone. It would need to redirect anonymous users through login, and give users logged in to a different account (including admins) some kind of error message.
Comment #404
brad.bulger CreditAttribution: brad.bulger commentedI am using the patch for 9.4.x from the issue fork, and the problem described in #159.9 above doesn't seem to have been fixed.
1) Log in to user account
2) Change email address
3) Log out
4) Log back in to same account
5) Load mail-change link from mail
This fails with a bad hash match.
Comment #405
DiDebruPatch doesn't apply for 9.5.2
Comment #406
DiDebruPatch doesn't apply for 9.5.2
Comment #407
No Sssweat CreditAttribution: No Sssweat commentedWhy bother? It has been 16 years...
Clearly this tells me that this just a "nice to have" feature that most people are living happily without.
Plus, it seems there are a lot of hoops, walls, and gates to jump through to get this into core.
This should've been moved into contrib or gotten scrapped all together a long time ago.
Comment #408
AaronMcHaleRe-rolls should be done for the latest Core branch, so 10.1.x, not 9.5.x.
Just because an issue has been around for a long time, and still has some work to do, doesn't make it any less valuable or worthy of inclusion in Core. If this functionality is needed for a site there are already contrib modules that will do this.
Comment #409
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch at #395 against 10.1.x. Please review
Comment #411
mxr576Updated the version information in the related CR and also triggered a new build on #409 because it might have failed for transient errors before.
Comment #412
mxr576Unfortunately, it also seems to me that the latest patch still solves this issue on the form level only and does not cover other presentation layers, like APIs. I searched for "JSONAPI" in this thread and I have only found that @larowlan was asked about that in #334 only... even though it is a fundamental building block of Drupal core now and there was an API first initiative.
Added #344 to remaining tasks to have a clear answer for that.
Comment #413
mxr576Comment #414
mxr576Let me gently disagree with this statement in #407. This is very much a requirement by enterprise companies and the fact that this is not OOTB in Drupal makes Drupal less appealing to them. Selling this as custom development does not sound good to them.
Being that said, we have also developed our custom solution once or twice, it is not perfect either, therefore it does not worth sharing either. Plus the customer owns the code.
Comment #415
luenemannComment #416
franksj CreditAttribution: franksj at Breakthrough Technologies, LLC commentedThere is a new level of indent in
user.schema.yml
so I had to adjust the spacing for theuser.mail
changes to make this apply to 9.5.4.Comment #417
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedUpdated patch for 9.5 support
Comment #419
J-LeeComment #420
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #421
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch #417 for 11.x branch, please review it.
Thanks!
Comment #422
karishmaamin CreditAttribution: karishmaamin at Specbee commentedFixed custom command failure #421. Please review
Comment #423
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixed CCF issues, Please have a look.
Comment #425
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixed fail test cases, Please have a look.
Comment #429
larowlanUpdating remaining tasks, hiding patches
Next steps are JSON:API and a reroll.
All other UX followups can be improved incrementally per #360
Comment #431
quietone CreditAttribution: quietone at PreviousNext commentedLocally, I apply the diff to 11.x and fixed the patch rejections from 5 files,
They were all straightforward except for AccountSettings form. That needed extra work due to #3382510: Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms. The it failed spell check and I changed usages of E-mail and Please. I also learned that the AccountSettings form does not have a test and I did not look for one.
To do because of the changes I made to rebase.
Comment #432
quietone CreditAttribution: quietone at PreviousNext commentedThere is a test of the Account settings form and it is failing.
Comment #434
LendudeAccount settings form has a test, and that was failing due to the changes here, so updated that test with the new settings. Change to Account settings form for two fields that needed to use #config_target
Fixed the autowire test and added some type hints and constructor property promotion while I was at it
Comment #436
larowlanPushed some minor cleanups.
Left a review from a security POV
Added new tasks to the issue summary as follows:
user_mail_notify
for sending the verification email, and a custom case inuser_mail
for this case. We should keep the account as is and pass an additional param for the new email.\Drupal\user\Controller\UserController::resetPassLogin
Comment #437
quietone CreditAttribution: quietone at PreviousNext commentedMade what I hope are improvements to the comments and some cleanup. Then did the low hanging fruit from @larowlan review.
Edit: s/handing/hanging/
Comment #438
darvanenI've made an initial attempt to add flood control. It passed a local test of one test class but then my rig broke and I need to stop for now so pushed as-is.
Comment #439
dwwRebased to 11.x, removed a few stray "revert" commits that had ended up in this branch.
Pushed a commit that fixes a new bug in
MailChangeController
.UserMailChangeTest
was failing (in GitLab CI and locally) because it was triggering this fatal error:That gets the test passing locally. Let's see what GitLab CI thinks.
Leaving NW since there are still a lot of un-addressed MR threads I haven't gotten to, yet. Only trying to get back to green tests for now.
Comment #440
dwwThe MR branch had conflicts when rebasing to 11.x due to #3277003: Harden user_pass_rehash() against attack. Since this is a security hardening issue, and these details are so important, I'm attaching a file showing the rebase conflict, and how I decided to merge the two versions. Would be wise for someone else to review this and confirm I've done it right, such that both this issue and 3277003 are happy.
Comment #441
darvanen#440: I've checked the change, and searched for any similar areas in the full diff, looks good to me.
Comment #442
catchDouble checked #440 as well and logic looks the same to me too.
Comment #443
larowlanCopying comms from slack about the fail in #440
Comment #444
quietone CreditAttribution: quietone at PreviousNext commentedI checked the old MR and ported the three unresolved comments/question to the current MR. My apologies if that is repeating answered questions. I figured it was better to do that instead of me reading all the comments to get up to speed.
Comment #445
quietone CreditAttribution: quietone at PreviousNext commentedI did a bit of work on this. This is new stuff for me so I just did what made sense to me today. There are still unresolved comments to address but it would be good to review the changes I made.
Comment #447
InaW CreditAttribution: InaW at Unic commentedI rerolled the patch from #409 for 10.2.3
Comment #449
larowlanLeft a review, there are also some aspects of #279 that are yet to be addressed
Comment #450
larowlanWe discussed this again in the monthly bugsmash meeting and resolved to move this to a feature request because:
a) it is adding new features
b) there is a contrib module for this
Involved in the discussion were catch, griffynh, Amber Hines Matz, quietone, longwave and myself.
Comment #451
lawxen CreditAttribution: lawxen commentedReroll #417 for drupal 9.5.11 without test code
Comment #452
lawxen CreditAttribution: lawxen commented