Skip to content

[Security] reusing AuthorizationChecker in AccessListener #27121

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Security] reusing AuthorizationChecker in AccessListener #27121

wants to merge 1 commit into from

Conversation

oleg-andreyev
Copy link
Contributor

@oleg-andreyev oleg-andreyev commented May 2, 2018 •
edited
Loading

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT
Fixed tickets #21571

While investigating why AuthenticationEvents::AUTHENTICATION_SUCCESS is not dispatched, noticed similarity between AccessListener and AuthorizationChecker and the only real difference is alwaysAuthenticate flag.

Case 1:
When UsernamePasswordToken is serialized it will be serialized with state of authenticated,
so when application will load token from session AccessListener won't call authenticate because authenticated=true, even if alwaysAuthenticate=true

Case 2:
User A has roles foo, bar and admin, User A is signed-in into application and token is persisted, later another User B with role admin, decided to restrict role admin for User A, so User A won't lose it's privileges until session is expired or logout, because token is persisted with roles and authenticated=true and AccessListener does not call authenticate even if alwaysAuthenticate=true

TODO

  • - reuse AuthorizationChecker in AccessListener
  • - adjust unit tests
  • - add deprecation notices
  • - update CHANGELOG and UPGRADE*

@oleg-andreyev oleg-andreyev changed the title reusing AuthorizationChecker in AccessListener [Security] reusing AuthorizationChecker in AccessListener May 2, 2018
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 2, 2018
@oleg-andreyev
symfony#27121 added BC to AccessListener::__construct
da085e7
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 2, 2018
@oleg-andreyev
symfony#27121 fixed code style issues
0d18b4b
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 2, 2018
@oleg-andreyev
symfony#27121 updated deprecation message
472167a
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 2, 2018
@oleg-andreyev
symfony#27121 reverted docblock for properties
10cc2ff
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 2, 2018
@oleg-andreyev
symfony#27121 WIP returned property order as original
a2132fd
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 2, 2018
@oleg-andreyev
symfony#27121 WIP returned use order as original
347b5e1
@oleg-andreyev
Copy link
Contributor Author

@nicolas-grekas @iltar @stof can you give an advice against what branch this changes should go?
So 2.7 version entered security fixes phase 2 days ago.

oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 2, 2018
@oleg-andreyev
symfony#27121 WIP split legacy test
635597e
@oleg-andreyev oleg-andreyev force-pushed the reuse-auth-checker-in-access-listener branch from 635597e to 5773f71 Compare May 2, 2018 20:26
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 2, 2018
@oleg-andreyev
symfony#27121 WIP split legacy test
5773f71
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 2, 2018
@oleg-andreyev
symfony#27121 WIP added service definition for security.access_listener
c8cb7c2
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 2, 2018
@oleg-andreyev
symfony#27121 WIP imported app/config.yml from FirewallEntryPoint/con…
6c00d2e 
…fig.yml
@nicolas-grekas nicolas-grekas added this to the 2.7 milestone May 2, 2018
@nicolas-grekas
Copy link
Member

@oleg-andreyev fortunately, 2.7 is still maintained until the end of May, so we still have a few weeks.

@nicolas-grekas
Copy link
Member

But actually, new deprecations are never introduced in bugfix-only releases, so as is, this should go on master (for 4.2).
If you think this is a bugfix, then the patch should be updated to make it really a bugfix, ie. no new public/protected methods, and no new deprecations.

@linaori
Copy link
Contributor

linaori commented May 3, 2018

My suggestion would be to fix this in 2.7 if it's a bug (which it does sound like), and then a feature for 4.2 to fix the constructor, making it an extra optional argument until the change of signature.

@oleg-andreyev
Copy link
Contributor Author

Ok, I'll prepare my PR for 2.7 as bugfix, and then will prepare PR with deprecation for master

@oleg-andreyev oleg-andreyev force-pushed the reuse-auth-checker-in-access-listener branch from 6c00d2e to dbdba7e Compare May 3, 2018 19:33
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 3, 2018
@oleg-andreyev
symfony#27121 WIP moved mocks into setUp
307b49c
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 3, 2018
@oleg-andreyev
symfony#27121 WIP injected AuthorizationChecker into AccessListener
c29f098
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 3, 2018
@oleg-andreyev
symfony#27121 update access_listener service definition
dbdba7e
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 3, 2018
@oleg-andreyev
symfony#27121 WIP phpcs fixes
85bf6d3
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request May 3, 2018
@oleg-andreyev
symfony#27121 WIP removed redundant unit
2250e6b
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Jul 30, 2018
@oleg-andreyev
symfony#27121 updated CHANGELOG.md and UPGRADE-4.2.md
543ea80
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Jul 30, 2018
@oleg-andreyev
symfony#27121 reverted changes to security_listeners.xml
fba0977
@oleg-andreyev oleg-andreyev force-pushed the reuse-auth-checker-in-access-listener branch from fba0977 to 834785a Compare July 31, 2018 21:40
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Jul 31, 2018
@oleg-andreyev
symfony#27121 reusing AuthorizationChecker into AccessListener
0118f5e 
- added deprecation notice about old-signature
- added deprecation annotation to properties
- added legacy group to old unit tests
- added new unit tests
- adjusted security.access_listener
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Jul 31, 2018
@oleg-andreyev
symfony#27121 updated CHANGELOG.md and UPGRADE-4.2.md
8dd08af
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Jul 31, 2018
@oleg-andreyev
symfony#27121 handle deprecation correctly
834785a
@oleg-andreyev oleg-andreyev force-pushed the reuse-auth-checker-in-access-listener branch from 834785a to d0edc06 Compare July 31, 2018 21:51
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Jul 31, 2018
@oleg-andreyev
symfony#27121 reusing AuthorizationChecker into AccessListener
d0edc06 
- added deprecation notice about old-signature
- added deprecation annotation to properties
- added legacy group to old unit tests
- added new unit tests
- adjusted security.access_listener
- updated CHANGELOG.md and UPGRADE-4.2.md
@oleg-andreyev oleg-andreyev force-pushed the reuse-auth-checker-in-access-listener branch from d0edc06 to 23b354e Compare March 2, 2019 20:53
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Mar 2, 2019
@oleg-andreyev
symfony#27121 reusing AuthorizationChecker into AccessListener
4511365 
- added deprecation notice about old-signature
- added deprecation annotation to properties
- added legacy group to old unit tests
- added new unit tests
- adjusted security.access_listener
- updated CHANGELOG.md and UPGRADE-4.2.md
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Mar 2, 2019
@oleg-andreyev
symfony#27121 updated UPGRADE-4.3.md
23b354e
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Mar 2, 2019
@oleg-andreyev
symfony#27121 update docblock and cs fixes
f9b5c89
@oleg-andreyev oleg-andreyev force-pushed the reuse-auth-checker-in-access-listener branch from f9b5c89 to 5e7ea57 Compare March 2, 2019 21:09
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Mar 2, 2019
@oleg-andreyev
symfony#27121 reusing AuthorizationChecker into AccessListener
cd5c282 
- added deprecation notice about old-signature
- added deprecation annotation to properties
- added legacy group to old unit tests
- added new unit tests
- adjusted security.access_listener
- updated CHANGELOG.md and UPGRADE-4.2.md
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Mar 2, 2019
@oleg-andreyev
symfony#27121 update docblock and cs fixes
5e7ea57
@oleg-andreyev oleg-andreyev force-pushed the reuse-auth-checker-in-access-listener branch from 5e7ea57 to 8215d34 Compare March 2, 2019 21:13
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Mar 2, 2019
@oleg-andreyev
symfony#27121 reusing AuthorizationChecker into AccessListener
8215d34 
- added deprecation notice about old-signature
- added deprecation annotation to properties
- added legacy group to old unit tests
- added new unit tests
- adjusted security.access_listener
@oleg-andreyev oleg-andreyev force-pushed the reuse-auth-checker-in-access-listener branch from 8215d34 to 3af6406 Compare March 2, 2019 21:16
oleg-andreyev added a commit to oleg-andreyev/symfony that referenced this pull request Mar 2, 2019
@oleg-andreyev
symfony#27121 reusing AuthorizationChecker into AccessListener
fcd8ff7 
- added deprecation notice about old-signature
- added deprecation annotation to properties
- added legacy group to old unit tests
- added new unit tests
- adjusted security.access_listener
@oleg-andreyev oleg-andreyev changed the base branch from master to 3.4 March 2, 2019 21:18
@oleg-andreyev oleg-andreyev changed the base branch from 3.4 to master March 2, 2019 21:20
@oleg-andreyev
symfony#27121 reusing AuthorizationChecker into AccessListener
a62740f 
- added deprecation notice about old-signature
- added deprecation annotation to properties
- added legacy group to old unit tests
- added new unit tests
- adjusted security.access_listener
@oleg-andreyev oleg-andreyev force-pushed the reuse-auth-checker-in-access-listener branch from fcd8ff7 to a62740f Compare March 2, 2019 21:21
@xabbuh
Copy link
Member

xabbuh commented Mar 8, 2019

If some of the proposed changes are a bugfix, we need to split this PR: Move everything that is not about the bugfix to a new PR targetting master and then rebase the changes for the bugfix onto the 3.4 branch.

@oleg-andreyev
Copy link
Contributor Author

This PR got messy, I'm closing it and will provide a cleaner version.

@oleg-andreyev oleg-andreyev mentioned this pull request Apr 18, 2019
Merged
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
fabpot added a commit that referenced this pull request Sep 6, 2019
@fabpot
feature #31177 #21571 Comparing roles to detected that users has chan…
8e50eb9 
…ged (oleg-andreyev)

This PR was merged into the 4.4 branch.

Discussion
----------

#21571 Comparing roles to detected that users has changed

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT
| Fixed tickets | #21571 (comment)
| Docs | symfony/symfony-docs#11457

**Case 1:**
User A has roles `foo, bar and admin`, User A is signed-in into application and token is persisted, later another User B with role `admin`, decided to restrict role `admin` for User A, so User A won't lose it's privileges until session is expired or logout, because token is persisted with `roles` and `authenticated=true` and roles are not compared.

Ref. to the previous attempt: #27121

Commits
-------

4f4c30d - updated AbstractToken to compare Roles - Updated isEqualTo method to match roles as default User implements EquatableInterface - added test case - bumped symfony/security-core to 4.4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas requested changes

@stof stof stof left review comments

@linaori linaori linaori requested changes

@dunglas dunglas dunglas approved these changes

@wouterj wouterj wouterj approved these changes

@lyrixx lyrixx Awaiting requested review from lyrixx

Assignees
No one assigned
Labels
Feature Security Status: Needs Work
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
10 participants
@oleg-andreyev @nicolas-grekas @linaori @fabpot @chalasr @wouterj @xabbuh @dunglas @stof @carsonbot