mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-14 02:15:57 +00:00
fix(skills): widen ListAccessible + ownership match to cover actor/user/sender identities
Prevents two silent regressions flagged by the #915 e2e audit: Flow Q — private skill invisible to creator in group: ListAccessible filtered s.owner_id = $2 (userID = group principal). After the ACTOR migration, new skills get owner_id = sender, so the publisher could not see their own private skill on the next group turn. Fixed by matching owner_id and skill_user_grants.user_id against both userID and ActorIDFromContext(ctx) in the query. The OR collapses in DM contexts where both resolve to the same value. Ownership check in skill_manage (patch + delete): Replaced the two-identity compare with isOwnerOfSkill helper that matches any of {actor, userID, senderID}. Covers (a) new rows with actor-based owner, (b) legacy rows with group-scope / tenant-merged owner, (c) rows from the short pre-merge-aware ActorID window where DM owners got the raw channel sender. A tenant-merged user reliably matches all three of their skills regardless of vintage.
This commit is contained in:
1 parent
3976cf1910
commit
2fcb9a9ee0
3 files changed
+59
-23
No files matched your search
@@ -133,16 +133,28 @@ func (s *PGSkillStore) RevokeFromUser(ctx context.Context, skillID uuid.UUID, us
|
||||
// ListAccessible returns skills accessible to a given agent+user combination.
|
||||
// Access logic: public → all, private → owner only, internal → check grants.
|
||||
// System skills (is_system=true) are always visible regardless of tenant.
|
||||
//
|
||||
// To preserve visibility across the ACTOR-vs-SCOPE split (#915), the query
|
||||
// matches owner_id and user grant rows against BOTH the caller-supplied
|
||||
// userID (scope identity, legacy rows) and the actor identity from ctx
|
||||
// (individual sender, new rows). In DM contexts these are identical and
|
||||
// the OR clause collapses; in group chats they diverge and the OR ensures
|
||||
// a publisher can still see their own private skill.
|
||||
func (s *PGSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, userID string) ([]store.SkillInfo, error) {
|
||||
actorID := store.ActorIDFromContext(ctx)
|
||||
if actorID == "" {
|
||||
actorID = userID
|
||||
}
|
||||
// tenant filter: system skills bypass it, tenant-owned skills are filtered
|
||||
tc, tcArgs, _, err := scopeClause(ctx, 3)
|
||||
// (bumped from $3 to $4 to accommodate the new actorID bind at $3).
|
||||
tc, tcArgs, _, err := scopeClause(ctx, 4)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
tenantCond := ""
|
||||
if tc != "" {
|
||||
// tc is " AND tenant_id = $3"; we need it as an OR condition inside the WHERE
|
||||
tenantCond = fmt.Sprintf(" AND (s.is_system = true OR s.tenant_id = $%d)", 3)
|
||||
// tc is " AND tenant_id = $4"; we need it as an OR condition inside the WHERE
|
||||
tenantCond = fmt.Sprintf(" AND (s.is_system = true OR s.tenant_id = $%d)", 4)
|
||||
_ = tc // tcArgs carries the value
|
||||
}
|
||||
// LEFT JOIN skill_tenant_configs to exclude per-tenant disabled skills.
|
||||
@@ -150,20 +162,20 @@ func (s *PGSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, us
|
||||
stcJoin := ""
|
||||
stcFilter := ""
|
||||
if len(tcArgs) > 0 {
|
||||
stcJoin = fmt.Sprintf(" LEFT JOIN skill_tenant_configs stc ON s.id = stc.skill_id AND stc.tenant_id = $%d", 3)
|
||||
stcJoin = fmt.Sprintf(" LEFT JOIN skill_tenant_configs stc ON s.id = stc.skill_id AND stc.tenant_id = $%d", 4)
|
||||
stcFilter = " AND (stc.enabled IS NULL OR stc.enabled = true)"
|
||||
}
|
||||
rows, err := s.db.QueryContext(ctx,
|
||||
`SELECT DISTINCT s.name, s.slug, s.description, s.version, s.file_path FROM skills s
|
||||
LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = $1
|
||||
LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND sug.user_id = $2`+stcJoin+`
|
||||
LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND (sug.user_id = $2 OR sug.user_id = $3)`+stcJoin+`
|
||||
WHERE s.status = 'active'`+tenantCond+stcFilter+` AND (
|
||||
s.is_system = true
|
||||
OR s.visibility = 'public'
|
||||
OR (s.visibility = 'private' AND s.owner_id = $2)
|
||||
OR (s.visibility = 'private' AND (s.owner_id = $2 OR s.owner_id = $3))
|
||||
OR (s.visibility = 'internal' AND (sag.id IS NOT NULL OR sug.id IS NOT NULL))
|
||||
)
|
||||
ORDER BY s.name`, append([]any{agentID, userID}, tcArgs...)...)
|
||||
ORDER BY s.name`, append([]any{agentID, userID, actorID}, tcArgs...)...)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@@ -133,7 +133,12 @@ func (s *SQLiteSkillStore) RevokeFromUser(ctx context.Context, skillID uuid.UUID
|
||||
}
|
||||
|
||||
// ListAccessible returns skills accessible to a given agent+user combination.
|
||||
// See PGSkillStore.ListAccessible for the ACTOR-vs-SCOPE dual-match rationale.
|
||||
func (s *SQLiteSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID, userID string) ([]store.SkillInfo, error) {
|
||||
actorID := store.ActorIDFromContext(ctx)
|
||||
if actorID == "" {
|
||||
actorID = userID
|
||||
}
|
||||
tClause, tArgs, err := scopeClauseAlias(ctx, "s")
|
||||
if err != nil {
|
||||
return nil, err
|
||||
@@ -147,8 +152,8 @@ func (s *SQLiteSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID
|
||||
stcFilter = " AND (stc.enabled IS NULL OR stc.enabled = 1)"
|
||||
}
|
||||
|
||||
// Build args: agentID, userID, [tenantID for tenant cond, tenantID for stc join]
|
||||
queryArgs := []any{agentID, userID}
|
||||
// Positional args: agentID, userID, actorID, [tenantID x2 if tenant-scoped], userID, actorID (private-owner clause)
|
||||
queryArgs := []any{agentID, userID, actorID}
|
||||
if len(tArgs) > 0 {
|
||||
queryArgs = append(queryArgs, tArgs...) // tenant cond
|
||||
queryArgs = append(queryArgs, tArgs...) // stc join
|
||||
@@ -159,15 +164,15 @@ func (s *SQLiteSkillStore) ListAccessible(ctx context.Context, agentID uuid.UUID
|
||||
rows, err := s.db.QueryContext(ctx,
|
||||
`SELECT DISTINCT s.name, s.slug, s.description, s.version, s.file_path FROM skills s
|
||||
LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = ?
|
||||
LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND sug.user_id = ?`+stcJoin+`
|
||||
LEFT JOIN skill_user_grants sug ON s.id = sug.skill_id AND (sug.user_id = ? OR sug.user_id = ?)`+stcJoin+`
|
||||
WHERE s.status = 'active'`+tenantCond+stcFilter+` AND (
|
||||
s.is_system = 1
|
||||
OR s.visibility = 'public'
|
||||
OR (s.visibility = 'private' AND s.owner_id = ?)
|
||||
OR (s.visibility = 'private' AND (s.owner_id = ? OR s.owner_id = ?))
|
||||
OR (s.visibility = 'internal' AND (sag.id IS NOT NULL OR sug.id IS NOT NULL))
|
||||
)
|
||||
ORDER BY s.name`,
|
||||
append(queryArgs, userID)...,
|
||||
append(queryArgs, userID, actorID)...,
|
||||
)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
|
||||
@@ -32,6 +32,25 @@ func NewSkillManageTool(skills store.SkillManageStore, baseDir, dataDir string,
|
||||
return &SkillManageTool{skills: skills, base: baseDir, dataDir: dataDir, loader: loader}
|
||||
}
|
||||
|
||||
// isOwnerOfSkill returns true if the caller owns the skill identified by slug.
|
||||
// Matches owner_id against three identities for backward compatibility (#915):
|
||||
// - ActorIDFromContext: current helper, merge-aware in DM, sender in groups
|
||||
// - UserIDFromContext: legacy rows pre-#915 (group-scope) and merged tenant id
|
||||
// - SenderIDFromContext: raw channel sender (covers the pre-merge-aware ActorID window)
|
||||
//
|
||||
// If the slug does not exist, returns true (caller sees "skill not found" error
|
||||
// from the downstream resolver — preserves the existing error surface).
|
||||
func isOwnerOfSkill(ctx context.Context, skills store.SkillManageStore, slug string) bool {
|
||||
ownerID, found := skills.GetSkillOwnerIDBySlug(ctx, slug)
|
||||
if !found {
|
||||
return true
|
||||
}
|
||||
actorID := store.ActorIDFromContext(ctx)
|
||||
userID := store.UserIDFromContext(ctx)
|
||||
senderID := store.SenderIDFromContext(ctx)
|
||||
return ownerID == actorID || ownerID == userID || ownerID == senderID
|
||||
}
|
||||
|
||||
// tenantSkillsDir returns the skills-store directory scoped to the calling agent's tenant.
|
||||
func (t *SkillManageTool) tenantSkillsDir(ctx context.Context) string {
|
||||
tid := store.TenantIDFromContext(ctx)
|
||||
@@ -235,13 +254,15 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any)
|
||||
}
|
||||
|
||||
// Ownership check: only the skill owner can patch.
|
||||
// Accept actor (new post-#915 format) or user-scope (legacy pre-#915 rows
|
||||
// where owner_id was set to the group principal). Legacy fallback lets
|
||||
// any group member manage skills created before the ACTOR migration;
|
||||
// they can re-publish under their individual identity to tighten ownership.
|
||||
actorID := store.ActorIDFromContext(ctx)
|
||||
legacyID := store.UserIDFromContext(ctx)
|
||||
if ownerID, found := t.skills.GetSkillOwnerIDBySlug(ctx, slug); found && ownerID != actorID && ownerID != legacyID {
|
||||
// Accept any of three identities the same human maps to:
|
||||
// - actor (current merge-aware helper — preferred for new rows)
|
||||
// - userID (legacy pre-#915 rows where owner_id was group principal
|
||||
// or the merged tenant identity)
|
||||
// - senderID (legacy rows from the pre-merge-aware ActorID window
|
||||
// where DM owners got the raw channel sender)
|
||||
// A DM user merged to "viettx" with Telegram ID "386246614" matches all
|
||||
// three of their skills regardless of when they were created.
|
||||
if !isOwnerOfSkill(ctx, t.skills, slug) {
|
||||
return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug))
|
||||
}
|
||||
|
||||
@@ -330,10 +351,8 @@ func (t *SkillManageTool) executeDelete(ctx context.Context, args map[string]any
|
||||
}
|
||||
|
||||
// Ownership check: only the skill owner can delete.
|
||||
// Same legacy fallback pattern as patch-flow above (#915).
|
||||
deleteActorID := store.ActorIDFromContext(ctx)
|
||||
deleteLegacyID := store.UserIDFromContext(ctx)
|
||||
if ownerID, found := t.skills.GetSkillOwnerIDBySlug(ctx, slug); found && ownerID != deleteActorID && ownerID != deleteLegacyID {
|
||||
// Same three-identity match as the patch flow above (#915).
|
||||
if !isOwnerOfSkill(ctx, t.skills, slug) {
|
||||
return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug))
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user