[RORDEV-1481] New audit serializers, including fully configurable serializer #1140
Conversation
| // Identifiers | ||
| case object Timestamp extends AuditValuePlaceholder | ||
|
|
||
| case object Id extends AuditValuePlaceholder |
There was a problem hiding this comment.
I think that we should discuss the field names, especially the various id types. At the moment they correspond to the names that already existed, but maybe we could make them more readable.
| } | ||
| } | ||
| object DefaultAuditLogSerializerV1 { | ||
| val defaultV1AuditFields: Map[String, AuditFieldValue] = Map( |
There was a problem hiding this comment.
Each serializer now uses the BaseAuditLogSerializer.serialize, only with different arguments:
- fields (serializers have the predefined list of fields, like this one; only
Configurableserializer gets its list of fields from config AllowedEventSerializationMode(that was the missing functionality - to log all ALLOWED events, even those for rules with INFO level verbose; there are new serializersReportingAllEventswith and without query; forConfigurableserializer this setting is defined in config)
| @@ -6,9 +6,17 @@ readonlyrest: | |||
| - type: index | |||
| index_template: "'audit_index'" | |||
| serializer: "tech.beshu.ror.audit.instances.DefaultAuditLogSerializerV1" | |||
| serialize_all_allowed_events: false | |||
| fields: | |||
| node_name_with_static_suffix: "{ES_NODE_NAME} with suffix" | |||
There was a problem hiding this comment.
It's maybe an overkill to allow that much configurability, but it basically allows to define a serializer in config, without writing any code.
Admins will be able to define any fields they want. The values are strings, that may contain any values defined by user, combined with value placeholders provided by ROR. If only our placeholder with number-like value is used, then it is reported as number. Other values and combinations of values are strings.
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala
Outdated
Show resolved
Hide resolved
audit/src/main/scala/tech/beshu/ror/audit/BaseAuditLogSerializer.scala
Outdated
Show resolved
Hide resolved
audit/src/main/scala/tech/beshu/ror/audit/BaseAuditLogSerializer.scala
Outdated
Show resolved
Hide resolved
| } match { | ||
| case Nil => "" | ||
| case singleElement :: Nil => singleElement | ||
| case multipleElements => multipleElements.mkString |
There was a problem hiding this comment.
I'm not sure about it.
I mean, it could be nice (maybe not in the current iteration of the feature) to define some form of field values composition, but this one (concatenation with two strings with no separator) doesn't seem to be useful. Maybe we should get rid of it now?
There was a problem hiding this comment.
This part of code is moved to core and refactored, but It
- allows to do configure field like that:
node_name_with_static_suffix: "{ES_NODE_NAME} with suffix" - it will be resolved to
AuditFieldValue.Combined(
List(
AuditFieldValue.EsNodeName,
AuditFieldValue.StaticText(" with suffix")
)
)
- and it will be serialized, for example, to string "ROR_SINGLE with suffix"
audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala
Outdated
Show resolved
Hide resolved
audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllEventsAuditLogSerializer.scala
Outdated
Show resolved
Hide resolved
...ain/scala/tech/beshu/ror/audit/instances/ReportingAllEventsWithQueryAuditLogSerializer.scala
Outdated
Show resolved
Hide resolved
| given auditLogSerializerDecoder(using context: AuditEnvironmentContext): Decoder[Option[AuditLogSerializer]] = Decoder.instance { c => | ||
| for { | ||
| serializeAllAllowedEvents <- c.downField("serialize_all_allowed_events").as[Option[Boolean]] | ||
| given Option[AllowedEventSerializationMode] = serializeAllAllowedEvents.map(c => if (c) SerializeAllAllowedEvents else SerializeOnlyAllowedEventsWithInfoLevelVerbose) |
There was a problem hiding this comment.
I'm not sure if passing the values using implicits is readable here. Why not declare decoder as a method?
There was a problem hiding this comment.
Decoders are refactored, I think this issue is solved and decoding is more readable.
| for { | ||
| serializeAllAllowedEvents <- c.downField("serialize_all_allowed_events").as[Option[Boolean]] | ||
| given Option[AllowedEventSerializationMode] = serializeAllAllowedEvents.map(c => if (c) SerializeAllAllowedEvents else SerializeOnlyAllowedEventsWithInfoLevelVerbose) | ||
| fields <- c.downField("fields").as[Option[Map[String, AuditFieldValue]]] |
There was a problem hiding this comment.
Adding "fields" option to any existing serializer is tempting, but when we do that, do we need the ConfigurableQueryAuditLogSerializer instance?
There was a problem hiding this comment.
It was only decoded to optional value, which was used only for configurable serialized. But it was not clean, decoders are refactored.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
audit/src/main/scala/tech/beshu/ror/audit/EnvironmentAwareAuditLogSerializer.scala (1)
27-32: Add an explicit type to environmentRelatedAuditFields to stabilize the public surfaceExplicitly typing this public val helps prevent unintended type widening if the initializer changes and documents the contract at the call site.
Apply this diff:
- val environmentRelatedAuditFields = Map( + val environmentRelatedAuditFields: Map[AuditFieldName, AuditFieldValue] = Map( AuditFieldName("es_node_name") -> AuditFieldValue.EsNodeName, AuditFieldName("es_cluster_name") -> AuditFieldValue.EsClusterName )audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsWithQueryAuditLogSerializer.scala (1)
24-24: Consider making the class final; optionally extend AuditLogSerializer directly
- final prevents accidental subclassing and matches typical serializer usage patterns.
- Since onResponse fully overrides behavior, extending AuditLogSerializer directly would avoid implying inheritance from DefaultAuditLogSerializer.
Apply this minimal hardening diff:
-class ReportingAllTypesOfEventsWithQueryAuditLogSerializer(environmentContext: AuditEnvironmentContext) extends DefaultAuditLogSerializer(environmentContext) { +final class ReportingAllTypesOfEventsWithQueryAuditLogSerializer(environmentContext: AuditEnvironmentContext) extends DefaultAuditLogSerializer(environmentContext) {If you prefer to extend AuditLogSerializer directly, I can provide a follow-up patch.
core/src/test/scala/tech/beshu/ror/unit/acl/factory/AuditSettingsTests.scala (1)
378-418: Add tests for serialize_all_allowed_events: true and ALLOWED/Error verbosity behaviorTo fully exercise AllowedEventSerializationMode:
- Positive: configurable=true with serialize_all_allowed_events: true should emit ALLOWED entries for Info and Error verbosity.
- Negative: configurable=true with serialize_all_allowed_events: false should emit ALLOWED only for Info (None for Error).
I can draft these test cases if helpful.
audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV1.scala (1)
19-23: Drop the self-import and qualify the reference to reduce indirectionYou can reference the companion object directly and remove the extra import.
Apply this diff:
import org.json.JSONObject import tech.beshu.ror.audit.* import tech.beshu.ror.audit.AuditSerializationHelper.{AllowedEventSerializationMode, AuditFieldName} -import tech.beshu.ror.audit.instances.QueryAuditLogSerializerV1.queryV1AuditFields @@ override def onResponse(responseContext: AuditResponseContext): Option[JSONObject] = AuditSerializationHelper.serialize( responseContext = responseContext, environmentContext = None, - fields = queryV1AuditFields, + fields = QueryAuditLogSerializerV1.queryV1AuditFields, allowedEventSerializationMode = AllowedEventSerializationMode.SerializeOnlyAllowedEventsWithInfoLevelVerbose )Also applies to: 26-32
core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala (1)
24-26: Nit: Consider making the class finalMarking the serializer final communicates intent (no subclassing) and can help the compiler optimize.
-class ConfigurableAuditLogSerializer(val environmentContext: AuditEnvironmentContext, +final class ConfigurableAuditLogSerializer(val environmentContext: AuditEnvironmentContext, val allowedEventSerializationMode: AllowedEventSerializationMode, val fields: Map[AuditFieldName, AuditFieldValue]) extends DefaultAuditLogSerializer(environmentContext) {audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsAuditLogSerializer.scala (1)
24-24: Optional: Reduce coupling by extending AuditLogSerializer directly (aligns with V2 serializers)You fully override onResponse; extending AuditLogSerializer directly mirrors DefaultAuditLogSerializerV2/QueryAuditLogSerializerV2 patterns and avoids unnecessary inheritance.
@@ -import tech.beshu.ror.audit.{AuditEnvironmentContext, AuditResponseContext, AuditSerializationHelper} +import tech.beshu.ror.audit.{AuditEnvironmentContext, AuditResponseContext, AuditSerializationHelper, AuditLogSerializer} @@ -class ReportingAllTypesOfEventsAuditLogSerializer(environmentContext: AuditEnvironmentContext) extends DefaultAuditLogSerializer(environmentContext) { +class ReportingAllTypesOfEventsAuditLogSerializer(environmentContext: AuditEnvironmentContext) extends AuditLogSerializer {Also applies to: 19-23
core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/AuditFieldValueDeserializer.scala (2)
33-35: Trim placeholder keys to be resilient to whitespaceConfig often ends up with spaces inside braces. Trimming avoids false negatives like "{ USER }".
- val key = m.group(1) + val key = m.group(1).trim
48-55: Cleaner error message: deduplicate invalid placeholdersIf the same unknown placeholder appears multiple times, the error lists duplicates. Deduplicate for clarity.
- case missingList => Left(s"There are invalid placeholder values: ${missingList.mkString(", ")}") + case missingList => Left(s"There are invalid placeholder values: ${missingList.distinct.mkString(", ")}")audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (4)
30-31: Prefer ISO_INSTANT and UTC for timestamp formattingUsing a literal 'Z' with GMT works, but ISO_INSTANT + UTC is clearer and standard. Also avoids confusion if the zone ever changes.
Apply this diff:
- private val timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss'Z'").withZone(ZoneId.of("GMT")) + private val timestampFormatter = DateTimeFormatter.ISO_INSTANT.withZone(ZoneId.of("UTC"))
71-73: Avoid inserting nulls into JSONObjectorg.json drops keys when value is null. If that’s intended, make it explicit and skip nulls; if not, use JSONObject.NULL. Skipping makes intent clearer.
Apply this diff:
- resolvedFields - .foldLeft(new JSONObject()) { case (soFar, (key, value)) => soFar.put(key, value) } + resolvedFields + .foldLeft(new JSONObject()) { + case (soFar, (key, value)) => + Option(value) match { + case Some(v) => soFar.put(key, v) + case None => soFar + } + } .mergeWith(requestContext.generalAuditEvents)
106-109: Content and size handling: consider truncation and KB rounding
- ContentLengthInKb uses integer division; consider rounding or keeping bytes only to avoid confusion for small payloads.
- Logging Content may produce very large audit entries. Consider truncating or gating by size to avoid log bloat.
109-110: Make environment fields consistent with other optionalsOther placeholders use orNull (which results in key omission via put); here empty string is emitted. Suggest using orNull for consistency.
Apply this diff:
- case AuditFieldValue.EsNodeName => environmentContext.map(_.esNodeName).getOrElse("") - case AuditFieldValue.EsClusterName => environmentContext.map(_.esClusterName).getOrElse("") + case AuditFieldValue.EsNodeName => environmentContext.map(_.esNodeName).orNull + case AuditFieldValue.EsClusterName => environmentContext.map(_.esClusterName).orNull
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/EnvironmentAwareAuditLogSerializer.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV1.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsAuditLogSerializer.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsWithQueryAuditLogSerializer.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/AuditFieldValueDeserializer.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala(5 hunks)core/src/test/scala/tech/beshu/ror/unit/acl/factory/AuditSettingsTests.scala(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala
🧰 Additional context used
🧬 Code Graph Analysis (10)
audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsWithQueryAuditLogSerializer.scala (6)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (1)
ror(21-85)audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (4)
ror(28-141)AllowedEventSerializationMode(133-137)serialize(32-52)SerializeAllAllowedEvents(136-136)audit/src/main/scala/tech/beshu/ror/audit/AuditResponseContext.scala (1)
AuditResponseContext(28-55)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala (1)
onResponse(27-35)audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsAuditLogSerializer.scala (1)
onResponse(26-34)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala (1)
onResponse(28-31)
audit/src/main/scala/tech/beshu/ror/audit/EnvironmentAwareAuditLogSerializer.scala (3)
audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (2)
ror(28-141)AuditFieldName(139-139)core/src/test/scala/tech/beshu/ror/unit/acl/factory/AuditSettingsTests.scala (1)
onResponse(1956-1961)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala (1)
onResponse(28-31)
audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (2)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (28)
ror(21-85)IsMatched(24-24)FinalState(26-26)Reason(28-28)User(30-30)ImpersonatedByUser(32-32)Action(34-34)InvolvedIndices(36-36)AclHistory(38-38)ProcessingDurationMillis(40-40)Timestamp(43-43)Id(45-45)CorrelationId(47-47)TaskId(49-49)ErrorType(52-52)ErrorMessage(54-54)Type(56-56)HttpMethod(59-59)HttpHeaderNames(61-61)HttpPath(63-63)XForwardedForHttpHeader(65-65)RemoteAddress(67-67)LocalAddress(69-69)Content(71-71)ContentLengthInBytes(73-73)ContentLengthInKb(75-75)EsNodeName(77-77)EsClusterName(79-79)audit/src/main/scala/tech/beshu/ror/audit/AuditResponseContext.scala (1)
AuditResponseContext(28-55)
core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala (8)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (1)
ror(21-85)audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (4)
ror(28-141)AllowedEventSerializationMode(133-137)AuditFieldName(139-139)serialize(32-52)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializer.scala (1)
DefaultAuditLogSerializer(21-21)audit/src/main/scala/tech/beshu/ror/audit/AuditResponseContext.scala (1)
AuditResponseContext(28-55)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala (1)
onResponse(26-34)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala (1)
onResponse(27-35)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala (1)
onResponse(27-35)audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsWithQueryAuditLogSerializer.scala (1)
onResponse(26-34)
audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsAuditLogSerializer.scala (7)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (1)
ror(21-85)audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (4)
ror(28-141)AllowedEventSerializationMode(133-137)serialize(32-52)SerializeAllAllowedEvents(136-136)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala (3)
DefaultAuditLogSerializerV2(25-35)DefaultAuditLogSerializerV2(37-40)onResponse(27-35)audit/src/main/scala/tech/beshu/ror/audit/AuditResponseContext.scala (1)
AuditResponseContext(28-55)core/src/test/scala/tech/beshu/ror/unit/acl/factory/AuditSettingsTests.scala (1)
onResponse(1956-1961)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala (1)
onResponse(27-35)audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsWithQueryAuditLogSerializer.scala (1)
onResponse(26-34)
core/src/test/scala/tech/beshu/ror/unit/acl/factory/AuditSettingsTests.scala (3)
audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (4)
ror(28-141)AllowedEventSerializationMode(133-137)AuditFieldName(139-139)SerializeOnlyAllowedEventsWithInfoLevelVerbose(134-134)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala (1)
ConfigurableAuditLogSerializer(24-31)core/src/test/scala/tech/beshu/ror/utils/TestsUtils.scala (2)
rorConfigFromUnsafe(383-385)testAuditEnvironmentContext(412-412)
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala (3)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (2)
ror(21-85)Reason(28-28)audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (5)
ror(28-141)AllowedEventSerializationMode(133-137)AuditFieldName(139-139)SerializeAllAllowedEvents(136-136)SerializeOnlyAllowedEventsWithInfoLevelVerbose(134-134)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala (1)
ConfigurableAuditLogSerializer(24-31)
audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala (6)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (1)
ror(21-85)audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (5)
ror(28-141)AllowedEventSerializationMode(133-137)AuditFieldName(139-139)serialize(32-52)SerializeOnlyAllowedEventsWithInfoLevelVerbose(134-134)audit/src/main/scala/tech/beshu/ror/audit/EnvironmentAwareAuditLogSerializer.scala (1)
EnvironmentAwareAuditLogSerializer(27-32)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala (1)
onResponse(27-35)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV1.scala (3)
onResponse(26-34)QueryAuditLogSerializerV1(24-34)QueryAuditLogSerializerV1(36-41)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala (1)
onResponse(28-31)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (2)
audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (1)
ror(28-141)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/RawRorConfigBasedCoreFactory.scala (1)
Reason(477-491)
audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala (7)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (1)
ror(21-85)audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (5)
ror(28-141)AllowedEventSerializationMode(133-137)AuditFieldName(139-139)serialize(32-52)SerializeOnlyAllowedEventsWithInfoLevelVerbose(134-134)audit/src/main/scala/tech/beshu/ror/audit/EnvironmentAwareAuditLogSerializer.scala (1)
EnvironmentAwareAuditLogSerializer(27-32)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala (3)
onResponse(26-34)DefaultAuditLogSerializerV1(24-34)DefaultAuditLogSerializerV1(36-63)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala (1)
onResponse(27-35)audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsAuditLogSerializer.scala (1)
onResponse(26-34)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala (1)
onResponse(28-31)
🔇 Additional comments (17)
audit/src/main/scala/tech/beshu/ror/audit/EnvironmentAwareAuditLogSerializer.scala (1)
20-20: Import looks correct and scoped appropriatelyImporting AuditFieldName from AuditSerializationHelper is consistent with where the type is declared. No issues here.
audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsWithQueryAuditLogSerializer.scala (1)
26-33: LGTM: correct wiring to emit all allowed events with query fieldsserialize uses queryV2AuditFields + SerializeAllAllowedEvents, which aligns with the “report all events including allowed” objective.
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (1)
19-85: No duplicate AuditFieldValue or AuditFieldName definitions detectedThe search shows only one AuditFieldValue.scala under audit/src/main/scala/tech/beshu/ror/audit and a single AuditFieldName in AuditSerializationHelper.scala. There are no conflicting definitions across modules.
core/src/test/scala/tech/beshu/ror/unit/acl/factory/AuditSettingsTests.scala (2)
378-418: Configurable serializer happy-path test is spot on
- Verifies serializer selection, environment context propagation, allowed mode, and precise fields mapping (including Combined and StaticText). This gives high confidence in the new configurable flow.
1288-1333: Good negative coverage for missing configurable settingsBoth cases (missing serialize_all_allowed_events and missing fields) are properly validated with clear error messages.
audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV1.scala (2)
26-33: LGTM: V1 semantics preserved with centralized serialization
- environmentContext=None (V1 didn’t include env fields).
- AllowedEventSerializationMode.SerializeOnlyAllowedEventsWithInfoLevelVerbose matches existing behavior.
- Uses queryV1AuditFields, which builds on default V1 fields + content.
All consistent with the intent of V1 serializers.
36-41: Field composition for queryV1AuditFields looks correctExtending defaultV1AuditFields with content matches the “query” variant’s scope without pulling in env fields (those are introduced in V2).
core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala (1)
28-30: LGTM: Correct delegation to centralized serialization helperThe override cleanly delegates to AuditSerializationHelper with environment, fields, and allowed-event mode. This keeps behavior consistent with the other serializers.
audit/src/main/scala/tech/beshu/ror/audit/instances/ReportingAllTypesOfEventsAuditLogSerializer.scala (1)
26-32: LGTM: All-allowed-events mode wired correctlyDelegation to serialize with defaultV2AuditFields and SerializeAllAllowedEvents matches the all-events requirement.
audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala (2)
27-33: LGTM: V2 query serializer correctly delegates to centralized helperUses queryV2AuditFields and the expected allowed-event mode. Matches the new architecture.
38-40: LGTM: Field set composition is correctMerging V1 query fields with environmentRelatedAuditFields aligns with the v2 model.
audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala (2)
27-33: LGTM: Centralized serialization with environment contextThis replaces ad-hoc JSON augmentation, keeping behavior consistent and testable.
38-39: LGTM: Default V2 fields = V1 fields + environment fieldsThe composition is clear and keeps evolution straightforward.
audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (1)
36-43: Allowed-events gating logic is correctConditions cleanly separate “only INFO-level allowed events” vs “all allowed events”. This aligns with the PR’s goal.
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala (3)
125-133: LGTM: unified serializer extraction per sinkDecoding the serializer at the sink level (log/index/data_stream) is clean and consistent with the new configurable path. Defaults are preserved when not provided.
Also applies to: 135-145, 147-157
212-241: Configurable vs class-based path reads well
- Mutually exclusive handling via configurable flag is clear.
- Helpful error messages when required fields for the configurable path are missing.
- Field map decoding via AuditFieldName/AuditFieldValue is tidy.
360-369: Backward-compat serializer lookup (outside/inside audit) looks goodGracefully supports legacy placement by preferring top-level and falling back to audit section. Defaults are sensible.
...main/scala/tech/beshu/ror/accesscontrol/audit/configurable/AuditFieldValueDeserializer.scala
Outdated
Show resolved
Hide resolved
| private def createSerializerInstanceFromClassName(fullClassName: String) | ||
| (using context: AuditEnvironmentContext): Either[AuditingSettingsCreationError, AuditLogSerializer] = { | ||
| val clazz = Class.forName(fullClassName) | ||
|
|
||
| def createInstanceOfEnvironmentAwareSerializer(): Try[Any] = | ||
| Try(clazz.getConstructor(classOf[AuditEnvironmentContext])).map(_.newInstance(context)) | ||
|
|
||
| def createInstanceOfSimpleSerializer(): Try[Any] = | ||
| Try(clazz.getDeclaredConstructor()).map(_.newInstance()) | ||
|
|
||
| val serializer = | ||
| createInstanceOfEnvironmentAwareSerializer() | ||
| .orElse(createInstanceOfSimpleSerializer()) | ||
| .getOrElse( | ||
| throw new IllegalStateException( | ||
| s"Class ${Class.forName(fullClassName).getName} is required to have either one (AuditEnvironmentContext) parameter constructor or constructor without parameters" | ||
| ) | ||
| ) | ||
|
|
||
| Try { | ||
| serializer match { | ||
| case serializer: tech.beshu.ror.audit.AuditLogSerializer => | ||
| Some(serializer) | ||
| case serializer: tech.beshu.ror.audit.EnvironmentAwareAuditLogSerializer => | ||
| Some(new EnvironmentAwareAuditLogSerializerAdapter(serializer, summon[AuditEnvironmentContext])) | ||
| case serializer: tech.beshu.ror.requestcontext.AuditLogSerializer[_] => | ||
| Some(new DeprecatedAuditLogSerializerAdapter(serializer)) | ||
| case _ => None | ||
| } | ||
| } match { | ||
| case Success(Some(customSerializer)) => | ||
| logger.info(s"Using custom serializer: ${customSerializer.getClass.getName}") | ||
| Right(customSerializer) | ||
| case Success(None) => Left(AuditingSettingsCreationError(Message(s"Class ${fullClassName.show} is not a subclass of ${classOf[AuditLogSerializer].getName.show} or ${classOf[tech.beshu.ror.requestcontext.AuditLogSerializer[_]].getName.show}"))) | ||
| case Failure(ex) => Left(AuditingSettingsCreationError(Message(s"Cannot create instance of class '${fullClassName.show}', error: ${ex.getMessage.show}"))) | ||
| } | ||
| Try { | ||
| serializer match { | ||
| case serializer: tech.beshu.ror.audit.AuditLogSerializer => | ||
| Some(serializer) | ||
| case serializer: tech.beshu.ror.audit.EnvironmentAwareAuditLogSerializer => | ||
| Some(new EnvironmentAwareAuditLogSerializerAdapter(serializer, summon[AuditEnvironmentContext])) | ||
| case serializer: tech.beshu.ror.requestcontext.AuditLogSerializer[_] => | ||
| Some(new DeprecatedAuditLogSerializerAdapter(serializer)) | ||
| case _ => None | ||
| } | ||
| } match { | ||
| case Success(Some(customSerializer)) => | ||
| logger.info(s"Using custom serializer: ${customSerializer.getClass.getName}") | ||
| Right(customSerializer) | ||
| case Success(None) => Left(AuditingSettingsCreationError(Message(s"Class ${fullClassName.show} is not a subclass of ${classOf[AuditLogSerializer].getName.show} or ${classOf[tech.beshu.ror.requestcontext.AuditLogSerializer[_]].getName.show}"))) | ||
| case Failure(ex) => Left(AuditingSettingsCreationError(Message(s"Cannot create instance of class '${fullClassName.show}', error: ${ex.getMessage.show}"))) | ||
| } | ||
| } |
There was a problem hiding this comment.
Uncaught Class.forName exceptions and double class loading in error path
Class loading is not wrapped, so ClassNotFoundException will escape as an unhandled exception. Also, the fallback error message calls Class.forName again, potentially throwing a second time. Wrap class loading/instantiation in Try/Either and avoid re-loading for messages.
Apply this diff to make the method exception-safe and avoid re-loading:
- private def createSerializerInstanceFromClassName(fullClassName: String)
- (using context: AuditEnvironmentContext): Either[AuditingSettingsCreationError, AuditLogSerializer] = {
- val clazz = Class.forName(fullClassName)
-
- def createInstanceOfEnvironmentAwareSerializer(): Try[Any] =
- Try(clazz.getConstructor(classOf[AuditEnvironmentContext])).map(_.newInstance(context))
-
- def createInstanceOfSimpleSerializer(): Try[Any] =
- Try(clazz.getDeclaredConstructor()).map(_.newInstance())
-
- val serializer =
- createInstanceOfEnvironmentAwareSerializer()
- .orElse(createInstanceOfSimpleSerializer())
- .getOrElse(
- throw new IllegalStateException(
- s"Class ${Class.forName(fullClassName).getName} is required to have either one (AuditEnvironmentContext) parameter constructor or constructor without parameters"
- )
- )
-
- Try {
- serializer match {
- case serializer: tech.beshu.ror.audit.AuditLogSerializer =>
- Some(serializer)
- case serializer: tech.beshu.ror.audit.EnvironmentAwareAuditLogSerializer =>
- Some(new EnvironmentAwareAuditLogSerializerAdapter(serializer, summon[AuditEnvironmentContext]))
- case serializer: tech.beshu.ror.requestcontext.AuditLogSerializer[_] =>
- Some(new DeprecatedAuditLogSerializerAdapter(serializer))
- case _ => None
- }
- } match {
- case Success(Some(customSerializer)) =>
- logger.info(s"Using custom serializer: ${customSerializer.getClass.getName}")
- Right(customSerializer)
- case Success(None) => Left(AuditingSettingsCreationError(Message(s"Class ${fullClassName.show} is not a subclass of ${classOf[AuditLogSerializer].getName.show} or ${classOf[tech.beshu.ror.requestcontext.AuditLogSerializer[_]].getName.show}")))
- case Failure(ex) => Left(AuditingSettingsCreationError(Message(s"Cannot create instance of class '${fullClassName.show}', error: ${ex.getMessage.show}")))
- }
- }
+ private def createSerializerInstanceFromClassName(fullClassName: String)
+ (using context: AuditEnvironmentContext): Either[AuditingSettingsCreationError, AuditLogSerializer] = {
+ Try(Class.forName(fullClassName)).toEither
+ .left.map(ex => AuditingSettingsCreationError(Message(s"Cannot load class ${fullClassName.show}: ${ex.getMessage.show}")))
+ .flatMap { clazz =>
+ def envAware(): Try[Any] = Try(clazz.getConstructor(classOf[AuditEnvironmentContext])).map(_.newInstance(context))
+ def simple(): Try[Any] = Try(clazz.getDeclaredConstructor()).map(_.newInstance())
+
+ envAware().orElse(simple()).toEither.left.map { ex =>
+ AuditingSettingsCreationError(Message(
+ s"Class ${clazz.getName.show} is required to have either one (AuditEnvironmentContext) parameter constructor or a no-arg constructor. Error: ${ex.getMessage.show}"
+ ))
+ }.flatMap { instance =>
+ Try {
+ instance match {
+ case s: tech.beshu.ror.audit.AuditLogSerializer =>
+ Some(s)
+ case s: tech.beshu.ror.audit.EnvironmentAwareAuditLogSerializer =>
+ Some(new EnvironmentAwareAuditLogSerializerAdapter(s, summon[AuditEnvironmentContext]))
+ case s: tech.beshu.ror.requestcontext.AuditLogSerializer[_] =>
+ Some(new DeprecatedAuditLogSerializerAdapter(s))
+ case _ => None
+ }
+ }.toEither.left.map { ex =>
+ AuditingSettingsCreationError(Message(s"Cannot create instance of class ${fullClassName.show}, error: ${ex.getMessage.show}"))
+ }.flatMap {
+ case Some(customSerializer) =>
+ logger.info(s"Using custom serializer: ${customSerializer.getClass.getName}")
+ Right(customSerializer)
+ case None =>
+ Left(AuditingSettingsCreationError(Message(
+ s"Class ${fullClassName.show} is not a subclass of ${classOf[AuditLogSerializer].getName.show} or ${classOf[tech.beshu.ror.requestcontext.AuditLogSerializer[_]].getName.show}"
+ )))
+ }
+ }
+ }
+ }📝 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.
| private def createSerializerInstanceFromClassName(fullClassName: String) | |
| (using context: AuditEnvironmentContext): Either[AuditingSettingsCreationError, AuditLogSerializer] = { | |
| val clazz = Class.forName(fullClassName) | |
| def createInstanceOfEnvironmentAwareSerializer(): Try[Any] = | |
| Try(clazz.getConstructor(classOf[AuditEnvironmentContext])).map(_.newInstance(context)) | |
| def createInstanceOfSimpleSerializer(): Try[Any] = | |
| Try(clazz.getDeclaredConstructor()).map(_.newInstance()) | |
| val serializer = | |
| createInstanceOfEnvironmentAwareSerializer() | |
| .orElse(createInstanceOfSimpleSerializer()) | |
| .getOrElse( | |
| throw new IllegalStateException( | |
| s"Class ${Class.forName(fullClassName).getName} is required to have either one (AuditEnvironmentContext) parameter constructor or constructor without parameters" | |
| ) | |
| ) | |
| Try { | |
| serializer match { | |
| case serializer: tech.beshu.ror.audit.AuditLogSerializer => | |
| Some(serializer) | |
| case serializer: tech.beshu.ror.audit.EnvironmentAwareAuditLogSerializer => | |
| Some(new EnvironmentAwareAuditLogSerializerAdapter(serializer, summon[AuditEnvironmentContext])) | |
| case serializer: tech.beshu.ror.requestcontext.AuditLogSerializer[_] => | |
| Some(new DeprecatedAuditLogSerializerAdapter(serializer)) | |
| case _ => None | |
| } | |
| } match { | |
| case Success(Some(customSerializer)) => | |
| logger.info(s"Using custom serializer: ${customSerializer.getClass.getName}") | |
| Right(customSerializer) | |
| case Success(None) => Left(AuditingSettingsCreationError(Message(s"Class ${fullClassName.show} is not a subclass of ${classOf[AuditLogSerializer].getName.show} or ${classOf[tech.beshu.ror.requestcontext.AuditLogSerializer[_]].getName.show}"))) | |
| case Failure(ex) => Left(AuditingSettingsCreationError(Message(s"Cannot create instance of class '${fullClassName.show}', error: ${ex.getMessage.show}"))) | |
| } | |
| Try { | |
| serializer match { | |
| case serializer: tech.beshu.ror.audit.AuditLogSerializer => | |
| Some(serializer) | |
| case serializer: tech.beshu.ror.audit.EnvironmentAwareAuditLogSerializer => | |
| Some(new EnvironmentAwareAuditLogSerializerAdapter(serializer, summon[AuditEnvironmentContext])) | |
| case serializer: tech.beshu.ror.requestcontext.AuditLogSerializer[_] => | |
| Some(new DeprecatedAuditLogSerializerAdapter(serializer)) | |
| case _ => None | |
| } | |
| } match { | |
| case Success(Some(customSerializer)) => | |
| logger.info(s"Using custom serializer: ${customSerializer.getClass.getName}") | |
| Right(customSerializer) | |
| case Success(None) => Left(AuditingSettingsCreationError(Message(s"Class ${fullClassName.show} is not a subclass of ${classOf[AuditLogSerializer].getName.show} or ${classOf[tech.beshu.ror.requestcontext.AuditLogSerializer[_]].getName.show}"))) | |
| case Failure(ex) => Left(AuditingSettingsCreationError(Message(s"Cannot create instance of class '${fullClassName.show}', error: ${ex.getMessage.show}"))) | |
| } | |
| } | |
| private def createSerializerInstanceFromClassName(fullClassName: String) | |
| (using context: AuditEnvironmentContext): Either[AuditingSettingsCreationError, AuditLogSerializer] = { | |
| Try(Class.forName(fullClassName)).toEither | |
| .left.map(ex => AuditingSettingsCreationError( | |
| Message(s"Cannot load class ${fullClassName.show}: ${ex.getMessage.show}") | |
| )) | |
| .flatMap { clazz => | |
| def envAware(): Try[Any] = | |
| Try(clazz.getConstructor(classOf[AuditEnvironmentContext])) | |
| .map(_.newInstance(context)) | |
| def simple(): Try[Any] = | |
| Try(clazz.getDeclaredConstructor()) | |
| .map(_.newInstance()) | |
| envAware() | |
| .orElse(simple()) | |
| .toEither | |
| .left.map { ex => | |
| AuditingSettingsCreationError(Message( | |
| s"Class ${clazz.getName.show} is required to have either one (AuditEnvironmentContext) parameter constructor or a no-arg constructor. Error: ${ex.getMessage.show}" | |
| )) | |
| } | |
| .flatMap { instance => | |
| Try { | |
| instance match { | |
| case s: tech.beshu.ror.audit.AuditLogSerializer => | |
| Some(s) | |
| case s: tech.beshu.ror.audit.EnvironmentAwareAuditLogSerializer => | |
| Some(new EnvironmentAwareAuditLogSerializerAdapter(s, summon[AuditEnvironmentContext])) | |
| case s: tech.beshu.ror.requestcontext.AuditLogSerializer[_] => | |
| Some(new DeprecatedAuditLogSerializerAdapter(s)) | |
| case _ => None | |
| } | |
| } | |
| .toEither | |
| .left.map(ex => | |
| AuditingSettingsCreationError( | |
| Message(s"Cannot create instance of class ${fullClassName.show}, error: ${ex.getMessage.show}") | |
| ) | |
| ) | |
| .flatMap { | |
| case Some(customSerializer) => | |
| logger.info(s"Using custom serializer: ${customSerializer.getClass.getName}") | |
| Right(customSerializer) | |
| case None => | |
| Left(AuditingSettingsCreationError(Message( | |
| s"Class ${fullClassName.show} is not a subclass of ${classOf[AuditLogSerializer].getName.show} or ${classOf[tech.beshu.ror.requestcontext.AuditLogSerializer[_]].getName.show}" | |
| ))) | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala
around lines 248 to 284, the method can throw ClassNotFoundException (and other
reflection exceptions) because Class.forName and later Class.forName in the
error path are not wrapped; to fix, wrap the initial
Class.forName(fullClassName) in a Try (or Either) and short-circuit to
Left(AuditingSettingsCreationError(...)) on failure, stop calling Class.forName
a second time for error messages (use the already-loaded clazz or the original
fullClassName string), wrap constructor lookups/instantiations in Try as you
already do and convert reflection failures into Left with clear message
including the original exception message; ensure no Class.forName or newInstance
calls escape as unchecked exceptions and return an appropriate Left for any
reflection failure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala (1)
20-23: Avoid self-import of companion object; reference it directly for clarityImporting from the same file’s companion object is unnecessary and slightly obscures where the symbol comes from. Refer to the companion explicitly to make the dependency obvious and avoid forward-imports.
Apply:
-import tech.beshu.ror.audit.instances.DefaultAuditLogSerializerV2.defaultV2AuditFields @@ - fields = defaultV2AuditFields, + fields = DefaultAuditLogSerializerV2.defaultV2AuditFields,Also applies to: 31-31
audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV1.scala (3)
20-22: Trim wildcard/self-imports for clarity and localityPrefer explicit imports and local companion-object import. This avoids namespace pollution and improves readability.
Apply this diff:
-import tech.beshu.ror.audit._ -import tech.beshu.ror.audit.AuditSerializationHelper.{AllowedEventSerializationMode, AuditFieldName} -import tech.beshu.ror.audit.instances.QueryAuditLogSerializerV1.queryV1AuditFields +import tech.beshu.ror.audit.{AuditFieldValue, AuditLogSerializer, AuditResponseContext, AuditSerializationHelper} +import tech.beshu.ror.audit.AuditSerializationHelper.{AllowedEventSerializationMode, AuditFieldName} +import QueryAuditLogSerializerV1.queryV1AuditFields
24-24: Consider sealing the class to prevent unintended extensionIf subclassing isn’t required, marking it final clarifies intent and can enable minor JVM optimizations.
-class QueryAuditLogSerializerV1 extends AuditLogSerializer { +final class QueryAuditLogSerializerV1 extends AuditLogSerializer {
36-41: Heads-up: logging request content can leak PII and bloat logsAdding “content” is expected for the query serializer, but it may include sensitive payloads and large bodies. Ensure:
- Redaction/truncation is in place (or documented) for sensitive fields and very large payloads.
- Users who need to exclude payloads can switch to the configurable serializer and drop “content”.
If redaction/truncation isn’t currently handled at resolve-time for AuditFieldValue.Content, consider adding it (e.g., size cap + redact patterns).
Would you like a follow-up patch proposal to introduce a capped/redacted Content resolver in AuditSerializationHelper and a config knob for max content size?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV1.scala(1 hunks)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala
- audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala
- audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala
🧰 Additional context used
🧬 Code Graph Analysis (2)
audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV1.scala (5)
audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (5)
ror(28-141)AllowedEventSerializationMode(133-137)AuditFieldName(139-139)serialize(32-52)SerializeOnlyAllowedEventsWithInfoLevelVerbose(134-134)audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (2)
ror(21-85)Content(71-71)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala (3)
onResponse(26-34)DefaultAuditLogSerializerV1(24-34)DefaultAuditLogSerializerV1(36-63)audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV2.scala (1)
onResponse(27-35)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/ConfigurableAuditLogSerializer.scala (1)
onResponse(28-31)
audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala (3)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (1)
ror(21-85)audit/src/main/scala/tech/beshu/ror/audit/EnvironmentAwareAuditLogSerializer.scala (1)
EnvironmentAwareAuditLogSerializer(27-32)audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV1.scala (2)
DefaultAuditLogSerializerV1(24-34)DefaultAuditLogSerializerV1(36-63)
🔇 Additional comments (4)
audit/src/main/scala/tech/beshu/ror/audit/instances/DefaultAuditLogSerializerV2.scala (3)
25-25: Good decoupling from V1; unified serializer contractExtending the common AuditLogSerializer and delegating to the helper improves cohesion and removes duplication that used to live in V1.
27-33: Confirm AllowedEventSerializationMode preserves V2 behaviorUsing AllowedEventSerializationMode.SerializeOnlyAllowedEventsWithInfoLevelVerbose looks right, but please confirm it preserves the historical Default V2 contract (i.e., which events are emitted and at what verbosity), and that only the new “ReportingAll*” serializers change inclusion semantics for ALLOWED events.
If helpful, add/verify tests that assert:
- DefaultAuditLogSerializerV2 emits DENIED/ERROR events as before.
- ALLOWED events appear (or not) exactly as they did prior to this refactor, including verbosity of fields.
37-39: Field set composition for V2 looks correctBuilding V2 as V1 fields plus environmentRelatedAuditFields is consistent and centralizes env fields in one place. Nice.
audit/src/main/scala/tech/beshu/ror/audit/instances/QueryAuditLogSerializerV1.scala (1)
26-33: Centralized serialization wiring looks correctThe delegate to AuditSerializationHelper with SerializeOnlyAllowedEventsWithInfoLevelVerbose and no environment context matches V1 behavior and keeps semantics consistent with Default V1 and Query V2 counterparts.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (1)
19-90: Consider moving audit configuration types to core moduleAs noted in a previous review, these audit configuration classes should be defined in ROR's core module since users won't be extending them. This would improve module boundaries and make the architecture clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/AuditFieldValueDeserializer.scala(1 hunks)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (2)
audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (1)
ror(28-141)core/src/main/scala/tech/beshu/ror/accesscontrol/factory/RawRorConfigBasedCoreFactory.scala (1)
Reason(477-491)
core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/AuditFieldValueDeserializer.scala (2)
audit/src/main/scala/tech/beshu/ror/audit/AuditFieldValue.scala (7)
ror(21-89)StaticText(85-85)Combined(87-87)IsMatched(24-24)FinalState(26-26)Reason(28-28)User(30-30)audit/src/main/scala/tech/beshu/ror/audit/AuditSerializationHelper.scala (1)
ror(28-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (Run all tests IT_es818x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es91x)
- GitHub Check: ror (Required checks AUDIT_BUILD_CHECK)
- GitHub Check: ror (Required checks COMPILE_CODEBASE_CHECK)
- GitHub Check: ror (Run all tests Unit tests)
- GitHub Check: ror (Required checks LICENSE_CHECK)
- GitHub Check: ror (Optional checks CVE_CHECK)
🔇 Additional comments (3)
core/src/main/scala/tech/beshu/ror/accesscontrol/audit/configurable/AuditFieldValueDeserializer.scala (1)
23-92: LGTM! Good error handling improvementsThe deserializer correctly handles unknown placeholders by returning
Noneand collecting them in the error list. The exhaustive pattern match with the default case preventsMatchErrorexceptions.core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala (2)
212-241: Well-structured configurable vs legacy serializer decodingThe decoder correctly implements mutual exclusivity between configurable and legacy modes, with proper error messages for missing required fields in configurable mode.
296-301: Clean field value decoder implementationThe decoder properly delegates to
AuditFieldValueDeserializer.deserializeand handles errors appropriately.
| private def createSerializerInstanceFromClassName(fullClassName: String) | ||
| (using context: AuditEnvironmentContext): Either[AuditingSettingsCreationError, AuditLogSerializer] = { | ||
| val clazz = Try(Class.forName(fullClassName)).fold( | ||
| { | ||
| case _: ClassNotFoundException => throw new IllegalStateException(s"Serializer with class name $fullClassName not found.") | ||
| case other => throw other | ||
| }, | ||
| identity | ||
| ) | ||
|
|
||
| def createInstanceOfEnvironmentAwareSerializer(): Try[Any] = | ||
| Try(clazz.getConstructor(classOf[AuditEnvironmentContext])).map(_.newInstance(context)) | ||
|
|
||
| def createInstanceOfSimpleSerializer(): Try[Any] = | ||
| Try(clazz.getDeclaredConstructor()).map(_.newInstance()) | ||
|
|
||
| val serializer = | ||
| createInstanceOfEnvironmentAwareSerializer() | ||
| .orElse(createInstanceOfSimpleSerializer()) | ||
| .getOrElse( | ||
| throw new IllegalStateException( | ||
| s"Class ${clazz.getName} is required to have either one (AuditEnvironmentContext) parameter constructor or constructor without parameters" | ||
| ) | ||
| ) | ||
|
|
||
| Try { | ||
| serializer match { | ||
| case serializer: tech.beshu.ror.audit.AuditLogSerializer => | ||
| Some(serializer) | ||
| case serializer: tech.beshu.ror.audit.EnvironmentAwareAuditLogSerializer => | ||
| Some(new EnvironmentAwareAuditLogSerializerAdapter(serializer, summon[AuditEnvironmentContext])) | ||
| case serializer: tech.beshu.ror.requestcontext.AuditLogSerializer[_] => | ||
| Some(new DeprecatedAuditLogSerializerAdapter(serializer)) | ||
| case _ => None | ||
| } | ||
| } match { | ||
| case Success(Some(customSerializer)) => | ||
| logger.info(s"Using custom serializer: ${customSerializer.getClass.getName}") | ||
| Right(customSerializer) | ||
| case Success(None) => Left(AuditingSettingsCreationError(Message(s"Class ${fullClassName.show} is not a subclass of ${classOf[AuditLogSerializer].getName.show} or ${classOf[tech.beshu.ror.requestcontext.AuditLogSerializer[_]].getName.show}"))) | ||
| case Failure(ex) => Left(AuditingSettingsCreationError(Message(s"Cannot create instance of class '${fullClassName.show}', error: ${ex.getMessage.show}"))) | ||
| } | ||
| Try { | ||
| serializer match { | ||
| case serializer: tech.beshu.ror.audit.AuditLogSerializer => | ||
| Some(serializer) | ||
| case serializer: tech.beshu.ror.audit.EnvironmentAwareAuditLogSerializer => | ||
| Some(new EnvironmentAwareAuditLogSerializerAdapter(serializer, summon[AuditEnvironmentContext])) | ||
| case serializer: tech.beshu.ror.requestcontext.AuditLogSerializer[_] => | ||
| Some(new DeprecatedAuditLogSerializerAdapter(serializer)) | ||
| case _ => None | ||
| } | ||
| } match { | ||
| case Success(Some(customSerializer)) => | ||
| logger.info(s"Using custom serializer: ${customSerializer.getClass.getName}") | ||
| Right(customSerializer) | ||
| case Success(None) => Left(AuditingSettingsCreationError(Message(s"Class ${fullClassName.show} is not a subclass of ${classOf[AuditLogSerializer].getName.show} or ${classOf[tech.beshu.ror.requestcontext.AuditLogSerializer[_]].getName.show}"))) | ||
| case Failure(ex) => Left(AuditingSettingsCreationError(Message(s"Cannot create instance of class '${fullClassName.show}', error: ${ex.getMessage.show}"))) | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve exception handling in createSerializerInstanceFromClassName
The current implementation has several issues:
- Throws uncaught exceptions instead of returning
Left - Re-throws exceptions in the Try.fold error handler
- Could provide more specific error messages
Apply this fix to properly handle exceptions:
private def createSerializerInstanceFromClassName(fullClassName: String)
(using context: AuditEnvironmentContext): Either[AuditingSettingsCreationError, AuditLogSerializer] = {
- val clazz = Try(Class.forName(fullClassName)).fold(
- {
- case _: ClassNotFoundException => throw new IllegalStateException(s"Serializer with class name $fullClassName not found.")
- case other => throw other
- },
- identity
- )
+ Try(Class.forName(fullClassName)) match {
+ case Failure(_: ClassNotFoundException) =>
+ return Left(AuditingSettingsCreationError(Message(s"Serializer class not found: ${fullClassName.show}")))
+ case Failure(ex) =>
+ return Left(AuditingSettingsCreationError(Message(s"Failed to load serializer class ${fullClassName.show}: ${ex.getMessage.show}")))
+ case Success(clazz) =>
+ instantiateSerializer(clazz, fullClassName)
+ }
+}
+private def instantiateSerializer(clazz: Class[_], fullClassName: String)
+ (using context: AuditEnvironmentContext): Either[AuditingSettingsCreationError, AuditLogSerializer] = {
def createInstanceOfEnvironmentAwareSerializer(): Try[Any] =
Try(clazz.getConstructor(classOf[AuditEnvironmentContext])).map(_.newInstance(context))
def createInstanceOfSimpleSerializer(): Try[Any] =
Try(clazz.getDeclaredConstructor()).map(_.newInstance())
- val serializer =
- createInstanceOfEnvironmentAwareSerializer()
- .orElse(createInstanceOfSimpleSerializer())
- .getOrElse(
- throw new IllegalStateException(
- s"Class ${clazz.getName} is required to have either one (AuditEnvironmentContext) parameter constructor or constructor without parameters"
- )
- )
+ val serializerTry = createInstanceOfEnvironmentAwareSerializer()
+ .orElse(createInstanceOfSimpleSerializer())
+
+ serializerTry match {
+ case Failure(ex) =>
+ Left(AuditingSettingsCreationError(Message(
+ s"Class ${clazz.getName.show} requires either a constructor with (AuditEnvironmentContext) parameter or no parameters. Error: ${ex.getMessage.show}"
+ )))
+ case Success(serializer) =>
+ adaptSerializer(serializer, fullClassName)
+ }
+}
+private def adaptSerializer(serializer: Any, fullClassName: String)
+ (using context: AuditEnvironmentContext): Either[AuditingSettingsCreationError, AuditLogSerializer] = {
Try {
serializer match {
case serializer: tech.beshu.ror.audit.AuditLogSerializer =>
Some(serializer)
case serializer: tech.beshu.ror.audit.EnvironmentAwareAuditLogSerializer =>
Some(new EnvironmentAwareAuditLogSerializerAdapter(serializer, summon[AuditEnvironmentContext]))
case serializer: tech.beshu.ror.requestcontext.AuditLogSerializer[_] =>
Some(new DeprecatedAuditLogSerializerAdapter(serializer))
case _ => None
}
} match {
case Success(Some(customSerializer)) =>
logger.info(s"Using custom serializer: ${customSerializer.getClass.getName}")
Right(customSerializer)
case Success(None) => Left(AuditingSettingsCreationError(Message(s"Class ${fullClassName.show} is not a subclass of ${classOf[AuditLogSerializer].getName.show} or ${classOf[tech.beshu.ror.requestcontext.AuditLogSerializer[_]].getName.show}")))
case Failure(ex) => Left(AuditingSettingsCreationError(Message(s"Cannot create instance of class '${fullClassName.show}', error: ${ex.getMessage.show}")))
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In the
core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/AuditingSettingsDecoder.scala
file, around lines 248 to 290, the `createSerializerInstanceFromClassName`
method has several issues with exception handling that should be addressed:
1. The method throws uncaught exceptions instead of returning `Left` in case of
errors. This should be fixed to properly handle errors and return the
appropriate `AuditingSettingsCreationError`.
2. The error handler in the `Try.fold` is re-throwing exceptions, which is not
necessary and can be simplified.
3. The error messages could be more specific to provide better context when an
error occurs.
To fix these issues, update the `createSerializerInstanceFromClassName` method
to:
- Catch and handle all exceptions properly, converting them to
`AuditingSettingsCreationError` instances with specific error messages.
- Remove the unnecessary exception re-throwing in the `Try.fold` error handler.
- Provide more meaningful error messages that give clear context about the
issue.
|
|
||
| object DefaultAuditLogSerializerV2 { | ||
| val defaultV2AuditFields: Map[AuditFieldName, AuditFieldValue] = | ||
| DefaultAuditLogSerializerV1.defaultV1AuditFields ++ environmentRelatedAuditFields |
There was a problem hiding this comment.
Are you sure we should refer to the V1 fields in the V2 implementation?
Maybe it'd be better to redefine them here?
| import tech.beshu.ror.audit.instances.DefaultAuditLogSerializerV2.defaultV2AuditFields | ||
| import tech.beshu.ror.audit.{AuditEnvironmentContext, AuditResponseContext, AuditSerializationHelper} | ||
|
|
||
| class ReportingAllTypesOfEventsAuditLogSerializer(environmentContext: AuditEnvironmentContext) extends DefaultAuditLogSerializer(environmentContext) { |
There was a problem hiding this comment.
I'm still thinking about this name. Because again, I had a problem recalling what "all types of events" mean.
WDYT about sth like this:
- let's rename DefaultAuditLogSerializerVX
SkipAllowedAuditLogSerializerVX(or sth like that) serializer - let's have "an alias" class DefaultAuditLogSerializerVX that extends SkipAllowedAuditLogSerializerVX
- let's rename
ReportingAllTypesOfEventsAuditLogSerializertoFullAuditLogSerializer(or sth like that`
WDYT?
| } | ||
| } | ||
|
|
||
| sealed trait AllowedEventSerializationMode |
There was a problem hiding this comment.
WDYT?
sealed trait AllowedEventMode
object AllowedEventMode {
case object IncludeAll extends AllowedEventMode
case object Include(eventTypes: NonEmptyList[EventType]) extends AllowedEventMode
}
The current version is too concreted. It'd be hard to maintain backward compatibility using it in the future.
| @@ -17,8 +17,16 @@ | |||
| package tech.beshu.ror.audit | |||
|
|
|||
| import org.json.JSONObject | |||
| import tech.beshu.ror.audit.AuditSerializationHelper.AuditFieldName | |||
|
|
|||
| trait EnvironmentAwareAuditLogSerializer { | |||
There was a problem hiding this comment.
I found no implementation of this serializer.
Now, I'm not sure if we still support the changes described in RORDEV-1414.
|
|
||
| class DefaultAuditLogSerializerV2(environmentContext: AuditEnvironmentContext) extends DefaultAuditLogSerializerV1 { | ||
| class DefaultAuditLogSerializerV2(environmentContext: AuditEnvironmentContext) extends AuditLogSerializer { |
There was a problem hiding this comment.
this alias could stay, but we should have a better name for this. See the previous, corresponding comment.
| .mergeWith(requestContext.generalAuditEvents) | ||
| } | ||
|
|
||
| private def resolvePlaceholder(auditValue: AuditFieldValue, |
There was a problem hiding this comment.
Once again, I was fooled by the name: AuditFieldValue ;)
For me, it's rather AuditFieldValueDescriptor. Because, base on this description and the available data, we create value for the audit event document's filed.
TBH, the requestContext, finalState, ..., error values should be gathered in some container for a sake of readability:
private def resolve(valueDescriptor: AuditFieldValueDescriptor, eventData: EventData): Anybut introducing and creating a new type for a sake of internal implementation may not be a base choice. That's why I propose to do it like this:
private def resolver(matched: Boolean,
finalState: String,
reason: String,
duration: FiniteDuration,
requestContext: AuditRequestContext,
environmentContext: Option[AuditEnvironmentContext],
error: Option[Throwable]): AuditFieldValueDescriptor => AnyNow, it's pretty clear that you create a resolver function to get value from the descriptor.
WDYT?
| */ | ||
| package tech.beshu.ror.audit | ||
|
|
||
| private[ror] sealed trait AuditFieldValue |
There was a problem hiding this comment.
private[ror]
Is it enough?
When we extend the sealed trait values set in the future, are we sure that users won't have to recompile their custom serializers?
🚀New (ES) added new audit serializers - ReportingAllEventsAuditLogSerializer, ReportingAllEventsWithQueryAuditLogSerializer
🚀New (ES) There is an option to define custom audit serializer in config, without implementation
Summary by CodeRabbit
Here are the updated release notes based on the provided changes:
New Features
Bug Fixes
Documentation
Refactor
Estimated code review effort: High