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

[DeepDiver1975]

Summary

Disables 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.

Approach

Enforcement is centralized in the OC\SubAdmin manager — the single chokepoint all ~28 consumers (settings UI, provisioning API, User.php permission bypass, group filtering, legacy menu/access guards) route through via GroupManager::getSubAdmin(). IConfig is injected and a private isEnabled() helper gates behavior:

• isSubAdmin() keeps the real-admin short-circuit first (admins retain access), then returns false for group-admin-only users when disabled — cascading to all consumers.
• Read methods (isSubAdminofGroup, getSubAdminsGroups, getGroupsSubAdmins, getAllSubAdmins) behave as if no subadmins exist.
• createSubAdmin() throws HintException; the two write callers (settings/ajax/togglesubadmins.php, provisioning API addSubAdmin) surface a clean error instead of a 500.
• deleteSubAdmin() and the post_delete* cleanup hooks stay enabled so admins can prune dormant assignments.

The Users settings page hides the "Group Admin for" column when disabled, and the option is documented in config.sample.php with a security note.

Testing

• tests/lib/SubAdminTest.php: updated to pass IConfig; setUp enables the feature so existing assertions still exercise it; added 4 tests covering disabled behavior (throw on create, empty reads, admin short-circuit preserved, delete still works). 18 tests pass.
• apps/provisioning_api/tests/UsersTest.php (91), settings + user controller tests (74) pass unchanged (they mock the manager).
• Acceptance: subadmin scenarios enable the feature via the central creation funnel in the Provisioning bootstrap trait, reverted in an @AfterScenario hook.
• A security review confirmed no bypass paths, correct short-circuit ordering, and that all consumers fail closed.

🤖 Generated with Claude Code

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

🤖 Automated review by Claude Code review agent.

Overview

This PR gates the sub-admin (group-admin) privilege-delegation feature behind a new allow_subadmins system config that defaults to false, as a security hardening / risk-mitigation change. The approach is sound: enforcement is centralized in the OC\SubAdmin manager, which is the single chokepoint that all ~28 consumers reach via GroupManager::getSubAdmin(). I verified this claim against the head ref (bf33e22) rather than taking it on trust.

Verification of the central-chokepoint claim — the gate is consistent at all entry points I could find:

• OCS / provisioning API — routes declare API::SUBADMIN_AUTH, which resolves in lib/private/legacy/api.php:334 through getGroupManager()->getSubAdmin()->isSubAdmin(). Since isSubAdmin() is gated, subadmin-only OCS endpoints (getUsers, addUser, addToGroup, etc.) correctly fall closed for group-admin-only users. ✅
• Web UI middleware — settings/Middleware/SubadminMiddleware.php receives its isSubAdmin flag from settings/Application.php:240, which calls the same central isSubAdmin(). ✅
• User.php permission bypass — lib/private/User/User.php:459 and :483 (canChangeDisplayName / canChangeMailAddress) route through getSubAdmin()->isSubAdmin(). ✅
• Legacy guards — lib/private/legacy/{util,json,app}.php all call getSubAdmin()->isSubAdmin(). ✅
• Settings controllers / MetaData / provisioning Users.php & Groups.php — all reach the gated isSubAdmin / isSubAdminofGroup / getSubAdminsGroups / getGroupsSubAdmins / getAllSubAdmins / isUserAccessible methods. ✅

Because isUserAccessible() short-circuits on isSubAdmin() (which is gated) and getSubAdminsGroups() returns [] when disabled, the stale-DB-rows authorization-bypass concern is addressed: even if group_admin rows remain in the DB after disabling, no consumer treats the user as a subadmin. testReadMethodsReturnEmptyWhenDisabled explicitly proves this. The real-admin short-circuit ordering in isSubAdmin() (admin check before the disabled check) is correct and tested (testIsSubAdminAsAdminWhenDisabled), so real admins retain access. Deletion + post_delete* cleanup hooks intentionally stay enabled so admins can prune dormant assignments — verified by testDeleteSubAdminWorksWhenDisabled.

This is a well-scoped, defensively-designed change with good test coverage of both enabled and disabled states.

Code quality / style

• The isEnabled() helper and the per-method early returns are clean and readable. Reusing HintException so the two write callers surface a friendly message instead of a 500 is the right call.
• Config flag read is consistent: getSystemValue('allow_subadmins', false) !== false. The !== false form (rather than truthy) is the established codebase idiom for these boolean opt-outs — siblings allow_user_to_change_display_name/allow_user_to_change_mail_address use the same === false comparison style. Good consistency. (Minor caveat below about string values.)
• config.sample.php documents the flag with a clear SECURITY NOTE and the upgrade implication ("existing group-admin assignments are ignored"). Good.
• Acceptance-test plumbing (enableSubadminFeature / @AfterScenario cleanupSubadminFeature, idempotency guard) is tidy.

Specific suggestions

1. Add a changelog entry (blocking for CI). The PR touches changelog/ nowhere, but owncloud/core enforces a changelog/unreleased/<issue> entry per PR (the directory exists and is populated at the head ref). More importantly, this is a behavior-breaking change on upgrade — every deployment currently using subadmins silently loses the feature until an admin sets 'allow_subadmins' => true. That belongs in a changelog/unreleased/ entry of type Change (not just in config.sample.php), so it surfaces in release notes. Please add one.

2. settings/users.php now reads the flag directly ($config->getSystemValue('allow_subadmins', false) !== false) in addition to relying on the central gate. Since getAllSubAdmins() already returns [] when disabled, this second flag-read is redundant for correctness and adds a second source of truth for the default value. It's harmless (it also hides the column), but consider a comment clarifying it's a UI optimization, or derive the column visibility from whether $subAdmins is empty, to keep the flag read in exactly one place.

3. String / env-style config values. getSystemValue('allow_subadmins', false) !== false treats any non-false value as enabled. If someone sets 'allow_subadmins' => 'false' (string) or 0, it will be treated as enabled because 'false' !== false. The sample doc uses a real boolean so this is an edge case, but a stricter === true check (matching "opt-in only via explicit true") would be safer for a security gate that defaults closed. At minimum, worth a deliberate decision.

4. createSubAdmin() uses \OC::$server->getL10N('lib') via the static service locator inside an otherwise DI-constructed class. Consistent with surrounding legacy code, but since IConfig is now injected, injecting IL10N (or a factory) would be cleaner and keep the class testable without the global container. Not blocking.

Potential issues / risks

• Upgrade/backward-compat (intended, but document it): confirmed this is a deliberate breaking change. The only mitigation for existing deployments is the config flag. Beyond the changelog ask above, consider whether release notes / upgrade docs should call this out prominently, since the impact is "delegated group admins stop working after upgrade with no in-product warning."
• No authorization bypass found via stale rows — addressed by the gating of all read/auth methods (see Overview). Good.
• addSubAdmin (provisioning) and togglesubadmins.php now catch HintException and return a clean OCS 103 / JSON error respectively — verified both write paths are covered. The OCS removeSubAdmin / deleteSubAdmin path is intentionally left working when disabled.
• Test gap (minor): the unit tests cover the manager thoroughly, but there is no test asserting that an OCS SUBADMIN_AUTH endpoint actually returns 401/forbidden for a group-admin-only user when the feature is disabled (the integration of the gate at the OCS layer). The acceptance suite enables the feature for subadmin scenarios but doesn't appear to assert the disabled-state denial end-to-end. Consider one acceptance scenario proving a subadmin is denied when allow_subadmins=false.

Overall: solid, fail-closed hardening with the gate correctly centralized. The main thing to resolve before merge is the missing changelog entry documenting the breaking upgrade behavior; the string-config and single-source-of-truth points are worth a quick look.

[phil-davis]

I added an acceptance test scenario for when the subadmin feature is disabled. I tagged it smokeTest as we want to always verify that having the subadmin feature disabled is effective.
An attempt to create a subadmin returns OCS status code 103, and the subadmin is not created.

[phil-davis]

A suggested changelog is in #41636
I am running full-ci there to verify that all the other API acceptance tests pass.

[DeepDiver1975]

🤖 Automated review by Claude Code review agent.

Re-review of feature/disable-subadmins-by-default at current head 40da2a90. The prior review was at bf33e22e; this pass focuses on what changed since then and the status of the three previously-flagged items.

Overview

Goal unchanged: disable the group-admin (subadmin) feature by default behind a new allow_subadmins system config, opt-in via 'allow_subadmins' => true. Enforcement remains centralized in OC\SubAdmin (single chokepoint via GroupManager::getSubAdmin()), with IConfig injected and a private isEnabled() gate.

What changed since bf33e22e (3 commits ahead, 1 behind):

• d70439cd — unrelated guzzle dependency bump (#41635), pulled in by rebase; not part of this feature.
• f07c6ed8 — the subadmin change itself, rebased (functionally identical to what was reviewed at bf33e22e).
• 40da2a90 — the one substantive new change: adds an acceptance test (createSubAdmin.feature) plus bootstrap plumbing in Provisioning.php for the "admin tries to create a subadmin when the feature is disabled" path, asserting OCS status 103 and that no assignment is created. The bootstrap now defaults the feature off, enables it on demand via enableSubadminFeature() (using SetupHelper::setSystemConfig(... 'boolean')), and reverts it in an @AfterScenario hook (cleanupSubadminFeature()).

The core enforcement logic in SubAdmin.php is unchanged from the prior review.

Security assessment (default-off gate)

• Fail-closed at all entry points — confirmed. isSubAdmin() keeps the real-admin short-circuit first, then returns false when disabled. Read methods (isSubAdminofGroup, getSubAdminsGroups, getGroupsSubAdmins, getAllSubAdmins) return empty/false. Because the OCS UsersController (SUBADMIN_AUTH, group filtering at lines ~332, 450, 458) and SubadminMiddleware all route through these methods via getSubAdmin(), the gate cascades consistently — no entry point queries group_admin directly to grant rights.
• No authz bypass with stale rows. Dormant group_admin rows are ignored while disabled (reads return empty), and deleteSubAdmin() + post-delete hooks remain enabled so admins can prune them — correct asymmetry (privilege-reducing ops stay open, privilege-granting ops are blocked).
• Test coverage for enabled + disabled — good. Unit tests cover throw-on-create, empty reads, admin short-circuit preserved, and delete-still-works when disabled. The new acceptance test now covers the disabled path at the API layer too.

Status of the three prior-flagged items

1. Missing changelog for breaking upgrade behavior — NOT addressed. There is still no changelog/unreleased/* entry for the subadmin behavior change. The only changelog file touched (PHPdependencies20260225onward) belongs to the unrelated guzzle commit. This is a default-changing, potentially breaking upgrade (existing subadmins silently lose access) and owncloud/core requires a changelog entry; please add one (type change/security) referencing #41634.

2. Redundant flag read in settings/users.php — NOT addressed. Line 75 still reads $config->getSystemValue('allow_subadmins', false) !== false before calling getAllSubAdmins(), which itself already returns [] when disabled. Harmless defense-in-depth, but redundant and duplicates the gate logic outside the manager. Optional, but worth simplifying to just if ($isAdmin) and letting the manager gate, OR keep it but acknowledge it as intentional belt-and-suspenders.

3. String-config edge case in !== false — NOT addressed. isEnabled() uses getSystemValue('allow_subadmins', false) !== false. A config value of string 'false' or '0' would be truthy under !== false and thus enable the feature — the opposite of the operator's intent and a fail-open surprise for that input. Recommend === true (strict opt-in) to match the documented 'allow_subadmins' => true contract, or normalize via a bool cast. The same pattern is duplicated in settings/users.php (item 2), reinforcing the value of centralizing on the manager.

Code quality / style

• The new acceptance bootstrap is clean: idempotent enableSubadminFeature() guard and config revert in @AfterScenario avoid cross-scenario leakage. Tracking via $subadminFeatureEnabled is appropriate.
• createSubAdmin() resolves L10N via \OC::$server->getL10N('lib') (service-locator) rather than the injected container — consistent with surrounding legacy code, acceptable here.
• Minor: the HintException is caught as \OC\HintException in two call sites; fine for this codebase.

Specific suggestions

• Add the changelog entry (item 1) — this is the main blocker for a default-changing security fix.
• Tighten isEnabled() to === true (item 3) to remove the fail-open string-config edge case.
• Optionally simplify the redundant gate in settings/users.php (item 2).

Summary

The security posture is solid and unchanged: the default-off gate is enforced fail-closed and consistently at every entry point, with no bypass via stale rows. The only new work since the prior review (commit 40da2a90) is welcome acceptance-test coverage for the disabled path. However, all three previously-flagged items remain unaddressed — most importantly the missing changelog (item 1) and the !== false fail-open edge case (item 3). Recommend addressing at least items 1 and 3 before merge.

[phil-davis]

I am running full-ci there to verify that all the other API acceptance tests pass.

#41636 has passed, all API acceptance tests pass.

You can cherry-pick the changelog from there, or write and push your own text to here.

[DeepDiver1975]

🤖 Automated review by Claude Code review agent.

Overview — what changed since the last review (40da2a9 → abbb607)

A gh api commit comparison shows the head is ahead by exactly one commit (abbb6076, "chore: add changelog for allow_subadmins setting") with no other code changes:

• Added changelog/unreleased/41634 (8 lines, "Security: disable group-admin feature by default behind allow_subadmins").

No security-relevant source files (lib/private/SubAdmin.php, lib/private/Group/Manager.php, the OCS/provisioning entry points, settings/users.php, the legacy guards) changed since the previous review. The fail-closed gating analysis from the 40da2a9 review therefore still holds in full.

Security re-confirmation (gate is still fail-closed at all entry points)

Re-verified against the current diff:

• isEnabled() reads getSystemValue('allow_subadmins', false) \!== false — default-off, single source of truth.
• isSubAdmin() keeps the real-admin short-circuit first, then returns false for group-admin-only users when disabled. Authorization cannot be bypassed by a stale group_admin row: every read path (isSubAdminofGroup, getSubAdminsGroups, getGroupsSubAdmins, getAllSubAdmins) returns empty/false before any DB query when disabled.
• Write path (createSubAdmin) throws HintException; both callers — OCS addSubAdmin and settings/ajax/togglesubadmins.php — now catch it and surface a clean error instead of a 500.
• deleteSubAdmin and the post_delete* cleanup hooks remain enabled (privilege-reducing only), so admins can prune dormant assignments.
• Test coverage for both enabled and disabled states is present: testCreateSubAdminThrowsWhenDisabled, testReadMethodsReturnEmptyWhenDisabled, testIsSubAdminAsAdminWhenDisabled (admin short-circuit preserved), testDeleteSubAdminWorksWhenDisabled; acceptance scenario added for the OCS-disabled path (status 103). setUp enables the feature so legacy assertions still exercise it.

No new authz bypass paths introduced. The change is functionally a no-op on the security surface.

Status of the 3 prior-flagged items

• (a) Missing changelog for breaking upgrade behavior — ADDRESSED. changelog/unreleased/41634 is added in this exact commit. It states the feature is disabled by default and how to opt back in. Two minor nits in the changelog text:
  • Missing space after the comma: system config,as a security → config, as a.
  • It does not explicitly call out the breaking upgrade behavior — i.e. that on upgrade, existing group-admin assignments stop taking effect until allow_subadmins => true is set. Worth a sentence for admins reading release notes. (If the separate changelog PR #41636/#41638 is the canonical one, consider whether this file is redundant with it.)
• (b) Redundant flag read in settings/users.php — STILL OPEN. Line still inlines $config->getSystemValue('allow_subadmins', false) \!== false rather than routing through the central SubAdmin chokepoint. Not a security issue (it is an extra, correct guard, and getAllSubAdmins() is itself gated), but it duplicates the gate logic outside the manager and can drift. Minor.
• (c) String-config edge case in \!== false — STILL OPEN. getSystemValue('allow_subadmins', false) \!== false treats any non-false value as enabled, including the string "false" or "0" that a hand-edited config.php might contain — these would (counter-intuitively) enable the feature. This is fail-open relative to operator intent for the string case. Low likelihood given the documented => false boolean form and the acceptance harness uses a real boolean, but a stricter check (e.g. cast/=== true) would be more robust. Note: tightening to === true would need the acceptance helper to keep passing a true boolean, which it does (setSystemConfig(..., 'true', ..., 'boolean')).

Code quality / style

• Clean, well-scoped, centralized in the manager. Consistent guard placement (early return before DB access).
• config.sample.php documentation is clear and includes the security note.

Specific suggestions (all non-blocking)

1. Fix the missing space in the changelog (config,as → config, as).
2. Optionally mention the upgrade-time behavior change in the changelog for operators.
3. Consider replacing inline reads in settings/users.php with a manager method to keep the gate single-sourced (item b).
4. Consider === true / boolean cast for the config check to harden against string config values (item c).

Verdict

No blocking issues. The security gate remains fail-closed at all entry points and is well tested. Prior item (a) is resolved by this commit; items (b) and (c) remain as minor, non-blocking improvements.

[phil-davis]

I fixed the spacing in the changelog that the review mentioned:
config,as → config, as

and pushed again.

Hopefully the automated review will be happier now.

[DeepDiver1975]

🤖 Automated review by Claude Code review agent.

Overview — what changed since the last review (abbb6076 → 3648fe54)

The branch was rebased onto a newer master (diverged: 4 ahead / 3 behind) and the feature commit was reworded, but the substantive security logic is unchanged from the previously reviewed abbb6076. The only functional deltas in this range are housekeeping:

• chore: add changelog for allow_subadmins setting — added changelog/unreleased/41634 (resolves prior item (a), the changelog, which was already noted as resolved).
• test: add acceptance test for when the subadmin feature is disabled — adds a @smokeTest scenario in createSubAdmin.feature plus the enableSubadminFeature() / cleanupSubadminFeature() (@AfterScenario) plumbing in bootstrap/Provisioning.php, so the default-off feature can be exercised and reverted per scenario.
• A merge/rebase touched an unrelated PHPdependencies… changelog (guzzle psr7) — not part of this feature.

The core gate in lib/private/SubAdmin.php, Manager.php, Users.php, togglesubadmins.php, and settings/users.php is byte-for-byte the same as the last reviewed head. No new entry points were introduced.

Fail-closed gate confirmation (re-verified at 3648fe54)

Confirmed the default-off gate is still fail-closed at all entry points:

• isEnabled() reads getSystemValue('allow_subadmins', false) — default false, so unconfigured = disabled.
• isSubAdmin() keeps the real-admin short-circuit first, then returns false when disabled — so all downstream consumers (User.php permission bypass, settings middleware, legacy menu/access guards) fail closed. Real admins correctly retain access.
• All read methods (isSubAdminofGroup, getSubAdminsGroups, getGroupsSubAdmins, getAllSubAdmins) return empty/false when disabled → no authz bypass via stale group_admin rows.
• createSubAdmin() throws HintException; both write callers catch it and surface a clean error (OCS 103 / JSON error) instead of a 500.
• deleteSubAdmin() + post_delete* cleanup hooks remain enabled (privilege-reducing, safe).

Test coverage for both enabled and disabled paths is present and adequate (SubAdminTest.php: 4 disabled-path tests incl. admin short-circuit preserved, empty reads over a real dormant row, throw-on-create, delete-still-works; plus the new disabled acceptance scenario).

Status of prior open items

• (b) Redundant flag read in settings/users.php — NOT addressed (still present). The added && $config->getSystemValue('allow_subadmins', false) !== false guard duplicates the gate already enforced inside getAllSubAdmins(), which returns [] when disabled. It is functionally harmless (and acts as a minor fast-path avoiding a DB call), so it is not a blocker — but it is redundant defense and slightly couples the page to the config key name. Optional cleanup, not required for merge.
• (c) !== false string-config edge case — NOT addressed (unchanged). getSystemValue('allow_subadmins', false) !== false treats any non-false value as enabled. A misconfiguration like 'allow_subadmins' => 'false' (string) or '0' would evaluate to enabled (string 'false' !== false). This is a fail-open edge under operator misconfiguration. Given the secure default is false and correct boolean usage is documented in config.sample.php, real-world risk is low, but a stricter check (=== true, or filter_var(..., FILTER_VALIDATE_BOOLEAN)) would be more robust and would also make the isEnabled() helper and the users.php guard consistent. Recommended, low priority.

Code quality / style

• Centralizing the gate in the OC\SubAdmin chokepoint with a single private isEnabled() helper is the right design — one place to reason about.
• The provisioning addSubAdmin try/catch correctly maps HintException to OCS 103 with the hint message; consistent with the AJAX handler.
• Acceptance-test plumbing (enable + @AfterScenario revert via SetupHelper::setSystemConfig/deleteSystemConfig) is clean and idempotent (guarded by $subadminFeatureEnabled).
• Minor: the disabled getAllSubAdmins short-circuit and the users.php guard express the same intent in two ways (see item (b)).

Potential issues / risks

• Only the string-config edge case (item c) remains as a theoretical fail-open under operator misconfiguration; secure-by-default behavior is correct.
• No bypass found via stale DB rows: every read path is gated.
• No new risks introduced by the rebase/changelog/test commits.

Verdict

Approve in spirit — the security objective (fail-closed, default-off group-admin) is correctly and consistently implemented and well tested. Items (b) and (c) remain open but are both non-blocking; addressing (c) with a strict boolean check would be the most valuable follow-up.

[DeepDiver1975]

🤖 Automated review by Claude Code review agent.

Overview — what changed since the last review (3648fe5 → 7964448)

Exactly one new commit (79644482, "strict boolean gate and single-source isEnabled()") sits on top of the previously reviewed head. It is ahead by 1 / behind by 0 and touches three files with surgical, low-risk changes:

• lib/private/SubAdmin.php — isEnabled() visibility changed private → public, and the gate condition tightened from \!== false to strict === true.
• settings/users.php — the inline config read ($config->getSystemValue('allow_subadmins', false) \!== false) replaced with \OC::$server->getGroupManager()->getSubAdmin()->isEnabled().
• changelog/unreleased/41634 — added two lines documenting the breaking upgrade behavior (existing group-admin assignments ignored until allow_subadmins => true).

No production code paths beyond these were altered since the last review, so the previously confirmed fail-closed enforcement at all entry points (OCS SUBADMIN_AUTH, middleware, User.php permission bypass, legacy menu/access guards) remains intact and unchanged.

Status of prior open items

• (b) Redundant flag read in settings/users.php — RESOLVED. The duplicated inline getSystemValue read is gone; the page now gates the "Group Admin for" column through the single-source SubAdmin::isEnabled(). There is now exactly one place that interprets the allow_subadmins config, which is the correct design. This also keeps settings/users.php consistent with getAllSubAdmins(), which itself returns [] when disabled — so even if the guard regressed, the page would degrade safely.

• (c) String-config edge case in the \!== false check — RESOLVED. The gate is now === true. A hand-edited config.php with 'allow_subadmins' => 'false', '0', '', 0, or null now fails closed (previously a non-empty string like 'false' would have been \!== false and failed open). Only the documented boolean true opt-in enables the feature. This is the strictly safer interpretation and matches the documented contract in config.sample.php ('allow_subadmins' => false).

Security review — fail-closed verification (re-confirmed at 7964448)

• Single chokepoint preserved. All ~28 consumers still route through GroupManager::getSubAdmin() → OC\SubAdmin, which now has IConfig injected and a single isEnabled() authority.
• isSubAdmin() ordering correct. Real-admin short-circuit (groupManager->isAdmin) runs first, then \!isEnabled() → return false. Real admins retain access; group-admin-only users lose elevation when disabled. No stale group_admin rows can grant authority because the row query is never reached when disabled.
• Read methods fail closed. getSubAdminsGroups, getGroupsSubAdmins, getAllSubAdmins, isSubAdminofGroup all short-circuit to empty/false before touching the DB — dormant rows are invisible.
• isUserAccessible inherits the gate transitively via getSubAdminsGroups/isSubAdminofGroup, so no separate bypass exists.
• Write path fails closed loudly. createSubAdmin() throws HintException; both callers (settings/ajax/togglesubadmins.php, provisioning addSubAdmin) catch it and surface a clean error (OCS 103 / JSON error) rather than a 500.
• deleteSubAdmin() and post_delete* cleanup hooks stay enabled — privilege-reducing operations remain available so admins can prune assignments while the feature is off. Confirmed by testDeleteSubAdminWorksWhenDisabled.
• Config read correct. getSystemValue('allow_subadmins', false) defaults to false; === true is the right comparison for a strict boolean opt-in.

Test coverage

Coverage for both states is intact and the new strict semantics are exercised:

• Enabled path: setUp() sets allow_subadmins => true so all legacy assertions still exercise the feature; constructor updated to pass IConfig.
• Disabled path: testCreateSubAdminThrowsWhenDisabled, testReadMethodsReturnEmptyWhenDisabled (covers all four reads + isSubAdmin/isUserAccessible), testIsSubAdminAsAdminWhenDisabled (admin short-circuit preserved), testDeleteSubAdminWorksWhenDisabled.
• Acceptance: new @smokeTest scenario asserts OCS 103 when creating a subadmin with the feature disabled; enableSubadminFeature()/cleanupSubadminFeature() (@AfterScenario) toggle via SetupHelper::setSystemConfig(..., 'boolean'), which writes a real boolean and therefore correctly satisfies the new === true gate.

One minor note: there is no unit test asserting that a string value like 'false' fails closed (the new (c) behavior). The acceptance harness sets a proper boolean, so the strict-string-handling change is covered only implicitly. Adding a one-line assertion (setSystemValue('allow_subadmins', 'true'); assertFalse($subAdmin->isEnabled())) would lock in the fail-closed contract against future regressions. Non-blocking.

Code quality / style

• Clean, minimal diff. Making isEnabled() public to serve as the single source of truth is the right call and eliminates the duplication flagged previously.
• Changelog now correctly documents the breaking upgrade behavior, which is important given this silently ignores existing assignments on upgrade.
• No new lint/style concerns; the === true is more explicit and self-documenting than the prior \!== false.

Verdict

Both previously open items (b) and (c) are fully resolved. The default-off gate remains fail-closed at every entry point, the config read is correct and now strict, and no authz bypass via stale group_admin rows is possible. The only remaining suggestion is a non-blocking unit test for the strict string-config behavior. LGTM.