Skip to content

[Security] Allow configuring a redirect url via route name when switching user #47343

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] Allow configuring a redirect url via route name when switching user #47343

wants to merge 1 commit into from

Conversation

94noni
Copy link
Contributor

@94noni 94noni commented Aug 19, 2022 •
edited
Loading

Q A
Branch? 6.2
Bug fix? no
New feature? yesish (first version is not released yet thus no bc)
Deprecations? no
Tickets Fix symfony/symfony-docs#17024 (comment) and discuss
License MIT

Change the switch user redirect "target url" to a "target route", cf ticket
First version can be found here #46338

Friendly ping @xabbuh from the ticket and @chalasr @fabpot as original reviewers

Before fixing tests, is adding such url generator arg in such position ok? the class is marked final

# config/packages/security.yaml
security:
    firewalls:
        main:
            switch_user: { role: CAN_SWITCH_USER , target_route: my_app_route }

@94noni 94noni requested review from wouterj and chalasr as code owners August 19, 2022 19:55
@carsonbot carsonbot added this to the 6.2 milestone Aug 19, 2022
@carsonbot
Copy link

Hey!

I think @johnkrovitch has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@94noni
Copy link
Contributor Author

94noni commented Aug 29, 2022

Status: needs work

@94noni 94noni force-pushed the ft-switch-redirect-route-name branch 2 times, most recently from 31f983e to 3e9d77a Compare August 29, 2022 20:31
@94noni
Copy link
Contributor Author

94noni commented Aug 30, 2022

@xabbuh thx for approving
Regarding my comment on tests and arg position, do you have a preference for ordering? Or should I just fix related tests

xabbuh
xabbuh requested changes Sep 2, 2022
View reviewed changes
@94noni 94noni force-pushed the ft-switch-redirect-route-name branch 4 times, most recently from 56cdc2c to fcb1f62 Compare September 12, 2022 05:21
@94noni
Copy link
Contributor Author

94noni commented Sep 12, 2022 •
edited
Loading

@xabbuh friendly ping, requested change regarding $urlGenerator is done, please tell me if it's ok

chalasr
chalasr approved these changes Sep 12, 2022
View reviewed changes
@94noni 94noni force-pushed the ft-switch-redirect-route-name branch from fcb1f62 to ccabea1 Compare September 12, 2022 13:36
@94noni
Copy link
Contributor Author

94noni commented Sep 12, 2022

when merged I will update the already created document PR
so feel free to close the autocreated PR or link it to symfony/symfony-docs#17024
thx !

@chalasr
Copy link
Member

chalasr commented Sep 12, 2022

Can you please add an entry in SecurityBundle/CHANGELOG.md? We forgot to do it in #46338

@94noni 94noni force-pushed the ft-switch-redirect-route-name branch from ccabea1 to 016a24a Compare September 12, 2022 15:19
@94noni
Copy link
Contributor Author

94noni commented Sep 12, 2022

Can you please add an entry in SecurityBundle/CHANGELOG.md? We forgot to do it in #46338

I'added one but please review it as the English wording may not be right/fluent perhaps :s

@chalasr
Copy link
Member

chalasr commented Sep 12, 2022

Looks as good as if it's been wrote by a native speaker 🤷‍♂️

@chalasr
Copy link
Member

chalasr commented Sep 12, 2022

Thank you @94noni.

@chalasr
Copy link
Member

chalasr commented Sep 12, 2022

Something went wrong while merging so I will close here, but your commit is in ce1f115.

@chalasr chalasr closed this Sep 12, 2022
@94noni 94noni deleted the ft-switch-redirect-route-name branch September 12, 2022 19:14
This was referenced Sep 13, 2022
Merged
Closed
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Sep 20, 2022
@javiereguiluz
minor #17024 [Security] Allow configuring a target url when switching…
4f5b7f3 
… user (94noni)

This PR was squashed before being merged into the 6.2 branch.

Discussion
----------

[Security] Allow configuring a target url when switching user

Documents symfony/symfony#47343
Supersed symfony/symfony#46338 (feature changed)
Close #17021

Commits
-------

90dc3c8 [Security] Allow configuring a target url when switching user
@94noni 94noni mentioned this pull request Jan 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh xabbuh approved these changes

@chalasr chalasr chalasr approved these changes

@wouterj wouterj Awaiting requested review from wouterj wouterj is a code owner

Assignees
No one assigned
Labels
Feature Security Status: Reviewed
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.
4 participants
@94noni @carsonbot @chalasr @xabbuh