Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[Security] [ACL] Fix finding ACLs from ObjectIdentity's with different types #7567

Closed
wants to merge 3 commits into from

3 participants

@gordalina
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

If more than one ObjectIdentity with different Type (Class name) is given to AclProvider::findAcls() it would throw an exception stating that it could not find the ACLs

This fixes this issue which was introduced in 2.2.0-RC3 - see commit 3c3a90b

/cc @iBiryukov @schmittjoh

@iBiryukov iBiryukov commented on the diff
src/Symfony/Component/Security/Acl/Dbal/AclProvider.php
@@ -263,9 +263,6 @@ protected function getAncestorLookupSql(array $batch)
for ($i = 0; $i < $count; $i++) {
if (!isset($types[$batch[$i]->getType()])) {
$types[$batch[$i]->getType()] = true;
- if ($count > 1) {

This is clearly wrong. The original intention here was a to carry out a small optimisation.
So, if we have two or more different entries in $types, then their is no need to do further search (it's sufficient for the if/else statement further below).

The code should be something like this:

if(sizeof($types) > 1) break;

Updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@iBiryukov

Great! :+1:

@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot merged branch gordalina/multiple-classes-oids-acls (PR #7567)
This PR was submitted for the master branch but it was merged into the 2.2 branch instead (closes #7567).

Discussion
----------

[Security] [ACL] Fix finding ACLs from ObjectIdentity's with different types

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

If more than one ObjectIdentity with different Type (Class name) is given to AclProvider::findAcls() it would throw an exception stating that it could not find the ACLs

This fixes this issue which was introduced in 2.2.0-RC3 - see commit 3c3a90b

/cc @iBiryukov @schmittjoh

Commits
-------

8b0bb57 [Security] [ACL] Fix finding ACLs from ObjectIdentity's with different types
7379b9e
@fabpot fabpot closed this
@mmucklo mmucklo referenced this pull request from a commit
@fabpot fabpot merged branch gordalina/multiple-classes-oids-acls (PR #7567)
This PR was submitted for the master branch but it was merged into the 2.2 branch instead (closes #7567).

Discussion
----------

[Security] [ACL] Fix finding ACLs from ObjectIdentity's with different types

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

If more than one ObjectIdentity with different Type (Class name) is given to AclProvider::findAcls() it would throw an exception stating that it could not find the ACLs

This fixes this issue which was introduced in 2.2.0-RC3 - see commit 3c3a90b

/cc @iBiryukov @schmittjoh

Commits
-------

8b0bb57 [Security] [ACL] Fix finding ACLs from ObjectIdentity's with different types
9ec22db
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
6 src/Symfony/Component/Security/Acl/Dbal/AclProvider.php
@@ -263,7 +263,11 @@ protected function getAncestorLookupSql(array $batch)
for ($i = 0; $i < $count; $i++) {
if (!isset($types[$batch[$i]->getType()])) {
$types[$batch[$i]->getType()] = true;
- if ($count > 1) {

This is clearly wrong. The original intention here was a to carry out a small optimisation.
So, if we have two or more different entries in $types, then their is no need to do further search (it's sufficient for the if/else statement further below).

The code should be something like this:

if(sizeof($types) > 1) break;

Updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ // if there is more than one type we can safely break out of the
+ // loop, because it is the differentiator factor on whether to
+ // query for only one or more class types
+ if (count($types) > 1) {
break;
}
}
View
17 src/Symfony/Component/Security/Tests/Acl/Dbal/AclProviderTest.php
@@ -72,6 +72,23 @@ public function testFindAcls()
$this->assertTrue($oids[1]->equals($acl1->getObjectIdentity()));
}
+ public function testFindAclsWithDifferentTypes()
+ {
+ $oids = array();
+ $oids[] = new ObjectIdentity('123', 'Bundle\SomeVendor\MyBundle\Entity\SomeEntity');
+ $oids[] = new ObjectIdentity('123', 'Bundle\MyBundle\Entity\AnotherEntity');
+
+ $provider = $this->getProvider();
+
+ $acls = $provider->findAcls($oids);
+ $this->assertInstanceOf('SplObjectStorage', $acls);
+ $this->assertCount(2, $acls);
+ $this->assertInstanceOf('Symfony\Component\Security\Acl\Domain\Acl', $acl0 = $acls->offsetGet($oids[0]));
+ $this->assertInstanceOf('Symfony\Component\Security\Acl\Domain\Acl', $acl1 = $acls->offsetGet($oids[1]));
+ $this->assertTrue($oids[0]->equals($acl0->getObjectIdentity()));
+ $this->assertTrue($oids[1]->equals($acl1->getObjectIdentity()));
+ }
+
public function testFindAclCachesAclInMemory()
{
$oid = new ObjectIdentity('1', 'foo');
Something went wrong with that request. Please try again.