-
Notifications
You must be signed in to change notification settings - Fork 12
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
Introduce query engine and schema generation #55
Introduce query engine and schema generation #55
Conversation
The SpiceDB schema for permissions-api was previously hardcoded in text. This commit adds a template for the SpiceDB schema as well as data types for defining resources and actions. An instance of the Schema struct is currently hardcoded in the "schema" command to match existing output. Signed-off-by: John Schaeffer <jschaeffer@equinix.com>
Signed-off-by: John Schaeffer <jschaeffer@equinix.com>
Signed-off-by: John Schaeffer <jschaeffer@equinix.com>
3c286bd
to
6c61524
Compare
} | ||
{{end}} | ||
`)) | ||
) |
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.
Should we already refer to the prefix as a namespace? This way it's easier to read and interpret that it's the intention.
Optional: true, | ||
Name: "tenant", | ||
Type: "tenant", | ||
Optional: 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.
Given that the subject resource is being deleted from here, do you have a notion of how we'll register subjects? Would we auto-create them as role assignments happen?
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 got me thinking - RESTful management of resources may not necessarily be what we want here. Resources in SpiceDB are defined only in terms of their relationships, so it's not possible to create a resource that doesn't have relationships.
It may be better to rework this so that the resource API doesn't manage resources, but rather manages roles and relationships.
Signed-off-by: John Schaeffer <jschaeffer@equinix.com>
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.
Makes sense so far to me. I also started suggesting fixes for things for you that the linter will complain about but didn't do all of them.
|
||
func init() { | ||
rootCmd.AddCommand(schemaCmd) | ||
|
||
schemaCmd.Flags().BoolVar(&dryRun, "dry-run", false, "dry run: print the schema instead of applying it") |
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.
❤️
schemaCmd = &cobra.Command{ | ||
Use: "schema", | ||
Short: "write the schema into SpiceDB", | ||
Run: func(cmd *cobra.Command, args []string) { |
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.
total nit but I prefer RunE
over a bunch of logger.Fatal
- not sure how you feel about that (yes, i know you didn't really change 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 think it's a good suggestion for a subsequent PR. Can you open a GH issue? Would be a nice low hanging fruit.
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.
Done! See #57.
} | ||
} | ||
|
||
func (e *Engine) SubjectHasPermission(ctx context.Context, subject types.Resource, action string, resource types.Resource, queryToken string) error { |
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 signature be nicer as this IMO:
func (e *Engine) SubjectHasPermission(ctx context.Context, subject, resource types.Resource, action string, queryToken string) error
Co-authored-by: E Camden Fisher <fish@fishnix.net> Signed-off-by: John Schaeffer <john@schaeffer.io>
One of the challenges of permissions management in permissions-api is that we will (likely) need to persist data in two places: SpiceDB for permissions management, and CRDB for other things like update times, role names, and so on. To make this easier in the long run, this PR introduces a few things:
query.Engine
: An engine for making authorization queries. The package name might change in the future, but since this is where the authorization logic lives today I figured I would keep itspicedbx.GenerateSchema
: A function for generating a SpiceDB schema based on resource type definitionsAPI handlers have also been updated to explicitly create and assign roles rather than using the
query
package to do so. This logic needs some refining, however, which will happen as this PR matures.This PR does not currently have the following:
query.Engine
testsspicedbx.GenerateSchema
tests