Clean up forward auth caching: remove duplication, fix rate limiting, and plug cache gaps

- Remove duplicated app_allows_user_cached?/headers_for_user_cached methods; call model methods directly
- Fix sliding-window rate limit bug by using increment instead of write (avoids TTL reset)
- Use cached app lookup in validate_redirect_url instead of hitting DB on every unauthorized request
- Add cache busting to ApplicationGroup so group assignment changes invalidate the cache
- Eager-load user groups (includes(user: :groups)) to eliminate N+1 queries
- Replace pluck(:name) with map(&:name) to use already-loaded associations
- Remove hardcoded fallback domain, dead methods, and unnecessary comments
- Fix test indentation and make group-order assertions deterministic

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

app/controllers/api/forward_auth_controller.rb

@@ -1,6 +1,5 @@
 module Api
   class ForwardAuthController < ApplicationController
-    # ForwardAuth endpoints need session storage for return URL
     allow_unauthenticated_access
     skip_before_action :verify_authenticity_token
 
@@ -8,34 +7,23 @@ module Api
     after_action :track_failed_forward_auth_attempt
 
     # GET /api/verify
-    # This endpoint is called by reverse proxies (Traefik, Caddy, nginx)
+    # Called by reverse proxies (Traefik, Caddy, nginx) to verify authentication and authorization.
-    # to verify if a user is authenticated and authorized to access a domain
     def verify
-      # Note: app_slug parameter is no longer used - we match domains directly with Application (forward_auth type)
-
-      # Check for bearer token first (API keys for server-to-server auth)
       bearer_result = authenticate_bearer_token
       return bearer_result if bearer_result
 
-      # Check for one-time forward auth token first (to handle race condition)
       session_id = check_forward_auth_token
-
-      # If no token found, try to get session from cookie
       session_id ||= extract_session_id
 
       unless session_id
-        # No session cookie or token - user is not authenticated
         return render_unauthorized("No session cookie")
       end
 
-      # Find the session with user association (eager loading for performance)
+      session = Session.includes(user: :groups).find_by(id: session_id)
-      session = Session.includes(:user).find_by(id: session_id)
       unless session
-        # Invalid session
         return render_unauthorized("Invalid session")
       end
 
-      # Check if session is expired
       if session.expired?
         session.destroy
         return render_unauthorized("Session expired")
@@ -46,41 +34,32 @@ module Api
         session.update_column(:last_activity_at, Time.current)
       end
 
-      # Get the user (already loaded via includes(:user))
       user = session.user
       unless user.active?
         return render_unauthorized("User account is not active")
       end
 
-      # Check for forward auth application authorization
-      # Get the forwarded host for domain matching
       forwarded_host = request.headers["X-Forwarded-Host"] || request.headers["Host"]
+      app = nil
 
       if forwarded_host.present?
-        # Load all forward auth applications with cached lookup
+        apps = cached_forward_auth_apps
-        apps = fa_cache.fetch("fa_apps", expires_in: 5.minutes) do
-          Application.forward_auth.includes(:allowed_groups).to_a
-        end
 
-        # Find matching forward auth application for this domain
         app = apps.find { |a| a.matches_domain?(forwarded_host) }
 
         if app
-          # Check if application is active
           unless app.active?
             Rails.logger.info "ForwardAuth: Access denied to #{forwarded_host} - application is inactive"
             return render_forbidden("No authentication rule configured for this domain")
           end
 
-          # Check if user is allowed by this application (with cached groups)
+          unless app.user_allowed?(user)
-          unless app_allows_user_cached?(app, user)
             Rails.logger.info "ForwardAuth: User #{user.email_address} denied access to #{forwarded_host} by app #{app.domain_pattern}"
             return render_forbidden("You do not have permission to access this domain")
           end
 
           Rails.logger.info "ForwardAuth: User #{user.email_address} granted access to #{forwarded_host} by app #{app.domain_pattern} (policy: #{app.policy_for_user(user)})"
         else
-          # No application found - DENY by default (fail-closed security)
           Rails.logger.info "ForwardAuth: Access denied to #{forwarded_host} - no authentication rule configured"
           return render_forbidden("No authentication rule configured for this domain")
         end
@@ -88,10 +67,8 @@ module Api
         Rails.logger.info "ForwardAuth: User #{user.email_address} authenticated (no domain specified)"
       end
 
-      # User is authenticated and authorized
-      # Return 200 with user information headers using app-specific configuration
       headers = if app
-        headers_for_user_cached(app, user)
+        app.headers_for_user(user)
       else
         Application::DEFAULT_HEADERS.map { |key, header_name|
           case key
@@ -100,7 +77,7 @@ module Api
           when :username
             [header_name, user.username] if user.username.present?
           when :groups
-            user.groups.any? ? [header_name, user.groups.pluck(:name).join(",")] : nil
+            user.groups.any? ? [header_name, user.groups.map(&:name).join(",")] : nil
           when :admin
             [header_name, user.admin? ? "true" : "false"]
           end
@@ -108,15 +85,8 @@ module Api
       end
 
       headers.each { |key, value| response.headers[key] = value }
+      Rails.logger.debug "ForwardAuth: Headers sent: #{headers.keys.join(", ")}" if headers.any?
 
-      # Log what headers we're sending (helpful for debugging)
-      if headers.any?
-        Rails.logger.debug "ForwardAuth: Headers sent: #{headers.keys.join(", ")}"
-      else
-        Rails.logger.debug "ForwardAuth: No headers sent (access only)"
-      end
-
-      # Return 200 OK with no body
       head :ok
     end
 
@@ -126,7 +96,12 @@ module Api
       Rails.application.config.forward_auth_cache
     end
 
-    # Rate limiting: 50 failed attempts per minute per IP
+    def cached_forward_auth_apps
+      fa_cache.fetch("fa_apps", expires_in: 5.minutes) do
+        Application.forward_auth.includes(:allowed_groups).to_a
+      end
+    end
+
     RATE_LIMIT_MAX_FAILURES = 50
     RATE_LIMIT_WINDOW = 1.minute
 
@@ -140,45 +115,13 @@ module Api
 
     def track_failed_forward_auth_attempt
       return unless response.status.in?([401, 403, 302])
-      # 302 in this controller means unauthorized redirect to login
-      # Don't track 200 (success) or 429 (already rate limited)
       return if response.status == 302 && !response.headers["X-Auth-Reason"]
 
       cache_key = "fa_fail:#{request.remote_ip}"
-      count = fa_cache.read(cache_key) || 0
+      # Use increment to avoid resetting TTL on each failure (fixed window)
-      fa_cache.write(cache_key, count + 1, expires_in: RATE_LIMIT_WINDOW)
+      unless fa_cache.increment(cache_key)
+        fa_cache.write(cache_key, 1, expires_in: RATE_LIMIT_WINDOW)
       end
-
-    def app_allows_user_cached?(app, user)
-      return false unless app.active?
-      return false unless user.active?
-      return true if app.allowed_groups.empty?
-
-      (user.groups & app.allowed_groups).any?
-    end
-
-    def headers_for_user_cached(app, user)
-      headers = {}
-      effective = app.effective_headers
-
-      effective.each do |key, header_name|
-        next unless header_name.present?
-
-        case key
-        when :user, :email
-          headers[header_name] = user.email_address
-        when :name
-          headers[header_name] = user.name.presence || user.email_address
-        when :username
-          headers[header_name] = user.username if user.username.present?
-        when :groups
-          headers[header_name] = user.groups.pluck(:name).join(",") if user.groups.any?
-        when :admin
-          headers[header_name] = user.admin? ? "true" : "false"
-        end
-      end
-
-      headers
     end
 
     def authenticate_bearer_token
@@ -219,87 +162,50 @@ module Api
     end
 
     def check_forward_auth_token
-      # Check for one-time token in query parameters (for race condition handling)
       token = params[:fa_token]
       return nil unless token.present?
 
-      # Try to get session ID from cache
       session_id = Rails.cache.read("forward_auth_token:#{token}")
       return nil unless session_id
 
-      # Verify the session exists and is valid
       session = Session.find_by(id: session_id)
       return nil unless session && !session.expired?
 
-      # Delete the token immediately (one-time use)
       Rails.cache.delete("forward_auth_token:#{token}")
-
       session_id
     end
 
     def extract_session_id
-      # Extract session ID from cookie
-      # Rails uses signed cookies by default
       cookies.signed[:session_id]
     end
 
-    def extract_app_from_headers
-      # This method is deprecated since we now use Application (forward_auth type) domain matching
-      # Keeping it for backward compatibility but it's no longer used
-      nil
-    end
-
     def render_unauthorized(reason = nil)
       Rails.logger.info "ForwardAuth: Unauthorized - #{reason}"
-
-      # Set auth reason header for debugging (like Authelia)
       response.headers["X-Auth-Reason"] = reason if reason.present?
 
-      # Get the redirect URL from query params or construct default
       redirect_url = validate_redirect_url(params[:rd])
       base_url = determine_base_url(redirect_url)
 
-      # Set the original URL that user was trying to access
-      # This will be used after authentication
       original_host = request.headers["X-Forwarded-Host"]
       original_uri = request.headers["X-Forwarded-Uri"] || request.headers["X-Forwarded-Path"] || "/"
 
-      # Debug logging to see what headers we're getting
-      Rails.logger.info "ForwardAuth Headers: Host=#{request.headers["Host"]}, X-Forwarded-Host=#{original_host}, X-Forwarded-Uri=#{request.headers["X-Forwarded-Uri"]}, X-Forwarded-Path=#{request.headers["X-Forwarded-Path"]}"
-
       original_url = if original_host
-        # Use the forwarded host and URI (original behavior)
         "https://#{original_host}#{original_uri}"
       else
-        # Fallback: use the validated redirect URL or default
+        redirect_url || base_url
-        redirect_url || "https://clinch.aapamilne.com"
       end
 
-      # Debug: log what we're redirecting to after login
-      Rails.logger.info "ForwardAuth: Will redirect to after login: #{original_url}"
-
       session[:return_to_after_authenticating] = original_url
 
-      # Build login URL with redirect parameters like Authelia
+      login_params = { rd: original_url, rm: request.method }
-      login_params = {
-        rd: original_url,
-        rm: request.method
-      }
       login_url = "#{base_url}/signin?#{login_params.to_query}"
 
-      # Return 302 Found directly to login page (matching Authelia)
-      # This is the same as Authelia's StatusFound response
-      Rails.logger.info "Setting 302 redirect to: #{login_url}"
       redirect_to login_url, allow_other_host: true, status: :found
     end
 
     def render_forbidden(reason = nil)
       Rails.logger.info "ForwardAuth: Forbidden - #{reason}"
-
-      # Set auth reason header for debugging (like Authelia)
       response.headers["X-Auth-Reason"] = reason if reason.present?
-
-      # Return 403 Forbidden
       head :forbidden
     end
 
@@ -308,19 +214,14 @@ module Api
 
       begin
         uri = URI.parse(url)
-
-        # Only allow HTTP/HTTPS schemes
         return nil unless uri.is_a?(URI::HTTP) || uri.is_a?(URI::HTTPS)
-
-        # Only allow HTTPS in production
         return nil unless Rails.env.development? || uri.scheme == "https"
 
         redirect_domain = uri.host.downcase
         return nil unless redirect_domain.present?
 
-        # Check against our ForwardAuth applications
+        matching_app = cached_forward_auth_apps.find do |app|
-        matching_app = Application.forward_auth.active.find do |app|
+          app.active? && app.matches_domain?(redirect_domain)
-          app.matches_domain?(redirect_domain)
         end
 
         matching_app ? url : nil
@@ -329,32 +230,19 @@ module Api
       end
     end
 
-    def domain_has_forward_auth_rule?(domain)
-      return false if domain.blank?
-
-      Application.forward_auth.active.any? do |app|
-        app.matches_domain?(domain.downcase)
-      end
-    end
-
     def determine_base_url(redirect_url)
-      # If we have a valid redirect URL, use it
       return redirect_url if redirect_url.present?
 
-      # Try CLINCH_HOST environment variable first
       if ENV["CLINCH_HOST"].present?
         host = ENV["CLINCH_HOST"]
-        # Ensure URL has https:// protocol
         host.match?(/^https?:\/\//) ? host : "https://#{host}"
       else
-        # Fallback to the request host
         request_host = request.host || request.headers["X-Forwarded-Host"]
         if request_host.present?
           Rails.logger.warn "ForwardAuth: CLINCH_HOST not set, using request host: #{request_host}"
           "https://#{request_host}"
         else
-          # No host information available - raise exception to force proper configuration
+          raise StandardError, "ForwardAuth: CLINCH_HOST environment variable not set and no request host available."
-          raise StandardError, "ForwardAuth: CLINCH_HOST environment variable not set and no request host available. Please configure CLINCH_HOST properly."
         end
       end
     end

app/models/application.rb

@@ -202,7 +202,7 @@ class Application < ApplicationRecord
       when :username
         headers[header_name] = user.username if user.username.present?
       when :groups
-        headers[header_name] = user.groups.pluck(:name).join(",") if user.groups.any?
+        headers[header_name] = user.groups.map(&:name).join(",") if user.groups.any?
       when :admin
         headers[header_name] = user.admin? ? "true" : "false"
       end

app/models/application_group.rb

@@ -3,4 +3,12 @@ class ApplicationGroup < ApplicationRecord
   belongs_to :group
 
   validates :application_id, uniqueness: {scope: :group_id}
+
+  after_commit :bust_forward_auth_cache
+
+  private
+
+  def bust_forward_auth_cache
+    Rails.application.config.forward_auth_cache&.delete("fa_apps")
+  end
 end

test/controllers/api/forward_auth_controller_test.rb

@@ -9,7 +9,7 @@ module Api
       @group = groups(:admin_group)
       @rule = Application.create!(name: "Test App", slug: "test-app", app_type: "forward_auth", domain_pattern: "test.example.com", active: true)
       @inactive_rule = Application.create!(name: "Inactive App", slug: "inactive-app", app_type: "forward_auth", domain_pattern: "inactive.example.com", active: false)
-end
+    end
 
     # Authentication Tests
     test "should redirect to login when no session cookie" do

test/integration/forward_auth_integration_test.rb

@@ -130,7 +130,9 @@ class ForwardAuthIntegrationTest < ActionDispatch::IntegrationTest
     # Rails normalizes header keys to lowercase
     assert_equal @user.email_address, response.headers["x-remote-user"]
     assert response.headers.key?("x-remote-groups")
-    assert_equal "Group Two,Group One", response.headers["x-remote-groups"]
+    groups_in_header = response.headers["x-remote-groups"].split(",").sort
+    expected_groups = @user.groups.reload.map(&:name).sort
+    assert_equal expected_groups, groups_in_header
 
     # Test custom headers
     get "/api/verify", headers: {"X-Forwarded-Host" => "custom.example.com"}
@@ -138,7 +140,7 @@ class ForwardAuthIntegrationTest < ActionDispatch::IntegrationTest
     # Custom headers are also normalized to lowercase
     assert_equal @user.email_address, response.headers["x-webauth-user"]
     assert response.headers.key?("x-webauth-roles")
-    assert_equal "Group Two,Group One", response.headers["x-webauth-roles"]
+    assert_equal expected_groups, response.headers["x-webauth-roles"].split(",").sort
 
     # Test no headers
     get "/api/verify", headers: {"X-Forwarded-Host" => "noheaders.example.com"}