Lazy grouping within IAppConfig

[ArtificialOwl]

Added features to the IAppConfig:

• lazy loading to avoid loading all app configs on each request. It split the configs in 2 groups; the normal (fast) config values that are loaded in all requests and the lazy config that contains values that are loaded only when one config key of this type is needed.
• typed value so that app knows what to expect when setting/getting config values,
• sensitivity is now set directly when storing a new config value.

limitation

• sensitivity status and type of a value cannot be removed/changed by mistake.
• While it is easy to configure value as sensitive on set, to switch back the status to non-sensitive the app must call updateSensitive()
• Once typed, a value must be deleted in order to change its type.

oc_appconfig

New fields in the appconfig table:

• lazy (bool): 0 is the normal/default way to store a config value,
• type (int): will store the type and the sensitive status of a config value. This is a bit flag using those const:

	VALUE_SENSITIVE = 1;
	VALUE_MIXED = 2;
	VALUE_STRING = 4;
	VALUE_INT = 8;
	VALUE_FLOAT = 16;
	VALUE_BOOL = 32;
	VALUE_ARRAY = 64;

occ

occ config:app:get

It adds the --details option to the config:app:get:

$ ./occ config:app:get myapp mykey --details
{
    "app": "myapp",
    "key": "mykey",
    "value": "ouila",
    "lazy": true,
    "type": 4,
    "typeString": "string",
    "sensitive": false
}

occ config:app:set

New options:

• --lazy, --no-lazy to change the way to store/retrieve the config value,
• --sensitive, --no-sensitive to change the sensitivity of a config value,
• --type to set/change the type of a value,
• Unless using --no-interaction a confirmation from the admin is needed when editing the type/status of a config value.

Note(s)

PR is big and adds huge edits. It should stays compatible with previous call to AppConfig.

[github-advanced-security]

Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

[nickvergessen]

Really not a fan of the caching strategy.
It sounds really risky to get database and cache out of sync easily with a long running task in OCC or request, etc.

As per chat however I like the general idea of having a lazy "block" and even more integrating the sensitive part directly.

Speaking of sensitive some apps use the ICrypto::encrypt() and ICrypto::dectrypt(). We could include that into the methods and help developers again to not have security optionally?

[nickvergessen]

This spams Unknown numeric type 0 very heavily for me.

[ArtificialOwl]

I am interested how you came to this as type should not be 0 in database

[nickvergessen]

Mostlikely from occ config:app:set -vvv ${dir##*/} enabled --value="no" which I have in my set up script to force disable all apps as an option.
The point is more that there will be a lot of "dirty" instances already out there and spaming their logs with warnings when the admin can not know what to do about it is meh.

[ArtificialOwl]

I cannot reproduce this issue and think we can merge as it is right now.
By default, occ config:app:set set value as mixed.

The only issue that I am seeing when using the occ config:app:set is that during the upgrading process, between the files are changed and the upgrade process is run.

Maybe we can edit the database (adding type+lazy field) already in 28 so we don't have error about missing field during the upgrade steps

[nickvergessen]

❌ Drone failures are related!

[nickvergessen]

Okay otherwise

[nickvergessen]

At least needs an issue in the guests app to make them aware of the breakage that is coming their way:
https://github.com/nextcloud/guests/blob/master/lib/AppConfigOverwrite.php

[juliusknorr]

Just noticed we also have two different IAppConfig interfaces (one generic and one as a app framework service). The app framework service should probably also get adapted to cover the new API as currently it is just a wrapper for the deprecated methods: https://github.com/nextcloud/server/blob/215aef3cbdc1963be1bb6bca5218ee0a4b7f1665/lib/public/AppFramework/Services/IAppConfig.php

[nickvergessen]

Should we just deprecate it? we shouldn't have 2 similar interfaces like this?

[juliusknorr]

I still like the service approach to simplify and not need to pass in the app id which makes the api a bit easier for app developers and and more restrictive to not mess with other apps configs

[nickvergessen]

Ah, missed that bit. It's basically what I abstracted away in talk by using my own config class :D

[ChristophWurst]

This would be important for https://docs.nextcloud.com/server/stable/admin_manual/configuration_server/occ_command.html#setting-a-single-configuration-value. @ArtificialOwl @sorbaugh please document this soon 🙏

[joshtrichards]

Array handling still needs to be documented. Particularly since it's diverged from system: https://docs.nextcloud.com/server/latest/admin_manual/occ_command.html#setting-an-array-configuration-value