diff --git a/docs/api.md b/docs/api.md index 56076d9..e5ed3f8 100644 --- a/docs/api.md +++ b/docs/api.md @@ -52,6 +52,10 @@ what a person reads is decided by the client, which knows what language they are | `forbidden` | 403 | Cross-site request, or admin needed. | | `not_found` | 404 | Does not exist, or you cannot see it. | | `conflict` | 409 | Already exists, or the state does not allow it. | +| `gmail_not_configured` | 409 | No Google OAuth client is registered on this server. | +| `ai_not_configured` | 409 | No AI provider has been set up. | +| `totp_not_enabled` | 409 | Two-factor is off, and the request needs it on. | +| `inbox_protected` | 409 | The Inbox cannot be deleted. | | `validation_failed` | 422 | Check `fields`. | | `rate_limited` | 429 | Too many attempts. | | `internal_error` | 500 | Our fault. | diff --git a/docs/openapi.yaml b/docs/openapi.yaml index dc738c6..5b46c37 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -1946,6 +1946,13 @@ components: type: object properties: address: { type: string, example: todo+abc123@example.dk } + configured: + type: boolean + description: | + Whether a mail server is set up at all. The address is real + either way, but with no mail server nothing on the internet + routes to it — and an interface that does not say so is + promising something that will not happen. Gmail: description: OK content: @@ -1995,6 +2002,10 @@ components: - rate_limited - internal_error - payload_too_large + - gmail_not_configured + - ai_not_configured + - totp_not_enabled + - inbox_protected error: { type: string } fields: type: object diff --git a/internal/httpapi/errorcodes_test.go b/internal/httpapi/errorcodes_test.go new file mode 100644 index 0000000..84434a3 --- /dev/null +++ b/internal/httpapi/errorcodes_test.go @@ -0,0 +1,149 @@ +package httpapi + +import ( + "os" + "path/filepath" + "regexp" + "sort" + "strings" + "testing" +) + +// The comment above the code list in errors.go says they are kept in one place +// "so the frontend's mapping can be checked against it rather than discovered +// from whatever a handler happened to write". Nothing checked it, and the +// frontend was quietly missing entries — which is not a cosmetic gap: a code +// with no message falls through to the server's English log prose, and a code +// mapped to the wrong sentence tells the person something untrue. Connecting +// Gmail without an OAuth client said "Det er der allerede." + +var ( + codeConst = regexp.MustCompile(`(?m)^\s*Code\w+\s*=\s*"([a-z_]+)"`) + messageKey = regexp.MustCompile(`(?m)^\s*([a-z_]+):\s*$|^\s*([a-z_]+):\s*'`) + messagesTop = "const MESSAGES = {" +) + +func TestEveryErrorCodeHasAMessageInTheFrontend(t *testing.T) { + codes := serverErrorCodes(t) + messages := frontendMessageKeys(t) + + var missing []string + for _, code := range codes { + if !messages[code] { + missing = append(missing, code) + } + } + sort.Strings(missing) + + for _, code := range missing { + t.Errorf("%q is returned by the API and has no message in web/src/lib/api.js — "+ + "a person would be shown the server's English log prose", code) + } +} + +func serverErrorCodes(t *testing.T) []string { + t.Helper() + + source, err := os.ReadFile("errors.go") + if err != nil { + t.Fatalf("read errors.go: %v", err) + } + var codes []string + for _, m := range codeConst.FindAllStringSubmatch(string(source), -1) { + codes = append(codes, m[1]) + } + if len(codes) == 0 { + t.Fatal("no codes were read out of errors.go — the scanner and the file disagree") + } + return codes +} + +// frontendMessageKeys reads the keys of the MESSAGES object. A small scanner +// rather than a JS parser, for the same reason the OpenAPI check uses one: it +// needs one thing out of a file this test also owns. +func frontendMessageKeys(t *testing.T) map[string]bool { + t.Helper() + + source, err := os.ReadFile(filepath.Join("..", "..", "web", "src", "lib", "api.js")) + if err != nil { + t.Fatalf("read api.js: %v", err) + } + body := string(source) + + start := strings.Index(body, messagesTop) + if start < 0 { + t.Fatalf("could not find %q in api.js", messagesTop) + } + body = body[start+len(messagesTop):] + end := strings.Index(body, "\n};") + if end < 0 { + t.Fatal("the MESSAGES object does not end where this scanner expects") + } + + keys := map[string]bool{} + for _, line := range strings.Split(body[:end], "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "//") { + continue + } + name, _, ok := strings.Cut(line, ":") + if ok { + keys[strings.TrimSpace(name)] = true + } + } + if len(keys) == 0 { + t.Fatal("no message keys were read out of api.js") + } + return keys +} + +// The four that used to be indistinguishable from "that already exists". +func TestTheMisleadingConflictsHaveTheirOwnCodes(t *testing.T) { + for _, code := range []string{ + CodeGmailNotConfigured, + CodeAINotConfigured, + CodeTOTPNotEnabled, + CodeInboxProtected, + } { + if code == CodeConflict { + t.Errorf("%q is still plain conflict", code) + } + } + + messages := frontendMessageKeys(t) + for _, code := range []string{"gmail_not_configured", "ai_not_configured", + "totp_not_enabled", "inbox_protected"} { + if !messages[code] { + t.Errorf("%q has no Danish message", code) + } + } +} + +// The Inbox is the one project that cannot be deleted, and saying "that already +// exists" about it helps nobody. +func TestDeletingTheInboxSaysWhy(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + _, projects := ts.do(t, "GET", "/api/v1/projects", nil) + list, _ := projects["projects"].([]any) + + var inboxID string + for _, raw := range list { + p := raw.(map[string]any) + if p["is_inbox"] == true { + inboxID = p["id"].(string) + } + } + if inboxID == "" { + t.Fatal("no inbox was found") + } + + resp, body := ts.do(t, "DELETE", "/api/v1/projects/"+inboxID, nil) + if resp.StatusCode != 409 { + t.Fatalf("status %d, want 409", resp.StatusCode) + } + if body["code"] != CodeInboxProtected { + t.Errorf("code = %v, want %q", body["code"], CodeInboxProtected) + } +} diff --git a/internal/httpapi/errors.go b/internal/httpapi/errors.go index 4b5817a..e174502 100644 --- a/internal/httpapi/errors.go +++ b/internal/httpapi/errors.go @@ -36,6 +36,17 @@ const ( CodeRateLimited = "rate_limited" CodeInternal = "internal_error" CodePayloadTooLarge = "payload_too_large" + + // Four situations that used to answer plain `conflict`, which the frontend + // renders as "that already exists". For these that is not merely vague, it is + // wrong: an unconfigured Gmail is not a Gmail that is already there, and a + // second factor that is off is not one that is on. A code exists so the + // interface can say the true thing — the server's own prose is English for a + // log, and deciding what a person reads is the frontend's job. + CodeGmailNotConfigured = "gmail_not_configured" + CodeAINotConfigured = "ai_not_configured" + CodeTOTPNotEnabled = "totp_not_enabled" + CodeInboxProtected = "inbox_protected" ) func writeError(w http.ResponseWriter, status int, code, message string) { diff --git a/internal/httpapi/gmail_handlers.go b/internal/httpapi/gmail_handlers.go index efda80c..36c6591 100644 --- a/internal/httpapi/gmail_handlers.go +++ b/internal/httpapi/gmail_handlers.go @@ -30,8 +30,8 @@ func (s *Server) handleGmailAuthorize(w http.ResponseWriter, r *http.Request) { cfg := s.gmailConfig() if !cfg.Configured() { - writeError(w, http.StatusConflict, CodeConflict, - "Gmail er ikke sat op på denne server — administratoren skal registrere en OAuth-klient hos Google") + writeError(w, http.StatusConflict, CodeGmailNotConfigured, + "no Google OAuth client is registered on this server") return } diff --git a/internal/httpapi/integrations_handlers.go b/internal/httpapi/integrations_handlers.go index 655897d..9cc7f41 100644 --- a/internal/httpapi/integrations_handlers.go +++ b/internal/httpapi/integrations_handlers.go @@ -18,6 +18,12 @@ import ( type mailAddressResponse struct { Address string `json:"address"` + // Configured says whether a mail server has been set up at all. Without one + // the address is still minted and still valid — the token is real, and the + // endpoint that receives mail works — but nothing on the internet routes to + // it yet, and an interface that shows the address without saying so is + // promising something that will not happen. + Configured bool `json:"configured"` } // handleGetMailAddress returns the personal address that turns an email into a @@ -34,7 +40,10 @@ func (s *Server) handleGetMailAddress(w http.ResponseWriter, r *http.Request) { s.internal(w, "mail token", err) return } - writeJSON(w, http.StatusOK, mailAddressResponse{Address: s.mailAddress(token)}) + writeJSON(w, http.StatusOK, mailAddressResponse{ + Address: s.mailAddress(token), + Configured: s.cfg.SMTP.Host != "", + }) } func (s *Server) handleRotateMailAddress(w http.ResponseWriter, r *http.Request) { @@ -47,7 +56,10 @@ func (s *Server) handleRotateMailAddress(w http.ResponseWriter, r *http.Request) s.internal(w, "set mail token", err) return } - writeJSON(w, http.StatusOK, mailAddressResponse{Address: s.mailAddress(token)}) + writeJSON(w, http.StatusOK, mailAddressResponse{ + Address: s.mailAddress(token), + Configured: s.cfg.SMTP.Host != "", + }) } func (s *Server) mailAddress(token string) string { @@ -55,7 +67,15 @@ func (s *Server) mailAddress(token string) string { if from := s.cfg.SMTP.From; strings.Contains(from, "@") { // The mail server's own domain, which is the one that can actually route // this — the web address may well be a tunnel hostname that receives no mail. - domain = from[strings.Index(from, "@")+1:] + // + // Except when it is the default. VERDANDE_SMTP_FROM falls back to + // verdande@localhost, so an instance with no mail server at all was + // handing out todo+…@localhost: an address that looks real, cannot + // possibly receive anything, and gives no hint which of the two it is. + // The public hostname is at least a domain that exists. + if d := from[strings.Index(from, "@")+1:]; d != "" && d != "localhost" { + domain = d + } } return "todo+" + token + "@" + domain } @@ -251,8 +271,8 @@ func (s *Server) handleAISplit(w http.ResponseWriter, r *http.Request) { } if !cfg.Configured() { // Not an error the user did anything to cause: the feature is simply off. - writeError(w, http.StatusConflict, CodeConflict, - "der er ikke sat en AI-udbyder op under indstillinger") + writeError(w, http.StatusConflict, CodeAINotConfigured, + "no AI provider is configured") return } @@ -299,8 +319,8 @@ func (s *Server) handleAISummary(w http.ResponseWriter, r *http.Request) { return } if !cfg.Configured() { - writeError(w, http.StatusConflict, CodeConflict, - "der er ikke sat en AI-udbyder op under indstillinger") + writeError(w, http.StatusConflict, CodeAINotConfigured, + "no AI provider is configured") return } diff --git a/internal/httpapi/integrations_test.go b/internal/httpapi/integrations_test.go index c3a663d..939b0eb 100644 --- a/internal/httpapi/integrations_test.go +++ b/internal/httpapi/integrations_test.go @@ -880,3 +880,52 @@ func TestMCPWithKeyIgnoresTheSessionCookie(t *testing.T) { resp.StatusCode) } } + +// --- mail to task ------------------------------------------------------------------ + +// The address used to come out as todo+…@localhost on any instance without a +// mail server, because VERDANDE_SMTP_FROM defaults to verdande@localhost and the +// domain was taken from it unconditionally. That address looks real, cannot +// receive anything, and gives no hint which of the two it is. +func TestMailAddressDoesNotClaimToBeLocalhost(t *testing.T) { + ts := newTestServerWith(t, func(c *config.Config) { + c.BaseURL = "https://todo.example.dk" + // Left at the default, as an instance with no mail server has it. + c.SMTP.From = "verdande@localhost" + }) + ts.bootstrap(t) + + _, body := ts.do(t, "GET", "/api/v1/mail-address", nil) + address, _ := body["address"].(string) + + if strings.HasSuffix(address, "@localhost") { + t.Errorf("address = %q — that can never receive mail", address) + } + if !strings.HasSuffix(address, "@todo.example.dk") { + t.Errorf("address = %q, want the public hostname", address) + } + if body["configured"] != false { + t.Error("no SMTP host is set, so configured should be false") + } +} + +// A real sender address still wins: the mail server's own domain is the one that +// can actually route this, and the web address may be a tunnel hostname. +func TestMailAddressPrefersTheConfiguredSender(t *testing.T) { + ts := newTestServerWith(t, func(c *config.Config) { + c.BaseURL = "https://tunnel-abc123.example.net" + c.SMTP.Host = "mail.firma.dk" + c.SMTP.From = "verdande@firma.dk" + }) + ts.bootstrap(t) + + _, body := ts.do(t, "GET", "/api/v1/mail-address", nil) + address, _ := body["address"].(string) + + if !strings.HasSuffix(address, "@firma.dk") { + t.Errorf("address = %q, want the sender's domain", address) + } + if body["configured"] != true { + t.Error("an SMTP host is set, so configured should be true") + } +} diff --git a/internal/httpapi/projects_handlers.go b/internal/httpapi/projects_handlers.go index 79c8c7a..695723d 100644 --- a/internal/httpapi/projects_handlers.go +++ b/internal/httpapi/projects_handlers.go @@ -141,7 +141,7 @@ func (s *Server) handleDeleteProject(w http.ResponseWriter, r *http.Request) { projectID := chi.URLParam(r, "projectID") if err := s.db.DeleteProject(r.Context(), projectID); err != nil { if strings.Contains(err.Error(), "Inbox cannot be deleted") { - writeError(w, http.StatusConflict, CodeConflict, "the Inbox cannot be deleted") + writeError(w, http.StatusConflict, CodeInboxProtected, "the Inbox cannot be deleted") return } s.storeError(w, "delete project", err) diff --git a/internal/httpapi/totp_handlers.go b/internal/httpapi/totp_handlers.go index 36d6d66..853d307 100644 --- a/internal/httpapi/totp_handlers.go +++ b/internal/httpapi/totp_handlers.go @@ -116,7 +116,7 @@ func (s *Server) handleRecoveryCodesRegenerate(w http.ResponseWriter, r *http.Re user := userFrom(r.Context()) if !user.TOTPEnabled { - writeError(w, http.StatusConflict, CodeConflict, + writeError(w, http.StatusConflict, CodeTOTPNotEnabled, "two-factor authentication is not switched on") return } diff --git a/web/src/lib/api.js b/web/src/lib/api.js index cea5fd9..2a9b62c 100644 --- a/web/src/lib/api.js +++ b/web/src/lib/api.js @@ -34,9 +34,29 @@ const MESSAGES = { validation_failed: 'Tjek felterne herunder.', payload_too_large: 'Det er for stort.', internal_error: 'Noget gik galt. Prøv igen.', - offline: 'Ingen forbindelse til serveren.' + bad_request: 'Anmodningen kunne ikke læses.', + // Client-only: a network failure is not an HTTP status, so the server has no + // code for it. + offline: 'Ingen forbindelse til serveren.', + + // Situations that used to arrive as a plain `conflict` and were therefore + // shown as "Det er der allerede." — which was not merely vague but wrong. An + // unconfigured Gmail is not a Gmail that is already connected. + gmail_not_configured: + 'Gmail er ikke sat op på denne server. Administratoren skal registrere en OAuth-klient hos Google og sætte VERDANDE_GMAIL_CLIENT_ID og _SECRET.', + ai_not_configured: 'Der er ikke valgt en AI-udbyder. Sæt den op under Indstillinger → AI.', + totp_not_enabled: 'To-faktor er ikke slået til.', + inbox_protected: 'Indbakken kan ikke slettes.' }; +/** + * What to show a person for a failed request. + * + * A code with no message here falls back to the server's prose, which is English + * and written for a log — not good, but better than swallowing the only + * explanation there is. A code that turns up in that fallback is a code this + * table is missing. + */ export function humanMessage(err) { if (!(err instanceof ApiError)) return MESSAGES.offline; return MESSAGES[err.code] ?? err.message ?? MESSAGES.internal_error; diff --git a/web/src/routes/indstillinger/integrationer/+page.svelte b/web/src/routes/indstillinger/integrationer/+page.svelte index ce6cf45..8fc8b42 100644 --- a/web/src/routes/indstillinger/integrationer/+page.svelte +++ b/web/src/routes/indstillinger/integrationer/+page.svelte @@ -96,10 +96,18 @@ let feedURL = $state(''); let mailAddress = $state(''); + /** Null until loaded, so the warning does not flash before the answer is in. */ + let mailConfigured = $state(null); $effect(() => { api.feed().then((r) => (feedURL = r.url)).catch(() => {}); - api.getMailAddress().then((r) => (mailAddress = r.address)).catch(() => {}); + api + .getMailAddress() + .then((r) => { + mailAddress = r.address; + mailConfigured = r.configured; + }) + .catch(() => {}); }); async function rotateFeed() { @@ -114,7 +122,9 @@ async function rotateMail() { if (!confirm('Ny adresse? Mail til den gamle bliver afvist.')) return; try { - mailAddress = (await api.rotateMailAddress()).address; + const r = await api.rotateMailAddress(); + mailAddress = r.address; + mailConfigured = r.configured; } catch (e) { app.toast(humanMessage(e)); } @@ -218,6 +228,16 @@
+ {#if mailConfigured === false} ++ Der er ingen mailserver sat op på denne server, så adressen herunder kan + ikke modtage noget endnu. Sæt VERDANDE_SMTP_HOST og + de øvrige SMTP-indstillinger, og få mailserveren til at aflevere post til + todo+* videre til + /inbound/mail. +
+ {/if} +