implement screen name/password validation

foodgroup/admin.go

@@ -154,18 +154,21 @@ func (s AdminService) InfoChangeRequest(ctx context.Context, sess *state.Session
 
 	// validateProposedName ensures that the name is valid
 	var validateProposedName = func(name state.DisplayScreenName) (ok bool, errorCode uint16) {
-		// proposed name is too long
-		if len(name) > 16 {
+		err := name.ValidateAIMHandle()
+		switch {
+		case errors.Is(err, state.ErrAIMHandleLength):
+			// proposed name is too long
 			return false, wire.AdminInfoErrorInvalidNickNameLength
+		case errors.Is(err, state.ErrAIMHandleInvalidFormat):
+			// character or spacing issues
+			return false, wire.AdminInfoErrorInvalidNickName
 		}
+
 		// proposed name does not match session name (e.g. malicious client)
 		if name.IdentScreenName() != sess.IdentScreenName() {
 			return false, wire.AdminInfoErrorValidateNickName
 		}
-		// proposed name ends in a space
-		if name[len(name)-1] == 32 {
-			return false, wire.AdminInfoErrorInvalidNickName
-		}
+
 		return true, 0
 	}
 

server/http/mgmt_api.go

@@ -201,6 +201,12 @@ func postUserHandler(w http.ResponseWriter, r *http.Request, userManager UserMan
 	}
 
 	sn := state.DisplayScreenName(input.ScreenName)
+
+	if err := sn.ValidateAIMHandle(); err != nil {
+		http.Error(w, fmt.Sprintf("invalid screen name: %s", err), http.StatusBadRequest)
+		return
+	}
+
 	user := state.User{
 		AuthKey:           newUUID().String(),
 		DisplayScreenName: sn,
@@ -208,8 +214,7 @@ func postUserHandler(w http.ResponseWriter, r *http.Request, userManager UserMan
 	}
 
 	if err := user.HashPassword(input.Password); err != nil {
-		logger.Error("error hashing user password in POST /user", "err", err.Error())
-		http.Error(w, "internal server error", http.StatusInternalServerError)
+		http.Error(w, fmt.Sprintf("invalid password: %s", err), http.StatusBadRequest)
 		return
 	}
 

server/http/mgmt_api_test.go

@@ -149,35 +149,41 @@ func TestUserHandler_GET(t *testing.T) {
 }
 
 func TestUserHandler_POST(t *testing.T) {
+	type insertUserParams struct {
+		user state.User
+		err  error
+	}
 	tt := []struct {
-		name           string
-		body           string
-		UUID           uuid.UUID
-		user           state.User
-		userHandlerErr error
-		want           string
-		statusCode     int
+		name             string
+		body             string
+		UUID             uuid.UUID
+		insertUserParams []insertUserParams
+		want             string
+		statusCode       int
 	}{
 		{
 			name: "with valid user",
 			body: `{"screen_name":"userA", "password":"thepassword"}`,
 			UUID: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b"),
-			user: func() state.User {
-				user := state.User{
-					AuthKey:           uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(),
-					DisplayScreenName: "userA",
-					IdentScreenName:   state.NewIdentScreenName("userA"),
-				}
-				assert.NoError(t, user.HashPassword("thepassword"))
-				return user
-			}(),
+			insertUserParams: []insertUserParams{
+				{
+					user: func() state.User {
+						user := state.User{
+							AuthKey:           uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(),
+							DisplayScreenName: "userA",
+							IdentScreenName:   state.NewIdentScreenName("userA"),
+						}
+						assert.NoError(t, user.HashPassword("thepassword"))
+						return user
+					}(),
+				},
+			},
 			want:       `User account created successfully.`,
 			statusCode: http.StatusCreated,
 		},
 		{
 			name:       "with malformed body",
 			body:       `{"screen_name":"userA", "password":"thepassword"`,
-			user:       state.User{},
 			want:       `malformed input`,
 			statusCode: http.StatusBadRequest,
 		},
@@ -185,35 +191,57 @@ func TestUserHandler_POST(t *testing.T) {
 			name: "user handler error",
 			body: `{"screen_name":"userA", "password":"thepassword"}`,
 			UUID: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b"),
-			user: func() state.User {
-				user := state.User{
-					AuthKey:           uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(),
-					DisplayScreenName: "userA",
-					IdentScreenName:   state.NewIdentScreenName("userA"),
-				}
-				assert.NoError(t, user.HashPassword("thepassword"))
-				return user
-			}(),
-			userHandlerErr: io.EOF,
-			want:           `internal server error`,
-			statusCode:     http.StatusInternalServerError,
+			insertUserParams: []insertUserParams{
+				{
+					user: func() state.User {
+						user := state.User{
+							AuthKey:           uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(),
+							DisplayScreenName: "userA",
+							IdentScreenName:   state.NewIdentScreenName("userA"),
+						}
+						assert.NoError(t, user.HashPassword("thepassword"))
+						return user
+					}(),
+					err: io.EOF,
+				},
+			},
+			want:       `internal server error`,
+			statusCode: http.StatusInternalServerError,
 		},
 		{
 			name: "duplicate user",
 			body: `{"screen_name":"userA", "password":"thepassword"}`,
 			UUID: uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b"),
-			user: func() state.User {
-				user := state.User{
-					AuthKey:           uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(),
-					DisplayScreenName: "userA",
-					IdentScreenName:   state.NewIdentScreenName("userA"),
-				}
-				assert.NoError(t, user.HashPassword("thepassword"))
-				return user
-			}(),
-			userHandlerErr: state.ErrDupUser,
-			want:           `user already exists`,
-			statusCode:     http.StatusConflict,
+			insertUserParams: []insertUserParams{
+				{
+					user: func() state.User {
+						user := state.User{
+							AuthKey:           uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b").String(),
+							DisplayScreenName: "userA",
+							IdentScreenName:   state.NewIdentScreenName("userA"),
+						}
+						assert.NoError(t, user.HashPassword("thepassword"))
+						return user
+					}(),
+					err: state.ErrDupUser,
+				},
+			},
+			want:       `user already exists`,
+			statusCode: http.StatusConflict,
+		},
+		{
+			name:       "invalid AIM screen name",
+			body:       `{"screen_name":"a", "password":"thepassword"}`,
+			UUID:       uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b"),
+			want:       `invalid screen name: screen name must be between 3 and 16 characters`,
+			statusCode: http.StatusBadRequest,
+		},
+		{
+			name:       "invalid AIM password",
+			body:       `{"screen_name":"userA", "password":"1"}`,
+			UUID:       uuid.MustParse("07c70701-ba68-49a9-9f9b-67a53816e37b"),
+			want:       `invalid password: password length must be between 4-16 characters`,
+			statusCode: http.StatusBadRequest,
 		},
 	}
 
@@ -223,10 +251,10 @@ func TestUserHandler_POST(t *testing.T) {
 			responseRecorder := httptest.NewRecorder()
 
 			userManager := newMockUserManager(t)
-			if tc.user.IdentScreenName.String() != "" {
+			for _, params := range tc.insertUserParams {
 				userManager.EXPECT().
-					InsertUser(tc.user).
-					Return(tc.userHandlerErr)
+					InsertUser(params.user).
+					Return(params.err)
 			}
 
 			newUUID := func() uuid.UUID { return tc.UUID }

state/user.go

@@ -3,7 +3,9 @@ package state
 import (
 	"bytes"
 	"errors"
+	"strconv"
 	"strings"
+	"unicode"
 
 	"github.com/google/uuid"
 
@@ -58,6 +60,50 @@ func NewIdentScreenName(screenName string) IdentScreenName {
 // This includes the original casing and spacing as defined by the user.
 type DisplayScreenName string
 
+var (
+	ErrAIMHandleLength        = errors.New("screen name must be between 3 and 16 characters")
+	ErrAIMHandleInvalidFormat = errors.New("screen name must start with a letter, cannot end with a space, and must contain only letters, numbers, and spaces")
+	ErrICQUINInvalidFormat    = errors.New("uin must be a number in the range 10000-2147483646")
+)
+
+// ValidateAIMHandle returns an error if the instance is not a valid AIM screen name.
+// Possible errors:
+//   - ErrAIMHandleLength: if the screen name is not between 3 and 16
+//     characters
+//   - ErrAIMHandleInvalidFormat: if the screen name does not start with a
+//     letter, ends with a space, or contains invalid characters
+func (s DisplayScreenName) ValidateAIMHandle() error {
+	if len(s) < 3 || len(s) > 16 {
+		return ErrAIMHandleLength
+	}
+
+	// Must start with a letter, cannot end with a space,
+	// and must contain only letters, numbers, and spaces
+	if !unicode.IsLetter(rune(s[0])) || s[len(s)-1] == ' ' {
+		return ErrAIMHandleInvalidFormat
+	}
+
+	for _, ch := range s {
+		if !unicode.IsLetter(ch) && !unicode.IsDigit(ch) && ch != ' ' {
+			return ErrAIMHandleInvalidFormat
+		}
+	}
+
+	return nil
+}
+
+// ValidateICQHandle returns an error if the instance is not a valid ICQ UIN.
+// Possible errors:
+//   - ErrICQUINInvalidFormat: if the UIN is not a number or is not in the valid
+//     range
+func (s DisplayScreenName) ValidateICQHandle() error {
+	uin, err := strconv.Atoi(string(s))
+	if err != nil || uin < 10000 || uin > 2147483646 {
+		return ErrICQUINInvalidFormat
+	}
+	return nil
+}
+
 // IdentScreenName converts the DisplayScreenName to an IdentScreenName by applying
 // the normalization process defined in NewIdentScreenName.
 func (s DisplayScreenName) IdentScreenName() IdentScreenName {
@@ -118,7 +164,32 @@ func (u *User) ValidateRoastedPass(roastedPass []byte) bool {
 // HashPassword computes MD5 hashes of the user's password. It computes both
 // weak and strong variants and stores them in the struct.
 func (u *User) HashPassword(passwd string) error {
+	if err := validateAIMPassword(passwd); err != nil {
+		return err
+	}
 	u.WeakMD5Pass = wire.WeakMD5PasswordHash(passwd, u.AuthKey)
 	u.StrongMD5Pass = wire.StrongMD5PasswordHash(passwd, u.AuthKey)
 	return nil
 }
+
+// validateAIMPassword returns an error if the AIM password is invalid.
+// A valid password is 4-16 characters long. The minimum password length is
+// set here for software preservation purposes; operators should set more
+// stringent password requirements.
+func validateAIMPassword(pass string) error {
+	if len(pass) < 4 || len(pass) > 16 {
+		return errors.New("password length must be between 4-16 characters")
+	}
+	return nil
+}
+
+// validateICQPassword returns an error if the ICQ password is invalid.
+// A valid password is 1-8 characters long. The minimum password length is set
+// here for software preservation purposes; operators should set more stringent
+// password requirements.
+func validateICQPassword(pass string) error {
+	if len(pass) < 1 || len(pass) > 8 {
+		return errors.New("password must be between 1 and 8 characters")
+	}
+	return nil
+}

state/user_test.go

@@ -0,0 +1,118 @@
+package state
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/mk6i/retro-aim-server/wire"
+)
+
+func TestUser_HashPassword(t *testing.T) {
+	tests := []struct {
+		name              string
+		user              User
+		password          string
+		expectedWeakMD5   []byte
+		expectedStrongMD5 []byte
+		wantError         bool
+	}{
+		{
+			name:              "Valid password",
+			user:              User{AuthKey: "someAuthKey"},
+			password:          "validPassword",
+			expectedWeakMD5:   wire.WeakMD5PasswordHash("validPassword", "someAuthKey"),
+			expectedStrongMD5: wire.StrongMD5PasswordHash("validPassword", "someAuthKey"),
+			wantError:         false,
+		},
+		{
+			name:              "Empty password",
+			user:              User{AuthKey: "someAuthKey"},
+			password:          "",
+			expectedWeakMD5:   nil,
+			expectedStrongMD5: nil,
+			wantError:         true,
+		},
+		{
+			name:              "Password too short",
+			user:              User{AuthKey: "someAuthKey"},
+			password:          "abc",
+			expectedWeakMD5:   nil,
+			expectedStrongMD5: nil,
+			wantError:         true,
+		},
+		{
+			name:              "Password too long",
+			user:              User{AuthKey: "someAuthKey"},
+			password:          "thispasswordistoolong",
+			expectedWeakMD5:   nil,
+			expectedStrongMD5: nil,
+			wantError:         true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := tt.user.HashPassword(tt.password)
+
+			if tt.wantError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, tt.expectedWeakMD5, tt.user.WeakMD5Pass)
+				assert.Equal(t, tt.expectedStrongMD5, tt.user.StrongMD5Pass)
+			}
+		})
+	}
+}
+
+func TestDisplayScreenName_ValidateAIMHandle(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   DisplayScreenName
+		wantErr error
+	}{
+		{"Valid handle", "User123", nil},
+		{"Too short", "Us", ErrAIMHandleLength},
+		{"Too long", "ThisIsAReallyLongScreenName", ErrAIMHandleLength},
+		{"Starts with number", "1User", ErrAIMHandleInvalidFormat},
+		{"Ends with space", "User123 ", ErrAIMHandleInvalidFormat},
+		{"Contains invalid character", "User@123", ErrAIMHandleInvalidFormat},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := tt.input.ValidateAIMHandle()
+			if tt.wantErr != nil {
+				assert.ErrorIs(t, err, tt.wantErr, "ValidateAIMHandle() error = %v, wantErr %v", err, tt.wantErr)
+			} else {
+				assert.NoError(t, err, "ValidateAIMHandle() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
+func TestDisplayScreenName_ValidateICQHandle(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   DisplayScreenName
+		wantErr error
+	}{
+		{"Valid UIN", "123456", nil},
+		{"Too low", "9999", ErrICQUINInvalidFormat},
+		{"Too high", "2147483647", ErrICQUINInvalidFormat},
+		{"Non-numeric", "abcd", ErrICQUINInvalidFormat},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := tt.input.ValidateICQHandle()
+			if tt.wantErr != nil {
+				assert.ErrorIs(t, err, tt.wantErr, "ValidateICQHandle() error = %v, wantErr %v", err, tt.wantErr)
+			} else {
+				assert.NoError(t, err, "ValidateICQHandle() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

wire/user.go

@@ -2,9 +2,7 @@ package wire
 
 import (
 	"crypto/md5"
-	"errors"
 	"io"
-	"unicode"
 )
 
 // WeakMD5PasswordHash hashes password and authKey for AIM v3.5-v4.7.
@@ -45,64 +43,3 @@ func RoastPassword(roastedPass []byte) []byte {
 	}
 	return clearPass
 }
-
-// ValidateAIMHandle returns an error if the AIM screen name is invalid.
-// A valid screen name meets the following criteria:
-//   - 3-16 letters/numbers
-//   - must start with a letter
-//   - doesn't end with a space
-func ValidateAIMHandle(screenName string) error {
-	if len(screenName) < 3 || len(screenName) > 16 {
-		return errors.New("screen name must be between 3 and 16 characters")
-	}
-
-	// Must start with a letter
-	if !unicode.IsLetter(rune(screenName[0])) {
-		return errors.New("screen name must start with a letter")
-	}
-
-	// Cannot end with a space
-	if screenName[len(screenName)-1] == ' ' {
-		return errors.New("screen name cannot end with a space")
-	}
-
-	// Must contain only letters, numbers, and spaces
-	for _, ch := range screenName {
-		if !unicode.IsLetter(ch) && !unicode.IsDigit(ch) && ch != ' ' {
-			return errors.New("screen name must contain only letters, numbers, and spaces")
-		}
-	}
-
-	return nil
-}
-
-// ValidateAIMPassword returns an error if the AIM password is invalid.
-// A valid password is 4-16 characters long. The minimum password length is
-// set here for software preservation purposes; operators should set more
-// stringent password requirements.
-func ValidateAIMPassword(pass string) error {
-	if len(pass) < 4 || len(pass) > 16 {
-		return errors.New("password must be between 4 and 16 characters")
-	}
-	return nil
-}
-
-// ValidateICQHandle returns an error if the ICQ UIN is invalid.
-// A valid UIN is a number in the range 10000-2147483646.
-func ValidateICQHandle(UIN uint32) error {
-	if UIN < 10000 || UIN > 2147483646 {
-		return errors.New("UIN must be between 10000 and 2147483646")
-	}
-	return nil
-}
-
-// ValidateICQPassword returns an error if the ICQ password is invalid.
-// A valid password is 1-8 characters long. The minimum password length is set
-// here for software preservation purposes; operators should set more stringent
-// password requirements.
-func ValidateICQPassword(pass string) error {
-	if len(pass) < 1 || len(pass) > 8 {
-		return errors.New("password must be between 1 and 8 characters")
-	}
-	return nil
-}

wire/user_test.go

@@ -1,108 +0,0 @@
-package wire
-
-import (
-	"errors"
-	"fmt"
-	"testing"
-)
-
-func TestValidateAIMHandle(t *testing.T) {
-	tests := []struct {
-		screenName string
-		expected   error
-	}{
-		{"ValidName", nil},
-		{"Valid Name", nil},
-		{"Val1dName", nil},
-		{"Va", errors.New("screen name must be between 3 and 16 characters")},
-		{"ThisIsAVeryLongScreenName", errors.New("screen name must be between 3 and 16 characters")},
-		{"1Invalid", errors.New("screen name must start with a letter")},
-		{"Invalid ", errors.New("screen name cannot end with a space")},
-		{"Inval!dName", errors.New("screen name must contain only letters, numbers, and spaces")},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.screenName, func(t *testing.T) {
-			err := ValidateAIMHandle(tt.screenName)
-			if err != nil && err.Error() != tt.expected.Error() {
-				t.Errorf("got %v, want %v", err, tt.expected)
-			}
-			if err == nil && tt.expected != nil {
-				t.Errorf("got %v, want %v", err, tt.expected)
-			}
-		})
-	}
-}
-
-func TestValidateAIMPassword(t *testing.T) {
-	tests := []struct {
-		password string
-		expected error
-	}{
-		{"pass1234", nil},
-		{"1234", nil},
-		{"pa", errors.New("password must be between 4 and 16 characters")},
-		{"thisisaverylongpassword", errors.New("password must be between 4 and 16 characters")},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.password, func(t *testing.T) {
-			err := ValidateAIMPassword(tt.password)
-			if err != nil && err.Error() != tt.expected.Error() {
-				t.Errorf("got %v, want %v", err, tt.expected)
-			}
-			if err == nil && tt.expected != nil {
-				t.Errorf("got %v, want %v", err, tt.expected)
-			}
-		})
-	}
-}
-
-func TestValidateICQHandle(t *testing.T) {
-	tests := []struct {
-		UIN      uint32
-		expected error
-	}{
-		{123456, nil},
-		{10000, nil},
-		{2147483646, nil},
-		{9999, errors.New("UIN must be between 10000 and 2147483646")},
-		{2147483647, errors.New("UIN must be between 10000 and 2147483646")},
-	}
-
-	for _, tt := range tests {
-		t.Run(fmt.Sprintf("%d", tt.UIN), func(t *testing.T) {
-			err := ValidateICQHandle(tt.UIN)
-			if err != nil && err.Error() != tt.expected.Error() {
-				t.Errorf("got %v, want %v", err, tt.expected)
-			}
-			if err == nil && tt.expected != nil {
-				t.Errorf("got %v, want %v", err, tt.expected)
-			}
-		})
-	}
-}
-
-func TestValidateICQPassword(t *testing.T) {
-	tests := []struct {
-		password string
-		expected error
-	}{
-		{"pass", nil},
-		{"12345678", nil},
-		{"", errors.New("password must be between 1 and 8 characters")},
-		{"123456789", errors.New("password must be between 1 and 8 characters")},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.password, func(t *testing.T) {
-			err := ValidateICQPassword(tt.password)
-			if err != nil && err.Error() != tt.expected.Error() {
-				t.Errorf("got %v, want %v", err, tt.expected)
-			}
-			if err == nil && tt.expected != nil {
-				t.Errorf("got %v, want %v", err, tt.expected)
-			}
-		})
-	}
-}