improve some permission check

models/repo_permission.go

@@ -71,6 +71,15 @@ func (p *Permission) CanWriteIssuesOrPulls(isPull bool) bool {
 	return p.CanWrite(UnitTypeIssues)
 }
 
+// CanReadIssuesOrPulls returns true if isPull is true and user could read pull requests and
+// returns true if isPull is false and user could read to issues
+func (p *Permission) CanReadIssuesOrPulls(isPull bool) bool {
+	if isPull {
+		return p.CanAccess(UnitTypePullRequests)
+	}
+	return p.CanAccess(UnitTypeIssues)
+}
+
 // GetUserRepoPermission returns the user permissions to the repository
 func GetUserRepoPermission(repo *Repository, user *User) (Permission, error) {
 	return getUserRepoPermission(x, repo, user)
@@ -96,7 +105,7 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss
 		return
 	}
 
-	// Admin has super access or user is the owner of the repository
+	// Admin or the owner has super access to the repository
 	if user.IsAdmin || user.ID == repo.OwnerID {
 		perm.AccessMode = AccessModeOwner
 		return

modules/context/permission.go

@@ -12,7 +12,7 @@ import (
 // RequireRepoAdmin returns a macaron middleware for requiring repository admin permission
 func RequireRepoAdmin() macaron.Handler {
 	return func(ctx *Context) {
-		if !ctx.IsSigned || (!ctx.Repo.IsAdmin() && !ctx.User.IsAdmin) {
+		if !ctx.IsSigned || !ctx.Repo.IsAdmin() {
 			ctx.NotFound(ctx.Req.RequestURI, nil)
 			return
 		}

routers/repo/issue.go

@@ -1039,6 +1039,11 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) {
 		return
 	}
 
+	if !ctx.IsSigned || (ctx.User.ID != issue.PosterID && !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull)) {
+		ctx.Error(403)
+		return
+	}
+
 	var attachments []string
 	if setting.AttachmentEnabled {
 		attachments = form.Files
@@ -1142,7 +1147,12 @@ func UpdateCommentContent(ctx *context.Context) {
 		return
 	}
 
-	if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin()) {
+	if err := comment.LoadIssue(); err != nil {
+		ctx.NotFoundOrServerError("LoadIssue", models.IsErrIssueNotExist, err)
+		return
+	}
+
+	if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
 		ctx.Error(403)
 		return
 	} else if comment.Type != models.CommentTypeComment && comment.Type != models.CommentTypeCode {
@@ -1176,7 +1186,12 @@ func DeleteComment(ctx *context.Context) {
 		return
 	}
 
-	if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin()) {
+	if err := comment.LoadIssue(); err != nil {
+		ctx.NotFoundOrServerError("LoadIssue", models.IsErrIssueNotExist, err)
+		return
+	}
+
+	if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
 		ctx.Error(403)
 		return
 	} else if comment.Type != models.CommentTypeComment && comment.Type != models.CommentTypeCode {
@@ -1419,6 +1434,11 @@ func ChangeIssueReaction(ctx *context.Context, form auth.ReactionForm) {
 		return
 	}
 
+	if !ctx.IsSigned || (ctx.User.ID != issue.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull)) {
+		ctx.Error(403)
+		return
+	}
+
 	if ctx.HasError() {
 		ctx.ServerError("ChangeIssueReaction", errors.New(ctx.GetErrMsg()))
 		return
@@ -1488,20 +1508,22 @@ func ChangeCommentReaction(ctx *context.Context, form auth.ReactionForm) {
 		return
 	}
 
-	issue, err := models.GetIssueByID(comment.IssueID)
-	checkIssueRights(ctx, issue)
-	if ctx.Written() {
+	if err := comment.LoadIssue(); err != nil {
+		ctx.NotFoundOrServerError("LoadIssue", models.IsErrIssueNotExist, err)
 		return
 	}
 
-	if ctx.HasError() {
-		ctx.ServerError("ChangeCommentReaction", errors.New(ctx.GetErrMsg()))
+	if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
+		ctx.Error(403)
+		return
+	} else if comment.Type != models.CommentTypeComment && comment.Type != models.CommentTypeCode {
+		ctx.Error(204)
 		return
 	}
 
 	switch ctx.Params(":action") {
 	case "react":
-		reaction, err := models.CreateCommentReaction(ctx.User, issue, comment, form.Content)
+		reaction, err := models.CreateCommentReaction(ctx.User, comment.Issue, comment, form.Content)
 		if err != nil {
 			log.Info("CreateCommentReaction: %s", err)
 			break
@@ -1513,9 +1535,9 @@ func ChangeCommentReaction(ctx *context.Context, form auth.ReactionForm) {
 			break
 		}
 
-		log.Trace("Reaction for comment created: %d/%d/%d/%d", ctx.Repo.Repository.ID, issue.ID, comment.ID, reaction.ID)
+		log.Trace("Reaction for comment created: %d/%d/%d/%d", ctx.Repo.Repository.ID, comment.Issue.ID, comment.ID, reaction.ID)
 	case "unreact":
-		if err := models.DeleteCommentReaction(ctx.User, issue, comment, form.Content); err != nil {
+		if err := models.DeleteCommentReaction(ctx.User, comment.Issue, comment, form.Content); err != nil {
 			ctx.ServerError("DeleteCommentReaction", err)
 			return
 		}
@@ -1527,7 +1549,7 @@ func ChangeCommentReaction(ctx *context.Context, form auth.ReactionForm) {
 			break
 		}
 
-		log.Trace("Reaction for comment removed: %d/%d/%d", ctx.Repo.Repository.ID, issue.ID, comment.ID)
+		log.Trace("Reaction for comment removed: %d/%d/%d", ctx.Repo.Repository.ID, comment.Issue.ID, comment.ID)
 	default:
 		ctx.NotFound(fmt.Sprintf("Unknown action %s", ctx.Params(":action")), nil)
 		return

routers/repo/issue_watch.go

@@ -14,23 +14,28 @@ import (
 )
 
 // IssueWatch sets issue watching
-func IssueWatch(c *context.Context) {
-	watch, err := strconv.ParseBool(c.Req.PostForm.Get("watch"))
-	if err != nil {
-		c.ServerError("watch is not bool", err)
+func IssueWatch(ctx *context.Context) {
+	issue := GetActionIssue(ctx)
+	if ctx.Written() {
 		return
 	}
 
-	issue := GetActionIssue(c)
-	if c.Written() {
+	if !ctx.IsSigned || (ctx.User.ID != issue.PosterID && !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull)) {
+		ctx.Error(403)
+		return
+	}
+
+	watch, err := strconv.ParseBool(ctx.Req.PostForm.Get("watch"))
+	if err != nil {
+		ctx.ServerError("watch is not bool", err)
 		return
 	}
 
-	if err := models.CreateOrUpdateIssueWatch(c.User.ID, issue.ID, watch); err != nil {
-		c.ServerError("CreateOrUpdateIssueWatch", err)
+	if err := models.CreateOrUpdateIssueWatch(ctx.User.ID, issue.ID, watch); err != nil {
+		ctx.ServerError("CreateOrUpdateIssueWatch", err)
 		return
 	}
 
-	url := fmt.Sprintf("%s/issues/%d", c.Repo.RepoLink, issue.Index)
-	c.Redirect(url, http.StatusSeeOther)
+	url := fmt.Sprintf("%s/issues/%d", ctx.Repo.RepoLink, issue.Index)
+	ctx.Redirect(url, http.StatusSeeOther)
 }

routers/routes/routes.go

@@ -562,7 +562,7 @@ func RegisterRoutes(m *macaron.Macaron) {
 			m.Post("", repo.UpdateCommentContent)
 			m.Post("/delete", repo.DeleteComment)
 			m.Post("/reactions/:action", bindIgnErr(auth.ReactionForm{}), repo.ChangeCommentReaction)
-		}, reqRepoIssuesOrPullsReader)
+		})
 		m.Group("/labels", func() {
 			m.Post("/new", bindIgnErr(auth.CreateLabelForm{}), repo.NewLabel)
 			m.Post("/edit", bindIgnErr(auth.CreateLabelForm{}), repo.UpdateLabel)
@@ -578,7 +578,7 @@ func RegisterRoutes(m *macaron.Macaron) {
 			m.Post("/delete", repo.DeleteMilestone)
 		}, reqRepoIssuesOrPullsWriter, context.RepoRef())
 
-		m.Combo("/compare/*", repo.MustAllowPulls, repo.SetEditorconfigIfExists).
+		m.Combo("/compare/*", reqRepoCodeReader, repo.MustAllowPulls, repo.SetEditorconfigIfExists).
 			Get(repo.SetDiffViewStyle, repo.CompareAndPullRequest).
 			Post(bindIgnErr(auth.CreateIssueForm{}), repo.CompareAndPullRequestPost)