address review comments

Author: richard-dennehy

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/dependencies/patches/hdfs/MethodReplacement.java renamed to build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/dependencies/patches/MethodReplacement.java

@@ -7,7 +7,7 @@
  * License v3.0 only", or the "Server Side Public License, v 1".
  */
 
-package org.elasticsearch.gradle.internal.dependencies.patches.hdfs;
+package org.elasticsearch.gradle.internal.dependencies.patches;
 
 import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.Opcodes;

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/dependencies/patches/azurecore/AzureCoreClassPatcher.java

@@ -11,6 +11,7 @@
 
 import org.elasticsearch.gradle.internal.dependencies.patches.PatcherInfo;
 import org.elasticsearch.gradle.internal.dependencies.patches.Utils;
+import org.gradle.api.artifacts.transform.CacheableTransform;
 import org.gradle.api.artifacts.transform.InputArtifact;
 import org.gradle.api.artifacts.transform.TransformAction;
 import org.gradle.api.artifacts.transform.TransformOutputs;
@@ -26,6 +27,7 @@
 
 import static org.elasticsearch.gradle.internal.dependencies.patches.PatcherInfo.classPatcher;
 
+@CacheableTransform
 public abstract class AzureCoreClassPatcher implements TransformAction<TransformParameters.None> {
 
     private static final String JAR_FILE_TO_PATCH = "azure-core-[\\d.]*\\.jar";

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/dependencies/patches/azurecore/ImplUtilsPatcher.java

@@ -9,7 +9,7 @@
 
 package org.elasticsearch.gradle.internal.dependencies.patches.azurecore;
 
-import org.elasticsearch.gradle.internal.dependencies.patches.hdfs.MethodReplacement;
+import org.elasticsearch.gradle.internal.dependencies.patches.MethodReplacement;
 import org.objectweb.asm.ClassVisitor;
 import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.Opcodes;

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/dependencies/patches/hdfs/ShellPatcher.java

@@ -9,6 +9,7 @@
 
 package org.elasticsearch.gradle.internal.dependencies.patches.hdfs;
 
+import org.elasticsearch.gradle.internal.dependencies.patches.MethodReplacement;
 import org.objectweb.asm.ClassVisitor;
 import org.objectweb.asm.ClassWriter;
 import org.objectweb.asm.MethodVisitor;

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/dependencies/patches/hdfs/ShutdownHookManagerPatcher.java

@@ -9,6 +9,7 @@
 
 package org.elasticsearch.gradle.internal.dependencies.patches.hdfs;
 
+import org.elasticsearch.gradle.internal.dependencies.patches.MethodReplacement;
 import org.objectweb.asm.ClassVisitor;
 import org.objectweb.asm.ClassWriter;
 import org.objectweb.asm.MethodVisitor;

gradle/verification-metadata.xml

@@ -3063,11 +3063,6 @@
             <sha256 value="015d5c229f3cd5c0ebf175c1da08d596d94043362ae9d92637d88848c90537c8" origin="Generated by Gradle"/>
          </artifact>
       </component>
-      <component group="org.apache.logging.log4j" name="log4j-slf4j2-impl" version="2.24.3">
-         <artifact name="log4j-slf4j2-impl-2.24.3.jar">
-            <sha256 value="cdaac22e40ec30c4096e1ebe8c454c8826c0d1c378d7db5d7b3ad166354b0bd3" origin="Generated by Gradle"/>
-         </artifact>
-      </component>
       <component group="org.apache.lucene" name="lucene-analysis-common" version="10.2.1">
          <artifact name="lucene-analysis-common-10.2.1.jar">
             <sha256 value="73e5dfac4c64ea5af6a0e70276c4cf3216085c05de3a6547d4240145bb362a7d" origin="Generated by Gradle"/>

plugins/microsoft-graph-authz/src/main/java/org/elasticsearch/xpack/security/authz/microsoft/MicrosoftGraphAuthzRealm.java

@@ -14,6 +14,7 @@
 import com.microsoft.graph.core.tasks.PageIterator;
 import com.microsoft.graph.models.Group;
 import com.microsoft.graph.models.GroupCollectionResponse;
+import com.microsoft.graph.models.odataerrors.ODataError;
 import com.microsoft.graph.serviceclient.GraphServiceClient;
 import com.microsoft.kiota.authentication.AzureIdentityAuthenticationProvider;
 
@@ -43,7 +44,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.ExecutorService;
+import java.util.stream.Collectors;
 
 public class MicrosoftGraphAuthzRealm extends Realm {
 
@@ -64,7 +65,7 @@ public class MicrosoftGraphAuthzRealm extends Realm {
     private final UserRoleMapper roleMapper;
     private final GraphServiceClient client;
     private final XPackLicenseState licenseState;
-    private final ExecutorService genericExecutor;
+    private final ThreadPool threadPool;
 
     public MicrosoftGraphAuthzRealm(UserRoleMapper roleMapper, RealmConfig config, ThreadPool threadPool) {
         super(config);
@@ -74,19 +75,12 @@ public MicrosoftGraphAuthzRealm(UserRoleMapper roleMapper, RealmConfig config, T
         var clientSecret = config.getSetting(MicrosoftGraphAuthzRealmSettings.CLIENT_SECRET);
 
         require(MicrosoftGraphAuthzRealmSettings.CLIENT_ID);
+        require(MicrosoftGraphAuthzRealmSettings.CLIENT_SECRET);
         require(MicrosoftGraphAuthzRealmSettings.TENANT_ID);
 
-        if (clientSecret.isEmpty()) {
-            throw new SettingsException(
-                "The configuration setting ["
-                    + RealmSettings.getFullSettingKey(config, MicrosoftGraphAuthzRealmSettings.CLIENT_SECRET)
-                    + "] is required"
-            );
-        }
-
         this.client = buildClient(clientSecret);
         this.licenseState = XPackPlugin.getSharedLicenseState();
-        this.genericExecutor = threadPool.generic();
+        this.threadPool = threadPool;
     }
 
     // for testing
@@ -102,12 +96,12 @@ public MicrosoftGraphAuthzRealm(UserRoleMapper roleMapper, RealmConfig config, T
         this.roleMapper = roleMapper;
         this.client = client;
         this.licenseState = licenseState;
-        this.genericExecutor = threadPool.generic();
+        this.threadPool = threadPool;
     }
 
-    private void require(Setting.AffixSetting<String> setting) {
+    private <T> void require(Setting.AffixSetting<T> setting) {
         final var value = config.getSetting(setting);
-        if (value.isEmpty()) {
+        if (value.toString().isEmpty()) {
             throw new SettingsException("The configuration setting [" + RealmSettings.getFullSettingKey(config, setting) + "] is required");
         }
     }
@@ -134,12 +128,11 @@ public void lookupUser(String principal, ActionListener<User> listener) {
             return;
         }
 
-        genericExecutor.execute(() -> {
+        threadPool.generic().execute(() -> {
             try {
-                final var userProperties = sdkFetchUserProperties(client, principal);
-                final var groups = sdkFetchGroupMembership(client, principal);
+                final var userProperties = fetchUserProperties(client, principal);
+                final var groups = fetchGroupMembership(client, principal);
 
-                // TODO confirm we don't need any other fields
                 final var userData = new UserRoleMapper.UserData(principal, null, groups, Map.of(), config);
 
                 roleMapper.resolveRoles(userData, listener.delegateFailureAndWrap((l, roles) -> {
@@ -151,18 +144,17 @@ public void lookupUser(String principal, ActionListener<User> listener) {
                         Map.of(),
                         true
                     );
-                    logger.trace("Entra ID user {}", user);
+                    logger.trace("Authorized user from Microsoft Graph {}", user);
                     l.onResponse(user);
                 }));
-            } catch (Exception e) {
+            } catch (ReflectiveOperationException | ODataError e) {
                 logger.error("failed to authenticate with realm", e);
                 listener.onFailure(e);
             }
         });
     }
 
     private GraphServiceClient buildClient(SecureString clientSecret) {
-        logger.trace("building client");
         final var credentialProviderBuilder = new ClientSecretCredentialBuilder().clientId(
             config.getSetting(MicrosoftGraphAuthzRealmSettings.CLIENT_ID)
         )
@@ -183,17 +175,17 @@ private GraphServiceClient buildClient(SecureString clientSecret) {
         );
     }
 
-    private Tuple<String, String> sdkFetchUserProperties(GraphServiceClient client, String userId) {
+    private Tuple<String, String> fetchUserProperties(GraphServiceClient client, String userId) {
         var response = client.users()
             .byUserId(userId)
             .get(requestConfig -> requestConfig.queryParameters.select = new String[] { "displayName", "mail" });
 
-        logger.trace("User [{}] has email [{}]", response.getDisplayName(), response.getMail());
+        logger.trace("Fetched user with name [{}] and email [{}] from Microsoft Graph", response.getDisplayName(), response.getMail());
 
         return Tuple.tuple(response.getDisplayName(), response.getMail());
     }
 
-    private List<String> sdkFetchGroupMembership(GraphServiceClient client, String userId) throws ReflectiveOperationException {
+    private List<String> fetchGroupMembership(GraphServiceClient client, String userId) throws ReflectiveOperationException {
         List<String> groups = new ArrayList<>();
 
         var groupMembership = client.users().byUserId(userId).transitiveMemberOf().graphGroup().get(requestConfig -> {
@@ -217,6 +209,10 @@ private List<String> sdkFetchGroupMembership(GraphServiceClient client, String u
 
         pageIterator.iterate();
 
+        if (logger.isTraceEnabled()) {
+            logger.trace("Fetched [{}] groups from Microsoft Graph: [{}]", groups.size(), String.join(", ", groups));
+        }
+
         return groups;
     }
 }

x-pack/plugin/security/qa/microsoft-graph-authz-tests/build.gradle

@@ -9,5 +9,6 @@ dependencies {
 }
 
 tasks.named("javaRestTest").configure {
+  // disable tests in FIPS mode as we need to use a custom truststore containing the certs used in MicrosoftGraphHttpFixture
   buildParams.withFipsEnabledOnly(it)
 }

x-pack/plugin/security/qa/microsoft-graph-authz-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/authz/microsoft/MicrosoftGraphAuthzPluginIT.java

@@ -68,7 +68,7 @@ public class MicrosoftGraphAuthzPluginIT extends ESRestTestCase {
         new TestUser("User3", "User 3", "[email protected]", List.of(), List.of())
     );
 
-    private static final MsGraphHttpFixture graphFixture = new MsGraphHttpFixture(TENANT_ID, CLIENT_ID, CLIENT_SECRET, TEST_USERS, 3);
+    private static final MicrosoftGraphHttpFixture graphFixture = new MicrosoftGraphHttpFixture(TENANT_ID, CLIENT_ID, CLIENT_SECRET, TEST_USERS, 3);
 
     public static ElasticsearchCluster cluster = initTestCluster();
 

x-pack/plugin/security/qa/microsoft-graph-authz-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/authz/microsoft/MsGraphHttpFixture.java renamed to x-pack/plugin/security/qa/microsoft-graph-authz-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/authz/microsoft/MicrosoftGraphHttpFixture.java

@@ -41,9 +41,9 @@
 import javax.net.ssl.KeyManager;
 import javax.net.ssl.SSLContext;
 
-public class MsGraphHttpFixture extends ExternalResource {
+public class MicrosoftGraphHttpFixture extends ExternalResource {
 
-    private static final Logger logger = LogManager.getLogger(MsGraphHttpFixture.class);
+    private static final Logger logger = LogManager.getLogger(MicrosoftGraphHttpFixture.class);
 
     private final String tenantId;
     private final String clientId;
@@ -58,7 +58,7 @@ public class MsGraphHttpFixture extends ExternalResource {
 
     private HttpsServer server;
 
-    public MsGraphHttpFixture(String tenantId, String clientId, String clientSecret, List<TestUser> users, int groupsPageSize) {
+    public MicrosoftGraphHttpFixture(String tenantId, String clientId, String clientSecret, List<TestUser> users, int groupsPageSize) {
         this.tenantId = tenantId;
         this.clientId = clientId;
         this.clientSecret = clientSecret;