amadan
app: fix round 1(c) on Task 3 -- build guard against sessions.UserID
The ban on sessions.UserID existed only as prose in four doc comments (models.go, store.go, handlers.go, testing.go): under the magic-link plugin this app uses, the session Subject is the verified email address, so sessions.UserID can only ever return (0, false) here, on every request -- a call that never errors, never panics, just quietly scopes a query to person/user id 0 and looks like an empty result. Same failure class rastrillo's own scope.Owned has on a shared table, which is exactly why a blanket ban costs nothing and catches something real -- but a comment is a code review that has to succeed every time, not a guarantee. internal/lint/no_sessions_userid_test.go walks this repo's own Go AST (go/parser, no type-checking needed) and flags call expressions whose Fun resolves, through each file's own import spec, to sessions.UserID -- covering a plain import, an aliased one, and a dot import, all as call expressions only. Deliberately not a text/grep scan: all four existing comments already mention the symbol by name, so a grep-based ban needs four exemptions on day one and an exemption list that starts at four only grows; walking the AST for an actual call needs none; a comment or string mentioning the name matches nothing. Verified by hand, three ways: planted a real sessions.UserID(r) call under a plain import, watched the guard fail with file/line/reason, removed it; repeated under an aliased import (sess "...sessions"); repeated under a dot import (bare UserID(r)). All three were caught; the guard is clean again with nothing planted.
1 file changed,
+185
−0
internal/lint/no_sessions_userid_test.go+185 −0
diff --git a/internal/lint/no_sessions_userid_test.go b/internal/lint/no_sessions_userid_test.go| new file mode 100644 |
| index 0000000..09f8c3e |
| --- /dev/null |
| +++ b/internal/lint/no_sessions_userid_test.go |
| @@ -0,0 +1,185 @@ |
| +// Package lint holds build-time guards over this repo's own source — |
| +// properties that used to be enforced only by prose in a doc comment, |
| +// promoted to something `go test ./...` (and so scripts/wire-gate.sh's |
| +// "go: vet, test, gofmt" step) cannot pass without. |
| +package lint |
| + |
| +import ( |
| + "go/ast" |
| + "go/parser" |
| + "go/token" |
| + "os" |
| + "path/filepath" |
| + "strconv" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// bannedSessionsUserIDPackage and bannedSessionsUserIDFunc identify |
| +// the one call this repo's own code must never make. See |
| +// TestNoSessionsUserID's own doc comment for why. |
| +const ( |
| + bannedSessionsUserIDPackage = "github.com/carlosframework/rastrillo/sessions" |
| + bannedSessionsUserIDFunc = "UserID" |
| +) |
| + |
| +// repoRoot resolves the module root from this test's own directory — |
| +// internal/lint, two levels below it — the same "../.." relative-path |
| +// convention testing.go's ReadFixture already uses from internal/docs |
| +// (also two levels below the root), which works because `go test` |
| +// always runs with the package's own directory as its working |
| +// directory. |
| +func repoRoot(t *testing.T) string { |
| + t.Helper() |
| + root, err := filepath.Abs(filepath.Join("..", "..")) |
| + if err != nil { |
| + t.Fatalf("lint: resolve repo root: %v", err) |
| + } |
| + if _, err := os.Stat(filepath.Join(root, "go.mod")); err != nil { |
| + t.Fatalf("lint: %s does not look like the module root (no go.mod): %v", root, err) |
| + } |
| + return root |
| +} |
| + |
| +// skipDir reports whether WalkDir should not descend into a |
| +// directory: version control, and the two large generated/vendored |
| +// trees this repo's own build produces (fixtures/node_modules from |
| +// `npm ci`, core/target from `cargo build`) — neither holds Go source |
| +// this app wrote, and both are large enough that walking them would |
| +// only slow this test down for nothing. |
| +func skipDir(name string) bool { |
| + switch name { |
| + case ".git", "node_modules", "target": |
| + return true |
| + } |
| + return false |
| +} |
| + |
| +// TestNoSessionsUserID is a build-time guard, not a comment: this |
| +// repo's own code must never call sessions.UserID (rastrillo/auth's |
| +// underlying sessions package), because under the magic-link plugin |
| +// this app uses, the session Subject is the verified email address |
| +// itself — sessions.UserID's own contract can only ever return |
| +// (0, false) here, on every request, with no way for that to change |
| +// short of switching auth plugins. A version of this app's own code |
| +// that called it anyway would not error or panic; it would silently |
| +// scope a query to person/user id 0 and behave like an empty result — |
| +// the same failure class rastrillo's own scope.Owned has on a table |
| +// that grows sharing (store.go's own doc comment on `owned`): wrong |
| +// answers that look like "you just have nothing here," not a crash |
| +// that gets noticed. |
| +// |
| +// Four comments (models.go, store.go, handlers.go, testing.go) have |
| +// said "never sessions.UserID" since Task 1/2. A comment is a code |
| +// review that has to succeed every single time a future change is |
| +// made near this code; this test is what makes a violation impossible |
| +// to land instead of merely discouraged. It walks this repo's own Go |
| +// AST rather than grepping for the symbol, deliberately: a text scan |
| +// would have to carve out an exemption for every one of those four |
| +// comments (and grow that exemption list forever), and could never |
| +// tell a real call apart from a comment or a string that merely |
| +// mentions the name. Flagging only actual call expressions — Fun |
| +// resolving, through the file's own import spec, to |
| +// sessions.UserID — needs no exemption list at all and cannot be |
| +// fooled by a comment quoting the symbol, including this test's own. |
| +// |
| +// Credit: adapted from the Sheets session's TestNoSessionsUserID, |
| +// the same guard for the same reason under the same auth plugin. |
| +func TestNoSessionsUserID(t *testing.T) { |
| + root := repoRoot(t) |
| + fset := token.NewFileSet() |
| + |
| + err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { |
| + if err != nil { |
| + return err |
| + } |
| + if d.IsDir() { |
| + if path != root && skipDir(d.Name()) { |
| + return filepath.SkipDir |
| + } |
| + return nil |
| + } |
| + if !strings.HasSuffix(path, ".go") { |
| + return nil |
| + } |
| + |
| + f, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) |
| + if err != nil { |
| + return err |
| + } |
| + |
| + // aliases maps this file's own local identifier for the |
| + // banned package to true (an ordinary named or default |
| + // import) — resolved per file, because the same package can |
| + // legally be imported under different local names in |
| + // different files. dotImported is set instead when the file |
| + // dot-imports it, which puts UserID in scope unqualified |
| + // (a bare *ast.Ident call, no selector at all) rather than as |
| + // sessions.UserID — the AST shape a plain "sessions.UserID" |
| + // text search would never even think to look for, and |
| + // exactly the kind of gap a symbol-precise walk closes for |
| + // free. |
| + aliases := map[string]bool{} |
| + dotImported := false |
| + for _, imp := range f.Imports { |
| + importPath, err := strconv.Unquote(imp.Path.Value) |
| + if err != nil || importPath != bannedSessionsUserIDPackage { |
| + continue |
| + } |
| + switch { |
| + case imp.Name == nil: |
| + aliases["sessions"] = true // the package's own default name |
| + case imp.Name.Name == ".": |
| + dotImported = true |
| + case imp.Name.Name == "_": |
| + // blank-imported for side effects only; nothing in |
| + // this file can reference it by name at all. |
| + default: |
| + aliases[imp.Name.Name] = true |
| + } |
| + } |
| + if len(aliases) == 0 && !dotImported { |
| + // This file does not import the banned package under any |
| + // name — nothing it writes can possibly call it, however |
| + // often it mentions the name in a comment or a string. |
| + return nil |
| + } |
| + |
| + ast.Inspect(f, func(n ast.Node) bool { |
| + call, ok := n.(*ast.CallExpr) |
| + if !ok { |
| + return true |
| + } |
| + switch fn := call.Fun.(type) { |
| + case *ast.SelectorExpr: |
| + if fn.Sel.Name != bannedSessionsUserIDFunc { |
| + return true |
| + } |
| + id, ok := fn.X.(*ast.Ident) |
| + if !ok || !aliases[id.Name] { |
| + return true |
| + } |
| + case *ast.Ident: |
| + if !dotImported || fn.Name != bannedSessionsUserIDFunc { |
| + return true |
| + } |
| + default: |
| + return true |
| + } |
| + pos := fset.Position(call.Pos()) |
| + rel, relErr := filepath.Rel(root, pos.Filename) |
| + if relErr != nil { |
| + rel = pos.Filename |
| + } |
| + t.Errorf( |
| + "%s:%d: calls sessions.UserID — banned in this app: under the magic-link plugin the session Subject is the verified email address, so this can only ever return (0, false) here; use auth.From(r).Address and Store.EnsurePerson instead (see handlers.go's owner helper)", |
| + rel, pos.Line, |
| + ) |
| + return true |
| + }) |
| + return nil |
| + }) |
| + if err != nil { |
| + t.Fatalf("lint: walk %s: %v", root, err) |
| + } |
| +} |