graphqlite
graphqlite copied to clipboard
thecodingmachine
@Logged vs @NotLogged
I'd like to suggest an additional NotLogged annotation that, instead, makes the assumption that all operations are authenticated operations, unless the NotLogged annotation has been added.
As it is now, security is set such that, any missing Logged annotation, could expose the API. I think this is dangerous, and would much prefer the option of taking the alternate approach to security.
To further expand on this. A simple BC solution would be to add a new method to the AuthenticationServiceInterface. Something like function getAuthenticationPolicy(): AuthenticationPolicy. The AuthenticationPolicy class can define defaults for the auth policy.
Additionally, a similar concept could be applied to the AuthorizationServiceInterface where this would be useful for settings like HideIfUnauthorized. If you have a default authorization policy, you could define that, if not logged, always hide when unauthorized.
These default policies can also support future settings.
FWIW we've put our auth layer higher in the stack and forgone the need for this within GraphQLite. I still think that it's a better assumption that operations require authentication by default. I realize that'd be a BC break. I guess a setting could be added :/
I do understand your idea on this. I'm not quite completely sure how to handle this.
In particular, in the future, there might be third party packages providing GraphQL controllers. I'm not sure whether those third party controllers should be part or not of those default settings.
Maybe we could have a AuthenticationPolicy by namespace?
An alternative might be to be able to put @Logged annotations in the class docblock (rather than in the methods docblock) as proposed in #138.
While not as safe as your proposal, it could be a step forward to increased security.
Anyway, this requires a bit of work. I'll focus on releasing v4.0 first, and I'll come back to this topic when 4.0 is released.
@moufmouf yea, as I was saying, we just put a layer in front of GraphQL to handle this, it can definitely wait. Unfortunately, the solution requires us to parse the graphql request body prior to passing the request to GraphQLite, which is redundant and less performant, but necessary in our case.
Ultimately, the issue is that annotations are terribly easy to overlook and miss. And when it comes to something like security, that's problematic. I'd be all for any way to define a policy. I just find the current method to be insufficient.
Following back on this. I think the @Logged annotation should be deprecated and removed from this lib entirely. I just don't see why we need auth being handled in this lib. If it's a symfony thing, I think it should be added to the symfony bundle instead.