fix(setup): render the TOTP step instead of the enroll flow's JSON
The first-run wizard's `admin.totp.begin` delegated to the enroll flow's
`TOTPBegin`, which is `webauthn.js`'s JSON API: the browser was shown raw
`{"otpauth_url":…}` and the wizard stalled at step 2, rendering no secret.
Extract the issue step into a write-free seam, `flow.Actions.IssueTOTP`, and
keep `TOTPBegin`'s JSON contract by having it call the seam. The wizard now
issues via the seam and renders its own HTML page (`text/html`, inside the
shell, no inline scripts) showing the otpauth URL, the human-readable secret
for manual entry, and the verify form (pre-session CSRF, A15).
`setup.Enroller` drops `TOTPBegin` — the wizard no longer calls it, and a
JSON-writing method on a no-JS seam is the coupling that caused the bug.
Tighten the tests that missed it: `setup_wizard_test.go` asserts the `begin`
response is `text/html` and carries the secret and the verify form (not merely
200), keeping the end-to-end begin→code→303 path; `enroll_login_test.go` pins
the enroll flow's JSON contract (`application/json` + `otpauth_url`).
Spec: docs/specs/spec-005-setup-wizard.md §11.
This commit is contained in:
parent
9fdc9eded6
commit
7c1826e115
6 changed files with 144 additions and 17 deletions
|
|
@ -23,7 +23,7 @@ import (
|
||||||
// stubEnroller satisfies setup.Enroller — this test never runs a ceremony.
|
// stubEnroller satisfies setup.Enroller — this test never runs a ceremony.
|
||||||
type stubEnroller struct{}
|
type stubEnroller struct{}
|
||||||
|
|
||||||
func (stubEnroller) TOTPBegin(http.ResponseWriter, *http.Request, string) error { return nil }
|
func (stubEnroller) IssueTOTP(context.Context, string) (string, error) { return "", nil }
|
||||||
func (stubEnroller) TOTPVerify(http.ResponseWriter, *http.Request, string) error { return nil }
|
func (stubEnroller) TOTPVerify(http.ResponseWriter, *http.Request, string) error { return nil }
|
||||||
|
|
||||||
// setupYAML is a fresh-instance config: setup mode, every secret a placeholder.
|
// setupYAML is a fresh-instance config: setup mode, every secret a placeholder.
|
||||||
|
|
|
||||||
|
|
@ -41,6 +41,11 @@ import (
|
||||||
// finish (no server-side table needed).
|
// finish (no server-side table needed).
|
||||||
const CeremonyCookieName = "myday_webauthn"
|
const CeremonyCookieName = "myday_webauthn"
|
||||||
|
|
||||||
|
// errNotPending reports that a GUID does not map to a pending identity inside
|
||||||
|
// its enrollment window — the ceremony's "not found" (A1). It is unexported:
|
||||||
|
// only this package renders a response for it (TOTPBegin's 404).
|
||||||
|
var errNotPending = errors.New("flow: no pending identity")
|
||||||
|
|
||||||
// Deps assembles the flow's dependencies.
|
// Deps assembles the flow's dependencies.
|
||||||
type Deps struct {
|
type Deps struct {
|
||||||
// Identities resolves GUIDs, activates identities and checks the window.
|
// Identities resolves GUIDs, activates identities and checks the window.
|
||||||
|
|
@ -223,23 +228,41 @@ func (a *Actions) EnrollFinish(w http.ResponseWriter, r *http.Request, guid stri
|
||||||
return writeJSON(w, http.StatusOK, []byte(`{"status":"credential-stored"}`))
|
return writeJSON(w, http.StatusOK, []byte(`{"status":"credential-stored"}`))
|
||||||
}
|
}
|
||||||
|
|
||||||
// TOTPBegin issues (and encrypts) the TOTP secret and returns the otpauth URL.
|
// IssueTOTP issues (and encrypts) a fresh TOTP secret for the identity behind
|
||||||
func (a *Actions) TOTPBegin(w http.ResponseWriter, r *http.Request, guid string) error {
|
// guid and returns the otpauth URL. It WRITES NO RESPONSE — the caller owns the
|
||||||
ctx := r.Context()
|
// reply: the JSON ceremony below (the WebAuthn client's contract), or SPEC-005's
|
||||||
|
// wizard, which is server-rendered and no-JS and renders the secret itself.
|
||||||
|
//
|
||||||
|
// A guid that does not resolve to a pending identity inside its bounded
|
||||||
|
// enrollment window reports errNotPending (mapped to 404 by TOTPBegin).
|
||||||
|
func (a *Actions) IssueTOTP(ctx context.Context, guid string) (string, error) {
|
||||||
idn, ok, err := a.pendingInWindow(ctx, guid)
|
idn, ok, err := a.pendingInWindow(ctx, guid)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return "", err
|
||||||
}
|
}
|
||||||
if !ok {
|
if !ok {
|
||||||
writeError(w, http.StatusNotFound, "not found")
|
return "", errNotPending
|
||||||
return nil
|
|
||||||
}
|
}
|
||||||
issued, err := a.d.TOTP.Begin(ctx, idn.ID, idn.GUID)
|
issued, err := a.d.TOTP.Begin(ctx, idn.ID, idn.GUID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return "", err
|
||||||
}
|
}
|
||||||
a.audit(ctx, "enroll.totp.begin", idn, "secret issued")
|
a.audit(ctx, "enroll.totp.begin", idn, "secret issued")
|
||||||
payload, err := json.Marshal(map[string]string{"otpauth_url": issued.URL})
|
return issued.URL, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// TOTPBegin answers the WebAuthn client with the otpauth URL as JSON —
|
||||||
|
// webauthn.js's contract, kept intact. It is IssueTOTP plus the JSON write.
|
||||||
|
func (a *Actions) TOTPBegin(w http.ResponseWriter, r *http.Request, guid string) error {
|
||||||
|
url, err := a.IssueTOTP(r.Context(), guid)
|
||||||
|
if err != nil {
|
||||||
|
if errors.Is(err, errNotPending) {
|
||||||
|
writeError(w, http.StatusNotFound, "not found")
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
payload, err := json.Marshal(map[string]string{"otpauth_url": url})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -399,7 +399,15 @@ func TestSpec006AC6_TOTPSecretEncryptedAndNeverLogged(t *testing.T) {
|
||||||
}
|
}
|
||||||
|
|
||||||
// 2) The otpauth URL is shown to the user (the one display) — but the
|
// 2) The otpauth URL is shown to the user (the one display) — but the
|
||||||
// secret never reached the log (A9).
|
// secret never reached the log (A9). The enroll ceremony answers the
|
||||||
|
// WebAuthn client (`webauthn.js`) with JSON, not a page (SPEC-006 §4): the
|
||||||
|
// setup wizard is the server-rendered exception (SPEC-005 §4).
|
||||||
|
if ct := rec.Header().Get("Content-Type"); !strings.HasPrefix(ct, "application/json") {
|
||||||
|
t.Errorf("totp.begin Content-Type = %q, want application/json (webauthn.js's contract)", ct)
|
||||||
|
}
|
||||||
|
if !strings.Contains(rec.Body.String(), `"otpauth_url"`) {
|
||||||
|
t.Error("totp.begin did not return the otpauth_url JSON field (webauthn.js's contract)")
|
||||||
|
}
|
||||||
if !strings.Contains(rec.Body.String(), "otpauth://") {
|
if !strings.Contains(rec.Body.String(), "otpauth://") {
|
||||||
t.Error("totp.begin did not return an otpauth URL")
|
t.Error("totp.begin did not return an otpauth URL")
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -241,17 +241,40 @@ func (h *setupHarness) beginAdmin(t *testing.T) {
|
||||||
|
|
||||||
// enrollAdmin runs step 2b — the TOTP-only ceremony (D-041) — and returns the
|
// enrollAdmin runs step 2b — the TOTP-only ceremony (D-041) — and returns the
|
||||||
// session id the flow minted on completion (SPEC-006 reused).
|
// session id the flow minted on completion (SPEC-006 reused).
|
||||||
|
//
|
||||||
|
// The `begin` response must be the wizard's server-rendered TOTP PAGE, not the
|
||||||
|
// enroll flow's JSON API (`{"otpauth_url":…}` — that is webauthn.js's contract):
|
||||||
|
// the browser must never be shown raw JSON. This is the regression the fix
|
||||||
|
// guards against, so it is asserted here (content type + secret + verify form).
|
||||||
func (h *setupHarness) enrollAdmin(t *testing.T) string {
|
func (h *setupHarness) enrollAdmin(t *testing.T) string {
|
||||||
t.Helper()
|
t.Helper()
|
||||||
|
|
||||||
if rec := h.post("admin.totp.begin", nil); rec.Code != http.StatusOK {
|
begin := h.post("admin.totp.begin", nil)
|
||||||
t.Fatalf("admin.totp.begin = %d, want 200", rec.Code)
|
if begin.Code != http.StatusOK {
|
||||||
|
t.Fatalf("admin.totp.begin = %d, want 200", begin.Code)
|
||||||
|
}
|
||||||
|
if ct := begin.Header().Get("Content-Type"); !strings.HasPrefix(ct, "text/html") {
|
||||||
|
t.Errorf("admin.totp.begin Content-Type = %q, want text/html (the wizard renders a page, not JSON)", ct)
|
||||||
|
}
|
||||||
|
body := begin.Body.String()
|
||||||
|
if strings.Contains(body, `"otpauth_url"`) {
|
||||||
|
t.Error("admin.totp.begin answered with the enroll flow's JSON, not the wizard's TOTP page")
|
||||||
|
}
|
||||||
|
if !strings.Contains(body, "otpauth://") {
|
||||||
|
t.Error("the TOTP page did not show the otpauth URL")
|
||||||
|
}
|
||||||
|
if !strings.Contains(body, "admin.totp.verify") {
|
||||||
|
t.Error("the TOTP page did not render the verify form")
|
||||||
}
|
}
|
||||||
|
|
||||||
secret, err := h.ids.TOTPSecret(h.ctx, 1)
|
secret, err := h.ids.TOTPSecret(h.ctx, 1)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("TOTPSecret: %v", err)
|
t.Fatalf("TOTPSecret: %v", err)
|
||||||
}
|
}
|
||||||
|
if !strings.Contains(body, string(secret)) {
|
||||||
|
t.Error("the TOTP page did not show the issued secret")
|
||||||
|
}
|
||||||
|
|
||||||
code, err := pqotp.GenerateCode(string(secret), time.Now())
|
code, err := pqotp.GenerateCode(string(secret), time.Now())
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("GenerateCode: %v", err)
|
t.Fatalf("GenerateCode: %v", err)
|
||||||
|
|
|
||||||
|
|
@ -3,6 +3,7 @@ package setup
|
||||||
import (
|
import (
|
||||||
"html"
|
"html"
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"net/url"
|
||||||
|
|
||||||
"git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/web"
|
"git.lohmar.co.uk/cclohmar/NextMyDay/internal/core/web"
|
||||||
)
|
)
|
||||||
|
|
@ -45,6 +46,47 @@ func wizardPage(st step, token, domain string) web.Page {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// totpPage renders the wizard's TOTP step (SPEC-005 §4) once the secret has
|
||||||
|
// been issued: the otpauth URL, its human-readable secret for manual entry, and
|
||||||
|
// the verify form. The wizard is server-rendered and works with no JavaScript,
|
||||||
|
// so the secret is shown as text rather than encoded in a QR image; the page
|
||||||
|
// renders inside the shell (no inline scripts) and the verify form carries the
|
||||||
|
// signed pre-session CSRF token (A15).
|
||||||
|
func totpPage(token, otpauthURL string) web.Page {
|
||||||
|
c := html.EscapeString(token)
|
||||||
|
u := html.EscapeString(otpauthURL)
|
||||||
|
secret := html.EscapeString(secretFromOtpauth(otpauthURL))
|
||||||
|
return web.Page{
|
||||||
|
Title: "MyDay setup",
|
||||||
|
Guest: true,
|
||||||
|
CSRF: token,
|
||||||
|
Content: web.Markup(`<h1>MyDay setup</h1>
|
||||||
|
<p>Add the authenticator, then enter a code from it to finish.</p>` +
|
||||||
|
`<section aria-label="enroll"><h2>2. Enroll the admin</h2>` +
|
||||||
|
`<p>A passkey can be added later, at first sign-in.</p>` +
|
||||||
|
`<section aria-label="totp"><h3>Authenticator app</h3>` +
|
||||||
|
`<p>Open this link in your authenticator app:</p>` +
|
||||||
|
`<p><a href="` + u + `">` + u + `</a></p>` +
|
||||||
|
`<p>Or enter this secret by hand:</p>` +
|
||||||
|
`<p><code>` + secret + `</code></p>` +
|
||||||
|
`<form method="post" action="/setup?do=setup&action=admin.totp.verify">` +
|
||||||
|
`<input type="hidden" name="csrf" value="` + c + `">` +
|
||||||
|
`<label>Code <input name="code" inputmode="numeric" autocomplete="one-time-code"></label>` +
|
||||||
|
`<button type="submit">Verify code</button></form></section></section>`),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// secretFromOtpauth pulls the shared secret out of an otpauth:// URL for the
|
||||||
|
// manual-entry fallback. It returns "" when the URL carries none — the page then
|
||||||
|
// shows the link alone. The secret is never logged.
|
||||||
|
func secretFromOtpauth(raw string) string {
|
||||||
|
u, err := url.Parse(raw)
|
||||||
|
if err != nil {
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
return u.Query().Get("secret")
|
||||||
|
}
|
||||||
|
|
||||||
// codeFormPage renders the D-044 first-run page — what `GET /setup` shows while
|
// codeFormPage renders the D-044 first-run page — what `GET /setup` shows while
|
||||||
// `setup: true` and a required `setup_code` has not yet been entered. It is
|
// `setup: true` and a required `setup_code` has not yet been entered. It is
|
||||||
// deliberately friendly: the point is that the domain must NOT look "down".
|
// deliberately friendly: the point is that the domain must NOT look "down".
|
||||||
|
|
|
||||||
|
|
@ -69,8 +69,17 @@ type Users interface {
|
||||||
// Enroller is the SPEC-006 TOTP ceremony, reused so the wizard never
|
// Enroller is the SPEC-006 TOTP ceremony, reused so the wizard never
|
||||||
// duplicates the work (SPEC-005 §4). Step 2 enrolls TOTP only (D-041) — the
|
// duplicates the work (SPEC-005 §4). Step 2 enrolls TOTP only (D-041) — the
|
||||||
// wizard never calls the WebAuthn registration actions.
|
// wizard never calls the WebAuthn registration actions.
|
||||||
|
//
|
||||||
|
// The seam carries no JSON-writing method: the wizard is server-rendered and
|
||||||
|
// no-JS, so it renders the issued secret and the verify form itself rather than
|
||||||
|
// delegating to the enroll flow's JSON API (`webauthn.js`'s contract) — the
|
||||||
|
// exact coupling that stalled the wizard's TOTP step (SPEC-005 §11).
|
||||||
type Enroller interface {
|
type Enroller interface {
|
||||||
TOTPBegin(w http.ResponseWriter, r *http.Request, guid string) error
|
// IssueTOTP issues (and persists) the TOTP secret and returns the otpauth
|
||||||
|
// URL, writing NO response — the caller owns the reply.
|
||||||
|
IssueTOTP(ctx context.Context, guid string) (string, error)
|
||||||
|
// TOTPVerify verifies a code: it completes the admin's enrollment (the
|
||||||
|
// wizard's TOTP-only mode, D-041) and mints the session.
|
||||||
TOTPVerify(w http.ResponseWriter, r *http.Request, guid string) error
|
TOTPVerify(w http.ResponseWriter, r *http.Request, guid string) error
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -330,9 +339,14 @@ func (a *Actions) ensureAdminIdentity(ctx context.Context) (repaired bool, err e
|
||||||
return true, nil
|
return true, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// TOTPBegin issues the admin's TOTP secret.
|
// TOTPBegin issues the admin's TOTP secret and renders the wizard's TOTP page:
|
||||||
|
// the otpauth URL with its human-readable secret and the verify form (SPEC-005
|
||||||
|
// §4). The wizard is server-rendered and works with no JavaScript, so it does
|
||||||
|
// NOT answer with the enroll flow's JSON (`webauthn.js`'s contract) — it issues
|
||||||
|
// the secret through the write-free IssueTOTP seam and renders the page itself.
|
||||||
func (a *Actions) TOTPBegin(w http.ResponseWriter, r *http.Request) error {
|
func (a *Actions) TOTPBegin(w http.ResponseWriter, r *http.Request) error {
|
||||||
guid, ok, err := a.pendingGUID(r.Context())
|
ctx := r.Context()
|
||||||
|
guid, ok, err := a.pendingGUID(ctx)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
|
@ -340,7 +354,15 @@ func (a *Actions) TOTPBegin(w http.ResponseWriter, r *http.Request) error {
|
||||||
writeError(w, http.StatusConflict, "no admin is awaiting enrollment")
|
writeError(w, http.StatusConflict, "no admin is awaiting enrollment")
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
return a.d.Enroll.TOTPBegin(w, r, guid)
|
url, err := a.d.Enroll.IssueTOTP(ctx, guid)
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
token, err := a.d.CSRF.Ensure(w, r)
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
return renderHTML(w, http.StatusOK, totpPage(token, url))
|
||||||
}
|
}
|
||||||
|
|
||||||
// TOTPVerify verifies the code, activating the identity and minting a session
|
// TOTPVerify verifies the code, activating the identity and minting a session
|
||||||
|
|
@ -557,7 +579,9 @@ func (a *Actions) adminGUID(ctx context.Context) (string, error) {
|
||||||
}
|
}
|
||||||
|
|
||||||
// pendingGUID returns the admin's GUID when its identity still awaits
|
// pendingGUID returns the admin's GUID when its identity still awaits
|
||||||
// enrollment; ok=false means there is nothing to enroll.
|
// enrollment — pending AND inside its bounded enrollment window (SPEC-004 §4,
|
||||||
|
// A1). ok=false means there is nothing to enroll: no identity, another status,
|
||||||
|
// or the window has closed.
|
||||||
func (a *Actions) pendingGUID(ctx context.Context) (guid string, ok bool, err error) {
|
func (a *Actions) pendingGUID(ctx context.Context) (guid string, ok bool, err error) {
|
||||||
idn, err := a.d.Identities.ByUserID(ctx, adminUserID)
|
idn, err := a.d.Identities.ByUserID(ctx, adminUserID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|
@ -569,6 +593,13 @@ func (a *Actions) pendingGUID(ctx context.Context) (guid string, ok bool, err er
|
||||||
if idn.Status != auth.StatusPendingEnrollment {
|
if idn.Status != auth.StatusPendingEnrollment {
|
||||||
return "", false, nil
|
return "", false, nil
|
||||||
}
|
}
|
||||||
|
inWindow, err := a.d.Identities.WithinEnrollmentWindow(ctx, adminUserID)
|
||||||
|
if err != nil {
|
||||||
|
return "", false, err
|
||||||
|
}
|
||||||
|
if !inWindow {
|
||||||
|
return "", false, nil
|
||||||
|
}
|
||||||
return idn.GUID, true, nil
|
return idn.GUID, true, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue