appview/oauth: add handler tests and fix errcheck lint violations #1224

+3 -3

appview/oauth/handler.go

··· 321 321 if err != nil { 322 322 return nil, fmt.Errorf("failed to create session: %v", err) 323 323 } 324 - defer sessionResp.Body.Close() 324 + defer func() { _ = sessionResp.Body.Close() }() 325 325 326 326 if sessionResp.StatusCode != http.StatusOK { 327 327 return nil, fmt.Errorf("failed to create session: HTTP %d", sessionResp.StatusCode) ··· 356 356 if err != nil { 357 357 return fmt.Errorf("failed to refresh session: %w", err) 358 358 } 359 - defer resp.Body.Close() 359 + defer func() { _ = resp.Body.Close() }() 360 360 361 361 if resp.StatusCode != http.StatusOK { 362 362 var errorResponse map[string]any ··· 430 430 if err != nil { 431 431 return fmt.Errorf("failed to add user to default service: %w", err) 432 432 } 433 - defer resp.Body.Close() 433 + defer func() { _ = resp.Body.Close() }() 434 434 435 435 if resp.StatusCode != http.StatusOK { 436 436 var errorResponse map[string]any

+360

appview/oauth/handler_test.go

··· 1 + package oauth 2 + 3 + import ( 4 + "context" 5 + "encoding/json" 6 + "log/slog" 7 + "net/http" 8 + "net/http/httptest" 9 + "net/url" 10 + "slices" 11 + "strings" 12 + "testing" 13 + "time" 14 + 15 + "github.com/bluesky-social/indigo/atproto/atcrypto" 16 + indigooauth "github.com/bluesky-social/indigo/atproto/auth/oauth" 17 + "github.com/gorilla/sessions" 18 + "github.com/posthog/posthog-go" 19 + "tangled.org/core/appview/config" 20 + ) 21 + 22 + // noopPosthog satisfies posthog.Client without doing anything. 23 + type noopPosthog struct{} 24 + 25 + func (noopPosthog) Close() error { return nil } 26 + func (noopPosthog) Enqueue(posthog.Message) error { return nil } 27 + func (noopPosthog) IsFeatureEnabled(posthog.FeatureFlagPayload) (interface{}, error) { 28 + return false, nil 29 + } 30 + func (noopPosthog) GetFeatureFlag(posthog.FeatureFlagPayload) (interface{}, error) { return false, nil } 31 + func (noopPosthog) GetFeatureFlagPayload(posthog.FeatureFlagPayload) (string, error) { return "", nil } 32 + func (noopPosthog) GetRemoteConfigPayload(string) (string, error) { return "", nil } 33 + func (noopPosthog) GetAllFlags(posthog.FeatureFlagPayloadNoKey) (map[string]interface{}, error) { 34 + return nil, nil 35 + } 36 + func (noopPosthog) ReloadFeatureFlags() error { return nil } 37 + func (noopPosthog) GetFeatureFlags() ([]posthog.FeatureFlag, error) { return nil, nil } 38 + func (noopPosthog) GetLastCapturedEvent() *posthog.Capture { return nil } 39 + 40 + // newTestOAuth builds a minimal *OAuth suitable for handler tests. 41 + // It uses an in-memory auth store (no Redis required) and a dev-mode config. 42 + // The returned *indigooauth.MemStore lets individual tests pre-seed auth state. 43 + func newTestOAuth(t *testing.T) (*OAuth, *indigooauth.MemStore) { 44 + t.Helper() 45 + 46 + privKey, err := atcrypto.GeneratePrivateKeyP256() 47 + if err != nil { 48 + t.Fatalf("generating test P-256 key: %v", err) 49 + } 50 + 51 + callbackURL := "http://127.0.0.1:3000/oauth/callback" 52 + cfg := indigooauth.NewLocalhostConfig(callbackURL, TangledScopes) 53 + if err := cfg.SetClientSecret(privKey, "test-key-1"); err != nil { 54 + t.Fatalf("setting client secret: %v", err) 55 + } 56 + 57 + memStore := indigooauth.NewMemStore() 58 + clientApp := indigooauth.NewClientApp(&cfg, memStore) 59 + 60 + appCfg := &config.Config{} 61 + appCfg.Core.Dev = true // disables TLS requirements and posthog 62 + 63 + sessStore := sessions.NewCookieStore([]byte("test-secret-32-bytes-1234567890ab")) 64 + 65 + return &OAuth{ 66 + ClientApp: clientApp, 67 + SessStore: sessStore, 68 + Config: appCfg, 69 + JwksUri: "http://127.0.0.1:3000/oauth/jwks.json", 70 + ClientName: "Test Client", 71 + ClientUri: "http://127.0.0.1:3000", 72 + Posthog: noopPosthog{}, 73 + Logger: slog.Default(), 74 + }, memStore 75 + } 76 + 77 + // seedState inserts a minimal AuthRequestData into store and returns the state key. 78 + func seedState(t *testing.T, store *indigooauth.MemStore, state string) { 79 + t.Helper() 80 + err := store.SaveAuthRequestInfo(context.Background(), indigooauth.AuthRequestData{ 81 + State: state, 82 + }) 83 + if err != nil { 84 + t.Fatalf("seeding auth state %q: %v", state, err) 85 + } 86 + } 87 + 88 + // TestClientMetadata verifies the /oauth/client-metadata.json endpoint returns 89 + // a well-formed OAuth client metadata document with all required fields. 90 + func TestClientMetadata(t *testing.T) { 91 + o, _ := newTestOAuth(t) 92 + router := o.Router() 93 + 94 + req := httptest.NewRequest(http.MethodGet, "/oauth/client-metadata.json", nil) 95 + rec := httptest.NewRecorder() 96 + router.ServeHTTP(rec, req) 97 + 98 + res := rec.Result() 99 + t.Cleanup(func() { _ = res.Body.Close() }) 100 + 101 + if res.StatusCode != http.StatusOK { 102 + t.Fatalf("status = %d, want 200", res.StatusCode) 103 + } 104 + if ct := res.Header.Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { 105 + t.Errorf("Content-Type = %q, want application/json", ct) 106 + } 107 + 108 + // Decode into a loose struct so we can check individual fields 109 + // while still detecting unexpected types through JSON decoding. 110 + var meta struct { 111 + ClientID string `json:"client_id"` 112 + GrantTypes []string `json:"grant_types"` 113 + Scope string `json:"scope"` 114 + ResponseTypes []string `json:"response_types"` 115 + RedirectURIs []string `json:"redirect_uris"` 116 + TokenEndpointAuthMethod string `json:"token_endpoint_auth_method"` 117 + DPoPBoundAccessTokens bool `json:"dpop_bound_access_tokens"` 118 + JWKSURI *string `json:"jwks_uri"` 119 + ClientName *string `json:"client_name"` 120 + ClientURI *string `json:"client_uri"` 121 + } 122 + if err := json.NewDecoder(res.Body).Decode(&meta); err != nil { 123 + t.Fatalf("decoding client metadata: %v", err) 124 + } 125 + 126 + if meta.ClientID == "" { 127 + t.Error("client_id must not be empty") 128 + } 129 + if !slices.Contains(meta.GrantTypes, "authorization_code") { 130 + t.Errorf("grant_types must include 'authorization_code'; got %v", meta.GrantTypes) 131 + } 132 + if !strings.Contains(meta.Scope, "atproto") { 133 + t.Errorf("scope must include 'atproto'; got %q", meta.Scope) 134 + } 135 + if !strings.Contains(meta.Scope, "identity:handle") { 136 + t.Errorf("scope must include 'identity:handle' (added by handler); got %q", meta.Scope) 137 + } 138 + if !slices.Contains(meta.ResponseTypes, "code") { 139 + t.Errorf("response_types must include 'code'; got %v", meta.ResponseTypes) 140 + } 141 + if len(meta.RedirectURIs) == 0 { 142 + t.Error("redirect_uris must not be empty") 143 + } 144 + if !meta.DPoPBoundAccessTokens { 145 + t.Error("dpop_bound_access_tokens must be true (required by AT Protocol)") 146 + } 147 + if meta.JWKSURI == nil || *meta.JWKSURI != o.JwksUri { 148 + t.Errorf("jwks_uri = %v, want %q", meta.JWKSURI, o.JwksUri) 149 + } 150 + if meta.ClientName == nil || *meta.ClientName != o.ClientName { 151 + t.Errorf("client_name = %v, want %q", meta.ClientName, o.ClientName) 152 + } 153 + if meta.ClientURI == nil || *meta.ClientURI != o.ClientUri { 154 + t.Errorf("client_uri = %v, want %q", meta.ClientURI, o.ClientUri) 155 + } 156 + } 157 + 158 + // TestJWKS verifies the /oauth/jwks.json endpoint returns a valid JWKS 159 + // containing the public P-256 key set up during OAuth initialization, 160 + // and that it contains no private key material. 161 + func TestJWKS(t *testing.T) { 162 + o, _ := newTestOAuth(t) 163 + router := o.Router() 164 + 165 + req := httptest.NewRequest(http.MethodGet, "/oauth/jwks.json", nil) 166 + rec := httptest.NewRecorder() 167 + router.ServeHTTP(rec, req) 168 + 169 + res := rec.Result() 170 + t.Cleanup(func() { _ = res.Body.Close() }) 171 + 172 + if res.StatusCode != http.StatusOK { 173 + t.Fatalf("status = %d, want 200", res.StatusCode) 174 + } 175 + if ct := res.Header.Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { 176 + t.Errorf("Content-Type = %q, want application/json", ct) 177 + } 178 + 179 + var jwks struct { 180 + Keys []map[string]any `json:"keys"` 181 + } 182 + if err := json.NewDecoder(res.Body).Decode(&jwks); err != nil { 183 + t.Fatalf("decoding JWKS: %v", err) 184 + } 185 + if len(jwks.Keys) == 0 { 186 + t.Fatal("JWKS must contain at least one key") 187 + } 188 + 189 + key := jwks.Keys[0] 190 + 191 + // A P-256 EC public key must carry these mandatory JWK fields. 192 + for _, field := range []string{"kty", "crv", "x", "y"} { 193 + if v, ok := key[field]; !ok || v == "" { 194 + t.Errorf("JWK missing or empty required field %q", field) 195 + } 196 + } 197 + if key["kty"] != "EC" { 198 + t.Errorf("kty = %v, want EC", key["kty"]) 199 + } 200 + if key["crv"] != "P-256" { 201 + t.Errorf("crv = %v, want P-256", key["crv"]) 202 + } 203 + // Private key component must never be exposed in the public JWKS. 204 + if _, hasD := key["d"]; hasD { 205 + t.Error("JWKS must not contain private key component 'd'") 206 + } 207 + } 208 + 209 + // TestCallback covers all reachable paths through the callback handler that 210 + // do not require a live PDS / auth server exchange. 211 + func TestCallback(t *testing.T) { 212 + tests := []struct { 213 + name string 214 + buildQuery func(store *indigooauth.MemStore) url.Values 215 + wantStatus int 216 + wantLocation string 217 + }{ 218 + { 219 + // ProcessCallback returns "missing state query param" – a generic error. 220 + name: "no query params redirects to login/oauth", 221 + buildQuery: func(_ *indigooauth.MemStore) url.Values { return url.Values{} }, 222 + wantStatus: http.StatusFound, 223 + wantLocation: "/login?error=oauth", 224 + }, 225 + { 226 + // ProcessCallback cannot find the state and returns a generic error. 227 + name: "unknown state redirects to login/oauth", 228 + buildQuery: func(_ *indigooauth.MemStore) url.Values { 229 + return url.Values{"state": {"no-such-state"}} 230 + }, 231 + wantStatus: http.StatusFound, 232 + wantLocation: "/login?error=oauth", 233 + }, 234 + { 235 + // ProcessCallback returns AuthRequestCallbackError – error code propagated. 236 + name: "auth server returns access_denied", 237 + buildQuery: func(store *indigooauth.MemStore) url.Values { 238 + const state = "state-access-denied" 239 + seedState(t, store, state) 240 + return url.Values{"state": {state}, "error": {"access_denied"}} 241 + }, 242 + wantStatus: http.StatusFound, 243 + wantLocation: "/login?error=access_denied", 244 + }, 245 + { 246 + name: "auth server returns server_error", 247 + buildQuery: func(store *indigooauth.MemStore) url.Values { 248 + const state = "state-server-error" 249 + seedState(t, store, state) 250 + return url.Values{"state": {state}, "error": {"server_error"}} 251 + }, 252 + wantStatus: http.StatusFound, 253 + wantLocation: "/login?error=server_error", 254 + }, 255 + { 256 + name: "auth server returns login_required", 257 + buildQuery: func(store *indigooauth.MemStore) url.Values { 258 + const state = "state-login-required" 259 + seedState(t, store, state) 260 + return url.Values{"state": {state}, "error": {"login_required"}} 261 + }, 262 + wantStatus: http.StatusFound, 263 + wantLocation: "/login?error=login_required", 264 + }, 265 + { 266 + // State exists, no error param, but iss/code are missing – generic error. 267 + name: "seeded state without code or iss redirects to login/oauth", 268 + buildQuery: func(store *indigooauth.MemStore) url.Values { 269 + const state = "state-no-code" 270 + seedState(t, store, state) 271 + return url.Values{"state": {state}} // no error, no code, no iss 272 + }, 273 + wantStatus: http.StatusFound, 274 + wantLocation: "/login?error=oauth", 275 + }, 276 + { 277 + // State exists, error param present but code/iss also provided – error wins. 278 + name: "error param takes precedence over code", 279 + buildQuery: func(store *indigooauth.MemStore) url.Values { 280 + const state = "state-error-with-code" 281 + seedState(t, store, state) 282 + return url.Values{ 283 + "state": {state}, 284 + "error": {"consent_required"}, 285 + "code": {"some-code"}, 286 + "iss": {"https://bsky.social"}, 287 + } 288 + }, 289 + wantStatus: http.StatusFound, 290 + wantLocation: "/login?error=consent_required", 291 + }, 292 + } 293 + 294 + for _, tt := range tests { 295 + t.Run(tt.name, func(t *testing.T) { 296 + o, store := newTestOAuth(t) 297 + router := o.Router() 298 + 299 + q := tt.buildQuery(store) 300 + target := "/oauth/callback" 301 + if len(q) > 0 { 302 + target += "?" + q.Encode() 303 + } 304 + 305 + req := httptest.NewRequest(http.MethodGet, target, nil) 306 + rec := httptest.NewRecorder() 307 + router.ServeHTTP(rec, req) 308 + 309 + res := rec.Result() 310 + t.Cleanup(func() { _ = res.Body.Close() }) 311 + 312 + if res.StatusCode != tt.wantStatus { 313 + t.Errorf("status = %d, want %d", res.StatusCode, tt.wantStatus) 314 + } 315 + if loc := res.Header.Get("Location"); loc != tt.wantLocation { 316 + t.Errorf("Location = %q, want %q", loc, tt.wantLocation) 317 + } 318 + }) 319 + } 320 + } 321 + 322 + // TestAppPasswordSession_isValid checks the session validity window logic 323 + // without any network calls. 324 + func TestAppPasswordSession_isValid(t *testing.T) { 325 + tests := []struct { 326 + name string 327 + expiresAt time.Time 328 + want bool 329 + }{ 330 + { 331 + name: "valid when expiry is in the future", 332 + expiresAt: time.Now().Add(time.Hour), 333 + want: true, 334 + }, 335 + { 336 + name: "invalid when expiry is in the past", 337 + expiresAt: time.Now().Add(-time.Second), 338 + want: false, 339 + }, 340 + { 341 + name: "invalid when expiry is zero value", 342 + expiresAt: time.Time{}, 343 + want: false, 344 + }, 345 + { 346 + name: "valid with typical 115-minute window", 347 + expiresAt: time.Now().Add(115 * time.Minute), 348 + want: true, 349 + }, 350 + } 351 + 352 + for _, tt := range tests { 353 + t.Run(tt.name, func(t *testing.T) { 354 + s := &AppPasswordSession{ExpiresAt: tt.expiresAt} 355 + if got := s.isValid(); got != tt.want { 356 + t.Errorf("isValid() = %v, want %v", got, tt.want) 357 + } 358 + }) 359 + } 360 + }

+1 -1

appview/oauth/oauth.go

··· 407 407 } 408 408 409 409 info.AccountDID = &parsedDid 410 - o.ClientApp.Store.SaveAuthRequestInfo(ctx, *info) 410 + _ = o.ClientApp.Store.SaveAuthRequestInfo(ctx, *info) 411 411 412 412 if err := o.SetAuthReturn(w, r, returnURL, false); err != nil { 413 413 return "", fmt.Errorf("failed to set auth return: %w", err)

appview/oauth: add handler tests and fix errcheck lint violations #1224

Summary#

What is covered#

What is not yet covered#

What's next#

History