src/actix/forwarded.rs (1)

1-28: Clean helper extraction — consider adding Forwarded header support for parity with tonic.

As noted in the tonic review, this file only checks X-Forwarded-For while src/tonic/forwarded.rs also checks the RFC 7239 Forwarded header. Otherwise the code is clean and well-structured.

src/migrations/single_to_cluster.rs (1)

27-31: Unnecessary .clone() — full_access is not used after constructing full_auth.

full_access is consumed only by Auth::new on Line 28 and never referenced again, so the .clone() allocates needlessly.

♻️ Suggested fix

     let full_access = Access::full("Migration from single to cluster");
-    let full_auth = Auth::new(full_access.clone(), None, None, AuthType::Internal);
+    let full_auth = Auth::new(full_access, None, None, AuthType::Internal);

src/tonic/api/points_internal_api.rs (1)

41-43: full_internal_auth() is duplicated across internal API files.

This helper is identical to the one in collections_internal_api.rs (lines 16-18). Consider extracting it into a shared location (e.g., a common module for internal tonic helpers) to avoid the duplication.

#!/bin/bash
# Verify how many places define full_internal_auth
rg -n 'fn full_internal_auth' --type=rust

src/tonic/logging.rs (1)

52-54: Error log doesn't include the error details.

The error is propagated but not logged. Consider logging a summary of the error for operational visibility, especially since the forwarded-IP context has also been removed.

Suggested improvement

                Err(error) => {
-                    log::error!("gRPC request error {method_name}");
+                    log::error!("gRPC request error {method_name}: {error}");
                    Err(error)
                }

#!/bin/bash
# Check if S::Error implements Display (needed for the suggestion)
# Look at the trait bound on error type
rg -n 'type Error' --type=rust -A2 -g '*logging*'

lib/storage/tests/integration/alias_tests.rs (1)

173-181: Consider a more descriptive operation name for the audit log.

The third argument "test" in check_collection_access is the operation name for audit logging. A more descriptive name like "get_collection" or "test_alias_resolution" would be clearer, though this is just test code so it's minor.

lib/storage/src/content_manager/toc/mod.rs (1)

364-373: The _multipass parameter acts as a capability token but isn't used — consider documenting intent.

The _multipass: &CollectionMultipass parameter serves as proof-of-authorization but is never read. A brief doc comment explaining this pattern (e.g., "multipass is required as proof of authorization but not inspected") would help future readers understand why it exists. Compare with the existing collection_aliases method on Lines 376-392, which actively filters aliases by access — this new method deliberately skips that filtering.

config/config.yaml (1)

366-380: Well-documented default configuration.

The commented-out block with clear warnings about trust_forwarded_headers is good. Two observations from the PR discussion worth considering:

1. There's no option for compressing rotated files (e.g., gzip). Given the reported volume (~1GB quickly under load), a compress_rotated: true option would be valuable.
2. There's no mention of what happens if the audit directory becomes unavailable (disk full, deleted, etc.) — the PR comments note that Qdrant silently continues without audit entries in this case.

These could be addressed as follow-ups.

src/actix/api/cluster_api.rs (1)

166-178: toc() is called before the access check.

On Line 167, dispatcher.toc(&auth, &pass) is called before the access check on Line 168. While toc() likely doesn't enforce authorization itself, it's a better defensive pattern to check access first and only then obtain the toc reference — consistent with other handlers like recover_current_peer (Line 69-70) and remove_peer (Line 86-89) which check access first.

The same pattern appears in delete_cluster_metadata_key (Lines 190-194).

Proposed fix

     helpers::time(async move {
+        auth.check_global_access(
+            AccessRequirements::new().write(),
+            "update_cluster_metadata_key",
+        )?;
         let toc = dispatcher.toc(&auth, &pass);
-        auth.check_global_access(
-            AccessRequirements::new().write(),
-            "update_cluster_metadata_key",
-        )?;
 
         toc.update_cluster_metadata(key.into_inner(), value.into_inner(), params.wait)

lib/storage/src/rbac/ops_checks.rs (1)

338-348: Audit log for collection meta-operations omits the collection name.

emit_audit is called with None for the collection, but many CollectionMetaOperations variants carry a collection name (e.g., CreateCollection, DeleteCollection, CreatePayloadIndex, DropPayloadIndex). This makes it harder to correlate audit entries with the affected collection.

Consider extracting the collection name from the operation and passing it:

♻️ Suggested approach

You could add a method like fn collection_name(&self) -> Option<&str> on CollectionMetaOperations (or on AuditableOperation) and use it here:

     pub(crate) fn check_collection_meta_operation(
         &self,
         operation: &CollectionMetaOperations,
     ) -> Result<(), StorageError> {
         let result = self
             .unlogged_access()
             .check_collection_meta_operation(operation);
-        self.emit_audit(operation.operation_name(), None, &result);
+        self.emit_audit(operation.operation_name(), operation.collection_name(), &result);
         result
     }

src/actix/auth.rs (1)

128-135: Duplicated remote-IP derivation logic.

The forwarded-header-then-peer-addr pattern appears in both the middleware success path (Lines 129–134) and the FromRequest fallback (Lines 171–176). Consider extracting a small helper (e.g., fn resolve_remote(...)) to keep them in sync and reduce duplication.

♻️ Example helper

fn resolve_remote_from_service_req(req: &ServiceRequest) -> Option<String> {
    if audit_trust_forwarded_headers() {
        forwarded::forwarded_for(req)
    } else {
        None
    }
    .or_else(|| req.peer_addr().map(|a| a.ip().to_string()))
}

A similar helper for HttpRequest (using forwarded_for_http) could DRY up the FromRequest path.

Also applies to: 170-176

src/common/telemetry.rs (1)

92-93: Comment is slightly misleading — only collection-level telemetry uses unlogged access.

The comment "Do not log access to telemetry" on line 92 applies only to CollectionsTelemetry::collect (line 110–112) and HardwareTelemetry::new (line 147), which use the unlogged_access() reference. However, ClusterTelemetry::collect, RequestsTelemetry::collect, and MemoryTelemetry::collect (lines 136–144) receive the full &auth and will emit audit entries via their internal check_global_access calls (visible in the relevant snippets for those methods).

If the intent is to suppress only the per-collection access checks (which would be noisy), consider clarifying the comment to reflect that:

-        // Do not log access to telemetry
-        let access = auth.unlogged_access();
+        // Use unlogged access for collection-level telemetry to avoid per-collection audit noise
+        let access = auth.unlogged_access();

lib/storage/src/audit.rs (1)

100-153: Audit logger write path looks solid.

The serialize-then-write approach ensures atomicity under the Mutex, and the non-blocking channel keeps the critical section short. Error handling via log::error! is appropriate for an audit side-channel.

One note: tracing_appender::non_blocking uses a bounded channel with a default of 128,000 buffered messages. Under the extreme write volumes noted in PR comments (millions of entries), the channel could saturate. By default, it's lossy—new messages are dropped when the queue fills. If audit completeness is important, consider using NonBlockingBuilder::default() with .lossy(false) to apply backpressure instead, or document this lossy behavior.

lib/storage/src/content_manager/toc/point_ops.rs (1)

79-108: Batch methods emit one audit entry per sub-request — may contribute to high log volume.

Each batch method (recommend_batch, core_search_batch, discover_batch, query_batch) calls auth.check_point_op() in a loop, emitting an audit entry per sub-request. As noted in the PR discussion, this can generate millions of entries under search/upsert workloads. Since audit logging is opt-in, this is acceptable, but consider whether a single batch-level audit entry (with a count) would suffice for deployments with high request throughput.

Also applies to: 125-155, 288-318, 357-387

src/common/update.rs (1)

930-957: Use the existing Auth::new_internal() helper instead of manually constructing with Auth::new().

The codebase already has Auth::new_internal(access: Access) which sets subject: None, remote: None, and auth_type: AuthType::Internal. The pattern at src/common/update.rs:953 and 1020 (and also in points_internal_api.rs:42 and collections_internal_api.rs:18) should use this existing helper:

♻️ Refactor suggestion

Replace:

Auth::new(Access::full("Internal API"), None, None, AuthType::Internal)

With:

Auth::new_internal(Access::full("Internal API"))

Apply to all 4 occurrences in the codebase.