From b7e9a7922548ed27088ac6a6ac9eab5407fc024b Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Tue, 1 Sep 2026 14:17:20 +0800 Subject: [PATCH] =?UTF-8?q?fix=F0=9F=90=9B:=20refuse=20an=20out-of-scope?= =?UTF-8?q?=20API=20update=20with=20the=20permission=20message?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit added a data-permission scope to SysApi.Update and returned early on db.Error, which left the RowsAffected check below it unreachable: First reports a row the scope excluded as ErrRecordNotFound, so the caller got "record not found" where the code meant to say "无权更新该数据". Map that one error to the permission message and drop the check it made dead. The two cases - the row does not exist, and the row exists but is not yours - have to look the same from outside, and now do. Found by Copilot's review of #889. --- app/admin/service/sys_api.go | 11 ++- app/admin/service/sys_api_permission_test.go | 70 ++++++++++++++++++++ 2 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 app/admin/service/sys_api_permission_test.go diff --git a/app/admin/service/sys_api.go b/app/admin/service/sys_api.go index b9bbdfec..fbfc4710 100644 --- a/app/admin/service/sys_api.go +++ b/app/admin/service/sys_api.go @@ -6,6 +6,8 @@ import ( "github.com/go-admin-team/go-admin-core/v2/sdk/runtime" "github.com/go-admin-team/go-admin-core/v2/sdk/service" + "gorm.io/gorm" + "go-admin/app/admin/models" "go-admin/app/admin/service/dto" "go-admin/common/actions" @@ -78,12 +80,15 @@ func (e *SysApi) Update(c *dto.SysApiUpdateReq, p *actions.DataPermission) error actions.Permission(model.TableName(), p), ).First(&model, c.GetId()) if err := db.Error; err != nil { + // First reports a row the data permission excluded exactly as it + // reports one that does not exist, and the caller should not be able + // to tell those apart either. + if errors.Is(err, gorm.ErrRecordNotFound) { + return errors.New("无权更新该数据") + } e.Log.Errorf("Service UpdateSysApi error:%s", err) return err } - if db.RowsAffected == 0 { - return errors.New("无权更新该数据") - } c.Generate(&model) db = e.Orm.Save(&model) if err := db.Error; err != nil { diff --git a/app/admin/service/sys_api_permission_test.go b/app/admin/service/sys_api_permission_test.go new file mode 100644 index 00000000..11c4e85c --- /dev/null +++ b/app/admin/service/sys_api_permission_test.go @@ -0,0 +1,70 @@ +package service + +import ( + "strings" + "testing" + + "github.com/glebarez/sqlite" + "github.com/go-admin-team/go-admin-core/v2/logger" + "github.com/go-admin-team/go-admin-core/v2/sdk/config" + "github.com/go-admin-team/go-admin-core/v2/sdk/service" + "gorm.io/gorm" + + "go-admin/app/admin/models" + "go-admin/app/admin/service/dto" + "go-admin/common/actions" +) + +// An update the data permission excludes has to be refused, and refused in a +// way that does not tell the caller whether the row exists. First reports both +// cases the same way - no rows - so the message has to come from there rather +// than from a RowsAffected check the error return has already skipped past. +func TestSysApiUpdateRefusesARowOutsideTheDataPermission(t *testing.T) { + db, err := gorm.Open(sqlite.Open("file:sysapi-perm?mode=memory&cache=shared"), &gorm.Config{}) + if err != nil { + t.Skipf("sqlite unavailable: %v", err) + } + if err := db.AutoMigrate(&models.SysApi{}); err != nil { + t.Skipf("automigrate: %v", err) + } + + prev := config.ApplicationConfig.EnableDP + config.ApplicationConfig.EnableDP = true + t.Cleanup(func() { config.ApplicationConfig.EnableDP = prev }) + + // Owned by user 1. + row := models.SysApi{Handle: "h", Title: "t", Path: "/api/v1/probe", Type: "BUS", Action: "GET"} + row.CreateBy = 1 + if err := db.Create(&row).Error; err != nil { + t.Fatal(err) + } + + e := &SysApi{Service: service.Service{Orm: db, Log: logger.NewHelper(logger.DefaultLogger)}} + req := &dto.SysApiUpdateReq{Id: row.Id, Title: "changed"} + + // User 2, scope 5: only rows they created. + outsider := &actions.DataPermission{DataScope: "5", UserId: 2, DeptId: 1, RoleId: 2} + err = e.Update(req, outsider) + if err == nil { + t.Fatal("the update was allowed on a row the data permission excludes") + } + if !strings.Contains(err.Error(), "无权更新该数据") { + t.Errorf("refused with %q, want the permission message; a raw database error tells the "+ + "caller the row exists", err) + } + + var after models.SysApi + if err := db.First(&after, row.Id).Error; err != nil { + t.Fatal(err) + } + if after.Title != "t" { + t.Errorf("the row was modified: title is now %q", after.Title) + } + + // The owner still gets through, so the scope is refusing rather than + // everything failing. + owner := &actions.DataPermission{DataScope: "5", UserId: 1, DeptId: 1, RoleId: 1} + if err := e.Update(&dto.SysApiUpdateReq{Id: row.Id, Title: "by owner"}, owner); err != nil { + t.Fatalf("the owner could not update their own row: %v", err) + } +}