diff --git a/common/actions/permission.go b/common/actions/permission.go index 6a42e31a..53aa8709 100644 --- a/common/actions/permission.go +++ b/common/actions/permission.go @@ -1,201 +1,48 @@ package actions import ( - "errors" - "github.com/gin-gonic/gin" - "github.com/go-admin-team/go-admin-core/v2/jwtauth/user" - log "github.com/go-admin-team/go-admin-core/v2/logger" - "github.com/go-admin-team/go-admin-core/v2/response" - "github.com/go-admin-team/go-admin-core/v2/sdk/config" - "github.com/go-admin-team/go-admin-core/v2/sdk/pkg" "gorm.io/gorm" + + contractactions "github.com/go-admin-team/go-admin-core/v2/sdk/contract/actions" ) -type DataPermission struct { - DataScope string - UserId int - DeptId int - RoleId int -} +// DataPermission is a thin alias of go-admin-core's sdk/contract/actions +// (PRD 006 F3/F5). +type DataPermission = contractactions.DataPermission -func PermissionAction() gin.HandlerFunc { - return func(c *gin.Context) { - // Permission() below returns the query untouched when data permission - // is off, so the lookup that feeds it has nothing to feed. It used to - // run anyway: a sys_user join on every list, detail, update and delete, - // with the result discarded. - if !config.ApplicationConfig.EnableDP { - c.Set(PermissionKey, new(DataPermission)) - c.Next() - return - } - - userId := user.GetUserIdStr(c) - if userId == "" { - c.Set(PermissionKey, new(DataPermission)) - c.Next() - return - } - - // The token already carries what the scope is decided by. Reading it - // there costs nothing, and goes no more stale than rolekey does - which - // Casbin has always read from the token. - if p, ok := permissionFromClaims(c); ok { - c.Set(PermissionKey, p) - c.Next() - return - } - - db, err := pkg.GetOrm(c) - if err != nil { - log.Error(err) - // Same fix as the newDataPermission branch below: without Abort, - // gin's "return means continue" semantics send the request on to - // the business handler with PermissionKey never set. The caller - // then reads a zero-value DataPermission, which used to fall - // into Permission()'s fail-open default - a database hiccup - // silently turning into "see everything". PRD 006 F14/H1. - response.Error(c, 500, err, "权限范围鉴定错误") - c.Abort() - return - } - msgID := pkg.GenerateMsgIDFromContext(c) - p, err := newDataPermission(db, userId) - if err != nil { - log.Errorf("MsgID[%s] PermissionAction error: %s", msgID, err) - response.Error(c, 500, err, "权限范围鉴定错误") - c.Abort() - return - } - c.Set(PermissionKey, p) - c.Next() - } -} - -// permissionFromClaims builds the scope from the token, reporting false when -// the token predates deptid being carried. Such a token still exists until it -// expires, and it has to keep working. -func permissionFromClaims(c *gin.Context) (*DataPermission, bool) { - claims := user.ExtractClaims(c) - if claims["deptid"] == nil || claims["datascope"] == nil { - return nil, false - } - scope, ok := claims["datascope"].(string) - if !ok { - return nil, false - } - return &DataPermission{ - DataScope: scope, - UserId: user.GetUserId(c), - DeptId: user.GetDeptId(c), - RoleId: user.GetRoleId(c), - }, true -} - -func newDataPermission(tx *gorm.DB, userId interface{}) (*DataPermission, error) { - var err error - p := &DataPermission{} - - err = tx.Table("sys_user"). - Select("sys_user.user_id", "sys_role.role_id", "sys_user.dept_id", "sys_role.data_scope"). - Joins("left join sys_role on sys_role.role_id = sys_user.role_id"). - Where("sys_user.user_id = ?", userId). - Scan(p).Error - if err != nil { - err = errors.New("获取用户数据出错 msg:" + err.Error()) - return nil, err - } - return p, nil -} - -// The five values sys_role.data_scope can hold. Front end's role editor -// calls them by the same names (go-admin-ui's sys-role/index.vue): "1" is -// 全部数据权限, "2" 自定义数据权限, "3" 本部门数据权限, "4" 本部门及以下数据权限, -// "5" 仅本人数据权限. -// -// DataScopeAll has to be a named, explicit case in Permission below rather -// than falling into default: it is a real, intentional configuration, not an -// absence of one, and default's job after PRD 006 F14/H2 is to catch values -// that are neither. Folding the two together is what made an unset or -// corrupted data_scope indistinguishable from "show everything" in the first -// place. +// The five values sys_role.data_scope can hold, referenced directly from +// go-admin-core's sdk/contract/actions rather than restated as literals - +// see that package's DataScope* doc comment and PRD 006's hard constraint 4. const ( - DataScopeAll = "1" - DataScopeCustom = "2" - DataScopeDept = "3" - DataScopeDeptTree = "4" - DataScopeSelf = "5" + DataScopeAll = contractactions.DataScopeAll + DataScopeCustom = contractactions.DataScopeCustom + DataScopeDept = contractactions.DataScopeDept + DataScopeDeptTree = contractactions.DataScopeDeptTree + DataScopeSelf = contractactions.DataScopeSelf ) -// IsValidDataScope reports whether s is one of the five values Permission -// recognizes. Anything else lands in Permission's fail-closed default, so -// code that persists data_scope (sys_role writes) should reject it before it -// reaches the database rather than let a typo or an empty string surface -// there silently. -func IsValidDataScope(s string) bool { - switch s { - case DataScopeAll, DataScopeCustom, DataScopeDept, DataScopeDeptTree, DataScopeSelf: - return true - default: - return false - } +// PermissionAction, Permission, GetPermissionFromContext and +// IsValidDataScope forward to go-admin-core's sdk/contract/actions (PRD 006 +// F3/F5). create.go/delete.go/index.go/update.go/view.go in this package +// (the generic CRUD actions, which do not move to core) call Permission and +// GetPermissionFromContext by these same names and are unchanged by the +// move: the names now resolve to forwards instead of local definitions, and +// the behaviour is identical either way. +func PermissionAction() gin.HandlerFunc { + return contractactions.PermissionAction() } func Permission(tableName string, p *DataPermission) func(db *gorm.DB) *gorm.DB { - return func(db *gorm.DB) *gorm.DB { - if !config.ApplicationConfig.EnableDP { - return db - } - switch p.DataScope { - case DataScopeAll: - return db - case DataScopeCustom: - return db.Where(tableName+".create_by in (select sys_user.user_id from sys_role_dept left join sys_user on sys_user.dept_id=sys_role_dept.dept_id where sys_role_dept.role_id = ?)", p.RoleId) - case DataScopeDept: - if p.DeptId <= 0 { - // A department id of 0 identifies no real department (see - // sys_dept.go: dept_path always starts with "/0/", the - // reserved root). Matching it literally would mean "every - // user whose dept_id happens to be unset", not "no one" - - // fail closed instead. PRD 006 F14/H3. - return db.Where("1 = 0") - } - return db.Where(tableName+".create_by in (SELECT user_id from sys_user where dept_id = ? )", p.DeptId) - case DataScopeDeptTree: - if p.DeptId <= 0 { - // dept_path is built as "/0/" + id + "/..." for every - // department (sys_dept.go), so a DeptId of 0 turns the LIKE - // pattern below into '%/0/%', which matches every row in - // sys_dept - full visibility instead of none. PRD 006 - // F14/H3. - return db.Where("1 = 0") - } - return db.Where(tableName+".create_by in (SELECT user_id from sys_user where sys_user.dept_id in(select dept_id from sys_dept where dept_path like ? ))", "%/"+pkg.IntToString(p.DeptId)+"/%") - case DataScopeSelf: - return db.Where(tableName+".create_by = ?", p.UserId) - default: - // Unrecognized scope: never configured, corrupted data, or a - // value a future version adds and this one does not know yet. - // Fail closed - match nothing - instead of silently falling - // back to "see everything". PRD 006 F14/H2. - return db.Where("1 = 0") - } - } + return contractactions.Permission(tableName, p) } -func getPermissionFromContext(c *gin.Context) *DataPermission { - p := new(DataPermission) - if pm, ok := c.Get(PermissionKey); ok { - switch pm.(type) { - case *DataPermission: - p = pm.(*DataPermission) - } - } - return p -} - -// GetPermissionFromContext 提供非action写法数据范围约束 func GetPermissionFromContext(c *gin.Context) *DataPermission { - return getPermissionFromContext(c) + return contractactions.GetPermissionFromContext(c) +} + +// IsValidDataScope reports whether s is one of the five values Permission +// recognizes. See go-admin-core's sdk/contract/actions.IsValidDataScope. +func IsValidDataScope(s string) bool { + return contractactions.IsValidDataScope(s) } diff --git a/common/actions/permission_test.go b/common/actions/permission_test.go index 903d083c..208c9267 100644 --- a/common/actions/permission_test.go +++ b/common/actions/permission_test.go @@ -1,4 +1,4 @@ -package actions +package actions_test import ( "net/http" @@ -6,201 +6,101 @@ import ( "testing" "github.com/gin-gonic/gin" - "github.com/glebarez/sqlite" + jwt "github.com/go-admin-team/go-admin-core/v2/jwtauth" "github.com/go-admin-team/go-admin-core/v2/sdk/config" - "gorm.io/gorm" + + "go-admin/common/actions" ) -// No database is placed in the context on purpose. The middleware needs one -// only to run the sys_user join, so reaching the handler proves it did not. -func runPermission(t *testing.T, claims jwt.MapClaims) (*DataPermission, bool) { - t.Helper() - gin.SetMode(gin.TestMode) +// The detailed data-permission regression suite (claims parsing, the +// GetOrm-unavailable abort, the SQL each data scope produces) now lives in +// go-admin-core's sdk/contract/actions, alongside the logic itself (PRD 006 +// F3). What is left to test here is the shim's own wiring: that this +// package's exported names still round-trip through the same *gin.Context +// key core's PermissionAction and GetPermissionFromContext use. +// +// This file lives in package actions_test, an external test, deliberately: +// it exercises PermissionAction and GetPermissionFromContext exactly as an +// app/admin Service does, through this package's public API only, not +// through anything internal a forward could paper over. - c, _ := gin.CreateTestContext(httptest.NewRecorder()) - c.Request = httptest.NewRequest(http.MethodGet, "/", nil) - if claims != nil { - c.Set(jwt.JwtPayloadKey, claims) - } - - PermissionAction()(c) - - value, exists := c.Get(PermissionKey) - if !exists { - return nil, false - } - p, _ := value.(*DataPermission) - return p, true -} - -// Permission() returns the query untouched when data permission is off, so the -// lookup feeding it has nothing to feed. It used to run regardless: a sys_user -// join on every list, detail, update and delete, discarded immediately. -func TestNoLookupWhenDataPermissionIsOff(t *testing.T) { - previous := config.ApplicationConfig.EnableDP - config.ApplicationConfig.EnableDP = false - t.Cleanup(func() { config.ApplicationConfig.EnableDP = previous }) - - if _, ok := runPermission(t, jwt.MapClaims{"identity": float64(7)}); !ok { - t.Fatal("the request needed a database even though data permission is off") - } -} - -func TestScopeComesFromTheTokenWhenItCarriesOne(t *testing.T) { +// TestPermissionKeyMatchesWhatPermissionActionSets guards PRD 006's hard +// constraint 4: PermissionKey must be declared as +// `const PermissionKey = contractactions.PermissionKey`, a direct +// reference, never a restated literal (see type.go). PermissionAction is +// core's middleware and always writes under core's own key. This test reads +// the value back with actions.PermissionKey exactly as code outside +// GetPermissionFromContext would - c.Get(actions.PermissionKey) is a real, +// if uncommon, way to read the value go-admin has always allowed, and it is +// the one call site where an independently declared PermissionKey would +// stop working without GetPermissionFromContext's own forward hiding it. +// +// If PermissionKey were ever re-declared as an independent literal in this +// package, a later edit to core's copy would make this test fail without a +// single byte of this package having changed - which is the silent-failure +// mode hard constraint 4 exists to rule out (evaluation S2). +func TestPermissionKeyMatchesWhatPermissionActionSets(t *testing.T) { previous := config.ApplicationConfig.EnableDP config.ApplicationConfig.EnableDP = true t.Cleanup(func() { config.ApplicationConfig.EnableDP = previous }) - p, ok := runPermission(t, jwt.MapClaims{ + gin.SetMode(gin.TestMode) + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + c.Request = httptest.NewRequest(http.MethodGet, "/", nil) + c.Set(jwt.JwtPayloadKey, jwt.MapClaims{ "identity": float64(7), "roleid": float64(3), "deptid": float64(5), - "datascope": "4", + "datascope": actions.DataScopeDeptTree, }) + + actions.PermissionAction()(c) + + value, ok := c.Get(actions.PermissionKey) if !ok { - t.Fatal("the token carried the scope and a database was still needed") + t.Fatal("PermissionAction did not set the key actions.PermissionKey names; the two have diverged") } - if p.DataScope != "4" || p.UserId != 7 || p.DeptId != 5 || p.RoleId != 3 { - t.Fatalf("scope read as %+v", p) + p, ok := value.(*actions.DataPermission) + if !ok || p.DataScope != actions.DataScopeDeptTree || p.DeptId != 5 { + t.Fatalf("value under actions.PermissionKey = %#v, want a DataPermission carrying the token's scope", value) } } -// A token minted before deptid was carried is still valid until it expires, and -// has to keep working - by falling back to the query, which needs a database. -func TestATokenWithoutDeptIdFallsBackToTheQuery(t *testing.T) { - previous := config.ApplicationConfig.EnableDP - config.ApplicationConfig.EnableDP = true - t.Cleanup(func() { config.ApplicationConfig.EnableDP = previous }) - - if _, ok := runPermission(t, jwt.MapClaims{ - "identity": float64(7), - "roleid": float64(3), - "datascope": "4", - }); ok { - t.Fatal("an old token was served from claims it does not have") - } -} - -// PRD 006 F14/H1. A token without deptid/datascope forces the fallback -// query, which needs pkg.GetOrm(c) - and no "db" key is set in this -// context, so GetOrm fails exactly as it would if a tenant's database were -// unreachable. Before the fix, that error was logged and the handler ran -// anyway with no data permission filter at all. -func TestPermissionActionAbortsWhenDBIsUnavailable(t *testing.T) { +// TestGetPermissionFromContextRoundTrips is the same guard from the other +// exported entry point: GetPermissionFromContext must read back exactly +// what PermissionAction wrote, both reached through this package's own +// forwards rather than core's directly. +func TestGetPermissionFromContextRoundTrips(t *testing.T) { previous := config.ApplicationConfig.EnableDP config.ApplicationConfig.EnableDP = true t.Cleanup(func() { config.ApplicationConfig.EnableDP = previous }) gin.SetMode(gin.TestMode) - r := gin.New() - handlerReached := false - r.Use(func(c *gin.Context) { - c.Set(jwt.JwtPayloadKey, jwt.MapClaims{"identity": float64(7)}) - }) - r.Use(PermissionAction()) - r.GET("/", func(c *gin.Context) { - handlerReached = true - c.Status(http.StatusOK) + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + c.Request = httptest.NewRequest(http.MethodGet, "/", nil) + c.Set(jwt.JwtPayloadKey, jwt.MapClaims{ + "identity": float64(7), + "roleid": float64(3), + "deptid": float64(5), + "datascope": actions.DataScopeSelf, }) - req := httptest.NewRequest(http.MethodGet, "/", nil) - w := httptest.NewRecorder() - r.ServeHTTP(w, req) + actions.PermissionAction()(c) - if handlerReached { - t.Fatal("the business handler ran with no database and no data permission filter set") + p := actions.GetPermissionFromContext(c) + if p.DataScope != actions.DataScopeSelf || p.UserId != 7 { + t.Fatalf("GetPermissionFromContext() = %+v, want DataScope=%q UserId=7", p, actions.DataScopeSelf) } } -// PRD 006 F14/H2 and H3. Table-driven over gorm DryRun so the exact SQL -// Permission produces for each scope is pinned down, not just "some WHERE -// clause got added". -func TestPermissionScopes(t *testing.T) { - previous := config.ApplicationConfig.EnableDP - config.ApplicationConfig.EnableDP = true - t.Cleanup(func() { config.ApplicationConfig.EnableDP = previous }) - - db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{DryRun: true}) - if err != nil { - t.Fatalf("open: %v", err) +func TestIsValidDataScope(t *testing.T) { + for _, s := range []string{actions.DataScopeAll, actions.DataScopeCustom, actions.DataScopeDept, actions.DataScopeDeptTree, actions.DataScopeSelf} { + if !actions.IsValidDataScope(s) { + t.Errorf("IsValidDataScope(%q) = false, want true", s) + } } - - const noRows = "SELECT * FROM `t` WHERE 1 = 0" - - cases := []struct { - name string - p *DataPermission - want string - vars []interface{} - }{ - { - name: "all", - p: &DataPermission{DataScope: DataScopeAll}, - want: "SELECT * FROM `t`", - }, - { - name: "custom", - p: &DataPermission{DataScope: DataScopeCustom, RoleId: 3}, - want: "SELECT * FROM `t` WHERE t.create_by in (select sys_user.user_id from sys_role_dept left join sys_user on sys_user.dept_id=sys_role_dept.dept_id where sys_role_dept.role_id = ?)", - vars: []interface{}{3}, - }, - { - name: "dept", - p: &DataPermission{DataScope: DataScopeDept, DeptId: 5}, - want: "SELECT * FROM `t` WHERE t.create_by in (SELECT user_id from sys_user where dept_id = ? )", - vars: []interface{}{5}, - }, - { - name: "dept-tree", - p: &DataPermission{DataScope: DataScopeDeptTree, DeptId: 5}, - want: "SELECT * FROM `t` WHERE t.create_by in (SELECT user_id from sys_user where sys_user.dept_id in(select dept_id from sys_dept where dept_path like ? ))", - vars: []interface{}{"%/5/%"}, - }, - { - name: "self", - p: &DataPermission{DataScope: DataScopeSelf, UserId: 7}, - want: "SELECT * FROM `t` WHERE t.create_by = ?", - vars: []interface{}{7}, - }, - // H2: an unrecognized scope must not read like "all data" any more. - {name: "unrecognized value", p: &DataPermission{DataScope: "6"}, want: noRows}, - // H2/H1: the zero-value DataPermission is what getPermissionFromContext - // and the two "give up and continue" branches in PermissionAction hand - // out when nothing else is available. - {name: "zero value (no scope at all)", p: &DataPermission{}, want: noRows}, - // H3: dept_path always starts with "/0/" (sys_dept.go), so DeptId 0 - // must not be allowed to build a pattern that matches every row. - {name: "dept with DeptId 0", p: &DataPermission{DataScope: DataScopeDept, DeptId: 0}, want: noRows}, - {name: "dept-tree with DeptId 0", p: &DataPermission{DataScope: DataScopeDeptTree, DeptId: 0}, want: noRows}, - {name: "dept-tree with negative DeptId", p: &DataPermission{DataScope: DataScopeDeptTree, DeptId: -1}, want: noRows}, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - stmt := db.Session(&gorm.Session{DryRun: true}). - Table("t"). - Scopes(Permission("t", tc.p)). - Find(&[]map[string]interface{}{}). - Statement - - if stmt.SQL.String() != tc.want { - t.Errorf("SQL = %q, want %q", stmt.SQL.String(), tc.want) - } - if tc.vars == nil { - if len(stmt.Vars) != 0 { - t.Errorf("vars = %v, want none", stmt.Vars) - } - return - } - if len(stmt.Vars) != len(tc.vars) { - t.Fatalf("vars = %v, want %v", stmt.Vars, tc.vars) - } - for i := range tc.vars { - if stmt.Vars[i] != tc.vars[i] { - t.Errorf("vars[%d] = %v, want %v", i, stmt.Vars[i], tc.vars[i]) - } - } - }) + if actions.IsValidDataScope("6") { + t.Error(`IsValidDataScope("6") = true, want false`) } } diff --git a/common/actions/type.go b/common/actions/type.go index 69beb8cc..cb8bf56f 100644 --- a/common/actions/type.go +++ b/common/actions/type.go @@ -1,5 +1,13 @@ package actions -const ( - PermissionKey = "dataPermission" -) +import contractactions "github.com/go-admin-team/go-admin-core/v2/sdk/contract/actions" + +// PermissionKey is a direct reference to go-admin-core's sdk/contract/actions +// constant, not a restated literal - see that package's PermissionKey doc +// comment. PRD 006's hard constraint 4 requires this form for exactly this +// symbol: PermissionAction (below) sets the gin context key it owns, and +// GetPermissionFromContext reads it back; an independently declared literal +// here would let the two silently drift apart if core's copy ever changed +// without this one following. common/actions/shim_test.go carries the +// regression test for that failure mode. +const PermissionKey = contractactions.PermissionKey