diff --git a/changelog/unreleased/OC10-75 b/changelog/unreleased/OC10-75 new file mode 100644 index 00000000000..1d4c6552653 --- /dev/null +++ b/changelog/unreleased/OC10-75 @@ -0,0 +1,9 @@ +Security: Restrict AppConfigController read methods to full admins only + +Subadmin users could read all oc_appconfig values including SMTP passwords, +LDAP bind credentials, and encryption master keys via the Settings API. +Removed @NoAdminRequired from getApps, getKeys, and getValue so that the +AdminMiddleware enforces full-admin-only access, consistent with the write +methods. + +https://github.com/owncloud/core/pull/41550 diff --git a/settings/Controller/AppConfigController.php b/settings/Controller/AppConfigController.php index 0e8da029fba..b96339fb5cd 100644 --- a/settings/Controller/AppConfigController.php +++ b/settings/Controller/AppConfigController.php @@ -27,9 +27,7 @@ /** * The code is mostly copied from core/ajax/appconfig.php - * Read methods (getApps, getKeys and getValue) are available to subadmins, - * which wasn't possible with the core/ajax/appconfig.php file. The rest of - * the methods require admin privileges. + * All methods require full admin privileges. * Note that the "hasKey" method is missing. You can do the same in a lot of * cases by trying to get the value of the key. * @@ -54,8 +52,6 @@ public function __construct( } /** - * @NoAdminRequired - * * Get the list of apps */ public function getApps() { @@ -63,8 +59,6 @@ public function getApps() { } /** - * @NoAdminRequired - * * Get the list of keys for that particular app * @param string $app */ @@ -73,8 +67,6 @@ public function getKeys($app) { } /** - * @NoAdminRequired - * * Get the value of the key for that app, or the default value provided * if it's missing. * @param string $app diff --git a/settings/js/users/users.js b/settings/js/users/users.js index a6d666911d1..968173332d8 100644 --- a/settings/js/users/users.js +++ b/settings/js/users/users.js @@ -751,18 +751,16 @@ $(document).ready(function () { // TODO: move other init calls inside of initialize UserList.initialize($('#userlist')); - OC.AppConfig.getValue('core', 'umgmt_set_password', 'false', function (data) { - var showPassword = $.parseJSON(data); - if (showPassword === true) { + (function () { + var showPassword = $('#CheckBoxPasswordOnUserCreate').prop('checked'); + if (showPassword) { $("#newuserpassword").show(); $("#newemail").hide(); - $('#CheckBoxPasswordOnUserCreate').attr('checked', true); } else { $("#newemail").show(); $("#newuserpassword").hide(); - $('#CheckBoxPasswordOnUserCreate').attr('checked', false); } - }); + }()); $userListBody.on('click', '.password', function (event) { event.stopPropagation(); diff --git a/tests/Settings/Controller/AppConfigControllerTest.php b/tests/Settings/Controller/AppConfigControllerTest.php index 26982b653a2..39fd4b0ed4e 100644 --- a/tests/Settings/Controller/AppConfigControllerTest.php +++ b/tests/Settings/Controller/AppConfigControllerTest.php @@ -156,4 +156,22 @@ public function testDeleteAppWrong(): void { $this->assertEquals([], $response->getData()); $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); } + + public function testGetAppsRequiresAdmin(): void { + $reflection = new \ReflectionMethod(AppConfigController::class, 'getApps'); + $docComment = (string)$reflection->getDocComment(); + $this->assertStringNotContainsString('@NoAdminRequired', $docComment); + } + + public function testGetKeysRequiresAdmin(): void { + $reflection = new \ReflectionMethod(AppConfigController::class, 'getKeys'); + $docComment = (string)$reflection->getDocComment(); + $this->assertStringNotContainsString('@NoAdminRequired', $docComment); + } + + public function testGetValueRequiresAdmin(): void { + $reflection = new \ReflectionMethod(AppConfigController::class, 'getValue'); + $docComment = (string)$reflection->getDocComment(); + $this->assertStringNotContainsString('@NoAdminRequired', $docComment); + } }