Refactor registration tests, remove hard-coded username validation (#3138)

### Pull Request Checklist

* [x] I have added Go unit tests or [Complement integration
tests](https://github.com/matrix-org/complement) for this PR _or_ I have
justified why this PR doesn't need tests
* [x] I have already signed off privately

This PR is in preparation for #3137 and removes the hard-coded username
validation (previously only dependent on `forceEmpty`).

---------

Co-authored-by: kegsay <7190048+kegsay@users.noreply.github.com>

1 files changed, 63 insertions, 59 deletions

diff --git a/clientapi/routing/register_test.go b/clientapi/routing/register_test.go
index 98455f80..7fa740e7 100644
--- a/clientapi/routing/register_test.go
+++ b/clientapi/routing/register_test.go
@@ -298,25 +298,29 @@ func Test_register(t *testing.T) {
 		guestsDisabled       bool
 		enableRecaptcha      bool
 		captchaBody          string
-		wantResponse         util.JSONResponse
+		// in case of an error, the expected response
+		wantErrorResponse util.JSONResponse
+		// in case of success, the expected username assigned
+		wantUsername string
 	}{
 		{
 			name:           "disallow guests",
 			kind:           "guest",
 			guestsDisabled: true,
-			wantResponse: util.JSONResponse{
+			wantErrorResponse: util.JSONResponse{
 				Code: http.StatusForbidden,
 				JSON: spec.Forbidden(`Guest registration is disabled on "test"`),
 			},
 		},
 		{
-			name: "allow guests",
-			kind: "guest",
+			name:         "allow guests",
+			kind:         "guest",
+			wantUsername: "1",
 		},
 		{
 			name:      "unknown login type",
 			loginType: "im.not.known",
-			wantResponse: util.JSONResponse{
+			wantErrorResponse: util.JSONResponse{
 				Code: http.StatusNotImplemented,
 				JSON: spec.Unknown("unknown/unimplemented auth type"),
 			},
@@ -324,25 +328,33 @@ func Test_register(t *testing.T) {
 		{
 			name:                 "disabled registration",
 			registrationDisabled: true,
-			wantResponse: util.JSONResponse{
+			wantErrorResponse: util.JSONResponse{
 				Code: http.StatusForbidden,
 				JSON: spec.Forbidden(`Registration is disabled on "test"`),
 			},
 		},
 		{
-			name:       "successful registration, numeric ID",
-			username:   "",
-			password:   "someRandomPassword",
-			forceEmpty: true,
+			name:         "successful registration, numeric ID",
+			username:     "",
+			password:     "someRandomPassword",
+			forceEmpty:   true,
+			wantUsername: "2",
 		},
 		{
 			name:     "successful registration",
 			username: "success",
 		},
 		{
+			name:         "successful registration, sequential numeric ID",
+			username:     "",
+			password:     "someRandomPassword",
+			forceEmpty:   true,
+			wantUsername: "3",
+		},
+		{
 			name:     "failing registration - user already exists",
 			username: "success",
-			wantResponse: util.JSONResponse{
+			wantErrorResponse: util.JSONResponse{
 				Code: http.StatusBadRequest,
 				JSON: spec.UserInUse("Desired user ID is already taken."),
 			},
@@ -352,14 +364,14 @@ func Test_register(t *testing.T) {
 			username: "LOWERCASED", // this is going to be lower-cased
 		},
 		{
-			name:         "invalid username",
-			username:     "#totalyNotValid",
-			wantResponse: *internal.UsernameResponse(internal.ErrUsernameInvalid),
+			name:              "invalid username",
+			username:          "#totalyNotValid",
+			wantErrorResponse: *internal.UsernameResponse(internal.ErrUsernameInvalid),
 		},
 		{
 			name:     "numeric username is forbidden",
 			username: "1337",
-			wantResponse: util.JSONResponse{
+			wantErrorResponse: util.JSONResponse{
 				Code: http.StatusBadRequest,
 				JSON: spec.InvalidUsername("Numeric user IDs are reserved"),
 			},
@@ -367,7 +379,7 @@ func Test_register(t *testing.T) {
 		{
 			name:      "disabled recaptcha login",
 			loginType: authtypes.LoginTypeRecaptcha,
-			wantResponse: util.JSONResponse{
+			wantErrorResponse: util.JSONResponse{
 				Code: http.StatusForbidden,
 				JSON: spec.Unknown(ErrCaptchaDisabled.Error()),
 			},
@@ -376,7 +388,7 @@ func Test_register(t *testing.T) {
 			name:            "enabled recaptcha, no response defined",
 			enableRecaptcha: true,
 			loginType:       authtypes.LoginTypeRecaptcha,
-			wantResponse: util.JSONResponse{
+			wantErrorResponse: util.JSONResponse{
 				Code: http.StatusBadRequest,
 				JSON: spec.BadJSON(ErrMissingResponse.Error()),
 			},
@@ -386,7 +398,7 @@ func Test_register(t *testing.T) {
 			enableRecaptcha: true,
 			loginType:       authtypes.LoginTypeRecaptcha,
 			captchaBody:     `notvalid`,
-			wantResponse: util.JSONResponse{
+			wantErrorResponse: util.JSONResponse{
 				Code: http.StatusUnauthorized,
 				JSON: spec.BadJSON(ErrInvalidCaptcha.Error()),
 			},
@@ -398,11 +410,11 @@ func Test_register(t *testing.T) {
 			captchaBody:     `success`,
 		},
 		{
-			name:            "captcha invalid from remote",
-			enableRecaptcha: true,
-			loginType:       authtypes.LoginTypeRecaptcha,
-			captchaBody:     `i should fail for other reasons`,
-			wantResponse:    util.JSONResponse{Code: http.StatusInternalServerError, JSON: spec.InternalServerError{}},
+			name:              "captcha invalid from remote",
+			enableRecaptcha:   true,
+			loginType:         authtypes.LoginTypeRecaptcha,
+			captchaBody:       `i should fail for other reasons`,
+			wantErrorResponse: util.JSONResponse{Code: http.StatusInternalServerError, JSON: spec.InternalServerError{}},
 		},
 	}
 
@@ -486,8 +498,8 @@ func Test_register(t *testing.T) {
 						t.Fatalf("unexpected registration flows: %+v, want %+v", r.Flows, cfg.Derived.Registration.Flows)
 					}
 				case spec.MatrixError:
-					if !reflect.DeepEqual(tc.wantResponse, resp) {
-						t.Fatalf("(%s), unexpected response: %+v, want: %+v", tc.name, resp, tc.wantResponse)
+					if !reflect.DeepEqual(tc.wantErrorResponse, resp) {
+						t.Fatalf("(%s), unexpected response: %+v, want: %+v", tc.name, resp, tc.wantErrorResponse)
 					}
 					return
 				case registerResponse:
@@ -505,6 +517,13 @@ func Test_register(t *testing.T) {
 					if r.DeviceID == "" {
 						t.Fatalf("missing deviceID in response")
 					}
+					// if an expected username is provided, assert that it is a match
+					if tc.wantUsername != "" {
+						wantUserID := strings.ToLower(fmt.Sprintf("@%s:%s", tc.wantUsername, "test"))
+						if wantUserID != r.UserID {
+							t.Fatalf("unexpected userID: %s, want %s", r.UserID, wantUserID)
+						}
+					}
 					return
 				default:
 					t.Logf("Got response: %T", resp.JSON)
@@ -541,44 +560,29 @@ func Test_register(t *testing.T) {
 
 				resp = Register(req, userAPI, &cfg.ClientAPI)
 
-				switch resp.JSON.(type) {
-				case spec.InternalServerError:
-					if !reflect.DeepEqual(tc.wantResponse, resp) {
-						t.Fatalf("unexpected response: %+v, want: %+v", resp, tc.wantResponse)
+				switch rr := resp.JSON.(type) {
+				case spec.InternalServerError, spec.MatrixError, util.JSONResponse:
+					if !reflect.DeepEqual(tc.wantErrorResponse, resp) {
+						t.Fatalf("unexpected response: %+v, want: %+v", resp, tc.wantErrorResponse)
 					}
 					return
-				case spec.MatrixError:
-					if !reflect.DeepEqual(tc.wantResponse, resp) {
-						t.Fatalf("unexpected response: %+v, want: %+v", resp, tc.wantResponse)
+				case registerResponse:
+					// validate the response
+					if tc.wantUsername != "" {
+						// if an expected username is provided, assert that it is a match
+						wantUserID := strings.ToLower(fmt.Sprintf("@%s:%s", tc.wantUsername, "test"))
+						if wantUserID != rr.UserID {
+							t.Fatalf("unexpected userID: %s, want %s", rr.UserID, wantUserID)
+						}
 					}
-					return
-				case util.JSONResponse:
-					if !reflect.DeepEqual(tc.wantResponse, resp) {
-						t.Fatalf("unexpected response: %+v, want: %+v", resp, tc.wantResponse)
+					if rr.DeviceID != *reg.DeviceID {
+						t.Fatalf("unexpected deviceID: %s, want %s", rr.DeviceID, *reg.DeviceID)
 					}
-					return
-				}
-
-				rr, ok := resp.JSON.(registerResponse)
-				if !ok {
-					t.Fatalf("expected a registerresponse, got %T", resp.JSON)
-				}
-
-				// validate the response
-				if tc.forceEmpty {
-					// when not supplying a username, one will be generated. Given this _SHOULD_ be
-					// the second user, set the username accordingly
-					reg.Username = "2"
-				}
-				wantUserID := strings.ToLower(fmt.Sprintf("@%s:%s", reg.Username, "test"))
-				if wantUserID != rr.UserID {
-					t.Fatalf("unexpected userID: %s, want %s", rr.UserID, wantUserID)
-				}
-				if rr.DeviceID != *reg.DeviceID {
-					t.Fatalf("unexpected deviceID: %s, want %s", rr.DeviceID, *reg.DeviceID)
-				}
-				if rr.AccessToken == "" {
-					t.Fatalf("missing accessToken in response")
+					if rr.AccessToken == "" {
+						t.Fatalf("missing accessToken in response")
+					}
+				default:
+					t.Fatalf("expected one of internalservererror, matrixerror, jsonresponse, registerresponse, got %T", resp.JSON)
 				}
 			})
 		}