fix(jans-cedarling): add cedarling log to lock log mapping#13554
Open
fix(jans-cedarling): add cedarling log to lock log mapping#13554
Conversation
* update tests to match new log format Signed-off-by: dagregi <dagmawi.m@proton.me>
Contributor
📝 WalkthroughWalkthroughConvert incoming Cedarling decision-log JSON into the Lock Server audit schema by adding a mapping module and updating REST and gRPC transports and tests to deserialize 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 |
mo-auto
added
comp-jans-cedarling
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. |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jans-cedarling/cedarling/src/lock/mod.rs (1)
559-564: 🧹 Nitpick | 🔵 TrivialUse
expect_errinstead ofassert!(result.is_err())pattern.Per coding guidelines, error checking in tests should use
result.expect_err("descriptive message")instead ofassert!(result.is_err()).♻️ Proposed fix
- let result = LockService::new(pdp_id, &config, None).await; - assert!(result.is_err()); - assert!(matches!( - result.unwrap_err(), + let error = LockService::new(pdp_id, &config, None) + .await + .expect_err("startup with invalid SSA should fail"); + assert!(matches!( + error, InitLockServiceError::InvalidSsaJwt(_) - )); + ), "expected InvalidSsaJwt error, got {error:?}");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/cedarling/src/lock/mod.rs` around lines 559 - 564, Replace the two-step test that uses assert!(result.is_err()) and then matches on result.unwrap_err() with a single expect_err call: call LockService::new(pdp_id, &config, None).await and use result.expect_err("descriptive message about expecting invalid SSA JWT") to capture the error, then assert that the captured error matches InitLockServiceError::InvalidSsaJwt(_) (keep the matches check) so you both provide a descriptive failure message and avoid unwrap_err on an assertion-only check.jans-cedarling/cedarling/src/lock/transport/rest.rs (1)
216-221: 🧹 Nitpick | 🔵 TrivialAdd descriptive message to the
matches!assertion.Per coding guidelines, all assertions must include a descriptive message explaining what is being tested.
♻️ Proposed fix
let result = transport.send_logs(&entries).await; - assert!(matches!( - result.unwrap_err(), - TransportError::Serialization(_) - )); + let error = result.expect_err("all malformed entries should cause serialization error"); + assert!( + matches!(error, TransportError::Serialization(_)), + "expected serialization error, got {error:?}" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/cedarling/src/lock/transport/rest.rs` around lines 216 - 221, The assertion using assert!(matches!(result.unwrap_err(), TransportError::Serialization(_))) lacks a descriptive message; update the test around the transport.send_logs(&entries).await call to include a clear assertion message explaining that a Serialization error is expected (mention TransportError::Serialization) and include the actual error (e.g., using "{:?}") so test failures show the observed error; keep the matches! check but append a message to the assert! macro referencing result.unwrap_err() for diagnostics.
🤖 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/cedarling/src/lock/transport/grpc.rs`:
- Around line 151-159: The parse_timestamp function uses a non-existent
cast_signed() on dt.timestamp_subsec_nanos(); update the Timestamp nanos
conversion in parse_timestamp to cast the u32 to i32 using a standard Rust cast
(i.e., replace the cast_signed() call on dt.timestamp_subsec_nanos() with an as
i32 cast) so Timestamp { seconds: dt.timestamp(), nanos: ... } compiles
correctly.
In `@jans-cedarling/cedarling/src/lock/transport/mapping.rs`:
- Around line 1-4: Add the standard Apache 2.0 license header (including the
Gluu, Inc. copyright notice) to the top of this Rust source file (mapping.rs in
the lock::transport module) so it precedes any use/import statements; ensure the
header text matches the project's canonical Apache-2.0 block used across other
Rust files in the repo.
- Around line 67-76: The code constructs client_id from
value.extra.get("lock_client_id") but then includes the same key in
context_information; to fix this remove or consume "lock_client_id" from
value.extra before building context_information. Specifically, when mapping into
the struct (referencing client_id and context_information), ensure you either
call value.extra.remove("lock_client_id") (or filter out that key) so
context_information is built from the remaining entries only, and keep client_id
assigned from the extracted value.
---
Outside diff comments:
In `@jans-cedarling/cedarling/src/lock/mod.rs`:
- Around line 559-564: Replace the two-step test that uses
assert!(result.is_err()) and then matches on result.unwrap_err() with a single
expect_err call: call LockService::new(pdp_id, &config, None).await and use
result.expect_err("descriptive message about expecting invalid SSA JWT") to
capture the error, then assert that the captured error matches
InitLockServiceError::InvalidSsaJwt(_) (keep the matches check) so you both
provide a descriptive failure message and avoid unwrap_err on an assertion-only
check.
In `@jans-cedarling/cedarling/src/lock/transport/rest.rs`:
- Around line 216-221: The assertion using assert!(matches!(result.unwrap_err(),
TransportError::Serialization(_))) lacks a descriptive message; update the test
around the transport.send_logs(&entries).await call to include a clear assertion
message explaining that a Serialization error is expected (mention
TransportError::Serialization) and include the actual error (e.g., using "{:?}")
so test failures show the observed error; keep the matches! check but append a
message to the assert! macro referencing result.unwrap_err() for diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e22de0ae-5717-4c35-90f0-cd1ffa2e5d9a
📒 Files selected for processing (5)
jans-cedarling/cedarling/src/lock/mod.rsjans-cedarling/cedarling/src/lock/transport/grpc.rsjans-cedarling/cedarling/src/lock/transport/mapping.rsjans-cedarling/cedarling/src/lock/transport/mod.rsjans-cedarling/cedarling/src/lock/transport/rest.rs
* remove duplicated lock_client_id Signed-off-by: dagregi <dagmawi.m@proton.me>
Contributor
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/cedarling/src/lock/transport/mapping.rs`:
- Around line 14-25: The fields log_kind, decision, action, and resource should
not silently default to empty strings; remove the #[serde(default)] from the
struct fields log_kind, decision, action, and resource so deserialization fails
on missing input, and add explicit validation in the conversion routine that
maps these struct fields to the output fields event_type, action,
decision_result, and requested_resource (returning a Result and not panicking).
Implement a small thiserror-backed enum (e.g., MappingValidationError) and in
the conversion function (the TryFrom/into conversion that produces the
audit/event object) check for empty or missing values and return
Err(MappingValidationError::MissingField("log_kind")) etc., so incomplete logs
fail fast with a descriptive error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d2b1163-3218-4a18-945d-25b337099c4c
📒 Files selected for processing (1)
jans-cedarling/cedarling/src/lock/transport/mapping.rs
Signed-off-by: dagregi <dagmawi.m@proton.me>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
jans-cedarling/cedarling/src/lock/transport/mapping.rs (1)
11-15: 🧹 Nitpick | 🔵 TrivialConsider adding context to
MissingFielderror variant.The error variant lacks information about which field is missing, making debugging harder. When a log entry fails validation, operators won't know which of the four checked fields (
log_kind,decision,action,resource) was empty.♻️ Proposed enhancement
#[derive(Debug, thiserror::Error)] pub(super) enum MappingValidationError { - #[error("missing required field")] - MissingField, + #[error("missing required field: {0}")] + MissingField(&'static str), }Then update the validation:
- if value.log_kind.is_empty() - || value.decision.is_empty() - || value.action.is_empty() - || value.resource.is_empty() - { - return Err(MappingValidationError::MissingField); - } + if value.log_kind.is_empty() { + return Err(MappingValidationError::MissingField("log_kind")); + } + if value.decision.is_empty() { + return Err(MappingValidationError::MissingField("decision")); + } + if value.action.is_empty() { + return Err(MappingValidationError::MissingField("action")); + } + if value.resource.is_empty() { + return Err(MappingValidationError::MissingField("resource")); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-cedarling/cedarling/src/lock/transport/mapping.rs` around lines 11 - 15, Change MappingValidationError::MissingField to carry the missing field name and update its error message (e.g., MissingField(String) with a thiserror format like "missing required field: {0}"), then update the validation logic that checks log_kind, decision, action, and resource to return MappingValidationError::MissingField("log_kind".into()) (or the appropriate field name) instead of the unit variant so logs and errors include which field was empty.
🤖 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/cedarling/src/lock/transport/mapping.rs`:
- Around line 11-15: Change MappingValidationError::MissingField to carry the
missing field name and update its error message (e.g., MissingField(String) with
a thiserror format like "missing required field: {0}"), then update the
validation logic that checks log_kind, decision, action, and resource to return
MappingValidationError::MissingField("log_kind".into()) (or the appropriate
field name) instead of the unit variant so logs and errors include which field
was empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9853559-2527-4980-96bf-6f7bf01035bd
📒 Files selected for processing (4)
jans-cedarling/cedarling/src/lock/mod.rsjans-cedarling/cedarling/src/lock/transport/grpc.rsjans-cedarling/cedarling/src/lock/transport/mapping.rsjans-cedarling/cedarling/src/lock/transport/rest.rs
haileyesus2433
approved these changes
Mar 24, 2026
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prepare
Description
Target issue
closes #13518
Implementation Details
New log format mapping - transforms Cedarling logs → Lock Server format
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
Tests
Chores