PR Review: feat: add authorization / RBAC system (Roadmap 2.3)

Overall this is a solid, well-structured addition. The pkg/authz package has clean interfaces, good test coverage, and the generated code pattern is clear. A few issues worth addressing before merge:

Bugs

1. Unused cookie import in generated handler — compile failure

In handler.go.tmpl (both multi and single), the cookie package is imported but never directly used in the generated handler code. cookie.Get is called internally by pkg/authz, not by the generated code itself. This will produce an imported and not used compile error on go build.

The golden file includes this import but the syntax-check test only looks for the string "syntax error" in the compiler output, so it slips through. Either remove the cookie import from the template, or use _ "..." if it's needed for a side effect (it isn't).

2. context.Background() in RequestDelete vs dbCtx in Edit

In the template, the Edit handler correctly uses dbCtx for the authz lookup:

if item, err := c.Queries.Get[[.ResourceNameSingular]]ByID(dbCtx, input.ID); err == nil {

But RequestDelete uses context.Background():

if item, err := c.Queries.Get[[.ResourceNameSingular]]ByID(context.Background(), input.ID); err == nil {

This should use dbCtx (or equivalent) for consistency and to respect request-scoped timeouts/cancellation.

3. DB error silently bypasses authz check

Both Edit and RequestDelete use if err == nil { ... } — meaning if the GetByID query fails (item not found, DB unavailable), the authorization check is skipped entirely and the operation proceeds. This is surprising: a missing item should return a clear error rather than silently proceeding to the update/delete path (which will also fail, but less clearly).

// Current: skip authz if item lookup fails
if item, err := c.Queries.GetPostByID(dbCtx, input.ID); err == nil {
    // authz check
}
// ... update proceeds regardless

// Better: return early on lookup failure
item, err := c.Queries.GetPostByID(dbCtx, input.ID)
if err != nil {
    return state, fmt.Errorf("item not found: %w", err)
}
// authz check on item

Security Concerns

4. Unauthenticated users can pass ownership check with empty created_by

When ctx.UserID() returns "", getUserRole short-circuits and returns "". UserFrom("", "") creates a non-nil user with an empty ID. In DefaultPolicy.Can, ActionUpdate/ActionDelete check:

return ownable.GetCreatedBy() == user.GetID()  // "" == "" → true

In practice the created_by TEXT NOT NULL REFERENCES users(id) constraint prevents empty created_by from being stored, so this isn't exploitable today. But it's a latent issue if the constraint is relaxed. Worth adding a guard in DefaultPolicy.Can:

case ActionUpdate, ActionDelete:
    if user.GetID() == "" {
        return false  // unauthenticated users can never own resources
    }
    ...

Down Migration Is Incomplete

5. authz/migration.sql.tmpl Down section doesn't remove the column

The Down migration drops the index but leaves the role column in place:

-- +goose Down
-- +goose StatementBegin
-- SQLite does not support DROP COLUMN directly.
-- To remove the role column, recreate the table without it.
DROP INDEX IF EXISTS idx_{{.TableName}}_role;
-- +goose StatementEnd

Since this is SQLite-targeted, consider either: (a) providing the full table-recreation script, or (b) documenting clearly that the Down migration is intentionally a no-op for column removal.

Design / Code Quality

6. Unused TokenLookup interface (pkg/authz/authenticator.go)

The TokenLookup interface (lines 1182–1184) defines a complex structural interface but is never used anywhere — CookieAuthenticator takes a plain func instead. This is dead code and should be removed.

7. Anonymous session ID is per-request, not per-browser

In GetSessionGroup, the fallback for users with no livetemplate-id cookie:

return fmt.Sprintf("anon-%d", time.Now().UnixNano()), nil

This generates a unique session ID for every request, meaning each WebSocket connection from the same anonymous browser gets a different session. LiveTemplate state won't be shared across tabs/reconnects for anonymous users. This should probably use a random UUID written into a new cookie, similar to how livetemplate-id itself is typically set.

8. Fragile patchSchemaWithRole string matching

The function tries two exact string patterns to find the insertion point in schema.sql. If the schema was generated with different whitespace or column ordering (e.g., updated_at before created_at), both patterns fail and the user gets a confusing error. Consider a more robust approach (e.g., regex or parsing the SQL structure, or at minimum, trying more pattern variants).

9. --with-authz not available in interactive UI (internal/ui/gen_resource.go)

The interactive lvt gen resource UI hardcodes withAuthz: false. The flag is only available via CLI. This is a minor gap worth noting — either add it to the interactive flow or document that --with-authz is CLI-only.

10. Duplicate template files between kits

single/templates/authz/ and multi/templates/authz/ are byte-for-byte identical. If these always stay in sync, they should either be shared from a common location or at minimum have a test asserting their equivalence.

Minor

11. Weak syntax check in integration test

output, _ := cmd.CombinedOutput()
if strings.Contains(string(output), "syntax error") {

This only catches the string "syntax error" and ignores the process exit code. Other compile errors (unused imports, type mismatches) are silently missed. The exit code from cmd.Run() or cmd.CombinedOutput() being non-nil is the right signal:

if output, err := cmd.CombinedOutput(); err != nil {
    t.Errorf("Handler has compile errors:\n%s", output)
}

Summary

The authz package itself (pkg/authz) is clean and well-tested. The main concerns are in the generated code template and the schema-patching logic. Happy to discuss any of these.

🤖 Generated with Claude Code

Code Review: feat: add authorization / RBAC system

Solid feature overall. The pkg/authz package is well-structured, tests are comprehensive, and the golden file approach for handler validation is great. Issues below are ordered by severity.

SECURITY - Fail-open authorization bypass on DB error: In both Edit and RequestDelete templates, the permission check uses the pattern 'if item, err := ...; err == nil'. If the DB is unavailable, auth is silently skipped. This is fail-open. Instead, fetch the item unconditionally, return an error on failure, then check permissions.

SECURITY - Unauthenticated writes with empty created_by: DefaultPolicy.Can returns true for ActionCreate for any non-nil User. UserFrom with empty strings is non-nil. If ctx.UserID() returns empty string due to invalid or expired cookie, CreatedBy gets an empty string, violating NOT NULL or corrupting ownership tracking. Guard against empty UserID in the generated Add handler, or have DefaultPolicy reject ActionCreate when the user ID is empty.

BUG - context.Background() in RequestDelete template: Edit uses dbCtx correctly but RequestDelete uses context.Background() for both the auth check and getUserRole. Request cancellation and deadlines will not propagate for deletes. Change both to dbCtx.

FRAGILE - patchSchemaWithRole hard-coded string patterns: Two hard-coded patterns find the role column insertion point. Different formatting in the auth schema will break this. Consider regex, or generate the role column as part of lvt gen auth from the start.

PERFORMANCE - getUserRole queries DB on every permission check: Every Edit and Delete fires two DB queries: one for the item, one for the user role. The role does not change within a request. Cache it in LiveTemplate session state or fetch both in one query.

API DESIGN - GenerateResource now has 13 positional parameters: 15+ test call sites all had to be updated for this one flag. An options struct (ResourceOptions) would make future additions non-breaking.

DEAD CODE - authz/schema.sql.tmpl contains only comments: The file has no SQL. It is loaded by kitLoader but never used. Remove it or document in GenerateAuthz why it exists.

IGNORED ERROR - qFile.WriteString in GenerateAuthz: The error from WriteString is silently discarded. It should be checked and propagated.

MIGRATION - Down does not roll back the column: After rollback the role column remains due to SQLite limitations. Document this in the generated migration comments, or include a table-rebuild in the Down section.

WHAT IS WORKING WELL: Clean pkg/authz design (Policy interface, DefaultPolicy, UserFrom/OwnedBy helpers are ergonomic and easy to extend). CookieAuthenticator is well-tested with a nice anonymous session group fallback. Golden test for --with-authz is a solid regression anchor. TestAuthzFullFlow doing a real go build + sqlc generate is exactly the right end-to-end validation. Good unit test coverage across authz.go, authenticator.go, and middleware.go.

Generated with Claude Code

Code Review: feat: add authorization / RBAC system (Roadmap 2.3)

Overall this is a solid, well-structured addition. The pkg/authz package is clean, the DefaultPolicy is sensible, and the test coverage is good. A few issues worth addressing before merge:

Bugs / Correctness

1. Fragile schema patching in patchSchemaWithRole (internal/generator/authz.go:120-156)

The function patches schema.sql using hardcoded string matching against specific whitespace and column ordering. If the auth generator ever changes its output format even slightly, this silently fails with the unhelpful message "could not find insertion point for role column". Consider parsing the SQL structurally or anchoring the pattern to the table name so it only patches the right CREATE TABLE block. Right now strings.Contains(content, target) could also match the wrong table if schemas follow the same pattern.

2. Prerequisite check is incomplete (commands/gen.go:151-154)

if _, err := os.Stat(filepath.Join(basePath, "app", "auth")); os.IsNotExist(err) {
    return fmt.Errorf("--with-authz requires authentication. Run 'lvt gen auth' and 'lvt gen authz' first")
}

This only checks that lvt gen auth was run. A user can run lvt gen auth → lvt gen resource posts --with-authz (skipping lvt gen authz) and the generated code will reference user.Role on the sqlc model, which won't exist because the role column was never added. Should also check that the role column is present in schema.sql, or that the authz migration directory has been generated.

3. Ignored write error (internal/generator/authz.go:119)

qFile.WriteString("\n\n")

Error return is discarded. Should be:

if _, err := qFile.WriteString("\n\n"); err != nil {
    qFile.Close()
    return fmt.Errorf("failed to write separator to queries.sql: %w", err)
}

4. Incomplete down migration (internal/kits/system/multi/templates/authz/migration.sql.tmpl)

The -- +goose Down section only drops the index, not the column. The comment says SQLite doesn't support DROP COLUMN, but the migration is still advertised as reversible. Either mark the migration as irreversible or document clearly that the down migration is partial. Users running lvt migration down expecting a full rollback will be surprised.

Performance

5. Double DB fetch for authz in Edit handler (template handler.go.tmpl)

In Edit, the item is fetched for the authz check, then GetAll[[.ResourceNamePlural]] is called separately. In ConfirmDelete, the item is fetched in both RequestDelete and ConfirmDelete for the re-check. This is intentional TOCTOU defense for the delete case and is fine, but the Edit / Update pair fetches the item twice (once for the check, once to find it in the list). Consider reusing the fetched item.

6. getUserRole hits the DB on every action

Since LiveTemplate uses long-lived WebSocket connections, the user's role is re-fetched from the database on every Edit, Update, RequestDelete, and ConfirmDelete. Consider caching the role in the controller state or session for the connection's lifetime, since roles change infrequently.

Design / API

7. GenerateResource positional bool parameter

Adding withAuthz bool as the 13th positional parameter makes an already long signature harder to maintain. The ~10 call-site updates to pass false show the fragility. Consider introducing a ResourceOptions struct for optional flags (WithAuthz, PaginationMode, PageSize, EditMode, ParentResource) that groups these knobs together.

8. Dead template file (internal/kits/system/multi/templates/authz/schema.sql.tmpl)

This file is 12 lines of comments explaining that it doesn't do anything. The actual patching is done by patchSchemaWithRole. The file is never loaded by the generator (the LoadKitTemplate call in authz.go only loads authz/migration.sql.tmpl and authz/queries.sql.tmpl). Remove it to avoid confusion.

Minor

9. authz.Register uses table name as the key (handler.go.tmpl:init())

authz.Register("[[.TableName]]", &authz.DefaultPolicy{})

The registration key is the table name (e.g., "posts"), but the Can() call also uses "[[.TableName]]". This is internally consistent but the public API docs for Register say "resource type name", which could confuse users trying to register custom policies — they'd need to know to use the table name, not the Go type name.

10. time.Sleep in TestAuthzFullFlow (integration_test.go)

The 3× time.Sleep(2 * time.Second) between generation steps should have a comment explaining why (timestamp collision prevention for migration filenames). Without context, this looks like a flaky-test workaround.

What's working well

• pkg/authz package is clean with a well-defined Policy interface and sensible DefaultPolicy defaults
• OwnedBy() and UserFrom() adapters are a nice touch — avoids forcing generated models to implement interfaces
• Re-checking auth in ConfirmDelete (TOCTOU defense) is exactly right for destructive actions
• Test coverage is comprehensive: unit tests, golden file tests, content validation, full flow test with real sqlc + go build
• The CookieAuthenticator.Identify returning "", nil (not an error) for invalid/expired tokens is the correct behavior for optional auth
• Template delimiter consistency within each template system is maintained

🤖 Generated with Claude Code