From 275ccbd9e544ce774b125b1ab27495b5ae778533 Mon Sep 17 00:00:00 2001 From: Slavi Pantaleev Date: Sat, 27 Aug 2022 08:11:21 +0300 Subject: [PATCH] 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