From 03350aa73f9defb78ead0a91299c010b58e2c4c6 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 9 Jul 2020 17:29:51 +0100 Subject: [PATCH] Make remaining sytest pass --- clientapi/auth/user_interactive.go | 40 ++++++++++++++++++++----- clientapi/auth/user_interactive_test.go | 6 ++-- clientapi/routing/device.go | 11 +++++-- sytest-whitelist | 3 ++ 4 files changed, 48 insertions(+), 12 deletions(-) diff --git a/clientapi/auth/user_interactive.go b/clientapi/auth/user_interactive.go index fa69afa90..581a85f0b 100644 --- a/clientapi/auth/user_interactive.go +++ b/clientapi/auth/user_interactive.go @@ -172,23 +172,48 @@ func (u *UserInteractive) NewSession() *util.JSONResponse { return u.Challenge(sessionID) } +// ResponseWithChallenge mixes together a JSON body (e.g an error with errcode/message) with the +// standard challenge response. +func (u *UserInteractive) ResponseWithChallenge(sessionID string, response interface{}) *util.JSONResponse { + mixedObjects := make(map[string]interface{}) + b, err := json.Marshal(response) + if err != nil { + ise := jsonerror.InternalServerError() + return &ise + } + _ = json.Unmarshal(b, &mixedObjects) + challenge := u.Challenge(sessionID) + b, err = json.Marshal(challenge.JSON) + if err != nil { + ise := jsonerror.InternalServerError() + return &ise + } + _ = json.Unmarshal(b, &mixedObjects) + + return &util.JSONResponse{ + Code: 401, + JSON: mixedObjects, + } +} + // Verify returns an error/challenge response to send to the client, or nil if the user is authenticated. // `bodyBytes` is the HTTP request body which must contain an `auth` key. -func (u *UserInteractive) Verify(ctx context.Context, bodyBytes []byte, device *api.Device) *util.JSONResponse { +// Returns the login that was verified for additional checks if required. +func (u *UserInteractive) Verify(ctx context.Context, bodyBytes []byte, device *api.Device) (*Login, *util.JSONResponse) { // TODO: rate limit // "A client should first make a request with no auth parameter. The homeserver returns an HTTP 401 response, with a JSON body" // https://matrix.org/docs/spec/client_server/r0.6.1#user-interactive-api-in-the-rest-api hasResponse := gjson.GetBytes(bodyBytes, "auth").Exists() if !hasResponse { - return u.NewSession() + return nil, u.NewSession() } // extract the type so we know which login type to use authType := gjson.GetBytes(bodyBytes, "auth.type").Str loginType, ok := u.Types[authType] if !ok { - return &util.JSONResponse{ + return nil, &util.JSONResponse{ Code: http.StatusBadRequest, JSON: jsonerror.BadJSON("unknown auth.type: " + authType), } @@ -199,7 +224,7 @@ func (u *UserInteractive) Verify(ctx context.Context, bodyBytes []byte, device * if _, ok = u.Sessions[sessionID]; !ok { // if the login type is part of a single stage flow then allow them to omit the session ID if !u.IsSingleStageFlow(authType) { - return &util.JSONResponse{ + return nil, &util.JSONResponse{ Code: http.StatusBadRequest, JSON: jsonerror.Unknown("missing or unknown auth.session"), } @@ -208,15 +233,16 @@ func (u *UserInteractive) Verify(ctx context.Context, bodyBytes []byte, device * r := loginType.Request() if err := json.Unmarshal([]byte(gjson.GetBytes(bodyBytes, "auth").Raw), r); err != nil { - return &util.JSONResponse{ + return nil, &util.JSONResponse{ Code: http.StatusBadRequest, JSON: jsonerror.BadJSON("The request body could not be decoded into valid JSON. " + err.Error()), } } - _, resErr := loginType.Login(ctx, r) + login, resErr := loginType.Login(ctx, r) if resErr == nil { u.AddCompletedStage(sessionID, authType) // TODO: Check if there's more stages to go and return an error + return login, nil } - return resErr + return nil, u.ResponseWithChallenge(sessionID, resErr.JSON) } diff --git a/clientapi/auth/user_interactive_test.go b/clientapi/auth/user_interactive_test.go index 3f80f95d9..d12652c08 100644 --- a/clientapi/auth/user_interactive_test.go +++ b/clientapi/auth/user_interactive_test.go @@ -41,7 +41,7 @@ func setup() *UserInteractive { func TestUserInteractiveChallenge(t *testing.T) { uia := setup() // no auth key results in a challenge - errRes := uia.Verify(ctx, []byte(`{}`), device) + _, errRes := uia.Verify(ctx, []byte(`{}`), device) if errRes == nil { t.Fatalf("Verify succeeded with {} but expected failure") } @@ -81,7 +81,7 @@ func TestUserInteractivePasswordLogin(t *testing.T) { }`), } for _, tc := range testCases { - errRes := uia.Verify(ctx, tc, device) + _, errRes := uia.Verify(ctx, tc, device) if errRes != nil { t.Errorf("Verify failed but expected success for request: %s - got %+v", string(tc), errRes) } @@ -162,7 +162,7 @@ func TestUserInteractivePasswordBadLogin(t *testing.T) { }, } for _, tc := range testCases { - errRes := uia.Verify(ctx, tc.body, device) + _, errRes := uia.Verify(ctx, tc.body, device) if errRes == nil { t.Errorf("Verify succeeded but expected failure for request: %s", string(tc.body)) continue diff --git a/clientapi/routing/device.go b/clientapi/routing/device.go index 9838fb69f..9d4d38905 100644 --- a/clientapi/routing/device.go +++ b/clientapi/routing/device.go @@ -177,7 +177,7 @@ func DeleteDeviceById( JSON: jsonerror.BadJSON("The request body could not be read: " + err.Error()), } } - errRes := userInteractiveAuth.Verify(ctx, bodyBytes, device) + login, errRes := userInteractiveAuth.Verify(ctx, bodyBytes, device) if errRes != nil { return *errRes } @@ -188,7 +188,14 @@ func DeleteDeviceById( return jsonerror.InternalServerError() } - defer req.Body.Close() // nolint: errcheck + // make sure that the access token being used matches the login creds used for user interactive auth, else + // 1 compromised access token could be used to logout all devices. + if login.Username() != localpart && login.Username() != device.UserID { + return util.JSONResponse{ + Code: 403, + JSON: jsonerror.Forbidden("Cannot delete another user's device"), + } + } if err := deviceDB.RemoveDevice(ctx, deviceID, localpart); err != nil { util.GetLogger(ctx).WithError(err).Error("deviceDB.RemoveDevice failed") diff --git a/sytest-whitelist b/sytest-whitelist index 8f87e0a68..2d606140c 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -34,6 +34,9 @@ PUT /device/{deviceId} gives a 404 for unknown devices GET /device/{deviceId} GET /devices PUT /device/{deviceId} updates device fields +DELETE /device/{deviceId} +DELETE /device/{deviceId} requires UI auth user to match device owner +DELETE /device/{deviceId} with no body gives a 401 POST /createRoom makes a public room POST /createRoom makes a private room POST /createRoom makes a private room with invites