Harmonize user session creation
Nobody has claimed this yet.
- Dominant language
- PHP
- Stars
- 36.9k
- Forks
- 5.2k
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 713
Description
Context
While implementing Oidc support in global scale, we checked how Saml was supported. The GSS master node actually listens to OCP\User\Events\BeforeUserLoggedInEvent. This event is emitted when authenticating via user_saml because the user session is created by user_saml's IApacheBackend user backend. This can be considered as a trick as a user backend should implement IApacheBackend only to deal with authentication during API requests using headers or session variables to get credentials.
The user_oidc app has a different approach to create the user session on login, it manipulates the user session:
$this->userSession->setUser($user);
$this->userSession->completeLogin($user, ['loginName' => $user->getUID(), 'password' => '']);
$this->userSession->createSessionToken($this->request, $user->getUID(), $user->getUID());
$this->userSession->createRememberMeToken($user);
This actually does not dispatch OCP\User\Events\BeforeUserLoggedInEvent so we had to manually dispatch it.
Suggestion
It would be nice to find a way to make sure all the apps implementing a user backend emit the correct events and complete the login process correctly. Event dispatching could be moved somewhere in the IUserSession implementation for example.
The goal would be to prevent breaking apps that took OC_User::loginWithApache as inspiration to implement their login logic when there is a change in the server.
cc @juliusknorr
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Compare the login paths in user_saml and user_oidc, then read OC_User::loginWithApache and the IUserSession implementation. Determine where session completion and BeforeUserLoggedInEvent dispatching should be unified, while preserving compatibility for apps based on loginWithApache; the issue is done when all user-backend login flows complete consistently and emit the correct event.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- authentication, backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100