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

Cedar Schema Annotations #48

Merged
merged 11 commits into from
Nov 19, 2024
Merged

Cedar Schema Annotations #48

merged 11 commits into from
Nov 19, 2024

Conversation

aaronjeline
Copy link
Contributor

@aaronjeline aaronjeline commented Feb 5, 2024

Rendered

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aaronjeline aaronjeline changed the title RFC for schema annotations Schema Annotations Feb 5, 2024
text/0048-schema-annotations.md Show resolved Hide resolved
text/0048-schema-annotations.md Outdated Show resolved Hide resolved
/# Stop users from accessing a high security document unless:
/# A) The principal and user are at the same location
/# B) The principal has a job level greater than 4
forbid(principal, action, resource) when {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the main use case motivating this RFC is documentation, then I would prefer doc strings over annotations.

In general, I don't find the docs use case too compelling. Is there some other example you have in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the docs case was what was asked for by someone in the slack, and I was seeing if people preferred the more general approach

Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws Feb 8, 2024

Choose a reason for hiding this comment

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

With the custom schema syntax I do think we'll see more requests for annotations in the schema eventually. It would feel natural to use the same annotation mechanism in policies and schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason we can't do both annotations and doc comments

Copy link
Contributor

Choose a reason for hiding this comment

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

In Rust, doc comments are actually just sugar for Rust's equivalent of annotations. (Specifically, sugar for the #[doc] attribute.) We could consider doing something similar. What that means for this RFC is that we could support just doc comments now, and later if we introduce general annotations we could have doc comments desugar into a special reserved annotation. Or, we could support general annotations now, and later we could introduce doc comments as a sugar on top. We don't have to worry about closing off one or the other path.

Choose a reason for hiding this comment

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

For a doc comment syntax, I think //# is preferable to /# since backwards compatible with valid line comments, and similar to other syntaxes for overloading comments. An example is //! , the esbuild legal comment syntax to preserve in output files .

//! Copyright Cedar Contributors
//! SPDX-License-Identifier: Apache-2.0

text/0048-schema-annotations.md Outdated Show resolved Hide resolved
text/0048-schema-annotations.md Show resolved Hide resolved
@nakedible-p
Copy link

nakedible-p commented Feb 11, 2024

Comments below. All my opinions only! I don't want to say IMHO in every step, so I'm just writing them in assertive form.

  • Schema is 50% validation, 50% documentation. Simply allowing a single line @doc("This is what this is for") is woefully inadequate to what is actually needed. If we look at GraphQL, OpenAPI, JSON Schema, Rustdoc, etc. then a documentation is usually of the form (regardless of actual syntax):
# Main explanation of the item

More descriptive explanation of the item, with much more detail in a multiline and multi paragraph format.

## Examples

How policies using this usually look.

## Warnings

Here's what to keep in mind when using the item.
  • All items need documentation. Your syntax is meant to be compact in defining a set of actions so you don't need to repeat groups and context and attributes on every action, but you still need documentation for each item. Each attribute needs documentation. Each context item needs documentation.
  • Think about formatting as well. Schema shouldn't depend on whitespace and you try to make it look nice, but do think how it will actually look when fully formatted out. Right now, you don't allow nearly enough extra commas, grouping parenthesis, etc. to make it natural to write something that isn't just a single line. For example, this is from a WIP schema using comments to document that I've tried to format as convenient and clear as possible:
  // Bootstrap actions that are allowed for both authenticated and enrolling terminals.
  action
    // Can establish an RPC connection
    //
    // This action merely gives permission to establish an RPC connection, but all
    // further access is checked separately.
    ptRpcConnect,

    // Can ask for new authentication tokens
    //
    // Enrolling terminals use this to ask for proper authentication tokens. Access
    // control for this is limited to a shared secret, where as the granted tokens
    // allow full access.
    ptTokenGrant
  in [
    ptBootstrapActions
  ] appliesTo {
    principal: [PaymentApplication, BootstrapApplication]
  } attributes {
    // Terminal hardware this application is running on
    hardware: Hardware,
  };
  • Consider if it's better to have the documentation precede the item or to be after the item in question. Both approaches are seen. Python docstrings are after, as are OpenAPI and JSON Schema description fields, where as GraphQL, Rustdoc, etc. have documentation preceding.
  • Whatever way is chosen, assume tools need to be able to generate HTML documentation from the schema, so strict syntax and programmatic way to parse that is crucial. Also consider that there is need for a top-level documentation - perhaps top level for a schema file, or just top-level for a certain namespace.

Perhaps a good example on how Cedar schemas with documentation should look like is to look at the GraphQL schema of GitHub: https://docs.github.com/public/schema.docs.graphql

And an example of what kind of documentation generator should exist for Cedar schemas is: https://graphdoc.io/

@max2me
Copy link

max2me commented Feb 13, 2024

@aaronjeline, do you have an example of annotations in JSON format of schema?

@Swolebrain
Copy link

@nakedible-p
What would you say to the idea of adding cedar functionality to graphql/openAPI doc formats as opposed to re-creating what they have? This is all high-level in my mind but i imagine a world where im defining a graphql schema and i can specify something like:


query GetBooks @cedarAction('GetBooksWithAuthor', resource=Book) {
  books {
    title
    author {
      name
    }
  }
}

That way, as I define my queries and mutations i can define the actions and resources that those queries map to.

@nakedible-p
Copy link

You already have this "embed authentication in graphql schema" thing with AWS Amplify: https://docs.amplify.aws/javascript/build-a-backend/graphqlapi/customize-authorization-rules/ It's not Cedar, but a very similar idea. Probably one day it can be Cedar as well.

@nakedible-p
Copy link

Here is a basic JSON schema with descriptions: https://json-schema.org/learn/miscellaneous-examples#basic

@max2me
Copy link

max2me commented Feb 26, 2024

Am I reading it correctly that this proposal would not support annotation individual attributes?

@khieta khieta added the pending This RFC is pending; for definitions see the README label Feb 27, 2024
Copy link
Contributor

@mwhicks1 mwhicks1 left a comment

Choose a reason for hiding this comment

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

I'd like to see this fleshed out to consider more use-cases. If doc strings are the only use-case, then a comment-based approach seems best to me. But we surely have other use-cases in mind, too, but we need to see them to evaluate possible designs.

text/0048-schema-annotations.md Outdated Show resolved Hide resolved
Comment on lines 21 to 23
1. Doc comments
```
@doc("This entity defines our central user type")
Copy link
Contributor

Choose a reason for hiding this comment

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

You have a number here as if this is the first of several examples, but it's the only one.

For doc comments, it makes more sense to me to define an opinionated literate format. This is what Rust, Java, etc. all do, and it's nicer to read and perhaps more natural to write (per your Alternative "Doc strings as comments"). But we could still also have arbitrary annotations to handle other use-cases (which it would be nice to see, here).

text/0048-schema-annotations.md Show resolved Hide resolved
@max2me
Copy link

max2me commented Feb 28, 2024

Can we please consider supporting annotation on individual attributes as well? This would allow integrating tools and services to embed data that helps authoring policies easier.

For example:

@doc("This entity defines our central user type")
entity User { 
    manager : User,

    @doc("Which team user belongs to")    
    @docLink("https://schemaDocs.example.com/User/team")
    team : String
};

@khieta
Copy link
Contributor

khieta commented May 16, 2024

I think #63, which proposes to support doc comments in schemas and policies, gives a better solution for the @doc(...) case -- do we still want schema annotations even if we already have schema doc comments?

@emina
Copy link
Contributor

emina commented May 28, 2024

I'd like to see this fleshed out to consider more use-cases. If doc strings are the only use-case, then a comment-based approach seems best to me. But we surely have other use-cases in mind, too, but we need to see them to evaluate possible designs.

One (real customer) use case is to share the same schema among several organizations, while allowing UI builders to display only parts of the schema that are relevant to a specific organization. For example, entity types A and B should show up in UI builders only for members of the organization X. And the UI builder extracts this association from the annotation @VisibleTo("X") on entity types A and B.

@nakedible-p
Copy link

Anothere example from AWS Amplify land on GraphQL:

type Customer @model @auth(rules: [{ allow: public }]) {
  id: ID!
  name: String! @index(name: "byNameAndPhoneNumber", sortKeyFields: ["phoneNumber"], queryField: "customerByNameAndPhone")
  phoneNumber: String
  accountRepresentativeID: ID! @index
}

Even though such things are not directly applicable, it doesn't take much imagination to figure out similar things applied to a Cedar schema.

Signed-off-by: Shaobo He <[email protected]>
Thus the full schema syntax becomes:
```
Schema := {Namespace}
Namespace := ('namespace' Path '{' {Decl} '}') | {Decl}
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on some recent discussion, we might want to support annotations on the namespace. I don't see a reason not to.

@shaobo-he-aws
Copy link
Contributor

shaobo-he-aws commented Nov 11, 2024

Can we please consider supporting annotation on individual attributes as well? This would allow integrating tools and services to embed data that helps authoring policies easier.

For example:

@doc("This entity defines our central user type")
entity User { 
    manager : User,

    @doc("Which team user belongs to")    
    @docLink("https://schemaDocs.example.com/User/team")
    team : String
};

Sure, we can. Just to double check. Are we looking into supporting entity attributes only? Note that attributes can also be found in context declarations and common type declarations. From the perspective of an implementation, it's better that we allow annotations where a record can legally show up.

Signed-off-by: Shaobo He <[email protected]>
@shaobo-he-aws shaobo-he-aws changed the title Schema Annotations Cedar Schema Annotations Nov 11, 2024
@max2me
Copy link

max2me commented Nov 12, 2024

I support this proposal as it will streamline the development of improved policy authoring tools while avoiding the need to separate configurations for schema meta data.

Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws left a comment

Choose a reason for hiding this comment

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

Looks good. I support this extension

text/0048-schema-annotations.md Show resolved Hide resolved
None of the three top-level constructs (EntityTypes, Actions, CommonTypes) in JSON schemas allow for arbitrary key/value pairs.
This means a new key can be safely added while preserving backwards compatibility.
The same fact also applies to entity attribute declarations.
This proposal reserves the `annotations` key at the top level of each of those constructs, which contains an Object, containing each annotation key as an Object key, associated with the annotation string.
Copy link
Contributor

@andrewmwells-amazon andrewmwells-amazon Nov 19, 2024

Choose a reason for hiding this comment

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

Maybe we could use __annotations if this is a concern since it's more obviously special.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can also use reserved __cedar::schema::annotations to be 100% secure.

text/0048-schema-annotations.md Outdated Show resolved Hide resolved
@shaobo-he-aws shaobo-he-aws merged commit 54f37b9 into main Nov 19, 2024
1 check failed
@shaobo-he-aws shaobo-he-aws deleted the schema-annotations branch November 19, 2024 20:49
@shaobo-he-aws shaobo-he-aws added final-comment-period This RFC is in its final comment period; for definitions see the README and removed pending This RFC is pending; for definitions see the README labels Nov 19, 2024
@shaobo-he-aws shaobo-he-aws restored the schema-annotations branch November 19, 2024 21:15
Copy link
Contributor

@emina emina left a comment

Choose a reason for hiding this comment

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

I support this proposal.

Formally the following rule is added to the Cedar grammar:
```
Annotation := '@' IDENT '(' STR ')'
Annotations := {Annotations}

Choose a reason for hiding this comment

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

Should this be

Annotations := {Annotation}

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. Will update it.

@shaobo-he-aws shaobo-he-aws added active This RFC is active; for definitions see the README and removed final-comment-period This RFC is in its final comment period; for definitions see the README labels Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
active This RFC is active; for definitions see the README
Projects
None yet
Development

Successfully merging this pull request may close these issues.