Skip to content

[SecurityBundle] Rename firewalls.logout.csrf_token_generator to firewalls.logout.csrf_token_manager #48387

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 22, 2022

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Nov 29, 2022

Q A
Branch? 6.3
Bug fix? no
New feature? no
Deprecations? yes
Tickets N/A
License MIT
Doc PR symfony/symfony-docs#17482

A long time ago, #6554 replaced CsrfProviderInterface by CsrfTokenGeneratorInterface, and #9216 split the latter into CsrfTokenManagerInterface and TokenGeneratorInterface. #9587 later introduced csrf_token_generator, which was already wrong at the time.

Given that token generators exist, it feels weird to have to set csrf_token_generator to security.csrf.token_manager as mentioned in the documentation.

As this confusion recently led to #48339, I propose to rename firewalls.logout.csrf_token_generator to firewalls.logout.csrf_token_manager.

@carsonbot
Copy link

Hey!

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

Cheers!

Carsonbot

@MatTheCat MatTheCat force-pushed the rename-csrf_token_generator branch from 90e4d48 to 5f56da3 Compare December 6, 2022 22:59
@fabpot fabpot force-pushed the rename-csrf_token_generator branch from 5f56da3 to 0a0a98a Compare December 22, 2022 16:59
@fabpot
Copy link
Member

fabpot commented Dec 22, 2022

Thank you @MatTheCat.

@fabpot fabpot merged commit 9df40aa into symfony:6.3 Dec 22, 2022
@@ -60,4 +60,5 @@ SecurityBundle
Validator
--------------

* Implementing the `ConstraintViolationInterface` without implementing the `getConstraint()` method is deprecated
* Implementing the `ConstraintViolationInterface` without implementing the `getConstraint()` method is deprecated
* Deprecate the `security.firewalls.logout.csrf_token_generator` config option, use `security.firewalls.logout.csrf_token_manager` instead
Copy link
Member

Choose a reason for hiding this comment

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

this should not be in the Validator section

Copy link
Member

Choose a reason for hiding this comment

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

fixed in 377982f

@MatTheCat MatTheCat deleted the rename-csrf_token_generator branch December 22, 2022 18:48
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Jan 30, 2023
…csrf_token_manager` (MatTheCat)

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

Discussion
----------

[Security] Rename logout’s `csrf_token_generator` to `csrf_token_manager`

Follow-up of symfony/symfony#48387

Should I still mention the deprecated `csrf_token_generator` in the documentation? If yes, how?

Commits
-------

ced6562 [Security] Rename logout’s `csrf_token_generator` to `csrf_token_manager`
@fabpot fabpot mentioned this pull request May 1, 2023
nicolas-grekas added a commit that referenced this pull request Jul 21, 2023
… to XSD (HeahDude)

This PR was merged into the 6.3 branch.

Discussion
----------

[SecurityBundle] Add `firewalls.logout.csrf_token_manager` to XSD

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | ~
| License       | MIT
| Doc PR        | ~

The XSD has been missed in #48387.

Commits
-------

742e5c2 [SecurityBundle] Add `firewalls.logout.csrf_token_manager` to XSD
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