[Security\Csrf] Split CsrfTokenGenerator into CsrfTokenManager and TokenGenerator #9216
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| if ($csrfTokenManager instanceof CsrfProviderInterface) { | ||
| $csrfTokenManager = new CsrfProviderAdapter($csrfTokenManager); | ||
| } elseif (null !== $csrfTokenManager && !$csrfTokenManager instanceof CsrfTokenManagerInterface) { | ||
| throw new UnexpectedTypeException($csrfTokenManager, 'CsrfProviderInterface or CsrfTokenManagerInterface'); |
There was a problem hiding this comment.
you should use FQCN in the message IMO
There was a problem hiding this comment.
I thought about that, but then it becomes insanely long
|
Thanks once again @stof for your invaluable feedback. I addressed your comments in the latest commit. |
fabpot
added a commit
that referenced
this pull request
Oct 7, 2013
…ger and TokenGenerator (bschussek) This PR was merged into the master branch. Discussion ---------- [Security\Csrf] Split CsrfTokenGenerator into CsrfTokenManager and TokenGenerator | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #9210 | License | MIT | Doc PR | - This is a follow-up PR of #6554 that splits the CsrfTokenGenerator into two separate classes for generating and managing CSRF tokens. As a consequence, it is now possible to explicitly remove or refresh CSRF tokens if they should be used only once. See #9210 for more information. Commits ------- d4bb5f4 [Security\Csrf] Split CsrfTokenGenerator into CsrfTokenManager and TokenGenerator
| * | ||
| * @param string $tokenId The token ID | ||
| * | ||
| * @return Boolean Returns true if a token existed for this ID, false |
There was a problem hiding this comment.
this does not correspond with the implementation. the implementation calls storage->remove which does not return a boolean but string|null. so either the implementation is wrong or this phpdoc is wrong.
Closed
fabpot
added a commit
that referenced
this pull request
Oct 17, 2013
This PR was squashed before being merged into the master branch (closes #9311). Discussion ---------- [Csrf] component fixes | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #9216 | License | MIT | Doc PR | - - [Csrf] fixed some phpdocs - [Csrf] fixed return types (also #9216 (comment) ) - [Csrf] fixed test class namespaces Commits ------- d7eb8ff [Csrf] component fixes
fabpot
added a commit
that referenced
this pull request
Jun 7, 2015
…precation (jakzal) This PR was merged into the 2.7 branch. Discussion ---------- [FrameworkBundle] Document form.csrf_provider service deprecation | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14899 | License | MIT | Doc PR | - `form.csrf_provider` was deprecated in #9216. Commits ------- 0fea66f [FrameworkBundle] Document form.csrf_provider service deprecation
This was referenced Jan 30, 2016
fabpot
added a commit
that referenced
this pull request
Dec 22, 2022
…enerator` to `firewalls.logout.csrf_token_manager` (MatTheCat) This PR was merged into the 6.3 branch. Discussion ---------- [SecurityBundle] Rename `firewalls.logout.csrf_token_generator` to `firewalls.logout.csrf_token_manager` | 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 <code>csrf_token_**generator**</code> to <code>security.csrf.token_**manager**</code> as mentioned in [the documentation](https://symfony.com/doc/current/reference/configuration/security.html#csrf-token-generator). As this confusion recently led to #48339, I propose to rename `firewalls.logout.csrf_token_generator` to `firewalls.logout.csrf_token_manager`. Commits ------- 0a0a98a [SecurityBundle] Rename `firewalls.logout.csrf_token_generator` to `firewalls.logout.csrf_token_manager`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up PR of #6554 that splits the CsrfTokenGenerator into two separate classes for generating and managing CSRF tokens. As a consequence, it is now possible to explicitly remove or refresh CSRF tokens if they should be used only once. See #9210 for more information.