Fix comment permissions (#28213) (#28216)

backport #28213

This PR will fix some missed checks for private repositories' data on
web routes and API routes.

Author: lunny

models/asymkey/gpg_key.go

@@ -92,10 +92,9 @@ func CountUserGPGKeys(ctx context.Context, userID int64) (int64, error) {
 	return db.GetEngine(ctx).Where("owner_id=? AND primary_key_id=''", userID).Count(&GPGKey{})
 }
 
-// GetGPGKeyByID returns public key by given ID.
-func GetGPGKeyByID(ctx context.Context, keyID int64) (*GPGKey, error) {
+func GetGPGKeyForUserByID(ctx context.Context, ownerID, keyID int64) (*GPGKey, error) {
 	key := new(GPGKey)
-	has, err := db.GetEngine(ctx).ID(keyID).Get(key)
+	has, err := db.GetEngine(ctx).Where("id=? AND owner_id=?", keyID, ownerID).Get(key)
 	if err != nil {
 		return nil, err
 	} else if !has {
@@ -225,19 +224,14 @@ func deleteGPGKey(ctx context.Context, keyID string) (int64, error) {
 
 // DeleteGPGKey deletes GPG key information in database.
 func DeleteGPGKey(ctx context.Context, doer *user_model.User, id int64) (err error) {
-	key, err := GetGPGKeyByID(ctx, id)
+	key, err := GetGPGKeyForUserByID(ctx, doer.ID, id)
 	if err != nil {
 		if IsErrGPGKeyNotExist(err) {
 			return nil
 		}
 		return fmt.Errorf("GetPublicKeyByID: %w", err)
 	}
 
-	// Check if user has access to delete this key.
-	if !doer.IsAdmin && doer.ID != key.OwnerID {
-		return ErrGPGKeyAccessDenied{doer.ID, key.ID}
-	}
-
 	ctx, committer, err := db.TxContext(ctx)
 	if err != nil {
 		return err

models/fixtures/comment.yml

@@ -66,3 +66,12 @@
   tree_path: "README.md"
   created_unix: 946684812
   invalidated: true
+
+-
+  id: 8
+  type: 0 # comment
+  poster_id: 2
+  issue_id: 4 # in repo_id 2
+  content: "comment in private pository"
+  created_unix: 946684811
+  updated_unix: 946684811

models/fixtures/issue.yml

@@ -61,7 +61,7 @@
   priority: 0
   is_closed: true
   is_pull: false
-  num_comments: 0
+  num_comments: 1
   created_unix: 946684830
   updated_unix: 978307200
   is_locked: false

models/issues/comment.go

@@ -1016,6 +1016,7 @@ type FindCommentsOptions struct {
 	Type        CommentType
 	IssueIDs    []int64
 	Invalidated util.OptionalBool
+	IsPull      util.OptionalBool
 }
 
 // ToConds implements FindOptions interface
@@ -1050,14 +1051,17 @@ func (opts *FindCommentsOptions) ToConds() builder.Cond {
 	if !opts.Invalidated.IsNone() {
 		cond = cond.And(builder.Eq{"comment.invalidated": opts.Invalidated.IsTrue()})
 	}
+	if opts.IsPull != util.OptionalBoolNone {
+		cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.IsTrue()})
+	}
 	return cond
 }
 
 // FindComments returns all comments according options
 func FindComments(ctx context.Context, opts *FindCommentsOptions) (CommentList, error) {
 	comments := make([]*Comment, 0, 10)
 	sess := db.GetEngine(ctx).Where(opts.ToConds())
-	if opts.RepoID > 0 {
+	if opts.RepoID > 0 || opts.IsPull != util.OptionalBoolNone {
 		sess.Join("INNER", "issue", "issue.id = comment.issue_id")
 	}
 

models/issues/content_history.go

@@ -218,9 +218,9 @@ func GetIssueContentHistoryByID(dbCtx context.Context, id int64) (*ContentHistor
 }
 
 // GetIssueContentHistoryAndPrev get a history and the previous non-deleted history (to compare)
-func GetIssueContentHistoryAndPrev(dbCtx context.Context, id int64) (history, prevHistory *ContentHistory, err error) {
+func GetIssueContentHistoryAndPrev(dbCtx context.Context, issueID, id int64) (history, prevHistory *ContentHistory, err error) {
 	history = &ContentHistory{}
-	has, err := db.GetEngine(dbCtx).ID(id).Get(history)
+	has, err := db.GetEngine(dbCtx).Where("id=? AND issue_id=?", id, issueID).Get(history)
 	if err != nil {
 		log.Error("failed to get issue content history %v. err=%v", id, err)
 		return nil, nil, err

models/issues/content_history_test.go

@@ -58,13 +58,13 @@ func TestContentHistory(t *testing.T) {
 	hasHistory2, _ := issues_model.HasIssueContentHistory(dbCtx, 10, 1)
 	assert.False(t, hasHistory2)
 
-	h6, h6Prev, _ := issues_model.GetIssueContentHistoryAndPrev(dbCtx, 6)
+	h6, h6Prev, _ := issues_model.GetIssueContentHistoryAndPrev(dbCtx, 10, 6)
 	assert.EqualValues(t, 6, h6.ID)
 	assert.EqualValues(t, 5, h6Prev.ID)
 
 	// soft-delete
 	_ = issues_model.SoftDeleteIssueContentHistory(dbCtx, 5)
-	h6, h6Prev, _ = issues_model.GetIssueContentHistoryAndPrev(dbCtx, 6)
+	h6, h6Prev, _ = issues_model.GetIssueContentHistoryAndPrev(dbCtx, 10, 6)
 	assert.EqualValues(t, 6, h6.ID)
 	assert.EqualValues(t, 4, h6Prev.ID)
 

models/project/project.go

@@ -311,6 +311,18 @@ func GetProjectByID(ctx context.Context, id int64) (*Project, error) {
 	return p, nil
 }
 
+// GetProjectForRepoByID returns the projects in a repository
+func GetProjectForRepoByID(ctx context.Context, repoID, id int64) (*Project, error) {
+	p := new(Project)
+	has, err := db.GetEngine(ctx).Where("id=? AND repo_id=?", id, repoID).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) {

models/repo/release.go

@@ -207,6 +207,21 @@ func GetReleaseByID(ctx context.Context, id int64) (*Release, error) {
 	return rel, nil
 }
 
+// GetReleaseForRepoByID returns release with given ID.
+func GetReleaseForRepoByID(ctx context.Context, repoID, id int64) (*Release, error) {
+	rel := new(Release)
+	has, err := db.GetEngine(ctx).
+		Where("id=? AND repo_id=?", id, repoID).
+		Get(rel)
+	if err != nil {
+		return nil, err
+	} else if !has {
+		return nil, ErrReleaseNotExist{id, ""}
+	}
+
+	return rel, nil
+}
+
 // FindReleasesOptions describes the conditions to Find releases
 type FindReleasesOptions struct {
 	db.ListOptions

models/webhook/webhook.go

@@ -392,39 +392,40 @@ func CreateWebhooks(ctx context.Context, ws []*Webhook) error {
 	return db.Insert(ctx, ws)
 }
 
-// getWebhook uses argument bean as query condition,
-// ID must be specified and do not assign unnecessary fields.
-func getWebhook(bean *Webhook) (*Webhook, error) {
-	has, err := db.GetEngine(db.DefaultContext).Get(bean)
+// GetWebhookByID returns webhook of repository by given ID.
+func GetWebhookByID(ctx context.Context, id int64) (*Webhook, error) {
+	bean := new(Webhook)
+	has, err := db.GetEngine(ctx).ID(id).Get(bean)
 	if err != nil {
 		return nil, err
 	} else if !has {
-		return nil, ErrWebhookNotExist{ID: bean.ID}
+		return nil, ErrWebhookNotExist{ID: id}
 	}
 	return bean, nil
 }
 
-// GetWebhookByID returns webhook of repository by given ID.
-func GetWebhookByID(id int64) (*Webhook, error) {
-	return getWebhook(&Webhook{
-		ID: id,
-	})
-}
-
 // GetWebhookByRepoID returns webhook of repository by given ID.
-func GetWebhookByRepoID(repoID, id int64) (*Webhook, error) {
-	return getWebhook(&Webhook{
-		ID:     id,
-		RepoID: repoID,
-	})
+func GetWebhookByRepoID(ctx context.Context, repoID, id int64) (*Webhook, error) {
+	webhook := new(Webhook)
+	has, err := db.GetEngine(ctx).Where("id=? AND repo_id=?", id, repoID).Get(webhook)
+	if err != nil {
+		return nil, err
+	} else if !has {
+		return nil, ErrWebhookNotExist{ID: id}
+	}
+	return webhook, nil
 }
 
 // GetWebhookByOwnerID returns webhook of a user or organization by given ID.
-func GetWebhookByOwnerID(ownerID, id int64) (*Webhook, error) {
-	return getWebhook(&Webhook{
-		ID:      id,
-		OwnerID: ownerID,
-	})
+func GetWebhookByOwnerID(ctx context.Context, ownerID, id int64) (*Webhook, error) {
+	webhook := new(Webhook)
+	has, err := db.GetEngine(ctx).Where("id=? AND owner_id=?", id, ownerID).Get(webhook)
+	if err != nil {
+		return nil, err
+	} else if !has {
+		return nil, ErrWebhookNotExist{ID: id}
+	}
+	return webhook, nil
 }
 
 // ListWebhookOptions are options to filter webhooks on ListWebhooksByOpts
@@ -482,38 +483,38 @@ func UpdateWebhookLastStatus(w *Webhook) error {
 	return err
 }
 
-// deleteWebhook uses argument bean as query condition,
+// DeleteWebhookByID uses argument bean as query condition,
 // ID must be specified and do not assign unnecessary fields.
-func deleteWebhook(bean *Webhook) (err error) {
-	ctx, committer, err := db.TxContext(db.DefaultContext)
+func DeleteWebhookByID(ctx context.Context, id int64) (err error) {
+	ctx, committer, err := db.TxContext(ctx)
 	if err != nil {
 		return err
 	}
 	defer committer.Close()
 
-	if count, err := db.DeleteByBean(ctx, bean); err != nil {
+	if count, err := db.DeleteByID(ctx, id, new(Webhook)); err != nil {
 		return err
 	} else if count == 0 {
-		return ErrWebhookNotExist{ID: bean.ID}
-	} else if _, err = db.DeleteByBean(ctx, &HookTask{HookID: bean.ID}); err != nil {
+		return ErrWebhookNotExist{ID: id}
+	} else if _, err = db.DeleteByBean(ctx, &HookTask{HookID: id}); err != nil {
 		return err
 	}
 
 	return committer.Commit()
 }
 
 // DeleteWebhookByRepoID deletes webhook of repository by given ID.
-func DeleteWebhookByRepoID(repoID, id int64) error {
-	return deleteWebhook(&Webhook{
-		ID:     id,
-		RepoID: repoID,
-	})
+func DeleteWebhookByRepoID(ctx context.Context, repoID, id int64) error {
+	if _, err := GetWebhookByRepoID(ctx, repoID, id); err != nil {
+		return err
+	}
+	return DeleteWebhookByID(ctx, id)
 }
 
 // DeleteWebhookByOwnerID deletes webhook of a user or organization by given ID.
-func DeleteWebhookByOwnerID(ownerID, id int64) error {
-	return deleteWebhook(&Webhook{
-		ID:      id,
-		OwnerID: ownerID,
-	})
+func DeleteWebhookByOwnerID(ctx context.Context, ownerID, id int64) error {
+	if _, err := GetWebhookByOwnerID(ctx, ownerID, id); err != nil {
+		return err
+	}
+	return DeleteWebhookByID(ctx, id)
 }

models/webhook/webhook_test.go

@@ -101,22 +101,22 @@ func TestCreateWebhook(t *testing.T) {
 
 func TestGetWebhookByRepoID(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
-	hook, err := GetWebhookByRepoID(1, 1)
+	hook, err := GetWebhookByRepoID(db.DefaultContext, 1, 1)
 	assert.NoError(t, err)
 	assert.Equal(t, int64(1), hook.ID)
 
-	_, err = GetWebhookByRepoID(unittest.NonexistentID, unittest.NonexistentID)
+	_, err = GetWebhookByRepoID(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID)
 	assert.Error(t, err)
 	assert.True(t, IsErrWebhookNotExist(err))
 }
 
 func TestGetWebhookByOwnerID(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
-	hook, err := GetWebhookByOwnerID(3, 3)
+	hook, err := GetWebhookByOwnerID(db.DefaultContext, 3, 3)
 	assert.NoError(t, err)
 	assert.Equal(t, int64(3), hook.ID)
 
-	_, err = GetWebhookByOwnerID(unittest.NonexistentID, unittest.NonexistentID)
+	_, err = GetWebhookByOwnerID(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID)
 	assert.Error(t, err)
 	assert.True(t, IsErrWebhookNotExist(err))
 }
@@ -174,21 +174,21 @@ func TestUpdateWebhook(t *testing.T) {
 func TestDeleteWebhookByRepoID(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	unittest.AssertExistsAndLoadBean(t, &Webhook{ID: 2, RepoID: 1})
-	assert.NoError(t, DeleteWebhookByRepoID(1, 2))
+	assert.NoError(t, DeleteWebhookByRepoID(db.DefaultContext, 1, 2))
 	unittest.AssertNotExistsBean(t, &Webhook{ID: 2, RepoID: 1})
 
-	err := DeleteWebhookByRepoID(unittest.NonexistentID, unittest.NonexistentID)
+	err := DeleteWebhookByRepoID(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID)
 	assert.Error(t, err)
 	assert.True(t, IsErrWebhookNotExist(err))
 }
 
 func TestDeleteWebhookByOwnerID(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	unittest.AssertExistsAndLoadBean(t, &Webhook{ID: 3, OwnerID: 3})
-	assert.NoError(t, DeleteWebhookByOwnerID(3, 3))
+	assert.NoError(t, DeleteWebhookByOwnerID(db.DefaultContext, 3, 3))
 	unittest.AssertNotExistsBean(t, &Webhook{ID: 3, OwnerID: 3})
 
-	err := DeleteWebhookByOwnerID(unittest.NonexistentID, unittest.NonexistentID)
+	err := DeleteWebhookByOwnerID(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID)
 	assert.Error(t, err)
 	assert.True(t, IsErrWebhookNotExist(err))
 }