Dashboard should not force redirect on password reset or override other redirects

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/

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.

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

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.

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.

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.

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.

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.

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.

@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)

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.

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.

Hello,

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

Thanks
Samit K.

This is still under discussion.

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.

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

https://www.drupal.org/project/drupal_cms/issues/3477300 📌 Create recipe for certain technical redirects with ECA Active

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

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 ✨ Use one-time login link instead of user login form in BrowserTestBase tests Fixed .

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

Thank you!

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.

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

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

Fixed 📌 Fix phpcs issues in gitlabCI Active and rebased with it, so phpcs checks will pass.

Merged!

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

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

penyaskito → closed merge request !14