Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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()
anduser_logout()
- replace all usages in core
Remaining tasks
NA
User interface changes
NA
API changes
user_login_finalize()
anduser_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
Comment | File | Size | Author |
---|---|---|---|
#55 | 2012976-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Issue fork drupal-2012976
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:
Comments
Comment #1
swentel CreditAttribution: swentel commentedIt's already on the UserController, so I think we're done here ?
Comment #2
ParisLiakos CreditAttribution: ParisLiakos commentednope. i dont think we should suggest calling controller methods directly. still the API call here is user_logout() which is procedural
Comment #3
Crell CreditAttribution: Crell commentedWe 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.
Comment #4
dawehnerI really wonder whether it is worth to make an interface here and I can't get some sleep.
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedif 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
Comment #7
dawehnerMark will come up with a better name anyway if needed.
Comment #8
dawehnerUps.
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedWe have UserAuth now, its probably a nice place to put those there, too
Comment #16
Mile23Comment #17
jibranWhich include file are we killing here?
Comment #19
kim.pepperYeah, I agree. This is just methods on user.module
Comment #26
apadernoThe issue summary must be updated too. It is not completely clear what should be done and why.
Comment #28
kim.pepperComment #29
kim.pepperComment #30
kim.pepperComment #31
kim.pepperComment #32
andypostIt 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
Comment #33
andypostComment #34
smustgrave CreditAttribution: smustgrave at Mobomo commentedAll threads appear to be resolved and tests all green.
This was tagged for CR updates so moving to NW for that.
Good job everyone!
Comment #35
kim.pepperUpdated CR
Comment #36
smustgrave CreditAttribution: smustgrave at Mobomo commentedUpdated CR reads well.
Thanks @kim.pepper!
Comment #37
lauriiiAdded few comments to the MR. Tagging for framework manager review to get a review on the new API.
Comment #38
kim.pepperThanks for the reviews. Addressed all feedback.
Comment #39
smustgrave CreditAttribution: smustgrave at Mobomo commentedWith 10.2 approaching going to remark this for committers/framework managers to take a look.
Comment #40
alexpott#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.
Comment #41
kim.pepperRemoved
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.
Comment #42
andypostOTOH there's
SessionManagerInterface
which can be extended withfinalizeLogin()
andfinalizeLogout()
methodsComment #43
znerol CreditAttribution: znerol commentedI'd prefer if
SessionManagerInterface
will go away in the long run. All of the remaining code inSessionManager
is supposed to be extracted to session handler decorators or even to upstream.Comment #45
kim.pepperHow about
UserSessionFinalizer
withfinalizeLogin()
andfinalizeLogout()
?Comment #46
andypostit sounds good idea, needs to check which services can get decorators to react of login/logout
Comment #47
andypostComment #49
larowlanLooks 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.
Comment #50
kim.pepperComment #51
smustgrave CreditAttribution: smustgrave at Mobomo commentedAppears 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.
Comment #52
alexpottI 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
Comment #53
kim.pepperRenamed to
UserSessionFinalizer
and made the service classfinal
.Comment #54
kim.pepperGetting this error in the test failure which I think is unrelated:
Looks like it was a random fail.
Comment #55
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #56
kim.pepperRebased on 11.x
Comment #57
andypostThank 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?
Comment #58
smustgrave CreditAttribution: smustgrave at Mobomo commentedAppears feedback has been addressed.
@andypost let me know if I'm wrong but sounds like in #57 you're asking a general question correct?
Comment #59
catchThe issue summary and change record no longer match the MR.
Comment #60
kim.pepperComment #61
kim.pepperUpdated 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.
Comment #62
quietone CreditAttribution: quietone at PreviousNext commentedI'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.
Comment #63
quietone CreditAttribution: quietone at PreviousNext commentedI 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.
Comment #64
quietone CreditAttribution: quietone at PreviousNext commentedI did not update the change record. Happy to do that if folks agree to the name change.
Comment #65
smustgrave CreditAttribution: smustgrave at Mobomo commentedI think Finalize makes sense too. I updated the CR and issue summary to reflect that.
Comment #66
catchThere 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.
Comment #67
longwave@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?
Comment #68
znerol CreditAttribution: znerol commentedI was thinking the same.
Comment #69
Berdir> @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.
Comment #70
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo this is something that should still move forward?
Comment #71
catchI 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.
Comment #72
cmlaraSpeaking 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.
Comment #73
solideogloria CreditAttribution: solideogloria commentedGlad to see there's an issue for this. I was going to open an issue if there wasn't one already.
Comment #74
smustgrave CreditAttribution: smustgrave at Mobomo commentedOnly moving to NW for the open threads from @catch on the MR.