feat: validate OIDC discovery document on custom provider create/update (#2390)

## Summary
- Fetch and validate OIDC discovery documents when creating or updating
custom OIDC providers, catching misconfigurations at admin time
- Validate required fields (issuer, authorization_endpoint,
token_endpoint, jwks_uri) and verify the discovery issuer matches the
configured issuer per OpenID Connect Discovery 1.0, Section 4.3
- Store validated discovery in the existing `cached_discovery` DB column
- Invalidate in-memory OIDC cache on provider update (issuer change) and
delete
- Keep network calls outside DB transactions to avoid holding
connections open

Author: cemalkilic

internal/api/custom_oauth_admin.go

@@ -1,9 +1,13 @@
 package api
 
 import (
+	"context"
+	"encoding/json"
+	"io"
 	"net/http"
 	"slices"
 	"strings"
+	"time"
 
 	"github.com/go-chi/chi/v5"
 	popslices "github.com/gobuffalo/pop/v6/slices"
@@ -208,6 +212,17 @@ func (a *API) adminCustomOAuthProviderCreate(w http.ResponseWriter, r *http.Requ
 	// Create provider model
 	provider := buildProviderFromParams(params, providerType)
 
+	// For OIDC providers, fetch and validate the discovery document before persisting.
+	// This catches misconfigurations (bad issuer URL, missing endpoints) at admin time
+	// rather than failing silently at user login time.
+	if providerType == models.ProviderTypeOIDC {
+		discovery, err := fetchAndValidateDiscovery(ctx, provider.GetDiscoveryURL(), params.Issuer)
+		if err != nil {
+			return err
+		}
+		provider.SetDiscoveryCache(discovery)
+	}
+
 	// Encrypt and store client secret
 	if err := provider.SetClientSecret(params.ClientSecret, config.Security.DBEncryption); err != nil {
 		return apierrors.NewInternalServerError("Error encrypting custom OAuth provider client secret").WithInternalError(err)
@@ -271,29 +286,46 @@ func (a *API) adminCustomOAuthProviderUpdate(w http.ResponseWriter, r *http.Requ
 		}
 	}
 
-	var provider *models.CustomOAuthProvider
-	err := db.Transaction(func(tx *storage.Connection) error {
-		var terr error
-		provider, terr = models.FindCustomOAuthProviderByIdentifier(tx, identifier)
-		if terr != nil {
-			if models.IsNotFoundError(terr) {
-				return apierrors.NewNotFoundError(apierrors.ErrorCodeCustomProviderNotFound, "Custom OAuth provider not found")
-			}
-			return apierrors.NewInternalServerError("Error retrieving custom OAuth provider").WithInternalError(terr)
+	// Read the existing provider outside the write transaction so the
+	// network call (discovery fetch) doesn't hold a transaction open.
+	provider, err := models.FindCustomOAuthProviderByIdentifier(db, identifier)
+	if err != nil {
+		if models.IsNotFoundError(err) {
+			return apierrors.NewNotFoundError(apierrors.ErrorCodeCustomProviderNotFound, "Custom OAuth provider not found")
 		}
+		return apierrors.NewInternalServerError("Error retrieving custom OAuth provider").WithInternalError(err)
+	}
 
-		// Update provider with new non-secret values
-		if terr := updateProviderFromParams(provider, params); terr != nil {
-			return terr
+	// Capture the current issuer before applying updates so we can
+	// invalidate the in-memory cache if it changes.
+	var oldIssuer string
+	if provider.IsOIDC() && provider.Issuer != nil {
+		oldIssuer = *provider.Issuer
+	}
+
+	// Update provider with new non-secret values
+	if err := updateProviderFromParams(provider, params); err != nil {
+		return err
+	}
+
+	// For OIDC providers, re-validate discovery when the issuer or discovery URL changes.
+	// This network call happens outside the transaction to avoid holding it open.
+	if provider.IsOIDC() && (params.Issuer != "" || params.DiscoveryURL != nil) {
+		discovery, err := fetchAndValidateDiscovery(ctx, provider.GetDiscoveryURL(), *provider.Issuer)
+		if err != nil {
+			return err
 		}
+		provider.SetDiscoveryCache(discovery)
+	}
 
-		// If a new client secret is provided, encrypt and store it (likely move to out of the transaction)
-		if params.ClientSecret != "" {
-			if terr := provider.SetClientSecret(params.ClientSecret, config.Security.DBEncryption); terr != nil {
-				return apierrors.NewInternalServerError("Error encrypting custom OAuth provider client secret").WithInternalError(terr)
-			}
+	// If a new client secret is provided, encrypt and store it
+	if params.ClientSecret != "" {
+		if err := provider.SetClientSecret(params.ClientSecret, config.Security.DBEncryption); err != nil {
+			return apierrors.NewInternalServerError("Error encrypting custom OAuth provider client secret").WithInternalError(err)
 		}
+	}
 
+	err = db.Transaction(func(tx *storage.Connection) error {
 		if terr := models.UpdateCustomOAuthProvider(tx, provider); terr != nil {
 			return apierrors.NewInternalServerError("Error updating custom OAuth provider").WithInternalError(terr)
 		}
@@ -307,6 +339,17 @@ func (a *API) adminCustomOAuthProviderUpdate(w http.ResponseWriter, r *http.Requ
 		return err
 	}
 
+	// Invalidate in-memory OIDC cache if the issuer changed or discovery was refreshed,
+	// so the next auth request picks up the new configuration.
+	if provider.IsOIDC() && provider.Issuer != nil {
+		if oldIssuer != "" && oldIssuer != *provider.Issuer {
+			a.oidcCache.Invalidate(oldIssuer)
+		}
+		if params.Issuer != "" || params.DiscoveryURL != nil {
+			a.oidcCache.Invalidate(*provider.Issuer)
+		}
+	}
+
 	return sendJSON(w, http.StatusOK, provider)
 }
 
@@ -326,6 +369,7 @@ func (a *API) adminCustomOAuthProviderDelete(w http.ResponseWriter, r *http.Requ
 
 	observability.LogEntrySetField(r, "identifier", identifier)
 
+	var issuerToInvalidate string
 	err := db.Transaction(func(tx *storage.Connection) error {
 		provider, terr := models.FindCustomOAuthProviderByIdentifier(tx, identifier)
 		if terr != nil {
@@ -335,6 +379,10 @@ func (a *API) adminCustomOAuthProviderDelete(w http.ResponseWriter, r *http.Requ
 			return apierrors.NewInternalServerError("Error retrieving custom OAuth provider").WithInternalError(terr)
 		}
 
+		if provider.IsOIDC() && provider.Issuer != nil {
+			issuerToInvalidate = *provider.Issuer
+		}
+
 		// TODO: Add admin audit logging here (see create endpoint for details)
 
 		if terr := models.DeleteCustomOAuthProvider(tx, provider.ID); terr != nil {
@@ -348,6 +396,10 @@ func (a *API) adminCustomOAuthProviderDelete(w http.ResponseWriter, r *http.Requ
 		return err
 	}
 
+	if issuerToInvalidate != "" {
+		a.oidcCache.Invalidate(issuerToInvalidate)
+	}
+
 	w.WriteHeader(http.StatusNoContent)
 	return nil
 }
@@ -619,6 +671,82 @@ func validateAuthorizationParams(params map[string]interface{}) error {
 	return nil
 }
 
+// maxDiscoveryResponseSize is the maximum size of an OIDC discovery response body (1 MB).
+const maxDiscoveryResponseSize = 1 << 20
+
+// discoveryFetchTimeout is the timeout for fetching an OIDC discovery document.
+const discoveryFetchTimeout = 10 * time.Second
+
+// fetchAndValidateDiscovery fetches the OIDC discovery document from the
+// provider's discovery URL and validates that it contains the required fields
+// per the OpenID Connect Discovery 1.0 specification. It also verifies that
+// the issuer in the discovery document matches the expected issuer.
+func fetchAndValidateDiscovery(ctx context.Context, discoveryURL, expectedIssuer string) (*models.OIDCDiscovery, error) {
+	resp, err := utilities.FetchURLWithTimeout(ctx, discoveryURL, discoveryFetchTimeout)
+	if err != nil {
+		return nil, apierrors.NewBadRequestError(
+			apierrors.ErrorCodeValidationFailed,
+			"Failed to fetch OIDC discovery document from %q: %v", discoveryURL, err,
+		)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		return nil, apierrors.NewBadRequestError(
+			apierrors.ErrorCodeValidationFailed,
+			"OIDC discovery endpoint %q returned HTTP %d, expected 200", discoveryURL, resp.StatusCode,
+		)
+	}
+
+	body, err := io.ReadAll(io.LimitReader(resp.Body, maxDiscoveryResponseSize))
+	if err != nil {
+		return nil, apierrors.NewBadRequestError(
+			apierrors.ErrorCodeValidationFailed,
+			"Failed to read OIDC discovery response from %q", discoveryURL,
+		)
+	}
+
+	var discovery models.OIDCDiscovery
+	if err := json.Unmarshal(body, &discovery); err != nil {
+		return nil, apierrors.NewBadRequestError(
+			apierrors.ErrorCodeValidationFailed,
+			"OIDC discovery document from %q is not valid JSON", discoveryURL,
+		)
+	}
+
+	// Validate required fields per OpenID Connect Discovery 1.0 spec
+	var missing []string
+	if discovery.Issuer == "" {
+		missing = append(missing, "issuer")
+	}
+	if discovery.AuthorizationEndpoint == "" {
+		missing = append(missing, "authorization_endpoint")
+	}
+	if discovery.TokenEndpoint == "" {
+		missing = append(missing, "token_endpoint")
+	}
+	if discovery.JwksURI == "" {
+		missing = append(missing, "jwks_uri")
+	}
+	if len(missing) > 0 {
+		return nil, apierrors.NewBadRequestError(
+			apierrors.ErrorCodeValidationFailed,
+			"OIDC discovery document is missing required fields: %s", strings.Join(missing, ", "),
+		)
+	}
+
+	// The issuer in the discovery document MUST exactly match the expected issuer
+	// per OpenID Connect Discovery 1.0, Section 4.3.
+	if discovery.Issuer != expectedIssuer {
+		return nil, apierrors.NewBadRequestError(
+			apierrors.ErrorCodeValidationFailed,
+			"OIDC discovery issuer mismatch: discovery document reports %q but expected %q", discovery.Issuer, expectedIssuer,
+		)
+	}
+
+	return &discovery, nil
+}
+
 // validateAttributeMapping ensures no sensitive system fields are targeted
 func validateAttributeMapping(mapping map[string]interface{}) error {
 	if mapping == nil {

internal/api/custom_oauth_admin_test.go

@@ -11,6 +11,7 @@ import (
 
 	popslices "github.com/gobuffalo/pop/v6/slices"
 	jwt "github.com/golang-jwt/jwt/v5"
+	"github.com/gofrs/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
@@ -107,40 +108,68 @@ func (ts *CustomOAuthAdminTestSuite) TestCreateOAuth2Provider() {
 	assert.Empty(ts.T(), provider.ClientSecret)
 }
 
-func (ts *CustomOAuthAdminTestSuite) TestCreateOIDCProvider() {
-	payload := map[string]interface{}{
-		"provider_type": "oidc",
-		"identifier":    "custom:self-keycloak",
-		"name":          "Keycloak",
-		"client_id":     "test-client-id",
-		"client_secret": "test-client-secret",
-		"issuer":        "https://example.com/realms/myrealm",
-		"scopes":        []string{"profile", "email"},
-		"pkce_enabled":  true,
-		"enabled":       true,
-	}
+func (ts *CustomOAuthAdminTestSuite) TestCreateOIDCProviderValidatesDiscovery() {
+	ts.Run("Unreachable issuer rejected", func() {
+		// An OIDC provider with an unresolvable issuer is caught by URL validation
+		payload := map[string]interface{}{
+			"provider_type": "oidc",
+			"identifier":    "custom:bad-issuer",
+			"name":          "Bad Issuer",
+			"client_id":     "test-client-id",
+			"client_secret": "test-client-secret",
+			"issuer":        "https://unreachable.example.com/realms/myrealm",
+			"scopes":        []string{"profile", "email"},
+		}
 
-	var body bytes.Buffer
-	require.NoError(ts.T(), json.NewEncoder(&body).Encode(payload))
+		var body bytes.Buffer
+		require.NoError(ts.T(), json.NewEncoder(&body).Encode(payload))
 
-	req := httptest.NewRequest(http.MethodPost, "/admin/custom-providers", &body)
-	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", ts.token))
+		req := httptest.NewRequest(http.MethodPost, "/admin/custom-providers", &body)
+		req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", ts.token))
 
-	w := httptest.NewRecorder()
-	ts.API.handler.ServeHTTP(w, req)
+		w := httptest.NewRecorder()
+		ts.API.handler.ServeHTTP(w, req)
 
-	require.Equal(ts.T(), http.StatusCreated, w.Code)
+		require.Equal(ts.T(), http.StatusBadRequest, w.Code)
 
-	var provider models.CustomOAuthProvider
-	require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&provider))
+		var apiErr apierrors.HTTPError
+		require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&apiErr))
+		assert.Equal(ts.T(), apierrors.ErrorCodeValidationFailed, apiErr.ErrorCode)
+	})
 
-	assert.Equal(ts.T(), models.ProviderTypeOIDC, provider.ProviderType)
-	assert.Equal(ts.T(), "custom:self-keycloak", provider.Identifier)
-	assert.Contains(ts.T(), provider.Scopes, "openid") // Auto-added for OIDC
-	assert.Contains(ts.T(), provider.Scopes, "profile")
+	ts.Run("Invalid discovery document rejected", func() {
+		// Use a real resolvable domain whose discovery endpoint returns non-JSON.
+		// This passes URL validation but fails at discovery fetch/parse.
+		payload := map[string]interface{}{
+			"provider_type": "oidc",
+			"identifier":    "custom:bad-discovery",
+			"name":          "Bad Discovery",
+			"client_id":     "test-client-id",
+			"client_secret": "test-client-secret",
+			"issuer":        "https://example.com",
+			"scopes":        []string{"profile", "email"},
+		}
 
-	// Ensure client secret is not exposed in JSON
-	assert.Empty(ts.T(), provider.ClientSecret)
+		var body bytes.Buffer
+		require.NoError(ts.T(), json.NewEncoder(&body).Encode(payload))
+
+		req := httptest.NewRequest(http.MethodPost, "/admin/custom-providers", &body)
+		req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", ts.token))
+
+		w := httptest.NewRecorder()
+		ts.API.handler.ServeHTTP(w, req)
+
+		require.Equal(ts.T(), http.StatusBadRequest, w.Code)
+
+		var apiErr apierrors.HTTPError
+		require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&apiErr))
+		assert.Equal(ts.T(), apierrors.ErrorCodeValidationFailed, apiErr.ErrorCode)
+		// Should fail at discovery fetch or parse stage
+		assert.True(ts.T(),
+			strings.Contains(apiErr.Message, "OIDC discovery") ||
+				strings.Contains(apiErr.Message, "Failed to fetch"),
+			"Expected discovery-related error, got: %s", apiErr.Message)
+	})
 }
 
 func (ts *CustomOAuthAdminTestSuite) TestCreateProviderValidation() {
@@ -429,7 +458,7 @@ func (ts *CustomOAuthAdminTestSuite) TestListProviders() {
 	// Create some providers
 	ts.createProvider(ts.createTestOAuth2Payload("oauth2-1"), http.StatusCreated)
 	ts.createProvider(ts.createTestOAuth2Payload("oauth2-2"), http.StatusCreated)
-	ts.createProvider(ts.createTestOIDCPayload("oidc-1", "https://oidc1.example.com"), http.StatusCreated)
+	ts.createOIDCProviderInDB("oidc-1", "https://oidc1.example.com")
 
 	req := httptest.NewRequest(http.MethodGet, "/admin/custom-providers", nil)
 	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", ts.token))
@@ -463,8 +492,8 @@ func (ts *CustomOAuthAdminTestSuite) TestListProvidersEmptyReturnsArray() {
 func (ts *CustomOAuthAdminTestSuite) TestListProvidersWithTypeFilter() {
 	// Create mixed providers
 	ts.createProvider(ts.createTestOAuth2Payload("oauth2-1"), http.StatusCreated)
-	ts.createProvider(ts.createTestOIDCPayload("oidc-1", "https://oidc1.example.com"), http.StatusCreated)
-	ts.createProvider(ts.createTestOIDCPayload("oidc-2", "https://oidc2.example.com"), http.StatusCreated)
+	ts.createOIDCProviderInDB("oidc-1", "https://oidc1.example.com")
+	ts.createOIDCProviderInDB("oidc-2", "https://oidc2.example.com")
 
 	// Filter by OAuth2
 	req := httptest.NewRequest(http.MethodGet, "/admin/custom-providers?type=oauth2", nil)
@@ -617,25 +646,31 @@ func (ts *CustomOAuthAdminTestSuite) createTestOAuth2Payload(identifier string)
 	}
 }
 
-func (ts *CustomOAuthAdminTestSuite) createTestOIDCPayload(identifier, issuer string) map[string]interface{} {
+// createOIDCProviderInDB inserts an OIDC provider directly into the database,
+// bypassing the admin handler (and its discovery validation). Use this for tests
+// that need an OIDC provider to exist but aren't testing the create flow.
+func (ts *CustomOAuthAdminTestSuite) createOIDCProviderInDB(identifier, issuer string) *models.CustomOAuthProvider {
 	if !strings.HasPrefix(identifier, "custom:") {
 		identifier = "custom:" + identifier
 	}
-	// If issuer is not provided or uses non-resolvable domain, use example.com
-	if issuer == "" || strings.Contains(issuer, "oidc1.example.com") || strings.Contains(issuer, "oidc2.example.com") {
-		issuer = "https://example.com/realms/" + identifier
-	}
-	return map[string]interface{}{
-		"provider_type": "oidc",
-		"identifier":    identifier,
-		"name":          "Test OIDC Provider",
-		"client_id":     "test-client-id",
-		"client_secret": "test-client-secret",
-		"issuer":        issuer,
-		"scopes":        []string{"profile", "email"},
-		"pkce_enabled":  true,
-		"enabled":       true,
+	id, err := uuid.NewV4()
+	require.NoError(ts.T(), err)
+
+	provider := &models.CustomOAuthProvider{
+		ID:           id,
+		ProviderType: models.ProviderTypeOIDC,
+		Identifier:   identifier,
+		Name:         "Test OIDC Provider",
+		ClientID:     "test-client-id",
+		ClientSecret: "test-client-secret",
+		Issuer:       &issuer,
+		Scopes:       []string{"openid", "profile", "email"},
+		PKCEEnabled:  true,
+		Enabled:      true,
 	}
+
+	require.NoError(ts.T(), models.CreateCustomOAuthProvider(ts.API.db, provider))
+	return provider
 }
 
 func (ts *CustomOAuthAdminTestSuite) createProvider(payload map[string]interface{}, expectedStatus int) *httptest.ResponseRecorder {