Skip to content

Commit d32f2bf

Browse files
JAORMXclaude
andauthored
Unify authentication with Identity struct across ToolHive (#2437)
* Unify authentication with Identity struct across ToolHive Consolidate the vMCP Identity infrastructure into pkg/auth to eliminate code duplication and simplify authentication flow throughout ToolHive. Changes: - Move Identity struct from pkg/vmcp/auth to pkg/auth as the canonical type for representing authenticated principals - Update all authentication middleware (OIDC, local, anonymous) to directly create and inject Identity into context - Remove duplicate IdentityMiddleware from vMCP (now redundant) - Update authz and audit packages to use IdentityFromContext - Add backward-compatible GetClaimsFromContext helper - Delete duplicate implementations: identity.go, context.go, and associated test files from pkg/vmcp/auth This reduces code by ~655 lines while maintaining full functionality and test coverage. All middleware now outputs Identity directly, eliminating the need for a separate Claims → Identity conversion layer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Address PR feedback: Fix token exchange integration and add test coverage This commit addresses all feedback from PR #2437 review: Critical fixes: - Fix token exchange middleware to use Identity from context instead of directly accessing ClaimsContextKey, preventing silent token exchange failures in production (pkg/auth/tokenexchange/middleware.go) - Update all token exchange tests to use WithIdentity pattern to match production auth flow (pkg/auth/tokenexchange/middleware_test.go) Code quality improvements: - Add nil safety check in GetClaimsFromContext to guard against edge case where nil Identity pointer is explicitly stored in context (pkg/auth/context.go) - Add WWW-Authenticate header for claims-to-identity conversion errors for RFC 6750 compliance (pkg/auth/token.go) Test coverage: - Restore test coverage from deleted pkg/vmcp/auth files: - pkg/auth/identity_test.go: Tests for Identity struct, MarshalJSON, String, and claimsToIdentity conversion - pkg/auth/context_test.go: Tests for context operations including edge cases for nil handling and backward compatibility All tests pass. Linting clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Remove dead ClaimsContextKey code ClaimsContextKey was a legacy context key that is no longer used after the Identity unification. All authentication middleware now uses IdentityContextKey via WithIdentity(), and GetClaimsFromContext() reads from Identity, never from ClaimsContextKey. Changes: - Remove ClaimsContextKey type definition from pkg/auth/context.go - Remove obsolete comment reference from pkg/auth/token.go - Update test to use realistic edge case (Identity with nil Claims) instead of testing impossible scenario with ClaimsContextKey All tests pass. No functional change to production code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 028cc6d commit d32f2bf

28 files changed

+784
-805
lines changed

pkg/audit/auditor.go

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -315,26 +315,26 @@ func (*Auditor) getClientIP(r *http.Request) string {
315315
func (*Auditor) extractSubjects(r *http.Request) map[string]string {
316316
subjects := make(map[string]string)
317317

318-
// Extract user information from JWT claims
319-
if claims, ok := auth.GetClaimsFromContext(r.Context()); ok {
320-
if sub, ok := claims["sub"].(string); ok && sub != "" {
321-
subjects[SubjectKeyUserID] = sub
318+
// Extract user information from Identity
319+
if identity, ok := auth.IdentityFromContext(r.Context()); ok {
320+
if identity.Subject != "" {
321+
subjects[SubjectKeyUserID] = identity.Subject
322322
}
323323

324-
if name, ok := claims["name"].(string); ok && name != "" {
325-
subjects[SubjectKeyUser] = name
326-
} else if preferredUsername, ok := claims["preferred_username"].(string); ok && preferredUsername != "" {
324+
if identity.Name != "" {
325+
subjects[SubjectKeyUser] = identity.Name
326+
} else if preferredUsername, ok := identity.Claims["preferred_username"].(string); ok && preferredUsername != "" {
327327
subjects[SubjectKeyUser] = preferredUsername
328-
} else if email, ok := claims["email"].(string); ok && email != "" {
329-
subjects[SubjectKeyUser] = email
328+
} else if identity.Email != "" {
329+
subjects[SubjectKeyUser] = identity.Email
330330
}
331331

332332
// Add client information if available
333-
if clientName, ok := claims["client_name"].(string); ok && clientName != "" {
333+
if clientName, ok := identity.Claims["client_name"].(string); ok && clientName != "" {
334334
subjects[SubjectKeyClientName] = clientName
335335
}
336336

337-
if clientVersion, ok := claims["client_version"].(string); ok && clientVersion != "" {
337+
if clientVersion, ok := identity.Claims["client_version"].(string); ok && clientVersion != "" {
338338
subjects[SubjectKeyClientVersion] = clientVersion
339339
}
340340
}

pkg/audit/auditor_test.go

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package audit
22

33
import (
44
"bytes"
5-
"context"
65
"encoding/json"
76
"fmt"
87
"net/http"
@@ -353,7 +352,13 @@ func TestExtractSubjects(t *testing.T) {
353352
}
354353

355354
req := httptest.NewRequest("GET", "/test", nil)
356-
ctx := context.WithValue(req.Context(), auth.ClaimsContextKey{}, claims)
355+
identity := &auth.Identity{
356+
Subject: claims["sub"].(string),
357+
Name: claims["name"].(string),
358+
Email: claims["email"].(string),
359+
Claims: claims,
360+
}
361+
ctx := auth.WithIdentity(req.Context(), identity)
357362
req = req.WithContext(ctx)
358363

359364
subjects := auditor.extractSubjects(req)
@@ -372,7 +377,11 @@ func TestExtractSubjects(t *testing.T) {
372377
}
373378

374379
req := httptest.NewRequest("GET", "/test", nil)
375-
ctx := context.WithValue(req.Context(), auth.ClaimsContextKey{}, claims)
380+
identity := &auth.Identity{
381+
Subject: claims["sub"].(string),
382+
Claims: claims,
383+
}
384+
ctx := auth.WithIdentity(req.Context(), identity)
376385
req = req.WithContext(ctx)
377386

378387
subjects := auditor.extractSubjects(req)
@@ -389,7 +398,12 @@ func TestExtractSubjects(t *testing.T) {
389398
}
390399

391400
req := httptest.NewRequest("GET", "/test", nil)
392-
ctx := context.WithValue(req.Context(), auth.ClaimsContextKey{}, claims)
401+
identity := &auth.Identity{
402+
Subject: claims["sub"].(string),
403+
Email: claims["email"].(string),
404+
Claims: claims,
405+
}
406+
ctx := auth.WithIdentity(req.Context(), identity)
393407
req = req.WithContext(ctx)
394408

395409
subjects := auditor.extractSubjects(req)

pkg/auth/anonymous.go

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,17 @@
22
package auth
33

44
import (
5-
"context"
65
"net/http"
76
"time"
87

98
"github.com/golang-jwt/jwt/v5"
109
)
1110

12-
// AnonymousMiddleware creates an HTTP middleware that sets up anonymous claims.
11+
// AnonymousMiddleware creates an HTTP middleware that sets up anonymous identity.
1312
// This is useful for testing and local environments where authorization policies
1413
// need to work without requiring actual authentication.
1514
//
16-
// The middleware sets up basic anonymous claims that can be used by authorization
15+
// The middleware sets up basic anonymous identity that can be used by authorization
1716
// policies, allowing them to function even when authentication is disabled.
1817
// This is heavily discouraged in production settings but is handy for testing
1918
// and local development environments.
@@ -31,9 +30,18 @@ func AnonymousMiddleware(next http.Handler) http.Handler {
3130
"name": "Anonymous User",
3231
}
3332

34-
// Add the anonymous claims to the request context using the same key
35-
// as the JWT middleware for consistency
36-
ctx := context.WithValue(r.Context(), ClaimsContextKey{}, claims)
33+
// Create Identity from claims
34+
identity := &Identity{
35+
Subject: "anonymous",
36+
Name: "Anonymous User",
37+
Email: "anonymous@localhost",
38+
Claims: claims,
39+
Token: "", // No token for anonymous auth
40+
TokenType: "Bearer",
41+
}
42+
43+
// Add the Identity to the request context
44+
ctx := WithIdentity(r.Context(), identity)
3745
next.ServeHTTP(w, r.WithContext(ctx))
3846
})
3947
}

pkg/auth/context.go

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
// Package auth provides authentication and authorization utilities.
2+
package auth
3+
4+
import (
5+
"context"
6+
"errors"
7+
8+
"github.com/golang-jwt/jwt/v5"
9+
)
10+
11+
// IdentityContextKey is the key used to store Identity in the request context.
12+
// This provides type-safe context storage and retrieval for authenticated identities.
13+
//
14+
// Using an empty struct as the key prevents collisions with other context keys,
15+
// as each empty struct type is distinct even if they have the same name in different packages.
16+
type IdentityContextKey struct{}
17+
18+
// WithIdentity stores an Identity in the context.
19+
// If identity is nil, the original context is returned unchanged.
20+
//
21+
// This function is typically called by authentication middleware after successful
22+
// authentication to make the identity available to downstream handlers.
23+
//
24+
// Example:
25+
//
26+
// identity := &Identity{Subject: "user123", Name: "Alice"}
27+
// ctx = WithIdentity(ctx, identity)
28+
func WithIdentity(ctx context.Context, identity *Identity) context.Context {
29+
if identity == nil {
30+
return ctx
31+
}
32+
return context.WithValue(ctx, IdentityContextKey{}, identity)
33+
}
34+
35+
// IdentityFromContext retrieves an Identity from the context.
36+
// Returns the identity and true if present, nil and false otherwise.
37+
//
38+
// This function is typically called by authorization middleware or handlers that need
39+
// to check who the authenticated user is.
40+
//
41+
// Example:
42+
//
43+
// identity, ok := IdentityFromContext(ctx)
44+
// if !ok {
45+
// return errors.New("no authenticated identity")
46+
// }
47+
// log.Printf("Request from user: %s", identity.Subject)
48+
func IdentityFromContext(ctx context.Context) (*Identity, bool) {
49+
identity, ok := ctx.Value(IdentityContextKey{}).(*Identity)
50+
return identity, ok
51+
}
52+
53+
// GetClaimsFromContext retrieves the claims from Identity in the request context.
54+
// This is a helper function for backward compatibility with code that expects MapClaims.
55+
// New code should use IdentityFromContext and access the Claims field directly.
56+
func GetClaimsFromContext(ctx context.Context) (jwt.MapClaims, bool) {
57+
if ctx == nil {
58+
return nil, false
59+
}
60+
61+
// Get Identity and return its Claims
62+
if identity, ok := IdentityFromContext(ctx); ok && identity != nil {
63+
if identity.Claims != nil {
64+
return jwt.MapClaims(identity.Claims), true
65+
}
66+
}
67+
68+
return nil, false
69+
}
70+
71+
// claimsToIdentity converts JWT claims to Identity struct.
72+
// It requires the 'sub' claim per OIDC Core 1.0 spec § 5.1.
73+
// The original token can be provided for passthrough scenarios.
74+
//
75+
// Note: The Groups field is intentionally NOT populated here.
76+
// Authorization logic MUST extract groups from the Claims map, as group claim
77+
// names vary by provider (e.g., "groups", "roles", "cognito:groups").
78+
func claimsToIdentity(claims jwt.MapClaims, token string) (*Identity, error) {
79+
// Validate required 'sub' claim per OIDC Core 1.0 spec
80+
sub, ok := claims["sub"].(string)
81+
if !ok || sub == "" {
82+
return nil, errors.New("missing or invalid 'sub' claim (required by OIDC Core 1.0 § 5.1)")
83+
}
84+
85+
identity := &Identity{
86+
Subject: sub,
87+
Claims: claims,
88+
Token: token,
89+
TokenType: "Bearer",
90+
}
91+
92+
// Extract optional standard claims
93+
if name, ok := claims["name"].(string); ok {
94+
identity.Name = name
95+
}
96+
if email, ok := claims["email"].(string); ok {
97+
identity.Email = email
98+
}
99+
100+
return identity, nil
101+
}

0 commit comments

Comments
 (0)