[Backport 2.x] [Improvement] Enable warnings during compilation (open…

…search-project#3358) Backport 40efea6 from 3358 Signed-off-by: Andrey Pleskach <[email protected]>
willyborankin · Oct 16, 2023 · 023c1a4 · 023c1a4
1 parent b5622c4
commit 023c1a4
Show file tree

Hide file tree

Showing 54 changed files with 392 additions and 332 deletions.
diff --git a/build.gradle b/build.gradle
@@ -95,6 +95,62 @@ spotbugsTest {
 java.sourceCompatibility = JavaVersion.VERSION_11
 java.targetCompatibility = JavaVersion.VERSION_11
 
+compileJava {
+    options.compilerArgs = [
+            '-Xlint:auxiliaryclass',
+            '-Xlint:cast',
+            '-Xlint:classfile',
+            '-Xlint:dep-ann',
+            '-Xlint:divzero',
+            '-Xlint:empty',
+            '-Xlint:exports',
+            '-Xlint:fallthrough',
+            '-Xlint:finally',
+            '-Xlint:module',
+            '-Xlint:opens',
+            '-Xlint:overloads',
+            '-Xlint:overrides',
+            '-Xlint:-processing',
+            '-Xlint:rawtypes',
+            '-Xlint:removal',
+            '-Xlint:requires-automatic',
+            '-Xlint:requires-transitive-automatic',
+            '-Xlint:static',
+            '-Xlint:unchecked',
+            '-Xlint:varargs',
+            '-Xlint:preview',
+            '-Werror']
+    options.encoding = 'UTF-8'
+}
+
+compileTestJava {
+    options.compilerArgs = [
+            '-Xlint:auxiliaryclass',
+            '-Xlint:cast',
+            '-Xlint:classfile',
+            '-Xlint:dep-ann',
+            '-Xlint:divzero',
+            '-Xlint:empty',
+            '-Xlint:exports',
+            '-Xlint:fallthrough',
+            '-Xlint:finally',
+            '-Xlint:module',
+            '-Xlint:opens',
+            '-Xlint:overloads',
+            '-Xlint:overrides',
+            '-Xlint:-processing',
+            '-Xlint:rawtypes',
+            '-Xlint:removal',
+            '-Xlint:requires-automatic',
+            '-Xlint:requires-transitive-automatic',
+            '-Xlint:static',
+            '-Xlint:unchecked',
+            '-Xlint:varargs',
+            '-Xlint:preview',
+            '-Werror']
+    options.encoding = 'UTF-8'
+}
+
 licenseHeaders.enabled = true
 
 // The following check that have never be enabled in security
@@ -539,7 +595,7 @@ dependencies {
     runtimeOnly 'com.eclipsesource.minimal-json:minimal-json:0.9.5'
     runtimeOnly 'commons-codec:commons-codec:1.16.0'
     runtimeOnly 'org.cryptacular:cryptacular:1.2.5'
-    runtimeOnly 'com.google.errorprone:error_prone_annotations:2.22.0'
+    compileOnly 'com.google.errorprone:error_prone_annotations:2.22.0'
     runtimeOnly 'com.sun.istack:istack-commons-runtime:4.2.0'
     runtimeOnly 'jakarta.xml.bind:jakarta.xml.bind-api:4.0.1'
     runtimeOnly 'org.ow2.asm:asm:9.6'
@@ -571,7 +627,7 @@ dependencies {
     runtimeOnly 'org.apache.commons:commons-text:1.10.0'
     runtimeOnly "org.glassfish.jaxb:jaxb-runtime:${jaxb_version}"
     runtimeOnly 'com.google.j2objc:j2objc-annotations:2.8'
-    runtimeOnly 'com.google.code.findbugs:jsr305:3.0.2'
+    compileOnly 'com.google.code.findbugs:jsr305:3.0.2'
     runtimeOnly 'org.lz4:lz4-java:1.8.0'
     runtimeOnly 'io.dropwizard.metrics:metrics-core:4.2.21'
     runtimeOnly 'org.slf4j:slf4j-api:1.7.36'

diff --git a/src/integrationTest/java/org/opensearch/node/PluginAwareNode.java b/src/integrationTest/java/org/opensearch/node/PluginAwareNode.java
@@ -26,7 +26,7 @@
 
 package org.opensearch.node;
 
-import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 
 import org.opensearch.common.settings.Settings;
@@ -36,11 +36,14 @@ public class PluginAwareNode extends Node {
 
     private final boolean clusterManagerEligible;
 
-    @SafeVarargs
-    public PluginAwareNode(boolean clusterManagerEligible, final Settings preparedSettings, final Class<? extends Plugin>... plugins) {
+    public PluginAwareNode(
+        boolean clusterManagerEligible,
+        final Settings preparedSettings,
+        final Collection<Class<? extends Plugin>> plugins
+    ) {
         super(
             InternalSettingsPreparer.prepareEnvironment(preparedSettings, Collections.emptyMap(), null, () -> System.getenv("HOSTNAME")),
-            Arrays.asList(plugins),
+            plugins,
             true
         );
         this.clusterManagerEligible = clusterManagerEligible;

diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/ClusterManager.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/ClusterManager.java
@@ -165,8 +165,8 @@ public Class<? extends Plugin>[] getPlugins() {
             return plugins.toArray(new Class[0]);
         }
 
-        public Class<? extends Plugin>[] pluginsWithAddition(List<Class<? extends Plugin>> additionalPlugins) {
-            return mergePlugins(plugins, additionalPlugins).toArray(Class[]::new);
+        public Collection<Class<? extends Plugin>> pluginsWithAddition(List<Class<? extends Plugin>> additionalPlugins) {
+            return mergePlugins(plugins, additionalPlugins);
         }
     }
 }
diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java
@@ -407,7 +407,7 @@ boolean hasAssignedType(NodeType type) {
 
         CompletableFuture<StartStage> start() {
             CompletableFuture<StartStage> completableFuture = new CompletableFuture<>();
-            Class<? extends Plugin>[] mergedPlugins = nodeSettings.pluginsWithAddition(additionalPlugins);
+            final Collection<Class<? extends Plugin>> mergedPlugins = nodeSettings.pluginsWithAddition(additionalPlugins);
             this.node = new PluginAwareNode(nodeSettings.containRole(NodeRole.CLUSTER_MANAGER), getOpenSearchSettings(), mergedPlugins);
 
             new Thread(new Runnable() {

diff --git a/src/main/java/org/opensearch/security/DefaultObjectMapper.java b/src/main/java/org/opensearch/security/DefaultObjectMapper.java
@@ -70,6 +70,8 @@ public class DefaultObjectMapper {
         YAML_MAPPER.enable(JsonParser.Feature.STRICT_DUPLICATE_DETECTION);
     }
 
+    private DefaultObjectMapper() {}
+
     public static void inject(final InjectableValues.Std injectableValues) {
         objectMapper.setInjectableValues(injectableValues);
         YAML_MAPPER.setInjectableValues(injectableValues);
@@ -220,7 +222,7 @@ public static TypeFactory getTypeFactory() {
         return objectMapper.getTypeFactory();
     }
 
-    public static Set<String> getFields(Class cls) {
+    public static Set<String> getFields(Class<?> cls) {
         return objectMapper.getSerializationConfig()
             .introspect(getTypeFactory().constructType(cls))
             .findProperties()

diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
@@ -634,6 +634,7 @@ public void clear(String reason) {
 
                 @Override
                 public Weight doCache(Weight weight, QueryCachingPolicy policy) {
+                    @SuppressWarnings("unchecked")
                     final Map<String, Set<String>> allowedFlsFields = (Map<String, Set<String>>) HeaderHelper.deserializeSafeFromHeader(
                         threadPool.getThreadContext(),
                         ConfigConstants.OPENDISTRO_SECURITY_FLS_FIELDS_HEADER
@@ -642,7 +643,7 @@ public Weight doCache(Weight weight, QueryCachingPolicy policy) {
                     if (SecurityUtils.evalMap(allowedFlsFields, index().getName()) != null) {
                         return weight;
                     } else {
-
+                        @SuppressWarnings("unchecked")
                         final Map<String, Set<String>> maskedFieldsMap = (Map<String, Set<String>>) HeaderHelper.deserializeSafeFromHeader(
                             threadPool.getThreadContext(),
                             ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_HEADER
@@ -727,6 +728,7 @@ public void onQueryPhase(SearchContext searchContext, long tookInNanos) {
                         return;
                     }
 
+                    @SuppressWarnings("unchecked")
                     final Map<String, Set<String>> maskedFieldsMap = (Map<String, Set<String>>) HeaderHelper.deserializeSafeFromHeader(
                         threadPool.getThreadContext(),
                         ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_HEADER
@@ -1864,6 +1866,7 @@ public Function<String, Predicate<String>> getFieldFilter() {
             if (threadPool == null) {
                 return field -> true;
             }
+            @SuppressWarnings("unchecked")
             final Map<String, Set<String>> allowedFlsFields = (Map<String, Set<String>>) HeaderHelper.deserializeSafeFromHeader(
                 threadPool.getThreadContext(),
                 ConfigConstants.OPENDISTRO_SECURITY_FLS_FIELDS_HEADER

diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java
@@ -22,7 +22,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Map.Entry;
+import java.util.Properties;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.stream.Collectors;
 
@@ -507,10 +507,7 @@ public void logDocumentRead(String index, String id, ShardId shardId, Map<String
                                 .collect(
                                     Collectors.toMap(
                                         entry -> "id",
-                                        entry -> new String(
-                                            BaseEncoding.base64().decode(((Entry<String, String>) entry).getValue()),
-                                            StandardCharsets.UTF_8
-                                        )
+                                        entry -> new String(BaseEncoding.base64().decode(entry.getValue()), StandardCharsets.UTF_8)
                                     )
                                 );
                             msg.addSecurityConfigMapToRequestBody(Utils.convertJsonToxToStructuredMap(map.get("id")), id);
@@ -711,19 +708,9 @@ protected void logExternalConfig() {
             sm.checkPermission(new SpecialPermission());
         }
 
-        final Map<String, String> envAsMap = AccessController.doPrivileged(new PrivilegedAction<Map<String, String>>() {
-            @Override
-            public Map<String, String> run() {
-                return System.getenv();
-            }
-        });
+        final Map<String, String> envAsMap = AccessController.doPrivileged((PrivilegedAction<Map<String, String>>) System::getenv);
 
-        final Map propsAsMap = AccessController.doPrivileged(new PrivilegedAction<Map>() {
-            @Override
-            public Map run() {
-                return System.getProperties();
-            }
-        });
+        final Properties propsAsMap = AccessController.doPrivileged((PrivilegedAction<Properties>) System::getProperties);
 
         final String sha256 = DigestUtils.sha256Hex(configAsMap.toString() + envAsMap.toString() + propsAsMap.toString());
         AuditMessage msg = new AuditMessage(AuditCategory.COMPLIANCE_EXTERNAL_CONFIG, clusterService, null, null);

diff --git a/src/main/java/org/opensearch/security/auditlog/impl/RequestResolver.java b/src/main/java/org/opensearch/security/auditlog/impl/RequestResolver.java
@@ -376,6 +376,7 @@ private static AuditMessage resolveInner(
         return msg;
     }
 
+    @SuppressWarnings("unchecked")
     private static void addIndicesSourceSafe(
         final AuditMessage msg,
         final String[] indices,
@@ -425,14 +426,15 @@ private static void addIndicesSourceSafe(
                     if (source instanceof BytesReference) {
                         msg.addTupleToRequestBody(convertSource(mediaType, (BytesReference) source));
                     } else {
-                        msg.addMapToRequestBody((Map) source);
+                        msg.addMapToRequestBody((Map<String, ?>) source);
                     }
                 }
             } else if (source != null) {
                 if (source instanceof BytesReference) {
                     msg.addTupleToRequestBody(convertSource(mediaType, (BytesReference) source));
                 } else {
-                    msg.addMapToRequestBody((Map) source);
+                    // noinspection unchecked
+                    msg.addMapToRequestBody((Map<String, ?>) source);
                 }
             }
         }

diff --git a/src/main/java/org/opensearch/security/auditlog/routing/AuditMessageRouter.java b/src/main/java/org/opensearch/security/auditlog/routing/AuditMessageRouter.java
@@ -101,6 +101,7 @@ protected final void close(List<AuditLogSink> sinks) {
         }
     }
 
+    @SuppressWarnings("unchecked")
     public final void enableRoutes(Settings settings) {
         checkState(isEnabled(), "AuditMessageRouter is disabled");
         if (categorySinks != null) {

diff --git a/src/main/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSink.java b/src/main/java/org/opensearch/security/auditlog/sink/InternalOpenSearchSink.java
@@ -71,7 +71,7 @@ public void close() throws IOException {
     public boolean doStore(final AuditMessage msg) {
 
         if (Boolean.parseBoolean(
-            (String) HeaderHelper.getSafeFromHeader(threadPool.getThreadContext(), ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER)
+            HeaderHelper.getSafeFromHeader(threadPool.getThreadContext(), ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER)
         )) {
             if (log.isTraceEnabled()) {
                 log.trace("audit log of audit log will not be executed");

diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/src/main/java/org/opensearch/security/auth/BackendRegistry.java
@@ -63,7 +63,6 @@
 import org.opensearch.security.filter.SecurityResponse;
 import org.opensearch.security.http.XFFResolver;
 import org.opensearch.security.securityconf.DynamicConfigModel;
-import org.opensearch.security.ssl.util.Utils;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.security.user.AuthCredentials;
 import org.opensearch.security.user.User;
@@ -364,7 +363,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
                 return false;
             }
 
-            final String tenant = Utils.coalesce(request.header("securitytenant"), request.header("security_tenant"));
+            final String tenant = resolveTenantFrom(request);
 
             if (isDebugEnabled) {
                 log.debug("Rest user '{}' is authenticated", authenticatedUser);
@@ -392,7 +391,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
             }
 
             if (authCredentials == null && anonymousAuthEnabled) {
-                final String tenant = Utils.coalesce(request.header("securitytenant"), request.header("security_tenant"));
+                final String tenant = resolveTenantFrom(request);
                 User anonymousUser = new User(User.ANONYMOUS.getName(), new HashSet<String>(User.ANONYMOUS.getRoles()), null);
                 anonymousUser.setRequestedTenant(tenant);
 
@@ -437,6 +436,10 @@ public boolean authenticate(final SecurityRequestChannel request) {
         return authenticated;
     }
 
+    private String resolveTenantFrom(final SecurityRequest request) {
+        return Optional.ofNullable(request.header("securitytenant")).orElse(request.header("security_tenant"));
+    }
+
     private void notifyIpAuthFailureListeners(SecurityRequestChannel request, AuthCredentials authCredentials) {
         notifyIpAuthFailureListeners(request.getRemoteAddress().map(InetSocketAddress::getAddress).orElse(null), authCredentials, request);
     }

diff --git a/src/main/java/org/opensearch/security/auth/UserInjector.java b/src/main/java/org/opensearch/security/auth/UserInjector.java
@@ -31,6 +31,7 @@
 import java.net.InetAddress;
 import java.net.UnknownHostException;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.Map;
 
 import com.google.common.base.Strings;
@@ -45,7 +46,6 @@
 import org.opensearch.security.filter.SecurityRequestChannel;
 import org.opensearch.security.http.XFFResolver;
 import org.opensearch.security.support.ConfigConstants;
-import org.opensearch.security.support.SecurityUtils;
 import org.opensearch.security.user.User;
 import org.opensearch.threadpool.ThreadPool;
 
@@ -150,7 +150,7 @@ public InjectedUser getInjectedUser() {
 
         // custom attributes
         if (parts.length > 3 && !Strings.isNullOrEmpty(parts[3])) {
-            Map<String, String> attributes = SecurityUtils.mapFromArray((parts[3].split(",")));
+            Map<String, String> attributes = mapFromArray((parts[3].split(",")));
             if (attributes == null) {
                 log.error("Could not parse custom attributes {}, user injection failed.", parts[3]);
                 return null;
@@ -204,4 +204,21 @@ boolean injectUser(SecurityRequestChannel request) {
 
         return true;
     }
+
+    protected Map<String, String> mapFromArray(String... keyValues) {
+        if (keyValues == null) {
+            return Map.of();
+        }
+        if (keyValues.length % 2 != 0) {
+            log.error("Expected even number of key/value pairs, got {}.", Arrays.toString(keyValues));
+            return null;
+        }
+        Map<String, String> map = new HashMap<>();
+
+        for (int i = 0; i < keyValues.length; i += 2) {
+            map.put(keyValues[i], keyValues[i + 1]);
+        }
+        return map;
+    }
+
 }
diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java
@@ -671,6 +671,7 @@ private static <T> Field getField(Class<T> cls, String name) {
             return AccessController.doPrivileged((PrivilegedAction<Field>) () -> getFieldPrivileged(cls, name));
         }
 
+        @SuppressWarnings("unchecked")
         private static <T, C> T getFieldValue(Field field, C c) {
             try {
                 return (T) field.get(c);

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/MigrateApiAction.java
@@ -222,7 +222,7 @@ public void onResponse(CreateIndexResponse response) {
                                     }
 
                                     br.execute(
-                                        new ConfigUpdatingActionListener(
+                                        new ConfigUpdatingActionListener<>(
                                             cTypes.build().toArray(new String[0]),
                                             client,
                                             new ActionListener<BulkResponse>() {

diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java
@@ -23,13 +23,13 @@
 import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.rest.RestRequest;
 import org.opensearch.security.DefaultObjectMapper;
-import org.opensearch.security.ssl.util.Utils;
 
 import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 
 import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE;
@@ -247,10 +247,8 @@ private ValidationResult<JsonNode> validatePassword(final RestRequest request, f
                 this.validationError = ValidationError.INVALID_PASSWORD_TOO_SHORT;
                 return ValidationResult.error(RestStatus.BAD_REQUEST, this);
             }
-            final String username = Utils.coalesce(
-                request.param("name"),
-                validationContext.hasParams() ? (String) validationContext.params()[0] : null
-            );
+            final String username = Optional.ofNullable(request.param("name"))
+                .orElseGet(() -> validationContext.hasParams() ? (String) validationContext.params()[0] : null);
             if (Strings.isNullOrEmpty(username)) {
                 if (LOGGER.isDebugEnabled()) {
                     LOGGER.debug("Unable to validate username because no user is given");