refactor(auth): extract modules/auth submodule and deprecate pkg/auth#429
refactor(auth): extract modules/auth submodule and deprecate pkg/auth#429lml2468 wants to merge 2 commits into
Conversation
Establish modules/auth/ as the canonical home for octo-server's
Resource-Server-facing token contract (TokenInfo + Encode/Decode +
CacheTokenParser). pkg/auth/ becomes a Deprecated alias shim with a
guard test so the six existing callers (main.go,
modules/{group,message,user,qrcode}/api.go, modules/user/api_manager.go)
keep compiling unchanged during the six-month deprecation window.
The relocation is byte-identical: v2 JSON envelope (v2: prefix) and
legacy "uid@name[@ROLE]" decode-fallback, sentinel error identity for
errors.Is, panic-on-nil-cache contract, resolver fail-open semantics,
and the "empty resolver result drops snapshot" UserLanguageResolver
invariant are all preserved verbatim. modules/auth/doc.go states the
OAuth2 Authorization-Server / Resource-Server boundary and the
dependency-direction invariant (modules/{user,bot_api,usersecret} ->
modules/auth, never the reverse) that PR-A2 will enforce via depguard.
This is PR-A1 of the Stage A refactor (epic #428) that culminates in
the verify / verify-bot / verify-api-key handlers moving out of
modules/user/api.go into modules/auth.
Brief: .octospec/tasks/modules-auth-skeleton/brief.md
Journal: .octospec/journal/shared/modules-auth-skeleton.md
Refs #428
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
This PR is relevant to octo-server and is ready to merge; I found no blocking correctness, security, or behavior-regression issues.
💬 Non-blocking
🔵 Suggestion: pkg/auth/aliases.go:63, :68, :74, :80, :88 re-export functions as mutable package variables. That preserves call signatures, but it also makes formerly immutable functions assignable by importers. Wrapper functions would preserve the old API shape more exactly. Given this is a temporary compatibility shim and current in-repo callers do not mutate these, this is not blocking.
✅ Highlights
The relocation keeps the token codec and CacheTokenParser behavior equivalent to the prior pkg/auth implementation, including legacy decoding, sentinel error wrapping, resolver fallback semantics, and nil-cache panic behavior.
The shim preserves type identity and sentinel identity, and pkg/auth/aliases_test.go directly guards the errors.Is contract.
Focused verification passed locally: go test ./modules/auth/... ./pkg/auth/....
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
作者回应（我 @lml2468 是本 PR 作者，回避投票，仅认领 + 评估）：
@Jerry-Xin 的 🟡 我同意，是合理的 API 卫生改进（nit 级，非阻塞）：
事实：var Encode = modulesauth.Encode / var Decode = ... 是可变包级变量，同包代码或测试理论上可 auth.Encode = someFn 重新赋值；改成包装函数 func Encode(i TokenInfo) (string, error) { return modulesauth.Encode(i) } 则不可被重新赋值。对 auth codec 这种安全敏感符号，不可变 func 形态更稳健，且 godoc/静态工具识别为真函数更准确（而非"碰巧是函数的变量"）。
为何 nit 非阻塞：
- 调用形式
auth.Encode(x)对 var 和 func 完全一致 → 改成 func 包装对 6 个调用点（main.go / group / message / user×5 / api_manager）零影响，纯内部形态变化。 - "可被重新赋值"不构成真实攻击面——能执行
auth.Encode = evil的代码已在进程内、本就能为所欲为，不是额外提权。所以是 API 卫生问题，不是安全漏洞。
我作为作者把 Encode/Decode 两个别名从 var 改成 thin wrapper func（type/error 别名保持 = 不变——那些本就该是 alias）。
补充：本 PR 的安全命门我已自查——old pkg/auth/tokeninfo.go vs new modules/auth/tokeninfo.go 的 Encode/Decode 实现体逐字节等价（diff 仅 package 注释差异），token codec 行为零偏移；TestAliasRoundTrip 钉死 shim==canonical 等价性；go build ./... 全过。这是纯结构提取 + 别名，行为完全保持。
mochashanyao
left a comment
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
octo-server PR#429 Review Report
Reviewer: Octo-Q (automated review)
PR: #429
Head SHA: 91b595f135fd935e28d55f5daf17e9c92bdda5e5
Title: refactor(auth): extract modules/auth submodule and deprecate pkg/auth
Routing: security_sensitive (automated review)
1. Verification Summary
| Item | Status | Evidence |
|---|---|---|
| Code relocation fidelity | ✅ | Executable code byte-identical; only comments updated (pkg/auth → modules/auth) and one whitespace alignment in parser_test.go:131-132 |
| Alias shim completeness | ✅ | All 12 exported symbols aliased: 5 types (type =), 5 functions (var =), 2 sentinels (value re-export) — pkg/auth/aliases.go:1-88 |
| Sentinel error identity | ✅ | var ErrEmptyToken = modulesauth.ErrEmptyToken preserves pointer identity for errors.Is; pinned by aliases_test.go:52-56 |
| Dependency direction | ✅ | modules/auth imports only stdlib + octo-lib; zero imports of pkg/auth or any modules/{user,bot_api,usersecret} |
| Consumer compatibility | ✅ | All 6 consumers (main.go, modules/{group,message,user,qrcode}/api.go, modules/user/api_manager.go) use patterns safe with type aliases + function-variable aliases |
| Test migration | ✅ | tokeninfo_test.go byte-identical; parser_test.go comment-only changes; full coverage of round-trip, legacy decode, sentinel errors, resolver fail-open, empty-drops-snapshot, panic-on-nil-cache |
| Old files deleted | ✅ | pkg/auth/ contains only aliases.go + aliases_test.go; 4 original files removed |
2. Findings
No P0/P1 findings.
P2 — Minor observations (non-blocking)
P2-1: "byte-identical" claim slightly overstated
- Diff-scope: pre-existing claim in PR description, not a code defect.
- The PR description states "byte-identical relocation" but the actual diff includes: (a)
tokeninfo.gopackage comment shortened from 3 lines to 1 line, (b)parser.go3 comment blocks updatedpkg/auth→modules/auth, (c)parser_test.gostruct field whitespace re-alignment. All changes are comment-only / whitespace-only — zero executable code changed. The claim is functionally correct but technically imprecise. - Severity: nit. No runtime impact.
P2-2: Function-variable aliases are theoretically mutable
- Diff-scope: new (introduced by this PR's alias shim design).
var Encode = modulesauth.Encodeinpkg/auth/aliases.go:72creates a package-level variable that could be reassigned at runtime (unlike afuncdeclaration). If any code didauth.Encode = myFunc, only the shim's copy would change, not the canonical package's.- Mitigation: No consumer takes addresses of or reassigns these function variables (verified by grep across all 6 call sites). The 6-month deprecation window limits exposure. Go convention treats package-level
varfunction aliases as read-only. - Severity: P2 maintainability note. Not a defect.
P2-3: depguard enforcement deferred to PR-A2
- Diff-scope: new (documented design decision).
modules/auth/doc.go:38-39states the dependency-direction invariant but enforcement relies on a CI depguard rule "added alongside PR-A2". Until then, the invariant is documentation-only.- Mitigation: Explicitly documented in
doc.goand the PR description. PR-A2 is the next PR in the series. The window is narrow and the team is aware. - Severity: P2 process note. Acceptable for a staged refactor.
3. Suggestions
- Consider adding a
//nolint:reassignor a simpleinit()guard that panics ifEncode != modulesauth.Encodeat boot, to make function-variable tampering fail-loud. Low priority — the existingaliases_test.goalready catches drift at test time. - When PR-A2 lands the depguard rule, consider also adding a
go vetcheck orstaticcheckrule for package-level function-variable reassignment inpkg/auth/.
4. Extra Findings
None. The .octospec/ files (journal, brief, context.yaml, log) are process metadata documenting the task. They are internally consistent with the PR's actual changes and follow the repo's octospec 4-phase flow.
5. Data-Flow Tracing
| Consumed Data | Upstream Source | Flows Correctly? |
|---|---|---|
TokenInfo struct fields (UID, Name, Role, Language) |
Encode() → JSON marshal → cache → Decode() → struct |
✅ Identical codec path; type TokenInfo = modulesauth.TokenInfo makes types interchangeable at compile time |
ErrEmptyToken / ErrInvalidToken sentinels |
errors.New() in modules/auth/tokeninfo.go:35-39 → re-exported by var ErrEmptyToken = modulesauth.ErrEmptyToken in pkg/auth/aliases.go:53-57 |
✅ Same pointer value; errors.Is works across packages. Verified by aliases_test.go:52-63 |
CacheTokenParser.Parse() → wkhttp.UserInfo |
Cache.Get() → Decode() → resolver hooks → struct |
✅ Identical Parse() method; type alias CacheTokenParser = modulesauth.CacheTokenParser preserves method set |
WithLanguageResolver / WithRoleResolver options |
Function-variable alias → same func(*CacheTokenParser) value |
✅ Variadic signature preserved; main.go:128-129 call sites compile unchanged |
NewCacheTokenParser constructor |
Function-variable alias → same func(cache.Cache, string, ...ParserOption) *CacheTokenParser |
✅ Panic-on-nil-cache contract preserved; main.go:125 call site unchanged |
6. Blind-Spot Checklist (R5 — security_sensitive PR, all items mandatory)
- C1 — Dual-path parity: N/A. Pure package relocation with alias shim. No add/remove, subscribe/unsubscribe, or create/delete symmetric paths. No guard/lint tool is introduced or claimed.
- C2 — Control-flow ordering / nested reuse: N/A. No new control flow. Existing
Parse()logic is relocated verbatim. No safety controls (regex/escape/sanitize) are introduced or modified. - C3 — Authorization boundary ≠ capability boundary: N/A. No permissions, jails, tools, or endpoints are added/removed/modified. The token parser that hydrates
wkhttp.UserInfo(consumed by downstream Space isolation + per-route ACLs) is functionally identical. - C4 — Authorization lifecycle / container-member state cascade: N/A. No auth logic changes. Resolver fail-open semantics (Redis/DB outage → snapshot fallback) and empty-drops-snapshot invariants (both language and role) are preserved verbatim.
- C5 — Build/note ≠ runtime path: Clear.
go build ./...clean. All imports resolve correctly. No relative paths, browser extensions, CLI packaging, or environment-dependent resolution. Thepkg/auth→modules/authimport chain uses absolute Go module paths that resolve identically at build and runtime. - C6 — Governance/policy/security document self-consistency: Clear.
modules/auth/doc.godocuments the dependency-direction invariant consistently with the PR's actual behavior. No conflict with existing SECURITY.md or disclosure policies. The.octospec/brief and journal are internally consistent.
7. Cross-Round Blocker Recheck (R6)
N/A — first review of this PR.
[Octo-Q] verdict: APPROVE
This is a clean, well-tested package relocation. All executable code is preserved verbatim; the alias shim correctly maintains the full exported surface (types, functions, sentinel errors) for the six existing consumers. No P0/P1 findings. The three P2 observations are minor documentation/maintainability notes that do not block landing.
…shim Address review feedback from @Jerry-Xin on #429: `var Encode = modulesauth.Encode` and friends re-export functions as mutable package variables, which would let any importer reassign the symbol package-globally (`pkgauth.Encode = customFn`). Wrapper functions preserve the exported call signature — including the variadic `NewCacheTokenParser(c, prefix, opts...)` — while keeping the symbols immutable, which is the more defensive shape for a shim package consumed by other code during the six-month deprecation window. Type aliases (`type X = modulesauth.X`) and sentinel-error re-exports (`var Err... = modulesauth.Err...`) remain — types must be aliases for assignability and sentinels must preserve value identity for errors.Is. Refs #428
|
Thanks @Jerry-Xin for the careful review and the wrapper-vs-var call-out. I've pushed a second commit that converts the five function re-exports ( Diff is in Verification re-run locally: |
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #429 (octo-server)
Verdict: APPROVED — reviewed at head 91b595f135fd935e28d55f5daf17e9c92bdda5e5 (merge-base 34050f64, direct parent — clean diff).
This is a clean, well-scoped refactor: pkg/auth (token codec + CacheTokenParser) is relocated to modules/auth, and pkg/auth becomes a // Deprecated: alias shim re-exporting every symbol. No HTTP route, wire contract, or auth behavior changes in this PR. It is correctly scoped as step A1 of the staged migration.
1. Verification
- ✅ Logic byte-identical after move.
modules/auth/parser.goandmodules/auth/tokeninfo.godiffer from the originals only in doc comments (pathpkg/auth→modules/auth). Diffing the two with comments/blank lines stripped yields zero logic delta. - ✅ Build / vet / test pass.
go build ./...(exit 0),go vet ./modules/auth/... ./pkg/auth/...(exit 0),go test ./modules/auth/... ./pkg/auth/...(bothok). - ✅ Complete alias coverage. All 12 originally-exported symbols are re-exported (
pkg/auth/aliases.go): 5 types via immutabletype X =aliases, 2 sentinel errors and 5 functions viavarre-exports. Every symbol used by the 6 call sites (main.go,modules/{group,message,qrcode,user}/api.go,modules/user/api_manager.go) resolves and compiles. - ✅ Sentinel-error identity preserved.
errors.Is(err, auth.ErrEmptyToken/ErrInvalidToken)still matches errors produced by the canonical package — re-exported by value, and pinned byaliases_test.go::TestAliasSentinelIdentity. - ✅ Parser wiring intact.
main.go:125still constructsauth.NewCacheTokenParser(...)withWithLanguageResolver+WithRoleResolverthrough the shim. - ✅ Dependency-direction invariant holds.
go list -deps ./modules/authshows zeroocto-server/modules/*dependencies —modules/authdoes not importuser/bot_api/usersecret, matching the load-bearing invariant documented inmodules/auth/doc.go. - ✅ gofmt clean on all changed Go files.
2. Security review (security-sensitive PR)
No security findings. The security-critical properties of the token parser are preserved exactly by the move:
- Fail-open-to-snapshot on resolver error for both role and language (
modules/auth/parser.go) — a Redis/DB outage degrades to the token snapshot rather than 5xx-ing authentication. Unchanged. - Role revocation — a resolver returning empty (no error) drops the baked-in role, so a demotion is honored within the resolver's cache TTL. Behavior and its test (
TestCacheTokenParserRoleResolverEmptyDropsRole) carried over intact. - Error mapping — infra errors are wrapped with
%wand must NOT collapse toErrTokenNotFound/ErrTokenInvalid;TestCacheTokenParserPropagatesCacheErrorstill guards this. - The deprecation shim adds no new trust boundary; it is a pure re-export consumed only by in-tree callers.
3. Non-blocking suggestions (P2 / nit — do NOT block merge)
- nit — function aliases are
varre-exports.var Encode = modulesauth.Encode(and the other four funcs) are package-level vars and are technically reassignable. This is the canonical Go idiom for aliasing functions (there is nofuncalias syntax) and the stdlib uses the same pattern for deprecation shims; for an internal binary with 6 trusted call sites and zero reassignments this is purely theoretical. No action needed — flagged only for completeness. - P2 — architecture docs still point at
pkg/auth.CLAUDE.md(lines 36, 57) andAGENTS.md(lines 38, 62) describe the auth middleware / token parsing as living inpkg/auth/. The canonical home is nowmodules/auth. Since caller migration is intentionally deferred to later PRs, updating these docs now would create a doc-vs-code mismatch — best folded into PR-A2+ when call sites actually move. Calling it out so it isn't forgotten.
4. Additional notes
- The
.octospecbrief / context / journal formodules-auth-skeletonaccurately describe the diff (no route moved in A1; verify handlers land in A3+). The stated verification gates match the actual change set. - The legacy
uid@name[@role]decode path and the v2 JSON envelope are both retained, so cached tokens from older binaries continue to decode during rollout. Good backward-compat hygiene.
Nothing here is a correctness, security, or build blocker. Approving.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Re-affirming APPROVE on head 0c419fd.
This commit ("use wrapper funcs instead of var aliases in pkg/auth shim") implements exactly the 🟡 API-hygiene note from the prior round — and does it correctly with no behavior change.
Verified the shim delta (pkg/auth/aliases.go):
- Encode / Decode / WithLanguageResolver / WithRoleResolver / NewCacheTokenParser are now thin forwarding funcs (
return modulesauth.X(...)) instead of reassignablevar = modulesauth.X. Exported call signatures preserved verbatim — notablyNewCacheTokenParser(c cache.Cache, prefix string, opts ...ParserOption)keeps its variadic shape, so callers likeNewCacheTokenParser(c, prefix, WithLanguageResolver(r), WithRoleResolver(rr))compile unchanged. - Type aliases (TokenInfo / ParserOption / CacheTokenParser / resolver types) remain
type =aliases → type identity preserved across packages. - Sentinel errors ErrEmptyToken / ErrInvalidToken are still re-exported by value (
var = ...) →errors.Is(err, auth.ErrEmptyToken)keeps matching errors from the canonical package.
Net effect: the symbols callers can no longer reassign are exactly the function symbols (the API-hygiene goal), while value-identity contracts (sentinels, types) are untouched. The canonical token codec in modules/auth is unchanged from the previously-verified byte-equivalent state. No security regression, no caller breakage.
No blocking items.
Merge gate note: reviewDecision is REVIEW_REQUIRED (main needs 3 approvals; my prior approve + mochashanyao's were auto-dismissed by this new commit, yujiawei has re-approved on this head). Needs the other reviewers to re-approve the new head, and the check-sprint CI job (a process check) to go green.
|
@mochashanyao would you mind re-running the review on the new head ( |
The PR-A1 shim's Deprecated marker said "six months after this shim was introduced" without giving the actual date — making it ambiguous when reviewers grep for upcoming removals. This PR pins: - An explicit removal date (2026-12-22 = 6 months from the alias introduction in PR-A1 #429). - A pointer to the migration tracker (Stage A epic #428) so anyone touching pkg/auth in the interim can see who owns the migration. - A concrete one-line migration recipe in the package doc, so a contributor reading the deprecation warning can act immediately (no need to dig through the broader epic to understand what the fix is). Net effect on the wire: the per-symbol Deprecated lines now show the removal date alongside the redirect, so godoc and the golangci-lint deprecated check produce a more actionable warning at every call site (already visible in the diagnostics on the six pkg/auth importers). No code change — comments only. Tests + lint + build all green. Refs #428
|
@mochashanyao gentle ping — your earlier review on #429 was auto-dismissed by the wrapper-funcs fix push (commit 0c419fd). The current head is unchanged since; would you mind re-reviewing? The delta from your prior review's head (91b595f) is just the alias shape change (var → wrapper funcs in pkg/auth/aliases.go) for API-hygiene per @Jerry-Xin's earlier note. |
|
📌 Retargeted to Rationale: too many in-flight stacked PRs make cross-service integration testing on
The stacked PRs above keep their existing base chain — when this PR merges, the next PR auto-updates. |
The PR-A1 shim's Deprecated marker said "six months after this shim was introduced" without giving the actual date — making it ambiguous when reviewers grep for upcoming removals. This PR pins: - An explicit removal date (2026-12-22 = 6 months from the alias introduction in PR-A1 #429). - A pointer to the migration tracker (Stage A epic #428) so anyone touching pkg/auth in the interim can see who owns the migration. - A concrete one-line migration recipe in the package doc, so a contributor reading the deprecation warning can act immediately (no need to dig through the broader epic to understand what the fix is). Net effect on the wire: the per-symbol Deprecated lines now show the removal date alongside the redirect, so godoc and the golangci-lint deprecated check produce a more actionable warning at every call site (already visible in the diagnostics on the six pkg/auth importers). No code change — comments only. Tests + lint + build all green. Refs #428
lml2468
added
stage:review
QA Engineer Verdict: APPROVEReviewer: qa-engineer (review-lead loop, persona drain) Verification (local, this tick)
Test coverage assessmentThe migrated suite (
Edge cases / open items
VerdictAPPROVE — test coverage is at least as strong as the pre-relocation state, and the shim is guarded against the two highest-risk drift modes (sentinel-identity break and divergent |
lml2468
added
review:done:qa:approve
Security Engineer Verdict: APPROVEReviewer: security-engineer (review-lead loop, persona drain) Threat-model verdict per dimension
Auth-specific spot-checks
Cleared-with-risk notes (non-blocking)
VerdictAPPROVE / CLEARED — pure relocation with no auth-behavior delta. Sentinel-identity, fail-open, role-revocation, and codec-wire contracts all preserved with test pinning. No new attack surface; the wrapper-func change actually shrinks one. |
lml2468
added
review:done:security:approve
Code Reviewer Verdict: APPROVEReviewer: code-reviewer (review-lead loop, persona drain) Correctness
Readability
Maintainability
Design fit
Non-blocking nits
CI state
VerdictAPPROVE — clean, well-scoped, well-documented PR-A1 with proper deprecation discipline. Wrapper-func conversion at |
lml2468
added
review:done:code:approve
Aggregate Verdict: APPROVED — awaiting human mergeHead SHA:
Outcome3 × APPROVE → APPROVED. Per the loop charter, review-lead will NOT merge — handing off to a human maintainer. Merge-gate notes for the human merger
Applying |
4 participants
Summary
Establish
modules/auth/as the canonical home for octo-server'sResource-Server-facing token contract —
TokenInfo,Encode/Decode,CacheTokenParser— so future PRs in Stage A can move theverify/verify-bot/verify-api-keyHTTP handlers out ofmodules/user/api.gointo a package that owns the wire contract.
This is PR-A1 of the six-PR Stage A refactor; subsequent PRs (A2–A5) add the
Lookup interfaces in
bot_api/usersecret, port the verify handlers,introduce
verify-api-key, and migrate route registration — each independentlyrevertible.
This PR is a pure relocation: no HTTP behavior changes, no
errcodechanges,no caller migration. The six existing
pkg/authimporters (main.go,modules/{group,message,user,qrcode}/api.go,modules/user/api_manager.go)continue to compile unchanged through a Deprecated alias shim that will be
removed six months after this PR merges.
Related Issue
Refs #428
Linked Spec
.octospec/tasks/modules-auth-skeleton/brief.md(journal:
.octospec/journal/shared/modules-auth-skeleton.md,rule injection record:
.octospec/tasks/modules-auth-skeleton/context.yaml)Changes
modules/auth/doc.go— package documentation establishing the OAuth2Authorization-Server / Resource-Server boundary and the dependency-direction
invariant (
modules/{user,bot_api,usersecret}→modules/auth, never thereverse). PR-A2 will enforce via
depguard.modules/auth/{tokeninfo,parser}.go(+_test.go) — byte-identicalrelocation of
pkg/auth/{tokeninfo,parser}.go. v2 JSON envelope (v2:prefix) and legacy
uid@name[@role]decode-fallback preserved; sentinelerrors preserve identity for
errors.Is;LanguageResolver/RoleResolverinterfaces, option constructors, andpanic-on-nil-cachecontract unchanged. Tests cover: round trip, legacy decode, sentinel
errors, V2-prefix-required guard, cache-error propagation, resolver
upgrade / fail-keeps-snapshot / empty-drops-snapshot (language + role).
pkg/auth/aliases.go(new) — Deprecated alias shim re-exporting everysymbol via
type =(types), function variables (functions, preservingvariadic signatures), and value re-export (sentinels, preserving
errors.Is).pkg/auth/aliases_test.go(new) — guard test pinning round-trip viashim names + sentinel-value identity. Fails loud if any alias drifts.
pkg/auth/{tokeninfo,parser,_test}.go— deleted (canonical copieslive in
modules/auth/)..octospec/{tasks,journal,log}— task brief, context, journal entry,log entry per the repo's octospec 4-phase flow.
Testing
Local verification mirrored CI exactly (
go test -race -shuffle=on -count=1 -timeout 5mper package against a fresh docker stack on default ports;DROP DATABASE test; CREATE DATABASE test CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci;+redis-cli FLUSHALLbefore each package, per.github/workflows/ci.yml):go test ./modules/auth/... ./pkg/auth/...→ PASS (both packages)go build ./...→ cleango vet ./...→ cleangolangci-lint run ./...→ 0 issues across the whole repomake i18n-extract-check→ exit 0 (no marker diff)make i18n-lint→ OK on both subchecks(
lint-direct-error-response,lint-unregistered-code)Full
go test ./...per-package loop → 77 of 80 packages pass.The three failing packages (
modules/{botfather,channel,robot}) failidentically on
main(verified by branch-flip comparison) and arethe pre-existing migration issues alluded to in
.github/workflows/ci.yml(search
OCTO migration TODO; tracker: OCTO migration test debt: ~20+ broken tests masked by stale main CI #17).modules/oidcwasflaky once under shuffle but passed two subsequent runs (pre-existing,
not auth-related).
Importer surface invariant:
grep -rE '"github.com/Mininglamp-OSS/octo-server/pkg/auth"' --include='*.go' .still returns the same 6 caller files;
modules/authis imported onlyby
pkg/auth's shim.Unit tests added/updated (
pkg/auth/aliases_test.go; existingtests moved with implementation)
Manually verified (full local CI-equivalent run, command-by-command
above)
COMPREHENSION
What does this change actually do to the load-bearing path?
It relocates the package that owns the token cache codec (
TokenInfo,Encode/Decode,v2:JSON envelope, legacyuid@name[@role]fallback) and the
CacheTokenParserthat octo-lib'sAuthMiddlewareuses to hydrate
wkhttp.UserInfoon every authenticated request. Thefunctional behavior is byte-identical to
pkg/auth. What changes isthe package boundary:
modules/authis now the destinationpackage for the
verify/verify-bot/verify-api-keyhandlers inlater PRs, with a documented invariant that it must not import
modules/{user,bot_api,usersecret}implementation packages.What could break because of it (dependents + failure mode)?
The six existing
pkg/authconsumers (main.go,modules/group/ api.go,modules/message/api.go,modules/user/api.go,modules/user/api_manager.go,modules/qrcode/api.go) readTokenInfo, write tokens viaEncode, constructCacheTokenParserwith the variadic
WithLanguageResolver/WithRoleResolveroptions,and match
ErrEmptyToken/ErrInvalidTokenviaerrors.Is. Thealias shim must preserve every name, type, function signature,
and sentinel-error identity exactly.
Highest-risk failure mode: sentinel-error re-export by value-copy
(
var X = errors.New(...)) would silently breakerrors.Is. Avoidedby re-exporting the same sentinel value
(
var ErrEmptyToken = modulesauth.ErrEmptyToken);aliases_test.gopins this with a direct value-identity assertion plus an
errors.Isround-trip via
Decode. Second risk: variadic signature drift onNewCacheTokenParser— avoided by function-variable aliasing(
var NewCacheTokenParser = modulesauth.NewCacheTokenParser) whichpreserves the full signature including variadic options.
Downstream of the AuthMiddleware, Space isolation and per-route ACLs
read the
UserInfothis package produces. Because the resolverfail-open semantics (Redis/DB outage → snapshot fallback) and the
"empty resolver result drops snapshot" invariants (both language and
role) are preserved verbatim — including the unexported field shape
that the test suite pins — there is no path by which this PR can
weaken Space isolation.
How do you know it works (specific test/repro/trace)?
Three concentric proofs:
modules/auth/{tokeninfo,parser}_test.go(the migrated suite) covers v2 round-trip, legacy decode, sentinel
errors (
ErrEmptyToken,ErrInvalidToken,wkhttp.Err{TokenMissing, TokenNotFound,TokenInvalid}), V2-prefix-required, cache-error mustnot collapse to auth sentinel, resolver upgrade /
failure-keeps-snapshot / empty-drops-snapshot for both language and
role, and panic-on-nil-cache. All green.
pkg/auth/aliases_test.goround-trips
Encode→Decodevia the shim names, cross-checks theshim's
Encodeoutput is byte-identical to the canonical package's,and pins
ErrEmptyToken == modulesauth.ErrEmptyToken/ErrInvalidToken == modulesauth.ErrInvalidTokenvalue-identityplus an
errors.Isround-trip viaDecode.go build ./...clean; the per-package
go test ./...loop is green for 77 of 80packages, with the three remaining failures verified pre-existing
via a branch-flip against
main(same panics, same migration-statecause, tracked under OCTO migration test debt: ~20+ broken tests masked by stale main CI #17).
golangci-lint run ./...reports 0issues,
make i18n-extract-checkproduces no diff,make i18n-lintgreen.
Checklist
pkg/auth/aliases_test.go; existingtests moved with implementation)
modules/auth/doc.go;.octospec/brief + context + journal + log)