From 1ba5cf0a249ef17b835413ab495a280af1c8e932 Mon Sep 17 00:00:00 2001
From: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Date: Thu, 4 Jun 2026 13:33:03 +0200
Subject: [PATCH] fix: avoid TypeError in when session user is null

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
---
 lib/Listener/UserLoggedInListener.php |  9 ++-
 lib/RestrictionManager.php            | 19 ++++--
 tests/unit/RestrictionManagerTest.php | 96 +++++++++++++++++++++++++++
 3 files changed, 118 insertions(+), 6 deletions(-)
 create mode 100644 tests/unit/RestrictionManagerTest.php

diff --git a/lib/Listener/UserLoggedInListener.php b/lib/Listener/UserLoggedInListener.php
index c488dbb43..82fb5672e 100644
--- a/lib/Listener/UserLoggedInListener.php
+++ b/lib/Listener/UserLoggedInListener.php
@@ -30,7 +30,12 @@ public function handle(Event $event): void {
 			return;
 		}
 
-		$this->restrictionManager->verifyAccess();
-		$this->restrictionManager->setupRestrictions();
+		// Use the user from the event rather than the user session: when the
+		// event is dispatched from the user_saml ACS handler the user is not
+		// yet bound to the session, so IUserSession::getUser() returns null
+		// (see https://github.com/nextcloud/guests/issues/1588).
+		$user = $event->getUser();
+		$this->restrictionManager->verifyAccess($user);
+		$this->restrictionManager->setupRestrictions($user);
 	}
 }
diff --git a/lib/RestrictionManager.php b/lib/RestrictionManager.php
index 17184bb90..c004d4559 100644
--- a/lib/RestrictionManager.php
+++ b/lib/RestrictionManager.php
@@ -38,12 +38,23 @@ public function __construct(
 	) {
 	}
 
-	public function verifyAccess(): void {
-		$this->whitelist->verifyAccess($this->userSession->getUser(), $this->request);
+	public function verifyAccess(?IUser $user = null): void {
+		$user ??= $this->userSession->getUser();
+
+		if ($user === null) {
+			// Nothing to verify without a user. The session is not always
+			// populated when this runs (e.g. the user_saml ACS handler
+			// dispatches UserLoggedInEvent before binding the user to the
+			// session), so callers should pass the user explicitly; guard
+			// here to avoid forwarding null to the non-nullable whitelist.
+			return;
+		}
+
+		$this->whitelist->verifyAccess($user, $this->request);
 	}
 
-	public function setupRestrictions(): void {
-		$user = $this->userSession->getUser();
+	public function setupRestrictions(?IUser $user = null): void {
+		$user ??= $this->userSession->getUser();
 
 		if ($user === null) {
 			// No user logged in, no restrictions needed
diff --git a/tests/unit/RestrictionManagerTest.php b/tests/unit/RestrictionManagerTest.php
new file mode 100644
index 000000000..15c97372d
--- /dev/null
+++ b/tests/unit/RestrictionManagerTest.php
@@ -0,0 +1,96 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Guests\Test\Unit;
+
+use OCA\Guests\AppWhitelist;
+use OCA\Guests\Config;
+use OCA\Guests\GuestManager;
+use OCA\Guests\RestrictionManager;
+use OCA\Guests\UserBackend;
+use OCP\Files\Config\IMountProviderCollection;
+use OCP\IRequest;
+use OCP\IServerContainer;
+use OCP\IUser;
+use OCP\IUserSession;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
+use Test\TestCase;
+
+class RestrictionManagerTest extends TestCase {
+	private AppWhitelist&MockObject $whitelist;
+
+	private IRequest&MockObject $request;
+
+	private IUserSession&MockObject $userSession;
+
+	private RestrictionManager $restrictionManager;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->whitelist = $this->createMock(AppWhitelist::class);
+		$this->request = $this->createMock(IRequest::class);
+		$this->userSession = $this->createMock(IUserSession::class);
+
+		$this->restrictionManager = new RestrictionManager(
+			$this->whitelist,
+			$this->request,
+			$this->userSession,
+			$this->createMock(IServerContainer::class),
+			$this->createMock(GuestManager::class),
+			$this->createMock(IMountProviderCollection::class),
+			$this->createMock(Config::class),
+			$this->createMock(UserBackend::class),
+			$this->createMock(LoggerInterface::class),
+		);
+	}
+
+	/**
+	 * Regression test for https://github.com/nextcloud/guests/issues/1588:
+	 * when the UserLoggedInEvent is dispatched from the user_saml ACS handler
+	 * the user is not yet bound to the session, so falling back to
+	 * IUserSession::getUser() yields null. verifyAccess() must not forward that
+	 * null to AppWhitelist::verifyAccess() (which is typed as non-nullable).
+	 */
+	public function testVerifyAccessIsSkippedWhenNoUser(): void {
+		$this->userSession->method('getUser')
+			->willReturn(null);
+
+		$this->whitelist->expects($this->never())
+			->method('verifyAccess');
+
+		$this->restrictionManager->verifyAccess();
+	}
+
+	public function testVerifyAccessUsesSessionUserWhenNoArgumentGiven(): void {
+		$user = $this->createMock(IUser::class);
+		$this->userSession->method('getUser')
+			->willReturn($user);
+
+		$this->whitelist->expects($this->once())
+			->method('verifyAccess')
+			->with($user, $this->request);
+
+		$this->restrictionManager->verifyAccess();
+	}
+
+	public function testVerifyAccessPrefersExplicitUserOverSession(): void {
+		// The listener passes the user from the event; the session getUser()
+		// must not even be consulted in that case.
+		$eventUser = $this->createMock(IUser::class);
+		$this->userSession->expects($this->never())
+			->method('getUser');
+
+		$this->whitelist->expects($this->once())
+			->method('verifyAccess')
+			->with($eventUser, $this->request);
+
+		$this->restrictionManager->verifyAccess($eventUser);
+	}
+}