fix: check owner when changing state of project

It was sufficiently checked for the repostiory case, but for user/org
project it was not checked and you could change the state of any
project by there mere knowledge of a ID.

models/project/project.go

@@ -307,6 +307,18 @@ func GetProjectForRepoByID(ctx context.Context, repoID, id int64) (*Project, err
 	return p, nil
 }
 
+// GetProjectForUserByID returns the project by id that belongs to the specified user.
+func GetProjectForUserByID(ctx context.Context, uid, id int64) (*Project, error) {
+	p := new(Project)
+	has, err := db.GetEngine(ctx).Where("id=? AND owner_id=?", id, uid).Get(p)
+	if err != nil {
+		return nil, err
+	} else if !has {
+		return nil, ErrProjectNotExist{ID: id}
+	}
+	return p, nil
+}
+
 // UpdateProject updates project properties
 func UpdateProject(ctx context.Context, p *Project) error {
 	if !IsCardTypeValid(p.CardType) {
@@ -344,42 +356,26 @@ func updateRepositoryProjectCount(ctx context.Context, repoID int64) error {
 	return nil
 }
 
-// ChangeProjectStatusByRepoIDAndID toggles a project between opened and closed
+// ChangeProjectStatus changes the status of the specified project to the state
-func ChangeProjectStatusByRepoIDAndID(ctx context.Context, repoID, projectID int64, isClosed bool) error {
+// specified via the `isClosed` argument.
-	ctx, committer, err := db.TxContext(ctx)
+func ChangeProjectStatus(ctx context.Context, p *Project, isClosed bool) error {
-	if err != nil {
+	if p.IsClosed == isClosed {
-		return err
-	}
-	defer committer.Close()
-
-	p := new(Project)
-
-	has, err := db.GetEngine(ctx).ID(projectID).Where("repo_id = ?", repoID).Get(p)
-	if err != nil {
-		return err
-	} else if !has {
-		return ErrProjectNotExist{ID: projectID, RepoID: repoID}
-	}
-
-	if err := changeProjectStatus(ctx, p, isClosed); err != nil {
-		return err
-	}
-
-	return committer.Commit()
-}
-
-func changeProjectStatus(ctx context.Context, p *Project, isClosed bool) error {
-	p.IsClosed = isClosed
-	p.ClosedDateUnix = timeutil.TimeStampNow()
-	count, err := db.GetEngine(ctx).ID(p.ID).Where("repo_id = ? AND is_closed = ?", p.RepoID, !isClosed).Cols("is_closed", "closed_date_unix").Update(p)
-	if err != nil {
-		return err
-	}
-	if count < 1 {
 		return nil
 	}
 
-	return updateRepositoryProjectCount(ctx, p.RepoID)
+	return db.WithTx(ctx, func(ctx context.Context) error {
+		p.IsClosed = isClosed
+		p.ClosedDateUnix = timeutil.TimeStampNow()
+		count, err := db.GetEngine(ctx).ID(p.ID).Cols("is_closed", "closed_date_unix").Update(p)
+		if err != nil {
+			return err
+		}
+		if count < 1 {
+			return nil
+		}
+
+		return updateRepositoryProjectCount(ctx, p.RepoID)
+	})
 }
 
 // DeleteProjectByID deletes a project from a repository. if it's not in a database

models/project/project_test.go

@@ -8,7 +8,6 @@ import (
 
 	"forgejo.org/models/db"
 	"forgejo.org/models/unittest"
-	"forgejo.org/modules/timeutil"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -48,42 +47,6 @@ func TestGetProjects(t *testing.T) {
 	assert.Len(t, projects, 1)
 }
 
-func TestProject(t *testing.T) {
-	require.NoError(t, unittest.PrepareTestDatabase())
-
-	project := &Project{
-		Type:         TypeRepository,
-		TemplateType: TemplateTypeBasicKanban,
-		CardType:     CardTypeTextOnly,
-		Title:        "New Project",
-		RepoID:       1,
-		CreatedUnix:  timeutil.TimeStampNow(),
-		CreatorID:    2,
-	}
-
-	require.NoError(t, NewProject(db.DefaultContext, project))
-
-	_, err := GetProjectByID(db.DefaultContext, project.ID)
-	require.NoError(t, err)
-
-	// Update project
-	project.Title = "Updated title"
-	require.NoError(t, UpdateProject(db.DefaultContext, project))
-
-	projectFromDB, err := GetProjectByID(db.DefaultContext, project.ID)
-	require.NoError(t, err)
-
-	assert.Equal(t, project.Title, projectFromDB.Title)
-
-	require.NoError(t, ChangeProjectStatusByRepoIDAndID(db.DefaultContext, project.RepoID, project.ID, true))
-
-	// Retrieve from DB afresh to check if it is truly closed
-	projectFromDB, err = GetProjectByID(db.DefaultContext, project.ID)
-	require.NoError(t, err)
-
-	assert.True(t, projectFromDB.IsClosed)
-}
-
 func TestProjectsSort(t *testing.T) {
 	require.NoError(t, unittest.PrepareTestDatabase())
 

routers/web/org/projects.go

@@ -218,8 +218,13 @@ func ChangeProjectStatus(ctx *context.Context) {
 	}
 	id := ctx.ParamsInt64(":id")
 
-	if err := project_model.ChangeProjectStatusByRepoIDAndID(ctx, 0, id, toClose); err != nil {
+	project, err := project_model.GetProjectForUserByID(ctx, ctx.ContextUser.ID, id)
-		ctx.NotFoundOrServerError("ChangeProjectStatusByRepoIDAndID", project_model.IsErrProjectNotExist, err)
+	if err != nil {
+		ctx.NotFoundOrServerError("GetProjectForUserByID", project_model.IsErrProjectNotExist, err)
+		return
+	}
+	if err := project_model.ChangeProjectStatus(ctx, project, toClose); err != nil {
+		ctx.ServerError("ChangeProjectStatus", err)
 		return
 	}
 	ctx.JSONRedirect(project_model.ProjectLinkForOrg(ctx.ContextUser, id))

routers/web/repo/projects.go

@@ -192,8 +192,13 @@ func ChangeProjectStatus(ctx *context.Context) {
 	}
 	id := ctx.ParamsInt64(":id")
 
-	if err := project_model.ChangeProjectStatusByRepoIDAndID(ctx, ctx.Repo.Repository.ID, id, toClose); err != nil {
+	project, err := project_model.GetProjectForRepoByID(ctx, ctx.Repo.Repository.ID, id)
-		ctx.NotFoundOrServerError("ChangeProjectStatusByRepoIDAndID", project_model.IsErrProjectNotExist, err)
+	if err != nil {
+		ctx.NotFoundOrServerError("GetProjectForRepoByID", project_model.IsErrProjectNotExist, err)
+		return
+	}
+	if err := project_model.ChangeProjectStatus(ctx, project, toClose); err != nil {
+		ctx.ServerError("ChangeProjectStatus", err)
 		return
 	}
 	ctx.JSONRedirect(project_model.ProjectLinkForRepo(ctx.Repo.Repository, id))