Add ExperimentalComputablePermissions API #1894
Conversation
josephschorr
force-pushed
the
experimental-reflection-apis-part-3
branch
from
151a55e
to
c1f0fb2
Compare
github-actions
bot
added
area/api v1
There was a problem hiding this comment.
LGTM overall, the only potential blocker I see is lack of tests and mitigation for schema recursion. I'd also like to see us supporting batched calls to both ComputablePermissions and DependentRelations, as I think that could be desirable for scenarios where you want to use these APIs for denormalization purposes.
| expectedError: "relation/permission `invalid` not found", | ||
| }, | ||
| { | ||
| name: "basic", |
There was a problem hiding this comment.
While I do understand this expected response from the perspective of the reachability graph (user:...), it seems inconsistent for an API called ExperimentalComputablePermissions. I wouldn't expect this to return relations, and so it also wouldn't make sense the API has a field IsPermission.
It's not the same with ExperimentalDependentRelations because the name of the RPC explicitly states "Relations" so it fits both relations and permissions.
The options that come to mind are:
- create a type
ExPermissionReferencethat drops that field, and filter for permissions in the controller - rename the RPC to something like
ExperimentalComputableRelationsas it could return both.
There was a problem hiding this comment.
I left it named permissions because that's how people will primarily use it; we could rename it, but I felt it was more discoverable that way. Both functions return both.
There was a problem hiding this comment.
It could confuse folks, but it's not a big deal anyway, just my 2 cents
| "...", | ||
|
|
||
| []string{"document#view", "document#viewer"}, | ||
| }, |
There was a problem hiding this comment.
add test that uses subject relation with a computed userset
There was a problem hiding this comment.
We have one though?
There was a problem hiding this comment.
oh I may missed it, just to be clear, something like relation members: team#members where members is a permission
|
|
||
| []string{"document#view", "document#viewer"}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
also add test where there is recursion in the schema. Also add to the other API tests
There was a problem hiding this comment.
We have one; a group refers to its own members
|
|
||
| subjectTypesToCheck := []*core.RelationReference{startingSubjectType} | ||
|
|
||
| // TODO(jschorr): optimize this to not require walking over all types recursively. |
There was a problem hiding this comment.
We may want to add some sort of max depth to prevent infinite recursion that could happen via the schema? Perhaps adding a context deadline if this is taking too long as a precaution?
There was a problem hiding this comment.
This is a structural walk, not a a data-driven one; there should be no case where it causes infinite recursion as the check below reduces the working set over time
| // RelationsEncounteredForSubject returns all relations that are encountered when walking outward from a subject+relation. | ||
| func (rg *ReachabilityGraph) RelationsEncounteredForSubject( | ||
| ctx context.Context, | ||
| allDefinitions []*core.NamespaceDefinition, |
There was a problem hiding this comment.
nit: does this need to be "all definitions" or it could be any subset? The argument name would invite to think it has to be provided with all namespaces in the instance
| return nil, err | ||
| } | ||
|
|
||
| nrg := ReachabilityGraphFor(&ValidatedNamespaceTypeSystem{nts}) |
There was a problem hiding this comment.
nit: shouldn't TypeSystemForNamespace return a ValidatedNamespaceTypeSystem then?
There was a problem hiding this comment.
No; we know (internally here) that it is already validated but if the type system was loaded from memory, it may not have been validated yet
No description provided.