From 05e0fd0357ebad6523349ffd42b40c9bbff8e122 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 12 May 2026 08:50:04 -0400 Subject: [PATCH] fix(plugins): Drop mutex between token-exchange and token-broker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #400 migrated both plugins to declare `Claims: []string{contracts.ClaimAuthorizationHeader}`, which made plugins.Build reject any chain that contained both. On review, David pointed out this diagnosis was wrong: token-exchange and token-broker have legitimate coexistence use cases (the former handles Keycloak- audience hosts, the latter handles federated-broker hosts, same deployment may need both on different routes). The correct fix for their interaction is per-route arbitration — "which handler runs for this specific host" — not per-chain mutual exclusion. Route-scoped arbitration is a separate design (still open on #398). This commit is the narrow revert that restores coexistence. Reverts: - tokenexchange/plugin.go: Capabilities() returns empty PluginCapabilities; contracts import dropped. - tokenbroker/plugin.go: same. - plugins_test.go: removes TestBuild_TokenExchangeAndTokenBroker_ConflictingClaims (the integration test that locked in the mutex) and the tokenbroker side-effect import that only existed for that test. Retained on purpose (no behavior change, no consumer today): - PluginCapabilities.Claims field + validateRelationships Claims loop + related tests in registry_test.go — the framework-level mutex mechanism is general-purpose and may have valid uses for plugins that genuinely conflict chain-wide rather than per- route. No in-tree plugin currently declares Claims. - authlib/contracts/claims.go + ClaimAuthorizationHeader constant — kept so that a future plugin declaring this specific claim doesn't have to re-invent the vocabulary. Documented as unreferenced-by-in-tree-plugins to avoid signal confusion. Operationally: after this lands, configuring both token-exchange and token-broker in the same outbound chain becomes the operator's responsibility to partition cleanly across routes — same as the pre-#400 status quo. The plugins' own OnRequest behavior is unchanged by this revert (it was also unchanged by #400 — the mutex was build-time only). Relates to #398. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/authlib/plugins/plugins_test.go | 39 ------------------- .../authlib/plugins/tokenbroker/plugin.go | 10 +---- .../authlib/plugins/tokenexchange/plugin.go | 9 +---- 3 files changed, 2 insertions(+), 56 deletions(-) diff --git a/authbridge/authlib/plugins/plugins_test.go b/authbridge/authlib/plugins/plugins_test.go index 32ddc905f..70dbeddf5 100644 --- a/authbridge/authlib/plugins/plugins_test.go +++ b/authbridge/authlib/plugins/plugins_test.go @@ -184,42 +184,3 @@ func TestBuild_ConfigureError(t *testing.T) { } } -// TestBuild_TokenExchangeAndTokenBroker_ConflictingClaims exercises the -// concrete case from issue #398: configuring both token-exchange and -// token-broker on the same outbound chain is now rejected at Build -// time because they both claim ClaimAuthorizationHeader. -func TestBuild_TokenExchangeAndTokenBroker_ConflictingClaims(t *testing.T) { - // Configure both with valid per-plugin config so the - // relationship check is what fails (not some earlier Configure - // error). token-broker requires broker_url; token-exchange - // requires the keycloak_url / keycloak_realm pair. - _, err := plugins.Build([]config.PluginEntry{ - { - Name: "token-exchange", - Config: []byte(`{ - "keycloak_url": "http://keycloak.example", - "keycloak_realm": "test", - "default_policy": "passthrough", - "identity": {"type": "client-secret"} - }`), - }, - { - Name: "token-broker", - Config: []byte(`{"broker_url": "http://broker.example"}`), - }, - }) - if err == nil { - t.Fatal("expected relationship conflict error for token-exchange + token-broker") - } - msg := err.Error() - for _, want := range []string{ - "token-exchange", - "token-broker", - "authorization_header", - "configure only one", - } { - if !strings.Contains(msg, want) { - t.Errorf("error %q does not mention %q", err, want) - } - } -} diff --git a/authbridge/authlib/plugins/tokenbroker/plugin.go b/authbridge/authlib/plugins/tokenbroker/plugin.go index 7c9ced1aa..e7f74a587 100644 --- a/authbridge/authlib/plugins/tokenbroker/plugin.go +++ b/authbridge/authlib/plugins/tokenbroker/plugin.go @@ -13,7 +13,6 @@ import ( "strings" "github.com/gobwas/glob" - "github.com/kagenti/kagenti-extensions/authbridge/authlib/contracts" "github.com/kagenti/kagenti-extensions/authbridge/authlib/pipeline" "github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins" "github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins/tokenbroker/client" @@ -155,14 +154,7 @@ func NewTokenBroker() *TokenBroker { return &TokenBroker{} } func (p *TokenBroker) Name() string { return "token-broker" } func (p *TokenBroker) Capabilities() pipeline.PluginCapabilities { - return pipeline.PluginCapabilities{ - // token-broker takes exclusive ownership of the outbound - // Authorization header — same claim as token-exchange, so - // configuring both in the same outbound chain fails at - // plugins.Build rather than letting one silently clobber - // the other. - Claims: []string{contracts.ClaimAuthorizationHeader}, - } + return pipeline.PluginCapabilities{} } func (p *TokenBroker) Configure(raw json.RawMessage) error { diff --git a/authbridge/authlib/plugins/tokenexchange/plugin.go b/authbridge/authlib/plugins/tokenexchange/plugin.go index 439488744..f5d80b042 100644 --- a/authbridge/authlib/plugins/tokenexchange/plugin.go +++ b/authbridge/authlib/plugins/tokenexchange/plugin.go @@ -13,7 +13,6 @@ import ( "github.com/kagenti/kagenti-extensions/authbridge/authlib/auth" "github.com/kagenti/kagenti-extensions/authbridge/authlib/config" - "github.com/kagenti/kagenti-extensions/authbridge/authlib/contracts" "github.com/kagenti/kagenti-extensions/authbridge/authlib/pipeline" "github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins" "github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins/tokenexchange/cache" @@ -212,13 +211,7 @@ func init() { func (p *TokenExchange) Name() string { return "token-exchange" } func (p *TokenExchange) Capabilities() pipeline.PluginCapabilities { - return pipeline.PluginCapabilities{ - // token-exchange takes exclusive ownership of the outbound - // Authorization header. Any other plugin that also claims this - // resource (e.g., token-broker) will fail at plugins.Build - // time rather than silently clobbering our replaced token. - Claims: []string{contracts.ClaimAuthorizationHeader}, - } + return pipeline.PluginCapabilities{} } func (p *TokenExchange) Configure(raw json.RawMessage) error {