Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Security] Add a method in the security helper to ease programmatic logout#41406

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 3 commits intosymfony:6.2fromjohnkrovitch:feature/auto-logout
Jul 20, 2022

Conversation

johnkrovitch
Copy link
Contributor

@johnkrovitchjohnkrovitch commentedMay 25, 2021
edited
Loading

QA
Branch?6.x
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#40663
LicenseMIT
Doc PR

This PR aims to ease the programmatic login using the Security helper, to fix (#40663).

A simple method has been added to the Security helper.

Thanks !

Warxcell, chalasr, HypeMC, buffcode, jannes-io, and artyuum reacted with thumbs up emoji
@nicolas-grekasnicolas-grekas added this to the5.4 milestoneMay 26, 2021
@johnkrovitchjohnkrovitchforce-pushed thefeature/auto-logout branch 2 times, most recently fromc4646e3 to23aad7bCompareMay 31, 2021 09:36
Copy link
Member

@SeldaekSeldaek 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 working on this! There are a few points which IMO need some work still

@wouterjwouterj modified the milestones:5.4,6.1Nov 4, 2021
@johnkrovitch
Copy link
ContributorAuthor

Thanks for your returns, I'm working on it.

@johnkrovitchjohnkrovitchforce-pushed thefeature/auto-logout branch 9 times, most recently frome824ed6 toc135401CompareDecember 2, 2021 14:53
Copy link
Member

@wouterjwouterj left a comment

Choose a reason for hiding this comment

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

Hi there! Thanks for updating this PR :)

I've left some final improvements. Some of them a bit tricky, so feel free to leave a comment asking for help if you don't understand how to apply them :)

@johnkrovitchjohnkrovitchforce-pushed thefeature/auto-logout branch 5 times, most recently from94c7372 to5d42598CompareDecember 2, 2021 15:47
@Seldaek
Copy link
Member

I wonder if there should be a way to bypass CSRF? One of the use cases where I've needed programmatic logout is when handling user invitations.. There if you click the invite email and you are already logged in we show "accept invite" or "create new account". If you click that we log you out and send you back to the invite page which then shows a registration form.

In this case, we authenticate the request has the token from the email anyway, and we absolutely do not need a CSRF token.. Of course we could add one on the "create new account" button I guess if needed, just doesn't seem necessary.

@chalasr
Copy link
Member

I think most use cases for this method are in similar situations. I'll think of a way to bypass it, will probably end up with a bool parameter. Thanks!

@Seldaek
Copy link
Member

@chalasr thanks, LGTM now I think - haven't reviewed the whole code in depth though, but on the surface the API makes sense to me.

@chalasrchalasrforce-pushed thefeature/auto-logout branch 3 times, most recently fromefc530c toea80c5cCompareJuly 19, 2022 15:52
@wouterj
Copy link
Member

I'm not 100% sure, but I'm leaning towardsnot allowing to bypass CSRF. If an application enabled logout CSRF as a way to prevent login csrf, I would expect all ways of logging out to be protected by CSRF. With this boolean, I no longer can be sure.

Of course,@Seldaek understands login CSRF and knows that CSRF would only be "added noise" in this specific scenario, but I think the framework has a duty to protect those unaware of login CSRF to be as safe as possible.

@chalasr
Copy link
Member

CSRF protection is pointless if logout happens on a GET request, or as a side effect of a state change of another resource which might itself already be CSRF protected using a different token, or Jordi's use case.
I strongly think such cases are the ones for which this method makes the most sense instead of requesting a regular logout endpoint, I would find it too bad to force dealing with it while it's not needed

@Seldaek
Copy link
Member

Yeah IMO there's a clear need for the bypass.. But I could live with csrf being enabled by default instead of bypassed by default.

@fabpot
Copy link
Member

I think that having the default value (for CSRF validation) set totrue by default is enough for me as it forces a conscious decision from the developer.

@fabpot
Copy link
Member

Thank you@johnkrovitch.

@johnkrovitch
Copy link
ContributorAuthor

You're welcome. Thank you all

@johnkrovitchjohnkrovitch deleted the feature/auto-logout branchOctober 5, 2022 09:00
wouterj added a commit to symfony/symfony-docs that referenced this pull requestOct 16, 2022
This PR was merged into the 6.2 branch.Discussion---------- docs: add docs for programmatic logoutThis PR adds documentation for the new feature "programmatic logout" merge in the PRsymfony/symfony#41406Commits-------ac46df4 [#17328] Minor changes5611f88 docs: add docs for programmatic logout
@fabpotfabpot mentioned this pull requestOct 24, 2022
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull requestJul 28, 2023
…to SecurityBundle (chalasr)This PR was merged into the 6.2 branch.Discussion----------[Security][SecurityBundle] Move the `Security` helper to SecurityBundle| Q             | A| ------------- | ---| Branch?       | 6.2| Bug fix?      | no| New feature?  | yes| Deprecations? | yes| Tickets       |Fixessymfony/symfony#46066 (comment)| License       | MIT| Doc PR        | todoThe `Security` helper is a high-level service providing an easy access to commonly-needed features coming from various low-level abstractions. Basically, it's a facade.Based on this, it makes sense to me to make it available only via the full-stack framework, as proposed by Wouter insymfony/symfony#46066 (comment).This unlocks #46066,symfony/symfony#41274 andsymfony/symfony#41406./cc@wouterj@johnkrovitch@KocalCommits-------7b91bcb068 [Security] Move the `Security` helper to SecurityBundle
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@SeldaekSeldaekSeldaek left review comments

@GwendolenLynchGwendolenLynchGwendolenLynch left review comments

@KocalKocalKocal left review comments

@fabpotfabpotfabpot approved these changes

@wouterjwouterjAwaiting requested review from wouterjwouterj is a code owner

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.

[Security][DX] RFC: A simple way to do programmatic logout
10 participants
@johnkrovitch@wouterj@fabpot@chalasr@stof@Seldaek@GwendolenLynch@Kocal@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp