From 179f6c27f166261007ce102cad9b526cf23eba77 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Wed, 10 Jun 2026 14:31:06 +0200 Subject: [PATCH 1/2] fix: prevent crash when SAML provider is not configured When user_saml is enabled (type=saml) but no IdP exists in the database, navigating to the login page caused a TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given in SAMLSettings.php line 147 Root cause: Application.php beforeware calls getListOfIdps() which sets LOADED_ALL on SAMLSettings. When SAMLController::login() then calls getOneLoginSettingsArray(1), ensureConfigurationsLoaded(1) returns early due to LOADED_ALL, leaving $this->configurations[1] undefined (null). The array_key_exists() call on null then throws a TypeError. Two fixes in SAMLSettings::getOneLoginSettingsArray(): - Replace the lone array_key_exists() call (line 156) with ?? '' to be consistent with every other field access in the method. - Add an early guard that throws InvalidArgumentException when the requested IdP is not in $configurations, converting a cryptic null-pointer into a clear error message. SAMLController::login() catches InvalidArgumentException and redirects to the existing genericError page with a translatable user-readable message instead of letting the exception propagate as a 500. Tests added: - SAMLSettingsTest: reproduces the exact LOADED_ALL bug scenario, tests the simple not-in-db case, happy path, and sp-entityId fallback. - SAMLControllerTest: verifies the controller redirects to genericError when getOneLoginSettingsArray() throws InvalidArgumentException. Signed-off-by: Misha M.-Kupriyanov --- lib/Controller/SAMLController.php | 12 ++- lib/SAMLSettings.php | 8 +- tests/unit/Controller/SAMLControllerTest.php | 27 ++++++ tests/unit/SAMLSettingsTest.php | 97 ++++++++++++++++++++ 4 files changed, 142 insertions(+), 2 deletions(-) diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php index 41bc6e533..f05dd6a8e 100644 --- a/lib/Controller/SAMLController.php +++ b/lib/Controller/SAMLController.php @@ -161,7 +161,16 @@ public function login(int $idp = 1): Http\RedirectResponse|Http\TemplateResponse $type = $this->appConfig->getAppValueString('type'); switch ($type) { case 'saml': - $settings = $this->samlSettings->getOneLoginSettingsArray($idp); + try { + $settings = $this->samlSettings->getOneLoginSettingsArray($idp); + } catch (\InvalidArgumentException $e) { + return new Http\RedirectResponse( + $this->urlGenerator->linkToRouteAbsolute( + 'user_saml.SAML.genericError', + ['reason' => 'notConfigured'] + ) + ); + } $auth = new Auth($settings); $passthroughParamsString = trim($settings['idp']['passthroughParameters'] ?? '') ; $passthroughParams = array_map(trim(...), explode(',', $passthroughParamsString)); @@ -555,6 +564,7 @@ public function genericError(string $reason): Http\TemplateResponse { $allowedMessages = [ 'userDisabled' => $this->l->t('This user account is disabled, please contact your administrator.'), 'authFailed' => $this->l->t('Authentication failed.'), + 'notConfigured' => $this->l->t('SAML authentication is not configured. Please ask your administrator to complete the SAML setup in the admin panel.'), ]; $message = $allowedMessages[$reason] ?? $this->l->t('Unknown error, please check the log file for more details.'); diff --git a/lib/SAMLSettings.php b/lib/SAMLSettings.php index 1877ee4b5..a13eae48e 100644 --- a/lib/SAMLSettings.php +++ b/lib/SAMLSettings.php @@ -131,6 +131,12 @@ public function usesSloWebServerDecode(int $idp): bool { public function getOneLoginSettingsArray(int $idp): array { $this->ensureConfigurationsLoaded($idp); + if (($this->configurations[$idp] ?? []) === []) { + throw new InvalidArgumentException( + "SAML provider #{$idp} does not exist or has not been configured yet." + ); + } + $settings = [ 'strict' => true, 'debug' => $this->config->getSystemValueBool('debug'), @@ -153,7 +159,7 @@ public function getOneLoginSettingsArray(int $idp): array { // "sloWebServerDecode" is not expected to be passed to the OneLogin class ], 'sp' => [ - 'entityId' => (array_key_exists('sp-entityId', $this->configurations[$idp]) && trim($this->configurations[$idp]['sp-entityId']) != '') + 'entityId' => (trim($this->configurations[$idp]['sp-entityId'] ?? '') !== '') ? $this->configurations[$idp]['sp-entityId'] : $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.getMetadata'), 'assertionConsumerService' => [ diff --git a/tests/unit/Controller/SAMLControllerTest.php b/tests/unit/Controller/SAMLControllerTest.php index fd2a3e143..0e8b8e2f0 100644 --- a/tests/unit/Controller/SAMLControllerTest.php +++ b/tests/unit/Controller/SAMLControllerTest.php @@ -421,4 +421,31 @@ public function testUserFilterNotApplicable(): void { $this->invokePrivate($this->samlController, 'assertGroupMemberships'); } + + public function testLoginWithUnconfiguredIdpRedirectsToGenericError(): void { + $this->appConfig->expects($this->once()) + ->method('getAppValueString') + ->with('type') + ->willReturn('saml'); + + $this->request->expects($this->any()) + ->method('getParam') + ->willReturn(''); + + $this->samlSettings->expects($this->once()) + ->method('getOneLoginSettingsArray') + ->with(1) + ->willThrowException(new \InvalidArgumentException('SAML provider #1 does not exist')); + + $errorUrl = 'https://example.com/error'; + $this->urlGenerator->expects($this->once()) + ->method('linkToRouteAbsolute') + ->with('user_saml.SAML.genericError', ['reason' => 'notConfigured']) + ->willReturn($errorUrl); + + $result = $this->samlController->login(1); + + $this->assertInstanceOf(RedirectResponse::class, $result); + $this->assertEquals($errorUrl, $result->getRedirectURL()); + } } diff --git a/tests/unit/SAMLSettingsTest.php b/tests/unit/SAMLSettingsTest.php index 3aa00d2cb..ad3402eff 100644 --- a/tests/unit/SAMLSettingsTest.php +++ b/tests/unit/SAMLSettingsTest.php @@ -124,4 +124,101 @@ public function testGetListOfIdps(bool $onlyComplete, array $configs, array $exp $this->assertSame($expected, $result); } + + /** + * Reproduces the exact crash: Application.php beforeware calls getListOfIdps() which + * sets LOADED_ALL on an empty DB. SAMLController::login() then calls + * getOneLoginSettingsArray(1). ensureConfigurationsLoaded(1) returns early due to + * LOADED_ALL, leaving $this->configurations[1] undefined (null). Without our guard + * this crashes with array_key_exists(): Argument #2 must be of type array, null given. + */ + public function testGetOneLoginSettingsArrayThrowsForMissingIdpAfterLoadAll(): void { + $this->mapper->expects($this->once()) + ->method('getAll') + ->willReturn([]); + + // Simulates Application.php beforeware — sets LOADED_ALL with empty DB + $this->samlSettings->getListOfIdps(); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('SAML provider #1 does not exist'); + $this->samlSettings->getOneLoginSettingsArray(1); + } + + public function testGetOneLoginSettingsArrayThrowsWhenIdpNotInDb(): void { + $this->mapper->expects($this->once()) + ->method('get') + ->with(42) + ->willReturn([]); + + $this->expectException(\InvalidArgumentException::class); + $this->samlSettings->getOneLoginSettingsArray(42); + } + + public function testGetOneLoginSettingsArrayReturnsStructuredArray(): void { + $this->mapper->method('get') + ->with(1) + ->willReturn([ + 'idp-entityId' => 'urn:example:idp', + 'idp-singleSignOnService.url' => 'https://idp.example.com/sso', + 'idp-x509cert' => 'CERTDATA', + 'sp-entityId' => 'urn:example:sp', + ]); + + $this->config->method('getSystemValueBool')->willReturn(false); + $this->urlGenerator->method('linkToRouteAbsolute')->willReturn('https://example.com/route'); + + $result = $this->samlSettings->getOneLoginSettingsArray(1); + + $this->assertArrayHasKey('sp', $result); + $this->assertArrayHasKey('idp', $result); + $this->assertArrayHasKey('security', $result); + $this->assertEquals('urn:example:sp', $result['sp']['entityId']); + $this->assertEquals('urn:example:idp', $result['idp']['entityId']); + $this->assertEquals('https://idp.example.com/sso', $result['idp']['singleSignOnService']['url']); + } + + public function testGetOneLoginSettingsArraySpEntityIdFallsBackToMetadataUrl(): void { + $this->mapper->method('get') + ->with(1) + ->willReturn([ + 'idp-entityId' => 'urn:example:idp', + 'idp-singleSignOnService.url' => 'https://idp.example.com/sso', + // sp-entityId intentionally absent + ]); + + $this->config->method('getSystemValueBool')->willReturn(false); + $this->urlGenerator->method('linkToRouteAbsolute') + ->willReturnMap([ + ['user_saml.SAML.getMetadata', [], 'https://example.com/metadata'], + ['user_saml.SAML.base', [], 'https://example.com/base'], + ['user_saml.SAML.assertionConsumerService', [], 'https://example.com/acs'], + ]); + + $result = $this->samlSettings->getOneLoginSettingsArray(1); + + $this->assertEquals('https://example.com/metadata', $result['sp']['entityId']); + } + + public function testGetOneLoginSettingsArraySpWhitespaceEntityIdFallsBackToMetadataUrl(): void { + $this->mapper->method('get') + ->with(1) + ->willReturn([ + 'idp-entityId' => 'urn:example:idp', + 'idp-singleSignOnService.url' => 'https://idp.example.com/sso', + 'sp-entityId' => ' ', // whitespace-only must also fall back + ]); + + $this->config->method('getSystemValueBool')->willReturn(false); + $this->urlGenerator->method('linkToRouteAbsolute') + ->willReturnMap([ + ['user_saml.SAML.getMetadata', [], 'https://example.com/metadata'], + ['user_saml.SAML.base', [], 'https://example.com/base'], + ['user_saml.SAML.assertionConsumerService', [], 'https://example.com/acs'], + ]); + + $result = $this->samlSettings->getOneLoginSettingsArray(1); + + $this->assertEquals('https://example.com/metadata', $result['sp']['entityId']); + } } From 61d9a6eb8821905a3928d3abecc62b468a71f73b Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Wed, 10 Jun 2026 14:31:43 +0200 Subject: [PATCH 2/2] feat: add saml:config:validate occ command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new occ command that support teams and administrators can run to get an immediate diagnosis of the user_saml configuration state: occ saml:config:validate Exit codes: 0 — all configured IdPs pass required-field validation 1 — SAML type not set (app is not active) 2 — active but one or more required fields are missing The command checks for the three fields required for a working SAML login: idp-entityId, idp-singleSignOnService.url, general-uid_mapping. Useful for support diagnostics and CI/monitoring health checks. Signed-off-by: Misha M.-Kupriyanov --- appinfo/info.xml | 1 + lib/Command/ConfigValidate.php | 74 ++++++++++++ tests/unit/Command/ConfigValidateTest.php | 139 ++++++++++++++++++++++ 3 files changed, 214 insertions(+) create mode 100644 lib/Command/ConfigValidate.php create mode 100644 tests/unit/Command/ConfigValidateTest.php diff --git a/appinfo/info.xml b/appinfo/info.xml index fe8130a1a..76584fd82 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -57,6 +57,7 @@ While theoretically any other authentication provider implementing either one of OCA\User_SAML\Command\ConfigDelete OCA\User_SAML\Command\ConfigGet OCA\User_SAML\Command\ConfigSet + OCA\User_SAML\Command\ConfigValidate OCA\User_SAML\Command\GetMetadata OCA\User_SAML\Command\GroupMigrationCopyIncomplete OCA\User_SAML\Command\GroupMigrationManual diff --git a/lib/Command/ConfigValidate.php b/lib/Command/ConfigValidate.php new file mode 100644 index 000000000..7b66b7b2b --- /dev/null +++ b/lib/Command/ConfigValidate.php @@ -0,0 +1,74 @@ +setName('saml:config:validate'); + $this->setDescription('Check whether user_saml is correctly configured'); + parent::configure(); + } + + #[\Override] + protected function execute(InputInterface $input, OutputInterface $output): int { + $type = $this->appConfig->getAppValueString('type', ''); + if ($type === '') { + $output->writeln('user_saml is not active (type not set). Configure it via the admin panel.'); + return 1; + } + $output->writeln('Type: ' . $type . ''); + + $idps = $this->samlSettings->getListOfIdps(); + if (empty($idps)) { + $output->writeln('No IdP providers configured. Add one via the admin panel or occ saml:config:create.'); + return 2; + } + + $exitCode = 0; + foreach ($idps as $id => $name) { + $cfg = $this->samlSettings->get($id); + $missing = []; + foreach (self::REQUIRED_FIELDS as $field) { + if (empty($cfg[$field])) { + $missing[] = $field; + } + } + $label = "IdP #{$id}" . ($name !== '' ? " ({$name})" : ''); + if (empty($missing)) { + $output->writeln(" {$label}: OK"); + } else { + $output->writeln(" {$label}: MISSING: " . implode(', ', $missing) . ''); + $exitCode = 2; + } + } + + return $exitCode; + } +} diff --git a/tests/unit/Command/ConfigValidateTest.php b/tests/unit/Command/ConfigValidateTest.php new file mode 100644 index 000000000..ae3810d82 --- /dev/null +++ b/tests/unit/Command/ConfigValidateTest.php @@ -0,0 +1,139 @@ +samlSettings = $this->createMock(SAMLSettings::class); + $this->appConfig = $this->createMock(IAppConfig::class); + $this->command = new ConfigValidate($this->samlSettings, $this->appConfig); + } + + private function makeInput(): InputInterface { + $input = $this->createMock(InputInterface::class); + $input->method('getOption')->willReturn(null); + $input->method('getArgument')->willReturn(null); + return $input; + } + + public function testExitsWithCodeOneWhenTypeNotSet(): void { + $this->appConfig->expects($this->once()) + ->method('getAppValueString') + ->with('type', '') + ->willReturn(''); + + $output = $this->createMock(OutputInterface::class); + $output->expects($this->once()) + ->method('writeln') + ->with($this->stringContains('not active')); + + $exitCode = $this->invokePrivate($this->command, 'execute', [$this->makeInput(), $output]); + + $this->assertEquals(1, $exitCode); + } + + public function testExitsWithCodeTwoWhenNoIdpsConfigured(): void { + $this->appConfig->expects($this->once()) + ->method('getAppValueString') + ->with('type', '') + ->willReturn('saml'); + + $this->samlSettings->expects($this->once()) + ->method('getListOfIdps') + ->willReturn([]); + + $output = $this->createMock(OutputInterface::class); + $output->expects($this->exactly(2)) + ->method('writeln') + ->willReturnCallback(function (string $msg): void { + // just capture; specific assertions follow via mock constraints + }); + + $exitCode = $this->invokePrivate($this->command, 'execute', [$this->makeInput(), $output]); + + $this->assertEquals(2, $exitCode); + } + + public function testExitsZeroWhenFullyConfigured(): void { + $this->appConfig->method('getAppValueString') + ->with('type', '') + ->willReturn('saml'); + + $this->samlSettings->method('getListOfIdps') + ->willReturn([1 => 'My IdP']); + + $this->samlSettings->method('get') + ->with(1) + ->willReturn([ + 'idp-entityId' => 'https://idp.example.com', + 'idp-singleSignOnService.url' => 'https://idp.example.com/sso', + 'general-uid_mapping' => 'uid', + ]); + + $output = $this->createMock(OutputInterface::class); + $messages = []; + $output->method('writeln') + ->willReturnCallback(function (string $msg) use (&$messages): void { + $messages[] = $msg; + }); + + $exitCode = $this->invokePrivate($this->command, 'execute', [$this->makeInput(), $output]); + + $this->assertEquals(0, $exitCode); + $this->assertTrue( + count(array_filter($messages, fn (string $m) => str_contains($m, 'OK'))) > 0, + 'Expected at least one "OK" message' + ); + } + + public function testExitsWithCodeTwoWhenRequiredFieldsMissing(): void { + $this->appConfig->method('getAppValueString') + ->with('type', '') + ->willReturn('saml'); + + $this->samlSettings->method('getListOfIdps') + ->willReturn([1 => 'My IdP']); + + $this->samlSettings->method('get') + ->with(1) + ->willReturn([]); // all required fields missing + + $output = $this->createMock(OutputInterface::class); + $messages = []; + $output->method('writeln') + ->willReturnCallback(function (string $msg) use (&$messages): void { + $messages[] = $msg; + }); + + $exitCode = $this->invokePrivate($this->command, 'execute', [$this->makeInput(), $output]); + + $this->assertEquals(2, $exitCode); + + $errorMessages = implode(' ', $messages); + $this->assertStringContainsString('idp-entityId', $errorMessages); + $this->assertStringContainsString('idp-singleSignOnService.url', $errorMessages); + $this->assertStringContainsString('general-uid_mapping', $errorMessages); + } +}