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

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

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,17 +52,13 @@ public function __construct(
 	}
 
 	/**
-	 * @NoAdminRequired
-	 *
 	 * Get the list of apps
 	 */
 	public function getApps() {
 		return new JSONResponse($this->appConfig->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

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();

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);
+	}
 }