Skip to content

Commit a61ddf6

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 a61ddf6

3 files changed

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

0 commit comments

Comments
 (0)