mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-14 22:17:36 +00:00
fix(gateway): guard config.* methods against non-master tenant scope
Non-master tenant admin calling config.patch/apply/get/schema would corrupt master *config.Config + config.json on disk via m.cfg.ReplaceFrom and m.Save, leaking master config across tenants. Add requireMasterScope middleware that rejects non-master tenant ctx (system-owner role still bypasses for consistency with requireTenantAdmin). Chain before requireOwner on all 4 config.* methods. Adds MsgConfigMasterScopeOnly i18n key (en/vi/zh) and 10 unit tests covering helper + middleware + chained fail-closed behavior.
This commit is contained in:
1 parent
dc482ff169
commit
b419f35248
6 files changed
+208
-12
No files matched your search
@@ -5,6 +5,7 @@ import (
|
|||||||
"encoding/json"
|
"encoding/json"
|
||||||
"log/slog"
|
"log/slog"
|
||||||
|
|
||||||
|
"github.com/google/uuid"
|
||||||
"github.com/titanous/json5"
|
"github.com/titanous/json5"
|
||||||
|
|
||||||
"github.com/nextlevelbuilder/goclaw/internal/bus"
|
"github.com/nextlevelbuilder/goclaw/internal/bus"
|
||||||
@@ -36,10 +37,10 @@ func (m *ConfigMethods) SetSystemConfigSync(fn func(ctx context.Context, cfg *co
|
|||||||
}
|
}
|
||||||
|
|
||||||
func (m *ConfigMethods) Register(router *gateway.MethodRouter) {
|
func (m *ConfigMethods) Register(router *gateway.MethodRouter) {
|
||||||
router.Register(protocol.MethodConfigGet, m.requireOwner(m.handleGet))
|
router.Register(protocol.MethodConfigGet, m.requireMasterScope(m.requireOwner(m.handleGet)))
|
||||||
router.Register(protocol.MethodConfigApply, m.requireOwner(m.handleApply))
|
router.Register(protocol.MethodConfigApply, m.requireMasterScope(m.requireOwner(m.handleApply)))
|
||||||
router.Register(protocol.MethodConfigPatch, m.requireOwner(m.handlePatch))
|
router.Register(protocol.MethodConfigPatch, m.requireMasterScope(m.requireOwner(m.handlePatch)))
|
||||||
router.Register(protocol.MethodConfigSchema, m.requireOwner(m.handleSchema))
|
router.Register(protocol.MethodConfigSchema, m.requireMasterScope(m.requireOwner(m.handleSchema)))
|
||||||
}
|
}
|
||||||
|
|
||||||
// requireOwner wraps a handler to only allow owner-role users.
|
// requireOwner wraps a handler to only allow owner-role users.
|
||||||
@@ -57,6 +58,40 @@ func (m *ConfigMethods) requireOwner(next gateway.MethodHandler) gateway.MethodH
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// requireMasterScope rejects config.* calls when the caller's ctx is scoped to
|
||||||
|
// a non-master tenant. System owner callers (bypass-all) are allowed through.
|
||||||
|
//
|
||||||
|
// Background: config.* mutates the master in-memory *config.Config and the
|
||||||
|
// on-disk config.json. A non-master tenant admin calling config.patch would
|
||||||
|
// corrupt master state + leak master config to other tenants. This guard keeps
|
||||||
|
// config.* strictly master-scoped until a tenant-aware refactor lands.
|
||||||
|
func (m *ConfigMethods) requireMasterScope(next gateway.MethodHandler) gateway.MethodHandler {
|
||||||
|
return func(ctx context.Context, client *gateway.Client, req *protocol.RequestFrame) {
|
||||||
|
if !isMasterScopeContext(ctx) {
|
||||||
|
locale := store.LocaleFromContext(ctx)
|
||||||
|
client.SendResponse(protocol.NewErrorResponse(
|
||||||
|
req.ID,
|
||||||
|
protocol.ErrUnauthorized,
|
||||||
|
i18n.T(locale, i18n.MsgConfigMasterScopeOnly),
|
||||||
|
))
|
||||||
|
return
|
||||||
|
}
|
||||||
|
next(ctx, client, req)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// isMasterScopeContext returns true when ctx should be treated as master-scope:
|
||||||
|
// (a) system owner role (bypass-all), or
|
||||||
|
// (b) tenant id is unset (uuid.Nil — legacy / system callers), or
|
||||||
|
// (c) tenant id equals store.MasterTenantID.
|
||||||
|
func isMasterScopeContext(ctx context.Context) bool {
|
||||||
|
if store.IsOwnerRole(ctx) {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
tid := store.TenantIDFromContext(ctx)
|
||||||
|
return tid == uuid.Nil || tid == store.MasterTenantID
|
||||||
|
}
|
||||||
|
|
||||||
func (m *ConfigMethods) handleGet(_ context.Context, client *gateway.Client, req *protocol.RequestFrame) {
|
func (m *ConfigMethods) handleGet(_ context.Context, client *gateway.Client, req *protocol.RequestFrame) {
|
||||||
client.SendResponse(protocol.NewOKResponse(req.ID, map[string]any{
|
client.SendResponse(protocol.NewOKResponse(req.ID, map[string]any{
|
||||||
"config": m.cfg.MaskedCopy(),
|
"config": m.cfg.MaskedCopy(),
|
||||||
|
|||||||
@@ -0,0 +1,157 @@
|
|||||||
|
package methods
|
||||||
|
|
||||||
|
import (
|
||||||
|
"context"
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"github.com/google/uuid"
|
||||||
|
|
||||||
|
"github.com/nextlevelbuilder/goclaw/internal/gateway"
|
||||||
|
"github.com/nextlevelbuilder/goclaw/internal/store"
|
||||||
|
"github.com/nextlevelbuilder/goclaw/pkg/protocol"
|
||||||
|
)
|
||||||
|
|
||||||
|
// ---- Tests: isMasterScopeContext helper ----
|
||||||
|
|
||||||
|
func TestIsMasterScopeContext_OwnerRole_Allowed(t *testing.T) {
|
||||||
|
ctx := store.WithRole(context.Background(), store.RoleOwner)
|
||||||
|
// Tenant set to a non-master tenant — owner role must still pass
|
||||||
|
ctx = store.WithTenantID(ctx, uuid.MustParse("11111111-1111-1111-1111-111111111111"))
|
||||||
|
if !isMasterScopeContext(ctx) {
|
||||||
|
t.Fatalf("owner role with non-master tenant should be allowed")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestIsMasterScopeContext_NilTenant_Allowed(t *testing.T) {
|
||||||
|
// Legacy / system callers without tenant scope → treated as master
|
||||||
|
ctx := context.Background()
|
||||||
|
if !isMasterScopeContext(ctx) {
|
||||||
|
t.Fatalf("nil tenant ctx should be allowed (master-scope fallback)")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestIsMasterScopeContext_MasterTenant_Allowed(t *testing.T) {
|
||||||
|
ctx := store.WithTenantID(context.Background(), store.MasterTenantID)
|
||||||
|
if !isMasterScopeContext(ctx) {
|
||||||
|
t.Fatalf("master tenant ctx should be allowed")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestIsMasterScopeContext_NonMasterTenantNoOwner_Denied(t *testing.T) {
|
||||||
|
tid := uuid.MustParse("22222222-2222-2222-2222-222222222222")
|
||||||
|
ctx := store.WithTenantID(context.Background(), tid)
|
||||||
|
if isMasterScopeContext(ctx) {
|
||||||
|
t.Fatalf("non-master tenant ctx without owner role must be denied")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestIsMasterScopeContext_NonMasterTenantWithOwnerRole_Allowed(t *testing.T) {
|
||||||
|
// A system owner visiting a tenant dashboard — bypass-all allows through
|
||||||
|
tid := uuid.MustParse("33333333-3333-3333-3333-333333333333")
|
||||||
|
ctx := store.WithTenantID(context.Background(), tid)
|
||||||
|
ctx = store.WithRole(ctx, store.RoleOwner)
|
||||||
|
if !isMasterScopeContext(ctx) {
|
||||||
|
t.Fatalf("owner role must bypass tenant scope check")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// ---- Tests: requireMasterScope middleware ----
|
||||||
|
|
||||||
|
func configGuardRequest(method string) *protocol.RequestFrame {
|
||||||
|
return &protocol.RequestFrame{
|
||||||
|
Type: protocol.FrameTypeRequest,
|
||||||
|
ID: "cfg-guard-req-1",
|
||||||
|
Method: method,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// nextCalledHandler returns a handler that flips *called to true when invoked.
|
||||||
|
func nextCalledHandler(called *bool) gateway.MethodHandler {
|
||||||
|
return func(_ context.Context, _ *gateway.Client, _ *protocol.RequestFrame) {
|
||||||
|
*called = true
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRequireMasterScope_MasterTenant_CallsNext(t *testing.T) {
|
||||||
|
m := &ConfigMethods{}
|
||||||
|
var called bool
|
||||||
|
h := m.requireMasterScope(nextCalledHandler(&called))
|
||||||
|
|
||||||
|
ctx := store.WithTenantID(context.Background(), store.MasterTenantID)
|
||||||
|
h(ctx, nullClient(), configGuardRequest(protocol.MethodConfigPatch))
|
||||||
|
|
||||||
|
if !called {
|
||||||
|
t.Fatalf("expected next handler to be called for master tenant ctx")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRequireMasterScope_NilTenant_CallsNext(t *testing.T) {
|
||||||
|
m := &ConfigMethods{}
|
||||||
|
var called bool
|
||||||
|
h := m.requireMasterScope(nextCalledHandler(&called))
|
||||||
|
|
||||||
|
h(context.Background(), nullClient(), configGuardRequest(protocol.MethodConfigGet))
|
||||||
|
|
||||||
|
if !called {
|
||||||
|
t.Fatalf("expected next handler to be called for nil tenant ctx (legacy compat)")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRequireMasterScope_OwnerRole_CallsNext(t *testing.T) {
|
||||||
|
m := &ConfigMethods{}
|
||||||
|
var called bool
|
||||||
|
h := m.requireMasterScope(nextCalledHandler(&called))
|
||||||
|
|
||||||
|
// System owner visiting a non-master tenant ctx — must pass
|
||||||
|
ctx := store.WithTenantID(context.Background(), uuid.MustParse("44444444-4444-4444-4444-444444444444"))
|
||||||
|
ctx = store.WithRole(ctx, store.RoleOwner)
|
||||||
|
h(ctx, nullClient(), configGuardRequest(protocol.MethodConfigApply))
|
||||||
|
|
||||||
|
if !called {
|
||||||
|
t.Fatalf("expected next handler to be called for owner role bypass")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRequireMasterScope_NonMasterTenantNoOwner_BlocksNext(t *testing.T) {
|
||||||
|
m := &ConfigMethods{}
|
||||||
|
var called bool
|
||||||
|
h := m.requireMasterScope(nextCalledHandler(&called))
|
||||||
|
|
||||||
|
// Non-master tenant admin (non-owner role) — must be blocked
|
||||||
|
ctx := store.WithTenantID(context.Background(), uuid.MustParse("55555555-5555-5555-5555-555555555555"))
|
||||||
|
ctx = store.WithRole(ctx, "admin")
|
||||||
|
h(ctx, nullClient(), configGuardRequest(protocol.MethodConfigPatch))
|
||||||
|
|
||||||
|
if called {
|
||||||
|
t.Fatalf("non-master tenant non-owner ctx must NOT reach next handler")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// ---- Test: middleware chain (requireMasterScope → requireOwner → handler) ----
|
||||||
|
|
||||||
|
// TestRequireMasterScope_ChainedWithRequireOwner asserts the master-scope guard
|
||||||
|
// fires BEFORE requireOwner (which uses client.IsOwner()). Non-master tenant ctx
|
||||||
|
// must be rejected even though nullClient has no owner role set — this protects
|
||||||
|
// the downstream handler from ever touching m.cfg for a non-master caller.
|
||||||
|
func TestRequireMasterScope_ChainedWithRequireOwner(t *testing.T) {
|
||||||
|
m := &ConfigMethods{}
|
||||||
|
var innerCalled bool
|
||||||
|
inner := nextCalledHandler(&innerCalled)
|
||||||
|
chain := m.requireMasterScope(m.requireOwner(inner))
|
||||||
|
|
||||||
|
// Non-master tenant ctx → master-scope guard rejects first
|
||||||
|
ctx := store.WithTenantID(context.Background(), uuid.MustParse("66666666-6666-6666-6666-666666666666"))
|
||||||
|
ctx = store.WithRole(ctx, "admin")
|
||||||
|
|
||||||
|
// Must not panic (handler with nil m.cfg is never reached)
|
||||||
|
defer func() {
|
||||||
|
if r := recover(); r != nil {
|
||||||
|
t.Fatalf("chained middleware panicked (guard did not fire early): %v", r)
|
||||||
|
}
|
||||||
|
}()
|
||||||
|
chain(ctx, nullClient(), configGuardRequest(protocol.MethodConfigPatch))
|
||||||
|
|
||||||
|
if innerCalled {
|
||||||
|
t.Fatalf("inner handler must not be reached when master-scope guard rejects")
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -105,8 +105,9 @@ func init() {
|
|||||||
MsgInvalidLogAction: "action must be 'start' or 'stop'",
|
MsgInvalidLogAction: "action must be 'start' or 'stop'",
|
||||||
|
|
||||||
// Config
|
// Config
|
||||||
MsgRawConfigRequired: "raw config is required",
|
MsgRawConfigRequired: "raw config is required",
|
||||||
MsgRawPatchRequired: "raw patch is required",
|
MsgRawPatchRequired: "raw patch is required",
|
||||||
|
MsgConfigMasterScopeOnly: "config.* methods are master-scope only; use tenant tool config endpoints for per-tenant overrides",
|
||||||
|
|
||||||
// Storage / File
|
// Storage / File
|
||||||
MsgCannotDeleteSkillsDir: "cannot delete skills directories",
|
MsgCannotDeleteSkillsDir: "cannot delete skills directories",
|
||||||
|
|||||||
@@ -105,8 +105,9 @@ func init() {
|
|||||||
MsgInvalidLogAction: "action phải là 'start' hoặc 'stop'",
|
MsgInvalidLogAction: "action phải là 'start' hoặc 'stop'",
|
||||||
|
|
||||||
// Config
|
// Config
|
||||||
MsgRawConfigRequired: "cấu hình raw là bắt buộc",
|
MsgRawConfigRequired: "cấu hình raw là bắt buộc",
|
||||||
MsgRawPatchRequired: "patch raw là bắt buộc",
|
MsgRawPatchRequired: "patch raw là bắt buộc",
|
||||||
|
MsgConfigMasterScopeOnly: "config.* chỉ áp dụng cho master scope; dùng endpoint tenant tool config cho override theo tenant",
|
||||||
|
|
||||||
// Storage / File
|
// Storage / File
|
||||||
MsgCannotDeleteSkillsDir: "không thể xóa thư mục skill",
|
MsgCannotDeleteSkillsDir: "không thể xóa thư mục skill",
|
||||||
|
|||||||
@@ -105,8 +105,9 @@ func init() {
|
|||||||
MsgInvalidLogAction: "action 必须是 'start' 或 'stop'",
|
MsgInvalidLogAction: "action 必须是 'start' 或 'stop'",
|
||||||
|
|
||||||
// Config
|
// Config
|
||||||
MsgRawConfigRequired: "raw 配置是必填项",
|
MsgRawConfigRequired: "raw 配置是必填项",
|
||||||
MsgRawPatchRequired: "raw 补丁是必填项",
|
MsgRawPatchRequired: "raw 补丁是必填项",
|
||||||
|
MsgConfigMasterScopeOnly: "config.* 方法仅适用于主作用域;使用租户工具配置端点进行租户级覆盖",
|
||||||
|
|
||||||
// Storage / File
|
// Storage / File
|
||||||
MsgCannotDeleteSkillsDir: "无法删除Skill目录",
|
MsgCannotDeleteSkillsDir: "无法删除Skill目录",
|
||||||
|
|||||||
@@ -106,8 +106,9 @@ const (
|
|||||||
MsgInvalidLogAction = "error.invalid_log_action" // "action must be 'start' or 'stop'"
|
MsgInvalidLogAction = "error.invalid_log_action" // "action must be 'start' or 'stop'"
|
||||||
|
|
||||||
// --- Config ---
|
// --- Config ---
|
||||||
MsgRawConfigRequired = "error.raw_config_required" // "raw config is required"
|
MsgRawConfigRequired = "error.raw_config_required" // "raw config is required"
|
||||||
MsgRawPatchRequired = "error.raw_patch_required" // "raw patch is required"
|
MsgRawPatchRequired = "error.raw_patch_required" // "raw patch is required"
|
||||||
|
MsgConfigMasterScopeOnly = "error.config_master_scope_only" // "config.* methods are master-scope only"
|
||||||
|
|
||||||
// --- Storage / File ---
|
// --- Storage / File ---
|
||||||
MsgCannotDeleteSkillsDir = "error.cannot_delete_skills_dir" // "cannot delete skills directories"
|
MsgCannotDeleteSkillsDir = "error.cannot_delete_skills_dir" // "cannot delete skills directories"
|
||||||
|
|||||||
Reference in new issue
Block a user