Fix permission check on org project operations (#36318)

models/project/column.go

@@ -213,6 +213,18 @@ func GetColumn(ctx context.Context, columnID int64) (*Column, error) {
 	return column, nil
 }
 
+func GetColumnByIDAndProjectID(ctx context.Context, columnID, projectID int64) (*Column, error) {
+	column := new(Column)
+	has, err := db.GetEngine(ctx).ID(columnID).And("project_id=?", projectID).Get(column)
+	if err != nil {
+		return nil, err
+	} else if !has {
+		return nil, ErrProjectColumnNotExist{ColumnID: columnID}
+	}
+
+	return column, nil
+}
+
 // UpdateColumn updates a project column
 func UpdateColumn(ctx context.Context, column *Column) error {
 	var fieldToUpdate []string

models/project/project.go

@@ -302,6 +302,18 @@ func GetProjectByID(ctx context.Context, id int64) (*Project, error) {
 	return p, nil
 }
 
+func GetProjectByIDAndOwner(ctx context.Context, id, ownerID int64) (*Project, error) {
+	p := new(Project)
+	has, err := db.GetEngine(ctx).ID(id).And("owner_id = ?", ownerID).Get(p)
+	if err != nil {
+		return nil, err
+	} else if !has {
+		return nil, ErrProjectNotExist{ID: id}
+	}
+
+	return p, nil
+}
+
 // GetProjectForRepoByID returns the projects in a repository
 func GetProjectForRepoByID(ctx context.Context, repoID, id int64) (*Project, error) {
 	p := new(Project)

routers/web/org/projects.go

@@ -205,22 +205,24 @@ func ChangeProjectStatus(ctx *context.Context) {
 	}
 	id := ctx.PathParamInt64("id")
 
-	if err := project_model.ChangeProjectStatusByRepoIDAndID(ctx, 0, id, toClose); err != nil {
+	project, err := project_model.GetProjectByIDAndOwner(ctx, id, ctx.ContextUser.ID)
-		ctx.NotFoundOrServerError("ChangeProjectStatusByRepoIDAndID", project_model.IsErrProjectNotExist, err)
-		return
-	}
-	ctx.JSONRedirect(project_model.ProjectLinkForOrg(ctx.ContextUser, id))
-}
-
-// DeleteProject delete a project
-func DeleteProject(ctx *context.Context) {
-	p, err := project_model.GetProjectByID(ctx, ctx.PathParamInt64("id"))
 	if err != nil {
 		ctx.NotFoundOrServerError("GetProjectByID", project_model.IsErrProjectNotExist, err)
 		return
 	}
-	if p.OwnerID != ctx.ContextUser.ID {
+
-		ctx.NotFound(nil)
+	if err := project_model.ChangeProjectStatusByRepoIDAndID(ctx, 0, project.ID, toClose); err != nil {
+		ctx.NotFoundOrServerError("ChangeProjectStatusByRepoIDAndID", project_model.IsErrProjectNotExist, err)
+		return
+	}
+	ctx.JSONRedirect(project_model.ProjectLinkForOrg(ctx.ContextUser, project.ID))
+}
+
+// DeleteProject delete a project
+func DeleteProject(ctx *context.Context) {
+	p, err := project_model.GetProjectByIDAndOwner(ctx, ctx.PathParamInt64("id"), ctx.ContextUser.ID)
+	if err != nil {
+		ctx.NotFoundOrServerError("GetProjectByID", project_model.IsErrProjectNotExist, err)
 		return
 	}
 
@@ -246,15 +248,11 @@ func RenderEditProject(ctx *context.Context) {
 		return
 	}
 
-	p, err := project_model.GetProjectByID(ctx, ctx.PathParamInt64("id"))
+	p, err := project_model.GetProjectByIDAndOwner(ctx, ctx.PathParamInt64("id"), ctx.ContextUser.ID)
 	if err != nil {
 		ctx.NotFoundOrServerError("GetProjectByID", project_model.IsErrProjectNotExist, err)
 		return
 	}
-	if p.OwnerID != ctx.ContextUser.ID {
-		ctx.NotFound(nil)
-		return
-	}
 
 	ctx.Data["projectID"] = p.ID
 	ctx.Data["title"] = p.Title
@@ -288,15 +286,11 @@ func EditProjectPost(ctx *context.Context) {
 		return
 	}
 
-	p, err := project_model.GetProjectByID(ctx, projectID)
+	p, err := project_model.GetProjectByIDAndOwner(ctx, projectID, ctx.ContextUser.ID)
 	if err != nil {
 		ctx.NotFoundOrServerError("GetProjectByID", project_model.IsErrProjectNotExist, err)
 		return
 	}
-	if p.OwnerID != ctx.ContextUser.ID {
-		ctx.NotFound(nil)
-		return
-	}
 
 	p.Title = form.Title
 	p.Description = form.Content
@@ -316,15 +310,12 @@ func EditProjectPost(ctx *context.Context) {
 
 // ViewProject renders the project with board view for a project
 func ViewProject(ctx *context.Context) {
-	project, err := project_model.GetProjectByID(ctx, ctx.PathParamInt64("id"))
+	project, err := project_model.GetProjectByIDAndOwner(ctx, ctx.PathParamInt64("id"), ctx.ContextUser.ID)
 	if err != nil {
 		ctx.NotFoundOrServerError("GetProjectByID", project_model.IsErrProjectNotExist, err)
 		return
 	}
-	if project.OwnerID != ctx.ContextUser.ID {
+
-		ctx.NotFound(nil)
-		return
-	}
 	if err := project.LoadOwner(ctx); err != nil {
 		ctx.ServerError("LoadOwner", err)
 		return
@@ -455,28 +446,15 @@ func DeleteProjectColumn(ctx *context.Context) {
 		return
 	}
 
-	project, err := project_model.GetProjectByID(ctx, ctx.PathParamInt64("id"))
+	project, err := project_model.GetProjectByIDAndOwner(ctx, ctx.PathParamInt64("id"), ctx.ContextUser.ID)
 	if err != nil {
 		ctx.NotFoundOrServerError("GetProjectByID", project_model.IsErrProjectNotExist, err)
 		return
 	}
 
-	pb, err := project_model.GetColumn(ctx, ctx.PathParamInt64("columnID"))
+	_, err = project_model.GetColumnByIDAndProjectID(ctx, ctx.PathParamInt64("columnID"), project.ID)
 	if err != nil {
-		ctx.ServerError("GetProjectColumn", err)
+		ctx.NotFoundOrServerError("GetColumnByIDAndProjectID", project_model.IsErrProjectColumnNotExist, err)
-		return
-	}
-	if pb.ProjectID != ctx.PathParamInt64("id") {
-		ctx.JSON(http.StatusUnprocessableEntity, map[string]string{
-			"message": fmt.Sprintf("ProjectColumn[%d] is not in Project[%d] as expected", pb.ID, project.ID),
-		})
-		return
-	}
-
-	if project.OwnerID != ctx.ContextUser.ID {
-		ctx.JSON(http.StatusUnprocessableEntity, map[string]string{
-			"message": fmt.Sprintf("ProjectColumn[%d] is not in Owner[%d] as expected", pb.ID, ctx.ContextUser.ID),
-		})
 		return
 	}
 
@@ -492,7 +470,7 @@ func DeleteProjectColumn(ctx *context.Context) {
 func AddColumnToProjectPost(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.EditProjectColumnForm)
 
-	project, err := project_model.GetProjectByID(ctx, ctx.PathParamInt64("id"))
+	project, err := project_model.GetProjectByIDAndOwner(ctx, ctx.PathParamInt64("id"), ctx.ContextUser.ID)
 	if err != nil {
 		ctx.NotFoundOrServerError("GetProjectByID", project_model.IsErrProjectNotExist, err)
 		return
@@ -520,30 +498,18 @@ func CheckProjectColumnChangePermissions(ctx *context.Context) (*project_model.P
 		return nil, nil
 	}
 
-	project, err := project_model.GetProjectByID(ctx, ctx.PathParamInt64("id"))
+	project, err := project_model.GetProjectByIDAndOwner(ctx, ctx.PathParamInt64("id"), ctx.ContextUser.ID)
 	if err != nil {
 		ctx.NotFoundOrServerError("GetProjectByID", project_model.IsErrProjectNotExist, err)
 		return nil, nil
 	}
 
-	column, err := project_model.GetColumn(ctx, ctx.PathParamInt64("columnID"))
+	column, err := project_model.GetColumnByIDAndProjectID(ctx, ctx.PathParamInt64("columnID"), project.ID)
 	if err != nil {
-		ctx.ServerError("GetProjectColumn", err)
+		ctx.NotFoundOrServerError("GetColumnByIDAndProjectID", project_model.IsErrProjectColumnNotExist, err)
-		return nil, nil
-	}
-	if column.ProjectID != ctx.PathParamInt64("id") {
-		ctx.JSON(http.StatusUnprocessableEntity, map[string]string{
-			"message": fmt.Sprintf("ProjectColumn[%d] is not in Project[%d] as expected", column.ID, project.ID),
-		})
 		return nil, nil
 	}
 
-	if project.OwnerID != ctx.ContextUser.ID {
-		ctx.JSON(http.StatusUnprocessableEntity, map[string]string{
-			"message": fmt.Sprintf("ProjectColumn[%d] is not in Repository[%d] as expected", column.ID, project.ID),
-		})
-		return nil, nil
-	}
 	return project, column
 }
 
@@ -595,24 +561,15 @@ func MoveIssues(ctx *context.Context) {
 		return
 	}
 
-	project, err := project_model.GetProjectByID(ctx, ctx.PathParamInt64("id"))
+	project, err := project_model.GetProjectByIDAndOwner(ctx, ctx.PathParamInt64("id"), ctx.ContextUser.ID)
 	if err != nil {
 		ctx.NotFoundOrServerError("GetProjectByID", project_model.IsErrProjectNotExist, err)
 		return
 	}
-	if project.OwnerID != ctx.ContextUser.ID {
-		ctx.NotFound(nil)
-		return
-	}
 
-	column, err := project_model.GetColumn(ctx, ctx.PathParamInt64("columnID"))
+	column, err := project_model.GetColumnByIDAndProjectID(ctx, ctx.PathParamInt64("columnID"), project.ID)
 	if err != nil {
-		ctx.NotFoundOrServerError("GetProjectColumn", project_model.IsErrProjectColumnNotExist, err)
+		ctx.NotFoundOrServerError("GetColumnByIDAndProjectID", project_model.IsErrProjectColumnNotExist, err)
-		return
-	}
-
-	if column.ProjectID != project.ID {
-		ctx.NotFound(nil)
 		return
 	}
 

routers/web/org/projects_test.go

@@ -4,11 +4,14 @@
 package org_test
 
 import (
+	"net/http"
 	"testing"
 
 	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/web/org"
 	"code.gitea.io/gitea/services/contexttest"
+	"code.gitea.io/gitea/services/forms"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -26,3 +29,30 @@ func TestCheckProjectColumnChangePermissions(t *testing.T) {
 	assert.NotNil(t, column)
 	assert.False(t, ctx.Written())
 }
+
+func TestChangeProjectStatusRejectsForeignProjects(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+	// project 4 is owned by user2 not user1
+	ctx, _ := contexttest.MockContext(t, "user1/-/projects/4/close")
+	contexttest.LoadUser(t, ctx, 1)
+	ctx.ContextUser = ctx.Doer
+	ctx.SetPathParam("action", "close")
+	ctx.SetPathParam("id", "4")
+
+	org.ChangeProjectStatus(ctx)
+
+	assert.Equal(t, http.StatusNotFound, ctx.Resp.WrittenStatus())
+}
+
+func TestAddColumnToProjectPostRejectsForeignProjects(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+	ctx, _ := contexttest.MockContext(t, "user1/-/projects/4/columns/new")
+	contexttest.LoadUser(t, ctx, 1)
+	ctx.ContextUser = ctx.Doer
+	ctx.SetPathParam("id", "4")
+	web.SetForm(ctx, &forms.EditProjectColumnForm{Title: "foreign"})
+
+	org.AddColumnToProjectPost(ctx)
+
+	assert.Equal(t, http.StatusNotFound, ctx.Resp.WrittenStatus())
+}