Set destination request parameter as part of login

Thanks for the refference code, I could have a quick thought and get over my initial hesitation.

Does this work instead? See patch file (untested code, quickly hacked together). After login, I assume we still get to this code block and can remove the 'destination' that was set by the Dashboard module. I assume this code is still executed with Dashboard module installed.

Gives better separation of concerns / we can keep this in the Controller class, because

• I would rather only set the destination parameter as you suggested, if the SAML login was successful
• But that would mean this would need to be somewhere in the middle of the login code, in SamlService.

FWIW:

I just realized this patch, technically, constitutes a behavior change, which I should document.

Now, IF a RelayState is set in the IDP response / post request, then hook code can still override it by setting a 'destination' on the current request. After this change, that will be impossible. (If someone really wants to do this, they'll have to do more work == implement some event subscriber.)

In practice, this translates to; the DESTINATION in /saml/login?destination=DESTINATION will override any hook code that sets a 'destination' on the current request. (Whereas right now, it does not.)

I think we actually want that.

Thanks Roderick -- this does look like exactly what we need. I agree this sounds like it should be the default behavior, but is it something that would fit as a toggle-able configuration option that's on by default?

I will see if I have time to test the patch today otherwise I'll have to wait until next week.

Nice. I'll set "needs review" which is actually needs testing... and wait. Then when that's done, merge*... and include in a release later on.

* (For this specific issue, I'm not too concerned about the exact MR flow because the change is small and -in the current module state- does not need automated tests.)

Feature flag is possible, but I think I'm OK with just implementing it and publishing a change record. This feels like an edge case. It's possible that some people experience a behavior change, but the impact should be quite small. And "now actually redirecting to where the POST request from the IdP says", shouldn't cause real breakage / doesn't have security implications.

I've added the patch and tested it. It is working as expected.

I changed the issue title to better align with the work you did here, although it may still need some tweaking.