From 8ad2e29930621ed13fa8c6d2b3c094b5b21d0ec1 Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Fri, 26 Aug 2022 09:38:36 +0300 Subject: [PATCH 01/10] Add support for configuring user whitelisting This does not do anything useful just yet. It will be hooked to access control checks later on. Wildcards are converted to regular expressions, because it was simpler to do that than to write (or include) some ugly wildcard matcher library. It also provides more flexibility, should we wish to use it later. Regular expressions should also work well performance-wise. We compile wildcards to regexes early on (during configuration processing) and fail if we detect a bad pattern. This is meant to catch various problems (typos or other mistakes) that could happen. For this to work, configuration building had to be redone, since it can now return an error. This may be useful in the future for validating other configuration settings. Related to https://gitlab.com/etke.cc/postmoogle/-/issues/1 --- README.md | 1 + bot/bot.go | 19 +++-- cmd/cmd.go | 10 ++- config/config.go | 19 ++++- config/types.go | 7 ++ utils/user.go | 98 +++++++++++++++++++++++ utils/user_test.go | 194 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 336 insertions(+), 12 deletions(-) create mode 100644 utils/user.go create mode 100644 utils/user_test.go diff --git a/README.md b/README.md index 3a5ecf1..42f6c68 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,7 @@ env vars * **POSTMOOGLE_DB_DSN** - database connection string * **POSTMOOGLE_DB_DIALECT** - database dialect (postgres, sqlite3) * **POSTMOOGLE_MAXSIZE** - max email size (including attachments) in megabytes +* **POSTMOOGLE_USERS** - a space-separated list of whitelisted users allowed to use the bridge. If not defined, everyone is allowed. Example rule: `@someone:example.com @another:example.com @bot.*:example.com @*:another.com` You can find default values in [config/defaults.go](config/defaults.go) diff --git a/bot/bot.go b/bot/bot.go index 83826be..db3a7e3 100644 --- a/bot/bot.go +++ b/bot/bot.go @@ -3,6 +3,7 @@ package bot import ( "context" "fmt" + "regexp" "sync" "github.com/getsentry/sentry-go" @@ -19,6 +20,7 @@ type Bot struct { federation bool prefix string domain string + allowedUsers []*regexp.Regexp rooms sync.Map log *logger.Logger lp *linkpearl.Linkpearl @@ -26,15 +28,16 @@ type Bot struct { } // New creates a new matrix bot -func New(lp *linkpearl.Linkpearl, log *logger.Logger, prefix, domain string, noowner, federation bool) *Bot { +func New(lp *linkpearl.Linkpearl, log *logger.Logger, prefix, domain string, noowner, federation bool, allowedUsers []*regexp.Regexp) *Bot { return &Bot{ - noowner: noowner, - federation: federation, - prefix: prefix, - domain: domain, - rooms: sync.Map{}, - log: log, - lp: lp, + noowner: noowner, + federation: federation, + prefix: prefix, + domain: domain, + allowedUsers: allowedUsers, + rooms: sync.Map{}, + log: log, + lp: lp, } } diff --git a/cmd/cmd.go b/cmd/cmd.go index dcd3ec7..f8d8cd1 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -26,7 +26,13 @@ var ( func main() { quit := make(chan struct{}) - cfg := config.New() + + cfg, err := config.New() + if err != nil { + log = logger.New("postmoogle.", "info") + log.Fatal("%s", err) + } + log = logger.New("postmoogle.", cfg.LogLevel) log.Info("#############################") @@ -81,7 +87,7 @@ func initBot(cfg *config.Config) { // nolint // Fatal = panic, not os.Exit() log.Fatal("cannot initialize matrix bot: %v", err) } - mxb = bot.New(lp, mxlog, cfg.Prefix, cfg.Domain, cfg.NoOwner, cfg.Federation) + mxb = bot.New(lp, mxlog, cfg.Prefix, cfg.Domain, cfg.NoOwner, cfg.Federation, cfg.Users) log.Debug("bot has been created") } diff --git a/config/config.go b/config/config.go index 4735b55..3223265 100644 --- a/config/config.go +++ b/config/config.go @@ -1,14 +1,28 @@ package config import ( + "fmt" + "gitlab.com/etke.cc/go/env" + "gitlab.com/etke.cc/postmoogle/utils" ) const prefix = "postmoogle" // New config -func New() *Config { +func New() (*Config, error) { env.SetPrefix(prefix) + + wildCardUserPatterns := env.Slice("users") + regexUserPatterns, err := utils.WildcardUserPatternsToRegexPatterns(wildCardUserPatterns) + if err != nil { + return nil, fmt.Errorf( + "failed to convert wildcard user patterns (`%s`) to regular expression: %s", + wildCardUserPatterns, + err, + ) + } + cfg := &Config{ Homeserver: env.String("homeserver", defaultConfig.Homeserver), Login: env.String("login", defaultConfig.Login), @@ -21,6 +35,7 @@ func New() *Config { Federation: env.Bool("federation"), MaxSize: env.Int("maxsize", defaultConfig.MaxSize), StatusMsg: env.String("statusmsg", defaultConfig.StatusMsg), + Users: *regexUserPatterns, Sentry: Sentry{ DSN: env.String("sentry.dsn", defaultConfig.Sentry.DSN), }, @@ -31,5 +46,5 @@ func New() *Config { }, } - return cfg + return cfg, nil } diff --git a/config/types.go b/config/types.go index 5dc8985..611c1ff 100644 --- a/config/types.go +++ b/config/types.go @@ -1,5 +1,7 @@ package config +import "regexp" + // Config of Postmoogle type Config struct { // Homeserver url @@ -26,6 +28,11 @@ type Config struct { MaxSize int // StatusMsg of the bot StatusMsg string + // Users holds regular expression patterns of users that are allowed to use the bridge. + // The regular expression patterns are compiled from wildcard patterns like: + // `@someone:example.com`, `@*:example.com`, `@bot.*:example.com`, `@someone:*`, `@someone:*.example.com` + // An empty list means that "everyone is allowed". + Users []*regexp.Regexp // DB config DB DB diff --git a/utils/user.go b/utils/user.go new file mode 100644 index 0000000..5f7f4e9 --- /dev/null +++ b/utils/user.go @@ -0,0 +1,98 @@ +package utils + +import ( + "fmt" + "regexp" + "strings" +) + +// WildcardUserPatternsToRegexPatterns converts a list of wildcard patterns to a list of regular expressions +func WildcardUserPatternsToRegexPatterns(wildCardPatterns []string) (*[]*regexp.Regexp, error) { + regexPatterns := make([]*regexp.Regexp, len(wildCardPatterns)) + + for idx, wildCardPattern := range wildCardPatterns { + regex, err := parseAllowedUserRule(wildCardPattern) + if err != nil { + return nil, fmt.Errorf("failed to parse allowed user rule `%s`: %s", wildCardPattern, err) + } + regexPatterns[idx] = regex + } + + return ®exPatterns, nil +} + +// MatchUserWithAllowedRegexes tells if the given user id is allowed to use the bot, according to the given whitelist +// An empty whitelist means "everyone is allowed" +func MatchUserWithAllowedRegexes(userID string, allowed []*regexp.Regexp) (bool, error) { + // No whitelisted users means everyone is whitelisted + if len(allowed) == 0 { + return true, nil + } + + for _, regex := range allowed { + if regex.MatchString(userID) { + return true, nil + } + } + + return false, nil +} + +// parseAllowedUserRule parses a user whitelisting rule and returns a regular expression which corresponds to it +// Example conversion: `@bot.*.something:*.example.com` -> `^bot\.([^:@]*)\.something:([^:@]*)\.example.com$` +// Example of recognized wildcard patterns: `@someone:example.com`, `@*:example.com`, `@bot.*:example.com`, `@someone:*`, `@someone:*.example.com` +func parseAllowedUserRule(wildCardRule string) (*regexp.Regexp, error) { + if !strings.HasPrefix(wildCardRule, "@") { + return nil, fmt.Errorf("rules need to be fully-qualified, starting with a @") + } + + remainingRule := wildCardRule[1:] + if strings.Contains(remainingRule, "@") { + return nil, fmt.Errorf("rules cannot contain more than one @") + } + + parts := strings.Split(remainingRule, ":") + if len(parts) != 2 { + return nil, fmt.Errorf("expected exactly 2 parts in the rule, separated by `:`") + } + + getRegexPatternForPart := func(part string) (string, error) { + if part == "" { + return "", fmt.Errorf("rejecting empty part") + } + + var pattern strings.Builder + for _, rune := range part { + if rune == '*' { + // We match everything except for `:` and `@`, because that would be an invalid MXID anyway + pattern.WriteString("([^:@]*)") + continue + } + + pattern.WriteString(regexp.QuoteMeta(string(rune))) + } + + return pattern.String(), nil + } + + localPart := parts[0] + localPartPattern, err := getRegexPatternForPart(localPart) + if err != nil { + return nil, fmt.Errorf("failed to convert local part `%s` to regex: %s", localPart, err) + } + + domainPart := parts[1] + domainPartPattern, err := getRegexPatternForPart(domainPart) + if err != nil { + return nil, fmt.Errorf("failed to convert domain part `%s` to regex: %s", domainPart, err) + } + + finalPattern := fmt.Sprintf("^@%s:%s$", localPartPattern, domainPartPattern) + + regex, err := regexp.Compile(finalPattern) + if err != nil { + return nil, fmt.Errorf("failed to compile regex `%s`: %s", finalPattern, err) + } + + return regex, nil +} diff --git a/utils/user_test.go b/utils/user_test.go new file mode 100644 index 0000000..e764d6d --- /dev/null +++ b/utils/user_test.go @@ -0,0 +1,194 @@ +package utils + +import "testing" + +func TestRuleToRegex(t *testing.T) { + type testDataDefinition struct { + name string + checkedValue string + expectedResult string + expectedError bool + } + + tests := []testDataDefinition{ + { + name: "simple pattern without wildcards succeeds", + checkedValue: "@someone:example.com", + expectedResult: `^@someone:example\.com$`, + expectedError: false, + }, + { + name: "pattern with wildcard as the whole local part succeeds", + checkedValue: "@*:example.com", + expectedResult: `^@([^:@]*):example\.com$`, + expectedError: false, + }, + { + name: "pattern with wildcard within the local part succeeds", + checkedValue: "@bot.*.something:example.com", + expectedResult: `^@bot\.([^:@]*)\.something:example\.com$`, + expectedError: false, + }, + { + name: "pattern with wildcard as the whole domain part succeeds", + checkedValue: "@someone:*", + expectedResult: `^@someone:([^:@]*)$`, + expectedError: false, + }, + { + name: "pattern with wildcard within the domain part succeeds", + checkedValue: "@someone:*.organization.com", + expectedResult: `^@someone:([^:@]*)\.organization\.com$`, + expectedError: false, + }, + { + name: "pattern with wildcard in both parts succeeds", + checkedValue: "@*:*", + expectedResult: `^@([^:@]*):([^:@]*)$`, + expectedError: false, + }, + { + name: "pattern that does not appear fully-qualified fails", + checkedValue: "someone:example.com", + expectedResult: ``, + expectedError: true, + }, + { + name: "pattern that does not appear fully-qualified fails", + checkedValue: "@someone", + expectedResult: ``, + expectedError: true, + }, + { + name: "pattern with empty domain part fails", + checkedValue: "@someone:", + expectedResult: ``, + expectedError: true, + }, + { + name: "pattern with empty local part fails", + checkedValue: "@:example.com", + expectedResult: ``, + expectedError: true, + }, + { + name: "pattern with multiple @ fails", + checkedValue: "@someone@someone:example.com", + expectedResult: ``, + expectedError: true, + }, + { + name: "pattern with multiple : fails", + checkedValue: "@someone:someone:example.com", + expectedResult: ``, + expectedError: true, + }, + } + + for _, testData := range tests { + func(testData testDataDefinition) { + t.Run(testData.name, func(t *testing.T) { + actualResult, err := parseAllowedUserRule(testData.checkedValue) + + if testData.expectedError { + if err != nil { + return + } + + t.Errorf("expected an error, but did not get one") + } + + if err != nil { + t.Errorf("did not expect an error, but got one: %s", err) + } + + if actualResult.String() == testData.expectedResult { + return + } + + t.Errorf( + "Expected `%s` to yield `%s`, not `%s`", + testData.checkedValue, + testData.expectedResult, + actualResult.String(), + ) + }) + }(testData) + } +} + +func TestMatch(t *testing.T) { + type testDataDefinition struct { + name string + checkedValue string + allowedUsers []string + expectedResult bool + } + + tests := []testDataDefinition{ + { + name: "Empty allowed users allows anyone", + checkedValue: "@someone:example.com", + allowedUsers: []string{}, + expectedResult: true, + }, + { + name: "Direct full mxid match is allowed", + checkedValue: "@someone:example.com", + allowedUsers: []string{"@someone:example.com"}, + expectedResult: true, + }, + { + name: "Direct full mxid match later on is allowed", + checkedValue: "@someone:example.com", + allowedUsers: []string{"@another:example.com", "@someone:example.com"}, + expectedResult: true, + }, + { + name: "No mxid match is not allowed", + checkedValue: "@someone:example.com", + allowedUsers: []string{"@another:example.com"}, + expectedResult: false, + }, + { + name: "mxid localpart wildcard match is allowed", + checkedValue: "@someone:example.com", + allowedUsers: []string{"@*:example.com"}, + expectedResult: true, + }, + { + name: "mxid localpart wildcard for another domain is not allowed", + checkedValue: "@someone:example.com", + allowedUsers: []string{"@*:another.com"}, + expectedResult: false, + }, + } + + for _, testData := range tests { + func(testData testDataDefinition) { + t.Run(testData.name, func(t *testing.T) { + allowedUserRegexes, err := WildcardUserPatternsToRegexPatterns(testData.allowedUsers) + if err != nil { + t.Error(err) + } + + actualResult, err := MatchUserWithAllowedRegexes(testData.checkedValue, *allowedUserRegexes) + if err != nil { + t.Error(err) + } + + if actualResult == testData.expectedResult { + return + } + + t.Errorf( + "Expected `%s` compared against `%v` to yield `%v`, not `%v`", + testData.checkedValue, + testData.allowedUsers, + testData.expectedResult, + actualResult, + ) + }) + }(testData) + } +} From f8a168b8e7de5936a3b8fa990a5aa07f1d6f0cfd Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Fri, 26 Aug 2022 10:10:05 +0300 Subject: [PATCH 02/10] Add a few more unit test cases --- utils/user_test.go | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/utils/user_test.go b/utils/user_test.go index e764d6d..93d4329 100644 --- a/utils/user_test.go +++ b/utils/user_test.go @@ -151,17 +151,47 @@ func TestMatch(t *testing.T) { expectedResult: false, }, { - name: "mxid localpart wildcard match is allowed", + name: "mxid localpart only wildcard match is allowed", checkedValue: "@someone:example.com", allowedUsers: []string{"@*:example.com"}, expectedResult: true, }, + { + name: "mxid localpart with wildcard match is allowed", + checkedValue: "@bot.abc:example.com", + allowedUsers: []string{"@bot.*:example.com"}, + expectedResult: true, + }, + { + name: "mxid localpart with wildcard match is not allowed when it does not match", + checkedValue: "@bot.abc:example.com", + allowedUsers: []string{"@employee.*:example.com"}, + expectedResult: false, + }, { name: "mxid localpart wildcard for another domain is not allowed", checkedValue: "@someone:example.com", allowedUsers: []string{"@*:another.com"}, expectedResult: false, }, + { + name: "mxid domainpart with only wildcard match is allowed", + checkedValue: "@someone:example.com", + allowedUsers: []string{"@someone:*"}, + expectedResult: true, + }, + { + name: "mxid domainpart with wildcard match is allowed", + checkedValue: "@someone:example.organization.com", + allowedUsers: []string{"@someone:*.organization.com"}, + expectedResult: true, + }, + { + name: "mxid domainpart with wildcard match is not allowed when it does not match", + checkedValue: "@someone:example.another.com", + allowedUsers: []string{"@someone:*.organization.com"}, + expectedResult: false, + }, } for _, testData := range tests { From bb754f9aa8ab81427c459c2d546629c0cbd3e48e Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Fri, 26 Aug 2022 10:19:33 +0300 Subject: [PATCH 03/10] Simplify MatchUserWithAllowedRegexes This used to return an error back when it was dealing with wildcards (which may or may not have compiled to a valid regex). But it now deals with pre-compiled regexes and has no chance of failing, so we need no `error` returns. --- utils/user.go | 8 ++++---- utils/user_test.go | 5 +---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/utils/user.go b/utils/user.go index 5f7f4e9..2b68ebe 100644 --- a/utils/user.go +++ b/utils/user.go @@ -23,19 +23,19 @@ func WildcardUserPatternsToRegexPatterns(wildCardPatterns []string) (*[]*regexp. // MatchUserWithAllowedRegexes tells if the given user id is allowed to use the bot, according to the given whitelist // An empty whitelist means "everyone is allowed" -func MatchUserWithAllowedRegexes(userID string, allowed []*regexp.Regexp) (bool, error) { +func MatchUserWithAllowedRegexes(userID string, allowed []*regexp.Regexp) bool { // No whitelisted users means everyone is whitelisted if len(allowed) == 0 { - return true, nil + return true } for _, regex := range allowed { if regex.MatchString(userID) { - return true, nil + return true } } - return false, nil + return false } // parseAllowedUserRule parses a user whitelisting rule and returns a regular expression which corresponds to it diff --git a/utils/user_test.go b/utils/user_test.go index 93d4329..ed6de85 100644 --- a/utils/user_test.go +++ b/utils/user_test.go @@ -202,10 +202,7 @@ func TestMatch(t *testing.T) { t.Error(err) } - actualResult, err := MatchUserWithAllowedRegexes(testData.checkedValue, *allowedUserRegexes) - if err != nil { - t.Error(err) - } + actualResult := MatchUserWithAllowedRegexes(testData.checkedValue, *allowedUserRegexes) if actualResult == testData.expectedResult { return From 698cb6b8b948eaf6de8c9390d811b81b74fbfd96 Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Fri, 26 Aug 2022 10:28:16 +0300 Subject: [PATCH 04/10] Fix imports lint error --- config/config.go | 1 + 1 file changed, 1 insertion(+) diff --git a/config/config.go b/config/config.go index 3223265..b9e8bed 100644 --- a/config/config.go +++ b/config/config.go @@ -4,6 +4,7 @@ import ( "fmt" "gitlab.com/etke.cc/go/env" + "gitlab.com/etke.cc/postmoogle/utils" ) From 58a1fa6b3ff62bd502774b6af16a16cd7907cd82 Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Fri, 26 Aug 2022 10:34:13 +0300 Subject: [PATCH 05/10] Do not check cognitive complexity in unit tests Works around: > utils/user_test.go:5:1: cognitive complexity 18 of func `TestRuleToRegex` is high (> 15) (gocognit) Alternatively, we may look for a way to skip this test only. It doesn't seem complex at all, so it looks like some false-positive. --- .golangci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.golangci.yml b/.golangci.yml index 16665c9..673ca55 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -83,6 +83,7 @@ issues: - path: _test\.go linters: - gocyclo + - gocognit - errcheck - dupl - gosec From 1100ee6b5fe6648a804d8ae3a34ef2966955d039 Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Fri, 26 Aug 2022 15:28:47 +0000 Subject: [PATCH 06/10] Improve error when configuration reading fails --- cmd/cmd.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index f8d8cd1..4ccf225 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -30,7 +30,7 @@ func main() { cfg, err := config.New() if err != nil { log = logger.New("postmoogle.", "info") - log.Fatal("%s", err) + log.Fatal("cannot read config: %v", err) } log = logger.New("postmoogle.", cfg.LogLevel) From 275ccbd9e544ce774b125b1ab27495b5ae778533 Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Sat, 27 Aug 2022 08:11:21 +0300 Subject: [PATCH 07/10] Implement feedback --- config/config.go | 8 +++--- config/types.go | 5 +--- utils/user.go | 65 +++++++++++++++++++++++++++------------------- utils/user_test.go | 6 ++--- 4 files changed, 46 insertions(+), 38 deletions(-) diff --git a/config/config.go b/config/config.go index b9e8bed..6c4804f 100644 --- a/config/config.go +++ b/config/config.go @@ -14,12 +14,12 @@ const prefix = "postmoogle" func New() (*Config, error) { env.SetPrefix(prefix) - wildCardUserPatterns := env.Slice("users") - regexUserPatterns, err := utils.WildcardUserPatternsToRegexPatterns(wildCardUserPatterns) + mxidPatterns := env.Slice("users") + regexPatterns, err := utils.WildcardMXIDsToRegexes(mxidPatterns) if err != nil { return nil, fmt.Errorf( "failed to convert wildcard user patterns (`%s`) to regular expression: %s", - wildCardUserPatterns, + mxidPatterns, err, ) } @@ -36,7 +36,7 @@ func New() (*Config, error) { Federation: env.Bool("federation"), MaxSize: env.Int("maxsize", defaultConfig.MaxSize), StatusMsg: env.String("statusmsg", defaultConfig.StatusMsg), - Users: *regexUserPatterns, + Users: *regexPatterns, Sentry: Sentry{ DSN: env.String("sentry.dsn", defaultConfig.Sentry.DSN), }, diff --git a/config/types.go b/config/types.go index 611c1ff..4920919 100644 --- a/config/types.go +++ b/config/types.go @@ -28,10 +28,7 @@ type Config struct { MaxSize int // StatusMsg of the bot StatusMsg string - // Users holds regular expression patterns of users that are allowed to use the bridge. - // The regular expression patterns are compiled from wildcard patterns like: - // `@someone:example.com`, `@*:example.com`, `@bot.*:example.com`, `@someone:*`, `@someone:*.example.com` - // An empty list means that "everyone is allowed". + // Users holds list of allowed users (wildcards supported), e.g.: @*:example.com, @bot.*:example.com, @admin:*. Empty = * Users []*regexp.Regexp // DB config diff --git a/utils/user.go b/utils/user.go index 2b68ebe..0e8c59e 100644 --- a/utils/user.go +++ b/utils/user.go @@ -6,12 +6,12 @@ import ( "strings" ) -// WildcardUserPatternsToRegexPatterns converts a list of wildcard patterns to a list of regular expressions -func WildcardUserPatternsToRegexPatterns(wildCardPatterns []string) (*[]*regexp.Regexp, error) { +// WildcardMXIDsToRegexes converts a list of wildcard patterns to a list of regular expressions +func WildcardMXIDsToRegexes(wildCardPatterns []string) (*[]*regexp.Regexp, error) { regexPatterns := make([]*regexp.Regexp, len(wildCardPatterns)) for idx, wildCardPattern := range wildCardPatterns { - regex, err := parseAllowedUserRule(wildCardPattern) + regex, err := parseMXIDWildcard(wildCardPattern) if err != nil { return nil, fmt.Errorf("failed to parse allowed user rule `%s`: %s", wildCardPattern, err) } @@ -21,9 +21,8 @@ func WildcardUserPatternsToRegexPatterns(wildCardPatterns []string) (*[]*regexp. return ®exPatterns, nil } -// MatchUserWithAllowedRegexes tells if the given user id is allowed to use the bot, according to the given whitelist -// An empty whitelist means "everyone is allowed" -func MatchUserWithAllowedRegexes(userID string, allowed []*regexp.Regexp) bool { +// Match tells if the given user id is allowed to use the bot, according to the given whitelist +func Match(userID string, allowed []*regexp.Regexp) bool { // No whitelisted users means everyone is whitelisted if len(allowed) == 0 { return true @@ -38,10 +37,18 @@ func MatchUserWithAllowedRegexes(userID string, allowed []*regexp.Regexp) bool { return false } -// parseAllowedUserRule parses a user whitelisting rule and returns a regular expression which corresponds to it +// parseMXIDWildcard parses a user whitelisting wildcard rule and returns a regular expression which corresponds to it +// // Example conversion: `@bot.*.something:*.example.com` -> `^bot\.([^:@]*)\.something:([^:@]*)\.example.com$` // Example of recognized wildcard patterns: `@someone:example.com`, `@*:example.com`, `@bot.*:example.com`, `@someone:*`, `@someone:*.example.com` -func parseAllowedUserRule(wildCardRule string) (*regexp.Regexp, error) { +// +// The `*` wildcard character is normally interpretted as "a number of literal characters or an empty string". +// Our implementation below matches this (yielding `([^:@])*`), which could provide a slightly suboptimal regex in these cases: +// - `@*:example.com` -> `^@([^:@])*:example\.com$`, although `^@([^:@])+:example\.com$` would be preferable +// - `@someone:*` -> `@someone:([^:@])*$`, although `@someone:([^:@])+$` would be preferable +// When it's a bare wildcard (`*`, instead of `*.example.com`) we likely prefer to yield a regex that matches **at least one character**. +// This probably doesn't matter because mxids that we'll match against are all valid and fully complete. +func parseMXIDWildcard(wildCardRule string) (*regexp.Regexp, error) { if !strings.HasPrefix(wildCardRule, "@") { return nil, fmt.Errorf("rules need to be fully-qualified, starting with a @") } @@ -56,25 +63,6 @@ func parseAllowedUserRule(wildCardRule string) (*regexp.Regexp, error) { return nil, fmt.Errorf("expected exactly 2 parts in the rule, separated by `:`") } - getRegexPatternForPart := func(part string) (string, error) { - if part == "" { - return "", fmt.Errorf("rejecting empty part") - } - - var pattern strings.Builder - for _, rune := range part { - if rune == '*' { - // We match everything except for `:` and `@`, because that would be an invalid MXID anyway - pattern.WriteString("([^:@]*)") - continue - } - - pattern.WriteString(regexp.QuoteMeta(string(rune))) - } - - return pattern.String(), nil - } - localPart := parts[0] localPartPattern, err := getRegexPatternForPart(localPart) if err != nil { @@ -96,3 +84,26 @@ func parseAllowedUserRule(wildCardRule string) (*regexp.Regexp, error) { return regex, nil } + +func getRegexPatternForPart(part string) (string, error) { + if part == "" { + return "", fmt.Errorf("rejecting empty part") + } + + var pattern strings.Builder + for _, rune := range part { + if rune == '*' { + // We match everything except for `:` and `@`, because that would be an invalid MXID anyway. + // + // If the whole part is `*` (only) instead of merely containing `*` within it, + // we may also consider replacing it with `([^:@]+)` (+, instead of *). + // See parseMXIDWildcard for notes about this. + pattern.WriteString("([^:@]*)") + continue + } + + pattern.WriteString(regexp.QuoteMeta(string(rune))) + } + + return pattern.String(), nil +} diff --git a/utils/user_test.go b/utils/user_test.go index ed6de85..5068204 100644 --- a/utils/user_test.go +++ b/utils/user_test.go @@ -88,7 +88,7 @@ func TestRuleToRegex(t *testing.T) { for _, testData := range tests { func(testData testDataDefinition) { t.Run(testData.name, func(t *testing.T) { - actualResult, err := parseAllowedUserRule(testData.checkedValue) + actualResult, err := parseMXIDWildcard(testData.checkedValue) if testData.expectedError { if err != nil { @@ -197,12 +197,12 @@ func TestMatch(t *testing.T) { for _, testData := range tests { func(testData testDataDefinition) { t.Run(testData.name, func(t *testing.T) { - allowedUserRegexes, err := WildcardUserPatternsToRegexPatterns(testData.allowedUsers) + allowedUserRegexes, err := WildcardMXIDsToRegexes(testData.allowedUsers) if err != nil { t.Error(err) } - actualResult := MatchUserWithAllowedRegexes(testData.checkedValue, *allowedUserRegexes) + actualResult := Match(testData.checkedValue, *allowedUserRegexes) if actualResult == testData.expectedResult { return From 3bc10bfe4f3d91b44098066344e06f208a0ac18c Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Sat, 27 Aug 2022 17:29:00 +0300 Subject: [PATCH 08/10] Honor allowed users list --- bot/command.go | 4 ++-- bot/settings.go | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/bot/command.go b/bot/command.go index ea19740..9a67a97 100644 --- a/bot/command.go +++ b/bot/command.go @@ -175,7 +175,7 @@ func (b *Bot) runStop(ctx context.Context, checkAllowed bool) { return } - if checkAllowed && !cfg.Allowed(b.noowner, evt.Sender) { + if checkAllowed && !cfg.Allowed(b.noowner, evt.Sender, b.allowedUsers) { b.Notice(ctx, evt.RoomID, "you don't have permission to do that") return } @@ -251,7 +251,7 @@ func (b *Bot) setOption(ctx context.Context, name, value string) { return } - if !cfg.Allowed(b.noowner, evt.Sender) { + if !cfg.Allowed(b.noowner, evt.Sender, b.allowedUsers) { b.Notice(ctx, evt.RoomID, "you don't have permission to do that, kupo") return } diff --git a/bot/settings.go b/bot/settings.go index ad37899..a7a103c 100644 --- a/bot/settings.go +++ b/bot/settings.go @@ -1,6 +1,7 @@ package bot import ( + "regexp" "strconv" "strings" @@ -20,7 +21,11 @@ type settingsOld struct { } // Allowed checks if change is allowed -func (s settings) Allowed(noowner bool, userID id.UserID) bool { +func (s settings) Allowed(noowner bool, userID id.UserID, allowedUsers []*regexp.Regexp) bool { + if !utils.Match(userID.String(), allowedUsers) { + return false + } + if noowner { return true } From 1d6b43a83ac1a434ba1d09faf365a2e79520d9c5 Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Sun, 28 Aug 2022 17:56:19 +0300 Subject: [PATCH 09/10] Reindent bot/bot.go --- bot/bot.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/bot/bot.go b/bot/bot.go index 832e2e2..d731b7e 100644 --- a/bot/bot.go +++ b/bot/bot.go @@ -31,15 +31,15 @@ type Bot struct { // New creates a new matrix bot func New(lp *linkpearl.Linkpearl, log *logger.Logger, prefix, domain string, noowner, federation bool, allowedUsers []*regexp.Regexp) *Bot { return &Bot{ - noowner: noowner, - federation: federation, - prefix: prefix, - domain: domain, + noowner: noowner, + federation: federation, + prefix: prefix, + domain: domain, allowedUsers: allowedUsers, - rooms: sync.Map{}, - log: log, - lp: lp, - mu: map[id.RoomID]*sync.Mutex{}, + rooms: sync.Map{}, + log: log, + lp: lp, + mu: map[id.RoomID]*sync.Mutex{}, } } From bd14987561f23de34908436555b78c3d3c1917fb Mon Sep 17 00:00:00 2001 From: Aine Date: Sun, 28 Aug 2022 18:36:01 +0300 Subject: [PATCH 10/10] move settings.Allowed() to bot.Allowed() --- bot/command.go | 4 ++-- bot/settings.go | 37 ++++++++++++++++++------------------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/bot/command.go b/bot/command.go index 2a6c9b2..0774aea 100644 --- a/bot/command.go +++ b/bot/command.go @@ -175,7 +175,7 @@ func (b *Bot) runStop(ctx context.Context, checkAllowed bool) { return } - if checkAllowed && !cfg.Allowed(b.noowner, evt.Sender, b.allowedUsers) { + if checkAllowed && !b.Allowed(evt.Sender, cfg) { b.Notice(ctx, evt.RoomID, "you don't have permission to do that") return } @@ -251,7 +251,7 @@ func (b *Bot) setOption(ctx context.Context, name, value string) { return } - if !cfg.Allowed(b.noowner, evt.Sender, b.allowedUsers) { + if !b.Allowed(evt.Sender, cfg) { b.Notice(ctx, evt.RoomID, "you don't have permission to do that, kupo") return } diff --git a/bot/settings.go b/bot/settings.go index 8c97d50..6ee78eb 100644 --- a/bot/settings.go +++ b/bot/settings.go @@ -1,7 +1,6 @@ package bot import ( - "regexp" "strconv" "strings" @@ -20,24 +19,6 @@ type settingsOld struct { NoSender bool } -// Allowed checks if change is allowed -func (s settings) Allowed(noowner bool, userID id.UserID, allowedUsers []*regexp.Regexp) bool { - if !utils.Match(userID.String(), allowedUsers) { - return false - } - - if noowner { - return true - } - - owner := s.Owner() - if owner == "" { - return true - } - - return owner == userID.String() -} - // Get option func (s settings) Get(key string) string { value := s[strings.ToLower(strings.TrimSpace(key))] @@ -123,3 +104,21 @@ 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() +}