chore(jans-cedarling): reduce PolicyStore visibility from pub to pub(crate)#14624
Open
olehbozhok wants to merge 8 commits into
Open
chore(jans-cedarling): reduce PolicyStore visibility from pub to pub(crate)#14624olehbozhok wants to merge 8 commits into
olehbozhok wants to merge 8 commits into
Conversation
…te)` Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Drop dead code from cedar schema parsing and policy store: - Simplify CedarSchemaJson to only track entity type names via a HashSet instead of parsing full EntityType/EntityShape; replace get_entity_schema with get_entity_type_name (callers only used the name). Delete entity_type.rs. - Drop the unused `required` field from Attribute variants and the unused Attribute::is_required(). Collapse Set/Extension to unit variants since their payloads were never read. - Remove unused principalTypes/resourceTypes from action AppliesTo. - Remove never-called default entity size validation (validate_default_entities and related error/limit code); the count limit check is retained. - Remove unused name/description/cedar_version fields from PolicyStore and the associated cedar version parsing helpers. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Follow-up to the schema/policy-store cleanup so the crate passes `cargo clippy --all-targets -- -D warnings`: - build_expr: match unit Attribute variants (String/Long/Boolean/Set/ Extension) without struct-pattern braces. - cedar_json: deserialize entityTypes via a MapAccess visitor that collects only keys, avoiding a HashMap with a zero-sized value type. - default_entities: match the explicit InvalidParentUid variant instead of a wildcard in test assertions; remove the now-unused from_hashmap test helper. - attribute: restrict the test-only constructor helpers to pub(crate). Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Member
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesCedar schema and authorization
Policy-store and API boundaries
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mo-auto
added
comp-jans-cedarling
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-cedarling/cedarling/src/common/cedar_schema/cedar_json/action.rs (1)
13-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winField visibility not narrowed along with struct visibility. Both
Action/AppliesToandCedarSchemawere changed frompubtopub(crate)in this PR, but their fields were leftpubinstead of also being narrowed — the same fix pattern applies to all three sites.
jans-cedarling/cedarling/src/common/cedar_schema/cedar_json/action.rs#L13-L19: changepub applies_to: AppliesTo,topub(crate) applies_to: AppliesTo,.jans-cedarling/cedarling/src/common/cedar_schema/cedar_json/action.rs#L33-L36: changepub context: Option<Attribute>,topub(crate) context: Option<Attribute>,.jans-cedarling/cedarling/src/common/cedar_schema/mod.rs#L14-L18: changepub schema,pub json,pub validator_schematopub(crate).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/common/cedar_schema/cedar_json/action.rs` around lines 13 - 19, The fields of the crate-visible schema types still expose public visibility. In jans-cedarling/cedarling/src/common/cedar_schema/cedar_json/action.rs:13-19, narrow Action.applies_to to pub(crate); in jans-cedarling/cedarling/src/common/cedar_schema/cedar_json/action.rs:33-36, narrow the Action/AppliesTo context field to pub(crate); and in jans-cedarling/cedarling/src/common/cedar_schema/mod.rs:14-18, narrow CedarSchema.schema, CedarSchema.json, and CedarSchema.validator_schema to pub(crate).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-cedarling/cedarling/src/common/cedar_schema/cedar_json.rs`:
- Around line 101-107: Update the doc comment for
CedarSchema::get_entity_type_name to describe resolving and returning the
qualified entity type name, including the default_namespace lookup behavior,
rather than merely checking whether the type exists. Leave the separate
entity_type_exists helper documentation and implementation unchanged.
In `@jans-cedarling/cedarling/src/common/policy_store.rs`:
- Line 49: Narrow the visibility of all currently public fields in PolicyStore
to pub(crate), or private where no external callers require access. Also
restrict DefaultEntities::inner to the most restrictive visibility supported by
its callers; update both affected definitions without changing their behavior.
---
Outside diff comments:
In `@jans-cedarling/cedarling/src/common/cedar_schema/cedar_json/action.rs`:
- Around line 13-19: The fields of the crate-visible schema types still expose
public visibility. In
jans-cedarling/cedarling/src/common/cedar_schema/cedar_json/action.rs:13-19,
narrow Action.applies_to to pub(crate); in
jans-cedarling/cedarling/src/common/cedar_schema/cedar_json/action.rs:33-36,
narrow the Action/AppliesTo context field to pub(crate); and in
jans-cedarling/cedarling/src/common/cedar_schema/mod.rs:14-18, narrow
CedarSchema.schema, CedarSchema.json, and CedarSchema.validator_schema to
pub(crate).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0110f6ec-32a2-435b-97ae-9769431210d7
📒 Files selected for processing (14)
jans-cedarling/cedarling/src/authz/build_ctx.rsjans-cedarling/cedarling/src/common/cedar_schema/cedar_json.rsjans-cedarling/cedarling/src/common/cedar_schema/cedar_json/action.rsjans-cedarling/cedarling/src/common/cedar_schema/cedar_json/attribute.rsjans-cedarling/cedarling/src/common/cedar_schema/cedar_json/entity_type.rsjans-cedarling/cedarling/src/common/cedar_schema/mod.rsjans-cedarling/cedarling/src/common/default_entities.rsjans-cedarling/cedarling/src/common/default_entities_limits.rsjans-cedarling/cedarling/src/common/policy_store.rsjans-cedarling/cedarling/src/common/policy_store/legacy_store/mod.rsjans-cedarling/cedarling/src/common/policy_store/legacy_store/test.rsjans-cedarling/cedarling/src/common/policy_store/manager.rsjans-cedarling/cedarling/src/entity_builder/build_expr.rsjans-cedarling/cedarling/src/lib.rs
💤 Files with no reviewable changes (4)
- jans-cedarling/cedarling/src/common/cedar_schema/cedar_json/entity_type.rs
- jans-cedarling/cedarling/src/lib.rs
- jans-cedarling/cedarling/src/common/policy_store/manager.rs
- jans-cedarling/cedarling/src/common/policy_store/legacy_store/mod.rs
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…DefaultEntities Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
coderabbitai
Bot
added
the
kind-enhancement
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jans-cedarling/cedarling/src/common/cedar_schema/cedar_json.rs (1)
218-229: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the crate assertion helper and describe this structural comparison.
This compares nested schema structures; use
test_utils::assert_eqand add a message explaining that only entity type names are retained.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/common/cedar_schema/cedar_json.rs` around lines 218 - 229, Update the structural comparison in the CedarSchemaJson test to use the crate’s test_utils::assert_eq helper instead of the standard assertion, and provide a message stating that only entity type names are retained.Sources: Coding guidelines, Learnings
jans-cedarling/cedarling/src/common/default_entities.rs (1)
1228-1237: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd diagnostic messages to the warning-content assertions.
jans-cedarling/cedarling/src/common/default_entities.rs#L1228-L1237: add messages to theentry_idequality and serialized-value assertions.jans-cedarling/cedarling/src/common/default_entities.rs#L1319-L1329: add equivalent messages for each warning in the loop.As per coding guidelines, “All assertions must include a descriptive message explaining what is being tested.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/common/default_entities.rs` around lines 1228 - 1237, Add descriptive diagnostic messages to the entry_id equality and value-content assertions in the DefaultEntityWarning match at jans-cedarling/cedarling/src/common/default_entities.rs lines 1228-1237. Add equivalent messages to each assertion inside the warning loop at lines 1319-1329, explaining the expected warning content being validated.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@jans-cedarling/cedarling/src/common/cedar_schema/cedar_json.rs`:
- Around line 218-229: Update the structural comparison in the CedarSchemaJson
test to use the crate’s test_utils::assert_eq helper instead of the standard
assertion, and provide a message stating that only entity type names are
retained.
In `@jans-cedarling/cedarling/src/common/default_entities.rs`:
- Around line 1228-1237: Add descriptive diagnostic messages to the entry_id
equality and value-content assertions in the DefaultEntityWarning match at
jans-cedarling/cedarling/src/common/default_entities.rs lines 1228-1237. Add
equivalent messages to each assertion inside the warning loop at lines
1319-1329, explaining the expected warning content being validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0e8786e9-61b3-4c9e-8da8-3109f2600da9
📒 Files selected for processing (3)
jans-cedarling/cedarling/src/common/cedar_schema/cedar_json.rsjans-cedarling/cedarling/src/common/default_entities.rsjans-cedarling/cedarling/src/common/policy_store.rs
Removing the required `name` field from PolicyStore also removed the "missing required field 'name'" validation gate. Malformed policy store fixtures now surface the next parse error instead, so the stale expectations no longer match. - Rust: drop the missing-name case from test_invalid_policy_store_entry; the schema and policies cases still assert their required-field errors. - Python: point the base64/utf8/schema error cases at the actual decode and schema-parse messages now returned. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
olehbozhok
force-pushed
the
jans-cedarling-14614
branch
from
July 24, 2026 15:25
36b950f to
f1568db
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jans-cedarling/cedarling/src/common/policy_store.rs (2)
235-235: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun rustfmt on the constructor signature.
This line exceeds the repository’s 100-character Rust limit. Let rustfmt wrap the arguments while preserving the configured indentation.
As per coding guidelines, Rust files must use the project rustfmt settings with a maximum line width of 100 characters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/common/policy_store.rs` at line 235, Run rustfmt on the PolicyStore constructor signature in new, using the repository’s configured 100-character width and indentation settings; preserve the constructor’s behavior and arguments.Source: Coding guidelines
713-725: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd descriptive messages to the new test assertions.
All added
assert!andassert_eq!calls should explain what is being verified so failures are actionable.As per coding guidelines, all assertions must include a descriptive message explaining what is being tested.
Also applies to: 727-749, 752-781
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/common/policy_store.rs` around lines 713 - 725, Add descriptive failure messages to every newly added assert! and assert_eq! in annotations_map_merges_policies and the additional tests at the referenced locations, stating the expected behavior each assertion verifies. Keep the existing test logic and expected values unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@jans-cedarling/cedarling/src/common/policy_store.rs`:
- Line 235: Run rustfmt on the PolicyStore constructor signature in new, using
the repository’s configured 100-character width and indentation settings;
preserve the constructor’s behavior and arguments.
- Around line 713-725: Add descriptive failure messages to every newly added
assert! and assert_eq! in annotations_map_merges_policies and the additional
tests at the referenced locations, stating the expected behavior each assertion
verifies. Keep the existing test logic and expected values unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8c7ec6cb-64b8-4f0f-b2ba-c2976ff2f58a
📒 Files selected for processing (2)
jans-cedarling/cedarling/src/common/policy_store.rsjans-cedarling/cedarling/src/lib.rs
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prepare
Description
Target issue
closes #14614
Implementation Details
Goal: reduce
PolicyStorevisibility frompubtopub(crate).PolicyStorewas exported publicly (re-exported viabindingsinlib.rs), but nothing outside the crate needs it. Making itpub(crate)first required removing the public API surface that referenced it and the dead code that kept unused fields/methods "live".Steps
Drop the public re-export — removed
pub use crate::common::policy_store::PolicyStore;from thebindingsmodule inlib.rs.Remove dead
PolicyStoresurface so nothing external depends on it:name,description,cedar_version.apply_default_entities_limits()method (which in turn removed thesemver::VersionandDefaultEntitiesLimitsdependencies, plus thedefault_entities_limitsenforcement path and related legacy-store/manager helpers and tests).Downgrade visibility —
PolicyStore→pub(crate). Applied the same narrowing to closely coupled types now crate-internal:TrustedIssuer,PoliciesContainer, and their methods.Simplify schema lookup exposed by the cleanup —
get_entity_schemareturned(type_name, type_schema)but every caller discarded the schema half; replaced withget_entity_type_namereturning only the name. Call sites inbuild_ctx.rsupdated. This also let unusedAttributevariant payloads (String,Long,Boolean,Set,Extension) become fieldless.Cleanup — final commit resolves clippy warnings surfaced by the removals.
Net: no behavioral change;
PolicyStoreand its coupled types are now crate-private, and the compiler enforces the removed surface stays unused.Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit