From 412413c12f08b7364485a2271c448cb293f941e6 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Wed, 9 Sep 2026 12:27:38 +0800 Subject: [PATCH] =?UTF-8?q?feat=E2=9C=A8:=20record=20which=20casbin=20poli?= =?UTF-8?q?cies=20an=20install=20created?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sys_app_casbin_grant has existed since the registry tables were added and nothing ever wrote to it. An uninstaller reading it would have found it empty, deleted no policy at all, and reported every one of them as an unattributable leftover - which is what "report and skip" looks like when the ledger was simply never written, and is indistinguishable from it working. grantToAdminRole now writes an entry for each policy it creates. The entry carries the tuple casbin_rule is unique on rather than a foreign key into it, because casbin_rule is not this project's table: the gorm adapter's SavePolicy truncates it and writes it back from memory, and SysRole.Update replaces a role's policy rows wholesale. Both rebuild the same tuple from the same sys_menu/sys_api data, so a match on the tuple survives what a row id does not. Only policies this install actually created are recorded - the insert is conditional and its RowsAffected says which. A policy that was already there was granted by somebody else and is not this app's to take away. The two ways that can be wrong are not equally bad, which is what settles it. Under-recording leaves a policy behind and the uninstall says so, because a policy naming this app's own path with no ledger entry is exactly what it reports as an orphan. Over-recording deletes somebody's authorization, silently. Between a visible leftover and an invisible deletion, take the leftover. The ledger insert is itself conditional, for a case the obvious retry test does not reach: on a plain re-run the policy still exists, so the insert is skipped before the ledger is touched. It is reached when the policy row was removed while its entry stayed, and a plain insert would then abort the whole seed on the ledger's unique index. There is a test for that specific shape, and replacing the insert with a plain one turns it red - which the plain retry test does not. Ordering: the ledger table is created by a framework migration, and version strings sort bare digits ahead of any app-prefixed one, so it exists before any application's seed runs. Nothing in the framework's own migrations calls SeedMenus. --- app/admin/service/seed.go | 50 +++++++++++- app/admin/service/seed_test.go | 144 ++++++++++++++++++++++++++++++++- 2 files changed, 189 insertions(+), 5 deletions(-) diff --git a/app/admin/service/seed.go b/app/admin/service/seed.go index e856566e..0e73a59a 100644 --- a/app/admin/service/seed.go +++ b/app/admin/service/seed.go @@ -5,6 +5,7 @@ import ( "fmt" "strconv" "strings" + "time" "gorm.io/gorm" @@ -76,7 +77,7 @@ func (adminSeeder) SeedMenus(tx *gorm.DB, appCode string, menus []seed.MenuSpec, if len(menuIDs) == 0 && len(apiRows) == 0 { return nil } - return grantToAdminRole(tx, menuIDs, apiRows) + return grantToAdminRole(tx, appCode, menuIDs, apiRows) } // seedApis writes one sys_api row per ApiSpec and returns them keyed by @@ -422,7 +423,7 @@ func pascalCase(s string) string { // framework migration sorts before every app-prefixed one - means that // should not happen in practice, but failing this call over it would be // worse than a menu with no grant yet. -func grantToAdminRole(tx *gorm.DB, menuIDs []int, apiRows map[string]models.SysApi) error { +func grantToAdminRole(tx *gorm.DB, appCode string, menuIDs []int, apiRows map[string]models.SysApi) error { var role models.SysRole if err := tx.Where("role_key = ?", adminRoleKey).First(&role).Error; err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { @@ -441,12 +442,53 @@ func grantToAdminRole(tx *gorm.DB, menuIDs []int, apiRows map[string]models.SysA } for _, a := range apiRows { - if err := tx.Exec( + res := tx.Exec( "INSERT INTO casbin_rule (ptype, v0, v1, v2, v3, v4, v5) SELECT 'p', ?, ?, ?, '', '', '' WHERE NOT EXISTS (SELECT 1 FROM casbin_rule WHERE ptype='p' AND v0=? AND v1=? AND v2=?)", role.RoleKey, a.Path, a.Action, role.RoleKey, a.Path, a.Action, - ).Error; err != nil { + ) + if res.Error != nil { + return res.Error + } + if res.RowsAffected == 0 { + // The policy was already there, so this install did not create + // it and it is not this app's to take away. Leaving it out of + // the ledger is what makes an uninstall report it instead of + // deleting it. + // + // The two ways this can be wrong are not equally bad, which is + // what settles it. Under-recording leaves a policy behind and + // the uninstall says so, because a policy naming an app's own + // path with no ledger entry is exactly what it lists as an + // orphan. Over-recording deletes a grant somebody else made, + // silently. Between a visible leftover and an invisible + // deletion of somebody's authorization, take the leftover. + continue + } + if err := recordGrant(tx, appCode, role.RoleKey, a.Path, a.Action); err != nil { return err } } return nil } + +// recordGrant writes down that this application's install created one casbin +// policy, keyed by the same tuple casbin_rule is unique on. +// +// A ledger rather than a column on casbin_rule, because casbin_rule is not +// this project's table: the gorm adapter's SavePolicy truncates it and writes +// it back from memory, which would drop any column added here, and +// SysRole.Update replaces a role's policy rows wholesale. The tuple survives +// both, because both rebuild it from the same sys_menu/sys_api data. +// +// Written with the same INSERT ... WHERE NOT EXISTS shape as the policy above +// rather than a plain insert: the ledger's unique index covers the tuple +// alone, so a duplicate would abort the whole seed instead of being the +// no-op it should be. +func recordGrant(tx *gorm.DB, appCode, roleKey, path, action string) error { + return tx.Exec( + "INSERT INTO sys_app_casbin_grant (app_code, ptype, v0, v1, v2, v3, v4, v5, created_at) "+ + "SELECT ?, 'p', ?, ?, ?, '', '', '', ? WHERE NOT EXISTS "+ + "(SELECT 1 FROM sys_app_casbin_grant WHERE ptype='p' AND v0=? AND v1=? AND v2=? AND v3='' AND v4='' AND v5='')", + appCode, roleKey, path, action, time.Now(), roleKey, path, action, + ).Error +} diff --git a/app/admin/service/seed_test.go b/app/admin/service/seed_test.go index 9fc02557..de220b75 100644 --- a/app/admin/service/seed_test.go +++ b/app/admin/service/seed_test.go @@ -32,7 +32,14 @@ func newSeedTestDB(t *testing.T) *gorm.DB { if err != nil { t.Fatalf("open: %v", err) } - if err := db.AutoMigrate(&models.SysMenu{}, &models.SysApi{}, &models.SysRole{}); err != nil { + // sys_app_casbin_grant is where grantToAdminRole records which policies + // this install created, so an uninstall can tell them from the ones + // somebody granted by hand. In a real database it is created by + // 1786700007000, which is a framework migration and therefore runs ahead + // of every application's - version strings sort bare digits before any + // app-prefixed one. + if err := db.AutoMigrate(&models.SysMenu{}, &models.SysApi{}, &models.SysRole{}, + &models.SysAppCasbinGrant{}); err != nil { t.Fatalf("automigrate: %v", err) } if err := db.Exec(`CREATE TABLE casbin_rule ( @@ -841,3 +848,138 @@ func TestSeedMenusPreservesAHandAddedBinding(t *testing.T) { t.Errorf("the seed's own binding count = %d, want 1 - it must survive the retry too", n) } } + +// Every policy grantToAdminRole creates has to be written down, or an +// uninstall has no way to tell this app's grants from a hand-made one and +// leaves all of them behind. +func TestSeedMenusRecordsTheGrantsItCreated(t *testing.T) { + db := newSeedTestDB(t) + role := seedAdminRole(t, db) + + apis := []seed.ApiSpec{ + {Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET", Handle: "apis.Order.GetPage-fm"}, + {Code: "create", Title: "Create order", Path: "/api/v1/order", Method: "POST", Handle: "apis.Order.Insert-fm"}, + } + if err := (adminSeeder{}).SeedMenus(db, "order", nil, apis); err != nil { + t.Fatalf("SeedMenus: %v", err) + } + + for _, a := range apis { + var n int64 + db.Model(&models.SysAppCasbinGrant{}). + Where("app_code = ? AND ptype = 'p' AND v0 = ? AND v1 = ? AND v2 = ?", + "order", role.RoleKey, a.Path, a.Method). + Count(&n) + if n != 1 { + t.Errorf("ledger rows for %s %s = %d, want 1", a.Method, a.Path, n) + } + } +} + +// A policy that was already there was not created by this install, so it is +// not this app's to take away later. Recording it would mean an uninstall +// deletes a grant somebody else made, and deletes it silently - the opposite +// mistake leaves a policy behind, which the uninstall reports. +func TestSeedMenusDoesNotClaimAPolicyItDidNotCreate(t *testing.T) { + db := newSeedTestDB(t) + role := seedAdminRole(t, db) + + if err := db.Exec( + "INSERT INTO casbin_rule (ptype, v0, v1, v2, v3, v4, v5) VALUES ('p', ?, '/api/v1/order', 'GET', '', '', '')", + role.RoleKey, + ).Error; err != nil { + t.Fatalf("pre-existing policy: %v", err) + } + + apis := []seed.ApiSpec{ + {Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET", Handle: "apis.Order.GetPage-fm"}, + {Code: "create", Title: "Create order", Path: "/api/v1/order", Method: "POST", Handle: "apis.Order.Insert-fm"}, + } + if err := (adminSeeder{}).SeedMenus(db, "order", nil, apis); err != nil { + t.Fatalf("SeedMenus: %v", err) + } + + var claimed int64 + db.Model(&models.SysAppCasbinGrant{}). + Where("v1 = ? AND v2 = ?", "/api/v1/order", "GET").Count(&claimed) + if claimed != 0 { + t.Errorf("the ledger claimed a policy that was already there (%d rows)", claimed) + } + // The one it did create is still recorded: the skip is per policy, not + // for the whole call. + var created int64 + db.Model(&models.SysAppCasbinGrant{}). + Where("v1 = ? AND v2 = ?", "/api/v1/order", "POST").Count(&created) + if created != 1 { + t.Errorf("ledger rows for the policy it did create = %d, want 1", created) + } + // And the pre-existing policy itself is untouched. + var policies int64 + db.Table("casbin_rule").Where("v1 = ? AND v2 = ?", "/api/v1/order", "GET").Count(&policies) + if policies != 1 { + t.Errorf("casbin_rule rows = %d, want the one that was already there", policies) + } +} + +// A migration that failed partway is re-run whole. The ledger must come out +// of a second run the same as the first, not with a duplicate or an error +// from its own unique index. +func TestSeedMenusLedgerSurvivesARetry(t *testing.T) { + db := newSeedTestDB(t) + seedAdminRole(t, db) + + apis := []seed.ApiSpec{ + {Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET", Handle: "apis.Order.GetPage-fm"}, + } + for i := 0; i < 2; i++ { + if err := (adminSeeder{}).SeedMenus(db, "order", nil, apis); err != nil { + t.Fatalf("SeedMenus run %d: %v", i+1, err) + } + } + + var n int64 + db.Model(&models.SysAppCasbinGrant{}).Count(&n) + if n != 1 { + t.Errorf("ledger has %d rows after two runs, want 1", n) + } +} + +// The ledger's own guard against a duplicate, which the plain retry above +// never reaches: there the policy still exists, so the insert is skipped +// before the ledger is touched at all. This is the case that does reach it - +// the policy row was removed while its ledger entry stayed, so the seed +// creates the policy again and writes a ledger entry that is already there. +// A plain insert would abort the whole seed on the ledger's unique index. +func TestSeedMenusLedgerToleratesAnEntryWhosePolicyWasRemoved(t *testing.T) { + db := newSeedTestDB(t) + seedAdminRole(t, db) + + apis := []seed.ApiSpec{ + {Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET", Handle: "apis.Order.GetPage-fm"}, + } + if err := (adminSeeder{}).SeedMenus(db, "order", nil, apis); err != nil { + t.Fatalf("first run: %v", err) + } + if err := db.Exec("DELETE FROM casbin_rule WHERE v1 = ? AND v2 = ?", "/api/v1/order", "GET").Error; err != nil { + t.Fatalf("removing the policy: %v", err) + } + var ledger int64 + db.Model(&models.SysAppCasbinGrant{}).Count(&ledger) + if ledger != 1 { + t.Fatalf("the ledger entry is gone, so this test is not set up: %d rows", ledger) + } + + if err := (adminSeeder{}).SeedMenus(db, "order", nil, apis); err != nil { + t.Fatalf("second run: %v", err) + } + + db.Model(&models.SysAppCasbinGrant{}).Count(&ledger) + if ledger != 1 { + t.Errorf("ledger has %d rows, want 1", ledger) + } + var policies int64 + db.Table("casbin_rule").Where("v1 = ? AND v2 = ?", "/api/v1/order", "GET").Count(&policies) + if policies != 1 { + t.Errorf("the policy was not put back: %d rows", policies) + } +}