From b289a81b8c73a8ee0f007ccdf680684d253dff90 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 25 Oct 2026 22:16:58 +0800 Subject: [PATCH 1/5] fix(server)!: confine requests naming no resource to the client's AllowedResources A request naming no RFC 8707 resource got an access token addressed to the issuer, and AllowedResources was never checked against it. Every resource server accepting the issuer as an audience (one migrating with AcceptIssuerAudience or the issuer in AdditionalAudiences, and the UserInfo endpoint) took that token, so a client refused a resource could reach it by naming none. For a client with AllowedResources, a request naming no resource now gets a default: with one entry, it is for that resource (recorded on the grant and checked against Config.Resources and the ResourcePolicy as a named one is); with several, it is refused with invalid_target. The default applies wherever a resource can be named (PAR, CIBA, client_credentials, IssueAccessToken and IssueRefreshToken) and to a grant made without resources before the client was confined, at its next code exchange, refresh or CIBA token request, so neither the embedder APIs nor an older grant can produce an issuer-audience token for a confined client. Clients without AllowedResources, and servers with resource indicators disabled, are unchanged. RegisteredClient.AllowedResources returns the registered list. BREAKING CHANGE: a client registered with AllowedResources no longer gets issuer-audience access tokens when resource indicators are enabled. With a single entry, a request naming no resource is for that resource; with several, it fails with invalid_target. Such a client that calls the UserInfo endpoint must list the issuer identifier in its AllowedResources (and the server must accept it as a resource), then name a resource in every request. See UPGRADING.md. Co-Authored-By: Claude Opus 5.5 --- fapitest/resource_indicators_allowed_test.go | 91 +++++++ fapitest/resource_indicators_test.go | 11 +- server/config.go | 6 +- server/embedder_access_token.go | 9 +- server/embedder_refresh.go | 6 +- server/resource_indicators.go | 36 ++- server/resource_indicators_default_test.go | 263 +++++++++++++++++++ serverresource/serverresource.go | 4 +- storage/client_repository.go | 39 ++- storage/client_repository_test.go | 6 + 10 files changed, 452 insertions(+), 19 deletions(-) create mode 100644 fapitest/resource_indicators_allowed_test.go create mode 100644 server/resource_indicators_default_test.go diff --git a/fapitest/resource_indicators_allowed_test.go b/fapitest/resource_indicators_allowed_test.go new file mode 100644 index 00000000..4bbb7389 --- /dev/null +++ b/fapitest/resource_indicators_allowed_test.go @@ -0,0 +1,91 @@ +package fapitest_test + +import ( + "context" + "fmt" + "testing" + + "github.com/idfoundry/fapigo/client" + "github.com/idfoundry/fapigo/fapitest" + "github.com/idfoundry/fapigo/server" + "github.com/idfoundry/fapigo/serverresource" +) + +// TestResourceIndicatorsAllowedResourcesDefault: a client confined by +// AllowedResources to one resource server that names no resource gets +// tokens for that resource, end to end, never ones addressed to the +// issuer, which another resource server migrating with +// AcceptIssuerAudience (or the issuer's own, UserInfo say) would accept. +func TestResourceIndicatorsAllowedResourcesDefault(t *testing.T) { + for _, opaque := range []bool{false, true} { + t.Run(fmt.Sprintf("opaque=%v", opaque), func(t *testing.T) { + h := fapitest.New(t, fapitest.Config{ + Profile: server.ProfileFAPISecurity, Resources: servedResources, + AllowedResources: []string{credentials}, OpaqueAccessTokens: opaque, + }) + ctx := context.Background() + _, err := h.RunAuthorizationCodeFlowWithRequest(ctx, client.BeginAuthorizationRequest{Scope: offlineScope, Resource: []string{payments}}) + wantServerError(t, "RunAuthorizationCodeFlow(payments)", err, "invalid_target") + + tokens, err := h.RunAuthorizationCodeFlow(ctx, offlineScope) + if err != nil { + t.Fatalf("RunAuthorizationCodeFlow: %v", err) + } + checkConfinedToCredentials(t, h, tokens, opaque) + + // A refresh naming no resource gets the grant's: the default. + refreshed, err := h.Client.RefreshTokens(ctx, client.RefreshTokenRequest{Tokens: tokens}) + if err != nil { + t.Fatalf("RefreshTokens: %v", err) + } + checkConfinedToCredentials(t, h, refreshed, opaque) + }) + } +} + +// TestResourceIndicatorsAllowedResourcesDefaultClientCredentials is +// TestResourceIndicatorsAllowedResourcesDefault with no user involved. +func TestResourceIndicatorsAllowedResourcesDefaultClientCredentials(t *testing.T) { + h := fapitest.New(t, fapitest.Config{ + Profile: server.ProfileFAPISecurity, ClientCredentialsGrant: true, + Resources: servedResources, AllowedResources: []string{credentials}, + }) + result, err := h.Client.RequestClientCredentialsToken(context.Background(), client.ClientCredentialsTokenRequest{Scope: []string{"accounts"}}) + if err != nil { + t.Fatalf("RequestClientCredentialsToken: %v", err) + } + checkConfinedToCredentials(t, h, client.TokenSet{AccessToken: result.AccessToken}, false) +} + +// TestResourceIndicatorsAllowedResourcesRequired: a client confined to +// several resource servers must name which a request is for. +func TestResourceIndicatorsAllowedResourcesRequired(t *testing.T) { + h := fapitest.New(t, fapitest.Config{ + Profile: server.ProfileFAPISecurity, ClientCredentialsGrant: true, + Resources: servedResources, AllowedResources: []string{credentials, payments}, + }) + ctx := context.Background() + _, err := h.RunAuthorizationCodeFlow(ctx, offlineScope) + wantServerError(t, "RunAuthorizationCodeFlow", err, "invalid_target") + _, err = h.Client.RequestClientCredentialsToken(ctx, client.ClientCredentialsTokenRequest{Scope: []string{"accounts"}}) + wantServerError(t, "RequestClientCredentialsToken", err, "invalid_target") + + tokens, err := h.RunAuthorizationCodeFlowWithRequest(ctx, client.BeginAuthorizationRequest{Scope: offlineScope, Resource: []string{payments}}) + if err != nil { + t.Fatalf("RunAuthorizationCodeFlow(payments): %v", err) + } + checkVerifierAudience(t, h, tokens, payments, []string{payments}) +} + +// checkConfinedToCredentials checks tokens are for credentials alone: +// accepted there, and refused by payments even while it accepts the +// issuer as an audience, and by the issuer's own resource server. +func checkConfinedToCredentials(t *testing.T, h *fapitest.Harness, tokens client.TokenSet, opaque bool) { + t.Helper() + if !opaque { + checkJWTAudience(t, tokens, `"`+credentials+`"`) + } + checkVerifierAudience(t, h, tokens, credentials, []string{credentials}) + refuseAtResource(t, h, tokens, serverresource.Options{Resource: payments, AcceptIssuerAudience: true}) + refuseAt(t, h, h.Resource, tokens) +} diff --git a/fapitest/resource_indicators_test.go b/fapitest/resource_indicators_test.go index 467a1545..972d3f74 100644 --- a/fapitest/resource_indicators_test.go +++ b/fapitest/resource_indicators_test.go @@ -422,13 +422,12 @@ func TestResourceIndicatorsClientCredentials(t *testing.T) { refuseAtResource(t, h, tokens, serverresource.Options{Resource: payments}) }) + // Confined to several resources, the client must name + // which: a token addressed to the issuer would be accepted + // wherever the issuer is. t.Run("no resource", func(t *testing.T) { - tokens, err := request(nil) - if err != nil { - t.Fatalf("RequestClientCredentialsToken: %v", err) - } - verifyAt(t, h, h.Resource, tokens) - refuseAtResource(t, h, tokens, serverresource.Options{Resource: accounts}) + _, err := request(nil) + wantServerError(t, "RequestClientCredentialsToken", err, "invalid_target") }) for _, tc := range []struct { diff --git a/server/config.go b/server/config.go index da8c821e..1e1a183d 100644 --- a/server/config.go +++ b/server/config.go @@ -362,7 +362,7 @@ type Limits struct { type Config struct { // Issuer is this server's issuer identifier — the audience client // assertions and request objects must be addressed to, and every - // access token's audience unless the client named resources (RFC + // access token's audience unless the token is for resources (RFC // 8707 resource indicators; see Resources). Issuer fapi.URL @@ -487,7 +487,9 @@ type Config struct { // neither set, the "resource" parameter is ignored, as any // unregistered parameter is, and every access token's audience is // Issuer. A request naming no resource also gets Issuer as its - // audience. + // audience — unless its client is confined by AllowedResources, + // whose single entry it is then for, or which, listing several, + // make naming one required (invalid_target otherwise). Resources []string // AttestationBasedClientAuthentication enables diff --git a/server/embedder_access_token.go b/server/embedder_access_token.go index 747bda2a..08ce9881 100644 --- a/server/embedder_access_token.go +++ b/server/embedder_access_token.go @@ -43,9 +43,10 @@ type IssueAccessTokenRequest struct { AuthorizationDetails []json.RawMessage // Resources are the RFC 8707 resource indicators the access token is - // restricted to, checked as IssueRefreshTokenRequest.Resources are: - // its "aud" is them instead of the issuer. Give a refresh token - // issued for the same grant the same Resources. + // restricted to, checked, and defaulted when empty, as + // IssueRefreshTokenRequest.Resources are: its "aud" is them instead + // of the issuer. Give a refresh token issued for the same grant the + // same Resources. Resources []string // GrantID, if set, names the grant, as IssueRefreshTokenRequest's @@ -59,7 +60,7 @@ type IssueAccessTokenRequest struct { // IssueAccessToken issues an access token for a grant the embedder // served itself (Config.AdditionalGrantTypes), as this server's own // token endpoint would: through Dependencies.AccessTokens, with this -// server's issuer, its audience (req.Resources, or the issuer) and +// server's issuer, its audience (req.Resources, or their default) and // Limits.AccessTokenLifetime, bound to // req.Binding, and carrying the same claims as the access tokens a // refresh token for the grant (IssueRefreshToken) later issues. Use it diff --git a/server/embedder_refresh.go b/server/embedder_refresh.go index d00850d1..62720399 100644 --- a/server/embedder_refresh.go +++ b/server/embedder_refresh.go @@ -56,7 +56,11 @@ type IssueRefreshTokenRequest struct { // access token a refresh issues is restricted to them, or to the // subset the refresh request names. Typically the "resource" // parameters of the token request the embedder served (see - // TokenEndpointRequest.Resources). Empty, the audience is the issuer. + // TokenEndpointRequest.Resources). Empty, the audience is the + // issuer, unless the client is confined by AllowedResources: then + // it's the client's single allowed resource, or, when it has + // several, invalid_target (see + // storage.RegisteredClientConfig.AllowedResources). Resources []string // GrantID, if set, names the grant so RevokeGrant can revoke it, as diff --git a/server/resource_indicators.go b/server/resource_indicators.go index 87b14bb6..ecf20242 100644 --- a/server/resource_indicators.go +++ b/server/resource_indicators.go @@ -190,10 +190,15 @@ func hexRune(raw []byte, i int) (rune, bool) { // named: each an absolute URI without a fragment (RFC 8707 §2), within // the client's AllowedResources, and listed in Config.Resources or // accepted by the ResourcePolicy. It returns them de-duplicated in the -// order given. Nil in, nil out. +// order given. A request naming none is first given the client's +// default (defaultResources), which is then checked the same way; nil +// when there is none. func (s *Server) checkRequestedResources(ctx context.Context, client storage.RegisteredClient, requested []string) ([]string, *Error) { if len(requested) == 0 { - return nil, nil + var err *Error + if requested, err = s.defaultResources(client); err != nil || len(requested) == 0 { + return nil, err + } } if len(requested) > resourceindicator.MaxPerRequest { return nil, newError(ErrorInvalidTarget, 400, fmt.Sprintf("at most %d resource indicators may be requested", resourceindicator.MaxPerRequest), nil) @@ -231,6 +236,26 @@ func (s *Server) checkRequestedResources(ctx context.Context, client storage.Reg } } +// defaultResources is what a request from client naming no resource is +// for. A client without AllowedResources gets none, so its tokens are +// addressed to the issuer. A client confined by AllowedResources can't +// be given an issuer-audience token, which every resource server that +// accepts the issuer as an audience would take, whatever the list says: +// with one entry, the request is for that one; with more, it must name +// which, so it's invalid_target. With resource indicators disabled, +// there is never a default: the "resource" parameter is ignored, and +// every token is addressed to the issuer. +func (s *Server) defaultResources(client storage.RegisteredClient) ([]string, *Error) { + if !s.resourceIndicatorsEnabled() { + return nil, nil + } + allowed := client.AllowedResources() + if len(allowed) > 1 { + return nil, newError(ErrorInvalidTarget, 400, "a resource is required: this client may request more than one, so must name which", nil) + } + return allowed, nil +} + // narrowResources returns the resources a token request may have its // access token restricted to: requested, when it named any, which must // all be among granted (RFC 8707 §2.2: a token request may narrow the @@ -287,12 +312,17 @@ func (s *Server) usableGrantAudience(ctx context.Context, client storage.Registe // use. When none is — resource indicators turned off, say — it must // authorize again. The refresh token keeps all of the grant's // resources, so one allowed again later can be named again. +// +// A grant without resources, made before the client was confined by +// AllowedResources, gets the client's default as a request naming none +// would (defaultResources): its single allowed resource, or +// invalid_target when it has several, so it must authorize again. func (s *Server) narrowedAudience(ctx context.Context, client storage.RegisteredClient, granted, requested []string) (string, []string, *Error) { resources, err := narrowResources(granted, requested) if err != nil { return "", nil, err } - if _, err := s.checkRequestedResources(ctx, client, resources); err != nil { + if resources, err = s.checkRequestedResources(ctx, client, resources); err != nil { return "", nil, err } audience, audiences := s.accessTokenAudience(resources) diff --git a/server/resource_indicators_default_test.go b/server/resource_indicators_default_test.go new file mode 100644 index 00000000..13f23b1a --- /dev/null +++ b/server/resource_indicators_default_test.go @@ -0,0 +1,263 @@ +package server_test + +import ( + "context" + "reflect" + "testing" + "time" + + fapi "github.com/idfoundry/fapigo" + "github.com/idfoundry/fapigo/server" + "github.com/idfoundry/fapigo/storage" + "github.com/idfoundry/fapigo/storage/memstore" +) + +// A client confined by AllowedResources to one resource that names none +// gets tokens for that resource, never ones addressed to the issuer: a +// resource server accepting the issuer as an audience (migrating with it +// in AdditionalAudiences, or the issuer's own) would take those whatever +// AllowedResources says. +func TestResourceIndicatorsAllowedResourcesSingleIsDefault(t *testing.T) { + h := withResources(t, newHarness(t, server.ProfileFAPISecurity, true), []string{resourceA, resourceB}, nil) + restrictClientResources(t, h, resourceA) + + _, err := pushWithResources(t, h, resourceB) + wantErrorCode(t, err, server.ErrorInvalidTarget) + + dpopKey := generateKey(t) + result, err := h.server.ExchangeAuthorizationCode(context.Background(), server.AuthorizationCodeExchangeRequest{ + HTTP: server.FormRequest{Parameters: exchangeFormParams(h.clientAssertion(t), grantForResources(t, h), testRedirectURI, testCodeVerifier)}, + DPoPProofs: []string{createDPoPProof(t, dpopKey, h.now)}, + }) + if err != nil { + t.Fatalf("ExchangeAuthorizationCode: %v", err) + } + wantAudience(t, result, resourceA) + for name, verifier := range map[string]func() error{ + "resource B, also accepting the issuer": func() error { + return verifyWithDPoP(t, audienceVerifier(t, h, nil, resourceB, testIssuer), dpopKey, result.AccessToken.Reveal(), h.now) + }, + "the issuer": func() error { + return verifyWithDPoP(t, audienceVerifier(t, h, nil, testIssuer), dpopKey, result.AccessToken.Reveal(), h.now) + }, + } { + if verifier() == nil { + t.Errorf("%s accepted a token of a client confined to resource A", name) + } + } + if err := verifyWithDPoP(t, audienceVerifier(t, h, nil, resourceA), dpopKey, result.AccessToken.Reveal(), h.now); err != nil { + t.Errorf("resource A refused the token: %v", err) + } + + // The grant recorded the default, so it holds even once the client + // may name more. + restrictClientResources(t, h, resourceA, resourceB) + refreshed, err := refreshWithResources(t, h, result.RefreshToken.Reveal()) + if err != nil { + t.Fatalf("refresh: %v", err) + } + wantAudience(t, refreshed, resourceA) + _, err = refreshWithResources(t, h, result.RefreshToken.Reveal(), resourceB) + wantErrorCode(t, err, server.ErrorInvalidTarget) +} + +// A client confined to several resources must name which a request is +// for. +func TestResourceIndicatorsAllowedResourcesSeveralRequireOne(t *testing.T) { + h := withResources(t, newHarness(t, server.ProfileFAPISecurity, true), []string{resourceA, resourceB}, nil) + restrictClientResources(t, h, resourceA, resourceB) + _, err := pushWithResources(t, h) + wantErrorCode(t, err, server.ErrorInvalidTarget) + if _, err := pushWithResources(t, h, resourceB); err != nil { + t.Fatalf("PushAuthorizationRequest naming one: %v", err) + } +} + +// The default is checked as a resource the request named would be: +// against Config.Resources and the ResourcePolicy. +func TestResourceIndicatorsAllowedResourcesDefaultIsChecked(t *testing.T) { + policy := &recordingResourcePolicy{} + h := withResources(t, newHarness(t, server.ProfileFAPISecurity, true), []string{resourceA}, policy) + restrictClientResources(t, h, resourceC) + if _, err := pushWithResources(t, h); err != nil { + t.Fatalf("PushAuthorizationRequest: %v", err) + } + if want := [][]string{{resourceC}}; !reflect.DeepEqual(policy.asked, want) { + t.Fatalf("policy asked %v, want %v", policy.asked, want) + } + + h = withResources(t, h, []string{resourceA}, nil) + _, err := pushWithResources(t, h) + wantErrorCode(t, err, server.ErrorInvalidTarget) +} + +// With resource indicators disabled AllowedResources has no effect: the +// "resource" parameter is ignored, and every token is addressed to the +// issuer. +func TestResourceIndicatorsAllowedResourcesIgnoredWhenDisabled(t *testing.T) { + h := newHarness(t, server.ProfileFAPISecurity, true) + restrictClientResources(t, h, resourceA, resourceB) + result, err := exchangeWithResources(t, h, grantForResources(t, h)) + if err != nil { + t.Fatalf("ExchangeAuthorizationCode: %v", err) + } + wantAudience(t, result, testIssuer) +} + +// A grant made without resources before its client was confined gets +// the default at its token requests too: the single allowed resource, +// or invalid_target when there are several. +func TestResourceIndicatorsAllowedResourcesDefaultForEarlierGrant(t *testing.T) { + h := withResources(t, newHarness(t, server.ProfileFAPISecurity, true), []string{resourceA, resourceB}, nil) + first, second := grantForResources(t, h), grantForResources(t, h) + issued, err := exchangeWithResources(t, h, grantForResources(t, h)) + if err != nil { + t.Fatalf("ExchangeAuthorizationCode: %v", err) + } + wantAudience(t, issued, testIssuer) + + restrictClientResources(t, h, resourceA) + result, err := exchangeWithResources(t, h, first) + if err != nil { + t.Fatalf("ExchangeAuthorizationCode: %v", err) + } + wantAudience(t, result, resourceA) + refreshed, err := refreshWithResources(t, h, issued.RefreshToken.Reveal()) + if err != nil { + t.Fatalf("refresh: %v", err) + } + wantAudience(t, refreshed, resourceA) + + restrictClientResources(t, h, resourceA, resourceB) + _, err = exchangeWithResources(t, h, second) + wantErrorCode(t, err, server.ErrorInvalidTarget) + _, err = refreshWithResources(t, h, issued.RefreshToken.Reveal()) + wantErrorCode(t, err, server.ErrorInvalidTarget) +} + +func TestResourceIndicatorsAllowedResourcesDefaultCIBA(t *testing.T) { + for name, tc := range map[string]struct { + allowed []string + want any + wantBegin server.ErrorCode + }{ + "one": {allowed: []string{resourceA}, want: resourceA}, + "several": {allowed: []string{resourceA, resourceB}, wantBegin: server.ErrorInvalidTarget}, + } { + t.Run(name, func(t *testing.T) { + h := newHarnessWithBackchannelConfig(t, memstore.NewBackchannelAuthenticationStore(), func(cfg *server.Config) { + cfg.Resources = []string{resourceA, resourceB} + }) + registerClientResources(t, h, true, tc.allowed...) + action := beginResourceCIBA(t, h, nil) + if tc.wantBegin != "" { + local, ok := action.(server.BackchannelAuthenticationLocalError) + if !ok || local.Error.Code() != tc.wantBegin { + t.Fatalf("action = %#v, want a local %q error", action, tc.wantBegin) + } + return + } + required, ok := action.(server.BackchannelInteractionRequired) + if !ok { + t.Fatalf("action = %T, want server.BackchannelInteractionRequired", action) + } + result, err := approveAndPollResourceCIBA(t, h, required, nil) + if err != nil { + t.Fatalf("ExchangeBackchannelAuthentication: %v", err) + } + wantAudience(t, result, tc.want) + }) + } +} + +func TestResourceIndicatorsAllowedResourcesDefaultClientCredentials(t *testing.T) { + for name, tc := range map[string]struct { + allowed []string + want any + wantErr server.ErrorCode + }{ + "one": {allowed: []string{resourceB}, want: resourceB}, + "several": {allowed: []string{resourceA, resourceB}, wantErr: server.ErrorInvalidTarget}, + } { + t.Run(name, func(t *testing.T) { + h := withResources(t, newHarnessWithClientCredentialsGrant(t, storage.SenderConstrainDPoP, true), []string{resourceA, resourceB}, nil) + client, err := storage.NewRegisteredClient(storage.RegisteredClientConfig{ + ID: testClientID, + RedirectURIs: []fapi.RegisteredRedirectURI{testRedirectURI}, + ClientAssertionAlgorithm: fapi.ES256, + SenderConstrain: storage.SenderConstrainDPoP, + AllowedScopes: []string{"accounts"}, + AllowsClientCredentialsGrant: true, + AllowedResources: tc.allowed, + }) + if err != nil { + t.Fatalf("NewRegisteredClient: %v", err) + } + h.clients.clients[testClientID] = client + result, err := h.server.RequestClientCredentialsToken(context.Background(), server.ClientCredentialsTokenRequest{ + HTTP: server.FormRequest{Parameters: clientCredentialsFormParams(h.clientAssertion(t), "accounts")}, + DPoPProofs: []string{createDPoPProof(t, generateKey(t), h.now)}, + }) + if tc.wantErr != "" { + wantErrorCode(t, err, tc.wantErr) + return + } + if err != nil { + t.Fatalf("RequestClientCredentialsToken: %v", err) + } + wantAudience(t, result, tc.want) + }) + } +} + +// The embedder's grants get the same default, so neither a token it +// issues nor one a refresh of its refresh token issues is addressed to +// the issuer for a confined client — even when it gives the refresh +// token fewer resources than the access token. +func TestResourceIndicatorsAllowedResourcesDefaultEmbedder(t *testing.T) { + for name, tc := range map[string]struct { + allowed []string + want any + wantErr server.ErrorCode + }{ + "one": {allowed: []string{resourceA}, want: resourceA}, + "several": {allowed: []string{resourceA, resourceB}, wantErr: server.ErrorInvalidTarget}, + } { + t.Run(name, func(t *testing.T) { + attesterKey := generateKey(t) + h := newAttestationHarness(t, time.Now(), attesterKey, server.RegisteredAttesterKeys{}, func(cfg *server.Config, c *storage.RegisteredClientConfig) { + cfg.AdditionalGrantTypes = []string{preAuthorizedCodeGrant} + cfg.Resources = []string{resourceA, resourceB} + c.AllowedScopes = []string{"openid", "accounts", "offline_access"} + c.AllowedResources = tc.allowed + }) + instance := &attestedInstance{t: t, h: h, attesterKey: attesterKey, instanceKey: generateKey(t)} + req := embedderAccessTokenRequest(t, instance, generateKey(t)) + result, err := h.server.IssueAccessToken(context.Background(), req) + refreshToken, refreshErr := h.server.IssueRefreshToken(context.Background(), server.IssueRefreshTokenRequest(req)) + if tc.wantErr != "" { + wantErrorCode(t, err, tc.wantErr) + wantErrorCode(t, refreshErr, tc.wantErr) + return + } + if err != nil || refreshErr != nil { + t.Fatalf("IssueAccessToken = %v, IssueRefreshToken = %v", err, refreshErr) + } + wantAudience(t, result, tc.want) + attestations, pops := instance.headers() + refreshed, err := h.server.RefreshAccessToken(context.Background(), server.RefreshTokenRequest{ + HTTP: server.FormRequest{Parameters: []server.FormParameter{ + formParam("client_id", testClientID.String()), + formParam("grant_type", "refresh_token"), + formParam("refresh_token", refreshToken.Reveal()), + }}, + DPoPProofs: []string{createDPoPProof(t, generateKey(t), h.now)}, + ClientAttestations: attestations, ClientAttestationPoPs: pops, + }) + if err != nil { + t.Fatalf("refresh: %v", err) + } + wantAudience(t, refreshed, tc.want) + }) + } +} diff --git a/serverresource/serverresource.go b/serverresource/serverresource.go index 6ad85e48..c74a6433 100644 --- a/serverresource/serverresource.go +++ b/serverresource/serverresource.go @@ -85,7 +85,9 @@ type Options struct { // addressed to the server's issuer identifier: those whose client // named no resource. Use it while clients move to sending // "resource", then turn it off — until then any token issued without - // a resource indicator is accepted here too. + // a resource indicator is accepted here too. (A client confined by + // storage.RegisteredClientConfig.AllowedResources is never issued + // one, so it can't reach this resource server that way.) AcceptIssuerAudience bool } diff --git a/storage/client_repository.go b/storage/client_repository.go index e042bed3..877edd9b 100644 --- a/storage/client_repository.go +++ b/storage/client_repository.go @@ -426,8 +426,28 @@ type RegisteredClientConfig struct { // (server.Config.Resources and its ResourcePolicy): set, a resource // outside it is invalid_target whatever the server would otherwise // accept. Empty, the client may name any resource the server - // accepts. Each is an absolute URI without a fragment (RFC 8707 - // §2), compared exactly. + // accepts, and a request naming none gets access tokens addressed + // to the issuer. Each is an absolute URI without a fragment (RFC + // 8707 §2), compared exactly. + // + // Set, it also confines the requests that name no resource, since a + // token addressed to the issuer is accepted by every resource server + // that accepts the issuer as an audience, whatever this list says. + // With exactly one entry, such a request is treated as naming that + // one: the grant records it, and its access tokens are addressed to + // it. With more than one, the request must name which it is for, or + // it's invalid_target. This holds wherever a resource can be named — + // at PAR, a CIBA request, client_credentials and the embedder's + // IssueAccessToken and IssueRefreshToken — and for a grant made + // without resources before the list was set, at its code exchange, + // refresh or CIBA token request. So a client confined here gets no + // issuer-audience access token at all: one that also calls the + // UserInfo endpoint, which accepts only those, must list the issuer + // identifier here too (and the server must accept it as a resource, + // in server.Config.Resources, say), and then, having more than one + // entry, name a resource in every request. The list has no effect while the + // server accepts no resource indicators (neither + // server.Config.Resources nor a ResourcePolicy). AllowedResources []string // AllowsClientCredentialsGrant permits this client to use the RFC @@ -1140,6 +1160,21 @@ func (c RegisteredClient) AllowsResource(resource string) bool { return ok } +// AllowedResources returns the RFC 8707 resource indicators this client +// is confined to, sorted, or nil when it registered none — see +// RegisteredClientConfig.AllowedResources. +func (c RegisteredClient) AllowedResources() []string { + if len(c.allowedResources) == 0 { + return nil + } + resources := make([]string, 0, len(c.allowedResources)) + for r := range c.allowedResources { + resources = append(resources, r) + } + slices.Sort(resources) + return resources +} + // AllowsAuthorizationDetailsType reports whether typ is one of the Rich // Authorization Request types this client is registered for — see // RegisteredClientConfig.AuthorizationDetailsTypes. diff --git a/storage/client_repository_test.go b/storage/client_repository_test.go index eaf1c7f3..aa5d22ad 100644 --- a/storage/client_repository_test.go +++ b/storage/client_repository_test.go @@ -683,6 +683,9 @@ func TestNewRegisteredClientAllowedResources(t *testing.T) { if !unrestricted.AllowsResource("https://rs.example") { t.Error("a client registered with no AllowedResources refuses a resource") } + if unrestricted.AllowedResources() != nil { + t.Errorf("AllowedResources() = %q, want nil", unrestricted.AllowedResources()) + } base.AllowedResources = []string{"https://rs.example", "urn:example:rs"} some, err := NewRegisteredClient(base) if err != nil { @@ -691,6 +694,9 @@ func TestNewRegisteredClientAllowedResources(t *testing.T) { if !some.AllowsResource("urn:example:rs") || some.AllowsResource("https://other.example") || some.AllowsResource("https://rs.example/") { t.Error("AllowsResource doesn't follow the registered list exactly") } + if got, want := some.AllowedResources(), []string{"https://rs.example", "urn:example:rs"}; !slices.Equal(got, want) { + t.Errorf("AllowedResources() = %q, want %q", got, want) + } for _, bad := range []string{"", "/relative", "https://rs.example#frag"} { base.AllowedResources = []string{"https://rs.example", bad} if _, err := NewRegisteredClient(base); err == nil { From 593801c20f76293e7e8885896751bde4f0d9c80c Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 25 Oct 2026 22:17:23 +0800 Subject: [PATCH 2/5] feat(server): show and narrow resources at consent; confine federation clients' resources InteractionRequest.Resources and BackchannelInteractionRequest.Resources carry the RFC 8707 resources the tokens would be restricted to (the client's AllowedResources default included), so where several resource servers or OpenID4VCI Credential Issuers share the authorization server, the consent step can show which one a token is for. The encoded InteractionRequest carries them too. GrantedAuthorization.Resources narrows them to the subset the resource owner approved; nil grants all of them, as before, and one that wasn't requested fails the completion with invalid_request, as an unrequested scope does. AutomaticRegistrationConfig.AllowedResources (federation and server) confines every client registered through OpenID Federation to the operator's resources, the way AllowedScopes sets their scopes: never read from an RP's own metadata. Each entry must be an absolute URI without a fragment, none repeated. Empty leaves them unconfined, as before. Co-Authored-By: Claude Opus 5.5 --- federation/automatic_registration.go | 35 +++- federation/automatic_registration_test.go | 38 ++++ server/authorization.go | 2 + server/automatic_registration_test.go | 42 ++++ server/backchannel_authentication.go | 12 +- server/backchannel_interaction.go | 5 + server/complete_authorization.go | 7 +- server/complete_backchannel_authentication.go | 8 +- server/config.go | 11 + server/interaction.go | 10 + server/interaction_encoding.go | 14 +- server/interaction_encoding_test.go | 1 + server/resource_indicators.go | 12 ++ server/resource_indicators_consent_test.go | 190 ++++++++++++++++++ server/resource_indicators_default_test.go | 3 + server/server.go | 1 + server/subject.go | 11 + 17 files changed, 388 insertions(+), 14 deletions(-) create mode 100644 server/resource_indicators_consent_test.go diff --git a/federation/automatic_registration.go b/federation/automatic_registration.go index c1a33219..f4387816 100644 --- a/federation/automatic_registration.go +++ b/federation/automatic_registration.go @@ -19,6 +19,7 @@ import ( intfed "github.com/idfoundry/fapigo/internal/federation" "github.com/idfoundry/fapigo/internal/jose" "github.com/idfoundry/fapigo/internal/mtls" + "github.com/idfoundry/fapigo/internal/resourceindicator" "github.com/idfoundry/fapigo/internal/strictjson" "github.com/idfoundry/fapigo/keys" "github.com/idfoundry/fapigo/storage" @@ -51,6 +52,19 @@ type AutomaticRegistrationConfig struct { // self-published authorization_details_types. Empty means none. AuthorizationDetailsTypes []string + // AllowedResources confines every automatically-registered client to + // these RFC 8707 resource indicators + // (storage.RegisteredClientConfig.AllowedResources) — for the same + // reason as AllowedScopes, never anything an RP publishes about + // itself: which resource servers a client may get tokens for is the + // operator's decision. Each an absolute URI without a fragment, none + // repeated. Empty means unconfined: an RP may name any resource the + // server accepts, and one naming none gets tokens addressed to the + // issuer. Set, a request naming none is for its single entry, or, + // with several, invalid_target (see + // storage.RegisteredClientConfig.AllowedResources). + AllowedResources []string + // MaxCacheAge bounds how long a resolved client's Trust Chain is // reused before it is resolved again — the actual cache lifetime // for a given client is min(MaxCacheAge, its own Trust Chain's @@ -373,6 +387,10 @@ func NewAutomaticClientRepository(underlying storage.ClientRepository, resolver if err := validateSubjectTypes(cfg.SubjectTypes, cfg.DefaultSubjectType); err != nil { return nil, err } + if err := validateAllowedResources(cfg.AllowedResources); err != nil { + return nil, err + } + cfg.AllowedResources = slices.Clone(cfg.AllowedResources) if clock == nil { return nil, fmt.Errorf("federation: clock is required") } @@ -384,6 +402,20 @@ func NewAutomaticClientRepository(underlying storage.ClientRepository, resolver }, nil } +// validateAllowedResources checks AutomaticRegistrationConfig.AllowedResources: +// each a resource indicator RFC 8707 §2 allows, none repeated. +func validateAllowedResources(resources []string) error { + for i, r := range resources { + if err := resourceindicator.Check(r); err != nil { + return fmt.Errorf("federation: config: allowed_resources[%d]: %w", i, err) + } + if slices.Contains(resources[:i], r) { + return fmt.Errorf("federation: config: allowed_resources[%d]: %q is repeated", i, r) + } + } + return nil +} + // ResolveClient implements storage.ClientRepository. func (a *AutomaticClientRepository) ResolveClient(ctx context.Context, id fapi.ClientID) (storage.RegisteredClient, error) { client, err := a.underlying.ResolveClient(ctx, id) @@ -755,7 +787,7 @@ type relyingPartyMetadata struct { // choiceLimits). // // a.cfg supplies the operator-level capability grants (AllowedScopes, -// AllowsClientCredentialsGrant, AllowsCIBA) that must never be inferred +// AllowedResources, AllowsClientCredentialsGrant, AllowsCIBA) that must never be inferred // from raw itself, plus the AllowedClientAuthMethods restriction raw's // own token_endpoint_auth_method is checked against — see AutomaticRegistrationConfig's own doc comments // for why. @@ -810,6 +842,7 @@ func (a *AutomaticClientRepository) registeredClientConfigFromMetadata(ctx conte SenderConstrain: senderConstrain, AllowedScopes: a.cfg.AllowedScopes, AuthorizationDetailsTypes: a.cfg.AuthorizationDetailsTypes, + AllowedResources: a.cfg.AllowedResources, AutomaticFederationRegistration: true, AllowsClientCredentialsGrant: a.cfg.AllowsClientCredentialsGrant, } diff --git a/federation/automatic_registration_test.go b/federation/automatic_registration_test.go index 68888949..cb47fee8 100644 --- a/federation/automatic_registration_test.go +++ b/federation/automatic_registration_test.go @@ -292,6 +292,16 @@ func TestNewAutomaticClientRepositoryRejectsInvalidConfig(t *testing.T) { cfg.FailureCacheAge = -time.Second return alwaysFailsRepository{}, resolver, f.fetcher, cfg, fixedClock{now: f.now} }, + "relative allowed resource": func() (storage.ClientRepository, *federation.Resolver, *fapihttp.Client, federation.AutomaticRegistrationConfig, federation.Clock) { + cfg := validAutomaticRegistrationConfig() + cfg.AllowedResources = []string{"/api"} + return alwaysFailsRepository{}, resolver, f.fetcher, cfg, fixedClock{now: f.now} + }, + "repeated allowed resource": func() (storage.ClientRepository, *federation.Resolver, *fapihttp.Client, federation.AutomaticRegistrationConfig, federation.Clock) { + cfg := validAutomaticRegistrationConfig() + cfg.AllowedResources = []string{"https://rs.example", "https://rs.example"} + return alwaysFailsRepository{}, resolver, f.fetcher, cfg, fixedClock{now: f.now} + }, "invalid allowed client auth method": func() (storage.ClientRepository, *federation.Resolver, *fapihttp.Client, federation.AutomaticRegistrationConfig, federation.Clock) { cfg := validAutomaticRegistrationConfig() cfg.AllowedClientAuthMethods = []storage.ClientAuthMethod{storage.ClientAuthMethod(255)} @@ -377,6 +387,9 @@ func TestAutomaticClientRepositoryResolveClientFallsBackToFederation(t *testing. if got.AllowsAuthorizationDetailsType("payment") { t.Error("AllowsAuthorizationDetailsType(payment) = true with no AutomaticRegistrationConfig.AuthorizationDetailsTypes") } + if got.AllowedResources() != nil { + t.Errorf("AllowedResources() = %q with no AutomaticRegistrationConfig.AllowedResources", got.AllowedResources()) + } } // unavailableRepository is a storage.ClientRepository whose backend @@ -428,6 +441,31 @@ func TestAutomaticClientRepositoryAuthorizationDetailsTypesComeFromConfig(t *tes } } +// TestAutomaticClientRepositoryAllowedResourcesComeFromConfig covers an +// automatically registered client being confined to the operator's +// resources, never ones from the RP's own metadata, and the config's +// slice being copied. +func TestAutomaticClientRepositoryAllowedResourcesComeFromConfig(t *testing.T) { + f := setupAutomaticRegistrationFixture(t, rpMetadataBuilder(t)) + cfg := validAutomaticRegistrationConfig() + cfg.AllowedResources = []string{"https://rs.example", "https://other.example"} + repo, err := federation.NewAutomaticClientRepository(alwaysFailsRepository{}, f.newResolver(t), f.fetcher, cfg, fixedClock{now: f.now}) + if err != nil { + t.Fatalf("NewAutomaticClientRepository: %v", err) + } + cfg.AllowedResources[0] = "https://changed.example" + got, err := repo.ResolveClient(context.Background(), fapi.ClientID(f.rpID)) + if err != nil { + t.Fatalf("ResolveClient: %v", err) + } + if want := []string{"https://other.example", "https://rs.example"}; !slices.Equal(got.AllowedResources(), want) { + t.Errorf("AllowedResources() = %q, want %q", got.AllowedResources(), want) + } + if got.AllowsResource("https://changed.example") { + t.Error("a change to the config's slice after construction reached the client") + } +} + func TestAutomaticClientRepositoryResolveClientDoesNotAllowClientCredentialsByDefault(t *testing.T) { f := setupAutomaticRegistrationFixture(t, rpMetadataBuilder(t)) resolver := f.newResolver(t) diff --git a/server/authorization.go b/server/authorization.go index ca45bac9..5215048b 100644 --- a/server/authorization.go +++ b/server/authorization.go @@ -3,6 +3,7 @@ package server import ( "context" "encoding/json" + "slices" "strings" "time" @@ -152,6 +153,7 @@ func (s *Server) BeginAuthorization(ctx context.Context, req BeginAuthorizationR interaction.RequiredSubject = request.requiredSubjectForApplication() interaction.ClientDisplay = client.Display() interaction.Extensions = extensions + interaction.Resources = slices.Clone(request.Resources) action := InteractionRequired{ Handle: InteractionHandle{value: handle}, diff --git a/server/automatic_registration_test.go b/server/automatic_registration_test.go index abfd3780..2a8c47b7 100644 --- a/server/automatic_registration_test.go +++ b/server/automatic_registration_test.go @@ -388,6 +388,48 @@ func TestNewWiresAutomaticRegistrationAllowedClientAuthMethods(t *testing.T) { } } +// TestNewWiresAutomaticRegistrationAllowedResources proves server.New +// passes AutomaticRegistrationConfig.AllowedResources through: the RP +// may name only those, and, confined to several, must name one. +func TestNewWiresAutomaticRegistrationAllowedResources(t *testing.T) { + f := setupAutomaticRegistrationFixture(t, testRedirectURI) + srv := newAutomaticRegistrationTestServer(t, f, func(cfg *server.Config, deps *server.Dependencies) { + cfg.Resources = []string{resourceA, resourceB, resourceC} + cfg.AutomaticRegistration.AllowedResources = []string{resourceA, resourceB} + }) + for name, tc := range map[string]struct { + extra map[string]string + want server.ErrorCode + }{ + "allowed": {extra: map[string]string{"resource": resourceB}}, + "not allowed": {extra: map[string]string{"resource": resourceC}, want: server.ErrorInvalidTarget}, + "none named": {want: server.ErrorInvalidTarget}, + } { + t.Run(name, func(t *testing.T) { + assertion, err := clientassertion.CreateAssertion(clientassertion.AssertionRequest{ + Signer: f.rpOIDCKey, Algorithm: fapi.ES256, KeyID: "rp-oidc", + ClientID: f.rpID, Audience: testIssuer, + Now: f.now, Lifetime: 30 * time.Second, + }) + if err != nil { + t.Fatalf("CreateAssertion: %v", err) + } + _, err = srv.PushAuthorizationRequest(context.Background(), server.PushAuthorizationRequest{ + HTTP: server.FormRequest{Parameters: plainFormParameters(t, assertion, tc.extra)}, + }) + if tc.want == "" { + if err != nil { + t.Fatalf("PushAuthorizationRequest: %v", err) + } + return + } + if code := serverErrorCode(t, err); code != tc.want { + t.Fatalf("error code = %q, want %q", code, tc.want) + } + }) + } +} + // TestNewWiresAutomaticRegistrationOnResolutionFailure proves server.New // passes AutomaticRegistrationConfig.OnResolutionFailure through: the // operator's hook hears why the RP was refused, while the RP itself diff --git a/server/backchannel_authentication.go b/server/backchannel_authentication.go index 50c31e32..28e4dc6c 100644 --- a/server/backchannel_authentication.go +++ b/server/backchannel_authentication.go @@ -190,7 +190,7 @@ func (s *Server) BeginBackchannelAuthentication(ctx context.Context, req BeginBa return s.backchannelBeginFail(ctx, client.ID(), dpopErr), nil } - interaction, err := s.backchannelInteraction(client, validated.params) + interaction, err := s.backchannelInteraction(client, validated.params, validated.resources) if err != nil { return s.backchannelBeginFail(ctx, client.ID(), newError(ErrorServerError, 500, "validated extension parameters could not be read back", err)), nil } @@ -684,11 +684,13 @@ func (s *Server) backchannelBeginFail(ctx context.Context, clientID fapi.ClientI } // backchannelInteraction is the BackchannelInteractionRequest for a -// request from client with params, as BeginBackchannelAuthentication and -// LookupBackchannelInteraction return it. -func (s *Server) backchannelInteraction(client storage.RegisteredClient, params map[string]json.RawMessage) (BackchannelInteractionRequest, error) { +// request from client with params, for resources, as +// BeginBackchannelAuthentication and LookupBackchannelInteraction return +// it. +func (s *Server) backchannelInteraction(client storage.RegisteredClient, params map[string]json.RawMessage, resources []string) (BackchannelInteractionRequest, error) { interaction := s.backchannelInteractionRequestFrom(client.ID(), params) interaction.ClientDisplay = client.Display() + interaction.Resources = slices.Clone(resources) extensions, err := s.interactionExtensions(params, coreBackchannelAuthenticationParameters, extension.SourceRequestObject) if err != nil { return BackchannelInteractionRequest{}, err @@ -725,7 +727,7 @@ func (s *Server) LookupBackchannelInteraction(ctx context.Context, handle Backch if err != nil { return BackchannelInteractionRequest{}, newError(ErrorServerError, 500, "the request's client could not be resolved", err) } - interaction, err := s.backchannelInteraction(client, request.Parameters) + interaction, err := s.backchannelInteraction(client, request.Parameters, request.Resources) if err != nil { return BackchannelInteractionRequest{}, newError(ErrorServerError, 500, "the request's extension parameters could not be read back", err) } diff --git a/server/backchannel_interaction.go b/server/backchannel_interaction.go index a9840427..069dcf89 100644 --- a/server/backchannel_interaction.go +++ b/server/backchannel_interaction.go @@ -146,6 +146,11 @@ type BackchannelInteractionRequest struct { // exactly, for the CIBA flow. AuthorizationDetails extension.RARValues + // Resources mirrors InteractionRequest.Resources for the CIBA flow: + // the resource servers the tokens would be for, which + // GrantedAuthorization.Resources can narrow. + Resources []string + // RequestedClaims is the identity claims the client asked for with // the "claims" parameter — see InteractionRequest.RequestedClaims. RequestedClaims RequestedClaims diff --git a/server/complete_authorization.go b/server/complete_authorization.go index 2b204729..8bc19af6 100644 --- a/server/complete_authorization.go +++ b/server/complete_authorization.go @@ -171,6 +171,11 @@ func (s *Server) completeAuthorize(ctx context.Context, clientID fapi.ClientID, return s.completeLocalFail(ctx, clientID, newError(ErrorInvalidRequest, 400, "granted authorization_details exceeds what was requested", err)), nil } + resources, resourcesErr := grantedResources(request.Resources, result.grant.Resources) + if resourcesErr != nil { + return s.completeLocalFail(ctx, clientID, resourcesErr), nil + } + if err := validateGrantedIDTokenClaims(result.grant.IDTokenClaims, s.cfg.Limits.MaxIDTokenClaimsBytes); err != nil { return s.completeLocalFail(ctx, clientID, newError(ErrorInvalidRequest, 400, "granted ID token claims are not valid", err)), nil } @@ -211,7 +216,7 @@ func (s *Server) completeAuthorize(ctx context.Context, clientID fapi.ClientID, ACR: result.auth.acr, AMR: result.auth.amr, AuthorizationDetails: grantedAuthorizationDetails, - Resources: request.Resources, + Resources: resources, TokenClaims: request.TokenClaims, RequestedIDTokenClaims: idTokenClaims, RequestedUserinfoClaims: userinfoClaims, diff --git a/server/complete_backchannel_authentication.go b/server/complete_backchannel_authentication.go index 1458be82..f89931db 100644 --- a/server/complete_backchannel_authentication.go +++ b/server/complete_backchannel_authentication.go @@ -176,6 +176,12 @@ func (s *Server) approveBackchannelAuthentication(ctx context.Context, clientID return wrapped } + resources, resourcesErr := grantedResources(request.Resources, result.grant.Resources) + if resourcesErr != nil { + s.audit(ctx, AuditEventCompleteBackchannelAuthentication, clientID, AuditOutcomeFailure, string(resourcesErr.Code())) + return resourcesErr + } + if claimsErr := validateGrantedIDTokenClaims(result.grant.IDTokenClaims, s.cfg.Limits.MaxIDTokenClaimsBytes); claimsErr != nil { wrapped := newError(ErrorInvalidRequest, 400, "granted ID token claims are not valid", claimsErr) s.audit(ctx, AuditEventCompleteBackchannelAuthentication, clientID, AuditOutcomeFailure, string(wrapped.Code())) @@ -216,7 +222,7 @@ func (s *Server) approveBackchannelAuthentication(ctx context.Context, clientID ACR: result.auth.acr, AMR: result.auth.amr, AuthorizationDetails: grantedAuthorizationDetails, - Resources: request.Resources, + Resources: resources, TokenClaims: request.TokenClaims, RequestedIDTokenClaims: idTokenClaims, RequestedUserinfoClaims: userinfoClaims, diff --git a/server/config.go b/server/config.go index 1e1a183d..87e31252 100644 --- a/server/config.go +++ b/server/config.go @@ -602,6 +602,17 @@ type AutomaticRegistrationConfig struct { // refuses one that isn't. AuthorizationDetailsTypes []string + // AllowedResources confines every automatically-registered client to + // these RFC 8707 resource indicators, as + // storage.RegisteredClientConfig.AllowedResources confines a + // statically registered one — see + // federation.AutomaticRegistrationConfig.AllowedResources for why + // this never comes from an RP's own metadata. Each an absolute URI + // without a fragment, none repeated. Empty (the default) leaves them + // unconfined: any resource Config.Resources or the ResourcePolicy + // accepts, and the issuer as audience when they name none. + AllowedResources []string + // MaxPathLength/MaxAuthorityHints/MaxStatementLifetime/MaxClockSkew // bound Trust Chain resolution itself — see federation.Limits, which these configure // directly. All required when TrustAnchors is set. diff --git a/server/interaction.go b/server/interaction.go index 56150c17..fc235976 100644 --- a/server/interaction.go +++ b/server/interaction.go @@ -230,6 +230,16 @@ type InteractionRequest struct { // of Tax Return TX-12345") instead of a bare scope string. AuthorizationDetails extension.RARValues + // Resources are the RFC 8707 resource indicators the access tokens + // from this authorization would be restricted to — the resource + // servers they can be used at: those the request named, or, for a + // client confined by storage.RegisteredClientConfig.AllowedResources + // that named none, its single allowed one. Nil when the tokens would + // be addressed to the issuer. Already validated; show them where + // the resource servers are several, so the user can tell which a + // token is for, and narrow them with GrantedAuthorization.Resources. + Resources []string + // RequestedClaims is the identity claims the client asked for with // the "claims" parameter. None is released unless the application // approves it in GrantedAuthorization.ApprovedIdentityClaims. diff --git a/server/interaction_encoding.go b/server/interaction_encoding.go index 79d86038..5d65ec2e 100644 --- a/server/interaction_encoding.go +++ b/server/interaction_encoding.go @@ -40,6 +40,7 @@ type encodedInteractionRequest struct { PolicyURI string `json:"policy_uri,omitempty"` TermsOfServiceURI string `json:"tos_uri,omitempty"` AuthorizationDetails extension.RARValues `json:"authorization_details"` + Resources []string `json:"resources,omitempty"` IDTokenClaims []string `json:"id_token_claims,omitempty"` UserInfoClaims []string `json:"userinfo_claims,omitempty"` Extensions extension.Values `json:"extensions"` @@ -57,10 +58,10 @@ type encodedInteractionRequest struct { // you would the rest of a session, never where the client or the user // agent can read it. It carries no integrity protection and needs none for // the grant's sake: CompleteAuthorization checks the granted scope, -// authorization details and identity claims against the request the -// server stored itself, so a modified copy can make a consent screen -// show less than was asked, and the grant built from it smaller, but -// never larger. Store it where only the application can write it all the +// authorization details, resources and identity claims against the +// request the server stored itself, so a modified copy can make a +// consent screen show less than was asked, and the grant built from it +// smaller, but never larger. Store it where only the application can write it all the // same, so what the end user sees is what was asked. func (r InteractionRequest) MarshalText() ([]byte, error) { if r.ClientID == "" { @@ -77,8 +78,8 @@ func (r InteractionRequest) MarshalText() ([]byte, error) { UILocales: r.UILocales, ClaimsLocales: r.ClaimsLocales, Display: r.Display, ClientName: r.ClientDisplay.Name, LogoURI: urlString(r.ClientDisplay.LogoURI), PolicyURI: urlString(r.ClientDisplay.PolicyURI), TermsOfServiceURI: urlString(r.ClientDisplay.TermsOfServiceURI), - AuthorizationDetails: r.AuthorizationDetails, - IDTokenClaims: r.RequestedClaims.IDToken, UserInfoClaims: r.RequestedClaims.UserInfo, + AuthorizationDetails: r.AuthorizationDetails, Resources: r.Resources, + IDTokenClaims: r.RequestedClaims.IDToken, UserInfoClaims: r.RequestedClaims.UserInfo, Extensions: r.Extensions, }) if err != nil { @@ -149,6 +150,7 @@ func ParseInteractionRequest(text string) (InteractionRequest, error) { ClaimsLocales: languageTags(slices.Values(e.ClaimsLocales)), Display: knownDisplay(e.Display), ClientDisplay: display, AuthorizationDetails: e.AuthorizationDetails, + Resources: e.Resources, RequestedClaims: RequestedClaims{IDToken: e.IDTokenClaims, UserInfo: e.UserInfoClaims}, Extensions: e.Extensions, } diff --git a/server/interaction_encoding_test.go b/server/interaction_encoding_test.go index dabdbd26..2132ed22 100644 --- a/server/interaction_encoding_test.go +++ b/server/interaction_encoding_test.go @@ -159,6 +159,7 @@ func TestInteractionRequestFullRoundTrip(t *testing.T) { Prompt: server.Prompt{server.PromptLogin, server.PromptConsent}, ClientDisplay: storage.ClientDisplay{Name: "RP", LogoURI: logo, PolicyURI: policy, TermsOfServiceURI: tos}, RequestedClaims: server.RequestedClaims{IDToken: []string{"email"}, UserInfo: []string{"name"}}, + Resources: []string{"https://rs-a.example", "https://rs-b.example/v1"}, Extensions: extensions, } type session struct { diff --git a/server/resource_indicators.go b/server/resource_indicators.go index ecf20242..5414163c 100644 --- a/server/resource_indicators.go +++ b/server/resource_indicators.go @@ -279,6 +279,18 @@ func narrowResources(granted, requested []string) ([]string, *Error) { return narrowed, nil } +// grantedResources is the resources an approval grants of requested, a +// pending request's: all of them when granted, the application's +// GrantedAuthorization.Resources, names none, and otherwise granted, +// de-duplicated, which must all be among requested. +func grantedResources(requested, granted []string) ([]string, *Error) { + resources, err := narrowResources(requested, granted) + if err != nil { + return nil, newError(ErrorInvalidRequest, 400, "granted resources exceed what was requested", err) + } + return resources, nil +} + // allowedGrantAudience is checkGrantStillAllowed for a token request // from grant asking for scope, followed by narrowedAudience for the // resources it requested. diff --git a/server/resource_indicators_consent_test.go b/server/resource_indicators_consent_test.go new file mode 100644 index 00000000..d45f6e96 --- /dev/null +++ b/server/resource_indicators_consent_test.go @@ -0,0 +1,190 @@ +package server_test + +import ( + "context" + "reflect" + "testing" + + "github.com/idfoundry/fapigo/server" + "github.com/idfoundry/fapigo/storage/memstore" +) + +// beginForResources pushes a request naming resources and begins its +// interaction. +func beginForResources(t *testing.T, h harness, resources ...string) server.InteractionRequired { + t.Helper() + pushed, err := pushWithResources(t, h, resources...) + if err != nil { + t.Fatalf("PushAuthorizationRequest: %v", err) + } + action, err := h.server.BeginAuthorization(context.Background(), server.BeginAuthorizationRequest{ + RequestURI: pushed.RequestURI.String(), ClientID: testClientID, + }) + if err != nil { + t.Fatalf("BeginAuthorization: %v", err) + } + required, ok := action.(server.InteractionRequired) + if !ok { + t.Fatalf("action = %T, want server.InteractionRequired", action) + } + return required +} + +// completeGrantingResources approves required as user-1, granting +// resources, and returns the code, or the local error. +func completeGrantingResources(t *testing.T, h harness, required server.InteractionRequired, resources []string) (string, *server.Error) { + t.Helper() + result, err := h.server.CompleteAuthorization(context.Background(), server.CompleteAuthorizationRequest{ + Handle: required.Handle, + Result: authorizedAsGranting(t, h, "user-1", server.GrantedAuthorization{ + Scope: []string{"openid", "accounts", "offline_access"}, Resources: resources, + }), + }) + if err != nil { + t.Fatalf("CompleteAuthorization: %v", err) + } + switch r := result.(type) { + case server.AuthorizationRedirect: + dest := r.Destination().URL() + return dest.Query().Get("code"), nil + case server.AuthorizationLocalError: + return "", r.Error + default: + t.Fatalf("result = %T", result) + return "", nil + } +} + +func authorizedAsGranting(t *testing.T, h harness, subject string, grant server.GrantedAuthorization) server.InteractionResult { + t.Helper() + subjectID, err := server.NewSubjectID(subject) + if err != nil { + t.Fatal(err) + } + authenticated, err := server.NewAuthenticatedSubject(subjectID) + if err != nil { + t.Fatal(err) + } + authCtx, err := server.NewAuthenticationContext(h.now, "", []string{"pwd"}) + if err != nil { + t.Fatal(err) + } + return server.Authorize(authenticated, authCtx, grant) +} + +// The consent step sees the resources a token would be for, the +// client's default among them, and can narrow them to a subset of what +// was requested, never widen them. +func TestInteractionRequestResources(t *testing.T) { + h := withResources(t, newHarness(t, server.ProfileFAPISecurity, true), []string{resourceA, resourceB, resourceC}, nil) + + if got := beginForResources(t, h).Interaction.Resources; got != nil { + t.Errorf("Resources of a request naming none = %q, want nil", got) + } + required := beginForResources(t, h, resourceA, resourceB, resourceA) + if want := []string{resourceA, resourceB}; !reflect.DeepEqual(required.Interaction.Resources, want) { + t.Errorf("Resources = %q, want %q", required.Interaction.Resources, want) + } + restrictClientResources(t, h, resourceC) + if got, want := beginForResources(t, h).Interaction.Resources, []string{resourceC}; !reflect.DeepEqual(got, want) { + t.Errorf("Resources of a confined client's request = %q, want its default %q", got, want) + } +} + +func TestCompleteAuthorizationNarrowsResources(t *testing.T) { + for name, tc := range map[string]struct { + requested, granted []string + want any + }{ + "omitted grants all": {requested: []string{resourceA, resourceB}, want: []any{resourceA, resourceB}}, + "narrowed": {requested: []string{resourceA, resourceB}, granted: []string{resourceB, resourceB}, want: resourceB}, + "none requested": {want: testIssuer}, + } { + t.Run(name, func(t *testing.T) { + h := withResources(t, newHarness(t, server.ProfileFAPISecurity, true), []string{resourceA, resourceB}, nil) + code, localErr := completeGrantingResources(t, h, beginForResources(t, h, tc.requested...), tc.granted) + if localErr != nil { + t.Fatalf("CompleteAuthorization: %v", localErr) + } + result, err := exchangeWithResources(t, h, code) + if err != nil { + t.Fatalf("ExchangeAuthorizationCode: %v", err) + } + wantAudience(t, result, tc.want) + // The grant is what was approved: a refresh can't name what + // the user left out. + if tc.granted != nil { + _, err := refreshWithResources(t, h, result.RefreshToken.Reveal(), resourceA) + wantErrorCode(t, err, server.ErrorInvalidTarget) + } + }) + } +} + +func TestCompleteAuthorizationRefusesUnrequestedResources(t *testing.T) { + for name, tc := range map[string]struct { + requested, granted []string + }{ + "another": {requested: []string{resourceA}, granted: []string{resourceB}}, + "one more": {requested: []string{resourceA}, granted: []string{resourceA, resourceB}}, + "none requested": {granted: []string{resourceA}}, + } { + t.Run(name, func(t *testing.T) { + h := withResources(t, newHarness(t, server.ProfileFAPISecurity, true), []string{resourceA, resourceB}, nil) + _, localErr := completeGrantingResources(t, h, beginForResources(t, h, tc.requested...), tc.granted) + if localErr == nil || localErr.Code() != server.ErrorInvalidRequest { + t.Fatalf("CompleteAuthorization error = %v, want invalid_request", localErr) + } + }) + } +} + +func TestBackchannelInteractionResources(t *testing.T) { + for name, tc := range map[string]struct { + granted []string + want any + wantErr server.ErrorCode + }{ + "omitted grants all": {want: []any{resourceA, resourceB}}, + "narrowed": {granted: []string{resourceA}, want: resourceA}, + "not requested": {granted: []string{resourceC}, wantErr: server.ErrorInvalidRequest}, + } { + t.Run(name, func(t *testing.T) { + h := newHarnessWithBackchannelConfig(t, memstore.NewBackchannelAuthenticationStore(), func(cfg *server.Config) { + cfg.Resources = []string{resourceA, resourceB, resourceC} + }) + required, ok := beginResourceCIBA(t, h, []string{resourceA, resourceB}).(server.BackchannelInteractionRequired) + if !ok { + t.Fatal("BeginBackchannelAuthentication didn't require an interaction") + } + want := []string{resourceA, resourceB} + if !reflect.DeepEqual(required.Interaction.Resources, want) { + t.Errorf("Interaction.Resources = %q, want %q", required.Interaction.Resources, want) + } + looked, err := h.server.LookupBackchannelInteraction(context.Background(), required.Handle) + if err != nil { + t.Fatalf("LookupBackchannelInteraction: %v", err) + } + if !reflect.DeepEqual(looked.Resources, want) { + t.Errorf("looked-up Resources = %q, want %q", looked.Resources, want) + } + + err = h.server.CompleteBackchannelAuthentication(context.Background(), server.CompleteBackchannelAuthenticationRequest{ + Handle: required.Handle, + Result: authorizedAsGranting(t, h, "user-1", server.GrantedAuthorization{Scope: []string{"openid", "accounts"}, Resources: tc.granted}), + }) + if tc.wantErr != "" { + wantErrorCode(t, err, tc.wantErr) + return + } + if err != nil { + t.Fatalf("CompleteBackchannelAuthentication: %v", err) + } + result, err := cibaTokenWithResources(t, h, required.AuthReqID.String()) + if err != nil { + t.Fatalf("ExchangeBackchannelAuthentication: %v", err) + } + wantAudience(t, result, tc.want) + }) + } +} diff --git a/server/resource_indicators_default_test.go b/server/resource_indicators_default_test.go index 13f23b1a..841f5c6a 100644 --- a/server/resource_indicators_default_test.go +++ b/server/resource_indicators_default_test.go @@ -161,6 +161,9 @@ func TestResourceIndicatorsAllowedResourcesDefaultCIBA(t *testing.T) { if !ok { t.Fatalf("action = %T, want server.BackchannelInteractionRequired", action) } + if !reflect.DeepEqual(required.Interaction.Resources, []string{resourceA}) { + t.Errorf("Interaction.Resources = %q, want the default", required.Interaction.Resources) + } result, err := approveAndPollResourceCIBA(t, h, required, nil) if err != nil { t.Fatalf("ExchangeBackchannelAuthentication: %v", err) diff --git a/server/server.go b/server/server.go index be320927..d96f04bc 100644 --- a/server/server.go +++ b/server/server.go @@ -84,6 +84,7 @@ func New(cfg Config, deps Dependencies) (*Server, error) { automaticClients, err := federation.NewAutomaticClientRepository(deps.Clients, resolver, deps.FederationHTTP, federation.AutomaticRegistrationConfig{ AllowedScopes: cfg.AutomaticRegistration.AllowedScopes, AuthorizationDetailsTypes: cfg.AutomaticRegistration.AuthorizationDetailsTypes, + AllowedResources: cfg.AutomaticRegistration.AllowedResources, MaxCacheAge: cfg.AutomaticRegistration.MaxCacheAge, FailureCacheAge: cfg.AutomaticRegistration.FailureCacheAge, AllowsClientCredentialsGrant: cfg.AutomaticRegistration.AllowsClientCredentialsGrant, diff --git a/server/subject.go b/server/subject.go index 354ada4d..78c2336d 100644 --- a/server/subject.go +++ b/server/subject.go @@ -165,6 +165,17 @@ type GrantedAuthorization struct { // same way a scope not present in the original request is rejected. AuthorizationDetails []json.RawMessage + // Resources narrows the RFC 8707 resource indicators the grant's + // access tokens are restricted to, to those the resource owner + // approved, from the request's own (InteractionRequest.Resources, + // or BackchannelInteractionRequest's). Nil or empty grants all of + // them, so an application that doesn't show resources grants what + // was requested. CompleteAuthorization/ + // CompleteBackchannelAuthentication reject one that wasn't + // requested, the same way they reject a scope that wasn't — so a + // request that named none can't be granted any. + Resources []string + // IDTokenClaims are additional claims the application wants in the // ID token issued for this authorization — e.g. provider-specific // claims describing the authenticated subject or who it is acting From 963e4841904d0f18c829c5d48378ea1ba4a34dfa Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 25 Oct 2026 22:17:23 +0800 Subject: [PATCH 3/5] docs: describe the AllowedResources default and consent-time resources Co-Authored-By: Claude Opus 5.5 --- GETTING_STARTED.md | 35 +++++++++++++++++++++++++++-------- UPGRADING.md | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/GETTING_STARTED.md b/GETTING_STARTED.md index a2b31088..3deb6239 100644 --- a/GETTING_STARTED.md +++ b/GETTING_STARTED.md @@ -1053,8 +1053,9 @@ func (p credentialIssuerPolicy) AllowResources(ctx context.Context, client stora A resource is checked against, in order: the client's own `storage.RegisteredClientConfig.AllowedResources`, if it registered any -(a client registered automatically through OpenID Federation has none, -so it may name any resource the server accepts); then +(a client registered automatically through OpenID Federation gets +`AutomaticRegistrationConfig.AllowedResources`, the same for all of +them, and without that may name any resource the server accepts); then `Config.Resources`; then, only for those the list doesn't name, `ResourcePolicy`. Anything refused is `invalid_target`. The client names resources at PAR — repeated `resource` form parameters, or a @@ -1065,7 +1066,18 @@ refresh; a refresh token always keeps the grant's whole set. The access token's `aud` is then the resource (a JSON string), or, for several, an array of them. A request naming no resource still gets the issuer as its audience, and with neither `Config.Resources` nor `ResourcePolicy` -set, `resource` is ignored as it always was. For a grant you serve +set, `resource` is ignored as it always was. + +A client confined by `AllowedResources` never gets an issuer-audience +token, though: every resource server accepting the issuer as an +audience would take it, whatever the list says. If it lists exactly one +resource, a request naming none — at PAR, in a CIBA request, for +client credentials, or to `IssueAccessToken`/`IssueRefreshToken` — is +for that one, recorded with the grant as if the client had named it and +checked against `Config.Resources` and `ResourcePolicy` the same way; +if it lists several, the request must name one, or it's +`invalid_target`. A grant made without resources before the client was +confined gets the same treatment at its next token request. For a grant you serve yourself (`Config.AdditionalGrantTypes`), pass `TokenEndpointRequest.Resources()` as `IssueAccessTokenRequest.Resources` and `IssueRefreshTokenRequest.Resources`, and check the resources your @@ -1112,10 +1124,14 @@ present it to another. Every FAPIgo access token is sender-constrained that doesn't want its resource servers to share a token narrows each token request to the one resource it's for. -The consent screen isn't told which resources a request names -(`InteractionRequest` doesn't carry them), so it can neither show them -to the user nor narrow them: they're the client's to choose and the -server's to allow. +The consent screen is told which resources the tokens would be for — +`InteractionRequest.Resources` (and `BackchannelInteractionRequest`'s +for CIBA), the client's default included — so where several resource +servers share the AS, show them: the user can then tell which one a +token is for. Approving, `GrantedAuthorization.Resources` can narrow +them to a subset of those; leaving it nil grants all of them, and one +that wasn't requested makes the completion fail, as an unrequested +scope does. While clients move to sending `resource`, a resource server can also accept tokens issued without one: `AcceptIssuerAudience: true` in @@ -1125,7 +1141,10 @@ server verifying the issuer audience accepts, works here too. A UserInfo endpoint keeps the default (no `Resource`): it accepts only tokens issued without a resource, so a client that also calls UserInfo names the AS's issuer as one of its resources (list it in `Config.Resources`) -and narrows each token request to the resource it's for. +and narrows each token request to the resource it's for. A client +confined by `AllowedResources` must list the issuer there too to call +UserInfo at all, and then, listing more than one, name a resource in +every request. ### Verify a request diff --git a/UPGRADING.md b/UPGRADING.md index d9e0a6ea..171b3d3e 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -17,6 +17,44 @@ and `keys/ephemeral` needs only the steps not marked *production only*. ## v0.61.0 +### A client confined by `AllowedResources` no longer gets issuer-audience tokens (server, storage) + +**Affects:** a server with resource indicators enabled (`Config.Resources` +or `Dependencies.ResourcePolicy`) where a client registered with +`storage.RegisteredClientConfig.AllowedResources` makes a request that +names no resource: at PAR, in a CIBA request, for client credentials, or +through `IssueAccessToken`/`IssueRefreshToken`. Also such a client's +grants made without resources, at their next code exchange, refresh or +CIBA token request. + +**Why:** a request naming no resource got an access token addressed to +the issuer, which `AllowedResources` was never checked against. Every +resource server accepting the issuer as an audience took it — one +migrating with `serverresource.Options.AcceptIssuerAudience` or the +issuer in `AdditionalAudiences`, and the UserInfo endpoint — so a client +refused a resource could get a token it accepts simply by naming none. + +**What fails:** for a client with exactly one `AllowedResources` entry, a +request naming no resource is now for that resource: the grant records +it, and its access tokens are addressed to it, not the issuer. It's +checked against `Config.Resources` and the `ResourcePolicy` as a named +one is, so if the server doesn't accept it the request fails with +`invalid_target`. For a client with several entries, a request naming +none fails with `invalid_target` ("a resource is required"). A grant made +without resources before the client was confined gets the same: its +single resource, or `invalid_target`, so the client must authorize +again. Nothing changes for clients without `AllowedResources`, or while +resource indicators are disabled. + +**What to change:** a confined client that also calls the UserInfo +endpoint, which accepts only issuer-audience tokens, lists the issuer +identifier in its `AllowedResources` (and the server accepts it, in +`Config.Resources`, say); having more than one entry, it then names a +resource in every request. A confined client with several entries names +the resource it wants on every request. To confine clients registered +through OpenID Federation too, set the new +`AutomaticRegistrationConfig.AllowedResources`. + ### A statically registered client authenticates with its static keys only (federation, server) **Affects:** a server with OpenID Federation automatic registration (or From f96641d7a9f6a4a2f72063ba8c30b6687423e12e Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 25 Oct 2026 22:22:43 +0800 Subject: [PATCH 4/5] refactor(server): check an approved grant, CIBA hints and binding_message in helpers Co-Authored-By: Claude Opus 5.5 --- server/backchannel_authentication.go | 60 ++++++++++++++-------- server/complete_authorization.go | 75 +++++++++++++++++----------- 2 files changed, 86 insertions(+), 49 deletions(-) diff --git a/server/backchannel_authentication.go b/server/backchannel_authentication.go index 28e4dc6c..8106ce8b 100644 --- a/server/backchannel_authentication.go +++ b/server/backchannel_authentication.go @@ -420,6 +420,30 @@ func (s *Server) validateBackchannelAuthenticationParameters(verified verifiedBa return verifiedBackchannelRequest{}, newError(ErrorInvalidScope, 400, "scope must include openid", nil) } + if hintErr := validateBackchannelHints(params); hintErr != nil { + return verifiedBackchannelRequest{}, hintErr + } + + if validateErr := validateClientNotificationToken(params, client); validateErr != nil { + return verifiedBackchannelRequest{}, validateErr + } + if err := validateClaimsParameter(params); err != nil { + return verifiedBackchannelRequest{}, newError(ErrorInvalidRequest, 400, err.Error(), nil) + } + if _, err := essentialACRValues(params); err != nil { + return verifiedBackchannelRequest{}, newError(ErrorInvalidRequest, 400, err.Error(), nil) + } + + if bindingErr := validateBindingMessage(params); bindingErr != nil { + return verifiedBackchannelRequest{}, bindingErr + } + + return verified, nil +} + +// validateBackchannelHints checks params carries exactly one identity +// hint (CIBA §7.1): login_hint, login_hint_token or id_token_hint. +func validateBackchannelHints(params map[string]json.RawMessage) *Error { hints := 0 for _, name := range [...]string{"login_hint", "login_hint_token", "id_token_hint"} { raw, ok := params[name] @@ -431,35 +455,31 @@ func (s *Server) validateBackchannelAuthenticationParameters(verified verifiedBa // reach the application as no hint at all // (backchannelInteractionRequestFrom keeps only string values). if v, err := jsonStringValue(raw); err != nil || v == "" { - return verifiedBackchannelRequest{}, newError(ErrorInvalidRequest, 400, name+" must be a non-empty string", err) + return newError(ErrorInvalidRequest, 400, name+" must be a non-empty string", err) } hints++ } if hints != 1 { - return verifiedBackchannelRequest{}, newError(ErrorInvalidRequest, 400, "exactly one of login_hint, login_hint_token, or id_token_hint is required", nil) + return newError(ErrorInvalidRequest, 400, "exactly one of login_hint, login_hint_token, or id_token_hint is required", nil) } + return nil +} - if validateErr := validateClientNotificationToken(params, client); validateErr != nil { - return verifiedBackchannelRequest{}, validateErr - } - if err := validateClaimsParameter(params); err != nil { - return verifiedBackchannelRequest{}, newError(ErrorInvalidRequest, 400, err.Error(), nil) +// validateBindingMessage checks params' binding_message, if any, is a +// string the authentication device can show (CIBA §7.1). +func validateBindingMessage(params map[string]json.RawMessage) *Error { + raw, ok := params["binding_message"] + if !ok { + return nil } - if _, err := essentialACRValues(params); err != nil { - return verifiedBackchannelRequest{}, newError(ErrorInvalidRequest, 400, err.Error(), nil) + bindingMessage, err := jsonStringValue(raw) + if err != nil { + return newError(ErrorInvalidRequest, 400, "binding_message must be a string", err) } - - if raw, ok := params["binding_message"]; ok { - bindingMessage, err := jsonStringValue(raw) - if err != nil { - return verifiedBackchannelRequest{}, newError(ErrorInvalidRequest, 400, "binding_message must be a string", err) - } - if !isAcceptableBindingMessage(bindingMessage) { - return verifiedBackchannelRequest{}, newError(ErrorInvalidBindingMessage, 400, "binding_message is not acceptable", nil) - } + if !isAcceptableBindingMessage(bindingMessage) { + return newError(ErrorInvalidBindingMessage, 400, "binding_message is not acceptable", nil) } - - return verified, nil + return nil } // validateClientNotificationToken checks client_notification_token's diff --git a/server/complete_authorization.go b/server/complete_authorization.go index 8bc19af6..0eb911ec 100644 --- a/server/complete_authorization.go +++ b/server/complete_authorization.go @@ -3,6 +3,7 @@ package server import ( "context" "crypto/sha256" + "encoding/json" "fmt" "net/url" "strings" @@ -161,31 +162,9 @@ func (s *Server) completeAuthorize(ctx context.Context, clientID fapi.ClientID, return s.completeErrorRedirect(ctx, clientID, redirectURI, state, "login_required", "the authenticated user isn't the one the request named", AuditOutcomeFailure) } - requestedScope, _ := jsonString(request.Parameters, "scope") - if err := validateGrantedScopeSubset(result.grant.Scope, requestedScope); err != nil { - return s.completeLocalFail(ctx, clientID, newError(ErrorInvalidRequest, 400, "granted scope exceeds requested scope", err)), nil - } - - grantedAuthorizationDetails, err := s.validateGrantedAuthorizationDetails(request.Parameters[authorizationDetailsParameter], result.grant.AuthorizationDetails) - if err != nil { - return s.completeLocalFail(ctx, clientID, newError(ErrorInvalidRequest, 400, "granted authorization_details exceeds what was requested", err)), nil - } - - resources, resourcesErr := grantedResources(request.Resources, result.grant.Resources) - if resourcesErr != nil { - return s.completeLocalFail(ctx, clientID, resourcesErr), nil - } - - if err := validateGrantedIDTokenClaims(result.grant.IDTokenClaims, s.cfg.Limits.MaxIDTokenClaimsBytes); err != nil { - return s.completeLocalFail(ctx, clientID, newError(ErrorInvalidRequest, 400, "granted ID token claims are not valid", err)), nil - } - if err := s.validateGrantID(result.grant.GrantID); err != nil { - return s.completeLocalFail(ctx, clientID, newError(ErrorInvalidRequest, 400, "the grant ID is not valid", err)), nil - } - - idTokenClaims, userinfoClaims, err := approvedClaimNames(request.Parameters, result.grant.ApprovedIdentityClaims) - if err != nil { - return s.completeLocalFail(ctx, clientID, newError(ErrorInvalidRequest, 400, "approved identity claims exceed what was requested", err)), nil + approved, approvedErr := s.checkApprovedGrant(request, result.grant) + if approvedErr != nil { + return s.completeLocalFail(ctx, clientID, approvedErr), nil } codeChallenge, err := jsonString(request.Parameters, "code_challenge") @@ -215,11 +194,11 @@ func (s *Server) completeAuthorize(ctx context.Context, clientID fapi.ClientID, AuthTime: result.auth.authTime, ACR: result.auth.acr, AMR: result.auth.amr, - AuthorizationDetails: grantedAuthorizationDetails, - Resources: resources, + AuthorizationDetails: approved.authorizationDetails, + Resources: approved.resources, TokenClaims: request.TokenClaims, - RequestedIDTokenClaims: idTokenClaims, - RequestedUserinfoClaims: userinfoClaims, + RequestedIDTokenClaims: approved.idTokenClaims, + RequestedUserinfoClaims: approved.userinfoClaims, IDTokenClaims: result.grant.IDTokenClaims, GrantID: result.grant.GrantID, }) @@ -247,6 +226,44 @@ func (s *Server) completeAuthorize(ctx context.Context, clientID fapi.ClientID, return AuthorizationRedirect{destination: destination}, nil } +// approvedGrant is what an application's GrantedAuthorization approves +// of a pending authorization request, checked against it. +type approvedGrant struct { + authorizationDetails json.RawMessage + resources []string + idTokenClaims, userinfoClaims []string +} + +// checkApprovedGrant checks grant against request, what the client +// asked for: its scope, authorization details, resources and identity +// claims may only narrow the request's, and its ID token claims and +// grant ID must be valid. Any other grant is invalid_request. +func (s *Server) checkApprovedGrant(request requestRecord, grant GrantedAuthorization) (approvedGrant, *Error) { + requestedScope, _ := jsonString(request.Parameters, "scope") + if err := validateGrantedScopeSubset(grant.Scope, requestedScope); err != nil { + return approvedGrant{}, newError(ErrorInvalidRequest, 400, "granted scope exceeds requested scope", err) + } + details, err := s.validateGrantedAuthorizationDetails(request.Parameters[authorizationDetailsParameter], grant.AuthorizationDetails) + if err != nil { + return approvedGrant{}, newError(ErrorInvalidRequest, 400, "granted authorization_details exceeds what was requested", err) + } + resources, resourcesErr := grantedResources(request.Resources, grant.Resources) + if resourcesErr != nil { + return approvedGrant{}, resourcesErr + } + if err := validateGrantedIDTokenClaims(grant.IDTokenClaims, s.cfg.Limits.MaxIDTokenClaimsBytes); err != nil { + return approvedGrant{}, newError(ErrorInvalidRequest, 400, "granted ID token claims are not valid", err) + } + if err := s.validateGrantID(grant.GrantID); err != nil { + return approvedGrant{}, newError(ErrorInvalidRequest, 400, "the grant ID is not valid", err) + } + idTokenClaims, userinfoClaims, err := approvedClaimNames(request.Parameters, grant.ApprovedIdentityClaims) + if err != nil { + return approvedGrant{}, newError(ErrorInvalidRequest, 400, "approved identity claims exceed what was requested", err) + } + return approvedGrant{authorizationDetails: details, resources: resources, idTokenClaims: idTokenClaims, userinfoClaims: userinfoClaims}, nil +} + func (s *Server) completeErrorRedirect(ctx context.Context, clientID fapi.ClientID, redirectURI, state, errorCode, description string, outcome AuditOutcome) (AuthorizationResult, error) { params := map[string]string{"error": errorCode, "state": state} // description may be the application's own reason (Deny, From a8c6b6165c2a38960d0b52f49c2695e8e7788f89 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 25 Oct 2026 22:27:58 +0800 Subject: [PATCH 5/5] refactor(server): check what makes a completion login_required in one helper Also covers a non-string CIBA binding_message. Co-Authored-By: Claude Opus 5.5 --- server/backchannel_test.go | 21 +++++++++ server/complete_authorization.go | 76 ++++++++++++++++++-------------- 2 files changed, 64 insertions(+), 33 deletions(-) diff --git a/server/backchannel_test.go b/server/backchannel_test.go index ad70fe1a..38601096 100644 --- a/server/backchannel_test.go +++ b/server/backchannel_test.go @@ -1331,6 +1331,27 @@ func TestBeginBackchannelAuthenticationRejectsBindingMessageWithControlCharacter } } +func TestBeginBackchannelAuthenticationRejectsNonStringBindingMessage(t *testing.T) { + h, _ := newHarnessWithBackchannel(t) + params := standardBackchannelParams(t) + params["binding_message"] = jsonRaw(t, 1234) + requestObj := h.backchannelRequestObject(t, params) + + action, err := h.server.BeginBackchannelAuthentication(context.Background(), server.BeginBackchannelAuthenticationRequest{ + HTTP: server.FormRequest{Parameters: backchannelFormParams(h.clientAssertion(t), requestObj)}, + }) + if err != nil { + t.Fatalf("BeginBackchannelAuthentication: %v", err) + } + localErr, ok := action.(server.BackchannelAuthenticationLocalError) + if !ok { + t.Fatalf("action = %T, want server.BackchannelAuthenticationLocalError", action) + } + if localErr.Error.Code() != server.ErrorInvalidRequest { + t.Fatalf("Code = %q, want %q", localErr.Error.Code(), server.ErrorInvalidRequest) + } +} + func TestBeginBackchannelAuthenticationRejectsWhenCIBADisabled(t *testing.T) { h := newHarness(t, server.ProfileFAPISecurity, true) // CIBA not configured diff --git a/server/complete_authorization.go b/server/complete_authorization.go index 0eb911ec..9a34dc94 100644 --- a/server/complete_authorization.go +++ b/server/complete_authorization.go @@ -127,39 +127,8 @@ func (s *Server) completeAuthorize(ctx context.Context, clientID fapi.ClientID, return s.completeLocalFail(ctx, clientID, authorizeErr), nil } - // OIDC Core §3.1.2.1: with max_age, an authentication older than - // max_age must be repeated, and if it isn't, the client gets an - // error, "typically login_required" (§3.1.2.1, prompt=login). - if maxAge, ok, _ := requestedMaxAge(request.Parameters); ok { - oldest := s.deps.Clock.Now().Add(-maxAge - s.cfg.Limits.MaxClockSkew) - if result.auth.authTime.Before(oldest) { - return s.completeErrorRedirect(ctx, clientID, redirectURI, state, "login_required", "the user's authentication is older than the requested max_age", AuditOutcomeFailure) - } - } - - // OIDC Core §3.1.2.1: prompt=login asks the OP to "reauthenticate the - // End-User even if the End-User is already authenticated", and if it - // can't, to return login_required. An authentication from before the - // request was pushed, allowing the same clock skew as max_age, didn't - // happen for this request. A record without PushedAt (written before - // it existed) isn't checked. - if prompt, _ := requestedPrompt(request.Parameters); prompt.Has(PromptLogin) && request.PushedAt != nil { - if result.auth.authTime.Before(request.PushedAt.Add(-s.cfg.Limits.MaxClockSkew)) { - return s.completeErrorRedirect(ctx, clientID, redirectURI, state, "login_required", "the user didn't authenticate again for prompt=login", AuditOutcomeFailure) - } - } - - // OIDC Core §5.5.1.1: an essential "acr" request with values must be - // met, or the outcome treated as a failed authentication. - if !meetsEssentialACR(request.EssentialACRValues, result.auth.acr) { - return s.completeErrorRedirect(ctx, clientID, redirectURI, state, "login_required", "the user's authentication doesn't meet the requested essential acr", AuditOutcomeFailure) - } - - // OIDC Core §5.5.1: a request naming its end user, by id_token_hint - // or a "sub" value, must not be answered for a different one; that - // user didn't authenticate, so it's login_required. - if !meetsRequiredSubject(request.RequiredSubject, clientSub) { - return s.completeErrorRedirect(ctx, clientID, redirectURI, state, "login_required", "the authenticated user isn't the one the request named", AuditOutcomeFailure) + if problem := s.loginRequiredProblem(request, result.auth, clientSub); problem != "" { + return s.completeErrorRedirect(ctx, clientID, redirectURI, state, "login_required", problem, AuditOutcomeFailure) } approved, approvedErr := s.checkApprovedGrant(request, result.grant) @@ -226,6 +195,47 @@ func (s *Server) completeAuthorize(ctx context.Context, clientID fapi.ClientID, return AuthorizationRedirect{destination: destination}, nil } +// loginRequiredProblem describes why auth, the authentication of the +// user whose sub at the client is clientSub, doesn't meet request, so +// the client gets login_required, or returns "" when it does. +func (s *Server) loginRequiredProblem(request requestRecord, auth AuthenticationContext, clientSub string) string { + // OIDC Core §3.1.2.1: with max_age, an authentication older than + // max_age must be repeated, and if it isn't, the client gets an + // error, "typically login_required" (§3.1.2.1, prompt=login). + if maxAge, ok, _ := requestedMaxAge(request.Parameters); ok { + oldest := s.deps.Clock.Now().Add(-maxAge - s.cfg.Limits.MaxClockSkew) + if auth.authTime.Before(oldest) { + return "the user's authentication is older than the requested max_age" + } + } + + // OIDC Core §3.1.2.1: prompt=login asks the OP to "reauthenticate the + // End-User even if the End-User is already authenticated", and if it + // can't, to return login_required. An authentication from before the + // request was pushed, allowing the same clock skew as max_age, didn't + // happen for this request. A record without PushedAt (written before + // it existed) isn't checked. + if prompt, _ := requestedPrompt(request.Parameters); prompt.Has(PromptLogin) && request.PushedAt != nil { + if auth.authTime.Before(request.PushedAt.Add(-s.cfg.Limits.MaxClockSkew)) { + return "the user didn't authenticate again for prompt=login" + } + } + + // OIDC Core §5.5.1.1: an essential "acr" request with values must be + // met, or the outcome treated as a failed authentication. + if !meetsEssentialACR(request.EssentialACRValues, auth.acr) { + return "the user's authentication doesn't meet the requested essential acr" + } + + // OIDC Core §5.5.1: a request naming its end user, by id_token_hint + // or a "sub" value, must not be answered for a different one; that + // user didn't authenticate, so it's login_required. + if !meetsRequiredSubject(request.RequiredSubject, clientSub) { + return "the authenticated user isn't the one the request named" + } + return "" +} + // approvedGrant is what an application's GrantedAuthorization approves // of a pending authorization request, checked against it. type approvedGrant struct {