From fdbb6de3d52a5f41f075812e3b87616685a21b9b Mon Sep 17 00:00:00 2001 From: George Goldberg Date: Fri, 14 Sep 2018 16:21:05 +0100 Subject: MM-11520: Make entity ID checks consistent across api4. (#9395) * MM-11520: Make entity ID checks consistent across api4. * Update tests. --- api4/oauth.go | 6 ++++++ api4/post.go | 6 ++++++ api4/status.go | 6 ++++++ api4/status_test.go | 6 +++++- api4/team.go | 6 +++++- api4/team_test.go | 7 ++----- api4/user.go | 6 ++++++ api4/webhook.go | 45 ++++++++++++++++++++++++++++++++------------- 8 files changed, 68 insertions(+), 20 deletions(-) (limited to 'api4') diff --git a/api4/oauth.go b/api4/oauth.go index 990f292e9..2ed6641ee 100644 --- a/api4/oauth.go +++ b/api4/oauth.go @@ -91,6 +91,12 @@ func updateOAuthApp(c *Context, w http.ResponseWriter, r *http.Request) { return } + // The app being updated in the payload must be the same one as indicated in the URL. + if oauthApp.Id != c.Params.AppId { + c.SetInvalidParam("app_id") + return + } + c.LogAudit("attempt") oldOauthApp, err := c.App.GetOAuthApp(c.Params.AppId) diff --git a/api4/post.go b/api4/post.go index 014174b37..46c688014 100644 --- a/api4/post.go +++ b/api4/post.go @@ -382,6 +382,12 @@ func updatePost(c *Context, w http.ResponseWriter, r *http.Request) { return } + // The post being updated in the payload must be the same one as indicated in the URL. + if post.Id != c.Params.PostId { + c.SetInvalidParam("post_id") + return + } + if !c.App.SessionHasPermissionToChannelByPost(c.Session, c.Params.PostId, model.PERMISSION_EDIT_POST) { c.SetPermissionError(model.PERMISSION_EDIT_POST) return diff --git a/api4/status.go b/api4/status.go index edba5460b..67b4d10bc 100644 --- a/api4/status.go +++ b/api4/status.go @@ -68,6 +68,12 @@ func updateUserStatus(c *Context, w http.ResponseWriter, r *http.Request) { return } + // The user being updated in the payload must be the same one as indicated in the URL. + if status.UserId != c.Params.UserId { + c.SetInvalidParam("user_id") + return + } + if !c.App.SessionHasPermissionToUser(c.Session, c.Params.UserId) { c.SetPermissionError(model.PERMISSION_EDIT_OTHER_USERS) return diff --git a/api4/status_test.go b/api4/status_test.go index afff8526d..ddfc879ae 100644 --- a/api4/status_test.go +++ b/api4/status_test.go @@ -121,7 +121,7 @@ func TestUpdateUserStatus(t *testing.T) { defer th.TearDown() Client := th.Client - toUpdateUserStatus := &model.Status{Status: "online"} + toUpdateUserStatus := &model.Status{Status: "online", UserId: th.BasicUser.Id} updateUserStatus, resp := Client.UpdateUserStatus(th.BasicUser.Id, toUpdateUserStatus) CheckNoError(t, resp) if updateUserStatus.Status != "online" { @@ -150,6 +150,7 @@ func TestUpdateUserStatus(t *testing.T) { } toUpdateUserStatus.Status = "online" + toUpdateUserStatus.UserId = th.BasicUser2.Id _, resp = Client.UpdateUserStatus(th.BasicUser2.Id, toUpdateUserStatus) CheckForbiddenStatus(t, resp) @@ -159,6 +160,9 @@ func TestUpdateUserStatus(t *testing.T) { t.Fatal("Should return online status") } + _, resp = Client.UpdateUserStatus(th.BasicUser.Id, toUpdateUserStatus) + CheckBadRequestStatus(t, resp) + Client.Logout() _, resp = Client.UpdateUserStatus(th.BasicUser2.Id, toUpdateUserStatus) diff --git a/api4/team.go b/api4/team.go index 2c229dce1..5a387ee35 100644 --- a/api4/team.go +++ b/api4/team.go @@ -137,7 +137,11 @@ func updateTeam(c *Context, w http.ResponseWriter, r *http.Request) { return } - team.Id = c.Params.TeamId + // The team being updated in the payload must be the same one as indicated in the URL. + if team.Id != c.Params.TeamId { + c.SetInvalidParam("team_id") + return + } if !c.App.SessionHasPermissionToTeam(c.Session, c.Params.TeamId, model.PERMISSION_MANAGE_TEAM) { c.SetPermissionError(model.PERMISSION_MANAGE_TEAM) diff --git a/api4/team_test.go b/api4/team_test.go index 8304c979d..547b8d4a9 100644 --- a/api4/team_test.go +++ b/api4/team_test.go @@ -322,11 +322,8 @@ func TestUpdateTeam(t *testing.T) { originalTeamId := team.Id team.Id = model.NewId() - if r, err := Client.DoApiPut(Client.GetTeamRoute(originalTeamId), team.ToJson()); err != nil { - t.Fatal(err) - } else { - uteam = model.TeamFromJson(r.Body) - } + r, _ := Client.DoApiPut(Client.GetTeamRoute(originalTeamId), team.ToJson()) + assert.Equal(t, http.StatusBadRequest, r.StatusCode) if uteam.Id != originalTeamId { t.Fatal("wrong team id") diff --git a/api4/user.go b/api4/user.go index 8d4a264f8..5e97122d7 100644 --- a/api4/user.go +++ b/api4/user.go @@ -583,6 +583,12 @@ func updateUser(c *Context, w http.ResponseWriter, r *http.Request) { return } + // The user being updated in the payload must be the same one as indicated in the URL. + if user.Id != c.Params.UserId { + c.SetInvalidParam("user_id") + return + } + if !c.App.SessionHasPermissionToUser(c.Session, user.Id) { c.SetPermissionError(model.PERMISSION_EDIT_OTHER_USERS) return diff --git a/api4/webhook.go b/api4/webhook.go index 90d9a7dc8..05426e776 100644 --- a/api4/webhook.go +++ b/api4/webhook.go @@ -67,17 +67,21 @@ func updateIncomingHook(c *Context, w http.ResponseWriter, r *http.Request) { return } - hookId := c.Params.HookId - updatedHook := model.IncomingWebhookFromJson(r.Body) if updatedHook == nil { c.SetInvalidParam("incoming_webhook") return } + // The hook being updated in the payload must be the same one as indicated in the URL. + if updatedHook.Id != c.Params.HookId { + c.SetInvalidParam("hook_id") + return + } + c.LogAudit("attempt") - oldHook, err := c.App.GetIncomingWebhook(hookId) + oldHook, err := c.App.GetIncomingWebhook(c.Params.HookId) if err != nil { c.Err = err return @@ -247,34 +251,49 @@ func updateOutgoingHook(c *Context, w http.ResponseWriter, r *http.Request) { return } - toUpdateHook := model.OutgoingWebhookFromJson(r.Body) - if toUpdateHook == nil { + updatedHook := model.OutgoingWebhookFromJson(r.Body) + if updatedHook == nil { c.SetInvalidParam("outgoing_webhook") return } + // The hook being updated in the payload must be the same one as indicated in the URL. + if updatedHook.Id != c.Params.HookId { + c.SetInvalidParam("hook_id") + return + } + c.LogAudit("attempt") - toUpdateHook.CreatorId = c.Session.UserId + oldHook, err := c.App.GetOutgoingWebhook(c.Params.HookId) + if err != nil { + c.Err = err + return + } + + if updatedHook.TeamId == "" { + updatedHook.TeamId = oldHook.TeamId + } - if !c.App.SessionHasPermissionToTeam(c.Session, toUpdateHook.TeamId, model.PERMISSION_MANAGE_WEBHOOKS) { - c.SetPermissionError(model.PERMISSION_MANAGE_WEBHOOKS) + if updatedHook.TeamId != oldHook.TeamId { + c.Err = model.NewAppError("updateOutgoingHook", "api.webhook.team_mismatch.app_error", nil, "user_id="+c.Session.UserId, http.StatusBadRequest) return } - oldHook, err := c.App.GetOutgoingWebhook(toUpdateHook.Id) - if err != nil { - c.Err = err + if !c.App.SessionHasPermissionToTeam(c.Session, updatedHook.TeamId, model.PERMISSION_MANAGE_WEBHOOKS) { + c.SetPermissionError(model.PERMISSION_MANAGE_WEBHOOKS) return } - if c.Session.UserId != oldHook.CreatorId && !c.App.SessionHasPermissionToTeam(c.Session, oldHook.TeamId, model.PERMISSION_MANAGE_OTHERS_WEBHOOKS) { + if c.Session.UserId != oldHook.CreatorId && !c.App.SessionHasPermissionToTeam(c.Session, updatedHook.TeamId, model.PERMISSION_MANAGE_OTHERS_WEBHOOKS) { c.LogAudit("fail - inappropriate permissions") c.SetPermissionError(model.PERMISSION_MANAGE_OTHERS_WEBHOOKS) return } - rhook, err := c.App.UpdateOutgoingWebhook(oldHook, toUpdateHook) + updatedHook.CreatorId = c.Session.UserId + + rhook, err := c.App.UpdateOutgoingWebhook(oldHook, updatedHook) if err != nil { c.Err = err return -- cgit v1.2.3-1-g7c22