diff --git a/README.md b/README.md index f69c647..02099d3 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,7 @@ Apple Reminders, forward mail into it, and add it to Claude as a connector. | CalDAV server (two-way VTODO) | Done, tested | | Mail-to-task | Done, tested | | AI layer (Anthropic/OpenAI/Google/local) | Built, degrades when unconfigured | -| Gmail: OAuth2 with PKCE, polling, one-way to tasks | Done, tested | +| Gmail: OAuth2 with PKCE, polling to tasks, unstars what it imports | Done, tested | | Google Calendar: read-only, laid over the task calendar | Built, never run against Google | | Calendar subscriptions: ICS and webcal, several per person | Done, tested | | Times on tasks, set by dragging in the week grid | Done, tested | diff --git a/docs/configuration.md b/docs/configuration.md index d6d3e92..814870b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -74,7 +74,7 @@ Then enable the APIs and scopes for whichever you want: | For | Enable | Scope | |---|---|---| -| Mailboxes | Gmail API | `gmail.readonly` | +| Mailboxes | Gmail API | `gmail.modify` | | Calendar | Google Calendar API | `calendar.readonly` | | Variable | What it does | diff --git a/docs/openapi.yaml b/docs/openapi.yaml index c424feb..cc33d7b 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -1250,7 +1250,7 @@ paths: The one click that opens Google's consent screen already exists. What it needs behind it is a registered client, and no app can conjure one: Google - issues no Gmail access to an unregistered client, and `gmail.readonly` is a + issues no Gmail access to an unregistered client, and `gmail.modify` is a restricted scope, so an id shipped inside a public image would be both extractable from it and unusable without Google's security review of every deployment that used it. diff --git a/internal/gmail/gmail.go b/internal/gmail/gmail.go index f112dfc..82e0a89 100644 --- a/internal/gmail/gmail.go +++ b/internal/gmail/gmail.go @@ -1,14 +1,20 @@ // Package gmail is the small slice of the Gmail API verdande needs, and the scope // it asks for. // -// The connection is one-way on purpose: a starred or labelled message becomes a -// task, and nothing verdande does afterwards touches the mailbox. Unstarring the -// message does not delete the task, completing the task does not unstar the -// message. Two-way would mean deciding what to do when both sides changed, and the -// only honest answer to that is a synchronisation model far bigger than the feature. +// The one thing verdande writes back is the star. A starred message becomes a +// task, and once it has, the star comes off — exactly as the IMAP side takes the +// flag off a mail it has imported, and for the same reason: the star meant "turn +// this into a task", the task now exists, so the star is no longer true. Nothing +// else is touched. Completing the task does not reach back to the mail; the mail +// is a pointer the task was made from, not a copy the two have to be kept in step. // -// Scope is gmail.readonly. verdande never needs to send, modify or delete, and -// asking for less is what makes the consent screen something a person can agree to. +// That one write is why the scope is gmail.modify rather than gmail.readonly. +// Removing the STARRED label is a modify, and Gmail offers no narrower scope that +// can do it — so the consent screen asks for more than it used to, and the trade +// is deliberate: a starred inbox that empties itself as the tasks are made, rather +// than one that fills with stars nothing ever clears. Still not send, still not +// delete — modify is labels, and the only label verdande removes is the one that +// triggered the task. // // The OAuth flow itself moved to internal/google when Calendar needed the same one. // The names below are aliases so that every call site — and this package's own @@ -27,8 +33,10 @@ import ( "github.com/kristianwind/verdande/internal/google" ) -// Read-only, and only mail. Not profile, not contacts, not send. -const Scope = "https://www.googleapis.com/auth/gmail.readonly" +// Read mail and change its labels, and only mail. Not profile, not contacts, not +// send, not delete. The single write is removing the STARRED label from a message +// that has become a task; see the package comment for why that costs modify. +const Scope = "https://www.googleapis.com/auth/gmail.modify" const defaultAPIBase = "https://gmail.googleapis.com/gmail/v1" @@ -169,6 +177,25 @@ func (c *Client) get(ctx context.Context, url string, out any) error { return google.Get(ctx, c.http, c.accessToken, url, out) } +// Unstar takes the star off the given messages — one call for all of them, the way +// the IMAP side unflags a batch, so a sweep that made ten tasks is one write and +// not ten. +// +// STARRED is a system label whose id is its name, so no lookup is needed; a custom +// label would need one, and verdande removes none. batchModify answers 204 with no +// body on success. An empty list is a no-op, not a call: batchModify with no ids is +// an error on Google's side and nothing worth making a round trip to hear. +func (c *Client) Unstar(ctx context.Context, ids []string) error { + if len(ids) == 0 { + return nil + } + body := struct { + IDs []string `json:"ids"` + RemoveLabelIDs []string `json:"removeLabelIds"` + }{IDs: ids, RemoveLabelIDs: []string{"STARRED"}} + return google.Post(ctx, c.http, c.accessToken, c.api()+"/users/me/messages/batchModify", body, nil) +} + // SenderName pulls "Anders Jensen" out of "Anders Jensen ", // falling back to the address when there is no display name. func SenderName(from string) string { diff --git a/internal/gmail/gmail_test.go b/internal/gmail/gmail_test.go index b1d53bf..54b4333 100644 --- a/internal/gmail/gmail_test.go +++ b/internal/gmail/gmail_test.go @@ -1,16 +1,81 @@ package gmail import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" "strings" "testing" ) // The scope is the one thing verdande chooses about the connection, and it is the // one the person reads on the consent screen. The flow itself is exercised in -// internal/google; what belongs here is that Gmail still asks for read-only mail. -func TestTheScopeIsReadOnlyMail(t *testing.T) { - if !strings.HasSuffix(Scope, "gmail.readonly") { - t.Errorf("scope = %q; verdande never needs to send or modify", Scope) +// internal/google; what belongs here is that Gmail asks for modify — read plus the +// one write it makes, taking the star off an imported message — and nothing wider. +func TestTheScopeIsModifyMail(t *testing.T) { + if !strings.HasSuffix(Scope, "gmail.modify") { + t.Errorf("scope = %q; want gmail.modify — read, plus removing the star", Scope) + } + // Modify is the ceiling. Send, or the full account, would be a different promise + // to the person on the consent screen than the one this feature needs. + for _, wider := range []string{"gmail.send", "mail.google.com", "gmail.settings"} { + if strings.Contains(Scope, wider) { + t.Errorf("scope = %q; must not include %q", Scope, wider) + } + } +} + +// Unstar removes the star from a batch in one call, and it addresses the messages +// it was given and only the STARRED label. +func TestUnstarRemovesOnlyTheStar(t *testing.T) { + var gotPath string + var gotBody struct { + IDs []string `json:"ids"` + AddLabelIDs []string `json:"addLabelIds"` + RemoveLabelIDs []string `json:"removeLabelIds"` + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + raw, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(raw, &gotBody) + w.WriteHeader(http.StatusNoContent) // what batchModify answers on success + })) + defer srv.Close() + + err := NewClient("token").At(srv.URL).Unstar(context.Background(), []string{"a", "b"}) + if err != nil { + t.Fatalf("Unstar: %v", err) + } + if !strings.HasSuffix(gotPath, "/users/me/messages/batchModify") { + t.Errorf("path = %q, want the batchModify endpoint", gotPath) + } + if strings.Join(gotBody.IDs, ",") != "a,b" { + t.Errorf("ids = %v, want [a b]", gotBody.IDs) + } + if strings.Join(gotBody.RemoveLabelIDs, ",") != "STARRED" { + t.Errorf("removeLabelIds = %v, want [STARRED]", gotBody.RemoveLabelIDs) + } + if len(gotBody.AddLabelIDs) != 0 { + t.Errorf("addLabelIds = %v, want none — verdande adds no label", gotBody.AddLabelIDs) + } +} + +// An empty batch is not a request: Gmail rejects a batchModify with no ids, and +// there is nothing to say to it anyway. +func TestUnstarOfNothingMakesNoCall(t *testing.T) { + called := false + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + called = true + })) + defer srv.Close() + + if err := NewClient("token").At(srv.URL).Unstar(context.Background(), nil); err != nil { + t.Fatalf("Unstar(nil): %v", err) + } + if called { + t.Error("Unstar(nil) made a request; it should make none") } } diff --git a/internal/google/google.go b/internal/google/google.go index af8dabe..3a12da1 100644 --- a/internal/google/google.go +++ b/internal/google/google.go @@ -215,6 +215,40 @@ func Get(ctx context.Context, client *http.Client, accessToken, url string, out return json.NewDecoder(resp.Body).Decode(out) } +// Post sends a JSON body to a Google API and, unlike Get, does not insist on a +// body coming back. A write call — modifying labels on a message — answers with +// 204 No Content when it worked, so anything 2xx is success and only the body is +// decoded, and only when the caller asked for one. +func Post(ctx context.Context, client *http.Client, accessToken, url string, body, out any) error { + payload, err := json.Marshal(body) + if err != nil { + return err + } + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(string(payload))) + if err != nil { + return err + } + req.Header.Set("Authorization", "Bearer "+accessToken) + req.Header.Set("Content-Type", "application/json") + + resp, err := client.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if resp.StatusCode == http.StatusUnauthorized { + return ErrUnauthorized + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return fmt.Errorf("google: %s", resp.Status) + } + if out == nil || resp.StatusCode == http.StatusNoContent { + return nil + } + return json.NewDecoder(resp.Body).Decode(out) +} + func randomString(n int) (string, error) { b := make([]byte, n) if _, err := rand.Read(b); err != nil { diff --git a/internal/httpapi/gmail_handlers.go b/internal/httpapi/gmail_handlers.go index cd7cd48..40bbc4d 100644 --- a/internal/httpapi/gmail_handlers.go +++ b/internal/httpapi/gmail_handlers.go @@ -251,10 +251,12 @@ func (s *Server) handleGmailSyncNow(w http.ResponseWriter, r *http.Request) { // takes a context and a user rather than a request. // maxGmailSeen bounds the list of message ids already turned into tasks. // -// Gmail is read-only here — the scope is `gmail.readonly`, deliberately, because -// asking for less is what makes the consent screen something a person can agree -// to — so verdande cannot unstar a message the way the IMAP side unflags one. The -// record has to live on this side, and a record on this side has to be bounded. +// The star is taken off an imported message now, so on a starred trigger the mail +// leaves the query on its own and this list is a safety net rather than the only +// record. It still earns its place: it covers the window between making the task +// and the star coming off, and it is the whole of the dedup for a label trigger, +// where the label is the person's and not ours to remove. A record on this side +// has to be bounded. // // A count rather than the previous rule of "whatever Gmail returned this run", // which was not a bound but a way of forgetting. @@ -381,6 +383,29 @@ func (s *Server) SyncGmail(ctx context.Context, user *store.User) (int, error) { s.hub.Publish(projectID, "task.created", toTaskJSON(*task)) } + // The star comes off every message this query returned that now has a task — + // the ones made this run and the ones a run before it already made. Gated to the + // triggers that are about the star: for a label trigger the star is the person's + // own and not ours to touch, and dedup there rests on the seen list alone. + // + // Best effort, exactly like the IMAP unflag: a task made is not undone by a star + // that would not come off. The likeliest reason it will not, right after this + // ships, is a connection still holding the old read-only grant — the write is a + // 403 until the account is reconnected, and this logs it rather than failing the + // sync. It self-heals: the mail stays starred, stays in the next query, and is + // tried again once the grant is there. + if box.Trigger == "starred" || box.Trigger == "both" { + unstar := make([]string, 0, len(ids)) + for _, id := range ids { + if seen[id] { + unstar = append(unstar, id) + } + } + if err := client.Unstar(ctx, unstar); err != nil { + s.log.Warn("gmail unstar", "err", err, "user", user.ID, "count", len(unstar)) + } + } + if created > 0 { // The ids this run saw, on top of the ones already known. // @@ -442,8 +467,8 @@ func (s *Server) handleVersion(w http.ResponseWriter, r *http.Request) { // // The one click that opens Google's consent screen already exists; what it needs // behind it is a registered client, and that is not something any app can conjure: -// Google issues no Gmail access to an unregistered client, and `gmail.readonly` is -// a restricted scope, so an id shipped inside a public image would be both +// Google issues no Gmail access to an unregistered client, and `gmail.modify` is a +// restricted scope, so an id shipped inside a public image would be both // extractable from it and unusable without Google's security review of every // deployment. The registration is a one-off. Having to edit a Rune's manifest and // recreate the container to paste the result was not. diff --git a/internal/httpapi/gmail_test.go b/internal/httpapi/gmail_test.go index f49f51a..1c7adba 100644 --- a/internal/httpapi/gmail_test.go +++ b/internal/httpapi/gmail_test.go @@ -1,14 +1,17 @@ package httpapi import ( - "github.com/kristianwind/verdande/internal/store" + "encoding/json" + "io" "net/http" "net/http/httptest" "net/url" + "strings" "testing" "time" "github.com/kristianwind/verdande/internal/config" + "github.com/kristianwind/verdande/internal/store" ) // A Gmail failure has to survive being reported. @@ -150,6 +153,84 @@ func TestASlowMailboxDoesNotHangTheRequest(t *testing.T) { t.Logf("came back in %s with %d", took.Round(time.Millisecond), resp.StatusCode) } +// A message that becomes a task has its star taken off — the write that the whole +// gmail.modify scope is for. The mail's own inbox empties as the tasks are made, +// the way the IMAP side unflags what it imports. +func TestASyncedGmailMessageIsUnstarred(t *testing.T) { + var unstarred struct { + hit bool + body struct { + IDs []string `json:"ids"` + RemoveLabelIDs []string `json:"removeLabelIds"` + } + } + google := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == "POST" && strings.HasSuffix(r.URL.Path, "/messages/batchModify"): + unstarred.hit = true + raw, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(raw, &unstarred.body) + w.WriteHeader(http.StatusNoContent) + case strings.Contains(r.URL.Path, "/messages/") && r.URL.Query().Get("format") == "metadata": + writeTestJSON(w, map[string]any{ + "id": "m1", "threadId": "t1", "snippet": "kort tekst", + "payload": map[string]any{"headers": []map[string]string{ + {"name": "From", "value": "Anders "}, + {"name": "Subject", "value": "Møde på fredag"}, + }}, + }) + case strings.HasSuffix(r.URL.Path, "/messages"): + writeTestJSON(w, map[string]any{"messages": []map[string]string{{"id": "m1"}}}) + default: + http.Error(w, "unexpected "+r.Method+" "+r.URL.Path, http.StatusNotFound) + } + })) + defer google.Close() + + ts := newTestServerWith(t, func(cfg *config.Config) { + cfg.GmailClientID = "test-client" + cfg.GmailClientSecret = "test-secret" + cfg.GoogleTokenURL = google.URL + "/token" + cfg.GmailAPIURL = google.URL + }) + ts.bootstrap(t) + + user, err := ts.db.UserByEmail(t.Context(), "kristian@example.dk") + if err != nil { + t.Fatal(err) + } + // A live access token, so the run goes straight to the list without a refresh. + if err := ts.db.SaveMailbox(t.Context(), &store.Mailbox{ + UserID: user.ID, Kind: "gmail", RefreshToken: "r", AccessToken: "a", + Trigger: "starred", ExpiresAt: time.Now().Add(time.Hour), + }); err != nil { + t.Fatal(err) + } + + resp, body := ts.do(t, "POST", "/api/v1/gmail/sync", nil) + if resp.StatusCode != http.StatusOK { + t.Fatalf("sync: status %d, want 200", resp.StatusCode) + } + if created, _ := body["created"].(float64); created != 1 { + t.Fatalf("created = %v, want 1", body["created"]) + } + + if !unstarred.hit { + t.Fatal("the message was never unstarred; its star is the one thing modify is for") + } + if strings.Join(unstarred.body.IDs, ",") != "m1" { + t.Errorf("unstarred ids = %v, want [m1]", unstarred.body.IDs) + } + if strings.Join(unstarred.body.RemoveLabelIDs, ",") != "STARRED" { + t.Errorf("removed = %v, want [STARRED]", unstarred.body.RemoveLabelIDs) + } +} + +func writeTestJSON(w http.ResponseWriter, v any) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(v) +} + func mustParse(t *testing.T, raw string) *url.URL { t.Helper() u, err := url.Parse(raw) diff --git a/web/src/lib/locales/da.js b/web/src/lib/locales/da.js index 70163c7..ffd45ec 100644 --- a/web/src/lib/locales/da.js +++ b/web/src/lib/locales/da.js @@ -660,7 +660,7 @@ export const da = { // --- settings: integrations -------------------------------------------------------- 'int.mailboxes': 'Postkasser', 'int.mailboxesHint': - 'Flag en mail i iCloud, Fastmail eller enhver anden udbyder, og den bliver en opgave. Én vej: fjerner du flaget, sker der ikke noget med opgaven. Office 365 kan ikke bruges her — Microsoft har lukket for kodeord på IMAP.', + 'Flag en mail i iCloud, Fastmail eller enhver anden udbyder, og den bliver en opgave — flaget fjernes, når opgaven er oprettet. Kun én vej: gør du selv noget ved mailen bagefter, sker der ikke noget med opgaven. Office 365 kan ikke bruges her — Microsoft har lukket for kodeord på IMAP.', 'int.addMailbox': 'Tilføj en postkasse', 'int.host': 'IMAP-server', 'int.username': 'Brugernavn', @@ -676,7 +676,7 @@ export const da = { 'int.fetching': 'Henter …', 'int.gmail': 'Gmail', 'int.gmailHint': - 'Stjernemarkér en mail, eller giv den en etiket, og den bliver en opgave. Envejs: at fjerne stjernen gør ikke noget ved opgaven.', + 'Stjernemarkér en mail, eller giv den en etiket, og den bliver en opgave — stjernen fjernes så (en etiket bliver, hvor den er). Det du selv gør ved mailen bagefter, ændrer ikke opgaven.', 'int.connectGmail': 'Forbind Gmail', 'int.fromEnv': 'Klienten er sat i miljøet (VERDANDE_GMAIL_CLIENT_ID), så den kan ikke ændres her. Det er med vilje: en værdi i runens manifest er en beslutning, der bliver anvendt igen ved hver genstart, og en formular, der kunne overskrive den, ville gøre manifestet til en løgn.', diff --git a/web/src/lib/locales/en.js b/web/src/lib/locales/en.js index 5501ecf..5a18bb3 100644 --- a/web/src/lib/locales/en.js +++ b/web/src/lib/locales/en.js @@ -656,7 +656,7 @@ export const en = { // --- settings: integrations -------------------------------------------------------- 'int.mailboxes': 'Mailboxes', 'int.mailboxesHint': - 'Flag an email in iCloud, Fastmail or any other provider and it becomes a task. One way: removing the flag does nothing to the task. Office 365 cannot be used here — Microsoft has closed IMAP to passwords.', + 'Flag an email in iCloud, Fastmail or any other provider and it becomes a task — the flag comes off once it has. One way only: what you do to the mail afterwards does not change the task. Office 365 cannot be used here — Microsoft has closed IMAP to passwords.', 'int.addMailbox': 'Add a mailbox', 'int.host': 'IMAP server', 'int.username': 'Username', @@ -672,7 +672,7 @@ export const en = { 'int.fetching': 'Fetching …', 'int.gmail': 'Gmail', 'int.gmailHint': - 'Star an email, or give it a label, and it becomes a task. One way: removing the star does nothing to the task.', + 'Star an email, or give it a label, and it becomes a task — the star then comes off (a label stays where it is). What you do to the mail afterwards does not change the task.', 'int.connectGmail': 'Connect Gmail', 'int.fromEnv': 'The client is set in the environment (VERDANDE_GMAIL_CLIENT_ID), so it cannot be changed here. That is deliberate: a value in the Rune’s manifest is a decision reapplied on every restart, and a form that could overwrite it would make the manifest a lie.', diff --git a/web/src/routes/indstillinger/integrationer/+page.svelte b/web/src/routes/indstillinger/integrationer/+page.svelte index d82f2f9..0b296b4 100644 --- a/web/src/routes/indstillinger/integrationer/+page.svelte +++ b/web/src/routes/indstillinger/integrationer/+page.svelte @@ -423,7 +423,7 @@ {#if app.user?.is_admin && client} {#if client.from_env}