[GitHub] [ozone] swagle commented on a change in pull request #2638: HDDS-5691. Restrict Recon NSSummaryEndpoint and ContainerEndpoint to admins.

[GitBox <gi...@apache.org>]

swagle commented on a change in pull request #2638:
URL: https://github.com/apache/ozone/pull/2638#discussion_r711390323



##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconRestServletModule.java
##########
@@ -32,73 +34,111 @@
 import org.glassfish.jersey.servlet.ServletContainer;
 import org.jvnet.hk2.guice.bridge.api.GuiceBridge;
 import org.jvnet.hk2.guice.bridge.api.GuiceIntoHK2Bridge;
+import org.reflections.Reflections;
+import org.reflections.scanners.SubTypesScanner;
+import org.reflections.scanners.TypeAnnotationsScanner;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.google.inject.Injector;
-import com.google.inject.Scopes;
-import com.google.inject.servlet.ServletModule;
+import javax.ws.rs.Path;
+import java.net.URL;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_ENABLED;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_ENABLED_DEFAULT;
 
 /**
  * Class to scan API Service classes and bind them to the injector.
  */
-public abstract class ReconRestServletModule extends ServletModule {
+public class ReconRestServletModule extends ServletModule {
+
+  public static final String BASE_API_PATH = "/api/v1";
+  public static final String API_PACKAGE = "org.apache.hadoop.ozone.recon.api";
 
   private static final Logger LOG =
       LoggerFactory.getLogger(ReconRestServletModule.class);
 
-  @Override
-  protected abstract void configureServlets();
+  private final ConfigurationSource conf;
 
-  /**
-   * Interface to provide packages for scanning.
-   */
-  public interface RestKeyBindingBuilder {
-    void packages(String... packages);
+  public ReconRestServletModule(ConfigurationSource conf) {
+    this.conf = conf;
   }
 
-  protected RestKeyBindingBuilder rest(String... urlPatterns) {
-    return new RestKeyBindingBuilderImpl(Arrays.asList(urlPatterns));
+  @Override
+  protected void configureServlets() {
+    configureApi(BASE_API_PATH, API_PACKAGE);
   }
 
-  private class RestKeyBindingBuilderImpl implements RestKeyBindingBuilder {
-    private List<String> paths;
-
-    RestKeyBindingBuilderImpl(List<String> paths) {
-      this.paths = paths;
-    }
+  private void configureApi(String baseApiPath, String... packages) {
+    StringBuilder sb = new StringBuilder();
+    Set<String> adminEndpoints = new HashSet<>();
 
-    private void checkIfPackageExistsAndLog(String pkg) {
-      String resourcePath = pkg.replace(".", "/");
-      URL resource = getClass().getClassLoader().getResource(resourcePath);
-      if (resource != null) {
-        LOG.info("rest({}).packages({})", paths, pkg);
-      } else {
-        LOG.info("No Beans in '{}' found. Requests {} will fail.", pkg, paths);
+    for (String pkg : packages) {
+      if (sb.length() > 0) {
+        sb.append(',');
+      }
+      checkIfPackageExistsAndLog(pkg, baseApiPath);
+      sb.append(pkg);
+      // Check for classes marked as admin only that will need an extra
+      // filter applied to their path.
+      Reflections reflections = new Reflections(pkg,
+          new TypeAnnotationsScanner(), new SubTypesScanner());
+      Set<Class<?>> adminEndpointClasses =
+          reflections.getTypesAnnotatedWith(AdminOnly.class);
+      adminEndpointClasses.stream()
+          .map(clss -> clss.getAnnotation(Path.class).value())
+          .forEachOrdered(adminEndpoints::add);
+      if (LOG.isDebugEnabled()) {
+        LOG.debug("Registered the following endpoint classes as admin only: {}",
+            adminEndpointClasses);
       }
     }
+    Map<String, String> params = new HashMap<>();
+    params.put("javax.ws.rs.Application",
+        GuiceResourceConfig.class.getCanonicalName());
+    if (sb.length() > 0) {
+      params.put("jersey.config.server.provider.packages", sb.toString());
+    }
+    bind(ServletContainer.class).in(Scopes.SINGLETON);
 
-    @Override
-    public void packages(String... packages) {
-      StringBuilder sb = new StringBuilder();
+    serve(baseApiPath + "/*").with(ServletContainer.class, params);
+    addFilters(baseApiPath, adminEndpoints);
+  }
 
-      for (String pkg : packages) {
-        if (sb.length() > 0) {
-          sb.append(',');
-        }
-        checkIfPackageExistsAndLog(pkg);
-        sb.append(pkg);
+  private void addFilters(String basePath, Set<String> subPaths) {
+    if (OzoneSecurityUtil.isHttpSecurityEnabled(conf)) {
+      filter(basePath + "/*").through(ReconAuthFilter.class);
+      if (LOG.isDebugEnabled()) {
+        LOG.debug("Added authentication filter to all paths under {}",
+            basePath);
       }
-      Map<String, String> params = new HashMap<>();
-      params.put("javax.ws.rs.Application",
-          GuiceResourceConfig.class.getCanonicalName());
-      if (sb.length() > 0) {
-        params.put("jersey.config.server.provider.packages", sb.toString());
+
+      boolean aclEnabled = conf.getBoolean(OZONE_ACL_ENABLED,
+          OZONE_ACL_ENABLED_DEFAULT);
+      if (aclEnabled) {
+        for (String path: subPaths) {
+          filter(basePath + path).through(ReconAdminFilter.class);

Review comment:
       Ideally, instead of string concat it is safer to use UrlBuilder which will take care of appending separators.




-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org