feat: multi issuer and new format#5
Open
SafinWasi wants to merge 113 commits intoJanssenProject:mainfrom
Open
Conversation
📝 WalkthroughWalkthroughThis PR restructures a Cedar policy store from a snapshot-based format to a modular architecture. It removes legacy policy snapshots, old-format policy files, and license/documentation; introduces new metadata, manifests, and schemas for both Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tarpDemo/policies/allow_student_read.cedar`:
- Line 10: The policy uses a substring check
context.tokens.jans_userinfo_token.getTag("role").contains("Student") which is
overly permissive; change the check to require an exact match (e.g., compare the
role tag for equality to "Student" instead of using contains) so only the
precise "Student" role passes (update the expression on
context.tokens.jans_userinfo_token.getTag("role")).
- Line 4: The action check in allow_student_read.cedar uses set-membership
syntax ("action in [Jans::Action::\"Read\"]") which is inconsistent with other
policies; change this to the equality form used elsewhere (action ==
Jans::Action::"Read") so the policy's action check matches user_can_read.cedar
and editor_can_share.cedar for consistency and easier maintenance.
In `@tarpDemo/policies/allow_teacher_secretdocument.cedar`:
- Around line 2-6: The permit rule currently allows any action on SecretDocument
by leaving the action unconstrained; update the rule that matches
"permit(principal, action, resource is Jans::SecretDocument)" to explicitly
restrict action to the intended operation (e.g., change to require action ==
Jans::Action::"Read" or action in [...] if multiple) so Teachers cannot perform
all 14 actions; ensure you modify the permit clause that references principal,
action, and resource is Jans::SecretDocument and mirror the style used in
security_admin_can_write_to_secretdocument.cedar and allow_student_read.cedar.
In `@tarpDemo/policies/cannot_write_if_not_admin.cedar`:
- Around line 8-10: The when-clause is using tag APIs but `role` is an attribute
on Userinfo_token; replace tag checks with attribute access: where you currently
call context.tokens.jans_userinfo_token.hasTag("role") and
context.tokens.jans_userinfo_token.getTag("role").contains("admin"), change to
check the attribute (e.g., context.tokens.jans_userinfo_token.role exists and
!context.tokens.jans_userinfo_token.role.contains("admin")) so the forbid rule
actually fires; apply the same fix to the other listed policies
(user_can_read_if_confidential_role, security_admin_can_write_to_secretdocument,
editor_can_share, allow_teacher_secretdocument, allow_student_read,
admin_can_write_to_application, etc.).
In `@tarpDemo/policies/editor_can_share.cedar`:
- Line 10: The policy uses
context.tokens.jans_userinfo_token.getTag("role").contains("Editor"), which
performs a substring match and may be too permissive; update the check in the
editor_can_share rule to perform an exact match against the role tag (use the
language's string-equality operator or exact-match function on the value
returned by getTag("role") instead of contains) so only the explicit "Editor"
role is allowed.
In `@tarpDemo/policies/user_can_read_if_confidential_role.cedar`:
- Around line 9-10: Replace tag-based access for the user's role with attribute
access: in the rule using context.tokens.jans_userinfo_token.hasTag("role") and
.getTag("role").contains("confidential"), change to the attribute pattern used
in tarpUnsignedDemo — use "context.tokens.jans_userinfo_token has role" to check
presence and
"context.tokens.jans_userinfo_token.role.contains(\"confidential\")" to test
membership; apply the same replacement across the other tarpDemo policies listed
(security_admin_can_write_to_secretdocument.cedar, editor_can_share.cedar,
cannot_write_if_not_admin.cedar, allow_student_read.cedar,
allow_teacher_secretdocument.cedar, admin_can_write_to_application.cedar) so all
reference the role attribute instead of tags.
In `@tarpDemo/policies/user_can_read_if_email_is_example.cedar`:
- Around line 8-10: The policy is using tag APIs on context.tokens.jans_id_token
(hasTag/getTag) which is wrong because id_token.email is an attribute (type
email_address) per the schema, and the .contains check is too permissive; update
the condition to read the attribute directly from context.tokens.jans_id_token
(e.g., access the email attribute) and perform an exact equality comparison to
"admin@example.com" so only that exact address matches.
In `@tarpDemo/policies/user_can_read_in_corp_network.cedar`:
- Around line 1-11: The policy id user_can_read_in_corp_network is misleading
because the rule uses a generic principal (which includes Workload) rather than
only users; rename the policy id to reflect the broader principal scope (e.g.,
can_read_in_corp_network or principal_can_read_in_corp_network) by updating the
`@id` value in the policy (user_can_read_in_corp_network ->
principal_can_read_in_corp_network) so the identifier matches the principal used
in the permit clause and keep the rest of the rule (principal, action ==
Jans::Action::"Read", resource is Jans::SecretDocument, and the when block)
unchanged.
In `@tarpDemo/policies/user_can_read.cedar`:
- Around line 3-7: The policy user_can_read currently uses a broad principal
constraint (`principal`) allowing Workload principals to match; change the
principal constraint back to `principal is Jans::User` to ensure only users with
a `context has tokens.jans_userinfo_token` can read Jans::Application, or
alternatively create a separate workload-specific policy (e.g.,
`workload_can_read`) that keeps `principal is Jans::Workload` and checks an
appropriate workload token (e.g., `tokens.jans_access_token`); apply the same
fix pattern to the related policies `user_can_search`, `user_can_execute`, and
`user_can_monitor`.
In `@tarpDemo/policies/user_cannot_execute_on_public_network.cedar`:
- Around line 1-11: Rename the policy id and rule to reflect that it applies to
any principal (not just users): change the
`@id`("user_cannot_execute_on_public_network") identifier and any internal name
references to `@id`("cannot_execute_on_public_network") (or similar neutral name).
Verify the forbid block (principal, action == Jans::Action::"Execute", resource
is Jans::Application) and the guards (context has tokens.jans_userinfo_token,
context has network_type, context.network_type == "public") remain unchanged
unless you also intend to broaden or tighten the guard; only the identifier/name
needs updating to avoid implying it targets users exclusively.
In `@tarpDemo/policies/user_cannot_read_if_compromised.cedar`:
- Around line 2-11: The forbid rule for principal/action/resource (principal,
action == Jans::Action::"Read", resource is Jans::SecretDocument) only checks
context has tokens.jans_userinfo_token so Workload principals bypass the
compromised-device restriction; update the rule to explicitly restrict to user
principals or add workload-specific checks: either add a guard like principal is
Jans::User (or principal.type == "User") so the rule only applies to users, or
extend the when clause to also handle workloads (e.g., check for a workload
token or principal is Jans::Workload and apply equivalent device_health checks)
in the forbid rule and replicate the same change for the other forbid rules that
currently rely on context has tokens.jans_userinfo_token.
In `@tarpDemo/policies/user_cannot_read_if_fraudulent.cedar`:
- Around line 2-11: The forbid rule in user_cannot_read_if_fraudulent.cedar
(forbid ... action == Jans::Action::"Read" ... resource is Jans::SecretDocument)
should not depend on tokens.jans_userinfo_token because permit rules (e.g.,
user_can_read_if_email_is_example.cedar) may grant Read with only
tokens.jans_id_token; remove the userinfo-only check or make it token-agnostic
by replacing "context has tokens.jans_userinfo_token" with either no token check
(just rely on context.fraud_indicators) or a union check like "context has
tokens.jans_userinfo_token || context has tokens.jans_id_token" so the
fraud_indicators.contains("high_risk") branch always blocks high-risk principals
regardless of token type. Ensure the forbid condition references
context.fraud_indicators.contains("high_risk") as before and that permit rules
remain consistent.
In `@tarpDemo/policies/user_cannot_write_from_restricted_country.cedar`:
- Around line 2-11: The existing forbid rule that prevents principal + action ==
Jans::Action::"Write" on resource is Jans::SecretDocument only checks context
has tokens.jans_userinfo_token, which misses Workload principals that present
tokens.jans_access_token; fix by either (A) adding a separate forbid rule
mirroring the current one but checking context has tokens.jans_access_token for
Workload principals, or (B) modify the when clause of the current forbid to
accept either token (e.g., require context has tokens.jans_userinfo_token ||
context has tokens.jans_access_token) and still check
context.geolocation.contains("restricted") so both User and Workload principals
are covered.
In `@tarpDemo/trusted-issuers/jans.json`:
- Around line 1-50: Add a POSIX trailing newline to this JSON file by appending
a newline character at the end of trusted-issuers/jans.json (after the closing
brace of the top-level object). Ensure the file still contains the existing
fields (e.g., "id": "1985d6d8...", "name": "Jans", and the "token_metadata"
block with "access_token", "id_token", and "userinfo_token") and only adds the
missing final newline so tooling that expects a newline-terminated file is
satisfied.
In `@tarpUnsignedDemo/manifest.json`:
- Around line 9-12: The manifest entry for the policy file
a2d0458ac26b69e4b191193fa307d941e8ab0ca2d5b1.cedar uses an opaque hash name
which harms maintainability; rename that file to a descriptive name such as
teacher_role_policy.cedar (or another name reflecting the policy intent), update
the manifest key to match the new filename and preserve the same size/checksum
values, and confirm that the TokensContext schema in tarpUnsignedDemo (which
only includes total_token_count) remains intentionally minimal for this unsigned
demo scope so future policies that reference jans_* tokens are not added without
schema updates.
In `@tarpUnsignedDemo/schema.cedarschema`:
- Around line 29-39: The schema declares full Access_token, id_token, and
Userinfo_token entity types but TokensContext lacks fields exposing them, which
can confuse readers; either remove these token entity definitions if you intend
TokensContext to remain minimal, or add corresponding fields to TokensContext to
make them referenceable (e.g., add tokens.access, tokens.id, tokens.userinfo or
similar), and ensure User.id_token and User.userinfo_token usage aligns with
your choice; update or delete the entity declarations (Access_token, id_token,
Userinfo_token) or extend TokensContext accordingly so the schema is consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97faff1d-4a18-403e-ad0a-a3a0df331518
📒 Files selected for processing (41)
449805c83e13f332b1b35eac6ffa93187fbd1c648085.json6d9f73b2d44ad4e7aa8f1182cde9f72dcbaa244f4327.jsonLICENSEtarpDemo/13b34711a196f382ade96677cefd513a2a6deff1b949.policytarpDemo/50c0ea31a9ed25a575b7538dec38cb953431651e7bec.policytarpDemo/76acfe86fb09731682f92c4fbf7d2e066813ce639404.policytarpDemo/870eaec25d9c8e9a642c4ed4d08b1ad480e6bc1348c7.policytarpDemo/9e2f1520bd292765609e8ce57add8d3c134e3a91759d.policytarpDemo/README.mdtarpDemo/a6f9c66ddcadeeec9e0f65420c89715b00a835ffdaec.policytarpDemo/e459eba6691d141497dd1317e44d3d5b3bdd9fd65150.policytarpDemo/f38f10e80c6f429153cf97781a4c6bcf29761a04259f.policytarpDemo/fe84f983188f1639a03ed77c0753fc893fbe0d4cf155.policytarpDemo/manifest.jsontarpDemo/metadata.jsontarpDemo/policies/admin_can_write_to_application.cedartarpDemo/policies/allow_student_read.cedartarpDemo/policies/allow_teacher_secretdocument.cedartarpDemo/policies/cannot_write_if_not_admin.cedartarpDemo/policies/editor_can_share.cedartarpDemo/policies/security_admin_can_write_to_secretdocument.cedartarpDemo/policies/user_can_compare.cedartarpDemo/policies/user_can_execute.cedartarpDemo/policies/user_can_monitor.cedartarpDemo/policies/user_can_read.cedartarpDemo/policies/user_can_read_if_confidential_role.cedartarpDemo/policies/user_can_read_if_email_is_example.cedartarpDemo/policies/user_can_read_in_corp_network.cedartarpDemo/policies/user_can_search.cedartarpDemo/policies/user_can_tag.cedartarpDemo/policies/user_cannot_execute_on_public_network.cedartarpDemo/policies/user_cannot_read_if_compromised.cedartarpDemo/policies/user_cannot_read_if_fraudulent.cedartarpDemo/policies/user_cannot_write_from_restricted_country.cedartarpDemo/schema.cedarschematarpDemo/trusted-issuers/jans.jsontarpUnsignedDemo/README.mdtarpUnsignedDemo/manifest.jsontarpUnsignedDemo/metadata.jsontarpUnsignedDemo/policies/a2d0458ac26b69e4b191193fa307d941e8ab0ca2d5b1.cedartarpUnsignedDemo/schema.cedarschema
💤 Files with no reviewable changes (14)
- tarpDemo/50c0ea31a9ed25a575b7538dec38cb953431651e7bec.policy
- tarpDemo/f38f10e80c6f429153cf97781a4c6bcf29761a04259f.policy
- tarpDemo/13b34711a196f382ade96677cefd513a2a6deff1b949.policy
- 6d9f73b2d44ad4e7aa8f1182cde9f72dcbaa244f4327.json
- tarpDemo/e459eba6691d141497dd1317e44d3d5b3bdd9fd65150.policy
- tarpDemo/a6f9c66ddcadeeec9e0f65420c89715b00a835ffdaec.policy
- tarpDemo/9e2f1520bd292765609e8ce57add8d3c134e3a91759d.policy
- tarpDemo/870eaec25d9c8e9a642c4ed4d08b1ad480e6bc1348c7.policy
- tarpDemo/README.md
- LICENSE
- tarpDemo/76acfe86fb09731682f92c4fbf7d2e066813ce639404.policy
- tarpDemo/fe84f983188f1639a03ed77c0753fc893fbe0d4cf155.policy
- tarpUnsignedDemo/README.md
- 449805c83e13f332b1b35eac6ffa93187fbd1c648085.json
| @id("allow_student_read") | ||
| permit ( | ||
| principal, | ||
| action in [Jans::Action::"Read"], |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent action syntax compared to other policies.
This policy uses action in [Jans::Action::"Read"] while other policies (e.g., user_can_read.cedar, editor_can_share.cedar) use action == Jans::Action::"...". Consider using consistent syntax across all policies for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tarpDemo/policies/allow_student_read.cedar` at line 4, The action check in
allow_student_read.cedar uses set-membership syntax ("action in
[Jans::Action::\"Read\"]") which is inconsistent with other policies; change
this to the equality form used elsewhere (action == Jans::Action::"Read") so the
policy's action check matches user_can_read.cedar and editor_can_share.cedar for
consistency and easier maintenance.
Comment on lines
2
to
11
| forbid ( | ||
| principal is Jans::User, | ||
| principal, | ||
| action == Jans::Action::"Read", | ||
| resource is Jans::SecretDocument | ||
| ) when { | ||
| ) | ||
| when { | ||
| context has tokens.jans_userinfo_token && | ||
| context has fraud_indicators && | ||
| context.fraud_indicators.contains("high_risk") | ||
| }; No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all permit rules for SecretDocument that might not require userinfo_token
rg -l 'SecretDocument' --type-add 'cedar:*.cedar' --type cedar | xargs -I{} sh -c 'echo "=== {} ===" && cat {}'Repository: JanssenProject/CedarlingQuickstart
Length of output: 2892
Fraud detection bypass: forbid rule doesn't apply to id_token-based permits.
This forbid rule requires tokens.jans_userinfo_token in the when clause, but user_can_read_if_email_is_example.cedar permits Read access to SecretDocument using only tokens.jans_id_token. When a principal provides only an id_token, the forbid rule's when condition evaluates to false and the fraud detection is skipped—allowing high-risk fraudulent principals to read SecretDocument if they have the required email tag.
The forbid rule must not depend on a specific token type in its when clause. Either remove the userinfo_token check from the forbid condition, or ensure all permit rules for SecretDocument consistently require the same token type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tarpDemo/policies/user_cannot_read_if_fraudulent.cedar` around lines 2 - 11,
The forbid rule in user_cannot_read_if_fraudulent.cedar (forbid ... action ==
Jans::Action::"Read" ... resource is Jans::SecretDocument) should not depend on
tokens.jans_userinfo_token because permit rules (e.g.,
user_can_read_if_email_is_example.cedar) may grant Read with only
tokens.jans_id_token; remove the userinfo-only check or make it token-agnostic
by replacing "context has tokens.jans_userinfo_token" with either no token check
(just rely on context.fraud_indicators) or a union check like "context has
tokens.jans_userinfo_token || context has tokens.jans_id_token" so the
fraud_indicators.contains("high_risk") branch always blocks high-risk principals
regardless of token type. Ensure the forbid condition references
context.fraud_indicators.contains("high_risk") as before and that permit rules
remain consistent.
Contributor
Author
There was a problem hiding this comment.
ID and userinfo tokens are coupled together during issuance
There was a problem hiding this comment.
@SafinWasi, thanks for the clarification! Since jans_id_token and jans_userinfo_token are always issued together, the forbid rule's context has tokens.jans_userinfo_token check will always hold whenever the id_token-based permit (e.g., user_can_read_if_email_is_example.cedar) fires — there is no bypass window. My concern was a false positive. I'll record this for future reviews.
(^˵◕ω◕˵^)
✏️ Learnings added
Learnt from: SafinWasi
Repo: JanssenProject/CedarlingQuickstart PR: 5
File: tarpDemo/policies/user_cannot_read_if_fraudulent.cedar:2-11
Timestamp: 2026-03-13T05:47:23.399Z
Learning: In the JanssenProject/CedarlingQuickstart Cedarling authorization model, `jans_id_token` and `jans_userinfo_token` are always coupled during token issuance. This means that whenever `context.tokens.jans_id_token` is present, `context.tokens.jans_userinfo_token` will also be present, and vice versa. Reviewers should not flag Cedar forbid rules that check for `tokens.jans_userinfo_token` as potentially bypassable via id_token-only flows, since both tokens are guaranteed to co-exist in the context. This applies broadly to all Cedar policy files under tarpDemo/policies/ and tarpUnsignedDemo/ in this repository.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: SafinWasi
Repo: JanssenProject/CedarlingQuickstart PR: 5
File: tarpDemo/policies/user_cannot_write_from_restricted_country.cedar:2-11
Timestamp: 2026-03-13T05:45:44.472Z
Learning: When reviewing Cedar policies, treat checks for tokens.jans_userinfo_token as a user-principal filter. Do not flag missing checks for tokens.jans_access_token in policies that gate conditions on context.has(tokens.jans_userinfo_token); such absence is intentional and by design. Apply this guidance broadly to all Cedar policy files under tarpDemo/policies (and similar policy directories) where the policy scope is User principals. If automated checks flag this, document the rationale and ensure CI/tests reflect this intentional distinction.
Learnt from: SafinWasi
Repo: JanssenProject/CedarlingQuickstart PR: 5
File: tarpDemo/policies/allow_teacher_secretdocument.cedar:2-6
Timestamp: 2026-03-13T05:43:25.249Z
Learning: In tarpDemo/policies/ allow_teacher_secretdocument.cedar (demo/quickstart context), reviewers should recognize that overly permissive Cedar policies are intentional for demonstration. Do not treat this as a production security issue. When reviewing, document that the repository is a demo with non-production data and ensure CI/docs clearly state this. If applicable, future real projects should replace permissive access with proper least-privilege controls before deployment.
Learnt from: SafinWasi
Repo: JanssenProject/CedarlingQuickstart PR: 5
File: tarpDemo/policies/cannot_write_if_not_admin.cedar:8-10
Timestamp: 2026-03-13T05:44:33.436Z
Learning: Guideline: In Cedar policies, access to JWT claims for token entities must use tag-based access, not entity attributes. Use hasTag("role") and getTag("role") to read claims (e.g., role) for all token entities (jans_userinfo_token, jans_access_token, jans_id_token). Do not use attribute-style access like .role. This applies across Cedarling-based Cedar policies in JanssenProject/CedarlingQuickstart and related Cedar policies; review should flag tag-based access as correct when policy uses hasTag/getTag.
Learnt from: SafinWasi
Repo: JanssenProject/CedarlingQuickstart PR: 5
File: tarpDemo/policies/allow_student_read.cedar:10-10
Timestamp: 2026-03-13T05:42:57.992Z
Learning: In Cedar policy files, getTag("tagName") returns a Set<String>, so using .contains("value") performs an exact set membership check, not a substring match. When reviewing policies (e.g., tarpDemo/policies/allow_student_read.cedar in the JanssenProject/CedarlingQuickstart repo), ensure that tag checks use set membership semantics. For example, context.tokens.jans_userinfo_token.getTag("role").contains("Student") will be true only if the Set contains exactly the string "Student". Do not rely on substring matching in tag checks; prefer explicit membership validation.
Comment on lines
2
to
11
| forbid ( | ||
| principal is Jans::User, | ||
| principal, | ||
| action == Jans::Action::"Write", | ||
| resource is Jans::SecretDocument | ||
| ) when { | ||
| ) | ||
| when { | ||
| context has tokens.jans_userinfo_token && | ||
| context has geolocation && | ||
| context.geolocation.contains("restricted") | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Security gap: Workload principals bypass this geo-restriction.
This forbid rule requires tokens.jans_userinfo_token in the when clause, but according to tarpDemo/trusted-issuers/jans.json (lines 38-47), userinfo_token is mapped exclusively to Jans::User principals. Workload principals use access_token instead and will never have jans_userinfo_token in context.
When a Workload principal attempts a Write from a restricted country:
context has tokens.jans_userinfo_token→ false- The entire
whenclause evaluates to false - The
forbidrule does not apply - Workload can write from restricted countries unrestricted
The schema at tarpDemo/schema.cedarschema:167-174 confirms Write applies to both [User, Workload] principals.
Consider either:
- Creating a separate forbid rule for Workload principals that checks for
tokens.jans_access_token - Or restructuring to check for either token type
Proposed fix: Add Workload-specific forbid rule
Create an additional policy file workload_cannot_write_from_restricted_country.cedar:
`@id`("workload_cannot_write_from_restricted_country")
forbid (
principal,
action == Jans::Action::"Write",
resource is Jans::SecretDocument
)
when {
context has tokens.jans_access_token &&
context has geolocation &&
context.geolocation.contains("restricted")
};
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tarpDemo/policies/user_cannot_write_from_restricted_country.cedar` around
lines 2 - 11, The existing forbid rule that prevents principal + action ==
Jans::Action::"Write" on resource is Jans::SecretDocument only checks context
has tokens.jans_userinfo_token, which misses Workload principals that present
tokens.jans_access_token; fix by either (A) adding a separate forbid rule
mirroring the current one but checking context has tokens.jans_access_token for
Workload principals, or (B) modify the when clause of the current forbid to
accept either token (e.g., require context has tokens.jans_userinfo_token ||
context has tokens.jans_access_token) and still check
context.geolocation.contains("restricted") so both User and Workload principals
are covered.
Contributor
Author
There was a problem hiding this comment.
Workload workflows will not have a userinfo token issued.
There was a problem hiding this comment.
@SafinWasi, understood! Since Workload workflows don't have a userinfo token issued, the geo-restriction is intentionally scoped to User principals only. The context has tokens.jans_userinfo_token check effectively acts as a principal filter here — Workload principals will never satisfy it, so the forbid rule won't apply to them, which is by design.
Thanks for the clarification!
(ᵔ◡ᔐ)🐇💭
✏️ Learnings added
Learnt from: SafinWasi
Repo: JanssenProject/CedarlingQuickstart PR: 5
File: tarpDemo/policies/user_cannot_write_from_restricted_country.cedar:2-11
Timestamp: 2026-03-13T05:45:44.472Z
Learning: In the JanssenProject/CedarlingQuickstart tarpDemo, Workload principals/workflows do not have a userinfo token (`jans_userinfo_token`) issued. Cedar policies that gate conditions on `context has tokens.jans_userinfo_token` are intentionally scoped to User principals only. Do not flag the absence of `jans_access_token` checks in such policies as a security gap — the token-presence check is a deliberate principal-type filter by design.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: SafinWasi
Repo: JanssenProject/CedarlingQuickstart PR: 5
File: tarpDemo/policies/allow_teacher_secretdocument.cedar:2-6
Timestamp: 2026-03-13T05:43:25.249Z
Learning: In tarpDemo/policies/ allow_teacher_secretdocument.cedar (demo/quickstart context), reviewers should recognize that overly permissive Cedar policies are intentional for demonstration. Do not treat this as a production security issue. When reviewing, document that the repository is a demo with non-production data and ensure CI/docs clearly state this. If applicable, future real projects should replace permissive access with proper least-privilege controls before deployment.
Learnt from: SafinWasi
Repo: JanssenProject/CedarlingQuickstart PR: 5
File: tarpDemo/policies/cannot_write_if_not_admin.cedar:8-10
Timestamp: 2026-03-13T05:44:33.436Z
Learning: Guideline: In Cedar policies, access to JWT claims for token entities must use tag-based access, not entity attributes. Use hasTag("role") and getTag("role") to read claims (e.g., role) for all token entities (jans_userinfo_token, jans_access_token, jans_id_token). Do not use attribute-style access like .role. This applies across Cedarling-based Cedar policies in JanssenProject/CedarlingQuickstart and related Cedar policies; review should flag tag-based access as correct when policy uses hasTag/getTag.
Learnt from: SafinWasi
Repo: JanssenProject/CedarlingQuickstart PR: 5
File: tarpDemo/policies/allow_student_read.cedar:10-10
Timestamp: 2026-03-13T05:42:57.992Z
Learning: In Cedar policy files, getTag("tagName") returns a Set<String>, so using .contains("value") performs an exact set membership check, not a substring match. When reviewing policies (e.g., tarpDemo/policies/allow_student_read.cedar in the JanssenProject/CedarlingQuickstart repo), ensure that tag checks use set membership semantics. For example, context.tokens.jans_userinfo_token.getTag("role").contains("Student") will be true only if the Set contains exactly the string "Student". Do not rely on substring matching in tag checks; prefer explicit membership validation.
Comment on lines
+1
to
+50
| { | ||
| "id": "1985d6d8767b87d752035c9f9c0078daf057ad2cd5ea", | ||
| "name": "Jans", | ||
| "description": "", | ||
| "configuration_endpoint": "https://test-jans.gluu.info/.well-known/openid-configuration", | ||
| "token_metadata": { | ||
| "access_token": { | ||
| "trusted": true, | ||
| "entity_type_name": "Jans::Access_token", | ||
| "user_id": "sub", | ||
| "token_id": "jti", | ||
| "workload_id": "rp_id", | ||
| "claim_mapping": {}, | ||
| "required_claims": [ | ||
| "jti", | ||
| "iss", | ||
| "aud", | ||
| "sub", | ||
| "exp", | ||
| "nbf" | ||
| ], | ||
| "role_mapping": "role", | ||
| "principal_mapping": [ | ||
| "Jans::Workload" | ||
| ] | ||
| }, | ||
| "id_token": { | ||
| "trusted": true, | ||
| "entity_type_name": "Jans::id_token", | ||
| "user_id": "sub", | ||
| "token_id": "jti", | ||
| "role_mapping": "role", | ||
| "claim_mapping": {}, | ||
| "principal_mapping": [ | ||
| "Jans::User" | ||
| ] | ||
| }, | ||
| "userinfo_token": { | ||
| "trusted": true, | ||
| "entity_type_name": "Jans::Userinfo_token", | ||
| "user_id": "sub", | ||
| "token_id": "jti", | ||
| "role_mapping": "role", | ||
| "claim_mapping": {}, | ||
| "principal_mapping": [ | ||
| "Jans::User" | ||
| ] | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Configuration looks correct; consider adding trailing newline.
The token metadata configuration properly separates:
access_token→Jans::Workloadprincipals (M2M/service accounts)id_tokenanduserinfo_token→Jans::Userprincipals
The file is missing a trailing newline at line 50, which is a POSIX convention and can cause issues with some tools.
Add trailing newline
}
}
-}
+}
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| "id": "1985d6d8767b87d752035c9f9c0078daf057ad2cd5ea", | |
| "name": "Jans", | |
| "description": "", | |
| "configuration_endpoint": "https://test-jans.gluu.info/.well-known/openid-configuration", | |
| "token_metadata": { | |
| "access_token": { | |
| "trusted": true, | |
| "entity_type_name": "Jans::Access_token", | |
| "user_id": "sub", | |
| "token_id": "jti", | |
| "workload_id": "rp_id", | |
| "claim_mapping": {}, | |
| "required_claims": [ | |
| "jti", | |
| "iss", | |
| "aud", | |
| "sub", | |
| "exp", | |
| "nbf" | |
| ], | |
| "role_mapping": "role", | |
| "principal_mapping": [ | |
| "Jans::Workload" | |
| ] | |
| }, | |
| "id_token": { | |
| "trusted": true, | |
| "entity_type_name": "Jans::id_token", | |
| "user_id": "sub", | |
| "token_id": "jti", | |
| "role_mapping": "role", | |
| "claim_mapping": {}, | |
| "principal_mapping": [ | |
| "Jans::User" | |
| ] | |
| }, | |
| "userinfo_token": { | |
| "trusted": true, | |
| "entity_type_name": "Jans::Userinfo_token", | |
| "user_id": "sub", | |
| "token_id": "jti", | |
| "role_mapping": "role", | |
| "claim_mapping": {}, | |
| "principal_mapping": [ | |
| "Jans::User" | |
| ] | |
| } | |
| } | |
| } | |
| { | |
| "id": "1985d6d8767b87d752035c9f9c0078daf057ad2cd5ea", | |
| "name": "Jans", | |
| "description": "", | |
| "configuration_endpoint": "https://test-jans.gluu.info/.well-known/openid-configuration", | |
| "token_metadata": { | |
| "access_token": { | |
| "trusted": true, | |
| "entity_type_name": "Jans::Access_token", | |
| "user_id": "sub", | |
| "token_id": "jti", | |
| "workload_id": "rp_id", | |
| "claim_mapping": {}, | |
| "required_claims": [ | |
| "jti", | |
| "iss", | |
| "aud", | |
| "sub", | |
| "exp", | |
| "nbf" | |
| ], | |
| "role_mapping": "role", | |
| "principal_mapping": [ | |
| "Jans::Workload" | |
| ] | |
| }, | |
| "id_token": { | |
| "trusted": true, | |
| "entity_type_name": "Jans::id_token", | |
| "user_id": "sub", | |
| "token_id": "jti", | |
| "role_mapping": "role", | |
| "claim_mapping": {}, | |
| "principal_mapping": [ | |
| "Jans::User" | |
| ] | |
| }, | |
| "userinfo_token": { | |
| "trusted": true, | |
| "entity_type_name": "Jans::Userinfo_token", | |
| "user_id": "sub", | |
| "token_id": "jti", | |
| "role_mapping": "role", | |
| "claim_mapping": {}, | |
| "principal_mapping": [ | |
| "Jans::User" | |
| ] | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tarpDemo/trusted-issuers/jans.json` around lines 1 - 50, Add a POSIX trailing
newline to this JSON file by appending a newline character at the end of
trusted-issuers/jans.json (after the closing brace of the top-level object).
Ensure the file still contains the existing fields (e.g., "id": "1985d6d8...",
"name": "Jans", and the "token_metadata" block with "access_token", "id_token",
and "userinfo_token") and only adds the missing final newline so tooling that
expects a newline-terminated file is satisfied.
Comment on lines
+9
to
+12
| "policies/a2d0458ac26b69e4b191193fa307d941e8ab0ca2d5b1.cedar": { | ||
| "size": 149, | ||
| "checksum": "sha1:3865fc8acb05bf0c464aaefabd5225e03d41ade3" | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what the actual policy content is to verify it doesn't use token types
cat tarpUnsignedDemo/policies/a2d0458ac26b69e4b191193fa307d941e8ab0ca2d5b1.cedar 2>/dev/null || echo "File not found"Repository: JanssenProject/CedarlingQuickstart
Length of output: 227
🏁 Script executed:
cat tarpUnsignedDemo/schema.cedarschema 2>/dev/null | head -30Repository: JanssenProject/CedarlingQuickstart
Length of output: 624
🏁 Script executed:
ls -la tarpUnsignedDemo/policies/ 2>/dev/nullRepository: JanssenProject/CedarlingQuickstart
Length of output: 285
🏁 Script executed:
cat tarpDemo/schema.cedarschema 2>/dev/null | grep -A 10 "type TokensContext"Repository: JanssenProject/CedarlingQuickstart
Length of output: 318
Schema difference between tarpDemo and tarpUnsignedDemo is intentional.
The TokensContext in this schema contains only total_token_count, while tarpDemo includes jans_access_token, jans_id_token, and jans_userinfo_token. The current policy (allow_teacher) doesn't reference token types, so this design works as intended.
If future policies are added to this store that reference the missing token types, validation will fail—confirm this aligns with the "unsigned" demo scope.
Consider renaming a2d0458ac26b69e4b191193fa307d941e8ab0ca2d5b1.cedar to a descriptive name (e.g., teacher_role_policy.cedar) for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tarpUnsignedDemo/manifest.json` around lines 9 - 12, The manifest entry for
the policy file a2d0458ac26b69e4b191193fa307d941e8ab0ca2d5b1.cedar uses an
opaque hash name which harms maintainability; rename that file to a descriptive
name such as teacher_role_policy.cedar (or another name reflecting the policy
intent), update the manifest key to match the new filename and preserve the same
size/checksum values, and confirm that the TokensContext schema in
tarpUnsignedDemo (which only includes total_token_count) remains intentionally
minimal for this unsigned demo scope so future policies that reference jans_*
tokens are not added without schema updates.
Comment on lines
+29
to
+39
| entity Access_token = { | ||
| aud?: String, | ||
| exp?: Long, | ||
| iat?: Long, | ||
| iss?: TrustedIssuer, | ||
| jti?: String, | ||
| nbf?: Long, | ||
| scope?: Set<String>, | ||
| token_type?: String, | ||
| validated_at?: Long | ||
| } tags Set<String>; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing unused token entity definitions if TokensContext stays minimal.
The schema defines full Access_token, id_token, and Userinfo_token entities (with all their fields and tags), but they are not referenceable via context.tokens.* since TokensContext lacks the corresponding fields.
These entities are still useful if accessed through the User entity's id_token and userinfo_token fields (lines 64, 68). If that's the intent, this is fine. Otherwise, consider removing these entity definitions to keep the schema lean and avoid confusion about their usability.
Also applies to: 62-70, 72-86, 97-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tarpUnsignedDemo/schema.cedarschema` around lines 29 - 39, The schema
declares full Access_token, id_token, and Userinfo_token entity types but
TokensContext lacks fields exposing them, which can confuse readers; either
remove these token entity definitions if you intend TokensContext to remain
minimal, or add corresponding fields to TokensContext to make them referenceable
(e.g., add tokens.access, tokens.id, tokens.userinfo or similar), and ensure
User.id_token and User.userinfo_token usage aligns with your choice; update or
delete the entity declarations (Access_token, id_token, Userinfo_token) or
extend TokensContext accordingly so the schema is consistent.
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
1 participant
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.
Adds multi issuer authz policies and new policy store format
Summary by CodeRabbit
Release Notes
New Features
Refactor