-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[RFC][v3] Authorization Rules in V3 #10237
base: master
Are you sure you want to change the base?
Conversation
```yaml | ||
kind: TypePermissions | ||
version: v2 | ||
definition: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do rules like these mean for the generated GraphQL schema?
Will internal_order_notes
always be visible, but we'll make it nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(or would accessing a field that one is not allowed trigger an error instead of a null
field?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal_order_notes
will not be visible in the GraphQL schema if the x-hasura-role
session variable is not set to admin
in the introspection query.
(Note that there is a separate conversation going on around eliminating dynamic role based schemas altogether but for the purpose of this RFC, let's assume we still want the GraphQL schema to contain only what's accessible)
|
||
### Implementation | ||
|
||
The above proposal has major implications for engine, the `lang-graphql` crate in particular. Currently, `lang-graphql` allows constructing a single graphql schema with information about all roles embedded into the schema and allows query validation / IR generation to happen in the context of a particular role only. However, with the above proposal, there is no notion of roles anymore. So, `lang-graphql` will need to move to an architecture where when trying to access a graphql field, instead of looking up an allowed set of roles, it can evaluate an arbitrary expression that evaluates to a boolean. The details of this architecture are out of scope for this RFC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This presumably means losing the ability to generate a GraphQL schema (and thus generate types etc) for only the queries / types that a given role is allowed to access. This feels like quite a price to pay, are we satisfied that users don't need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we would still generate the GraphQL schema only according to what's accessible to a given set of session variables.
When an introspection query comes in, we would evaluate which fields/types are accessible based on the session variables associated with the request and generate a schema containing only those types/fields.
version: v1 | ||
definition: | ||
typeName: User | ||
roles: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this isn't the root concern of this RFC, but surely there are other ways to make such a permission less arduous to define, for instance?
permissions:
- roles:
- developer_base
- developer_with_gov_access
- support_agent_base
- support_agent_with_gov_access
output:
allowedFields: [id]
- roles:
- developer_with_pii_access
- developer_with_pii_access_and_gov_access
- support_agent_with_pii_access
- support_agent_with_pii_access_and_gov_access
output:
allowedFields: [id, name, email]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, for type permissions itself, it's not as big a deal, but you would still need to model 8 different roles in your authn/authz system. And now imagine, another attribute like 'hippa_access' gets added, now I would have 16 roles to manage.
So, fundamentally the issue is that it's not always feasible to enumerate all possible authz states that can exist in my system since that can grow exponentially with each independent attribute in my system.
fieldName: is_gov | ||
operator: _eq | ||
value: literal: true | ||
- role: support_agent_with_gov_access # Same for support_agent_with_pii_access_and_gov_access |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're concerned about the usability of metadata, then it doesn't seem wild that eventually we're going to want to abstract these predicates too and refer to them by name.
.....
- role: support_agent_with_gov_access # Same for support_agent_with_pii_access_and_gov_access
select:
filter:
relationship:
name: tickets
predicate: agent_predicate
---
kind: Predicate
version: v1
definition:
name: agent_predicate
fieldComparison:
field: agent_id
operator: _eq
value: sessionVariable: x-hasura-agent-id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a world where my rules are composable, the ability to reuse predicates isn't that important.
The fundamental problem that is being addressed is not the ability to reuse parts of my metadata, but not having to enumerate all possible authorization states in my system.
# Rule 2: Allow name and email if pii access is allowed | ||
- allowFields: [name, email] | ||
condition: | ||
equals: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we considered the somewhat opposite design to this, where we use a role
somewhat like a JWT claim.
For example, instead of inhabiting a single role, on a given request a user might have both the has-pii
role AND the can-delete
role.
Then instead of inlining conditions, we could have a top-level piece of metadata that works out which roles are applicable for a request using predicate conditions like this?
kind: RolePredicate
version: v1
definition:
name: has-pii
condition:
equals:
left: sessionVariable: x-hasura-has-pii-access
right: literal: true
Feels like if repetition is a concern, this might be neater, and would allow us to buy back a few more static guarantees (ie, if a request has roles X and Y, the schema will be like this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sketched out what this might look like here: https://gist.github.com/danieljharvey/2718cf1a8fc340a5b21063bc5e843b36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can still have static guarantees with the current design, i.e., if a request has session variables x-hasura-has-pii-access: true
and x-hasura-role: developer
, then the schema will be like this.
Also, there is a subtle difference between role predicates and independent rules w.r.t how type permissions and model permissions interact.
Eg: With independent rules:
- allow row 1 of model X, if condition1
- allow all rows of model X, if condition 2
- allow all fields of model X, if condition1
- allow field A of model X, if condition 2
If both condition1 and condition2 are true, i can see that all rows and fields will be accessible.
But, if I used role predicates:
Roles:
- role1 if condition1
- role2 if condition2
Permissions:
- role1 can access all fields of row 1 of model X
- role2 can access field A of all rows of model X
Now if both condition1 and condition2 are true, then I would expect to have access to field A of all rows, and all fields of row 1.
This sort of cell-level authorization we explicitly don't want to support.
|
||
Note, however, that cell-level authorization will not be supported by this system, since you define type (column) permissions and model (row) permissions independently. | ||
|
||
3. **Are any changes required to the NDC protocol?** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if row and column permissions are disjoint, would you not want to push down the cell-level predicates? Or are you proposing that we redact data on the engine side.
That would be simpler but result in more network overhead.
I think it'd be worth spelling this out here.
```yaml | ||
allowObjects: | ||
relationship: | ||
name: 'my_relationship' | ||
relatedObjectAllowed: true | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this effectively autogenerates the permissions on the target model, what does it mean if one was to apply this attribute on the reverse relationship (ie the relationship from the target to the source)? Or if the target model already had permissions defined? It's unclear to me how this composes with everything else.
authorizationRules: | ||
# Rule 1: Allow users to see their own orders | ||
- allowObjects: | ||
fieldComparison: | ||
fieldName: user_id | ||
operator: _eq | ||
value: | ||
sessionVariable: x-hasura-user-id | ||
condition: | ||
equals: | ||
left: sessionVariable: x-hasura-role | ||
right: literal: user | ||
# Rule 2: Disallow anyone except admins to see hidden orders | ||
- denyObjects: | ||
fieldComparison: | ||
fieldName: is_hidden | ||
operator: _eq | ||
value: | ||
literal: true | ||
condition: | ||
not: | ||
equals: | ||
left: sessionVariable: x-hasura-role | ||
right: literal: admin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope we're planning on writing an expression evaluation system that evaluates and simplifies these expressions before pushing them down to the connectors? Otherwise this is going to get ugly and we'll be spitting very complex where clauses into databases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is that (for now at least) expressions are evaluated in the engine only, since they only depend on the values of session variables. I agree that pushing down subexpressions is not a tractable plan.
1. **Will existing metadata continue to be work?** | ||
|
||
Yes, existing metadata (using the `v1` version of permissions objects) will continue to work and trivially maps to the new system where the condition is an equality check on the `x-hasura-role` session variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider letting the user pick which way they'd prefer to express their permissions, either this advanced system, or the existing role based system (which we just desugar into the the advanced system behind the scenes). This advanced system is very powerful, but also less obvious how to use it to recreate the simple use case and I contend that users who just need basic role-based permissions would find the old mechanism easier to author and understand.
To be clear, this doesn't mean just retaining support for the v1 ModelPermissions and TypePermissions, it means including both options on v2. Just retaining the old v1 types is not as good as it implicitly discourages their use because they are an "old version".
Description
Rendered