Skip to content

Make the server router method-aware to drop in-handler method guards #4907

Description

@nabokihms

Problem Description

The server package is being decomposed into per-domain handler packages (discovery, userinfo, introspection, device) that mount their own routes through the server/router Mux abstraction.

That abstraction registers a path and a handler but does not match the HTTP method. As a result every handler repeats a method guard with an ad-hoc error response, e.g.:

func (h *Handler) handleDeviceCode(w http.ResponseWriter, r *http.Request) {
	if r.Method != http.MethodPost {
		h.renderError(r, w, http.StatusBadRequest, "Invalid device code request type")
		h.writeError(w, oauth2.InvalidRequest, "", http.StatusBadRequest)
		return
	}
	...
}

These guards are noise, they differ per handler ("Requested resource does not exist." vs "Invalid device code request type" vs "Method not allowed."), and they diverge from the idiomatic gorilla/mux approach where the router matches the method and returns 405 Method Not Allowed.

Proposed Solution

Make router.Mux method-aware and drop the in-handler guards. Registration takes the allowed methods (variadic, empty means any, so existing calls keep working):

type Mux interface {
	HandleFunc(pattern string, h http.HandlerFunc, methods ...string)
	HandleCORS(pattern string, h http.HandlerFunc, methods ...string)
	// ...
}

func (h *Handler) Mount(m router.Mux) {
	m.HandleFunc("/device", h.handleDeviceExchange, http.MethodGet)
	m.HandleFunc("/device/code", h.handleDeviceCode, http.MethodPost)
	// ...
}

The underlying gorilla router adds .Methods(...), a shared MethodNotAllowedHandler renders a consistent 405, and the switch r.Method / if r.Method != ... guards are removed from every handler.

Scope: applied uniformly across all extracted handlers, with a per-endpoint method review (e.g. /userinfo accepts GET and POST per OIDC, /keys and /.well-known are GET, /token and /token/introspect are POST).

Alternatives Considered

  • Keep the per-handler method guards (status quo): works, but is repetitive and inconsistent.
  • Add method-aware registration only for some handlers: leaves the routing behaviour inconsistent across endpoints.

Additional Information

This is a behaviour change: a wrong method currently returns a custom 400 from the handler; after the change it returns a uniform 405, so the per-route messages collapse and a few wrong-method tests need updating (for example the TestHandleDeviceCode "GET" case). It is cross-cutting (the router.Mux abstraction, the server's routeMux adapter, and every handler's Mount), so it is best done as its own PR rather than folded into a behaviour-preserving extraction.

Came up while extracting the device flow in #4906.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions