One fixed key meant two probes overlapping - two /ready requests, or two
instances against the one redis, which is the normal deployment - overwrote each
other's value between the write and the read, and each concluded the cache was
broken. A readiness probe that reports false negatives under load pulls healthy
instances out of the pool, which is worse than not probing.
The key now carries eight random bytes. The written value is still read back,
because that is what tells a healthy cache apart from one that answers "miss"
for everything, and the key is deleted afterwards on a best-effort basis - its
error is dropped deliberately, since the verdict is already decided and a cache
that cannot delete what it just wrote is not a reason to refuse traffic.
The first version of the concurrency test had no teeth: the probes are short
enough that the scheduler ran them one after another, so the fixed-key
counter-proof passed three times out of three. The fake cache now holds every
writer until all of them have written, which makes the interleaving the test is
about actually happen. With one key that is a deterministic 15 failures out of
16 - only the last writer's value survives - and with a key per probe, none.
/health returned 200 without asking anything. Whatever it was meant to say, an
orchestrator reading it learned only that a process was accepting connections.
The two questions are not the same one, and the answers differ:
- /health stays a bare 200. It answers "should I restart you", and a process
whose database is unreachable does not want restarting - that turns one
outage into a crash loop and discards the connection pool, the cache and
every request in flight on the way.
- /ready is new. It answers "should I send you requests", fails while a
dependency is unreachable, and fails from the moment shutdown begins.
That last part is what the life-cycle phases bought. BeginDraining sits next to
BeginShutdown, before the server stops accepting, so a load balancer is told to
stop sending while this instance can still finish what it holds. Reversed - and
that is where it was - the connections are cut first and the probe reports it
afterwards.
The queue is deliberately not checked. Nothing on AdapterQueue answers "are you
reachable" without publishing something, the memory backend cannot fail, and a
queue that is down degrades logging rather than stopping requests: a reason to
alert, not a reason to leave the pool.
The cache probe writes and reads back rather than only reading. A cache that
answers "miss" for every key - a client pointed at the wrong server - is
indistinguishable from a healthy one on a read alone.
Every check runs behind a recover, and that is not defensive habit. The test for
"nothing configured" found the reason: GetCacheAdapter builds a wrapper around
whatever is configured and returns it even when nothing is, so the value is not
nil, the cache inside it is, and Set dereferences it. A nil check cannot see
that, and GetQueueAdapter behaves the same way. Whatever the cause, a probe is
the last thing that should be able to take the process down - the caller is
asking whether this instance is well, and killing it is the wrong reply.
The counter-proof compiles and fails: without the recover, the unconfigured case
panics rather than reporting two failed checks.
Setup installs the cache and queue adapters on sdk.Runtime, which is a
package-level singleton, and records what it installed in this package's own
variables. The test left all of it behind, so what a later test in this binary
saw depended on whether this one had run - the leak cmd/api's freshRuntime and
common/middleware's copy of it exist to prevent.
sdk.Runtime is swapped for a fresh one and everything is put back in Cleanup.
Issue #892 is that a reload replaces the queue adapter and the consumers
registered against the previous one are left attached to a queue nobody
publishes to any more.
The fix has two halves and only one of them was covered. attachQueueConsumers
gives a new queue its own consumers and the same queue none, which cmd/api
tests against a queue it controls by passing generation numbers in by hand. What
nothing asserted is that a reload actually produces a new generation for it to
notice - the half that lives in this package.
Setup is what config re-runs on every change, so calling it twice is what a
reload does here. The generation goes 0, 1, 2.
The counter-proof compiles and fails: dropping the increment in setupQueue
reports that the first Setup installed no queue at all, because a generation
that never moves is indistinguishable from never having been set.
This closes the loop rather than adding coverage for its own sake: with both
halves asserted, the claim that #892 is fixed rests on tests instead of on
reading the two functions and believing they meet.
AfterListen promises a hook that the port answers. The bind was moved onto the
caller's goroutine to keep that promise, but with ssl enabled there was a second
way to fail after the announcement: ServeTLS reads the certificate files itself,
on the serving goroutine, so a bad path or an unreadable key surfaced once the
hooks had already run.
tls.LoadX509KeyPair now runs before anything is announced, and its error is
returned from startServing the way a failed bind is. ServeTLS still does the
real work - handing it a tls.Listener built here instead would take over the
HTTP/2 negotiation it sets up, and quietly drop h2 for every TLS deployment. The
cost is one extra read of the certificate at startup.
The fatal in the serving goroutine said "listen:". Neither the bind nor the
certificate reaches it any more, so it says "serve:".
The test covers the certificate path alongside the bind: neither may announce
the phase, and neither may seal it.
The counter-proof is not clean, and saying so is the point. Removing the check
does turn the run red, but through log.Fatal killing the process from the
serving goroutine - "fatal serve: open no-such.pem: no such file or directory" -
rather than through the assertion. That still demonstrates the defect, because
the process could only get there after startServing had returned successfully
and the phase had been announced; it cannot be observed from inside the test,
because the fatal races the assertion that would report it.
Also: the redis-backed queue tests now fail instead of skipping when CI is set
and GO_ADMIN_TEST_REDIS_ADDR is not. A workflow that renamed the variable or
dropped the service would otherwise stay green while those two tests quietly did
nothing - the same shape as the defect they exist to cover. Locally, with no CI
in the environment, they still skip.
buildRouter is split out of run() so the order it establishes can be asserted:
the phase is announced, then initRouter builds the engine, then runStartupHooks
drains the registries.
The test covers both halves of the distinction the contract draws. A
BeforeRouter callback sees no engine - that is what the phase means, the last
point at which a module can still affect how routes are built. A callback in the
before registry, two lines later, sees one. The names invite treating them as
the same moment and they are not.
No database is involved. AuthInit reads ApplicationConfig.Mode and JwtConfig and
nothing else, and building a router registers handlers rather than calling them,
so the whole sequence runs in a package test with two package-level values set.
freshRuntime swaps the global runtime for the duration: runStartupHooks seals
the registries it drains, and a sealed registry silently drops everything
registered afterwards, which would leave every later test in this binary passing
while proving nothing.
The counter-proof compiles and fails - announcing the phase after initRouter
reports "BeforeRouter saw engine &{...}, want nil".
The rule that consumers are registered before the queue is started had no test
that could fail on the backend it exists for. Everything so far ran on the
memory queue, which is the default: queue.Memory's Register starts another
consumer goroutine whatever the state, so the wrong order passes there. A suite
that only exercises the default reports success for a queue that accepts no
consumers at all.
CI gets a redis service, and two tests build the queue the way setupQueue does -
through config.QueueConfig.Setup, so what is under test is the adapter this
repository actually gets, LegacyQueueAdapter included. They skip without
GO_ADMIN_TEST_REDIS_ADDR, so a developer with no server still gets a green run.
Registering first and starting second delivers the message. Starting first and
registering second is refused: no consumer group was created, so Append comes
back with storage.ErrNoHandler. Pinning that particular error rather than "some
error" is deliberate - the test is about the missing consumer, and a connection
failure that happened to error too would otherwise pass for it.
Running it corrected something written two commits ago. The claim there was
that a late registration loses consumers "with nothing said". Only half of that
holds: the registration is silent, because Register returns nothing, but every
publish afterwards fails loudly - ErrNoHandler, logged at error level by both
call sites in common/middleware - while the log rows are never written. The
symptom is missing rows plus a lot of noise, not a quiet nothing. That commit's
message and the contract doc both say so now.
core's contract says what the four phases promise. This says which line of
cmd/api/server.go each of them is, which is the part an application author
cannot read out of core.
Three things are recorded because getting them wrong is silent:
- BeforeRouter is not the before registry. Those callbacks run from
runStartupHooks, which is after initRouter has built the engine; the phase
is before it. The two are one line apart in the same function and describing
them as equivalent is the mistake this paragraph exists to prevent.
- AfterResource runs again after every configuration reload, so a callback
there is idempotent with respect to a resource rather than doing nothing
the second time. The queue consumers are the worked example, in both
directions: a new adapter must get consumers, the same adapter must not get
them twice. Identity has to come from where the resource is created -
GetQueueAdapter and GetQueuePrefix build a fresh wrapper per call, so two
of them never compare equal however often the adapter underneath changed.
- Consumers must be registered before the queue is started, and which
implementation is behind the interface decides whether that matters:
QueueConfig.Setup hands back queue.NewMemory when there is no redis
section, and that one does not care; a redis section reaches
LegacyQueueAdapter, whose Register cannot report a refusal. The table says
so rather than leaving "only on redis" as a claim. What is silent is the
registration alone - every publish afterwards is refused with ErrNoHandler
and logged at error level, so the symptom is missing log rows plus a lot of
noise, not a quiet nothing.
The third-layer table loses its "queue consumers are lost after a hot reload"
row, which is what AfterResource is for, and gains the honest replacement: the
four phases are the only mount points there are. There is no "after the routes
are installed, before the socket is listening".
AfterListen is described as the port being bound rather than Serve being in its
accept loop. Serve runs on another goroutine and may not have reached the first
Accept; what is true is that the bind returned, so the kernel is queueing
connections. A bind that fails produces no phase at all - the error returns
from run() and the banner never prints.
The per-tenant setup ended with `defer crontab.Stop()` on the line above
`select {}`. The select never returned, so the deferred call was unreachable
for the life of the process: the scheduler had never once been stopped. And
because setup never returned, the `for k, db := range dbs` loop in Setup never
reached its second iteration - with several tenant databases configured, only
whichever one came first out of the map ever got a scheduler at all.
Both fall out of deleting the select, which was blocking for nothing: cron.Start
is `go c.run()` and has never needed anything to hold the caller.
The stop becomes a BeforeExit callback. cron.Stop returns a context that closes
once the jobs already running have finished, so the shutdown budget has
something real to bound - and giving up on that wait leaves those jobs running
until the process exits, which is better than holding the whole shutdown open
for one job that will not end.
Startup moves from a bare goroutine in run() onto AfterListen. Two reasons: the
phase runs behind core's panic guard, which does not reach across a goroutine
boundary, so a panic while loading jobs used to take the process down; and the
jobs it starts can call the API, which is only true once the socket is
accepting. It can be synchronous now precisely because setup returns.
Tested where it can be: startCrontab is split out so a scheduler can be started
with no database in sight. The job runs every second; after RunShutdown, two
and a half seconds of silence is the assertion. The counter-proof - registering
no callback, which is what this commit replaces - compiles and reports "the job
fired 2 more times after shutdown".
There is one test, not several, because BeforeExit closes to further
registration once it has run; a second RunShutdown in the same binary would
find an empty registry and pass while proving nothing.
**The multi-tenant half has no test.** setup needs a *gorm.DB per tenant before
it reaches the line that was blocking, and this repository's CI has no database
- `make build` is CGO_ENABLED=0 with no sqlite tag. It is the same defect
though: the loop could not advance past a call that never returned.
setupQueue ended with `go queueAdapter.Run()`, and the three log consumers were
registered afterwards, from setup(). The contract implementations refuse a
registration once the queue is running - memqueue and the redis queue both
answer storage.ErrQueueAlreadyStarted - and Register cannot report it: it
returns nothing, which its own comment in core records as the reason the
interface is deprecated. Start first and register second, across two
goroutines, and the registration is dropped without the caller being able to
tell.
What follows is not quiet. No consumer group was created, so redis refuses
every later publish with storage.ErrNoHandler, and go-admin logs that at error
level from both call sites while the login and operation log rows are simply
never written. The silence is in the registration; the cost shows up on every
request after it.
Which implementation is behind the interface depends on the configuration.
config.QueueConfig.Setup returns queue.NewMemory directly when there is no
redis section - and that one does not care about the order, because its
Register just starts another consumer goroutine. Only a redis section reaches
storage.LegacyQueueAdapter, which wraps the contract implementation and
therefore refuses. So the defect is invisible in the default deployment and
shows up only where redis is configured, dropping the login log, the operation
log and the api check - the three things #892 was about.
The start therefore moves to the code that registers, and nothing starts the
queue but that.
The registration also moves onto AfterResource. It has to: a reload rebuilds
the adapter, and consumers attached to the one that existed at start-up are
attached to a queue nobody publishes to any more. Being on that phase means
running again on every reload, so the callback is idempotent with respect to a
given queue rather than "does nothing the second time" - registering twice on
the same queue would give every message two consumers and write every log row
twice.
Identity for that comes from common/storage, where the adapter is built, as a
generation counter. It cannot come from the accessors: GetQueueAdapter and
GetQueuePrefix build a fresh runtime.Queue wrapper on every call, so comparing
two of them compares two wrappers and never matches however many times the
adapter underneath has been replaced. A counter also keeps the comparison on a
uint64 rather than an `==` between two interface values, which would panic on
an adapter type that is not comparable.
Generation 0 means the configuration has no queue section, so nothing was
installed and the runtime hands back its own memory queue. That case still gets
consumers, because the registration this replaces was unconditional and
dropping it would stop the logs for anyone who commented the section out.
Two things fixed on the way past:
- `if q := sdk.Runtime.GetQueueAdapter(); q != nil { q.Shutdown() }` was
always true. GetQueueAdapter never returns nil - with nothing configured
the runtime falls back to its own memory queue and wraps that - so the
first start shut down the fallback queue before anything had used it. Only
an adapter this package installed is shut down now.
- config.Setup becomes bootstrap.SetupConfig, which is what announces
AfterResource, and announces it after the callbacks that build the
resources rather than before.
attachConsumersOnce is split out so the order and the once-ness can be checked
against a queue the test controls; neither can be read back out of a real
adapter. Four tests cover the ordering, both directions of the idempotency
rule, and the unconfigured case. Both counter-proofs compile and fail: calling
Run before the registrations reports each of the three as "came after Run", and
dropping the generation guard reports eight calls where four are wanted.
One honest limit: the counter-proof for the ordering makes Run synchronous.
The original arrangement started the queue on another goroutine, and a race
cannot be made to fail every time - which is the reason the order is enforced
by structure here instead of being left to be noticed in use.
A module can now register cleanup and have it happen. Until this commit the
process stopped serving and returned; anything a module had set up went down
with the process rather than being taken down.
BeginShutdown is said first, before anything is dismantled. Without it a
configuration reload arriving in this window re-runs AfterResource - rebuilding
the pool and the queue adapter and re-registering consumers - on top of cleanup
that has already run.
The cleanup runs whether or not Shutdown reported an error, which is the whole
reason that error stopped being fatal in the first place: Shutdown fails exactly
when connections were still in flight, and that is when there is most left to
take down.
The two budgets are spent one after the other, so what has to fit inside the
orchestrator's grace period is their sum. `docker stop` allows 10s by default
before SIGKILL; 5+3 leaves room to finish returning. Raising one without
lowering the other buys nothing.
Both halves are tested through the existing subprocess child, which now
registers a BeforeExit callback of its own:
- after a Shutdown that timed out, the callback still runs. Moving the call
into the success branch reports "the BeforeExit callback did not run after
a failed Shutdown".
- a callback that outlasts its budget is abandoned, not awaited. It sleeps
two seconds against a 300ms budget; RunShutdown reports the deadline, the
process exits cleanly inside one second, and the callback's own marker
never appears. Widening the budget to five seconds makes the test time out
waiting for the exit, which is what "awaited" looks like.
Both counter-proofs compile and fail.
Two phases are now announced from the command that serves traffic, so a module
can attach to them instead of being called by name from here.
The listener is opened by this goroutine rather than left to ListenAndServe,
which binds on the goroutine that serves. That mattered for the phase: a hook
on AfterListen is promised a reachable port, and with the bind happening out of
sight there was no way to keep that promise - "address already in use" surfaced
on a goroutine nobody read, after the banner had already announced the server
was up. It is now returned from run() and the process exits non-zero without
claiming anything.
AfterListen is announced synchronously. Moving it to a goroutine to save the
few milliseconds would let it overlap the shutdown, and on a fast SIGTERM the
cleanup callbacks could finish before the startup ones did.
What is left in the serving goroutine is still log.Fatal, deliberately: the
bind is no longer among the errors that reach it, so what remains is a serve
that failed after the port was taken, and carrying on would park the process on
<-quit with nothing serving. ServeTLS is the one case that can still fail
immediately, since it reads the certificate files - with ssl enabled a hook can
still run against a server on its way down. That is not a regression (the old
code printed the banner in the same situation) and it is not fixed here.
BeforeRouter is placed before initRouter, which is a different moment from the
before registry runStartupHooks drains: those callbacks run after the engine
has been built, not before it.
AfterListen is tested here, in one test rather than two because the phase seals
itself once it has run: a second test would find a closed registry and pass
while proving nothing. Both counter-proofs compile and fail - announcing on a
failed bind reports "AfterListen ran 1 times after a failed bind", and
`go RunPhase(...)` reports "ran 0 times, want 1" against the hook's own pause.
BeforeRouter's placement is not asserted in this commit. The test for it comes
with the buildRouter extraction later in this branch.
It carries the life-cycle phases and the shutdown registry. Nothing here uses
them yet - the wiring is the commits that follow, and keeping the bump on its
own means a bisect can tell "the dependency moved" apart from "the host
started calling into it".
The comment claimed bindings were collected as the body was walked so that a
registration only saw definitions above it. That is the single-pass design this
started as. The code does two passes - one to collect, one to report - and a
registration therefore sees every binding in the function.
That is the point rather than an accident: a `.Use` written below a route is
still part of the chain, because the chain is assembled before anything is
served. The price is that a name reused for two different things in one
function resolves to the last assignment, which the comment now says instead of
promising an ordering the code does not keep.
GetPermissionFromContext cannot fail. When no middleware put a *DataPermission
in the context it returns the zero value, and the zero value's DataScope is the
empty string - which is not one of the five scopes Permission recognises, so it
takes the default branch and fails closed. The query is handed `1 = 0` and
matches nothing.
The endpoint then reports "not found" or "no permission" for rows that plainly
exist, and only where enabledp is true. With data permissions off - the
repository default - Permission returns the query untouched and the missing
middleware costs nothing at all. A test suite and a CI that run on the default
cannot see it.
That is what happened to /api/v1/getinfo: it read the permission on a group
carrying only the JWT middleware, so every login on a deployment with data
permissions enabled ended in a 401 from the endpoint the browser calls
immediately after signing in, and went back to the login page. Three /sys-api
routes had the same shape.
The check matches a handler to the group it is registered on, through the AST
rather than through the text - a scratch grep for the same thing reported four
false positives from a comment that happened to contain the function's name,
and before that, a dozen from matching handler names across packages. Handlers
are keyed by package, type and method, so two SysUser types are two handlers.
Subgroups inherit their parent's chain, as gin does, and a `.Use` written below
a registration still counts, because the chain is assembled before anything is
served.
Either half is a fix and the message says both, because which one is right
depends on the route. A handler reading other people's rows wants the
middleware. A handler reading the caller's own row - id from the token - wants
no scope at all: DataScopeSelf matches on create_by, so scoping a self-read
rejects every user who did not create their own account. Reporting only "add
the middleware" would have turned /getinfo from broken into worse.
Five tests: the mistake, both fixes, subgroup inheritance, and a same-named
handler in another package. TestThisRepositoryIsClean covers the real tree, and
it is what fails on the commit before this one - four findings, all real.
Logging in on a deployment with enabledp: true ends on the login page. The
login itself succeeds - sys_login_log records it - and then /api/v1/getinfo
answers 401 "登录失败", which sends the browser straight back.
The query behind it reads:
SELECT * FROM sys_user WHERE sys_user.user_id = 1 AND 1 = 0 AND deleted_at = 0
The 1 = 0 comes from the data-permission scope. GetInfo asked for a permission
with GetPermissionFromContext, but the group this route sits in installs only
the JWT middleware - no PermissionAction - so nothing ever put one in the
context and what came back was the zero value. An unset scope is not one of the
five recognised ones, and since unknown scopes began failing closed rather than
silently matching every row, that zero value now means "match nothing".
The route was working by accident before, and only on deployments that enable
data permissions: the repository default is enabledp: false, where Permission
returns the query untouched. That is why the local suite and CI are both green
and the demo site is not.
Two different faults, so two different fixes:
/getinfo reads the caller's own row - the id comes from the token. A data
scope answers "whose rows may this user see", so there is nothing left for it
to restrict, and applying one is not a stricter version of the query but a
broken one: DataScopeSelf matches on create_by, and an account is created by
whoever added it, so a scoped self-read would 401 every user who did not create
their own account. It now goes through GetSelf, which does no scoping at all -
which is how GetProfile has always read the same row.
/sys-api is the opposite case. Its three handlers do read the permission, and
they are listing and updating other people's rows, so the middleware belongs
there and was simply missing. Added.
Those four endpoints were found by checking every handler that reads the
permission against the group it is registered on. The check reports four before
this commit and none after.
No test. Both paths need a *gorm.DB with sys_user and sys_role rows before they
reach the line that matters, and this repository's CI has no database - `make
build` is CGO_ENABLED=0 with no sqlite tag. What can be tested is the shape of
the mistake rather than its effect, and that belongs in tools/checksilent as a
rule of its own; it is not in this commit because a site that cannot be logged
into should not wait for it.
Moving the dial to just before the shutdown removed one flake and introduced
another: Shutdown only waits for connections the server has already accepted,
so calling it in the gap between the dial and the accept finds nothing to
wait for and returns cleanly. The test then fails on its own "this proves
nothing" guard - which it did, after passing once.
A ConnState hook closes both gaps deterministically. The connection is opened
late enough not to age past the five seconds net/http stops counting it at,
and the child does not proceed until the server has taken it off the
listener.
Ran five times in a row rather than once, because a single green run is what
made the previous version look fixed.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
net/http stops counting a StateNew connection against Shutdown once it is
more than five seconds old. The connection was opened when the child started
and the parent then waited for readiness before signalling, so on a slow run
the connection could age past that mark and Shutdown would succeed - and the
test would fail on its own "this proves nothing" guard rather than on the
behaviour it is there to pin.
Opening it immediately before the shutdown keeps the timeout deterministic.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The previous commit split arming from waiting so a caller could arm first,
wrote a comment saying a signal landing in between reaches the default
handler and kills the process, used it that way in the subprocess test - and
then left run() calling the combined helper after the whole readiness banner.
The window it warned about was still there in the one place that ships.
The signals are now armed before the server starts serving, and the wait
happens where it did. The disposition is restored right after the first
signal rather than deferred, so a shutdown that hangs can still be
interrupted by a second one.
waitForStopSignal goes away: run() was its only caller, and what was worth
keeping from its comment is now on armStopSignals.
Note that no test covers this ordering. The subprocess test drives
armStopSignals directly, which is what makes it a test of the mechanism
rather than of run(); moving the call back below the banner leaves it green.
Verified by reading the sequence in run(), not by a failing test.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Three defects on one path, none of which could be seen from the code alone.
**SIGTERM was never registered.** signal.Notify listened for os.Interrupt
only, and Go terminates the process outright for a signal nothing handles.
`docker stop`, a Kubernetes pod deletion and `systemctl stop` all send
SIGTERM, so every line of the graceful shutdown below the wait was dead code
outside a terminal: measured on a real binary, SIGINT printed "Shutdown
Server ..." and "Server exiting" and SIGTERM printed neither.
**A stuck shutdown could not be interrupted.** quit is buffered and
signal.Notify stays armed after the first delivery, so further signals only
refill the buffer. That was harmless while SIGTERM went to the default
handler - it was the escape hatch. Registering it removes the hatch, so the
disposition is now restored once the first signal is taken, and a second one
kills the process the default way. Arming is split from waiting so a caller
can arm before it announces readiness; a signal in between reaches the
default handler, which is the very failure being fixed.
**A failed Shutdown skipped everything after it.** log.Fatal is an
unconditional os.Exit(1), and Shutdown reports an error precisely when
connections were still in flight - the moment the cleanup that follows
matters most. It is an error now, and the process carries on.
That failure is closer than it looks. net/http only treats a StateNew
connection as idle once it is over five seconds old, so a connection opened
shortly before the signal that has sent nothing holds the whole budget: with
the shipped settings.yml (readtimeout 1) the server closes it first and
shutdown takes 5ms, but with settings.demo.yml (readtimeout 10000) the same
connection made shutdown take 5.04s and exit 1, printing no "Server
exiting". The default configuration is what has been hiding this.
The wait and the shutdown are extracted so the subprocess tests can drive the
real functions against an empty http.Server: CI has no database, and none of
this needs one.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The note explained the empty-shim summary as the expected state "until the
contract packages are lowered into core". They are lowered, and the shims
exist - so a count of zero now means they stopped being aliases, or stopped
being here, which is the interesting case rather than the ordinary one.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The contract-shim-alias message built its suggested line from path.Base of
the import path, so it told the author to write
type ControlBy = models.ControlBy
in a file whose import is `contractmodels "…/sdk/contract/models"`. Every
shim in this repository aliases that import, so the suggestion never
compiled as written - in the one message whose whole job is to be pasted in.
qualifiedType already read the in-source identifier to resolve the import;
it now returns it.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Widening menu-sort-overflow to see a contract MenuSpec made it fire on
app/admin/service/seed_test.go, on the case that asserts SeedMenus rejects
a sort of 900. The check was reading the proof that it works as a defect.
That is not specific to this one guard: menu-sort-overflow,
config-value-truncation, menu-id-collision and modeltime-mix are all about
a value that reaches a real database through a migration, a test fixture
reaches none, and every one of those guards needs a test that writes the
value it rejects. Skip _test.go in all four.
The two import and alias checks keep scanning tests - those are about the
dependency graph, where a test file's import is as real as any other, and
TestContractImportBoundaryCoversTestFiles already pins that.
Both directions are covered: a fixture is ignored, a real seed is still
reported.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The naming rule was documented; what happens when it is broken was not.
It now panics naming the offending file, which is worth saying out loud
because the alternative it replaced was silent: a name that is not a
timestamp used to register as its own version, and that migration would
never run and never report anything.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Sort has an upper bound: sys_menu.sort is built as a tinyint, sqlite ignores
the width, and an overflow surfaces as Error 1264 partway through a migration
rather than as a rejected value.
MenuSpec carries no menu name, and the host synthesises one from the app code
and the spec code rather than using Code directly - two applications both
choosing "list" would otherwise share a keep-alive cache key on the frontend.
Nothing in the type says so, and every Seeder implementer would have to
rediscover it.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The sort-overflow check recognised a SysMenu literal from this repository's
model packages and nothing else. An application installed from outside cannot
reach that type - it describes the same row as a seed.MenuSpec and hands it to
the host's Seeder - so the check went quiet for exactly the author furthest
from the schema it protects.
Not hypothetical: this repository's own reference application shipped a Sort
of 200, past the tinyint sys_menu.sort is built as, and this check passed it.
sqlite ignores the width, so it would have surfaced first on a real install,
as Error 1264 partway through a migration with everything after it unapplied.
The check still cannot see an application in the module cache; that half is
the Seeder's runtime validation. This closes the half that is in the tree.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Two of the three bullets on the contract surface disagreed with
docs/contract.md and with core's own. Registration is constrained by
ordering - it must happen before the startup hooks run - not by being
written inside `init()`; and the claim that core's setters take no lock
is not true of them. The check table gains the new alias rule.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The list of stable packages named four packages of this repository, on
the stated grounds that deduplicating app/demo's imports produces
exactly those four. That reasoning was wrong in the one direction that
matters: it sends a third-party author to depend on the host, and the
host is a fork that every user edits. `go-admin` is also not a
resolvable module path - it has no dot in its first element - so an
application cannot require it at all without a replace directive, which
is ignored outside the main module.
Rewritten around what core promises instead, and around a different
question: not "which packages does an application import" but "which
conventions fail without saying anything". Those are now spelled out
one by one, each with the mechanism that makes it silent - the response
envelope the frontend reads by `code`, the tenant-scoped connection,
`create_by` and the soft-delete marker, the data-scope middleware, the
transaction shape, and the `apps/` prefix a packaged application's menu
component must carry.
Also states two things the document was missing: installing an
application means trusting it with the host's database connection, at
the same level of trust as importing any other Go package - there is no
sandbox here and this does not pretend otherwise - and wiring an
application in touches two places, not one, where missing the second
means the migrations simply do not run, with no error.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
A shim of a core contract type written as `type X pkg.Y` instead of
`type X = pkg.Y` keeps the fields and drops the method set, so anything
embedding it stops satisfying the interfaces it satisfied before.
The compiler catches that only where the method set is actually
exercised. This repository exercises some of the contract types through
an interface and some not at all, so the ones it does not exercise
compile here and break in a fork or a third-party application - which
is the half nobody is watching.
The trigger is the right-hand side of the declaration rather than a
list of package names, so it covers whatever the lowering ends up
shaping without a list to keep in step. Until the contract packages
land in core there is nothing here to guard, and the summary says so
rather than letting the silence read as a pass.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The comment called Visible "0" "hidden by default" and then said an
administrator should not have to unhide the menu - which cannot both be
true. "0" is shown; every menu this repository seeds, including the demo
product menu that is visible on the demo site, uses it.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
grantToAdminRole does two independent things - it grants the menus to the
admin role and writes a casbin rule per api - and SeedMenus skipped the
whole call whenever the menu list came back empty.
An application is free to register apis with no menus: endpoints another
service calls, a webhook, a UI mounted somewhere else. Those installs wrote
their sys_api rows and then no casbin rule for any of them, so every one of
those endpoints was denied to everyone, admin included - from a migration
that reported success and left rows in the table to prove it had run. There
is nothing to look at afterwards that says what went wrong.
Guard on both lists instead, so nothing registered stays a no-op and apis
alone still get granted.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The host kept its own copy of the version-naming rule, byte-identical to
the one in contract/migration: slice the leading 13 characters, no check.
Two copies of a convention that applications also have to follow is two
things to keep in step, and the copies had already stopped matching - core
now rejects a name that carries no timestamp, and this one still accepted
"add_orders.go" and registered a migration under that string as its
version, which nothing would ever match and nothing would report.
Delegate instead, so there is one implementation of the rule and an app's
migration and a host migration derive their version the same way.
The test pins the reject case, not just the happy path: a re-divergence
that only sliced would still pass the happy path.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
core's seed package defines what an application may ask for and leaves the
writing to the host, which is the only side that knows its own tables. No
host implemented it, so SeedMenus returned ErrNoSeeder and an application's
menus never appeared.
adminSeeder writes all four kinds of row, not the two an obvious reading
would stop at: without sys_menu_api_rule and the sys_role_menu / casbin_rule
grants, the menu exists and no role can reach it.
Ids are always autoincrement, never caller-assigned - checksilent's
menu-id-collision check reads literals in this repository's tree and cannot
see an application in the module cache, so the collision is removed by
construction instead of guarded. The runtime validation covers what a static
scan cannot reach for a third-party spec: duplicate codes, unresolved parents
and api references, an unknown kind, and a sort outside sys_menu.sort's
tinyint range.
MenuSpec carries no menu name, so one is synthesised from the app code and
the spec code - two applications both choosing "list" would otherwise collide
on the frontend's keep-alive key.
It lives in app/admin/service because cmd links both subcommands into one
binary, so its init runs whichever one is invoked, and cmd/migrate never has
to import app/admin to reach it.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
sys_migration already carries app_code; sys_menu and sys_api did not, so
nothing said which application seeded a row - which is what an uninstall or
an audit would have to ask.
The migration adds the columns through the runtime models rather than
cmd/migrate/migration/models, whose frozen ModelTime is wrong for anything
ordered after the soft-delete conversion.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
core's sdk/contract/migration keeps its own process-wide registry, because
that is the only door open to an application that must not import the host.
Nothing here ever opened it: ForApp("crm").SetVersion(...) compiled,
registered, and then never ran - no error, no mention in status, nothing.
mergedEntries unions the host's own registry with contract/migration's
Snapshot(), and status, run and AppCodes all read through it, so migrate,
status, --dry-run and --app see an application's migrations exactly as they
see the host's. Version namespacing already keeps the two apart, so a key
collision should not be reachable; the host's own registration wins if one
ever is, rather than being silently replaced.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The comment said the test had to be declared first because Go runs a
package's tests in source order. That is not a guarantee, and it is not what
makes this work: the test that registers RoleCheck puts it back in a
t.Cleanup, and the guard here turns a wrong order into a loud failure rather
than a silent pass. Verified with go test -shuffle on seeds that run the two
in either order.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The Create handler's @Param named dto.OrderCreateReq, but this file imports
that package as orderdto. swag stops on it:
ParseComment error ... cannot find type definition: dto.OrderCreateReq
The @Success annotations name models.Response, which resolves - through
--parseDependency - to core's sdk/contract/models.Response rather than to
this package. That is the right envelope, and worth a note next to the
import, because the obvious "correction" is wrong: core's response.Response,
which the framework's own handlers name, carries no data field, so switching
to it would document these endpoints as returning no payload.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
sys_menu.sort is `gorm:"size:4"`, which MySQL builds as a tinyint holding
-128..127. Sort: 200 passes every sqlite test - sqlite ignores the width -
and fails on a real install with Error 1264, partway through a migration.
This is the exact incident class checksilent's menu-sort-overflow check
exists to prevent, and it reached a hand-written deliverable anyway: that
check only recognises a SysMenu literal from the host's model packages, so
a seed.MenuSpec is invisible to it. Widening the check is tracked
separately; this is the value it would have caught.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Registration goes through core's package-level facades: SetAppRouters for
the routes, migration.ForApp for the schema, and seed.MenuSpec/ApiSpec
for the menu rows - none of which requires importing the host.
The menu component is spelled apps/order/order/index. The frontend tells
a packaged view from a built-in one by that first segment alone, and
getting it wrong is silent: the page falls back to the not-installed
placeholder while the console names a src/views path that was never going
to exist. The tests assert that prefix, that every Parent reference
closes, and that every ApiCode resolves - the three ways a menu graph is
wrong without anything saying so.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
No generic CRUD action anywhere: real business - a cross-table order
placement, a payment transition - is what the contract surface has to
carry, and the actions cover only the single-table case that a real
application outgrows immediately.
The transaction is Orm.Transaction(), not the Begin/defer shape that
app/admin/service/sys_role.go and three other files use. That shape
commits a half-written transaction when the body panics, because the
deferred check reads err, which a panic leaves nil.
The payment transition guards concurrency through the update itself -
WHERE status = 'pending' plus RowsAffected - rather than a read followed
by a write.
The tests cover both rollback paths, because they fail differently: a
mid-transaction error returns, a panic unwinds - and the second is what
tells Orm.Transaction() apart from the shape it replaces. The concurrency
test pins the pool to one writer so sqlite's own single-writer semantics
cannot stand in for the guard being tested. The data-scope tests assert
the fail-closed direction too: an unrecognised scope must return no rows
rather than every row.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
A reference application for a third-party author: its own module, and a
require list that names go-admin-core and nothing else. The point of the
example is that constraint - an application that reaches for the host
cannot be installed through a module proxy at all, because `go-admin` has
no dot in its first path element and a replace directive is ignored
outside the main module.
Two tables rather than one, because a single-table example proves only
what the generic CRUD actions already proved. The interesting question is
whether the contract surface holds up for business that spans tables.
The table names carry an app_ prefix: "order" is a reserved word, and
Permission() interpolates the table name into raw SQL without quoting.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
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
The privilege-escalation tests skipped themselves when opening the in-memory
database or running AutoMigrate failed. Both depend on nothing outside the
process, so a failure there means the environment is genuinely broken - and a
security regression that quietly does not run is worse than one that is
missing, because CI stays green either way.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
`NULL NOT IN (...)` evaluates to NULL rather than true, so the previous
condition left a NULL data_scope exactly as it found it - and NULL is the one
value that most needs the repair: it scans into a Go string as "", which is
what the fail-closed default now refuses.
The column has no NOT NULL constraint, so the value is reachable from any
writer that is not the admin UI.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
app/admin/models/datascope.go carried a second copy of the scope logic with no
callers. Its department-tree pattern was written as "%" + id + "%" instead of
"%/" + id + "/%", so dept_id 1 also matched /11/, /21/ and /100/ - visibility
into unrelated subtrees.
It sat where someone looking for a data permission example would find it. The
copy that is actually wired up stays in common/actions.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The seed fix only reaches new installations. An install that imported the old
db.sql already has an administrator with an empty data_scope, and after the
fail-closed change that account sees nothing.
The migration rewrites any value outside "1".."5" to "1", which is the
behaviour those rows had before. Plain SQL rather than the frozen migration
models, per the rule that migrations after 1786700003000 must not use them.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The shipped seed data left data_scope empty for the built-in administrator.
That was harmless while an unrecognized scope meant "see everything"; with the
previous commits it means the opposite, so a fresh install with data permission
enabled would have blinded its own default account on every list endpoint.
The admin short-circuit does not help here: role_key == "admin" bypasses Casbin,
not the data permission scopes, which never look at role_key.
The value is "1" - all data - which is the behaviour the empty string used to
produce, so this restores the intent rather than tightening it. A test reads
both seed files back so the pair cannot drift apart again.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Nothing checked what went into sys_role.data_scope, so creating a role without
a dataScope stored an empty string - the value that used to be indistinguishable
from "see everything".
All three DTOs that write the column are validated, not just the insert path:
they target the same column, and guarding one entrance while leaving two open
would not be a guard.
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
Two directions, because the fix has to hold both: an attacker with no policy on
this route cannot raise another user's role, and a self-edit cannot raise its
own. The second one is what keeps the fix from being "just remove the route
from CasbinExclude", which would break the profile page.
The tests drive the handler directly rather than through the router, because
the middleware is exactly what does not run for this route - the defence lives
in the handler, so that is where it has to be proven.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The profile page posts the whole user object back, including roleId, deptId and
status, because it renders from a full SysUser it fetched earlier. A caller
editing their own record can therefore hand back a tampered roleId.
Self-edits now reload those three fields from the database and ignore whatever
the request carried. For an honest client this is a no-op - the values it sends
are already its own - so the profile page keeps working unchanged.
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 package-level AppRouters slice keeps working and keeps running first, so
a fork that only ever appended to it sees no change. What is new is that the
core registry runs too, and that before callbacks run at all - this server
never had a loop for them.
Both go through core RunAppRouters / RunBefore, which brings the panic guard
and the seal with them.
Six checks, five at ERROR and one - the cross-repository menu-name comparison -
at WARN, because it can only match by regular expression across two modules and
a false positive that fails CI teaches people to silence the tool.
The summary names which contract roots were actually scanned: core/ is a
separate module with no directory here, and a check that quietly covers less
than it claims is worse than no check.
--app rejects a code nothing was registered under, on all three paths. It used
to take a typo as "nothing matched" and report success: migrate said the app
was unknown and still exited 0, while --dry-run and status printed the same
words an up-to-date database produces.
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.
go.work points this module at a local checkout of go-admin-core while the
two are developed together. It is a local tool and must never be committed:
CI resolves core from go.mod.
checksilent is where `go build ./tools/checksilent` drops its binary - four
megabytes beside the server one, which was already ignored by name.
The previous commit added a data-permission scope to SysApi.Update and
returned early on db.Error, which left the RowsAffected check below it
unreachable: First reports a row the scope excluded as ErrRecordNotFound,
so the caller got "record not found" where the code meant to say
"无权更新该数据".
Map that one error to the permission message and drop the check it made
dead. The two cases - the row does not exist, and the row exists but is
not yours - have to look the same from outside, and now do.
Found by Copilot's review of #889.
SysApi.Update took a DataPermission and never used it, so with data
permission enabled the update reached rows the caller could not read
through GetPage, Get or Remove, which all scope the query. It also
reported "无权更新该数据" for a row that simply did not exist, a message
that only becomes true once the scope is applied.
Drops the Debug() left on the query, which logged the statement for
every call.
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.
BeforeCreate and BeforeUpdate run Encrypt on whatever is in the struct,
and a user read from the database carries the stored hash in Password.
Hashing it again produces a hash of a hash: the password that user knows
stops matching, they cannot log in, and nothing reports an error.
Only the Omit("password") on SysUser.Update stood between that and the
stored credential. Any other write to this model - a profile update
written the way every other model here is written - destroys the
password, permanently and silently.
Encrypt now returns early when Password already parses as a bcrypt hash.
That also removes the round SysUser.Update was paying and discarding:
306ns where it was 54.7ms, on a route reachable without the permission
check, since PUT /api/v1/sys-user is in CasbinExclude.
The cost of deciding from the value is that a password which is itself a
well-formed bcrypt hash would be stored unchanged. That is a
60-character string beginning "$2a$", and it grants whoever set it no
access they did not already have.
The repository has 19 test files and nothing was running any of them. Both
workflows build with go build, which does not compile _test.go, the Makefile's
test target was commented out, and there is no pre-commit hook. Every test in
the tree, including the schema guards that exist precisely to catch a silent
breakage, only ran when someone remembered to type go test.
Enables the commented-out target and calls it from go.yml, the one workflow
that fires on every push and pull request. build.yml is left alone: it skips
documentation-only changes and deploys on master, so it is the wrong place for
a gate that should never be skipped.
Runs with -race. common/actions reuses model instances across concurrent
requests, so a Generate() that returns in place rather than a copy leaks data
between them, which a single-threaded run cannot see.
Verified locally: the suite passes under CGO_ENABLED=0 and under -race, and
make test exits non-zero when a test fails, so the step actually gates.
Claude-Session: https://claude.ai/code/session_01DJhM6LvhkNPej35wy9F7Aq
The hazard had no signal at its point of contact. Someone adding a business
module is told to copy 1786700001000_demo_menu.go, which imports the frozen
seed models - correct for that file, wrong for anything ordered after the
soft-delete conversion. The frozen ModelTime carried no comment at all, so
opening it taught the reader nothing.
Documents the boundary in three places the author actually passes through:
the frozen type itself, the contributor guide, and the module-scaffolding
skill, which previously said to copy the reference file verbatim and now
says to copy its structure but not its imports.
Claude-Session: https://claude.ai/code/session_01DJhM6LvhkNPej35wy9F7Aq
zhangwenjian