Skip to content

Conversation

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Oct 9, 2025

Background

When the client is installed in a bigger organization via tools like Microsoft SCCM (now known as MECM) or similar tools it can be required to specify some pre-configured values.
Platform specific configuration storage shall be used (e.g. registry on MS)

Solution

Two configuration values are now supported:

  • Setup/ServerUrl: allows to specify the server url in the account setup
  • Setup/AllowServerUrlChange: allows to specify if the url can be changed - if false the given Setup/ServerUrl is the only allowed server to be connected to
  • OpenIDConnect/*: adds a bunch of paremeters to configure OpenIdConnect

Configuration will be read from:

  • linux: /etc/ownCloud/ownCloud.ini
  • mac: /Library/Preferences/com.owncloud.desktopclient/ownCloud.ini
  • windows: HKEY_LOCAL_MACHINE\Software\ownCloud\ownCloud

Further Configuration Values

  • OpenID Connect client id and client secret
  • including the capability to use PKCE without a secret
  • [ ]

@update-docs
Copy link

update-docs bot commented Oct 9, 2025

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@DeepDiver1975 DeepDiver1975 changed the title feat: allow account setup configuration via system based configuratio… [DC-135] feat: allow account setup configuration via system based configuratio… Jan 16, 2026
@DeepDiver1975 DeepDiver1975 force-pushed the feat/global-configuration branch from f445526 to e40cda6 Compare January 16, 2026 14:08
…d settings from a system location. skipUpdateCheck is using SystemConfig already. Proxy settings will follow
@DeepDiver1975 DeepDiver1975 changed the title [DC-135] feat: allow account setup configuration via system based configuratio… [DC-135] feat: introduce system based configuration aka group policy Jan 16, 2026
@DeepDiver1975 DeepDiver1975 marked this pull request as ready for review January 16, 2026 16:06
// parent with nam to ensure we reset when the nam is reset
// todo: #22 - the parenting here is highly questionable, as is the use of the shared account ptr
_oAuthJob = new AccountBasedOAuth(_account, this);
SystemConfig systemConfig;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think either the SystemConfig or the openIdConfig should be a member of credentials, then we don't need to load the config over and over again.

const QString wellKnownPathC = QStringLiteral("/.well-known/openid-configuration");

auto defaultOauthPromptValue()
auto defaultOauthPromptValue(const OpenIdConfig& config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fond of auto's when the return type is not obvious

*/
#include "urlpagecontroller.h"

#include "../../libsync/config/systemconfig.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that doesn't look right - see the same file included in oauthpagecontroller. cpp. I think you should only need config/systemconfig.h


namespace {
// Setup related keys
static constexpr auto KEY_SETUP_ALLOW_SERVER_URL_CHANGE = "Setup/AllowServerUrlChange";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to make a group decision about how to declare const strings for settings keys going forward. I don't like the anonymous namespace as you have to know where to find it. and "auto" is not cool imo for decl'd values or function returns.

IMO we should prefer (private whenever possible) static const members a la:

inline static const QString KEY_SETUP_ALLOW_SERVER_URL_CHANGE = "Setup/AllowServerUrlChange";

then we have a clear view into the possible keys as they are clearly outlined right in the class decl

if (os == QOperatingSystemVersion::Windows) {
// we use HKEY_LOCAL_MACHINE\Software\Policies since this is the location whe GPO operates
return QString("HKEY_LOCAL_MACHINE\\Software\\Policies\\%1\\%2").arg(theme.vendor(), theme.appNameGUI());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we alternately use appNameGUI vs appName?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

casing - "owncloud" vs "ownCloud"

In registry paths: "HKEY_LOCAL_MACHINE\Software\Policies\ownCloud\ownCloud
On linux: /etc/owncloud/owncloud.ini

auto format = Utility::isWindows() ? QSettings::NativeFormat : QSettings::IniFormat;
QSettings system(configPath(QOperatingSystemVersion::currentType(), *Theme::instance()), format);

_allowServerURLChange = system.value(KEY_SETUP_ALLOW_SERVER_URL_CHANGE, true).toBool();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this right? don't we have the case where there is no value in system config for allow url change BUT theme defines url (kw for sure). In that case allow url change should be false, right?

_allowServerURLChange = system.value(KEY_SETUP_ALLOW_SERVER_URL_CHANGE, true).toBool();
_serverUrl = system.value(KEY_SETUP_SERVER_URL, QString()).toString();
_skipUpdateCheck = system.value(KEY_UPDATER_SKIP_UPDATE_CHECK, false).toBool();
_moveToTrash = system.value(KEY_SETUP_MOVE_TO_TRASH, false).toBool();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also have a user setting = move to trash. How is this system setting different and does it take precedence over the user setting, in which case maybe we need to hide that option in the settings gui or ?

if (_systemConfig.moveToTrash()) {
return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah per above this imo won't work. Imagine: user has switched move to trash off. does it automatically turn back on in the gui, and either way, why is it still moving to trash? confusion ensues.

I also have doubts about user settings doing this "resolve" behavior since the system config has precedence I would expect the resolution to happen there

if (connection.isEmpty())
con = defaultConnection();
if (_systemConfig.skipUpdateCheck()) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to comment for moveToTrash I think this is very confusing and goes against the idea that system config takes precedence and does the resolving

fallback = getValue(skipUpdateCheckC(), QString(), fallback);

QVariant value = getPolicySetting(skipUpdateCheckC(), fallback);
QVariant value = getValue(skipUpdateCheckC(), QString(), fallback);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand this at all. please add a comment to explain it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants