From fd446567d5679fd6a272dd7a9ccaa0119243002a Mon Sep 17 00:00:00 2001 From: muralx Date: Wed, 17 Jun 2026 00:20:34 +0100 Subject: [PATCH] fix: shared-client Close() safety in adapters; harden CI workflows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - mcp, mark3labs: `(*Adapter).Close()` is now a no-op for adapters built with `NewAdapterFromClientAndResource`. Such adapters do not own the caller-supplied `*authplane.Client`, so closing one adapter no longer tears down a client shared across other adapters. `NewAdapter`-built adapters still own and close the client they create. - http: `(*Adapter).RequireScopes` now names every missing scope in the `error_description` instead of returning on the first miss, by delegating to `claims.RequireScopes`. Middleware-enforced and code-enforced scope checks now surface the same diagnostic (RFC 6750 §3). - ci: add CodeQL analysis and workflow linting (actionlint + shellcheck), pin third-party actions to commit SHAs, and clear Scorecard findings. - mcp: bump indirect `golang.org/x/sys` to v0.44.0. --- .github/workflows/backport-fixes.yml | 5 ++ .github/workflows/ci.yml | 4 + .github/workflows/codeql.yml | 73 +++++++++++++++++++ .github/workflows/cut-release.yml | 5 ++ .github/workflows/dependency-review.yml | 4 + .github/workflows/release.yml | 5 ++ .github/workflows/security.yml | 4 +- .github/workflows/workflows-lint.yml | 64 ++++++++++++++++ core/resource/verifier/claims.go | 11 +-- http/pkg/authplanehttp/adapter.go | 18 +++-- http/pkg/authplanehttp/adapter_test.go | 25 +++++++ mark3labs/pkg/authplanemark3labs/adapter.go | 25 +++++-- .../authplanemark3labs/constructor_test.go | 26 +++++++ mcp/go.mod | 2 +- mcp/go.sum | 4 +- mcp/pkg/authplanemcp/adapter.go | 36 +++++---- mcp/pkg/authplanemcp/constructor_test.go | 26 +++++++ 17 files changed, 298 insertions(+), 39 deletions(-) create mode 100644 .github/workflows/codeql.yml create mode 100644 .github/workflows/workflows-lint.yml diff --git a/.github/workflows/backport-fixes.yml b/.github/workflows/backport-fixes.yml index c913339..d5b4c62 100644 --- a/.github/workflows/backport-fixes.yml +++ b/.github/workflows/backport-fixes.yml @@ -31,6 +31,11 @@ concurrency: group: backport-${{ inputs.fromBranch }}-to-${{ inputs.toBranch }} cancel-in-progress: false +# Least-privilege default; the backport job below elevates to the writes +# it needs (push the backport branch, open a PR). +permissions: + contents: read + jobs: backport: runs-on: ubuntu-latest diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4a00090..e5d1adf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,6 +6,10 @@ on: branches: - main +# Least-privilege default for all jobs; this workflow only reads the repo. +permissions: + contents: read + jobs: quality: runs-on: ubuntu-latest diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 0000000..f40d951 --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,73 @@ +name: CodeQL + +# Static analysis (SAST) for the Go modules in this repo. Results are +# uploaded to the GitHub Security tab. +# +# Gated to public repositories only: CodeQL scanning requires GitHub +# Advanced Security, which is free on public repos but not enabled on the +# private fork — running it there would fail every PR. The `if:` guard +# keys off the repo's actual visibility, so this works for any public +# clone and stays a no-op on private ones. Checking visibility == 'public' +# (rather than != 'private') also fails safe: if the field is ever absent +# the job is skipped rather than run. + +on: + push: + branches: + - main + pull_request: + branches: + - main + schedule: + # Wednesdays 06:00 UTC. + - cron: "0 6 * * 3" + workflow_dispatch: + +# Least-privilege default; the job below grants only the writes CodeQL needs. +permissions: + contents: read + +jobs: + analyze: + name: Analyze (Go) + # Public repos only — see the header comment. Skips cleanly on the fork. + if: github.event.repository.visibility == 'public' + runs-on: ubuntu-latest + permissions: + # Required for CodeQL to upload results to the Security tab. + security-events: write + contents: read + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Setup Go + uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 + with: + go-version: "1.25.x" + check-latest: true + + - name: Initialize CodeQL + uses: github/codeql-action/init@c35d1b164463ee62a100735382aaaa525c5d3496 # codeql-bundle-v2.25.6 + with: + languages: go + # This repo is a Go workspace of four independent modules; the + # default autobuild can't span them, so we build each module + # manually below. + build-mode: manual + + - name: Build modules + # Compile every module so the extractor sees all packages. go.work + # ties the modules together for local dev; here we build each one + # in its own directory to keep the extractor's view complete. + run: | + for module in core http mcp mark3labs; do + echo "::group::build $module" + (cd "$module" && go build ./...) + echo "::endgroup::" + done + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@c35d1b164463ee62a100735382aaaa525c5d3496 # codeql-bundle-v2.25.6 + with: + category: "/language:go" diff --git a/.github/workflows/cut-release.yml b/.github/workflows/cut-release.yml index 25e9262..36e8a63 100644 --- a/.github/workflows/cut-release.yml +++ b/.github/workflows/cut-release.yml @@ -44,6 +44,11 @@ concurrency: group: cut-release cancel-in-progress: false +# Least-privilege default; the cut job below elevates to contents: write +# to push the new release branch. +permissions: + contents: read + jobs: cut: runs-on: ubuntu-latest diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index ec0cf66..f1a7a42 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -3,6 +3,10 @@ name: Dependency Review on: pull_request: +# Least-privilege default; the job below grants itself the writes it needs. +permissions: + contents: read + jobs: dependency-review: runs-on: ubuntu-latest diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index d871bc8..c9721bf 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -50,6 +50,11 @@ concurrency: group: release-${{ github.ref }} cancel-in-progress: false +# Least-privilege default; the release job below elevates to contents: write +# to push tags/branch and create the GitHub Release. +permissions: + contents: read + jobs: release: runs-on: ubuntu-latest diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index afc3e67..8b93675 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -56,7 +56,9 @@ jobs: cache-dependency-path: "${{ matrix.module }}/go.sum" - name: Install govulncheck - run: go install golang.org/x/vuln/cmd/govulncheck@latest + # Pinned to a tagged release rather than @latest so the toolchain is + # reproducible and the build can't be moved by an upstream retag. + run: go install golang.org/x/vuln/cmd/govulncheck@v1.3.0 - name: Run govulncheck # Exits non-zero on any known vulnerability in the module's call graph diff --git a/.github/workflows/workflows-lint.yml b/.github/workflows/workflows-lint.yml new file mode 100644 index 0000000..dfadf36 --- /dev/null +++ b/.github/workflows/workflows-lint.yml @@ -0,0 +1,64 @@ +name: Lint workflows + +# Catches workflow YAML / shell-in-`run:` regressions at PR time so a +# typo can't reach a release tag and surface only when a publish run +# fails. Scoped to changes under `.github/workflows/**` to keep CI +# overhead off unrelated PRs. + +on: + pull_request: + paths: + - ".github/workflows/**" + push: + branches: + - main + paths: + - ".github/workflows/**" + +permissions: + contents: read + +jobs: + actionlint: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + # Pulls the matching actionlint binary release from GitHub Releases + # via the upstream download script. The script is fetched by commit + # SHA (not a mutable tag) and sha256-verified before it runs — this + # closes Scorecard's "downloadThenRun not pinned by hash" gap. The + # script then checksum-verifies the actionlint binary it pulls from + # the matching release. + # + # To bump: change ACTIONLINT_VERSION, set ACTIONLINT_SCRIPT_SHA to the + # commit the new tag points at (`gh api repos/rhysd/actionlint/commits/vX.Y.Z -q .sha`), + # and update ACTIONLINT_SCRIPT_SHA256 to that file's sha256. + # + # Install dir is passed explicitly as the script's second positional + # arg so the workflow doesn't couple to the script's internal default + # of $PWD (which happens to be $GITHUB_WORKSPACE after checkout — + # a coincidence, not a contract). + - name: Install actionlint + env: + ACTIONLINT_VERSION: "1.7.7" + ACTIONLINT_SCRIPT_SHA: "03d0035246f3e81f36aed592ffb4bebf33a03106" + ACTIONLINT_SCRIPT_SHA256: "221d1d16c03e4e4fcd867de34104e8d479bdce20ccdfa553b9a5c0dc29bf6af2" + ACTIONLINT_INSTALL_DIR: ${{ runner.temp }}/actionlint + run: | + mkdir -p "${ACTIONLINT_INSTALL_DIR}" + script="${ACTIONLINT_INSTALL_DIR}/download-actionlint.bash" + curl -fsSL -o "${script}" \ + "https://raw.githubusercontent.com/rhysd/actionlint/${ACTIONLINT_SCRIPT_SHA}/scripts/download-actionlint.bash" + echo "${ACTIONLINT_SCRIPT_SHA256} ${script}" | sha256sum -c - + bash "${script}" "${ACTIONLINT_VERSION}" "${ACTIONLINT_INSTALL_DIR}" + echo "${ACTIONLINT_INSTALL_DIR}" >> "${GITHUB_PATH}" + "${ACTIONLINT_INSTALL_DIR}/actionlint" -version + + # `-shellcheck=shellcheck` makes the shellcheck dependency explicit + # rather than relying on actionlint's implicit lookup against the + # runner image's $PATH; if the Ubuntu image ever drops shellcheck the + # job fails loudly instead of silently degrading. + - name: Run actionlint + run: actionlint -color -shellcheck=shellcheck diff --git a/core/resource/verifier/claims.go b/core/resource/verifier/claims.go index 65e3b7d..a55b246 100644 --- a/core/resource/verifier/claims.go +++ b/core/resource/verifier/claims.go @@ -103,13 +103,10 @@ func (c *VerifiedClaims) HasScope(scope string) bool { // Thin wrapper over [RequireScopes] so the singular helper carries the // same enriched `required scope "X"; token has scopes: …` shape the // plural one does. The wire body produced from this path is byte-identical -// to a `claims.RequireScopes(scope)` call. Note that the adapter middleware -// at github.com/authplane/go-sdk/http/pkg/authplanehttp.Adapter.RequireScopes -// still loops over scopes and returns on the first miss, so a multi-scope -// adapter failure names only one scope; a direct -// `claims.RequireScopes("a", "b", ...)` call names every missing scope. -// The two paths converge only on the single-missing-scope case until the -// adapter is switched over to the plural helper. +// to a `claims.RequireScopes(scope)` call, and the adapter middleware at +// github.com/authplane/go-sdk/http/pkg/authplanehttp.Adapter.RequireScopes +// delegates to RequireScopes too, so middleware-enforced and code-enforced +// paths produce the same error shape on a multi-scope failure. func (c *VerifiedClaims) RequireScope(scope string) error { return c.RequireScopes(scope) } diff --git a/http/pkg/authplanehttp/adapter.go b/http/pkg/authplanehttp/adapter.go index fe91821..4902228 100644 --- a/http/pkg/authplanehttp/adapter.go +++ b/http/pkg/authplanehttp/adapter.go @@ -199,6 +199,10 @@ func (a *Adapter) Middleware() func(http.Handler) http.Handler { // for all required scopes. Returns 403 with RFC 6750 WWW-Authenticate header // (including scope= parameter) if any scope is missing. Returns 401 if no claims // are in context (i.e., Middleware was not applied upstream). +// +// On failure the error_description names every missing scope (not just the first), +// matching the shape produced by a direct claims.RequireScopes call so middleware- +// enforced and code-enforced paths surface the same diagnostic to clients. func (a *Adapter) RequireScopes(scopes ...string) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -207,14 +211,12 @@ func (a *Adapter) RequireScopes(scopes ...string) func(http.Handler) http.Handle a.writeAuthError(w, verifier.ErrTokenMissing) return } - for _, scope := range scopes { - if err := claims.RequireScope(scope); err != nil { - a.writeAuthError(w, &resource.ScopeError{ - RequiredScopes: scopes, - Err: err, - }) - return - } + if err := claims.RequireScopes(scopes...); err != nil { + a.writeAuthError(w, &resource.ScopeError{ + RequiredScopes: scopes, + Err: err, + }) + return } next.ServeHTTP(w, r) }) diff --git a/http/pkg/authplanehttp/adapter_test.go b/http/pkg/authplanehttp/adapter_test.go index 34555f5..191f69d 100644 --- a/http/pkg/authplanehttp/adapter_test.go +++ b/http/pkg/authplanehttp/adapter_test.go @@ -341,6 +341,31 @@ func TestRequireScopesMultipleOneMissing(t *testing.T) { } } +// TestRequireScopesMultipleAllMissingNamesEveryScope verifies the middleware +// surfaces all missing scopes (not just the first) in the error_description, +// matching the shape of a direct claims.RequireScopes call. The +// WWW-Authenticate header carries the scopes space-separated per RFC 6750 §3; +// the enriched quoted-list shape lives in the JSON body's error_description. +func TestRequireScopesMultipleAllMissingNamesEveryScope(t *testing.T) { + e := newTestEnv(t) + handler := e.adapter.Middleware()(e.adapter.RequireScopes("tools/admin", "tools/superuser")(okHandler())) + token := e.makeToken(t, []string{"tools/add"}, time.Now().Add(time.Hour)) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/mcp/admin", nil) + req.Header.Set("Authorization", "Bearer "+token) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403", rec.Code) + } + if got := rec.Header().Get("WWW-Authenticate"); !strings.Contains(got, `scope="tools/admin tools/superuser"`) { + t.Errorf("WWW-Authenticate = %q, want scope=\"tools/admin tools/superuser\"", got) + } + body := rec.Body.String() + if !strings.Contains(body, `\"tools/admin\"`) || !strings.Contains(body, `\"tools/superuser\"`) { + t.Errorf("body = %s, want error_description to name every missing scope (not just the first)", body) + } +} + // Case-insensitive scheme tests func TestMiddleware_BearerCaseInsensitive(t *testing.T) { diff --git a/mark3labs/pkg/authplanemark3labs/adapter.go b/mark3labs/pkg/authplanemark3labs/adapter.go index 31b0b87..e090ee0 100644 --- a/mark3labs/pkg/authplanemark3labs/adapter.go +++ b/mark3labs/pkg/authplanemark3labs/adapter.go @@ -78,6 +78,7 @@ type Adapter struct { client *authplane.Client prmHandler http.Handler // mark3labs PRM handler, built once at construction + ownsClient bool // true when this Adapter constructed the client and must close it } // ClaimsFromContext returns the VerifiedClaims injected by Middleware (and @@ -138,13 +139,15 @@ func NewAdapter(ctx context.Context, options Options) (*Adapter, error) { Adapter: authplanehttp.New(res), client: client, prmHandler: server.NewProtectedResourceMetadataHandler(prmConfigFromResource(res)), + ownsClient: true, }, nil } // NewAdapterFromClientAndResource creates an Adapter from an already-configured -// authplane.Client and resource.Resource. Adapter.Close() still calls client.Close(); -// when sharing a client across adapters, manage client lifecycle yourself and let -// the adapters go out of scope. +// authplane.Client and resource.Resource. The caller retains ownership of the +// client — adapter.Close() is a no-op for adapters created this way, so a +// client shared across multiple adapters keeps running even after one of them +// is closed. // // Returns an error if client or res is nil. The signature matches the sibling // mcp adapter — neither constructor panics, and both surface programming @@ -160,6 +163,7 @@ func NewAdapterFromClientAndResource(client *authplane.Client, res *resource.Res Adapter: authplanehttp.New(res), client: client, prmHandler: server.NewProtectedResourceMetadataHandler(prmConfigFromResource(res)), + ownsClient: false, }, nil } @@ -325,8 +329,10 @@ func prmConfigFromResource(res *resource.Resource) server.ProtectedResourceMetad // Client returns the underlying authplane.Client, providing access to all SDK // operations: TokenExchange, Revoke, Introspect, ClientCredentials, DPoPSigner, etc. // -// Do not call Close() on the returned client directly — call adapter.Close() instead, -// as the adapter owns the client lifecycle. +// For adapters built with NewAdapter, do not call Close() on the returned client +// directly — call adapter.Close() instead, as the adapter owns the client lifecycle. +// For adapters built with NewAdapterFromClientAndResource, the caller owns the +// client and is responsible for closing it. func (a *Adapter) Client() *authplane.Client { return a.client } @@ -334,9 +340,14 @@ func (a *Adapter) Client() *authplane.Client { // Close stops all background goroutines and releases resources held by the // underlying client. It is safe to call multiple times. // -// When using NewAdapterFromClientAndResource and sharing a client across multiple -// adapters, call client.Close() directly instead of adapter.Close(). +// For adapters built with NewAdapter, Close closes the underlying client. +// For adapters built with NewAdapterFromClientAndResource, the caller owns the +// client; Close is a no-op so a client shared across multiple adapters keeps +// running even after one of them is closed. func (a *Adapter) Close() error { + if !a.ownsClient { + return nil + } return a.client.Close() } diff --git a/mark3labs/pkg/authplanemark3labs/constructor_test.go b/mark3labs/pkg/authplanemark3labs/constructor_test.go index 96b1861..70b91f6 100644 --- a/mark3labs/pkg/authplanemark3labs/constructor_test.go +++ b/mark3labs/pkg/authplanemark3labs/constructor_test.go @@ -105,6 +105,32 @@ func TestNewAdapterFromClientAndResource(t *testing.T) { } } +// TestNewAdapterFromClientAndResource_CloseDoesNotCloseSharedClient verifies +// that closing an adapter built from an externally-owned client leaves the +// shared client running — the lifecycle contract documented on +// NewAdapterFromClientAndResource and Close. +func TestNewAdapterFromClientAndResource_CloseDoesNotCloseSharedClient(t *testing.T) { + e := newTestEnv(t) + + adapter2, err := authplanemark3labs.NewAdapterFromClientAndResource(e.adapter.Client(), e.adapter.Resource()) + if err != nil { + t.Fatalf("NewAdapterFromClientAndResource: %v", err) + } + if err := adapter2.Close(); err != nil { + t.Fatalf("adapter2.Close: %v", err) + } + + // The shared client must still serve requests through the original adapter. + token := e.makeToken(t, []string{"tools/add"}, time.Now().Add(time.Hour)) + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/mcp", nil) + req.Header.Set("Authorization", "Bearer "+token) + rec := httptest.NewRecorder() + e.adapter.AuthMiddleware(okHandler()).ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Errorf("status = %d, want 200; shared client must remain usable after adapter2.Close()", rec.Code) + } +} + // TestNewAdapterFromClientAndResourceNilClient verifies that a nil client // yields a clear error rather than a panic — the constructor's signature // matches the sibling mcp adapter. diff --git a/mcp/go.mod b/mcp/go.mod index 6d495b4..6098250 100644 --- a/mcp/go.mod +++ b/mcp/go.mod @@ -16,7 +16,7 @@ require ( github.com/segmentio/encoding v0.5.4 // indirect github.com/yosida95/uritemplate/v3 v3.0.2 // indirect golang.org/x/oauth2 v0.34.0 // indirect - golang.org/x/sys v0.40.0 // indirect + golang.org/x/sys v0.44.0 // indirect ) replace github.com/authplane/go-sdk/core => ../core diff --git a/mcp/go.sum b/mcp/go.sum index 0e05292..6a9bcfa 100644 --- a/mcp/go.sum +++ b/mcp/go.sum @@ -16,7 +16,7 @@ github.com/yosida95/uritemplate/v3 v3.0.2 h1:Ed3Oyj9yrmi9087+NczuL5BwkIc4wvTb5zI github.com/yosida95/uritemplate/v3 v3.0.2/go.mod h1:ILOh0sOhIJR3+L/8afwt/kE++YT040gmv5BQTMR2HP4= golang.org/x/oauth2 v0.34.0 h1:hqK/t4AKgbqWkdkcAeI8XLmbK+4m4G5YeQRrmiotGlw= golang.org/x/oauth2 v0.34.0/go.mod h1:lzm5WQJQwKZ3nwavOZ3IS5Aulzxi68dUSgRHujetwEA= -golang.org/x/sys v0.40.0 h1:DBZZqJ2Rkml6QMQsZywtnjnnGvHza6BTfYFWY9kjEWQ= -golang.org/x/sys v0.40.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= +golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= +golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/tools v0.41.0 h1:a9b8iMweWG+S0OBnlU36rzLp20z1Rp10w+IY2czHTQc= golang.org/x/tools v0.41.0/go.mod h1:XSY6eDqxVNiYgezAVqqCeihT4j1U2CCsqvH3WhQpnlg= diff --git a/mcp/pkg/authplanemcp/adapter.go b/mcp/pkg/authplanemcp/adapter.go index ed839b9..8b7ea90 100644 --- a/mcp/pkg/authplanemcp/adapter.go +++ b/mcp/pkg/authplanemcp/adapter.go @@ -67,9 +67,10 @@ type Options struct { // Always call Close() when the adapter is no longer needed to stop background // refresh goroutines and release HTTP connections. type Adapter struct { - client *authplane.Client - resource *resource.Resource - prmURL string // full URL for WWW-Authenticate ResourceMetadataURL + client *authplane.Client + resource *resource.Resource + prmURL string // full URL for WWW-Authenticate ResourceMetadataURL + ownsClient bool // true when this Adapter constructed the client and must close it } // NewAdapter creates and initializes an Adapter. It calls authplane.NewClient, @@ -110,9 +111,10 @@ func NewAdapter(ctx context.Context, options Options) (*Adapter, error) { } return &Adapter{ - client: client, - resource: res, - prmURL: res.PRMURL(), + client: client, + resource: res, + prmURL: res.PRMURL(), + ownsClient: true, }, nil } @@ -137,9 +139,10 @@ func NewAdapterFromClientAndResource(client *authplane.Client, res *resource.Res return nil, errors.New("authplane-mcp: res must not be nil") } return &Adapter{ - client: client, - resource: res, - prmURL: res.PRMURL(), + client: client, + resource: res, + prmURL: res.PRMURL(), + ownsClient: false, }, nil } @@ -216,8 +219,10 @@ func (a *Adapter) WellKnownPRMPath() string { // Client returns the underlying authplane.Client, providing access to all SDK // operations: TokenExchange, Revoke, Introspect, ClientCredentials, DPoPSigner, etc. // -// Do not call Close() on the returned client directly — call adapter.Close() instead, -// as the adapter owns the client lifecycle. +// For adapters built with NewAdapter, do not call Close() on the returned client +// directly — call adapter.Close() instead, as the adapter owns the client lifecycle. +// For adapters built with NewAdapterFromClientAndResource, the caller owns the +// client and is responsible for closing it. func (a *Adapter) Client() *authplane.Client { return a.client } @@ -244,9 +249,14 @@ func TokenFromContext(ctx context.Context) string { // Close stops all background goroutines and releases resources held by the // underlying client. It is safe to call Close multiple times. // -// When using NewAdapterFromClientAndResource and sharing a client across multiple -// adapters, call client.Close() directly instead of adapter.Close(). +// For adapters built with NewAdapter, Close closes the underlying client. +// For adapters built with NewAdapterFromClientAndResource, the caller owns the +// client; Close is a no-op so a client shared across multiple adapters keeps +// running even after one of them is closed. func (a *Adapter) Close() error { + if !a.ownsClient { + return nil + } return a.client.Close() } diff --git a/mcp/pkg/authplanemcp/constructor_test.go b/mcp/pkg/authplanemcp/constructor_test.go index e04fc68..235754f 100644 --- a/mcp/pkg/authplanemcp/constructor_test.go +++ b/mcp/pkg/authplanemcp/constructor_test.go @@ -107,6 +107,32 @@ func TestNewAdapterFromClientAndResource(t *testing.T) { } } +// TestNewAdapterFromClientAndResource_CloseDoesNotCloseSharedClient verifies +// that closing an adapter built from an externally-owned client leaves the +// shared client running — the lifecycle contract documented on +// NewAdapterFromClientAndResource and Close. +func TestNewAdapterFromClientAndResource_CloseDoesNotCloseSharedClient(t *testing.T) { + e := newTestEnv(t) + + adapter2, err := authplanemcp.NewAdapterFromClientAndResource(e.adapter.Client(), e.adapter.Resource()) + if err != nil { + t.Fatalf("NewAdapterFromClientAndResource: %v", err) + } + if err := adapter2.Close(); err != nil { + t.Fatalf("adapter2.Close: %v", err) + } + + // The shared client must still serve requests through the original adapter. + token := e.makeToken(t, []string{"tools/add"}, time.Now().Add(time.Hour)) + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/mcp", nil) + req.Header.Set("Authorization", "Bearer "+token) + rec := httptest.NewRecorder() + e.adapter.AuthMiddleware(okHandler()).ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Errorf("status = %d, want 200; shared client must remain usable after adapter2.Close()", rec.Code) + } +} + // TestNewAdapterFromClientAndResourceNilClient verifies that a nil client // yields a clear error rather than a later nil-pointer dereference inside // AuthMiddleware/Close.