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

[Security] deprecate the Role and SwitchUserRole classes #22048

Merged
merged 1 commit into from Feb 25, 2019

Conversation

@xabbuh
Copy link
Member

xabbuh commented Mar 17, 2017

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #20824
License MIT
Doc PR symfony/symfony-docs#11047

In #20801, we deprecated the RoleInterface. The next logical step would be to also deprecate the Role class. However, we currently have the SwitchUserRole class (a sub-class of Role) that acts as an indicator to check whether or not the authenticated user switched to another user.

This PR proposes an alternative solution to the usage of the special SwitchUserRole class by storing the original token inside the UsernamePasswordToken. This PR is not complete, but rather acts as a proof of concept of how we could get rid of the Role and the SwitchUserRole classes.

Please share your opinions whether you think this is a valid approach and I will be happy to finalise the PR.

@xabbuh xabbuh force-pushed the xabbuh:deprecate-role-class branch from 729a3aa to 71b26ae Mar 17, 2017

@xabbuh xabbuh force-pushed the xabbuh:deprecate-role-class branch 2 times, most recently from a8df9f2 to 1aec9b7 Mar 18, 2017

fabpot added a commit that referenced this pull request Mar 22, 2017

minor #22049 [Security] simplify the SwitchUserListenerTest (xabbuh)
This PR was merged into the 2.7 branch.

Discussion
----------

[Security] simplify the SwitchUserListenerTest

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

While working on #22048 I noticed that the `SwitchUserListenerTest` was more complicated than necessary by mocking a lot of stuff that didn't need to be mocked.

Commits
-------

923bbdb [Security] simplify the SwitchUserListenerTest

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Mar 24, 2017

@xabbuh xabbuh changed the base branch from master to 3.4 May 18, 2017

@xabbuh

This comment has been minimized.

Copy link
Member Author

xabbuh commented Jul 12, 2017

@symfony/deciders I would like to hear your opinion on this topic? Do you think it's worth to finish this approach? Should we take another road?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Aug 31, 2017

@fabpot @stof you might be the best to help here?

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Aug 31, 2017

IMO it's a good move. We still need to be able to get both impersonator/impersonated user from the token, I guess it's planed

@xabbuh

This comment has been minimized.

Copy link
Member Author

xabbuh commented Aug 31, 2017

IMO it's a good move. We still need to be able to get both impersonator/impersonated user from the token, I guess it's planed

@chalasr I guess the SwitchUserToken (or maybe better SwitchedUserToken) suggested by @HeahDude could be a good solution.

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Aug 31, 2017

Agreed, that would remove the need for isUserSwitched.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Sep 26, 2017

@xabbuh will you have tome to finish this PR for 3.4?

@xabbuh

This comment has been minimized.

Copy link
Member Author

xabbuh commented Sep 26, 2017

Yes, I am working on it.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Oct 8, 2017

should we move to 4.1?

@xabbuh

This comment has been minimized.

Copy link
Member Author

xabbuh commented Oct 8, 2017

I think so.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

@xabbuh xabbuh force-pushed the xabbuh:deprecate-role-class branch from 1aec9b7 to daf09cb Nov 15, 2017

@xabbuh xabbuh changed the base branch from 3.4 to master Nov 15, 2017

@xabbuh xabbuh force-pushed the xabbuh:deprecate-role-class branch from daf09cb to 7194244 Nov 15, 2017

@xabbuh xabbuh force-pushed the xabbuh:deprecate-role-class branch from 7194244 to 2054024 Nov 23, 2017

@xabbuh xabbuh force-pushed the xabbuh:deprecate-role-class branch 6 times, most recently from 9083257 to 5ead142 Feb 22, 2019

@xabbuh xabbuh force-pushed the xabbuh:deprecate-role-class branch 3 times, most recently from b871a65 to b2cfbb3 Feb 22, 2019

@xabbuh xabbuh force-pushed the xabbuh:deprecate-role-class branch from b2cfbb3 to d7aaa61 Feb 22, 2019

@xabbuh

This comment has been minimized.

Copy link
Member Author

xabbuh commented Feb 22, 2019

tests pass now

Status: Needs Review

@fabpot

fabpot approved these changes Feb 23, 2019

@javiereguiluz
Copy link
Member

javiereguiluz left a comment

Nice!

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 25, 2019

Thank you @xabbuh.

@fabpot fabpot merged commit d7aaa61 into symfony:master Feb 25, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 25, 2019

feature #22048 [Security] deprecate the Role and SwitchUserRole class…
…es (xabbuh)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Security] deprecate the Role and SwitchUserRole classes

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #20824
| License       | MIT
| Doc PR        | symfony/symfony-docs#11047

In #20801, we deprecated the `RoleInterface`. The next logical step would be to also deprecate the `Role` class. However, we currently have the `SwitchUserRole` class (a sub-class of `Role`) that acts as an indicator to check whether or not the authenticated user switched to another user.

This PR proposes an alternative solution to the usage of the special `SwitchUserRole` class by storing the original token inside the `UsernamePasswordToken`. This PR is not complete, but rather acts as a proof of concept of how we could get rid of the `Role` and the `SwitchUserRole` classes.

Please share your opinions whether you think this is a valid approach and I will be happy to finalise the PR.

Commits
-------

d7aaa61 deprecate the Role and SwitchUserRole classes

@xabbuh xabbuh deleted the xabbuh:deprecate-role-class branch Feb 25, 2019

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Feb 26, 2019

minor #11047 document the deprecation of the role classes (xabbuh)
This PR was merged into the master branch.

Discussion
----------

document the deprecation of the role classes

see symfony/symfony#22048

Commits
-------

0fbef77 document the deprecation of the role classes
@@ -15,6 +15,8 @@
* RoleHierarchyInterface is the interface for a role hierarchy.
*
* @author Fabien Potencier <fabien@symfony.com>
*
* @deprecated since Symfony 4.3, to be removed in 5.0.

This comment has been minimized.

@stof

stof Feb 26, 2019

Member

Why deprecating this interface instead of asking to implement the new method in it ? Removing the interface would remove a supported extension point.

This comment has been minimized.

@xabbuh

xabbuh Feb 26, 2019

Author Member

I don't remember why I did that initially. I guess it was due to an earlier implementation. #30388 will revert this part.

symfony-splitter pushed a commit to symfony/workflow that referenced this pull request Mar 23, 2019

feature #30388 [Security] undeprecate the RoleHierarchyInterface (xab…
…buh)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Security] undeprecate the RoleHierarchyInterface

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/symfony#22048 (comment)
| License       | MIT
| Doc PR        |

Instead of deprecating the interface it is sufficient to deprecate its
getReachableRoles() method and add a new getReachableRoleNames() method
in Symfony 5.

Commits
-------

2d3f2b7a74 undeprecate the RoleHierarchyInterface

fabpot added a commit that referenced this pull request Mar 23, 2019

feature #30388 [Security] undeprecate the RoleHierarchyInterface (xab…
…buh)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Security] undeprecate the RoleHierarchyInterface

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22048 (comment)
| License       | MIT
| Doc PR        |

Instead of deprecating the interface it is sufficient to deprecate its
getReachableRoles() method and add a new getReachableRoleNames() method
in Symfony 5.

Commits
-------

2d3f2b7 undeprecate the RoleHierarchyInterface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.