IBX-12454 Dangling policy limitation values block role copy by vidarl · Pull Request #824 · ibexa/core

vidarl · 2026-09-09T16:04:22Z

🎫 Issue	IBX-12454

Description:

RoleService::copyRole() rebuilds a RoleCreateStruct from the existing Role's persisted
Policies and runs create-time validation over it, so a Role the Repository loads without complaint
cannot be copied once a Limitation value names a deleted entity. deleteContentType() leaves such
values behind. Node, Subtree, Section, ObjectState, Role and MemberOf limitations dangle the
same way.

copyRole()
already documents validation of "a policy limitation in the $roleCopyStruct" — the caller's
struct, not the source Role. Moved the validation above the loop that appends the copied Role's
Policies. Values that no longer resolve are copied rather than pruned: an empty Limitation array
becomes '*' in RoleDomainMapper::buildPersistencePolicyObject(), so dropping the last value of
a Limitation would widen the Policy instead of narrowing it.

PR also contains a ibexa:roles:cleanup-limitation-values command that repairs existing installations:

It only reports dangling policy values by default
--fix tries to fix (remove) dangling values
- Removes a Policy rather than leaving one of its Limitations empty.
- UserPermissions limitations with two-dimensional lists are covered too
  - "roles" [15] with 15 gone, "user_groups" [11]: reported only, never changed — pruning would
    leave "roles" [], which grants every Role, while removing the Policy would take away the
    grant "user_groups" [11] still carries
  - "roles" [15] and "user_groups" [11] with both gone: Policy removed, since it grants nothing as it
    stands

Two unrelated defects are fixed alongside,

getLimitationErrors() returned null on 4.6 (fixed on 5.0 by 72cf197, never backported)
MemberOfLimitationType and RoleLimitationType rejected the string values storage hands them.

For QA:

See ticket for how to reproduce.

The command reports the same values, and is the quickest way to find a Role to test against:

php bin/console ibexa:roles:cleanup-limitation-values                  # report only
php bin/console ibexa:roles:cleanup-limitation-values --fix            # asks before writing
php bin/console ibexa:roles:cleanup-limitation-values --fix --force    # no question, for scripts

Documentation:

New maintenance command ibexa:roles:cleanup-limitation-values — reports Policy Limitation values
that no longer resolve to an existing entity, and removes them under --fix (--force skips the
confirmation). Worth documenting that a Policy left with no Limitation values is removed rather than
emptied, and that a UserPermissions Limitation is left to a human whenever repairing it would
change what the Policy grants, since that limitation reads an empty list as no restriction on that
dimension.

…yRole()

…alues

…urning null

…tion

sonarqubecloud · 2026-09-09T16:51:04Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.7% Duplication on New Code

See analysis details on SonarQube Cloud

konradoboza · 2026-09-11T06:41:25Z

+<error>Before you continue:</error>
+- Make sure to back up your database.
+- Take the installation offline. The database should not be modified while the script is being executed.
+- Run this command in production environment using <info>--env=prod</info>
+EOT;


Indentation.

konradoboza · 2026-09-11T06:42:07Z

+                self::DEFAULT_REPOSITORY_USER
+            )
+            ->setHelp(
+                <<<EOT


Indentation.

konradoboza · 2026-09-11T06:45:13Z

+        ) {
+            $output->writeln('<comment>Nothing was changed. Use --force to skip this question.</comment>');
+
+            return 0;


Suggested change

return 0;

return Command::SUCCESS;

konradoboza · 2026-09-11T06:46:26Z

+            $output->writeln(sprintf('<info>Role "%s" (%d) updated.</info>', $role->identifier, $roleId));
+        }
+
+        return $failed ? 1 : 0;


Suggested change

return $failed ? 1 : 0;

return $failed ? Command::FAILURE : Command::SUCCESS;

konradoboza · 2026-09-11T06:52:46Z

+    private const MISSING_ENTITY_MESSAGE = "limitationValues[%key%] => '%value%' does not exist in the backend";
+
+    /** @var \Ibexa\Contracts\Core\Repository\RoleService&\PHPUnit\Framework\MockObject\MockObject */
+    private $roleService;


Suggested change

private $roleService;

private RoleService $roleService;

konradoboza · 2026-09-11T06:55:47Z

+     */
+    private function buildRepository(): MockObject
+    {
+        $userService = $this->createMock(UserService::class);


Shouldn't this be a Stub instead? See https://github.com/ibexa/internal-ai/pull/3.

konradoboza · 2026-09-11T06:56:35Z

+    /**
+     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\Exception
+     */
+    public function testCopyRoleWithLimitationValueOfDeletedContentType(): void


I presume this one fails before introducing your changes, right?

konradoboza · 2026-09-11T06:57:31Z

+        $roleCopyStruct->addPolicy($policyCreateStruct);
+
+        try {
+            $roleService->copyRole(


Wouldn't it be more readable to have ->expectException here?

konradoboza · 2026-09-11T06:58:02Z

+    /**
+     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\Exception
+     */
+    private function createContentType(): int


Don't we have a helper in the testing framework somewhere doing exactly that?

konradoboza · 2026-09-11T06:58:08Z

+    /**
+     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\Exception
+     */
+    private function createRoleWithContentTypeLimitation(int $contentTypeId): Role


Same question.

mikadamczyk · 2026-09-11T07:24:27Z

+
+        $danglingValues = [];
+
+        if ($this->isListOfValues($limitationValues)) {


Could we only auto-fix Limitation types that have an explicit repair rule? Limitation types can be added by extensions. This command currently decides how to remove values only from the array shape and the validation message. It does not know what an empty list means for a custom Limitation type. For example, an empty list may mean "allow everything" for one type, but "allow nothing" for another. Running --fix could therefore change permissions unexpectedly. Could we report unknown or custom types and leave them for manual repair instead?

mikadamczyk · 2026-09-11T07:27:26Z

+    {
+        // Emptying "roles" would make UserPermissionsLimitationType::evaluate() grant every Role,
+        // and storage cannot tell an emptied value list from one that was never restricted.
+        $limitation = new ContentTypeLimitation(['limitationValues' => ['roles' => [15], 'user_groups' => [11]]]);


Could we cover this with the real UserPermissionsLimitation and its Limitation Type? This test puts UserPermissions-shaped values into ContentTypeLimitation and uses a generic mock, so it does not verify the real acceptance and permission semantics that make --fix security-sensitive.

mikadamczyk · 2026-09-11T07:54:34Z

+     *
+     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\BadStateException
+     */
+    private function applyToDraft(RoleDraft $roleDraft, array $findingsByPolicyId, Role $role, int $roleId): void


$roleId here duplicates $role->id. $role is loaded from that same $roleId right above, in line 241, so they're always equal. Could we drop the $roleId parameter from applyToDraft() and use $role->id at its one use site? Same redundancy shows up in fix() itself (lines 252, 262, 267, 287, 295), where $roleId (the loop key) and $role->id are used interchangeably for the same value. Picking one consistently would remove the duplication.

mikadamczyk · 2026-09-11T07:55:53Z

+     *
+     * @return \Ibexa\Contracts\Core\Repository\Values\User\Limitation[]
+     */
+    private function pruneLimitations(Policy $policyDraft, array $findings): array


pruneLimitations() types its parameter as Policy $policyDraft, but the only caller always passes an actual PolicyDraft. Could we tighten the type to PolicyDraft so the signature matches what it's actually called with, instead of the wider parent type paired with a "draft" name?

mikadamczyk · 2026-09-11T07:58:01Z

+            );
+    }
+
+    protected function execute(InputInterface $input, OutputInterface $output): int


execute() does five different things inline: scan all roles, print skipped items, print "nothing found" short-circuit, render the findings table, and drive the confirm and fix flow. Could we extract the scanning loop building $findings and $skipped into a private scan(): array returning both lists? This would shorten execute() to orchestration only and make the scan logic independently testable and readable, without the CLI plumbing around it.

mikadamczyk · 2026-09-11T08:05:20Z

+/**
+ * @internal
+ */
+final class DanglingLimitationValues


Every call site names variables of this type $finding or $findings, never $danglingLimitationValues. This is really a per-Limitation finding not just a values holder. Also, the file already uses two different words for the same idea: "dangling" in class and method names, but "missing" in MISSING_ENTITY_SUFFIX and "no longer resolve" in the command's own description. Could we settle on one, e.g. UnresolvedLimitationFinding (matches the command's own wording), instead of adding "dangling" as a third term?

mikadamczyk · 2026-09-11T08:16:09Z

@@ -49,8 +49,12 @@ public function acceptValue(APILimitationValue $limitationValue): void
        }

        foreach ($limitationValue->limitationValues as $key => $id) {


This block is duplicated almost verbatim in RoleLimitationType::acceptValue() (differs only in the regex allowing a leading -). Both classes already extend AbstractPersistenceLimitationType. Could we move this into a shared protected helper there, e.g. normalizeIntId($id, string $key, bool $allowNegative = false), and call it from both acceptValue() methods?

IBX-12454: Validate only caller-supplied Policies in RoleService::cop…

a396b8d

…yRole()

vidarl requested a review from a team September 9, 2026 16:11

vidarl added Doc needed The changes require some documentation Ready for review labels Sep 9, 2026

ibexa-workflow-automation-1 Bot requested review from Steveb-p, ViniTou, alongosz, barw4, bnowak, ciastektk, konradoboza, mikadamczyk, tbialcz and wiewiurdp and removed request for a team September 9, 2026 16:12

vidarl changed the title ~~IBX-12454 dangling policy limitation values block role copy 4.6~~ IBX-12454 Dangling policy limitation values block role copy Sep 9, 2026

Vidar Langseid added 6 commits September 9, 2026 18:48

Added integration tests for copying a Role with dangling Limitation v…

734847e

…alues

Fixed LimitationValidationException::getLimitationErrors() always ret…

42a366d

…urning null

Added ibexa:roles:cleanup-limitation-values command

055eab3

Dropped obsolete PHPStan baseline entry for LimitationValidationExcep…

d81513f

…tion

Documented the actual shape of getLimitationErrors() per throwing method

89b7557

Normalized stored Limitation values in MemberOf and Role acceptValue()

0b493aa

vidarl force-pushed the IBX-12454_Dangling_policy_limitation_values_block_role_copy_4.6 branch from 5fbb9f6 to 0b493aa Compare September 9, 2026 16:48

konradoboza reviewed Sep 11, 2026

View reviewed changes

mikadamczyk reviewed Sep 11, 2026

View reviewed changes

	return $failed ? 1 : 0;
	return $failed ? Command::FAILURE : Command::SUCCESS;


		$danglingValues = [];

		if ($this->isListOfValues($limitationValues)) {

		@@ -49,8 +49,12 @@ public function acceptValue(APILimitationValue $limitationValue): void
		}

		foreach ($limitationValue->limitationValues as $key => $id) {

Conversation

vidarl commented Sep 9, 2026

Description:

For QA:

Documentation:

Uh oh!

sonarqubecloud Bot commented Sep 9, 2026

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants