diff --git a/authn/README.md b/authn/README.md index 7a614071..e02bfb89 100644 --- a/authn/README.md +++ b/authn/README.md @@ -7,33 +7,33 @@ JWT-based authentication. Validates tokens against a JWKS endpoint, extracts ide ```go import "github.com/OpenNSW/core/authn" -manager, err := authn.NewManager(userProfileSvc, authn.Config{ +manager, err := authn.NewManager(authn.Config{ JWKSURL: "https://idp.example.com/.well-known/jwks.json", Issuer: "https://idp.example.com", Audience: "my-api", - // Optional: declare any claims beyond the fixed schema you need. See - // "Extra claims" below. - UserClaims: authn.ClaimSpec{Optional: []string{"email", "ouId", "ouHandle"}}, + // Optional: make standard identity claims required. See "Standard identity claims" below. + Claims: authn.ClaimsConfig{ + OUID: authn.ClaimConfig{Required: true}, + }, }) ``` -`UserProfileService` is optional. When provided, it is called on the first appearance of a user token to create or retrieve a persisted user record (e.g. to assign an internal user ID). Pass `nil` to skip user persistence. - -`Config` (and `ClaimSpec`) carry `yaml` struct tags, so it can be embedded in a larger application config struct and populated generically (e.g. via `yaml.Unmarshal`): +`Config` carries `yaml` struct tags, so it can be embedded in a larger application config struct and populated generically (e.g. via `yaml.Unmarshal`): ```yaml authn: jwksURL: https://idp.example.com/.well-known/jwks.json issuer: https://idp.example.com audience: my-api - userClaims: - optional: [email, ouId, ouHandle] + claims: + ouId: + required: true ``` -> **Note:** every token must carry a `grant_type` claim — it decides user vs. client principal, and a token without it is rejected with `unsupported grant type`. This is not a standard access-token claim (RFC 9068 does not define it), so an IdP that omits it will not work with this package as-is. +A token is accepted when its signature, `iss`, `aud` and `exp` check out, it carries non-empty `sub` and `client_id` claims, and it carries every standard identity claim configured as required. How the token was obtained (`grant_type`) plays no part: a person signing in through a client and a system using client credentials get the same `Principal`, distinguished only by the roles, scopes and claims they carry. Every other claim in the token is passed through as `ExtraClaims` — nothing has to be declared. -> **Note:** `audience` is what binds a token to this service: any client the IdP issues a token for this audience is accepted. Which clients may call the service is authorization policy, not this package's concern. For client-credentials tokens, restrict it in authz or the application using `ClientPrincipal.ClientID`. User (`authorization_code`) tokens produce a `UserPrincipal` that does not carry the originating client ID, so restricting those by client must be done another way (e.g. IdP-side policy or a dedicated audience per client group). If your IdP sets `aud` to the requesting client rather than the API, configure an audience / API resource for the service in the IdP. +> **Note:** `audience` is what binds a token to this service: any client the IdP issues a token for this audience is accepted. Which clients may call the service is authorization policy, not this package's concern — restrict it in authz or the application using `AuthContext.ClientID()`. If your IdP sets `aud` to the requesting client rather than the API, configure an audience / API resource for the service in the IdP. ## Middleware @@ -45,159 +45,148 @@ mux.Handle("/api/v1/tasks", manager.RequireAuthMiddleware()(handler)) mux.Handle("/api/v1/public", manager.OptionalAuthMiddleware()(handler)) ``` -## Reading identity in handlers +`authn` only authenticates. Anything your application does with the validated principal — provisioning a user record, loading a profile, attaching tenant data — goes in your own middleware, chained after `authn`'s: ```go -authCtx := authn.GetAuthContext(r.Context()) -if authCtx == nil { - // no token present (only possible with OptionalAuthMiddleware) - return +// in the application, not in authn +func ProvisionUser(users *user.Service) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + authCtx := authn.GetAuthContext(r.Context()) + u, err := users.GetOrCreate(r.Context(), authCtx.Subject(), authCtx.Email(), authCtx.OUID()) + if err != nil { + http.Error(w, "user provisioning failed", http.StatusInternalServerError) + return + } + next.ServeHTTP(w, r.WithContext(user.NewContext(r.Context(), u))) + }) + } } -// Human user token -if authCtx.Type() == authn.UserPrincipalType { - fmt.Println(authCtx.User.ID) // internal persisted ID (if UserProfileService set) - fmt.Println(authCtx.User.Roles) // role claims - fmt.Println(authCtx.User.ExtraClaims.String("email")) // "" unless declared — see "Extra claims" - fmt.Println(authCtx.User.ExtraClaims.String("ouHandle")) -} - -// Machine-to-machine token -if authCtx.Type() == authn.ClientPrincipalType { - fmt.Println(authCtx.Client.ClientID) -} -``` - -`AuthContext` also exposes a nil-safe accessor seam that works for either principal type without branching — `Type()`, `Subject()`, `Roles()`, `Scopes()`, and `ExtraClaims()`: - -```go -// Safe even when authCtx is nil or the request was a client token. -email := authn.GetAuthContext(r.Context()).ExtraClaims().String("email") -``` - -## Extra claims (beyond the fixed schema) +// wrap all protected routes once, not per route +api := http.NewServeMux() +api.HandleFunc("POST /api/x", handlerX) -`authn`'s fixed schema is deliberately small: `client_id`, `grant_type`, `scope`, and `roles` (plus the JWT registered claims). Anything else — `email`, `phone_number`, `ouId`, `ouHandle`, an IdP-specific claim like `given_name`, or anything your identity provider emits — is declared explicitly via `Config` fields (`Manager` path) or functional options (direct `TokenExtractor` construction). Separate declarations exist for user-principal (`authorization_code` grant) and client-principal (`client_credentials` grant) tokens, since some claims are only meaningful for one or the other. - -```go -manager, err := authn.NewManager(userProfileSvc, authn.Config{ - JWKSURL: "...", - Issuer: "...", - Audience: "...", - - UserClaims: authn.ClaimSpec{ - Optional: []string{"email", "ouId", "ouHandle", "given_name"}, - Required: []string{"phone_number"}, - }, - ClientClaims: authn.ClaimSpec{ - Optional: []string{"department"}, - }, -}) +root := http.NewServeMux() +root.Handle("/api/", manager.RequireAuthMiddleware()(ProvisionUser(users)(api))) ``` -Or, constructing a `TokenExtractor` directly: - -```go -extractor, err := authn.NewTokenExtractor(jwksURL, issuer, audience, - authn.WithUserClaims(authn.ClaimSpec{ - Optional: []string{"email", "ouId", "ouHandle"}, - Required: []string{"phone_number"}, - }), -) -``` +The application owns that policy: who gets a record, what goes on the context, and whether a failure rejects the request. -Reading extra claims in a handler: +## Reading identity in handlers ```go authCtx := authn.GetAuthContext(r.Context()) -if authCtx == nil || authCtx.Type() != authn.UserPrincipalType { +if authCtx == nil { + // no token present (only possible with OptionalAuthMiddleware) return } -email := authCtx.User.ExtraClaims.String("email") // "" if not declared/present -groups := authCtx.User.ExtraClaims.Strings("groups") // JSON array of strings, or a space-delimited string -roles := authCtx.User.Roles // roles are fixed schema, not an extra claim -``` - -If a `UserProfileService` implementation needs one of these values (e.g. to scope users by organization), declare it and read it from the principal passed into `GetOrCreateUser` — see "Principal fields" below. -**Semantics:** -- **User-scoped vs. client-scoped.** `UserClaims` applies only to user-principal tokens, `ClientClaims` only to client-principal tokens. A name declared on one side is never extracted from the other. -- **Optional:** a claim that is absent, JSON-null, or an empty/whitespace-only string is silently skipped — the token is never rejected. -- **Required:** the token is rejected unless the claim carries a usable value — a non-blank string, or a non-empty array of non-blank strings. A number, boolean, object, empty array or mixed array is *not* accepted, because it would read back as `""`/`nil` and silently defeat the requirement. A name listed in both `Optional` and `Required` (in any order, across any number of calls) is required. -- Claim names are matched **exactly** — JWT claim names are case-sensitive (RFC 7519 §4) and only whitespace-trimmed. Nested lookups are not supported: a name is a top-level key, which is what lets namespaced names like `https://app.example.com/roles` work verbatim. -- Values are the claim's JSON-decoded Go representation, with one normalization: every JSON string a value *directly* carries is whitespace-trimmed — the value itself, and the elements of a top-level array of strings. Nested objects are never rewritten. (Fixed-schema claims such as `sub` are decoded separately and are not trimmed.) -- Use `ExtraClaims.String(name)` for a plain string value and `ExtraClaims.Strings(name)` for a list-shaped claim. `Strings` accepts a JSON array of strings *or* a single space-delimited string (the convention the `scope` claim uses), so it whitespace-splits free text — use `String` for anything that is not list-shaped. Both are nil-safe and return the zero value for absent/wrong-shape claims; they never panic or error. -- A claim already bound by `authn` — the fixed schema, any case variant of it, or your configured `RolesClaim` — cannot be declared as an extra claim. Construction (or `Config.Validate()`) fails fast. Case variants are rejected because `encoding/json` matches struct tags case-insensitively, so a payload key of `Roles` still lands in the fixed-schema roles field. -- A blank claim name is a construction error, not a silent no-op — a trailing comma in `MY_CLAIMS=email,` would otherwise drop a rule you asked for. - -## Remapping the roles claim +fmt.Println(authCtx.Subject()) // JWT "sub" +fmt.Println(authCtx.ClientID()) // JWT "client_id" +fmt.Println(authCtx.Roles()) // role claims +fmt.Println(authCtx.Scopes()) // scope claims +fmt.Println(authCtx.Email()) // "email", "" if absent +fmt.Println(authCtx.OUID(), authCtx.OUHandle()) // "ouId", "ouHandle", "" if absent +fmt.Println(authCtx.ExtraClaims().String("given_name")) // any other claim — see "Extra claims" +``` -Roles populate `Principal.Roles` and `AuthContext.Roles()`, which is what the [`authz`](../authz/README.md) seam consumes. If your IdP does not emit a top-level `roles` claim, point `authn` at the one it does emit: +The accessors are nil-safe, so they can be called on a nil `*AuthContext`: ```go -authn.Config{ /* ... */ RolesClaim: "groups" } // or authn.WithRolesClaim("groups") +email := authn.GetAuthContext(r.Context()).Email() ``` -The claim must be a top-level JSON array of strings — exactly the shape the default `roles` claim must have. An absent claim yields no roles; a present claim of any *other* shape rejects the token, so a mistyped name fails loudly on the first request rather than silently disabling every role check. - -Dotted paths into nested objects (Keycloak's `realm_access.roles`) are **not** supported — flatten them with an IdP-side protocol mapper. Exact matching is what keeps namespaced names such as `https://app.example.com/roles` working. - -## Principal fields - -**`UserContext`** +The same values are available as fields on `authCtx.Principal`. -| Field | Description | -|---|---| -| `ID` | Internal persisted user ID (set by `UserProfileService`) | -| `IDPUserID` | Subject claim from the token | -| `Roles` | Role claims | -| `Scopes` | Scope claims | -| `ExtraClaims` | Claims beyond the fixed schema, populated only for names you declared — see "Extra claims" above | +## Standard identity claims -**`ClientContext`** +`email`, `ouId` and `ouHandle` have dedicated fields: `Principal.Email`, `Principal.OUID` and `Principal.OUHandle`, also available as `Email()`, `OUID()` and `OUHandle()` on `AuthContext`. Values are whitespace-trimmed. -| Field | Description | -|---|---| -| `ClientID` | Client ID claim | -| `Roles` | Role claims | -| `Scopes` | Scope claims | -| `ExtraClaims` | Claims beyond the fixed schema, populated only for names you declared — see "Extra claims" above | +Each is **optional by default**: a token without the claim (or with a JSON-null or blank value) is accepted and the field is `""`. Configure a claim as required to reject such tokens instead: -**`UserProfileService`** +```yaml +claims: + email: {} # optional (same as omitting it) + ouId: + required: true + ouHandle: + required: false +``` ```go -type UserProfileService interface { - GetOrCreateUser(ctx context.Context, principal *UserPrincipal) (string, error) -} +authn.Config{ /* ... */ Claims: authn.ClaimsConfig{OUID: authn.ClaimConfig{Required: true}} } +// or authn.WithClaims(authn.ClaimsConfig{OUID: authn.ClaimConfig{Required: true}}) ``` -Called on first login for user-principal tokens. `principal.Subject` is the JWT `sub` claim — the IdP's user ID, *not* the persisted ID you return. `principal.ExtraClaims` holds whatever claims you declared; index it with `principal.ExtraClaims.String("email")`, etc. +- **Required applies to every token**, including client-credentials tokens issued to systems, which typically carry no email or organization unit. Only require a claim every caller's token carries; check anything route-specific in your own middleware or handler. +- A present claim that is not a JSON string rejects the token. -`principal` is never nil and **must not be mutated**: the middleware has already built the request's `AuthContext` from it and shares the same `ExtraClaims` map. +## Extra claims (beyond the fixed schema) -## Migrating from the fixed email/phone/ouId/ouHandle fields +`authn`'s fixed schema is deliberately small: `sub`, `client_id`, `scope`, `roles`, the standard identity claims `email`, `ouId` and `ouHandle`, plus the JWT registered claims `iss`, `aud`, `exp`, `nbf`, `iat`, `jti`. Every other claim in the token — `phone_number`, `grant_type`, an IdP-specific claim like `given_name`, a namespaced claim like `https://app.example.com/department` — is passed through in `ExtraClaims`, keyed by its exact name. -Those four claims used to be fixed-schema fields, populated on every user token. They are IdP-specific rather than standard OIDC, so they are now ordinary extra claims that you declare. +```go +authCtx := authn.GetAuthContext(r.Context()) +name := authCtx.ExtraClaims().String("given_name") // "" if absent or not a string +groups := authCtx.ExtraClaims().Strings("groups") // JSON array of strings, or a space-delimited string +dept := authCtx.ExtraClaims()["department"] // any other shape: index the map directly +``` -| Before | After | -|---|---| -| `authCtx.User.Email` | `authCtx.User.ExtraClaims.String("email")` + declare `"email"` | -| `authCtx.User.PhoneNumber` (`*string`) | `...String("phone_number")` + declare it | -| `authCtx.User.OUID` | `...String("ouId")` + declare it | -| `authCtx.User.OUHandle` | `...String("ouHandle")` + declare it | -| `UserPrincipal.UserID` | `UserPrincipal.Subject` | -| `GetOrCreateUser(ctx, idpUserID, email, phone, ouID, ouHandle)` | `GetOrCreateUser(ctx, principal)` | +`authn` does not decide which of these a request needs. If your application depends on one, check it in your own middleware or handler. -**Declaring is not optional.** An undeclared claim reads back as `""` even when the signed token carries it, and nothing warns you — the compiler catches the removed struct fields and the changed `GetOrCreateUser` signature, but it cannot catch a missing declaration. If a value is load-bearing (an organization/tenant identifier used for scoping, say), put it in `Required` so a token without it is rejected rather than silently scoped to `""`: +**Semantics:** +- Claims `authn` binds itself never appear in `ExtraClaims`, so each value has one home: read `Subject()`, not `ExtraClaims()["sub"]`. Case variants of fixed-schema names (`Roles`, `SUB`) are excluded too, because `encoding/json` matches struct tags case-insensitively and binds them to the fixed field. +- A claim whose value is JSON null or an empty/whitespace-only string is dropped, so it reads the same as a claim the token never carried. A token with no other claims has a nil `ExtraClaims`. +- Claim names are matched **exactly** — JWT claim names are case-sensitive (RFC 7519 §4). Nested lookups are not supported: a name is a top-level key, which is what lets namespaced names like `https://app.example.com/roles` work verbatim. +- Values are the claim's JSON-decoded Go representation, with one normalization: every JSON string a value *directly* carries is whitespace-trimmed — the value itself, and the elements of a top-level array of strings. Nested objects are never rewritten. (Fixed-schema claims such as `sub` are decoded separately and are not trimmed.) +- Use `ExtraClaims.String(name)` for a plain string value and `ExtraClaims.Strings(name)` for a list-shaped claim. `Strings` accepts a JSON array of strings *or* a single space-delimited string (the convention the `scope` claim uses), so it whitespace-splits free text — use `String` for anything that is not list-shaped. Both are nil-safe and return the zero value for absent/wrong-shape claims; they never panic or error. -```go -UserClaims: authn.ClaimSpec{ - Required: []string{"ouId"}, - Optional: []string{"email", "phone_number", "ouHandle"}, -}, -``` +## Principal fields -Earlier versions rejected a user token missing `email`, `ouId`, or `ouHandle`. That check is now yours to declare. +| Field | Claim | Required? | Description | +|---------------|-------------|------------------------------|------------------------------------------------------------------------------| +| `Subject` | `sub` | Always | The IdP's ID for the caller | +| `ClientID` | `client_id` | Always | The OAuth2 client the token was issued to | +| `Roles` | `roles` | No | Must be an array of strings if present; nil if absent | +| `Scopes` | `scope` | No | Space-delimited; nil if absent | +| `Email` | `email` | Configurable (default: no) | `""` if absent — see "Standard identity claims" above | +| `OUID` | `ouId` | Configurable (default: no) | `""` if absent | +| `OUHandle` | `ouHandle` | Configurable (default: no) | `""` if absent | +| `ExtraClaims` | — | — | Every other claim in the token — see "Extra claims" above | + +Validation also requires `exp`, and checks `iss` and `aud` against `issuer` and `audience`. + +Roles populate `AuthContext.Roles()`, which is what the [`authz`](../authz/README.md) seam consumes. They are read from a top-level `roles` claim only; a role claim under another name, or nested (such as `realm_access.roles`), has to be mapped to `roles` on the IdP side. A `roles` claim of any shape other than an array of strings rejects the token, rather than silently reading as "no roles". + +## Migrating from user and client principals + +Earlier versions split every caller into a user or a client principal by the token's `grant_type` claim. There is now one `Principal`. + +| Before | After | +|--------------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `NewManager(userProfileSvc, cfg)` | `NewManager(cfg)` | +| `Middleware(userProfileSvc, extractor)` / `RequireAuth(userProfileSvc, extractor)` | `Middleware(extractor)` / `RequireAuth(extractor)` | +| `UserProfileService.GetOrCreateUser` | Your own middleware chained after `authn` — see "Middleware" | +| `authCtx.User.ID` (persisted ID) | Whatever your provisioning middleware puts on the context | +| `authCtx.User.IDPUserID` | `authCtx.Subject()` | +| `authCtx.Client.ClientID` | `authCtx.ClientID()` | +| `authCtx.User.Roles` / `authCtx.Client.Roles` (and `Scopes`, `ExtraClaims`) | `authCtx.Roles()` (and `Scopes()`, `ExtraClaims()`) | +| `authCtx.Type()`, `UserPrincipalType`, `ClientPrincipalType` | Removed — branch on roles, scopes or claims instead | +| `authCtx.User.ExtraClaims.String("email")` (and `"ouId"`, `"ouHandle"`) | `authCtx.Email()` (and `OUID()`, `OUHandle()`) | +| `UserClaims` / `ClientClaims` (`ClaimSpec{Optional, Required}`), `WithUserClaims` / `WithClientClaims` | `email` / `ouId` / `ouHandle`: `Claims` (`ClaimsConfig`) / `WithClaims`. Any other claim: `ExtraClaims`, no declaration; check required ones in your own middleware | +| `RolesClaim` / `WithRolesClaim` | Removed — roles are always read from `roles`; map a differently-named roles claim to `roles` on the IdP side | + +Behaviour changes the compiler will not catch: + +- **`Subject()` now returns the JWT `sub` claim.** For user tokens it used to return the persisted user ID from `UserProfileService` (falling back to `sub`), and for client tokens it returned the `client_id`. Anything keyed by `Subject()` — stored records, audit entries, cache keys — must be checked. +- **`sub` is required on every token.** Client-credentials tokens without `sub` used to be accepted; they are now rejected. +- **`grant_type` is ignored.** Tokens without it, or with any value, are accepted. It now passes through in `ExtraClaims` like any other claim. +- **Only `email`, `ouId` and `ouHandle` can be required.** A token missing any other claim you previously listed as `Required` is now accepted; move that check into your own middleware. Requirements now apply to every token, not just user or client tokens. +- **`email`, `ouId` and `ouHandle` are no longer in `ExtraClaims`**: `ExtraClaims().String("email")` now returns `""`; use `Email()`. +- **`ExtraClaims` now holds every other claim**, not just declared ones. Code that treated "present in `ExtraClaims`" as "declared in config" must be checked. +- **A leftover `rolesClaim` key in a yaml config is silently ignored**, so roles are read from `roles` instead. If your IdP emits roles under another name, every caller now has no roles until the IdP maps them to `roles`. +- **The JSON shape of `AuthContext` changed**, for anything that serializes it. ## Health check diff --git a/authn/config.go b/authn/config.go index 3669c105..afe923c1 100644 --- a/authn/config.go +++ b/authn/config.go @@ -18,18 +18,29 @@ type Config struct { Audience string `yaml:"audience"` InsecureSkipTLSVerify bool `yaml:"insecureSkipTLSVerify"` - // UserClaims declares extra JWT claims (beyond authn's fixed schema, e.g. - // "email", "phone_number", "ouId", "ouHandle", "given_name") to extract - // for user-principal (authorization_code grant) tokens. ClientClaims is - // the client-credential (M2M) analogue. See WithUserClaims / - // WithClientClaims. Zero value = no extra claims extracted. - UserClaims ClaimSpec `yaml:"userClaims"` - ClientClaims ClaimSpec `yaml:"clientClaims"` + // Claims configures the standard identity claims. See WithClaims. Zero + // value = every claim optional. + Claims ClaimsConfig `yaml:"claims"` +} + +// ClaimsConfig configures each standard identity claim — "email", "ouId" and +// "ouHandle" — which populate the Principal fields of the same name. +// +// ClaimsConfig carries yaml struct tags, so it can be populated generically +// (e.g. via yaml.Unmarshal) as part of Config. +type ClaimsConfig struct { + Email ClaimConfig `yaml:"email"` + OUID ClaimConfig `yaml:"ouId"` + OUHandle ClaimConfig `yaml:"ouHandle"` +} - // RolesClaim overrides which claim carries the principal's roles, for - // IdPs that do not emit a top-level "roles" claim. See WithRolesClaim. - // Empty = "roles". - RolesClaim string `yaml:"rolesClaim"` +// ClaimConfig configures one standard identity claim. +type ClaimConfig struct { + // Required rejects a token whose claim is absent or blank. The + // requirement applies to every token, including client-credentials + // tokens issued to systems, which typically carry no email or + // organization unit. Default false: the Principal field is "" instead. + Required bool `yaml:"required"` } func (c Config) Validate() error { @@ -48,33 +59,5 @@ func (c Config) Validate() error { if c.Audience == "" { return fmt.Errorf("audience is required") } - - if err := validateRolesClaimName(c.rolesClaim()); err != nil { - return err - } - for _, decl := range []struct { - what string - names []string - }{ - {"UserClaims.Optional", c.UserClaims.Optional}, - {"UserClaims.Required", c.UserClaims.Required}, - {"ClientClaims.Optional", c.ClientClaims.Optional}, - {"ClientClaims.Required", c.ClientClaims.Required}, - } { - if err := validateClaimNames(decl.what, decl.names, c.rolesClaim()); err != nil { - return err - } - } - return nil } - -// rolesClaim resolves the configured roles claim name, treating an unset field -// as the default. A whitespace-only value is left as-is so Validate reports it -// as a typo rather than silently defaulting. -func (c Config) rolesClaim() string { - if c.RolesClaim == "" { - return defaultRolesClaim - } - return c.RolesClaim -} diff --git a/authn/config_test.go b/authn/config_test.go index 8ea7af01..c8fa07a9 100644 --- a/authn/config_test.go +++ b/authn/config_test.go @@ -44,73 +44,3 @@ func TestConfigValidate(t *testing.T) { }) } } - -func TestConfigValidate_RejectsFixedSchemaExtraClaim(t *testing.T) { - valid := Config{ - JWKSURL: "https://localhost/jwks", - Issuer: "https://localhost/token", - Audience: "TRADER_PORTAL_APP", - } - - tests := []struct { - name string - mutate func(*Config) - }{ - {"optional user claim", func(c *Config) { c.UserClaims.Optional = []string{"scope"} }}, - {"required user claim", func(c *Config) { c.UserClaims.Required = []string{"client_id"} }}, - {"optional client claim", func(c *Config) { c.ClientClaims.Optional = []string{"roles"} }}, - {"required client claim", func(c *Config) { c.ClientClaims.Required = []string{"sub"} }}, - // Case variants: encoding/json binds struct tags case-insensitively, - // so these still land in the fixed-schema fields and must be rejected. - {"case-variant user claim", func(c *Config) { c.UserClaims.Optional = []string{"Roles"} }}, - {"case-variant client claim", func(c *Config) { c.ClientClaims.Optional = []string{"Client_Id"} }}, - {"blank claim name", func(c *Config) { c.UserClaims.Required = []string{"email", " "} }}, - {"roles claim collides with extra claim", func(c *Config) { - c.RolesClaim = "groups" - c.UserClaims.Optional = []string{"groups"} - }}, - {"roles claim is a fixed-schema name", func(c *Config) { c.RolesClaim = "scope" }}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - cfg := valid - tt.mutate(&cfg) - if err := cfg.Validate(); err == nil { - t.Fatalf("expected error for fixed-schema claim declaration") - } - }) - } -} - -// TestConfigValidate_AllowsFormerlyFixedClaims locks in the schema shrink at -// the Config layer too: email/phone_number/ouId/ouHandle used to be -// hardcoded fixed-schema fields and are now legitimate extra-claim -// declarations. -func TestConfigValidate_AllowsFormerlyFixedClaims(t *testing.T) { - cfg := Config{ - JWKSURL: "https://localhost/jwks", - Issuer: "https://localhost/token", - Audience: "TRADER_PORTAL_APP", - UserClaims: ClaimSpec{Optional: []string{"email", "phone_number", "ouId", "ouHandle", "given_name"}}, - } - if err := cfg.Validate(); err != nil { - t.Fatalf("expected nil error, got %v", err) - } -} - -// TestConfigValidate_AllowsNamespacedClaimNames locks in that a claim name is -// matched exactly rather than parsed: Auth0-style namespaced claims contain -// dots and slashes and must not be mistaken for a nested path. -func TestConfigValidate_AllowsNamespacedClaimNames(t *testing.T) { - cfg := Config{ - JWKSURL: "https://localhost/jwks", - Issuer: "https://localhost/token", - Audience: "TRADER_PORTAL_APP", - RolesClaim: "https://app.example.com/roles", - UserClaims: ClaimSpec{Optional: []string{"https://app.example.com/department"}}, - } - if err := cfg.Validate(); err != nil { - t.Fatalf("expected nil error, got %v", err) - } -} diff --git a/authn/context.go b/authn/context.go index 7012fc32..b11897bf 100644 --- a/authn/context.go +++ b/authn/context.go @@ -7,90 +7,11 @@ import ( "context" ) -// UserProfileService defines the contract for managing user profiles. -// Implementations are responsible for persisting and managing user records in their system. -// -// This interface is OPTIONAL when using the auth package. If not provided (nil), -// user creation on first login will be skipped. This allows: -// -// 1. Systems that don't track user profiles - just use auth for token validation -// 2. Systems that manage user profiles separately - implement this interface -// 3. Systems that handle user creation elsewhere - pass nil -// -// The whole authenticated principal is passed rather than a list of named -// identity values, because everything except the JWT "sub" claim (email, phone -// number, organization/tenant identifiers, ...) is IdP/consumer-specific. -// Naming those as parameters is what forced this interface to change once -// already; passing the principal means adding a claim never changes the -// signature again. -// -// Example implementation: -// -// type MyUserService struct { -// db *sql.DB -// } -// -// func (s *MyUserService) GetOrCreateUser(ctx context.Context, principal *authn.UserPrincipal) (string, error) { -// // Your implementation to create or fetch the user idempotently -// email := principal.ExtraClaims.String("email") -// persistedID := "generated-id" -// if err := s.db.Exec("INSERT INTO users ...", principal.Subject, email).Error; err != nil { -// return "", err -// } -// return persistedID, nil -// } -// -// authManager := auth.NewManager(myUserService, cfg.Auth) // myUserService can be nil -type UserProfileService interface { - // GetOrCreateUser creates or retrieves a user profile. - // - // principal is never nil, and MUST NOT be mutated: the middleware has - // already built the request's AuthContext from it and shares the same - // ExtraClaims map, so writing to it would alter the live request context. - // (ExtraClaims is also nil unless claims were declared — nil maps read - // fine but panic on assignment.) - // - // - principal.Subject is the identity provider's user ID (the JWT "sub" - // claim), NOT the persisted ID this method returns. - // - principal.ExtraClaims holds the claims declared via WithUserClaims / - // Config.UserClaims; nil if none were declared. - // - // Implementation notes: - // - Should be idempotent: calling multiple times with the same subject should be safe - // - Called during first login after token validation - // - Errors are logged but don't block authentication - // - Should not return error if user already exists - // Returns user ID of the created or existing user, or an error if the operation fails. - GetOrCreateUser(ctx context.Context, principal *UserPrincipal) (string, error) -} - -// UserContext represents a user principal's runtime context injected into each request. -// It includes identity fields and principal-derived roles. -// Note: Per-request NSWData is not persisted here; services requiring user metadata -// should call the user profile service on-demand. -type UserContext struct { - ID string `json:"id"` - IDPUserID string `json:"idpUserId"` - Roles []string `json:"roles"` - Scopes []string `json:"scopes"` - ExtraClaims ExtraClaims `json:"extraClaims,omitempty"` -} - -// ClientContext represents a machine client's context. -type ClientContext struct { - ClientID string `json:"clientId"` - Roles []string `json:"roles"` - Scopes []string `json:"scopes"` - ExtraClaims ExtraClaims `json:"extraClaims,omitempty"` -} - // AuthContext is the transient authentication context injected into each request -// by the auth middleware. -// For user principals, User contains identity fields and roles. -// For client principals (M2M), Client is set. +// by the auth middleware. It carries the Principal resolved from the request's +// token. type AuthContext struct { - User *UserContext - Client *ClientContext + Principal Principal } // ContextKey is a custom type for context keys to avoid collisions. @@ -108,7 +29,7 @@ const AuthContextKey ContextKey = "authContext" // if authCtx == nil { // // Handle unauthorized request // } -// userID := authCtx.User.ID +// subject := authCtx.Subject() func GetAuthContext(ctx context.Context) *AuthContext { authCtx, ok := ctx.Value(AuthContextKey).(*AuthContext) if !ok { @@ -118,87 +39,79 @@ func GetAuthContext(ctx context.Context) *AuthContext { } // The methods below form a small, stable seam over the authenticated principal so -// that a future authz layer can depend on a narrow interface it defines itself +// that an authz layer can depend on a narrow interface it defines itself // (e.g. interface{ Roles() []string; Scopes() []string; Subject() string }) which -// *AuthContext satisfies structurally — rather than reaching into the concrete -// User/Client fields or branching on principal type. They are nil-safe. +// *AuthContext satisfies structurally. They are nil-safe. -// Type reports the principal type of the context: UserPrincipalType, -// ClientPrincipalType, or "" when unauthenticated. -func (a *AuthContext) Type() PrincipalType { - switch { - case a == nil: +// Subject returns the principal's JWT "sub" claim, or "" when unauthenticated. +func (a *AuthContext) Subject() string { + if a == nil { return "" - case a.User != nil: - return UserPrincipalType - case a.Client != nil: - return ClientPrincipalType - default: + } + return a.Principal.Subject +} + +// ClientID returns the OAuth2 client the principal's token was issued to, or "" +// when unauthenticated. +func (a *AuthContext) ClientID() string { + if a == nil { return "" } + return a.Principal.ClientID } -// Subject returns a stable identifier for the principal: the resolved user ID -// (falling back to the IdP user ID) for users, the client ID for clients, or "". -func (a *AuthContext) Subject() string { - switch { - case a == nil: +// Email returns the principal's "email" claim, or "" when absent or +// unauthenticated. +func (a *AuthContext) Email() string { + if a == nil { return "" - case a.User != nil: - if a.User.ID != "" { - return a.User.ID - } - return a.User.IDPUserID - case a.Client != nil: - return a.Client.ClientID - default: + } + return a.Principal.Email +} + +// OUID returns the principal's "ouId" claim, or "" when absent or +// unauthenticated. +func (a *AuthContext) OUID() string { + if a == nil { return "" } + return a.Principal.OUID } -// Roles returns the granted roles for the principal (user or client), or nil. +// OUHandle returns the principal's "ouHandle" claim, or "" when absent or +// unauthenticated. +func (a *AuthContext) OUHandle() string { + if a == nil { + return "" + } + return a.Principal.OUHandle +} + +// Roles returns the granted roles for the principal, or nil. func (a *AuthContext) Roles() []string { - switch { - case a == nil: - return nil - case a.User != nil: - return a.User.Roles - case a.Client != nil: - return a.Client.Roles - default: + if a == nil { return nil } + return a.Principal.Roles } -// Scopes returns the granted OAuth2 scopes for the principal (user or client), or nil. +// Scopes returns the granted OAuth2 scopes for the principal, or nil. func (a *AuthContext) Scopes() []string { - switch { - case a == nil: - return nil - case a.User != nil: - return a.User.Scopes - case a.Client != nil: - return a.Client.Scopes - default: + if a == nil { return nil } + return a.Principal.Scopes } -// ExtraClaims returns the consumer-declared extra claims for the principal -// (user or client), or nil. Nil-safe, and ExtraClaims' own methods are nil-safe -// too, so authCtx.ExtraClaims().String("email") never panics. +// ExtraClaims returns the principal's claims beyond the fixed schema, or nil. +// Nil-safe, and ExtraClaims' own methods are nil-safe too, so +// authCtx.ExtraClaims().String("email") never panics. // // Like Roles and Scopes, this returns the live value rather than a copy — do // not mutate it. func (a *AuthContext) ExtraClaims() ExtraClaims { - switch { - case a == nil: - return nil - case a.User != nil: - return a.User.ExtraClaims - case a.Client != nil: - return a.Client.ExtraClaims - default: + if a == nil { return nil } + return a.Principal.ExtraClaims } diff --git a/authn/context_test.go b/authn/context_test.go index 5c608d4d..6fedb487 100644 --- a/authn/context_test.go +++ b/authn/context_test.go @@ -5,34 +5,30 @@ package authn import ( "context" + "slices" "testing" ) // TestGetAuthContextFromRequest tests context retrieval. func TestGetAuthContext_FromRequest(t *testing.T) { - uc := &UserContext{ - ID: "persisted-user-id", - IDPUserID: testUserID, - Roles: []string{"exporter"}, + authCtx := &AuthContext{Principal: Principal{ + Subject: testUserID, + ClientID: testClientID, + Roles: []string{"exporter"}, ExtraClaims: ExtraClaims{ "email": testEmail, "ouId": testOUID, "ouHandle": testOUHandle, }, - } - authCtx := &AuthContext{User: uc} + }} ctx := context.WithValue(context.Background(), AuthContextKey, authCtx) retrieved := GetAuthContext(ctx) if retrieved == nil { - t.Error("expected to retrieve auth context") - return - } - if retrieved.User == nil { - t.Fatalf("expected user context to be set") + t.Fatal("expected to retrieve auth context") } - if retrieved.User.ID != "persisted-user-id" || retrieved.User.IDPUserID != testUserID { - t.Errorf("got user context %v", retrieved.User) + if retrieved.Subject() != testUserID || retrieved.ClientID() != testClientID { + t.Errorf("got principal %+v", retrieved.Principal) } } @@ -55,46 +51,50 @@ func TestGetAuthContext_WrongType(t *testing.T) { } } -// TestUserContext_JSONUnmarshaling tests UserContext structure. -func TestUserContext_Structure(t *testing.T) { - uc := &UserContext{ - ID: "persisted-user-id", - IDPUserID: testUserID, - Roles: []string{"exporter"}, - ExtraClaims: ExtraClaims{ - "email": testEmail, - "phone_number": testPhone, - "ouId": testOUID, - "ouHandle": testOUHandle, - }, - } +// TestAuthContext_Accessors locks in that every accessor reads straight +// through to the Principal and is nil-safe. +func TestAuthContext_Accessors(t *testing.T) { + authCtx := &AuthContext{Principal: Principal{ + Subject: testUserID, + ClientID: testClientID, + Roles: []string{"exporter"}, + Scopes: []string{"nsw:task:read"}, + Email: testEmail, + OUID: testOUID, + OUHandle: testOUHandle, + }} - if uc.ID != "persisted-user-id" { - t.Errorf("got user id %s, want persisted-user-id", uc.ID) + if got := authCtx.Email(); got != testEmail { + t.Errorf("Email() = %q, want %q", got, testEmail) + } + if got := authCtx.OUID(); got != testOUID { + t.Errorf("OUID() = %q, want %q", got, testOUID) } - if uc.IDPUserID != testUserID { - t.Errorf("got idp user id %s, want %s", uc.IDPUserID, testUserID) + if got := authCtx.OUHandle(); got != testOUHandle { + t.Errorf("OUHandle() = %q, want %q", got, testOUHandle) } - if uc.ExtraClaims.String("email") != testEmail { - t.Errorf("got email %s, want %s", uc.ExtraClaims.String("email"), testEmail) + if got := authCtx.Subject(); got != testUserID { + t.Errorf("Subject() = %q, want %q", got, testUserID) } - if uc.ExtraClaims.String("phone_number") != testPhone { - t.Errorf("got phone number %s, want %s", uc.ExtraClaims.String("phone_number"), testPhone) + if got := authCtx.ClientID(); got != testClientID { + t.Errorf("ClientID() = %q, want %q", got, testClientID) } - if uc.ExtraClaims.String("ouId") != testOUID { - t.Errorf("got ou id %s, want %s", uc.ExtraClaims.String("ouId"), testOUID) + if got := authCtx.Roles(); !slices.Equal(got, []string{"exporter"}) { + t.Errorf("Roles() = %v, want [exporter]", got) } - if uc.ExtraClaims.String("ouHandle") != testOUHandle { - t.Errorf("got ou handle %s, want %s", uc.ExtraClaims.String("ouHandle"), testOUHandle) + if got := authCtx.Scopes(); !slices.Equal(got, []string{"nsw:task:read"}) { + t.Errorf("Scopes() = %v, want [nsw:task:read]", got) } - if len(uc.Roles) != 1 || uc.Roles[0] != "exporter" { - t.Errorf("got roles %v, want [exporter]", uc.Roles) + + var nilCtx *AuthContext + if nilCtx.Subject() != "" || nilCtx.ClientID() != "" || nilCtx.Roles() != nil || nilCtx.Scopes() != nil || + nilCtx.Email() != "" || nilCtx.OUID() != "" || nilCtx.OUHandle() != "" { + t.Error("expected zero values from a nil AuthContext") } } // TestAuthContext_ExtraClaims completes the accessor seam: consumers should be -// able to read extra claims without branching on principal type or nil-checking -// two levels. +// able to read extra claims without nil-checking two levels. func TestAuthContext_ExtraClaims(t *testing.T) { tests := []struct { name string @@ -103,9 +103,8 @@ func TestAuthContext_ExtraClaims(t *testing.T) { }{ {"nil receiver", nil, ""}, {"empty context", &AuthContext{}, ""}, - {"user principal", &AuthContext{User: &UserContext{ExtraClaims: ExtraClaims{"email": testEmail}}}, testEmail}, - {"client principal", &AuthContext{Client: &ClientContext{ExtraClaims: ExtraClaims{"email": "svc@example.com"}}}, "svc@example.com"}, - {"user with nil claims", &AuthContext{User: &UserContext{}}, ""}, + {"principal with claims", &AuthContext{Principal: Principal{ExtraClaims: ExtraClaims{"email": testEmail}}}, testEmail}, + {"principal with nil claims", &AuthContext{Principal: Principal{Subject: testUserID}}, ""}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/authn/extra_claims.go b/authn/extra_claims.go index 989df908..be1289a4 100644 --- a/authn/extra_claims.go +++ b/authn/extra_claims.go @@ -7,15 +7,15 @@ import ( "encoding/base64" "encoding/json" "fmt" - "log/slog" - "maps" "slices" "strings" ) -// ExtraClaims holds JWT claims outside authn's fixed schema, declared -// explicitly by a consumer via WithUserClaims (user principals) or -// WithClientClaims (client principals). +// ExtraClaims holds every claim in a validated token outside authn's fixed +// schema (sub, client_id, scope, the roles claim, and the JWT registered +// claims), which already have dedicated Principal fields or are consumed by +// validation. Nothing has to be declared: whatever the IdP puts in the token +// is here. Deciding which of these a request needs is the application's job. // // Values are the claim's JSON-decoded Go representation (string, []any, // float64, bool, map[string]any), with one normalization: every JSON string a @@ -99,7 +99,7 @@ func decodeJWTPayload(tokenString string) (map[string]any, error) { // // Everything else counts as present — including an empty array, false, and 0 — // so this is a test for "the IdP said nothing here", not a test for -// usefulness. Required claims apply requiredClaimValue on top. +// usefulness. func claimAbsent(value any) bool { switch v := value.(type) { case nil: @@ -136,95 +136,35 @@ func normalizeClaimValue(value any) any { } } -// requiredClaimValue reports whether value satisfies the shape a REQUIRED claim -// must have, returning it normalized. +// extractExtraClaims returns every claim in an already-decoded payload that is +// not part of the fixed schema, normalized. Absent-valued claims (JSON null, or +// a blank string) are dropped, so they read the same as claims the token never +// carried. // -// The shape is deliberately narrow: a non-blank JSON string, or a non-empty -// array whose every element is a non-blank JSON string. A required declaration -// is a consumer asserting "my application cannot run without this attribute"; -// a number, boolean, object, empty array or mixed array cannot satisfy that, -// and accepting one only defers the failure to the code that indexes the claim -// with String/Strings and silently gets "" or nil. -func requiredClaimValue(value any) (any, bool) { - switch v := value.(type) { - case string: - s := strings.TrimSpace(v) - return s, s != "" - case []any: - if len(v) == 0 { - return nil, false - } - out := make([]any, len(v)) - for i, item := range v { - s, ok := item.(string) - if !ok { - return nil, false - } - if s = strings.TrimSpace(s); s == "" { - return nil, false - } - out[i] = s - } - return out, true - default: - return nil, false - } -} - -// extractDeclared pulls the claims named in declared (name -> required) out of -// an already-decoded payload. Names are matched exactly: JWT claim names are -// case-sensitive (RFC 7519 §4), unlike the struct-tag binding the fixed schema -// relies on. Names are walked in sorted order, so a token violating two -// required claims always fails on the same one. -func extractDeclared(payload map[string]any, declared map[string]bool) (ExtraClaims, error) { - // nil rather than an empty map, so "nothing declared" reads the same way - // whether or not a custom roles claim forced the payload decode. - if len(declared) == 0 { - return nil, nil - } - extra := make(ExtraClaims, len(declared)) - for _, name := range slices.Sorted(maps.Keys(declared)) { - required := declared[name] - value, present := payload[name] - - if !present || claimAbsent(value) { - if required { - // Claim NAMES only, never values: these errors reach the - // middleware's Debug log with request context, and email / - // phone_number are exactly the claims people mark required. - return nil, fmt.Errorf("jwt missing or empty required claim %q", name) - } +// The fixed-schema exclusion folds case: encoding/json binds struct tags +// case-insensitively, so a payload key of "Roles" or "SUB" already landed in a +// fixed field and must not surface a second time under ExtraClaims. +func extractExtraClaims(payload map[string]any) ExtraClaims { + var extra ExtraClaims + for name, value := range payload { + if fixedSchemaClaims[foldClaimName(name)] || claimAbsent(value) { continue } - - if required { - normalized, ok := requiredClaimValue(value) - if !ok { - return nil, fmt.Errorf("jwt required claim %q must be a non-empty string or a non-empty array of non-empty strings", name) - } - extra[name] = normalized - continue + if extra == nil { + extra = make(ExtraClaims) } extra[name] = normalizeClaimValue(value) } - return extra, nil + return extra } -// resolveClaims produces the two payload-derived pieces of a principal that -// depend on consumer configuration — the roles slice and the declared extra -// claims — sharing a single payload decode between them. -// -// When nothing is declared and roles come from the default "roles" claim (every -// consumer that has not opted in), there is nothing to read that -// jwt.ParseWithClaims has not already bound, so no second decode happens. -func (te *TokenExtractor) resolveClaims(tokenString string, claims *tokenClaims, declared map[string]bool) (ExtraClaims, []string, error) { - // The "" guard matters: a TokenExtractor built as a struct literal (only - // possible in-package) has no rolesClaim and must not read as "custom". - custom := te.rolesClaim != "" && te.rolesClaim != defaultRolesClaim - - if len(declared) == 0 && !custom { - roles, err := parseRolesClaim(defaultRolesClaim, claims.Roles.value) - return nil, roles, err +// resolveClaims produces the roles slice and the extra claims of a principal. +// The extra claims need a second decode of the payload, since tokenClaims only +// binds the fixed schema. +func resolveClaims(tokenString string, claims *tokenClaims) (ExtraClaims, []string, error) { + roles, err := parseRolesClaim(claims.Roles.value) + if err != nil { + return nil, nil, err } payload, err := decodeJWTPayload(tokenString) @@ -232,21 +172,5 @@ func (te *TokenExtractor) resolveClaims(tokenString string, claims *tokenClaims, return nil, nil, fmt.Errorf("failed to decode token payload for claim extraction: %w", err) } - rolesName, rolesRaw := defaultRolesClaim, claims.Roles.value - if custom { - rolesName, rolesRaw = te.rolesClaim, payload[te.rolesClaim] - if rolesRaw == nil { - slog.Debug("configured roles claim absent from token", "claim", rolesName) - } - } - roles, err := parseRolesClaim(rolesName, rolesRaw) - if err != nil { - return nil, nil, err - } - - extra, err := extractDeclared(payload, declared) - if err != nil { - return nil, nil, err - } - return extra, roles, nil + return extractExtraClaims(payload), roles, nil } diff --git a/authn/extra_claims_test.go b/authn/extra_claims_test.go index fbe924fa..cb798ae1 100644 --- a/authn/extra_claims_test.go +++ b/authn/extra_claims_test.go @@ -95,209 +95,17 @@ func TestExtraClaims_StringsNilVsEmpty(t *testing.T) { } } -func TestUserClaims_Optional(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithUserClaims(ClaimSpec{Optional: []string{"email"}})) +// TestExtraClaims_PassThrough locks in that every non-fixed claim reaches +// ExtraClaims without being declared, in its decoded shape. +func TestExtraClaims_PassThrough(t *testing.T) { + extractor, privateKey, cleanup := newTokenExtractor(t) defer cleanup() - t.Run("present is extracted", func(t *testing.T) { - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["email"] = testEmail - p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err != nil { - t.Fatalf("extract: %v", err) - } - if got := p.UserPrincipal.ExtraClaims.String("email"); got != testEmail { - t.Fatalf("email = %q, want %q", got, testEmail) - } - }) - - t.Run("absent is skipped, no error", func(t *testing.T) { - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err != nil { - t.Fatalf("extract: %v", err) - } - if got := p.UserPrincipal.ExtraClaims.String("email"); got != "" { - t.Fatalf("expected no email, got %q", got) - } - }) - - t.Run("empty string is treated as absent", func(t *testing.T) { - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["email"] = " " - p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err != nil { - t.Fatalf("extract: %v", err) - } - if got := p.UserPrincipal.ExtraClaims.String("email"); got != "" { - t.Fatalf("expected no email, got %q", got) - } - }) -} - -func TestUserClaims_Required(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithUserClaims(ClaimSpec{Required: []string{"email"}})) - defer cleanup() - - t.Run("present succeeds", func(t *testing.T) { - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["email"] = testEmail - if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)); err != nil { - t.Fatalf("extract: %v", err) - } - }) - - t.Run("absent rejects the token", func(t *testing.T) { - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err == nil || !strings.Contains(err.Error(), `missing or empty required claim "email"`) { - t.Fatalf("expected required-claim error, got %v", err) - } - }) - - t.Run("empty value rejects the token", func(t *testing.T) { - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["email"] = "" - if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)); err == nil { - t.Fatalf("expected error for empty required claim") - } - }) -} - -// TestRequiredClaims_RejectUnusableShapes covers the gap the old -// presence-only gate left: a "required" claim carrying a number, bool, object, -// empty array or mixed array satisfied the check and then read back as "" via -// String, silently defeating the consumer's own assertion. The pre-generalized -// code rejected these because the fields were typed *string. -func TestRequiredClaims_RejectUnusableShapes(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithUserClaims(ClaimSpec{Required: []string{"ouId"}})) - defer cleanup() - - rejected := map[string]any{ - "number": 42, - "bool": true, - "zero": 0, - "empty array": []any{}, - "empty object": map[string]any{}, - "mixed array": []any{"a", 1}, - "array of blanks": []any{" "}, - "array with a blank": []any{"a", ""}, - "nested object value": map[string]any{"value": "OU-1"}, - } - for name, value := range rejected { - t.Run(name, func(t *testing.T) { - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["ouId"] = value - _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err == nil { - t.Fatalf("expected required claim %v (%T) to be rejected", value, value) - } - if strings.Contains(err.Error(), "OU-1") { - t.Fatalf("claim value leaked into the error: %v", err) - } - }) - } - - t.Run("non-empty string array is accepted", func(t *testing.T) { - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["ouId"] = []any{"OU-1", "OU-2"} - p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err != nil { - t.Fatalf("extract: %v", err) - } - if got := p.UserPrincipal.ExtraClaims.Strings("ouId"); len(got) != 2 || got[0] != "OU-1" { - t.Fatalf("ouId = %#v, want [OU-1 OU-2]", got) - } - }) -} - -// TestRequiredClaims_FailureIsDeterministic pins extractDeclared's sorted -// walk. Ranging a map directly would report an arbitrary one of several unmet -// required claims, making the startup/rejection message vary run to run. -func TestRequiredClaims_FailureIsDeterministic(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, - WithUserClaims(ClaimSpec{Required: []string{"zulu", "alpha", "mike"}})) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID // none of the three required claims are present - - var first string - for i := range 8 { - _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err == nil { - t.Fatalf("expected required-claim error") - } - if i == 0 { - first = err.Error() - continue - } - if err.Error() != first { - t.Fatalf("rejection message is not deterministic:\n %q\n %q", first, err.Error()) - } - } - if !strings.Contains(first, `"alpha"`) { - t.Fatalf("expected the first name in sorted order, got %q", first) - } -} - -// TestClaims_JSONNullIsAbsent pins the documented contract for a JSON-null -// claim: unlike an empty array or false, null means "the IdP said nothing -// here", so an optional declaration skips it and a required one rejects. -func TestClaims_JSONNullIsAbsent(t *testing.T) { - t.Run("optional null is skipped", func(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithUserClaims(ClaimSpec{Optional: []string{"email"}})) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["email"] = nil - - p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err != nil { - t.Fatalf("extract: %v", err) - } - if _, present := p.UserPrincipal.ExtraClaims["email"]; present { - t.Fatalf("expected a null claim to be skipped, got %#v", p.UserPrincipal.ExtraClaims) - } - }) - - t.Run("required null rejects the token", func(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithUserClaims(ClaimSpec{Required: []string{"email"}})) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["email"] = nil - - _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err == nil || !strings.Contains(err.Error(), `missing or empty required claim "email"`) { - t.Fatalf("expected required-claim error, got %v", err) - } - }) -} - -// TestOptionalClaims_PreserveNonStringShapes is the counterpart to -// TestRequiredClaims_RejectUnusableShapes: optional declarations stay -// deliberately permissive, and the value reaches the consumer exactly as -// decoded so it can index the map directly for shapes String/Strings do not -// cover. It also pins that normalization is shallow — nested strings are not -// rewritten. -func TestOptionalClaims_PreserveNonStringShapes(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, - WithUserClaims(ClaimSpec{Optional: []string{"count", "flag", "meta", "mixed", "empty"}})) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID + claims := newBaseClaims() + claims["phone_number"] = testPhone + claims["given_name"] = "Trader" + claims["grant_type"] = "client_credentials" + claims["https://app.example.com/department"] = "customs" claims["count"] = 42 claims["flag"] = true claims["meta"] = map[string]any{"nested": " padded "} @@ -308,8 +116,18 @@ func TestOptionalClaims_PreserveNonStringShapes(t *testing.T) { if err != nil { t.Fatalf("extract: %v", err) } - extra := p.UserPrincipal.ExtraClaims + extra := p.ExtraClaims + for name, want := range map[string]string{ + "phone_number": testPhone, + "given_name": "Trader", + "grant_type": "client_credentials", + "https://app.example.com/department": "customs", + } { + if got := extra.String(name); got != want { + t.Errorf("%s = %q, want %q", name, got, want) + } + } if got := extra["count"]; got != 42.0 { t.Errorf("count = %#v, want 42.0", got) } @@ -330,187 +148,82 @@ func TestOptionalClaims_PreserveNonStringShapes(t *testing.T) { } } -// TestExtraClaims_ValuesAreTrimmed pins the normalization: emptiness was -// already judged on the trimmed string, so storing the untrimmed one meant a -// padded value reached UserProfileService and any equality comparison there. -func TestExtraClaims_ValuesAreTrimmed(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, - WithUserClaims(ClaimSpec{Optional: []string{"email", "groups"}, Required: []string{"ouId"}})) +// TestExtraClaims_ExcludesFixedSchema locks in that each value has one home: +// a claim authn binds itself never reappears in ExtraClaims, including case +// variants that encoding/json bound to the fixed field anyway. +func TestExtraClaims_ExcludesFixedSchema(t *testing.T) { + extractor, privateKey, cleanup := newTokenExtractor(t) defer cleanup() - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["email"] = " trader@example.com \n" - claims["ouId"] = " OU-001 " - claims["groups"] = []any{" a ", "b "} + claims := newBaseClaims() + claims["jti"] = "token-1" + claims["scope"] = "nsw:task:read" + claims["roles"] = []string{"exporter"} + claims["email"] = testEmail + claims["ouId"] = testOUID + claims["ouHandle"] = testOUHandle + claims["phone_number"] = testPhone p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) if err != nil { t.Fatalf("extract: %v", err) } - extra := p.UserPrincipal.ExtraClaims - if got := extra.String("email"); got != testEmail { - t.Fatalf("email = %q, want %q", got, testEmail) - } - if got := extra.String("ouId"); got != testOUID { - t.Fatalf("ouId = %q, want %q", got, testOUID) - } - if got := extra.Strings("groups"); len(got) != 2 || got[0] != "a" || got[1] != "b" { - t.Fatalf("groups = %#v, want [a b]", got) - } -} - -func TestClaims_RequiredWinsRegardlessOfDeclarationOrder(t *testing.T) { - orders := []struct { - name string - opts []Option - }{ - {"required then optional", []Option{ - WithUserClaims(ClaimSpec{Required: []string{"email"}}), - WithUserClaims(ClaimSpec{Optional: []string{"email"}}), - }}, - {"optional then required", []Option{ - WithUserClaims(ClaimSpec{Optional: []string{"email"}}), - WithUserClaims(ClaimSpec{Required: []string{"email"}}), - }}, - {"both in one spec", []Option{ - WithUserClaims(ClaimSpec{Optional: []string{"email"}, Required: []string{"email"}}), - }}, + if len(p.ExtraClaims) != 1 || p.ExtraClaims.String("phone_number") != testPhone { + t.Fatalf("ExtraClaims = %#v, want only phone_number", p.ExtraClaims) } - for _, tt := range orders { - t.Run(tt.name, func(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, tt.opts...) - defer cleanup() - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)); err == nil { - t.Fatalf("expected required-claim error regardless of declaration order") - } - }) - } -} + t.Run("case variants", func(t *testing.T) { + claims := newBaseClaims() + claims["Roles"] = []string{"exporter"} + claims["SCOPE"] = "nsw:task:read" + claims["OUID"] = testOUID -func TestClaims_UserAndClientDeclarationsAreIsolated(t *testing.T) { - t.Run("user-scoped required claim does not reject client tokens", func(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithUserClaims(ClaimSpec{Required: []string{"employee_id"}})) - defer cleanup() - - claims := newBaseClaims(ClientCredentialsGrant) - claims["sub"] = "SOME_CLIENT" - if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)); err != nil { - t.Fatalf("expected client token to pass despite unmet user-scoped required claim: %v", err) + p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) + if err != nil { + t.Fatalf("extract: %v", err) } - }) - - t.Run("client-scoped required claim does not reject user tokens", func(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithClientClaims(ClaimSpec{Required: []string{"cost_center"}})) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)); err != nil { - t.Fatalf("expected user token to pass despite unmet client-scoped required claim: %v", err) + if len(p.ExtraClaims) != 0 { + t.Fatalf("ExtraClaims = %#v, want none", p.ExtraClaims) } }) } -func TestClientClaims_Optional(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithClientClaims(ClaimSpec{Optional: []string{"department"}})) +func TestExtraClaims_AbsentValuesAreDropped(t *testing.T) { + extractor, privateKey, cleanup := newTokenExtractor(t) defer cleanup() - claims := newBaseClaims(ClientCredentialsGrant) - claims["sub"] = "SOME_CLIENT" - claims["department"] = "customs" + claims := newBaseClaims() + claims["email"] = nil + claims["ouId"] = " " + p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) if err != nil { t.Fatalf("extract: %v", err) } - if got := p.ClientPrincipal.ExtraClaims.String("department"); got != "customs" { - t.Fatalf("department = %q, want customs", got) + if p.ExtraClaims != nil { + t.Fatalf("ExtraClaims = %#v, want nil", p.ExtraClaims) } } -func TestClientClaims_Required(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithClientClaims(ClaimSpec{Required: []string{"department"}})) +// TestExtraClaims_ValuesAreTrimmed pins the normalization: a padded value +// would otherwise fail any equality comparison a consumer makes. +func TestExtraClaims_ValuesAreTrimmed(t *testing.T) { + extractor, privateKey, cleanup := newTokenExtractor(t) defer cleanup() - claims := newBaseClaims(ClientCredentialsGrant) - claims["sub"] = "SOME_CLIENT" - if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)); err == nil { - t.Fatalf("expected error for missing required client claim") - } -} + claims := newBaseClaims() + claims["phone_number"] = " " + testPhone + " \n" + claims["groups"] = []any{" a ", "b "} -func TestNewTokenExtractor_RejectsFixedSchemaClaimNames(t *testing.T) { - // Case variants matter: encoding/json matches struct tags - // case-insensitively, so "Roles" still lands in tokenClaims.Roles even - // though a case-sensitive guard would wave it through. - names := []string{ - "client_id", "grant_type", "scope", "roles", "sub", "iss", "aud", "exp", - "Roles", "SCOPE", "Client_Id", "SUB", + p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) + if err != nil { + t.Fatalf("extract: %v", err) } - for _, name := range names { - t.Run(name+"/optional", func(t *testing.T) { - if _, err := NewTokenExtractor("https://localhost/jwks", testIssuer, testClientID, WithUserClaims(ClaimSpec{Optional: []string{name}})); err == nil { - t.Fatalf("expected error declaring fixed-schema claim %q", name) - } - }) - t.Run(name+"/required", func(t *testing.T) { - if _, err := NewTokenExtractor("https://localhost/jwks", testIssuer, testClientID, WithUserClaims(ClaimSpec{Required: []string{name}})); err == nil { - t.Fatalf("expected error declaring fixed-schema claim %q", name) - } - }) + extra := p.ExtraClaims + if got := extra.String("phone_number"); got != testPhone { + t.Fatalf("phone_number = %q, want %q", got, testPhone) } - - t.Run("client variant is also rejected", func(t *testing.T) { - if _, err := NewTokenExtractor("https://localhost/jwks", testIssuer, testClientID, WithClientClaims(ClaimSpec{Required: []string{"scope"}})); err == nil { - t.Fatalf("expected error declaring fixed-schema claim via client option") - } - }) - - t.Run("blank name is rejected rather than silently dropped", func(t *testing.T) { - // The natural result of strings.Split on an env var with a trailing - // comma. Dropping it silently means an enforcement rule the caller - // asked for never applies. - if _, err := NewTokenExtractor("https://localhost/jwks", testIssuer, testClientID, WithUserClaims(ClaimSpec{Required: []string{"email", ""}})); err == nil { - t.Fatalf("expected error for a blank claim name") - } - }) - - t.Run("collision error names every offender deterministically", func(t *testing.T) { - var first string - for i := range 8 { - _, err := NewTokenExtractor("https://localhost/jwks", testIssuer, testClientID, - WithUserClaims(ClaimSpec{Optional: []string{"scope", "roles", "sub"}})) - if err == nil { - t.Fatalf("expected error") - } - if i == 0 { - first = err.Error() - continue - } - if err.Error() != first { - t.Fatalf("error message is not deterministic:\n %q\n %q", first, err.Error()) - } - } - for _, want := range []string{"scope", "roles", "sub"} { - if !strings.Contains(first, want) { - t.Fatalf("error %q does not mention %q", first, want) - } - } - }) -} - -// TestNewTokenExtractor_AllowsFormerlyFixedClaimNames locks in the schema -// shrink: email/phone_number/ouId/ouHandle used to be hardcoded fixed-schema -// fields and are now legitimate extra-claim declarations. -func TestNewTokenExtractor_AllowsFormerlyFixedClaimNames(t *testing.T) { - for _, name := range []string{"email", "phone_number", "ouId", "ouHandle", "given_name"} { - t.Run(name, func(t *testing.T) { - if _, err := NewTokenExtractor("https://localhost/jwks", testIssuer, testClientID, WithUserClaims(ClaimSpec{Optional: []string{name}})); err != nil { - t.Fatalf("expected %q to be a legitimate extra claim, got error: %v", name, err) - } - }) + if got := extra.Strings("groups"); len(got) != 2 || got[0] != "a" || got[1] != "b" { + t.Fatalf("groups = %#v, want [a b]", got) } } diff --git a/authn/fixed_schema.go b/authn/fixed_schema.go index 270e2521..76db8baf 100644 --- a/authn/fixed_schema.go +++ b/authn/fixed_schema.go @@ -6,34 +6,31 @@ package authn import ( "encoding/json" "fmt" - "slices" "strings" ) -// defaultRolesClaim is the claim name authn reads roles from unless a consumer -// points it elsewhere with WithRolesClaim. It must stay in sync with the json -// tag on tokenClaims.Roles. -const defaultRolesClaim = "roles" +// rolesClaim is the claim authn reads roles from. It must stay in sync with the +// json tag on tokenClaims.Roles. +const rolesClaim = "roles" -// fixedSchemaClaims guards against a consumer redeclaring a claim name that is -// already part of authn's fixed schema as an extra claim. +// fixedSchemaClaims lists the claims authn binds itself, which are therefore +// kept out of ExtraClaims so each value has exactly one home. // -// This is purely a mechanical name-collision guard, not a "standard claims" -// allowlist: being in this map means the claim already has a dedicated field -// (or, for sub/client_id/grant_type, drives the package's own dispatch logic), -// not that it's inherently more "generic" than a claim that isn't. It should -// only grow when a claim actually graduates from "extra" to "fixed" — -// demonstrated load-bearing across multiple independent consumers, the same -// way ouId/ouHandle/email/phone_number were evaluated and found not to -// qualify — never pre-emptively. +// Being in this map means the claim already has a dedicated Principal field or +// is consumed by validation, not that it's inherently more "generic" than a +// claim that isn't. It should only grow when a claim graduates from "extra" to +// "fixed" because consumers depend on it, never pre-emptively. // // Every key MUST be lowercase, because lookups fold the candidate name (see // foldClaimName). TestFixedSchemaClaims_KeysAreFolded enforces that. var fixedSchemaClaims = map[string]bool{ - "client_id": true, - "grant_type": true, - "scope": true, - defaultRolesClaim: true, + "client_id": true, + "scope": true, + rolesClaim: true, + // Standard identity claims (see ClaimsConfig). + "email": true, + "ouid": true, + "ouhandle": true, // JWT registered claims (RFC 7519 §4.1), always present via jwt.RegisteredClaims. "iss": true, "sub": true, @@ -44,84 +41,21 @@ var fixedSchemaClaims = map[string]bool{ "jti": true, } -// foldClaimName normalizes a claim name for fixed-schema collision checks only. +// foldClaimName normalizes a claim name for fixed-schema membership checks only. // -// JWT claim names are case-sensitive (RFC 7519 §4) and extraction matches them -// exactly, so this is deliberately NOT applied when reading a payload. The -// collision guard has to fold because the fixed schema is bound by -// encoding/json, whose struct-tag matching IS case-insensitive: without -// folding, declaring "Roles" or "Client_Id" passes the guard while the very -// same payload key still lands in tokenClaims.Roles / tokenClaims.ClientID. +// JWT claim names are case-sensitive (RFC 7519 §4), so this is deliberately +// NOT applied to the names stored in ExtraClaims. Membership checks have to +// fold because the fixed schema is bound by encoding/json, whose struct-tag +// matching IS case-insensitive: a payload key of "Roles" or "Client_Id" lands +// in tokenClaims.Roles / tokenClaims.ClientID just as the lowercase key would. func foldClaimName(name string) string { return strings.ToLower(strings.TrimSpace(name)) } -// validateClaimNames reports every problem with one set of declared extra-claim -// names at once — blank names, and names already bound by authn — so a -// consumer fixes all of them in one pass instead of one per restart. what names -// the declaration site (e.g. "UserClaims.Required") and appears in the error. -// -// rolesClaim is reserved alongside the fixed schema: once roles are read from -// "groups", that name has a dedicated field (Principal.Roles) and declaring it -// as an extra claim would surface one claim under two meanings. -func validateClaimNames(what string, names []string, rolesClaim string) error { - var blank bool - var reserved []string - - for _, name := range names { - trimmed := strings.TrimSpace(name) - if trimmed == "" { - blank = true - continue - } - folded := foldClaimName(trimmed) - if fixedSchemaClaims[folded] || folded == foldClaimName(rolesClaim) { - reserved = append(reserved, trimmed) - } - } - - if blank { - return fmt.Errorf("%s contains an empty claim name", what) - } - if len(reserved) > 0 { - // Sorted and de-duplicated so the message is identical run to run; - // callers hit this at startup and may assert on it in tests. - slices.Sort(reserved) - reserved = slices.Compact(reserved) - return fmt.Errorf( - "%s declares claim(s) %v that authn already binds (fixed schema: client_id, grant_type, scope, %s, and the JWT registered claims)", - what, reserved, rolesClaim, - ) - } - return nil -} - -// validateRolesClaimName reports whether name is usable as the roles claim. -// Any case-spelling of "roles" is fine — it IS the roles claim. Any other -// fixed-schema name is not: it already feeds a different field, and pointing -// roles at it would bind one claim to two meanings (WithRolesClaim("scope") -// would try to read the space-delimited scope string as a roles array and -// reject every token). -func validateRolesClaimName(name string) error { - trimmed := strings.TrimSpace(name) - if trimmed == "" { - return fmt.Errorf("roles claim name is empty") - } - if folded := foldClaimName(trimmed); folded != defaultRolesClaim && fixedSchemaClaims[folded] { - return fmt.Errorf("roles claim %q is part of authn's fixed schema and already feeds a different field", trimmed) - } - return nil -} - // rolesValue captures the raw "roles" claim without imposing a shape at JSON -// decode time. -// -// Binding the field as []string would make the SHAPE of the literal "roles" -// claim a hard token-validity condition even for consumers who moved roles -// elsewhere with WithRolesClaim: struct tags are static, so a token carrying -// `"roles": "legacy-string"` would be rejected by encoding/json before authn -// ever looked at the configured claim. Capturing the raw value defers that -// decision to parseRolesClaim, which applies it only to the claim in force. +// decode time, so parseRolesClaim can judge it: binding the field as []string +// would reject a wrong-shaped claim with an opaque encoding/json error, and +// would leave role strings untrimmed. type rolesValue struct{ value any } func (r *rolesValue) UnmarshalJSON(data []byte) error { @@ -144,9 +78,8 @@ func (r *rolesValue) UnmarshalJSON(data []byte) error { // Rejecting rather than ignoring a wrong-shaped value is also deliberate: roles // feed authz.Principal, and a silently empty roles slice reads as "granted // nothing" — which fails closed for a require-role check, but fails OPEN for -// any check phrased as "deny if the principal has role X". A mistyped -// WithRolesClaim must fail loudly on the first request. -func parseRolesClaim(name string, value any) ([]string, error) { +// any check phrased as "deny if the principal has role X". +func parseRolesClaim(value any) ([]string, error) { switch v := value.(type) { case nil: return nil, nil @@ -155,12 +88,12 @@ func parseRolesClaim(name string, value any) ([]string, error) { for _, item := range v { role, ok := item.(string) if !ok { - return nil, fmt.Errorf("jwt claim %q must be an array of strings", name) + return nil, fmt.Errorf("jwt claim %q must be an array of strings", rolesClaim) } roles = append(roles, strings.TrimSpace(role)) } return roles, nil default: - return nil, fmt.Errorf("jwt claim %q must be an array of strings", name) + return nil, fmt.Errorf("jwt claim %q must be an array of strings", rolesClaim) } } diff --git a/authn/fixed_schema_test.go b/authn/fixed_schema_test.go index 7503d4ad..5607ba89 100644 --- a/authn/fixed_schema_test.go +++ b/authn/fixed_schema_test.go @@ -5,7 +5,6 @@ package authn import ( "encoding/json" - "strings" "testing" ) @@ -26,12 +25,12 @@ func TestFixedSchemaClaims_KeysAreFolded(t *testing.T) { // even though a case-sensitive guard would not recognise them. func TestTokenClaims_JSONBindingIsCaseInsensitive(t *testing.T) { var c tokenClaims - raw := `{"Roles":["admin"],"Client_ID":"evil","GRANT_TYPE":"client_credentials","SCOPE":"a b"}` + raw := `{"Roles":["admin"],"Client_ID":"evil","SCOPE":"a b"}` if err := json.Unmarshal([]byte(raw), &c); err != nil { t.Fatalf("unmarshal: %v", err) } - roles, err := parseRolesClaim(defaultRolesClaim, c.Roles.value) + roles, err := parseRolesClaim(c.Roles.value) if err != nil { t.Fatalf("parseRolesClaim: %v", err) } @@ -41,9 +40,6 @@ func TestTokenClaims_JSONBindingIsCaseInsensitive(t *testing.T) { if c.ClientID != "evil" { t.Fatalf(`"Client_ID" did not bind to tokenClaims.ClientID: %q`, c.ClientID) } - if c.GrantType != ClientCredentialsGrant { - t.Fatalf(`"GRANT_TYPE" did not bind to tokenClaims.GrantType: %q`, c.GrantType) - } if len(c.Scopes) != 2 { t.Fatalf(`"SCOPE" did not bind to tokenClaims.Scopes: %#v`, c.Scopes) } @@ -71,7 +67,7 @@ func TestParseRolesClaim(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := parseRolesClaim("roles", tt.value) + got, err := parseRolesClaim(tt.value) if tt.wantErr { if err == nil { t.Fatalf("expected error, got %#v", got) @@ -91,180 +87,30 @@ func TestParseRolesClaim(t *testing.T) { } } +// TestRolesClaim covers roles at the token level: parseRolesClaim's shape rules +// (see TestParseRolesClaim) decide whether the whole token is accepted. func TestRolesClaim(t *testing.T) { - t.Run("custom claim populates roles", func(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithRolesClaim("groups")) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["groups"] = []string{"exporter", "broker"} - // The literal "roles" claim is ignored once roles move elsewhere. - claims["roles"] = []string{"should-be-ignored"} - - p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err != nil { - t.Fatalf("extract: %v", err) - } - if got := p.UserPrincipal.Roles; len(got) != 2 || got[0] != "exporter" || got[1] != "broker" { - t.Fatalf("roles = %#v, want [exporter broker]", got) - } - }) - - t.Run("applies to client principals too", func(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithRolesClaim("groups")) - defer cleanup() - - claims := newBaseClaims(ClientCredentialsGrant) - claims["sub"] = "SOME_CLIENT" - claims["groups"] = []string{"ingest"} - - p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err != nil { - t.Fatalf("extract: %v", err) - } - if got := p.ClientPrincipal.Roles; len(got) != 1 || got[0] != "ingest" { - t.Fatalf("roles = %#v, want [ingest]", got) - } - }) - - t.Run("absent custom claim yields no roles, not an error", func(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithRolesClaim("groups")) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID + extractor, privateKey, cleanup := newTokenExtractor(t) + defer cleanup() - p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) + t.Run("absent claim yields no roles, not an error", func(t *testing.T) { + p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, newBaseClaims())) if err != nil { - t.Fatalf("expected a user with no roles to authenticate, got %v", err) + t.Fatalf("expected a caller with no roles to authenticate, got %v", err) } - if len(p.UserPrincipal.Roles) != 0 { - t.Fatalf("roles = %#v, want none", p.UserPrincipal.Roles) + if p.Roles != nil { + t.Fatalf("roles = %#v, want nil", p.Roles) } }) - t.Run("wrong-shaped custom claim rejects the token", func(t *testing.T) { + t.Run("wrong shape rejects the token", func(t *testing.T) { // A silently empty Roles reads as "granted nothing", which fails OPEN // for any check phrased as "deny if the principal has role X". - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithRolesClaim("groups")) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["groups"] = "exporter broker" + claims := newBaseClaims() + claims["roles"] = "exporter broker" if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)); err == nil { t.Fatalf("expected a space-delimited roles string to be rejected") } }) - - t.Run("remapping decouples from the literal roles claim", func(t *testing.T) { - // With roles bound as []string this token was rejected by - // encoding/json before the configured claim was ever read. - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithRolesClaim("groups")) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["groups"] = []string{"exporter"} - claims["roles"] = "legacy-string" - - p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err != nil { - t.Fatalf("expected the unused roles claim's shape to be irrelevant, got %v", err) - } - if got := p.UserPrincipal.Roles; len(got) != 1 || got[0] != "exporter" { - t.Fatalf("roles = %#v, want [exporter]", got) - } - }) - - t.Run("default claim still rejects a wrong shape", func(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractor(t) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["roles"] = "exporter" - - if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)); err == nil { - t.Fatalf("expected a bare-string roles claim to be rejected") - } - }) -} - -func TestRolesClaim_NameValidation(t *testing.T) { - construct := func(opts ...Option) error { - _, err := NewTokenExtractor("https://localhost/jwks", testIssuer, testClientID, opts...) - return err - } - - t.Run("any case of roles is accepted", func(t *testing.T) { - for _, name := range []string{"roles", "Roles", "ROLES"} { - if err := construct(WithRolesClaim(name)); err != nil { - t.Fatalf("WithRolesClaim(%q): %v", name, err) - } - } - }) - - t.Run("other fixed-schema names are rejected", func(t *testing.T) { - for _, name := range []string{"scope", "sub", "client_id", "SCOPE", "exp"} { - if err := construct(WithRolesClaim(name)); err == nil { - t.Fatalf("expected WithRolesClaim(%q) to be rejected", name) - } - } - }) - - t.Run("blank is rejected", func(t *testing.T) { - for _, name := range []string{"", " "} { - if err := construct(WithRolesClaim(name)); err == nil { - t.Fatalf("expected WithRolesClaim(%q) to be rejected", name) - } - } - }) - - t.Run("namespaced name is accepted verbatim", func(t *testing.T) { - if err := construct(WithRolesClaim("https://app.example.com/roles")); err != nil { - t.Fatalf("expected a namespaced claim name to be accepted, got %v", err) - } - }) - - t.Run("cannot also be declared as an extra claim, in either option order", func(t *testing.T) { - orders := map[string][]Option{ - "roles first": {WithRolesClaim("groups"), WithUserClaims(ClaimSpec{Optional: []string{"groups"}})}, - "claims first": {WithUserClaims(ClaimSpec{Optional: []string{"groups"}}), WithRolesClaim("groups")}, - "client claims": {WithRolesClaim("groups"), WithClientClaims(ClaimSpec{Required: []string{"groups"}})}, - } - for name, opts := range orders { - t.Run(name, func(t *testing.T) { - err := construct(opts...) - if err == nil { - t.Fatalf("expected the roles claim to be reserved") - } - if !strings.Contains(err.Error(), "groups") { - t.Fatalf("error should name the offending claim, got %v", err) - } - }) - } - }) -} - -// TestRolesClaim_NoDeclaredClaimsYieldsNilExtraClaims pins that remapping -// roles (which forces a payload decode) does not turn the "nothing declared" -// case from a nil ExtraClaims into an empty non-nil map. -func TestRolesClaim_NoDeclaredClaimsYieldsNilExtraClaims(t *testing.T) { - extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithRolesClaim("groups")) - defer cleanup() - - claims := newBaseClaims(AuthorizationCodeGrant) - claims["sub"] = testUserID - claims["groups"] = []string{"exporter"} - - p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) - if err != nil { - t.Fatalf("extract: %v", err) - } - if p.UserPrincipal.ExtraClaims != nil { - t.Fatalf("expected nil ExtraClaims, got %#v", p.UserPrincipal.ExtraClaims) - } } diff --git a/authn/manager.go b/authn/manager.go index 481514e3..4c56e419 100644 --- a/authn/manager.go +++ b/authn/manager.go @@ -15,37 +15,22 @@ import ( // It encapsulates the token extraction and middleware creation, // providing a clean interface for the HTTP server to use. // -// This manager pattern keeps auth logic self-contained and optionally delegates user persistence -// to a UserProfileService (if provided). The manager can work without a user profile service, -// making it suitable for any authentication use case. +// The manager only authenticates: it validates the token and injects the +// resulting Principal. Anything an application does with that principal (e.g. +// provisioning a user record) belongs in its own middleware chained after it. type Manager struct { - userProfileService UserProfileService - tokenExtractor *TokenExtractor - middleware func(http.Handler) http.Handler + tokenExtractor *TokenExtractor + middleware func(http.Handler) http.Handler } // NewManager creates and initializes a new auth manager. // This is the single entry point for all auth initialization in the application. // -// userProfileService is OPTIONAL. If not provided (nil), user creation on first login is disabled. -// tokenExtractor is REQUIRED and is always initialized by this constructor. -// This allows the auth package to be used in systems that don't track user profiles -// while still guaranteeing auth token parsing is available. -// -// Usage examples: -// -// // With user profile service (NSW example) -// userProfileService := user.NewService(db) -// authManager := auth.NewManager(userProfileService, cfg.Auth) -// -// // Without user profile service (generic auth only) -// authManager := auth.NewManager(nil, cfg.Auth) +// Usage: // -// // With custom user profile service -// customService := &MyCustomUserService{} -// authManager := auth.NewManager(customService, cfg.Auth) -func NewManager(userProfileService UserProfileService, authConfig Config) (*Manager, error) { - slog.Info("initializing auth manager", "user_profile_service_enabled", userProfileService != nil) +// authManager, err := auth.NewManager(cfg.Auth) +func NewManager(authConfig Config) (*Manager, error) { + slog.Info("initializing auth manager") httpClient := &http.Client{Timeout: 10 * time.Second} if authConfig.InsecureSkipTLSVerify { @@ -56,7 +41,7 @@ func NewManager(userProfileService UserProfileService, authConfig Config) (*Mana tokenExtractor, err := NewTokenExtractorWithClient( authConfig.JWKSURL, authConfig.Issuer, authConfig.Audience, httpClient, - buildClaimOptions(authConfig)..., + buildOptions(authConfig)..., ) if err != nil { return nil, fmt.Errorf("failed to initialize token extractor: %w", err) @@ -66,29 +51,16 @@ func NewManager(userProfileService UserProfileService, authConfig Config) (*Mana } return &Manager{ - userProfileService: userProfileService, - tokenExtractor: tokenExtractor, - middleware: Middleware(userProfileService, tokenExtractor), + tokenExtractor: tokenExtractor, + middleware: Middleware(tokenExtractor), }, nil } -// buildClaimOptions translates Config's claim fields into TokenExtractor +// buildOptions translates Config's optional fields into TokenExtractor // Options, so Config-driven (NewManager) and direct (NewTokenExtractor) -// callers share identical claim semantics. -func buildClaimOptions(cfg Config) []Option { - var opts []Option - if !cfg.UserClaims.isZero() { - opts = append(opts, WithUserClaims(cfg.UserClaims)) - } - if !cfg.ClientClaims.isZero() { - opts = append(opts, WithClientClaims(cfg.ClientClaims)) - } - // Only when set: WithRolesClaim("") would blank the extractor's default - // and be rejected by validateRolesClaimName. - if cfg.RolesClaim != "" { - opts = append(opts, WithRolesClaim(cfg.RolesClaim)) - } - return opts +// callers share identical semantics. +func buildOptions(cfg Config) []Option { + return []Option{WithClaims(cfg.Claims)} } // Middleware returns the auth middleware function. @@ -100,9 +72,8 @@ func buildClaimOptions(cfg Config) []Option { // // The middleware: // 1. Extracts Authorization header -// 2. Parses token into user or client principal -// 3. For user principals, creates user record if it's their first login -// 4. Injects context into request +// 2. Parses token into a Principal +// 3. Injects context into request func (m *Manager) Middleware() func(http.Handler) http.Handler { return m.middleware } // RequireAuthMiddleware returns a middleware that requires authentication. @@ -115,7 +86,7 @@ func (m *Manager) Middleware() func(http.Handler) http.Handler { return m.middle // authManager.RequireAuthMiddleware()(handler), // ) func (m *Manager) RequireAuthMiddleware() func(http.Handler) http.Handler { - return RequireAuth(m.userProfileService, m.tokenExtractor) + return RequireAuth(m.tokenExtractor) } // OptionalAuthMiddleware returns a middleware for endpoints that work with or without auth. @@ -131,12 +102,11 @@ func (m *Manager) RequireAuthMiddleware() func(http.Handler) http.Handler { func (m *Manager) OptionalAuthMiddleware() func(http.Handler) http.Handler { return m.middleware } // Health checks if the auth system is functioning properly. -// Since the UserProfileService is optional, this only verifies that the auth -// system components are initialized correctly. +// It verifies that the auth system components are initialized correctly. // // Usage in server startup: // -// authManager := auth.NewManager(userProfileService, cfg.Auth) +// authManager := auth.NewManager(cfg.Auth) // if err := authManager.Health(); err != nil { // log.Fatalf("auth system health check failed: %v", err) // } @@ -155,7 +125,7 @@ func (m *Manager) Health() error { // // Usage: // -// authManager := auth.NewManager(userService, cfg.Auth) +// authManager := auth.NewManager(cfg.Auth) // defer authManager.Close() func (m *Manager) Close() error { slog.Debug("auth manager closing") diff --git a/authn/manager_test.go b/authn/manager_test.go index fb1c42a1..61311888 100644 --- a/authn/manager_test.go +++ b/authn/manager_test.go @@ -9,14 +9,14 @@ import ( "testing" ) -func TestNewManager_AllowsNilUserProfileService(t *testing.T) { +func TestNewManager_Defaults(t *testing.T) { cfg := Config{ JWKSURL: "https://localhost/jwks", Issuer: "https://localhost/token", Audience: "TRADER_PORTAL_APP", } - manager, err := NewManager(nil, cfg) + manager, err := NewManager(cfg) if err != nil { t.Fatalf("expected nil error, got %v", err) } @@ -24,9 +24,6 @@ func TestNewManager_AllowsNilUserProfileService(t *testing.T) { t.Fatalf("expected manager, got nil") return } - if manager.userProfileService != nil { - t.Fatalf("expected nil userProfileService, got %T", manager.userProfileService) - } if manager.tokenExtractor == nil { t.Fatal("expected tokenExtractor to be initialized") } @@ -41,7 +38,7 @@ func TestNewManager_InvalidConfig(t *testing.T) { Audience: "TRADER_PORTAL_APP", } - if _, err := NewManager(nil, cfg); err == nil { + if _, err := NewManager(cfg); err == nil { t.Fatalf("expected error for invalid config") } } @@ -61,7 +58,7 @@ func TestNewManager_InsecureSkipTLSVerify(t *testing.T) { InsecureSkipTLSVerify: true, } - manager, err := NewManager(nil, cfg) + manager, err := NewManager(cfg) if err != nil { t.Fatalf("expected nil error, got %v", err) } @@ -84,7 +81,7 @@ func TestManager_Health_Success(t *testing.T) { Audience: "TRADER_PORTAL_APP", } - manager, err := NewManager(nil, cfg) + manager, err := NewManager(cfg) if err != nil { t.Fatalf("expected nil error, got %v", err) } @@ -100,7 +97,7 @@ func TestManager_MiddlewareFunctions(t *testing.T) { Audience: "TRADER_PORTAL_APP", } - manager, err := NewManager(nil, cfg) + manager, err := NewManager(cfg) if err != nil { t.Fatalf("expected nil error, got %v", err) } @@ -129,7 +126,7 @@ func TestManager_RequireAuthMiddleware(t *testing.T) { Audience: "TRADER_PORTAL_APP", } - manager, err := NewManager(nil, cfg) + manager, err := NewManager(cfg) if err != nil { t.Fatalf("expected nil error, got %v", err) } @@ -159,73 +156,19 @@ func TestManager_Close(t *testing.T) { } } -func TestNewManager_WiresExtraClaims(t *testing.T) { +func TestNewManager_WiresClaims(t *testing.T) { cfg := Config{ JWKSURL: "https://localhost/jwks", Issuer: "https://localhost/token", Audience: "TRADER_PORTAL_APP", - // "email" appears in both slices: required must win on the Config - // path too, independently of the order buildClaimOptions emits. - UserClaims: ClaimSpec{Optional: []string{"email", "given_name"}, Required: []string{"ouHandle", "email"}}, - ClientClaims: ClaimSpec{Optional: []string{"department"}, Required: []string{"cost_center"}}, - RolesClaim: "groups", + Claims: ClaimsConfig{OUID: ClaimConfig{Required: true}}, } - manager, err := NewManager(nil, cfg) + manager, err := NewManager(cfg) if err != nil { t.Fatalf("expected nil error, got %v", err) } - - if got := manager.tokenExtractor.rolesClaim; got != "groups" { - t.Fatalf("rolesClaim = %q, want groups", got) - } - - wantUser := map[string]bool{"email": true, "given_name": false, "ouHandle": true} - if len(manager.tokenExtractor.userExtraClaims) != len(wantUser) { - t.Fatalf("userExtraClaims = %#v, want %#v", manager.tokenExtractor.userExtraClaims, wantUser) - } - for name, required := range wantUser { - if got, ok := manager.tokenExtractor.userExtraClaims[name]; !ok || got != required { - t.Fatalf("userExtraClaims[%q] = (%v, %v); want (%v, true)", name, got, ok, required) - } - } - - wantClient := map[string]bool{"department": false, "cost_center": true} - if len(manager.tokenExtractor.clientExtraClaims) != len(wantClient) { - t.Fatalf("clientExtraClaims = %#v, want %#v", manager.tokenExtractor.clientExtraClaims, wantClient) - } - for name, required := range wantClient { - if got, ok := manager.tokenExtractor.clientExtraClaims[name]; !ok || got != required { - t.Fatalf("clientExtraClaims[%q] = (%v, %v); want (%v, true)", name, got, ok, required) - } - } -} - -func TestNewManager_RejectsFixedSchemaExtraClaim(t *testing.T) { - cfg := Config{ - JWKSURL: "https://localhost/jwks", - Issuer: "https://localhost/token", - Audience: "TRADER_PORTAL_APP", - UserClaims: ClaimSpec{Optional: []string{"scope"}}, - } - if _, err := NewManager(nil, cfg); err == nil { - t.Fatalf("expected error for fixed-schema claim declaration") - } -} - -// TestNewManager_RejectsRolesClaimCollision covers the ordering trap: the -// roles claim is reserved, and buildClaimOptions emits WithUserClaims before -// WithRolesClaim, so the collision can only be caught after every option has -// been applied. -func TestNewManager_RejectsRolesClaimCollision(t *testing.T) { - cfg := Config{ - JWKSURL: "https://localhost/jwks", - Issuer: "https://localhost/token", - Audience: "TRADER_PORTAL_APP", - RolesClaim: "groups", - UserClaims: ClaimSpec{Optional: []string{"groups"}}, - } - if _, err := NewManager(nil, cfg); err == nil { - t.Fatalf("expected error declaring the roles claim as an extra claim") + if got := manager.tokenExtractor.claims; got != cfg.Claims { + t.Fatalf("claims = %+v, want %+v", got, cfg.Claims) } } diff --git a/authn/middleware.go b/authn/middleware.go index 1490d685..8f3b2a96 100644 --- a/authn/middleware.go +++ b/authn/middleware.go @@ -12,22 +12,23 @@ import ( // Middleware creates an HTTP middleware that extracts and injects authentication context. // This middleware: // 1. Extracts the Authorization header -// 2. Parses the token into a user principal or client principal -// 3. For user principals, resolves user profile idempotently if UserProfileService is provided -// 4. Injects the auth context into the request +// 2. Parses the token into a Principal +// 3. Injects the auth context into the request // // Behavior summary: // - Missing Authorization header: request proceeds without auth context. // - Invalid token: request is rejected with 401. // - Auth dependencies unavailable: request is rejected with 500. -// - User principal on first login: resolves (get-or-create) user profile if service is provided. // // This design allows: // - Public endpoints (no auth required) // - Protected endpoints (check for context) // - Optional auth endpoints (use context if available) -// - Generic auth that works with or without a user profile service -func Middleware(userProfileService UserProfileService, tokenExtractor *TokenExtractor) func(http.Handler) http.Handler { +// +// Anything an application wants to do with the validated principal (e.g. +// provisioning a user record) belongs in its own middleware chained after this +// one; see the package README. +func Middleware(tokenExtractor *TokenExtractor) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { authHeader := r.Header.Get("Authorization") @@ -62,26 +63,7 @@ func Middleware(userProfileService UserProfileService, tokenExtractor *TokenExtr return } - if principal.UserPrincipal == nil && principal.ClientPrincipal == nil { - slog.InfoContext(r.Context(), "token missing both userPrincipal and clientPrincipal") - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusUnauthorized) - _, _ = w.Write([]byte(`{"error":"unauthorized","message":"invalid authentication token"}`)) - return - } - - authCtx := buildAuthContext(principal) - if principal.UserPrincipal != nil && userProfileService != nil { - user := principal.UserPrincipal - userID, err := userProfileService.GetOrCreateUser(r.Context(), user) - if err != nil { - slog.ErrorContext(r.Context(), "failed to get or create user profile", "idp_user_id", user.Subject, "error", err) - } else if userID != "" { - authCtx.User.ID = userID - slog.DebugContext(r.Context(), "resolved user profile", "idp_user_id", user.Subject, "user_id", userID) - } - } - + authCtx := &AuthContext{Principal: *principal} ctx := context.WithValue(r.Context(), AuthContextKey, authCtx) next.ServeHTTP(w, r.WithContext(ctx)) }) @@ -94,14 +76,14 @@ func Middleware(userProfileService UserProfileService, tokenExtractor *TokenExtr // // Usage: // -// mux.Handle("POST /api/protected", auth.RequireAuth(userProfileService, tokenExtractor)(handler)) +// mux.Handle("POST /api/protected", auth.RequireAuth(tokenExtractor)(handler)) // // TODO_JWT_FUTURE: Consider adding: // - Different auth levels (basic, standard, admin) // - Claim validation beyond token signature // - Rate limiting per user -func RequireAuth(userProfileService UserProfileService, tokenExtractor *TokenExtractor) func(http.Handler) http.Handler { - authMiddleware := Middleware(userProfileService, tokenExtractor) +func RequireAuth(tokenExtractor *TokenExtractor) func(http.Handler) http.Handler { + authMiddleware := Middleware(tokenExtractor) return func(next http.Handler) http.Handler { return authMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if GetAuthContext(r.Context()) == nil { diff --git a/authn/middleware_test.go b/authn/middleware_test.go index 9d60e509..c6e3fee9 100644 --- a/authn/middleware_test.go +++ b/authn/middleware_test.go @@ -5,32 +5,11 @@ package authn import ( "context" - "errors" "net/http" "net/http/httptest" "testing" ) -// MockUserService is a mock implementation of UserProfileService for testing. -type MockUserService struct { - getOrCreateID string - getOrCreateErr error - getOrCreateCalls int - lastPrincipal *UserPrincipal -} - -func (m *MockUserService) GetOrCreateUser(ctx context.Context, principal *UserPrincipal) (string, error) { - m.getOrCreateCalls++ - m.lastPrincipal = principal - if m.getOrCreateErr != nil { - return "", m.getOrCreateErr - } - if m.getOrCreateID != "" { - return m.getOrCreateID, nil - } - return "mock-user-id", nil -} - // TestAuthMiddleware_NoToken tests middleware when no auth header provided func TestAuthMiddleware_NoToken(t *testing.T) { // Create a test handler that checks for auth context @@ -42,10 +21,9 @@ func TestAuthMiddleware_NoToken(t *testing.T) { w.WriteHeader(http.StatusOK) }) - // Create middleware with nil dependencies - // This is acceptable for this test case since no token means the middleware - // won't attempt to use user service or TokenExtractor - middleware := Middleware(nil, nil) + // A nil TokenExtractor is acceptable here: with no token the middleware + // never uses it. + middleware := Middleware(nil) handlerWithMiddleware := middleware(testHandler) // Make a test request without Authorization header @@ -68,7 +46,7 @@ func TestAuthMiddleware_UninitializedDependencies(t *testing.T) { }) // With nil tokenExtractor, middleware should return 500 - middleware := Middleware(nil, nil) + middleware := Middleware(nil) handlerWithMiddleware := middleware(testHandler) req := httptest.NewRequest("GET", "http://example.com/test", nil) @@ -95,9 +73,7 @@ func TestAuthMiddleware_InvalidToken(t *testing.T) { tokenExtractor, _, cleanup := newTokenExtractor(t) defer cleanup() - // Use mock user service to ensure this test validates token behavior - mockUserService := &MockUserService{} - middleware := Middleware(mockUserService, tokenExtractor) + middleware := Middleware(tokenExtractor) handlerWithMiddleware := middleware(testHandler) req := httptest.NewRequest("GET", "http://example.com/test", nil) @@ -114,88 +90,6 @@ func TestAuthMiddleware_InvalidToken(t *testing.T) { } } -func TestBuildAuthContext_UserPrincipalOnly(t *testing.T) { - principal := &Principal{ - Type: UserPrincipalType, - UserPrincipal: &UserPrincipal{ - Subject: testUserID, - Roles: []string{"exporter"}, - ExtraClaims: ExtraClaims{ - "email": testEmail, - "phone_number": testPhone, - "ouId": testOUID, - "ouHandle": testOUHandle, - }, - }, - } - - authCtx := buildAuthContext(principal) - - if authCtx.User == nil || authCtx.User.IDPUserID != testUserID { - t.Fatalf("expected idp user id to be set from user principal") - } - if authCtx.User.ID != "" { - t.Fatalf("expected persisted user id to be empty for user principal only") - } - if authCtx.User.ExtraClaims.String("email") != testEmail { - t.Fatalf("expected email to be set, got %s", authCtx.User.ExtraClaims.String("email")) - } - if authCtx.User.ExtraClaims.String("phone_number") != testPhone { - t.Fatalf("expected phone number to be set, got %s", authCtx.User.ExtraClaims.String("phone_number")) - } - if authCtx.User.ExtraClaims.String("ouId") != testOUID { - t.Fatalf("expected ou id to be set, got %s", authCtx.User.ExtraClaims.String("ouId")) - } - if authCtx.User.ExtraClaims.String("ouHandle") != testOUHandle { - t.Fatalf("expected ou handle to be set, got %s", authCtx.User.ExtraClaims.String("ouHandle")) - } - if authCtx.Client != nil { - t.Fatalf("expected client id to be nil when client principal is absent") - } - if len(authCtx.User.Roles) != 1 || authCtx.User.Roles[0] != "exporter" { - t.Fatalf("expected roles to be set, got %v", authCtx.User.Roles) - } -} - -func TestBuildAuthContext_ClientPrincipalOnly(t *testing.T) { - principal := &Principal{ - Type: ClientPrincipalType, - ClientPrincipal: &ClientPrincipal{ClientID: "CLIENT-001"}, - } - - authCtx := buildAuthContext(principal) - - if authCtx.Client == nil || authCtx.Client.ClientID != "CLIENT-001" { - t.Fatalf("expected client id to be set from client principal") - } - if authCtx.User != nil { - t.Fatalf("expected user fields to be nil when user principal is absent") - } -} - -func TestBuildAuthContext_NilPrincipal(t *testing.T) { - authCtx := buildAuthContext(nil) - if authCtx == nil { - t.Fatalf("expected auth context") - return - } - if authCtx.User != nil || authCtx.Client != nil { - t.Fatalf("expected empty auth context, got %+v", authCtx) - } -} - -func TestBuildAuthContext_UnknownType(t *testing.T) { - principal := &Principal{Type: PrincipalType("unknown")} - authCtx := buildAuthContext(principal) - if authCtx == nil { - t.Fatalf("expected auth context") - return - } - if authCtx.User != nil || authCtx.Client != nil { - t.Fatalf("expected empty auth context, got %+v", authCtx) - } -} - func TestAuthMiddleware_ValidClientCredentialsToken(t *testing.T) { tokenExtractor, privateKey, cleanup := newTokenExtractor(t) defer cleanup() @@ -210,16 +104,16 @@ func TestAuthMiddleware_ValidClientCredentialsToken(t *testing.T) { t.Fatalf("expected auth context") return } - if authCtx.Client == nil || authCtx.Client.ClientID != "TRADER_PORTAL_APP" { - t.Fatalf("expected client id TRADER_PORTAL_APP, got %v", authCtx.Client) + if authCtx.ClientID() != testClientID { + t.Fatalf("expected client id %s, got %q", testClientID, authCtx.ClientID()) } - if authCtx.User != nil { - t.Fatalf("expected user id to be nil for client principal") + if authCtx.Subject() != "FCAU_TO_NSW" { + t.Fatalf("expected subject FCAU_TO_NSW, got %q", authCtx.Subject()) } w.WriteHeader(http.StatusOK) }) - handlerWithMiddleware := Middleware(&MockUserService{}, tokenExtractor)(testHandler) + handlerWithMiddleware := Middleware(tokenExtractor)(testHandler) req := httptest.NewRequest(http.MethodGet, "http://example.com/test", nil) req.Header.Set("Authorization", "Bearer "+signedToken) recorder := httptest.NewRecorder() @@ -234,7 +128,7 @@ func TestAuthMiddleware_ValidClientCredentialsToken(t *testing.T) { } } -func TestAuthMiddleware_UserPrincipal_NoUserProfileService(t *testing.T) { +func TestAuthMiddleware_ValidUserToken(t *testing.T) { tokenExtractor, privateKey, cleanup := newTokenExtractor(t) defer cleanup() @@ -244,28 +138,22 @@ func TestAuthMiddleware_UserPrincipal_NoUserProfileService(t *testing.T) { testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { testHandlerCalled = true authCtx := GetAuthContext(r.Context()) - if authCtx == nil || authCtx.User == nil { - t.Fatalf("expected auth context with user") - } - if authCtx.User.IDPUserID != testUserID { - t.Fatalf("expected idp user id %s, got %s", testUserID, authCtx.User.IDPUserID) + if authCtx == nil { + t.Fatalf("expected auth context") } - if authCtx.User.ID != "" { - t.Fatalf("expected persisted user id to be empty, got %s", authCtx.User.ID) + if authCtx.Subject() != testUserID { + t.Fatalf("expected subject %s, got %s", testUserID, authCtx.Subject()) } - if len(authCtx.User.Roles) != 1 || authCtx.User.Roles[0] != "exporter" { - t.Fatalf("expected roles [exporter], got %v", authCtx.User.Roles) + if authCtx.ClientID() != testClientID { + t.Fatalf("expected client id %s, got %s", testClientID, authCtx.ClientID()) } - // email/phone_number/ouId/ouHandle are no longer part of the fixed - // schema. This extractor declared no extra claims, so none of them - // should be surfaced even though the signed token carries them. - if len(authCtx.User.ExtraClaims) != 0 { - t.Fatalf("expected no extra claims to be extracted, got %#v", authCtx.User.ExtraClaims) + if roles := authCtx.Roles(); len(roles) != 1 || roles[0] != "exporter" { + t.Fatalf("expected roles [exporter], got %v", roles) } w.WriteHeader(http.StatusOK) }) - handlerWithMiddleware := Middleware(nil, tokenExtractor)(testHandler) + handlerWithMiddleware := Middleware(tokenExtractor)(testHandler) req := httptest.NewRequest(http.MethodGet, "http://example.com/test", nil) req.Header.Set("Authorization", "Bearer "+signedToken) recorder := httptest.NewRecorder() @@ -280,72 +168,26 @@ func TestAuthMiddleware_UserPrincipal_NoUserProfileService(t *testing.T) { } } -func TestAuthMiddleware_UserPrincipal_GetOrCreateUser(t *testing.T) { +func TestAuthMiddleware_IdentityClaims(t *testing.T) { tokenExtractor, privateKey, cleanup := newTokenExtractor(t) defer cleanup() signedToken := newUserToken(t, privateKey) - mockUserService := &MockUserService{getOrCreateID: "existing-user-id"} - - testHandlerCalled := false - testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - testHandlerCalled = true - authCtx := GetAuthContext(r.Context()) - if authCtx == nil || authCtx.User == nil { - t.Fatalf("expected auth context with user") - } - if authCtx.User.ID != "existing-user-id" { - t.Fatalf("expected persisted user id existing-user-id, got %s", authCtx.User.ID) - } - w.WriteHeader(http.StatusOK) - }) - - handlerWithMiddleware := Middleware(mockUserService, tokenExtractor)(testHandler) - req := httptest.NewRequest(http.MethodGet, "http://example.com/test", nil) - req.Header.Set("Authorization", "Bearer "+signedToken) - recorder := httptest.NewRecorder() - - handlerWithMiddleware.ServeHTTP(recorder, req) - - if recorder.Code != http.StatusOK { - t.Fatalf("expected status 200, got %d", recorder.Code) - } - if !testHandlerCalled { - t.Fatalf("expected handler to be called for valid token") - } - if mockUserService.getOrCreateCalls != 1 { - t.Fatalf("expected GetOrCreateUser to be called once, got %d", mockUserService.getOrCreateCalls) - } - if mockUserService.lastPrincipal.Subject != testUserID { - t.Fatalf("expected GetOrCreateUser to be called with %s, got %s", testUserID, mockUserService.lastPrincipal.Subject) - } -} - -func TestAuthMiddleware_UserPrincipal_CreatesUser(t *testing.T) { - // This extractor declares email/phone_number/ouId/ouHandle as optional - // extra claims, so the principal's ExtraClaims are populated from the - // signed token as expected. - tokenExtractor, privateKey, cleanup := newTokenExtractorWithOptions(t, - WithUserClaims(ClaimSpec{Optional: []string{"email", "phone_number", "ouId", "ouHandle"}})) - defer cleanup() - - signedToken := newUserToken(t, privateKey) - mockUserService := &MockUserService{getOrCreateID: "created-user-id"} testHandlerCalled := false testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { testHandlerCalled = true authCtx := GetAuthContext(r.Context()) - if authCtx == nil || authCtx.User == nil { - t.Fatalf("expected auth context with user") + if authCtx.Email() != testEmail || authCtx.OUID() != testOUID || authCtx.OUHandle() != testOUHandle { + t.Fatalf("unexpected standard claims: %+v", authCtx.Principal) } - if authCtx.User.ID != "created-user-id" { - t.Fatalf("expected persisted user id created-user-id, got %s", authCtx.User.ID) + if got := authCtx.ExtraClaims().String("phone_number"); got != testPhone { + t.Fatalf("phone_number = %q, want %q", got, testPhone) } w.WriteHeader(http.StatusOK) }) - handlerWithMiddleware := Middleware(mockUserService, tokenExtractor)(testHandler) + handlerWithMiddleware := Middleware(tokenExtractor)(testHandler) req := httptest.NewRequest(http.MethodGet, "http://example.com/test", nil) req.Header.Set("Authorization", "Bearer "+signedToken) recorder := httptest.NewRecorder() @@ -358,86 +200,42 @@ func TestAuthMiddleware_UserPrincipal_CreatesUser(t *testing.T) { if !testHandlerCalled { t.Fatalf("expected handler to be called for valid token") } - if mockUserService.getOrCreateCalls != 1 { - t.Fatalf("expected GetOrCreateUser to be called once, got %d", mockUserService.getOrCreateCalls) - } - got := mockUserService.lastPrincipal - if got.Subject != testUserID || - got.ExtraClaims.String("email") != testEmail || - got.ExtraClaims.String("phone_number") != testPhone || - got.ExtraClaims.String("ouId") != testOUID || - got.ExtraClaims.String("ouHandle") != testOUHandle { - t.Fatalf("unexpected GetOrCreateUser principal: %+v", got) - } } -// TestAuthMiddleware_UserPrincipal_GetOrCreateUser_ExtraClaimsNotDeclared is a -// regression test for a real footgun: a UserProfileService implementation that -// depends on org/OU-like extra claims silently receives an empty ExtraClaims -// map if the consumer never declared them via WithUserClaims — even though the -// signed JWT carries them. This documents that behavior explicitly as -// intentional, rather than leaving it as an unwritten trap. -func TestAuthMiddleware_UserPrincipal_GetOrCreateUser_ExtraClaimsNotDeclared(t *testing.T) { +// TestRequireAuth_ChainedApplicationMiddleware locks in the extension point +// that replaced UserProfileService: an application middleware chained after +// RequireAuth sees the validated principal and can enrich the request context +// before the handler runs. +func TestRequireAuth_ChainedApplicationMiddleware(t *testing.T) { tokenExtractor, privateKey, cleanup := newTokenExtractor(t) defer cleanup() - signedToken := newUserToken(t, privateKey) - mockUserService := &MockUserService{getOrCreateID: "created-user-id"} - - testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }) - - handlerWithMiddleware := Middleware(mockUserService, tokenExtractor)(testHandler) - req := httptest.NewRequest(http.MethodGet, "http://example.com/test", nil) - req.Header.Set("Authorization", "Bearer "+signedToken) - recorder := httptest.NewRecorder() - - handlerWithMiddleware.ServeHTTP(recorder, req) - - if recorder.Code != http.StatusOK { - t.Fatalf("expected status 200, got %d", recorder.Code) - } - if len(mockUserService.lastPrincipal.ExtraClaims) != 0 { - t.Fatalf("expected empty extra claims when none were declared, got %#v", mockUserService.lastPrincipal.ExtraClaims) + type userKey struct{} + provision := func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + subject := GetAuthContext(r.Context()).Subject() + next.ServeHTTP(w, r.WithContext(context.WithValue(r.Context(), userKey{}, "user-for-"+subject))) + }) } -} -func TestAuthMiddleware_UserPrincipal_GetOrCreateUserError(t *testing.T) { - tokenExtractor, privateKey, cleanup := newTokenExtractor(t) - defer cleanup() - - signedToken := newUserToken(t, privateKey) - mockUserService := &MockUserService{getOrCreateErr: errors.New("db down")} - - testHandlerCalled := false + var got any testHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - testHandlerCalled = true - authCtx := GetAuthContext(r.Context()) - if authCtx == nil || authCtx.User == nil { - t.Fatalf("expected auth context with user") - } - if authCtx.User.ID != "" { - t.Fatalf("expected persisted user id to be empty, got %s", authCtx.User.ID) - } + got = r.Context().Value(userKey{}) w.WriteHeader(http.StatusOK) }) - handlerWithMiddleware := Middleware(mockUserService, tokenExtractor)(testHandler) - req := httptest.NewRequest(http.MethodGet, "http://example.com/test", nil) - req.Header.Set("Authorization", "Bearer "+signedToken) + protected := RequireAuth(tokenExtractor)(provision(testHandler)) + req := httptest.NewRequest(http.MethodGet, "http://example.com/protected", nil) + req.Header.Set("Authorization", "Bearer "+newUserToken(t, privateKey)) recorder := httptest.NewRecorder() - handlerWithMiddleware.ServeHTTP(recorder, req) + protected.ServeHTTP(recorder, req) if recorder.Code != http.StatusOK { t.Fatalf("expected status 200, got %d", recorder.Code) } - if !testHandlerCalled { - t.Fatalf("expected handler to be called for valid token") - } - if mockUserService.getOrCreateCalls != 1 { - t.Fatalf("expected GetOrCreateUser to be called once, got %d", mockUserService.getOrCreateCalls) + if got != "user-for-"+testUserID { + t.Fatalf("expected chained middleware value user-for-%s, got %v", testUserID, got) } } @@ -450,7 +248,7 @@ func TestRequireAuth_UnauthenticatedRequest(t *testing.T) { tokenExtractor, _, cleanup := newTokenExtractor(t) defer cleanup() - protected := RequireAuth(&MockUserService{}, tokenExtractor)(testHandler) + protected := RequireAuth(tokenExtractor)(testHandler) req := httptest.NewRequest(http.MethodGet, "http://example.com/protected", nil) recorder := httptest.NewRecorder() @@ -476,7 +274,7 @@ func TestRequireAuth_ValidClientCredentialsToken(t *testing.T) { w.WriteHeader(http.StatusOK) }) - protected := RequireAuth(&MockUserService{}, tokenExtractor)(testHandler) + protected := RequireAuth(tokenExtractor)(testHandler) req := httptest.NewRequest(http.MethodGet, "http://example.com/protected", nil) req.Header.Set("Authorization", "Bearer "+signedToken) recorder := httptest.NewRecorder() diff --git a/authn/options.go b/authn/options.go index 871193fd..53e093e0 100644 --- a/authn/options.go +++ b/authn/options.go @@ -3,108 +3,15 @@ package authn -import "strings" - // Option configures optional behavior on TokenExtractor construction. // // Options never fail on their own: they record intent verbatim and every check -// runs afterwards in validateConfig, so NewTokenExtractor reports a bad -// declaration as a construction error regardless of the order options were -// passed in. +// runs afterwards in validateConfig, which NewTokenExtractor reports as a +// construction error. type Option func(*TokenExtractor) -// ClaimSpec declares the extra JWT claims — those beyond authn's fixed schema -// (client_id, grant_type, scope, roles, and the JWT registered claims) — to -// extract for one principal type. -// -// Optional claims are best-effort: a name that is absent, JSON-null, or an -// empty/whitespace-only string is silently skipped and never fails a token. -// Required claims must carry a usable value — a non-blank string, or a -// non-empty array of non-blank strings — or ExtractPrincipalFromHeader rejects -// the token. A name listed in both is required. -// -// Names are matched exactly against the token payload: JWT claim names are -// case-sensitive (RFC 7519 §4). Nested lookups are not supported; a name is a -// top-level key, which is what makes namespaced names like -// "https://app.example.com/roles" work unchanged. -// -// ClaimSpec carries yaml struct tags, so it can be populated generically -// (e.g. via yaml.Unmarshal) as part of Config. -type ClaimSpec struct { - Optional []string `yaml:"optional"` - Required []string `yaml:"required"` -} - -func (s ClaimSpec) isZero() bool { - return len(s.Optional) == 0 && len(s.Required) == 0 -} - -// WithUserClaims declares extra claims to extract from user-principal -// (authorization_code grant) tokens. Extracted values surface on -// UserPrincipal.ExtraClaims / UserContext.ExtraClaims. -// -// Repeated calls accumulate rather than replace, so a name declared required by -// any call stays required. -func WithUserClaims(spec ClaimSpec) Option { - return func(te *TokenExtractor) { te.userClaims.merge(spec) } -} - -// WithClientClaims is the WithUserClaims analogue for client-credential (M2M) -// tokens, populating ClientPrincipal.ExtraClaims / ClientContext.ExtraClaims. -// -// The declaration is separate because the two token types carry different -// claim sets: a client token may carry e.g. "department" or "cost_center", -// while "email"/"ouHandle"/"given_name" only appear on user tokens. A name -// declared for one principal type is never extracted from the other. -func WithClientClaims(spec ClaimSpec) Option { - return func(te *TokenExtractor) { te.clientClaims.merge(spec) } -} - -// WithRolesClaim points authn at a differently-named claim for the roles that -// populate Principal.Roles and AuthContext.Roles(), for identity providers -// that do not emit a top-level "roles" claim (Okta commonly uses "groups"). -// Defaults to "roles". The name applies to both user and client principals. -// -// The claim must be a top-level JSON array of strings — exactly the shape the -// default "roles" claim must have. An absent claim yields no roles; a present -// claim of any other shape rejects the token, so a mistyped name fails loudly -// on the first request instead of silently disabling every role check. -// -// Dotted paths into nested objects (Keycloak's realm_access.roles) are NOT -// supported: names are matched exactly, which is what keeps namespaced claim -// names such as "https://app.example.com/roles" working. Flatten a nested -// roles claim with an IdP-side protocol mapper. -// -// The configured name is reserved: it cannot also be declared as an extra -// claim via WithUserClaims/WithClientClaims, since it already has a dedicated -// field. -func WithRolesClaim(name string) Option { - // Recorded verbatim; validateRolesClaimName judges it in validateConfig, - // so this behaves identically whether it runs before or after the - // WithUserClaims/WithClientClaims calls it interacts with. - return func(te *TokenExtractor) { te.rolesClaim = name } -} - -func (s *ClaimSpec) merge(other ClaimSpec) { - s.Optional = append(s.Optional, other.Optional...) - s.Required = append(s.Required, other.Required...) -} - -// resolve flattens a declaration into the name -> required map the extraction -// path uses. Required is applied after Optional, so a name appearing in both -// (in either slice, from any number of merged calls) ends up required. -// Returns nil for an empty spec, which is what lets extraction skip its -// payload decode entirely for consumers who never opted in. -func (s ClaimSpec) resolve() map[string]bool { - if s.isZero() { - return nil - } - declared := make(map[string]bool, len(s.Optional)+len(s.Required)) - for _, name := range s.Optional { - declared[strings.TrimSpace(name)] = false - } - for _, name := range s.Required { - declared[strings.TrimSpace(name)] = true - } - return declared +// WithClaims configures the standard identity claims (see ClaimsConfig). A +// later call replaces an earlier one. +func WithClaims(claims ClaimsConfig) Option { + return func(te *TokenExtractor) { te.claims = claims } } diff --git a/authn/scopes_test.go b/authn/scopes_test.go index 3e7af0d0..a51c9589 100644 --- a/authn/scopes_test.go +++ b/authn/scopes_test.go @@ -61,7 +61,7 @@ func TestExtractPrincipal_SurfacesScopesAndRoles(t *testing.T) { defer cleanup() t.Run("user token scopes", func(t *testing.T) { - claims := newBaseClaims(AuthorizationCodeGrant) + claims := newBaseClaims() claims["sub"] = testUserID claims["email"] = testEmail claims["ouId"] = testOUID @@ -73,16 +73,13 @@ func TestExtractPrincipal_SurfacesScopesAndRoles(t *testing.T) { if err != nil { t.Fatalf("extract: %v", err) } - if p.UserPrincipal == nil { - t.Fatal("expected user principal") - } - if !sameScopes(p.UserPrincipal.Scopes, []string{"nsw:task:read", "nsw:storage:read"}) { - t.Fatalf("user scopes = %v", p.UserPrincipal.Scopes) + if !sameScopes(p.Scopes, []string{"nsw:task:read", "nsw:storage:read"}) { + t.Fatalf("user scopes = %v", p.Scopes) } }) t.Run("client token roles and scopes", func(t *testing.T) { - claims := newBaseClaims(ClientCredentialsGrant) + claims := newBaseClaims() claims["sub"] = testClientID claims["roles"] = []string{"AgencyM2M"} claims["scope"] = "nsw:task:write nsw:consignment:read" @@ -91,79 +88,18 @@ func TestExtractPrincipal_SurfacesScopesAndRoles(t *testing.T) { if err != nil { t.Fatalf("extract: %v", err) } - if p.ClientPrincipal == nil { - t.Fatal("expected client principal") - } - if !sameScopes(p.ClientPrincipal.Scopes, []string{"nsw:task:write", "nsw:consignment:read"}) { - t.Fatalf("client scopes = %v", p.ClientPrincipal.Scopes) + if !sameScopes(p.Scopes, []string{"nsw:task:write", "nsw:consignment:read"}) { + t.Fatalf("client scopes = %v", p.Scopes) } - if !sameScopes(p.ClientPrincipal.Roles, []string{"AgencyM2M"}) { - t.Fatalf("client roles = %v", p.ClientPrincipal.Roles) + if !sameScopes(p.Roles, []string{"AgencyM2M"}) { + t.Fatalf("client roles = %v", p.Roles) } }) } -func TestBuildAuthContext_SurfacesScopesAndRoles(t *testing.T) { - uc := buildAuthContext(&Principal{ - Type: UserPrincipalType, - UserPrincipal: &UserPrincipal{ - Subject: "u1", - ExtraClaims: ExtraClaims{"email": testEmail, "ouId": testOUID, "ouHandle": testOUHandle}, - Roles: []string{"Trader"}, Scopes: []string{"nsw:task:read"}, - }, - }) - if uc.User == nil || !sameScopes(uc.User.Scopes, []string{"nsw:task:read"}) { - t.Fatalf("user scopes not surfaced: %#v", uc.User) - } - - cc := buildAuthContext(&Principal{ - Type: ClientPrincipalType, - ClientPrincipal: &ClientPrincipal{ - ClientID: "NPQS_TO_NSW", Roles: []string{"AgencyM2M"}, Scopes: []string{"nsw:task:write"}, - }, - }) - if cc.Client == nil || !sameScopes(cc.Client.Scopes, []string{"nsw:task:write"}) || !sameScopes(cc.Client.Roles, []string{"AgencyM2M"}) { - t.Fatalf("client roles/scopes not surfaced: %#v", cc.Client) - } -} - // Compile-time proof that *AuthContext satisfies the accessor seam used by authz. var _ interface { - Type() PrincipalType Subject() string Roles() []string Scopes() []string } = (*AuthContext)(nil) - -func TestAuthContext_AccessorSeam(t *testing.T) { - user := &AuthContext{User: &UserContext{ID: "u1", IDPUserID: "idp1", Roles: []string{"Trader"}, Scopes: []string{"nsw:task:read"}}} - if user.Type() != UserPrincipalType { - t.Fatalf("user Type = %q", user.Type()) - } - if user.Subject() != "u1" { - t.Fatalf("user Subject = %q", user.Subject()) - } - if !sameScopes(user.Roles(), []string{"Trader"}) || !sameScopes(user.Scopes(), []string{"nsw:task:read"}) { - t.Fatalf("user roles/scopes = %v / %v", user.Roles(), user.Scopes()) - } - - // Subject falls back to IdP user ID when resolved ID is empty. - if got := (&AuthContext{User: &UserContext{IDPUserID: "idp2"}}).Subject(); got != "idp2" { - t.Fatalf("subject fallback = %q, want idp2", got) - } - - client := &AuthContext{Client: &ClientContext{ClientID: "NPQS_TO_NSW", Roles: []string{"AgencyM2M"}, Scopes: []string{"nsw:task:write"}}} - if client.Type() != ClientPrincipalType || client.Subject() != "NPQS_TO_NSW" { - t.Fatalf("client Type/Subject = %q / %q", client.Type(), client.Subject()) - } - if !sameScopes(client.Roles(), []string{"AgencyM2M"}) || !sameScopes(client.Scopes(), []string{"nsw:task:write"}) { - t.Fatalf("client roles/scopes = %v / %v", client.Roles(), client.Scopes()) - } - - // Nil-safe: empty context and nil receiver return zero values. - for _, a := range []*AuthContext{{}, nil} { - if a.Type() != "" || a.Subject() != "" || a.Roles() != nil || a.Scopes() != nil { - t.Fatalf("expected zero values for %#v", a) - } - } -} diff --git a/authn/test_helpers_test.go b/authn/test_helpers_test.go index 9c437638..cf26c344 100644 --- a/authn/test_helpers_test.go +++ b/authn/test_helpers_test.go @@ -35,8 +35,7 @@ func newTokenExtractor(t *testing.T) (*TokenExtractor, *rsa.PrivateKey, func()) } // newTokenExtractorWithOptions mirrors newTokenExtractor but threads Options -// through to NewTokenExtractor, for tests exercising WithUserClaims / -// WithClientClaims / WithRolesClaim. +// through to NewTokenExtractor, for tests exercising WithClaims. func newTokenExtractorWithOptions(t *testing.T, opts ...Option) (*TokenExtractor, *rsa.PrivateKey, func()) { t.Helper() privateKey, err := rsa.GenerateKey(rand.Reader, 2048) @@ -67,22 +66,24 @@ func newTokenExtractorWithOptions(t *testing.T, opts ...Option) (*TokenExtractor return extractor, privateKey, jwksServer.Close } -func newBaseClaims(grantType AllowedGrantType) jwt.MapClaims { +func newBaseClaims() jwt.MapClaims { now := time.Now() return jwt.MapClaims{ - "iss": testIssuer, - "aud": testClientID, - "client_id": testClientID, - "grant_type": grantType, - "iat": now.Add(-1 * time.Minute).Unix(), - "nbf": now.Add(-1 * time.Minute).Unix(), - "exp": now.Add(10 * time.Minute).Unix(), + "iss": testIssuer, + "aud": testClientID, + "sub": testUserID, + "client_id": testClientID, + "iat": now.Add(-1 * time.Minute).Unix(), + "nbf": now.Add(-1 * time.Minute).Unix(), + "exp": now.Add(10 * time.Minute).Unix(), } } +// newUserToken signs a token as issued to a person signing in through a client: +// a user subject carrying identity claims. func newUserToken(t *testing.T, privateKey *rsa.PrivateKey) string { t.Helper() - claims := newBaseClaims(AuthorizationCodeGrant) + claims := newBaseClaims() claims["sub"] = testUserID claims["email"] = testEmail claims["phone_number"] = testPhone @@ -92,9 +93,11 @@ func newUserToken(t *testing.T, privateKey *rsa.PrivateKey) string { return signToken(t, privateKey, claims) } +// newClientToken signs a token as issued to a system via client credentials: +// the subject is the client itself and no identity claims are carried. func newClientToken(t *testing.T, privateKey *rsa.PrivateKey) string { t.Helper() - claims := newBaseClaims(ClientCredentialsGrant) + claims := newBaseClaims() claims["sub"] = "FCAU_TO_NSW" return signToken(t, privateKey, claims) } diff --git a/authn/token_parser.go b/authn/token_parser.go index f300a003..8f0c7aa8 100644 --- a/authn/token_parser.go +++ b/authn/token_parser.go @@ -15,13 +15,6 @@ import ( "github.com/golang-jwt/jwt/v5" ) -type AllowedGrantType string - -const ( - AuthorizationCodeGrant AllowedGrantType = "authorization_code" - ClientCredentialsGrant AllowedGrantType = "client_credentials" -) - // spaceDelimitedScope unmarshals the OAuth2 "scope" claim, which is a single // space-delimited string (RFC 6749 §3.3), e.g. "nsw:task:read nsw:storage:read". // It defensively also accepts a JSON array of strings. An absent claim leaves it @@ -42,56 +35,55 @@ func (s *spaceDelimitedScope) UnmarshalJSON(data []byte) error { return nil } -// tokenClaims is deliberately minimal: only claims the package's own -// mechanics depend on (client_id/grant_type for principal-type dispatch and -// client-id validation, scope for OAuth2 permission scoping, roles for the -// authz seam both user and client principals rely on) plus the JWT -// registered claims (sub/iss/aud/exp/...) needed for signature/expiry/issuer/ -// audience validation. Everything else (email, phone_number, ouId, ouHandle, -// given_name, ...) is IdP/consumer-specific and flows through the -// extra-claims mechanism (see extra_claims.go) instead of a dedicated field. +// tokenClaims is deliberately small: the claims every Principal carries (sub +// and client_id, both required; scope for OAuth2 permission scoping; roles for +// the authz seam), the standard identity claims (email, ouId, ouHandle; each +// optional unless configured as required), plus the JWT registered claims +// (iss/aud/exp/...) needed for signature/expiry/issuer/audience validation. +// Everything else (phone_number, given_name, ...) passes through as +// ExtraClaims (see extra_claims.go) instead of a dedicated field. // -// Roles is captured shape-free (see rolesValue) so that WithRolesClaim can -// genuinely move roles elsewhere; parseRolesClaim enforces the shape on -// whichever claim is actually in force. +// Roles is captured shape-free (see rolesValue); parseRolesClaim enforces its +// shape. type tokenClaims struct { jwt.RegisteredClaims - ClientID string `json:"client_id"` - GrantType AllowedGrantType `json:"grant_type"` - Roles rolesValue `json:"roles"` - Scopes spaceDelimitedScope `json:"scope,omitempty"` + ClientID string `json:"client_id"` + Roles rolesValue `json:"roles"` + Scopes spaceDelimitedScope `json:"scope,omitempty"` + Email string `json:"email"` + OUID string `json:"ouId"` + OUHandle string `json:"ouHandle"` } -type PrincipalType string - -const ( - UserPrincipalType PrincipalType = "user" - ClientPrincipalType PrincipalType = "client" -) - -type ClientPrincipal struct { - ClientID string `json:"clientId"` - Roles []string `json:"roles"` - Scopes []string `json:"scopes"` - ExtraClaims ExtraClaims `json:"extraClaims,omitempty"` -} +// Principal is the authenticated caller a validated token describes. There is +// one shape for every token, however it was obtained: a person signing in +// through a client and a system using client credentials are distinguished +// only by what they carry (roles, scopes, claims), never by kind. +type Principal struct { + // Subject is the JWT "sub" claim: the identity provider's ID for the + // caller. Always non-empty: a token without "sub" is rejected. + Subject string `json:"subject"` + // ClientID is the JWT "client_id" claim: the OAuth2 client the token was + // issued to. Always non-empty: a token without "client_id" is rejected. + ClientID string `json:"clientId"` + // Roles is the JWT "roles" claim, nil when the token does not carry it. A + // "roles" claim that is not an array of strings rejects the token. + Roles []string `json:"roles"` + // Scopes is the space-delimited JWT "scope" claim, nil when the token does + // not carry it. + Scopes []string `json:"scopes"` + + // Email, OUID and OUHandle are the standard identity claims "email", + // "ouId" and "ouHandle", whitespace-trimmed. Each is "" when the token + // does not carry it, unless configured as required (see ClaimsConfig), + // in which case such a token is rejected. + Email string `json:"email,omitempty"` + OUID string `json:"ouId,omitempty"` + OUHandle string `json:"ouHandle,omitempty"` -type UserPrincipal struct { - // Subject is the JWT "sub" claim: the identity provider's ID for the user. - // It is NOT the internally persisted user ID a UserProfileService returns - // — see UserContext, which carries both as ID and IDPUserID. - Subject string `json:"subject"` - Roles []string `json:"roles"` - Scopes []string `json:"scopes"` ExtraClaims ExtraClaims `json:"extraClaims,omitempty"` } -type Principal struct { - Type PrincipalType `json:"type"` - UserPrincipal *UserPrincipal `json:"userPrincipal,omitempty"` - ClientPrincipal *ClientPrincipal `json:"clientPrincipal,omitempty"` -} - type jwksResponse struct { Keys []jwk `json:"keys"` } @@ -108,26 +100,15 @@ type jwk struct { const defaultJWKSCacheTTL = 5 * time.Minute // TokenExtractor handles token extraction and parsing from HTTP headers. -// It validates JWT signatures using JWKS and resolves a user principal -// or client principal based on grant type. +// It validates JWT signatures using JWKS and resolves the token's Principal. type TokenExtractor struct { jwksURL string expIssuer string expAudience string httpClient *http.Client - // userClaims / clientClaims are the raw declarations accumulated from - // WithUserClaims / WithClientClaims. They are validated and flattened into - // userExtraClaims / clientExtraClaims (name -> required) by validateConfig, - // so declaration order never affects the outcome. - userClaims ClaimSpec - clientClaims ClaimSpec - userExtraClaims map[string]bool - clientExtraClaims map[string]bool - - // rolesClaim is the claim name roles are read from; defaultRolesClaim - // unless overridden by WithRolesClaim. - rolesClaim string + // claims configures the standard identity claims; see WithClaims. + claims ClaimsConfig cacheMu sync.RWMutex cachedJWKS *jwksResponse @@ -147,7 +128,7 @@ func NewTokenExtractorWithClient(jwksURL, issuer, audience string, httpClient *h } // newExtractor is the single construction path, so a defaulted field -// (rolesClaim) can only be forgotten in one place. +// (jwksCacheTTL) can only be forgotten in one place. func newExtractor(jwksURL, issuer, audience string, httpClient *http.Client, opts ...Option) (*TokenExtractor, error) { extractor := &TokenExtractor{ jwksURL: strings.TrimSpace(jwksURL), @@ -155,7 +136,6 @@ func newExtractor(jwksURL, issuer, audience string, httpClient *http.Client, opt expAudience: strings.TrimSpace(audience), jwksCacheTTL: defaultJWKSCacheTTL, httpClient: httpClient, - rolesClaim: defaultRolesClaim, } for _, opt := range opts { @@ -182,39 +162,13 @@ func (te *TokenExtractor) validateConfig() error { if te.httpClient == nil { return fmt.Errorf("http client is not configured") } - - // Claim declarations are judged here rather than inside the options, so - // that WithRolesClaim and WithUserClaims/WithClientClaims produce the same - // result whichever order they were passed in. - if err := validateRolesClaimName(te.rolesClaim); err != nil { - return err - } - te.rolesClaim = strings.TrimSpace(te.rolesClaim) - - for _, decl := range []struct { - what string - names []string - }{ - {"WithUserClaims optional", te.userClaims.Optional}, - {"WithUserClaims required", te.userClaims.Required}, - {"WithClientClaims optional", te.clientClaims.Optional}, - {"WithClientClaims required", te.clientClaims.Required}, - } { - if err := validateClaimNames(decl.what, decl.names, te.rolesClaim); err != nil { - return err - } - } - - te.userExtraClaims = te.userClaims.resolve() - te.clientExtraClaims = te.clientClaims.resolve() - return nil } // ExtractPrincipalFromHeader extracts the principal from Authorization header. // Expected header format: "Bearer ". // JWT signature is validated against configured JWKS endpoint, then claims are -// mapped into either UserPrincipal or ClientPrincipal. +// mapped into a Principal. func (te *TokenExtractor) ExtractPrincipalFromHeader(authHeader string) (*Principal, error) { if authHeader == "" { return nil, fmt.Errorf("authorization header is empty") @@ -246,63 +200,46 @@ func (te *TokenExtractor) ExtractPrincipalFromHeader(authHeader string) (*Princi return nil, fmt.Errorf("jwt missing exp claim") } + if claims.Subject == "" { + return nil, fmt.Errorf("jwt missing sub claim") + } + if claims.ClientID == "" { return nil, fmt.Errorf("jwt missing client_id claim") } - switch claims.GrantType { - case AuthorizationCodeGrant: - extra, roles, err := te.resolveClaims(tokenString, claims, te.userExtraClaims) - if err != nil { - return nil, err - } - userPrincipal, err := te.userPrincipalFromClaims(claims, extra, roles) - if err != nil { - return nil, err - } - return &Principal{ - Type: UserPrincipalType, - UserPrincipal: userPrincipal, - }, nil - case ClientCredentialsGrant: - extra, roles, err := te.resolveClaims(tokenString, claims, te.clientExtraClaims) - if err != nil { - return nil, err - } - clientPrincipal, err := te.clientPrincipalFromClaims(claims, extra, roles) - if err != nil { - return nil, err + email := strings.TrimSpace(claims.Email) + ouID := strings.TrimSpace(claims.OUID) + ouHandle := strings.TrimSpace(claims.OUHandle) + for _, c := range []struct { + name string + config ClaimConfig + value string + }{ + {"email", te.claims.Email, email}, + {"ouId", te.claims.OUID, ouID}, + {"ouHandle", te.claims.OUHandle, ouHandle}, + } { + if c.config.Required && c.value == "" { + // Claim NAMES only, never values: this error reaches the + // middleware's Debug log with request context. + return nil, fmt.Errorf("jwt missing or empty required claim %q", c.name) } - return &Principal{ - Type: ClientPrincipalType, - ClientPrincipal: clientPrincipal, - }, nil - default: - return nil, fmt.Errorf("unsupported grant type: %q", claims.GrantType) } -} -func (te *TokenExtractor) userPrincipalFromClaims(claims *tokenClaims, extra ExtraClaims, roles []string) (*UserPrincipal, error) { - if claims.Subject == "" { - return nil, fmt.Errorf("jwt missing sub claim for user principal") + extra, roles, err := resolveClaims(tokenString, claims) + if err != nil { + return nil, err } - return &UserPrincipal{ + return &Principal{ Subject: claims.Subject, - Roles: roles, - Scopes: []string(claims.Scopes), - ExtraClaims: extra, - }, nil -} - -func (te *TokenExtractor) clientPrincipalFromClaims(claims *tokenClaims, extra ExtraClaims, roles []string) (*ClientPrincipal, error) { - if claims.ClientID == "" { - return nil, fmt.Errorf("jwt missing client_id claim for client principal") - } - return &ClientPrincipal{ ClientID: claims.ClientID, Roles: roles, - Scopes: []string(claims.Scopes), + Scopes: claims.Scopes, + Email: email, + OUID: ouID, + OUHandle: ouHandle, ExtraClaims: extra, }, nil } diff --git a/authn/token_parser_test.go b/authn/token_parser_test.go index 6e3665c2..65bc4a1f 100644 --- a/authn/token_parser_test.go +++ b/authn/token_parser_test.go @@ -8,6 +8,7 @@ import ( "crypto/rsa" "encoding/base64" "encoding/json" + "fmt" "math/big" "net/http" "net/http/httptest" @@ -26,13 +27,13 @@ func TestTokenExtractor_ExtractPrincipalFromHeader(t *testing.T) { validClientToken := newClientToken(t, privateKey) tests := []struct { - name string - authHeader string - want string - wantErr bool + name string + authHeader string + wantSubject string + wantErr bool }{ - {name: "valid user token", authHeader: "Bearer " + validUserToken, want: testUserID}, - {name: "valid client token", authHeader: "Bearer " + validClientToken, want: testClientID}, + {name: "valid user token", authHeader: "Bearer " + validUserToken, wantSubject: testUserID}, + {name: "valid client token", authHeader: "Bearer " + validClientToken, wantSubject: "FCAU_TO_NSW"}, {name: "missing header", authHeader: "", wantErr: true}, {name: "invalid token", authHeader: "Bearer invalid.jwt.token", wantErr: true}, } @@ -46,15 +47,9 @@ func TestTokenExtractor_ExtractPrincipalFromHeader(t *testing.T) { if tt.wantErr { return } - got := "" - if principal != nil && principal.UserPrincipal != nil { - got = principal.UserPrincipal.Subject - } - if principal != nil && principal.ClientPrincipal != nil { - got = principal.ClientPrincipal.ClientID - } - if got != tt.want { - t.Fatalf("unexpected principal id: got %q want %q", got, tt.want) + if principal.Subject != tt.wantSubject || principal.ClientID != testClientID { + t.Fatalf("unexpected principal: got (%q, %q) want (%q, %q)", + principal.Subject, principal.ClientID, tt.wantSubject, testClientID) } }) } @@ -84,7 +79,7 @@ func TestTokenExtractor_ExtractPrincipalFromHeader_MissingClaims(t *testing.T) { defer cleanup() baseClaims := func() jwt.MapClaims { - claims := newBaseClaims(AuthorizationCodeGrant) + claims := newBaseClaims() claims["sub"] = testUserID claims["email"] = testEmail claims["phone_number"] = testPhone @@ -113,19 +108,12 @@ func TestTokenExtractor_ExtractPrincipalFromHeader_MissingClaims(t *testing.T) { }, errSubstr: "jwt missing client_id claim", }, - { - name: "unsupported grant_type", - mutate: func(claims jwt.MapClaims) { - claims["grant_type"] = "password" - }, - errSubstr: "unsupported grant type", - }, { name: "missing sub", mutate: func(claims jwt.MapClaims) { delete(claims, "sub") }, - errSubstr: "jwt missing sub claim for user principal", + errSubstr: "jwt missing sub claim", }, } @@ -142,56 +130,57 @@ func TestTokenExtractor_ExtractPrincipalFromHeader_MissingClaims(t *testing.T) { } } -func TestUserPrincipalFromClaims(t *testing.T) { - claims := &tokenClaims{RegisteredClaims: jwt.RegisteredClaims{Subject: testUserID}} - extra := ExtraClaims{"email": testEmail, "ouId": testOUID, "ouHandle": testOUHandle} - - principal, err := (&TokenExtractor{}).userPrincipalFromClaims(claims, extra, []string{"exporter"}) - if err != nil { - t.Fatalf("userPrincipalFromClaims() error = %v", err) - } - if principal.Subject != testUserID { - t.Fatalf("unexpected principal: %#v", principal) - } - if len(principal.Roles) != 1 || principal.Roles[0] != "exporter" { - t.Fatalf("unexpected roles: %#v", principal.Roles) - } - if principal.ExtraClaims.String("email") != testEmail || principal.ExtraClaims.String("ouId") != testOUID || principal.ExtraClaims.String("ouHandle") != testOUHandle { - t.Fatalf("unexpected extra claims mapping: %#v", principal.ExtraClaims) - } +// TestTokenExtractor_IgnoresGrantType locks in that how a token was obtained +// has no bearing on whether it is accepted or on the Principal it yields: +// grant_type used to dispatch between user and client principals and reject +// anything else. +func TestTokenExtractor_IgnoresGrantType(t *testing.T) { + extractor, privateKey, cleanup := newTokenExtractor(t) + defer cleanup() - // sub is the only claim userPrincipalFromClaims itself still requires: - // email/ouId/ouHandle moved to the generic extra-claims mechanism (see - // TestTokenExtractor_UserToken_NoLongerRequiresEmailOrOU) and are no - // longer validated here at all. - t.Run("missing sub is an error", func(t *testing.T) { - if _, err := (&TokenExtractor{}).userPrincipalFromClaims(&tokenClaims{}, nil, nil); err == nil { - t.Fatalf("expected error for missing sub claim") + for _, grantType := range []any{nil, "authorization_code", "client_credentials", "password", 42} { + claims := newBaseClaims() + if grantType != nil { + claims["grant_type"] = grantType } - }) - - t.Run("nil extra claims is fine", func(t *testing.T) { - claims := &tokenClaims{RegisteredClaims: jwt.RegisteredClaims{Subject: testUserID}} - principal, err := (&TokenExtractor{}).userPrincipalFromClaims(claims, nil, nil) + principal, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) if err != nil { - t.Fatalf("userPrincipalFromClaims() error = %v", err) + t.Fatalf("grant_type %v: expected success, got %v", grantType, err) } - if principal.ExtraClaims != nil { - t.Fatalf("expected nil extra claims, got %#v", principal.ExtraClaims) + if principal.Subject != testUserID || principal.ClientID != testClientID { + t.Fatalf("grant_type %v: unexpected principal %#v", grantType, principal) } - }) + } +} + +// TestTokenExtractor_GrantTypeIsAnOrdinaryClaim covers the other half of +// dropping dispatch: grant_type is no longer reserved, so it passes through to +// ExtraClaims like any other claim for a consumer that cares. +func TestTokenExtractor_GrantTypeIsAnOrdinaryClaim(t *testing.T) { + extractor, privateKey, cleanup := newTokenExtractor(t) + defer cleanup() + + claims := newBaseClaims() + claims["grant_type"] = "client_credentials" + + principal, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) + if err != nil { + t.Fatalf("expected success, got %v", err) + } + if got := principal.ExtraClaims.String("grant_type"); got != "client_credentials" { + t.Fatalf("ExtraClaims grant_type = %q, want client_credentials", got) + } } // TestTokenExtractor_UserToken_NoLongerRequiresEmailOrOU documents a // deliberate behavior change: email/ouId/ouHandle used to be hard-required // fixed-schema claims. They are now ordinary extra claims, so a token that -// omits them entirely is accepted unless a consumer opts into requiring them -// via WithUserClaims(ClaimSpec{Required: ...}). +// omits them entirely is accepted; requiring them is the application's job. func TestTokenExtractor_UserToken_NoLongerRequiresEmailOrOU(t *testing.T) { extractor, privateKey, cleanup := newTokenExtractor(t) defer cleanup() - claims := newBaseClaims(AuthorizationCodeGrant) + claims := newBaseClaims() claims["sub"] = testUserID // Deliberately omit email/phone_number/ouId/ouHandle/roles entirely. @@ -208,14 +197,14 @@ func TestTokenExtractor_AcceptsAnyClientIDForAudience(t *testing.T) { extractor, privateKey, cleanup := newTokenExtractor(t) defer cleanup() - claims := newBaseClaims(ClientCredentialsGrant) + claims := newBaseClaims() claims["client_id"] = "SOME_OTHER_CLIENT" principal, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) if err != nil { t.Fatalf("expected success for a token issued for this audience, got %v", err) } - if principal.ClientPrincipal == nil || principal.ClientPrincipal.ClientID != "SOME_OTHER_CLIENT" { + if principal.ClientID != "SOME_OTHER_CLIENT" { t.Fatalf("unexpected principal: %#v", principal) } } @@ -243,7 +232,7 @@ func TestTokenExtractor_JWKSIsCached(t *testing.T) { })) defer jwksServer.Close() - claims := newBaseClaims(AuthorizationCodeGrant) + claims := newBaseClaims() claims["sub"] = testUserID claims["email"] = testEmail claims["phone_number"] = testPhone @@ -311,7 +300,7 @@ func TestTokenExtractor_RefreshesJWKSOnUnknownKid(t *testing.T) { })) defer jwksServer.Close() - oldClaims := newBaseClaims(AuthorizationCodeGrant) + oldClaims := newBaseClaims() oldClaims["sub"] = testUserID oldClaims["email"] = testEmail oldClaims["phone_number"] = testPhone @@ -326,7 +315,7 @@ func TestTokenExtractor_RefreshesJWKSOnUnknownKid(t *testing.T) { t.Fatalf("failed to sign old token: %v", err) } - newClaims := newBaseClaims(AuthorizationCodeGrant) + newClaims := newBaseClaims() newClaims["sub"] = testUserID newClaims["email"] = testEmail newClaims["phone_number"] = testPhone @@ -380,7 +369,7 @@ func TestTokenExtractor_UnknownKidAfterRefresh(t *testing.T) { })) defer jwksServer.Close() - claims := newBaseClaims(AuthorizationCodeGrant) + claims := newBaseClaims() claims["sub"] = testUserID claims["email"] = testEmail claims["phone_number"] = testPhone @@ -416,7 +405,7 @@ func TestTokenExtractor_JWKSFetchError(t *testing.T) { })) defer jwksServer.Close() - claims := newBaseClaims(AuthorizationCodeGrant) + claims := newBaseClaims() claims["sub"] = testUserID claims["email"] = testEmail claims["phone_number"] = testPhone @@ -497,3 +486,93 @@ func TestParseRSAPublicKey_InvalidData(t *testing.T) { }) } } + +func TestTokenExtractor_StandardClaims(t *testing.T) { + extractor, privateKey, cleanup := newTokenExtractor(t) + defer cleanup() + + t.Run("populated and trimmed", func(t *testing.T) { + claims := newBaseClaims() + claims["email"] = " " + testEmail + " \n" + claims["ouId"] = testOUID + claims["ouHandle"] = testOUHandle + + p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) + if err != nil { + t.Fatalf("extract: %v", err) + } + if p.Email != testEmail || p.OUID != testOUID || p.OUHandle != testOUHandle { + t.Fatalf("unexpected standard claims: %+v", p) + } + }) + + t.Run("optional by default: absent or blank reads as empty", func(t *testing.T) { + claims := newBaseClaims() + claims["ouId"] = " " + + p, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) + if err != nil { + t.Fatalf("expected success with no standard claims, got %v", err) + } + if p.Email != "" || p.OUID != "" || p.OUHandle != "" { + t.Fatalf("expected empty standard claims, got %+v", p) + } + }) + + t.Run("a non-string value rejects the token", func(t *testing.T) { + claims := newBaseClaims() + claims["email"] = 42 + + if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)); err == nil { + t.Fatal("expected error for a non-string email claim") + } + }) +} + +func TestTokenExtractor_RequiredStandardClaims(t *testing.T) { + tests := []struct { + name string + config ClaimsConfig + claim string + }{ + {"email", ClaimsConfig{Email: ClaimConfig{Required: true}}, "email"}, + {"ouId", ClaimsConfig{OUID: ClaimConfig{Required: true}}, "ouId"}, + {"ouHandle", ClaimsConfig{OUHandle: ClaimConfig{Required: true}}, "ouHandle"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, WithClaims(tt.config)) + defer cleanup() + + for _, value := range []any{nil, "", " "} { + claims := newBaseClaims() + if value != nil { + claims[tt.claim] = value + } + _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)) + want := fmt.Sprintf("jwt missing or empty required claim %q", tt.claim) + if err == nil || err.Error() != want { + t.Fatalf("value %#v: expected %q, got %v", value, want, err) + } + } + + claims := newBaseClaims() + claims[tt.claim] = "present" + if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + signToken(t, privateKey, claims)); err != nil { + t.Fatalf("expected success when %s is present, got %v", tt.claim, err) + } + }) + } + + // Required applies to every token: a client-credentials token without an + // email is rejected too. + t.Run("applies to client tokens", func(t *testing.T) { + extractor, privateKey, cleanup := newTokenExtractorWithOptions(t, + WithClaims(ClaimsConfig{Email: ClaimConfig{Required: true}})) + defer cleanup() + + if _, err := extractor.ExtractPrincipalFromHeader("Bearer " + newClientToken(t, privateKey)); err == nil { + t.Fatal("expected a client token without email to be rejected") + } + }) +} diff --git a/authn/utils.go b/authn/utils.go index 397deeab..beb6c4e4 100644 --- a/authn/utils.go +++ b/authn/utils.go @@ -10,41 +10,6 @@ import ( "math/big" ) -func buildAuthContext(principal *Principal) *AuthContext { - if principal == nil { - return &AuthContext{} - } - - switch principal.Type { - case UserPrincipalType: - if principal.UserPrincipal == nil { - return &AuthContext{} - } - return &AuthContext{ - User: &UserContext{ - IDPUserID: principal.UserPrincipal.Subject, - Roles: principal.UserPrincipal.Roles, - Scopes: principal.UserPrincipal.Scopes, - ExtraClaims: principal.UserPrincipal.ExtraClaims, - }, - } - case ClientPrincipalType: - if principal.ClientPrincipal == nil { - return &AuthContext{} - } - return &AuthContext{ - Client: &ClientContext{ - ClientID: principal.ClientPrincipal.ClientID, - Roles: principal.ClientPrincipal.Roles, - Scopes: principal.ClientPrincipal.Scopes, - ExtraClaims: principal.ClientPrincipal.ExtraClaims, - }, - } - default: - return &AuthContext{} - } -} - func parseRSAPublicKey(key jwk) (*rsa.PublicKey, error) { if key.Kty != "RSA" { return nil, fmt.Errorf("unsupported jwk key type: %s", key.Kty)