Review OIDC - Authorization Code grant

oidc/core/authorization_code/flow.go

@@ -42,32 +42,29 @@ func Must(cfg *Config) (*Flow, error) {
 }
 
 // ValidateAuthorizationRequest validates OIDC-specific parameters in the
-// authorization request. It is a no-op when the openid scope is absent.
+// /authorize request. It is a no-op when the openid scope is absent.
 func (f *Flow) ValidateAuthorizationRequest(r *requests.AuthorizationRequest) error {
 	if isOIDCReq := r.Scopes.ContainOpenID(); !isOIDCReq {
 		return nil
 	}
 
-	if err := r.ValidateDisplay(false); err != nil {
-		return err
-	}
-
 	if err := f.validateNonce(r); err != nil {
 		return err
 	}
 
-	if err := f.validatePrompt(r); err != nil {
-		return err
-	}
-
 	return nil
 }
 
-// ValidateConsentRequest re-runs authorization request validation then enforces
-// prompt and user-presence rules. When prompt is absent and user is nil, it
-// defaults to prompt=login so the handler can redirect to the login page.
+// ValidateConsentRequest validates OIDC prompt rules at the consent step.
+// It is a no-op when the openid scope is absent. When prompt is absent and
+// user is nil, defaults to prompt=login so the handler can redirect to the
+// login page.
 func (f *Flow) ValidateConsentRequest(r *requests.AuthorizationRequest) error {
-	if err := f.ValidateAuthorizationRequest(r); err != nil {
+	if isOIDCReq := r.Scopes.ContainOpenID(); !isOIDCReq {
+		return nil
+	}
+
+	if err := f.validatePrompt(r); err != nil {
 		return err
 	}
 
@@ -129,7 +126,7 @@ func (f *Flow) ProcessToken(r *requests.TokenRequest, _ models.Token, data map[s
 // validateNonce checks that nonce is present (when required) and has not been
 // used before (when ExistNonce is configured).
 func (f *Flow) validateNonce(r *requests.AuthorizationRequest) error {
-	if err := r.ValidateNonce(f.requireNonce); err != nil {
+	if err := r.CheckNonce(f.requireNonce); err != nil {
 		return err
 	}
 

oidc/core/authorization_code/flow_test.go

@@ -131,31 +131,6 @@ func TestFlow_ValidateAuthorizationRequest(t *testing.T) {
 		assert.NoError(t, f2.ValidateAuthorizationRequest(r))
 	})
 
-	t.Run("prompt_none_combined_with_other_returns_error", func(t *testing.T) {
-		r := authReq("openid")
-		r.Nonce = "nonce-1"
-		r.Prompts = types.NewPrompts([]string{"none", "login"})
-		err := f.ValidateAuthorizationRequest(r)
-		require.Error(t, err)
-		assert.Contains(t, err.Error(), "none")
-	})
-
-	t.Run("prompt_none_alone_ok", func(t *testing.T) {
-		f2 := New(validConfig().SetRequireNonce(false))
-		r := authReq("openid")
-		r.Prompts = types.NewPrompts([]string{"none"})
-		assert.NoError(t, f2.ValidateAuthorizationRequest(r))
-	})
-
-	t.Run("prompt_login_resets_max_age_to_zero", func(t *testing.T) {
-		r := authReq("openid")
-		r.Nonce = "nonce-1"
-		r.Prompts = types.NewPrompts([]string{"login"})
-		require.NoError(t, f.ValidateAuthorizationRequest(r))
-		require.NotNil(t, r.MaxAge)
-		assert.Equal(t, uint(0), *r.MaxAge)
-	})
-
 	t.Run("valid_request_with_nonce", func(t *testing.T) {
 		r := authReq("openid", "profile")
 		r.Nonce = "nonce-1"
@@ -225,13 +200,23 @@ func TestFlow_ValidateConsentRequest(t *testing.T) {
 		assert.NoError(t, f.ValidateConsentRequest(r))
 	})
 
-	t.Run("validation_error_from_auth_request_propagates", func(t *testing.T) {
-		// requireNonce=true (default): missing nonce must bubble up.
-		f := New(validConfig())
+	t.Run("prompt_none_combined_with_other_returns_error", func(t *testing.T) {
+		f := New(cfg)
 		r := authReq("openid")
+		r.Prompts = types.NewPrompts([]string{"none", "login"})
 		err := f.ValidateConsentRequest(r)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "nonce")
+		assert.Contains(t, err.Error(), "none")
+	})
+
+	t.Run("prompt_login_resets_max_age_to_zero", func(t *testing.T) {
+		f := New(cfg)
+		r := authReq("openid")
+		r.User = &sql.User{UserID: "user-1"}
+		r.Prompts = types.NewPrompts([]string{"login"})
+		require.NoError(t, f.ValidateConsentRequest(r))
+		require.NotNil(t, r.MaxAge)
+		assert.Equal(t, uint(0), *r.MaxAge)
 	})
 }
 

requests/authorization.go

@@ -41,10 +41,12 @@ type AuthorizationRequest struct {
 	Request *http.Request
 }
 
-// NewAuthorizationRequestFromHttp parses an authorization request from an
-// HTTP request. It reads all standard OAuth 2.0 and OIDC parameters from the
-// URL query string. Returns an error only if max_age is present but cannot be
-// parsed as a non-negative integer.
+// NewAuthorizationRequestFromHttp parses an OAuth 2.0 / OIDC authorization
+// request from r. Parameters are read from the URL query string (or form
+// body after r.ParseForm). Invalid or missing parameters are not validated
+// here — that is left to the grant flow. The only exception is max_age: if
+// present but not a valid non-negative integer, it is silently ignored and
+// defaults to 0.
 func NewAuthorizationRequestFromHttp(r *http.Request) (*AuthorizationRequest, error) {
 	authReq := &AuthorizationRequest{
 		ResponseType:        types.NewResponseType(r.FormValue("response_type")),
@@ -65,33 +67,30 @@ func NewAuthorizationRequestFromHttp(r *http.Request) (*AuthorizationRequest, er
 		Request:             r,
 	}
 
-	if maxAge := r.FormValue("max_age"); maxAge != "" {
-		ma, err := strconv.ParseUint(maxAge, 10, 64)
-		if err != nil {
-			return nil, autherrors.InvalidRequestError().
-				WithDescription("\"max_age\" must be a non-negative integer").
-				WithState(authReq.State)
+	maxAge := types.NewMaxAge(0)
+	if v := r.FormValue("max_age"); v != "" {
+		if ma, err := strconv.ParseUint(v, 10, 64); err == nil {
+			maxAge = types.NewMaxAge(uint(ma))
 		}
-
-		authReq.MaxAge = types.NewMaxAge(uint(ma))
 	}
+	authReq.MaxAge = maxAge
 
 	return authReq, nil
 }
 
-// ValidateResponseType returns an error if response_type is missing. Required
+// CheckResponseType returns an error if response_type is missing. Required
 // by default; pass false to treat it as optional.
-func (r *AuthorizationRequest) ValidateResponseType(required ...bool) error {
+func (r *AuthorizationRequest) CheckResponseType(required ...bool) error {
 	if isRequired(true, required...) && r.ResponseType.IsEmpty() {
 		return autherrors.InvalidRequestError().WithDescription("missing \"response_type\" in request").WithState(r.State)
 	}
 
 	return nil
 }
 
-// ValidateClientID returns an error if client_id is missing. Required by
+// CheckClientID returns an error if client_id is missing. Required by
 // default; pass false to treat it as optional.
-func (r *AuthorizationRequest) ValidateClientID(required ...bool) error {
+func (r *AuthorizationRequest) CheckClientID(required ...bool) error {
 	if isRequired(true, required...) && r.ClientID == "" {
 		return autherrors.InvalidRequestError().
 			WithDescription("missing \"client_id\" in request").
@@ -101,21 +100,9 @@ func (r *AuthorizationRequest) ValidateClientID(required ...bool) error {
 	return nil
 }
 
-// ValidateRedirectURI returns an error if redirect_uri is missing. Required
-// by default; pass false to treat it as optional.
-func (r *AuthorizationRequest) ValidateRedirectURI(required ...bool) error {
-	if isRequired(true, required...) && r.RedirectURI == "" {
-		return autherrors.InvalidRequestError().
-			WithDescription("missing \"redirect_uri\" in request").
-			WithState(r.State)
-	}
-
-	return nil
-}
-
-// ValidateNonce returns an error if nonce is missing. Required by default;
+// CheckNonce returns an error if nonce is missing. Required by default;
 // pass false to treat it as optional.
-func (r *AuthorizationRequest) ValidateNonce(required ...bool) error {
+func (r *AuthorizationRequest) CheckNonce(required ...bool) error {
 	if isRequired(true, required...) && r.Nonce == "" {
 		return autherrors.InvalidRequestError().
 			WithDescription("missing \"nonce\" in request").
@@ -126,45 +113,6 @@ func (r *AuthorizationRequest) ValidateNonce(required ...bool) error {
 	return nil
 }
 
-// ValidateResponseMode returns an error if response_mode is missing. Not
-// required by default; pass true to enforce it.
-func (r *AuthorizationRequest) ValidateResponseMode(required ...bool) error {
-	if isRequired(false, required...) && r.ResponseMode.IsEmpty() {
-		return autherrors.InvalidRequestError().
-			WithDescription("missing \"response_mode\" in request").
-			WithState(r.State).
-			WithRedirectURI(r.RedirectURI)
-	}
-
-	return nil
-}
-
-// ValidateDisplay returns an error if display is missing. Not required by
-// default; pass true to enforce it.
-func (r *AuthorizationRequest) ValidateDisplay(required ...bool) error {
-	if isRequired(false, required...) && r.Display.IsEmpty() {
-		return autherrors.InvalidRequestError().
-			WithDescription("missing \"display\" in request").
-			WithState(r.State).
-			WithRedirectURI(r.RedirectURI)
-	}
-
-	return nil
-}
-
-// ValidatePrompts returns an error if prompt is missing. Not required by
-// default; pass true to enforce it.
-func (r *AuthorizationRequest) ValidatePrompts(required ...bool) error {
-	if isRequired(false, required...) && len(r.Prompts) == 0 {
-		return autherrors.InvalidRequestError().
-			WithDescription("missing \"prompt\" in request").
-			WithState(r.State).
-			WithRedirectURI(r.RedirectURI)
-	}
-
-	return nil
-}
-
 // Method returns the HTTP method of the underlying request.
 func (r *AuthorizationRequest) Method() string {
 	return r.Request.Method

requests/authorization_test.go

@@ -23,100 +23,51 @@ func TestNewAuthorizationRequestFromHttp(t *testing.T) {
 		assert.Equal(t, types.NewMaxAge(300), req.MaxAge)
 	})
 
-	t.Run("invalid max_age returns error", func(t *testing.T) {
+	t.Run("invalid_max_age_ignored", func(t *testing.T) {
 		r := httptest.NewRequest("GET", "/?max_age=abc", nil)
 		req, err := NewAuthorizationRequestFromHttp(r)
-		authErr := autherrors.ToAuthLibError(err)
-		assert.Equal(t, autherrors.ErrInvalidRequest, authErr.Code)
-		assert.Nil(t, req)
+		assert.NoError(t, err)
+		assert.NotNil(t, req)
 	})
 }
 
-func TestAuthorizationRequest_ValidateResponseType(t *testing.T) {
+func TestAuthorizationRequest_CheckResponseType(t *testing.T) {
 	req := &AuthorizationRequest{}
-	err := req.ValidateResponseType()
+	err := req.CheckResponseType()
 	authErr := autherrors.ToAuthLibError(err)
 	assert.Equal(t, autherrors.ErrInvalidRequest, authErr.Code)
 
 	req.ResponseType = types.ResponseTypeCode
-	assert.NoError(t, req.ValidateResponseType())
+	assert.NoError(t, req.CheckResponseType())
 
 	// optional when false is passed
 	empty := &AuthorizationRequest{}
-	assert.NoError(t, empty.ValidateResponseType(false))
+	assert.NoError(t, empty.CheckResponseType(false))
 }
 
-func TestAuthorizationRequest_ValidateClientID(t *testing.T) {
+func TestAuthorizationRequest_CheckClientID(t *testing.T) {
 	req := &AuthorizationRequest{}
-	err := req.ValidateClientID()
+	err := req.CheckClientID()
 	authErr := autherrors.ToAuthLibError(err)
 	assert.Equal(t, autherrors.ErrInvalidRequest, authErr.Code)
 
-	assert.NoError(t, req.ValidateClientID(false))
+	assert.NoError(t, req.CheckClientID(false))
 
 	req.ClientID = "myclient"
-	assert.NoError(t, req.ValidateClientID())
-}
-
-func TestAuthorizationRequest_ValidateRedirectURI(t *testing.T) {
-	req := &AuthorizationRequest{}
-	err := req.ValidateRedirectURI()
-	authErr := autherrors.ToAuthLibError(err)
-	assert.Equal(t, autherrors.ErrInvalidRequest, authErr.Code)
-
-	assert.NoError(t, req.ValidateRedirectURI(false))
-
-	req.RedirectURI = "https://example.com/cb"
-	assert.NoError(t, req.ValidateRedirectURI())
+	assert.NoError(t, req.CheckClientID())
 }
 
-func TestAuthorizationRequest_ValidateNonce(t *testing.T) {
+func TestAuthorizationRequest_CheckNonce(t *testing.T) {
 	req := &AuthorizationRequest{}
 
 	// required by default
-	err := req.ValidateNonce()
+	err := req.CheckNonce()
 	authErr := autherrors.ToAuthLibError(err)
 	assert.Equal(t, autherrors.ErrInvalidRequest, authErr.Code)
 
 	// optional when false is passed
-	assert.NoError(t, req.ValidateNonce(false))
+	assert.NoError(t, req.CheckNonce(false))
 
 	req.Nonce = "mynonce"
-	assert.NoError(t, req.ValidateNonce())
-}
-
-func TestAuthorizationRequest_ValidateResponseMode(t *testing.T) {
-	req := &AuthorizationRequest{}
-	assert.NoError(t, req.ValidateResponseMode())
-
-	err := req.ValidateResponseMode(true)
-	authErr := autherrors.ToAuthLibError(err)
-	assert.Equal(t, autherrors.ErrInvalidRequest, authErr.Code)
-
-	req.ResponseMode = types.NewResponseMode("query")
-	assert.NoError(t, req.ValidateResponseMode(true))
-}
-
-func TestAuthorizationRequest_ValidateDisplay(t *testing.T) {
-	req := &AuthorizationRequest{}
-	assert.NoError(t, req.ValidateDisplay())
-
-	err := req.ValidateDisplay(true)
-	authErr := autherrors.ToAuthLibError(err)
-	assert.Equal(t, autherrors.ErrInvalidRequest, authErr.Code)
-
-	req.Display = types.DisplayPage
-	assert.NoError(t, req.ValidateDisplay(true))
-}
-
-func TestAuthorizationRequest_ValidatePrompts(t *testing.T) {
-	req := &AuthorizationRequest{}
-	assert.NoError(t, req.ValidatePrompts())
-
-	err := req.ValidatePrompts(true)
-	authErr := autherrors.ToAuthLibError(err)
-	assert.Equal(t, autherrors.ErrInvalidRequest, authErr.Code)
-
-	req.Prompts = types.NewPrompts([]string{"login"})
-	assert.NoError(t, req.ValidatePrompts(true))
+	assert.NoError(t, req.CheckNonce())
 }