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

IDX-470: Change PolicySet.IsAuthorized to take an EntityGetter interface in place of an EntityMap #71

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

kjamieson-sdm
Copy link
Collaborator

This changes the PolicySet IsAuthorized() method to take an EntityGetter interface instead of a concrete EntityMap instance, allowing alternative entity getter implementations to be passed to the authorization.

This is just a wrapper around the underlying map get, in preparation for
introducing an EntityGetter interface.

Signed-off-by: Kevin Jamieson <[email protected]>
This defines an interface for retrieving an Entity by EntityUID, implemented by
EntityMap, in preparation for using this in IsAuthorized().

Signed-off-by: Kevin Jamieson <[email protected]>
This replaces the concrete EntityMap passed to IsAuthorized() with an
EntityGetter interface, allowing alternative implementations of this interface
to be supplied for authorization.

While this is hypothetically a breaking change, we think that is extremely
unlikely to be the case in practice. Any code passing an EntityMap will
continue to work - the only potential breakage would be if a caller were
directly passing a map[EntityUID]Entity here.

Signed-off-by: Kevin Jamieson <[email protected]>
@kjamieson-sdm
Copy link
Collaborator Author

While very unlikely (IMO), this could technically be a breaking change if some existing caller is passing a map[EntityUID]Entity to IsAuthorized instead of a EntityMap.

If we think that risk is too great, we can fall back to the alternative of introducing some new IsAuthorized or Authorizer implementation under x/exp instead.

@philhassey
Copy link
Collaborator

My only thought is maybe if we add something to the README just noting our definition of non-breaking is that would be nice. Something like:

  • we may add variadics to functions that don't have them in cases where we want to expand the arguments of a function / method
  • we may change concrete types to interfaces in cases where we want to expand the variety of arguments a function / method can take

Copy link
Collaborator

@philhassey philhassey left a comment

Choose a reason for hiding this comment

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

LGTM, with that one add to README

At present time, the maintainers are reserving the right to:
* Add variadics to existing functions/methods.
* Replace concrete types with interfaces in the parameters to functions/methods
  (as occurred in this patch set).

While strictly breaking changes, we are asserting that the benefits of being
able to make changes of these forms without a major revision outweigh the
unlikely possibility of breaking code written against cedar-go.

We may of course choose to revisit and revise these policies in the future,
especially as cedar-go gains wider usage.

Signed-off-by: Kevin Jamieson <[email protected]>
@kjamieson-sdm kjamieson-sdm merged commit 224fafe into cedar-policy:main Dec 3, 2024
3 checks passed
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.

2 participants