Address code review feedback: use git-proxy.yml, add env vars, remove GitHub auth filter, use info log

Copilot · coopernetes · Copilot · commit fc4df6d5a051 · 2025-12-15T23:20:32.000Z
- Changed config files from application.yml/application-local.yml to git-proxy.yml/git-proxy-local.yml for Jetty separation from Spring
- Added environment variable override support with GITPROXY_ prefix (GITPROXY_SERVER_PORT, GITPROXY_GITPROXY_BASEPATH)
- Removed GitHubUserAuthenticatedFilter entirely (GitHub enforces auth itself, proxy forwards errors transparently)
- Changed disabled provider log level from debug to info for better visibility
- Updated CONFIGURATION.md to document new file names and environment variable support
- Removed GitHub authentication filter from config examples

Co-authored-by: coopernetes &lt;57812123+coopernetes@users.noreply.github.com&gt;
diff --git a/CONFIGURATION.md b/CONFIGURATION.md
@@ -1,14 +1,24 @@
 # Configuration Guide
 
-The Jetty-based GitProxy server supports configuration via YAML files. This allows you to configure providers, filters, and other server settings without modifying code.
+The Jetty-based GitProxy server supports configuration via YAML files and environment variables. This allows you to configure providers, filters, and other server settings without modifying code.
 
 ## Configuration Files
 
 The server loads configuration from the following files in order:
-1. `src/main/resources/application.yml` - Base configuration
-2. `src/main/resources/application-local.yml` - Local overrides (merged with base)
+1. `src/main/resources/git-proxy.yml` - Base configuration
+2. `src/main/resources/git-proxy-local.yml` - Local overrides (merged with base)
+3. Environment variables with `GITPROXY_` prefix
 
-Configuration from `application-local.yml` will override or extend settings from `application.yml`.
+Configuration from `git-proxy-local.yml` will override or extend settings from `git-proxy.yml`, and environment variables will override both.
+
+## Environment Variable Overrides
+
+You can override certain configuration values using environment variables with the `GITPROXY_` prefix:
+
+- `GITPROXY_SERVER_PORT`: Override the server port (e.g., `GITPROXY_SERVER_PORT=9090`)
+- `GITPROXY_GITPROXY_BASEPATH`: Override the base path (e.g., `GITPROXY_GITPROXY_BASEPATH=/proxy`)
+
+Note: Whitelist configurations are not supported via environment variables due to their complex structure.
 
 ## Server Configuration
 
@@ -61,29 +71,7 @@ git-proxy:
 
 Filters control access to repositories and enforce policies.
 
-### GitHub User Authentication Filter
-
-Requires authentication for GitHub operations.
-
-```yaml
-git-proxy:
-  filters:
-    github-user-authenticated:
-      enabled: true
-      order: 1
-      operations:
-        - PUSH
-      required-auth-schemes: bearer, token, basic  # Can be comma-separated or list
-      providers:
-        - github
-```
-
-Options:
-- `enabled` (boolean): Enable/disable the filter
-- `order` (int): Filter execution order (lower numbers run first)
-- `operations` (list): Git operations to apply filter to (PUSH, FETCH)
-- `required-auth-schemes` (string or list): Required authentication schemes (bearer, token, basic)
-- `providers` (list): Provider names to apply filter to
+Note: GitHub already enforces authentication for push operations using personal access tokens (PATs). The proxy transparently forwards requests upstream and returns errors from GitHub directly, so authentication checking is handled by GitHub itself.
 
 ### Whitelist Filters
 
diff --git a/src/main/java/org/finos/gitproxy/config/JettyConfigurationBuilder.java b/src/main/java/org/finos/gitproxy/config/JettyConfigurationBuilder.java
@@ -3,7 +3,6 @@
 import java.net.URI;
 import java.util.*;
 import lombok.extern.slf4j.Slf4j;
-import org.finos.gitproxy.git.HttpAuthScheme;
 import org.finos.gitproxy.git.HttpOperation;
 import org.finos.gitproxy.provider.*;
 import org.finos.gitproxy.servlet.filter.*;
@@ -41,7 +40,7 @@ public List<GitProxyProvider> buildProviders() {
 
             boolean enabled = (Boolean) providerConfig.getOrDefault("enabled", false);
             if (!enabled) {
-                log.debug("Provider {} is disabled, skipping", providerName);
+                log.info("Provider {} is disabled, skipping", providerName);
                 continue;
             }
 
@@ -108,9 +107,6 @@ public List<GitProxyFilter> buildFiltersForProvider(GitProxyProvider provider) {
         filters.add(new ForceGitClientFilter());
         filters.add(new ParseGitRequestFilter(provider));
 
-        // Build GitHub user authenticated filter
-        filters.addAll(buildGitHubUserAuthFilter(provider, filtersConfig));
-
         // Build whitelist filters
         filters.addAll(buildWhitelistFilters(provider, filtersConfig));
 
@@ -121,74 +117,6 @@ public List<GitProxyFilter> buildFiltersForProvider(GitProxyProvider provider) {
         return filters;
     }
 
-    /** Build GitHub user authentication filter if configured. */
-    @SuppressWarnings("unchecked")
-    private List<GitProxyFilter> buildGitHubUserAuthFilter(
-            GitProxyProvider provider, Map<String, Object> filtersConfig) {
-        List<GitProxyFilter> filters = new ArrayList<>();
-
-        if (!(provider instanceof GitHubProvider)) {
-            return filters;
-        }
-
-        Object githubUserAuthConfig = filtersConfig.get("github-user-authenticated");
-        if (githubUserAuthConfig == null) {
-            return filters;
-        }
-
-        Map<String, Object> authConfig = (Map<String, Object>) githubUserAuthConfig;
-        boolean enabled = (Boolean) authConfig.getOrDefault("enabled", false);
-        if (!enabled) {
-            return filters;
-        }
-
-        // Check if this provider is in the list
-        List<String> providerList = (List<String>) authConfig.get("providers");
-        if (providerList != null && !providerList.contains(provider.getName())) {
-            return filters;
-        }
-
-        int order = (Integer) authConfig.getOrDefault("order", 1);
-        Object authSchemesObj = authConfig.get("required-auth-schemes");
-
-        Set<HttpAuthScheme> schemes = new HashSet<>();
-        if (authSchemesObj != null) {
-            List<String> authSchemes;
-            if (authSchemesObj instanceof List) {
-                authSchemes = (List<String>) authSchemesObj;
-            } else if (authSchemesObj instanceof String) {
-                // Handle comma-separated string
-                String authSchemesStr = (String) authSchemesObj;
-                authSchemes = Arrays.asList(authSchemesStr.split("\\s*,\\s*"));
-            } else {
-                authSchemes = new ArrayList<>();
-            }
-
-            for (String scheme : authSchemes) {
-                switch (scheme.toLowerCase().trim()) {
-                    case "basic":
-                        schemes.add(HttpAuthScheme.BASIC);
-                        break;
-                    case "bearer":
-                        schemes.add(HttpAuthScheme.BEARER);
-                        break;
-                    case "token":
-                        schemes.add(HttpAuthScheme.TOKEN);
-                        break;
-                }
-            }
-        } else {
-            schemes.add(HttpAuthScheme.BEARER);
-        }
-
-        GitHubUserAuthenticatedFilter filter =
-                new GitHubUserAuthenticatedFilter(order, (GitHubProvider) provider, schemes);
-        filters.add(filter);
-        log.info("Created GitHubUserAuthenticatedFilter for provider: {}", provider.getName());
-
-        return filters;
-    }
-
     /** Build whitelist filters if configured. */
     @SuppressWarnings("unchecked")
     private List<GitProxyFilter> buildWhitelistFilters(GitProxyProvider provider, Map<String, Object> filtersConfig) {
diff --git a/src/main/java/org/finos/gitproxy/config/JettyConfigurationLoader.java b/src/main/java/org/finos/gitproxy/config/JettyConfigurationLoader.java
@@ -8,14 +8,18 @@
 
 /**
  * Configuration loader for the Jetty-based GitProxy application. This class reads configuration from YAML files
- * (application.yml, application-local.yml) and provides a structured representation of the configuration that can be
- * used to bootstrap the Jetty server with appropriate providers and filters.
+ * (git-proxy.yml, git-proxy-local.yml) and environment variables, providing a structured representation of the
+ * configuration that can be used to bootstrap the Jetty server with appropriate providers and filters.
+ *
+ * <p>Environment variables with the prefix GITPROXY_ can override configuration values. For example: -
+ * GITPROXY_SERVER_PORT=9090 overrides server.port - GITPROXY_GITPROXY_BASEPATH=/proxy overrides git-proxy.base-path
  */
 @Slf4j
 public class JettyConfigurationLoader {
 
-    private static final String DEFAULT_CONFIG = "application.yml";
-    private static final String LOCAL_CONFIG = "application-local.yml";
+    private static final String DEFAULT_CONFIG = "git-proxy.yml";
+    private static final String LOCAL_CONFIG = "git-proxy-local.yml";
+    private static final String ENV_PREFIX = "GITPROXY_";
 
     private Map<String, Object> config;
 
@@ -24,8 +28,8 @@ public JettyConfigurationLoader() {
     }
 
     /**
-     * Load configuration from YAML files. Loads application.yml first, then overlays application-local.yml if it
-     * exists.
+     * Load configuration from YAML files and environment variables. Loads git-proxy.yml first, then overlays
+     * git-proxy-local.yml if it exists, and finally applies environment variable overrides.
      */
     private Map<String, Object> loadConfiguration() {
         Yaml yaml = new Yaml();
@@ -39,6 +43,8 @@ private Map<String, Object> loadConfiguration() {
                     baseConfig = loaded;
                     log.info("Loaded base configuration from {}", DEFAULT_CONFIG);
                 }
+            } else {
+                log.warn("Base configuration file {} not found, using defaults", DEFAULT_CONFIG);
             }
         } catch (IOException e) {
             log.warn("Failed to load base configuration from {}: {}", DEFAULT_CONFIG, e.getMessage());
@@ -52,11 +58,16 @@ private Map<String, Object> loadConfiguration() {
                     deepMerge(baseConfig, localConfig);
                     log.info("Loaded and merged local configuration from {}", LOCAL_CONFIG);
                 }
+            } else {
+                log.debug("Local configuration file {} not found, skipping", LOCAL_CONFIG);
             }
         } catch (IOException e) {
             log.debug("No local configuration found at {}", LOCAL_CONFIG);
         }
 
+        // Apply environment variable overrides
+        applyEnvironmentOverrides(baseConfig);
+
         return baseConfig;
     }
 
@@ -80,6 +91,44 @@ private void deepMerge(Map<String, Object> base, Map<String, Object> overlay) {
         }
     }
 
+    /**
+     * Apply environment variable overrides to configuration. Supported environment variables: - GITPROXY_SERVER_PORT:
+     * Override server port - GITPROXY_GITPROXY_BASEPATH: Override git-proxy base path
+     */
+    @SuppressWarnings("unchecked")
+    private void applyEnvironmentOverrides(Map<String, Object> config) {
+        Map<String, String> env = System.getenv();
+
+        // Override server port
+        String portEnv = env.get(ENV_PREFIX + "SERVER_PORT");
+        if (portEnv != null) {
+            try {
+                int port = Integer.parseInt(portEnv);
+                Map<String, Object> serverConfig =
+                        (Map<String, Object>) config.computeIfAbsent("server", k -> new HashMap<>());
+                serverConfig.put("port", port);
+                log.info("Overriding server port from environment: {}", port);
+            } catch (NumberFormatException e) {
+                log.warn("Invalid port value in GITPROXY_SERVER_PORT: {}", portEnv);
+            }
+        }
+
+        // Override base path
+        String basePathEnv = env.get(ENV_PREFIX + "GITPROXY_BASEPATH");
+        if (basePathEnv != null) {
+            Map<String, Object> gitProxyConfig =
+                    (Map<String, Object>) config.computeIfAbsent("git-proxy", k -> new HashMap<>());
+            gitProxyConfig.put("base-path", basePathEnv);
+            log.info("Overriding base path from environment: {}", basePathEnv);
+        }
+
+        // Log other GITPROXY_ variables for visibility
+        env.keySet().stream()
+                .filter(k -> k.startsWith(ENV_PREFIX))
+                .filter(k -> !k.equals(ENV_PREFIX + "SERVER_PORT") && !k.equals(ENV_PREFIX + "GITPROXY_BASEPATH"))
+                .forEach(k -> log.debug("Found environment variable {} (not currently supported)", k));
+    }
+
     /** Get the git-proxy configuration section. */
     @SuppressWarnings("unchecked")
     public Map<String, Object> getGitProxyConfig() {
diff --git a/src/main/resources/git-proxy-local.yml b/src/main/resources/git-proxy-local.yml
@@ -0,0 +1,60 @@
+management:
+  endpoints:
+    web:
+      exposure:
+        include: "*"
+logging:
+  level:
+    org.finos.gitproxy: DEBUG
+    org.finos.gitproxy.git: DEBUG
+#    org.eclipse.jgit.transport: DEBUG
+git-proxy:
+  filters:
+    whitelists:
+      - enabled: true
+        order: 5
+        operations:
+          - FETCH
+          - PUSH
+        providers:
+          - gitlab
+        slugs:
+          - coopernetes/test-repo
+        owners:
+          - finosfoundation
+        names:
+          - hello-world
+      - enabled: true
+        order: 10
+        operations:
+          - PUSH
+        owners:
+          - finosfoundation
+        providers:
+          - gitlab
+      - enabled: true
+        order: 10
+        operations:
+          - PUSH
+        slugs:
+          - finos/git-proxy
+        providers:
+          - github
+      - enabled: true
+        order: 20
+        operations:
+          - FETCH
+        owners:
+          - finos
+        providers:
+          - github
+      - enabled: true
+        order: 30
+        operations:
+          - FETCH
+          - PUSH
+        slugs:
+          - coopernetes/test-repo
+          - coopernetes/test-repo2
+        providers:
+          - github
diff --git a/src/main/resources/git-proxy.yml b/src/main/resources/git-proxy.yml
@@ -0,0 +1,37 @@
+spring:
+  application:
+    name: jgit-proxy
+server:
+  port: 8080
+management:
+  endpoints:
+    web:
+      exposure:
+        include: mappings, health, info
+logging:
+  level:
+    org.finos.gitproxy: INFO
+# The below is a minimal configuration needed for the app to start and is the default shipped with the jar.
+# By default, the app supports proxying to GitHub, GitLab, and Bitbucket. Users should override this configuration
+# to suit their needs.
+git-proxy:
+#  base-path: "/git"
+  providers:
+    github:
+      enabled: true
+    gitlab:
+      enabled: true
+    bitbucket:
+      enabled: true
+#    internal-github:
+#      enabled: true
+#      servlet-path: /enterprise-github
+#      uri: https://githubserver.example.com
+#    internal-gitlab:
+#      enabled: true
+#      servlet-path: /external-github
+#      uri: https://gitlabserver.other.example.com
+    debian-gitlab:
+      enabled: true
+      servlet-path: /debian
+      uri: https://salsa.debian.org/
diff --git a/src/test/resources/git-proxy.yml b/src/test/resources/git-proxy.yml
@@ -0,0 +1,8 @@
+git-proxy:
+  providers:
+    github:
+      enabled: true
+logging:
+  level:
+    com.fasterxml.jackson: TRACE
+    org.springframework.boot.context.config: TRACE
