From 27fcf8d30a8c8dda281739c84af4033c93d96faf Mon Sep 17 00:00:00 2001
From: Ethan Koenig <etk39@cornell.edu>
Date: Fri, 13 Jan 2017 21:14:48 -0500
Subject: [PATCH] Bug fixes for webhook API (#650)

---
 models/webhook.go           |  4 +++-
 routers/api/v1/api.go       | 44 ++++++++++++++++++++++++++++++++++---
 routers/api/v1/org/hook.go  |  6 ++++-
 routers/api/v1/repo/hook.go |  7 ++++--
 4 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/models/webhook.go b/models/webhook.go
index c9c3c21a4602..8f5c561939c2 100644
--- a/models/webhook.go
+++ b/models/webhook.go
@@ -258,8 +258,10 @@ func deleteWebhook(bean *Webhook) (err error) {
 		return err
 	}
 
-	if _, err = sess.Delete(bean); err != nil {
+	if count, err := sess.Delete(bean); err != nil {
 		return err
+	} else if count == 0 {
+		return ErrWebhookNotExist{ID: bean.ID}
 	} else if _, err = sess.Delete(&HookTask{HookID: bean.ID}); err != nil {
 		return err
 	}
diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go
index 69b9a272e0d9..c30db1a33acb 100644
--- a/routers/api/v1/api.go
+++ b/routers/api/v1/api.go
@@ -119,6 +119,44 @@ func reqRepoWriter() macaron.Handler {
 	}
 }
 
+func reqOrgMembership() macaron.Handler {
+	return func(ctx *context.APIContext) {
+		var orgID int64
+		if ctx.Org.Organization != nil {
+			orgID = ctx.Org.Organization.ID
+		} else if ctx.Org.Team != nil {
+			orgID = ctx.Org.Team.OrgID
+		} else {
+			ctx.Error(500, "", "reqOrgMembership: unprepared context")
+			return
+		}
+
+		if !models.IsOrganizationMember(orgID, ctx.User.ID) {
+			ctx.Error(403, "", "Must be an organization member")
+			return
+		}
+	}
+}
+
+func reqOrgOwnership() macaron.Handler {
+	return func(ctx *context.APIContext) {
+		var orgID int64
+		if ctx.Org.Organization != nil {
+			orgID = ctx.Org.Organization.ID
+		} else if ctx.Org.Team != nil {
+			orgID = ctx.Org.Team.OrgID
+		} else {
+			ctx.Error(500, "", "reqOrgOwnership: unprepared context")
+			return
+		}
+
+		if !models.IsOrganizationOwner(orgID, ctx.User.ID) {
+			ctx.Error(403, "", "Must be an organization member")
+			return
+		}
+	}
+}
+
 func orgAssignment(args ...bool) macaron.Handler {
 	var (
 		assignOrg  bool
@@ -362,9 +400,9 @@ func RegisterRoutes(m *macaron.Macaron) {
 				m.Combo("").Get(org.ListHooks).
 					Post(bind(api.CreateHookOption{}), org.CreateHook)
 				m.Combo("/:id").Get(org.GetHook).
-					Patch(bind(api.EditHookOption{}), org.EditHook).
-					Delete(org.DeleteHook)
-			})
+					Patch(reqOrgOwnership(), bind(api.EditHookOption{}), org.EditHook).
+					Delete(reqOrgOwnership(), org.DeleteHook)
+			}, reqOrgMembership())
 		}, orgAssignment(true))
 		m.Group("/teams/:teamid", func() {
 			m.Get("", org.GetTeam)
diff --git a/routers/api/v1/org/hook.go b/routers/api/v1/org/hook.go
index ebea13e56abb..3a0b74ac21d1 100644
--- a/routers/api/v1/org/hook.go
+++ b/routers/api/v1/org/hook.go
@@ -58,7 +58,11 @@ func DeleteHook(ctx *context.APIContext) {
 	org := ctx.Org.Organization
 	hookID := ctx.ParamsInt64(":id")
 	if err := models.DeleteWebhookByOrgID(org.ID, hookID); err != nil {
-		ctx.Error(500, "DeleteWebhookByOrgID", err)
+		if models.IsErrWebhookNotExist(err) {
+			ctx.Status(404)
+		} else {
+			ctx.Error(500, "DeleteWebhookByOrgID", err)
+		}
 		return
 	}
 	ctx.Status(204)
diff --git a/routers/api/v1/repo/hook.go b/routers/api/v1/repo/hook.go
index 51f64e345209..2e3b655a12b2 100644
--- a/routers/api/v1/repo/hook.go
+++ b/routers/api/v1/repo/hook.go
@@ -59,9 +59,12 @@ func EditHook(ctx *context.APIContext, form api.EditHookOption) {
 // DeleteHook delete a hook of a repository
 func DeleteHook(ctx *context.APIContext) {
 	if err := models.DeleteWebhookByRepoID(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id")); err != nil {
-		ctx.Error(500, "DeleteWebhookByRepoID", err)
+		if models.IsErrWebhookNotExist(err) {
+			ctx.Status(404)
+		} else {
+			ctx.Error(500, "DeleteWebhookByRepoID", err)
+		}
 		return
 	}
-
 	ctx.Status(204)
 }