Disable the user.login.http route by default and move to REST

Should the route move to rest.module? We could either enable it by default only if rest.module is enabled, or add it as a config option there?

More than once I wondered why this wasn't provided by REST module so that makes sense to me.

How would this work in terms of BC? Would we need to deprecate the route in the user module when we move it to rest, then remove it from user in D12?

Is there a use case for using the REST login route without having the REST module installed? It seems like a niche thing to do, we could just document in the release notes that you have to enable the REST module now if you are using this.

I'm not sure how to deprecate the route.
We only support deprecations on route aliases but when can't make user.login.http an alias of the new route because users without the rest module will not have the new route.

I had the same issue on #3542528: Deprecate route comment.new_comments_node_links, deprecated the old route and create a new one with a different name.

Do we need to change the paths here? It would be better if users of the endpoints didn't have to update their code.

Wondering if we can mark them as deprecated in user.routing.yml then alter the routes in rest.module (if it's installed) to remove the deprecation again. Then in Drupal 12 we move them directly to rest.routing.yml.

When do the deprecations get triggered for deprecated routes? It's not clear to me how people will discover this change if they are using the endpoints externally.

I see route aliasing a la Symfony 5.4 seems to have been agreed as a way to deprecate routes. See #19 and #20 in: old issue on route aliasing.

When do the deprecations get triggered for deprecated routes? It's not clear to me how people will discover this change if they are using the endpoints externally.

At #10 @mstrelan refers to his participation in a route deprecation involving the comment module: deprecation of a route

Route aliasing and deprecation was only ever for deprecating route names, for people generating links to the old route etc. not for actual requests to the URL.

I think we probably need to trigger a deprecation from the controller in this case.

#19 would mean a stable API for clients (as long as the site itself enables rest module) and seems like it otherwise would work to me.

I think the user module could do a route alter to restore the deprecated route if the rest module is not installed, rather than the rest module doing it. But I will admit I haven't been following closely.

Re #19 I came across this issue controller resolver.The advantage of a controller resolver is the controller is never hit by the request. By putting the code inside it and making it conditional on the route, it is immaterial which core module is involved. Ideas like #21 might be easier to implement in the controller resolver?

I have left a couple of code comments.

Had this opened from yesterday but wanted to verify we could still deprecate in 11.3 with the alpha out and we are good.

Resolved the threads as the deprecations as is are correct for routes.

Reviewed the CR https://www.drupal.org/node/3552724 well detailed and written and before/after snippets for how others can replace their code.

Going to mark but will mention is this one of the larger changes that maybe should be removed in 13

Took a quick stab at credit saving.

I think we need to do something along the lines of #19, we don't want to force users/clients to change the paths they use just because of this.

Implemented #19 by changing the rest.module paths so they are the same as user.module's, and adding an event subscriber to change the deprecated controller to the non-deprecated one if REST is installed.

Added some review comments as well.

Even wondering if we can keep the same route names somehow - then users with REST module see no changes at all, and users who were using this feature just have to enable it?

This should work without changing paths for end users - now you either have to install REST module to allow REST logins, or do nothing if you already had it installed.

Did some more self review, maybe we should make further minor changes here?

One of the deprecations appears to be pointed at 10.3? But they should be updated to 11.4 now I believe with beta out.

Seems like high potential disruption. Should it be removed in 13 vs 12

As a security improvement the benefits hopefully outweigh the disruption. If you are using these paths, you just need to enable the REST module. If you are referring to the route names internally, you do need to update your code, but I would hope that is rare - given most users of this functionality are doing it from outside Drupal to get access to Drupal.

Gotcha,

Did leave some small comments on the MR.

Believe feedback for this one has been addressed

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This one was positive, conflict in the baseline. Rebased and restoring status.

JK actually a phpstan error.

I validated the most recent changes. They are in line with modifications necessary for making PHPStan happy - adding trait functions to the baseline and updating a deprecated function call.

Since I'd never looked at this one before I gave the whole MR a review. But it's been looked at enough by other people. I didn't find anything to comment on. So all I can say is that the most recent changes are good.

Updated the deprecation messages to be from 11.4.0, otherwise looks looks great so went ahead and committed/pushed to main, thanks!

We'll need a backport MR here for 11.x

The CR is still draft

@kim.pepper thats because we need to backport to 11.x first.

ah ok

Backported via cherry-pick from main and manual fix of merge conflicts.

This is worth calling out in the release notes I think.

This is worth calling out in the release notes I think.

Agreed, I've seen several sites that have custom workarounds for this.

I waited a bit to see if there would be a follow-up commit to update the baseline, but since there hasn't been one yet I'm setting the status to Needs Work.

Aside from that, I reviewed the backport by comparing it to the version committed to main. They're practically identical with the exception of the baseline. Once that's updated, then I'll RTBC it.

Seems like a good backport

Some trickiness here in the backport that the tests caught. In Drupal 11 we still need to support both UserAuthInterface and UserAuthenticationInterface. The backported RestAuthenticationController only handled the latter, so I had to copy the code that handles both interfaces (including the deprecation notice) from UserAuthenticationController.

The BC test is in UserJsonBasicAuthDecoratedTest which now passes locally. The fix is self-contained in the last commit to the backport branch.

Oh nice find, forgot about that fun even when I did the user deprecation removal.

Committed/pushed the backport MR to 11.x, thanks!

Per #64 I've reverted this from the main, 11.x and 11.4.x branches.

Given this is security hardening, it would be good to try to get a better bc layer in and recommit it to 11.4 if we can, although not much time to do that and getting it into main/11.5 properly is more important than rushing.

Opened MR !15891 to fix phpstan in head. NR for that.

Committed the baseline hotfix, thanks. Back to NW for #64.

Should the status of the change record for this be changed back to draft since this change was reverted?
https://www.drupal.org/node/3552724

Tried to address #64, also rebased against main and modernised a bit of the new REST controller.

Re #75 I also unpublished the change record for the time being.

I'm not sure how this fits in with #3578868: [policy, no patch] Move REST module to contrib - what should sites that want to support logging users in, which JSON:API doesn't cover, but don't want the rest of REST module do? Even if we don't move REST to contrib it seems like this might want to be in its own dedicated module instead?

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Think this is going to miss the 12 removal timeframe and have to go to 13?