PR comments; Move ValidatePassword; add more tests

clientapi/admin_test.go

@@ -21,7 +21,7 @@ import (
 )
 
 func TestAdminResetPassword(t *testing.T) {
-	alice := test.NewUser(t, test.WithAccountType(uapi.AccountTypeAdmin))
+	aliceAdmin := test.NewUser(t, test.WithAccountType(uapi.AccountTypeAdmin))
 	bob := test.NewUser(t, test.WithAccountType(uapi.AccountTypeUser))
 	vhUser := &test.User{ID: "@vhuser:vh1"}
 
@@ -45,9 +45,9 @@ func TestAdminResetPassword(t *testing.T) {
 
 		// Create the users in the userapi and login
 		accessTokens := map[*test.User]string{
-			alice:  "",
+			aliceAdmin: "",
-			bob:    "",
+			bob:        "",
-			vhUser: "",
+			vhUser:     "",
 		}
 		for u := range accessTokens {
 			localpart, serverName, _ := gomatrixserverlib.SplitID('@', u.ID)
@@ -88,20 +88,27 @@ func TestAdminResetPassword(t *testing.T) {
 		}{
 			{name: "Missing auth", requestingUser: bob, wantOK: false, userID: bob.ID},
 			{name: "Bob is denied access", requestingUser: bob, wantOK: false, withHeader: true, userID: bob.ID},
-			{name: "Alice is allowed access", requestingUser: alice, wantOK: true, withHeader: true, userID: bob.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{
+			{name: "Alice is allowed access", requestingUser: aliceAdmin, wantOK: true, withHeader: true, userID: bob.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{
-				"password": "newPass",
+				"password": util.RandomString(8),
 			})},
-			{name: "Alice is allowed access, missing userID", requestingUser: alice, wantOK: false, withHeader: true, userID: ""}, // this 404s
+			{name: "missing userID does not call function", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: ""}, // this 404s
-			{name: "Alice is allowed access, empty password", requestingUser: alice, wantOK: false, withHeader: true, userID: bob.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{
+			{name: "rejects empty password", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: bob.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{
 				"password": "",
 			})},
-			{name: "Alice is allowed access, unknown server name", requestingUser: alice, wantOK: false, withHeader: true, userID: "@doesnotexist:localhost", requestOpt: test.WithJSONBody(t, map[string]interface{}{})},
+			{name: "rejects unknown server name", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: "@doesnotexist:localhost", requestOpt: test.WithJSONBody(t, map[string]interface{}{})},
-			{name: "Alice is allowed access, unknown user", requestingUser: alice, wantOK: false, withHeader: true, userID: "@doesnotexist:test", requestOpt: test.WithJSONBody(t, map[string]interface{}{})},
+			{name: "rejects unknown user", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: "@doesnotexist:test", requestOpt: test.WithJSONBody(t, map[string]interface{}{})},
-			{name: "Alice is allowed access, different vhost", requestingUser: alice, wantOK: true, withHeader: true, userID: vhUser.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{
+			{name: "allows changing password for different vhost", requestingUser: aliceAdmin, wantOK: true, withHeader: true, userID: vhUser.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{
-				"password": "newPass",
+				"password": util.RandomString(8),
+			})},
+			{name: "rejects existing user, missing body", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: bob.ID},
+			{name: "rejects invalid userID", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: "!notauserid:test", requestOpt: test.WithJSONBody(t, map[string]interface{}{})},
+			{name: "rejects invalid json", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: bob.ID, requestOpt: test.WithJSONBody(t, `{invalidJSON}`)},
+			{name: "rejects too weak password", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: bob.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{
+				"password": util.RandomString(6),
+			})},
+			{name: "rejects too long password", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: bob.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{
+				"password": util.RandomString(513),
 			})},
-			{name: "Alice is allowed access, existing user, missing body", requestingUser: alice, wantOK: false, withHeader: true, userID: bob.ID},
-			{name: "Alice is allowed access, invalid userID", requestingUser: alice, wantOK: false, withHeader: true, userID: "!notauserid:test", requestOpt: test.WithJSONBody(t, map[string]interface{}{})},
 		}
 
 		for _, tc := range testCases {
@@ -117,6 +124,7 @@ func TestAdminResetPassword(t *testing.T) {
 
 				rec := httptest.NewRecorder()
 				base.DendriteAdminMux.ServeHTTP(rec, req)
+				t.Logf("%s", rec.Body.String())
 				if tc.wantOK && rec.Code != http.StatusOK {
 					t.Fatalf("expected http status %d, got %d: %s", http.StatusOK, rec.Code, rec.Body.String())
 				}

clientapi/routing/admin.go

@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/gorilla/mux"
+	"github.com/matrix-org/dendrite/internal"
 	"github.com/matrix-org/gomatrixserverlib"
 	"github.com/matrix-org/util"
 	"github.com/nats-io/nats.go"
@@ -114,7 +115,7 @@ func AdminResetPassword(req *http.Request, cfg *config.ClientAPI, device *userap
 	if err != nil {
 		return util.JSONResponse{
 			Code: http.StatusBadRequest,
-			JSON: jsonerror.BadJSON(err.Error()),
+			JSON: jsonerror.InvalidArgumentValue(err.Error()),
 		}
 	}
 	accAvailableResp := &userapi.QueryAccountAvailabilityResponse{}
@@ -148,6 +149,11 @@ func AdminResetPassword(req *http.Request, cfg *config.ClientAPI, device *userap
 			JSON: jsonerror.MissingArgument("Expecting non-empty password."),
 		}
 	}
+
+	if resErr := internal.ValidatePassword(request.Password); resErr != nil {
+		return *resErr
+	}
+
 	updateReq := &userapi.PerformPasswordUpdateRequest{
 		Localpart:     localpart,
 		ServerName:    serverName,

clientapi/routing/password.go

@@ -7,6 +7,7 @@ import (
 	"github.com/matrix-org/dendrite/clientapi/auth/authtypes"
 	"github.com/matrix-org/dendrite/clientapi/httputil"
 	"github.com/matrix-org/dendrite/clientapi/jsonerror"
+	"github.com/matrix-org/dendrite/internal"
 	"github.com/matrix-org/dendrite/setup/config"
 	"github.com/matrix-org/dendrite/userapi/api"
 	"github.com/matrix-org/gomatrixserverlib"
@@ -81,7 +82,7 @@ func Password(
 	sessions.addCompletedSessionStage(sessionID, authtypes.LoginTypePassword)
 
 	// Check the new password strength.
-	if resErr = validatePassword(r.NewPassword); resErr != nil {
+	if resErr = internal.ValidatePassword(r.NewPassword); resErr != nil {
 		return *resErr
 	}
 

clientapi/routing/register.go

@@ -30,6 +30,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/matrix-org/dendrite/internal"
 	"github.com/tidwall/gjson"
 
 	"github.com/matrix-org/dendrite/internal/eventutil"
@@ -60,8 +61,6 @@ var (
 )
 
 const (
-	minPasswordLength = 8   // http://matrix.org/docs/spec/client_server/r0.2.0.html#password-based
-	maxPasswordLength = 512 // https://github.com/matrix-org/synapse/blob/v0.20.0/synapse/rest/client/v2_alpha/register.py#L161
 	maxUsernameLength = 254 // http://matrix.org/speculator/spec/HEAD/intro.html#user-identifiers TODO account for domain
 	sessionIDLength   = 24
 )
@@ -315,23 +314,6 @@ func validateApplicationServiceUsername(localpart string, domain gomatrixserverl
 	return nil
 }
 
-// validatePassword returns an error response if the password is invalid
-func validatePassword(password string) *util.JSONResponse {
-	// https://github.com/matrix-org/synapse/blob/v0.20.0/synapse/rest/client/v2_alpha/register.py#L161
-	if len(password) > maxPasswordLength {
-		return &util.JSONResponse{
-			Code: http.StatusBadRequest,
-			JSON: jsonerror.BadJSON(fmt.Sprintf("'password' >%d characters", maxPasswordLength)),
-		}
-	} else if len(password) > 0 && len(password) < minPasswordLength {
-		return &util.JSONResponse{
-			Code: http.StatusBadRequest,
-			JSON: jsonerror.WeakPassword(fmt.Sprintf("password too weak: min %d chars", minPasswordLength)),
-		}
-	}
-	return nil
-}
-
 // validateRecaptcha returns an error response if the captcha response is invalid
 func validateRecaptcha(
 	cfg *config.ClientAPI,
@@ -636,7 +618,7 @@ func Register(
 			return *resErr
 		}
 	}
-	if resErr := validatePassword(r.Password); resErr != nil {
+	if resErr := internal.ValidatePassword(r.Password); resErr != nil {
 		return *resErr
 	}
 
@@ -1138,7 +1120,7 @@ func handleSharedSecretRegistration(cfg *config.ClientAPI, userAPI userapi.Clien
 	if resErr := validateUsername(ssrr.User, cfg.Matrix.ServerName); resErr != nil {
 		return *resErr
 	}
-	if resErr := validatePassword(ssrr.Password); resErr != nil {
+	if resErr := internal.ValidatePassword(ssrr.Password); resErr != nil {
 		return *resErr
 	}
 	deviceID := "shared_secret_registration"

internal/validate.go

@@ -0,0 +1,44 @@
+// Copyright 2022 The Matrix.org Foundation C.I.C.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package internal
+
+import (
+	"fmt"
+	"net/http"
+
+	"github.com/matrix-org/dendrite/clientapi/jsonerror"
+	"github.com/matrix-org/util"
+)
+
+const minPasswordLength = 8 // http://matrix.org/docs/spec/client_server/r0.2.0.html#password-based
+
+const maxPasswordLength = 512 // https://github.com/matrix-org/synapse/blob/v0.20.0/synapse/rest/client/v2_alpha/register.py#L161
+
+// ValidatePassword returns an error response if the password is invalid
+func ValidatePassword(password string) *util.JSONResponse {
+	// https://github.com/matrix-org/synapse/blob/v0.20.0/synapse/rest/client/v2_alpha/register.py#L161
+	if len(password) > maxPasswordLength {
+		return &util.JSONResponse{
+			Code: http.StatusBadRequest,
+			JSON: jsonerror.BadJSON(fmt.Sprintf("password too long: max %d characters", maxPasswordLength)),
+		}
+	} else if len(password) > 0 && len(password) < minPasswordLength {
+		return &util.JSONResponse{
+			Code: http.StatusBadRequest,
+			JSON: jsonerror.WeakPassword(fmt.Sprintf("password too weak: min %d chars", minPasswordLength)),
+		}
+	}
+	return nil
+}