Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
-
It can be nice to have the full support of Enums for security voters and maybe roles too. For now, in most places
and here
|
BetaWas this translation helpful?Give feedback.
All reactions
Replies: 6 comments 19 replies
-
Well, enums are meant to represent aclosed list of values. In the security system, the list of permission attributes is unknown. It is an open list. So enums are not suited for that. |
BetaWas this translation helpful?Give feedback.
All reactions
-
Yep, I got your thought, but what about inconsistency in parameters definition? I mean in some places |
BetaWas this translation helpful?Give feedback.
All reactions
-
imho - roles - is closed lists. |
BetaWas this translation helpful?Give feedback.
All reactions
👍 5
-
It's rarely the case when building application with a complex or dynamic role system |
BetaWas this translation helpful?Give feedback.
All reactions
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
-
With match expressions now supporting exhaustive match hinting in some IDEs. And bits of code like this common in custom voters.
Attributes allowing the support of enum values. Supporting enums in voters is clear improvement. |
BetaWas this translation helpful?Give feedback.
All reactions
👍 4
-
@tarlepp in your specific project, it likely is a closed list. But in |
BetaWas this translation helpful?Give feedback.
All reactions
-
I am using Roles as Enums, because it's finite list of values. But I find it very strange that up until But when getting voter, it suddenly starts to work with only strings. At least accept Now I have to do |
BetaWas this translation helpful?Give feedback.
All reactions
-
Accepting stringable won't help you, as enums cannot be stringable. |
BetaWas this translation helpful?Give feedback.
All reactions
-
I would like to get some clarification on this issue. I would argue that most real life applications out there will have any form of constant holder for Roles / Permissions / similar tokens. When taking into considerations that database also holds values like this, then having them stored as an enum makes a hell of a lot of sense. I see absolutely no harm in supporting backedEnums for the isGranted functionality. NOT doing so, honestly just promotes userland code errors. Why the hard stance against this? |
BetaWas this translation helpful?Give feedback.
All reactions
-
Enums provide a benefit to avoid errors when you can use them as the type of parameters to restrict things. But the AuthorizationCheckerInterface cannot use such application-specific enum as its parameter type, as it cannot depend on it. PHP enums are representing a type with aclosed list of values. But as far as |
BetaWas this translation helpful?Give feedback.
All reactions
-
@stof Christophe, sorry, but I still don't understand your point. I like the DRY principle (as many do, I bet). So in my app I have a set of permissions like that: enum ProjectPermission:string{case View ='view';case MarkAsDone ='markAsDone';} To me it's a closed set of options, if I need more, I'll add, but it's pertinent to one entity and it's not open. protectedfunctionsupports(string$attribute,mixed$subject):bool {returnin_array($attribute, ProjectPermission::cases(),true) &&$subjectinstanceof Project; } And, returning to the DRY principle, in my controller I could do: #[Route('/{slug}', name:'project_view', methods: ['GET'])]publicfunctionview(string$slug,Request$request,ProjectRepository$projectRepository):Response {$project =$projectRepository->findBySlug($slug);if (!$project || !$this->isGranted(ProjectPermission::View,$project)) {throw$this->createNotFoundException('This project does not exist'); }return$this->render('project/view.html.twig', ['project' =>$project, ]); } I think it's beautiful and clean. However, without enums I need to use constants, then do an array of all the constants for calling |
BetaWas this translation helpful?Give feedback.
All reactions
👍 4
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
-
What about usingBackedEnum::tryFrom #[IsGranted(attribute: Permission::SOMETHING->value)] protectedfunctionsupports(string$attribute,mixed$subject):bool{$user =$this->security->getUser();if (!$userinstanceof UserInterface) {returnfalse; }return Permission::tryFrom($attribute)instanceof Permission;} |
BetaWas this translation helpful?Give feedback.
All reactions
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
-
I am by no means a reference is anything symfony-related but here are my two cents on this topic nonetheless. I honestly don't see how allowing a I.e, given a Role enum defined as the following :
passing However I don't think forcing people to go through hoops by using an imposed string format with And forcing people to use constants instead of enums would definitely go against the exact argument used earlier : roles are supposed to be a closed list. Also, simply allowing All it would take is one line of code to automatically retrieve the value property if the passed element is a StringBackedEnum... |
BetaWas this translation helpful?Give feedback.
All reactions
👍 4
-
|
BetaWas this translation helpful?Give feedback.
All reactions
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
-
Is this implemented now?
in Coding principles shouldnt undermine the dev experience. |
BetaWas this translation helpful?Give feedback.
All reactions
-
That's kind of the issue though. For your domain to use your types consistently, the framework has to support your domain's type somehow. The framework can't support an
It's always safer to start final and then open something up versus defaulting to a "everything has to be open" policy, especially with a platform as widely used as Symfony. A fair number of attribute classes started final and were then opened through later PRs for very similar reasons.
While this is true in theory, it would also force B/C breaking changes on everyone using the |
BetaWas this translation helpful?Give feedback.
All reactions
-
Making voters generate the cache key does not work: it would require knowing which voter to ask for the cache key before being able to read the cache to know which voter supports that attribute, which is a chicken-and-egg problem. |
BetaWas this translation helpful?Give feedback.
All reactions
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
-
No it doesn't; like, ideally it would, but having easy support for it in the application code is much more important. I.e. having correct, strict typing for methods like One option to make this way more versatile while also allowing implementation flexibility would be to completely ignore Enums, and instead allow a new Interface to be accepted by the annotation, voters, etc. For example, we could have a RoleInterface, that would have a single method returning string (called |
BetaWas this translation helpful?Give feedback.
All reactions
-
@Amunak you can already use whatever representation you want in your own code (strings, enum, a doctrine relation, etc...) for the roles, as long as you handle converting them to the string type expected by |
BetaWas this translation helpful?Give feedback.
All reactions
-
@stof but that still doesn't allow you to also just use the app's implementation for the security attributes and such. This would be a nice way to unify it all, in my opinion. |
BetaWas this translation helpful?Give feedback.