Problem/Motivation

The forced redirect on login is very naive, always occurs, and cannot be adjusted from the UI.

When a user requests a password reset, they are logged in then force-redirected to the dashboard if there is one available for them. This makes resetting their password much more difficult.

If a login redirect module like Login Destination is in use, dashboard module's redirect will override that module's redirect.

Steps to reproduce

  1. Setup a dashboard to be available for a user of a given role.
  2. Request a password reset as that user.
  3. Follow the link in the email and submit the form.

Expected behaviour: the user should be presented with a password reset form.
Actual behaviour: the user is redirected to the dashboard.

Proposed resolution

  1. Do not ever redirect from the password reset login.
  2. Provide a configuration setting to opt out of the forced redirect so other login redirect modules can be used.

Issue fork dashboard-3466196

Command icon Show commands

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

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

About issue forks

Comments

John Pitcairn created an issue. See original summary.

thejimbirch’s picture

thejimbirch commented

Some folks are using the ECA module to create custom login workflows.

Here is an excellent example in the feature demo.

https://ecaguide.org/library/use%20case/eca_feature_demo/

johnpitcairn’s picture

johnpitcairn commented

That's not the point. There are many ways to create login redirects. This module shouldn't blindly override those, especially if it's from a password reset.

thejimbirch’s picture

thejimbirch commented

Oh, I agree with you John! Just adding an additional way that people are currently using for login redirection.

thejimbirch’s picture

thejimbirch commented
Issue tags: +Starshot blocker

I am running into this working on the Drupal CMS SEO recipe. I am creating an SEO dashboard, and because I am using this module, now all users that login get redirected to the dashboard.

John's proposed resolutions are good. An alternate solution would be to remove all redirection from this module and let other solutions handle that workflow.

Adding Starshot blocker tag.

thejimbirch opened merge request !14

thejimbirch’s picture

thejimbirch commented
Status: Active » Needs review

I created a merge request that removes the redirection completely. For my use case/testing, it appears to be working as expected with no errors or warnings logged.

There are PHPstan errors and failed tests in the pipeline, but I believe they are unrelated.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs review » Needs work

The aim of Dashboard module is to provide a better experience for users right after login.

Agree that current implementation could be too aggressive, and alternatives could be discussed.

However, I believe that removing it completely is too much.

Being tagged as a Starshot blocker, we have to take care of it somehow.

Could you please explain your specific needs to try to find a better way to handle login redirects?

Maybe a setting to toggle redirect could be enough for the problematic scenario.

johnpitcairn’s picture

johnpitcairn commented
  1. Users who have requested a password reset should not be redirected to a dashboard before they are able to set a new password.
  2. If another login redirect is present, eg from Login Redirect module, or configured via an ECA rule, dashboard should not override that.

Item 2 could possibly be met by a simple enable/disable checkbox. Item 1 cannot.

My recommendation would be that if redirection is desired, starshot should use a dedicated configurable login redirect module or ECA rule. We should not rely on a simplistic solution here, it will only wind up needing special case logic that is already solved in contrib. Redirect should be out of scope for this module.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

Totally agree that 1 has to be addressed.

My concern was that MR approach to solve the bug was removing one of the main module features.

thejimbirch’s picture

thejimbirch commented

I am proposing that redirection shouldn't be "one of the main module features". In a composable ecosystem such as Drupal you can rely on the many existing solutions to handle the redirection, and concentrate on the other dashboard features.

ECA is already in Drupal CMS. You could lower the amount of code you have to manage by including an ECA config that handles the redirection. Then it could be turned off or altered by the site owner.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

@thejimbirch The idea of this module is eventually become part of core, and IMHO the default experience when you log in into drupal core is the raison d'etre of the issue in the ideas queue that ended up being the initiative where this originated.

I'm of course open to allow people creating alternatives, but don't think we should rely on any other module for this, or even a setting. AFAIK now it would be possible to intercept this with hook_module_implements_alter. So unless we are preventing ECA or other modules to prevent our redirect, I think this is a won't fix (aside of the reset password form, which is definitely a bug)

johnpitcairn’s picture

johnpitcairn commented

So every contrib or custom module or ECA rule or whatever that redirects at login may be broken as soon as dashboard is installed. And all those implementations will have to add implements_alter() code to fix it.

Aren't we aiming to be a lower-code platform? How is somebody who just uses an ECA rule via the UI supposed to deal with this?

Its hard to know what's out there in the wild that will be disrupted - I think this really does need to be controlled by a setting in the UI.

johnpitcairn’s picture

johnpitcairn commented

ECA is already in Drupal CMS. You could lower the amount of code you have to manage by including an ECA config that handles the redirection. Then it could be turned off or altered by the site owner.

Yes, exactly. Why do we need to handle redirect here at all if it can be done better elsewhere in core, and at the same time introduce people to the power of ECA?

samit.310@gmail.com made their first commit to this issue’s fork.

samitk’s picture

samitk commented
Status: Needs work » Needs review

Hello,

I have fixed the failed phpunit test cases, Please review.

Thanks
Samit K.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Active

This is still under discussion.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Active » Needs work

We discussed this at DrupalCon with @timplunket and @pameeela.

We agreed that redirecting to the dashboard on login is something that we should do.

We agreed that we should respect any destination argument and NOT redirect in that case.
I think that should solve the password reset form issue too, but in any case we want specific test coverage for that usecase.

Thanks everyone for raising up this concern.

thejimbirch’s picture

thejimbirch commented

Redirect to the dashboard after login was added to Drupal CMS yesterday using ECA.

https://www.drupal.org/project/drupal_cms/issues/3477300

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

That doesn't change that this still needs to happen here. We can reuse some of the test code from there, thanks!

plopesc opened merge request !27

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Created new MR !27 adding exceptions for destination query parameter and one time login links.

It has been interesting because in order to make test compatible with both D10 & D11, we had to add some tricky logic due to #3469309: Use one-time login link instead of user login form in BrowserTestBase tests.

Hope once this is merged the Drupal CMS repository could rely on these redirects instead of custom ones.

Thank you!

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

I think the MR !14 is the correct one here as it removes the redirect by the dashboard module entirely. Redirects are now managed by ECA and there's no need for something in here at all. If this module wants to keep the redirect feature for outside Starshot usage, then it should probably be configurable so that it can be turned off in Starshot.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented
Status: Needs review » Reviewed & tested by the community

This looks like the correct approach to me, too. Thanks, @plopesc!

penyaskito changed the visibility of the branch 3466196-remove-redirect to hidden.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

Fixed #3479666: Fix phpcs issues in gitlabCI and rebased with it, so phpcs checks will pass.

  • penyaskito committed f002c01f on 2.x authored by plopesc 
    Issue #3466196 by plopesc, penyaskito, john pitcairn, mtift: Dashboard...
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

Merged!

See #3466196-18: Dashboard should not force redirect on password reset or override other redirects

Status: Fixed » Closed (fixed)

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

penyaskito closed merge request !14