Skip to content

Commit 7964448

Browse files
DeepDiver1975claude
andcommitted
fix(subadmin): strict boolean gate and single-source isEnabled()
Address remaining review feedback on the allow_subadmins gate: - Tighten the gate to a strict `=== true` check so only the documented boolean opt-in enables the feature; string values such as 'false' or '0' in a hand-edited config.php now fail closed instead of fail open. - Make SubAdmin::isEnabled() public and route settings/users.php through it, removing the duplicated inline config read so the gate has a single source of truth in the OC\SubAdmin manager. - Document the breaking upgrade behavior in the changelog: existing group-admin assignments are ignored until allow_subadmins => true. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
1 parent 3648fe5 commit 7964448

3 files changed

Lines changed: 5 additions & 3 deletions

File tree

changelog/unreleased/41634

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,7 @@ Disable the subadmin (group-admin) feature by default behind a new
44
allow_subadmins system config, as a security risk-mitigation.
55
The feature's code path has known security shortcomings; deployments
66
that rely on it can opt back in with 'allow_subadmins' => true in config.php.
7+
On upgrade, existing group-admin assignments are ignored until an admin
8+
sets 'allow_subadmins' => true in config.php.
79

810
https://github.com/owncloud/core/pull/41634

lib/private/SubAdmin.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,8 @@ public function __construct(
8484
*
8585
* @return bool
8686
*/
87-
private function isEnabled() {
88-
return $this->config->getSystemValue('allow_subadmins', false) !== false;
87+
public function isEnabled() {
88+
return $this->config->getSystemValue('allow_subadmins', false) === true;
8989
}
9090

9191
/**

settings/users.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@
7272
$recoveryAdminEnabled = OC_App::isEnabled('encryption') &&
7373
$config->getAppValue('encryption', 'recoveryAdminEnabled', null);
7474

75-
if ($isAdmin && $config->getSystemValue('allow_subadmins', false) !== false) {
75+
if ($isAdmin && \OC::$server->getGroupManager()->getSubAdmin()->isEnabled()) {
7676
$subAdmins = \OC::$server->getGroupManager()->getSubAdmin()->getAllSubAdmins();
7777
// New class returns IUser[] so convert back
7878
$result = [];

0 commit comments

Comments
 (0)