From 8c9287339e986eb9a764e25f017738074945e82f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Wed, 17 Jun 2026 23:53:09 +0200 Subject: [PATCH 1/3] fix: read OIDC Prompt from the Prompt key, not the Ports key (#12557) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AppConfig::loadOpenIdConfigFromSystemConfig() read the OpenID Connect Prompt value from OidcPortsKey instead of OidcPromptKey. As a result an administrator-provided Prompt in the system configuration was ignored and prompt was instead populated with the raw Ports string (e.g. "8080,8443"), which is not a valid OIDC prompt parameter. Since a valid system OIDC config fully replaces the theme config, the configured prompt was unreachable. Read prompt from OidcPromptKey, and add a regression test that exercises loadOpenIdConfigFromSystemConfig() and asserts prompt, ports and scopes are each sourced from their own key. The method is made public (mirroring configPath) so it is reachable from the test. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- changelog/unreleased/12557.md | 9 +++++++++ src/libsync/config/appconfig.cpp | 2 +- src/libsync/config/appconfig.h | 9 ++++++++- test/testappconfig.cpp | 30 ++++++++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 changelog/unreleased/12557.md diff --git a/changelog/unreleased/12557.md b/changelog/unreleased/12557.md new file mode 100644 index 00000000000..4d380ba6e14 --- /dev/null +++ b/changelog/unreleased/12557.md @@ -0,0 +1,9 @@ +Bugfix: Read OIDC Prompt from the correct system configuration key + +The OpenID Connect Prompt value provided through the system configuration was +read from the Ports key instead of the Prompt key. As a result the +administrator-provided Prompt was ignored and replaced with the raw Ports +value, which is not a valid OIDC prompt parameter. The Prompt is now read from +the Prompt key. + +https://github.com/owncloud/client/issues/12557 diff --git a/src/libsync/config/appconfig.cpp b/src/libsync/config/appconfig.cpp index 95abf60ade1..d9851a5df00 100644 --- a/src/libsync/config/appconfig.cpp +++ b/src/libsync/config/appconfig.cpp @@ -68,7 +68,7 @@ OpenIdConfig AppConfig::loadOpenIdConfigFromSystemConfig(const QSettings &system QString clientId = system.value(OidcClientIdKey, QString()).toString(); QString clientSecret = system.value(OidcClientSecretKey, QString()).toString(); QString scopes = system.value(OidcScopesKey, QString()).toString(); - QString prompt = system.value(OidcPortsKey, QString()).toString(); + QString prompt = system.value(OidcPromptKey, QString()).toString(); QVector ports; QVariant portsVar = system.value(OidcPortsKey, QString()).toString(); diff --git a/src/libsync/config/appconfig.h b/src/libsync/config/appconfig.h index de9fe3a3b4b..b1c8d3fa4a2 100644 --- a/src/libsync/config/appconfig.h +++ b/src/libsync/config/appconfig.h @@ -103,9 +103,16 @@ class OWNCLOUDSYNC_EXPORT AppConfig */ static QString configPath(const QOperatingSystemVersion::OSType& os, const Theme& theme); + /** + * Load the OpenID Connect configuration from a system QSettings instance. + * @param system the system configuration settings + * @return An OpenIdConfig object populated from the [OpenIDConnect] section. + * @internal kept public for testing purposes + */ + static OpenIdConfig loadOpenIdConfigFromSystemConfig(const QSettings &system); + private: static OpenIdConfig loadOpenIdConfigFromTheme(); - static OpenIdConfig loadOpenIdConfigFromSystemConfig(const QSettings &system); private: // System settings keys // Setup related keys diff --git a/test/testappconfig.cpp b/test/testappconfig.cpp index 5abd9882c78..0e9c244dea6 100644 --- a/test/testappconfig.cpp +++ b/test/testappconfig.cpp @@ -5,6 +5,9 @@ #include "libsync/owncloudtheme.h" #include "libsync/config/appconfig.h" +#include +#include + class TestAppConfig : public QObject { Q_OBJECT @@ -17,6 +20,33 @@ private Q_SLOTS: QCOMPARE(OCC::AppConfig::configPath(QOperatingSystemVersion::MacOS, t), QString("/Library/Preferences/com.owncloud.desktopclient/ownCloud.ini")); QCOMPARE(OCC::AppConfig::configPath(QOperatingSystemVersion::Unknown, t), QString("/etc/ownCloud/ownCloud.ini")); } + + void testLoadOpenIdConfig() + { + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString path = dir.filePath(QStringLiteral("system.ini")); + + { + QSettings settings(path, QSettings::IniFormat); + settings.setValue(QStringLiteral("OpenIDConnect/ClientId"), QStringLiteral("my-client-id")); + settings.setValue(QStringLiteral("OpenIDConnect/ClientSecret"), QStringLiteral("my-client-secret")); + settings.setValue(QStringLiteral("OpenIDConnect/Ports"), QStringLiteral("8080,8443")); + settings.setValue(QStringLiteral("OpenIDConnect/Scopes"), QStringLiteral("openid email")); + settings.setValue(QStringLiteral("OpenIDConnect/Prompt"), QStringLiteral("select_account consent")); + settings.sync(); + } + + QSettings settings(path, QSettings::IniFormat); + const OCC::OpenIdConfig cfg = OCC::AppConfig::loadOpenIdConfigFromSystemConfig(settings); + + // Prompt must be read from the Prompt key, not the Ports key (regression test for #12557) + QCOMPARE(cfg.prompt(), QStringLiteral("select_account consent")); + QCOMPARE(cfg.ports(), (QList{8080, 8443})); + QCOMPARE(cfg.scopes(), QStringLiteral("openid email")); + QCOMPARE(cfg.clientId(), QStringLiteral("my-client-id")); + QCOMPARE(cfg.clientSecret(), QStringLiteral("my-client-secret")); + } }; QTEST_GUILESS_MAIN(TestAppConfig) From c164e48219f9fb244562f1738cc5b3b7306afaed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Wed, 1 Jul 2026 00:32:02 +0200 Subject: [PATCH 2/3] refactor: test OIDC system config via the public AppConfig interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback (#12559): do not widen the visibility of the private loadOpenIdConfigFromSystemConfig() helper for testing. Instead add a public AppConfig(const QSettings &) constructor that injects the system configuration source, keeping the internal helpers private. The regression test now constructs a real AppConfig from a fake system config and asserts the resulting values through openIdConfig(), exactly as production code consumes them. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- src/libsync/config/appconfig.cpp | 28 +++++++++++++++++++++------- src/libsync/config/appconfig.h | 25 +++++++++++++++++++------ test/testappconfig.cpp | 11 +++++++---- 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/src/libsync/config/appconfig.cpp b/src/libsync/config/appconfig.cpp index d9851a5df00..be9164a2689 100644 --- a/src/libsync/config/appconfig.cpp +++ b/src/libsync/config/appconfig.cpp @@ -19,20 +19,34 @@ namespace chrono = std::chrono; AppConfig::AppConfig() { - _serverUrl = Theme::instance()->overrideServerUrlV2(); - // If a theme provides a hardcoded URL, do not allow for URL change. - _allowServerURLChange = Theme::instance()->overrideServerUrlV2().isEmpty(); - _skipUpdateCheck = false; - _openIdConfig = loadOpenIdConfigFromTheme(); + loadThemeDefaults(); if (!Theme::instance()->allowSystemConfigOverrides()) return; - // Load all overrides - + // Load all overrides from the system config path auto format = Utility::isWindows() ? QSettings::NativeFormat : QSettings::IniFormat; const QSettings system(configPath(QOperatingSystemVersion::currentType(), *Theme::instance()), format); + applySystemConfig(system); +} + +AppConfig::AppConfig(const QSettings &system) +{ + loadThemeDefaults(); + applySystemConfig(system); +} +void AppConfig::loadThemeDefaults() +{ + _serverUrl = Theme::instance()->overrideServerUrlV2(); + // If a theme provides a hardcoded URL, do not allow for URL change. + _allowServerURLChange = Theme::instance()->overrideServerUrlV2().isEmpty(); + _skipUpdateCheck = false; + _openIdConfig = loadOpenIdConfigFromTheme(); +} + +void AppConfig::applySystemConfig(const QSettings &system) +{ _serverUrl = system.value(SetupServerUrlKey, QString()).toString(); if (system.contains(SetupAllowServerUrlChangeKey)) { _allowServerURLChange = system.value(SetupAllowServerUrlChangeKey).toBool(); diff --git a/src/libsync/config/appconfig.h b/src/libsync/config/appconfig.h index b1c8d3fa4a2..65033765990 100644 --- a/src/libsync/config/appconfig.h +++ b/src/libsync/config/appconfig.h @@ -67,6 +67,16 @@ class OWNCLOUDSYNC_EXPORT AppConfig { public: explicit AppConfig(); + + /** + * Construct an AppConfig from an explicitly provided system configuration. + * The theme defaults are loaded first and then overridden with the values from @p system, + * mirroring the behavior of the default constructor without reading the on-disk/registry path. + * @param system the system configuration settings to apply on top of the theme defaults + * @internal kept public for testing purposes + */ + explicit AppConfig(const QSettings &system); + /** * Determine if changing the server URL is allowed based on system configuration. * This value is only relevant if SystemConfig::serverUrl() returns a non-empty string. @@ -103,16 +113,19 @@ class OWNCLOUDSYNC_EXPORT AppConfig */ static QString configPath(const QOperatingSystemVersion::OSType& os, const Theme& theme); +private: /** - * Load the OpenID Connect configuration from a system QSettings instance. - * @param system the system configuration settings - * @return An OpenIdConfig object populated from the [OpenIDConnect] section. - * @internal kept public for testing purposes + * Initialize all settings with the defaults provided by the current theme. */ - static OpenIdConfig loadOpenIdConfigFromSystemConfig(const QSettings &system); + void loadThemeDefaults(); + + /** + * Apply the overrides from the given system configuration on top of the already loaded theme defaults. + */ + void applySystemConfig(const QSettings &system); -private: static OpenIdConfig loadOpenIdConfigFromTheme(); + static OpenIdConfig loadOpenIdConfigFromSystemConfig(const QSettings &system); private: // System settings keys // Setup related keys diff --git a/test/testappconfig.cpp b/test/testappconfig.cpp index 0e9c244dea6..6750e104bda 100644 --- a/test/testappconfig.cpp +++ b/test/testappconfig.cpp @@ -33,15 +33,18 @@ private Q_SLOTS: settings.setValue(QStringLiteral("OpenIDConnect/ClientSecret"), QStringLiteral("my-client-secret")); settings.setValue(QStringLiteral("OpenIDConnect/Ports"), QStringLiteral("8080,8443")); settings.setValue(QStringLiteral("OpenIDConnect/Scopes"), QStringLiteral("openid email")); - settings.setValue(QStringLiteral("OpenIDConnect/Prompt"), QStringLiteral("select_account consent")); + // Prompt value is intentionally different from the Ports value and the theme default so + // the regression test for #12557 cannot pass by coincidence. + settings.setValue(QStringLiteral("OpenIDConnect/Prompt"), QStringLiteral("login")); settings.sync(); } - QSettings settings(path, QSettings::IniFormat); - const OCC::OpenIdConfig cfg = OCC::AppConfig::loadOpenIdConfigFromSystemConfig(settings); + // Drive a real AppConfig instance through its public interface, exactly as production code does. + const QSettings settings(path, QSettings::IniFormat); + const OCC::OpenIdConfig cfg = OCC::AppConfig(settings).openIdConfig(); // Prompt must be read from the Prompt key, not the Ports key (regression test for #12557) - QCOMPARE(cfg.prompt(), QStringLiteral("select_account consent")); + QCOMPARE(cfg.prompt(), QStringLiteral("login")); QCOMPARE(cfg.ports(), (QList{8080, 8443})); QCOMPARE(cfg.scopes(), QStringLiteral("openid email")); QCOMPARE(cfg.clientId(), QStringLiteral("my-client-id")); From 41d1b7ff77ca0a4b35e44a4c83d936d4536cb952 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Wed, 1 Jul 2026 00:35:56 +0200 Subject: [PATCH 3/3] test: restore original OIDC prompt test value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "select_account consent" already differs from the Ports value, so the regression test catches the #12557 bug without changing it. Revert the unnecessary value change and the misleading comment. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- test/testappconfig.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/testappconfig.cpp b/test/testappconfig.cpp index 6750e104bda..e94ee590fe2 100644 --- a/test/testappconfig.cpp +++ b/test/testappconfig.cpp @@ -33,9 +33,7 @@ private Q_SLOTS: settings.setValue(QStringLiteral("OpenIDConnect/ClientSecret"), QStringLiteral("my-client-secret")); settings.setValue(QStringLiteral("OpenIDConnect/Ports"), QStringLiteral("8080,8443")); settings.setValue(QStringLiteral("OpenIDConnect/Scopes"), QStringLiteral("openid email")); - // Prompt value is intentionally different from the Ports value and the theme default so - // the regression test for #12557 cannot pass by coincidence. - settings.setValue(QStringLiteral("OpenIDConnect/Prompt"), QStringLiteral("login")); + settings.setValue(QStringLiteral("OpenIDConnect/Prompt"), QStringLiteral("select_account consent")); settings.sync(); } @@ -44,7 +42,7 @@ private Q_SLOTS: const OCC::OpenIdConfig cfg = OCC::AppConfig(settings).openIdConfig(); // Prompt must be read from the Prompt key, not the Ports key (regression test for #12557) - QCOMPARE(cfg.prompt(), QStringLiteral("login")); + QCOMPARE(cfg.prompt(), QStringLiteral("select_account consent")); QCOMPARE(cfg.ports(), (QList{8080, 8443})); QCOMPARE(cfg.scopes(), QStringLiteral("openid email")); QCOMPARE(cfg.clientId(), QStringLiteral("my-client-id"));