feat: derive OAuth2 client type from token_endpoint_auth_method (#28043)

Adds an OAuth2 client type (public vs confidential, RFC 7591 §2) derived from the requested auth method instead of hardcoded confidential. The type is stored and guarded here, but no endpoint enforces on it yet; public behavior at the token endpoint follows in the next PR in the stack. - Client type is derived once and reused by both registration and redirect URI validation, so they can't disagree - IsPublic() fails closed: an unrecognized or missing value reads as confidential - RFC 7592 update (PUT) now rejects moving a client between public and confidential (400) instead of silently flipping it when the auth method is omitted - Discovery still doesn't advertise "none"; follows once the token endpoint honors it ### Behavior by client shape `client_type` is derived from `token_endpoint_auth_method` at POST and pinned at PUT. RFC 7592 GET/PUT authenticate with the registration access token, not the client secret, so neither endpoint reads a secret. | Registered with | Stored `client_type` / method | GET reports | PUT that flips the method | |------------------------------------|---------------------------------------|-----------------------|-----------------------------------------| | omitted, or `client_secret_basic` | `confidential` / `client_secret_basic` | `client_secret_basic` | `none` → 400 `invalid_client_metadata` | | `none` (new) | `public` / `none` | `none` | `client_secret_*` → 400 `invalid_client_metadata` | | `none` (before this PR) | `confidential` / `none` | `none` | either → 200, type stays `confidential` | - PUT still replaces every other RFC 7591 field. `client_type` is the only pinned one; the method may move within a type (`client_secret_basic` ↔ `client_secret_post`). - Row 3 is the only shape where the two columns disagree. The guard fires only on a method change that crosses the type line, so those clients keep managing themselves instead of being locked out of their own configuration endpoint. - The token endpoint does not consult `client_type` yet, so every client still authenticates with a secret and registration still issues one. Split out of #27873, second in the stack (on top of #28041). Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client
2026-09-24 15:04:27 +08:00 · 2026-08-18 21:12:44 -07:00
parent 4d13bef74d
commit 663f41ffa9
11 changed files with 526 additions and 27 deletions
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"slices"
 	"strings"
 	"time"

@@ -269,11 +270,44 @@ const (
 	OAuth2TokenEndpointAuthMethodNone              OAuth2TokenEndpointAuthMethod = "none"
 )

-func (m OAuth2TokenEndpointAuthMethod) Valid() bool {
-	switch m {
-	case OAuth2TokenEndpointAuthMethodClientSecretBasic,
+// AllOAuth2TokenEndpointAuthMethods returns every accepted token endpoint auth
+// method. Valid() is defined in terms of it, so what registration accepts
+// cannot drift from what this function reports.
+//
+// Discovery metadata does not yet derive from it:
+// coderd/oauth2provider/metadata.go's TokenEndpointAuthMethodsSupported is
+// hardcoded to {client_secret_basic, client_secret_post} and does not
+// advertise "none", even though "none" is accepted here. A follow-up PR
+// wires the token endpoint to honor "none"; only once that lands should
+// discovery advertise it too.
+func AllOAuth2TokenEndpointAuthMethods() []OAuth2TokenEndpointAuthMethod {
+	return []OAuth2TokenEndpointAuthMethod{
+		OAuth2TokenEndpointAuthMethodClientSecretBasic,
 		OAuth2TokenEndpointAuthMethodClientSecretPost,
-		OAuth2TokenEndpointAuthMethodNone:
+		OAuth2TokenEndpointAuthMethodNone,
+	}
+}
+
+func (m OAuth2TokenEndpointAuthMethod) Valid() bool {
+	return slices.Contains(AllOAuth2TokenEndpointAuthMethods(), m)
+}
+
+// OAuth2ClientType is how a client authenticates at the token endpoint
+// (RFC 7591 §2, OAuth 2.1 §2.1). A confidential client authenticates with a
+// secret; a public client authenticates with PKCE alone. It is derived from
+// the requested token_endpoint_auth_method and stored on the app. A
+// follow-up PR wires the token endpoint to read it when deciding whether to
+// require a client secret.
+type OAuth2ClientType string
+
+const (
+	OAuth2ClientTypeConfidential OAuth2ClientType = "confidential"
+	OAuth2ClientTypePublic       OAuth2ClientType = "public"
+)
+
+func (t OAuth2ClientType) Valid() bool {
+	switch t {
+	case OAuth2ClientTypeConfidential, OAuth2ClientTypePublic:
 		return true
 	}
 	return false
@@ -527,14 +561,19 @@ func (req OAuth2ClientRegistrationRequest) ApplyDefaults() OAuth2ClientRegistrat
 	return req
 }

-// DetermineClientType determines if client is public or confidential
-func (*OAuth2ClientRegistrationRequest) DetermineClientType() string {
-	// For now, default to confidential
-	// In the future, we might detect based on:
-	// - token_endpoint_auth_method == "none" -> public
-	// - application_type == "native" -> might be public
-	// - Other heuristics
-	return "confidential"
+// DetermineClientType determines if client is public or confidential, based
+// on the requested token_endpoint_auth_method (RFC 7591 §2, OAuth 2.1 §2.1).
+//
+// Only "none" reads as public; every other value, including an omitted one,
+// reads as confidential, so this is safe to call before ApplyDefaults(). A
+// caller that also compares the request's auth method against a stored one must
+// apply defaults first, or an omitted field compares as "" and looks like a
+// change the client did not request.
+func (req *OAuth2ClientRegistrationRequest) DetermineClientType() OAuth2ClientType {
+	if req.TokenEndpointAuthMethod == OAuth2TokenEndpointAuthMethodNone {
+		return OAuth2ClientTypePublic
+	}
+	return OAuth2ClientTypeConfidential
 }

 // GenerateClientName generates a client name if not provided
@@ -0,0 +1,88 @@
+package codersdk_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/v2/codersdk"
+)
+
+// TestOAuth2ClientRegistrationRequest_DetermineClientType verifies that the
+// client type is derived from the requested token_endpoint_auth_method
+// (RFC 7591 §2, OAuth 2.1 §2.1), not hardcoded to "confidential".
+func TestOAuth2ClientRegistrationRequest_DetermineClientType(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		authMethod codersdk.OAuth2TokenEndpointAuthMethod
+		// applyDefaults runs ApplyDefaults() before DetermineClientType(),
+		// matching the real request path where an omitted auth method is
+		// defaulted to "client_secret_basic" before this check ever runs.
+		applyDefaults bool
+		// wantAuthMethodAfterDefaults pins what ApplyDefaults() does to
+		// authMethod, so a case that runs applyDefaults also verifies
+		// ApplyDefaults left (or changed) the field as expected before
+		// DetermineClientType() reads it. Only checked when applyDefaults
+		// is true.
+		wantAuthMethodAfterDefaults codersdk.OAuth2TokenEndpointAuthMethod
+		expectedType                string
+	}{
+		{
+			name:         "NoneIsPublic",
+			authMethod:   codersdk.OAuth2TokenEndpointAuthMethodNone,
+			expectedType: "public",
+		},
+		{
+			name:         "ClientSecretBasicIsConfidential",
+			authMethod:   codersdk.OAuth2TokenEndpointAuthMethodClientSecretBasic,
+			expectedType: "confidential",
+		},
+		{
+			name:         "ClientSecretPostIsConfidential",
+			authMethod:   codersdk.OAuth2TokenEndpointAuthMethodClientSecretPost,
+			expectedType: "confidential",
+		},
+		{
+			// ApplyDefaults only fills an empty auth method; it must not
+			// touch an explicit "none". If it ever grew a rule that did,
+			// the pre-defaults Validate() call and the post-defaults
+			// storage call would disagree about this client's type.
+			name:                        "NoneStaysPublicAfterApplyDefaults",
+			authMethod:                  codersdk.OAuth2TokenEndpointAuthMethodNone,
+			applyDefaults:               true,
+			wantAuthMethodAfterDefaults: codersdk.OAuth2TokenEndpointAuthMethodNone,
+			expectedType:                "public",
+		},
+		{
+			// An omitted auth method must not be read as public. Without
+			// ApplyDefaults the empty string also falls through to
+			// confidential, so this is safe in either order, but the real
+			// path always defaults first.
+			name:                        "OmittedDefaultsToConfidentialAfterApplyDefaults",
+			applyDefaults:               true,
+			wantAuthMethodAfterDefaults: codersdk.OAuth2TokenEndpointAuthMethodClientSecretBasic,
+			expectedType:                "confidential",
+		},
+		{
+			name:         "OmittedIsConfidentialWithoutApplyDefaults",
+			expectedType: "confidential",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			req := codersdk.OAuth2ClientRegistrationRequest{
+				TokenEndpointAuthMethod: tt.authMethod,
+			}
+			if tt.applyDefaults {
+				req = req.ApplyDefaults()
+				require.Equal(t, tt.wantAuthMethodAfterDefaults, req.TokenEndpointAuthMethod)
+			}
+			require.Equal(t, tt.expectedType, string(req.DetermineClientType()))
+		})
+	}
+}
@@ -16,7 +16,9 @@ func (req *OAuth2ClientRegistrationRequest) Validate() error {
 		return xerrors.New("redirect_uris is required for authorization code flow")
 	}

-	if err := validateRedirectURIs(req.RedirectURIs, req.TokenEndpointAuthMethod); err != nil {
+	// The client type is derived once, by DetermineClientType, so which RFC 8252
+	// rules apply here cannot drift from what gets stored in client_type.
+	if err := validateRedirectURIs(req.RedirectURIs, req.DetermineClientType()); err != nil {
 		return xerrors.Errorf("invalid redirect_uris: %w", err)
 	}

@@ -118,8 +120,11 @@ func validateScheme(u *url.URL) error {
 	return nil
 }

-// validateRedirectURIs validates redirect URIs according to RFC 7591, 8252
-func validateRedirectURIs(uris []string, tokenEndpointAuthMethod OAuth2TokenEndpointAuthMethod) error {
+// validateRedirectURIs validates redirect URIs according to RFC 7591, 8252.
+// clientType selects which rules apply and is derived by DetermineClientType,
+// the single owner of that mapping, so this cannot disagree with the type the
+// app is stored as.
+func validateRedirectURIs(uris []string, clientType OAuth2ClientType) error {
 	if len(uris) == 0 {
 		return xerrors.New("at least one redirect URI is required")
 	}
@@ -144,8 +149,7 @@ func validateRedirectURIs(uris []string, tokenEndpointAuthMethod OAuth2TokenEndp
 			continue
 		}

-		// Determine if this is a public client based on token endpoint auth method
-		isPublicClient := tokenEndpointAuthMethod == OAuth2TokenEndpointAuthMethodNone
+		isPublicClient := clientType == OAuth2ClientTypePublic

 		// Handle different validation for public vs confidential clients
 		if uri.Scheme == "http" || uri.Scheme == "https" {