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

Update security config to use the new authenticator-based system #13507

Merged

Conversation

rimas-kudelis
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets none
License MIT

This patch migrates security config to the new authenticator-based system introduced in Symfony 5.1.
Guard-based authentication has been deprecated in Symfony 5.3. Luckily, not much needs to change for us since Sylius didn't have any custom Guard authenticators to begin with.

The patch also drops always_authenticate_before_granting: true from config. This option has been deprecated since Symfony 5.4, and it's unclear why or whether at all it was needed in the first place, and it caused me personally some headache (see symfony/symfony#43375).

Similar PR for Sylius Standard: Sylius/Sylius-Standard#660.

@rimas-kudelis rimas-kudelis requested a review from a team as a code owner January 19, 2022 06:41
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Jan 19, 2022
@loic425
Copy link
Member

loic425 commented Jan 19, 2022

@Zales0123 @lchrusciel
I checked the documentations, and everything seems ok. cf docs links below

@rimas-kudelis
Copy link
Contributor Author

BTW i'm not sure about about password hashing config (the three lines with sha512 in the security config). I didn't touch these lines, bet are they really necessary?

@loic425
Copy link
Member

loic425 commented Jan 19, 2022

BTW i'm not sure about about password hashing config (the three lines with sha512 in the security config). I didn't touch these lines, bet are they really necessary?

Hum... In a Symfony app, that should be auto. I think we can do this in another PR.

@rimas-kudelis
Copy link
Contributor Author

The lines were added in #12329, so I suppose it has something to do with tests maybe.
But then I guess these lines should go into test/security.yaml instead.

And there's no need to auto explicitly, it's the default anyway, isn't it? Symfony docs don't even mention these options anymore for some reason.

@loic425
Copy link
Member

loic425 commented Jan 19, 2022

And there's no need to auto explicitly, it's the default anyway, isn't it? Symfony docs don't even mention these options anymore for some reason.

For the auto configuration, it's configured via flex recipe => https://github.com/symfony/recipes/blob/master/symfony/security-bundle/5.3/config/packages/security.yaml

@rimas-kudelis
Copy link
Contributor Author

So I guess I'll remove these lines in a separate PR then, and we'll see whether tests will still pass? 😁 Does that sound okay?

@Roshyo
Copy link
Contributor

Roshyo commented Jan 19, 2022

I also agree to remove it in a separate PR

Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

Thanks a lot! However, this config is used only in test env. Perhaps, we should add some recommendation in UPGRADE.md

@lchrusciel lchrusciel changed the base branch from master to 1.10 January 21, 2022 15:16
@lchrusciel
Copy link
Member

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "chore/update-security-config" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

@rimas-kudelis
Copy link
Contributor Author

rimas-kudelis commented Jan 24, 2022

@lchrusciel but this change is specific to Symfony 5.1+. Shouldn't it target master since that's the only place where Symfony 4.4 is no longer supported?

@lchrusciel
Copy link
Member

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "chore/update-security-config" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

@lchrusciel lchrusciel changed the base branch from 1.10 to master January 24, 2022 09:20
@lchrusciel
Copy link
Member

Hey Rimas, my mistake. Thanks for mentioning it

@lchrusciel lchrusciel closed this Jan 24, 2022
@lchrusciel lchrusciel reopened this Jan 24, 2022
@lchrusciel lchrusciel added the Maintenance CI configurations, READMEs, releases, etc. label Jan 24, 2022
@lchrusciel lchrusciel merged commit 72ce268 into Sylius:master Jan 24, 2022
@lchrusciel
Copy link
Member

Thanks, Rimas! 🎉

@rimas-kudelis rimas-kudelis deleted the chore/update-security-config branch January 24, 2022 13:21
Copy link
Member

@GSadee GSadee left a comment

Choose a reason for hiding this comment

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

@rimas-kudelis If I see correctly, you've changed only security.yaml file in the test application in ApiBundle, what about the main application config/packages/security.yaml?

@rimas-kudelis
Copy link
Contributor Author

@GSadee ouch, that one somehow escaped my attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs. Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants