Skip to content

[Routing][SecurityBundle] Add LogoutRouteLoader #50946

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
Merged
merged 1 commit into from
Oct 15, 2023
Merged

[Routing][SecurityBundle] Add LogoutRouteLoader #50946

merged 1 commit into from
Oct 15, 2023

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Jul 11, 2023 •
edited
Loading

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #50920
License MIT
Doc PR symfony/symfony-docs#19000

#50920 is about avoiding for users to create logout routes. Given we don’t want to allow bundles registering routes, I added a LogoutRouteLoader service bearing the routing.route_loader tag to be imported by the user. Such import could be added to the SecurityBundle recipe:

# config/routes/security.yaml
logout:
    resource: security.route_loader.logout
    type: service

To invalidate routes when logout paths change, I stored them in a parameter so that the ContainerParametersResourceChecker can check the collection. Not sure if it’s okay or if a better way exists.

@carsonbot carsonbot added this to the 6.4 milestone Jul 11, 2023
@carsonbot carsonbot changed the title [SecurityBundle][Routing] Add LogoutRouteLoader [Routing][SecurityBundle] Add LogoutRouteLoader Jul 11, 2023
wouterj
wouterj reviewed Aug 10, 2023
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very cool to me. Thanks for creating this PR :)

I feel like the ContainerParameterResource is probably the only way to do this indeed. Maybe someone else has a better suggestion?

@MatTheCat MatTheCat force-pushed the ticket_50920 branch 3 times, most recently from e79f533 to 10097af Compare August 11, 2023 14:26
@MatTheCat
Copy link
Contributor Author

Gave it a shot. Still open to suggestions!

@Spomky Spomky mentioned this pull request Aug 20, 2023
Closed
chalasr
chalasr requested changes Sep 17, 2023
View reviewed changes
This was referenced Oct 6, 2023
Merged
Merged
stof
stof requested changes Oct 6, 2023
View reviewed changes
@wouterj wouterj added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 6, 2023
MatTheCat added a commit to MatTheCat/symfony that referenced this pull request Oct 7, 2023
@MatTheCat
Address symfony#50946 (review)
4513e2e
MatTheCat added a commit to MatTheCat/symfony that referenced this pull request Oct 7, 2023
@MatTheCat
Address symfony#50946 (review)
17d1964
MatTheCat added a commit to MatTheCat/symfony that referenced this pull request Oct 7, 2023
@MatTheCat
Address symfony#50946 (review)
ccac0b7
maxhelias
maxhelias reviewed Oct 11, 2023
View reviewed changes
@wouterj
Copy link
Member

wouterj commented Oct 15, 2023

I like the nice little DX improvement here! Thanks for working on this Mathieu! Let's get this in 6.4/7.0

@wouterj wouterj merged commit ef1197d into symfony:6.4 Oct 15, 2023
@MatTheCat MatTheCat deleted the ticket_50920 branch October 15, 2023 12:41
@alamirault alamirault mentioned this pull request Oct 15, 2023
Merged
fabpot added a commit that referenced this pull request Oct 16, 2023
@fabpot
minor #52068 [SecurityBundle] Fix LogoutRouteLoader PHPDoc (alamirault)
eadd41a 
This PR was merged into the 6.4 branch.

Discussion
----------

[SecurityBundle] Fix LogoutRouteLoader PHPDoc

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

Fix little typo in phpdoc, introduced in #50946

cc `@MatTheCat` for review :)

Commits
-------

dbbc5bf [SecurityBundle] Fix LogoutRouteLoader phpdoc
@maelanleborgne maelanleborgne mentioned this pull request Oct 17, 2023
Open
This was referenced Oct 21, 2023
Merged
Merged
wouterj added a commit to symfony/symfony-docs that referenced this pull request Dec 12, 2023
@wouterj
feature #19000 [Routing][Security] Document the LogoutRouteLoader (…
f0f261d 
…MatTheCat)

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

Discussion
----------

[Routing][Security] Document the `LogoutRouteLoader`

Related to
* symfony/symfony#50946

Commits
-------

8906132 [Routing][Security] Document the `LogoutRouteLoader`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof requested changes

@wouterj wouterj wouterj approved these changes

@chalasr chalasr chalasr approved these changes

+1 more reviewer

@maxhelias maxhelias maxhelias approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Feature Routing SecurityBundle ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

[Security] Automatically create logout route if it does not exists
6 participants
@MatTheCat @wouterj @stof @chalasr @maxhelias @carsonbot