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 inheritance is broken in 1.2.X #905

Closed
alexzaporozhets opened this Issue Jul 24, 2013 · 34 comments

Comments

Projects
None yet
5 participants
@alexzaporozhets

alexzaporozhets commented Jul 24, 2013

After update from version 1.1 ACL inheritance stop to work properly.

@niden

This comment has been minimized.

Show comment
Hide comment
@niden

niden Jul 26, 2013

Member

@alexzaporozhets Could you please provide more information i.e. example of the code you are running or even better create a pull request with a failing test?

Thanks!

Member

niden commented Jul 26, 2013

@alexzaporozhets Could you please provide more information i.e. example of the code you are running or even better create a pull request with a failing test?

Thanks!

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Aug 5, 2013

Any news about the issue? We cannot update to 1.2 because of it.

alexzaporozhets commented Aug 5, 2013

Any news about the issue? We cannot update to 1.2 because of it.

@mikemirten

This comment has been minimized.

Show comment
Hide comment
@mikemirten

mikemirten Aug 8, 2013

Problem with the inheritance chain:

$acl = new AclEngine();

$acl->setDefaultAction(Acl::DENY);

$acl->addRole('user');
$acl->addRole('admin', 'user');
$acl->addRole('developer', 'admin');

$acl->addResource('tickets', ['list', 'open', 'close']);

$acl->allow('user', 'tickets', 'open');

var_dump($acl->isAllowed('user', 'tickets', 'open')); // returns 1
var_dump($acl->isAllowed('admin', 'tickets', 'open')); // returns 1
var_dump($acl->isAllowed('developer', 'tickets', 'open')); // returns 0 (!)

PHP 5.5.1
Phalcon 1.2.1

mikemirten commented Aug 8, 2013

Problem with the inheritance chain:

$acl = new AclEngine();

$acl->setDefaultAction(Acl::DENY);

$acl->addRole('user');
$acl->addRole('admin', 'user');
$acl->addRole('developer', 'admin');

$acl->addResource('tickets', ['list', 'open', 'close']);

$acl->allow('user', 'tickets', 'open');

var_dump($acl->isAllowed('user', 'tickets', 'open')); // returns 1
var_dump($acl->isAllowed('admin', 'tickets', 'open')); // returns 1
var_dump($acl->isAllowed('developer', 'tickets', 'open')); // returns 0 (!)

PHP 5.5.1
Phalcon 1.2.1

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Aug 12, 2013

@niden any news about this bug?

alexzaporozhets commented Aug 12, 2013

@niden any news about this bug?

@niden

This comment has been minimized.

Show comment
Hide comment
@niden

niden Aug 15, 2013

Member

Not yet buddy it is a high priority we need to check @alexzaporozhets

Stay tuned we will address this as soon as possible.

Member

niden commented Aug 15, 2013

Not yet buddy it is a high priority we need to check @alexzaporozhets

Stay tuned we will address this as soon as possible.

@abaranoff

This comment has been minimized.

Show comment
Hide comment
@abaranoff

abaranoff Aug 25, 2013

I have same issue with inheritance in 1.2.3

abaranoff commented Aug 25, 2013

I have same issue with inheritance in 1.2.3

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets commented Sep 18, 2013

Any news?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 18, 2013

@alexzaporozhets I am working on this but it may take some time as I am not familiar at with how ACL in Phalcon works.

ghost commented Sep 18, 2013

@alexzaporozhets I am working on this but it may take some time as I am not familiar at with how ACL in Phalcon works.

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 18, 2013

That is a good news, try to compare code with version 1.1

http://docs.phalconphp.com/en/latest/reference/acl.html

alexzaporozhets commented Sep 18, 2013

That is a good news, try to compare code with version 1.1

http://docs.phalconphp.com/en/latest/reference/acl.html

@ghost ghost referenced this issue Sep 18, 2013

Merged

Fix broken ACL inheritance #1251

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 18, 2013

Fix submitted, but I need more test cases (ideally from real world applications) — looks like we have no unit tests for ACL at all :-(

ghost commented Sep 18, 2013

Fix submitted, but I need more test cases (ideally from real world applications) — looks like we have no unit tests for ACL at all :-(

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 18, 2013

Hi, I can add a simple unit-test for ACL.
Should I fork Phalcon and send pull-request? What is procedure for contributing?

alexzaporozhets commented Sep 18, 2013

Hi, I can add a simple unit-test for ACL.
Should I fork Phalcon and send pull-request? What is procedure for contributing?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 18, 2013

Ideally yes, just make sure to submit the pull request against 1.3.0 branch.

Or just paste the code here and I will turn it into the test myself.

ghost commented Sep 18, 2013

Ideally yes, just make sure to submit the pull request against 1.3.0 branch.

Or just paste the code here and I will turn it into the test myself.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 19, 2013

Collaborator

This is fixed in the 1.2.4/1.3.0 branch

Collaborator

ghost commented Sep 19, 2013

This is fixed in the 1.2.4/1.3.0 branch

@phalcon phalcon closed this Sep 19, 2013

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 20, 2013

Hi, we tried to do update - the problem is remains

alexzaporozhets commented Sep 20, 2013

Hi, we tried to do update - the problem is remains

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 20, 2013

@alexzaporozhets There should be a file, ext/tests/issue-905.phpt. If you run

php issue-905.phpt

what do you see?

The expected result should look like this:

--TEST--
ACL inheritance is broken in 1.2.X - https://github.com/phalcon/cphalcon/issues/905
--SKIPIF--
--FILE--
int(1)
int(1)
int(1)
--EXPECT--
int(1)
int(1)
int(1)

ghost commented Sep 20, 2013

@alexzaporozhets There should be a file, ext/tests/issue-905.phpt. If you run

php issue-905.phpt

what do you see?

The expected result should look like this:

--TEST--
ACL inheritance is broken in 1.2.X - https://github.com/phalcon/cphalcon/issues/905
--SKIPIF--
--FILE--
int(1)
int(1)
int(1)
--EXPECT--
int(1)
int(1)
int(1)
@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 20, 2013

I cannot build 1.3.0:
/usr/include/php5/ext/pcre/php_pcre.h:29:18: fatal error: pcre.h: No such file or directory
compilation terminated.
make: *** [phalcon.lo] Error 1

1.2.4:

vagrant@timedoctor:~/cphalcon/build$ php aclTest.php
--TEST--
ACL inheritance is broken in 1.2.X - #905
--SKIPIF--
--FILE--
Phalon\Version 1.2.4 RC 1
int(1)
int(1)
Segmentation fault

alexzaporozhets commented Sep 20, 2013

I cannot build 1.3.0:
/usr/include/php5/ext/pcre/php_pcre.h:29:18: fatal error: pcre.h: No such file or directory
compilation terminated.
make: *** [phalcon.lo] Error 1

1.2.4:

vagrant@timedoctor:~/cphalcon/build$ php aclTest.php
--TEST--
ACL inheritance is broken in 1.2.X - #905
--SKIPIF--
--FILE--
Phalon\Version 1.2.4 RC 1
int(1)
int(1)
Segmentation fault

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 20, 2013

For 1.3.0, could you please run

apt-get build-dep php5-dev

and try to recompile again?

1.2.4 — looking

ghost commented Sep 20, 2013

For 1.3.0, could you please run

apt-get build-dep php5-dev

and try to recompile again?

1.2.4 — looking

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 20, 2013

This line produces segmentation fault:
var_dump((int)$acl->isAllowed('developer', 'tickets', 'open'));

alexzaporozhets commented Sep 20, 2013

This line produces segmentation fault:
var_dump((int)$acl->isAllowed('developer', 'tickets', 'open'));

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 20, 2013

vagrant@timedoctor:~/cphalcon$ php /home/vagrant/cphalcon/build/aclTest.php
--TEST--
ACL inheritance is broken in 1.2.X - #905
--SKIPIF--
--FILE--
Phalon\Version 1.3.0 BETA 1
int(1)
int(1)
int(0)
--EXPECT--
int(1)
int(1)
int(1)

alexzaporozhets commented Sep 20, 2013

vagrant@timedoctor:~/cphalcon$ php /home/vagrant/cphalcon/build/aclTest.php
--TEST--
ACL inheritance is broken in 1.2.X - #905
--SKIPIF--
--FILE--
Phalon\Version 1.3.0 BETA 1
int(1)
int(1)
int(0)
--EXPECT--
int(1)
int(1)
int(1)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 20, 2013

1.3.0 - did you build it from ext/ or from build/?

ghost commented Sep 20, 2013

1.3.0 - did you build it from ext/ or from build/?

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets commented Sep 20, 2013

from build

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 20, 2013

At first I executed: apt-get build-dep php5-dev and after that build was successful

alexzaporozhets commented Sep 20, 2013

At first I executed: apt-get build-dep php5-dev and after that build was successful

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 20, 2013

Please build it from ext/

build/ was not updated.

ghost commented Sep 20, 2013

Please build it from ext/

build/ was not updated.

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 20, 2013

Build 1.2.4? or 1.3.0?

alexzaporozhets commented Sep 20, 2013

Build 1.2.4? or 1.3.0?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 20, 2013

1.3.0.

1.3.0 build from ext/ should pass all tests.

ghost commented Sep 20, 2013

1.3.0.

1.3.0 build from ext/ should pass all tests.

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 20, 2013

Ok, I will do it. Is it possible to fix 1.2.4?

alexzaporozhets commented Sep 20, 2013

Ok, I will do it. Is it possible to fix 1.2.4?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost commented Sep 20, 2013

Sure

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 20, 2013

Can you give a time estimate for fix? We are waiting to do the update.

alexzaporozhets commented Sep 20, 2013

Can you give a time estimate for fix? We are waiting to do the update.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 20, 2013

For 1.2.4:

diff --git a/ext/acl/adapter/memory.c b/ext/acl/adapter/memory.c
index aa41c91..01be425 100644
--- a/ext/acl/adapter/memory.c
+++ b/ext/acl/adapter/memory.c
@@ -725,14 +725,13 @@ static int phalcon_role_adapter_memory_check_inheritance(zval *role, zval *resou
                if (phalcon_array_isset(access_list, access_key)) {
                        phalcon_array_fetch(&have_access, access_list, access_key, PH_NOISY);
                        found = Z_TYPE_P(have_access) != IS_NULL;
+                       zval_ptr_dtor(&have_access);
+                       have_access = NULL;
                }
                else {
                        found = 0;
                }

-               zval_ptr_dtor(&have_access);
-               have_access = NULL;
-
                zval_dtor(access_key);
                ZVAL_NULL(access_key);

I will submit the pull request later, as I need to fix several other bugs.

ghost commented Sep 20, 2013

For 1.2.4:

diff --git a/ext/acl/adapter/memory.c b/ext/acl/adapter/memory.c
index aa41c91..01be425 100644
--- a/ext/acl/adapter/memory.c
+++ b/ext/acl/adapter/memory.c
@@ -725,14 +725,13 @@ static int phalcon_role_adapter_memory_check_inheritance(zval *role, zval *resou
                if (phalcon_array_isset(access_list, access_key)) {
                        phalcon_array_fetch(&have_access, access_list, access_key, PH_NOISY);
                        found = Z_TYPE_P(have_access) != IS_NULL;
+                       zval_ptr_dtor(&have_access);
+                       have_access = NULL;
                }
                else {
                        found = 0;
                }

-               zval_ptr_dtor(&have_access);
-               have_access = NULL;
-
                zval_dtor(access_key);
                ZVAL_NULL(access_key);

I will submit the pull request later, as I need to fix several other bugs.

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 26, 2013

Hi, did you submit a pull request to 1.2.4 branch?

alexzaporozhets commented Sep 26, 2013

Hi, did you submit a pull request to 1.2.4 branch?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost commented Sep 26, 2013

Yes

@alexzaporozhets

This comment has been minimized.

Show comment
Hide comment
@alexzaporozhets

alexzaporozhets Sep 28, 2013

We did a test update yesterday (1.2.4) the problem remains.

alexzaporozhets commented Sep 28, 2013

We did a test update yesterday (1.2.4) the problem remains.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 28, 2013

Same — what does

php issue-905.phpt

display?

ghost commented Sep 28, 2013

Same — what does

php issue-905.phpt

display?

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