Cleanup and deprecate confusing annotations

[oojacoboo]

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.

[oprypkhantc]

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 in graphqlite itself. I'm unaware of how other frameworks handle this kind of things.

[Lappihuan]

It works the same in symfony