Implement feedback

config/config.go

@@ -14,12 +14,12 @@ const prefix = "postmoogle"
 func New() (*Config, error) {
 	env.SetPrefix(prefix)
 
-	wildCardUserPatterns := env.Slice("users")
+	mxidPatterns := env.Slice("users")
-	regexUserPatterns, err := utils.WildcardUserPatternsToRegexPatterns(wildCardUserPatterns)
+	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),
 		},

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.
+	// Users holds list of allowed users (wildcards supported), e.g.: @*:example.com, @bot.*:example.com, @admin:*. Empty = *
-	// 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

utils/user.go

@@ -6,12 +6,12 @@ import (
 	"strings"
 )
 
-// WildcardUserPatternsToRegexPatterns converts a list of wildcard patterns to a list of regular expressions
+// WildcardMXIDsToRegexes converts a list of wildcard patterns to a list of regular expressions
-func WildcardUserPatternsToRegexPatterns(wildCardPatterns []string) (*[]*regexp.Regexp, error) {
+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 &regexPatterns, nil
 }
 
-// MatchUserWithAllowedRegexes tells if the given user id is allowed to use the bot, according to the given whitelist
+// Match 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 Match(userID string, allowed []*regexp.Regexp) bool {
-func MatchUserWithAllowedRegexes(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
+}

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