Improve username validation logic

io/userRegistration.go

@@ -18,13 +18,9 @@ import (
 	"gitlab.com/xx_network/comms/messages"
 	"gitlab.com/xx_network/crypto/signature/rsa"
 	"gitlab.com/xx_network/primitives/id"
-	"regexp"
-	"strings"
 	"time"
 )
 
-var usernameRegex = regexp.MustCompile("^[a-zA-Z0-9_\\-\\!@#$%\\^\\*\\?]*$")
-
 // Endpoint which handles a users attempt to register
 func registerUser(msg *pb.UDBUserRegistration, permPublicKey *rsa.PublicKey,
 	store *storage.Storage) (*messages.Ack, error) {
@@ -44,22 +40,17 @@ func registerUser(msg *pb.UDBUserRegistration, permPublicKey *rsa.PublicKey,
 			"Please try again")
 	}
 
-	// Check if username contains acceptable characters
-	if !isValidUsername(username) {
-		return nil, errors.Errorf("Username %q "+
-			"must contain only printable non-whitespace ASCII characters", username)
-	}
+	flattened := canonicalize(username)
 
-	flatten := func(s string) string {
-		return strings.ToLower(s)
+	// Check if username is valid
+	if err := isValidUsername(flattened); err != nil {
+		return nil, errors.Errorf("Username %q is invalid: %v", username, err)
 	}
-	// TODO: flatten username
-	flattened := flatten(username)
 
 	// Check if username is taken
 	err = store.CheckUser(flattened, uid)
 	if err != nil {
-		return &messages.Ack{}, errors.Errorf("Username %s is already taken. "+
+		return &messages.Ack{}, errors.Errorf("Username %q is already taken. "+
 			"Please try again", username)
 	}
 
@@ -136,10 +127,3 @@ func registerUser(msg *pb.UDBUserRegistration, permPublicKey *rsa.PublicKey,
 
 	return &messages.Ack{}, nil
 }
-
-// isValidUsername determines whether the username is valid under a
-// pre-defined ASCII subset. The ASCII subset is defined as the following characters:
-// "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-!@#$%^*?"
-func isValidUsername(username string) bool {
-	return usernameRegex.MatchString(username)
-}

io/validation.go

+////////////////////////////////////////////////////////////////////////////////
+// Copyright © 2020 Privategrity Corporation                                   /
+//                                                                             /
+// All rights reserved.                                                        /
+////////////////////////////////////////////////////////////////////////////////
+
+package io
+
+import (
+	"github.com/pkg/errors"
+	"regexp"
+	"strings"
+)
+
+// Character limits for usernames.
+const (
+	minimumUsernameLength = 4
+	maximumUsernameLength = 32
+)
+
+// usernameRegex is the regular expression for the expected characters as follows:
+//  abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-!@#$%^*?
+var usernameRegex = regexp.MustCompile("^[a-zA-Z0-9_\\-\\!@#$%\\^\\*\\?]*$")
+
+// isValidUsername determines whether the username is of an acceptable length and
+// whether it contains allowed character. The allowed characters are defined
+// by usernameRegex.
+func isValidUsername(username string) error {
+	// Check for acceptable length
+	if len(username) < minimumUsernameLength || len(username) > maximumUsernameLength {
+		return errors.Errorf("username length %d is not between %d and %d",
+			len(username), minimumUsernameLength, maximumUsernameLength)
+	}
+
+	// Check is username contains allowed characters only
+	if !usernameRegex.MatchString(username) {
+		return errors.Errorf("username can only contain alphanumberics and the symbols !, @, #, $, %%, ^, *, ?")
+	}
+
+	return nil
+}
+
+// canonicalize reduces the username to its canonical form. For the purposes
+// of internal usage only.
+func canonicalize(username string) string {
+	return strings.ToLower(username)
+}

io/validation_test.go

+////////////////////////////////////////////////////////////////////////////////
+// Copyright © 2020 Privategrity Corporation                                   /
+//                                                                             /
+// All rights reserved.                                                        /
+////////////////////////////////////////////////////////////////////////////////
+
+package io
+
+import "testing"
+
+// Tests whether good usernames are considered valid.
+func TestIsValidUsername_GoodUsernames(t *testing.T) {
+	// Construct a list of good username
+	goodUsernames := []string{
+		"abcdefghijklmnopqrstuvwxyzABCDEF",
+		"GHIJKLMNOPQRSTUVWXYZ0123456789_-",
+		"!@#$%^*?",
+		"john_doe",
+		"daMan",
+		"josh!!!!!",
+		"Mr?George",
+	}
+
+	// Test whether every good username is valid
+	for _, goodUsername := range goodUsernames {
+		err := isValidUsername(goodUsername)
+		if err != nil { // If invalid, fail test
+			t.Errorf("isValidUsername failed with username %q: %v", goodUsername, err)
+		}
+	}
+
+}
+
+// Tests whether invalid usernames are considered invalid.
+func TestIsValidUsername_BadUsernames(t *testing.T) {
+	// Construct a list of bad usernames
+	badUsernames := []string{
+		"",
+		"  ",
+		"pie",
+		"123456789012345678901234567890123",
+		"аdМіnіѕТrаТоr",
+		"ÁdmïNIstrátör",
+		"𝔞𝔡𝔪𝔦𝔫",
+		"á̵͖͔͇̟͙̜͙̀͑̒̀̕ď̶̦̣̲m̴̡̬̺̯̩͂i̶͚͍̝̋ͅn̶̤̙̩͎̠͍̙̱̹̏",
+		"finished",
+	}
+
+	t.Logf("%s", usernameRegex.String())
+
+	// Test if every bad username is invalid
+	for _, badUsername := range badUsernames {
+		err := isValidUsername(badUsername)
+		if err == nil { // If considered valid, fail test
+			t.Errorf("isValidUsername did not fail with username %q", badUsername)
+		}
+	}
+
+}
+
+// Consistency test for the canonicalize function.
+func TestCanonicalize(t *testing.T) {
+	inputList := []string{
+		"John_Doe",
+		"HELLO",
+		"hello",
+		"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-!@#$%^*?",
+	}
+
+	expected := []string{
+		"john_doe",
+		"hello",
+		"hello",
+		"abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz0123456789_-!@#$%^*?",
+	}
+
+	for i, input := range inputList {
+		received := canonicalize(input)
+		if received != expected[i] {
+			t.Errorf("canonicalize did not produce expeccted result with %q"+
+				"\nExpected: %s"+
+				"\nReceived: %s", input, expected[i], received)
+		}
+	}
+}