Conversation
…n B) Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds multi-tenant isolation support for the internal Casbin GORM adapter, plus SQLite migration robustness fixes, so each enforcer can load/save only the intended tenant’s policies.
Changes:
- Implement tenant-scoped policy loading via a column filter (
filter_field/filter_value) and expose it through Casbin’spersist.FilteredAdapter. - Support per-tenant policy tables via templated
table_nameresolved at adapter creation time using{{.Tenant}}. - Fix SQLite migrator behavior for table/index/column existence checks and adjust index creation to avoid SQLite’s global index-name conflicts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| internal/gorm_adapter.go | Adds filtered + per-table behavior, custom migration/index creation, and filtered adapter support. |
| internal/module_casbin.go | Extends adapter config, resolves table_name templates, and adjusts mutation methods when using a filtered enforcer. |
| internal/sqlite_dialector.go | Introduces an SQLite-specific migrator override for HasTable / HasIndex / HasColumn. |
| internal/module_casbin_storage_test.go | Adds tests for tenant filter mode and per-tenant table mode, plus validation tests. |
Comments suppressed due to low confidence (1)
internal/gorm_adapter.go:269
- When tenant filtering is enabled, RemoveFilteredPolicy does not automatically add the adapter’s tenant filter condition, so a filtered enforcer could still remove policies belonging to other tenants if invoked with broad parameters. Consider always AND-ing the configured filterField=filterValue onto the delete query (or rejecting calls that don’t include it) while IsFiltered()/filter config is active.
// RemoveFilteredPolicy removes policy rules matching the given filter.
func (a *gormAdapter) RemoveFilteredPolicy(sec, ptype string, fieldIndex int, fieldValues ...string) error {
query := a.table().Where("ptype = ?", ptype)
fields := []string{"v0", "v1", "v2", "v3", "v4", "v5"}
for i, v := range fieldValues {
if v != "" {
col := fields[fieldIndex+i]
query = query.Where(col+" = ?", v)
}
}
return query.Delete(&casbinRule{}).Error
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if field != "" && value != "" { | ||
| // field is already validated against validFilterFields (v0-v5), so | ||
| // backtick-quoting is safe and provides defence-in-depth against any | ||
| // future code path that might supply an unvalidated column name. | ||
| q = q.Where("`"+field+"` = ?", value) | ||
| } |
There was a problem hiding this comment.
loadWithFilter builds the WHERE clause using backtick-quoted identifiers (""+field+" = ?"). Backticks are not valid identifier quoting on PostgreSQL, so tenant filtering will fail on the postgres driver. Use GORM’s clause API / dialector quoting to build an identifier-safe equality expression without hard-coding quote characters.
| if filterField != "" && !validFilterFields[filterField] { | ||
| return nil, fmt.Errorf("gorm casbin adapter: invalid filter_field %q (must be v0-v5)", filterField) | ||
| } | ||
| if err := migrateTable(db, tableName); err != nil { |
There was a problem hiding this comment.
newGORMAdapter accepts filterField/filterValue independently. If only one is set (e.g., filter_field provided but filter_value empty), LoadPolicy won’t apply a filter and IsFiltered will never flip, but the config looks like tenant isolation is enabled. Consider validating that filterField and filterValue are either both set or both empty, and fail adapter creation on partial configuration.
| // AddPolicy adds a policy rule to the database. | ||
| func (a *gormAdapter) AddPolicy(sec, ptype string, rule []string) error { | ||
| r := lineToRule(ptype, rule) | ||
| return a.db.Create(&r).Error | ||
| return a.table().Create(&r).Error | ||
| } | ||
|
|
||
| // RemovePolicy removes a policy rule from the database. | ||
| func (a *gormAdapter) RemovePolicy(sec, ptype string, rule []string) error { | ||
| r := lineToRule(ptype, rule) | ||
| return a.db.Where(&r).Delete(&casbinRule{}).Error | ||
| return a.table().Where(&r).Delete(&casbinRule{}).Error | ||
| } |
There was a problem hiding this comment.
When tenant filtering is enabled, AddPolicy/RemovePolicy currently write/delete rows solely based on the provided rule values, without enforcing the adapter’s filter scope. A caller can accidentally insert/delete another tenant’s rows by passing a rule with a different vN value than the configured filter. To make tenant isolation reliable, consider validating that the rule’s filtered field matches filterValue (or overriding it) and returning an error when it doesn’t.
| func migrateTable(db *gorm.DB, tableName string) error { | ||
| if !db.Migrator().HasTable(tableName) { | ||
| if err := db.Table(tableName).AutoMigrate(&casbinRule{}); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
migrateTable skips AutoMigrate whenever the table already exists, regardless of dialect. On postgres/mysql this means existing deployments won’t get schema drift corrected (missing columns, type changes, etc.), which AutoMigrate previously handled. If the intent is SQLite-only safety, consider restricting the “only migrate new tables” behavior to the SQLite dialector, or documenting that existing tables will not be auto-migrated on any DB.
| return db.Exec(fmt.Sprintf( | ||
| `CREATE UNIQUE INDEX %q ON %q ("ptype","v0","v1","v2","v3","v4","v5")`, | ||
| idxName, tableName, | ||
| )).Error |
There was a problem hiding this comment.
migrateTable uses a raw CREATE UNIQUE INDEX %q ON %q ("ptype",...) statement. %q produces a Go string literal (double-quoted with backslash escapes) and the double-quote identifier quoting is not portable (notably breaks MySQL unless ANSI_QUOTES is enabled). Consider using GORM/dialector identifier quoting (e.g., via the dialector’s QuoteTo / clause builders) or a migrator-based index creation approach so the SQL is correct across sqlite/postgres/mysql.
|
@copilot apply changes based on the comments in this thread Fix the linter errors |
…tion, tenant scope Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Applied all fixes from the review thread in commit
|
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Fixed in |
Multi-tenant apps using the GORM adapter currently load all tenants' policies into every enforcer. This adds two independent isolation strategies.
Option A – Column filter (
filter_field/filter_value)Applies a
WHERE <col> = <value>clause on everyLoadPolicy, isolating one tenant's rows in a shared table.Implements
persist.FilteredAdapter.IsFiltered()startsfalseso Casbin callsLoadPolicyduringNewEnforcerinit (Casbin skipsLoadPolicywhenIsFiltered()=trueat construction), then flipstrueafter the first filtered load to prevent accidental bulkSavePolicyfrom clobbering other tenants.SavePolicywith an active filter deletes/reinserts only the scoped rows.CasbinModulemutation methods skip the redundantenforcer.SavePolicy()call whenIsFiltered()is true.Option B – Per-tenant table (
table_nametemplate)table_namenow accepts Go templates resolved at adapter-creation time. The newtenantfield is exposed as{{.Tenant}}.Also fixes a pre-existing bug where
tableNamewas stored ongormAdapterbut never used — all queries were hitting the struct's defaultcasbin_ruletable.SQLite migrator fixes
The custom
sqliteMigratorpreviously fell back to GORM's generic migrator forHasTable,HasIndex, andHasColumn, all of which queryinformation_schema(non-existent in SQLite), causing silent failures and data loss when a second connection calledAutoMigrateon an existing table.HasTable/HasIndex/HasColumnnow querysqlite_master/pragma_table_infomigrateTableonly runsAutoMigratefor new tables; the composite unique index is created separately with a per-table name (uidx_<tableName>) to avoid SQLite's global index-name namespace conflict across multiple tenant tablesSecurity
filter_fieldvalidated against whitelist{v0…v5}; backtick-quoted in the generatedWHEREclause as defence-in-depthtable_namevalidated to allow only[a-zA-Z0-9_-]before use in rawCREATE INDEXSQLOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.