-
Notifications
You must be signed in to change notification settings - Fork 684
[DC-135] feat: introduce system based configuration aka group policy #12367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
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. |
cbff648 to
f445526
Compare
…onfig + add OpenIdConnect configuration
f445526 to
e40cda6
Compare
…d settings from a system location. skipUpdateCheck is using SystemConfig already. Proxy settings will follow
| // 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; |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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()); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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; | ||
| } | ||
|
|
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
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 setupSetup/AllowServerUrlChange: allows to specify if the url can be changed - if false the givenSetup/ServerUrlis the only allowed server to be connected toOpenIDConnect/*: adds a bunch of paremeters to configure OpenIdConnectConfiguration will be read from:
Further Configuration Values