Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup and deprecate confusing annotations #583

Open
oojacoboo opened this issue Mar 19, 2023 · 4 comments
Open

Cleanup and deprecate confusing annotations #583

oojacoboo opened this issue Mar 19, 2023 · 4 comments

Comments

@oojacoboo
Copy link
Collaborator

oojacoboo commented Mar 19, 2023

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
Copy link
Contributor

oprypkhantc commented Mar 22, 2023

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
Copy link
Contributor

It works the same in symfony

@oojacoboo
Copy link
Collaborator Author

@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 to Logged and Right.

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 the hide argument.
  • if #[Right] is used with hide, 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 the SchemaFactory to default everything to hidden when not authenticated that isn't attributed with #[Public].

@oprypkhantc
Copy link
Contributor

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 with graphql-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 hide argument 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants