fix(subadmin): disable group-admin feature by default behind allow_subadmins

apps/provisioning_api/lib/Users.php

@@ -600,10 +600,14 @@ public function addSubAdmin($parameters) {
 			return new Result(null, 100);
 		}
 		// Go
-		if ($subAdminManager->createSubAdmin($user, $group)) {
-			return new Result(null, 100);
-		} else {
-			return new Result(null, 103, 'Unknown error occurred');
+		try {
+			if ($subAdminManager->createSubAdmin($user, $group)) {
+				return new Result(null, 100);
+			} else {
+				return new Result(null, 103, 'Unknown error occurred');
+			}
+		} catch (\OC\HintException $e) {
+			return new Result(null, 103, $e->getHint());
 		}
 	}
 

changelog/unreleased/41634

@@ -0,0 +1,10 @@
+Security: disable group-admin feature by default behind allow_subadmins
+
+Disable the subadmin (group-admin) feature by default behind a new
+allow_subadmins system config, as a security risk-mitigation.
+The feature's code path has known security shortcomings; deployments
+that rely on it can opt back in with 'allow_subadmins' => true in config.php.
+On upgrade, existing group-admin assignments are ignored until an admin
+sets 'allow_subadmins' => true in config.php.
+
+https://github.com/owncloud/core/pull/41634

config/config.sample.php

@@ -218,6 +218,19 @@
  */
 'allow_user_to_change_mail_address' => true,
 
+/**
+ * Allow or disallow the group administrator (subadmin) feature
+ * `true` allows administrators to delegate user management of specific groups
+ * to non-admin users (group admins / subadmins).
+ * `false` (default) disables the feature entirely: existing group-admin
+ * assignments are ignored and no new ones can be created.
+ *
+ * SECURITY NOTE: this feature is disabled by default as a risk-mitigation.
+ * Only enable it if you require delegated group administration and understand
+ * that group admins can manage users within their groups.
+ */
+'allow_subadmins' => false,
+
 /**
  * Define the lifetime of the remember-login cookie
  * The remember-login cookie is set when the user clicks the `remember` checkbox

lib/private/Group/Manager.php

@@ -487,7 +487,8 @@ public function getSubAdmin() {
 			$this->subAdmin = new \OC\SubAdmin(
 				$this->userManager,
 				$this,
-				\OC::$server->getDatabaseConnection()
+				\OC::$server->getDatabaseConnection(),
+				\OC::$server->getConfig()
 			);
 		}
 

lib/private/SubAdmin.php

@@ -28,6 +28,7 @@
 
 use OC\Hooks\PublicEmitter;
 use OCP\Events\EventEmitterTrait;
+use OCP\IConfig;
 use OCP\IUser;
 use OCP\IUserManager;
 use OCP\IGroup;
@@ -45,21 +46,27 @@ class SubAdmin extends PublicEmitter {
 	/** @var IDBConnection */
 	private $dbConn;
 
+	/** @var IConfig */
+	private $config;
+
 	/**
 	 * @param IUserManager $userManager
 	 * @param IGroupManager $groupManager
 	 * @param IDBConnection $dbConn
+	 * @param IConfig $config
 	 */
 	public function __construct(
 		IUserManager $userManager,
 		IGroupManager $groupManager,
-		IDBConnection $dbConn
+		IDBConnection $dbConn,
+		IConfig $config
 	) {
 		'@phan-var \OC\User\Manager $userManager';
 		$this->userManager = $userManager;
 		'@phan-var \OC\Group\Manager $groupManager';
 		$this->groupManager = $groupManager;
 		$this->dbConn = $dbConn;
+		$this->config = $config;
 
 		$this->userManager->listen('\OC\User', 'postDelete', function ($user) {
 			$this->post_deleteUser($user);
@@ -69,13 +76,33 @@ public function __construct(
 		});
 	}
 
+	/**
+	 * Whether the group admin (subadmin) feature is enabled.
+	 *
+	 * Disabled by default as a security risk-mitigation; admins opt in via the
+	 * `allow_subadmins` system config.
+	 *
+	 * @return bool
+	 */
+	public function isEnabled() {
+		return $this->config->getSystemValue('allow_subadmins', false) === true;
+	}
+
 	/**
 	 * add a SubAdmin
 	 * @param IUser $user user to be SubAdmin
 	 * @param IGroup $group group $user becomes subadmin of
 	 * @return bool
+	 * @throws \OC\HintException if the subadmin feature is disabled
 	 */
 	public function createSubAdmin(IUser $user, IGroup $group) {
+		if (!$this->isEnabled()) {
+			$l = \OC::$server->getL10N('lib');
+			throw new \OC\HintException(
+				$l->t('Group admin feature is disabled'),
+				$l->t('The group admin (subadmin) feature has been disabled by the administrator')
+			);
+		}
 		return $this->emittingCall(function () use (&$user, &$group) {
 			$qb = $this->dbConn->getQueryBuilder();
 
@@ -125,6 +152,9 @@ public function deleteSubAdmin(IUser $user, IGroup $group) {
 	 * @return IGroup[]
 	 */
 	public function getSubAdminsGroups(IUser $user) {
+		if (!$this->isEnabled()) {
+			return [];
+		}
 		$qb = $this->dbConn->getQueryBuilder();
 
 		$result = $qb->select('gid')
@@ -150,6 +180,9 @@ public function getSubAdminsGroups(IUser $user) {
 	 * @return IUser[]
 	 */
 	public function getGroupsSubAdmins(IGroup $group) {
+		if (!$this->isEnabled()) {
+			return [];
+		}
 		$qb = $this->dbConn->getQueryBuilder();
 
 		$result = $qb->select('uid')
@@ -174,6 +207,9 @@ public function getGroupsSubAdmins(IGroup $group) {
 	 * @return array
 	 */
 	public function getAllSubAdmins() {
+		if (!$this->isEnabled()) {
+			return [];
+		}
 		$qb = $this->dbConn->getQueryBuilder();
 
 		$result = $qb->select('*')
@@ -203,6 +239,9 @@ public function getAllSubAdmins() {
 	 * @return bool
 	 */
 	public function isSubAdminofGroup(IUser $user, IGroup $group) {
+		if (!$this->isEnabled()) {
+			return false;
+		}
 		$qb = $this->dbConn->getQueryBuilder();
 
 		/*
@@ -232,6 +271,12 @@ public function isSubAdmin(IUser $user) {
 			return true;
 		}
 
+		// When the feature is disabled, group-admin-only users have no
+		// elevated rights (real admins are handled by the short-circuit above)
+		if (!$this->isEnabled()) {
+			return false;
+		}
+
 		$qb = $this->dbConn->getQueryBuilder();
 
 		$result = $qb->select('gid')

settings/ajax/togglesubadmins.php

@@ -39,7 +39,12 @@
 if ($isSubAdminOfGroup) {
 	$subAdminManager->deleteSubAdmin($targetUserObject, $targetGroupObject);
 } else {
-	$subAdminManager->createSubAdmin($targetUserObject, $targetGroupObject);
+	try {
+		$subAdminManager->createSubAdmin($targetUserObject, $targetGroupObject);
+	} catch (\OC\HintException $e) {
+		OC_JSON::error(['data' => ['message' => $e->getHint()]]);
+		exit();
+	}
 }
 
 OC_JSON::success();

settings/users.php

@@ -72,7 +72,7 @@
 $recoveryAdminEnabled = OC_App::isEnabled('encryption') &&
 						$config->getAppValue('encryption', 'recoveryAdminEnabled', null);
 
-if ($isAdmin) {
+if ($isAdmin && \OC::$server->getGroupManager()->getSubAdmin()->isEnabled()) {
 	$subAdmins = \OC::$server->getGroupManager()->getSubAdmin()->getAllSubAdmins();
 	// New class returns IUser[] so convert back
 	$result = [];

tests/acceptance/features/apiProvisioning-v1/createSubAdmin.feature

@@ -25,6 +25,15 @@ Feature: create a subadmin
     And the HTTP status code should be "200"
     And user "nonexistentuser" should not be a subadmin of group "brand-new-group"
 
+  @smokeTest
+  Scenario: admin tries to create a subadmin when the subadmin feature is disabled
+    Given user "brand-new-user" has been created with default attributes and without skeleton files
+    And group "brand-new-group" has been created
+    When the administrator tries to make user "brand-new-user" a subadmin of group "brand-new-group" using the provisioning API with the subadmin feature disabled
+    Then the OCS status code should be "103"
+    And the HTTP status code should be "200"
+    And user "brand-new-user" should not be a subadmin of group "brand-new-group"
+
 
   Scenario: admin tries to create a subadmin using a group which does not exist
     Given user "brand-new-user" has been created with default attributes and without skeleton files

tests/acceptance/features/bootstrap/Provisioning.php

@@ -49,6 +49,12 @@ trait Provisioning {
 	private array $createdRemoteUsers = [];
 	private array $enabledApps = [];
 	private array $disabledApps = [];
+
+	/**
+	 * Whether this scenario enabled the group admin (subadmin) feature, which
+	 * is disabled by default. Used to revert the config after the scenario.
+	 */
+	private bool $subadminFeatureEnabled = false;
 	private array $startingGroups = [];
 	private array $createdRemoteGroups = [];
 	private array $createdGroups = [];
@@ -4247,21 +4253,52 @@ public function adminMakesUserSubadminOfGroupUsingTheProvisioningApi(
 		);
 	}
 
+	/**
+	 * @When /^the administrator tries to make user "([^"]*)" a subadmin of group "([^"]*)" using the provisioning API with the subadmin feature disabled$/
+	 *
+	 * @param string $user
+	 * @param string $group
+	 *
+	 * @return void
+	 */
+	public function adminTriesToMakeUserSubadminOfGroupUsingTheProvisioningApiWithSubadminFeatureDisabled(
+		string $user,
+		string $group
+	):void {
+		$user = $this->getActualUsername($user);
+		$this->userMakesUserASubadminOfGroupUsingTheProvisioningApi(
+			$this->getAdminUsername(),
+			$user,
+			$group,
+			false
+		);
+	}
+
 	/**
 	 * @When user :user makes user :otherUser a subadmin of group :group using the provisioning API
 	 *
 	 * @param string $user
 	 * @param string $otherUser
 	 * @param string $group
+	 * @param bool $enableSubadminFeature
 	 *
 	 * @return void
 	 * @throws Exception
 	 */
 	public function userMakesUserASubadminOfGroupUsingTheProvisioningApi(
 		string $user,
 		string $otherUser,
-		string $group
+		string $group,
+		bool $enableSubadminFeature = true
 	):void {
+		if ($enableSubadminFeature) {
+			// The subadmin feature is disabled by default; enable it so these
+			// scenarios can exercise it. Reverted in cleanupSubadminFeature().
+			$this->enableSubadminFeature();
+		} else {
+			// Make sure that the Subadmin feature is disabled.
+			$this->cleanupSubadminFeature();
+		}
 		$actualUser = $this->getActualUsername($user);
 		$actualPassword = $this->getUserPassword($actualUser);
 		$actualSubadminUsername = $this->getActualUsername($otherUser);
@@ -4279,6 +4316,43 @@ public function userMakesUserASubadminOfGroupUsingTheProvisioningApi(
 		);
 	}
 
+	/**
+	 * The group admin (subadmin) feature is disabled by default. Enable it so
+	 * scenarios that exercise subadmins keep working. Tracked so it can be
+	 * reverted after the scenario.
+	 *
+	 * @return void
+	 * @throws Exception
+	 */
+	public function enableSubadminFeature():void {
+		if ($this->subadminFeatureEnabled) {
+			return;
+		}
+		SetupHelper::setSystemConfig(
+			'allow_subadmins',
+			'true',
+			$this->getStepLineRef(),
+			'boolean'
+		);
+		$this->subadminFeatureEnabled = true;
+	}
+
+	/**
+	 * @AfterScenario
+	 *
+	 * @return void
+	 * @throws Exception
+	 */
+	public function cleanupSubadminFeature():void {
+		if ($this->subadminFeatureEnabled) {
+			SetupHelper::deleteSystemConfig(
+				'allow_subadmins',
+				$this->getStepLineRef()
+			);
+			$this->subadminFeatureEnabled = false;
+		}
+	}
+
 	/**
 	 * @When the administrator gets all the groups where user :user is subadmin using the provisioning API
 	 *