Skip to content

feat(server): plan for auth and read-only mode (#312)#334

Open
le-yams wants to merge 3 commits into
cooklang:mainfrom
le-yams:feat/server_auth_and_read_only_mode
Open

feat(server): plan for auth and read-only mode (#312)#334
le-yams wants to merge 3 commits into
cooklang:mainfrom
le-yams:feat/server_auth_and_read_only_mode

Conversation

@le-yams
Copy link
Copy Markdown

@le-yams le-yams commented Apr 29, 2026

Hello there,

This PR addresses cooklang/cookcli#312 — making the cook server write operations require authentication, while keeping anonymous read access available by default.

⚠️ This PR currently contains only the design/plan document — no implementation yet. I'm opening it as a draft to gather feedback on the approach before I start coding. Once the plan is validated I'll push the implementation in follow-up commits on this same branch.

What's in here

A single new file: docs/plans/2026-04-29-server-auth-plan.md, structured as:

  • Threat model & non-goals
  • Configuration format (server.toml with mandatory plain: / bcrypt: prefixed password) and an extensible PasswordVerifier trait with a plain Password enum (extended later by adding a variant)
  • CLI surface (--auth, --no-auth, --auth-config --enable-auth, --server-config, plus cook server hash-password subcommand)
  • Three AuthMode states (Authenticated / ReadOnly / Disabled) and a resolution table Two AuthMode states (Authenticated / Disabled) and a resolution table; --enable-auth without credentials is a startup error rather than a third ReadOnly variant
  • Server-side persistent sessions (HashMap + JSON file, 7-day TTL)
  • Full read/write classification of every API and UI route
  • Frontend changes (template AuthContext, hidden write actions, login page)
  • A 7-phase implementation breakdown A 6-phase implementation breakdown where each phase pairs code with the tests that lock its behaviour
  • Definition of done

Notable design decisions (vs. the issue)

  • Default mode is Disabled (legacy behavior preserved with a console warning) instead of ReadOnly, to avoid a breaking change for existing users. Switching to read-only requires explicit --auth Enabling protection requires explicit --enable-auth together with credentials in server.toml; passing the flag alone aborts at startup.
  • Mandatory algorithm prefix on the password field (no auto-detection) to remove ambiguity.
  • Local auth and CookCloud sync stay separate: the existing /api/sync/* flow is unchanged, just protected by the new local auth.

Looking for feedback on

  • Overall approach and architecture
  • The default-mode decision (Disabled vs strictly following the issue's ReadOnly)
  • Route classification — anything I'm miscategorizing?
  • Whether the PasswordVerifier trait abstraction is overkill for shipping just plain + bcrypt dropped the trait in favor of a plain Password enum
  • Any prior art / preferences in the codebase I might have missed

Disclosure

I'm new to Rust, so I'm using Claude Code to help me explore the codebase, design the plan, and (later) write idiomatic Rust. All design decisions and the final wording of this plan are my own — I've reviewed everything carefully — but it's only fair to flag that a coding assistant is in the loop.

le-yams and others added 3 commits April 30, 2026 17:18
Replace the prior design doc (server-auth-readonly-design.md) with a
single forward-looking plan where each phase pairs code with the tests
that lock its behavior. Reflects the final CLI (--enable-auth as the
source of truth, --server-config, mandatory plain:/bcrypt: password
prefix) and a deliberately small architecture — two modes
(Authenticated / Disabled), --enable-auth without creds errors out at
startup, password verifier as an enum, require_auth mounted before
the /api nest to sidestep OriginalUri pitfalls.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@le-yams le-yams force-pushed the feat/server_auth_and_read_only_mode branch from 181bc41 to a38c29e Compare April 30, 2026 16:10
@le-yams
Copy link
Copy Markdown
Author

le-yams commented Apr 30, 2026

Hello again,

I've reworked the plan to fold the test strategy in and simplify the CLI.

CLI simplification: Single --enable-auth flag as the source of truth (drops --auth / --no-auth); --server-config (was --auth-config); presence of server.toml alone never activates auth.

Architecture simplification: Two modes only — Authenticated / Disabled; --enable-auth without creds is a startup error rather than a third ReadOnly variant. Password is a plain enum (Plain / Bcrypt) instead of a trait + factory. require_auth is mounted before the /api nest so req.uri() works directly, removing the OriginalUri regression test. Session TTL hardcoded to 7 days; hash-password is bcrypt-only; auth strings stay inline (no .ftl churn until a translation lands).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant