From 71a3a85b263383729d2be5f3deb55cda865549ee Mon Sep 17 00:00:00 2001 From: Jerry Yan <792602257@qq.com> Date: Fri, 8 May 2026 08:30:26 +0800 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20code=20review=20I2=20=E2=80=94=20ad?= =?UTF-8?q?min=20expires=5Fat=20must=20be=20future=20+=20Part=203=20tx=20T?= =?UTF-8?q?ODO=20+=20scope=20JSON=20round-trip=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - IssueAdmin now rejects past/now ExpiresAt with CodeMcpTokenExpiresRequired - TestIssueAdmin_ExpiresMustBeFuture covers the new path - IssueSystemToken doc comment notes pgRepo currently ignores ctx tx; Part 3 must add Repository.WithTx for true same-transaction guarantee with acp_sessions (spec §6.1) - New scope_test.go (whitebox) verifies JSON round-trip preserves nil-vs-empty semantics on Tools / ProjectIDs and exercises AllowsTool / AllowsProject edges that Authenticate / future tool gating depend on --- internal/mcp/scope_test.go | 83 ++++++++++++++++++++++++++++++ internal/mcp/token_service.go | 9 ++++ internal/mcp/token_service_test.go | 15 ++++++ 3 files changed, 107 insertions(+) create mode 100644 internal/mcp/scope_test.go diff --git a/internal/mcp/scope_test.go b/internal/mcp/scope_test.go new file mode 100644 index 0000000..72b240b --- /dev/null +++ b/internal/mcp/scope_test.go @@ -0,0 +1,83 @@ +package mcp + +import ( + "encoding/json" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Scope 的 JSON round-trip 必须保留 nil-vs-empty 语义： +// - nil Tools/ProjectIDs = "允许全部" +// - empty slice = "全部禁止" +// repository.go 的 scopeToJSON / scopeFromJSON 是 encoding/json 的薄封装， +// 这里直接测公开类型的 marshal/unmarshal 路径，不耦合具体 helper 实现。 + +func TestScope_JSON_RoundTrip_Empty(t *testing.T) { + t.Parallel() + + raw, err := json.Marshal(Scope{}) + require.NoError(t, err) + assert.JSONEq(t, "{}", string(raw), "empty Scope should marshal to {} via omitempty") + + var got Scope + require.NoError(t, json.Unmarshal(raw, &got)) + assert.Nil(t, got.Tools, "Tools must remain nil — meaning 'allow all'") + assert.Nil(t, got.ProjectIDs, "ProjectIDs must remain nil — meaning 'allow all'") +} + +func TestScope_JSON_RoundTrip_Populated(t *testing.T) { + t.Parallel() + + pid := uuid.New() + in := Scope{ + Tools: []string{"list_issues", "create_issue"}, + ProjectIDs: []uuid.UUID{pid}, + } + raw, err := json.Marshal(in) + require.NoError(t, err) + + var out Scope + require.NoError(t, json.Unmarshal(raw, &out)) + assert.Equal(t, in.Tools, out.Tools) + assert.Equal(t, in.ProjectIDs, out.ProjectIDs) +} + +func TestScope_NilVsEmpty_AllowsTool(t *testing.T) { + t.Parallel() + + var nilScope *Scope + assert.True(t, nilScope.AllowsTool("anything"), "nil receiver allows all") + + zero := &Scope{} // Tools/ProjectIDs both nil + assert.True(t, zero.AllowsTool("anything"), "nil Tools allows all") + + empty := &Scope{Tools: []string{}} + assert.False(t, empty.AllowsTool("anything"), "empty Tools forbids all") + + allow := &Scope{Tools: []string{"list_issues"}} + assert.True(t, allow.AllowsTool("list_issues")) + assert.False(t, allow.AllowsTool("create_issue")) +} + +func TestScope_NilVsEmpty_AllowsProject(t *testing.T) { + t.Parallel() + + pidA := uuid.New() + pidB := uuid.New() + + var nilScope *Scope + assert.True(t, nilScope.AllowsProject(pidA)) + + zero := &Scope{} + assert.True(t, zero.AllowsProject(pidA), "nil ProjectIDs allows all") + + empty := &Scope{ProjectIDs: []uuid.UUID{}} + assert.False(t, empty.AllowsProject(pidA), "empty ProjectIDs forbids all") + + allow := &Scope{ProjectIDs: []uuid.UUID{pidA}} + assert.True(t, allow.AllowsProject(pidA)) + assert.False(t, allow.AllowsProject(pidB)) +} diff --git a/internal/mcp/token_service.go b/internal/mcp/token_service.go index 9713f53..d276042 100644 --- a/internal/mcp/token_service.go +++ b/internal/mcp/token_service.go @@ -79,6 +79,9 @@ func (s *tokenService) IssueAdmin(ctx context.Context, in IssueAdminInput) (Issu if in.ExpiresAt.IsZero() { return IssueResult{}, errs.New(errs.CodeMcpTokenExpiresRequired, "expires_at is required for admin token") } + if !in.ExpiresAt.After(time.Now()) { + return IssueResult{}, errs.New(errs.CodeMcpTokenExpiresRequired, "expires_at must be in the future") + } plain, hash, err := NewToken() if err != nil { @@ -128,6 +131,12 @@ func (s *tokenService) IssueAdmin(ctx context.Context, in IssueAdminInput) (Issu // FK 校验失败。spec §6.1 明确建议同事务提交。本 service 不直接管事务边界（由 // ACP SessionService 在 Create 流程内编排），但调用顺序由调用方保证。 // +// TODO(Part 3): 当前 pgRepo 持有 *pgxpool.Pool 并忽略 ctx 中可能存在的 pgx tx， +// 因此严格意义上 IssueSystemToken 与 acp_sessions row 的写入并不在同一事务里 —— +// 调用顺序虽然能避开 FK 错误，但不是真正的原子提交。要兑现 spec §6.1 同事务 +// 保证，Part 3 需要为 Repository 增加 `WithTx(tx pgx.Tx) Repository` 构造， +// 并在 acp.SessionService.Create 中以同一事务执行 acp_sessions 插入与本次签发。 +// // 默认 ExpiresIn = 24h；默认 Tools 为内置 PM 工具白名单（spec §6.2）。 // 写一条 audit `mcp.token.create_system`。 func (s *tokenService) IssueSystemToken(ctx context.Context, in IssueSystemTokenInput) (IssueResult, error) { diff --git a/internal/mcp/token_service_test.go b/internal/mcp/token_service_test.go index 8e21980..798c4e0 100644 --- a/internal/mcp/token_service_test.go +++ b/internal/mcp/token_service_test.go @@ -196,6 +196,21 @@ func TestIssueAdmin_ExpiresRequired(t *testing.T) { assert.Equal(t, errs.CodeMcpTokenExpiresRequired, ae.Code) } +func TestIssueAdmin_ExpiresMustBeFuture(t *testing.T) { + t.Parallel() + svc := mcp.NewTokenService(newFakeRepo(), &fakeAudit{}) + + _, err := svc.IssueAdmin(context.Background(), mcp.IssueAdminInput{ + UserID: uuid.New(), + Name: "x", + ExpiresAt: time.Now().Add(-time.Hour), // past + CreatedBy: uuid.New(), + }) + ae, ok := errs.As(err) + require.True(t, ok) + assert.Equal(t, errs.CodeMcpTokenExpiresRequired, ae.Code) +} + func TestIssueSystemToken_Defaults(t *testing.T) { t.Parallel() repo := newFakeRepo()