OAK-12331: add an audit event SPI with commit-attached capture and observer drain by dulvac · Pull Request #3059 · apache/jackrabbit-oak

dulvac · 2026-07-30T13:22:11Z

Implementation half of OAK-12331. Pairs with #3058, which carries the
documentation; this PR touches no files under oak-doc/.

Draft for now: opening it early for feedback on the SPI shape before the
capture-site coverage grows.

What this adds

A pluggable audit SPI, so a consumer bundle can observe repository
operations without patching Oak internals. Off by default behind the
FT_OAK-12331 toggle. With the toggle off, or with no audit module
deployed, capture short-circuits through a NOOP sink.

The SPI lives in oak-core-spi, in the newly exported package
org.apache.jackrabbit.oak.spi.audit:

AuditEvent / AuditEventImpl: immutable event carrying domain, type,
timestamp and payload
AuditEvents: static facade over a pluggable Sink, NOOP until an audit
module installs one
AuditEventListener: the consumer contract, selecting by domain and
ordering by rank
AuditEventEmitter: OSGi-facing emitter for the fire-and-forget path
AuditBufferLifecycle: session lifecycle callouts used by MutableRoot

oak-security-spi contributes the security domain name and the
user-management type vocabulary, in the newly exported
org.apache.jackrabbit.oak.spi.security.audit plus UserAuditTypes, so
listener bundles compile against a stable set of strings rather than the
producer-side helpers. The pipeline itself sits in oak-core under
org.apache.jackrabbit.oak.security.audit and stays bundle-internal.

How it works

Capture sites buffer events per session in a ThreadLocal.
AuditDrainObserver runs as an Observer on the root NodeStore. Observers
fire after durable persistence and never for a failed merge, so a
dispatched event always corresponds to a persisted write. The drain is
destructive, so a store that invokes the observer twice for one merge
dedupes itself.

CommitMetadataDecorator stamps the three reserved commit.* keys at drain
time and overwrites anything a caller supplied. The fire-and-forget path
strips those keys instead. Their presence is therefore an attestation that
an event came from the commit-attached pipeline, which is what the tests in
CommitMetadataDecoratorTest pin.

An outer Throwable barrier in contentChanged keeps a failing listener
from surfacing as a commit failure to the merge caller. CompositeObserver
gives no per-observer isolation, so without the barrier a bad listener would
also break peer observers such as JCR observation.

MutableRoot drains on refresh() and on a failed merge. It deliberately
does not drain on rebase(), which keeps transient changes and replays them
on the new base; audit events captured alongside those surviving changes
have to survive with them.

AuditConfigurationImpl owns the lifecycle. It registers as
AuditConfiguration only, contributes no commit hooks, and is not a
SecurityConfiguration, so it stays out of
SecurityProvider.getConfiguration(). Under OSGi it publishes the observer
as a service for ObserverTracker; embedded callers get it from
getDrainObserver() and attach it themselves.

Capture sites

Only the two UserManagerImpl.onGroupUpdate overloads so far, covering
single and bulk group membership changes. Path resolution failures there
drop the event and log rather than failing the surrounding update, with
repeat warnings demoted to DEBUG. More capture sites are meant to follow in
separate issues.

Testing

Module	Tests
`oak-core-spi`	243, all passing
`oak-security-spi`	1110, all passing
`oak-core`	4873, all passing
`oak-run-commons`	250, all passing

oak-benchmarks compiles; it gains two runs measuring capture-site and
empty-commit overhead. Also adds an audit-enabled in-memory fixture to
oak-run-commons.

The SecurityProviderRegistrationTest change is a comment plus keeping the
existing count at 6, documenting that audit is deliberately not a
SecurityConfiguration. No assertion was weakened.

Review notes

Both newly exported packages pass the maven-bundle-plugin baseline check.
Worth a look from anyone who has worked on MutableRoot or the observer
chain, since the commit-attached path depends on observer timing
guarantees.

…server drain Adds a pluggable audit SPI so consumers can observe repository operations without patching Oak internals. Off by default behind the FT_OAK-12331 feature toggle. With the toggle off, or with no audit module deployed, the capture path short-circuits through a NOOP sink. New SPI, oak-core-spi (package org.apache.jackrabbit.oak.spi.audit): AuditEvent, AuditEventImpl immutable event: domain, type, timestamp, payload AuditEvents static facade over a pluggable Sink; NOOP until an audit module installs one AuditEventListener consumer contract, selects by domain, orders by rank AuditEventEmitter OSGi-facing emitter for the fire-and-forget path AuditBufferLifecycle session lifecycle callouts used by MutableRoot oak-security-spi contributes the security domain name and the user-management type vocabulary (SecurityAuditDomain, UserAuditTypes) so listener bundles compile against a stable set of strings. Pipeline, oak-core (org.apache.jackrabbit.oak.security.audit): Capture sites buffer events per session in a ThreadLocal. AuditDrainObserver runs as an Observer on the root NodeStore, and because observers fire after durable persistence and never for a failed merge, a dispatched event always corresponds to a persisted write. The drain is destructive, so a store that invokes the observer twice for one merge dedupes itself. CommitMetadataDecorator stamps the three reserved commit.* keys at drain time, overwriting anything a caller supplied; the fire-and-forget path strips those keys instead, so their presence attests that an event came from Oak's commit-attached pipeline. An outer Throwable barrier in contentChanged keeps a failing listener from surfacing as a commit failure to the merge caller. MutableRoot drains on refresh() and on a failed merge, and deliberately does not drain on rebase(), which keeps transient changes and replays them on the new base. The audit events captured alongside those surviving changes have to survive with them. The only instrumented capture sites so far are the two UserManagerImpl onGroupUpdate overloads, covering single and bulk group membership changes. Path resolution failures there drop the event and log rather than failing the surrounding update, with repeat warnings demoted to DEBUG. AuditConfigurationImpl owns the lifecycle. It registers as AuditConfiguration only, contributes no commit hooks, and is not a SecurityConfiguration, so it stays out of SecurityProvider.getConfiguration(). Under OSGi it publishes the observer as a service for ObserverTracker to pick up; embedded callers get it from getDrainObserver() and attach it themselves. Also adds an audit-enabled in-memory fixture to oak-run-commons and two oak-benchmarks runs that measure capture-site and empty-commit overhead. Documentation is submitted separately as a companion change.

dulvac · 2026-07-30T13:35:00Z

implements #3058

joerghoh

A first brief review, will continue tomorrow.

joerghoh · 2026-07-30T19:20:55Z

+     * session slot (re-armed when the slot is recreated after a drain).
+     */
+    private static final class SessionBuffer {
+        final List<AuditEvent> events = new ArrayList<>(4);


Nit: really an ArrayList? Re-allocating that that array if the capacity must be increased (and 4 is smaller than the default of 10) can be expensive.

The common case is that there's one or two events per oak session. Would you prefer I leave the default of 10?

yes, I would leave it with the default.

Done, using the default.

Review feedback from apache#3059. Replaces the raw String domain and type on the audit SPI with AuditDomain and AuditType, so the values can be constrained where they are built rather than trusted everywhere they are read. Both are validated in their static factory: non-blank, no colon, no whitespace, and nothing JcrNameParser rejects as a node name. A listener that persists events into the repository can therefore build a path from a domain without escaping it, which is what prompted the change. The colon is excluded separately because JcrNameParser accepts it as a namespace prefix, and a prefix means nothing for a flat identifier. Embedded whitespace is excluded for the same reason. They are separate types rather than one, so passing a type where a domain belongs no longer compiles. Neither is an enum: consumer bundles define their own domains, so the set is open. The exported constants change type with them, SecurityAuditDomain.NAME becoming SecurityAuditDomain.DOMAIN and the two UserAuditTypes membership constants becoming AuditType. Payload keys stay String, since they are map keys and not identifiers. Also from the same review: - AuditEvent.isCommitAttested(event) replaces the advice that listeners check for the three reserved payload keys themselves. The key names are an implementation detail and a hand-rolled check breaks silently if they move. - Those keys are now oak.commit.* rather than commit.*, matching the oak. prefix the domain constants already use. The decorator reads them from the SPI so the strip path and the attestation check cannot drift apart. - AuditConfigurationImpl is now AuditPipeline. It implements a one-method interface, but its job is owning the toggle, buffer, registry and observer registration. - Dropped the empty @ObjectClassDefinition. It configured nothing, and adding one later is additive. - The activation log line said "activated" next to a bare toggle boolean, which read as a contradiction. It now says whether events will be captured, and the class javadoc separates wired from toggled-on from isActive(). - Product-specific sample domains removed from javadoc, the TOCTOU comment in initialize() cut to the reason for the ordering, and a note added that the toggle starts disabled because Feature backs it with a fresh AtomicBoolean.

Review feedback from apache#3059. Replaces the raw String domain and type on the audit SPI with AuditDomain and AuditType, so the values can be constrained where they are built rather than trusted everywhere they are read. Both are validated in their static factory: non-blank, no colon, no whitespace, and nothing JcrNameParser rejects as a node name. A listener that persists events into the repository can therefore build a path from a domain without escaping it, which is what prompted the change. The colon is excluded separately because JcrNameParser accepts it as a namespace prefix, and a prefix means nothing for a flat identifier. Embedded whitespace is excluded for the same reason. They are separate types rather than one, so passing a type where a domain belongs no longer compiles. Neither is an enum: consumer bundles define their own domains, so the set is open. The exported constants change type with them, SecurityAuditDomain.NAME becoming SecurityAuditDomain.DOMAIN and the two UserAuditTypes membership constants becoming AuditType. Payload keys stay String, since they are map keys and not identifiers. Also from the same review: - AuditEvent.isCommitAttested(event) replaces the advice that listeners check for the three reserved payload keys themselves. The key names are an implementation detail and a hand-rolled check breaks silently if they move. - Those keys are now oak.commit.* rather than commit.*, matching the oak. prefix the domain constants already use. The decorator reads them from the SPI so the strip path and the attestation check cannot drift apart. - AuditConfigurationImpl is now AuditPipeline. It implements a one-method interface, but its job is owning the toggle, buffer, registry and observer registration. - Dropped the empty @ObjectClassDefinition. It configured nothing, and adding one later is additive. - The activation log line said "activated" next to a bare toggle boolean, which read as a contradiction. It now says whether events will be captured, and the class javadoc separates wired from toggled-on from isActive(). - Product-specific sample domains removed from javadoc, the TOCTOU comment in initialize() cut to the reason for the ordering, and a note added that the toggle starts disabled because Feature backs it with a fresh AtomicBoolean. Co-authored-by: Jörg Hoh <joerghoh@users.noreply.github.com>

Requested in review on PR apache#3058: there was no way to see the pipeline from outside, and a slow listener runs on the commit thread, so it costs commit latency with nothing to point at. AuditMonitor wraps a StatisticsProvider and records: - security.audit.events;domain=<domain> — events dispatched per domain - security.audit.events.dropped;domain=<domain> — events discarded at the per-session buffer cap - security.audit.listener.duration;listener=<class> — time in onEvents - security.audit.listener.failures;listener=<class> — listener throws The provider is looked up on the whiteboard rather than injected as a DS reference, so OSGi and embedded callers share the one path through initialize(). Deployments without a provider get AuditMonitor.NOOP. Two counting decisions worth knowing about. An event is counted once per domain, not once per delivery, so N listeners on a domain do not multiply the rate. And it is counted only when a listener actually consumed it: a listener that unregisters between capture and drain leaves a domain in the grouped map that nothing consumed, and counting that would overstate the rate. Both are covered by AuditMonitorWiringTest. Recording sits inside the existing per-listener Throwable barriers, so a metrics failure cannot break a dispatch. A listener that throws is still timed: it burned commit-thread time before it threw.

Follow-up to the review on PR apache#3058: - Define "capture site" on first use, instead of leaving the term to context. - Say explicitly that the drain hangs off the commit rather than off Session.save(), and name the operations that commit implicitly. - Scope the segment-store dispatch caveat to the segment store, and state that it does not apply to the document or composite stores. - Add a clustering section: events are node-local, what that means for a cluster-wide listener, and where the node id comes from. - Replace the hand-rolled "check for three payload keys" advice with AuditEvents.hasCommitMetadata(event), and publish the key names as constants on AuditEvent. - Add a monitoring section covering the per-domain event meter, the per-listener timer and failure meter, and the dropped-event meter. - Move Design rules to the top of the Implementation chapter and restate its back-references so each rule stands alone. The SPI helper, the key constants, and the metrics are documented here but implemented in the companion PR apache#3059.

The design doc named AuditDomain and AuditType without saying why they exist. The rationale is from the review on PR apache#3059: constraining the value at construction keeps a domain usable as a JCR node name, so a listener that persists events into the repository can build a path from it without escaping. Records the actual rules (non-blank, JcrNameParser, no colon, no whitespace), why the colon is rejected rather than escaped, and why neither type is an enum. Also fixes a bullet list in audit.md that an earlier edit had split in two, orphaning Timestamp and Payload below a paragraph.

dulvac · 2026-08-05T11:20:14Z

@joerghoh thanks for the review. I addressed your feedback, minus two open questions - the package scope and arraylist default size.

joerghoh · 2026-08-05T11:02:33Z

+     * session slot (re-armed when the slot is recreated after a drain).
+     */
+    private static final class SessionBuffer {
+        final List<AuditEvent> events = new ArrayList<>(4);


yes, I would leave it with the default.

joerghoh · 2026-08-05T11:08:08Z

+     * @return an immutable copy of the staged events, or {@code null} when
+     *         nothing was staged for the session on the current thread.
+     */
+    @Nullable


Why Nullable? This would a perfect case to return an empty list; which eliminates the need for explicit null checks.

Agreed. Both peek and drain return List.of() now. The only production caller had a redundant events == null || in front of its isEmpty(), and that is gone.

joerghoh · 2026-08-05T11:08:45Z

+     * @return the staged events, or {@code null} when nothing was
+     *         staged for the session on the current thread.
+     */
+    @Nullable


Same as above: why Nullable?

Covered with peek above.

From the review on PR apache#3059: - Move AuditEventImpl into spi.audit.impl, which is not exported. The class was already package-private, so it never reached the API surface, but BND computes the baseline version per package: editing it could have bumped the exported package's version with no API change. It has to be public now, since AuditEvent.of in the parent package constructs it. - Rename AuditEvents to AuditDispatch. Oak's plural-facade convention (PropertyValues, NodeStates) means "factory for the singular type", and this class never builds an AuditEvent — AuditEvent.of does. It installs a sink and routes, so name it for that. - Return an empty list from AuditBuffer.peek and drain instead of null. The one production caller had a redundant null check; the tests now assert emptiness rather than null. - Drop the explicit ArrayList capacity in SessionBuffer and use the default. - Say "unmodifiable shallow copy" on peek rather than "defensive copy". The reviewer read it as a deep copy; List.copyOf is not one, and the shared AuditEvent instances are safe because events are immutable. The AuditDomain / AuditType value types the same review asked for landed earlier in c955987.

dulvac · 2026-08-06T14:14:27Z

Second round is in.

AuditEventImpl moved to spi.audit.impl, which is not exported.
AuditEvents renamed to AuditDispatch.
AuditBuffer.peek and drain return empty lists instead of null.
SessionBuffer uses the default ArrayList capacity.
The "defensive copy" wording is fixed, though to shallow rather than deep. Details in the thread.

AuditDomain and AuditType went in earlier, in c955987.

2 things left so far, @joerghoh: whether you want Instant instead of long on getTimestamp(). And an ack if you agree with the AuditDispatch naming.

joerghoh · 2026-08-07T15:50:35Z

@dulvac if Oak prefers long for timestamps, then let's use that. +1 to AuditDispatch

joerghoh reviewed Jul 30, 2026

View reviewed changes

joerghoh reviewed Jul 31, 2026

View reviewed changes

dulvac mentioned this pull request Aug 4, 2026

OAK-12331: document the audit SPI #3058

Draft

dulvac force-pushed the issue/OAK-12331-impl branch from 4c3c1bb to c955987 Compare August 4, 2026 11:57

joerghoh reviewed Aug 5, 2026

View reviewed changes

Uh oh!

Conversation

dulvac commented Jul 30, 2026

What this adds

How it works

Capture sites

Testing

Review notes

Uh oh!

dulvac commented Jul 30, 2026

Uh oh!

joerghoh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dulvac commented Aug 5, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dulvac commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joerghoh commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dulvac commented Aug 6, 2026 •

edited

Loading