Skip to content

[Routing][Security] Document the LogoutRouteLoader #19000

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
Dec 12, 2023

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Oct 6, 2023

@javiereguiluz javiereguiluz added Routing Security Waiting Code Merge Docs for features pending to be merged labels Oct 9, 2023
@carsonbot carsonbot changed the title [Security] Document the LogoutRouteLoader [Routing][Security] Document the LogoutRouteLoader Oct 9, 2023
@carsonbot carsonbot added this to the next milestone Oct 9, 2023
wouterj added a commit to symfony/symfony that referenced this pull request Oct 15, 2023
…TheCat)

This PR was merged into the 6.4 branch.

Discussion
----------

[Routing][SecurityBundle] Add `LogoutRouteLoader`

| 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](#37786), 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:

```yaml
# 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.

Commits
-------

0a558d0 [SecurityBundle][Routing] Add `LogoutRouteLoader`
@wouterj wouterj removed the Waiting Code Merge Docs for features pending to be merged label Oct 15, 2023
Copy link
Contributor

@alamirault alamirault left a comment

Choose a reason for hiding this comment

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

Thanks @MatTheCat for this PR and improving DX !

I had so many time forget to create logout route, or an empty controller felt weird in source code

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.

Thanks for providing all the necessary PRs around your feature!

I've made some suggestions to move away all unnecessary details and showcase how simple this is now :)

Comment on lines +1847 to +1848
Symfony will then un-authenticate users navigating to the configured ``path``,
and redirect them to the configured ``target``.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Symfony will then un-authenticate users navigating to the configured ``path``,
and redirect them to the configured ``target``.
Symfony will then un-authenticate users navigating to the configured ``path``,
and redirect them to the configured ``target``. You can generate URLs to this
path using the ``_security_<firewallname>`` route name (e.g. ``_security_main``).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn’t the LogoutUrlGenerator be mentioned instead?

@wouterj wouterj force-pushed the logout-route-loader branch from 209f22b to 8906132 Compare December 12, 2023 22:12
@wouterj wouterj merged commit f0f261d into symfony:6.4 Dec 12, 2023
@wouterj
Copy link
Member

wouterj commented Dec 12, 2023

Thank you @MatTheCat for also providing the docs of this cool feature!

@MatTheCat MatTheCat deleted the logout-route-loader branch December 13, 2023 05:07
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.

7 participants