From 6c0664defc54742e8ea5572bbf90efe73d3c9af7 Mon Sep 17 00:00:00 2001 From: Quint Daenen Date: Tue, 24 Mar 2026 16:37:46 +0100 Subject: [PATCH 1/2] feat: ensure all handlers explicitly set HTTP status codes Wrap ResponseWriter in ServeHTTP with a statusResponseWriter that guarantees WriteHeader is always called explicitly. This allows observability middleware to reliably capture status codes, which was not possible when handlers relied on net/http's implicit 200 on first Write call. The wrapper also guards against duplicate WriteHeader calls by ignoring subsequent invocations after the first. Closes #183 Co-Authored-By: iwarapter --- handlers_test.go | 95 ++++++++++++++++++++++++++++++++++++++++++++++++ server.go | 24 ++++++++++++ 2 files changed, 119 insertions(+) diff --git a/handlers_test.go b/handlers_test.go index 4f002d2..6e88656 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -992,6 +992,101 @@ func newTestResourceHandler() ResourceHandler { } } +// statusRecordingResponseWriter wraps an http.ResponseWriter and records +// whether WriteHeader was called explicitly, simulating logging middleware. +type statusRecordingResponseWriter struct { + http.ResponseWriter + calledWriteHeader bool + status int +} + +func (w *statusRecordingResponseWriter) WriteHeader(status int) { + w.calledWriteHeader = true + w.status = status + w.ResponseWriter.WriteHeader(status) +} + +func TestServerExplicitStatusCodes(t *testing.T) { + tests := []struct { + name string + method string + target string + body io.Reader + expectedStatus int + }{ + { + name: "GET resource", + method: http.MethodGet, + target: "/Users/0001", + expectedStatus: http.StatusOK, + }, + { + name: "GET resources", + method: http.MethodGet, + target: "/Users", + expectedStatus: http.StatusOK, + }, + { + name: "POST resource", + method: http.MethodPost, + target: "/Users", + body: strings.NewReader(`{"userName": "test", "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"]}`), + expectedStatus: http.StatusCreated, + }, + { + name: "PUT resource", + method: http.MethodPut, + target: "/Users/0001", + body: strings.NewReader(`{"userName": "test_replace", "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"]}`), + expectedStatus: http.StatusOK, + }, + { + name: "DELETE resource", + method: http.MethodDelete, + target: "/Users/0001", + expectedStatus: http.StatusNoContent, + }, + { + name: "GET ResourceTypes", + method: http.MethodGet, + target: "/ResourceTypes", + expectedStatus: http.StatusOK, + }, + { + name: "GET ResourceType", + method: http.MethodGet, + target: "/ResourceTypes/User", + expectedStatus: http.StatusOK, + }, + { + name: "GET Schemas", + method: http.MethodGet, + target: "/Schemas", + expectedStatus: http.StatusOK, + }, + { + name: "GET ServiceProviderConfig", + method: http.MethodGet, + target: "/ServiceProviderConfig", + expectedStatus: http.StatusOK, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(tt.method, tt.target, tt.body) + rr := httptest.NewRecorder() + w := &statusRecordingResponseWriter{ResponseWriter: rr} + newTestServer(t).ServeHTTP(w, req) + + if !w.calledWriteHeader { + t.Error("handler did not explicitly call WriteHeader") + } + assertEqualStatusCode(t, tt.expectedStatus, w.status) + }) + } +} + func newTestServer(t *testing.T) Server { userSchema := getUserSchema() userSchemaExtension := getUserExtensionSchema() diff --git a/server.go b/server.go index c701bcc..4419305 100644 --- a/server.go +++ b/server.go @@ -86,9 +86,33 @@ func NewServer(args *ServerArgs, opts ...ServerOption) (Server, error) { return *s, nil } +// statusResponseWriter wraps http.ResponseWriter to ensure WriteHeader is +// always called explicitly. If Write is called without a prior WriteHeader, +// it defaults to http.StatusOK. Subsequent WriteHeader calls are ignored. +// This allows observability middleware to reliably capture status codes. +type statusResponseWriter struct { + http.ResponseWriter + wroteHeader bool +} + +func (w *statusResponseWriter) WriteHeader(status int) { + if !w.wroteHeader { + w.wroteHeader = true + w.ResponseWriter.WriteHeader(status) + } +} + +func (w *statusResponseWriter) Write(b []byte) (int, error) { + if !w.wroteHeader { + w.WriteHeader(http.StatusOK) + } + return w.ResponseWriter.Write(b) +} + // ServeHTTP dispatches the request to the handler whose pattern most closely matches the request URL. func (s Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/scim+json") + w = &statusResponseWriter{ResponseWriter: w} path := strings.TrimPrefix(r.URL.Path, "/v2") From 499f16bd38471104ca2988c16b86b5a2dc1ae9c5 Mon Sep 17 00:00:00 2001 From: Quint Daenen Date: Tue, 24 Mar 2026 16:49:04 +0100 Subject: [PATCH 2/2] chore: build goarrange via nix and fix formatting --- flake.nix | 25 ++++-- handlers_test.go | 190 +++++++++++++++++++++++----------------------- schema/schemas.go | 22 +++--- server.go | 46 +++++------ 4 files changed, 147 insertions(+), 136 deletions(-) diff --git a/flake.nix b/flake.nix index edf75b5..fcad099 100644 --- a/flake.nix +++ b/flake.nix @@ -12,15 +12,25 @@ devShells = forAllSystems (system: let pkgs = nixpkgs.legacyPackages.${system}; - ci = pkgs.writeShellScriptBin "ci" '' + goarrange = pkgs.buildGoModule { + pname = "goarrange"; + version = "1.0.0"; + src = pkgs.fetchFromGitHub { + owner = "jdeflander"; + repo = "goarrange"; + rev = "v1.0.0"; + hash = "sha256-V03BgTeWcAspMHGUHlAgSbiTaoZ42hgb/Zb/yqZ2m+k="; + }; + vendorHash = "sha256-Xhxfiw1WeXFHrYIYvUytEtMzMbSxOrignmUC5kVna0o="; + }; + lint = pkgs.writeShellScriptBin "lint" '' set -euo pipefail - echo "--- test ---" - go test -v ./... + echo "--- format ---" + go fmt ./... + goarrange run -r + git diff --quiet echo "--- lint ---" golangci-lint run -E misspell,godot,whitespace ./... - echo "--- arrange ---" - command -v goarrange >/dev/null || go install github.com/jdeflander/goarrange@v1.0.0 - test -z "$(goarrange run -r -d)" echo "--- tidy ---" go mod tidy git diff --quiet go.mod go.sum @@ -31,7 +41,8 @@ packages = [ pkgs.go_1_26 pkgs.golangci-lint - ci + goarrange + lint ]; }; } diff --git a/handlers_test.go b/handlers_test.go index 6e88656..ac34f13 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -91,6 +91,87 @@ func TestInvalidRequests(t *testing.T) { } } +func TestServerExplicitStatusCodes(t *testing.T) { + tests := []struct { + name string + method string + target string + body io.Reader + expectedStatus int + }{ + { + name: "GET resource", + method: http.MethodGet, + target: "/Users/0001", + expectedStatus: http.StatusOK, + }, + { + name: "GET resources", + method: http.MethodGet, + target: "/Users", + expectedStatus: http.StatusOK, + }, + { + name: "POST resource", + method: http.MethodPost, + target: "/Users", + body: strings.NewReader(`{"userName": "test", "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"]}`), + expectedStatus: http.StatusCreated, + }, + { + name: "PUT resource", + method: http.MethodPut, + target: "/Users/0001", + body: strings.NewReader(`{"userName": "test_replace", "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"]}`), + expectedStatus: http.StatusOK, + }, + { + name: "DELETE resource", + method: http.MethodDelete, + target: "/Users/0001", + expectedStatus: http.StatusNoContent, + }, + { + name: "GET ResourceTypes", + method: http.MethodGet, + target: "/ResourceTypes", + expectedStatus: http.StatusOK, + }, + { + name: "GET ResourceType", + method: http.MethodGet, + target: "/ResourceTypes/User", + expectedStatus: http.StatusOK, + }, + { + name: "GET Schemas", + method: http.MethodGet, + target: "/Schemas", + expectedStatus: http.StatusOK, + }, + { + name: "GET ServiceProviderConfig", + method: http.MethodGet, + target: "/ServiceProviderConfig", + expectedStatus: http.StatusOK, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(tt.method, tt.target, tt.body) + rr := httptest.NewRecorder() + w := &statusRecordingResponseWriter{ResponseWriter: rr} + newTestServer(t).ServeHTTP(w, req) + + if !w.calledWriteHeader { + t.Error("handler did not explicitly call WriteHeader") + } + assertEqualStatusCode(t, tt.expectedStatus, w.status) + }) + } +} + func TestServerMeEndpoint(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/Me", nil) rr := httptest.NewRecorder() @@ -992,101 +1073,6 @@ func newTestResourceHandler() ResourceHandler { } } -// statusRecordingResponseWriter wraps an http.ResponseWriter and records -// whether WriteHeader was called explicitly, simulating logging middleware. -type statusRecordingResponseWriter struct { - http.ResponseWriter - calledWriteHeader bool - status int -} - -func (w *statusRecordingResponseWriter) WriteHeader(status int) { - w.calledWriteHeader = true - w.status = status - w.ResponseWriter.WriteHeader(status) -} - -func TestServerExplicitStatusCodes(t *testing.T) { - tests := []struct { - name string - method string - target string - body io.Reader - expectedStatus int - }{ - { - name: "GET resource", - method: http.MethodGet, - target: "/Users/0001", - expectedStatus: http.StatusOK, - }, - { - name: "GET resources", - method: http.MethodGet, - target: "/Users", - expectedStatus: http.StatusOK, - }, - { - name: "POST resource", - method: http.MethodPost, - target: "/Users", - body: strings.NewReader(`{"userName": "test", "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"]}`), - expectedStatus: http.StatusCreated, - }, - { - name: "PUT resource", - method: http.MethodPut, - target: "/Users/0001", - body: strings.NewReader(`{"userName": "test_replace", "schemas": ["urn:ietf:params:scim:schemas:core:2.0:User"]}`), - expectedStatus: http.StatusOK, - }, - { - name: "DELETE resource", - method: http.MethodDelete, - target: "/Users/0001", - expectedStatus: http.StatusNoContent, - }, - { - name: "GET ResourceTypes", - method: http.MethodGet, - target: "/ResourceTypes", - expectedStatus: http.StatusOK, - }, - { - name: "GET ResourceType", - method: http.MethodGet, - target: "/ResourceTypes/User", - expectedStatus: http.StatusOK, - }, - { - name: "GET Schemas", - method: http.MethodGet, - target: "/Schemas", - expectedStatus: http.StatusOK, - }, - { - name: "GET ServiceProviderConfig", - method: http.MethodGet, - target: "/ServiceProviderConfig", - expectedStatus: http.StatusOK, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - req := httptest.NewRequest(tt.method, tt.target, tt.body) - rr := httptest.NewRecorder() - w := &statusRecordingResponseWriter{ResponseWriter: rr} - newTestServer(t).ServeHTTP(w, req) - - if !w.calledWriteHeader { - t.Error("handler did not explicitly call WriteHeader") - } - assertEqualStatusCode(t, tt.expectedStatus, w.status) - }) - } -} - func newTestServer(t *testing.T) Server { userSchema := getUserSchema() userSchemaExtension := getUserExtensionSchema() @@ -1129,3 +1115,17 @@ func newTestServer(t *testing.T) Server { } return s } + +// statusRecordingResponseWriter wraps an http.ResponseWriter and records +// whether WriteHeader was called explicitly, simulating logging middleware. +type statusRecordingResponseWriter struct { + http.ResponseWriter + calledWriteHeader bool + status int +} + +func (w *statusRecordingResponseWriter) WriteHeader(status int) { + w.calledWriteHeader = true + w.status = status + w.ResponseWriter.WriteHeader(status) +} diff --git a/schema/schemas.go b/schema/schemas.go index a966f3f..17c40c4 100644 --- a/schema/schemas.go +++ b/schema/schemas.go @@ -697,9 +697,9 @@ func ServiceProviderConfigSchema() Schema { Attributes: []CoreAttribute{ SimpleCoreAttribute(SimpleStringParams(StringParams{ Description: optional.NewString("An HTTP-addressable URL pointing to the service provider's human-consumable help documentation."), - Mutability: AttributeMutabilityReadOnly(), - Name: "documentationUri", - Required: false, + Mutability: AttributeMutabilityReadOnly(), + Name: "documentationUri", + Required: false, })), ComplexCoreAttribute(ComplexParams{ Description: optional.NewString("A complex type that specifies PATCH configuration options."), @@ -763,9 +763,9 @@ func ServiceProviderConfigSchema() Schema { }), ComplexCoreAttribute(ComplexParams{ Description: optional.NewString("A complex type that specifies configuration options related to changing a password."), - Mutability: AttributeMutabilityReadOnly(), - Name: "changePassword", - Required: true, + Mutability: AttributeMutabilityReadOnly(), + Name: "changePassword", + Required: true, SubAttributes: []SimpleParams{ SimpleBooleanParams(BooleanParams{ Description: optional.NewString("A Boolean value specifying whether or not the operation is supported."), @@ -812,9 +812,9 @@ func ServiceProviderConfigSchema() Schema { SubAttributes: []SimpleParams{ SimpleStringParams(StringParams{ Description: optional.NewString("The authentication scheme. This specification defines the values 'oauth', 'oauth2', 'oauthbearertoken', 'httpbasic', and 'httpdigest'."), - Mutability: AttributeMutabilityReadOnly(), - Name: "type", - Required: true, + Mutability: AttributeMutabilityReadOnly(), + Name: "type", + Required: true, }), SimpleStringParams(StringParams{ Description: optional.NewString("The common authentication scheme name, e.g., HTTP Basic."), @@ -830,8 +830,8 @@ func ServiceProviderConfigSchema() Schema { }), SimpleStringParams(StringParams{ Description: optional.NewString("An HTTP-addressable URL pointing to the authentication scheme's specification."), - Mutability: AttributeMutabilityReadOnly(), - Name: "specUri", + Mutability: AttributeMutabilityReadOnly(), + Name: "specUri", }), SimpleStringParams(StringParams{ Description: optional.NewString("An HTTP-addressable URL pointing to the authentication scheme's usage documentation."), diff --git a/server.go b/server.go index 4419305..a13f5b0 100644 --- a/server.go +++ b/server.go @@ -86,29 +86,6 @@ func NewServer(args *ServerArgs, opts ...ServerOption) (Server, error) { return *s, nil } -// statusResponseWriter wraps http.ResponseWriter to ensure WriteHeader is -// always called explicitly. If Write is called without a prior WriteHeader, -// it defaults to http.StatusOK. Subsequent WriteHeader calls are ignored. -// This allows observability middleware to reliably capture status codes. -type statusResponseWriter struct { - http.ResponseWriter - wroteHeader bool -} - -func (w *statusResponseWriter) WriteHeader(status int) { - if !w.wroteHeader { - w.wroteHeader = true - w.ResponseWriter.WriteHeader(status) - } -} - -func (w *statusResponseWriter) Write(b []byte) (int, error) { - if !w.wroteHeader { - w.WriteHeader(http.StatusOK) - } - return w.ResponseWriter.Write(b) -} - // ServeHTTP dispatches the request to the handler whose pattern most closely matches the request URL. func (s Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/scim+json") @@ -281,3 +258,26 @@ func WithLogger(logger Logger) ServerOption { } } } + +// statusResponseWriter wraps http.ResponseWriter to ensure WriteHeader is +// always called explicitly. If Write is called without a prior WriteHeader, +// it defaults to http.StatusOK. Subsequent WriteHeader calls are ignored. +// This allows observability middleware to reliably capture status codes. +type statusResponseWriter struct { + http.ResponseWriter + wroteHeader bool +} + +func (w *statusResponseWriter) Write(b []byte) (int, error) { + if !w.wroteHeader { + w.WriteHeader(http.StatusOK) + } + return w.ResponseWriter.Write(b) +} + +func (w *statusResponseWriter) WriteHeader(status int) { + if !w.wroteHeader { + w.wroteHeader = true + w.ResponseWriter.WriteHeader(status) + } +}