Fix failing tests on symfony 6 #100

acrobat · 2021-12-03T07:22:31Z

AuthenticationTrustResolverInterface::isAnonymous() is called, but that class has been removed in Symfony 6.
Some tests use the AnonymousToken class which has also been removed.
The constant AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY is accessed. Gone as well.

jordisala1991 · 2021-12-03T08:24:58Z

You will need to check for method_exists (on the tests too).

acrobat · 2021-12-03T12:51:47Z

Yes indeed, this was a quick test to fix it and to open the draft PR. But I will make some time in the next few days to finish this!

acrobat · 2021-12-04T13:41:53Z

PR is ready for review. I'm not sure how I can solve that latest psalm error, if any can help me with this?

jordisala1991 · 2021-12-04T14:03:28Z

You need to use the following annotation @psalm-suppress UndefinedInterfaceMethod just before the line where it is complaining. This kind of error is due to psalm checking on a version where this method does not exist.

acrobat · 2021-12-04T14:43:45Z

Thanks @jordisala1991, I wasn't sure if that was the "correct" fix. Php-cs-fixer is not happy with the psalm docblock but it is correct so can be ignored

Domain/SecurityIdentityRetrievalStrategy.php

wouterj · 2021-12-11T10:02:36Z

Domain/SecurityIdentityRetrievalStrategy.php

+        $anonymousRole = \defined('\Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter::PUBLIC_ACCESS') ? AuthenticatedVoter::PUBLIC_ACCESS : AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY;
        // add built-in special roles
        if ($this->authenticationTrustResolver->isFullFledged($token)) {
            $sids[] = new RoleSecurityIdentity(AuthenticatedVoter::IS_AUTHENTICATED_FULLY);
            $sids[] = new RoleSecurityIdentity(AuthenticatedVoter::IS_AUTHENTICATED_REMEMBERED);
-            $sids[] = new RoleSecurityIdentity(AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY);
+            $sids[] = new RoleSecurityIdentity($anonymousRole);
        } elseif ($this->authenticationTrustResolver->isRememberMe($token)) {
            $sids[] = new RoleSecurityIdentity(AuthenticatedVoter::IS_AUTHENTICATED_REMEMBERED);
-            $sids[] = new RoleSecurityIdentity(AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY);
-        } elseif ($this->authenticationTrustResolver->isAnonymous($token)) {
-            $sids[] = new RoleSecurityIdentity(AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY);
+            $sids[] = new RoleSecurityIdentity($anonymousRole);
+        } elseif ($this->isNotAuthenticated($token)) {
+            $sids[] = new RoleSecurityIdentity($anonymousRole);
        }


I've never used ACL, so I'm not sure about this, but I guess change the security identity is a BC break (as these are persisted, so IS_AUTHENTICATED_ANONYMOUSLY might be persisted still?). Can you confirm (or reject) this?

If this is the case, we should support both token names I think (with hardcoded strings) and then somehow trigger a deprecation when the anonymous role is used.

Hmm, you're right. I will add the old role back, but I'm not sure on how to deprecate this? Would be a warning/notice in an upgrade file be enough? Or even the symfony deprecation itself is enough?

It would be great if we can add a specialized deprecation notice about it.

Can we do something in AclProvider::findAcls(), that triggers whenever we fetch a security identity with the old attribute name?

Domain/SecurityIdentityRetrievalStrategy.php

VincentLanglet

A psalm-suppress UndefinedInterfaceMethod should be added for the psalm error

jordisala1991 · 2021-12-16T02:31:56Z

A psalm-suppress UndefinedInterfaceMethod should be added for the psalm error

Not really, on Symfony they do not use it, @wouterj requested to remove it on a previous comment.

VincentLanglet · 2021-12-16T07:32:45Z

A psalm-suppress UndefinedInterfaceMethod should be added for the psalm error

Not really, on Symfony they do not use it, @wouterj requested to remove it on a previous comment.

So this is ready to be merged then ^^

acrobat · 2021-12-16T08:05:53Z

There is one left todo and that is find a way to trigger a deprecation when the old anonymous role string is used instead of the new public role

VincentLanglet · 2021-12-19T10:52:53Z

There is one left todo and that is find a way to trigger a deprecation when the old anonymous role string is used instead of the new public role

@wouterj Couldn't the deprecation being added later (with maybe an issue about it) ? Symfony 6 support shouldn't be blocked by a missing deprecation.

VincentLanglet · 2022-01-05T14:00:14Z

@wouterj Couldn't the deprecation being added later (with maybe an issue about it) ? Symfony 6 support shouldn't be blocked by a missing deprecation.

Friendly ping @derrabus @wouterj

wouterj · 2022-01-05T14:56:37Z

We not only need a deprecation, but the BC layer is missing as well (keeping the IS_AUTHENTICATED_ANONYMOUSLY). This means the current state of the PR is a hard backwards compatibility break, which is worse than not supporting Symfony 6.

jordisala1991 · 2022-01-05T17:49:04Z

@acrobat do you have time for finishing the PR? If not I will try to finish it

acrobat · 2022-01-06T11:20:20Z

@jordisala1991 I have some time to do fixes, but extra help is welcome! Maybe you can send a pr to my fork to include the remaining fixes.

I've pushed a some changes that I had locally to avoid the hard BC break with the anonymous role, so I just needs some help with the deprecation wouter asked for!

EDIT: seems that my last "fixes" broke some tests, I will take a look later today to make them pass again!

wouterj · 2022-01-06T11:24:05Z

Thank you @acrobat. Changes are looking good to me (once you've fixed the failures).

acrobat · 2022-01-06T12:51:58Z

Tests are passing again 🎉

wouterj · 2022-01-06T16:23:56Z

Question to all the ACL users in this PR: Is getSecurityIdentities() used in a normal application? Or is it's usage limited to voters (e.g. the AclVoter provided by this package)?

I'm asking because making an argument nullable is I'm afraid a BC break. But if it's only used in a voter, that isn't that bad as these will get a NullToken instance instead of null, so we can pass this token to the getSecurityIdentities(). However this fix no longer holds true if the method is called outside a voter.

derrabus · 2022-01-06T16:28:49Z

making an argument nullable is I'm afraid a BC break.

We can document that and bump the major version.

acrobat · 2022-01-06T18:57:35Z

I've added a breakpoint in Symfony\Component\Security\Acl\Domain\SecurityIdentityRetrievalStrategy::getSecurityIdentities and this method is only called from the AclVoter class in our case. This is an app with "default" usage of the acl component/bundle

acrobat · 2022-01-07T15:03:37Z

@wouterj now that I'm thinking about your comment of the nullable argument. I've did this change because of the deprecation in AnonymousToken -> @deprecated since 5.4, anonymous is now represented by the absence of a token, but if I understand your comment correctly this will now be a NullToken instead of an anonymous token?

If that's the case we don't even need to make it nullable.

EDIT: I've tested this in a new application with the new authentication setup and a NullToken is indeed passed, in my last commit I've removed the nullable argument change and adapted the code to the NullToken. This is can reviewed again

wouterj · 2022-01-07T15:44:21Z

if I understand your comment correctly this will now be a NullToken instead of an anonymous token?

Yes and no :) "anonymous is now represented by the absence of a token" is true for all parts of the application, except from the voter. This allows voters to vote on "anonymous" access (e.g. if some blog posts are premium only, but others can be read by anyone).
Given your comment, I think it's safe to assume no null be used for this method. And if it does, we'll surely get a bug report about it. :)

wouterj

Looks good to me!

Thanks for all your work and persistence :)

jordisala1991 · 2022-01-19T08:24:50Z

Question to all the ACL users in this PR: Is getSecurityIdentities() used in a normal application? Or is it's usage limited to voters (e.g. the AclVoter provided by this package)?

I tried a search on the whole sonata codebase through github and we have no usage of that method.

wouterj · 2022-01-19T16:29:40Z

Thank you Jeroen!

This PR was merged into the 3.x-dev branch. Discussion ---------- Fix symfony 5.3 security incompatiblity 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 https://github.com/symfony/security-acl/blob/04d6fadd671d72ff322e20840e510030753e008a/Domain/SecurityIdentityRetrievalStrategy.php#L81-L83 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) Commits ------- 94c127b Fix symfony 5.3 incompatibility

acrobat mentioned this pull request Dec 3, 2021

Symfony 6 support #99

Closed

acrobat force-pushed the fix-tests-for-symfony6 branch 13 times, most recently from 1dfe279 to 02271e7 Compare December 4, 2021 13:39

acrobat marked this pull request as ready for review December 4, 2021 13:41

acrobat force-pushed the fix-tests-for-symfony6 branch 4 times, most recently from efaefff to c551a66 Compare December 4, 2021 14:43

jordisala1991 reviewed Dec 4, 2021

View reviewed changes

Domain/SecurityIdentityRetrievalStrategy.php Outdated Show resolved Hide resolved

acrobat force-pushed the fix-tests-for-symfony6 branch 3 times, most recently from 2b949f5 to dcb0336 Compare December 4, 2021 14:58

jordisala1991 reviewed Dec 4, 2021

View reviewed changes

Domain/SecurityIdentityRetrievalStrategy.php Outdated Show resolved Hide resolved

acrobat force-pushed the fix-tests-for-symfony6 branch from dcb0336 to c918eed Compare December 4, 2021 16:08

wouterj reviewed Dec 11, 2021

View reviewed changes

Domain/SecurityIdentityRetrievalStrategy.php Outdated Show resolved Hide resolved

acrobat force-pushed the fix-tests-for-symfony6 branch from c918eed to 0fb9eb4 Compare December 11, 2021 11:07

VincentLanglet reviewed Dec 15, 2021

View reviewed changes

acrobat force-pushed the fix-tests-for-symfony6 branch from bc3bea5 to 704fc28 Compare January 6, 2022 12:50

wouterj approved these changes Jan 7, 2022

View reviewed changes

jordisala1991 approved these changes Jan 19, 2022

View reviewed changes

Fix failing tests on symfony 6

3492ce6

wouterj force-pushed the fix-tests-for-symfony6 branch from 3137202 to 3492ce6 Compare January 19, 2022 16:29

wouterj merged commit 49bbad0 into symfony:main Jan 19, 2022

acrobat deleted the fix-tests-for-symfony6 branch January 19, 2022 16:46

acrobat mentioned this pull request Feb 13, 2022

Fix symfony 5.3 security incompatiblity #106

Merged

Uh oh!

Fix failing tests on symfony 6 #100

Fix failing tests on symfony 6 #100

Uh oh!

Conversation

acrobat commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jordisala1991 commented Dec 3, 2021

Uh oh!

acrobat commented Dec 3, 2021

Uh oh!

acrobat commented Dec 4, 2021

Uh oh!

jordisala1991 commented Dec 4, 2021

Uh oh!

acrobat commented Dec 4, 2021

Uh oh!

Uh oh!

Uh oh!

wouterj Dec 11, 2021

Choose a reason for hiding this comment

Uh oh!

acrobat Dec 11, 2021

Choose a reason for hiding this comment

Uh oh!

wouterj Dec 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VincentLanglet left a comment

Choose a reason for hiding this comment

Uh oh!

jordisala1991 commented Dec 16, 2021

Uh oh!

VincentLanglet commented Dec 16, 2021

Uh oh!

acrobat commented Dec 16, 2021

Uh oh!

VincentLanglet commented Dec 19, 2021

Uh oh!

VincentLanglet commented Jan 5, 2022

Uh oh!

wouterj commented Jan 5, 2022

Uh oh!

jordisala1991 commented Jan 5, 2022

Uh oh!

acrobat commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wouterj commented Jan 6, 2022

Uh oh!

acrobat commented Jan 6, 2022

Uh oh!

wouterj commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derrabus commented Jan 6, 2022

Uh oh!

acrobat commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acrobat commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wouterj commented Jan 7, 2022

Uh oh!

wouterj left a comment

Choose a reason for hiding this comment

Uh oh!

jordisala1991 commented Jan 19, 2022

Uh oh!

wouterj commented Jan 19, 2022

Uh oh!

Uh oh!

acrobat commented Dec 3, 2021 •

edited

Loading

acrobat commented Jan 6, 2022 •

edited

Loading

wouterj commented Jan 6, 2022 •

edited

Loading

acrobat commented Jan 6, 2022 •

edited

Loading

acrobat commented Jan 7, 2022 •

edited

Loading