Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PIM-9779: fix ACE orders after finding ACLs #14206

Merged
merged 3 commits into from
Apr 1, 2021
Merged

PIM-9779: fix ACE orders after finding ACLs #14206

merged 3 commits into from
Apr 1, 2021

Conversation

nmarniesse
Copy link
Collaborator

@nmarniesse nmarniesse commented Mar 31, 2021

Fix https://akeneo.atlassian.net/browse/PIM-9779

On a very specific environment, we cannot add a specific ACL for a specific role. This behaviour is very strange, but we are not the only one to observe the problem. It's in the symfony/security-acl bundle, it's exactly the same as:
symfony/security-acl#5
symfony/security-acl#23
symfony/security-acl#24
symfony/security-acl#28

To fix it a PR was opened symfony/security-acl#29, approved by one person, but never merged.

We had 2 choices:

  • fix the problem by code by applying the patch described by the PR (or equivalent)
  • fix by SQL query

We think the fix by SQL query is too risky to be executed on all customers. Because we don't understand all the usecases/features of the security-acl bundle, and it's difficult to go back in case of errors/side effects/whatever
Plus there is a internal cache for ACLs that complicates the operation.

The fix

The fix is an adaptation of the PR: after fetching the ACLs we re-order the ACEs. My first idea was to applied strictly the PR modification, but the hydrateObjectIdentities method is private and can't be override easily (too many private things + too many inheritence).
So we fix the ACE order after loading the ACLs.

Bonus

For information here is a query to return the "bad" entries in the database. The ace_order should begin by 0 and should be incremental by class_id

WITH acl_entries_with_expected_ace_order AS (
    SELECT id,
           class_id,
           security_identity_id,
           ace_order,
           ROW_NUMBER() over (PARTITION BY class_id ORDER BY ace_order) - 1 AS expected_ace_order
    FROM acl_entries
    WHERE object_identity_id IS NULL -- if we remove this condition, we have a lot of additional entries. Is it normal?
    ORDER BY class_id, ace_order
)
SELECT * FROM acl_entries_with_expected_ace_order WHERE ace_order != expected_ace_order;

@nmarniesse nmarniesse force-pushed the PIM-9779 branch 2 times, most recently from f82689b to fb62f83 Compare March 31, 2021 14:29
Copy link
Contributor

@mmetayer mmetayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -61,5 +61,8 @@
<directory suffix="EndToEnd.php">src</directory>
</testsuite>

<testsuite name="OroSecurityBundle">
<file>src/Oro/Bundle/SecurityBundle/Tests/Unit/Acl/Dbal/MutableAclProviderTest.php</file>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Oro tests were deactivated. Here I re-activate one test file only

@jmleroux jmleroux added this to the Serenity milestone Mar 31, 2021
Copy link
Contributor

@BitOne BitOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job Nini!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants