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

Improve Debug Messages for PolicySet #1254

Open
2 tasks
ShiromMakkad opened this issue Oct 1, 2024 · 2 comments
Open
2 tasks

Improve Debug Messages for PolicySet #1254

ShiromMakkad opened this issue Oct 1, 2024 · 2 comments
Labels
feature-request This issue requets a substantial new feature

Comments

@ShiromMakkad
Copy link

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?

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change
@ShiromMakkad ShiromMakkad added feature-request This issue requets a substantial new feature pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. labels Oct 1, 2024
@cdisselkoen
Copy link
Contributor

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.

@cdisselkoen cdisselkoen removed the pending-triage The cedar maintainers haven't looked at this yet. Automicaly added to all new issues. label Oct 7, 2024
@ShiromMakkad
Copy link
Author

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

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
Projects
None yet
Development

No branches or pull requests

2 participants