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

ACL: Inserting multiple ObjectFieldAces throws DB constraint violations (sf 2.3.1 - 2.3.6) #9433

Closed
mogoman opened this issue Nov 3, 2013 · 5 comments

Comments

@mogoman
Copy link

mogoman commented Nov 3, 2013

There seems to be a problem when updating ACL when using object field aces. It is fairy simply to reproduce inside a test controller:

  1. create a simple domain object:

    $demoObj         = new Demo();
    $demoObj->setId(2);
    
    $oid  = ObjectIdentity::fromDomainObject($demoObj);
    $fieldName = "name";
    
  2. give the object a simple ROLE based ACL:

    $aclProvider    = $this->get('security.acl.provider');
    $acl  = $aclProvider->createAcl($oid);
    
    $roleUser  = new RoleSecurityIdentity('ROLE_USER');
    $mask      = new MaskBuilder(4); // 4 = EDIT
    
    $acl->insertobjectFieldAce($fieldName, $roleUser, $mask->get());
    
    $aclProvider->updateAcl($acl);
    
  3. add another ROLE to the ACL:

    $acl  = $aclProvider->findAcl($oid);
    $roleUser  = new RoleSecurityIdentity('ROLE_FOO');
    $mask      = new MaskBuilder(4); // 4 = EDIT
    
    $acl->insertobjectFieldAce($fieldName, $roleUser, $mask->get());
    $aclProvider->updateAcl($acl);
    
  4. add one more ROLE to the ACL:

    $acl  = $aclProvider->findAcl($oid);
    $roleUser  = new RoleSecurityIdentity('ROLE_BAR');
    $mask      = new MaskBuilder(4); // 4 = EDIT
    
    $acl->insertobjectFieldAce($fieldName, $roleUser, $mask->get());
    $aclProvider->updateAcl($acl);
    

running step 4 produces the following:

    An exception occurred while executing 'UPDATE acl_entries SET ace_order
    = 1 WHERE id = 40':

    SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry
    '19-22-name-1' for key 
    'UNIQ_46C8B806EA000B103D9AB4A64DEF17BCE4289BF4'

If you remove the offending index from the database, the update works perfectly (the ace_order), so the problem seems to be that Symfony is not updating the ACL items in the correct order.

The above does not affect class field aces.

There is a second issue related to the above where the following throws the same execption:

    $index = 0;
    $acl->deleteObjectFieldAce($index, $fieldName);
    $acl->insertObjectFieldAce($fieldName, $roleUser, $mask->get());
    $aclProvider->updateAcl($acl);

there is a workaround by doing:

    $index = 0;
    $acl->deleteObjectFieldAce($index, $fieldName);
    $aclProvider->updateAcl($acl);
    $acl->insertObjectFieldAce($fieldName, $roleUser, $mask->get());
    $aclProvider->updateAcl($acl);

Once again this does not affect class field aces.

@jillro
Copy link

jillro commented Nov 9, 2013

@jillro
Copy link

jillro commented Nov 9, 2013

Reading MutableAclProvider.php updateFieldAceProperty method, I think classFieldAce and objectFieldAce updates follow the same process.

In every case, there is a moment in the process where to ACE have the same value for the order field. That's a problem because as we can see in Schema.php there is a UNIQUE key on class_id, object_identity_id, field_name, and ace_order.

The bug does not occure when we use ClassField, Object or Class scope because in those case at least one of the field is NULL so the unique constraint does not apply. (Which by the way is not what I would have expected from MySQL but that's another question...). But in any case, the problem is that at some moment of the update process, some entries have the same order value, whatever the scope of the entry.

@jillro
Copy link

jillro commented Nov 9, 2013

The problem seems to occur in fact during execution of updateAces() method. We should iterate the ACE in the reverse order to update them. I am working on a patch.

@mogoman
Copy link
Author

mogoman commented Nov 9, 2013

Thanks for the update and reproducing the problem (and patching of course...)

jillro pushed a commit to jillro/symfony that referenced this issue Nov 9, 2013
jillro pushed a commit to jillro/symfony that referenced this issue Nov 9, 2013
@jillro
Copy link

jillro commented Nov 10, 2013

I wrote a new test, and if we iterate the ACE in reverse order, the same problem appears when we delete ACEs. Need to rework on it.

jillro pushed a commit to jillro/symfony that referenced this issue Nov 10, 2013
jillro pushed a commit to jillro/symfony that referenced this issue Nov 10, 2013
fabpot pushed a commit that referenced this issue Nov 22, 2013
fabpot added a commit that referenced this issue Nov 22, 2013
This PR was squashed before being merged into the 2.2 branch (closes #9485).

Discussion
----------

[Acl] Fix for issue #9433

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9433
| License       | MIT
| Doc PR        |

Two new test for issue #9433 :
`testUpdateAclInsertingMultipleObjectFieldAcesThrowsDBConstraintViolations()`
`testUpdateAclDeletingObjectFieldAcesThrowsDBConstraintViolations()`

The change to `updateAces()` line 857 is enough to make the first test succeed. When changing the `order` field value to a higher value, we must first change the value of the next entry (and all the next entries recursively) to preserve uniqueness of the `order` field in the database.

All the other changes are for the second test. In the former `updateAcl()` method, we commit the changes of the existing ACEs to the database before deleting or adding the new ones. We must delete the old ACEs before changing the existing ACEs in order to preserve uniqueness of the `order` field in the database.

Commits
-------

a38fab9 [Acl] Fix for issue #9433
@fabpot fabpot closed this as completed Nov 22, 2013
fabpot added a commit that referenced this issue Nov 23, 2013
* 2.2:
  No Entity Manager defined exception
  fixed CS
  [Acl] Fix for issue #9433
  [Validator] fix docblock typos
  [DependencyInjection] removed the unused Reference and Parameter classes use statements from the compiled container class
  Fix mistake in translation's service definition.
  if handler_id is identical to null fix
  CS fix
  Fixed ModelChoiceList tests in Propel1 bridge.
  [AclProvider] Fix incorrect behaviour when partial results returned from cache
  Check if the pipe array is empty before calling stream_select()
  re-factor Propel1 ModelChoiceList
  [Locale] fixed the failing test described in #9455
  [Process] fix phpdoc and timeout of 0
  bug #9445 [BrowserKit] fixed protocol-relative url redirection

Conflicts:
	src/Symfony/Component/BrowserKit/Tests/ClientTest.php
	src/Symfony/Component/Locale/Tests/Stub/StubIntlDateFormatterTest.php
fabpot added a commit that referenced this issue Nov 23, 2013
* 2.3: (24 commits)
  Add german translation for several validators (Greater/Equal/Less)
  No Entity Manager defined exception
  fixed CS
  [Acl] Fix for issue #9433
  [Validator] fix docblock typos
  [DependencyInjection] removed the unused Reference and Parameter classes use statements from the compiled container class
  Removed useless check if self::$trustProxies is set
  Fix mistake in translation's service definition.
  if handler_id is identical to null fix
  CS fix
  Fixed ModelChoiceList tests in Propel1 bridge.
  [AclProvider] Fix incorrect behaviour when partial results returned from cache
  Check if the pipe array is empty before calling stream_select()
  [Intl] fixed datetime test as described in #9455
  bumped Symfony version to 2.3.8
  updated VERSION for 2.3.7
  updated CHANGELOG for 2.3.7
  re-factor Propel1 ModelChoiceList
  [Form] Added method Form::getClickedButton() to remove memory leak in FormValidator
  [Locale] fixed the failing test described in #9455
  ...

Conflicts:
	src/Symfony/Bridge/Propel1/Form/ChoiceList/ModelChoiceList.php
	src/Symfony/Bridge/Propel1/Tests/Fixtures/ItemQuery.php
	src/Symfony/Bridge/Propel1/Tests/Form/ChoiceList/ModelChoiceListTest.php
	src/Symfony/Bridge/Propel1/Tests/Propel1TestCase.php
	src/Symfony/Component/Form/Tests/CompoundFormTest.php
	src/Symfony/Component/HttpKernel/Kernel.php
	src/Symfony/Component/Process/Process.php
jderusse pushed a commit to jderusse/symfony that referenced this issue Dec 15, 2020
jderusse pushed a commit to jderusse/symfony that referenced this issue Dec 15, 2020
This PR was squashed before being merged into the 2.2 branch (closes symfony#9485).

Discussion
----------

[Acl] Fix for issue symfony#9433

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#9433
| License       | MIT
| Doc PR        |

Two new test for issue symfony#9433 :
`testUpdateAclInsertingMultipleObjectFieldAcesThrowsDBConstraintViolations()`
`testUpdateAclDeletingObjectFieldAcesThrowsDBConstraintViolations()`

The change to `updateAces()` line 857 is enough to make the first test succeed. When changing the `order` field value to a higher value, we must first change the value of the next entry (and all the next entries recursively) to preserve uniqueness of the `order` field in the database.

All the other changes are for the second test. In the former `updateAcl()` method, we commit the changes of the existing ACEs to the database before deleting or adding the new ones. We must delete the old ACEs before changing the existing ACEs in order to preserve uniqueness of the `order` field in the database.

Commits
-------

a38fab9 [Acl] Fix for issue symfony#9433
jderusse pushed a commit to jderusse/symfony that referenced this issue Dec 15, 2020
* 2.2:
  No Entity Manager defined exception
  fixed CS
  [Acl] Fix for issue symfony#9433
  [Validator] fix docblock typos
  [DependencyInjection] removed the unused Reference and Parameter classes use statements from the compiled container class
  Fix mistake in translation's service definition.
  if handler_id is identical to null fix
  CS fix
  Fixed ModelChoiceList tests in Propel1 bridge.
  [AclProvider] Fix incorrect behaviour when partial results returned from cache
  Check if the pipe array is empty before calling stream_select()
  re-factor Propel1 ModelChoiceList
  [Locale] fixed the failing test described in symfony#9455
  [Process] fix phpdoc and timeout of 0
  bug symfony#9445 [BrowserKit] fixed protocol-relative url redirection

Conflicts:
	src/Symfony/Component/BrowserKit/Tests/ClientTest.php
	src/Symfony/Component/Locale/Tests/Stub/StubIntlDateFormatterTest.php
jderusse pushed a commit to jderusse/symfony that referenced this issue Dec 15, 2020
* 2.3: (24 commits)
  Add german translation for several validators (Greater/Equal/Less)
  No Entity Manager defined exception
  fixed CS
  [Acl] Fix for issue symfony#9433
  [Validator] fix docblock typos
  [DependencyInjection] removed the unused Reference and Parameter classes use statements from the compiled container class
  Removed useless check if self::$trustProxies is set
  Fix mistake in translation's service definition.
  if handler_id is identical to null fix
  CS fix
  Fixed ModelChoiceList tests in Propel1 bridge.
  [AclProvider] Fix incorrect behaviour when partial results returned from cache
  Check if the pipe array is empty before calling stream_select()
  [Intl] fixed datetime test as described in symfony#9455
  bumped Symfony version to 2.3.8
  updated VERSION for 2.3.7
  updated CHANGELOG for 2.3.7
  re-factor Propel1 ModelChoiceList
  [Form] Added method Form::getClickedButton() to remove memory leak in FormValidator
  [Locale] fixed the failing test described in symfony#9455
  ...

Conflicts:
	src/Symfony/Bridge/Propel1/Form/ChoiceList/ModelChoiceList.php
	src/Symfony/Bridge/Propel1/Tests/Fixtures/ItemQuery.php
	src/Symfony/Bridge/Propel1/Tests/Form/ChoiceList/ModelChoiceListTest.php
	src/Symfony/Bridge/Propel1/Tests/Propel1TestCase.php
	src/Symfony/Component/Form/Tests/CompoundFormTest.php
	src/Symfony/Component/HttpKernel/Kernel.php
	src/Symfony/Component/Process/Process.php
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

No branches or pull requests

3 participants