Skip to content

Conversation

peterrehm
Copy link
Contributor

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

Given the following role hierarchy configuration

security:
    role_hierarchy:
        ROLE_SUPER_ADMIN: [ROLE_ADMIN, ROLE_ALLOWED_TO_SWITCH]
        ROLE_ADMIN:       [ROLE_EMPLOYEE]
        ROLE_EMPLOYEE:    [ROLE_SALES]

If you were checking the user roles in the web profiler as an user with the assigned
role ROLE_ADMIN you saw only the following output.

bildschirmfoto 2014-12-08 um 12 31 25

This was kind of tricky since pages where you were checking is_granted('ROLE_EMPLOYEE')
granted access. Debugging was hard for newcomers to the project if they did not understand
the role hierarchy.

With this adjustment you will see the assigned roles as well as the inherited roles separately as
follows:

bildschirmfoto 2014-12-08 um 12 23 59


public function __construct(SecurityContextInterface $context = null)
public function __construct(SecurityContextInterface $context = null, RoleHierarchyInterface $roleHierarchy)
Copy link
Member

Choose a reason for hiding this comment

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

Should accept null to keep BC, in which case, you just don't get the new feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix right now. BC in case someone has defined his own Collector?

Copy link
Member

Choose a reason for hiding this comment

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

That's needed also for Silex-WebProfiler for instance.

@fabpot
Copy link
Member

fabpot commented Dec 11, 2014

Apparently, that breaks the tests.

@peterrehm
Copy link
Contributor Author

@fabpot Yes, I had one local commit missing - now it should be okay.

if (!in_array($role, $assignedRoles)) {
$inheritedRoles[] = $role;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

just use array_diff() instead of reimplementing in userland. This will be more readable, and the C implementation is likely to win over a userland implementation (even though I haven't benchmarked it and this is not critical here)

@peterrehm
Copy link
Contributor Author

@stof Thanks for the valuable feedback, has been updated.

@fabpot
Copy link
Member

fabpot commented Dec 12, 2014

@peterrehm Can you also have a look at the tests?

@peterrehm
Copy link
Contributor Author

@fabpot It looks like they broke due to the switch to array_diff as suggested by @stof.
I reverted those changes to the userland implementation.

It is all due to the diff with objects

$role1 = new \Symfony\Component\Security\Core\Role\Role('ROLE_USER');

$assignedRoles = array($role1);
$allRoles = array($role1);

var_dump(array_diff($assignedRoles, $allRoles));

Fails with the following error:

PHP Catchable fatal error:  Object of class Symfony\Component\Security\Core\Role\Role could not be converted to string in /www/symfony/play.php on line 10

@peterrehm
Copy link
Contributor Author

Now it should be okay. However there are many failing tests due to the depreciation messages.

@fabpot
Copy link
Member

fabpot commented Dec 12, 2014

Thank you @peterrehm.

@fabpot fabpot merged commit 31dc672 into symfony:2.7 Dec 12, 2014
fabpot added a commit that referenced this pull request Dec 12, 2014
…filer (peterrehm)

This PR was merged into the 2.7 branch.

Discussion
----------

[DX][Profiler] Show the inherited roles in the web profiler

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

Given the following role hierarchy configuration

````php
security:
    role_hierarchy:
        ROLE_SUPER_ADMIN: [ROLE_ADMIN, ROLE_ALLOWED_TO_SWITCH]
        ROLE_ADMIN:       [ROLE_EMPLOYEE]
        ROLE_EMPLOYEE:    [ROLE_SALES]
````

If you were checking the user roles in the web profiler as an user with the assigned
role `ROLE_ADMIN` you saw only the following output.

![bildschirmfoto 2014-12-08 um 12 31 25](https://cloud.githubusercontent.com/assets/2010989/5338601/26fd4c90-7ed6-11e4-961b-12103ddddf50.png)

This was kind of tricky since pages where you were checking `is_granted('ROLE_EMPLOYEE')`
granted access. Debugging was hard for newcomers to the project if they did not understand
the role hierarchy.

With this adjustment you will see the assigned roles as well as the inherited roles separately as
follows:

![bildschirmfoto 2014-12-08 um 12 23 59](https://cloud.githubusercontent.com/assets/2010989/5338622/5b0ffc58-7ed6-11e4-9863-27c9105897df.png)

Commits
-------

31dc672 Show the inherited roles in the web profiler
@peterrehm peterrehm deleted the profiler-roles branch January 11, 2015 10:48
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.

4 participants