[DX] [FrameworkBundle] Added Crontoller::isCsrfTokenValid #11602
Conversation
lyrixx
commented
Aug 7, 2014
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | |
| License | MIT |
| Doc PR |
|
👍 |
| use Symfony\Component\Form\Form; | ||
| use Symfony\Component\Form\FormBuilder; | ||
| use Symfony\Component\Form\FormTypeInterface; | ||
| use Symfony\Component\HttpFoundation\RedirectResponse; |
There was a problem hiding this comment.
Please don't reorder use statements. It makes merging maintenance branches harder
|
👍 |
| public function isCsrfTokenValid($id, $token) | ||
| { | ||
| if (!$this->container->has('security.csrf.token_manager')) { | ||
| throw new \LogicException('The SecurityBundle is not registered in your application.'); |
There was a problem hiding this comment.
The CSRF protection is not configured by SecurityBundle but by FrameworkBundle. A better error message would be CSRF protection is not enabled in your application
|
👍 |
| @@ -168,6 +169,23 @@ public function createAccessDeniedException($message = 'Access Denied', \Excepti | ||
| } | ||
|
|
||
| /** | ||
| * Checks the validity of a CsrfToken |
There was a problem hiding this comment.
I would say of a CSRF token. the way you wrote it looks like it is a class name, and the method does not accept a CsrfToken instance as argument
|
@stof fixed. (thanks) |
|
👍 |
2 similar comments
|
👍 |
|
👍 |
| * | ||
| * @return boolean | ||
| */ | ||
| public function isCsrfTokenValid($id, $token) |
There was a problem hiding this comment.
This method should be protected, right?
There was a problem hiding this comment.
yes (even if all existing helpers are protected for a weird reason)
|
👍 |
| * @param string $id The id used when generating the token | ||
| * @param string $token The actual token sent with the request that should be validated | ||
| * | ||
| * @return boolean |
|
👍 |
|
Can you add it in the CHANGELOG file of FrameworkBundle ? |
|
@stof I wanted to do that, But I see this file is never update for a new method: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md So Should I update the file, or it's OK? |
|
@lyrixx IMO, the fact that we add a new helper method should be part of the changelog (improving DX without telling it to our users is bad from a DX point of view) |
|
@stof ok. I just pushed a new commit. |
|
Thank you @lyrixx. |
…lid (lyrixx) This PR was merged into the 2.6-dev branch. Discussion ---------- [DX] [FrameworkBundle] Added Crontoller::isCsrfTokenValid | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Commits ------- 479c833 [FrameworkBundle] Added Crontoller::isCsrfTokenValid