From f25cce237e14af6041229c8248ded30d37b8cb51 Mon Sep 17 00:00:00 2001 From: CicadaCinema <52425971+CicadaCinema@users.noreply.github.com> Date: Wed, 22 Nov 2023 12:15:16 +0000 Subject: [PATCH] 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> --- clientapi/routing/register_test.go | 122 +++++++++++++++-------------- 1 file 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) } }) }