Skip to content

[Security] Add impersonation support for stateless authentication #24260

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

Merged
merged 1 commit into from
Sep 30, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Sep 19, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets https://github.com/lafourchette/SwitchUserStatelessBundle/issues/10#issuecomment-330434589
License MIT
Doc PR n/a

The switch_user listener triggers a redirection in case of success and thus does not play well with stateless authentication which is common nowadays (as opposed to other listeners like the exception one).
This adds a new stateless option to the switch_user listener, if set to true then no redirection is triggered during user switching.

This will avoid the need for lafourchette/SwitchUserStatelessBundle which just duplicated the symfony SwitchUserListener (with config factory) at a given state to avoid the 2 LOC which are causing the redirection.
The bundle is not actively maintained and the listener it provides is out of date due to the missing upstream additions and bug fixes (see https://github.com/lafourchette/SwitchUserStatelessBundle/issues/10).

@stof
Copy link
Member

stof commented Sep 20, 2017

should we actually have this extra configuration ? couldn't we just make the switch_user listener stateless when the firewall itself is stateless ?

@chalasr
Copy link
Member Author

chalasr commented Sep 20, 2017

@stof ideally yes, like the exceptionlistener. But disabling the redirection for stateless firewalls now would be a BC Break right?

@nicolas-grekas
Copy link
Member

disabling the redirection for stateless firewalls now would be a BC Break right?

should we plan any deprecation here? Like when the option doesn't match the statelessyness of the firewall, then plan to deprecate the option in 4.1 and make it automatic meanwhile?

@chalasr
Copy link
Member Author

chalasr commented Sep 28, 2017

Deprecation added

@chalasr chalasr removed the request for review from dunglas September 28, 2017 14:36
@chalasr chalasr force-pushed the switch_user_stateless branch from 32fc7bb to 0f57086 Compare September 28, 2017 15:44
@chalasr chalasr force-pushed the switch_user_stateless branch from 0f57086 to e7a5803 Compare September 30, 2017 11:13
@chalasr
Copy link
Member Author

chalasr commented Sep 30, 2017

Rebased. ping @symfony/deciders

@fabpot
Copy link
Member

fabpot commented Sep 30, 2017

Thank you @chalasr.

@fabpot fabpot merged commit e7a5803 into symfony:3.4 Sep 30, 2017
fabpot added a commit that referenced this pull request Sep 30, 2017
…hentication (chalasr)

This PR was merged into the 3.4 branch.

Discussion
----------

[Security] Add impersonation support for stateless authentication

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | https://github.com/lafourchette/SwitchUserStatelessBundle/issues/10#issuecomment-330434589
| License       | MIT
| Doc PR        | n/a

The `switch_user` listener triggers a redirection in case of success and thus does not play well with stateless authentication which is common nowadays (as opposed to other listeners like the [exception one](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php#L187..#L189)).
This adds a new `stateless` option to the `switch_user` listener, if set to true then no redirection is triggered during user switching.

This will avoid the need for [lafourchette/SwitchUserStatelessBundle](https://github.com/lafourchette/SwitchUserStatelessBundle) which just duplicated the symfony SwitchUserListener (with config factory) at a given state to avoid the 2 LOC which are causing the redirection.
The bundle is not actively maintained and the listener it provides is out of date due to the missing upstream additions and bug fixes (see https://github.com/lafourchette/SwitchUserStatelessBundle/issues/10).

Commits
-------

e7a5803 [Security] Add user impersonation support for stateless authentication
@chalasr chalasr deleted the switch_user_stateless branch September 30, 2017 16:42
fabpot added a commit that referenced this pull request Oct 5, 2017
…halasr)

This PR was merged into the 3.4 branch.

Discussion
----------

[Security] Look at headers for switch_user username

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24260
| License       | MIT
| Doc PR        | n/a

Allowing `switch_user.parameter` config node to be a header name.
It's supported by SwitchUserStatelessBundle and I think it makes sense.
Forgotten in #24260 so targets 3.4 but not a blocker.

Commits
-------

3c80195 [Security] Look at headers for switch user username parameter
This was referenced Oct 18, 2017
DougHayward added a commit to DougHayward/LexikJWTAuthenticationBundle that referenced this pull request Feb 23, 2018
Change to the documentation as [lafourchette/SwitchUserStatelessBundle](https://github.com/lafourchette/SwitchUserStatelessBundle) no longer appears to be maintained and is no longer necessary since this pull request symfony/symfony#24260
chalasr added a commit to lexik/LexikJWTAuthenticationBundle that referenced this pull request Mar 4, 2018
…eDoug)

This PR was merged into the 2.x-dev branch.

Discussion
----------

Update index.md - Remove reference to lafourchette.

Change to the documentation as [lafourchette/SwitchUserStatelessBundle](https://github.com/lafourchette/SwitchUserStatelessBundle) no longer appears to be maintained and is no longer necessary since this pull request symfony/symfony#24260

Commits
-------

beb93f9 Update index.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants