joerghoh commented on code in PR #3059: URL: https://github.com/apache/jackrabbit-oak/pull/3059#discussion_r3689063603
########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/audit/AuditConfigurationImpl.java: ########## @@ -0,0 +1,493 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.security.audit; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.osgi.OsgiWhiteboard; +import org.apache.jackrabbit.oak.spi.audit.AuditBufferLifecycle; +import org.apache.jackrabbit.oak.spi.audit.AuditConfiguration; +import org.apache.jackrabbit.oak.spi.audit.AuditEvent; +import org.apache.jackrabbit.oak.spi.audit.AuditEventListener; +import org.apache.jackrabbit.oak.spi.audit.AuditEvents; +import org.apache.jackrabbit.oak.spi.commit.Observer; +import org.apache.jackrabbit.oak.spi.toggle.Feature; +import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.BundleContext; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Deactivate; +import org.osgi.service.metatype.annotations.Designate; +import org.osgi.service.metatype.annotations.ObjectClassDefinition; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default {@link AuditConfiguration} implementation. Contributes the + * audit pipeline to Oak: + * <ul> + * <li>Registers a {@link Feature} toggle gating capture and dispatch.</li> + * <li>Installs a per-session buffer ({@link AuditBuffer}) into + * {@link AuditBufferLifecycle}.</li> + * <li>Installs the {@link AuditEvents} sink that routes capture-site + * calls into the buffer.</li> + * <li>Tracks {@code AuditEventListener} services on the Whiteboard + * via {@link WhiteboardAuditEventListenerRegistry}.</li> + * <li>Registers an {@link AuditDrainObserver} as an OSGi {@link Observer} + * service; Oak's {@code ObserverTracker} (in {@code oak-jcr}'s + * {@code RepositoryManager}) picks it up and subscribes it to the root + * NodeStore. The observer drains the buffer on commit success and + * dispatches events to listeners.</li> + * </ul> + * Registered as {@link AuditConfiguration} only — not a + * {@code SecurityConfiguration}, contributes no commit hooks, not reachable + * via {@code SecurityProvider.getConfiguration(AuditConfiguration.class)}. + * Embedded callers obtain the drain observer via {@link #getDrainObserver()}. + * <p> + * When the feature toggle is disabled, capture is a no-op and the observer + * short-circuits — see {@link AuditEvents#isEnabled()} and + * {@link AuditDrainObserver#contentChanged}. + */ +@Component(service = AuditConfiguration.class) +@Designate(ocd = AuditConfigurationImpl.Configuration.class) +public class AuditConfigurationImpl implements AuditConfiguration { Review Comment: In my opinion the class name should be different; its main purpose is not to be the implementation class of an AuditConfiguration interface, but rather the implementation of a pipeline (see the javadoc above). ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/audit/AuditConfigurationImpl.java: ########## @@ -0,0 +1,493 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.security.audit; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.osgi.OsgiWhiteboard; +import org.apache.jackrabbit.oak.spi.audit.AuditBufferLifecycle; +import org.apache.jackrabbit.oak.spi.audit.AuditConfiguration; +import org.apache.jackrabbit.oak.spi.audit.AuditEvent; +import org.apache.jackrabbit.oak.spi.audit.AuditEventListener; +import org.apache.jackrabbit.oak.spi.audit.AuditEvents; +import org.apache.jackrabbit.oak.spi.commit.Observer; +import org.apache.jackrabbit.oak.spi.toggle.Feature; +import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.BundleContext; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Deactivate; +import org.osgi.service.metatype.annotations.Designate; +import org.osgi.service.metatype.annotations.ObjectClassDefinition; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default {@link AuditConfiguration} implementation. Contributes the + * audit pipeline to Oak: + * <ul> + * <li>Registers a {@link Feature} toggle gating capture and dispatch.</li> + * <li>Installs a per-session buffer ({@link AuditBuffer}) into + * {@link AuditBufferLifecycle}.</li> + * <li>Installs the {@link AuditEvents} sink that routes capture-site + * calls into the buffer.</li> + * <li>Tracks {@code AuditEventListener} services on the Whiteboard + * via {@link WhiteboardAuditEventListenerRegistry}.</li> + * <li>Registers an {@link AuditDrainObserver} as an OSGi {@link Observer} + * service; Oak's {@code ObserverTracker} (in {@code oak-jcr}'s + * {@code RepositoryManager}) picks it up and subscribes it to the root + * NodeStore. The observer drains the buffer on commit success and + * dispatches events to listeners.</li> + * </ul> + * Registered as {@link AuditConfiguration} only — not a + * {@code SecurityConfiguration}, contributes no commit hooks, not reachable + * via {@code SecurityProvider.getConfiguration(AuditConfiguration.class)}. + * Embedded callers obtain the drain observer via {@link #getDrainObserver()}. + * <p> + * When the feature toggle is disabled, capture is a no-op and the observer + * short-circuits — see {@link AuditEvents#isEnabled()} and + * {@link AuditDrainObserver#contentChanged}. + */ +@Component(service = AuditConfiguration.class) +@Designate(ocd = AuditConfigurationImpl.Configuration.class) +public class AuditConfigurationImpl implements AuditConfiguration { + + /** + * Feature toggle name, following the {@code FT_OAK-<issue>} convention + * in {@code AGENTS.md}. Disabled by default: this is a new feature, + * not a bug fix. + * <p> + * <strong>Why not on the public SPI interface + * ({@link AuditConfiguration}):</strong> moving this constant to the + * SPI would commit the literal value to the public surface forever. + * It stays impl-local; promoting it later is a binary-additive change + * if a need arises. + */ + public static final String FEATURE_TOGGLE_NAME = "FT_OAK-12331"; + + @ObjectClassDefinition(name = "Apache Jackrabbit Oak AuditConfiguration", + description = "Audit event pipeline. Capture and dispatch are " + + "gated by the '" + FEATURE_TOGGLE_NAME + "' feature toggle " + + "(disabled by default).") + @interface Configuration { + // Configuration is currently empty by design: capture/dispatch behavior + // is controlled exclusively by the feature toggle. Listeners are + // contributed via OSGi services / the Whiteboard. + } Review Comment: This is redundant, and you can always add such a configuration later if you really need it. I would remove it. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/audit/AuditConfigurationImpl.java: ########## @@ -0,0 +1,493 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.security.audit; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.osgi.OsgiWhiteboard; +import org.apache.jackrabbit.oak.spi.audit.AuditBufferLifecycle; +import org.apache.jackrabbit.oak.spi.audit.AuditConfiguration; +import org.apache.jackrabbit.oak.spi.audit.AuditEvent; +import org.apache.jackrabbit.oak.spi.audit.AuditEventListener; +import org.apache.jackrabbit.oak.spi.audit.AuditEvents; +import org.apache.jackrabbit.oak.spi.commit.Observer; +import org.apache.jackrabbit.oak.spi.toggle.Feature; +import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.BundleContext; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Deactivate; +import org.osgi.service.metatype.annotations.Designate; +import org.osgi.service.metatype.annotations.ObjectClassDefinition; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default {@link AuditConfiguration} implementation. Contributes the + * audit pipeline to Oak: + * <ul> + * <li>Registers a {@link Feature} toggle gating capture and dispatch.</li> + * <li>Installs a per-session buffer ({@link AuditBuffer}) into + * {@link AuditBufferLifecycle}.</li> + * <li>Installs the {@link AuditEvents} sink that routes capture-site + * calls into the buffer.</li> + * <li>Tracks {@code AuditEventListener} services on the Whiteboard + * via {@link WhiteboardAuditEventListenerRegistry}.</li> + * <li>Registers an {@link AuditDrainObserver} as an OSGi {@link Observer} + * service; Oak's {@code ObserverTracker} (in {@code oak-jcr}'s + * {@code RepositoryManager}) picks it up and subscribes it to the root + * NodeStore. The observer drains the buffer on commit success and + * dispatches events to listeners.</li> + * </ul> + * Registered as {@link AuditConfiguration} only — not a + * {@code SecurityConfiguration}, contributes no commit hooks, not reachable + * via {@code SecurityProvider.getConfiguration(AuditConfiguration.class)}. + * Embedded callers obtain the drain observer via {@link #getDrainObserver()}. + * <p> + * When the feature toggle is disabled, capture is a no-op and the observer + * short-circuits — see {@link AuditEvents#isEnabled()} and + * {@link AuditDrainObserver#contentChanged}. + */ +@Component(service = AuditConfiguration.class) +@Designate(ocd = AuditConfigurationImpl.Configuration.class) +public class AuditConfigurationImpl implements AuditConfiguration { + + /** + * Feature toggle name, following the {@code FT_OAK-<issue>} convention + * in {@code AGENTS.md}. Disabled by default: this is a new feature, + * not a bug fix. + * <p> + * <strong>Why not on the public SPI interface + * ({@link AuditConfiguration}):</strong> moving this constant to the + * SPI would commit the literal value to the public surface forever. + * It stays impl-local; promoting it later is a binary-additive change + * if a need arises. + */ + public static final String FEATURE_TOGGLE_NAME = "FT_OAK-12331"; + + @ObjectClassDefinition(name = "Apache Jackrabbit Oak AuditConfiguration", + description = "Audit event pipeline. Capture and dispatch are " + + "gated by the '" + FEATURE_TOGGLE_NAME + "' feature toggle " + + "(disabled by default).") + @interface Configuration { + // Configuration is currently empty by design: capture/dispatch behavior + // is controlled exclusively by the feature toggle. Listeners are + // contributed via OSGi services / the Whiteboard. + } + + private static final Logger log = LoggerFactory.getLogger(AuditConfigurationImpl.class); + + // Package-private (not private) on the internal-state fields so + // AuditConfigurationImplTest can mock-replace them to verify the + // dispose-order invariant via Mockito InOrder. Production callers MUST + // NOT touch these fields directly — go through initialize() / dispose(). + // + // JMM-safety. featureToggle, buffer, registry, and drainObserver are + // package-private and non-volatile by design. The invariant they rely on: + // they are mutated only by initialize(Whiteboard) and dispose(), which + // the contract specifies must each run exactly once and on the same + // thread; reads happen either + // (a) on that same thread — the static AuditEvents.record / dispatch + // facade reads through the AuditEvents.sink field (itself volatile, + // providing the publication barrier), and getDrainObserver() is + // called by activate() on the SCR thread AFTER initialize() on the + // SCR thread; or + // (b) on a commit thread that arrives via Observer.contentChanged, + // after the ServiceRegistration publication barrier established + // by BundleContext.registerService in activate(). + // Both paths satisfy the JMM happens-before contract without per-field + // volatile. If a future change adds a "share the singleton across + // pipelines" pattern OR cross-thread mutation of these fields, this + // invariant breaks — at that point the fields MUST be made volatile (or + // properly immutable via constructor injection). + Feature featureToggle; + AuditBuffer buffer; + WhiteboardAuditEventListenerRegistry registry; + + /** + * Singleton {@link AuditDrainObserver} instance constructed by + * {@link #initialize(Whiteboard)} and zeroed by {@link #dispose()}. + * Exposed via {@link #getDrainObserver()} as an {@link Observer}. + * <p> + * Singleton-not-factory by design: each {@code AuditConfigurationImpl} + * owns at most one Observer because (a) the {@link AuditBuffer} + * {@code ThreadLocal} is buffer-instance-scoped, so multiple Observer + * instances would compete for the same drain, and (b) the destructive + * {@code buffer.drain(sessionId)} contract is the cleanup mechanism — + * double-attach would mask future non-destructive-drain refactors. + */ + AuditDrainObserver drainObserver; + + /** + * OSGi service registration for the {@link AuditDrainObserver}. Held + * so {@link #deactivate} can unregister and let {@code ObserverTracker} + * close its subscription on the root NodeStore. {@code null} outside + * the OSGi-active window; embedded callers manage observer lifetime + * through their own {@code ((Observable) store).addObserver(...)} call + * (see class Javadoc). + */ + ServiceRegistration<?> observerRegistration; + + public AuditConfigurationImpl() { + super(); + } + + @SuppressWarnings("UnusedDeclaration") + @Activate + private void activate(@NotNull Configuration configuration, + @NotNull BundleContext bundleContext, + @NotNull Map<String, Object> properties) { + // Step 1-4: install sinks/registry/buffer/toggle. Capture-site + // record(...) calls reach the buffer as soon as initialize returns. + // The singleton AuditDrainObserver is also constructed inside + // initialize() (step 5 below) so the impl is fully wired before + // we publish anything externally. + initialize(new OsgiWhiteboard(bundleContext)); + // Step 6 LAST: publish the Observer service. ObserverTracker + // (oak-store-spi/.../spi/commit/ObserverTracker.java, instantiated + // per-NodeStoreService in DocumentNodeStoreService, SegmentNodeStoreRegistrar, + // CompositeNodeStoreService) subscribes it to the root NodeStore. + // Any commit thread racing with activation that reaches step 6 before + // ObserverTracker has noticed the service will simply miss the drain + // on this one commit — events stay in the per-thread buffer until the + // next commit on the same session. No correctness risk. + observerRegistration = bundleContext.registerService( + Observer.class.getName(), getDrainObserver(), null); + } + + /** + * Non-OSGi entry point for wiring up the audit pipeline. Called by + * {@link #activate} in OSGi deployments after the {@code BundleContext} + * has been unwrapped into an {@code OsgiWhiteboard}, and by embedded + * callers (tests, {@code OakFixture}) directly. + * <p> + * <strong>Embedded callers must follow up with + * {@link #getDrainObserver()}</strong> to obtain the Observer and attach + * it to the root NodeStore. See {@link #getDrainObserver()} Javadoc for + * the recommended attach pattern and the {@code Oak.with(Observer)} + * caveat. + * <p> + * <strong>Must be called exactly once per instance.</strong> Calling + * it more than once orphans the previous {@code Feature} toggle and + * registry tracker, and silently overwrites the static + * {@link AuditEvents} / {@link AuditBufferLifecycle} sinks. To rewire, + * call {@link #dispose()} first. + * <p> + * <strong>Activation ordering rationale.</strong> + * {@link AuditBufferLifecycle#install AuditBufferLifecycle.install(buffer)} + * runs before + * {@link AuditEvents#install AuditEvents.install(BufferSink)} so that any + * concurrent capture arriving in the install window goes through the + * NOOP sink (no buffer write) rather than through a live {@code BufferSink} + * with an orphaned lifecycle handle. The inverse ordering would minimize + * lifecycle bypass but maximize silent capture loss; we prefer the former. + * + * @param whiteboard the whiteboard to register the {@code Feature} + * toggle and {@code AuditEventListener} tracker on; + * non-null. + */ + public void initialize(@NotNull Whiteboard whiteboard) { + featureToggle = Feature.newFeature(FEATURE_TOGGLE_NAME, whiteboard); + + registry = new WhiteboardAuditEventListenerRegistry(); + registry.start(whiteboard); + + buffer = new AuditBuffer(); + AuditBufferLifecycle.install(buffer); + + AuditEvents.install(new BufferSink(featureToggle, registry, buffer)); + + // Construct the singleton AuditDrainObserver LAST in initialize(). + // This closes a potential TOCTOU window in the OSGi @Activate flow: + // ObserverTracker (which subscribes to Observer services on a + // background thread) might fire the bootstrap CommitInfo.EMPTY_EXTERNAL + // invocation before @Activate returns. With the singleton constructed + // here (inside initialize, which @Activate calls FIRST), getDrainObserver() + // is safe to call from anywhere in @Activate after this point. Review Comment: Is this deep explanation really necessary here? ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/audit/AuditConfigurationImpl.java: ########## @@ -0,0 +1,493 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.security.audit; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.osgi.OsgiWhiteboard; +import org.apache.jackrabbit.oak.spi.audit.AuditBufferLifecycle; +import org.apache.jackrabbit.oak.spi.audit.AuditConfiguration; +import org.apache.jackrabbit.oak.spi.audit.AuditEvent; +import org.apache.jackrabbit.oak.spi.audit.AuditEventListener; +import org.apache.jackrabbit.oak.spi.audit.AuditEvents; +import org.apache.jackrabbit.oak.spi.commit.Observer; +import org.apache.jackrabbit.oak.spi.toggle.Feature; +import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.BundleContext; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Deactivate; +import org.osgi.service.metatype.annotations.Designate; +import org.osgi.service.metatype.annotations.ObjectClassDefinition; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default {@link AuditConfiguration} implementation. Contributes the + * audit pipeline to Oak: + * <ul> + * <li>Registers a {@link Feature} toggle gating capture and dispatch.</li> + * <li>Installs a per-session buffer ({@link AuditBuffer}) into + * {@link AuditBufferLifecycle}.</li> + * <li>Installs the {@link AuditEvents} sink that routes capture-site + * calls into the buffer.</li> + * <li>Tracks {@code AuditEventListener} services on the Whiteboard + * via {@link WhiteboardAuditEventListenerRegistry}.</li> + * <li>Registers an {@link AuditDrainObserver} as an OSGi {@link Observer} + * service; Oak's {@code ObserverTracker} (in {@code oak-jcr}'s + * {@code RepositoryManager}) picks it up and subscribes it to the root + * NodeStore. The observer drains the buffer on commit success and + * dispatches events to listeners.</li> + * </ul> + * Registered as {@link AuditConfiguration} only — not a + * {@code SecurityConfiguration}, contributes no commit hooks, not reachable + * via {@code SecurityProvider.getConfiguration(AuditConfiguration.class)}. + * Embedded callers obtain the drain observer via {@link #getDrainObserver()}. + * <p> + * When the feature toggle is disabled, capture is a no-op and the observer + * short-circuits — see {@link AuditEvents#isEnabled()} and + * {@link AuditDrainObserver#contentChanged}. + */ +@Component(service = AuditConfiguration.class) +@Designate(ocd = AuditConfigurationImpl.Configuration.class) +public class AuditConfigurationImpl implements AuditConfiguration { + + /** + * Feature toggle name, following the {@code FT_OAK-<issue>} convention + * in {@code AGENTS.md}. Disabled by default: this is a new feature, + * not a bug fix. + * <p> + * <strong>Why not on the public SPI interface + * ({@link AuditConfiguration}):</strong> moving this constant to the + * SPI would commit the literal value to the public surface forever. + * It stays impl-local; promoting it later is a binary-additive change + * if a need arises. + */ + public static final String FEATURE_TOGGLE_NAME = "FT_OAK-12331"; + + @ObjectClassDefinition(name = "Apache Jackrabbit Oak AuditConfiguration", + description = "Audit event pipeline. Capture and dispatch are " + + "gated by the '" + FEATURE_TOGGLE_NAME + "' feature toggle " + + "(disabled by default).") + @interface Configuration { + // Configuration is currently empty by design: capture/dispatch behavior + // is controlled exclusively by the feature toggle. Listeners are + // contributed via OSGi services / the Whiteboard. + } + + private static final Logger log = LoggerFactory.getLogger(AuditConfigurationImpl.class); + + // Package-private (not private) on the internal-state fields so + // AuditConfigurationImplTest can mock-replace them to verify the + // dispose-order invariant via Mockito InOrder. Production callers MUST + // NOT touch these fields directly — go through initialize() / dispose(). + // + // JMM-safety. featureToggle, buffer, registry, and drainObserver are + // package-private and non-volatile by design. The invariant they rely on: + // they are mutated only by initialize(Whiteboard) and dispose(), which + // the contract specifies must each run exactly once and on the same + // thread; reads happen either + // (a) on that same thread — the static AuditEvents.record / dispatch + // facade reads through the AuditEvents.sink field (itself volatile, + // providing the publication barrier), and getDrainObserver() is + // called by activate() on the SCR thread AFTER initialize() on the + // SCR thread; or + // (b) on a commit thread that arrives via Observer.contentChanged, + // after the ServiceRegistration publication barrier established + // by BundleContext.registerService in activate(). + // Both paths satisfy the JMM happens-before contract without per-field + // volatile. If a future change adds a "share the singleton across + // pipelines" pattern OR cross-thread mutation of these fields, this + // invariant breaks — at that point the fields MUST be made volatile (or + // properly immutable via constructor injection). + Feature featureToggle; + AuditBuffer buffer; + WhiteboardAuditEventListenerRegistry registry; + + /** + * Singleton {@link AuditDrainObserver} instance constructed by + * {@link #initialize(Whiteboard)} and zeroed by {@link #dispose()}. + * Exposed via {@link #getDrainObserver()} as an {@link Observer}. + * <p> + * Singleton-not-factory by design: each {@code AuditConfigurationImpl} + * owns at most one Observer because (a) the {@link AuditBuffer} + * {@code ThreadLocal} is buffer-instance-scoped, so multiple Observer + * instances would compete for the same drain, and (b) the destructive + * {@code buffer.drain(sessionId)} contract is the cleanup mechanism — + * double-attach would mask future non-destructive-drain refactors. + */ + AuditDrainObserver drainObserver; + + /** + * OSGi service registration for the {@link AuditDrainObserver}. Held + * so {@link #deactivate} can unregister and let {@code ObserverTracker} + * close its subscription on the root NodeStore. {@code null} outside + * the OSGi-active window; embedded callers manage observer lifetime + * through their own {@code ((Observable) store).addObserver(...)} call + * (see class Javadoc). + */ + ServiceRegistration<?> observerRegistration; + + public AuditConfigurationImpl() { + super(); + } + + @SuppressWarnings("UnusedDeclaration") + @Activate + private void activate(@NotNull Configuration configuration, + @NotNull BundleContext bundleContext, + @NotNull Map<String, Object> properties) { + // Step 1-4: install sinks/registry/buffer/toggle. Capture-site + // record(...) calls reach the buffer as soon as initialize returns. + // The singleton AuditDrainObserver is also constructed inside + // initialize() (step 5 below) so the impl is fully wired before + // we publish anything externally. + initialize(new OsgiWhiteboard(bundleContext)); + // Step 6 LAST: publish the Observer service. ObserverTracker + // (oak-store-spi/.../spi/commit/ObserverTracker.java, instantiated + // per-NodeStoreService in DocumentNodeStoreService, SegmentNodeStoreRegistrar, + // CompositeNodeStoreService) subscribes it to the root NodeStore. + // Any commit thread racing with activation that reaches step 6 before + // ObserverTracker has noticed the service will simply miss the drain + // on this one commit — events stay in the per-thread buffer until the + // next commit on the same session. No correctness risk. + observerRegistration = bundleContext.registerService( + Observer.class.getName(), getDrainObserver(), null); + } + + /** + * Non-OSGi entry point for wiring up the audit pipeline. Called by + * {@link #activate} in OSGi deployments after the {@code BundleContext} + * has been unwrapped into an {@code OsgiWhiteboard}, and by embedded + * callers (tests, {@code OakFixture}) directly. + * <p> + * <strong>Embedded callers must follow up with + * {@link #getDrainObserver()}</strong> to obtain the Observer and attach + * it to the root NodeStore. See {@link #getDrainObserver()} Javadoc for + * the recommended attach pattern and the {@code Oak.with(Observer)} + * caveat. + * <p> + * <strong>Must be called exactly once per instance.</strong> Calling + * it more than once orphans the previous {@code Feature} toggle and + * registry tracker, and silently overwrites the static + * {@link AuditEvents} / {@link AuditBufferLifecycle} sinks. To rewire, + * call {@link #dispose()} first. + * <p> + * <strong>Activation ordering rationale.</strong> + * {@link AuditBufferLifecycle#install AuditBufferLifecycle.install(buffer)} + * runs before + * {@link AuditEvents#install AuditEvents.install(BufferSink)} so that any + * concurrent capture arriving in the install window goes through the + * NOOP sink (no buffer write) rather than through a live {@code BufferSink} + * with an orphaned lifecycle handle. The inverse ordering would minimize + * lifecycle bypass but maximize silent capture loss; we prefer the former. + * + * @param whiteboard the whiteboard to register the {@code Feature} + * toggle and {@code AuditEventListener} tracker on; + * non-null. + */ + public void initialize(@NotNull Whiteboard whiteboard) { + featureToggle = Feature.newFeature(FEATURE_TOGGLE_NAME, whiteboard); Review Comment: Above it's stated that this feature is disabled by default. Where exactly is this default value set? if it's not explicitly set (but the default behavior of a ``Feature``) please add a brief comment here. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/audit/AuditConfigurationImpl.java: ########## @@ -0,0 +1,493 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.security.audit; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.osgi.OsgiWhiteboard; +import org.apache.jackrabbit.oak.spi.audit.AuditBufferLifecycle; +import org.apache.jackrabbit.oak.spi.audit.AuditConfiguration; +import org.apache.jackrabbit.oak.spi.audit.AuditEvent; +import org.apache.jackrabbit.oak.spi.audit.AuditEventListener; +import org.apache.jackrabbit.oak.spi.audit.AuditEvents; +import org.apache.jackrabbit.oak.spi.commit.Observer; +import org.apache.jackrabbit.oak.spi.toggle.Feature; +import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.BundleContext; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Deactivate; +import org.osgi.service.metatype.annotations.Designate; +import org.osgi.service.metatype.annotations.ObjectClassDefinition; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default {@link AuditConfiguration} implementation. Contributes the + * audit pipeline to Oak: + * <ul> + * <li>Registers a {@link Feature} toggle gating capture and dispatch.</li> + * <li>Installs a per-session buffer ({@link AuditBuffer}) into + * {@link AuditBufferLifecycle}.</li> + * <li>Installs the {@link AuditEvents} sink that routes capture-site + * calls into the buffer.</li> + * <li>Tracks {@code AuditEventListener} services on the Whiteboard + * via {@link WhiteboardAuditEventListenerRegistry}.</li> + * <li>Registers an {@link AuditDrainObserver} as an OSGi {@link Observer} + * service; Oak's {@code ObserverTracker} (in {@code oak-jcr}'s + * {@code RepositoryManager}) picks it up and subscribes it to the root + * NodeStore. The observer drains the buffer on commit success and + * dispatches events to listeners.</li> + * </ul> + * Registered as {@link AuditConfiguration} only — not a + * {@code SecurityConfiguration}, contributes no commit hooks, not reachable + * via {@code SecurityProvider.getConfiguration(AuditConfiguration.class)}. + * Embedded callers obtain the drain observer via {@link #getDrainObserver()}. + * <p> + * When the feature toggle is disabled, capture is a no-op and the observer + * short-circuits — see {@link AuditEvents#isEnabled()} and + * {@link AuditDrainObserver#contentChanged}. + */ +@Component(service = AuditConfiguration.class) +@Designate(ocd = AuditConfigurationImpl.Configuration.class) +public class AuditConfigurationImpl implements AuditConfiguration { + + /** + * Feature toggle name, following the {@code FT_OAK-<issue>} convention + * in {@code AGENTS.md}. Disabled by default: this is a new feature, + * not a bug fix. + * <p> + * <strong>Why not on the public SPI interface + * ({@link AuditConfiguration}):</strong> moving this constant to the + * SPI would commit the literal value to the public surface forever. + * It stays impl-local; promoting it later is a binary-additive change + * if a need arises. + */ + public static final String FEATURE_TOGGLE_NAME = "FT_OAK-12331"; + + @ObjectClassDefinition(name = "Apache Jackrabbit Oak AuditConfiguration", + description = "Audit event pipeline. Capture and dispatch are " + + "gated by the '" + FEATURE_TOGGLE_NAME + "' feature toggle " + + "(disabled by default).") + @interface Configuration { + // Configuration is currently empty by design: capture/dispatch behavior + // is controlled exclusively by the feature toggle. Listeners are + // contributed via OSGi services / the Whiteboard. + } + + private static final Logger log = LoggerFactory.getLogger(AuditConfigurationImpl.class); + + // Package-private (not private) on the internal-state fields so + // AuditConfigurationImplTest can mock-replace them to verify the + // dispose-order invariant via Mockito InOrder. Production callers MUST + // NOT touch these fields directly — go through initialize() / dispose(). + // + // JMM-safety. featureToggle, buffer, registry, and drainObserver are + // package-private and non-volatile by design. The invariant they rely on: + // they are mutated only by initialize(Whiteboard) and dispose(), which + // the contract specifies must each run exactly once and on the same + // thread; reads happen either + // (a) on that same thread — the static AuditEvents.record / dispatch + // facade reads through the AuditEvents.sink field (itself volatile, + // providing the publication barrier), and getDrainObserver() is + // called by activate() on the SCR thread AFTER initialize() on the + // SCR thread; or + // (b) on a commit thread that arrives via Observer.contentChanged, + // after the ServiceRegistration publication barrier established + // by BundleContext.registerService in activate(). + // Both paths satisfy the JMM happens-before contract without per-field + // volatile. If a future change adds a "share the singleton across + // pipelines" pattern OR cross-thread mutation of these fields, this + // invariant breaks — at that point the fields MUST be made volatile (or + // properly immutable via constructor injection). + Feature featureToggle; + AuditBuffer buffer; + WhiteboardAuditEventListenerRegistry registry; + + /** + * Singleton {@link AuditDrainObserver} instance constructed by + * {@link #initialize(Whiteboard)} and zeroed by {@link #dispose()}. + * Exposed via {@link #getDrainObserver()} as an {@link Observer}. + * <p> + * Singleton-not-factory by design: each {@code AuditConfigurationImpl} + * owns at most one Observer because (a) the {@link AuditBuffer} + * {@code ThreadLocal} is buffer-instance-scoped, so multiple Observer + * instances would compete for the same drain, and (b) the destructive + * {@code buffer.drain(sessionId)} contract is the cleanup mechanism — + * double-attach would mask future non-destructive-drain refactors. + */ + AuditDrainObserver drainObserver; + + /** + * OSGi service registration for the {@link AuditDrainObserver}. Held + * so {@link #deactivate} can unregister and let {@code ObserverTracker} + * close its subscription on the root NodeStore. {@code null} outside + * the OSGi-active window; embedded callers manage observer lifetime + * through their own {@code ((Observable) store).addObserver(...)} call + * (see class Javadoc). + */ + ServiceRegistration<?> observerRegistration; + + public AuditConfigurationImpl() { + super(); + } + + @SuppressWarnings("UnusedDeclaration") + @Activate + private void activate(@NotNull Configuration configuration, + @NotNull BundleContext bundleContext, + @NotNull Map<String, Object> properties) { + // Step 1-4: install sinks/registry/buffer/toggle. Capture-site + // record(...) calls reach the buffer as soon as initialize returns. + // The singleton AuditDrainObserver is also constructed inside + // initialize() (step 5 below) so the impl is fully wired before + // we publish anything externally. + initialize(new OsgiWhiteboard(bundleContext)); + // Step 6 LAST: publish the Observer service. ObserverTracker + // (oak-store-spi/.../spi/commit/ObserverTracker.java, instantiated + // per-NodeStoreService in DocumentNodeStoreService, SegmentNodeStoreRegistrar, + // CompositeNodeStoreService) subscribes it to the root NodeStore. + // Any commit thread racing with activation that reaches step 6 before + // ObserverTracker has noticed the service will simply miss the drain + // on this one commit — events stay in the per-thread buffer until the + // next commit on the same session. No correctness risk. + observerRegistration = bundleContext.registerService( + Observer.class.getName(), getDrainObserver(), null); + } + + /** + * Non-OSGi entry point for wiring up the audit pipeline. Called by + * {@link #activate} in OSGi deployments after the {@code BundleContext} + * has been unwrapped into an {@code OsgiWhiteboard}, and by embedded + * callers (tests, {@code OakFixture}) directly. + * <p> + * <strong>Embedded callers must follow up with + * {@link #getDrainObserver()}</strong> to obtain the Observer and attach + * it to the root NodeStore. See {@link #getDrainObserver()} Javadoc for + * the recommended attach pattern and the {@code Oak.with(Observer)} + * caveat. + * <p> + * <strong>Must be called exactly once per instance.</strong> Calling + * it more than once orphans the previous {@code Feature} toggle and + * registry tracker, and silently overwrites the static + * {@link AuditEvents} / {@link AuditBufferLifecycle} sinks. To rewire, + * call {@link #dispose()} first. + * <p> + * <strong>Activation ordering rationale.</strong> + * {@link AuditBufferLifecycle#install AuditBufferLifecycle.install(buffer)} + * runs before + * {@link AuditEvents#install AuditEvents.install(BufferSink)} so that any + * concurrent capture arriving in the install window goes through the + * NOOP sink (no buffer write) rather than through a live {@code BufferSink} + * with an orphaned lifecycle handle. The inverse ordering would minimize + * lifecycle bypass but maximize silent capture loss; we prefer the former. + * + * @param whiteboard the whiteboard to register the {@code Feature} + * toggle and {@code AuditEventListener} tracker on; + * non-null. + */ + public void initialize(@NotNull Whiteboard whiteboard) { + featureToggle = Feature.newFeature(FEATURE_TOGGLE_NAME, whiteboard); + + registry = new WhiteboardAuditEventListenerRegistry(); + registry.start(whiteboard); + + buffer = new AuditBuffer(); + AuditBufferLifecycle.install(buffer); + + AuditEvents.install(new BufferSink(featureToggle, registry, buffer)); + + // Construct the singleton AuditDrainObserver LAST in initialize(). + // This closes a potential TOCTOU window in the OSGi @Activate flow: + // ObserverTracker (which subscribes to Observer services on a + // background thread) might fire the bootstrap CommitInfo.EMPTY_EXTERNAL + // invocation before @Activate returns. With the singleton constructed + // here (inside initialize, which @Activate calls FIRST), getDrainObserver() + // is safe to call from anywhere in @Activate after this point. + drainObserver = new AuditDrainObserver(featureToggle, buffer, registry); + + log.info("Audit pipeline activated. Toggle '{}' = {}.", + FEATURE_TOGGLE_NAME, featureToggle.isEnabled()); Review Comment: That's a bit confusing here. If the feature toggle is disabled, does an activated pipeline even matter? In other words: Instead of just printing ``true`` or ``false`` we should be more explicit what this enabled/disabled feature toggle means, especially if the audit pipeline will always be active. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/audit/AuditConfigurationImpl.java: ########## @@ -0,0 +1,493 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.security.audit; + +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.osgi.OsgiWhiteboard; +import org.apache.jackrabbit.oak.spi.audit.AuditBufferLifecycle; +import org.apache.jackrabbit.oak.spi.audit.AuditConfiguration; +import org.apache.jackrabbit.oak.spi.audit.AuditEvent; +import org.apache.jackrabbit.oak.spi.audit.AuditEventListener; +import org.apache.jackrabbit.oak.spi.audit.AuditEvents; +import org.apache.jackrabbit.oak.spi.commit.Observer; +import org.apache.jackrabbit.oak.spi.toggle.Feature; +import org.apache.jackrabbit.oak.spi.whiteboard.Whiteboard; +import org.jetbrains.annotations.NotNull; +import org.osgi.framework.BundleContext; +import org.osgi.framework.ServiceRegistration; +import org.osgi.service.component.annotations.Activate; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Deactivate; +import org.osgi.service.metatype.annotations.Designate; +import org.osgi.service.metatype.annotations.ObjectClassDefinition; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default {@link AuditConfiguration} implementation. Contributes the + * audit pipeline to Oak: + * <ul> + * <li>Registers a {@link Feature} toggle gating capture and dispatch.</li> + * <li>Installs a per-session buffer ({@link AuditBuffer}) into + * {@link AuditBufferLifecycle}.</li> + * <li>Installs the {@link AuditEvents} sink that routes capture-site + * calls into the buffer.</li> + * <li>Tracks {@code AuditEventListener} services on the Whiteboard + * via {@link WhiteboardAuditEventListenerRegistry}.</li> + * <li>Registers an {@link AuditDrainObserver} as an OSGi {@link Observer} + * service; Oak's {@code ObserverTracker} (in {@code oak-jcr}'s + * {@code RepositoryManager}) picks it up and subscribes it to the root + * NodeStore. The observer drains the buffer on commit success and + * dispatches events to listeners.</li> + * </ul> + * Registered as {@link AuditConfiguration} only — not a + * {@code SecurityConfiguration}, contributes no commit hooks, not reachable + * via {@code SecurityProvider.getConfiguration(AuditConfiguration.class)}. + * Embedded callers obtain the drain observer via {@link #getDrainObserver()}. + * <p> + * When the feature toggle is disabled, capture is a no-op and the observer + * short-circuits — see {@link AuditEvents#isEnabled()} and + * {@link AuditDrainObserver#contentChanged}. + */ +@Component(service = AuditConfiguration.class) +@Designate(ocd = AuditConfigurationImpl.Configuration.class) +public class AuditConfigurationImpl implements AuditConfiguration { + + /** + * Feature toggle name, following the {@code FT_OAK-<issue>} convention + * in {@code AGENTS.md}. Disabled by default: this is a new feature, + * not a bug fix. + * <p> + * <strong>Why not on the public SPI interface + * ({@link AuditConfiguration}):</strong> moving this constant to the + * SPI would commit the literal value to the public surface forever. + * It stays impl-local; promoting it later is a binary-additive change + * if a need arises. + */ + public static final String FEATURE_TOGGLE_NAME = "FT_OAK-12331"; + + @ObjectClassDefinition(name = "Apache Jackrabbit Oak AuditConfiguration", + description = "Audit event pipeline. Capture and dispatch are " + + "gated by the '" + FEATURE_TOGGLE_NAME + "' feature toggle " + + "(disabled by default).") + @interface Configuration { + // Configuration is currently empty by design: capture/dispatch behavior + // is controlled exclusively by the feature toggle. Listeners are + // contributed via OSGi services / the Whiteboard. + } + + private static final Logger log = LoggerFactory.getLogger(AuditConfigurationImpl.class); + + // Package-private (not private) on the internal-state fields so + // AuditConfigurationImplTest can mock-replace them to verify the + // dispose-order invariant via Mockito InOrder. Production callers MUST + // NOT touch these fields directly — go through initialize() / dispose(). + // + // JMM-safety. featureToggle, buffer, registry, and drainObserver are + // package-private and non-volatile by design. The invariant they rely on: + // they are mutated only by initialize(Whiteboard) and dispose(), which + // the contract specifies must each run exactly once and on the same + // thread; reads happen either + // (a) on that same thread — the static AuditEvents.record / dispatch + // facade reads through the AuditEvents.sink field (itself volatile, + // providing the publication barrier), and getDrainObserver() is + // called by activate() on the SCR thread AFTER initialize() on the + // SCR thread; or + // (b) on a commit thread that arrives via Observer.contentChanged, + // after the ServiceRegistration publication barrier established + // by BundleContext.registerService in activate(). + // Both paths satisfy the JMM happens-before contract without per-field + // volatile. If a future change adds a "share the singleton across + // pipelines" pattern OR cross-thread mutation of these fields, this + // invariant breaks — at that point the fields MUST be made volatile (or + // properly immutable via constructor injection). + Feature featureToggle; + AuditBuffer buffer; + WhiteboardAuditEventListenerRegistry registry; + + /** + * Singleton {@link AuditDrainObserver} instance constructed by + * {@link #initialize(Whiteboard)} and zeroed by {@link #dispose()}. + * Exposed via {@link #getDrainObserver()} as an {@link Observer}. + * <p> + * Singleton-not-factory by design: each {@code AuditConfigurationImpl} + * owns at most one Observer because (a) the {@link AuditBuffer} + * {@code ThreadLocal} is buffer-instance-scoped, so multiple Observer + * instances would compete for the same drain, and (b) the destructive + * {@code buffer.drain(sessionId)} contract is the cleanup mechanism — + * double-attach would mask future non-destructive-drain refactors. + */ + AuditDrainObserver drainObserver; + + /** + * OSGi service registration for the {@link AuditDrainObserver}. Held + * so {@link #deactivate} can unregister and let {@code ObserverTracker} + * close its subscription on the root NodeStore. {@code null} outside + * the OSGi-active window; embedded callers manage observer lifetime + * through their own {@code ((Observable) store).addObserver(...)} call + * (see class Javadoc). + */ + ServiceRegistration<?> observerRegistration; + + public AuditConfigurationImpl() { + super(); + } + + @SuppressWarnings("UnusedDeclaration") + @Activate + private void activate(@NotNull Configuration configuration, + @NotNull BundleContext bundleContext, + @NotNull Map<String, Object> properties) { + // Step 1-4: install sinks/registry/buffer/toggle. Capture-site + // record(...) calls reach the buffer as soon as initialize returns. + // The singleton AuditDrainObserver is also constructed inside + // initialize() (step 5 below) so the impl is fully wired before + // we publish anything externally. + initialize(new OsgiWhiteboard(bundleContext)); + // Step 6 LAST: publish the Observer service. ObserverTracker + // (oak-store-spi/.../spi/commit/ObserverTracker.java, instantiated + // per-NodeStoreService in DocumentNodeStoreService, SegmentNodeStoreRegistrar, + // CompositeNodeStoreService) subscribes it to the root NodeStore. + // Any commit thread racing with activation that reaches step 6 before + // ObserverTracker has noticed the service will simply miss the drain + // on this one commit — events stay in the per-thread buffer until the + // next commit on the same session. No correctness risk. + observerRegistration = bundleContext.registerService( + Observer.class.getName(), getDrainObserver(), null); + } + + /** + * Non-OSGi entry point for wiring up the audit pipeline. Called by + * {@link #activate} in OSGi deployments after the {@code BundleContext} + * has been unwrapped into an {@code OsgiWhiteboard}, and by embedded + * callers (tests, {@code OakFixture}) directly. + * <p> + * <strong>Embedded callers must follow up with + * {@link #getDrainObserver()}</strong> to obtain the Observer and attach + * it to the root NodeStore. See {@link #getDrainObserver()} Javadoc for + * the recommended attach pattern and the {@code Oak.with(Observer)} + * caveat. + * <p> + * <strong>Must be called exactly once per instance.</strong> Calling + * it more than once orphans the previous {@code Feature} toggle and + * registry tracker, and silently overwrites the static + * {@link AuditEvents} / {@link AuditBufferLifecycle} sinks. To rewire, + * call {@link #dispose()} first. + * <p> + * <strong>Activation ordering rationale.</strong> + * {@link AuditBufferLifecycle#install AuditBufferLifecycle.install(buffer)} + * runs before + * {@link AuditEvents#install AuditEvents.install(BufferSink)} so that any + * concurrent capture arriving in the install window goes through the + * NOOP sink (no buffer write) rather than through a live {@code BufferSink} + * with an orphaned lifecycle handle. The inverse ordering would minimize + * lifecycle bypass but maximize silent capture loss; we prefer the former. + * + * @param whiteboard the whiteboard to register the {@code Feature} + * toggle and {@code AuditEventListener} tracker on; + * non-null. + */ + public void initialize(@NotNull Whiteboard whiteboard) { + featureToggle = Feature.newFeature(FEATURE_TOGGLE_NAME, whiteboard); + + registry = new WhiteboardAuditEventListenerRegistry(); + registry.start(whiteboard); + + buffer = new AuditBuffer(); + AuditBufferLifecycle.install(buffer); + + AuditEvents.install(new BufferSink(featureToggle, registry, buffer)); + + // Construct the singleton AuditDrainObserver LAST in initialize(). + // This closes a potential TOCTOU window in the OSGi @Activate flow: + // ObserverTracker (which subscribes to Observer services on a + // background thread) might fire the bootstrap CommitInfo.EMPTY_EXTERNAL + // invocation before @Activate returns. With the singleton constructed + // here (inside initialize, which @Activate calls FIRST), getDrainObserver() + // is safe to call from anywhere in @Activate after this point. + drainObserver = new AuditDrainObserver(featureToggle, buffer, registry); + + log.info("Audit pipeline activated. Toggle '{}' = {}.", + FEATURE_TOGGLE_NAME, featureToggle.isEnabled()); + } + + /** + * Returns the singleton {@link Observer} bound to this pipeline's + * buffer, registry, and feature toggle. Constructed once by + * {@link #initialize(Whiteboard)} and cached for the lifetime of this + * {@code AuditConfigurationImpl} instance; zeroed by {@link #dispose()}. + * <p> + * The singleton shape is deliberate. Each {@code AuditConfigurationImpl} + * owns at most one Observer because (a) the {@link AuditBuffer} + * {@code ThreadLocal} is buffer-instance-scoped, so multiple Observer + * instances would compete for the same drain on every commit thread, + * and (b) the destructive {@code buffer.drain(sessionId)} contract is + * the cleanup mechanism — a future non-destructive-drain refactor + * would silently turn double-attach into double-dispatch. + * <p> + * Embedded callers pass the returned Observer to + * {@code ((Observable) store).addObserver(...)}, holding the returned + * {@code Closeable} for tear-down. {@code Oak.with(Observer)} is + * <em>not</em> a reliable embedded path when the caller also passes + * {@code Oak.with(Whiteboard)} to replace Oak's default whiteboard: + * the auto-attach at {@code Oak.java:300-302} is wired to the default + * whiteboard's anonymous override only. + * <p> + * OSGi callers never invoke this method directly — {@code @Activate} + * does, then publishes the singleton via + * {@code BundleContext.registerService(...)}. + * + * @return the singleton drain observer; never {@code null}. + * @throws IllegalStateException when called before + * {@link #initialize(Whiteboard)} OR after {@link #dispose()} + * (both states leave {@code drainObserver == null}). + */ + public @NotNull Observer getDrainObserver() { + if (drainObserver == null) { + throw new IllegalStateException( + "AuditConfigurationImpl.initialize(...) must be called first" + + " (or dispose() has already run)"); + } + return drainObserver; + } + + // Package-private (was private) so the test suite can invoke the Review Comment: ```suggestion // Package-private so the test suite can invoke the ``` ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserManagerImpl.java: ########## @@ -407,6 +422,68 @@ void onGroupUpdate(@NotNull Group group, boolean isRemove, boolean isContentId, } } + /** + * Records a single-member audit event for the given group update. + * Fires only on success — the upstream {@code MembershipWriter} + * does not invoke {@code onGroupUpdate} for the failure path. + */ + private void recordSingleMembershipAuditEvent(@NotNull Group group, boolean isRemove, @NotNull Authorizable member) { + try { + String groupPath = group.getPath(); + String memberId = member.getID(); + String memberPath = member.getPath(); + AuditEvents.record(root, isRemove + ? UserAuditEvents.memberRemoved(groupPath, memberId, memberPath) + : UserAuditEvents.memberAdded(groupPath, memberId, memberPath)); + } catch (RepositoryException e) { + // Path resolution failed — drop the event rather than fail the + // surrounding group update. It is an audit-completeness signal: + // a successful membership change produced no audit event. + warnAuditPathResolutionFailed("failed to resolve path for group membership update", e); + } + } + + /** + * Records a bulk-membership audit event when {@code memberIds} is + * non-empty. {@code memberIds} is the successful subset (failed + * entries already filtered by {@code MembershipWriter}); when every + * entry failed, {@code memberIds} is empty and no event is emitted. + * {@code failedIds} is carried through to the listener for audit + * completeness — listeners can distinguish "happened" vs "rejected". + */ + private void recordBulkMembershipAuditEvent(@NotNull Group group, boolean isRemove, boolean isContentId, @NotNull Set<String> memberIds, @NotNull Set<String> failedIds) { + if (memberIds.isEmpty()) { + return; + } + try { + String groupPath = group.getPath(); + AuditEvents.record(root, isRemove + ? UserAuditEvents.membersRemovedBulk(groupPath, memberIds, isContentId, failedIds) + : UserAuditEvents.membersAddedBulk(groupPath, memberIds, isContentId, failedIds)); + } catch (RepositoryException e) { + warnAuditPathResolutionFailed("failed to resolve group path for bulk membership update", e); Review Comment: ```suggestion warnAuditPathResolutionFailed("failed to resolve group path for bulk membership update, not recording audit event", e); ``` ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserManagerImpl.java: ########## @@ -407,6 +422,68 @@ void onGroupUpdate(@NotNull Group group, boolean isRemove, boolean isContentId, } } + /** + * Records a single-member audit event for the given group update. + * Fires only on success — the upstream {@code MembershipWriter} + * does not invoke {@code onGroupUpdate} for the failure path. + */ + private void recordSingleMembershipAuditEvent(@NotNull Group group, boolean isRemove, @NotNull Authorizable member) { + try { + String groupPath = group.getPath(); + String memberId = member.getID(); + String memberPath = member.getPath(); + AuditEvents.record(root, isRemove + ? UserAuditEvents.memberRemoved(groupPath, memberId, memberPath) + : UserAuditEvents.memberAdded(groupPath, memberId, memberPath)); + } catch (RepositoryException e) { + // Path resolution failed — drop the event rather than fail the + // surrounding group update. It is an audit-completeness signal: + // a successful membership change produced no audit event. + warnAuditPathResolutionFailed("failed to resolve path for group membership update", e); Review Comment: ```suggestion warnAuditPathResolutionFailed("failed to resolve path for group membership update, not recording the audit event", e); ``` ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/audit/CommitMetadataDecorator.java: ########## @@ -0,0 +1,217 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.oak.security.audit; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.apache.jackrabbit.oak.spi.audit.AuditEvent; +import org.apache.jackrabbit.oak.spi.commit.CommitInfo; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Enforces the {@code commit.*} trust contract at both dispatch + * boundaries: {@link #decorate} stamps commit-attached payloads with + * commit metadata (sessionId, userId, timestamp) at drain time, and + * {@link #stripReservedCommitKeys} removes caller-supplied values for the + * same three keys from fire-and-forget payloads before delivery. + * <p> + * Both operations return NEW {@link AuditEvent} instances that wrap the + * originals; the input events are not mutated. The wrapper's payload map + * is unmodifiable. + * + * <h3>Security invariant</h3> + * Both halves enforce the same property: listeners can treat the presence + * of {@link #KEY_SESSION_ID}, {@link #KEY_USER_ID}, or {@link #KEY_TIMESTAMP} + * in a dispatched payload as Oak-attested — see the normative trust contract + * on {@link org.apache.jackrabbit.oak.spi.audit.AuditEvent#getPayload()}. + * {@link #decorate} <strong>unconditionally overwrites</strong> the three + * keys with the values from the {@link CommitInfo} captured for the + * surrounding commit; {@link #stripReservedCommitKeys} removes + * caller-supplied values for the same keys on the fire-and-forget path. + * Weakening either half — {@code putIfAbsent} / {@code computeIfAbsent} / + * conditional {@code put} in the decorator, or skipping the strip at + * dispatch — is a regression in the trust model. + * + * <h3>Payload null-value contract</h3> + * The decorator trusts the no-null-keys/no-null-values contract documented + * on {@link org.apache.jackrabbit.oak.spi.audit.AuditEventListener#onEvents}. + * Buggy event implementations that violate it may leak null values to + * listeners — runtime validation is the event author's responsibility, + * not the decorator's. Adding per-entry null checks here would impose + * hot-path cost for what is an SPI-contract violation. + */ +final class CommitMetadataDecorator { + + private static final Logger log = LoggerFactory.getLogger(CommitMetadataDecorator.class); + + static final String KEY_SESSION_ID = "commit.sessionId"; + static final String KEY_USER_ID = "commit.userId"; + static final String KEY_TIMESTAMP = "commit.timestamp"; Review Comment: would it make sense to prefix these values with something like ``oak.``? ``commit`` feels a bit generic and not necessarily tied to to oak. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
