amadan

oficina / docs Public

Skip sign-in only for an admitted session, keep the deep link on retry, re-pin estilo

Review findings (Astra, #65):

- A guest whose last share was revoked kept a valid session; admission
  sent / to /signin and the already-signed-in shortcut sent /signin
  back to /, forever. Both shortcuts (standalone and Home's /signin) now
  ask h.admitted, which runs the signed-in guard chain around a probe.
  Regressions through the real router, standalone and Home, both red on
  92e74ea (and the Home one under a mutation of its fix).
- "Use a different address" linked to a bare /signin, a fresh start that
  remembers the list, so the deep link was lost. It now carries the
  remembered page as return_to (spec §9.2). Regression red on 92e74ea.
- estilo re-pinned to 410c41a, whose narrow title layout keeps Rename
  reachable on a phone.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev fd644136fe7b2f3680b77b7561a77516416a3736 parent 92e74ea
8 files changed, +218 −14
  • go.mod +1 −1
  • go.sum +2 −2
  • internal/docs/app.go +5 −0
  • internal/docs/auth.go +15 −1
  • internal/docs/handlers.go +3 −0
  • internal/docs/home_routes.go +6 −3
  • internal/docs/signin_loop_test.go +160 −0
  • internal/docs/signin_return.go +26 −7
diff --git a/go.mod b/go.mod
index c6cede7..2ae1e2a 100644
--- a/go.mod
+++ b/go.mod
@@ -3,7 +3,7 @@ module github.com/carlosframework/docs
go 1.26.5
require (
- amadan.net/oficina/estilo.git v0.0.0-20260923182543-5b397d9c09df
+ amadan.net/oficina/estilo.git v0.0.0-20260923192124-410c41a95d8f
amadan.net/oficina/home/handoff v0.0.0-20260908102137-481d2d57f489
amadan.net/oficina/home/roster v0.0.0-20260908123429-74dda570e0cf
amadan.net/rastrillo/native/server v0.0.0-20260912204600-33ffe9941734
diff --git a/go.sum b/go.sum
index 93b0895..704b3b9 100644
--- a/go.sum
+++ b/go.sum
@@ -1,5 +1,5 @@
-amadan.net/oficina/estilo.git v0.0.0-20260923182543-5b397d9c09df h1:N7l6YbG1T37Q1IJKwE92EG/B5zVH/deACQ+FSlXILR8=
-amadan.net/oficina/estilo.git v0.0.0-20260923182543-5b397d9c09df/go.mod h1:4u0jNBtIdMLOsJyrScfqoEuYDJF7BBM3qM9YWbzt6aw=
+amadan.net/oficina/estilo.git v0.0.0-20260923192124-410c41a95d8f h1:FoYZf3Q6JFbff6hOoKSROFDJLllXFPCWuTgOSb1+2vs=
+amadan.net/oficina/estilo.git v0.0.0-20260923192124-410c41a95d8f/go.mod h1:4u0jNBtIdMLOsJyrScfqoEuYDJF7BBM3qM9YWbzt6aw=
amadan.net/oficina/home/handoff v0.0.0-20260908102137-481d2d57f489 h1:RPKtyry/71i06o8L+gjAaUyexbmG56kO1YPs601URAM=
amadan.net/oficina/home/handoff v0.0.0-20260908102137-481d2d57f489/go.mod h1:YISpzFJ+iMgrM0rUL032OSzsAQbP4/nozvXLCCOcn1k=
amadan.net/oficina/home/roster v0.0.0-20260908123429-74dda570e0cf h1:SRKjCUgtk42WwR7VSn+V0eLNk20mfiY8dQmzdw13Sbo=
diff --git a/internal/docs/app.go b/internal/docs/app.go
index 9af9048..4bde5b7 100644
--- a/internal/docs/app.go
+++ b/internal/docs/app.go
@@ -552,6 +552,11 @@ func New(cfg Config) (*App, error) {
return nil, err
}
}
+ if home == nil {
+ h.remembered = standalone.returnPath
+ } else {
+ h.remembered = home.navigation.returnPath
+ }
r := chi.NewRouter()
// Every route that is not a route is the 404 page for a browser, and a
// page GET to a POST-only path is too (errors.go).
diff --git a/internal/docs/auth.go b/internal/docs/auth.go
index 3a561fb..7a0c508 100644
--- a/internal/docs/auth.go
+++ b/internal/docs/auth.go
@@ -447,6 +447,20 @@ func (h *handlers) signinAgainPath() string {
return "/signin"
}
+// signinAgainHref is "Use a different address": the form again, carrying
+// the page this sign-in was started from (spec §9.2: "keeping the
+// return"). A bare /signin is a fresh start, which remembers the list, so
+// the deep link would be lost (review finding, Astra, #65).
+func (h *handlers) signinAgainHref(r *http.Request) string {
+ href := h.signinAgainPath()
+ if h.remembered != nil {
+ if path := h.remembered(r); path != "/" {
+ href += "?return_to=" + url.QueryEscape(path)
+ }
+ }
+ return href
+}
+
// signin serves GET /signin — the form itself, and the landing spot
// for every outcome rauth.Auth's handlers redirect back to
// (?sent=1, ?err=rate|address|expired|keymail|1, ?force=1). rate,
@@ -476,7 +490,7 @@ func (h *handlers) signin(w http.ResponseWriter, r *http.Request) {
Icon: icon,
Heading: title,
Body: rastrillo.T(r, "docs.signin.sent.body"),
- Link: &chrome.Link{Href: h.signinAgainPath(), Label: rastrillo.T(r, "docs.signin.sent.again")},
+ Link: &chrome.Link{Href: h.signinAgainHref(r), Label: rastrillo.T(r, "docs.signin.sent.again")},
}
} else {
form := &chrome.SignIn{
diff --git a/internal/docs/handlers.go b/internal/docs/handlers.go
index b5a9d2c..1820800 100644
--- a/internal/docs/handlers.go
+++ b/internal/docs/handlers.go
@@ -101,6 +101,9 @@ type handlers struct {
// switcher's "All apps", empty when none is configured.
peek func(*http.Request) (string, bool)
directory string
+ // remembered is the page a sign-in in progress will return to
+ // (signin_return.go), "/" when none.
+ remembered func(*http.Request) string
}
// listPage is GET /'s view data (spec §6.4). Title and the group
diff --git a/internal/docs/home_routes.go b/internal/docs/home_routes.go
index a5abdfb..188a490 100644
--- a/internal/docs/home_routes.go
+++ b/internal/docs/home_routes.go
@@ -118,9 +118,12 @@ func (h *homeRoutes) signin(w http.ResponseWriter, r *http.Request) {
return
}
// Already signed in to Docs: straight back, with no form and no new
- // Home exchange (spec §9.2). Read, not required: a person with no
- // session goes on to Home as before.
- if _, ok := h.bridge.sessions.From(r); ok {
+ // Home exchange (spec §9.2). A person with no session goes on to Home
+ // as before.
+ // Admitted, not merely holding a session: a refused identity must be
+ // able to reach sign-in, or the guard and this shortcut redirect to
+ // each other forever (h.handlers.admitted).
+ if _, ok := h.bridge.sessions.From(r); ok && h.handlers.admitted(r) {
alreadySignedIn(w, r)
return
}
diff --git a/internal/docs/signin_loop_test.go b/internal/docs/signin_loop_test.go
new file mode 100644
index 0000000..fe8a9b2
--- /dev/null
+++ b/internal/docs/signin_loop_test.go
@@ -0,0 +1,160 @@
+package docs
+
+import (
+ "net/http"
+ "net/http/httptest"
+ "regexp"
+ "strings"
+ "testing"
+)
+
+// browser is a cookie jar over the real router, asking for pages the way a
+// browser does (Accept: text/html, the app's own Host).
+type browser struct {
+ t *testing.T
+ app *App
+ jar map[string]*http.Cookie
+}
+
+func newBrowser(t *testing.T, app *App) *browser {
+ return &browser{t: t, app: app, jar: map[string]*http.Cookie{}}
+}
+
+func (b *browser) do(method, path, form string) *httptest.ResponseRecorder {
+ req := httptest.NewRequest(method, DefaultOrigin+path, strings.NewReader(form))
+ req.Header.Set("Accept", "text/html")
+ req.Header.Set("Sec-Fetch-Site", "same-origin")
+ if form != "" {
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ }
+ for _, c := range b.jar {
+ req.AddCookie(c)
+ }
+ rec := httptest.NewRecorder()
+ b.app.Handler().ServeHTTP(rec, req)
+ for _, c := range rec.Result().Cookies() {
+ if c.MaxAge < 0 {
+ delete(b.jar, c.Name)
+ } else {
+ b.jar[c.Name] = c
+ }
+ }
+ return rec
+}
+
+func (b *browser) signIn(address string) {
+ link, err := b.app.TestSignIn(b.t, address)
+ if err != nil {
+ b.t.Fatal(err)
+ }
+ b.do("GET", link, "")
+ b.do("GET", localLinkCompletePath, "")
+}
+
+// Review finding (Astra, #65): a guest whose last share is revoked still
+// holds a valid session, and admission rightly refuses it — so GET /
+// sends them to /signin. The already-signed-in shortcut must not send
+// them straight back to /, or the two redirect to each other forever.
+// Skipping the form needs a session the signed-in routes would admit.
+func TestARevokedGuestIsNotBouncedBetweenTheListAndSignIn(t *testing.T) {
+ app, err := NewForTestWith(t, Config{AllowedAddresses: "alice@example.com"})
+ if err != nil {
+ t.Fatal(err)
+ }
+ alice := newBrowser(t, app)
+ alice.signIn("alice@example.com")
+ created := alice.do("POST", "/documents", "title=Minutes")
+ sqid := strings.TrimPrefix(created.Header().Get("Location"), "/d/")
+ if rec := alice.do("POST", "/d/"+sqid+"/share", "address=guest@example.com&role=reader"); rec.Code != http.StatusSeeOther {
+ t.Fatalf("share: %d", rec.Code)
+ }
+
+ guest := newBrowser(t, app)
+ guest.signIn("guest@example.com")
+ if rec := guest.do("GET", "/d/"+sqid, ""); rec.Code != http.StatusOK {
+ t.Fatalf("the guest cannot read the shared document before revocation: %d", rec.Code)
+ }
+ // Already signed in and admitted: /signin goes straight back.
+ if rec := guest.do("GET", "/signin", ""); rec.Code != http.StatusSeeOther {
+ t.Fatalf("an admitted guest was shown the sign-in form again: %d", rec.Code)
+ }
+
+ if rec := alice.do("POST", "/d/"+sqid+"/share/revoke", "address=guest@example.com"); rec.Code != http.StatusSeeOther {
+ t.Fatalf("revoke: %d", rec.Code)
+ }
+ list := guest.do("GET", "/", "")
+ if list.Code != http.StatusSeeOther || !strings.HasPrefix(list.Header().Get("Location"), "/signin") {
+ t.Fatalf("a revoked guest reached the list: %d %q", list.Code, list.Header().Get("Location"))
+ }
+ signin := guest.do("GET", list.Header().Get("Location"), "")
+ if signin.Code != http.StatusOK {
+ t.Fatalf("GET %s: %d to %q — the list and sign-in redirect to each other", list.Header().Get("Location"), signin.Code, signin.Header().Get("Location"))
+ }
+ if !strings.Contains(signin.Body.String(), `<form rst-form estilo-signin-form`) {
+ t.Fatal("the revoked guest is not offered a way to sign in as someone else")
+ }
+}
+
+// Review finding (Astra, #65): "Use a different address" on the sent page
+// went to a bare /signin, which a fresh start treats as "remember the
+// list", so the deep link was lost. The retry keeps the remembered page.
+func TestUseADifferentAddressKeepsTheDeepLink(t *testing.T) {
+ app, _ := NewForTest(t)
+ owner := newBrowser(t, app)
+ owner.signIn("alice@example.com")
+ created := owner.do("POST", "/documents", "title=Minutes")
+ deep := created.Header().Get("Location") + "?from=mail"
+
+ b := newBrowser(t, app)
+ signin := b.do("GET", deep, "").Header().Get("Location")
+ b.do("GET", signin, "")
+ sent := b.do("POST", "/signin", "address=typo@example.com")
+ sentPage := b.do("GET", sent.Header().Get("Location"), "")
+ m := regexp.MustCompile(`<p><a href="([^"]+)">docs.signin.sent.again</a></p>`).FindStringSubmatch(sentPage.Body.String())
+ if m == nil {
+ t.Fatalf("no \"Use a different address\" link on the sent page (%d)", sentPage.Code)
+ }
+ again := strings.ReplaceAll(m[1], "&amp;", "&")
+ if rec := b.do("GET", again, ""); rec.Code != http.StatusOK {
+ t.Fatalf("GET %s: %d", again, rec.Code)
+ }
+ // The right address this time, in the same browser.
+ link, err := app.TestSignIn(t, "alice@example.com")
+ if err != nil {
+ t.Fatal(err)
+ }
+ b.do("GET", link, "")
+ landed := b.do("GET", localLinkCompletePath, "")
+ if landed.Header().Get("Location") != deep {
+ t.Fatalf("after \"Use a different address\" (%s) the sign-in landed on %q, want %q", again, landed.Header().Get("Location"), deep)
+ }
+}
+
+// The same loop through Home's /signin, which has the same shortcut: a
+// guest admitted only by a share, signed in locally, then revoked.
+func TestARevokedGuestIsNotBouncedThroughHomeSignIn(t *testing.T) {
+ f := newRouterHomeFixture(t)
+ alice := f.browser()
+ alice.localSignin(t, "alice@example.com")
+ created := alice.do("POST", "/documents", "title=Minutes")
+ sqid := strings.TrimPrefix(created.Header().Get("Location"), "/d/")
+ if rec := alice.do("POST", "/d/"+sqid+"/share", "address=carol@example.com&role=reader"); rec.Code != http.StatusSeeOther {
+ t.Fatalf("share: %d", rec.Code)
+ }
+ carol := f.browser()
+ carol.localSignin(t, "carol@example.com")
+ if rec := carol.do("GET", "/signin", ""); rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/" {
+ t.Fatalf("an admitted guest was not sent straight back: %d %q", rec.Code, rec.Header().Get("Location"))
+ }
+ if rec := alice.do("POST", "/d/"+sqid+"/share/revoke", "address=carol@example.com"); rec.Code != http.StatusSeeOther {
+ t.Fatalf("revoke: %d", rec.Code)
+ }
+ list := carol.do("GET", "/", "")
+ if list.Code != http.StatusSeeOther || !strings.HasPrefix(list.Header().Get("Location"), "/signin") {
+ t.Fatalf("a revoked guest reached the list: %d %q", list.Code, list.Header().Get("Location"))
+ }
+ next := carol.do("GET", list.Header().Get("Location"), "")
+ if next.Code == http.StatusSeeOther && next.Header().Get("Location") == "/" {
+ t.Fatal("Home's /signin sent a refused session straight back to / — the two redirect to each other")
+ }
+}
diff --git a/internal/docs/signin_return.go b/internal/docs/signin_return.go
index 5cbfbb2..4bd40f4 100644
--- a/internal/docs/signin_return.go
+++ b/internal/docs/signin_return.go
@@ -87,15 +87,34 @@ func alreadySignedIn(w http.ResponseWriter, r *http.Request) {
http.Redirect(w, r, target, http.StatusSeeOther)
}
-// standaloneSignin is GET /signin without Home: already signed in goes
-// back; a return_to is remembered for the landing; then the form.
+// admitted reports whether the signed-in routes would let this request's
+// session in right now, by running the same guard chain (h.requireCurrent:
+// session, identity, admission) around a probe. A session is not enough:
+// a guest whose last share was revoked still holds a valid one, admission
+// sends GET / to /signin, and skipping the form for them sent /signin
+// straight back to / — forever (review finding, Astra, #65).
+func (h *handlers) admitted(r *http.Request) bool {
+ if h.requireCurrent == nil {
+ return false
+ }
+ probe := r.Clone(r.Context())
+ probe.Method = http.MethodGet
+ probe.URL = &url.URL{Path: "/"}
+ probe.RequestURI = "/"
+ reached := false
+ h.requireCurrent(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached = true })).
+ ServeHTTP(&signinResponse{header: make(http.Header)}, probe)
+ return reached
+}
+
+// standaloneSignin is GET /signin without Home: already signed in (and
+// admitted) goes back; a return_to is remembered for the landing; then
+// the form.
func (h *handlers) standaloneSignin(n *localLinkNavigation) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
- if !signinOutcome(r) {
- if _, ok := h.viewer(r); ok {
- alreadySignedIn(w, r)
- return
- }
+ if !signinOutcome(r) && h.admitted(r) {
+ alreadySignedIn(w, r)
+ return
}
// Starting a sign-in remembers where to come back to, and a plain
// /signin remembers the list: a page remembered by an earlier,