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

Fix symfony 5.3 security incompatiblity #106

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

acrobat
Copy link
Contributor

@acrobat acrobat commented Feb 13, 2022

Follow up of #100

The AuthenticationTrustResolverInterface::isAuthenticated method was only added in symfony 5.4 but the AuthenticatedVoter::PUBLIC_ACCESS constant was already available in symfony 5.3, this causes an error when using security-acl 3.3.0 with symfony 5.3.x

if (\defined('\Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter::PUBLIC_ACCESS')) {
return !$this->authenticationTrustResolver->isAuthenticated($token);
}

The first commit actually refactors the test case because even when executing the tests on symfony 5.3 they still pass as we mocked that class/method. I've also changed some things in the github ci workflow because I wasn't able to get symfony 5.3 dependencies with the ramsey/composer-install packages (lowest = install, highest = update but this would cause dev packages in this setup), so I'm not sure if this is the desired setup. Let me know if I need the change anything.

The second commit actually fixes the problem (the first commit shows a failing test on symfony 5.3 first)

@derrabus derrabus requested a review from wouterj February 13, 2022 13:35
@derrabus
Copy link
Member

derrabus commented Feb 13, 2022

Do you really need this fix? That's an incredibly complex test setup for an EOL Symfony branch. 😕

@acrobat
Copy link
Contributor Author

acrobat commented Feb 13, 2022

Hmm yes, I thought 5.3 was still supported but you are right as this was also my feeling that the workflow got more complex. Maybe revert the workflow change but keep the test changes and code fix so the library is compatible? Or bump the version constraint for 3.3.x to `symfony/security-core: "^4.4|^5.4|^6.0"?

@derrabus
Copy link
Member

I'd be in favor of only merging the second commit and tag a release, if @wouterj agrees. For 3.4 we should make the bump you've suggested.

@acrobat acrobat force-pushed the fix-sf53-incompatibility branch from 1c3469e to a0ceb91 Compare February 14, 2022 11:01
@acrobat
Copy link
Contributor Author

acrobat commented Feb 14, 2022

@derrabus That works for me! I've updated the PR to only include the last commit with the fix. The other can still be viewed here -> acrobat@50f1abe

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

(I'm not close to a laptop this week, change is OK by me and if @derrabus can do a release afterwards that would be great 😃)

Domain/SecurityIdentityRetrievalStrategy.php Outdated Show resolved Hide resolved
@acrobat acrobat force-pushed the fix-sf53-incompatibility branch from a0ceb91 to 37d5165 Compare February 14, 2022 17:33
@acrobat acrobat force-pushed the fix-sf53-incompatibility branch from 37d5165 to 94c127b Compare February 14, 2022 17:34
@derrabus
Copy link
Member

Thank you @acrobat.

@derrabus derrabus merged commit 47af09b into symfony:main Feb 15, 2022
@derrabus
Copy link
Member

https://github.com/symfony/security-acl/releases/tag/v3.3.1

@acrobat acrobat deleted the fix-sf53-incompatibility branch February 16, 2022 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants