diff --git a/apps/provisioning_api/lib/Users.php b/apps/provisioning_api/lib/Users.php index 355b86baaac9..6662ed26576f 100644 --- a/apps/provisioning_api/lib/Users.php +++ b/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()); } } diff --git a/changelog/unreleased/41634 b/changelog/unreleased/41634 new file mode 100644 index 000000000000..118ca51867cf --- /dev/null +++ b/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 diff --git a/config/config.sample.php b/config/config.sample.php index c3159758cd2a..fe603c2c3795 100644 --- a/config/config.sample.php +++ b/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 diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 0ffde51eb8a1..60762862bf5a 100644 --- a/lib/private/Group/Manager.php +++ b/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() ); } diff --git a/lib/private/SubAdmin.php b/lib/private/SubAdmin.php index 30bec67c0e0b..67ef1cfbcdc8 100644 --- a/lib/private/SubAdmin.php +++ b/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') diff --git a/settings/ajax/togglesubadmins.php b/settings/ajax/togglesubadmins.php index 3a5eb6af452f..20f86f698ee4 100644 --- a/settings/ajax/togglesubadmins.php +++ b/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(); diff --git a/settings/users.php b/settings/users.php index 0b3c3a681bc9..22edc54e44d4 100644 --- a/settings/users.php +++ b/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 = []; diff --git a/tests/acceptance/features/apiProvisioning-v1/createSubAdmin.feature b/tests/acceptance/features/apiProvisioning-v1/createSubAdmin.feature index df57303d0001..01f6005844bc 100644 --- a/tests/acceptance/features/apiProvisioning-v1/createSubAdmin.feature +++ b/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 diff --git a/tests/acceptance/features/bootstrap/Provisioning.php b/tests/acceptance/features/bootstrap/Provisioning.php index 540937fd6f73..e50f807c7220 100644 --- a/tests/acceptance/features/bootstrap/Provisioning.php +++ b/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,12 +4253,34 @@ 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 @@ -4260,8 +4288,17 @@ public function adminMakesUserSubadminOfGroupUsingTheProvisioningApi( 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 * diff --git a/tests/lib/SubAdminTest.php b/tests/lib/SubAdminTest.php index 62ec20f218a1..ddc30b4092af 100644 --- a/tests/lib/SubAdminTest.php +++ b/tests/lib/SubAdminTest.php @@ -34,12 +34,15 @@ class SubAdminTest extends TestCase { /** @var \OCP\IDBConnection */ private $dbConn; + /** @var \OCP\IConfig */ + private $config; + /** @var \OCP\IUser[] */ private $users; /** @var \OCP\IGroup[] */ private $groups; - + public function setUp(): void { $this->users = []; $this->groups = []; @@ -47,6 +50,12 @@ public function setUp(): void { $this->userManager = \OC::$server->getUserManager(); $this->groupManager = \OC::$server->getGroupManager(); $this->dbConn = \OC::$server->getDatabaseConnection(); + $this->config = \OC::$server->getConfig(); + + // The subadmin feature is disabled by default; enable it so the + // existing assertions exercise the feature. Individual tests that + // verify the disabled behavior set the value explicitly. + $this->config->setSystemValue('allow_subadmins', true); // Create 3 users and 3 groups for ($i = 0; $i < 3; $i++) { @@ -96,6 +105,8 @@ public function tearDown(): void { ->where($qb->expr()->eq('uid', $qb->createNamedParameter('orphanedUser'))) ->orWhere($qb->expr()->eq('gid', $qb->createNamedParameter('orphanedGroup'))) ->execute(); + + $this->config->deleteSystemValue('allow_subadmins'); } public function testCreateSubAdmin() { @@ -115,7 +126,7 @@ function (GenericEvent $event) use (&$calledAfterCreate) { $calledAfterCreate[] = $event; } ); - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[0])); $this->assertInstanceOf(GenericEvent::class, $calledAfterCreate[1]); $this->assertInstanceOf(GenericEvent::class, $calledBeforeCreate[1]); @@ -162,7 +173,7 @@ function (GenericEvent $event) use (&$calledAfterCreate) { } public function testDeleteSubAdmin() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[0])); $calledBeforeCreate = []; \OC::$server->getEventDispatcher()->addListener( @@ -215,7 +226,7 @@ function (GenericEvent $event) use (&$calledAfterCreate) { } public function testGetSubAdminsGroups() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[0])); $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[1])); @@ -231,7 +242,7 @@ public function testGetSubAdminsGroups() { } public function testGetGroupsSubAdmins() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[0])); $this->assertTrue($subAdmin->createSubAdmin($this->users[1], $this->groups[0])); @@ -247,7 +258,7 @@ public function testGetGroupsSubAdmins() { } public function testGetAllSubAdmin() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[0])); $this->assertTrue($subAdmin->createSubAdmin($this->users[1], $this->groups[1])); @@ -262,7 +273,7 @@ public function testGetAllSubAdmin() { } public function testIsSubAdminofGroup() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[0])); $this->assertTrue($subAdmin->isSubAdminOfGroup($this->users[0], $this->groups[0])); @@ -273,7 +284,7 @@ public function testIsSubAdminofGroup() { } public function testIsSubAdmin() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[0])); $this->assertTrue($subAdmin->isSubAdmin($this->users[0])); @@ -283,14 +294,14 @@ public function testIsSubAdmin() { } public function testIsSubAdminAsAdmin() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->groupManager->get('admin')->addUser($this->users[0]); $this->assertTrue($subAdmin->isSubAdmin($this->users[0])); } public function testIsUserAccessible() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->groups[0]->addUser($this->users[1]); $this->groups[1]->addUser($this->users[1]); $this->groups[1]->addUser($this->users[2]); @@ -306,12 +317,12 @@ public function testIsUserAccessible() { } public function testIsUserAccessibleAsUser() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->assertFalse($subAdmin->isUserAccessible($this->users[0], $this->users[1])); } public function testIsUserAccessibleAdmin() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[0])); $this->groupManager->get('admin')->addUser($this->users[1]); @@ -319,7 +330,7 @@ public function testIsUserAccessibleAdmin() { } public function testPostDeleteUser() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $user = \array_shift($this->users); foreach ($this->groups as $group) { @@ -331,7 +342,7 @@ public function testPostDeleteUser() { } public function testPostDeleteGroup() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $group = \array_shift($this->groups); foreach ($this->users as $user) { @@ -343,7 +354,7 @@ public function testPostDeleteGroup() { } public function testHooks() { - $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn); + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); $test = $this; $u = $this->users[0]; @@ -368,4 +379,61 @@ public function testHooks() { $this->assertTrue($subAdmin->deleteSubAdmin($u, $g)); $this->assertEquals(2, $count); } + + public function testCreateSubAdminThrowsWhenDisabled() { + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); + $this->config->setSystemValue('allow_subadmins', false); + + $this->expectException(\OC\HintException::class); + $subAdmin->createSubAdmin($this->users[0], $this->groups[0]); + } + + public function testReadMethodsReturnEmptyWhenDisabled() { + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); + // Create an assignment while enabled + $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[0])); + + // Disable the feature: reads must behave as if no subadmins exist + $this->config->setSystemValue('allow_subadmins', false); + + $this->assertSame([], $subAdmin->getSubAdminsGroups($this->users[0])); + $this->assertSame([], $subAdmin->getGroupsSubAdmins($this->groups[0])); + $this->assertSame([], $subAdmin->getAllSubAdmins()); + $this->assertFalse($subAdmin->isSubAdminOfGroup($this->users[0], $this->groups[0])); + $this->assertFalse($subAdmin->isSubAdmin($this->users[0])); + $this->assertFalse($subAdmin->isUserAccessible($this->users[0], $this->users[1])); + + // Cleanup the dormant row (deletion stays enabled) + $this->config->setSystemValue('allow_subadmins', true); + $this->assertTrue($subAdmin->deleteSubAdmin($this->users[0], $this->groups[0])); + } + + public function testIsSubAdminAsAdminWhenDisabled() { + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); + $this->groupManager->get('admin')->addUser($this->users[0]); + + $this->config->setSystemValue('allow_subadmins', false); + + // Real admins keep their privileges even when the feature is disabled + $this->assertTrue($subAdmin->isSubAdmin($this->users[0])); + } + + public function testDeleteSubAdminWorksWhenDisabled() { + $subAdmin = new \OC\SubAdmin($this->userManager, $this->groupManager, $this->dbConn, $this->config); + $this->assertTrue($subAdmin->createSubAdmin($this->users[0], $this->groups[0])); + + $this->config->setSystemValue('allow_subadmins', false); + + // Deletion reduces privilege, so it must remain possible when disabled + $this->assertTrue($subAdmin->deleteSubAdmin($this->users[0], $this->groups[0])); + + $qb = $this->dbConn->getQueryBuilder(); + $result = $qb->select(['gid', 'uid']) + ->from('group_admin') + ->where($qb->expr()->eq('gid', $qb->createNamedParameter($this->groups[0]->getGID()))) + ->andWhere($qb->expr()->eq('uid', $qb->createNamedParameter($this->users[0]->getUID()))) + ->execute() + ->fetchAssociative(); + $this->assertEmpty($result); + } }