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..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(); @@ -68,7 +82,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..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. @@ -104,6 +114,16 @@ class OWNCLOUDSYNC_EXPORT AppConfig static QString configPath(const QOperatingSystemVersion::OSType& os, const Theme& theme); private: + /** + * Initialize all settings with the defaults provided by the current theme. + */ + void loadThemeDefaults(); + + /** + * Apply the overrides from the given system configuration on top of the already loaded theme defaults. + */ + void applySystemConfig(const QSettings &system); + static OpenIdConfig loadOpenIdConfigFromTheme(); static OpenIdConfig loadOpenIdConfigFromSystemConfig(const QSettings &system); diff --git a/test/testappconfig.cpp b/test/testappconfig.cpp index 5abd9882c78..e94ee590fe2 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,34 @@ 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(); + } + + // 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.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)