Skip to content

Commit 4dffa43

Browse files
tgrunnagleclaude
andcommitted
Drop redundant platformUserID param from UpstreamFilter
The canonical user is already carried as principal.PlatformUserID, so the separate platformUserID argument to FilterUpstreams was redundant. Remove it and have consumers read principal.PlatformUserID; update the doc, the stub filter, and the tests accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 3dd8352 commit 4dffa43

3 files changed

Lines changed: 11 additions & 21 deletions

File tree

pkg/authserver/server/handlers/callback_test.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -752,12 +752,10 @@ func TestCallbackHandler_Filter_ReceivesFirstUpstreamIdentity(t *testing.T) {
752752
"groups": []any{"engineering", "admins"},
753753
}, filter.capturedPrincipal.Claims, "principal.Claims must carry the first upstream's claims")
754754

755-
// The platform user ID is the canonical (resolved) ToolHive user: non-empty,
756-
// equal to principal.PlatformUserID, and distinct from the raw upstream subject.
757-
assert.NotEmpty(t, filter.capturedUser, "platform user ID must be populated")
758-
assert.Equal(t, filter.capturedUser, filter.capturedPrincipal.PlatformUserID,
759-
"the standalone platform user ID must mirror principal.PlatformUserID")
760-
assert.NotEqual(t, "user-from-provider1", filter.capturedUser,
755+
// principal.PlatformUserID is the canonical (resolved) ToolHive user: non-empty
756+
// and distinct from the raw upstream subject.
757+
assert.NotEmpty(t, filter.capturedPrincipal.PlatformUserID, "platform user ID must be populated")
758+
assert.NotEqual(t, "user-from-provider1", filter.capturedPrincipal.PlatformUserID,
761759
"platform user ID is the canonical ToolHive user, not the raw upstream subject")
762760
}
763761

pkg/authserver/server/handlers/handler.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,11 @@ type Handler struct {
6262
//
6363
// FilterUpstreams receives, in order:
6464
// - ctx: the request context of the first leg's callback.
65-
// - platformUserID: the canonical ToolHive user ID resolved from the first
66-
// upstream (the stable internal identifier; equals principal.PlatformUserID).
6765
// - principal: the upstream-derived identity for authorization decisions. Its
68-
// Subject is the claim-mapped upstream subject (OIDC SubjectClaim, or "sub");
69-
// Claims carries the ID-token/userinfo claims; Name and Email are populated
70-
// when the upstream provides them. It carries NO tokens — it is the
66+
// PlatformUserID is the canonical ToolHive user ID (the stable internal
67+
// identifier); Subject is the claim-mapped upstream subject (OIDC SubjectClaim,
68+
// or "sub"); Claims carries the ID-token/userinfo claims; Name and Email are
69+
// populated when the upstream provides them. It carries NO tokens — it is the
7170
// credential-free auth.PrincipalInfo projection. The filter MUST treat
7271
// principal (including its Claims map) as read-only.
7372
// - configured: the names of the non-first configured upstreams, in configured
@@ -95,7 +94,6 @@ type Handler struct {
9594
type UpstreamFilter interface {
9695
FilterUpstreams(
9796
ctx context.Context,
98-
platformUserID string,
9997
principal auth.PrincipalInfo,
10098
configured []string,
10199
) ([]string, error)
@@ -278,7 +276,7 @@ func (h *Handler) computeChain(ctx context.Context, principal auth.PrincipalInfo
278276
return append(chain, restNames...), nil
279277
}
280278

281-
keep, err := h.filter.FilterUpstreams(ctx, principal.PlatformUserID, principal, restNames)
279+
keep, err := h.filter.FilterUpstreams(ctx, principal, restNames)
282280
if err != nil {
283281
return nil, fmt.Errorf("upstream filter failed: %w", err)
284282
}

pkg/authserver/server/handlers/handler_chain_test.go

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,26 +32,22 @@ var twoUpstreamChain = []string{"provider-1", "provider-2"}
3232
// stubUpstreamFilter is a hand-written test double for UpstreamFilter, mirroring
3333
// the mockIDPProvider pattern used elsewhere in this package. It records how many
3434
// times it was called and the arguments it was passed (the non-first upstream
35-
// names, the platform user ID, and the resolved principal), and returns a canned
36-
// keep set or error.
35+
// names and the resolved principal), and returns a canned keep set or error.
3736
type stubUpstreamFilter struct {
3837
keep []string
3938
err error
4039
calls int
4140
capturedArgs []string
42-
capturedUser string
4341
capturedPrincipal auth.PrincipalInfo
4442
}
4543

4644
func (f *stubUpstreamFilter) FilterUpstreams(
4745
_ context.Context,
48-
platformUserID string,
4946
principal auth.PrincipalInfo,
5047
configured []string,
5148
) ([]string, error) {
5249
f.calls++
5350
f.capturedArgs = configured
54-
f.capturedUser = platformUserID
5551
f.capturedPrincipal = principal
5652
if f.err != nil {
5753
return nil, f.err
@@ -172,10 +168,8 @@ func TestComputeChain(t *testing.T) {
172168
"filter must receive non-first configured upstreams in order")
173169
}
174170
if tt.wantCalls > 0 {
175-
assert.Equal(t, principal.PlatformUserID, tt.filter.capturedUser,
176-
"filter must receive the platform user ID")
177171
assert.Equal(t, principal, tt.filter.capturedPrincipal,
178-
"filter must receive the resolved principal (subject + claims) verbatim")
172+
"filter must receive the resolved principal (platform user, subject + claims) verbatim")
179173
}
180174
}
181175
})

0 commit comments

Comments
 (0)