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

CLI should support reading JSON formatted policy sets (as opposed to single policies) #950

Closed
2 tasks
john-h-kastner-aws opened this issue Jun 4, 2024 · 9 comments · Fixed by #1057
Closed
2 tasks
Labels
feature-request This issue requets a substantial new feature good-first-issue Good for newcomers. A smaller issue that someone new to the Cedar codebase should be able to tackle papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request

Comments

@john-h-kastner-aws
Copy link
Contributor

Category

CLI features/changes

Describe the feature you'd like to request

The Cedar CLI supports JSON formatted policies by passing the --policy-format json argument, but this expects exactly one policy in the JSON policy.

We recently added a JSON format for policy sets in #783, but did not add support for this format to the CLI. We should update the CLI to accept this format. It ideally should continue to accept the single policy format to avoid making an unnecessary breaking change.

Describe alternatives you've considered

.

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
@john-h-kastner-aws john-h-kastner-aws added pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. feature-request This issue requets a substantial new feature backlog papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request and removed pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. labels Jun 4, 2024
@john-h-kastner-aws john-h-kastner-aws added the good-first-issue Good for newcomers. A smaller issue that someone new to the Cedar codebase should be able to tackle label Jun 12, 2024
@memark
Copy link
Contributor

memark commented Jun 29, 2024

The actual deserializing the policy set from json appears to be easy.

About the non-breaking behaviour, the current logic looks like this:

  1. try parsing as policy
  2. if fails, try parsing as template
  3. if fails, return (template parsing) error

Where in this small cascade do we want to add the "try parsing as policy set" step?

@john-h-kastner-aws
Copy link
Contributor Author

john-h-kastner-aws commented Jul 2, 2024

The procedure I had in mind would look at JSON attributes to decided what it needs to parse. Something like:

Parse policies as a serde_json::Value using serde_json::from_str. Error if it is not a JSON object. Then parse based on contents of object:

  1. If it contains any of the attributes required for JSON policies, then try to parse as a JSON policy (with the current logic to possibly parse as a template).
  2. If it contains any of the attributes required for JSON policy set, then try to parse as a JSON policy set.
  3. Otherwise, error.

The attributes for policies/policy-sets are disjoint, so we can easily tell when an object is intended to be one or the other. There should probably be helpful error messages if it has none of the attributes or attributes from both.

@memark
Copy link
Contributor

memark commented Jul 2, 2024

Ok, sounds reasonable. (It would introduce some coupling to the actual json representations though, but it might be worth it in terms of simplicity.)

If we determine that it's not a policy set, should we then call the same logic as above (try parse as policy, try parse as template, error)?
I see now that this was already answered above.

@muse254
Copy link

muse254 commented Jul 3, 2024

I would like to start development on this given we are heading the 'check if set and parse as set' path.
@memark what did you mean by coupling?

If I was to implement it that logic would remain as is, so far as I can tell

If we determine that it's not a policy set, should we then call the same logic as above (try parse as policy, try parse as template, error)?

Granted I might be missing a lot of context but happy to try this on for size

@memark
Copy link
Contributor

memark commented Jul 3, 2024

I would like to start development on this given we are heading the 'check if set and parse as set' path.

I'm actually planning to develop this myself. That's why I'm investigating and discussing the solution here. :)

@memark what did you mean by coupling?

I mean that the CLI would now know the structure of the json schema, and use it to decide what parser to call. As of right now, the CLI knows nothing about this.

@muse254
Copy link

muse254 commented Jul 3, 2024

@memark awesome, looking forward to your contribution. I didn't realise you were planning to develop it :)

@memark memark mentioned this issue Jul 4, 2024
3 tasks
@muse254
Copy link

muse254 commented Jul 10, 2024

Is it safe to have this issue closed since it's take care of? @memark @john-h-kastner-aws

@memark
Copy link
Contributor

memark commented Jul 10, 2024

@muse254 Actually, it's not implemented yet. The PR above is just a first refactoring step. I'm currently working on the second PR, adding the actual functionality.

@muse254
Copy link

muse254 commented Jul 10, 2024

I had not given the merged PR a cursory look, thats ok @memark

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue requets a substantial new feature good-first-issue Good for newcomers. A smaller issue that someone new to the Cedar codebase should be able to tackle papercut Small annoyances in the Cedar SDK. Lower priority fixes than bugs. Smaller than a fature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants