Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

Status: Needs work » Needs review

samhassell requested that failed test be re-tested.

Status: Needs review » Needs work

The last submitted patch, , failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
agentrickard’s picture

FileSize
993 bytes

Under that logic, isn't the password _always_ a hash? Should we ever send it to the user with a token?

samhassell’s picture

The password isn't a hash at the time the user first submits it during account creation. Currently by default Drupal will send out the password in the default sign up email (user.module ~ line 2200) - Apart from that yes it should always be a hash and therefore not be included as a token.

Perhaps it shouldn't be available as a token at all - in that case we would need a larger patch altering the default emails being sent out.

I guess from a user experience POV sending the password is good, but from a security POV it is bad. Though if someone has access to a users email account its trivial to request a new password anyway. Hrm.

I think we should just fix the admin problem so user:password is only available where it should be. Any other ideas?

(Marked http://drupal.org/node/652838 and http://drupal.org/node/345821 as duplicates of this issue.)

agentrickard’s picture

No, I think you're on the right track. If you can get a patch to pass tests, I'll review.

Re-test of 660302-password-token.patch from comment #5 was requested by adpo.

moshe weitzman’s picture

Seems to me like we can just remove that token and never send the password. We don't do distrubuted auth in core anymore and openid users never need to know their pass

agentrickard’s picture

@moshe

When an admin creates an account, is a one-time login sent instead of a user-pass combo?

samhassell’s picture

Both a one-time login and the user/pass combo are sent.

user.module:2224 onwards :

	A site administrator at [site:name] has created an account for you. You may now log in to [site:login-url] using the following username and password:

	username: [user:name]
	password: [user:password]

	You may also log in by clicking on this link or copying and pasting it in your browser:

	[user:one-time-login-url]

	This is a one-time login, so it can be used only once.

	After logging in, you will be redirected to [user:edit-url] so you can change your password.

We could switch it to:

	A site administrator at [site:name] has created an account for you. You may now log in to [site:login-url] by clicking on this link or copying and pasting it in your browser:

	[user:one-time-login-url]

	This is a one-time login, so it can be used only once.

	After logging in, you will be redirected to [user:edit-url] so you can change your password.

A similar change would be made at 2220 for the user-initiated registration.

Also 2345-2347 where it creates the password token could be removed.

webchick’s picture

The ability to mail a user their username/password from admin/people/create is a really valuable feature and one of the only things in Drupal 6 that tested really well. :P So I'd like to not lose it, even though I agree that from a security standpoint sending only one-time login links is best.

webchick’s picture

Actually, after further discussion on IRC, I actually think I'm totally on crack! :D

Not only would this be more secure, but:

* It would also prevent the user from accidentally making an e-mail template that sends out the password hash, and getting very confused people asking them why $P$CYFZ5WRTrPNciT915C1URBYI8WJyfW0 isn't working as their password. :P

* It also seems to be what most sites on the Internet are doing these days "Click the link to verify your account..."

One suggestion about the template is having the username in there is really helpful. I can't remember if I'm "webchick" or "webchickenator" or "angie" or "lullabotangie" or what, so could refer back to an e-mail about that.

webchick’s picture

Issue tags: +Usability

Also, yoroy gave a sign off on this too, so it has UX team approval. The exact wording and stuff will have to be worked out.

samhassell’s picture

Righteo,

Here's another patch.

In this one the no approval required text is based off of the original text, but with references to the password removed.

Thank you for registering at [site:name]. You may now log in to [site:login-url] using the following username:

username: [user:name]

You may also log in by clicking on this link or copying and pasting it in your browser:

[user:one-time-login-url]

This is a one-time login, so it can be used only once.

After logging in, you will be redirected to [user:edit-url] so you can change your password.


--  [site:name] team

The Admin-created text is completely rewritten in the style of the Activated text:

[user:name],

A site administrator at [site:name] has created an account for you. You may now log in to [site:login-url] by clicking on this link or copying and pasting it in your browser:

[user:one-time-login-url]

This is a one-time login, so it can be used only once.

After logging in, you will be redirected to [user:edit-url] so you can change your password.

Once you have set your own password, you will be able to log in to [site:login-url] in the future using:

username: [user:name]


--  [site:name] team

I guess we should choose one of these styles as one must be superior for some reason, right?

Or we use something completely different.

Thoughts?

jhodgdon’s picture

I like the admin one just fine.

The first one seems strange - giving the user name, not the password, and not mentioning the password -- perhaps it should say something like "with the password you created"?

jhodgdon’s picture

Status: Needs review » Needs work

I forgot to change status on this

jhodgdon’s picture

After applying this patch, I still get this message:

      drupal_set_message(t('Your password and further instructions have been sent to your e-mail address.'));

That can't be right... (user.module around line 3219)

Also, it seems that when I create an account as an anon user, I don't put in a password, so should the email that goes out say anything about being able to log in aside from the one-time login link? If the password is generated automatically, the user won't know what it is and can only use the one-time link?

And here's a small rewording I wrote for the admin-set-up message, for consideration for the next patch:

[user:name],

A site administrator at [site:name] has created an account for you. You may now log in to [site:login-url] by clicking this link or copying and pasting it into your browser:

[user:one-time-login-url]

This link can be used only once.

After logging in, you will be redirected to [user:edit-url] so you can change your password.

Once you have set your own password, you will be able to log in to [site:login-url] in the future using your password and:

Username: [user:name]


--  [site:name] team
reglogge’s picture

Regarding the admin-setup-message read more like this (suggested change highlighted):

[user:name],

A site administrator at [site:name] has created an account for you. You may now log in to [site:login-url] by clicking this link or copying and pasting it into your browser:

[user:one-time-login-url]

This link can be used only once.

After logging in, you will be redirected to [user:edit-url] so you can change your password set your own password.

Once you have set your own password, you will be able to log in to [site:login-url] in the future with the following credentials:

Username: [user:name]
Password: Your own password

-- [site:name] team

The reason being that from a user's perspective, he/she never set a password before, so "changing" it would be the wrong message. The user acting on this notification actually "Sets" his/her own password after logging in with the one-time link.

reglogge’s picture

FileSize
7.26 KB

#18: You're right, the confirmation mail for users that registered themselves is broken. Right now (after applying the patch from #15) it says:

Thank you for registering at [site:name]. You may now log in to
[site:login-url] using the following username:

username: [user:name]

You may also log in by clicking on this link or copying and pasting it in
your browser:
...

Problem is, the user of course cannot log in just with his username. This results in an error. He also never sets a password during registration.

The clean solution is to completely scratch the mailing of user/pw credentials (which are very insecure anyway) from all these confirmation mails and only ever send out one-time login-links.

Additionally, after checking the "lost password" confirmation mail (which already uses a one-time login-link), I find the problem that there remains the an unresolved token in the actual mail being sent:

reglogge,

A request to reset the password for your account has been made at
drupal.localhost.

You may now log in to [site:uri-brief] by clicking on this link or copying
and pasting it in your browser:

http://drupal.localhost/user/reset/4/1263463211/9325039ab3df6bb1ffe79d8ff6dc635a

This is a one-time login, so it can be used only once. It expires after one
day and nothing will happen if it's not used.

After logging in, you will be redirected to
http://drupal.localhost/user/4/edit so you can change your password.

Summing up, we should do three things:
- rewrite all confirmation mails to use only one-time login-links and stop all transmission of actual user credentials (username/pw-combos)
- rewrite the corresponding messages like jhogdon found in #18 to something like A welcome message with further instructions has been sent to your e-mail address.
- clean up the wrong token in the "lost password" confirmation mail

I have attached a rerolled patch that solves these issues

reglogge’s picture

I have no idea why the bot doesn't accept the patch for testing...

reglogge’s picture

FileSize
7.26 KB

2nd try for the patch

reglogge’s picture

Status: Needs work » Needs review

D'Oh

Status: Needs review » Needs work
Issue tags: -Usability

The last submitted patch, confirmation_mails.patch, failed testing.

Status: Needs work » Needs review

Re-test of confirmation_mails.patch from comment #22 was requested by reglogge.

Status: Needs review » Needs work
Issue tags: +Usability

The last submitted patch, confirmation_mails.patch, failed testing.

reglogge’s picture

New patch attached that should pass

reglogge’s picture

Status: Needs work » Needs review

again

reglogge’s picture

Title: Password recovery email in user administration page lists password as a token » Registration and password recovery emails should not contain passwords
Priority: Normal » Critical

Correct me if I'm wrong, but this issue seems critical to me, especially after the comments by webchick and moshe.

Users can be totally turned off when sent confusing mails upon registering at a site!

What the latest patch contains:

- All mails that users get sent after either a) registering themselves, b) having an account created for them by an admin, c) having an account activated by an admin or d) requesting a new password now don't contain the actual password but only a one-time log-in-link.
- The wording of the mails reflects that fact and instructs the user to log in with this link and then change or set his/her password. It now reads

Thank you for registering at [site:name]. You may now log in by clicking on this link or copying and pasting it in your browser:
 
[user:one-time-login-url]
 
This is a one-time login, so it can be used only once.
 
After logging in, you will be redirected to [user:edit-url] so you can set your password.
 
Once you have set your own password, you will be able to log in to [site:login-url] in the future using:

username: [user:name]
password: Your own password
 
 --  [site:name] team

- adjust the messages shown to users after registering to "A welcome message with further instructions has been sent to your e-mail address."
- fixes the wrong token from #20
- adjusted test (create-user) in the user-module

Status: Needs review » Needs work

The last submitted patch, 660302_confirmation_mails.patch, failed testing.

jhodgdon’s picture

One minor suggestion: Can we change

This is a one-time login, so it can be used only once.

to say just

This link can be used only once to log in.

or something else that is less redundant?

reglogge’s picture

That makes sense :-)

Will incorporate.

I'm just fighting with the testbot that keeps rejecting my patches for totally unrelated test-fails (in color module for heaven's sake!)

jhodgdon’s picture

That last test failure (locale/translation) I've seen before. Just request a retest and it will likely go away.

reglogge’s picture

One more thing:

The registration mails as well as the password reset mail contain this paragraph:

After logging in, you will be redirected to [user:edit-url] so you can set your password.

Clicking on those links from the e-mail invariably results in an "Access denied" page, since the user obviously is not logged in when he receives such an e-mail.

Another way to confuse users! We send them an e-mail with a link that inescapably results in an unfriendly error-message.

How about just removing this paragraph from the e-mails?

reglogge’s picture

we could append jhogdon's sentence from #31 this way:

This link can be used only once to log in and leads you to a page where you can set your password.

This is exactly what happens.

reglogge’s picture

Status: Needs work » Needs review
FileSize
8.43 KB

Patch incorporating everything from #29 through #35

jhodgdon’s picture

Status: Needs review » Needs work

Given #34, should we also remove the [user:edit-url] token, since the link won't work anyway?

There are still some style issues in these messages:

- We don't say "clicking on this link". We say "clicking this link". (see http://drupal.org/node/338208)
- "pasting it in your browser"... I think this should either be "pasting it to" or "pasting it into"?
- "you will be able to log in to [site:login-url]"... How about "you will be able to log in at [site:login-url]"?
- The reset email is using kind of a different syntax than the others.
- "A site administrator at [site:name] has created an account for you. You may now log in to [site:login-url] by clicking on this link or copying ..." ... Why provide a link to site:login-url that won't do anything for them, because they have to use the link?

jhodgdon’s picture

In case that was confusing, here's my suggestion for the admin email:

A site administrator at [site:name] has created an account for you. You may now log in by clicking this link or copying and pasting it to your browser:

[user:one-time-login-url]

This link can be used only once to log in and leads you to a page where you can set your password.

After setting your password, you will be able to log in at [site:login-url] in the future using:

username: [user:name]
password: Your password
reglogge’s picture

Status: Needs work » Needs review
FileSize
8.67 KB

New patch incorporating suggestions from #37 and #38.

[user:edit-url] token is gone from all user-facing e-mails. Admins still will receive mails with this token when being notified of an application for a user account.

jhodgdon’s picture

I like all the email texts now.

One small question:

+  $email_token_help = t('Available variables are:') . ' [site:name], [site:url], [user:name], [user:mail], [site:login-url], [user:edit-url], [user:one-time-login-url], [user:cancel-url].';

I'm not sure about the translatability of this.... We usually try to put the whole sentence/paragraph into t(), including punctuation and spaces. So I think it should be:

+  $email_token_help = t('Available variables are: @variable-list.', array('@variable-list' => '[site:name], [site:url], [user:name], [user:mail], [site:login-url], [user:edit-url], [user:one-time-login-url], [user:cancel-url]'));

Sorry for not noticing this in my earlier review.

reglogge’s picture

I'm not sure either, but it seems to me as if this is not a problem since everything after 'Available variables are:' is just a list of tokens which would not be translated anyway. I wouldn't change it therefore.

Also: how do we get this committed now? Should somebody (Bojhan maybe) have a look at it?

jhodgdon’s picture

That's not quite true. There is a period at the end of the sentence. I'm not sure every language uses "." as the end of the sentence, or that every language would use the ordering "here is the stuff: stuff." for this sentence construction.

jhodgdon’s picture

It's not super-critical for alpha, so it won't get committed until sometime next week at the earliest. You can ask on #drupal-docs for another UI/docs person to review the text.

I do think that t() change needs to get into the patch.

reglogge’s picture

ok, didn’'t see it this way, but you know more about these things...

new patch with changes from #40

jhodgdon’s picture

Looks good to me, but as I participated in writing the email texts, I cannot mark it RTBC. Please ask on #drupal-contrib or #drupal-doc on IRC for a review.

jhodgdon’s picture

Uh oh... Something I just thought of. The tokens inside the email messages shouldn't be changed by translators, correct? So I think we need to modify the messages a bit more. For example:

Thank you for registering at !site-name-token. You may now log in by clicking this link or copying and pasting it to your browser:
 
 !one-time-login-token

etc.

And then of course the second argument to the t() wrapper:

array('!site-name-token' => '[site:name]')

etc.

Thoughts?

jhodgdon’s picture

Status: Needs review » Needs work

Also, think about someone who speaks Chinese, for instance, seeing [site:name] -- if they don't read English, will they know what this means in the email? I don't think so...

So I think the help text that explains the tokens should also allow translators to put in an explanation of what they are. Something like this would allow that:

$email_token_help = t('You can use the following in your email message text: @site-name-token, @site-url-token, (etc)', array('@site-name-token' => '[site:name]', ...));

So someone could translate this into "You can use the following in your email message text: @site-name-token (name of your site), ..." (in their language).

reglogge’s picture

Status: Needs work » Needs review
FileSize
15.79 KB

rerolled patch with suggestions from #46 and #47.

jhodgdon’s picture

Thanks for doing that... Again, someone besides me probably needs to review this.

alexanderpas’s picture

subscribing

+++ modules/user/user.module	18 Jan 2010 21:32:46 -0000
@@ -2247,138 +2247,135 @@ function _user_mail_text($key, $language
+This link can be used only once to log in and leads you to a page where you can set your password.

This link can only be used once to log in and will lead you to a page where you can set your password.
(not sure which is better)

otherwise it looks very good, no additional comments.

Powered by Dreditor.

reglogge’s picture

Status: Needs review » Needs work
FileSize
88.82 KB

I found a problem with the tokens being displayed to the user after implementing #47. When visiting the page where the site admin can change the text of e-mails (/admin/config/people/accounts), the tokens are displayed correctly with the exception of [site:name] which is rendered as the actual name of the site (in my case drupal7.localhost).

I can't find the error that leads to this behavior. The placeholder !site-name-token doesn't get replaced with [site:name] in other mail templates as well. The same is true for !site-login-url-token not being replaced by [site:login-url]. The other replacements work. D'oh!

Corresponding sample code from the patch in #48 to user.module:

$text = t('Account details for !user-name-token at !site-name-token', array('!user-name-token' => '[user:name]', '!site-name-token' => '[site:name]'), array('langcode' => $langcode));
...
$text = t("!user-name-token,

Thank you for registering at !site-name-token. You may now log in by clicking this link or copying and pasting it to your browser:

!one-time-login-token

This link can be used only once to log in and leads you to a page where you can set your password.

After setting your password, you will be able to log in at !site-login-url-token in the future using:

username: !user-name-token
password: Your password

--  !site-name-token team", array('!user-name-token' => '[user:name]', '!site-name-token' => '[site:name]', '!one-time-login-token' => '[user:one-time-login-url]', '!site-login-url-token' => '[site:login-url]'), array('langcode' => $langcode));

Screenshot attached - help needed.

jhodgdon’s picture

I've confirmed this behavior on my test box from the above patch, and am looking into it.

jhodgdon’s picture

Here's the problem...

All of those email messages are in function _user_mail_text() in user.module.

Whether they're displayed on the admin screen (so you can edit the text of the messages) or they're being used to send email messages, at the end of _user_mail_text() they are being passed through token_replace().

That's wrong... If they're being displayed for admin, they shouldn't be passed through token_replace() at all. I have no idea if/how this used to work...

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
21.65 KB

Here's a patch. It is the patch from #48, with the suggested change from #50, and a fix for the issue identified in #51.

reglogge’s picture

FileSize
22.25 KB

Ah, this works now!

I found two more small things and attached a patch (based on #54) to fix them:
- mail-footers (-- [site:name] team) were missing from the mails 'Account blocked' and 'Account cancelled'
- in the 'Account blocked' mail there was a '\n\n' between the salutation and the message body instead of just 2 empty line as in all other mails.

reglogge’s picture

FileSize
22.25 KB

corrected small typo ('e-mail' instead of 'email').

pwolanin’s picture

Can someone adjust the title to reflect the current patch? Recovery emails have never contained a plain text password.

reglogge’s picture

Title: Registration and password recovery emails should not contain passwords » Registration e-mails should not contain passwords

done

Dries’s picture

I reviewed this patch and decided to commit it to CVS HEAD. Thanks.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Can we mark this fixed or does it need something else (test or docs?)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Just forgot to update the status.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Alan.Guggenheim’s picture

Status: Closed (fixed) » Needs work

I am sorry, but I do not think the issue is completely solved yet. The one time login links do not work all the time (plain text emails or some email clients will either truncate, insert hard cr, or even add ... and [1] and move link to bottom...
It makes it very difficult and sometimes impossible for people to use.
So for the password reset email at least, I think we need to be able to also send a token with a temporary password in text mode. Or we need to fix the long html strings issue.

webchick’s picture

Status: Needs work » Fixed

I don't mind a contrib module (such as LoginToboggan, perhaps) adding this functionality, but core should exercise security best practices. And it is decidedly insecure to send plain-text passwords in e-mails. Almost all of the services I've signed up for in the past year use this same "Click here to log in and choose your password." approach.

alexanderpas’s picture

I can see where #63 is coming from... (remember those "AOL USERS CLICK HERE" links in emails)
but it should probaly be fixed in another issue (with a link back to here perhaps.)

however, as webchick already indicated, passwords won't be sent in emails.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Alan.Guggenheim’s picture

Status: Closed (fixed) » Active

I just upgraded to 6.16 and the issue is still there. On one hand, we still send the password in plain text in the regsitartion email, on the other, the procedure to click on the one time link is borken for 90% of the users, as it does not redircet them to a simple screen where to change the password, so they do not know what to do and where to go.

On 6.16, when a new user registers, he gets an email that says:
Thank you for registering at . You may now log in by clicking on this link or copying and pasting it in your browser:
http:///user/validate/5873/1268094884/99a2e7...
This is a one-time login, so it can be used only once. After logging in, you will be redirected to http:///user/5873/edit so you can change your password.
This is actually wrong, as he does not get re-directed.
He gets a message saying:
•Login successful.
•You have just used your one-time login link. It is no longer possible to use this link to login. Please change your password.

And there is no indication on how to change the password!!!

jhodgdon’s picture

Version: 7.x-dev » 6.x-dev
Status: Active » Patch (to be ported)

This was a Drupal 7.x issue, and was fixed there.

If you want this open for discussion of fixing in Drupal 6, here's the correct issue status.

Alan.Guggenheim’s picture

Thanks. Do I need to open a new issue, or will this now be the right place to discuss and resolve?

jhodgdon’s picture

Using this issue is fine. The next step would be to make a patch for Drupal 6.

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs work

Uh, oh. Let's get back this to D7 first. This patch changed lots of things in email templates. The patch did not make any attempt to update existing email templates if they were the same as defaults or inform the user if their templates were different to the defaults, that tokens and default suggested templates changed. This could be a huge update headache for people, since the emails will not contain any data if the old placeholders are used (since they are totally different now). We totally need update functions here.

grendzy’s picture

Issue tags: +D7 upgrade path
sun’s picture

wow. It's also wrong to replace replacement tokens with replacement tokens.

The stated reason for this change was "translators should not change the tokens". This is totally moot. Drupal core does not allow to translate variables. And if a contributed module allows a user to translate variables, then it's the responsibility of that module to ensure that this user is a trusted user and knows what he's doing.

Effectively, this change broke integration with extensible tokens for an invalid reason. Please revert that.

yoroy’s picture

Might be handy to update this issue's title to better reflect what needs doing here…

grendzy’s picture

Title: Registration e-mails should not contain passwords » Revert invalid translation of tokens in user registration emails; migration path for missing !password token

ok, how's this?

yoroy’s picture

I would have if I understood what's going on here. :) Just trying to keep the criticals moving. Thanks!

Sylvain Lecoy’s picture

And what are the options if you run drupal as a web service : for example an IFRAME, and you want your user being activated by an admin first, but as he never set his password, he can't log.

zgchenai’s picture

CAN anyone tell me how can i do this in drupal 6?

catch’s picture

#56 no longer cleanly reverts. I don't have time to resolve the conflicts, for someone who does that's shouldn't be too hard of a task though. Looks like we need to start again from scratch here.

jhodgdon’s picture

Title: Revert invalid translation of tokens in user registration emails; migration path for missing !password token » Migration path for missing !password token

RE #73 - the problem is that it is not OK just to insert something like
'[user-name], [another-token], ...'
into a message in all languages, I think (the commas and spaces between the tokens, that is). That is why this was translated the way it was. Translators would not be able to change the literal tokens, just the way they're presented in the message.

Re #71, that is a valid issue - what to do with existing email templates.

sun’s picture

I'm slightly confused now.

[tokens] are string replacement variables. t() has custom support for !replacement @variables.

The patch in #56 changed the code, so user mail text variables contain t()-style string replacements that are replaced with [token] replacement tokens via t(), which in turn are replaced with the actual values of the [tokens], but only when sending a user mail, resp. passing TRUE as last argument.

You may have to read that again. Am I the only one who sees a problem here, or did I miss an important argument for this doubled abstraction? In case of the latter, this should have been documented in code.

The user mail variables were purposively migrated from t()-style string replacements to [tokens], since the new token system is extensible. Aside from that, it would be horribly confusing to have two different types in style and syntax of replacement strings in a single template (which is the case for user mail templates now).

jhodgdon’s picture

Yeah. But the point of the t() stuff is that the *default* template, with the tokens inserted into it, which is supplied by Drupal when you install Drupal, needs to be translated (by the translation team, not a site owner) into different languages. That way, when you install Drupal in, for instance, Chinese, you don't get an English-language email message that maybe you cannot even read and understand (before you change it) as your default.

sun’s picture

Sure, all strings need to be translated. But that's no reason to replace replacement variables with replacement variables, which has been done here.

jhodgdon’s picture

heyrocker and I were just discussing this on IRC. Some comments about the migration path:

(8:02:49 AM) jhodgdon: I don't think we can really convert existing templates, because if someone has overridden the default, and it has [user:password] in it, then they will need to manually rewrite the text they were sending.
(8:02:56 AM) jhodgdon: All we can probably do is flag an error somewhere.
(8:03:23 AM) jhodgdon: I mean, just removing [user:password] will leave them with text in the message saying "Here's your password: (blank)".
(8:03:30 AM) jhodgdon: And if we don't notify them, ...
(8:03:34 AM) heyrocker: right
(8:03:46 AM) heyrocker: if we can detect we can throw a warning
(8:03:47 AM) jhodgdon: So my feeling is that they should get an error message during the hook_update_N?
(8:03:51 AM) heyrocker: right
(8:04:16 AM) jhodgdon: and if they go to the page that lets them edit their messages, they should get an error/warning there.
(8:04:26 AM) heyrocker: we should replace it with '' also though, rather than let it go out with user:password i think
(8:04:38 AM) jhodgdon: Agreed.
8:07:42 AM) jhodgdon: actually if you remove the [user:password] and replace it with '' during upgrade, you can't then warn them about it when they go to edit their email message texts.
(8:08:06 AM) heyrocker: um
(8:08:20 AM) heyrocker: what if we add a new token for user:password that is always ''
(8:08:29 AM) heyrocker: that seems hacky

Not sure what the conclusion should be...

jhodgdon’s picture

Another note, regarding the translations.

These kinds of lines in function user_mail_text:

$text = t('Account details for !user-name-token at !site-name-token', array('!user-name-token' => '[user:name]', '!site-name-token' => '[site:name]'), array('langcode' => $langcode));

These are used to create the default messages, if they haven't been customized by admins.

These default messages need to be translated. This one, in English, should be:
"Account details for [user:name] at [site:name]"

We need to ensure that the tokens are not translated, e.g. [user:name] -> [usario:nombre], which is what the use of arrays was supposed to accomplish. If translators are universally aware that things in [] must be left alone, we can put them directly into the text.

As another note, the D6 version of user_mail_text used ! in the translatable strings.

However, in other places in core we are using [] now, such as:

function system_message_action_form($context) {
  $form['message'] = array(
    '#type' => 'textarea',
    '#title' => t('Message'),
    '#default_value' => isset($context['message']) ? $context['message'] : '',
    '#required' => TRUE,
    '#rows' => '8',
    '#description' => t('The message to be displayed to the current user. You may include placeholders like [node:title], [user:name], and [comment:body] to represent data that will be different each time message is sent. Not all placeholders will be available in all contexts.'),
  );
  return $form;
}

So we could probably go ahead and simplify it and hope the translators understand what is going on.

apaderno’s picture

I see the point of sun. Users could translate !site-name-token as well; replacing a token with a t()-placeholder could not resolve the issue.

If somebody would translate a token like [user:name] then the translated token would appear in the page (as token functions are expecting [user:name]); in such cases, somebody would report that the translation is wrong.
If we are worried for that, then a translator could replace a link shown by a Drupal core module with a link to his own site (and maybe a hacked site). We depend from the translators to correctly translate the strings; there is nothing the core code can do to avoid wrong translations.

jhodgdon’s picture

I think we are now all agreed on this.

I believe heyrocker is working on a patch to put the [tokens] back into the default messages, and take care of the migration issue.

gdd’s picture

Status: Needs work » Needs review
FileSize
13.35 KB

This patch removes the double-translated tokens and returns them to their original versions. It also adds an update function which takes any email templates containing '!password' and removes it. If this happens it returns a notice to the user that they will want to review these templates. This has not been tested, I need to figure out if the upgrade path is currently working well enough to actually test it. In the meantime letting the bot get a shot at it.

jhodgdon’s picture

- It looks like the warning message on update is being printed whether or not any of the saved messages actually had !password in the message. Should only print if the messages had !password in them.

- Do we need an upgrade path to convert the D6 !username, etc. tokens to the D7 equivalents, or has that already been done?

jhodgdon’s picture

Also:
- docblock not standards compliant on the update function
- space at end of a line in update function (about 6 lines down, the blank line has trailing whitespace)

And in the _user_mail_text() function, there are a couple of spots that still need changing:

After setting your password, you will be able to log in at !site-login-url-token in the future using:
...
A site administrator at !site-name-token has created an account for you. You may now log in by clicking this link or copying and pasting it to your browser:
...
After setting your password, you will be able to log in at !site-login-url-token in the future using:
jhodgdon’s picture

Title: Migration path for missing !password token » Migration path for changed user email tokens; don't complicate translation of default messages

Changing title to reflect current issue status. There is not any current migration path for any of the user email tokens...

gdd’s picture

FileSize
14.38 KB

OK new patch attached which does the following

- Fixes the warning message bug from #90
- Fixes docblock to include a single line description
- Fixes lingering old tokens in _user_email_text()
- Adds upgrades for all email tokens, not just !password
- Removes trailing space

gdd’s picture

FileSize
14.27 KB

A couple more updates from discussion with davereid.

- There was an extra if ($result) from a previous patch that is no longer relevant and has been removed
- Combined the old/new arrays into one associative array for better readability and maintainability.

jhodgdon’s picture

Status: Needs review » Needs work

a) Don't do this please:

+  $email_token_help = t('Available variables are:') . ' [site:name], [site:url], [user:name], [user:mail], [site:login-url], [user:edit-url], [user:password], [user:one-time-login-url], [user:cancel-url].';

You are assuming that in all languages, if they translate "Available variables are:' and then you concatenate that literal string that you've put afterwards, that it will work. Please just put the whole thing inside t(), so they can translate the whole thing into the right order and stuff.

b)

+
+/**
+ * Update email templates to use new tokens.
+ *

Update -> Updates

c) It would be helpful to have a link to the page where they can edit/review the messages in the message about "password isn't there any more".

d)

-Thank you for registering at !site-name-token. You may now log in by clicking this link or copying and pasting it to your browser:
+Thank you for registering at [site:name]. You may now log in to [site:login-url] using the following username and password:

User name and password? Keep the same message as before would be better...

e)

+        $text = $text = t('Account details for [user:name] at [site:name] (canceled)', array(), array('langcode' => $langcode));

extra $text = in this line

gdd’s picture

FileSize
14.35 KB

Hm, looks like I regressed a couple things there? Weird. New patch attached, I think we've got it here.

gdd’s picture

Status: Needs work » Needs review

Note in this patch I also added a check so we only have to calculate the message once even if multiple templates have !password in them.

roper.’s picture

Points D and E from #95 are still present..?

gdd’s picture

FileSize
14.43 KB

Issue E is actually fixed in that patch, but here is a new patch with D fixed as well. Been at this too long today.

jhodgdon’s picture

I'd like to modify the doc a bit in one place... new patch coming in a few minutes...

jhodgdon’s picture

FileSize
14.3 KB

There was also an extra : at the end of one of the lines in one of the default email messages.

And that
$text = $text =
thing was still there too.

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community

This looks great. The only thing I've noticed is I'd like for token.module to easily replace the help text, but I'll file that as a separate issue since it's unrelated to the update.

roper.’s picture

$text = $text =
Still there. :-P
Looks like it was in ~4 different spots originally, but one still remains (in case 'cancel_confirm_subject').

Also, and this might be stretching it, but in the update: strpos($row->value, '!password') would eval to FALSE if "!password" is at the beginning of the message, although that's unlikely I'm sure.

strpos($row->value, '!password') !== FALSE

Perhaps..? Probably not necessary though... Otherwise looks good!

roper.’s picture

Status: Reviewed & tested by the community » Needs work

CNW for dupe variable...

Dries’s picture

Status: Needs work » Reviewed & tested by the community

I'd like sun to sign off on this issue.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
14.29 KB

Cross post on last status update...

I've removed the last $text = $text =

sun, if you could be so kind as to review...

Status: Needs review » Needs work

The last submitted patch, 660302-morecleanup.patch, failed testing.

Stevel’s picture

user_update_7009 is taken.

This is still valid (from #103)

Also, and this might be stretching it, but in the update: strpos($row->value, '!password') would eval to FALSE if "!password" is at the beginning of the message, although that's unlikely I'm sure.

strpos($row->value, '!password') !== FALSE
jhodgdon’s picture

Status: Needs work » Needs review
FileSize
14.31 KB

It does seem HIGHLY unlikely that someone would put !password at the very start of an email message, but I suppose it is remotely possible.

Actually, the empty($message) logic was backwards in that line anyway. Here's a fixed patch. Not sure why that other one didn't apply...

jhodgdon’s picture

whoops, extra space in there

Stevel’s picture

Status: Needs review » Needs work

I still see user_update_7009 present here, but that update function already exists in current HEAD. I'm still reviewing the different instances of tokens.

The last submitted patch, 660302-fixlogicstrpos-nospace.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
14.29 KB

Apparently, Dries is faster than I am today. Reroll.

Stevel’s picture

Status: Needs review » Needs work
+++ modules/user/user.admin.inc	25 Jun 2010 19:45:41 -0000
@@ -422,7 +422,7 @@
+  $email_token_help = t('Available variables are: [site:name], [site:url], [user:name], [user:mail], [site:login-url], [user:edit-url], [user:password], [user:one-time-login-url], [user:cancel-url].');

I thought [user:password] was supposed to be gone?

+++ modules/user/user.module	25 Jun 2010 19:45:41 -0000
---  !site-name-token team", array('!user-name-token' => '[user:name]', '!site-name-token' => '[site:name]'), array('langcode' => $langcode));
-You may now cancel your account on !site-url-brief-token by clicking this link or copying and pasting it into your browser:
+You may now cancel your account on [site:url-brief] by clicking this link or copying and pasting it into your browser:

!site-url-brief-token isn't included in the update function
edit: it isn't included in $email_token_help either.

Powered by Dreditor.

gdd’s picture

Status: Needs work » Needs review
FileSize
14.46 KB

Addressed both these issues, and also added [site:url-brief] to $user_mail_text because it was missing from there as well (which is why I missed it in the udpate function.)

Stevel’s picture

Status: Needs review » Reviewed & tested by the community

This is looking good now.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks great. Looks like Dries likes it too.

Committed to HEAD. Thanks!

lyricnz’s picture

Looks like this patch got committed just after #838438: Test the upgrade path, which caused the hook_update_N to collide. See #839172: Cannot redeclare user_update_7010().

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Schnitzel’s picture

Only a hint for people who are confused, why the password functionality is away in D7 (like me) and searches a functionality to bring back the password in mail.

I wrote a patch for loggintobogan, which creates the password token: http://drupal.org/node/1165126