mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-11 03:13:24 +00:00
refactor(permissions): remove auto-add file writer, add config type constants
- Remove auto-add logic that granted file_writer permission to the first group/guild member who chatted with the bot - Add ConfigTypeFileWriter and ConfigTypeHeartbeat constants, replace all hardcoded config_type strings across callers - Add bootstrap exception: /addwriter and !addwriter allow first writer to be added when no writers exist yet - Optimize writer commands: reuse cached ListFileWriters result for both permission check and last-writer guard, reducing DB queries per command - Add freshness directive to file writer system prompt so bot prioritizes current list over stale references in conversation history
This commit is contained in:
1 parent
f4369c51e4
commit
39bb90bd60
10 files changed
+70
-80
No files matched your search
@@ -2,9 +2,6 @@ package cmd
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"log/slog"
|
||||
"strings"
|
||||
|
||||
"github.com/google/uuid"
|
||||
|
||||
@@ -17,38 +14,13 @@ import (
|
||||
// buildEnsureUserProfile creates the user profile resolution callback.
|
||||
// Creates/resolves user profile and returns effective workspace.
|
||||
// Separated from seeding to allow independent lifecycle management.
|
||||
func buildEnsureUserProfile(as store.AgentStore, configPermStore store.ConfigPermissionStore) agent.EnsureUserProfileFunc {
|
||||
func buildEnsureUserProfile(as store.AgentStore) agent.EnsureUserProfileFunc {
|
||||
return func(ctx context.Context, agentID uuid.UUID, userID, workspace, channel string) (string, bool, error) {
|
||||
isNew, effectiveWs, err := as.GetOrCreateUserProfile(ctx, agentID, userID, workspace, channel)
|
||||
if err != nil {
|
||||
return effectiveWs, false, err
|
||||
}
|
||||
|
||||
// Auto-add first group member as a file writer (bootstrap the allowlist).
|
||||
// Only needed for truly new profiles — existing groups already have their writers.
|
||||
if isNew && configPermStore != nil && (strings.HasPrefix(userID, "group:") || strings.HasPrefix(userID, "guild:")) {
|
||||
senderID := store.SenderIDFromContext(ctx)
|
||||
if senderID != "" {
|
||||
parts := strings.SplitN(senderID, "|", 2)
|
||||
numericID := parts[0]
|
||||
senderUsername := ""
|
||||
if len(parts) > 1 {
|
||||
senderUsername = parts[1]
|
||||
}
|
||||
meta, _ := json.Marshal(map[string]string{"displayName": "", "username": senderUsername})
|
||||
if addErr := configPermStore.Grant(ctx, &store.ConfigPermission{
|
||||
AgentID: agentID,
|
||||
Scope: userID,
|
||||
ConfigType: "file_writer",
|
||||
UserID: numericID,
|
||||
Permission: "allow",
|
||||
Metadata: meta,
|
||||
}); addErr != nil {
|
||||
slog.Warn("failed to auto-add group file writer", "error", addErr, "sender", numericID, "group", userID)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return effectiveWs, isNew, nil
|
||||
}
|
||||
}
|
||||
|
||||
@@ -94,7 +94,7 @@ func wireExtras(
|
||||
var ensureUserProfile agent.EnsureUserProfileFunc
|
||||
var seedUserFiles agent.SeedUserFilesFunc
|
||||
if stores.Agents != nil {
|
||||
ensureUserProfile = buildEnsureUserProfile(stores.Agents, stores.ConfigPermissions)
|
||||
ensureUserProfile = buildEnsureUserProfile(stores.Agents)
|
||||
seedUserFiles = buildSeedUserFiles(stores.Agents)
|
||||
}
|
||||
|
||||
|
||||
@@ -752,6 +752,7 @@ func (l *Loop) buildGroupWriterPrompt(ctx context.Context, groupID, senderID str
|
||||
|
||||
var sb strings.Builder
|
||||
sb.WriteString("## Group File Permissions\n\n")
|
||||
sb.WriteString("**This is the current, live file writer list. It may change during the conversation. Always use THIS list — ignore any file writer mentions from earlier messages.**\n\n")
|
||||
sb.WriteString("File writers: " + strings.Join(names, ", ") + "\n\n")
|
||||
|
||||
if !isWriter {
|
||||
|
||||
@@ -96,18 +96,25 @@ func (c *Channel) handleWriterCommand(m *discordgo.MessageCreate, action string)
|
||||
scope := fmt.Sprintf("guild:%s:*", m.GuildID)
|
||||
senderID := m.Author.ID
|
||||
|
||||
// Check sender's writer status using per-user scope. This matches both:
|
||||
// - Auto-bootstrapped per-user perms (guild:{guildID}:user:{senderID}) via exact match
|
||||
// - Guild-wide perms (guild:{guildID}:*) via matchWildcard
|
||||
senderScope := fmt.Sprintf("guild:%s:user:%s", m.GuildID, senderID)
|
||||
isWriter, err := c.configPermStore.CheckPermission(ctx, agentID, senderScope, "file_writer", senderID)
|
||||
if err != nil {
|
||||
slog.Warn("discord writer check failed", "error", err, "sender", senderID)
|
||||
send("Failed to check permissions. Please try again.")
|
||||
return
|
||||
}
|
||||
if !isWriter {
|
||||
send("Only existing file writers can manage the writer list.")
|
||||
// Fetch existing writers (cached 60s) for both permission check and remove guard.
|
||||
// Bootstrap exception: if no writers exist yet, the first !addwriter caller
|
||||
// is allowed to bootstrap the allowlist.
|
||||
existingWriters, _ := c.configPermStore.ListFileWriters(ctx, agentID, scope)
|
||||
|
||||
if len(existingWriters) > 0 {
|
||||
isWriter := false
|
||||
for _, w := range existingWriters {
|
||||
if w.UserID == senderID {
|
||||
isWriter = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if !isWriter {
|
||||
send("Only existing file writers can manage the writer list.")
|
||||
return
|
||||
}
|
||||
} else if action == "remove" {
|
||||
send("No file writers configured yet. Use !addwriter to add the first one.")
|
||||
return
|
||||
}
|
||||
|
||||
@@ -146,7 +153,7 @@ func (c *Channel) handleWriterCommand(m *discordgo.MessageCreate, action string)
|
||||
if err := c.configPermStore.Grant(ctx, &store.ConfigPermission{
|
||||
AgentID: agentID,
|
||||
Scope: scope,
|
||||
ConfigType: "file_writer",
|
||||
ConfigType: store.ConfigTypeFileWriter,
|
||||
UserID: targetID,
|
||||
Permission: "allow",
|
||||
Metadata: meta,
|
||||
@@ -158,25 +165,20 @@ func (c *Channel) handleWriterCommand(m *discordgo.MessageCreate, action string)
|
||||
send(fmt.Sprintf("Added %s as a file writer.", targetName))
|
||||
|
||||
case "remove":
|
||||
writers, listErr := c.configPermStore.List(ctx, agentID, "file_writer", scope)
|
||||
if listErr != nil {
|
||||
slog.Warn("discord list writers for remove failed", "error", listErr)
|
||||
send("Failed to check writers. Please try again.")
|
||||
return
|
||||
}
|
||||
if len(writers) <= 1 {
|
||||
// Prevent removing the last writer (reuse cached existingWriters)
|
||||
if len(existingWriters) <= 1 {
|
||||
send("Cannot remove the last file writer.")
|
||||
return
|
||||
}
|
||||
// Revoke guild-wide permission.
|
||||
if err := c.configPermStore.Revoke(ctx, agentID, scope, "file_writer", targetID); err != nil {
|
||||
if err := c.configPermStore.Revoke(ctx, agentID, scope, store.ConfigTypeFileWriter, targetID); err != nil {
|
||||
slog.Warn("discord remove writer failed", "error", err, "target", targetID)
|
||||
send("Failed to remove writer. Please try again.")
|
||||
return
|
||||
}
|
||||
// Also revoke auto-bootstrapped per-user permission (guild:{guildID}:user:{userID}).
|
||||
// Also revoke per-user scoped permission if it exists (guild:{guildID}:user:{userID}).
|
||||
perUserScope := fmt.Sprintf("guild:%s:user:%s", m.GuildID, targetID)
|
||||
_ = c.configPermStore.Revoke(ctx, agentID, perUserScope, "file_writer", targetID)
|
||||
_ = c.configPermStore.Revoke(ctx, agentID, perUserScope, store.ConfigTypeFileWriter, targetID)
|
||||
send(fmt.Sprintf("Removed %s from file writers.", targetName))
|
||||
}
|
||||
}
|
||||
@@ -212,7 +214,7 @@ func (c *Channel) handleListWriters(m *discordgo.MessageCreate) {
|
||||
// via matchWildcard in CheckPermission.
|
||||
scope := fmt.Sprintf("guild:%s:*", m.GuildID)
|
||||
|
||||
writers, err := c.configPermStore.List(ctx, agentID, "file_writer", scope)
|
||||
writers, err := c.configPermStore.List(ctx, agentID, store.ConfigTypeFileWriter, scope)
|
||||
if err != nil {
|
||||
slog.Warn("discord list writers failed", "error", err)
|
||||
send("Failed to list writers. Please try again.")
|
||||
@@ -220,7 +222,7 @@ func (c *Channel) handleListWriters(m *discordgo.MessageCreate) {
|
||||
}
|
||||
|
||||
if len(writers) == 0 {
|
||||
send("No file writers configured for this server. The first person to interact with the bot will be added automatically.")
|
||||
send("No file writers configured for this server. Use !addwriter to add one.")
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
@@ -105,7 +105,7 @@ func (c *Channel) handleBotCommand(ctx context.Context, message *telego.Message,
|
||||
if err == nil {
|
||||
groupID := fmt.Sprintf("group:%s:%s", c.Name(), chatIDStr)
|
||||
senderNumericID := strings.SplitN(senderID, "|", 2)[0]
|
||||
isWriter, err := c.configPermStore.CheckPermission(ctx, agentID, groupID, "file_writer", senderNumericID)
|
||||
isWriter, err := c.configPermStore.CheckPermission(ctx, agentID, groupID, store.ConfigTypeFileWriter, senderNumericID)
|
||||
if err != nil {
|
||||
slog.Warn("security.reset_writer_check_failed", "error", err, "sender", senderNumericID)
|
||||
// fail-open: allow reset if DB check fails
|
||||
|
||||
@@ -44,15 +44,25 @@ func (c *Channel) handleWriterCommand(ctx context.Context, message *telego.Messa
|
||||
groupID := fmt.Sprintf("group:%s:%s", c.Name(), chatIDStr)
|
||||
senderNumericID := strings.SplitN(senderID, "|", 2)[0]
|
||||
|
||||
// Check if sender is an existing writer (only writers can manage the list)
|
||||
isWriter, err := c.configPermStore.CheckPermission(ctx, agentID, groupID, "file_writer", senderNumericID)
|
||||
if err != nil {
|
||||
slog.Warn("writer check failed", "error", err, "sender", senderNumericID)
|
||||
send("Failed to check permissions. Please try again.")
|
||||
return
|
||||
}
|
||||
if !isWriter {
|
||||
send("Only existing file writers can manage the writer list.")
|
||||
// Fetch existing writers (cached 60s) for both permission check and remove guard.
|
||||
// Bootstrap exception: if no writers exist yet, the first /addwriter caller
|
||||
// is allowed to bootstrap the allowlist.
|
||||
existingWriters, _ := c.configPermStore.ListFileWriters(ctx, agentID, groupID)
|
||||
|
||||
if len(existingWriters) > 0 {
|
||||
isWriter := false
|
||||
for _, w := range existingWriters {
|
||||
if w.UserID == senderNumericID {
|
||||
isWriter = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if !isWriter {
|
||||
send("Only existing file writers can manage the writer list.")
|
||||
return
|
||||
}
|
||||
} else if action == "remove" {
|
||||
send("No file writers configured yet. Use /addwriter to add the first one.")
|
||||
return
|
||||
}
|
||||
|
||||
@@ -79,7 +89,7 @@ func (c *Channel) handleWriterCommand(ctx context.Context, message *telego.Messa
|
||||
if err := c.configPermStore.Grant(ctx, &store.ConfigPermission{
|
||||
AgentID: agentID,
|
||||
Scope: groupID,
|
||||
ConfigType: "file_writer",
|
||||
ConfigType: store.ConfigTypeFileWriter,
|
||||
UserID: targetID,
|
||||
Permission: "allow",
|
||||
Metadata: meta,
|
||||
@@ -91,13 +101,12 @@ func (c *Channel) handleWriterCommand(ctx context.Context, message *telego.Messa
|
||||
send(fmt.Sprintf("Added %s as a file writer.", targetName))
|
||||
|
||||
case "remove":
|
||||
// Prevent removing the last writer
|
||||
writers, _ := c.configPermStore.List(ctx, agentID, "file_writer", groupID)
|
||||
if len(writers) <= 1 {
|
||||
// Prevent removing the last writer (reuse cached existingWriters)
|
||||
if len(existingWriters) <= 1 {
|
||||
send("Cannot remove the last file writer.")
|
||||
return
|
||||
}
|
||||
if err := c.configPermStore.Revoke(ctx, agentID, groupID, "file_writer", targetID); err != nil {
|
||||
if err := c.configPermStore.Revoke(ctx, agentID, groupID, store.ConfigTypeFileWriter, targetID); err != nil {
|
||||
slog.Warn("remove writer failed", "error", err, "target", targetID)
|
||||
send("Failed to remove writer. Please try again.")
|
||||
return
|
||||
@@ -135,7 +144,7 @@ func (c *Channel) handleListWriters(ctx context.Context, chatID int64, chatIDStr
|
||||
|
||||
groupID := fmt.Sprintf("group:%s:%s", c.Name(), chatIDStr)
|
||||
|
||||
writers, err := c.configPermStore.List(ctx, agentID, "file_writer", groupID)
|
||||
writers, err := c.configPermStore.List(ctx, agentID, store.ConfigTypeFileWriter, groupID)
|
||||
if err != nil {
|
||||
slog.Warn("list writers failed", "error", err)
|
||||
send("Failed to list writers. Please try again.")
|
||||
@@ -143,7 +152,7 @@ func (c *Channel) handleListWriters(ctx context.Context, chatID int64, chatIDStr
|
||||
}
|
||||
|
||||
if len(writers) == 0 {
|
||||
send("No file writers configured for this group. The first person to interact with the bot will be added automatically.")
|
||||
send("No file writers configured for this group. Use /addwriter to add one.")
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
@@ -321,7 +321,7 @@ func (h *ChannelInstancesHandler) handleWriterGroups(w http.ResponseWriter, r *h
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
perms, err := h.configPermStore.List(r.Context(), agentID, "file_writer", "")
|
||||
perms, err := h.configPermStore.List(r.Context(), agentID, store.ConfigTypeFileWriter, "")
|
||||
if err != nil {
|
||||
slog.Error("channel_instances.writer_groups", "error", err)
|
||||
locale := store.LocaleFromContext(r.Context())
|
||||
@@ -357,7 +357,7 @@ func (h *ChannelInstancesHandler) handleListWriters(w http.ResponseWriter, r *ht
|
||||
writeError(w, http.StatusBadRequest, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgRequired, "group_id"))
|
||||
return
|
||||
}
|
||||
perms, err := h.configPermStore.List(r.Context(), agentID, "file_writer", groupID)
|
||||
perms, err := h.configPermStore.List(r.Context(), agentID, store.ConfigTypeFileWriter, groupID)
|
||||
if err != nil {
|
||||
slog.Error("channel_instances.list_writers", "error", err)
|
||||
writeError(w, http.StatusInternalServerError, protocol.ErrInternal, i18n.T(locale, i18n.MsgFailedToList, "writers"))
|
||||
@@ -415,7 +415,7 @@ func (h *ChannelInstancesHandler) handleAddWriter(w http.ResponseWriter, r *http
|
||||
if err := h.configPermStore.Grant(r.Context(), &store.ConfigPermission{
|
||||
AgentID: agentID,
|
||||
Scope: body.GroupID,
|
||||
ConfigType: "file_writer",
|
||||
ConfigType: store.ConfigTypeFileWriter,
|
||||
UserID: body.UserID,
|
||||
Permission: "allow",
|
||||
Metadata: meta,
|
||||
@@ -440,7 +440,7 @@ func (h *ChannelInstancesHandler) handleRemoveWriter(w http.ResponseWriter, r *h
|
||||
return
|
||||
}
|
||||
// Prevent removing the last writer (same guard as Telegram /removewriter)
|
||||
writers, _ := h.configPermStore.List(r.Context(), agentID, "file_writer", groupID)
|
||||
writers, _ := h.configPermStore.List(r.Context(), agentID, store.ConfigTypeFileWriter, groupID)
|
||||
allowCount := 0
|
||||
for _, p := range writers {
|
||||
if p.Permission == "allow" {
|
||||
@@ -451,7 +451,7 @@ func (h *ChannelInstancesHandler) handleRemoveWriter(w http.ResponseWriter, r *h
|
||||
writeError(w, http.StatusConflict, protocol.ErrFailedPrecondition, i18n.T(locale, i18n.MsgCannotRemoveLastWriter))
|
||||
return
|
||||
}
|
||||
if err := h.configPermStore.Revoke(r.Context(), agentID, groupID, "file_writer", userID); err != nil {
|
||||
if err := h.configPermStore.Revoke(r.Context(), agentID, groupID, store.ConfigTypeFileWriter, userID); err != nil {
|
||||
slog.Error("channel_instances.remove_writer", "error", err)
|
||||
writeError(w, http.StatusInternalServerError, protocol.ErrInternal, i18n.T(locale, i18n.MsgFailedToDelete, "writer", "internal error"))
|
||||
return
|
||||
|
||||
@@ -10,6 +10,12 @@ import (
|
||||
"github.com/google/uuid"
|
||||
)
|
||||
|
||||
// Config type constants for agent_config_permissions.config_type column.
|
||||
const (
|
||||
ConfigTypeFileWriter = "file_writer" // Group file write access
|
||||
ConfigTypeHeartbeat = "heartbeat" // Heartbeat config access
|
||||
)
|
||||
|
||||
// ConfigPermission represents an allow/deny rule for agent configuration.
|
||||
type ConfigPermission struct {
|
||||
ID uuid.UUID `json:"id"`
|
||||
@@ -59,7 +65,7 @@ func CheckFileWriterPermission(ctx context.Context, permStore ConfigPermissionSt
|
||||
return nil // system context (cron, subagent)
|
||||
}
|
||||
numericID := strings.SplitN(senderID, "|", 2)[0]
|
||||
allowed, err := permStore.CheckPermission(ctx, agentID, userID, "file_writer", numericID)
|
||||
allowed, err := permStore.CheckPermission(ctx, agentID, userID, ConfigTypeFileWriter, numericID)
|
||||
if err != nil {
|
||||
return nil // fail-open
|
||||
}
|
||||
|
||||
@@ -212,7 +212,7 @@ func (b *ContextFileInterceptor) WriteFile(ctx context.Context, path, content st
|
||||
senderID := store.SenderIDFromContext(ctx)
|
||||
if senderID != "" && b.permStore != nil {
|
||||
numericID := strings.SplitN(senderID, "|", 2)[0]
|
||||
allowed, err := b.permStore.CheckPermission(ctx, agentID, userID, "file_writer", numericID)
|
||||
allowed, err := b.permStore.CheckPermission(ctx, agentID, userID, store.ConfigTypeFileWriter, numericID)
|
||||
if err != nil {
|
||||
slog.Warn("security.group_file_writer_check_failed",
|
||||
"error", err, "sender", numericID, "file", fileName, "group", userID)
|
||||
|
||||
@@ -285,7 +285,7 @@ func (t *HeartbeatTool) checkPermission(ctx context.Context, agentID uuid.UUID)
|
||||
}
|
||||
}
|
||||
|
||||
allowed, err := t.permStore.CheckPermission(ctx, agentID, scope, "heartbeat", numericID)
|
||||
allowed, err := t.permStore.CheckPermission(ctx, agentID, scope, store.ConfigTypeHeartbeat, numericID)
|
||||
if err != nil {
|
||||
return fmt.Errorf("permission check failed: %w", err)
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user