One-time login link failure messages are misleading because they are not marked as errors

The message doesn't have a status in D8 either, so I'm bumping the version. It's an easy change, so this is a good Novice issue.

I'm not sure if this is a bug. It's maybe more of a task.

I just added the 'error' message type.
Here is the patch.

         if ($reset_link_user = $this->userStorage->load($uid)) {
           drupal_set_message($this->t('Another user (%other_user) is already logged into the site on this computer, but you tried to use a one-time link for user %resetting_user. Please <a href="@logout">logout</a> and try using the link again.',

This is an unrelated issue, but this strikes me as a bad idea. On a shared computer, User A does their thing and walks away. User B gets a reset link in their e-mail and follows it. Drupal does not allow User B to use their one-time login link, but instead openly discloses that User A is logged in -- and keeps User A logged in. User B now knows they are logged in as User A and they have free reign to go wreak havoc. Am I paranoid or does anyone else think this is perhaps a separate issue that needs some discussion?

@OpsTao
Please read the backport policy. Changes must be applied to the current development version, D8, before being applied to older versions.

It's a very small change and could be applied to D8 quickly once it passes review, which I'll try to do today. Then it can be backported to D7.

I'm at the DrupalConLA sprint, as a novice contributor - checking out this issue. Update: Looks like this issue's in flux - letting it go...

At DrupalCon LA and going to look into this issue.

Reviewed and tested patch. All three use cases work as intended.

Tested the patch in comment #3 under the 3 scenarios in the issue description; link expired, invalid and already logged in. Error and warning messages are being displayed as expected.

From @jpourzal: I am at the DrupalConLA sprint and just tested the patch with edutrul. It appears to be work since the message now appears in yellow, instead of green. Screenshot is attached(post_patch_message.png). (apologies to i8flan, we did not realize you had begun working on this)
edutrul: (apologies to i8flan, jpourzal couldn't submit the form because of request role issue...) So that's why I am posting this comment for him.

Anybody have thoughts for the security worries in #5?

The overall change to make this a warning is ok. One nitpick unrelated to the original issue is that we want to avoid using the word "Please" in the interface copy.

Public security issues should be considered as critical.

RE the issue brought up in comment #5, my impression is that it is not related to the UI improvements provided by this patch and should be created as a separate issue if it is deemed that it should be addressed.

This is just some helpful security improvement, not fixing an actual sechole, so it is not critical. The patch looks good though.

Thanks @tim.plunkett!

I'm working on this issue during sprint @ la drupalcon

I tested three cases as follows and the results look ok

- different logged-in user : warning message

- logged out status - expired time : error message

- logged out status - wrong key : error message

Cleaning up metadata.

RTBC++, thanks for the screenshots @gyuhyon!

I've re-rolled the patch from #3.

There were some conflicts whilst re-rolling, but they were resolved.

CONFLICT (content): Merge conflict in core/modules/user/src/Controller/UserController.php

Tested the re-rolled patch from #29 and same results as before. All three scenarios are working as expected.

In that case, re-marking as RTBC. :)

(Making the title a bit more specific since I didn't understand the scope of the issue at first.)

Thanks for the fix and for the screenshots; they definitely help clarify the issue. I agree this is a normal bugfix as the patch is currently written (simply marking these messages as errors or warnings as appropriate. I reviewed the patch locally with git diff --color-words to confirm that the only changes are making these messages warnings and errors instead of happy green success messages. :)

As a followup, we should get rid of the uses of "please". Reference: https://www.drupal.org/node/604342 (No need to block this change on that, though, since the pleases exist in HEAD).

Regarding @tibbsa's feedback in #5, I had the same unease with that, but that should be filed as a separate issue. Also see the related issue: #889772: Following a password reset link while logged in leaves users unable to change their password

This issue only changes user-facing strings, CSS, or markup, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x.

The addition of the warning/error parameters should be backport-able.

The addition of the warning/error parameters should be backport-able.

They are indeed.

For reference, I was able to test the expired link by adding $conf['user_password_reset_timeout'] = '1'; to settings.php.

Patch #36 fixes for account cancellation link but leaves off expired login link. Fixed the same in attached patch.

Here's an interdiff between the patches in #36 and #37.

I don't think that I'm able to RTBC #37 as I wrote the patch in #36, and there's not enough difference between the two.

The changes look good, but what about:

110.  // Invalid one-time link specifies an unknown user.
111.  drupal_set_message(t('The one-time login link you clicked is invalid.'));

(@opdavies, thanks for the testing tip!)

@markpavlitski, here's an updated patch and a new interdiff that includes that extra line.

We should also check if this line also exists in Drupal 8.

RTBC for the patch in #40 and the attached D8 patch addresses the two additional messages fixed in #37 and #40.

Moved into #2561685: Follow up to: One-time login link failure messages are misleading because they are not marked as errors

(#41 is a D8 patch)

(#41 is a D8 patch)

I've moved the D8 patch in to a follow-on ticket to avoid confusion: #2561685: Follow up to: One-time login link failure messages are misleading because they are not marked as errors

Committed to 7.x - thanks!

The user login process tends to be heavily customized by particular sites, so I bet there's someone who cares about this or who is altering $_SESSION['messages']['status'] to change these or something crazy like that. So I'm putting this in CHANGELOG.txt and the release notes as a precaution.

By the way, for this (from #5):

This is an unrelated issue, but this strikes me as a bad idea. On a shared computer, User A does their thing and walks away. User B gets a reset link in their e-mail and follows it. Drupal does not allow User B to use their one-time login link, but instead openly discloses that User A is logged in -- and keeps User A logged in. User B now knows they are logged in as User A and they have free reign to go wreak havoc. Am I paranoid or does anyone else think this is perhaps a separate issue that needs some discussion?

Most sites that allow users to log in make it obvious who the current logged-in user is as soon as they visit the site and look at the page, so I'm not sure why this extra message would be a problem?

It could make sense to have a separate issue to discuss logging user A out automatically in this situation, though (after all, user B is trying to log in). In practice I don't think this scenario commonly occurs at all, except for developers testing things with two different accounts on their machine :)