Problem/Motivation

We are calling the procedural functions user_login_finalize() and user_logout() in user.module.

However, these have a load of dependencies such as

  • AccountInterface
  • SessionInterface
  • SessionManagerInterface
  • EntityTypeManagerInterface
  • ModuleHandlerInterface
  • LoggerInterface

Let's create a service and do proper dependency injection.

Steps to reproduce

NA

Proposed resolution

  • create a new \Drupal\user\UserSessionFinalize
  • define a new 'user.session_finalize' service
  • deprecate user_login_finalize() and user_logout()
  • replace all usages in core

Remaining tasks

NA

User interface changes

NA

API changes

  • user_login_finalize() and user_logout() are deprecated
  • a new 'user.session_finalizer' / \Drupal\User\UserSessionFinalizer service is added

Data model changes

NA

Release notes snippet

NA

Original summary

Lets move this somewhere..maybe the User entity controller? i am not really sure cause it has to do with session

Issue fork drupal-2012976

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

It's already on the UserController, so I think we're done here ?

ParisLiakos’s picture

nope. i dont think we should suggest calling controller methods directly. still the API call here is user_logout() which is procedural

Crell’s picture

We need a service or method on a service that explicitly logs out an active user. It should be called FROM a controller, but not be on a controller. Controllers should not have meaningful business logic in them.

dawehner’s picture

Status: Active » Needs review
FileSize
5.82 KB

I really wonder whether it is worth to make an interface here and I can't get some sleep.

Status: Needs review » Needs work

The last submitted patch, user-2012976-4.patch, failed testing.

ParisLiakos’s picture

if we are introducing a separate service for this i think we should generalize it more and include similar functions eg
user_login_finalize

maybe call it UserHandler? or AccountHandler

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.8 KB
11.05 KB

Mark will come up with a better name anyway if needed.

dawehner’s picture

FileSize
12.02 KB
11.05 KB

Ups.

Status: Needs review » Needs work

The last submitted patch, user_handler-2012976-8.patch, failed testing.

ParisLiakos’s picture

Title: Objectify user_logout() » Objectify user_logout() and user_login_finalize()
Issue summary: View changes

We have UserAuth now, its probably a nice place to put those there, too

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Issue tags: +Kill includes
jibran’s picture

Which include file are we killing here?

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kim.pepper’s picture

Issue tags: -Kill includes

Yeah, I agree. This is just methods on user.module

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

apaderno’s picture

Version: 9.5.x-dev » 11.x-dev
Issue tags: +Needs issue summary update

The issue summary must be updated too. It is not completely clear what should be done and why.

kim.pepper’s picture

Status: Needs work » Needs review
kim.pepper’s picture

kim.pepper’s picture

kim.pepper’s picture

Title: Objectify user_logout() and user_login_finalize() » Deprecate user_logout() and user_login_finalize() and replace with a service
andypost’s picture

It looks nice but naming confusing me, as just landed #2536052: Remove try...catch from SessionHandler::write with another session handler

Maybe user module should just decorate \Drupal\Core\Session\SessionHandler?
it's the real storage service which is mussing in related issue

andypost’s picture

smustgrave’s picture

Status: Needs review » Needs work

All threads appear to be resolved and tests all green.

This was tagged for CR updates so moving to NW for that.

Good job everyone!

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record updates

Updated CR

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Updated CR reads well.

Thanks @kim.pepper!

lauriii’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs framework manager review

Added few comments to the MR. Tagging for framework manager review to get a review on the new API.

kim.pepper’s picture

Thanks for the reviews. Addressed all feedback.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

With 10.2 approaching going to remark this for committers/framework managers to take a look.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

#32 was never really addressed - the name UserSessionHandler implies something that extends \SessionHandlerInterface like \Drupal\Core\Session\SessionHandler but this is not that.

Also by introducing \Drupal\user\UserSessionHandlerInterface and the service we are creating a new swappable API which feels like something we don't actually want to do. Are there use-cases?

We also need to update the deprecations for 10.3.

kim.pepper’s picture

Status: Needs work » Needs review

Removed UserSessionHandlerInterface and updated deprecation notices to 10.3.0.

Not sure what a better name is? Maybe UserLoginHandler?

Back to NR for feedback on that suggestion.

andypost’s picture

OTOH there's SessionManagerInterface which can be extended with finalizeLogin() and finalizeLogout() methods

znerol’s picture

OTOH there's SessionManagerInterface which can be extended with finalizeLogin() and finalizeLogout() methods

I'd prefer if SessionManagerInterface will go away in the long run. All of the remaining code in SessionManager is supposed to be extracted to session handler decorators or even to upstream.

Prashant.c made their first commit to this issue’s fork.

kim.pepper’s picture

How about UserSessionFinalizer with finalizeLogin() and finalizeLogout() ?

andypost’s picture

extracted to session handler decorators

it sounds good idea, needs to check which services can get decorators to react of login/logout

andypost’s picture

larowlan made their first commit to this issue’s fork.

larowlan’s picture

Status: Needs review » Needs work
Issue tags: -Needs framework manager review

Looks great.
Couple of places where we're missing docs (can't use @inheritdoc if there is no parent interface).

#45 sounds like a good idea.

This feels like a case where we would want to make the service final to make it a bit harder to swap out.

Looks to have failing tests.

kim.pepper’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Appears all feedback has been addressed.

Since this has gone through several rounds of reviews I'm going to remark it vs the new #needs-review-queue-initiative strategy of waiting a week.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think #51 is premature. #45 suggests a name for the class and there's general agreement but that's not what is the MR.

Also the service class is not final as suggested by #49

kim.pepper’s picture

Status: Needs work » Needs review

Renamed to UserSessionFinalizer and made the service class final.

kim.pepper’s picture

Getting this error in the test failure which I think is unrelated:

1) Drupal\Tests\standard\FunctionalJavascript\StandardJavascriptTest::testBigPipe
TypeError: trim(): Argument #1 ($string) must be of type string, bool given

Looks like it was a random fail.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
90 bytes

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.

kim.pepper’s picture

Status: Needs work » Needs review

Rebased on 11.x

andypost’s picture

Thank you! It looks ready!

btw as the new service is public it makes it an extension point, literally hooks user_(ligin|logout) can be implemented as the service decorators?

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Appears feedback has been addressed.

@andypost let me know if I'm wrong but sounds like in #57 you're asking a general question correct?

catch’s picture

Status: Reviewed & tested by the community » Needs review

The issue summary and change record no longer match the MR.

kim.pepper’s picture

Issue summary: View changes
kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Updated CR and IS. As I just updated the service and method names, and there are no code changes, I think it's safe to re-RTBC.

quietone’s picture

Status: Reviewed & tested by the community » Needs review

I'm triaging RTBC issues. I read the IS and the comments. I didn't find any unanswered questions.

I read the MR and made some comments. One of which is questioning the name of the service so I am setting to NR to settle that.

quietone’s picture

I chatted with @kim.pepper who wasn't not opposed to changing UserSessionFinalizer to UserSessionFinalize. Therefore, I have made a commit for that. And also updated a comment.

quietone’s picture

I did not update the change record. Happy to do that if folks agree to the name change.

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I think Finalize makes sense too. I updated the CR and issue summary to reflect that.

catch’s picture

Status: Reviewed & tested by the community » Needs review

There are some things I'm not sure about with this one but not sure I can articulate all of them. Left some review comments on the MR for now.

longwave’s picture

@catch asked for further opinions on this. My first thought was that really we are just moving code around; procedural code is now in a service that isn't meant to be swapped (because it's declared final), so what do we gain here?

Also, a new service with seven very different dependencies is a bit of a code smell. Should login and logout be events instead? Then each subsystem can declare event subscribers and react as they see fit, and each part would be replaceable individually if someone wanted to switch something out?

znerol’s picture

Should login and logout be events instead? Then each subsystem can declare event subscribers and react as they see fit, and each part would be replaceable individually if someone wanted to switch something out?

I was thinking the same.

Berdir’s picture

> @catch asked for further opinions on this. My first thought was that really we are just moving code around; procedural code is now in a service that isn't meant to be swapped (because it's declared final), so what do we gain here?

See #1905334: Only load all modules when a hook gets invoked. What we gain is that we move one of the few remaining .module functions called from controllers and other OOP code, allowing us to load .module files only when a hook is invoked.

> Should login and logout be events instead? Then each subsystem can declare event subscribers and react as they see fit, and each part would be replaceable individually if someone wanted to switch something out?

We already have login/logout hooks. This is the API responsible for calling these hooks. I think we shouldn't have events *and* hooks for the same thing, so is the proposal to replace the existing hooks with an event and the sole API for finalizing login/logout would be to invoke the respective event?

That does sound sensible, we do have some existing examples for that, for example around file name sanitization. But that would also make this a considerably larger issue as we'd need to convert existing hook_user_login/logout implementations, deprecate the hook and also think about the API design of these hooks. One of the major use cases for hook_user_login() is to control where the user is redirected to afterwards and that's only possible with workarounds atm which tend to break other implementations of that hook.

FWIW, basically everything the service does is a user module thing, I don't think we could really split that up much apart from separating login and logout.

> Given we log separately here, do we need the logging to be done in the finalize service itself? I think we could take that out of the service and callers are responsible for logging if they want to.

I can see an argument for both options. Ensuring that each login is properly logged seems important from a security perspective, so it seems sensible to do it in the API. But separating would avoid duplicate logs and logs could be more specific.

smustgrave’s picture

So this is something that should still move forward?

catch’s picture

I think we shouldn't have events *and* hooks for the same thing, so is the proposal to replace the existing hooks with an event and the sole API for finalizing login/logout would be to invoke the respective event?

I think what @longwave meant here was that the methods added here would be a wrapper around the even dispatcher, and the implementation would move into the event - e.g. the logging could then be its own event listener which a site could remove if they wanted to. I haven't thought through the impact of this myself yet.

cmlara’s picture

Speaking as a voice for contrib that interfaces with authentication.

My reading is that user_login_finalize() is currently public API and that it is intended to be called whenever a module wishes to indicate a user has logged-in and user_logout() is intended to be called whenever we wish to logout a user no matter how they were logged into the system.

Both of these global functions currently perform double duty, where they trigger actions for contrib to act upon and they setup/tear down User module internal information (setting the user in the session).

I recently mistakenly thought user_login_finalize() was only used for session based auth and that all others would provide their own authentication provider event listener, however in TFA we recently encountered a bug (due to our increasing security) that forced me to realize the user_login_finalize() method is called by modules that perform external authentication, ref #3426189: 2.x-dev incompatible with other modules calling user_login_finalize().

This raises the question what is the future vision for this new service? Is it intended to only be used by the user module and the rest of us should invoking the hooks ourselves or is it intended to still be called by 3rd party contrib to 'log in' a user?

If its intended to be used by the user module only than I would suggest the deprecation should be changed to indicate these methods have been deprecated with no direct replacment, the user should instead call the necessary hooks/events themselves. The service may not even be necessary if the actions can be done in-line.

If its intended to be used by contrib than I believe we need to either put this on an interface or at a minimal make the class non-final to allow it to be mocked in unit tests. If the class stays final with no interface a helper trait (that provisions it in a dummy mode) for unit tests would be helpful to avoid boilerplate code in contrib. Addtionaly it would likely be helpful if the User module specific items (setting up a session with the UID set) are moved outside of the service and instead done where elsewhere.

From a TFA maintainer standpoint:
Having this as a service that could be decorated would allow a long term cleaner fix to #3426189: 2.x-dev incompatible with other modules calling user_login_finalize() and allow us to be involved in more locations to provide enhanced security.
Having this as internal we could likely provide our own authentication provider and cleanup some of our code. It would probably make intercepting the login form more complicated however I doubt it would be a blocker.

In either case I do agree that user_login_finalize() and user_logout() should be be removed from procedural code.

Tagging as related to #2901795: Facilitate 2FA+MultiFactor compatibility (2FA/two-factor -> MFA/multi-factor) as user_login_finalize() and replacements impact the MFA ecosystem.

solideogloria’s picture

Glad to see there's an issue for this. I was going to open an issue if there wasn't one already.

smustgrave’s picture

Status: Needs review » Needs work

Only moving to NW for the open threads from @catch on the MR.