Skip to content

Add is operator #5

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

Merged
merged 11 commits into from
Jul 28, 2023
Merged

Add is operator #5

merged 11 commits into from
Jul 28, 2023

Conversation

khieta
Copy link
Contributor

@khieta khieta commented Jun 16, 2023

This RFC was migrated from cedar-policy/cedar#94

Rendered

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

(Aside: the issue of "input validation" brought up in [Use case 1](#use-case-1-input-validation) could also be addressed by performing validation against the schema for every input query, but that deserves its own RFC.)

2. Another way to simulate `is` is to add an `entity_type` attribute to entities, and check the type of an entity using `resource.entity_type == "File"` (for example).
However, this requires users to manually add an entity's type as an attribute, and leads to the possibility that an entity might be created with an attribute that doesn't actually match its type.
Copy link
Contributor

Choose a reason for hiding this comment

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

This alternative also does not allow for any short circuiting in the typechecker. Although, that short circuiting is what allow users to go against our best practices for actions, so it could be viewed as a feature of the alternative.

@mwhicks1
Copy link
Contributor

I move that we accept this RFC but with usage of is limited to conditions, and as its own expression not combined with in. This is a two-way door. We have consensus on this aspect, it presents no risk to scope-based policy indexing, and as we get more experience we can choose to allow is in policy scopes and/or combination with in as usage becomes more compelling.

@khieta
Copy link
Contributor Author

khieta commented Jun 28, 2023

Thanks for the feedback everyone! I am working on revising the initial draft -- hopefully I'll have it posted tomorrow. Here's a sneak peak of what I plan on changing:

  • do not allow is in the policy scope
  • do not allow combined syntax like .. is .. in ..
  • remove "example 1", which can be solved more naturally by Validate requests #11
  • spend some more time thinking about how is interacts with the best practice of "typed actions"

@jeffsec-aws
Copy link

jeffsec-aws commented Jun 28, 2023

One example I was working on is ability to define a policy to a specific entity like:

permit (
    principal is User, // here we want to mean any entity of type User while some Admin and Partner entity types might also exist
    action == Action::"read",
    resource == Document::"ToS"
);

I agree that this can be implemented in the condition clause instead to prevent change of other operations like listing policies. It would have the representation:

permit (
    principal,
    action == Action::"read",
    resource == Document::"ToS"
) when {
   principal  is User, // here we want to mean any entity of type User while some Admin and Partner entity types might also exist
};

While some mitigations would also solve the question at stake, is operator existence is more scalable and would prevent to

  • have to make the schema larger by creating specific purpose actions ():
        "globalRead": {
            "attributes": {},
            "appliesTo": {
                "principalTypes": [ "User"],
                "resourceTypes": [ "Document" ],
            }
        }
    
  • have to go the Group relationship way:
    permit (
        principal in Group::"All users" // This has to be maintained and passed as an entity component.... which can be large, therefore the caveat vs being able to use == User or == User::"*"
        action == Action::"read",
        resource == Document::"ToS"
    );
    

@khieta
Copy link
Contributor Author

khieta commented Jun 30, 2023

Revision (finally) posted! Based on in-person discussion, I decided to leave the proposal to include is and is ... in in the scope unchanged, but I did make several revisions to the motivating example.

@cdisselkoen cdisselkoen added the pending This RFC is pending; for definitions see the README label Jul 3, 2023
@jeffsec-aws
Copy link

jeffsec-aws commented Jul 5, 2023

The md file for this RFC has a Typo.

The text spells out

In many cases, this can also be done by adding restrictions to the schema. For example, the appliesTo field for each action in the schema allows users to specify which entity types can be used as principals and resources for that action. In the example [above](https://github.com/cedar-policy/rfcs/blob/feature/khieta/is-operator/text/0005-is-operator.md#basic-example), the schema could specify that applyFile should only apply to principals of type User and resources of type File. However, this doesn't help if the user is not using a schema, the action is unconstrained, or the policy refers to an action that may act on multiple principal/resource types.

While the example above is:

permit(
  principal is User,
  action == Action::"viewFile",
  resource is File in Folder::"public"
);

Therefore the action should be viewFile and not applyFile in the sentence the schema could specify that applyFile should only apply to principals of type User

@khieta
Copy link
Contributor Author

khieta commented Jul 5, 2023

Fixed the typo. Thanks @jeffsec-aws

@anwarmamat anwarmamat self-requested a review July 11, 2023 13:43
@khieta
Copy link
Contributor Author

khieta commented Jul 21, 2023

The final comment period (FCP) for this RFC is starting now, with intent to accept. The FCP will end 2023-07-28 at noon PT / 3pm ET / 7pm UTC. Please add comments, and especially any objections, if you have any. For more on the RFC process, see https://github.com/cedar-policy/rfcs.

@khieta khieta 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 Jul 21, 2023
@khieta khieta merged commit 4534fa7 into main Jul 28, 2023
@khieta khieta deleted the feature/khieta/is-operator branch July 28, 2023 19:10
@khieta khieta 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 Jul 28, 2023
@khieta khieta added landed This RFC has landed; for definitions see the README and removed active This RFC is active; for definitions see the README labels Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
landed This RFC has landed; for definitions see the README
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants