Skip to content

Commit 53c26b1

Browse files
committed
fix: Avoid redirecting to login with an incomplete configuration
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
1 parent 7d86211 commit 53c26b1

3 files changed

Lines changed: 140 additions & 2 deletions

File tree

lib/AppInfo/Application.php

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,13 @@ public function boot(IBootContext $context): void {
167167
}
168168

169169
$multipleUserBackEnds = $samlSettings->allowMultipleUserBackEnds();
170-
$configuredIdps = $samlSettings->getListOfIdps();
170+
$configuredIdps = $samlSettings->getListOfIdps(true);
171+
// If no IdP has the minimum required config (entityId + SSO URL), fall through to normal login
172+
// only for regular SAML mode. Environment-variable mode can still redirect to SAMLController::login()
173+
// without requiring configured IdP metadata.
174+
if ($type === 'saml' && empty($configuredIdps)) {
175+
return;
176+
}
171177
$showLoginOptions = $type !== 'environment-variable' && ($multipleUserBackEnds || count($configuredIdps) > 1);
172178

173179
if ($redirectSituation === true && $showLoginOptions) {

lib/SAMLSettings.php

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,17 @@ public function __construct(
9090
* @return array<int, string>
9191
* @throws Exception
9292
*/
93-
public function getListOfIdps(): array {
93+
public function getListOfIdps(bool $onlyComplete = false): array {
9494
$this->ensureConfigurationsLoaded();
9595

9696
$result = [];
9797
foreach ($this->configurations as $configID => $config) {
98+
if ($onlyComplete
99+
&& (trim((string)($config['idp-entityId'] ?? '')) === ''
100+
|| trim((string)($config['idp-singleSignOnService.url'] ?? '')) === '')
101+
) {
102+
continue;
103+
}
98104
// no fancy array_* method, because there might be thousands
99105
$result[$configID] = $config['general-idp0_display_name'] ?? '';
100106
}

tests/unit/SAMLSettingsTest.php

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\User_SAML\Tests;
11+
12+
use OCA\User_SAML\Db\ConfigurationsMapper;
13+
use OCA\User_SAML\SAMLSettings;
14+
use OCP\AppFramework\Services\IAppConfig;
15+
use OCP\IConfig;
16+
use OCP\ISession;
17+
use OCP\IURLGenerator;
18+
use PHPUnit\Framework\Attributes\DataProvider;
19+
use PHPUnit\Framework\MockObject\MockObject;
20+
use Test\TestCase;
21+
22+
class SAMLSettingsTest extends TestCase {
23+
private IURLGenerator&MockObject $urlGenerator;
24+
private IConfig&MockObject $config;
25+
private IAppConfig&MockObject $appConfig;
26+
private ISession&MockObject $session;
27+
private ConfigurationsMapper&MockObject $mapper;
28+
private SAMLSettings $samlSettings;
29+
30+
protected function setUp(): void {
31+
parent::setUp();
32+
33+
$this->urlGenerator = $this->createMock(IURLGenerator::class);
34+
$this->config = $this->createMock(IConfig::class);
35+
$this->appConfig = $this->createMock(IAppConfig::class);
36+
$this->session = $this->createMock(ISession::class);
37+
$this->mapper = $this->createMock(ConfigurationsMapper::class);
38+
39+
$this->samlSettings = new SAMLSettings(
40+
$this->urlGenerator,
41+
$this->config,
42+
$this->appConfig,
43+
$this->session,
44+
$this->mapper,
45+
);
46+
}
47+
48+
private static function dataGetListOfIdps(): array {
49+
return [
50+
'empty-all' => [
51+
false,
52+
[],
53+
[],
54+
],
55+
'empty-complete' => [
56+
true,
57+
[],
58+
[],
59+
],
60+
'entityId-and-ssoUrl-missing' => [
61+
true,
62+
[
63+
1 => [
64+
'general-idp0_display_name' => 'My IdP',
65+
// no idp-entityId, no idp-singleSignOnService.url
66+
],
67+
],
68+
[],
69+
],
70+
'only-whitespace' => [
71+
true,
72+
[
73+
1 => [
74+
'general-idp0_display_name' => 'My IdP',
75+
'idp-entityId' => ' ',
76+
'idp-singleSignOnService.url' => "\t",
77+
],
78+
],
79+
[],
80+
],
81+
'configured' => [
82+
true,
83+
[
84+
1 => [
85+
'general-idp0_display_name' => 'My IdP',
86+
'idp-entityId' => 'https://idp.example.com',
87+
'idp-singleSignOnService.url' => 'https://idp.example.com/sso',
88+
],
89+
],
90+
[1 => 'My IdP'],
91+
],
92+
'partially-configured' => [
93+
true,
94+
[
95+
1 => [
96+
'general-idp0_display_name' => 'Configured IdP',
97+
'idp-entityId' => 'https://idp.example.com',
98+
'idp-singleSignOnService.url' => 'https://idp.example.com/sso',
99+
],
100+
2 => [
101+
'general-idp0_display_name' => 'Missing SSO URL',
102+
'idp-entityId' => 'https://idp2.example.com',
103+
// missing idp-singleSignOnService.url
104+
],
105+
3 => [
106+
'general-idp0_display_name' => 'Missing Entity ID',
107+
// missing idp-entityId
108+
'idp-singleSignOnService.url' => 'https://idp3.example.com/sso',
109+
],
110+
],
111+
[1 => 'Configured IdP'],
112+
],
113+
];
114+
}
115+
116+
#[DataProvider('dataGetListOfIdps')]
117+
public function testGetListOfIdps(bool $onlyComplete, array $configs, array $expected): void {
118+
$this->mapper->expects($this->once())
119+
->method('getAll')
120+
->willReturn($configs);
121+
122+
$result = $this->samlSettings->getListOfIdps($onlyComplete);
123+
124+
$this->assertSame($expected, $result);
125+
}
126+
}

0 commit comments

Comments
 (0)