From a0576549625232fda33bbeea95be0aa4ee3880eb Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Mon, 29 Aug 2022 10:25:17 +0300 Subject: [PATCH] Put command access checks on the command level Checking using `settings.Allowed` is odd. Not all commands are related to setting configuration settings. Admin commands are coming in the future, for which this is certainly not the case. We now do access checks early on (during command processing), so command handlers can be clean of access checks. If we're inside of a command handler, the user is privileged to run it. --- bot/access.go | 37 ++++++++++ bot/bot.go | 7 +- bot/command.go | 185 ++++++++++++++++++++++++++---------------------- bot/settings.go | 18 ----- bot/sync.go | 2 +- 5 files changed, 146 insertions(+), 103 deletions(-) create mode 100644 bot/access.go diff --git a/bot/access.go b/bot/access.go new file mode 100644 index 0000000..bbf48a3 --- /dev/null +++ b/bot/access.go @@ -0,0 +1,37 @@ +package bot + +import ( + "fmt" + + "maunium.net/go/mautrix/id" + + "gitlab.com/etke.cc/postmoogle/utils" +) + +type accessCheckerFunc func(id.UserID, id.RoomID) (bool, error) + +func (b *Bot) allowAnyone(actorID id.UserID, targetRoomID id.RoomID) (bool, error) { + return true, nil +} + +func (b *Bot) allowOwner(actorID id.UserID, targetRoomID id.RoomID) (bool, error) { + if !utils.Match(actorID.String(), b.allowedUsers) { + return false, nil + } + + if b.noowner { + return true, nil + } + + cfg, err := b.getSettings(targetRoomID) + if err != nil { + return false, fmt.Errorf("failed to retrieve settings: %v", err) + } + + owner := cfg.Owner() + if owner == "" { + return true, nil + } + + return owner == actorID.String(), nil +} diff --git a/bot/bot.go b/bot/bot.go index 3c8a662..bcb7cb1 100644 --- a/bot/bot.go +++ b/bot/bot.go @@ -22,6 +22,7 @@ type Bot struct { domain string allowedUsers []*regexp.Regexp allowedAdmins []*regexp.Regexp + commands commandList rooms sync.Map log *logger.Logger lp *linkpearl.Linkpearl @@ -38,7 +39,7 @@ func New( allowedUsers []*regexp.Regexp, allowedAdmins []*regexp.Regexp, ) *Bot { - return &Bot{ + b := &Bot{ noowner: noowner, federation: federation, prefix: prefix, @@ -50,6 +51,10 @@ func New( lp: lp, mu: map[id.RoomID]*sync.Mutex{}, } + + b.commands = b.buildCommandList() + + return b } // Error message to the log and matrix room diff --git a/bot/command.go b/bot/command.go index 2fb754c..58a3cbe 100644 --- a/bot/command.go +++ b/bot/command.go @@ -11,11 +11,18 @@ import ( "gitlab.com/etke.cc/postmoogle/utils" ) +const ( + commandHelp = "help" + commandStop = "stop" + commandMailboxes = "mailboxes" +) + type ( command struct { - key string - description string - sanitizer func(string) string + key string + description string + sanitizer func(string) string + accessChecker accessCheckerFunc } commandList []command ) @@ -29,73 +36,85 @@ func (c commandList) get(key string) *command { return nil } -var commands = commandList{ - // special commands - { - key: "help", - description: "Show this help message", - }, - { - key: "stop", - description: "Disable bridge for the room and clear all configuration", - }, - {}, // delimiter - // options commands - { - key: optionMailbox, - description: "Get or set mailbox of the room", - sanitizer: utils.Mailbox, - }, - { - key: optionOwner, - description: "Get or set owner of the room", - sanitizer: func(s string) string { return s }, - }, - {}, // delimiter - { - key: optionNoSender, - description: fmt.Sprintf( - "Get or set `%s` of the room (`true` - hide email sender; `false` - show email sender)", - optionNoSender, - ), - sanitizer: utils.SanitizeBoolString, - }, - { - key: optionNoSubject, - description: fmt.Sprintf( - "Get or set `%s` of the room (`true` - hide email subject; `false` - show email subject)", - optionNoSubject, - ), - sanitizer: utils.SanitizeBoolString, - }, - { - key: optionNoHTML, - description: fmt.Sprintf( - "Get or set `%s` of the room (`true` - ignore HTML in email; `false` - parse HTML in emails)", - optionNoHTML, - ), - sanitizer: utils.SanitizeBoolString, - }, - { - key: optionNoThreads, - description: fmt.Sprintf( - "Get or set `%s` of the room (`true` - ignore email threads; `false` - convert email threads into matrix threads)", - optionNoThreads, - ), - sanitizer: utils.SanitizeBoolString, - }, - { - key: optionNoFiles, - description: fmt.Sprintf( - "Get or set `%s` of the room (`true` - ignore email attachments; `false` - upload email attachments)", - optionNoFiles, - ), - sanitizer: utils.SanitizeBoolString, - }, +func (b *Bot) buildCommandList() commandList { + return commandList{ + // special commands + { + key: commandHelp, + description: "Show this help message", + accessChecker: b.allowAnyone, + }, + { + key: commandStop, + description: "Disable bridge for the room and clear all configuration", + accessChecker: b.allowOwner, + }, + {}, // delimiter + // options commands + { + key: optionMailbox, + description: "Get or set mailbox of the room", + sanitizer: utils.Mailbox, + accessChecker: b.allowOwner, + }, + { + key: optionOwner, + description: "Get or set owner of the room", + sanitizer: func(s string) string { return s }, + accessChecker: b.allowOwner, + }, + {}, // delimiter + { + key: optionNoSender, + description: fmt.Sprintf( + "Get or set `%s` of the room (`true` - hide email sender; `false` - show email sender)", + optionNoSender, + ), + sanitizer: utils.SanitizeBoolString, + accessChecker: b.allowOwner, + }, + { + key: optionNoSubject, + description: fmt.Sprintf( + "Get or set `%s` of the room (`true` - hide email subject; `false` - show email subject)", + optionNoSubject, + ), + sanitizer: utils.SanitizeBoolString, + accessChecker: b.allowOwner, + }, + { + key: optionNoHTML, + description: fmt.Sprintf( + "Get or set `%s` of the room (`true` - ignore HTML in email; `false` - parse HTML in emails)", + optionNoHTML, + ), + sanitizer: utils.SanitizeBoolString, + accessChecker: b.allowOwner, + }, + { + key: optionNoThreads, + description: fmt.Sprintf( + "Get or set `%s` of the room (`true` - ignore email threads; `false` - convert email threads into matrix threads)", + optionNoThreads, + ), + sanitizer: utils.SanitizeBoolString, + accessChecker: b.allowOwner, + }, + { + key: optionNoFiles, + description: fmt.Sprintf( + "Get or set `%s` of the room (`true` - ignore email attachments; `false` - upload email attachments)", + optionNoFiles, + ), + sanitizer: utils.SanitizeBoolString, + accessChecker: b.allowOwner, + }, + } } func (b *Bot) handleCommand(ctx context.Context, evt *event.Event, commandSlice []string) { - if cmd := commands.get(commandSlice[0]); cmd == nil { + cmd := b.commands.get(commandSlice[0]) + if cmd == nil { return } @@ -104,11 +123,21 @@ func (b *Bot) handleCommand(ctx context.Context, evt *event.Event, commandSlice return } + allowed, err := cmd.accessChecker(evt.Sender, evt.RoomID) + if err != nil { + b.Error(ctx, evt.RoomID, err.Error()) + return + } + if !allowed { + b.Notice(ctx, evt.RoomID, "not allowed to do that, kupo") + return + } + switch commandSlice[0] { - case "help": + case commandHelp: b.sendHelp(ctx, evt.RoomID) - case "stop": - b.runStop(ctx, true) + case commandStop: + b.runStop(ctx) default: b.handleOption(ctx, commandSlice) } @@ -157,7 +186,7 @@ func (b *Bot) sendHelp(ctx context.Context, roomID id.RoomID) { var msg strings.Builder msg.WriteString("The following commands are supported:\n\n") - for _, cmd := range commands { + for _, cmd := range b.commands { if cmd.key == "" { msg.WriteString("\n---\n") continue @@ -192,7 +221,7 @@ func (b *Bot) sendHelp(ctx context.Context, roomID id.RoomID) { b.Notice(ctx, roomID, msg.String()) } -func (b *Bot) runStop(ctx context.Context, checkAllowed bool) { +func (b *Bot) runStop(ctx context.Context) { evt := eventFromContext(ctx) cfg, err := b.getSettings(evt.RoomID) if err != nil { @@ -200,11 +229,6 @@ func (b *Bot) runStop(ctx context.Context, checkAllowed bool) { return } - if checkAllowed && !b.Allowed(evt.Sender, cfg) { - b.Notice(ctx, evt.RoomID, "you don't have permission to do that") - return - } - mailbox := cfg.Get(optionMailbox) if mailbox == "" { b.Notice(ctx, evt.RoomID, "that room is not configured yet") @@ -252,7 +276,7 @@ func (b *Bot) getOption(ctx context.Context, name string) { } func (b *Bot) setOption(ctx context.Context, name, value string) { - cmd := commands.get(name) + cmd := b.commands.get(name) if cmd != nil { value = cmd.sanitizer(value) } @@ -272,11 +296,6 @@ func (b *Bot) setOption(ctx context.Context, name, value string) { return } - if !b.Allowed(evt.Sender, cfg) { - b.Notice(ctx, evt.RoomID, "you don't have permission to do that, kupo") - return - } - old := cfg.Get(name) cfg.Set(name, value) diff --git a/bot/settings.go b/bot/settings.go index 0f93d37..4efd21f 100644 --- a/bot/settings.go +++ b/bot/settings.go @@ -98,21 +98,3 @@ func (b *Bot) getSettings(roomID id.RoomID) (settings, error) { func (b *Bot) setSettings(roomID id.RoomID, cfg settings) error { return utils.UnwrapError(b.lp.GetClient().SetRoomAccountData(roomID, settingskey, cfg)) } - -// Allowed checks if change is allowed -func (b *Bot) Allowed(userID id.UserID, cfg settings) bool { - if !utils.Match(userID.String(), b.allowedUsers) { - return false - } - - if b.noowner { - return true - } - - owner := cfg.Owner() - if owner == "" { - return true - } - - return owner == userID.String() -} diff --git a/bot/sync.go b/bot/sync.go index 7b1deba..f23eeb3 100644 --- a/bot/sync.go +++ b/bot/sync.go @@ -94,7 +94,7 @@ func (b *Bot) onLeave(ctx context.Context) { count := len(members) if count == 1 && members[0] == b.lp.GetClient().UserID { b.log.Info("no more users left in the %s room", evt.RoomID) - b.runStop(ctx, false) + b.runStop(ctx) _, err := b.lp.GetClient().LeaveRoom(evt.RoomID) if err != nil { b.Error(ctx, evt.RoomID, "cannot leave empty room: %v", err)