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

Change EntityUID to always be concrete #72

Closed
1 of 2 tasks
khieta opened this issue May 25, 2023 · 2 comments
Closed
1 of 2 tasks

Change EntityUID to always be concrete #72

khieta opened this issue May 25, 2023 · 2 comments
Labels
backlog We hope to work on this in the future internal-improvement Refactoring, performance improvement, or other non-breaking change

Comments

@khieta
Copy link
Contributor

khieta commented May 25, 2023

Category

Other

Describe the feature you'd like to request

Currently, an EntityUID stores a reference to its EntityType, which can be either Concrete or Unspecified. We introduced the EntityType type to support omitting entities in the input query. For example: when the "resource" of a query should have no impact on the authorization result, then users should not need to provide a resource entity when calling is_authorized.

However, we never actually expect an EntityUID to be "Unspecified", which leads to extra error handling code in several locations.

Describe the solution you'd like

In CedarCore, the type EntityUID should always be concrete (i.e., its EntityType component should just be a Name), and "Unspecified" entities should only be possible when constructing a Query. This requires some refactoring of the code, but should introduce no breaking changes.

Describe alternatives you've considered

The current implementation works, but is more complicated than necessary.

Additional context

No response

Is this something that you'd be interested in working on?

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change
@khieta khieta added backlog We hope to work on this in the future internal-improvement Refactoring, performance improvement, or other non-breaking change labels Jun 6, 2023
@cdisselkoen
Copy link
Contributor

cdisselkoen commented Feb 13, 2024

Started implementing this and ran into the fact that the evaluator still needs to be able to evaluate unspecified entities, which means Value needs to be able to express unspecified entities. I think we should add a case to Literal, or even better, change the Literal::EntityUID case to Literal::Entity with an enum specified/unspecified (so that existing code matching on Literal::EntityUID and assuming that covers all possible entity cases doesn't break). This is of course a little bit of a stretch as unspecified entities are not allowed as literals, but unspecified entity currently lives in the ValueKind::Literal arm of Value, so it's not making the situation worse than it already is. Nonetheless, the complication of adding a case to Literal might negate the benefit of removing a case from EntityType.

@khieta
Copy link
Contributor Author

khieta commented Jun 3, 2024

Superseded by #919

@khieta khieta closed this as completed Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog We hope to work on this in the future internal-improvement Refactoring, performance improvement, or other non-breaking change
Projects
None yet
Development

No branches or pull requests

5 participants