fix(jans-cedarling): cedarling adapter should provide API with generic data types #13629
fix(jans-cedarling): cedarling adapter should provide API with generic data types #13629
Conversation
… methods Add new methods to the CedarlingAdapter for improved authorization handling: - `authorizeMultiIssuer(Map<String, String>, String, JSONObject, JSONObject)` for multi-issuer JWT authorization. - `authorizeUnsigned(String, String, JSONObject, JSONObject)` for single principal authorization using JSON strings. - `authorizeUnsignedFromJson(List<String>, String, JSONObject, JSONObject)` for multiple principals from JSON strings. - Overloaded `authorizeUnsigned(List<EntityData>, String, JSONObject, JSONObject)` for direct EntityData usage. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
…d files Introduce new JSON configuration files for multi-issuer support: - `bootstrap-multi-issuer.json`: Contains application settings and policy store configurations. - `multi_issuer_access_token_payload.json`: Defines the structure of access tokens for multi-issuer scenarios. - `multi_issuer_resource.json`: Maps entities and includes organizational information. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
…authorization tests Add new test cases to CedarlingAdapterTest for multi-issuer authorization scenarios: - `testAuthorizeUnsignedFromJsonList`: Tests authorization with multiple principals from a JSON list. - `testAuthorizeUnsignedWithSingleJsonString`: Tests authorization with a single principal as a JSON string. - `testAuthorizeMultiIssuerWithMapConversion`: Validates multi-issuer authorization using a map of tokens. Also, introduce new constants for multi-issuer configuration file paths. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
…lti-issuer authorization Enhance the sample post-authentication script to demonstrate the new multi-issuer authorization method. Key changes include: - Introduction of JWT-based authorization using `authorizeMultiIssuer` with token mapping. - Updated initialization logic to handle new configuration requirements. - Improved error handling and logging for Cedarling initialization. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
✅ 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. |
📝 WalkthroughWalkthroughReplace v16 token/principals flows with Cedarling v17 multi‑issuer JWT support: add Map/JSON overloads in CedarlingAdapter, update sample to post‑authn JWT flow using authorizeMultiIssuer, and add tests + fixtures for multi‑issuer scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/src/main/java/io/jans/cedarling/binding/wrapper/CedarlingAdapter.java`:
- Around line 143-151: The authorizeUnsigned method currently calls
context.toString() without a null check which can throw NPE; update the call
that passes the context to cedarling.authorizeUnsigned to mirror
authorizeMultiIssuer's null-safe pattern (i.e., compute a String ctx = (context
== null ? null : context.toString()) and pass that instead), touching the
authorizeUnsigned method and the cedarling.authorizeUnsigned invocation to
ensure null contexts are handled safely.
- Around line 59-70: Add a null-safety guard at the start of the
authorizeMultiIssuer(Map<String,String> tokens, String action, JSONObject
resource, JSONObject context) method: check if tokens is null and throw an
IllegalArgumentException (or AuthorizeException if preferred) with a clear
message like "tokens must not be null" before constructing TokenInput instances,
so TokenInput creation and the for loop are never executed with a null map.
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/CedarlingAdapterTest.java`:
- Around line 96-121: The test creates a CedarlingAdapter (multiIssuerAdapter)
in testAuthorizeMultiIssuerWithMapConversion but never closes it; wrap the
adapter instantiation in a try-with-resources (or explicitly call close()) so
CedarlingAdapter (which implements AutoCloseable) is closed after use to prevent
resource leaks; modify the testAuthorizeMultiIssuerWithMapConversion method to
construct multiIssuerAdapter in a try (...) { ... } block that contains the
existing test logic.
🪄 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: bd0dd163-24cb-45a8-8562-46ef109b97c7
📒 Files selected for processing (6)
jans-cedarling/bindings/cedarling-java/docs/sample_cedarling_post_authn.javajans-cedarling/bindings/cedarling-java/src/main/java/io/jans/cedarling/binding/wrapper/CedarlingAdapter.javajans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/CedarlingAdapterTest.javajans-cedarling/bindings/cedarling-java/src/test/resources/config/bootstrap-multi-issuer.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.json
…ingAdapter - Added a null check for the `tokens` parameter in the `authorizeMultiIssuer` method to prevent IllegalArgumentException. - Updated the `authorizeUnsigned` method to handle null `context` values gracefully by converting them to a string. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
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/src/main/java/io/jans/cedarling/binding/wrapper/CedarlingAdapter.java`:
- Around line 68-71: In CedarlingAdapter, validate inputs in the generic wrapper
overloads before looping or calling the UniFFI parser: check that the
Map<String,String> tokens is non-null and that no key or value in tokens is null
(throw IllegalArgumentException with a clear message), and likewise validate
principalJson is non-null and each element of principalsJson (if present) is
non-null; perform these null checks in the same methods where TokenInput is
constructed (referencing TokenInput and the tokens variable) and in the other
overloads that handle principalJson/principalsJson (the methods covering the
104-137 range), and fail fast with IllegalArgumentException rather than letting
an NPE surface from the UniFFI parser.
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/CedarlingAdapterTest.java`:
- Around line 63-94: Add two focused tests in CedarlingAdapterTest to cover the
null-handling branches: (1) call adapter.authorizeUnsigned(principalsString,
action, resource, null) and assert it does not throw (result != null,
result.getPrincipals() != null, expected decision false) to ensure
authorizeUnsigned stays NPE-free when context is null; (2) call
adapter.authorizeMultiIssuer(null, action, resource, context) and assert it
throws IllegalArgumentException to ensure the multi-issuer null check remains
enforced. Use the existing fixture setup (action/resource/context/principals
strings) and the same assertion style as the other 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: c223ec8d-a935-4d93-8175-37ac9f85e9a3
📒 Files selected for processing (2)
jans-cedarling/bindings/cedarling-java/src/main/java/io/jans/cedarling/binding/wrapper/CedarlingAdapter.javajans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/CedarlingAdapterTest.java
…ingAdapter - Added null checks for `tokens`, `principalJson`, and `principalsJson` parameters to prevent IllegalArgumentExceptions. - Updated documentation to clarify that certain parameters must not be null. - Ensured that null `context` values are sent as empty JSON objects to the engine. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
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/src/main/java/io/jans/cedarling/binding/wrapper/CedarlingAdapter.java`:
- Around line 82-94: The authorizeMultiIssuer method (and the other overload
around the authorize delegate at lines ~163-171) currently calls
resource.toString() without null checks; change both delegate overloads (e.g.,
authorizeMultiIssuer(List<TokenInput>, String, JSONObject, JSONObject) and the
similar authorize(...) overload) to validate that resource is not null and throw
an IllegalArgumentException with a clear message (e.g., "resource must not be
null") before calling resource.toString(), so callers receive the consistent
IllegalArgumentException pattern instead of a NullPointerException.
In
`@jans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/CedarlingAdapterTest.java`:
- Around line 63-148: Add focused negative unit tests for the remaining
null-guard branches: create tests that call
CedarlingAdapter.authorizeUnsigned(principalsString...) with principalJson ==
null and with a principals list containing a null element (covering
authorizeUnsignedFromJson null-element behavior), tests that call
CedarlingAdapter.authorizeMultiIssuer(tokens...) with tokens == null keys/values
(null key or null value entry) and a test that calls
authorizeMultiIssuer(tokens, action, resource, null) to ensure the null-context
branch is exercised; each test should assert the proper exception or result
shape using AuthorizeResult / MultiIssuerAuthorizeResult as appropriate to lock
the new compatibility contracts.
🪄 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: adddc336-269a-4229-9a9a-a6dd9812baf8
📒 Files selected for processing (2)
jans-cedarling/bindings/cedarling-java/src/main/java/io/jans/cedarling/binding/wrapper/CedarlingAdapter.javajans-cedarling/bindings/cedarling-java/src/test/java/io/jans/cedarling/binding/wrapper/CedarlingAdapterTest.java
…Adapter Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/main/java/io/jans/cedarling/binding/wrapper/CedarlingAdapter.java`:
- Around line 87-98: In CedarlingAdapter.add fast null-checks to the public list
overloads: in authorizeMultiIssuer(List<TokenInput> tokens, ...) validate tokens
!= null and that none of tokens' elements are null (throw
IllegalArgumentException with a clear message) before calling
cedarling.authorizeMultiIssuer; do the same in
authorizeUnsigned(List<EntityData> entities, ...) — check the list and each
element for null and throw IllegalArgumentException early so both API surfaces
fail consistently.
🪄 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: 5de7afc5-707a-4403-9d7e-e01abeaf63aa
📒 Files selected for processing (1)
jans-cedarling/bindings/cedarling-java/src/main/java/io/jans/cedarling/binding/wrapper/CedarlingAdapter.java
Prepare
Description
Target issue
closes #13595
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