Skip to content
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

document old way of checking validity of CSRF token #5818

Closed
wants to merge 5 commits into from
Closed

document old way of checking validity of CSRF token #5818

wants to merge 5 commits into from

Conversation

snoek09
Copy link

@snoek09 snoek09 commented Oct 20, 2015

Q A
Doc fix? yes
New docs? yes
Applies to all
Fixed tickets Related to #4668

@xabbuh
Copy link
Member

xabbuh commented Oct 20, 2015

👍

Sometimes you want to use CSRF protection in an action where you don't want to use a
Symfony form.

If, for example, you're doing a DELETE action, you can use the
Copy link
Contributor

Choose a reason for hiding this comment

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

DELETE in backticks?

Copy link
Author

Choose a reason for hiding this comment

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

I checked the docs for mentions of HTTP methods. In a sentence they are never in backticks.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@snoek09
Copy link
Author

snoek09 commented Oct 24, 2015

@xabbuh please let me know if the example code is fine now.

@snoek09
Copy link
Author

snoek09 commented Dec 1, 2015

@xabbuh is this PR finished?

Symfony form.

If, for example, you're doing a DELETE action, you can use the
:method:`Symfony\\Component\\Form\\Extension\\Csrf\\CsrfProvider\\DefaultCsrfProvider::isCsrfTokenValid`
Copy link
Member

Choose a reason for hiding this comment

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

I think this should link to the method in the interface instead (it doesn't matter which implementation is registered as the service).

@xabbuh
Copy link
Member

xabbuh commented Dec 1, 2015

@snoek09 Sorry for missing your last comment. I left a last minor comment. After that this looks good to be merged to me.

@snoek09
Copy link
Author

snoek09 commented Dec 1, 2015

@xabbuh No problem. I updated the link.

@OskarStark
Copy link
Contributor

👍

@snoek09
Copy link
Author

snoek09 commented Dec 9, 2015

@xabbuh I think this is ready to be merged.

@xabbuh
Copy link
Member

xabbuh commented Dec 9, 2015

@weaverryan @wouterj @javiereguiluz Ready to go here? :)

method to check the CSRF token::

$csrf = $this->container->get('form.csrf_provider');
$intention = 'authenticate';
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to show this without showing a template? When you generate the CSRF token in the template, you would need to also include authenticate then, right?

@weaverryan
Copy link
Member

I added some comments, but I like it! Short and simple.

To be clear, this is not an "old way" of checking CSRF, correct? It seems totally valid and current.

Thanks!

@ogizanagi
Copy link
Contributor

@weaverryan : No it isn't, because the form.csrf_provider service is deprecated since 2.4, as well as the whole CsrfProviderInterface. We use security.csrf.token_manager instead.

However, I wonder if the book/controller.rst page is the right place for this ? In #4668, @wouterj suggested to create a new entry in the cookbook instead, and to me it actually makes much more sense to have this in a Controller or Security cookbook section.
I know PRs for 2.6+ are already merged (#5325), but what do you think ?

@snoek09
Copy link
Author

snoek09 commented Dec 15, 2015

@ogizanagi is right. The example code has to work with 2.3 using the 'old way'.
I also agree that this documentation should be moved to the cookbook.

@xabbuh
Copy link
Member

xabbuh commented Dec 15, 2015

I think we can merge it just as it is and then work on converting this into a cookbook recipe afterwards. What do you think?

@ogizanagi
Copy link
Contributor

👍

@snoek09
Copy link
Author

snoek09 commented Dec 15, 2015

I like that idea @xabbuh.

@xabbuh
Copy link
Member

xabbuh commented Dec 31, 2015

@weaverryan @wouterj @javiereguiluz Okay with merging and thinking about how to rework the whole part afterwards?

@javiereguiluz
Copy link
Member

@xabbuh yes!

@weaverryan
Copy link
Member

👍

@xabbuh
Copy link
Member

xabbuh commented Jan 11, 2016

Thank you Henry.

xabbuh added a commit that referenced this pull request Jan 11, 2016
…k09)

This PR was squashed before being merged into the 2.3 branch (closes #5818).

Discussion
----------

document old way of checking validity of CSRF token

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | yes
| Applies to    | all
| Fixed tickets | Related to #4668

Commits
-------

8257cc8 document old way of checking validity of CSRF token
@xabbuh xabbuh closed this Jan 11, 2016
@snoek09 snoek09 deleted the document-old-way-checking-validity-csrf-token branch January 11, 2016 10:22
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