amadan
idear: TokenFrom, and stop claiming the compiler catches a Models mistake (Task 6, fix round 1)
SKILL.md and the example both said idear's models cannot go in an app's Models list "because idear does not export them". Member and Invitation ARE exported — the example's own templates render idear.Member — and what is unexported is the LIST. The sentence did not merely overstate: it told a reader the compiler would catch a mistake that only a test catches, and therefore told them not to write the test. Both now say what schema.go already said correctly, name the guard, and §7 spells out the check. Export TokenFrom(r) — the reader CarryToken's stash already had. An app's RenderSignup needed two pieces of folklore to re-seed its hidden invite field after a failed signup: the field name, and the fact that the body was already parsed. Now it needs neither, and the field name stays inside the package that chose it. It deliberately does NOT fall back to re-reading the body: that fallback would work when CarryToken is unmounted, which is exactly the misconfiguration idear keeps loud. And test the workaround, which was the one piece of copy-this-code with no coverage at all. A signup posted with a valid token and a five-character password must come back 422 with the token still in the form, and the second attempt — the one that actually fails in the wild — must succeed. Deleting TokenFrom from renderSignup previously left the whole suite green; it now turns that test, and only that test, red. Six smaller SKILL.md corrections: both public routes are rate-limited, not one; the rastrillo import paths are stated; BootSchema is shown being applied and not just merged; the checklist covers the signup token and app-wide CSRF; From/Grantable/WithMember/TokenFrom are documented; and the no-flash.Take rule keeps its two real reasons instead of one wrong one — a second Take re-reads the same unmutated cookie, so it duplicates a notice only where the page also renders MembersPage.Notice, and on the invitation page it eats an unrelated pending flash instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7 files changed,
+264
−38
SKILL.md+87 −13admit.go+28 −0admit_test.go+66 −0example/README.md+7 −2example/app_test.go+49 −1example/models.go+16 −13example/render.go+11 −9
diff --git a/SKILL.md b/SKILL.md| index 5e0697c..fab2fea 100644 |
| --- a/SKILL.md |
| +++ b/SKILL.md |
| @@ -32,18 +32,42 @@ checkout and is never right. |
| ## 2. Wire it |
| +Everything below comes from Rastrillo's own packages plus this one: |
| + |
| +```go |
| +import ( |
| + "amadan.net/rastrillo/idear" |
| + |
| + "github.com/carlosframework/rastrillo/csrf" |
| + "github.com/carlosframework/rastrillo/db" |
| + "github.com/carlosframework/rastrillo/flash" |
| + "github.com/carlosframework/rastrillo/migrate" |
| + "github.com/carlosframework/rastrillo/password" // or .../auth for keymail |
| + "github.com/carlosframework/rastrillo/sessions" |
| + "github.com/go-chi/chi/v5" |
| +) |
| +``` |
| + |
| Five things, in this order. Every step is load-bearing; `example/app.go` is |
| this same list with the reasons attached. |
| **1. Schema.** `idear.Schema` merges into **`BootSchema`**, never into the |
| -app's own `Schema`: |
| +app's own `Schema` — and `BootSchema` is what gets applied at boot: |
| ```go |
| var BootSchema = migrate.Merge(sessions.Schema, idear.Schema, Schema) |
| + |
| +// in App(), before anything else runs: |
| +if _, err := migrate.Apply(context.Background(), d, BootSchema); err != nil { |
| + return nil, err |
| +} |
| ``` |
| -and idear's models **never** go in the app's `Models` list — there is no way |
| -to put them there, because idear does not export them. See §7. |
| +idear's models **never** go in the app's `Models` list, and **nothing stops |
| +you putting them there**: idear exports the *types* (`idear.Member`, |
| +`idear.Invitation` — your templates will use them) but not the *list*. So |
| +`[]any{&Note{}, &idear.Member{}}` compiles, and only a test catches it. See |
| +§7 for what it costs and §9 for the test. |
| **2. The roster**, once per process: |
| @@ -142,9 +166,13 @@ POST /members/{id}/remove Admin (deactivate, never delete) |
| POST /members/{id}/restore Admin |
| POST /members/transfer Owner (the only path to Owner) |
| GET /invitations/{token} public (rate-limited) |
| -POST /invitations/{token} public (signed-in reconciliation) |
| +POST /invitations/{token} public (rate-limited; reconciliation) |
| ``` |
| +**Both** public routes are rate-limited, not just the lookup: one reads a |
| +secret and the other spends it, and the limiter is not optional — `RateLimit` |
| +can only be widened, never switched off. |
| + |
| `POST /invitations/{token}` is the **reconciliation** route and it is not |
| decoration. Admission cannot be one transaction — `Admitting` calls the |
| app's opaque `Create` and then writes the Member — so a failure between them |
| @@ -155,9 +183,19 @@ every route until they open their invitation link while signed in. |
| ## 4. Rendering |
| Two callbacks, following `password.Config.RenderSignin`. **Neither may write |
| -a status, and neither may call `flash.Take`** — idear writes 400/403/404/500 |
| -itself before calling out, and it has already taken the flash and handed it |
| -over as `Notice`/`Error`. |
| +a status, and neither may call `flash.Take`.** |
| + |
| +No status, because idear writes 400/403/404/500 itself before calling out; |
| +the first `WriteHeader` wins and a second is a logged no-op. |
| + |
| +No `flash.Take`, for two different reasons. On the members page idear has |
| +*already* taken the flash and handed it back as `Notice`/`Error`, so a |
| +second `Take` reads the same unmutated request cookie again — harmless on |
| +its own, but it shows the notice **twice** on any page that also renders |
| +`MembersPage.Notice`, which is every page worth writing. On the invitation |
| +page there is no idear flash at all, and a `Take` there simply **eats an |
| +unrelated pending notice** the visitor was owed on their next page. Render |
| +what idear hands you. |
| - `MembersPage{Viewer, Members, Invitations, Grantable, Error, Notice}`. |
| Build the role selector from **`Grantable`**, never from the three |
| @@ -169,10 +207,22 @@ over as `Notice`/`Error`. |
| form with `<input type="hidden" name="invite" value="{{.Token}}">`. |
| `password.PageData` has nowhere to carry a token, so a signup that fails |
| -validation re-renders a form whose hidden field would come back empty. Read |
| -it back off the posted form in `RenderSignup` (`r.PostFormValue("invite")` — |
| -`CarryToken` has already parsed it), or the invitee's second attempt is |
| -refused. |
| +validation — a password under eight characters, say — re-renders a form |
| +whose hidden field comes back **empty**, and the invitee's *second* attempt |
| +is refused for holding no token. The symptom is "invited people can never |
| +join", one step later than the mistake. `idear.TokenFrom(r)` hands back what |
| +`CarryToken` lifted off that very POST: |
| + |
| +```go |
| +func renderSignup(w http.ResponseWriter, r *http.Request, d password.PageData) { |
| + render(w, r, "signup", signupView{PageData: d, Invite: idear.TokenFrom(r)}) |
| +} |
| +``` |
| + |
| +It reads only what `CarryToken` stashed — never the query string, where a |
| +live credential must not be, and never the body directly, so a missing |
| +`CarryToken` stays loud instead of being papered over. Write the test for |
| +this one; see §9. |
| ## 5. Roles, and the store |
| @@ -201,6 +251,18 @@ Refusals are classes, matched with `errors.Is`: `ErrInvalid` (400), |
| `ErrOwnerExists` for the flows that produce them. `ErrLastOwner` unwraps to |
| `ErrForbidden`. |
| +Four more exported helpers, none of them optional reading: |
| + |
| +```go |
| +idear.From(r) *Member // the viewer Require resolved; nil outside it |
| +idear.Grantable(actor *Member) []Role // what actor may hand out — the function |
| + // behind MembersPage.Grantable, for an app |
| + // building its own role selector elsewhere |
| +idear.WithMember(r, m) *http.Request // put a viewer on a request: for a test, or |
| + // an app that resolves membership its own way |
| +idear.TokenFrom(r) string // §4 |
| +``` |
| + |
| `Subject` is the join to the app's identity and is a **`string` on every |
| path** — never an `int64`, never `sessions.UserID`. Under password it is the |
| decimal user id (`strconv.FormatInt(id, 10)`); under keymail it is the |
| @@ -270,7 +332,10 @@ Each of these cost a review round. |
| generate`/`check` diff one app's Schema against that same app's Models as |
| a matched pair. Mixed in, `check` is permanently red and `generate` emits |
| a second, GORM-flavoured `CREATE TABLE idear_members` that collides at |
| - boot with idear's own. |
| + boot with idear's own. The compiler will not stop you — `idear.Member` and |
| + `idear.Invitation` are exported types, and only the *list* is not — so a |
| + test is the guard: `migrate.Generate(ctx, Schema.All(), Models)` must |
| + return zero changes. |
| - **An Admin may grant only Member.** `checkInviteRole` is strictly-below, |
| so a selector offering Admin to an Admin 403s on every submit and one |
| offering Owner 403s for everybody. Build it from `MembersPage.Grantable`, |
| @@ -332,8 +397,12 @@ Cover at least: |
| - a posted `role=owner` never lands, on any path, for any actor; |
| - an invited address cannot be claimed **without the token**; |
| - a removed member still signs in and still gets 404 on `/`; |
| +- an invited signup that fails validation re-renders a form still carrying |
| + the token, and the **second** attempt succeeds; |
| +- a cross-origin POST to an idear route is refused 403; |
| - the app's own Schema and Models agree (`migrate.Generate` returning zero |
| - changes is `rastrillo migration check` in test form). |
| + changes is `rastrillo migration check` in test form) — which is also what |
| + catches an idear model added to `Models`. |
| ## Checklist before you call a mount done |
| @@ -345,3 +414,8 @@ Cover at least: |
| 6. `Site` is set; `ClientKey` is set if there is a proxy. |
| 7. The role selector is built from `Grantable`. |
| 8. One identity plugin, not two. |
| +9. `RenderSignup` seeds its hidden `invite` field from `idear.TokenFrom(r)`, |
| + **and a test posts a failing signup to prove it** — this is the piece a |
| + rewritten signup page loses silently. |
| +10. `csrf.Protect(origin)` is mounted app-wide, above every group, so it |
| + covers idear's routes as well as yours. |
diff --git a/admit.go b/admit.go| index 0aefb1b..6dc1fc4 100644 |
| --- a/admit.go |
| +++ b/admit.go |
| @@ -91,6 +91,34 @@ func inviteToken(ctx context.Context) string { |
| return token |
| } |
| +// TokenFrom returns the invitation token CarryToken lifted off this |
| +// request's form, or "". |
| +// |
| +// It exists for one caller — the app's password.Config.RenderSignup — |
| +// and it closes a gap that is otherwise silent. password re-renders |
| +// the signup page on a validation failure (a password under eight |
| +// characters, say) with a PageData carrying Error, Email and ReturnTo |
| +// and NOWHERE to put a token. A hidden "invite" field seeded from that |
| +// page data comes back empty, so the invitee's SECOND attempt is |
| +// refused for holding no token — and the symptom, "invited people can |
| +// never join", shows up only on the second try. Seed it from here |
| +// instead: |
| +// |
| +// func renderSignup(w http.ResponseWriter, r *http.Request, d password.PageData) { |
| +// render(w, r, "signup", signupView{PageData: d, Invite: idear.TokenFrom(r)}) |
| +// } |
| +// |
| +// It reads what CarryToken stashed and nothing else. That keeps the |
| +// field name inside the package that chose it, and it means the app |
| +// never has to reason about whether the body has already been parsed. |
| +// |
| +// On a request that did not pass through CarryToken — the GET signup |
| +// page, or a POST where the middleware is not mounted — it is "". The |
| +// second case is deliberate: a fallback that re-read the body here |
| +// would paper over a missing CarryToken, which is the one |
| +// misconfiguration that closes the instance to every invitee. |
| +func TokenFrom(r *http.Request) string { return inviteToken(r.Context()) } |
| + |
| // subjectForID is the Subject a Member row must carry for an app whose |
| // identity plugin is password. |
| // |
diff --git a/admit_test.go b/admit_test.go| index 4e0d6fc..98d769d 100644 |
| --- a/admit_test.go |
| +++ b/admit_test.go |
| @@ -1066,3 +1066,69 @@ func TestSubjectIsCanonicalWhicheverSpellingWritesIt(t *testing.T) { |
| } |
| }) |
| } |
| + |
| +// TestTokenFromReadsWhatCarryTokenStashed pins the reader every app's |
| +// RenderSignup uses to re-seed the hidden invite field after a failed |
| +// signup — the workaround for password.PageData having nowhere to |
| +// carry a token. |
| +// |
| +// The three cases are the three states an app can be in: mounted |
| +// (the token comes back), NOT mounted (empty, and deliberately so — |
| +// a body re-read here would hide the one misconfiguration that closes |
| +// the instance), and mounted with nothing posted (empty). |
| +func TestTokenFromReadsWhatCarryTokenStashed(t *testing.T) { |
| + h := ideartest.New(t) |
| + rs := h.Roster |
| + |
| + post := func(form url.Values) *http.Request { |
| + req := httptest.NewRequest(http.MethodPost, "/signup", strings.NewReader(form.Encode())) |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + return req |
| + } |
| + |
| + for _, tc := range []struct { |
| + name string |
| + form url.Values |
| + carry bool |
| + want string |
| + }{ |
| + {"carried", url.Values{"invite": {"a-token"}}, true, "a-token"}, |
| + {"not mounted", url.Values{"invite": {"a-token"}}, false, ""}, |
| + {"nothing posted", url.Values{"email": {"who@corp.test"}}, true, ""}, |
| + } { |
| + t.Run(tc.name, func(t *testing.T) { |
| + var got string |
| + inner := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { |
| + got = idear.TokenFrom(r) |
| + }) |
| + var handler http.Handler = inner |
| + if tc.carry { |
| + handler = rs.CarryToken(inner) |
| + } |
| + handler.ServeHTTP(httptest.NewRecorder(), post(tc.form)) |
| + if got != tc.want { |
| + t.Errorf("TokenFrom = %q, want %q", got, tc.want) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// TestTokenFromIgnoresTheQueryString is TestCarryTokenIgnoresTheQuery |
| +// String's assertion at the reader: a token in the URL must not reach |
| +// a re-rendered signup form either, or the page would hand back a |
| +// credential that leaked through access logs and Referer headers as a |
| +// value the next POST treats as carried. |
| +func TestTokenFromIgnoresTheQueryString(t *testing.T) { |
| + h := ideartest.New(t) |
| + var got string |
| + inner := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { |
| + got = idear.TokenFrom(r) |
| + }) |
| + req := httptest.NewRequest(http.MethodPost, "/signup?invite=from-the-url", |
| + strings.NewReader(url.Values{"email": {"who@corp.test"}}.Encode())) |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + h.Roster.CarryToken(inner).ServeHTTP(httptest.NewRecorder(), req) |
| + if got != "" { |
| + t.Errorf("TokenFrom = %q from the query string, want %q", got, "") |
| + } |
| +} |
diff --git a/example/README.md b/example/README.md| index ea49ae3..1bc7833 100644 |
| --- a/example/README.md |
| +++ b/example/README.md |
| @@ -41,11 +41,11 @@ keeps the token out of the browser entirely. |
| | -------------- | ------------------------------------------------------------------- | |
| | `app.go` | the whole wiring, in the order it has to happen, with the reasons | |
| | `models.go` | the app's own `User`, `BootSchema`, and a seed through the real flows | |
| -| `render.go` | the two idear render callbacks, and why neither writes a status | |
| +| `render.go` | the two idear render callbacks, and `idear.TokenFrom` in `RenderSignup` | |
| | `handlers.go` | `idear.From(r)`, and an app route gated on Admin | |
| | `app_test.go` | the whole flow through real HTTP — the thing to copy | |
| -Three things in here are load-bearing rather than stylistic, and each has a |
| +Four things in here are load-bearing rather than stylistic, and each has a |
| comment at the site saying so: |
| - **`/` is behind `Require`.** Under the password plugin, deactivation is |
| @@ -58,3 +58,8 @@ comment at the site saying so: |
| - **One 404 renderer**, bound once and handed to both `idear.Config.NotFound` |
| and chi's own `NotFound`. Two of them is a membership oracle, and it is |
| the one misconfiguration idear cannot detect at runtime. |
| +- **`renderSignup` seeds its hidden `invite` field from |
| + `idear.TokenFrom(r)`.** `password.PageData` has nowhere to carry a token, |
| + so without this a signup that fails validation re-renders a form with an |
| + empty field and the invitee's *second* attempt is refused. If you rewrite |
| + this page — the likeliest thing to do to it — keep that line. |
diff --git a/example/app_test.go b/example/app_test.go| index 8530d4e..b073a29 100644 |
| --- a/example/app_test.go |
| +++ b/example/app_test.go |
| @@ -141,7 +141,12 @@ func (c *client) follow(res *result) *result { |
| // one. The "invite" field is what rs.CarryToken reads. |
| func (c *client) signUp(email, invite string) *result { |
| c.ta.t.Helper() |
| - form := url.Values{"email": {email}, "password": {"demo-password"}} |
| + return c.signUpWith(email, invite, "demo-password") |
| +} |
| + |
| +func (c *client) signUpWith(email, invite, pw string) *result { |
| + c.ta.t.Helper() |
| + form := url.Values{"email": {email}, "password": {pw}} |
| if invite != "" { |
| form.Set("invite", invite) |
| } |
| @@ -518,3 +523,46 @@ func TestSchemaAndModelsAgree(t *testing.T) { |
| t.Fatalf("Schema and Models disagree: %d pending change(s)", len(changes)) |
| } |
| } |
| + |
| +// TestInvitedSignupSurvivesAValidationFailure is the workaround in |
| +// renderSignup, pinned. |
| +// |
| +// password.PageData has Error, Email and ReturnTo and NOWHERE to put |
| +// an invitation token, so a signup that fails validation re-renders a |
| +// form whose hidden "invite" field comes back empty unless the app |
| +// puts it back. The invitee's FIRST attempt then looks fine and their |
| +// SECOND is refused for holding no token — "invited people can never |
| +// join", surfacing one step later than the mistake. |
| +// |
| +// This is the one piece of copy-this-code an app cannot get from |
| +// idear's mount, and rewriting the signup page is the likeliest thing |
| +// a copier does. Deleting idear.TokenFrom(r) from render.go turns the |
| +// 422 assertion below red. |
| +func TestInvitedSignupSurvivesAValidationFailure(t *testing.T) { |
| + ta := newTestApp(t) |
| + |
| + ada := ta.visitor() |
| + want(t, ada.signUp("ada@example.test", ""), http.StatusSeeOther, "claim") |
| + res := ada.post("/members/invitations", url.Values{ |
| + "email": {"kim@example.test"}, "role": {"admin"}, |
| + }) |
| + want(t, res, http.StatusSeeOther, "invite") |
| + token := ta.tokenFrom(ada.follow(res)) |
| + |
| + // A password too short for password's own rule: 422, and the form |
| + // comes back. The token must come back WITH it. |
| + kim := ta.visitor() |
| + res = kim.signUpWith("kim@example.test", token, "short") |
| + want(t, res, http.StatusUnprocessableEntity, "signup with a short password") |
| + if !strings.Contains(res.Body, `name="invite" value="`+token+`"`) { |
| + t.Fatalf("the re-rendered signup form dropped the invitation token: %q", res.Body) |
| + } |
| + |
| + // The second attempt — the one a real invitee makes — still works, |
| + // and still lands at the invited role rather than being refused. |
| + res = kim.signUpWith("kim@example.test", token, "demo-password") |
| + want(t, res, http.StatusSeeOther, "second signup attempt") |
| + if got := ta.member("kim@example.test"); got.Role != idear.RoleAdmin { |
| + t.Fatalf("second attempt admitted at %q, want admin", got.Role) |
| + } |
| +} |
diff --git a/example/models.go b/example/models.go| index eb58f47..2613477 100644 |
| --- a/example/models.go |
| +++ b/example/models.go |
| @@ -18,20 +18,23 @@ import ( |
| // Models is every model THIS APP's schema generator manages. |
| // |
| -// idear's models are deliberately NOT in it, and there is no way to |
| -// put them in it — idear does not export them. `rastrillo migration |
| -// generate` and `rastrillo migration check` replay this app's Schema |
| -// into a scratch database and diff the result against this list, as a |
| -// matched pair. idear's tables are created by idear's OWN migrations, |
| -// which reach the database through BootSchema below and never through |
| -// Schema, so a list that named &idear.Member{} would be diffed against |
| -// a schema that has no idear migrations in it: `check` goes |
| -// permanently red, and `generate` writes a second, GORM-flavoured |
| -// CREATE TABLE idear_members into this app's migration file that |
| -// collides at boot with idear's own. |
| +// idear's models are deliberately NOT in it, and NOTHING STOPS YOU |
| +// PUTTING THEM THERE: idear exports the types (idear.Member, |
| +// idear.Invitation — this app's own templates use them) but not the |
| +// list, so `[]any{&User{}, &Post{}, &idear.Member{}}` compiles fine. |
| // |
| -// TestSchemaAndModelsAgree is `migration check` in test form; it is |
| -// what catches this if it is ever got wrong. |
| +// It is wrong because `rastrillo migration generate` and `rastrillo |
| +// migration check` replay this app's Schema into a scratch database |
| +// and diff the result against this list, as a MATCHED PAIR. idear's |
| +// tables are created by idear's OWN migrations, which reach the |
| +// database through BootSchema below and never through Schema, so a |
| +// list naming &idear.Member{} is diffed against a schema that has no |
| +// idear migrations in it: `check` goes permanently red, and `generate` |
| +// writes a second, GORM-flavoured CREATE TABLE idear_members into this |
| +// app's migration file that collides at boot with idear's own. |
| +// |
| +// TestSchemaAndModelsAgree is `migration check` in test form, and it |
| +// is the only thing that catches this — the compiler will not. |
| var Models = []any{&User{}, &Post{}} |
| // User is the app's own identity row: an address and a password hash, |
diff --git a/example/render.go b/example/render.go| index 3456e67..2c3218d 100644 |
| --- a/example/render.go |
| +++ b/example/render.go |
| @@ -168,10 +168,15 @@ func (a *app) renderInvitation(w http.ResponseWriter, r *http.Request, d idear.I |
| // password.PageData carries Error, Email and ReturnTo and has nowhere |
| // to put a token, so a signup that fails validation — a password under |
| // eight characters, say — re-renders a form whose hidden "invite" |
| -// field would come back empty, and the invitee's second attempt would |
| -// be refused for having no token. Reading it back off the posted form |
| -// (CarryToken has already parsed it) is what keeps the second attempt |
| -// working. |
| +// field would come back empty, and the invitee's SECOND attempt would |
| +// be refused for having no token. The symptom is "invited people can |
| +// never join", and it only appears on the second try. |
| +// |
| +// idear.TokenFrom(r) is what keeps that attempt working: it hands back |
| +// what CarryToken already lifted off this very POST. |
| +// TestInvitedSignupSurvivesAValidationFailure is what notices if this |
| +// is ever dropped — which is the likeliest thing to happen to anyone |
| +// who rewrites this page. |
| type signupView struct { |
| password.PageData |
| Invite string |
| @@ -182,9 +187,6 @@ func (a *app) renderSignin(w http.ResponseWriter, r *http.Request, d password.Pa |
| } |
| func (a *app) renderSignup(w http.ResponseWriter, r *http.Request, d password.PageData) { |
| - invite := r.PostFormValue("invite") |
| - if invite == "" { |
| - invite = r.URL.Query().Get("invite") |
| - } |
| - a.execute(w, r, 0, "signup", flash.Flash{}, false, signupView{PageData: d, Invite: invite}) |
| + a.execute(w, r, 0, "signup", flash.Flash{}, false, |
| + signupView{PageData: d, Invite: idear.TokenFrom(r)}) |
| } |