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] Dispatch an event when "logout user on change" steps in #31138

Conversation

@Simperfit
Copy link
Contributor

commented Apr 17, 2019

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

This adds a new event when the user has been changed and has been log out from the apps, it allow someone to register to this event and do something with either to token or the refreshedUser.

@Simperfit Simperfit force-pushed the Simperfit:feature/add-a-new-event-when-the-user-has-been-logout-on-change branch 2 times, most recently from 1396daf to 452978a Apr 17, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 17, 2019

@Simperfit Simperfit force-pushed the Simperfit:feature/add-a-new-event-when-the-user-has-been-logout-on-change branch 2 times, most recently from 1129413 to 8f767ee Apr 18, 2019

@Simperfit Simperfit marked this pull request as ready for review Apr 18, 2019

@linaori
Copy link
Contributor

left a comment

Great idea!

@Simperfit Simperfit force-pushed the Simperfit:feature/add-a-new-event-when-the-user-has-been-logout-on-change branch from 8f767ee to 36bb3b2 Apr 18, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Status: Needs Review

@Simperfit Simperfit force-pushed the Simperfit:feature/add-a-new-event-when-the-user-has-been-logout-on-change branch from 36bb3b2 to b6ad28a Apr 18, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Status: Needs Review

@Simperfit Simperfit force-pushed the Simperfit:feature/add-a-new-event-when-the-user-has-been-logout-on-change branch from b6ad28a to 5765f69 Apr 18, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Thanks @linaori and @noniagriconomie for the review !

@noniagriconomie

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@Simperfit do not forget to update the doc accordingly when PR is finished :)
https://github.com/symfony/symfony-docs/pull/11450/files#diff-2f19b023cc3b9935394ee5a983d40cdcR286

@chalasr chalasr force-pushed the Simperfit:feature/add-a-new-event-when-the-user-has-been-logout-on-change branch 2 times, most recently from 95f1071 to ecdeab8 Apr 26, 2019

@chalasr

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Reverted my suggestion about allowing to reauthenticate the token from a listener, bad idea. Also renamed to DeauthenticatedEvent to save us from renaming the event in 5.0 as we want to get rid of the User concept (#30914).
Ready to go 👍

@xabbuh

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

If we also do not want to rely on the user concept, would it make sense to pass the old and the refreshed token instead of the user?

@chalasr chalasr force-pushed the Simperfit:feature/add-a-new-event-when-the-user-has-been-logout-on-change branch 2 times, most recently from b901b0b to abec9c1 Apr 27, 2019

@chalasr

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

@xabbuh 👍 Changed to getOriginalToken() and getDeauthenticatedToken().
Wording suggestions welcome (getRefreshedToken() vs getDeauthenticatedToken()?)

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

I like getRefreshedToken() better :p

@chalasr chalasr force-pushed the Simperfit:feature/add-a-new-event-when-the-user-has-been-logout-on-change branch from abec9c1 to 40e4218 Apr 27, 2019

@chalasr

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

Renamed

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Thanks for helping me finishing this @chalasr ;).

@xabbuh

xabbuh approved these changes Apr 28, 2019

@chalasr

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

Thank you @Simperfit.

@chalasr chalasr merged commit 40e4218 into symfony:master Apr 28, 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

chalasr added a commit that referenced this pull request Apr 28, 2019

feature #31138 [Security] Dispatch an event when "logout user on chan…
…ge" steps in (Simperfit)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Security] Dispatch an event when "logout user on change" steps in

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #26902   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#11450 <!-- required for new features -->

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

This adds a new event when the user has been changed and has been log out from the apps, it allow someone to register to this event and do something with either to token or the refreshedUser.

Commits
-------

40e4218 [Security] Dispatch an event when "logout user on change" steps in

@Simperfit Simperfit deleted the Simperfit:feature/add-a-new-event-when-the-user-has-been-logout-on-change branch Apr 28, 2019

@@ -23,6 +23,7 @@ CHANGELOG
* Dispatch `SwitchUserEvent` on `security.switch_user`
* Deprecated `Argon2iPasswordEncoder`, use `SodiumPasswordEncoder` instead
* Deprecated `BCryptPasswordEncoder`, use `NativePasswordEncoder` instead
* Added `DeauthenticatedEvent` dispatched in case the user has changed when trying to refresh it

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 29, 2019

Member

What is "it" here? The user?

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 29, 2019

Member

I think "it" refers to the Token here. What about "Added DeauthenticatedEvent dispatched in case the user has changed when trying to refresh the token"?

use Symfony\Contracts\EventDispatcher\Event;
/**
* Deauthentication happens in case the user has changed when trying to refresh it.

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 29, 2019

Member

Sentence should be changed as well

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

OskarStark added a commit to symfony/symfony-docs that referenced this pull request May 7, 2019

minor #11450 [Security] Dispatch an event when "logout user on change…
…" steps in (Simperfit)

This PR was merged into the master branch.

Discussion
----------

[Security] Dispatch an event when "logout user on change" steps in

<!--

If your pull request fixes a BUG, use the oldest maintained branch that contains
the bug (see https://symfony.com/roadmap for the list of maintained branches).

If your pull request documents a NEW FEATURE, use the same Symfony branch where
the feature was introduced (and `master` for features of unreleased versions).

-->

This documents the new event implemented in symfony/symfony#31138

Commits
-------

b5e6038 [Security] Dispatch an event when "logout user on change" steps in

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

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.