Remove the auditable field from AuthorizationInfo (#91261)

The auditable field was meant to determine whether authorization should
be audited or not. However, in practice, this field is always true and
what actually enabling auditing is by using different AuditTrail
implementations. The field is hence not necessary and only adds clutter
to the code. It is also arguable whether auditable or not belongs to the
AuthorizationInfo class. As a result, this PR removes this field from
AuthorizationInfo and its subclass IndexAuthorizationInfo.

Relates: https://github.com/elastic/elasticsearch/pull/91180/files#r1011344119

plugins/examples/security-authorization-engine/src/main/java/org/elasticsearch/example/CustomAuthorizationEngine.java

@@ -94,10 +94,10 @@ public class CustomAuthorizationEngine implements AuthorizationEngine {
                 }
                 IndicesAccessControl indicesAccessControl =
                     new IndicesAccessControl(true, Collections.unmodifiableMap(indexAccessControlMap));
-                listener.onResponse(new IndexAuthorizationResult(true, indicesAccessControl));
+                listener.onResponse(new IndexAuthorizationResult(indicesAccessControl));
             }, listener::onFailure));
         } else {
-            listener.onResponse(new IndexAuthorizationResult(true, IndicesAccessControl.DENIED));
+            listener.onResponse(new IndexAuthorizationResult(IndicesAccessControl.DENIED));
         }
     }
 

plugins/examples/security-authorization-engine/src/test/java/org/elasticsearch/example/CustomAuthorizationEngineTests.java

@@ -64,7 +64,6 @@ public class CustomAuthorizationEngineTests extends ESTestCase {
             engine.authorizeRunAs(info, authzInfo, resultFuture);
             AuthorizationResult result = resultFuture.actionGet();
             assertThat(result.isGranted(), is(false));
-            assertThat(result.isAuditable(), is(true));
         }
 
         // authorized
@@ -80,7 +79,6 @@ public class CustomAuthorizationEngineTests extends ESTestCase {
             engine.authorizeRunAs(info, authzInfo, resultFuture);
             AuthorizationResult result = resultFuture.actionGet();
             assertThat(result.isGranted(), is(true));
-            assertThat(result.isAuditable(), is(true));
         }
     }
 
@@ -97,7 +95,6 @@ public class CustomAuthorizationEngineTests extends ESTestCase {
             engine.authorizeClusterAction(requestInfo, authzInfo, resultFuture);
             AuthorizationResult result = resultFuture.actionGet();
             assertThat(result.isGranted(), is(true));
-            assertThat(result.isAuditable(), is(true));
         }
 
         // unauthorized
@@ -114,7 +111,6 @@ public class CustomAuthorizationEngineTests extends ESTestCase {
             engine.authorizeClusterAction(unauthReqInfo, authzInfo, resultFuture);
             AuthorizationResult result = resultFuture.actionGet();
             assertThat(result.isGranted(), is(false));
-            assertThat(result.isAuditable(), is(true));
         }
     }
 
@@ -142,7 +138,6 @@ public class CustomAuthorizationEngineTests extends ESTestCase {
                 indicesMap, resultFuture);
             IndexAuthorizationResult result = resultFuture.actionGet();
             assertThat(result.isGranted(), is(true));
-            assertThat(result.isAuditable(), is(true));
             IndicesAccessControl indicesAccessControl = result.getIndicesAccessControl();
             assertNotNull(indicesAccessControl.getIndexPermissions("index"));
         }
@@ -163,7 +158,6 @@ public class CustomAuthorizationEngineTests extends ESTestCase {
                 indicesMap, resultFuture);
             IndexAuthorizationResult result = resultFuture.actionGet();
             assertThat(result.isGranted(), is(false));
-            assertThat(result.isAuditable(), is(true));
             IndicesAccessControl indicesAccessControl = result.getIndicesAccessControl();
             assertNull(indicesAccessControl.getIndexPermissions("index"));
         }

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/AuthorizationEngine.java

@@ -504,34 +504,23 @@ public interface AuthorizationEngine {
     }
 
     /**
-     * Represents the result of authorization. This includes whether the actions should be granted
-     * and if this should be considered an auditable event.
+     * Represents the result of authorization to tell whether the actions should be granted
      */
     class AuthorizationResult {
 
         private final boolean granted;
-        private final boolean auditable;
 
         /**
-         * Create an authorization result with the provided granted value that is auditable
+         * Create an authorization result with the provided granted value
          */
         public AuthorizationResult(boolean granted) {
-            this(granted, true);
-        }
-
-        public AuthorizationResult(boolean granted, boolean auditable) {
             this.granted = granted;
-            this.auditable = auditable;
         }
 
         public boolean isGranted() {
             return granted;
         }
 
-        public boolean isAuditable() {
-            return auditable;
-        }
-
         /**
          * Returns additional context about an authorization failure, if {@link #isGranted()} is false.
          */
@@ -541,14 +530,14 @@ public interface AuthorizationEngine {
         }
 
         /**
-         * Returns a new authorization result that is granted and auditable
+         * Returns a new authorization result that is granted
          */
         public static AuthorizationResult granted() {
             return new AuthorizationResult(true);
         }
 
         /**
-         * Returns a new authorization result that is denied and auditable
+         * Returns a new authorization result that is denied
          */
         public static AuthorizationResult deny() {
             return new AuthorizationResult(false);
@@ -564,8 +553,8 @@ public interface AuthorizationEngine {
 
         private final IndicesAccessControl indicesAccessControl;
 
-        public IndexAuthorizationResult(boolean auditable, IndicesAccessControl indicesAccessControl) {
-            super(indicesAccessControl == null || indicesAccessControl.isGranted(), auditable);
+        public IndexAuthorizationResult(IndicesAccessControl indicesAccessControl) {
+            super(indicesAccessControl == null || indicesAccessControl.isGranted());
             this.indicesAccessControl = indicesAccessControl;
         }
 

x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileCancellationIntegTests.java

@@ -410,7 +410,7 @@ public class ProfileCancellationIntegTests extends AbstractProfileIntegTestCase
                     Map<String, IndexAbstraction> aliasOrIndexLookup,
                     ActionListener<IndexAuthorizationResult> listener
                 ) {
-                    listener.onResponse(new IndexAuthorizationResult(true, IndicesAccessControl.ALLOW_NO_INDICES));
+                    listener.onResponse(new IndexAuthorizationResult(IndicesAccessControl.ALLOW_NO_INDICES));
                 }
 
                 @Override

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

@@ -86,7 +86,6 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 
@@ -350,26 +349,10 @@ public class AuthorizationService {
         if (isRunAs) {
             ActionListener<AuthorizationResult> runAsListener = wrapPreservingContext(ActionListener.wrap(result -> {
                 if (result.isGranted()) {
-                    if (result.isAuditable()) {
-                        auditTrail.runAsGranted(
-                            requestId,
-                            authentication,
-                            action,
-                            request,
-                            authzInfo.getAuthenticatedUserAuthorizationInfo()
-                        );
-                    }
+                    auditTrail.runAsGranted(requestId, authentication, action, request, authzInfo.getAuthenticatedUserAuthorizationInfo());
                     authorizeAction(requestInfo, requestId, authzInfo, listener);
                 } else {
-                    if (result.isAuditable()) {
-                        auditTrail.runAsDenied(
-                            requestId,
-                            authentication,
-                            action,
-                            request,
-                            authzInfo.getAuthenticatedUserAuthorizationInfo()
-                        );
-                    }
+                    auditTrail.runAsDenied(requestId, authentication, action, request, authzInfo.getAuthenticatedUserAuthorizationInfo());
                     listener.onFailure(runAsDenied(authentication, authzInfo, action));
                 }
             }, e -> {
@@ -747,7 +730,6 @@ public class AuthorizationService {
             final ActionListener<Collection<Tuple<String, IndexAuthorizationResult>>> bulkAuthzListener = ActionListener.wrap(
                 collection -> {
                     final Map<String, IndicesAccessControl> actionToIndicesAccessControl = new HashMap<>();
-                    final AtomicBoolean audit = new AtomicBoolean(false);
                     collection.forEach(tuple -> {
                         final IndicesAccessControl existing = actionToIndicesAccessControl.putIfAbsent(
                             tuple.v1(),
@@ -756,9 +738,6 @@ public class AuthorizationService {
                         if (existing != null) {
                             throw new IllegalStateException("a value already exists for action " + tuple.v1());
                         }
-                        if (tuple.v2().isAuditable()) {
-                            audit.set(true);
-                        }
                     });
 
                     for (BulkItemRequest item : request.items()) {
@@ -790,7 +769,7 @@ public class AuthorizationService {
                                     null
                                 )
                             );
-                        } else if (audit.get()) {
+                        } else {
                             auditTrail.explicitIndexAccessEvent(
                                 requestId,
                                 AuditLevel.ACCESS_GRANTED,
@@ -950,38 +929,34 @@ public class AuthorizationService {
         @Override
         public void onResponse(T result) {
             if (result.isGranted()) {
-                if (result.isAuditable()) {
-                    auditTrailService.get()
-                        .accessGranted(
-                            requestId,
-                            requestInfo.getAuthentication(),
-                            requestInfo.getAction(),
-                            requestInfo.getRequest(),
-                            authzInfo
-                        );
-                }
+                auditTrailService.get()
+                    .accessGranted(
+                        requestId,
+                        requestInfo.getAuthentication(),
+                        requestInfo.getAction(),
+                        requestInfo.getRequest(),
+                        authzInfo
+                    );
                 try {
                     responseConsumer.accept(result);
                 } catch (Exception e) {
                     failureConsumer.accept(e);
                 }
             } else {
-                handleFailure(result.isAuditable(), result.getFailureContext(requestInfo, restrictedIndices), null);
+                handleFailure(result.getFailureContext(requestInfo, restrictedIndices), null);
             }
         }
 
         @Override
         public void onFailure(Exception e) {
-            handleFailure(true, null, e);
+            handleFailure(null, e);
         }
 
-        private void handleFailure(boolean audit, @Nullable String context, @Nullable Exception e) {
+        private void handleFailure(@Nullable String context, @Nullable Exception e) {
             Authentication authentication = requestInfo.getAuthentication();
             String action = requestInfo.getAction();
             TransportRequest request = requestInfo.getRequest();
-            if (audit) {
-                auditTrailService.get().accessDenied(requestId, authentication, action, request, authzInfo);
-            }
+            auditTrailService.get().accessDenied(requestId, authentication, action, request, authzInfo);
             failureConsumer.accept(actionDenied(authentication, authzInfo, action, request, context, e));
         }
     }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java

@@ -307,7 +307,7 @@ public class RBACEngine implements AuthorizationEngine {
                         if (parsedScrollId.hasLocalIndices()) {
                             listener.onResponse(authorizeIndexActionName(action, authorizationInfo, null));
                         } else {
-                            listener.onResponse(new IndexAuthorizationResult(true, null));
+                            listener.onResponse(new IndexAuthorizationResult(null));
                         }
                     }, listener::onFailure), ((SearchScrollRequest) request)::parseScrollId).run();
                 } else {
@@ -319,21 +319,21 @@ public class RBACEngine implements AuthorizationEngine {
                     // The DLS/FLS permissions are used inside the {@code DirectoryReader} that {@code SecurityIndexReaderWrapper}
                     // built while handling the initial search request. In addition, for consistency, the DLS/FLS permissions from
                     // the originating search request are attached to the thread context upon validating the scroll.
-                    listener.onResponse(new IndexAuthorizationResult(true, null));
+                    listener.onResponse(new IndexAuthorizationResult(null));
                 }
             } else if (isAsyncRelatedAction(action)) {
                 if (SubmitAsyncSearchAction.NAME.equals(action)) {
                     // authorize submit async search but don't fill in the DLS/FLS permissions
                     // the `null` IndicesAccessControl parameter indicates that this action has *not* determined
                     // which DLS/FLS controls should be applied to this action
-                    listener.onResponse(new IndexAuthorizationResult(true, null));
+                    listener.onResponse(new IndexAuthorizationResult(null));
                 } else {
                     // async-search actions other than submit have a custom security layer that checks if the current user is
                     // the same as the user that submitted the original request so no additional checks are needed here.
-                    listener.onResponse(new IndexAuthorizationResult(true, IndicesAccessControl.ALLOW_NO_INDICES));
+                    listener.onResponse(new IndexAuthorizationResult(IndicesAccessControl.ALLOW_NO_INDICES));
                 }
             } else if (action.equals(ClosePointInTimeAction.NAME)) {
-                listener.onResponse(new IndexAuthorizationResult(true, IndicesAccessControl.ALLOW_NO_INDICES));
+                listener.onResponse(new IndexAuthorizationResult(IndicesAccessControl.ALLOW_NO_INDICES));
             } else {
                 assert false
                     : "only scroll and async-search related requests are known indices api that don't "
@@ -346,9 +346,7 @@ public class RBACEngine implements AuthorizationEngine {
                 );
             }
         } else if (isChildActionAuthorizedByParent(requestInfo, authorizationInfo)) {
-            listener.onResponse(
-                new IndexAuthorizationResult(true, requestInfo.getOriginatingAuthorizationContext().getIndicesAccessControl())
-            );
+            listener.onResponse(new IndexAuthorizationResult(requestInfo.getOriginatingAuthorizationContext().getIndicesAccessControl()));
         } else if (request instanceof IndicesRequest.Replaceable replaceable && replaceable.allowsRemoteIndices()) {
             // remote indices are allowed
             indicesAsyncSupplier.getAsync(ActionListener.wrap(resolvedIndices -> {
@@ -388,7 +386,7 @@ public class RBACEngine implements AuthorizationEngine {
                         // all wildcard expressions have been resolved and only the security plugin could have set '-*' here.
                         // '-*' matches no indices so we allow the request to go through, which will yield an empty response
                         if (resolvedIndices.isNoIndicesPlaceholder()) {
-                            listener.onResponse(new IndexAuthorizationResult(true, IndicesAccessControl.ALLOW_NO_INDICES));
+                            listener.onResponse(new IndexAuthorizationResult(IndicesAccessControl.ALLOW_NO_INDICES));
                         } else {
                             assert resolvedIndices.getLocal().stream().noneMatch(Regex::isSimpleMatchPattern)
                                 || ((IndicesRequest) request).indicesOptions().expandWildcardExpressions() == false
@@ -487,7 +485,7 @@ public class RBACEngine implements AuthorizationEngine {
         IndicesAccessControl grantedValue
     ) {
         final Role role = ensureRBAC(authorizationInfo).getRole();
-        return new IndexAuthorizationResult(true, role.checkIndicesAction(action) ? grantedValue : IndicesAccessControl.DENIED);
+        return new IndexAuthorizationResult(role.checkIndicesAction(action) ? grantedValue : IndicesAccessControl.DENIED);
 
     }
 
@@ -842,7 +840,7 @@ public class RBACEngine implements AuthorizationEngine {
     ) {
         final Role role = ensureRBAC(authorizationInfo).getRole();
         final IndicesAccessControl accessControl = role.authorize(action, indices, aliasAndIndexLookup, fieldPermissionsCache);
-        return new IndexAuthorizationResult(true, accessControl);
+        return new IndexAuthorizationResult(accessControl);
     }
 
     private static RBACAuthorizationInfo ensureRBAC(AuthorizationInfo authorizationInfo) {

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/interceptor/ResizeRequestInterceptor.java

@@ -74,15 +74,13 @@ public final class ResizeRequestInterceptor implements RequestInterceptor {
                     if (authzResult.isGranted()) {
                         listener.onResponse(null);
                     } else {
-                        if (authzResult.isAuditable()) {
-                            auditTrail.accessDenied(
-                                extractRequestId(threadContext),
-                                requestInfo.getAuthentication(),
-                                requestInfo.getAction(),
-                                request,
-                                authorizationInfo
-                            );
-                        }
+                        auditTrail.accessDenied(
+                            extractRequestId(threadContext),
+                            requestInfo.getAuthentication(),
+                            requestInfo.getAction(),
+                            request,
+                            authorizationInfo
+                        );
                         listener.onFailure(
                             Exceptions.authorizationError(
                                 "Resizing an index is not allowed when the target index " + "has more permissions than the source index"