symfony/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php

Line 34 ina3afd87

* @param array $attributes An array of attributes associated with the method being invoked

Should this doc be changed tostring[] in Symfony 6?

As I said, if we force typing on astring[], we will break the backward compatibility of some voters including the voterSymfony\Component\Security\Core\Authorization\Voter\ExpressionVoter that wait for an instance of the classSymfony\Component\ExpressionLanguage\Expression.

Only the helperVoter has this problem, because it is the only voter who expects astring to validate its use in thesupports method. Either thesupports method returnsfalse if$attribute is not a string, either you must do the verification just before calling thesupports method.

This change should be covered by a test. Also, your change might break some (probably rare) scenarios that would work right now:

• $attribute could be a stringable object.
• A voter could have removed thestring type declaration from thesupports() method signature.

@derrabus

Of course, I'm going to add a test :-)

$attribute could be a stringable object.

Indeed, we can very well have a stringable object, however, as has never been indicated in the documentation or the phpdoc, some voters are therefore not stringable.

A voter could have removed the string type declaration from the supports() method signature.

It was my first intention, however, we will break the Backward Compatibility if some voters override thesupports() method.. It is for this reason that I preferred to opt for a 'pre-test' of verification of the attribute.

Indeed, we can very well have a stringable object, however, as has never been indicated in the documentation or the phpdoc, some voters are therefore not stringable.

All voters that extend the abstractVoter class are. You are breaking that case:https://3v4l.org/2N1Hp

we will break the Backward Compatibility if some voters override thesupports() method.

I'm not suggesting to change thesupports() method. This is the case you might break:https://3v4l.org/MrcDq

Not all tests are passed in php 7.3, but these are tests for the Cache component. There was no error in the previous commit of this PR, so maybe just restart the tests for php 7.3 will remove the errorError: Call to undefined method Doctrine\DBAL\Driver\PDOSqlite\Driver::getName() (problem with a dependency version of Doctrine DBAL?).

@derrabus We do indeed agree on the 2 points. However, send a non-stringable attribute to a voter extending the abstract class `Voter', and you will get the error:

Argument 1 passed to FooVoter::supports() must be of the type string, object given

Given that theAccessDecisionManager and theAuthorizationChecker classes accept attributes of any type (mixed), the problem with the attributes not stringable (see the test in this PR).

As I said, if we force typing on a string[], we will break the backward compatibility

I know, This is why I said v6.

However, send a non-stringable attribute to a voter extending the abstract class `Voter', and you will get the error:

Not necessarily, please see the second 3v4l link I posted.

Why limit arbitrarily the type to an array of strings? It is only the aid class to build the Voter that has become blocking.

In addition, if we just change the phpdoc tostring[] for the$attributes argument ofSymfony\Component\Security\Core\Authorization\Voter\VoterInterface::vote(), we will also need to change the phpdoc forSymfony/Component/Security/Core/Authorization/AuthorizationCheckerInterface::isGranted() andSymfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface::decide().

The principle of thesupports() method is precisely to validate if the subject and attributes are supported by the voter.

Not necessarily, please see the second 3v4l link I posted.

Remove my modification in the classVoter and run the test of this PR, and you get the error.

Not necessarily, please see the second 3v4l link I posted.

Remove my modification in the classVoter and run the test of this PR, and you get the error.

Again, please see the second 3v4l link I posted above. It shows you a voter that passes.

Sorry, but I will repeat myself :-), but the test passes with my modification, but without my modification you get:

1) Symfony\Component\Security\Core\Tests\Authorization\Voter\VoterTest::testVote with data set #9 (array(stdClass Object ()), 0, stdClass Object (), 'ACCESS_ABSTAIN if attributes ...trings')TypeError: Argument 1 passed to Symfony\Component\Security\Core\Tests\Authorization\Voter\VoterTest_Voter::supports() must be of the type string, object given, called in E:\Git\php\symfony\symfony\src\Symfony\Component\Security\Core\Authorization\Voter\Voter.php on line 33E:\Git\php\symfony\symfony\src\Symfony\Component\Security\Core\Tests\Authorization\Voter\VoterTest.php:70E:\Git\php\symfony\symfony\src\Symfony\Component\Security\Core\Authorization\Voter\Voter.php:33E:\Git\php\symfony\symfony\src\Symfony\Component\Security\Core\Tests\Authorization\Voter\VoterTest.php:59

All right, I give up. 🤷🏻‍♂️

@francoispluchino try a testcase with a custom voter that removes the string type here;

we could simply remove the string types in core:

Yes, I had misunderstood the meaning of your answer, I am in the process of making the modification for this case.

@ro0NL If we delete the typestring, we break the voters implementing the methodsupports(string $attribute, $subject) :

PHP Fatal error:Declaration of Symfony\Component\Security\Core\Tests\Authorization\Voter\VoterTest_Voter::supports(string$attribute,$object): bool must be compatible withSymfony\Component\Security\Core\Authorization\Voter\Voter::supports($attribute,$subject) in E:\Git\php\symfony\symfony\src\Symfony\Component\Security\Core\Tests\Authorization\Voter\VoterTest.php on line72

Ah true. We could reflect e.g.$attributeMustBeString, or just catch the type error.

I just added the test concerning the use case of attributes with a typeobject but stringable (in rapport withthis comment).

@derrabus

Not necessarily, please see the second 3v4l link I posted.

As I point out to@ro0NL, your example can break compatibility with Voters implementing the new signature (seethis comment).

However, thank you for your example concerning the fact that a stringable object can no longer be validated by a Voter when it was the case before my PR. I just add this use case.

There's still the second case:https://3v4l.org/MrcDq, where attributes are just objects (non stringable), or maybe someone uses ints.array $attributes impliesmixed $attribute. It can be anything, depending on the voter removing the typehint yes/no.

@ro0NL Yes, but in this case, the Voter cannot extend the abstract classVoter, and so, the problem does not exist. Precisely the error arrives when you want to validate an attribute withobject type which is not stringable, and that a Voter extend the abstract classVoter. When this Voter tries to check out if the attribute is supported or not, the error occurs just before because the object cannot be converted to a string with the auto convertion by the type of the$attribute argument in thesupports() method.

Im not sure i follow now 😕

https://3v4l.org/MrcDq bool(true)
with your patchhttps://3v4l.org/1Ef7n bool(false)

I don't think it is advisable to remove the typing from the methodVoter::supports() and to update the phpdoc to indicate that the$attribute argument is now of typemixed. As indicated, we risk breaking compatibility with the classes being the new signature ofVoter::supports(). It is a helper class, it should keep for the majority cases, namely the use of a string for the attribute. Personally, I have no preference as to whether or not to keep strong typing on the methodVoter::supports(), but I would like to know the opinion of each as well as the Core team, if possible.

Im not sure i follow now 😕

https://3v4l.org/MrcDq bool(true)
with your patchhttps://3v4l.org/1Ef7n bool(false)

You remove the type of the$attribute argument on thesupports method. See my previous comment.

Note: However, your example is still valid, but if we remove the type in the abstract class, an error is thrown if the Voter use the new signature. It is for this reason that I chose to do a test before calling the methodsupports().

if we remove the type in the abstract class

We cannot do this and nobody suggested to do this.

Yet, the type can be removed by userland code when implementing the voter. That is a valid thing to do in php and that is all that my second example shows. And since the type declaration wasn't there in Symfony 2/3/4, there are probably quite a few voter implementations out there that do not have the type declaration.

Expecting an object (or anything but a string) at this point might not be an execution path that was intended, but it's one that currently works and that will be broken by the change you're suggesting. But I don't really know how to deal with it. It might even be okay to break it.

Yes, I was looking for a solution when we delete the type. However, I don't see how to check whether the typing is present or not on the instantiated class.

The problem arises when we want to call the methodisGranted() with an attribute which is an instance of a non-stringable object, and it passes in all voters including voters extending the abstract classVoter.

However, I don't see how to check whether the typing is present or not on the instantiated class.

You you catch and parse theTypeError though. But we probably want to avoid that kind of dark magic if we can. 😎

It's not a problem to add the méthode__toString() on the class that is used for theattribute argument when it is own code, but it's a problem with third-party libraries. Givent that the methodisGranted() accepts attributes with a typemixed, we cannot just say that it is enough to break compatibility for the Symfony 5.x, alas.

You you catch and parse the TypeError though. But we probably want to avoid that kind of dark magic if we can. 😎

Totally agree with you, but I don't see any other more elegant solution.

I just modified the PR to use a try/catch for the TypeError, and added tests for the attributes of typeobject stringable andinteger (so that removes the type of the$attribute argument in the final class).

Apart fromExpressionVoter, are there other bundles/projects using non-string attributes? I recently had a (private) talk aboutExpressionVoter and I'm convinced it's not necessarily correct, so we can/should change it. If there are no good use-cases for non-string attributes, I think it's good to consider deprecating non-string attributes.

@wouterj how would you implement theExpressionVoter without allowing non-string attributes ?

I'm all in favor of restricting what we can use as attributes but I'm not sure it's a reasonable move.

Another thing to take into account is that PHP will probably add support for enums at some points and people will want to use enums as attributes.

Let's not take PHP enums into consideration. First of all, it'll take a while before it will be added (it's already discussed for many years). Secondly, as everyone is now migrating to strict typed code bases, they will need to provide compatibility with current day strict typing for enums (e.g. an enum of strings should be able to be passed to astring type check).

AboutExpressionVoter: I don't think it should be a voter. The only use-case is inaccess_control. In other cases, it's much better to just write plain PHP logic instead of using the expression language. Anyways, let's not hijack this ticket with proposed changes in something else - I'll create a PR once I have some time to work on it.

Another thing to take into account is that PHP will probably add support for enums at some points and people will want to use enums as attributes.

That's a change we can hardly anticipate right now, can we? There are however libraries likemyclabs/php-enum that emulate enums in php. They should work already because the enum instances are stringable.

@wouterj. I aggree with you, but for the moment, it is necessary to fix the bug, and depreciate or not for version 5.2 and prohibit the use of a non-string attribute in version 6.0, if we decide to take this direction.

Personally I have a use case which led me to do it like votingExpressionVoter when I used a string attribute before.

Example 1:

In my case, I use several voters including one which allows to validate permissions according to the subject. This subject can very well be a string (name of the class), an instance of a class, or an instance of aFieldVote class containing the name of the field and the instance or name of the class. In any case, we do have an authorization attribute and a subject.

In attribute, I used a permission name in the same style as theRoleVoter, namely, the role name in uppercase prefixed withROLE_. This scenario is "natural" because the name of the role has always been formed with theROLE_ prefix. However, for myPermissionVoter, I must indicate the name of the permission as an attribute. This gave for theupdate andedit permissions:

// Check the authorization on the object$authorizationChecker->isGranted('perm_update',$entity);$authorizationChecker->isGranted('perm_update', PostInterface::class);// Check the authorization on the field of object$authorizationChecker->isGranted('perm_edit',newFieldVote($entity,'title'));$authorizationChecker->isGranted('perm_edit',newFieldVote(PostInterface::class,'title'));

As you can see, it is not "natural" for a user to have to do magic by prefixing the name of the permission with:permission_,perm_,permission:,perm:,PERMISSION_,PERM_,PERMISSION:,PERM:,permission/,permission@, etc... or any other symbol to separate.

Of course, I could use an array of the style['permission', 'update'] or any other style, but you see the problem, it's easy to make a mistake. Also, using simply the permission name as an attribute can behave unexpectedly with other Voters.

Using the example below is much more meaningful and logical, while being certain of the expected behavior:

// Check the authorization on the object$authorizationChecker->isGranted(newPermissionVote('update'),$entity);$authorizationChecker->isGranted(newPermissionVote('update'), PostInterface::class);// Check the authorization on the field of object$authorizationChecker->isGranted(newPermissionVote('edit'),newFieldVote($entity,'title'));$authorizationChecker->isGranted(newPermissionVote('edit'),newFieldVote(PostInterface::class,'title'));

You can see the old version of the component using the prefixperm_ as attribute inthis bundle (the new version is not yet open source).

The fact of define the permission in the attribute and leaving the subject to the$subject attribute that we want to validate, goes according to the logic initially planned by the Security component. However, we could have put an attribute of typestring like for examplepermission and created a configurable object to include the name of the permission and the subject. But you find that it becomes an unintuitive hack by doing in this way, while not making it easier to use.

Of course, it is possible to make the classPermissionVote stringable, which I just did to avoid the error with the current version of Symfony, but this is a workaround compared to the current documentation of the methodAuthorizationChecker::isGranted().

Example 2:

There is also another reason to use something other than an attribute string. This time, it's with a component validating Oauth2 scopes. The Voter waits in attribute for an instance of the classScopeVoter which can be configurable, and no subject, because subject, is always the authenticated user.

The classScopeVote can have a string or a list of string in its constructor which are the required Oauth2 scopes. It also has an option to validate if at least one scope is in the list or if all scopes are required for the subject tested.

You will therefore understand that there is still a possibility of unexpected behavior if we put in attribute, only the name of the scope without prefix likescope_, and it would also not be logical to put the "configuration" of the check with the subject tested in the$subject argument, that would be contrary to the original logic.

// Check if the current user is authorized to read a Post with a his Oauth2 scopes$authorizationChecker->isGranted(newScopeVote(['post','post.readonly'],false));// Check if the current user is authorized to manage a Post with a his Oauth2 scopes$authorizationChecker->isGranted(newScopeVote('post'));

Note:

• post scope is to manage the Post entity (read, create, update, delete, etc)
• post.readonly scope is to read only Post
• The second argument of theScopeVote class is to check if all the scopes listed are required, by default, this value istrue

Another problem if we use a string instead of an instance of an object, it is likely easier that another Voter can validate the attribute and the subject when it was not intended for him. With an object, we can be certain that we reach the correct Voter which checks if the instance of the attribute is compatible, while making the code more readable and understandable. And as I said, put apermission string in attribute (orscope string in attribute), and in subject, an instance for the configuration and the subject does not correspond to the initial logic of the component. And using an array for the attribute will make the code more difficult to understand, in contrast to the class.

For example, it is more complicated to have:

$authorizationChecker->isGranted('permission',newPermissionVote('edit',newFieldVote(PostInterface::class,'title')));$authorizationChecker->isGranted('scope',newScopeVote('post'));

that:

$authorizationChecker->isGranted(newPermissionVote('edit'),newFieldVote(PostInterface::class,'title'));$authorizationChecker->isGranted(newScopeVote('post'));

Regarding strict typing, I agree with you, and it is also for this reason that I used classes as an attribute, knowing that the documentation of Symfony authorized it. Of course, we can very well decide to deprecate the use of non-string attribute, but I think it would be more appropriate to allow at least stringable objects.

This comment is very long, and I hope I have been understandable enough with my English and the wish to be as brief as possible, but it is ultimately quite difficult :-).

@wouterj. I aggree with you, but for the moment, it is necessary to fix the bug, and depreciate or not for version 5.2 and prohibit the use of a non-string attribute in version 6.0, if we decide to take this direction.

Yes, agreed. But I think the bug can be fixed a bit more "hacky" if we know we can clean it up in 6.0 :)

Thanks for your interesting insights! I need some time to process all the information - I'll follow-up with an RFC issue. Let's finish this bug report - as I agree that a bug has been introduced.

Thank you@francoispluchino.

This is very confusing, and still NOT fixed in Symfony 6.0 - the type there is STILL string, butisGranted still acceptsmixed