cedar-policy / rfcs

Apache License 2.0
10 stars 8 forks source link

eid as attribute #54

Closed mwhicks1 closed 8 months ago

mwhicks1 commented 8 months ago

Proposes to make an entity reference's EID a pseudo-attribute $id.

Rendered

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

hakanson commented 8 months ago

Is there a potential drawback / anti-pattern where Cedar policies will use $id and String values where before they used Entities? For example, this policy has owner as an Entity

permit (principal is PhotoApp::User, action, resource is PhotoApp::Photo)
when { resource.owner == principal };

example slice that would be passed in using __entity

  {
    "uid": { "type": "PhotoApp::Photo", "id": "JaneDoe.jpg" },
    "parents": [],
    "attrs": {
      "owner": {
        "__entity": { "type": "PhotoApp::User", "id": "JaneDoe" }
      }
    }
  }

This RFC would enable policies where owner is an String matching the id

permit (principal is PhotoApp::User, action, resource is PhotoApp::Photo)
when { resource.owner == principal.$id };
  {
    "uid": { "type": "PhotoApp::Photo", "id": "JaneDoe.jpg" },
    "parents": [],
    "attrs": {
      "owner": "JaneDoe"
    }
  }

The owner attribute as String may seem more obvious to early policy authors, but using an Entity in the schema enables future conditions to use other attributes of that Entity beyond the id. I'm not opposed to the RFC, just thinking about future best practices documentation, as this might affect backwards compatible Cedar schema authoring.

mwhicks1 commented 8 months ago

Cedar policies will use $id and String values where before they used Entities?

Yes, I could imagine this happening. Put another way, we are setting resource.owner to be an entity's EID and not the entity itself, which means that different-typed "entities" (one of which is stored as just its string EID) could be deemed equal, e.g., User::"a9edd19b-46f3-486b-887d-4c378aced880".$id == Admin::"a9edd19b-46f3-486b-887d-4c378aced880".$id while User::"a9edd19b-46f3-486b-887d-4c378aced880" != Admin::"a9edd19b-46f3-486b-887d-4c378aced880".

I'm not sure how tempting this would be to do, though, if users follow our advice of using unique IDs for EIDs of entities like users, resources, etc. The EID is going to be non-sensical (like a9edd19b-46f3-486b-887d-4c378aced880 above), so you won't be tempted to store it in an attribute directly. The $id attribute is useful only when the EID is meaningful, which should be the case for enums and Action entities.

andrewmwells-amazon commented 8 months ago

I feel like this would encourage bad practices. For Actions, I think policies should check action in MyActionGroup instead of action.$id like "prefix*", so in my understanding this RFC would only be useful for enumerated entities.

If enough people ask for a feature like this, I'd rather implement alternative C of https://github.com/cedar-policy/rfcs/pull/53 than add this.

khieta commented 8 months ago

Agree with @andrewmwells-amazon's comment above. Also see @D-McAdams's post on why not to allow wildcard matching for Entity Ids: https://cedarland.blog/design/why-no-entity-wildcards/content.html.

emina commented 8 months ago

Agree with @andrewmwells-amazon's comment above. Also see @D-McAdams's post on why not to allow wildcard matching for Entity Ids: https://cedarland.blog/design/why-no-entity-wildcards/content.html.

Agree with the above (as well as Andrew’s and Kevin’s points) and would vote against adding this feature.

aaronjeline commented 8 months ago

Voting against, and bikeshedding on syntax, I'd prefer this as a function instead of a wacky looking attribute. Ie:

id(Foo::"bar") == "bar"

I feel this introduces less confusion about the data model.

mwhicks1 commented 8 months ago

The masses have spoken! Let's drop this. We may revisit depending on how things go with enumerated entities.