You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@answer.apache.org by li...@apache.org on 2026/10/12 06:58:38 UTC
(answer) 01/02: fix(security): enforce collection visibility controls
This is an automated email from the ASF dual-hosted git repository.
LinkinStars pushed a commit to branch fix/2.0.4/user
in repository https://gitbox.apache.org/repos/asf/answer.git
commit bca2a69792a081574987c4798c316e37a3eb9010
Author: LinkinStars <li...@foxmail.com>
AuthorDate: Fri Oct 9 16:34:41 2026 +0800
fix(security): enforce collection visibility controls
Apply consistent authorization checks to collection operations and listings.
---
cmd/wire_gen.go | 2 +-
internal/controller/collection_controller.go | 1 +
internal/controller/question_controller.go | 1 +
internal/entity/collection_entity.go | 5 +-
internal/repo/collection/collection_repo.go | 9 ++-
.../collection/collection_repo_visibility_test.go | 76 ++++++++++++++++++++++
internal/schema/collection_group_schema.go | 9 +--
internal/schema/question_schema.go | 7 +-
internal/service/collection/collection_service.go | 20 ++++++
.../collection_service_visibility_test.go | 74 +++++++++++++++++++++
internal/service/content/question_service.go | 12 +++-
.../content/question_service_visibility_test.go | 28 ++++++++
12 files changed, 231 insertions(+), 13 deletions(-)
diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go
index 66e16278..8c8a689f 100644
--- a/cmd/wire_gen.go
+++ b/cmd/wire_gen.go
@@ -228,7 +228,7 @@ func initApplication(debug bool, serverConf *conf.Server, dbConf *data.Database,
followService := follow.NewFollowService(followFollowRepo, followRepo, tagCommonRepo)
followController := controller.NewFollowController(followService)
collectionGroupRepo := collection.NewCollectionGroupRepo(dataData)
- collectionService := collection2.NewCollectionService(collectionRepo, collectionGroupRepo, questionCommon)
+ collectionService := collection2.NewCollectionService(collectionRepo, collectionGroupRepo, questionCommon, objService)
collectionController := controller.NewCollectionController(collectionService)
questionController := controller.NewQuestionController(questionService, answerService, rankService, siteInfoCommonService, captchaService, rateLimitMiddleware)
answerController := controller.NewAnswerController(answerService, rankService, captchaService, siteInfoCommonService, rateLimitMiddleware)
diff --git a/internal/controller/collection_controller.go b/internal/controller/collection_controller.go
index 78560be0..0c70d454 100644
--- a/internal/controller/collection_controller.go
+++ b/internal/controller/collection_controller.go
@@ -56,6 +56,7 @@ func (cc *CollectionController) CollectionSwitch(ctx *gin.Context) {
req.ObjectID = uid.DeShortID(req.ObjectID)
req.UserID = middleware.GetLoginUserIDFromContext(ctx)
+ req.IsAdminModerator = middleware.GetUserIsAdminModerator(ctx)
resp, err := cc.collectionService.CollectionSwitch(ctx, req)
handler.HandleResponse(ctx, err, resp)
diff --git a/internal/controller/question_controller.go b/internal/controller/question_controller.go
index 8294cdcb..389750e1 100644
--- a/internal/controller/question_controller.go
+++ b/internal/controller/question_controller.go
@@ -933,6 +933,7 @@ func (qc *QuestionController) PersonalCollectionPage(ctx *gin.Context) {
}
req.UserID = middleware.GetLoginUserIDFromContext(ctx)
+ req.IsAdminModerator = middleware.GetUserIsAdminModerator(ctx)
resp, err := qc.questionService.PersonalCollectionPage(ctx, req)
handler.HandleResponse(ctx, err, resp)
diff --git a/internal/entity/collection_entity.go b/internal/entity/collection_entity.go
index 24ae232f..f4cb00ba 100644
--- a/internal/entity/collection_entity.go
+++ b/internal/entity/collection_entity.go
@@ -33,8 +33,9 @@ type Collection struct {
type CollectionSearch struct {
Collection
- Page int `json:"page" form:"page"` // Query number of pages
- PageSize int `json:"page_size" form:"page_size"` // Search page size
+ Page int `json:"page" form:"page"` // Query number of pages
+ PageSize int `json:"page_size" form:"page_size"` // Search page size
+ IsAdminModerator bool `json:"-"`
}
// TableName collection table name
diff --git a/internal/repo/collection/collection_repo.go b/internal/repo/collection/collection_repo.go
index 482cb075..ffd89ce7 100644
--- a/internal/repo/collection/collection_repo.go
+++ b/internal/repo/collection/collection_repo.go
@@ -203,12 +203,17 @@ func (cr *collectionRepo) SearchList(ctx context.Context, search *entity.Collect
offset := search.Page * search.PageSize
session := cr.data.DB.Context(ctx).Where("")
if len(search.UserID) > 0 {
- session = session.And("user_id = ?", search.UserID)
+ session = session.Join("INNER", "question", "collection.object_id = question.id").
+ And("collection.user_id = ?", search.UserID)
} else {
return rows, count, nil
}
+ if !search.IsAdminModerator {
+ session = session.And("(question.status IN (?, ?) AND question.show = ?) OR question.user_id = ?",
+ entity.QuestionStatusAvailable, entity.QuestionStatusClosed, entity.QuestionShow, search.UserID)
+ }
session = session.Limit(search.PageSize, offset)
- count, err = session.OrderBy("updated_at desc").FindAndCount(&rows)
+ count, err = session.OrderBy("collection.updated_at desc").FindAndCount(&rows)
if err != nil {
return rows, count, err
}
diff --git a/internal/repo/collection/collection_repo_visibility_test.go b/internal/repo/collection/collection_repo_visibility_test.go
new file mode 100644
index 00000000..e03337a9
--- /dev/null
+++ b/internal/repo/collection/collection_repo_visibility_test.go
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package collection
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/answer/internal/base/data"
+ "github.com/apache/answer/internal/entity"
+)
+
+func TestSearchListExcludesRestrictedQuestionsForOtherUsers(t *testing.T) {
+ ctx := context.Background()
+ db, err := data.NewDB(false, &data.Database{Driver: "sqlite", Connection: ":memory:"})
+ if err != nil {
+ t.Fatalf("create SQLite database: %v", err)
+ }
+ t.Cleanup(func() { _ = db.Close() })
+ if err := db.Sync2(new(entity.Question), new(entity.Collection)); err != nil {
+ t.Fatalf("create tables: %v", err)
+ }
+
+ questions := []*entity.Question{
+ {ID: "1001", UserID: "author", Status: entity.QuestionStatusAvailable, Show: entity.QuestionShow},
+ {ID: "1002", UserID: "author", Status: entity.QuestionStatusAvailable, Show: entity.QuestionHide},
+ {ID: "1003", UserID: "author", Status: entity.QuestionStatusPending, Show: entity.QuestionShow},
+ {ID: "1004", UserID: "viewer", Status: entity.QuestionStatusAvailable, Show: entity.QuestionHide},
+ {ID: "1005", UserID: "author", Status: entity.QuestionStatusClosed, Show: entity.QuestionShow},
+ }
+ for _, question := range questions {
+ if _, err := db.Context(ctx).Insert(question); err != nil {
+ t.Fatalf("insert question %s: %v", question.ID, err)
+ }
+ if _, err := db.Context(ctx).Insert(&entity.Collection{ID: "200" + question.ID[3:], UserID: "viewer", ObjectID: question.ID}); err != nil {
+ t.Fatalf("insert collection for %s: %v", question.ID, err)
+ }
+ }
+
+ repo := &collectionRepo{data: &data.Data{DB: db}}
+ collections, total, err := repo.SearchList(ctx, &entity.CollectionSearch{
+ Collection: entity.Collection{UserID: "viewer"},
+ Page: 1,
+ PageSize: 10,
+ })
+ if err != nil {
+ t.Fatalf("search collections: %v", err)
+ }
+ if total != 3 || len(collections) != 3 {
+ t.Fatalf("visible collections = %d (total %d), want 3", len(collections), total)
+ }
+ visible := map[string]bool{}
+ for _, collection := range collections {
+ visible[collection.ObjectID] = true
+ }
+ if !visible["1001"] || !visible["1004"] || !visible["1005"] || visible["1002"] || visible["1003"] {
+ t.Fatalf("visible collections = %v, want 1001, 1004, and 1005", visible)
+ }
+}
diff --git a/internal/schema/collection_group_schema.go b/internal/schema/collection_group_schema.go
index 3177f3f1..3a4ac2cc 100644
--- a/internal/schema/collection_group_schema.go
+++ b/internal/schema/collection_group_schema.go
@@ -28,10 +28,11 @@ const (
// CollectionSwitchReq switch collection request
type CollectionSwitchReq struct {
- ObjectID string `validate:"required" json:"object_id"`
- GroupID string `validate:"required" json:"group_id"`
- Bookmark bool `validate:"omitempty" json:"bookmark"`
- UserID string `json:"-"`
+ ObjectID string `validate:"required" json:"object_id"`
+ GroupID string `validate:"required" json:"group_id"`
+ Bookmark bool `validate:"omitempty" json:"bookmark"`
+ UserID string `json:"-"`
+ IsAdminModerator bool `json:"-"`
}
// CollectionSwitchResp switch collection response
diff --git a/internal/schema/question_schema.go b/internal/schema/question_schema.go
index 4c6d2ead..6b24b550 100644
--- a/internal/schema/question_schema.go
+++ b/internal/schema/question_schema.go
@@ -508,9 +508,10 @@ type PersonalAnswerPageReq struct {
}
type PersonalCollectionPageReq struct {
- Page int `validate:"omitempty,min=1" form:"page"`
- PageSize int `validate:"omitempty,min=1" form:"page_size"`
- UserID string `json:"-"`
+ Page int `validate:"omitempty,min=1" form:"page"`
+ PageSize int `validate:"omitempty,min=1" form:"page_size"`
+ UserID string `json:"-"`
+ IsAdminModerator bool `json:"-"`
}
type GetQuestionLinkReq struct {
diff --git a/internal/service/collection/collection_service.go b/internal/service/collection/collection_service.go
index 6f02ccf9..8402c324 100644
--- a/internal/service/collection/collection_service.go
+++ b/internal/service/collection/collection_service.go
@@ -22,33 +22,53 @@ package collection
import (
"context"
+ "github.com/apache/answer/internal/base/reason"
"github.com/apache/answer/internal/entity"
"github.com/apache/answer/internal/schema"
collectioncommon "github.com/apache/answer/internal/service/collection_common"
questioncommon "github.com/apache/answer/internal/service/question_common"
+ "github.com/segmentfault/pacman/errors"
)
+type ObjectInfoService interface {
+ GetInfo(ctx context.Context, objectID string) (*schema.SimpleObjectInfo, error)
+}
+
// CollectionService user service
type CollectionService struct {
collectionRepo collectioncommon.CollectionRepo
collectionGroupRepo CollectionGroupRepo
questionCommon *questioncommon.QuestionCommon
+ objectInfoService ObjectInfoService
}
func NewCollectionService(
collectionRepo collectioncommon.CollectionRepo,
collectionGroupRepo CollectionGroupRepo,
questionCommon *questioncommon.QuestionCommon,
+ objectInfoService ObjectInfoService,
) *CollectionService {
return &CollectionService{
collectionRepo: collectionRepo,
collectionGroupRepo: collectionGroupRepo,
questionCommon: questionCommon,
+ objectInfoService: objectInfoService,
}
}
func (cs *CollectionService) CollectionSwitch(ctx context.Context, req *schema.CollectionSwitchReq) (
resp *schema.CollectionSwitchResp, err error) {
+ objectInfo, err := cs.objectInfoService.GetInfo(ctx, req.ObjectID)
+ if err != nil {
+ if serviceErr, ok := err.(*errors.Error); ok && serviceErr.Reason == reason.ObjectNotFound {
+ return nil, errors.NotFound(reason.QuestionNotFound)
+ }
+ return nil, err
+ }
+ if err := objectInfo.CheckVisibility(req.UserID, req.IsAdminModerator); err != nil {
+ return nil, err
+ }
+
collectionGroup, err := cs.collectionGroupRepo.CreateDefaultGroupIfNotExist(ctx, req.UserID)
if err != nil {
return nil, err
diff --git a/internal/service/collection/collection_service_visibility_test.go b/internal/service/collection/collection_service_visibility_test.go
new file mode 100644
index 00000000..79f886dd
--- /dev/null
+++ b/internal/service/collection/collection_service_visibility_test.go
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package collection
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/answer/internal/base/constant"
+ "github.com/apache/answer/internal/base/reason"
+ "github.com/apache/answer/internal/entity"
+ "github.com/apache/answer/internal/schema"
+ "github.com/segmentfault/pacman/errors"
+)
+
+type collectionTestObjectInfoService struct {
+ objectInfo *schema.SimpleObjectInfo
+ err error
+}
+
+func (s collectionTestObjectInfoService) GetInfo(context.Context, string) (*schema.SimpleObjectInfo, error) {
+ return s.objectInfo, s.err
+}
+
+func TestCollectionSwitchRejectsHiddenQuestionForOtherUsers(t *testing.T) {
+ service := &CollectionService{objectInfoService: collectionTestObjectInfoService{
+ objectInfo: &schema.SimpleObjectInfo{
+ ObjectType: constant.QuestionObjectType,
+ QuestionCreatorUserID: "question-owner",
+ QuestionStatus: entity.QuestionStatusAvailable,
+ QuestionShow: entity.QuestionHide,
+ },
+ }}
+
+ _, err := service.CollectionSwitch(context.Background(), &schema.CollectionSwitchReq{
+ ObjectID: "question-id",
+ UserID: "other-user",
+ Bookmark: true,
+ })
+ if err == nil {
+ t.Fatal("CollectionSwitch() allowed another user to bookmark a hidden question")
+ }
+}
+
+func TestCollectionSwitchReturnsNotFoundForMissingQuestion(t *testing.T) {
+ service := &CollectionService{objectInfoService: collectionTestObjectInfoService{
+ err: errors.BadRequest(reason.ObjectNotFound),
+ }}
+ _, err := service.CollectionSwitch(context.Background(), &schema.CollectionSwitchReq{ObjectID: "question-id", UserID: "other-user"})
+ if err == nil {
+ t.Fatal("CollectionSwitch() allowed a missing question")
+ }
+ serviceErr, ok := err.(*errors.Error)
+ if !ok || serviceErr.Code != 404 || serviceErr.Reason != reason.QuestionNotFound {
+ t.Fatalf("CollectionSwitch() error = %#v, want QuestionNotFound", err)
+ }
+}
diff --git a/internal/service/content/question_service.go b/internal/service/content/question_service.go
index ced0e010..ed33f9ea 100644
--- a/internal/service/content/question_service.go
+++ b/internal/service/content/question_service.go
@@ -1284,6 +1284,7 @@ func (qs *QuestionService) PersonalCollectionPage(ctx context.Context, req *sche
collectionSearch.UserID = req.UserID
collectionSearch.Page = req.Page
collectionSearch.PageSize = req.PageSize
+ collectionSearch.IsAdminModerator = req.IsAdminModerator
collectionList, total, err := qs.collectionCommon.SearchList(ctx, collectionSearch)
if err != nil {
return nil, err
@@ -1302,7 +1303,7 @@ func (qs *QuestionService) PersonalCollectionPage(ctx context.Context, req *sche
id = uid.EnShortID(id)
}
_, ok := questionMaps[id]
- if ok {
+ if ok && canViewPersonalCollectionQuestion(questionMaps[id], req.UserID, req.IsAdminModerator) {
questionMaps[id].LastAnsweredUserInfo = nil
questionMaps[id].UpdateUserInfo = nil
questionMaps[id].Content = ""
@@ -1317,6 +1318,15 @@ func (qs *QuestionService) PersonalCollectionPage(ctx context.Context, req *sche
return pager.NewPageModel(total, list), nil
}
+func canViewPersonalCollectionQuestion(question *schema.QuestionInfoResp, userID string, isAdminModerator bool) bool {
+ return (&schema.SimpleObjectInfo{
+ ObjectType: constant.QuestionObjectType,
+ QuestionCreatorUserID: question.UserID,
+ QuestionStatus: question.Status,
+ QuestionShow: question.Show,
+ }).CheckVisibility(userID, isAdminModerator) == nil
+}
+
func (qs *QuestionService) SearchUserTopList(ctx context.Context, userName string, loginUserID string) ([]*schema.UserQuestionInfo, []*schema.UserAnswerInfo, error) {
answerlist := make([]*schema.AnswerInfo, 0)
diff --git a/internal/service/content/question_service_visibility_test.go b/internal/service/content/question_service_visibility_test.go
index 5cfeb5b7..fd0d98cf 100644
--- a/internal/service/content/question_service_visibility_test.go
+++ b/internal/service/content/question_service_visibility_test.go
@@ -138,3 +138,31 @@ func TestCheckQuestionVisibility(t *testing.T) {
})
}
}
+
+func TestCanViewPersonalCollectionQuestion(t *testing.T) {
+ testCases := []struct {
+ name string
+ status int
+ show int
+ author string
+ viewer string
+ isAdmin bool
+ allow bool
+ }{
+ {"public question", entity.QuestionStatusAvailable, entity.QuestionShow, "author", "viewer", false, true},
+ {"closed question", entity.QuestionStatusClosed, entity.QuestionShow, "author", "viewer", false, true},
+ {"hidden question", entity.QuestionStatusAvailable, entity.QuestionHide, "author", "viewer", false, false},
+ {"pending question", entity.QuestionStatusPending, entity.QuestionShow, "author", "viewer", false, false},
+ {"owner hidden question", entity.QuestionStatusAvailable, entity.QuestionHide, "author", "author", false, true},
+ {"moderator hidden question", entity.QuestionStatusAvailable, entity.QuestionHide, "author", "viewer", true, true},
+ }
+
+ for _, testCase := range testCases {
+ t.Run(testCase.name, func(t *testing.T) {
+ question := &schema.QuestionInfoResp{UserID: testCase.author, Status: testCase.status, Show: testCase.show}
+ if got := canViewPersonalCollectionQuestion(question, testCase.viewer, testCase.isAdmin); got != testCase.allow {
+ t.Fatalf("canViewPersonalCollectionQuestion() = %t, want %t", got, testCase.allow)
+ }
+ })
+ }
+}