nextcloud / nextcloud/server

Harmonize user session creation

Open
#50,194 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

♻️ refactor 0. Needs triage feature: authentication technical debt
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.