From 7287ad54a83caf0ed7ad3ebe878f61843cde620a Mon Sep 17 00:00:00 2001 From: mostafa Date: Thu, 6 Aug 2026 17:32:21 +0330 Subject: [PATCH 1/2] fix: don't set session user in getCurrentUserId Reverts the setSessionUser() calls added in #1376. IApacheBackend::getCurrentUserId() is called from the first line of loginWithApache(), which guards its whole login block on the active user not already being set. Setting the session user inside getCurrentUserId() satisfies that guard before core reaches it, so the entire block gets skipped: no oc_authtoken row, no remember-me cookie, no filesystem setup, no login events. The missing token row breaks any later request that reuses the session cookie. Session::validateSession() looks up a token by session id, finds none, and calls logout(), which strips the cookie and returns a 401. Confirmed independently against master and 8.11.0-dev: bearer request then cookie-only request goes 200 then 401 with the calls in place, 200 then 200 with them removed. DAV also reaches this code through apps/dav's own handleApacheAuth() call site, confirmed 207 both before and after. Signed-off-by: mostafa Co-Authored-By: Claude Sonnet 5 --- lib/User/Backend.php | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/lib/User/Backend.php b/lib/User/Backend.php index e06aee48..87d392b4 100644 --- a/lib/User/Backend.php +++ b/lib/User/Backend.php @@ -37,7 +37,6 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; -use OCP\IUserSession; use OCP\Server; use OCP\User\Backend\ABackend; use OCP\User\Backend\ICountUsersBackend; @@ -369,12 +368,10 @@ public function getCurrentUserId(): string { } $this->session->set('last-password-confirm', $this->timeFactory->getTime() + 4 * 365 * 24 * 3600); - $this->setSessionUser($userId); return $userId; } elseif ($this->userExists($tokenUserId)) { $this->checkFirstLogin($tokenUserId); $this->session->set('last-password-confirm', $this->timeFactory->getTime() + 4 * 365 * 24 * 3600); - $this->setSessionUser($tokenUserId); return $tokenUserId; } else { // check if the user exists locally @@ -396,7 +393,6 @@ public function getCurrentUserId(): string { } $this->checkFirstLogin($tokenUserId); $this->session->set('last-password-confirm', $this->timeFactory->getTime() + 4 * 365 * 24 * 3600); - $this->setSessionUser($tokenUserId); return $tokenUserId; } } @@ -417,31 +413,6 @@ private function isAcceptableUserId(mixed $userId): bool { return is_string($userId) && $userId !== '' && trim($userId) !== ''; } - /** - * Set the user in IUserSession after bearer token validation. - * Without this, DI-injected $userId is null in OCS controllers - * and CalDAV plugins, causing 500 errors in Deck, Talk, and Tasks. - * - * Note: IUserSession is resolved via Server::get() rather than constructor - * injection to avoid a circular dependency (IUserSession depends on this Backend). - */ - private function setSessionUser(string $userId): void { - try { - $userSession = Server::get(IUserSession::class); - $currentUser = $userSession->getUser(); - - // Only fetch and set if the session doesn't already have this user - if ($currentUser === null || $currentUser->getUID() !== $userId) { - $user = $this->userManager->get($userId); - if ($user !== null) { - $userSession->setUser($user); - } - } - } catch (\Throwable $e) { - $this->logger->debug('Failed to set session user after bearer validation: ' . $e->getMessage()); - } - } - /** * * Performs first-login initialisation (home folder setup, skeleton copy, events) From 54a4a6245fe3b2262c754a6baf4a95df2b5451df Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Tue, 11 Aug 2026 10:31:12 +0100 Subject: [PATCH 2/2] test: cover the bearer path not setting the session user getCurrentUserId() runs as the first statement of OC_User::loginWithApache(), which guards its whole login block on the session user not being set yet. IUserSession::setUser() persists 'user_id', the key OC_User::getUser() reads, so setting the session user from inside getCurrentUserId() closed that guard and left the request without an oc_authtoken row. Cover the three bearer return paths: each must resolve the user id without IUserSession::setUser() being called and without 'user_id' reaching the session. setVolatileActiveUser() is deliberately not covered, it does not persist 'user_id' and so does not close the guard. Fails on the three call sites removed here, passes without them. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- tests/unit/User/BackendTest.php | 233 ++++++++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) create mode 100644 tests/unit/User/BackendTest.php diff --git a/tests/unit/User/BackendTest.php b/tests/unit/User/BackendTest.php new file mode 100644 index 00000000..8045da60 --- /dev/null +++ b/tests/unit/User/BackendTest.php @@ -0,0 +1,233 @@ +getCurrentUserId(); + * if ($uid) { + * if (self::getUser() !== $uid) { // <- closed if we set the user above + * self::setUserId($uid); + * ... + * $userSession->createSessionToken($request, $uid, $uid, $password); + * + * OC_User::getUser() reads the session key 'user_id', which is exactly what + * IUserSession::setUser() writes. So setting the session user from inside + * getCurrentUserId() closes that guard and no oc_authtoken row is ever written for + * the request, which breaks every endpoint that needs a session token afterwards. + * + * The backend must therefore only *resolve* the user and leave logging them in to + * the server. Note that setVolatileActiveUser() is deliberately not covered here: + * it does not persist 'user_id' and so does not close the guard. + * + * @see https://github.com/nextcloud/user_oidc/issues/1452 + * + * Extends \Test\TestCase (not PHPUnit's) for overwriteService(), because the backend + * resolves IUserSession and the token validators through Server::get(). + */ +class BackendTest extends \Test\TestCase { + private const TOKEN_USER_ID = 'oidc-token-user'; + private const PROVIDER_ID = 1; + + private IConfig&MockObject $config; + private UserMapper&MockObject $userMapper; + private LoggerInterface&MockObject $logger; + private IRequest&MockObject $request; + private ISession&MockObject $session; + private IURLGenerator&MockObject $urlGenerator; + private IEventDispatcher&MockObject $eventDispatcher; + private DiscoveryService&MockObject $discoveryService; + private ProviderMapper&MockObject $providerMapper; + private ProviderService&MockObject $providerService; + private ProvisioningService&MockObject $provisioningService; + private LdapService&MockObject $ldapService; + private IUserManager&MockObject $userManager; + private ITimeFactory&MockObject $timeFactory; + + private Backend $backend; + + protected function setUp(): void { + parent::setUp(); + + $this->config = $this->createMock(IConfig::class); + $this->userMapper = $this->createMock(UserMapper::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->request = $this->createMock(IRequest::class); + $this->session = $this->createMock(ISession::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->discoveryService = $this->createMock(DiscoveryService::class); + $this->providerMapper = $this->createMock(ProviderMapper::class); + $this->providerService = $this->createMock(ProviderService::class); + $this->provisioningService = $this->createMock(ProvisioningService::class); + $this->ldapService = $this->createMock(LdapService::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->timeFactory = $this->createMock(ITimeFactory::class); + + $this->backend = new Backend( + $this->config, + $this->userMapper, + $this->logger, + $this->request, + $this->session, + $this->urlGenerator, + $this->eventDispatcher, + $this->discoveryService, + $this->providerMapper, + $this->providerService, + $this->provisioningService, + $this->ldapService, + $this->userManager, + $this->timeFactory, + ); + } + + /** + * Auto-provisioning enabled (the default): the user already exists, so it is + * reused instead of being created. + */ + public function testBearerAuthWithAutoProvisioningDoesNotLogTheUserIn(): void { + $this->givenAValidBearerToken(); + + $user = $this->givenAUserThatHasLoggedInBefore(self::TOKEN_USER_ID); + $this->userManager->method('userExists')->with(self::TOKEN_USER_ID)->willReturn(true); + $this->userManager->method('get')->with(self::TOKEN_USER_ID)->willReturn($user); + $this->ldapService->method('isLdapDeletedUser')->with($user)->willReturn(false); + + $this->assertResolvesUserWithoutLoggingIn(self::TOKEN_USER_ID); + } + + /** + * Auto-provisioning disabled and the user is known to this backend. + */ + public function testBearerAuthWithoutAutoProvisioningDoesNotLogTheUserIn(): void { + $this->givenAValidBearerToken(['auto_provision' => false]); + + $user = $this->givenAUserThatHasLoggedInBefore(self::TOKEN_USER_ID); + $this->userMapper->method('userExists')->with(self::TOKEN_USER_ID)->willReturn(true); + $this->userManager->method('get')->with(self::TOKEN_USER_ID)->willReturn($user); + + $this->assertResolvesUserWithoutLoggingIn(self::TOKEN_USER_ID); + } + + /** + * Auto-provisioning disabled and the user lives in another backend (for + * instance synced from LDAP). + */ + public function testBearerAuthForUserOfAnotherBackendDoesNotLogTheUserIn(): void { + $this->givenAValidBearerToken(['auto_provision' => false]); + + $user = $this->givenAUserThatHasLoggedInBefore(self::TOKEN_USER_ID); + $this->userMapper->method('userExists')->with(self::TOKEN_USER_ID)->willReturn(false); + $this->userManager->method('userExists')->with(self::TOKEN_USER_ID)->willReturn(true); + $this->userManager->method('get')->with(self::TOKEN_USER_ID)->willReturn($user); + $this->ldapService->method('isLdapDeletedUser')->with($user)->willReturn(false); + + $this->assertResolvesUserWithoutLoggingIn(self::TOKEN_USER_ID); + } + + /** + * Wires up a request carrying a bearer token that the self encoded validator + * accepts for a provider with bearer checking turned on. + * + * @param array $systemConfig the 'user_oidc' system config + */ + private function givenAValidBearerToken(array $systemConfig = []): void { + $this->config->method('getSystemValue')->with('user_oidc', [])->willReturn($systemConfig); + $this->request->method('getHeader') + ->with(Application::OIDC_API_REQ_HEADER) + ->willReturn('Bearer a-valid-token'); + + $provider = new Provider(); + $provider->setId(self::PROVIDER_ID); + $provider->setIdentifier('test-provider'); + $this->providerMapper->method('getProviders')->willReturn([$provider]); + + $this->providerService->method('getSetting')->willReturnMap([ + [self::PROVIDER_ID, ProviderService::SETTING_CHECK_BEARER, '0', '1'], + [self::PROVIDER_ID, ProviderService::SETTING_RESTRICT_LOGIN_TO_GROUPS, '0', '0'], + [self::PROVIDER_ID, ProviderService::SETTING_BEARER_PROVISIONING, '0', '0'], + ]); + + $this->discoveryService->method('obtainDiscovery')->willReturn([]); + $this->timeFactory->method('getTime')->willReturn(1700000000); + + $validator = $this->createMock(SelfEncodedValidator::class); + $validator->method('isValidBearerToken')->willReturn(self::TOKEN_USER_ID); + $this->overwriteService(SelfEncodedValidator::class, $validator); + } + + private function givenAUserThatHasLoggedInBefore(string $uid): IUser&MockObject { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + // a non-zero last login keeps checkFirstLogin() away from the filesystem + $user->method('getLastLogin')->willReturn(1600000000); + $user->method('getBackendClassName')->willReturn(Application::APP_ID); + + return $user; + } + + /** + * The regression: the bearer token must resolve to $expectedUserId without any + * of it landing in the user session, so that OC_User::loginWithApache() still + * runs createSessionToken() and the rest of the login. + */ + private function assertResolvesUserWithoutLoggingIn(string $expectedUserId): void { + $userSession = $this->createMock(IUserSession::class); + $userSession->expects($this->never()) + ->method('setUser'); + $this->overwriteService(IUserSession::class, $userSession); + // without this the never() expectation above would hold vacuously and the + // test would stay green even with the session user being set + $this->assertSame( + $userSession, + Server::get(IUserSession::class), + 'the IUserSession mock did not replace the real service' + ); + + $this->session->method('set')->willReturnCallback( + function (string $key, mixed $value): void { + $this->assertNotSame( + 'user_id', + $key, + 'getCurrentUserId() must not put the user into the session: OC_User::loginWithApache() ' + . 'then skips createSessionToken() and the request gets no auth token.' + ); + } + ); + + $this->assertSame($expectedUserId, $this->backend->getCurrentUserId()); + } +}