Two properties the previous shape broke silently: GetHandlerFunc must
report ok for the JwtToken key, and every module must read back the same
instance. Reverting the registration to the unbound method expression
still compiles and turns the first of these red, which is the failure
this pins.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Four modules each called AuthInit and built their own instance, so which
one Runtime handed back was decided by whichever module initialised last.
The JwtToken key was also registered as an unbound method expression,
which GetHandlerFunc's type assertion can never match - the key was
registered and unusable at the same time.
The instance is now built once here and registered as a bound closure.
Modules read it back through GetAuthMiddleware, which is fatal rather
than nil when called before InitMiddleware has run: a process without a
JWT middleware should not reach the point of serving a request.
Only one call site needs the instance itself rather than the handler
(admin's /login, for LoginHandler); the thirty-odd MiddlewareFunc() call
sites are unchanged.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
IndexAction closes over one dto.Index and serves every request to the route
from it; Generate exists so each request gets its own instance, and every
implementation in this repository returns a copy for that reason. The probe
returned the receiver, which made it the one shape IndexAction is not
written against - and inconsistent with probeRow in the same file, which
already copied.
A single-request test cannot tell the two apart, so the assertion is on
Generate itself rather than on the action's behaviour.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
create.go/delete.go/index.go/update.go/view.go were not lowered to
core (PRD 006 F3) and still call actions.Permission directly, in this
repository, on a code path core's own test suite knows nothing about:
core pins down what Permission builds for a given scope, but nothing
covered whether this package's five Actions still remember to call it
at all. TestIndexActionAppliesDataPermission runs IndexAction exactly
as a real request would, against a real in-memory database, and
inspects the SQL GORM actually executed - not just that the handler
returned success, which it would just as happily do with the filter
missing entirely.
The SQL is captured through a gorm.io/gorm/logger.Interface wrapper
rather than read back from IndexAction's own *gorm.DB: IndexAction
builds and executes its query in one unbroken chain
(Model().Scopes().Find()...Count()) and never hands the built
statement back to its caller, so there is nothing else to inspect it
through.
Counterproof performed and reverted (not part of this commit): with
Permission(object.TableName(), p) removed from IndexAction's Scopes
call, the test failed with the captured SQL carrying no WHERE clause
at all (`SELECT * FROM action_probe_row LIMIT 10`); index.go was then
restored to its committed content (`git diff --exit-code` verified
clean).
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
DataPermission, PermissionAction, Permission, GetPermissionFromContext,
IsValidDataScope, PermissionKey and the five DataScope* constants now
forward to go-admin-core's sdk/contract/actions, which carries the
already-fixed logic from feat/006-security-prereq (PRD 006 F14/H1-H3).
create.go/delete.go/index.go/update.go/view.go - the five generic CRUD
actions - are untouched: they call Permission and
GetPermissionFromContext by the same names, which now resolve to
forwards with identical behaviour, and stay in this package rather
than moving to core (PRD 006 F3: core's exports are a permanent
promise every fork inherits, and CRUD shape is this framework's most
volatile surface).
PermissionKey is declared as `const PermissionKey =
contractactions.PermissionKey`, a direct reference rather than a
restated literal, per PRD 006's hard constraint 4: PermissionAction
sets this gin context key and GetPermissionFromContext reads it back,
and an independently declared copy could silently drift from core's if
one were ever edited without the other. permission_test.go replaces
the detailed data-permission regression suite - which now lives in
core, next to the logic itself - with a test of this package's own
wiring: that PermissionAction and both of this package's own read
paths (GetPermissionFromContext, and c.Get(actions.PermissionKey)
directly) still meet on the same key.
Counterproof performed and reverted (not part of this commit): with
PermissionKey redeclared here as the literal "dataPermission" and
core's copy changed to a different value, TestPermissionKeyMatches-
WhatPermissionActionSets went red while GetPermissionFromContext's own
round-trip stayed green - confirming the exported constant, not the
GetPermissionFromContext wrapper, is what an independent literal would
put at risk.
PRD 006 F3/F5.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
AutoForm, ObjectById/ObjectGetReq/ObjectDeleteReq, Pagination,
GeneralDelDto/GeneralGetDto and Index/Control are now type aliases of
go-admin-core's sdk/contract/dto; OrderDest, MakeCondition and
Paginate forward to the same package (functions cannot be aliased the
way types can).
MakeCondition no longer reads common/global.Driver to choose which SQL
dialect to resolve search tags against. The lowered version reads
db.Dialector.Name() from inside the closure it returns instead, which
is always the driver the caller's own *gorm.DB is bound to - correct
even with more than one database open with different drivers, which a
single process-wide variable could never be. global.Driver is marked
Deprecated accordingly; it is still set and still readable for fork
code that reads it directly.
PRD 006 F2/F5.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
ControlBy, Model, ModelTime, ActiveRecord, BaseUser, Response, Page,
Migration and the menu type constants now read `type X = pkg.X` /
`const X = pkg.X` against go-admin-core's sdk/contract/models instead
of defining these shapes locally. Every embed, GORM tag and JSON tag
is unchanged - a type alias is the same type, not a new one - and
every existing import of go-admin/common/models keeps compiling with
no changes of its own (verified with `git diff --exit-code` over the
70 files that import common/models, common/dto or common/actions).
The menu type constants (Directory/Menu/Button) are declared as direct
references rather than restated literals: an independently written
copy of the same value can be edited out of step with go-admin-core's,
where a direct reference cannot (PRD 006 hard constraint 4).
PRD 006 F1/F5.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
A table over all five scopes plus the ones that are not scopes, asserting the
generated SQL rather than a boolean, because the defect was that two different
intentions produced the same query.
The rows that matter are the negative ones: an unrecognized value, a zero
value, and a department scope with a non-positive id. Each was verified to go
red with its own fix reverted and the others in place, so a regression names
the defect it belongs to.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
dept_path is built as "/0/" + id + "/..." for every department, so a DeptId of
0 turns the department-tree pattern into '%/0/%' - which matches every row in
sys_dept. The scope meant to narrow visibility to one subtree returned the
whole organisation instead.
Both department scopes now refuse a non-positive id rather than building a
pattern from it. The admin DTO validates deptId, but seed scripts, SSO and
third-party registration paths do not, and after the contract move the caller
is no longer ours to control.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The switch ended in `default: return db`, which is the same answer as "this
role may see everything". That made a legitimate scope indistinguishable from a
broken one: "1" (all data) had no case of its own and fell into default too, so
"1", "", "6" and a zero value all produced byte-identical SQL.
Three changes, in this order, because reversing them would break "1":
- the five scope values become named constants, so a reader can tell which
string means what without consulting the seed data
- "1" gets an explicit case, which is what frees default to mean "not a
scope I recognise"
- default now matches nothing rather than everything
SysRole DTOs accept the scope unvalidated, so an empty string reaches this
switch from ordinary use, not just from a corrupted row.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
PermissionAction logged the error and returned. Gin treats a plain return as
"carry on", so the request reached the business handler with PermissionKey
never set - and a zero DataPermission means Permission() adds no WHERE clause
at all. A database hiccup turned into full visibility, silently.
The neighbouring newDataPermission branch already aborts. This one now does the
same.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
PUT /api/v1/sys-user sits in CasbinExclude so the profile page can reach it,
which means AuthCheckRole never runs for this route. The handler took the
target user id from the request body, so any authenticated caller could edit
another user's record - including their roleId.
The route has to stay excluded: the profile page and the admin user list share
this one endpoint, so removing the exclusion would break self-service editing
for every non-admin role. The check therefore moves into the handler: when the
target is not the caller, the request is put through Casbin explicitly.
EnforceRoleFor carries the same admin short-circuit and enforcement AuthCheckRole
uses, so a route that opts out of the middleware can still ask the same question.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The warning on Authorizator matters to anyone keeping a copy of that file, not
to one particular consumer, and it reads better addressed to all of them: check
what reads those context keys before taking this change.
The map Authorizator receives is built by IdentityHandler in the same file and
carries IdentityKey / UserName / RoleKey / UserId / RoleIds / DataScope - not
user and role. Both assertions failed on every request, and because the ok
result was discarded the five c.Set calls stored zero values and the function
returned true anyway. Nothing in this repository or in core reads those keys.
go-admin-pro has its own copy of this file and does read them; this change
must not be carried over there verbatim.
common/middleware imported app/admin/service/dto for two string constants,
which made a package apps are told to build on depend on one particular app.
LoggerToFile is registered on the engine, so every POST, PUT, GET and
DELETE had its body copied into memory - through a bytes.Buffer, a
ReadAll and a string conversion - before any handler ran. The only
consumer is operParam on the operation-log row, which is written when
logger.enableddb is on, and that is off in the shipped configuration.
There was no size limit either, and a file upload is a POST like any
other: a 1MB request allocated 4.3MB here and a 16MB upload allocated
about 67MB, to build a value nobody stored.
The body is now read only when the operation log will use it, and at
most 32KB of it. The handler still receives the whole request: it reads
the copied part from memory and the rest from the connection, so what
this holds is bounded however large the request is. 32KB also keeps the
value inside the TEXT column it is written to.
The bufio.Writer this replaces was never flushed. Nothing was truncated
only because bytes.Buffer implements io.ReaderFrom, so io.Copy bypassed
the buffer entirely - a different destination would have dropped the
tail of every request body.
AuthCheckRole walks CasbinExclude for every non-admin request, and used
casbin's util.KeyMatch2 to test each entry. That delegates to
util.RegexMatch, which is regexp.MatchString - it compiles its pattern on
every call - so a 32-entry list cost about 2,566 allocations per request
before the request reached Enforce.
Test the method first, which rules out most entries with a string
compare, and take the path test from go-admin-core, whose KeyMatch2
answers the same thing without recompiling. The scan drops to 52ns and no
allocations.
The loop moves out of AuthCheckRole so the tests exercise the code a
request runs rather than a copy of it, and an allocation budget fails if
the uncached matcher comes back.
setupSimpleDatabase runs once per configured database - one per host in
the multi-tenant configuration - and passed the same empty key to
mycasbin.Setup every time. Setup caches per key, so every host after the
first was handed the enforcer built from the first host's database and
was authorized against a casbin_rule table that was not its own.
Takes effect with the go-admin-core release that keys the cache; before
it, Setup ignored the argument entirely.
A rejected request answered 200 with the failure only in the body, so every
layer that reads the status line counted it as served: load balancers, metrics,
client-side retry. A load test against this reported the limiter's own
rejections as successful traffic and overstated throughput more than tenfold.
The threshold was a constant in the middleware, which made 200 QPS the ceiling
of every deployment with nothing in the configuration to reveal it. It now
reads extend.rateLimit.inboundQPS; an absent value keeps 200, so an upgrade
changes nothing, and zero disables the limiter for a deployment behind its own
gateway.
Also drops Strategy: system.BBR. Reading sentinel's source, the adaptive
strategy is consulted only for Load and CpuUsage - for InboundQPS the trigger
count is compared directly - so it read as if the limit adapted to the machine
when it never did.
The startup line printed the DSN whole:
* => goadmin:<password>@tcp(host:3306)/go-admin?...
So every deployment wrote its own database credential into its own logs,
where a log shipper, a support bundle or a screenshot of a terminal
carries it onward. Found while reading deploy output, which is exactly
how it leaks.
The host and username stay - they are what makes the line worth printing
- and only the password is replaced. Both DSN shapes this project accepts
are covered, a sqlite path is left alone, and anything unparseable is
withheld rather than echoed, since it may hold a credential too.
The scope is decided by the user id, the role id, the department and the
data_scope string. Three of the four were already in the token; deptid
was not, though core's user.GetDeptId has always read that claim. Adding
it removes a sys_user join from every list, detail, update and delete.
This goes no more stale than rolekey does, which Casbin has read from
the token since the beginning: both settle on the next login.
A token minted before this still works. Its claims are incomplete, and
the lookup runs for it as before.
Permission() returns the query untouched when EnableDP is false, so the
lookup feeding it has nothing to feed. The lookup ran anyway: a sys_user
join against sys_role on every list, detail, update and delete, with the
result discarded.
enabledp is false in settings.full.yml, so this was the default.
Each implementation keeps its provider client in an interface{} field that
Setup assigns, so an unconfigured store holds nil - and asserting nil to
the provider's client type panics:
panic: interface conversion: interface {} is nil, not *oss.Client
The upload endpoint reaches that path for any request naming a provider
the deployment never configured.
Three more things were wrong in the same files. OXS.Setup printed a
failure and returned the store anyway, handing back exactly the broken
object that panics. HuaWeiOBS.UpLoad printed the provider's error and
returned nil, so a failed upload reported success. Both it and
QiNiuKODO.UpLoad asserted the local path was a string without checking.
The tests asked the reader to paste their own credentials, so they failed
for everyone who did not. They now cover the guards and skip the part
that needs a provider unless credentials are in the environment.
Every import of the module changes, not only the seven packages that
moved out of sdk/pkg: Go requires the major version in the path from v2
on. Both happen in one pass —
go run github.com/go-admin-team/go-admin-core/tools/coreupgrade@v2.0.0 -w -v2 .
go mod tidy
— which is the command the release notes give, run here as a consumer
would run it. 210 imports across 95 files.
The compatibility shims this used are gone in v2, so the paths that
moved had to move: sdk/pkg/captcha, sdk/pkg/jwtauth and its user
package, sdk/pkg/response and sdk/pkg/casbin.
The count of unformatted files is unchanged at 34, none of them touched
by this: the tool reformats a file only if it was already gofmt clean,
so a migration cannot disappear into whitespace.
opens is a map, so opens[c.Driver] on a driver this build does not carry
returns a nil function, and gorm.Open calls it. The operator saw a nil
dereference inside gorm with nothing naming the driver.
sqlite3 is the case that bites: it needs cgo and is only compiled in
under the sqlite3 build tag, so the same config file works on one binary
and dies on another. Resolve the driver first and say which ones this
build supports.
sys_user.username, sys_role.role_key and sys_dict_type.dict_type had no
unique index. Uniqueness was a SELECT COUNT followed by an INSERT, which
two concurrent requests both pass — and login resolves a username with
First, so which of the two accounts answers is whichever the database
returns.
The index cannot be on the key alone, because a soft-deleted row keeps
occupying the name and a deleted user's username could never be used
again. It has to include the delete marker, and the marker has to be
non-null: two live rows are (alice, NULL) and (alice, NULL), and NULL is
not equal to NULL, so an index over a nullable marker admits both. That
is the worst of the three states — a constraint that reads as protection
and binds nothing — and there is a test that demonstrates it rather than
asserting it.
ModelTime.DeletedAt is milliseconds since the epoch now, zero while the
row is live. Sixteen tables carry it; the migration converts each one,
preserving when each deleted row was deleted, then adds the three
indexes.
Written to be re-runnable rather than transactional, because DDL does not
roll back on MySQL and an operator whose first attempt failed halfway
should have nothing to do but run it again. It refuses before altering
anything if a table already holds duplicates, naming them, rather than
letting the index fail and leaving the operator to guess.
The timestamp conversion happens in Go: turning a timestamp into epoch
milliseconds is spelled differently by every dialect this supports, and
these row counts do not justify four versions of it.
The pinned core dated from April, before sdk stopped being a separate module,
so the build resolved sdk packages from the old module and core packages from
the new one. Dropping the separate requirement is what makes the two agree
again.
Most of the diff is renames that came with that: the tenant accessors gained a
ByTenant suffix, GetDb now returns one database and GetAllDb the map, and
casbin moved to v3.
The change that matters is four call sites moving from GetMemoryQueue to
GetQueuePrefix. GetMemoryQueue returns a queue fixed at construction, so the
login log, the operate log and the api check ran in process no matter what the
settings file selected — a second instance saw none of it. GetQueuePrefix
returns whatever the configuration built, which is the point of being able to
configure a queue at all.
Verified against core at main: build and vet clean. The two file_store failures
are unchanged from before this branch; they need cloud credentials.
zhangwenjian