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

Introduce query engine and schema generation #55

Merged
merged 5 commits into from
Mar 29, 2023

Conversation

jnschaeffer
Copy link
Contributor

@jnschaeffer jnschaeffer commented Mar 28, 2023

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:

  • Namespace: A logical grouping of resource types. All resources in a namespace can refer to each other, but resources between namespaces cannot. Mostly intended for easier testing in SpiceDB
  • 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 it
  • spicedbx.GenerateSchema: A function for generating a SpiceDB schema based on resource type definitions

API 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 tests
  • spicedbx.GenerateSchema tests

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 <[email protected]>
@jnschaeffer jnschaeffer marked this pull request as ready for review March 28, 2023 16:32
}
{{end}}
`))
)
Copy link
Contributor

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

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?

Copy link
Contributor Author

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.

JAORMX
JAORMX previously approved these changes Mar 29, 2023
fishnix
fishnix previously approved these changes Mar 29, 2023
Copy link
Contributor

@fishnix fishnix left a 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")
Copy link
Contributor

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

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 😆 )

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! See #57.

internal/query/roles.go Outdated Show resolved Hide resolved
internal/query/service.go Show resolved Hide resolved
}
}

func (e *Engine) SubjectHasPermission(ctx context.Context, subject types.Resource, action string, resource types.Resource, queryToken string) error {
Copy link
Contributor

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

internal/query/tenants.go Show resolved Hide resolved
internal/query/tenants.go Show resolved Hide resolved
internal/query/tenants.go Show resolved Hide resolved
@jnschaeffer jnschaeffer dismissed stale reviews from fishnix and JAORMX via 44c32f0 March 29, 2023 18:05
Co-authored-by: E Camden Fisher <[email protected]>
Signed-off-by: John Schaeffer <[email protected]>
@jnschaeffer jnschaeffer merged commit 10fa28c into infratographer:main Mar 29, 2023
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

Successfully merging this pull request may close these issues.

3 participants