amadan

oficina / calendar Public

Review: error pages resolve the viewer like every page

Handlers now raise errors through h.errorPage, which resolves the viewer
with h.viewer (a Home session's person), so an error page carries the
same chrome as the page that failed. The free ErrorPage stays for the
outer, sessionless recoverer. A live session whose person cannot be
resolved (a removed Home person on /) still gets the account menu, to
sign out from, but it says "Signed in" and names nobody: no
"Account: " over an empty address.

viewerFrom no longer resolves a Home session twice per page: it reads the
person requirePerson already put on the request, takes the Home path only
for a Home session, and copes with Home being switched off.

The new assertion in TestHomeRouterRetainsExistingCalendarIdentity
failed before ("drew an empty identity"). Fable 5.1 review of #17, P3.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 1d6483d492cfd9dcac54d5689405821d5df8ef5f parent 56f1c47
12 files changed, +86 −36
  • internal/calendar/app.go +13 −0
  • internal/calendar/chrome_calendar.go +10 −2
  • internal/calendar/handlers_booking_form.go +2 −2
  • internal/calendar/handlers_booking_settings.go +1 −1
  • internal/calendar/handlers_event_delete.go +5 −5
  • internal/calendar/handlers_events.go +9 −9
  • internal/calendar/handlers_misc.go +3 −3
  • internal/calendar/handlers_quick.go +1 −1
  • internal/calendar/home.go +1 −1
  • internal/calendar/home_router_test.go +15 −0
  • internal/calendar/person.go +5 −5
  • internal/calendar/render.go +21 −7
diff --git a/internal/calendar/app.go b/internal/calendar/app.go
index b006a91..6fcdc9f 100644
--- a/internal/calendar/app.go
+++ b/internal/calendar/app.go
@@ -15,6 +15,7 @@ import (
rauth "github.com/carlosframework/rastrillo/auth"
"github.com/carlosframework/rastrillo/db"
"github.com/carlosframework/rastrillo/migrate"
+ "github.com/carlosframework/rastrillo/sessions"
"amadan.net/oficina/calendar/internal/calendar/seal"
)
@@ -165,6 +166,18 @@ func New(cfg Config) (*App, error) {
h.homeEnabled = true
h.directory = homeSettings.Directory
h.viewerFrom = func(r *http.Request) (rauth.Identity, bool) {
+ // requirePerson has already resolved the person on every page
+ // behind it; resolve again only where it has not run (public
+ // pages, errors raised before it).
+ if p, ok := CurrentPerson(r); ok {
+ return rauth.Identity{Address: p.Address}, true
+ }
+ if a.home == nil {
+ return viewer(r)
+ }
+ if s, ok := sessions.Current(r); !ok || s.Method != connect.HomeMethod {
+ return viewer(r)
+ }
p, err := a.homePerson(r)
return rauth.Identity{Address: p.Address}, err == nil
}
diff --git a/internal/calendar/chrome_calendar.go b/internal/calendar/chrome_calendar.go
index 0e051fe..ef7a1b8 100644
--- a/internal/calendar/chrome_calendar.go
+++ b/internal/calendar/chrome_calendar.go
@@ -36,8 +36,16 @@ func calendarChrome(r *http.Request, signedIn bool, address string, mode chrome.
if directory != "" {
bar.Switcher = &chrome.Switcher{Label: "Oficina apps", AllApps: estilomenu.Item{Href: directory, Label: "All apps"}}
}
- bar.Account = &chrome.Account{TriggerLabel: "Account: " + address, Initials: chrome.Initials("", address), Address: address,
- Labels: chrome.AccountLabels{SignedInAs: "Signed in as", Appearance: "Appearance", Help: "Help"},
+ // A live session whose person could not be resolved (a removed Home
+ // person on an error page) still gets the menu, to sign out from, but
+ // names nobody: "Signed in", the person glyph, no address (spec §8.2,
+ // as the hub directory does).
+ trigger, signedInAs := "Account: "+address, "Signed in as"
+ if address == "" {
+ trigger, signedInAs = "Account", "Signed in"
+ }
+ bar.Account = &chrome.Account{TriggerLabel: trigger, Initials: chrome.Initials("", address), Address: address,
+ Labels: chrome.AccountLabels{SignedInAs: signedInAs, Appearance: "Appearance", Help: "Help"},
Items: []estilomenu.Item{{Href: "/settings", Label: "Settings"}, {Href: "/settings/booking", Label: "Booking pages"}, {Href: "/import", Label: "Import a calendar"}},
Appearance: &chrome.AppearanceMenu{Action: "/appearance", Return: r.URL.RequestURI(), Options: chrome.AppearanceOptions(mode, "Light", "Dark", "System")}, Help: []estilomenu.Item{{Href: "/help/open-source", Label: "Open source"}}, SignOut: chrome.SignOut{Action: "/signout", Label: "Sign out"}}
return bar
diff --git a/internal/calendar/handlers_booking_form.go b/internal/calendar/handlers_booking_form.go
index e900119..0138e37 100644
--- a/internal/calendar/handlers_booking_form.go
+++ b/internal/calendar/handlers_booking_form.go
@@ -88,7 +88,7 @@ func (h *handlers) editBookingPage(w http.ResponseWriter, r *http.Request) {
id, _ := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64)
page, err := h.store.BookingPage(r.Context(), p.ID, id)
if NotFound(err) {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
if err != nil {
@@ -130,7 +130,7 @@ func (h *handlers) saveBookingPage(w http.ResponseWriter, r *http.Request) {
err = h.store.UpdateBookingPage(r.Context(), p.ID, f.ID, in, windows)
}
if NotFound(err) {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
if ve, ok := Invalid(err); ok {
diff --git a/internal/calendar/handlers_booking_settings.go b/internal/calendar/handlers_booking_settings.go
index 2203b05..05c029e 100644
--- a/internal/calendar/handlers_booking_settings.go
+++ b/internal/calendar/handlers_booking_settings.go
@@ -54,7 +54,7 @@ func (h *handlers) publishBookingPage(w http.ResponseWriter, r *http.Request) {
_ = r.ParseForm()
err := h.store.PublishBookingPage(r.Context(), p.ID, id, r.FormValue("publish") == "1")
if NotFound(err) {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
if err != nil {
diff --git a/internal/calendar/handlers_event_delete.go b/internal/calendar/handlers_event_delete.go
index 6931676..1ac09b3 100644
--- a/internal/calendar/handlers_event_delete.go
+++ b/internal/calendar/handlers_event_delete.go
@@ -12,7 +12,7 @@ func (h *handlers) deleteEvent(w http.ResponseWriter, r *http.Request) {
p, _ := CurrentPerson(r)
id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64)
if err != nil {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
_ = r.ParseForm()
@@ -41,9 +41,9 @@ func (h *handlers) deleteEvent(w http.ResponseWriter, r *http.Request) {
func (h *handlers) deleteFailed(w http.ResponseWriter, r *http.Request, err error) {
switch {
case NotFound(err):
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
case err == ErrReadOnly:
- ErrorPage(w, r, http.StatusForbidden, "")
+ h.errorPage(w, r, http.StatusForbidden, "")
default:
h.fail(w, r, "delete event", err)
}
@@ -56,12 +56,12 @@ func (h *handlers) confirmDelete(w http.ResponseWriter, r *http.Request) {
p, _ := CurrentPerson(r)
id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64)
if err != nil {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
e, err := h.store.Event(r.Context(), p.ID, id)
if NotFound(err) {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
if err != nil {
diff --git a/internal/calendar/handlers_events.go b/internal/calendar/handlers_events.go
index 4699e90..f773d76 100644
--- a/internal/calendar/handlers_events.go
+++ b/internal/calendar/handlers_events.go
@@ -73,7 +73,7 @@ func (h *handlers) createEvent(w http.ResponseWriter, r *http.Request) {
e, err := h.store.CreateEvent(r.Context(), p.ID, in)
switch {
case NotFound(err):
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
case err != nil:
if ve, isVE := Invalid(err); isVE {
@@ -101,12 +101,12 @@ func (h *handlers) showEvent(w http.ResponseWriter, r *http.Request) {
p, _ := CurrentPerson(r)
id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64)
if err != nil {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
e, err := h.store.Event(r.Context(), p.ID, id)
if NotFound(err) {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
if err != nil {
@@ -120,7 +120,7 @@ func (h *handlers) showEvent(w http.ResponseWriter, r *http.Request) {
if raw := chi.URLParam(r, "occ"); raw != "" {
local, ok := expandCompact(raw)
if !ok {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
occurrence = local
@@ -161,12 +161,12 @@ func (h *handlers) editEvent(w http.ResponseWriter, r *http.Request) {
p, _ := CurrentPerson(r)
id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64)
if err != nil {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
e, err := h.store.Event(r.Context(), p.ID, id)
if NotFound(err) {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
if err != nil {
@@ -209,7 +209,7 @@ func (h *handlers) updateEvent(w http.ResponseWriter, r *http.Request) {
p, _ := CurrentPerson(r)
id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64)
if err != nil {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
in, f, ok := h.readEventForm(r, p)
@@ -247,9 +247,9 @@ func (h *handlers) updateEvent(w http.ResponseWriter, r *http.Request) {
}
switch {
case NotFound(err):
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
case err == ErrReadOnly:
- ErrorPage(w, r, http.StatusForbidden, "")
+ h.errorPage(w, r, http.StatusForbidden, "")
case err != nil:
h.fail(w, r, "update event", err)
default:
diff --git a/internal/calendar/handlers_misc.go b/internal/calendar/handlers_misc.go
index a85ea46..9ead269 100644
--- a/internal/calendar/handlers_misc.go
+++ b/internal/calendar/handlers_misc.go
@@ -174,7 +174,7 @@ func (h *handlers) importICS(w http.ResponseWriter, r *http.Request) {
}
calID, _ := strconv.ParseInt(r.FormValue("calendar_id"), 10, 64)
if _, err := h.store.Calendar(r.Context(), p.ID, calID); err != nil {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return
}
file, _, err := r.FormFile("file")
@@ -242,12 +242,12 @@ func (h *handlers) calendarParam(w http.ResponseWriter, r *http.Request) (Person
p, _ := CurrentPerson(r)
id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64)
if err != nil {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return p, CalendarView{}, false
}
cal, err := h.store.Calendar(r.Context(), p.ID, id)
if NotFound(err) {
- ErrorPage(w, r, http.StatusNotFound, "")
+ h.errorPage(w, r, http.StatusNotFound, "")
return p, CalendarView{}, false
}
if err != nil {
diff --git a/internal/calendar/handlers_quick.go b/internal/calendar/handlers_quick.go
index 1655ae6..800e832 100644
--- a/internal/calendar/handlers_quick.go
+++ b/internal/calendar/handlers_quick.go
@@ -115,7 +115,7 @@ func (h *handlers) quickParse(w http.ResponseWriter, r *http.Request) {
func (h *handlers) quickAdd(w http.ResponseWriter, r *http.Request) {
p, _ := CurrentPerson(r)
if err := r.ParseForm(); err != nil {
- ErrorPage(w, r, http.StatusBadRequest, "")
+ h.errorPage(w, r, http.StatusBadRequest, "")
return
}
text := strings.TrimSpace(r.FormValue("text"))
diff --git a/internal/calendar/home.go b/internal/calendar/home.go
index 70e4c90..b60a4c1 100644
--- a/internal/calendar/home.go
+++ b/internal/calendar/home.go
@@ -66,7 +66,7 @@ func (a *App) newHome(settings *connect.DecodedCredentials, list allowlist) (*ho
}
var buf bytes.Buffer
if err := tree.ExecuteTemplate(&buf, "layout", a.h.page(r, data.Title, view)); err != nil {
- ErrorPage(w, r, http.StatusInternalServerError, "")
+ a.h.errorPage(w, r, http.StatusInternalServerError, "")
return
}
w.Header().Set("Cache-Control", "no-store")
diff --git a/internal/calendar/home_router_test.go b/internal/calendar/home_router_test.go
index 94db8f7..e1469bf 100644
--- a/internal/calendar/home_router_test.go
+++ b/internal/calendar/home_router_test.go
@@ -114,6 +114,10 @@ func TestHomeRouterRetainsExistingCalendarIdentity(t *testing.T) {
f.local(t)
if w := f.do("GET", "/day/2026-09-10", ""); w.Code != 200 {
t.Fatal("local calendar", w.Code, w.Body.String())
+ } else if !strings.Contains(w.Body.String(), `aria-label="Account: alice@example.test"`) {
+ // With Home on, a LOCAL session's chrome still names the person
+ // (viewerFrom only takes the Home path for a Home session).
+ t.Fatal("local session in Home mode drew no account menu")
}
var person Person
if err := f.app.db.G.Where("address=?", "alice@example.test").First(&person).Error; err != nil {
@@ -196,6 +200,17 @@ func TestHomeRouterRetainsExistingCalendarIdentity(t *testing.T) {
}
if w := f.do("GET", "/", ""); w.Code != 404 {
t.Fatal("deleted account admitted", w.Code)
+ } else {
+ // The 404 still has a session to sign out of, but no identity:
+ // the account menu says "Signed in" and names nobody, rather than
+ // "Account: " and "Signed in as" over an empty address.
+ body := w.Body.String()
+ if strings.Contains(body, `aria-label="Account: "`) || strings.Contains(body, "Signed in as") {
+ t.Fatal("error page drew an empty identity in the account menu")
+ }
+ if !strings.Contains(body, "estilo-account") || !strings.Contains(body, ">Signed in<") || !strings.Contains(body, "Sign out") {
+ t.Fatal("error page lost the account menu for a live session")
+ }
}
replacement, err := f.app.store.EnsurePerson(t.Context(), "replacement@example.test")
if err != nil || replacement.ID == person.ID {
diff --git a/internal/calendar/person.go b/internal/calendar/person.go
index be8e2b9..b35370c 100644
--- a/internal/calendar/person.go
+++ b/internal/calendar/person.go
@@ -30,12 +30,12 @@ func (a *App) requirePerson(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if session, ok := sessions.Current(r); ok && session.Method == connect.HomeMethod {
if a.home == nil {
- ErrorPage(w, r, http.StatusNotFound, "")
+ a.h.errorPage(w, r, http.StatusNotFound, "")
return
}
p, err := a.homePerson(r)
if err != nil {
- ErrorPage(w, r, http.StatusNotFound, "")
+ a.h.errorPage(w, r, http.StatusNotFound, "")
return
}
next.ServeHTTP(w, r.WithContext(context.WithValue(r.Context(), personCtxKey{}, p)))
@@ -48,11 +48,11 @@ func (a *App) requirePerson(next http.Handler) http.Handler {
// reaching here means the two disagree. Refuse rather than
// continue with no viewer.
a.logger.Error("calendar: session with no identity reached requirePerson", "path", r.URL.Path)
- ErrorPage(w, r, http.StatusInternalServerError, "")
+ a.h.errorPage(w, r, http.StatusInternalServerError, "")
return
}
if a.home != nil && !a.home.admitted.allows(id.Address) {
- ErrorPage(w, r, http.StatusNotFound, "")
+ a.h.errorPage(w, r, http.StatusNotFound, "")
return
}
p, err := a.store.EnsurePersonSetup(r.Context(), id.Address)
@@ -61,7 +61,7 @@ func (a *App) requirePerson(next http.Handler) http.Handler {
// request, and `carlos logs` is platform-held storage we
// cannot grep afterwards.
a.logger.Error("calendar: ensure person", "err", err)
- ErrorPage(w, r, http.StatusInternalServerError, "")
+ a.h.errorPage(w, r, http.StatusInternalServerError, "")
return
}
next.ServeHTTP(w, r.WithContext(context.WithValue(r.Context(), personCtxKey{}, p)))
diff --git a/internal/calendar/render.go b/internal/calendar/render.go
index f9b966c..da8593d 100644
--- a/internal/calendar/render.go
+++ b/internal/calendar/render.go
@@ -187,14 +187,28 @@ func render(w http.ResponseWriter, name string, data any) {
// Options.ErrorPage at, so a panic gets the same page a handled failure
// would.
func ErrorPage(w http.ResponseWriter, r *http.Request, status int, ref string) {
+ id, ok := viewer(r)
+ _, session := sessions.Current(r)
+ renderError(w, r, status, ref, id.Address, ok || session, "")
+}
+
+// errorPage is ErrorPage for a request the app is serving: the viewer is
+// resolved the way every other page resolves it (h.viewer, which knows a
+// Home session's person), so the error page carries the same chrome. A
+// live session whose person cannot be resolved still gets the account
+// menu, to sign out from, with no identity in it.
+func (h *handlers) errorPage(w http.ResponseWriter, r *http.Request, status int, ref string) {
+ id, ok := h.viewer(r)
+ _, session := sessions.Current(r)
+ if !ok {
+ id.Address = ""
+ }
+ renderError(w, r, status, ref, id.Address, ok || session, h.directory)
+}
+
+func renderError(w http.ResponseWriter, r *http.Request, status int, ref, address string, signed bool, directory string) {
mode := chrome.Appearance(r)
title, errPage := errorData(r, status, ref)
- signed, address := false, ""
- if id, ok := viewer(r); ok {
- signed, address = true, id.Address
- } else if _, ok := sessions.Current(r); ok {
- signed = true
- }
w.WriteHeader(status)
render(w, "errors", PageData{
Title: title,
@@ -203,7 +217,7 @@ func ErrorPage(w http.ResponseWriter, r *http.Request, status int, ref string) {
Path: r.URL.Path,
SignedIn: signed,
Address: address,
- Chrome: calendarChrome(r, signed, address, mode, title, ""),
+ Chrome: calendarChrome(r, signed, address, mode, title, directory),
Data: errPage,
})
}