search

cedar-policy / cedar

Implementation of the Cedar Policy Language
https://www.cedarpolicy.com
Apache License 2.0
892 stars 80 forks source link

Improve Debug Messages for PolicySet #1254

Open ShiromMakkad opened 1 month ago

ShiromMakkad commented 1 month ago

Category

User level API features/changes

Describe the feature you'd like to request

Right now, printing a debug formatted policy set looks like: PolicySet { ast: PolicySet { templates: {PolicyID("p-PLGRA7f4vobdGt8J9jS68M"): Template { body: TemplateBody { id: PolicyID("p-PLGRA7f4vobdGt8J9jS68M"), loc: Some(Loc { span: SourceSpan { offset: SourceOffset(0), length: 37 }, src: "forbid (principal, action, resource);" }), annotations: Annotations({}), effect: Forbid, principal_constraint: PrincipalConstraint { constraint: Any }, action_constraint: Any, resource_constraint: ResourceConstraint { constraint: Any }, non_scope_constraints: Expr { expr_kind: Lit(Bool(true)), source_loc: Some(Loc { span: SourceSpan{ offset: SourceOffset(0), length: 37 }, src: "forbid (principal, action, resource);" }), data: () } }, slots: [] }}, links: {PolicyID("p-PLGRA7f4vobdGt8J9jS68M"): Policy { template: Template { body: TemplateBody { id: PolicyID("p-PLGRA7f4vobdGt8J9jS68M"), loc: Some(Loc { span: SourceSpan { offset: SourceOffset(0), length: 37 }, src: "forbid (principal, action, resource);" }), annotations: Annotations({}), effect: Forbid, principal_constraint: PrincipalConstraint { constraint: Any }, action_constraint: Any, resource_constraint: ResourceConstraint { constraint: Any }, non_scope_constraints: Expr { expr_kind: Lit(Bool(true)), source_loc: Some(Loc { span: SourceSpan { offset: SourceOffset(0), length: 37 }, src: "forbid (principal, action, resource);" }), data: () } }, slots: [] }, link: None, values: {} }}, template_to_links_map: {PolicyID("p-PLGRA7f4vobdGt8J9jS68M"): {PolicyID("p-PLGRA7f4vobdGt8J9jS68M")}} }, policies: {PolicyId(PolicyID("p-PLGRA7f4vobdGt8J9jS68M")): Policy { ast: Policy { template: Template { body: TemplateBody { id: PolicyID("p-PLGRA7f4vobdGt8J9jS68M"), loc: Some(Loc { span: SourceSpan { offset: SourceOffset(0), length: 37 }, src: "forbid (principal, action, resource);" }), annotations: Annotations({}), effect: Forbid, principal_constraint: PrincipalConstraint { constraint: Any }, action_constraint: Any, resource_constraint: ResourceConstraint { constraint: Any }, non_scope_constraints: Expr { expr_kind: Lit(Bool(true)), source_loc: Some(Loc { span: SourceSpan { offset: SourceOffset(0), length: 37 }, src: "forbid (principal, action, resource);" }), data: () } }, slots: [] }, link: None, values: {} }, lossless: Text { text: "forbid (principal, action, resource);", slots: {} } }}, templates: {} } for an extremely simple policy set.

I'd prefer a shorter, easier to read version. I think simply printing a list of policy ids would be helpful. We don't use template linked policies, so I'm open to suggestions there.

Describe alternatives you've considered

We could print a display formatted policy set but that has three problems:

  1. We'd have to manually derive Debug on structs that include a PolicySet.
  2. Printing a policy set of 100+ policies is very verbose
  3. For our purposes, we can't expose the contents of a policy set. The policy ids are ok but the contents are PII.

You could also:

  1. You could print the contents of policies. I don't think this is as useful since the Display trait already does that.
  2. You could truncate the policy list after N policies.

Additional context

You might also want to create a Debug implementation for a single policy as part of the change.

Also, I want to open a similar issue for Entities and Context depending on the discussion here. For entities, I'm thinking of using the Entity uid. Not sure about Context.

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

cdisselkoen commented 1 month ago

We discussed as a team. We agree that the current representation is not useful, and exposes internal details that don't need to be exposed. (And in particular that the loc representation is far too verbose.)

However, we don't want the Debug representation to have less information that the Display representation, as that seems backwards. In particular, we feel that the Debug representation should include both the policy ids and contents; if you need a different representation, you can implement one manually (eg, manual Debug on your "structs that include a PolicySet" that you mentioned above).

My suggested resolution for whoever picks up this issue is to create a custom Debug impl for PolicySet that displays the policy ids, contents, information about template links, etc but either drops the source locations or at least displays them more concisely. The Debug impl for the internal ast::PolicySet should probably remain as-is for internal debugging purposes.

ShiromMakkad commented 1 month ago

This makes sense to me. Will still be helpful for us for our debugging and we can create a custom solution for prod.