feat(jans-cedarling): sync test-cases and README of java binding as per new policy st…#13779
Open
feat(jans-cedarling): sync test-cases and README of java binding as per new policy st…#13779
Conversation
…ore format Signed-off-by: duttarnab <arnab.bdutta@gmail.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. |
duttarnab
changed the title
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:
📝 WalkthroughWalkthroughSwitched the Cedarling Java binding from a Post-Authn-based flow to an UpdateToken-based flow: removed PostAuthn sample and legacy policy-store JSONs, added an UpdateToken custom script/class using authorizeUnsigned/authorizeMultiIssuer, and updated tests and fixtures for unsigned and multi-issuer scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
area-documentation
![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: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jans-cedarling/bindings/cedarling-java/docs/bootstrap.json`:
- Around line 2-3: Update the CEDARLING_POLICY_STORE_LOCAL_FN value so it points
to the deployed runtime location (e.g., the static upload path) instead of the
local ./docs file; specifically change the bootstrap JSON entry for
CEDARLING_POLICY_STORE_LOCAL_FN so it references the uploaded path used by the
runtime (for example "custom/static/update_token_script.cjar") so Cedarling can
load the policy store when it reads the bootstrap content.
In
`@jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java`:
- Around line 29-58: The init method(s) currently return true even on failure
and log success despite cedarlingAdapter possibly being null, causing a later
NPE in modifyAccessToken(); update the init(Map<String, SimpleCustomProperty>
configurationAttributes) (and the other init overload around lines 60-89) to
return false when BOOTSTRAP_JSON_PATH is missing or when
CedarlingException/Exception is caught, and only log the "Initialization
successful" message when cedarlingAdapter has been successfully constructed and
loadFromJson completed; ensure cedarlingAdapter is non-null before returning
true so modifyAccessToken() won't dereference a null adapter.
- Around line 142-145: The current code path in the authorized branch
(result.getDecision()) calls context.overwriteAccessTokenScopes(accessToken,
Sets.newHashSet("openid", "profile")), which removes any previously granted
scopes (e.g., email, address, offline_access). Instead, preserve existing scopes
by computing a union of the token’s current scopes and the required scopes
before updating: retrieve the existing scopes from the accessToken (or via the
context helper), combine them with "openid" and "profile" (deduplicate), then
call overwriteAccessTokenScopes with that union (or use an add/apply-scopes
helper if one exists) so previously granted scopes are not dropped.
- Around line 99-106: The sample token hook methods modifyIdToken(Object jwr,
Object tokenContext) and modifyRefreshToken(Object refreshToken, Object
tokenContext) currently return false which vetoes token issuance; change both to
return true to act as safe no-op implementations (matching
ExternalUpdateTokenService's default behavior) so copying this template won't
reject valid tokens. Ensure you update the return values in both method
overrides: modifyIdToken and modifyRefreshToken.
In `@jans-cedarling/bindings/cedarling-java/README.md`:
- Around line 142-156: The ordered list in README.md uses mixed numbering that
triggers MD029/MD045 and the image lacks alt text; update the Steps block
(references: the numbered items and the image line containing
mapping_post_authn_script_with_client.png) to use a consistent ordered-list
style (e.g., make every list item prefixed with "1.") and add descriptive alt
text to the image (e.g., "Mapping post-authn script with client") so the image
markdown becomes .
- Around line 74-85: Insert blank lines before the fenced code block starting
with ```bash and after the closing ``` so the block is separated from
surrounding text, and add a single blank line between the end of that fenced
block and the following heading "#### Schema"; locate the fenced block
containing the policy snippet (lines with `@id`("Allow_authorization_code") and
principal.grantTypes.contains("authorization_code")) and the "#### Schema"
heading and ensure there is an empty line above the ```bash and another empty
line between the ``` closing fence and the "#### Schema" heading.
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java`:
- Around line 186-193: The testTTLExpiry is timing-sensitive because it sleeps a
fixed 1.5s after calling adapter.pushDataCtx("ttlKey", value, 1L); instead use a
bounded poll loop that repeatedly calls adapter.getDataCtx("ttlKey") until it
returns null or a timeout elapses (e.g., 5s max) to avoid flaky scheduling;
update the test method testTTLExpiry to replace Thread.sleep(1500) with a loop
that checks adapter.getDataCtx("ttlKey") at short intervals (e.g., 100-200ms)
and asserts null once expired, failing only after the overall timeout.
- Around line 14-18: The header comment references the wrong test class name;
update the note to mention MultiIssuerAuthzTest (not UnsignedAuthzTest) so the
policy-store hint matches this file. Edit the comment block at the top of
MultiIssuerAuthzTest.java to replace "UnsignedAuthzTest.java" with
"MultiIssuerAuthzTest.java" and confirm the file name/reference in the text and
any example path remains consistent with the MultiIssuerAuthzTest context.
- Around line 44-56: The setup method setUp() is annotated with `@BeforeMethod`
which resets CedarlingAdapter before every test, breaking the dependency chain
where testGetLogIds(), testGetLogsByTag(), and testPopLogs() depend on state
produced by testAuthorizeSuccess(); change the lifecycle so the adapter and
bootstrap are initialized once for the test class (e.g., replace `@BeforeMethod`
with `@BeforeClass` or use a `@BeforeAll-equivalent`) so setUp() (which creates
CedarlingAdapter, loads bootstrapJson, and initializes action/resource) runs
only once and preserves authorization/log state across testAuthorizeSuccess()
and the dependent tests in MultiIssuerAuthzTest; ensure any fields used across
tests remain instance/static as needed and teardown does not clear logs between
dependent tests.
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java`:
- Around line 161-168: The testTTLExpiry uses a fixed Thread.sleep(1500) which
is timing-sensitive; replace the fixed sleep with a polling loop that repeatedly
calls adapter.getDataCtx("ttlKey") until it returns null or a max timeout
elapses (e.g., 3–5s), sleeping briefly between polls (e.g., 50–200ms) and then
assert null or fail if timeout is reached; locate the logic in testTTLExpiry
that calls adapter.pushDataCtx and adapter.getDataCtx and implement the
poll-with-timeout there (or alternatively increase the sleep margin) to make the
assertion robust.
- Around line 31-42: The logging tests currently rely on authorization logs
produced by testAuthorizeWithPrincipalsList() but setUp() reinitializes
CedarlingAdapter for every test; update the logging tests (testGetLogIds,
testGetLogById, testGetLogsByTag, testPopLogs) to invoke authorizeUnsigned(...)
themselves (using the same action/resource/context/principalsString setup) so
they operate on an adapter that actually performed authorization, or
alternatively change setUp()/adapter lifecycle to persist adapter state across
those tests; also make testTTLExpiry() robust by replacing the fixed
Thread.sleep(1500) with a polling loop that checks for expiration up to a
timeout (or increase the sleep margin) to avoid flakiness.
In
`@jans-cedarling/bindings/cedarling-java/src/test/resources/config/multiIssuer/bootstrap.json`:
- Around line 1-10: This bootstrap JSON is missing the
CEDARLING_POLICY_STORE_VALIDATE_CHECKSUM setting which the unsigned config
includes; add the CEDARLING_POLICY_STORE_VALIDATE_CHECKSUM key (set to false to
match the unsigned bootstrap and avoid CJAR checksum failures) alongside the
existing CEDARLING_POLICY_STORE_LOCAL_FN/TestStore_multiIssuer.cjar entry so the
multi-issuer test explicitly disables checksum validation.
🪄 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
Run ID: 45abf31e-a109-4d2c-824e-36a965dc5748
📒 Files selected for processing (37)
jans-cedarling/bindings/cedarling-java/README.mdjans-cedarling/bindings/cedarling-java/docs/action.txtjans-cedarling/bindings/cedarling-java/docs/bootstrap.jsonjans-cedarling/bindings/cedarling-java/docs/policy-store.jsonjans-cedarling/bindings/cedarling-java/docs/principals.jsonjans-cedarling/bindings/cedarling-java/docs/resource.jsonjans-cedarling/bindings/cedarling-java/docs/sample_cedarling_post_authn.javajans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.javajans-cedarling/bindings/cedarling-java/docs/update_token_script.cjarjans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/CedarlingAdapterTest.javajans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.javajans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.javajans-cedarling/bindings/cedarling-java/src/test/resources/config/access_token_payload.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/action.txtjans-cedarling/bindings/cedarling-java/src/test/resources/config/bootstrap-multi-issuer.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/bootstrap.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/id_token_payload.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/multiIssuer/TestStore_multiIssuer.cjarjans-cedarling/bindings/cedarling-java/src/test/resources/config/multiIssuer/access_token.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/multiIssuer/action.txtjans-cedarling/bindings/cedarling-java/src/test/resources/config/multiIssuer/bootstrap.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/multiIssuer/context.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/multiIssuer/id_token.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/multiIssuer/resource.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/multiIssuer/userinfo.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/multi_issuer_access_token_payload.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/multi_issuer_resource.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/policy-store.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/principals.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/resource.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/unsigned/TestStore_unsigned.cjarjans-cedarling/bindings/cedarling-java/src/test/resources/config/unsigned/action.txtjans-cedarling/bindings/cedarling-java/src/test/resources/config/unsigned/bootstrap.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/unsigned/context.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/unsigned/principals.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/unsigned/resource.jsonjans-cedarling/bindings/cedarling-java/src/test/resources/config/user_info_payload.json
💤 Files with no reviewable changes (17)
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/action.txt
- jans-cedarling/bindings/cedarling-java/docs/action.txt
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/principals.json
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/multi_issuer_resource.json
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/resource.json
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/access_token_payload.json
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/multi_issuer_access_token_payload.json
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/user_info_payload.json
- jans-cedarling/bindings/cedarling-java/docs/policy-store.json
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/id_token_payload.json
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/bootstrap.json
- jans-cedarling/bindings/cedarling-java/docs/principals.json
- jans-cedarling/bindings/cedarling-java/docs/resource.json
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/policy-store.json
- jans-cedarling/bindings/cedarling-java/src/test/resources/config/bootstrap-multi-issuer.json
- jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/CedarlingAdapterTest.java
- jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_post_authn.java
Signed-off-by: duttarnab <arnab.bdutta@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java`:
- Line 25: The logger declaration uses the wrong class reference; change the
static Logger initialization (the variable named log) to call
LoggerFactory.getLogger(...) with the current declaring class (the
sample_cedarling_update_token class name used in this file) instead of
CustomScriptManager.class so logs are attributed correctly; locate the log field
declaration and replace the class literal with the actual class symbol for this
file.
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java`:
- Around line 14-18: The file comment in UnsignedAuthzTest.java currently points
to the wrong test-resources directory; update the comment to reference the
unsigned test resources directory (the folder containing
TestStore_unsigned.cjar) instead of the multiIssuer path so it correctly directs
maintainers to the unsigned test resources for UnsignedAuthzTest.
🪄 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
Run ID: 7661effd-8f6d-48fc-9fec-7a24d3369824
📒 Files selected for processing (6)
jans-cedarling/bindings/cedarling-java/README.mdjans-cedarling/bindings/cedarling-java/docs/bootstrap.jsonjans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.javajans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.javajans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.javajans-cedarling/bindings/cedarling-java/src/test/resources/config/unsigned/bootstrap.json
Signed-off-by: duttarnab <arnab.bdutta@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java (2)
103-111:⚠️ Potential issue | 🟠 MajorNo-op token hooks should not veto token issuance.
modifyIdToken(...)andmodifyRefreshToken(...)returnfalse, which can reject otherwise valid flows when this sample is copied as-is.Proposed fix
`@Override` public boolean modifyIdToken(Object jwr, Object tokenContext) { - return false; + return true; } `@Override` public boolean modifyRefreshToken(Object refreshToken, Object tokenContext) { - return false; + return true; }#!/bin/bash set -euo pipefail sample="$(fd "sample_cedarling_update_token.java" --type f | head -n1)" service="$(fd "ExternalUpdateTokenService.java" --type f | head -n1)" echo "---- Sample hook return values ----" nl -ba "$sample" | sed -n '103,111p' echo "---- Framework handling/defaults ----" rg -n "modifyIdToken|modifyRefreshToken|return true|return false" "$service" -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java` around lines 103 - 111, The sample no-op token hooks currently return false in modifyIdToken(Object jwr, Object tokenContext) and modifyRefreshToken(Object refreshToken, Object tokenContext), which can veto token issuance when the sample is reused; update both methods to return true (or otherwise indicate success) so they do not reject valid flows by default—locate the two methods in sample_cedarling_update_token.java and change their return values from false to true (or call/propagate the framework's success result if the implementation should delegate).
146-149:⚠️ Potential issue | 🟠 MajorScope overwrite drops already granted scopes.
Replacing scopes with only
openid/profilecan unintentionally remove existing permissions on authorized flows.#!/bin/bash set -euo pipefail sample="$(fd "sample_cedarling_update_token.java" --type f | head -n1)" access="$(fd "AccessToken.java" --type f | head -n1)" ctx="$(fd "ExternalUpdateTokenContext.java" --type f | head -n1)" echo "---- Current overwrite call ----" nl -ba "$sample" | sed -n '146,149p' echo "---- AccessToken scope-related API ----" rg -n "scope|get.*Scope|set.*Scope" "$access" -C2 echo "---- ExternalUpdateTokenContext scope helpers ----" rg -n "scope|overwriteAccessTokenScopes|add.*Scope|get.*Scope" "$ctx" -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java` around lines 146 - 149, The current code calls context.overwriteAccessTokenScopes(accessToken, Sets.newHashSet("openid", "profile")) which replaces any previously granted scopes; instead retrieve the token's existing scopes (e.g., accessToken.getScope() or similar), compute the union with the desired scopes ("openid","profile"), and then apply the merged set—preferably by calling context.addAccessTokenScopes(accessToken, mergedSet) if available, or by passing the mergedSet to overwriteAccessTokenScopes; update the code in the block that checks result.getDecision() to merge rather than unconditionally overwrite.jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java (2)
167-173:⚠️ Potential issue | 🟡 MinorReplace fixed sleep in TTL test with poll-until-timeout.
Thread.sleep(1500)remains timing-sensitive and can make this test flaky under jitter/slow CI.Proposed stabilization
`@Test` public void testTTLExpiry() throws Exception { adapter.pushDataCtx("ttlKey", new JSONObject().put("data", "temp"), 1L); - - Thread.sleep(1500); - - assertNull(adapter.getDataCtx("ttlKey"), "Data should expire after TTL"); + long deadline = System.currentTimeMillis() + 5000L; + while (System.currentTimeMillis() < deadline && adapter.getDataCtx("ttlKey") != null) { + Thread.sleep(100); + } + assertNull(adapter.getDataCtx("ttlKey"), "Data should expire after TTL"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java` around lines 167 - 173, In testTTLExpiry replace the fixed Thread.sleep(1500) with a poll-until-timeout loop that repeatedly calls adapter.getDataCtx("ttlKey") until it returns null or a max timeout elapses (e.g., 2-3 seconds) to avoid timing flakiness; keep the initial adapter.pushDataCtx("ttlKey", new JSONObject().put("data", "temp"), 1L) and assert that getDataCtx returns null after the poll finishes (fail the test if timeout reached), using the testTTLExpiry method and adapter.getDataCtx/adapter.pushDataCtx identifiers to locate the code.
36-47:⚠️ Potential issue | 🟠 MajorLogging tests still don’t exercise logs from
authorizeUnsigned(...).Because
@BeforeMethodrecreatesadapterfor each test, the logging tests run on fresh state even withdependsOnMethods, so they validate generic startup logs rather than auth-call logs.#!/bin/bash set -euo pipefail f="$(fd "UnsignedAuthzTest.java" --type f | head -n1)" echo "Inspecting: $f" echo "---- Setup lifecycle ----" nl -ba "$f" | sed -n '36,47p' echo "---- Logging tests block ----" nl -ba "$f" | sed -n '109,135p' echo "---- authorizeUnsigned call sites in file ----" rg -n "authorizeUnsigned\\(" "$f"Also applies to: 109-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java` around lines 36 - 47, The tests recreate adapter in setUp(), so the logging assertions never see logs emitted by authorizeUnsigned(...) because each test gets a fresh CedarlingAdapter; to fix, initialize and load the adapter once for the test class (move adapter creation and adapter.loadFromJson(...) out of setUp() into a `@BeforeClass` or a method that only runs once) or guard setUp() to skip reinitialization when adapter is non-null, and ensure the test that calls authorizeUnsigned(...) runs before the logging assertions (or remove dependsOnMethods reliance) so the logging tests (the block around the existing logging tests) observe logs produced by authorizeUnsigned; refer to UnsignedAuthzTest, setUp, adapter, and authorizeUnsigned when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java`:
- Around line 29-60: Both init overloads duplicate bootstrap validation and
Cedarling initialization; extract that shared logic into a single private helper
(e.g., private boolean initBootstrap(Map<String, SimpleCustomProperty> config)
or private boolean initBootstrap(String bootstrapFilePath)) and have both init
methods call it. Move the check for "BOOTSTRAP_JSON_PATH", the
readFile(bootstrapFilePath) call, creation of new CedarlingAdapter(),
cedarlingAdapter.loadFromJson(bootstrapJson), and the
CedarlingException/Exception handling and logging into that helper so
init(Map<String, SimpleCustomProperty>) and the other init overload simply
delegate and return its boolean result.
---
Duplicate comments:
In
`@jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java`:
- Around line 103-111: The sample no-op token hooks currently return false in
modifyIdToken(Object jwr, Object tokenContext) and modifyRefreshToken(Object
refreshToken, Object tokenContext), which can veto token issuance when the
sample is reused; update both methods to return true (or otherwise indicate
success) so they do not reject valid flows by default—locate the two methods in
sample_cedarling_update_token.java and change their return values from false to
true (or call/propagate the framework's success result if the implementation
should delegate).
- Around line 146-149: The current code calls
context.overwriteAccessTokenScopes(accessToken, Sets.newHashSet("openid",
"profile")) which replaces any previously granted scopes; instead retrieve the
token's existing scopes (e.g., accessToken.getScope() or similar), compute the
union with the desired scopes ("openid","profile"), and then apply the merged
set—preferably by calling context.addAccessTokenScopes(accessToken, mergedSet)
if available, or by passing the mergedSet to overwriteAccessTokenScopes; update
the code in the block that checks result.getDecision() to merge rather than
unconditionally overwrite.
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java`:
- Around line 167-173: In testTTLExpiry replace the fixed Thread.sleep(1500)
with a poll-until-timeout loop that repeatedly calls
adapter.getDataCtx("ttlKey") until it returns null or a max timeout elapses
(e.g., 2-3 seconds) to avoid timing flakiness; keep the initial
adapter.pushDataCtx("ttlKey", new JSONObject().put("data", "temp"), 1L) and
assert that getDataCtx returns null after the poll finishes (fail the test if
timeout reached), using the testTTLExpiry method and
adapter.getDataCtx/adapter.pushDataCtx identifiers to locate the code.
- Around line 36-47: The tests recreate adapter in setUp(), so the logging
assertions never see logs emitted by authorizeUnsigned(...) because each test
gets a fresh CedarlingAdapter; to fix, initialize and load the adapter once for
the test class (move adapter creation and adapter.loadFromJson(...) out of
setUp() into a `@BeforeClass` or a method that only runs once) or guard setUp() to
skip reinitialization when adapter is non-null, and ensure the test that calls
authorizeUnsigned(...) runs before the logging assertions (or remove
dependsOnMethods reliance) so the logging tests (the block around the existing
logging tests) observe logs produced by authorizeUnsigned; refer to
UnsignedAuthzTest, setUp, adapter, and authorizeUnsigned when applying the
change.
🪄 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
Run ID: cc1c4902-fe14-45a0-bf70-1d2c7086f1e4
📒 Files selected for processing (2)
jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.javajans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java
Signed-off-by: duttarnab <arnab.bdutta@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java (2)
186-193:⚠️ Potential issue | 🟡 MinorReplace fixed sleep with bounded polling in the TTL test to avoid flakiness.
Line 190 uses a fixed
Thread.sleep(1500)for a 1-second TTL, which is timing-sensitive under scheduler/load variance.🔧 Proposed stabilization
@@ - Thread.sleep(1500); - - assertNull(adapter.getDataCtx("ttlKey"), "Data should expire after TTL"); + long deadline = System.currentTimeMillis() + 5000; // 5s max wait + while (System.currentTimeMillis() < deadline) { + if (adapter.getDataCtx("ttlKey") == null) { + return; + } + Thread.sleep(150); + } + fail("Data should expire after TTL within timeout");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java` around lines 186 - 193, In testTTLExpiry replace the fixed Thread.sleep(1500) with a bounded polling loop that repeatedly calls adapter.getDataCtx("ttlKey") until it returns null or a short timeout elapses (e.g., 2-3 seconds) to avoid flakiness; keep using adapter.pushDataCtx("ttlKey", value, 1L) to set TTL, poll at small intervals (e.g., 50-100ms) inside testTTLExpiry, and fail the test if the timeout is reached while the value is still non-null.
43-55:⚠️ Potential issue | 🟠 MajorPreserve log-state intent; current lifecycle resets the adapter before dependent logging tests.
@BeforeMethodon Line 43 recreatesadapterfor every test, so tests on Lines 143, 149, and 155 do not observe the logs produced bytestAuthorizeSuccess(). They run against a fresh instance.🔧 Proposed fix (keep tests independent and deterministic)
@@ - `@Test`(dependsOnMethods = "testAuthorizeSuccess") - public void testGetLogIds() { + private void authorizeOnceForLogSetup() throws Exception { + adapter.authorizeMultiIssuer(createValidTokens(), action, resource, emptyContext()); + } + + `@Test` + public void testGetLogIds() throws Exception { + authorizeOnceForLogSetup(); List<String> logEntries = adapter.getLogIds(); assertFalse(logEntries.isEmpty(), "Log IDs should not be empty"); } - `@Test`(dependsOnMethods = "testAuthorizeSuccess") + `@Test` public void testGetLogsByTag() throws LogException { + authorizeOnceForLogSetup(); assertFalse(adapter.getLogsByTag("System").isEmpty(), "Logs for 'System' tag should not be empty"); } - `@Test`(dependsOnMethods = "testAuthorizeSuccess") + `@Test` public void testPopLogs() throws LogException { + authorizeOnceForLogSetup(); List<String> logEntries = adapter.popLogs(); assertFalse(logEntries.isEmpty(), "Popped logs should not be empty"); }In TestNG, does `@BeforeMethod` run before every invoked `@Test` method, including tests that use dependsOnMethods?Also applies to: 143-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java` around lines 43 - 55, The setUp() method reinitializes adapter for every test (it’s annotated `@BeforeMethod`), which clears logs needed by tests that depend on testAuthorizeSuccess(); change test fixture initialization so adapter is created once and not reset between tests—move adapter/JWTCreator/bootstrapJson initialization and adapter.loadFromJson(...) out of setUp() into a one-time setup annotated `@BeforeClass` (or create a new `@BeforeClass` method that calls an initAdapter() helper) and leave per-test setup only for truly test-local state; ensure the existing setUp() no longer recreates adapter so testAuthorizeSuccess()’s logs remain available to dependent tests.jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java (2)
103-110:⚠️ Potential issue | 🟠 MajorReturn
truefrom untouched ID/refresh-token hooks.
falseis not a neutral stub here; if these callbacks run, they veto ID/refresh-token creation. For a sample that does not modify those tokens, both methods should succeed and leave the token unchanged.🔧 Suggested fix
`@Override` public boolean modifyIdToken(Object jwr, Object tokenContext) { - return false; + return true; } `@Override` public boolean modifyRefreshToken(Object refreshToken, Object tokenContext) { - return false; + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java` around lines 103 - 110, The modifyIdToken and modifyRefreshToken hooks currently return false which vetoes token creation; update both methods (modifyIdToken and modifyRefreshToken) to return true when you do not modify the token so the hooks succeed and leave the token unchanged (i.e., replace the false return values with true in those methods).
146-149:⚠️ Potential issue | 🟠 MajorDo not drop previously granted scopes.
overwriteAccessTokenScopes(...)replaces the token's entire scope set withopenidandprofile. That strips scopes granted earlier in the flow, such asoffline_access. Merge the required scopes with the existing set before updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java` around lines 146 - 149, The code currently calls context.overwriteAccessTokenScopes(accessToken, Sets.newHashSet("openid", "profile")) which replaces all previously granted scopes; instead, read the token's current scope set (from accessToken or via context/getter), create a union with the required scopes {"openid","profile"}, and pass that merged set to context.overwriteAccessTokenScopes(accessToken, mergedSet) so existing scopes like "email" or "offline_access" are preserved; update the logic around result.getDecision() to perform this merge before calling overwriteAccessTokenScopes.jans-cedarling/bindings/cedarling-java/README.md (1)
145-159:⚠️ Potential issue | 🟡 MinorClean up the list/image lint errors in this steps block.
Add alt text to the screenshot and use the repository's ordered-list style (
1.prefixes) instead of5.on Line 159.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/README.md` around lines 145 - 159, Update the Steps block in README.md to use the repository's preferred ordered-list style by changing the numeric prefixes so every list item uses "1." instead of "5." for the last item, and add meaningful alt text to the embedded screenshot (replace the empty alt in ./docs/mapping_update_token_script_with_client.png with something like "Mapping update token script with client UI") so the image has accessible alt text; touch the BOOTSTRAP_JSON_PATH table entry only as needed to retain current key/value formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java`:
- Around line 119-157: Replace using the client's configured grants with the
actual request grant: instead of iterating context.getClient().getGrantTypes(),
use context.getGrant().getGrantType() (or its string value) when building the
principal/resource JSON via buildPrincipalJson and buildResourceJson so
Cedarling receives the actual request grant type; call
cedarlingAdapter.authorizeUnsigned with those updated entities and, if the
decision is false (no match), return true to skip modifying the access token
rather than returning false which forbids token creation; keep existing
exception handling around AuthorizeException/EntityException and only return
false on exceptions.
In `@jans-cedarling/bindings/cedarling-java/README.md`:
- Around line 72-88: The README.md uses h5 headings ("##### Policies" and "#####
Schema") which cause a level jump from the enclosing h3 and trigger MD001;
update these headings to h4 (change "##### Policies" to "#### Policies" and
"##### Schema" to "#### Schema") so heading levels are consistent and the
markdown linter passes.
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java`:
- Around line 101-102: In MultiIssuerAuthzTest (the test class), remove the two
stale commented-out assertions "//assertNotNull(result, "Result should not be
null");" and "//assertFalse(result.getDecision(), "Authorization should DENY for
invalid token");" from the exception-path test so the test relies solely on the
existing expectedExceptions behavior (clean up the commented lines in
MultiIssuerAuthzTest.java).
---
Duplicate comments:
In
`@jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.java`:
- Around line 103-110: The modifyIdToken and modifyRefreshToken hooks currently
return false which vetoes token creation; update both methods (modifyIdToken and
modifyRefreshToken) to return true when you do not modify the token so the hooks
succeed and leave the token unchanged (i.e., replace the false return values
with true in those methods).
- Around line 146-149: The code currently calls
context.overwriteAccessTokenScopes(accessToken, Sets.newHashSet("openid",
"profile")) which replaces all previously granted scopes; instead, read the
token's current scope set (from accessToken or via context/getter), create a
union with the required scopes {"openid","profile"}, and pass that merged set to
context.overwriteAccessTokenScopes(accessToken, mergedSet) so existing scopes
like "email" or "offline_access" are preserved; update the logic around
result.getDecision() to perform this merge before calling
overwriteAccessTokenScopes.
In `@jans-cedarling/bindings/cedarling-java/README.md`:
- Around line 145-159: Update the Steps block in README.md to use the
repository's preferred ordered-list style by changing the numeric prefixes so
every list item uses "1." instead of "5." for the last item, and add meaningful
alt text to the embedded screenshot (replace the empty alt in
./docs/mapping_update_token_script_with_client.png with something like "Mapping
update token script with client UI") so the image has accessible alt text; touch
the BOOTSTRAP_JSON_PATH table entry only as needed to retain current key/value
formatting.
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java`:
- Around line 186-193: In testTTLExpiry replace the fixed Thread.sleep(1500)
with a bounded polling loop that repeatedly calls adapter.getDataCtx("ttlKey")
until it returns null or a short timeout elapses (e.g., 2-3 seconds) to avoid
flakiness; keep using adapter.pushDataCtx("ttlKey", value, 1L) to set TTL, poll
at small intervals (e.g., 50-100ms) inside testTTLExpiry, and fail the test if
the timeout is reached while the value is still non-null.
- Around line 43-55: The setUp() method reinitializes adapter for every test
(it’s annotated `@BeforeMethod`), which clears logs needed by tests that depend on
testAuthorizeSuccess(); change test fixture initialization so adapter is created
once and not reset between tests—move adapter/JWTCreator/bootstrapJson
initialization and adapter.loadFromJson(...) out of setUp() into a one-time
setup annotated `@BeforeClass` (or create a new `@BeforeClass` method that calls an
initAdapter() helper) and leave per-test setup only for truly test-local state;
ensure the existing setUp() no longer recreates adapter so
testAuthorizeSuccess()’s logs remain available to dependent tests.
🪄 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
Run ID: fbcbe18e-0d4b-46c0-b267-aa6e12873c0d
⛔ Files ignored due to path filters (1)
jans-cedarling/bindings/cedarling-java/docs/mapping_update_token_script_with_client.pngis excluded by!**/*.png
📒 Files selected for processing (3)
jans-cedarling/bindings/cedarling-java/README.mdjans-cedarling/bindings/cedarling-java/docs/sample_cedarling_update_token.javajans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java
Signed-off-by: duttarnab <arnab.bdutta@gmail.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java (2)
164-170:⚠️ Potential issue | 🟡 MinorTTL test remains flaky with fixed sleep.
A fixed 1.5s delay for 1s TTL can intermittently fail. Prefer bounded polling with timeout.
⏱️ Suggested fix (poll until expired or timeout)
`@Test` public void testTTLExpiry() throws Exception { adapter.pushDataCtx("ttlKey", new JSONObject().put("data", "temp"), 1L); - - Thread.sleep(1500); - - assertNull(adapter.getDataCtx("ttlKey"), "Data should expire after TTL"); + long deadline = System.currentTimeMillis() + 5000L; + while (System.currentTimeMillis() < deadline && adapter.getDataCtx("ttlKey") != null) { + Thread.sleep(100L); + } + assertNull(adapter.getDataCtx("ttlKey"), "Data should expire after TTL"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java` around lines 164 - 170, The testTTLExpiry currently uses a fixed Thread.sleep(1500) which is flaky; replace the fixed sleep with bounded polling: repeatedly call adapter.getDataCtx("ttlKey") in a loop (sleeping short intervals, e.g., 50-200ms) until it returns null or a max timeout elapses (e.g., 5s), and then assert that the result is null (or fail if timeout reached). Update the testTTLExpiry method to use this polling pattern around adapter.getDataCtx and keep the original adapter.pushDataCtx("ttlKey", new JSONObject().put("data", "temp"), 1L) call.
36-47:⚠️ Potential issue | 🟠 MajorDependent logging tests are not asserting authorization-generated logs under current lifecycle.
@BeforeMethodrebuildsadapterbefore each test, so logging tests run on a fresh instance and can pass without proving logs came fromauthorizeUnsigned(...).testGetLogByIdalso has a silent-pass branch when no logs exist.💡 Suggested fix (seed logs per logging test + avoid silent pass)
+ private void seedAuthorizationLogs() throws Exception { + adapter.authorizeUnsigned(principalsString, action, resource, context); + } - `@Test`(dependsOnMethods = "testAuthorizeWithPrincipalsList") - public void testGetLogIds() { + `@Test` + public void testGetLogIds() throws Exception { + seedAuthorizationLogs(); List<String> logEntries = adapter.getLogIds(); assertFalse(logEntries.isEmpty(), "Log IDs should not be empty"); } - `@Test`(dependsOnMethods = "testAuthorizeWithPrincipalsList") + `@Test` public void testGetLogById() throws Exception { + seedAuthorizationLogs(); List<String> logEntries = adapter.getLogIds(); - - if (!logEntries.isEmpty()) { - String log = adapter.getLogById(logEntries.get(0)); - assertNotNull(log, "Log entry should not be null"); - } + assertFalse(logEntries.isEmpty(), "Log IDs should not be empty"); + String log = adapter.getLogById(logEntries.get(0)); + assertNotNull(log, "Log entry should not be null"); } - `@Test`(dependsOnMethods = "testAuthorizeWithPrincipalsList") - public void testGetLogsByTag() throws LogException { + `@Test` + public void testGetLogsByTag() throws Exception { + seedAuthorizationLogs(); List<String> logs = adapter.getLogsByTag("System"); assertFalse(logs.isEmpty(), "Logs by tag should not be empty"); } - `@Test`(dependsOnMethods = "testAuthorizeWithPrincipalsList") - public void testPopLogs() throws LogException { + `@Test` + public void testPopLogs() throws Exception { + seedAuthorizationLogs(); List<String> logs = adapter.popLogs(); assertFalse(logs.isEmpty(), "Popped logs should not be empty"); }Also applies to: 106-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java` around lines 36 - 47, Tests that assert authorization-generated logs are running against a freshly re-created adapter in setUp(), so they can pass without exercising authorizeUnsigned(...) and testGetLogById silently passes when no logs exist; update the logging-related tests to first seed logs by calling CedarlingAdapter.authorizeUnsigned(...) (or another real authorization path) within the test (or move adapter initialization to a `@BeforeClass` for those cases) so logs are actually produced, and change testGetLogById to assert failure when no logs are returned (remove the silent-pass branch) to ensure the test fails if logs are not present.jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java (2)
183-189:⚠️ Potential issue | 🟡 MinorTTL assertion is timing-sensitive due to fixed sleep.
A fixed
Thread.sleep(1500)against 1-second TTL is still flaky under scheduling jitter. Use bounded polling.⏱️ Suggested fix (poll with timeout)
`@Test` public void testTTLExpiry() throws Exception { JSONObject value = new JSONObject().put("data", "temp"); adapter.pushDataCtx("ttlKey", value, 1L); - - Thread.sleep(1500); - - assertNull(adapter.getDataCtx("ttlKey"), "Data should expire after TTL"); + long deadline = System.currentTimeMillis() + 5000L; + while (System.currentTimeMillis() < deadline && adapter.getDataCtx("ttlKey") != null) { + Thread.sleep(100L); + } + assertNull(adapter.getDataCtx("ttlKey"), "Data should expire after TTL"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java` around lines 183 - 189, The testTTLExpiry is flaky because it uses a fixed Thread.sleep(1500) after adapter.pushDataCtx("ttlKey", value, 1L); instead of polling; change the test to poll adapter.getDataCtx("ttlKey") with a short sleep (e.g., 50–200ms) in a loop until it returns null or until a reasonable timeout (e.g., 2–3 seconds) is reached, then assert that the value is null; update references in the test method testTTLExpiry and calls to adapter.pushDataCtx and adapter.getDataCtx accordingly so the test waits deterministically for TTL expiry instead of using a single fixed sleep.
43-55:⚠️ Potential issue | 🟠 MajorLogging tests currently validate fresh-adapter logs, not logs produced by
testAuthorizeSuccess.With
@BeforeMethod, each logging test gets a newCedarlingAdapter;dependsOnMethodsonly orders execution and does not preserve instance state between tests. These assertions can pass without validating post-authorization logging behavior.💡 Suggested fix (make each logging test seed its own authorization)
+ private void seedAuthorizationLogs() throws Exception { + adapter.authorizeMultiIssuer(createValidTokens(), action, resource, emptyContext()); + } - `@Test`(dependsOnMethods = "testAuthorizeSuccess") - public void testGetLogIds() { + `@Test` + public void testGetLogIds() throws Exception { + seedAuthorizationLogs(); List<String> logEntries = adapter.getLogIds(); assertFalse(logEntries.isEmpty(), "Log IDs should not be empty"); } - `@Test`(dependsOnMethods = "testAuthorizeSuccess") - public void testGetLogsByTag() throws LogException { + `@Test` + public void testGetLogsByTag() throws Exception { + seedAuthorizationLogs(); assertFalse(adapter.getLogsByTag("System").isEmpty(), "Logs for 'System' tag should not be empty"); } - `@Test`(dependsOnMethods = "testAuthorizeSuccess") - public void testPopLogs() throws LogException { + `@Test` + public void testPopLogs() throws Exception { + seedAuthorizationLogs(); List<String> logEntries = adapter.popLogs(); assertFalse(logEntries.isEmpty(), "Popped logs should not be empty"); }Also applies to: 140-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java` around lines 43 - 55, The logging tests in MultiIssuerAuthzTest rely on a fresh adapter from setUp and therefore never validate logs produced by testAuthorizeSuccess; fix each logging test (the methods that assert log output referenced near the setUp block and again near lines 140-156) to explicitly perform the authorization step they expect to validate rather than relying on test ordering — e.g., call the same authorization flow used in testAuthorizeSuccess (invoke CedarlingAdapter.authorize or the specific method you use to run the authorization with action and resource and capture logs) at the start of each logging test so the adapter generates the logs being asserted; keep setUp only for common initialization (adapter, jwtCreator, action, resource) and remove any implicit dependency on testAuthorizeSuccess.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.java`:
- Around line 183-189: The testTTLExpiry is flaky because it uses a fixed
Thread.sleep(1500) after adapter.pushDataCtx("ttlKey", value, 1L); instead of
polling; change the test to poll adapter.getDataCtx("ttlKey") with a short sleep
(e.g., 50–200ms) in a loop until it returns null or until a reasonable timeout
(e.g., 2–3 seconds) is reached, then assert that the value is null; update
references in the test method testTTLExpiry and calls to adapter.pushDataCtx and
adapter.getDataCtx accordingly so the test waits deterministically for TTL
expiry instead of using a single fixed sleep.
- Around line 43-55: The logging tests in MultiIssuerAuthzTest rely on a fresh
adapter from setUp and therefore never validate logs produced by
testAuthorizeSuccess; fix each logging test (the methods that assert log output
referenced near the setUp block and again near lines 140-156) to explicitly
perform the authorization step they expect to validate rather than relying on
test ordering — e.g., call the same authorization flow used in
testAuthorizeSuccess (invoke CedarlingAdapter.authorize or the specific method
you use to run the authorization with action and resource and capture logs) at
the start of each logging test so the adapter generates the logs being asserted;
keep setUp only for common initialization (adapter, jwtCreator, action,
resource) and remove any implicit dependency on testAuthorizeSuccess.
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java`:
- Around line 164-170: The testTTLExpiry currently uses a fixed
Thread.sleep(1500) which is flaky; replace the fixed sleep with bounded polling:
repeatedly call adapter.getDataCtx("ttlKey") in a loop (sleeping short
intervals, e.g., 50-200ms) until it returns null or a max timeout elapses (e.g.,
5s), and then assert that the result is null (or fail if timeout reached).
Update the testTTLExpiry method to use this polling pattern around
adapter.getDataCtx and keep the original adapter.pushDataCtx("ttlKey", new
JSONObject().put("data", "temp"), 1L) call.
- Around line 36-47: Tests that assert authorization-generated logs are running
against a freshly re-created adapter in setUp(), so they can pass without
exercising authorizeUnsigned(...) and testGetLogById silently passes when no
logs exist; update the logging-related tests to first seed logs by calling
CedarlingAdapter.authorizeUnsigned(...) (or another real authorization path)
within the test (or move adapter initialization to a `@BeforeClass` for those
cases) so logs are actually produced, and change testGetLogById to assert
failure when no logs are returned (remove the silent-pass branch) to ensure the
test fails if logs are not present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7402536e-1f05-4335-bb33-13dc7d8368e7
📒 Files selected for processing (2)
jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/MultiIssuerAuthzTest.javajans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/UnsignedAuthzTest.java
Signed-off-by: duttarnab <arnab.bdutta@gmail.com>
Signed-off-by: duttarnab <arnab.bdutta@gmail.com>
duttarnab
requested review from
SafinWasi,
haileyesus2433,
jgomer2001 and
olehbozhok
jgomer2001
reviewed
Apr 15, 2026
olehbozhok
previously approved these changes
Apr 15, 2026
SafinWasi
requested changes
Apr 16, 2026
Contributor
SafinWasi
left a comment
SafinWasi
left a comment
There was a problem hiding this comment.
Overall looks good to me, though cjar file should not be checked into version control. Use folder format instead.
Contributor
There was a problem hiding this comment.
I don't think it's a good idea to check in a cjar file directly to version control. Maybe use folder-based policy store instead?
Contributor
Author
There was a problem hiding this comment.
Getting following error when using folder-based policy
uniffi.cedarling_uniffi.CedarlingException$InitializationFailed: errorMsg=Unsupported policy store file format for: ./resources/config/multiIssuer/policy-store. Supported formats include: JSON, YAML
haileyesus2433
previously approved these changes
Apr 16, 2026
Signed-off-by: duttarnab <arnab.bdutta@gmail.com>
duttarnab
dismissed stale reviews from haileyesus2433 and olehbozhok
via
ea2115a
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
6 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.
…ore format
Prepare
Description
Target issue
closes #13778
Implementation Details
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
New Features
Documentation
Tests
Chores