diff --git a/app/admin/apis/sys_user.go b/app/admin/apis/sys_user.go index 70d20688..07b8c6dd 100644 --- a/app/admin/apis/sys_user.go +++ b/app/admin/apis/sys_user.go @@ -1,6 +1,7 @@ package apis import ( + "errors" "github.com/gin-gonic/gin/binding" "go-admin/app/admin/models" "golang.org/x/crypto/bcrypt" @@ -15,6 +16,7 @@ import ( "go-admin/app/admin/service" "go-admin/app/admin/service/dto" "go-admin/common/actions" + "go-admin/common/middleware" ) type SysUser struct { @@ -149,12 +151,34 @@ func (e SysUser) Update(c *gin.Context) { return } - req.SetUpdateBy(user.GetUserId(c)) + callerId := user.GetUserId(c) + + // This route is in CasbinExclude so the personal-center screen can edit + // the caller's own record without a policy grant (see settings.go). That + // exclusion covers the whole route, not just the caller's own record, and + // the request carries the target userId in the body - so without this + // check here, any authenticated caller could edit any other user, up to + // and including their roleId. When the target is someone else, ask Casbin + // directly for the permission AuthCheckRole skipped. + if req.UserId != callerId { + allowed, err := middleware.EnforceRoleFor(c, c.Request.URL.Path, c.Request.Method) + if err != nil { + e.Logger.Error(err) + e.Error(500, err, err.Error()) + return + } + if !allowed { + e.Error(http.StatusForbidden, errors.New("无权更新其他用户数据"), "对不起,您没有该接口访问权限,请联系管理员") + return + } + } + + req.SetUpdateBy(callerId) //数据权限检查 p := actions.GetPermissionFromContext(c) - err = s.Update(&req, p) + err = s.Update(&req, p, callerId) if err != nil { e.Logger.Error(err) return diff --git a/app/admin/apis/sys_user_privesc_test.go b/app/admin/apis/sys_user_privesc_test.go new file mode 100644 index 00000000..b2e7eb45 --- /dev/null +++ b/app/admin/apis/sys_user_privesc_test.go @@ -0,0 +1,175 @@ +package apis + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/glebarez/sqlite" + mycasbin "github.com/go-admin-team/go-admin-core/v2/casbin" + jwt "github.com/go-admin-team/go-admin-core/v2/jwtauth" + "github.com/go-admin-team/go-admin-core/v2/logger" + "github.com/go-admin-team/go-admin-core/v2/sdk" + "github.com/go-admin-team/go-admin-core/v2/sdk/pkg" + "gorm.io/gorm" + + "go-admin/app/admin/models" +) + +// PUT /api/v1/sys-user is in settings.go's CasbinExclude so the +// personal-center screen (go-admin-ui's userInfo.vue) can edit the caller's +// own record without holding a policy grant on this route. AuthCheckRole +// skips Enforce entirely for an excluded route, so this file's job is to pin +// what the handler itself now has to hold shut: the target userId comes from +// the request body, and nothing upstream of the handler ever checked it +// against the caller. + +// setupPrivescDB wires an in-memory database and a Casbin enforcer with an +// empty policy - the state of a fresh install for any role but admin - under +// a tenant unique to the calling test, so mycasbin's process-wide enforcer +// cache can't hand one test's database to another. +func setupPrivescDB(t *testing.T) (*gorm.DB, string) { + t.Helper() + + // Fatalf, not Skipf: this database is in-memory sqlite with no external + // dependency, so failing to open or migrate it means the environment is + // actually broken. Skipping here would let these two anti-privesc + // regression tests silently stop running while CI stays green - a + // standing assertion that never fires is worse than no assertion. + db, err := gorm.Open(sqlite.Open("file::memory:"), &gorm.Config{}) + if err != nil { + t.Fatalf("sqlite unavailable: %v", err) + } + if err := db.AutoMigrate(&models.SysUser{}); err != nil { + t.Fatalf("automigrate: %v", err) + } + + tenant := "sys-user-privesc-" + t.Name() + + previousInterval := mycasbin.ReloadInterval + mycasbin.ReloadInterval = 0 // opt out of the background reload goroutine; the test never writes a policy + t.Cleanup(func() { mycasbin.ReloadInterval = previousInterval }) + + e := mycasbin.Setup(db, tenant) + previousEnforcer := sdk.Runtime.GetCasbinByTenant(tenant) + sdk.Runtime.SetCasbinByTenant(tenant, e) + t.Cleanup(func() { sdk.Runtime.SetCasbinByTenant(tenant, previousEnforcer) }) + + return db, tenant +} + +// callUpdate drives SysUser.Update the way the router does for an +// authenticated, non-admin caller: JWT claims already decoded into the +// context (that is jwtauth's job, not this handler's) and a database - but +// without AuthCheckRole, since that middleware never runs Enforce for this +// route at all. +func callUpdate(t *testing.T, db *gorm.DB, tenant string, callerId int, body map[string]interface{}) *httptest.ResponseRecorder { + t.Helper() + gin.SetMode(gin.TestMode) + + raw, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal request body: %v", err) + } + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPut, "/api/v1/sys-user", bytes.NewReader(raw)) + c.Request.Host = tenant + c.Request.Header.Set("Content-Type", "application/json") + + c.Set("db", db) + c.Set(pkg.LoggerKey, logger.NewHelper(logger.DefaultLogger)) + c.Set(jwt.JwtPayloadKey, jwt.MapClaims{ + "identity": float64(callerId), + "rolekey": "ordinary-role", // holds no Casbin policy anywhere in this test + }) + + SysUser{}.Update(c) + return w +} + +// TestUpdate_CannotEscalatePrivilegeThroughAnotherUsersRecord is the +// regression for H6. Before the fix, an ordinary authenticated user could PUT +// a body naming another user's id and change that user's roleId - the route +// being Casbin-excluded meant no permission check ever ran, and the data +// permission scope that would otherwise gate this is off by default. +func TestUpdate_CannotEscalatePrivilegeThroughAnotherUsersRecord(t *testing.T) { + db, tenant := setupPrivescDB(t) + + victim := models.SysUser{Username: "bob", NickName: "Bob", RoleId: 2, DeptId: 1, Status: "1"} + if err := db.Create(&victim).Error; err != nil { + t.Fatal(err) + } + attacker := models.SysUser{Username: "alice", NickName: "Alice", RoleId: 2, DeptId: 1, Status: "1"} + if err := db.Create(&attacker).Error; err != nil { + t.Fatal(err) + } + + const elevatedRoleId = 1 // a role the attacker does not hold and has no policy for + + callUpdate(t, db, tenant, attacker.UserId, map[string]interface{}{ + "userId": victim.UserId, + "username": victim.Username, + "nickName": "pwned", + "phone": "13800000000", + "email": "bob@example.com", + "roleId": elevatedRoleId, + "deptId": victim.DeptId, + "status": victim.Status, + }) + + var after models.SysUser + if err := db.First(&after, victim.UserId).Error; err != nil { + t.Fatal(err) + } + if after.RoleId == elevatedRoleId { + t.Fatalf("an attacker with no Casbin permission on this route escalated the victim's roleId to %d", after.RoleId) + } + if after.NickName == "pwned" { + t.Fatalf("an attacker with no Casbin permission on this route modified another user's record: %+v", after) + } +} + +// TestUpdate_SelfEditCannotChangePrivilegedFields covers the case the +// CasbinExclude entry exists for: the personal-center screen has to keep +// working for the caller's own record. The fields that screen exposes +// (nickName/phone/email/sex) must still save, while roleId/deptId/status stay +// whatever the database already had even if the request carries something +// else - a compromised or hand-crafted client is the only way that request +// would ever differ from what the honest form sends. +func TestUpdate_SelfEditCannotChangePrivilegedFields(t *testing.T) { + db, tenant := setupPrivescDB(t) + + self := models.SysUser{Username: "carol", NickName: "Carol", RoleId: 2, DeptId: 1, Status: "1"} + if err := db.Create(&self).Error; err != nil { + t.Fatal(err) + } + + const elevatedRoleId = 1 + + callUpdate(t, db, tenant, self.UserId, map[string]interface{}{ + "userId": self.UserId, + "username": self.Username, + "nickName": "Carol Updated", + "phone": "13900000000", + "email": "carol@example.com", + "roleId": elevatedRoleId, // tampered; must not take effect + "deptId": self.DeptId, + "status": self.Status, + }) + + var after models.SysUser + if err := db.First(&after, self.UserId).Error; err != nil { + t.Fatal(err) + } + if after.RoleId == elevatedRoleId { + t.Fatalf("a self-edit changed the caller's own roleId to %d", after.RoleId) + } + if after.NickName != "Carol Updated" { + t.Fatalf("the legitimate personal-center edit did not go through: %+v", after) + } +} diff --git a/app/admin/service/sys_user.go b/app/admin/service/sys_user.go index 8361e5d4..839a9b3b 100644 --- a/app/admin/service/sys_user.go +++ b/app/admin/service/sys_user.go @@ -84,7 +84,16 @@ func (e *SysUser) Insert(c *dto.SysUserInsertReq) error { } // Update 修改SysUser对象 -func (e *SysUser) Update(c *dto.SysUserUpdateReq, p *actions.DataPermission) error { +// +// callerId is who is asking, not who SetUpdateBy recorded - that field only +// says who to blame, it never constrained who could be edited. When the +// target is the caller themselves, roleId/deptId/status are kept at whatever +// the database already has no matter what the request body carries: this is +// the personal-center screen's route (see CasbinExclude in settings.go, and +// the check in the API handler ahead of this call), and letting a caller +// grant themselves a different role or department through it would be a +// privilege escalation the exclusion was never meant to open. +func (e *SysUser) Update(c *dto.SysUserUpdateReq, p *actions.DataPermission, callerId int) error { var err error var model models.SysUser db := e.Orm.Scopes( @@ -98,6 +107,11 @@ func (e *SysUser) Update(c *dto.SysUserUpdateReq, p *actions.DataPermission) err return errors.New("无权更新该数据") } + if model.UserId == callerId { + c.RoleId = model.RoleId + c.DeptId = model.DeptId + c.Status = model.Status + } c.Generate(&model) update := e.Orm.Model(&model).Where("user_id = ?", &model.UserId).Omit("password", "salt").Updates(&model) if err = update.Error; err != nil { diff --git a/common/middleware/permission.go b/common/middleware/permission.go index 993fde77..99a9aa5e 100644 --- a/common/middleware/permission.go +++ b/common/middleware/permission.go @@ -59,6 +59,33 @@ func AuthCheckRole() gin.HandlerFunc { } } +// EnforceRoleFor reports whether the caller's role has explicit Casbin +// permission to act on path with method. +// +// AuthCheckRole never calls Enforce for a route CasbinExclude lists - that +// is the whole point of the list. A handler on such a route can still need +// the real answer for part of what it does: sys_user.go's Update shares its +// excluded route between the personal-center screen editing the caller's own +// record (which is why the route is excluded at all) and an admin editing +// someone else's, and only the second case is meant to require a policy +// grant. That handler asks here instead of assuming the middleware already +// checked. +func EnforceRoleFor(c *gin.Context, path, method string) (bool, error) { + data, ok := c.Get(jwtauth.JwtPayloadKey) + if !ok { + return false, nil + } + v, ok := data.(jwtauth.MapClaims) + if !ok { + return false, nil + } + if v["rolekey"] == "admin" { + return true, nil + } + e := sdk.Runtime.GetCasbinByTenant(c.Request.Host) + return e.Enforce(v["rolekey"], path, method) +} + // excludedFromCasbin reports whether the route skips the permission check. // // It runs for every non-admin request, so the order matters: the method rules