From d1d2d16738a248846ea4367fe2b33485d56db6cd Mon Sep 17 00:00:00 2001 From: Till <2353100+S7evinK@users.noreply.github.com> Date: Thu, 22 Dec 2022 11:54:03 +0100 Subject: [PATCH] Fix reset password endpoint (#2921) Fixes the admin password reset endpoint. It was using a wrong variable, so could not detect the user. Adds some more checks to validate we can actually change the password. --- clientapi/admin_test.go | 134 ++++++++++++++++++++++++++++++ clientapi/clientapi.go | 5 +- clientapi/routing/admin.go | 38 +++++++-- clientapi/routing/password.go | 3 +- clientapi/routing/register.go | 24 +----- clientapi/routing/routing.go | 14 +++- docs/administration/4_adminapi.md | 7 +- internal/httputil/httpapi.go | 6 +- internal/validate.go | 44 ++++++++++ test/user.go | 4 +- 10 files changed, 237 insertions(+), 42 deletions(-) create mode 100644 clientapi/admin_test.go create mode 100644 internal/validate.go diff --git a/clientapi/admin_test.go b/clientapi/admin_test.go new file mode 100644 index 00000000..0d973f35 --- /dev/null +++ b/clientapi/admin_test.go @@ -0,0 +1,134 @@ +package clientapi + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/matrix-org/dendrite/clientapi/auth/authtypes" + "github.com/matrix-org/dendrite/keyserver" + "github.com/matrix-org/dendrite/roomserver" + "github.com/matrix-org/dendrite/setup/config" + "github.com/matrix-org/gomatrixserverlib" + "github.com/matrix-org/util" + "github.com/tidwall/gjson" + + "github.com/matrix-org/dendrite/test" + "github.com/matrix-org/dendrite/test/testrig" + "github.com/matrix-org/dendrite/userapi" + uapi "github.com/matrix-org/dendrite/userapi/api" +) + +func TestAdminResetPassword(t *testing.T) { + aliceAdmin := test.NewUser(t, test.WithAccountType(uapi.AccountTypeAdmin)) + bob := test.NewUser(t, test.WithAccountType(uapi.AccountTypeUser)) + vhUser := &test.User{ID: "@vhuser:vh1"} + + ctx := context.Background() + test.WithAllDatabases(t, func(t *testing.T, dbType test.DBType) { + base, baseClose := testrig.CreateBaseDendrite(t, dbType) + defer baseClose() + + // add a vhost + base.Cfg.Global.VirtualHosts = append(base.Cfg.Global.VirtualHosts, &config.VirtualHost{ + SigningIdentity: gomatrixserverlib.SigningIdentity{ServerName: "vh1"}, + }) + + rsAPI := roomserver.NewInternalAPI(base) + // Needed for changing the password/login + keyAPI := keyserver.NewInternalAPI(base, &base.Cfg.KeyServer, nil, rsAPI) + userAPI := userapi.NewInternalAPI(base, &base.Cfg.UserAPI, nil, keyAPI, rsAPI, nil) + keyAPI.SetUserAPI(userAPI) + // We mostly need the userAPI for this test, so nil for other APIs/caches etc. + AddPublicRoutes(base, nil, nil, nil, nil, nil, userAPI, nil, nil, nil) + + // Create the users in the userapi and login + accessTokens := map[*test.User]string{ + aliceAdmin: "", + bob: "", + vhUser: "", + } + for u := range accessTokens { + localpart, serverName, _ := gomatrixserverlib.SplitID('@', u.ID) + userRes := &uapi.PerformAccountCreationResponse{} + password := util.RandomString(8) + if err := userAPI.PerformAccountCreation(ctx, &uapi.PerformAccountCreationRequest{ + AccountType: u.AccountType, + Localpart: localpart, + ServerName: serverName, + Password: password, + }, userRes); err != nil { + t.Errorf("failed to create account: %s", err) + } + + req := test.NewRequest(t, http.MethodPost, "/_matrix/client/v3/login", test.WithJSONBody(t, map[string]interface{}{ + "type": authtypes.LoginTypePassword, + "identifier": map[string]interface{}{ + "type": "m.id.user", + "user": u.ID, + }, + "password": password, + })) + rec := httptest.NewRecorder() + base.PublicClientAPIMux.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("failed to login: %s", rec.Body.String()) + } + accessTokens[u] = gjson.GetBytes(rec.Body.Bytes(), "access_token").String() + } + + testCases := []struct { + name string + requestingUser *test.User + userID string + requestOpt test.HTTPRequestOpt + wantOK bool + withHeader bool + }{ + {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: aliceAdmin, wantOK: true, withHeader: true, userID: bob.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{ + "password": util.RandomString(8), + })}, + {name: "missing userID does not call function", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: ""}, // this 404s + {name: "rejects empty password", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: bob.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{ + "password": "", + })}, + {name: "rejects unknown server name", requestingUser: aliceAdmin, wantOK: false, withHeader: true, userID: "@doesnotexist:localhost", 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: "allows changing password for different vhost", requestingUser: aliceAdmin, wantOK: true, withHeader: true, userID: vhUser.ID, requestOpt: test.WithJSONBody(t, map[string]interface{}{ + "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), + })}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + req := test.NewRequest(t, http.MethodPost, "/_dendrite/admin/resetPassword/"+tc.userID) + if tc.requestOpt != nil { + req = test.NewRequest(t, http.MethodPost, "/_dendrite/admin/resetPassword/"+tc.userID, tc.requestOpt) + } + + if tc.withHeader { + req.Header.Set("Authorization", "Bearer "+accessTokens[tc.requestingUser]) + } + + 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()) + } + }) + } + }) +} diff --git a/clientapi/clientapi.go b/clientapi/clientapi.go index 080d4d9f..62ffa615 100644 --- a/clientapi/clientapi.go +++ b/clientapi/clientapi.go @@ -57,10 +57,7 @@ func AddPublicRoutes( } routing.Setup( - base.PublicClientAPIMux, - base.PublicWellKnownAPIMux, - base.SynapseAdminMux, - base.DendriteAdminMux, + base, cfg, rsAPI, asAPI, userAPI, userDirectoryProvider, federation, syncProducer, transactionsCache, fsAPI, keyAPI, diff --git a/clientapi/routing/admin.go b/clientapi/routing/admin.go index be8073c3..8419622d 100644 --- a/clientapi/routing/admin.go +++ b/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" @@ -98,20 +99,40 @@ func AdminEvacuateUser(req *http.Request, cfg *config.ClientAPI, device *userapi } func AdminResetPassword(req *http.Request, cfg *config.ClientAPI, device *userapi.Device, userAPI userapi.ClientUserAPI) util.JSONResponse { + if req.Body == nil { + return util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.Unknown("Missing request body"), + } + } vars, err := httputil.URLDecodeMapValues(mux.Vars(req)) if err != nil { return util.ErrorResponse(err) } - serverName := cfg.Matrix.ServerName - localpart, ok := vars["localpart"] - if !ok { + var localpart string + userID := vars["userID"] + localpart, serverName, err := cfg.Matrix.SplitLocalID('@', userID) + if err != nil { return util.JSONResponse{ Code: http.StatusBadRequest, - JSON: jsonerror.MissingArgument("Expecting user localpart."), + JSON: jsonerror.InvalidArgumentValue(err.Error()), } } - if l, s, err := cfg.Matrix.SplitLocalID('@', localpart); err == nil { - localpart, serverName = l, s + accAvailableResp := &userapi.QueryAccountAvailabilityResponse{} + if err = userAPI.QueryAccountAvailability(req.Context(), &userapi.QueryAccountAvailabilityRequest{ + Localpart: localpart, + ServerName: serverName, + }, accAvailableResp); err != nil { + return util.JSONResponse{ + Code: http.StatusInternalServerError, + JSON: jsonerror.InternalAPIError(req.Context(), err), + } + } + if accAvailableResp.Available { + return util.JSONResponse{ + Code: http.StatusNotFound, + JSON: jsonerror.Unknown("User does not exist"), + } } request := struct { Password string `json:"password"` @@ -128,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, diff --git a/clientapi/routing/password.go b/clientapi/routing/password.go index 9772f669..cd88b025 100644 --- a/clientapi/routing/password.go +++ b/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 } diff --git a/clientapi/routing/register.go b/clientapi/routing/register.go index 801000f6..4abbcdf9 100644 --- a/clientapi/routing/register.go +++ b/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" diff --git a/clientapi/routing/routing.go b/clientapi/routing/routing.go index a510761e..69b46214 100644 --- a/clientapi/routing/routing.go +++ b/clientapi/routing/routing.go @@ -20,6 +20,7 @@ import ( "strings" "github.com/gorilla/mux" + "github.com/matrix-org/dendrite/setup/base" "github.com/matrix-org/gomatrixserverlib" "github.com/matrix-org/util" "github.com/nats-io/nats.go" @@ -49,7 +50,7 @@ import ( // applied: // nolint: gocyclo func Setup( - publicAPIMux, wkMux, synapseAdminRouter, dendriteAdminRouter *mux.Router, + base *base.BaseDendrite, cfg *config.ClientAPI, rsAPI roomserverAPI.ClientRoomserverAPI, asAPI appserviceAPI.AppServiceInternalAPI, @@ -63,7 +64,14 @@ func Setup( extRoomsProvider api.ExtraPublicRoomsProvider, mscCfg *config.MSCs, natsClient *nats.Conn, ) { - prometheus.MustRegister(amtRegUsers, sendEventDuration) + publicAPIMux := base.PublicClientAPIMux + wkMux := base.PublicWellKnownAPIMux + synapseAdminRouter := base.SynapseAdminMux + dendriteAdminRouter := base.DendriteAdminMux + + if base.EnableMetrics { + prometheus.MustRegister(amtRegUsers, sendEventDuration) + } rateLimits := httputil.NewRateLimits(&cfg.RateLimiting) userInteractiveAuth := auth.NewUserInteractive(userAPI, cfg) @@ -631,7 +639,7 @@ func Setup( ).Methods(http.MethodGet, http.MethodPost, http.MethodOptions) v3mux.Handle("/auth/{authType}/fallback/web", - httputil.MakeHTMLAPI("auth_fallback", func(w http.ResponseWriter, req *http.Request) *util.JSONResponse { + httputil.MakeHTMLAPI("auth_fallback", base.EnableMetrics, func(w http.ResponseWriter, req *http.Request) *util.JSONResponse { vars := mux.Vars(req) return AuthFallback(w, req, vars["authType"], cfg) }), diff --git a/docs/administration/4_adminapi.md b/docs/administration/4_adminapi.md index 56e19a8b..c521cbc9 100644 --- a/docs/administration/4_adminapi.md +++ b/docs/administration/4_adminapi.md @@ -44,7 +44,9 @@ This endpoint will instruct Dendrite to part the given local `userID` in the URL all rooms which they are currently joined. A JSON body will be returned containing the room IDs of all affected rooms. -## POST `/_dendrite/admin/resetPassword/{localpart}` +## POST `/_dendrite/admin/resetPassword/{userID}` + +Reset the password of a local user. Request body format: @@ -54,9 +56,6 @@ Request body format: } ``` -Reset the password of a local user. The `localpart` is the username only, i.e. if -the full user ID is `@alice:domain.com` then the local part is `alice`. - ## GET `/_dendrite/admin/fulltext/reindex` This endpoint instructs Dendrite to reindex all searchable events (`m.room.message`, `m.room.topic` and `m.room.name`). An empty JSON body will be returned immediately. diff --git a/internal/httputil/httpapi.go b/internal/httputil/httpapi.go index 127d1fac..383913c6 100644 --- a/internal/httputil/httpapi.go +++ b/internal/httputil/httpapi.go @@ -198,7 +198,7 @@ func MakeExternalAPI(metricsName string, f func(*http.Request) util.JSONResponse // MakeHTMLAPI adds Span metrics to the HTML Handler function // This is used to serve HTML alongside JSON error messages -func MakeHTMLAPI(metricsName string, f func(http.ResponseWriter, *http.Request) *util.JSONResponse) http.Handler { +func MakeHTMLAPI(metricsName string, enableMetrics bool, f func(http.ResponseWriter, *http.Request) *util.JSONResponse) http.Handler { withSpan := func(w http.ResponseWriter, req *http.Request) { span := opentracing.StartSpan(metricsName) defer span.Finish() @@ -211,6 +211,10 @@ func MakeHTMLAPI(metricsName string, f func(http.ResponseWriter, *http.Request) } } + if !enableMetrics { + return http.HandlerFunc(withSpan) + } + return promhttp.InstrumentHandlerCounter( promauto.NewCounterVec( prometheus.CounterOpts{ diff --git a/internal/validate.go b/internal/validate.go new file mode 100644 index 00000000..fc685ad5 --- /dev/null +++ b/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 +} diff --git a/test/user.go b/test/user.go index 692eae35..95a8f83e 100644 --- a/test/user.go +++ b/test/user.go @@ -47,7 +47,7 @@ var ( type User struct { ID string - accountType api.AccountType + AccountType api.AccountType // key ID and private key of the server who has this user, if known. keyID gomatrixserverlib.KeyID privKey ed25519.PrivateKey @@ -66,7 +66,7 @@ func WithSigningServer(srvName gomatrixserverlib.ServerName, keyID gomatrixserve func WithAccountType(accountType api.AccountType) UserOpt { return func(u *User) { - u.accountType = accountType + u.AccountType = accountType } }