long-agent/user-system
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: update admin flows and review report

Browse Source
This commit is contained in:
long-agent
2026-04-10 08:09:48 +08:00
parent f1bbba48c3
commit dbff591039
7 changed files with 610 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

310
docs/code-review/PROJECT_REAL_COMPLETION_REVIEW_2026-04-10.md Normal file
View File

@@ -0,0 +1,310 @@
# Project Real Completion Review 2026-04-10
## Scope
- Review date: 2026-04-10
- Workspace: `D:\usersystem`
- Branch context: `fix/status-review-sync-20260409`
- Review method: command execution plus targeted code inspection
- Review scope:
- the branch delta above `origin/main`
- current uncommitted workspace changes
- current status of previously identified project-level blockers
## Executive Summary
The project is materially healthier than the 2026-04-09 snapshot:
- `go vet ./...` is green
- `go build ./cmd/server` is green
- `go test ./... -short -count=1` is green
- frontend `lint`, `build`, `test:run`, and `test:coverage` are green
- `govulncheck` and production `npm audit` are green
However, this branch still cannot be honestly declared release-closed.
Current hard blockers or material risks:
- full `go test ./... -count=1` is still red because of the `LL_001` login-log pagination SLA gate
- the documented browser E2E entrypoint is still not green in this review environment
- the newly implemented role/admin-management path introduces real authorization and consistency risks
- avatar upload is still a visible stub
- frontend tests still emit jsdom native-dialog noise after a green run
## Commands Executed
### Backend
```powershell
$env:GOROOT='D:\Program Files\Go'
$env:GOCACHE='D:\usersystem\.gocache'
$env:GOMODCACHE='D:\usersystem\.gomodcache'
go test ./... -short -count=1
go vet ./...
go build ./cmd/server
go test ./... -count=1
go run golang.org/x/vuln/cmd/govulncheck@latest ./...
```
### Frontend
```powershell
cd frontend/admin
npm.cmd run lint
npm.cmd run build
npm.cmd run test:run
npm.cmd run test:coverage
npm.cmd run e2e:full:win
npm.cmd audit --omit=dev --json --registry=https://registry.npmjs.org/
```
## Verification Results
### Passed
- `go test ./... -short -count=1`
- `go vet ./...`
- `go build ./cmd/server`
- `npm.cmd run lint`
- `npm.cmd run build`
- `npm.cmd run test:run`
- `59` files
- `325` tests
- `npm.cmd run test:coverage`
- `59` files
- `325` tests
- overall coverage:
- statements `88.96%`
- branches `78.35%`
- functions `86.01%`
- lines `89.55%`
- `go run golang.org/x/vuln/cmd/govulncheck@latest ./...`
- output: `No vulnerabilities found.`
- `npm.cmd audit --omit=dev --json --registry=https://registry.npmjs.org/`
- production vulnerability counts: `0 / 0 / 0 / 0 / 0`
### Failed
- `go test ./... -count=1`
- failed in `internal/service.TestScale_LL_001_180DayLoginLogRetention`
- observed `P99=2.2259254s`
- threshold `2s`
- `npm.cmd run e2e:full:win`
- failed during the backend build/bootstrap step in `frontend/admin/scripts/run-playwright-auth-e2e.ps1`
- current observed build output still reports unresolved module packages from the wrapper's temp-cache build path
### Passed but still noisy
- `npm.cmd run test:run`
- green exit code
- still emits jsdom `Not implemented: window.alert` traces after the success summary
- `npm.cmd run test:coverage`
- green exit code
- still emits the same jsdom native-dialog traces after the coverage summary
## Findings
### High
#### 1. Any authenticated user can now enumerate arbitrary users' role assignments
Files:
- `internal/api/router/router.go:212`
- `internal/api/handler/user_handler.go:245`
Details:
- `GET /api/v1/users/:id/roles` is registered with no permission middleware.
- `GetUserRoles` now returns real role data for the requested `:id`.
- This route used to be inert because the handler always returned an empty array; after the current implementation, it becomes a real authorization gap.
Impact:
- any logged-in user can query the effective role set of any user ID
- this leaks privilege information and enables role reconnaissance against admin or privileged accounts
Required fix:
- restrict the route to self-access or explicit admin/`user:manage` permission
- add negative tests proving one user cannot read another user's roles
#### 2. `DeleteAdmin` can remove the caller's own admin role and can also remove the last remaining admin
Files:
- `internal/service/user_service.go:353`
- `internal/api/router/router.go:321`
Details:
- the implementation contains a comment noting that self-removal must be checked, but no such check exists.
- there is also no guard against removing the final admin role assignment from the system.
- the route is exposed on the admin-management API and returns success after deleting the role link.
Impact:
- an admin can accidentally or maliciously demote themselves mid-session
- the system can be left without any admin users, blocking governance and operational recovery paths
Required fix:
- pass current operator ID into the service and block self-demotion
- block deletion when the target is the last remaining enabled admin
- add regression tests for both cases
### Medium
#### 3. `AssignRoles` and `CreateAdmin` are not transactional and can leave RBAC state partially applied
Files:
- `internal/service/user_service.go:252`
- `internal/service/user_service.go:311`
- comparison baseline: `internal/service/auth_admin_bootstrap.go:92`
Details:
- `AssignRoles` deletes all existing role links before recreating them, but the operation is not wrapped in a transaction.
- `CreateAdmin` creates the user first and then creates the admin role link, also without transactional protection or rollback.
- the existing bootstrap flow already shows the correct failure-closed pattern by deleting the user if role assignment fails.
Impact:
- a failed role write can strip a user of all roles
- a failed admin-role write can leave an active non-admin account behind while the API reports failure
Required fix:
- execute both flows inside a single database transaction
- or at minimum add compensating rollback for every post-create failure path
#### 4. `CreateAdmin` regresses existing validation and role resolution patterns
Files:
- `internal/service/user_service.go:283`
- `internal/service/user_service.go:313`
- `internal/service/user_service.go:319`
- comparison baseline: `internal/service/auth_admin_bootstrap.go:42`
- comparison baseline: `internal/service/auth_admin_bootstrap.go:64`
Details:
- admin role resolution is hardcoded as `const AdminRoleID = 1` instead of loading the role by stable code.
- username existence is checked with `GetByUsername`, but any repository error is silently ignored unless a record is returned.
- password strength validation is skipped entirely; the code hashes whatever string is provided.
Impact:
- admin creation behavior can diverge from the rest of the authentication stack
- non-`record not found` repository errors can be masked
- password policy enforcement for administrator accounts becomes weaker than the bootstrap path
Required fix:
- use `ExistsByUsername` / `ExistsByEmail` and fail on repository errors
- reuse the same password validation path as admin bootstrap
- resolve the admin role by code (`admin`), not by assumed numeric ID
#### 5. Avatar upload is still a user-facing stub
Files:
- `internal/api/handler/avatar_handler.go:17`
- `internal/api/handler/user_handler.go:321`
- `frontend/admin/src/pages/admin/ProfileSecurityPage/ProfileSecurityPage.tsx:258`
- `frontend/admin/src/pages/admin/ProfileSecurityPage/ProfileSecurityPage.tsx:616`
Details:
- frontend profile UI still allows avatar upload.
- both backend avatar handlers still return `"avatar upload not implemented"`.
Impact:
- a visible user flow still cannot complete end-to-end
- status and completion narratives must continue to treat avatar upload as open
### Low
#### 6. `ui-consistency.test.tsx` still emits forbidden native-dialog noise even though the suite is green
File:
- `frontend/admin/src/components/common/ui-consistency.test.tsx:167`
- `frontend/admin/src/components/common/ui-consistency.test.tsx:199`
Details:
- the recent timeout/lint fix is real; `npm.cmd run lint` now passes.
- but the test file still calls `alert(...)` directly.
- jsdom therefore prints `Not implemented: window.alert` traces after both `test:run` and `test:coverage`.
Impact:
- test output remains noisy
- native-dialog usage is still present in a codebase that explicitly treats `window.alert` / `confirm` / `prompt` / `open` as defect signals
Required fix:
- replace direct native-dialog calls with spies, stubs, or project-native feedback primitives
## Historical Findings Rechecked
The following 2026-04-09 blockers are no longer current in this review:
- frontend `lint` is no longer red
- frontend `build` is no longer red
- frontend `test:coverage` no longer times out in this review window
- the `ui-consistency` timeout reassignment lint issue has been fixed
- `GetUserRoles` / `AssignRoles` are no longer backend stubs
- `CreateAdmin` / `DeleteAdmin` are no longer backend stubs
The following important blockers are still current:
- avatar upload remains stubbed
- full backend verification is still blocked by the `LL_001` SLA gate
- the documented browser-level E2E entrypoint is still not green in this review environment
## Open Questions / Notes
- The current `e2e:full:win` failure is still concentrated in the wrapper's backend build phase. The repo-level `go build ./cmd/server` command is green under the repo-local cache used for normal verification, but the wrapper's temp-cache build path is still not robust in this review run.
- The newly implemented admin-management code paths do not yet have the same depth of negative-path coverage as the rest of the auth/bootstrap flows. This is a testing gap in addition to the code risks above.
## Real Completion Assessment
### Can be honestly claimed
- backend short-path verification is green
- backend `go vet` and `go build` are green
- frontend `lint`, `build`, unit tests, and coverage are green
- current local `govulncheck` run is clean
- current production npm dependency audit is clean
### Cannot be honestly claimed
- the full verification matrix is green
- browser-level E2E closure is currently re-verified
- admin-management and role-management flows are fully hardened
- avatar upload is fully implemented
## Final Conclusion
This project is closer to release shape than the 2026-04-09 snapshot, but it is still not release-closed.
The largest changes since the previous review are positive on the surface:
- more of the matrix is green
- role/admin endpoints are no longer stubs
- frontend lint/build/tests are now passing
But the newly activated role/admin path now carries real authorization and consistency risks that are more serious than the old stub state, because they can now affect live permissions and admin governance.
The accurate 2026-04-10 position is:
- most routine verification gates are green
- one full backend SLA gate is still red
- browser E2E is still not re-verified closed
- the new RBAC/admin code needs hardening before this branch can be treated as production-ready