fix(security): OC10-75 - restrict AppConfigController read methods to full admins only

[DeepDiver1975]

Summary

• Security fix (CVSS 7.7): Removes @NoAdminRequired from getApps(), getKeys(), and getValue() in AppConfigController so the AppFramework's AdminMiddleware blocks Subadmin users from reading all oc_appconfig values (SMTP passwords, LDAP bind credentials, encryption master keys, etc.)
• Collateral fix: Replaces a now-blocked OC.AppConfig.getValue AJAX call in settings/js/users/users.js with a synchronous DOM read of the server-rendered checkbox state — preserving correct UI behavior on the Subadmin-accessible /settings/users page
• Updated the AppConfigController class docblock to accurately reflect that all methods now require full admin privileges

Details

The three read endpoints were annotated with @NoAdminRequired, which caused SecurityMiddleware to skip the admin check. Subadmins could exploit this to enumerate all app config keys and read any value — including credentials stored by SMTP, LDAP, OAuth, and encryption apps.

Write methods (setValue, deleteKey, deleteApp) were already admin-only. This PR makes the read methods consistent with them.

A cross-repo search of the entire ownCloud GitHub org found no other apps using the affected endpoints from Subadmin accessible UI, with one exception fixed here.

Reported by Pablo Giovanni. Confirmed on ownCloud 10.14.0 and 10.16.1.

Fixes: OC10-75

Test Plan

• Log in as Subadmin → GET /index.php/settings/appconfig → HTTP 403
• Log in as full admin → GET /index.php/settings/appconfig → HTTP 200
• Log in as Subadmin → /settings/users → password/email field toggles correctly on page load
• Log in as full admin → /settings/users → same behavior

[update-docs]

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.

[jvillafanez]

We need to check how subadmins are impacted in the web UI. There might be broken operations.

[DeepDiver1975]

We need to check how subadmins are impacted in the web UI. There might be broken operations.

Based on the analysis on where these endpoints are called - only users.js was affected.

But please feel free to test this as well - an additional pair of eyes is much appreciated 🙏

[jvillafanez]

The checkboxes of the settings (bottom left corner) behaves weirdly for subadmins. The request returns with a 403, but both the checkbox and the affected field act as if it was successful. Of course, the settings aren't saved, so reloading the page will revert the page back.

Side note, the subdamin can't change the groups of the users he's managing. There have been changes in that regard, so I'm not sure about the correct state. It could be an expected behavior.

[DeepDiver1975]

The checkboxes of the settings (bottom left corner) behaves weirdly for subadmins. The request returns with a 403, but both the checkbox and the affected field act as if it was successful. Of course, the settings aren't saved, so reloading the page will revert the page back.

I guess not saving the state is acceptable as side effect of this sec fix. The UI behaves correct ...

[DeepDiver1975]

Side note, the subdamin can't change the groups of the users he's managing. There have been changes in that regard, so I'm not sure about the correct state. It could be an expected behavior.

no idea when (if?) this changed - never really using subadmin ....

[jvillafanez]

Side note, the subdamin can't change the groups of the users he's managing. There have been changes in that regard, so I'm not sure about the correct state. It could be an expected behavior.

This seems to be expected (https://github.com/owncloud/core/blob/master/settings/js/users/users.js#L433-L438). Basically, for the subdamin, we require the users to be part of a group controlled by the subadmin. If there is only one group, it won't be possible to remove the user from that group, but if there are multiple groups the subadmin can move the users around those groups.
It seems to be a client-only check. The server allows removing the user. from the group.
It might be better to show the group as disabled if we can't interact with it, but I guess it's out of the scope at the moment.