Repository navigation
Cleanup and deprecate confusing annotations #583
Description
Activity
We do use
#[Logged]to require authentication in controller methods. The naming is confusing though: I've never heard of anyone using the term "logged", neither for users nor in code. It's always been either "Log in" or "Sign in" on frontend and "Authenticated" in code.On the
#[HideIfUnauthorized]I was wondering myself why it's not just a parameter for#[Logged]and#[Right], but there's one benefit of having a separate annotation: it could be allowed to be declared class-level, i.e. globally for all of the fields. This might make sense in some places, although we're not using#[HideIfUnauthorized]ourselves.Also, if there are no plans to allow fields to be resolver-dynamic (i.e. deciding whether to hide it on every call to ->resolve), it might make sense to just deprecate it altogether to avoid confusion.
On
#[Security], I'm not a fan of the Symfony's expression language. It looks like a hack caused by limitations of annotations & attributes - i.e. being unable to pass a closure in an attribute's constructor. I'd rather have real methods than a different language used as a string. Plus, if we ever do have closures in attributes, real methods could be easily refactored into closures, while an expression language would have to be refactored by hand.#[Right]we don't yet use either, but it does seem pretty useful. Again, the naming is a bit off. I've never heard anyone using the term "right" for this. "Authorize" or "Permission" or "Can" all seem a lot more intuitive and are widespread. In Laravel there's a concept of policies - you can define a policy class which handles authorization for a specific entity, i.e.:class NotePolicy { public function create(User $authenticated): Response { return $this->allowWhenAdmin($authenticated) ?? $this->denyWhenOverTheLimit($authenticated) ?? Response::allow(); } public function update(User $authenticated, Note $note): Response { return $this->allowWhenAdmin($authenticated) ?? $this->denyWithoutAccess($authenticated, $note) ?? Response::allow(); } }
then you map your entity to a policy (
->policy(Note::class, NotePolicy::class)) and you're set; you can use it like so:$gate->forUser($auth)->authorize('create', Note::class); $gate->forUser($auth)->authorize('update', $note)
The same thing is easily applied on input/query fields:
#[Input] class Something { #[Field] #[Right('view')] // <-- call ->authorize('view', $this->otherEntity) public OtherEntity $otherEntity; }
So from Laravel's point of view,
#[Right]does make a lot of sense, but I'm not sure if it fits ingraphqliteitself. I'm unaware of how other frameworks handle this kind of things.Reacted by Oriano de StefaniIt works the same in symfony
@oprypkhantc I agree on the Symfony expression language. Luckily in PHP 8.5, support for closures will be there and it's already merged in.
https://wiki.php.net/rfc/closures_in_const_expr
I propose that we refactor the
#[Security]attribute for closures in conjunction with PHP 8.5 support. Also look into deprecating Symfony's expression language support.--
As for
#[Logged],#[Right], and#[HideIfUnauthorized], the real issue here is that these are dynamic attributes in terms of schema generation, preventing us from properly caching it. Assuming that this support is needed, and it may be, we may be able to cache schemas that do not use these attributes at all, offering those a performance boost.If we only hide types/fields when unauthorized, and not based on a "right", we could potentially cache 2 versions of the schema, in this case, and deliver back the appropriate one. However, with the
#[Right], there won't be any reasonable way to cache the schema.#[HideIfUnauthorized]- I say we fully deprecate this attribute for removal and add a boolean argument toLoggedandRight.As for
#[Logged]and#[Right], while the names aren't the best, I think they're good enough.So, after these changes, we'd be able to cache one or two versions of the schema.
- If
#[Logged]is used, a secondary version of the schema could be cached ("authenticated" and "notAuthenticated" versions), based on thehideargument. - if
#[Right]is used withhide, no caching would be enabled.
In addition to these changes, we could actually add another attribute,
#[Public], which would do the opposite of#[Logged](would be nice to actually have these names as true antonyms). We could then add a property to theSchemaFactoryto default everything to hidden when not authenticated that isn't attributed with#[Public].- If
Having closures in attributes is awesome. Finally can we have custom inline validation in attributes. I'm also amazed that PHP is one of the first languages (the first?) to allow this :)
On other attributes:
- I agree
#[HideIfUnauthorized]should be removed. Although it is useful, I don't think it fits well withgraphql-php/graphqlite. They're not built with schemas with dynamic fields in mind, and I don't think it's worth the effort to try to change that. Those who have a really good use case for it can build multiple different schemas, and use custom attributes to separate them - I'm not sure there are many schemas that don't use
#[Logged]or some other form of "authenticated" attribute - I don't think
hideargument is a good idea for the same reason I don't think#[HideIfUnauthorized]should exist. It requires creating a schema from scratch for each request. Although Performance problem even for small query on big schema. #569 might improve the performance of that, long running servers are the future and will still be more performant than any kind of caching, as long as the schema is shared between requests and not rebuilt for each one of them
As always, I'm advocating for something like FrankenPHP :) It's pretty trivial to setup and gains you so much performance.
- I agree
The following annotations are confusing and have a lot of overlap in their documented functionality.
#[Logged]#[Right]#[HideIfUnauthorized]Personally, we don't use any of them and I find the
#[Security]annotation to be more than appropriate, especially with the expression language extensibility. Nonetheless, we're left with these others in the library. This ticket is to open a discussion into the current use of these annotations and potentially clean them up and deprecate where possible.Further,
#[HideIfUnauthorized], is dynamic in nature, causing the inability to cache types/schema for performance purposes. See #569 for more discussion on this topic.As I understand it,
#[HideIfUnauthorized]is required to hide the field, regardless of#[Logged]or#[Right]being declared. In this case, I don't see why#[Logged]and#[Right]cannot implement an argument that determines if the field is hidden or not:#[Logged(hide: true)]. TBH, I don't even see the purpose of#[Logged]or#[Right], if the field isn't being hidden. So, why aren't these hiding by default and what are they doing when declared alone?Additionally, can
#[Logged]and#[Right]be merged together, and/or replaced by#[Security]?All of these annotations are terribly confusing and the design is pretty awful IMO. I'd like to hear from others in how you're using them, suggestions for improvements, but with the overall goal of cleaning these up and deprecating them as much as possible.