feat(gmail): the OAuth client can be entered instead of deployed
The one click that opens Google's consent screen already existed. 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 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 using it. The registration is a one-off. Having to edit a Rune's manifest and recreate the container to paste the result was not. `VERDANDE_GMAIL_CLIENT_ID` and `_SECRET` still win when set. A value in the environment is a deliberate deployment decision, applied again on every recreate, and a form that could quietly override it would make the manifest a lie — so the page says where the value came from, and PUT answers 409. The redirect URI is spelled out on the page rather than left to be reconstructed. It is derived from VERDANDE_BASE_URL, it has to match Google's copy exactly, and the error Google returns when it does not names neither value. Its own table rather than `user_settings`: an OAuth client is the instance's, not one person's — and `user_settings` has `user_id` as its whole primary key, so a second scope there would silently overwrite the AI provider's key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kristian Wind
co-authored by
Claude Opus 5
Claude Opus 5
parent
101ca33069
commit
31bf0557df
@@ -10,12 +10,44 @@ import (
|
||||
"github.com/kristianwind/verdande/internal/store"
|
||||
)
|
||||
|
||||
func (s *Server) gmailConfig() gmail.Config {
|
||||
return gmail.Config{
|
||||
// gmailConfig is the OAuth client this instance authorises with.
|
||||
//
|
||||
// The environment wins when it is set. A value in the environment is a deliberate
|
||||
// deployment decision — it is in the Rune's manifest, under version control,
|
||||
// applied on every recreate — and a form that could quietly override it would make
|
||||
// the manifest a lie. The stored pair is for the ordinary case, where nobody wants
|
||||
// to redeploy a container to paste a client id.
|
||||
func (s *Server) gmailConfig(ctx context.Context) gmail.Config {
|
||||
cfg := gmail.Config{
|
||||
ClientID: s.cfg.GmailClientID,
|
||||
ClientSecret: s.cfg.GmailClientSecret,
|
||||
RedirectURL: s.cfg.GmailRedirectURL(),
|
||||
}
|
||||
if cfg.ClientID != "" && cfg.ClientSecret != "" {
|
||||
return cfg
|
||||
}
|
||||
|
||||
stored, err := s.db.InstanceSettings(ctx, "gmail")
|
||||
if err != nil {
|
||||
// Not fatal: an unconfigured Gmail and a database that could not answer
|
||||
// look the same from here, and the caller's next step is the same either
|
||||
// way — say it is not set up.
|
||||
return cfg
|
||||
}
|
||||
if id, _ := stored["client_id"].(string); cfg.ClientID == "" {
|
||||
cfg.ClientID = id
|
||||
}
|
||||
if secret, _ := stored["client_secret"].(string); cfg.ClientSecret == "" {
|
||||
cfg.ClientSecret = secret
|
||||
}
|
||||
return cfg
|
||||
}
|
||||
|
||||
// gmailFromEnv says whether the deployment set the client, which the settings page
|
||||
// needs so it can explain why the fields are not editable rather than accepting
|
||||
// changes that will never take effect.
|
||||
func (s *Server) gmailFromEnv() bool {
|
||||
return s.cfg.GmailClientID != "" && s.cfg.GmailClientSecret != ""
|
||||
}
|
||||
|
||||
// handleGmailAuthorize starts the flow: it mints a PKCE pair, remembers it against
|
||||
@@ -27,7 +59,7 @@ func (s *Server) gmailConfig() gmail.Config {
|
||||
// defaults.
|
||||
func (s *Server) handleGmailAuthorize(w http.ResponseWriter, r *http.Request) {
|
||||
user := userFrom(r.Context())
|
||||
cfg := s.gmailConfig()
|
||||
cfg := s.gmailConfig(r.Context())
|
||||
|
||||
if !cfg.Configured() {
|
||||
writeError(w, http.StatusConflict, CodeGmailNotConfigured,
|
||||
@@ -102,7 +134,7 @@ func (s *Server) handleGmailCallback(w http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
token, err := s.gmailConfig().Exchange(r.Context(), code, gmail.PKCE{Verifier: verifier})
|
||||
token, err := s.gmailConfig(r.Context()).Exchange(r.Context(), code, gmail.PKCE{Verifier: verifier})
|
||||
if err != nil {
|
||||
s.log.Error("gmail exchange", "err", err, "user", user.ID)
|
||||
http.Redirect(w, r, settingsURL+"?gmail=failed", http.StatusFound)
|
||||
@@ -185,7 +217,7 @@ func (s *Server) SyncGmail(ctx context.Context, user *store.User) (int, error) {
|
||||
access := str("access_token")
|
||||
expires, _ := settings["expires_at"].(float64)
|
||||
if access == "" || int64(expires) <= time.Now().Unix() {
|
||||
token, err := s.gmailConfig().Refresh(ctx, refresh)
|
||||
token, err := s.gmailConfig(ctx).Refresh(ctx, refresh)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
}
|
||||
@@ -300,3 +332,96 @@ func (s *Server) handleVersion(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
writeJSON(w, http.StatusOK, status)
|
||||
}
|
||||
|
||||
// --- the OAuth client itself ------------------------------------------------------
|
||||
|
||||
// The registration Google requires, enterable rather than deployed.
|
||||
//
|
||||
// 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
|
||||
// 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.
|
||||
//
|
||||
// Administrators only, and sessions only: this is the instance's identity to
|
||||
// Google, and a leaked API token must not be able to read or replace it.
|
||||
|
||||
type gmailClientJSON struct {
|
||||
ClientID string `json:"client_id"`
|
||||
// Secret is written and never read back, like the AI key: a settings page that
|
||||
// repopulates a password field is one that will eventually leak into a
|
||||
// screenshot.
|
||||
ClientSecret string `json:"client_secret,omitempty"`
|
||||
HasSecret bool `json:"has_secret"`
|
||||
// FromEnv says the deployment set this, in which case the stored pair is
|
||||
// ignored and the fields are not worth editing. Sent so the page can say why
|
||||
// rather than accepting changes that will never take effect.
|
||||
FromEnv bool `json:"from_env"`
|
||||
// RedirectURI is what has to be registered with Google, spelled out. It is
|
||||
// derived from VERDANDE_BASE_URL and must match exactly — it is the single
|
||||
// most likely thing to be wrong, and the error Google gives for it names
|
||||
// neither value.
|
||||
RedirectURI string `json:"redirect_uri"`
|
||||
}
|
||||
|
||||
func (s *Server) handleGetGmailClient(w http.ResponseWriter, r *http.Request) {
|
||||
stored, err := s.db.InstanceSettings(r.Context(), "gmail")
|
||||
if err != nil {
|
||||
s.internal(w, r, "gmail client", err)
|
||||
return
|
||||
}
|
||||
out := gmailClientJSON{
|
||||
FromEnv: s.gmailFromEnv(),
|
||||
RedirectURI: s.cfg.GmailRedirectURL(),
|
||||
}
|
||||
if out.FromEnv {
|
||||
out.ClientID = s.cfg.GmailClientID
|
||||
out.HasSecret = true
|
||||
} else {
|
||||
out.ClientID, _ = stored["client_id"].(string)
|
||||
secret, _ := stored["client_secret"].(string)
|
||||
out.HasSecret = secret != ""
|
||||
}
|
||||
writeJSON(w, http.StatusOK, out)
|
||||
}
|
||||
|
||||
func (s *Server) handleSetGmailClient(w http.ResponseWriter, r *http.Request) {
|
||||
var req gmailClientJSON
|
||||
if err := decodeJSON(w, r, &req); err != nil {
|
||||
return
|
||||
}
|
||||
if s.gmailFromEnv() {
|
||||
writeError(w, http.StatusConflict, CodeConflict,
|
||||
"the Gmail client is set in the environment and cannot be changed here")
|
||||
return
|
||||
}
|
||||
|
||||
stored, err := s.db.InstanceSettings(r.Context(), "gmail")
|
||||
if err != nil {
|
||||
s.internal(w, r, "gmail client", err)
|
||||
return
|
||||
}
|
||||
// An empty secret means "leave it alone", not "delete it" — otherwise
|
||||
// correcting a typo in the client id would silently clear the secret.
|
||||
secret, _ := stored["client_secret"].(string)
|
||||
if req.ClientSecret != "" {
|
||||
secret = strings.TrimSpace(req.ClientSecret)
|
||||
}
|
||||
id := strings.TrimSpace(req.ClientID)
|
||||
|
||||
if fields := map[string]string{}; id == "" && secret != "" {
|
||||
fields["client_id"] = "required"
|
||||
writeFieldErrors(w, fields)
|
||||
return
|
||||
}
|
||||
|
||||
if err := s.db.SetInstanceSettings(r.Context(), "gmail", map[string]any{
|
||||
"client_id": id, "client_secret": secret,
|
||||
}); err != nil {
|
||||
s.internal(w, r, "save gmail client", err)
|
||||
return
|
||||
}
|
||||
w.WriteHeader(http.StatusNoContent)
|
||||
}
|
||||
|
||||
@@ -1153,3 +1153,130 @@ func TestMCPCreateRefusesASecondLevelOfSubtask(t *testing.T) {
|
||||
t.Errorf("a sub-task of a sub-task was accepted: %v", response)
|
||||
}
|
||||
}
|
||||
|
||||
// The Gmail OAuth client, enterable rather than deployed.
|
||||
//
|
||||
// The registration itself cannot be avoided — Google issues no Gmail access to an
|
||||
// unregistered client — but having to edit a Rune's manifest and recreate the
|
||||
// container to paste the result can be, and that is what this covers.
|
||||
func TestTheGmailClientCanBeSetFromTheInterface(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
ts.bootstrap(t)
|
||||
|
||||
_, empty := ts.do(t, "GET", "/api/v1/gmail/client", nil)
|
||||
if empty["client_id"] != "" || empty["has_secret"] != false {
|
||||
t.Errorf("a fresh instance already has a client: %v", empty)
|
||||
}
|
||||
if empty["from_env"] != false {
|
||||
t.Errorf("from_env = %v with nothing in the environment", empty["from_env"])
|
||||
}
|
||||
// The single most likely thing to be wrong, so it is spelled out rather than
|
||||
// left for somebody to reconstruct.
|
||||
if empty["redirect_uri"] != "http://127.0.0.1/oauth/gmail/callback" {
|
||||
t.Errorf("redirect_uri = %v", empty["redirect_uri"])
|
||||
}
|
||||
|
||||
resp, _ := ts.do(t, "PUT", "/api/v1/gmail/client", map[string]any{
|
||||
"client_id": "1234.apps.googleusercontent.com", "client_secret": "hemmelig",
|
||||
})
|
||||
if resp.StatusCode != http.StatusNoContent {
|
||||
t.Fatalf("save: status %d", resp.StatusCode)
|
||||
}
|
||||
|
||||
_, saved := ts.do(t, "GET", "/api/v1/gmail/client", nil)
|
||||
if saved["client_id"] != "1234.apps.googleusercontent.com" {
|
||||
t.Errorf("client_id = %v", saved["client_id"])
|
||||
}
|
||||
if saved["has_secret"] != true {
|
||||
t.Error("has_secret is false after saving one")
|
||||
}
|
||||
// Never read back. A settings page that repopulates a password field is one
|
||||
// that will eventually leak into a screenshot.
|
||||
if _, present := saved["client_secret"]; present {
|
||||
t.Error("the secret came back in the response")
|
||||
}
|
||||
|
||||
// Saving only the id must not clear the secret — otherwise correcting a typo
|
||||
// silently breaks the connection.
|
||||
ts.do(t, "PUT", "/api/v1/gmail/client", map[string]any{
|
||||
"client_id": "5678.apps.googleusercontent.com",
|
||||
})
|
||||
_, again := ts.do(t, "GET", "/api/v1/gmail/client", nil)
|
||||
if again["client_id"] != "5678.apps.googleusercontent.com" || again["has_secret"] != true {
|
||||
t.Errorf("saving the id alone lost the secret: %v", again)
|
||||
}
|
||||
|
||||
// And the authorise call now has something to build a URL with, which is the
|
||||
// whole point: one click, and Google's consent screen.
|
||||
resp, body := ts.do(t, "POST", "/api/v1/gmail/authorize", nil)
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
t.Fatalf("authorize with a stored client: status %d, body %v", resp.StatusCode, body)
|
||||
}
|
||||
if url, _ := body["url"].(string); !strings.Contains(url, "5678.apps.googleusercontent.com") {
|
||||
t.Errorf("the consent URL does not carry the stored client: %v", body["url"])
|
||||
}
|
||||
}
|
||||
|
||||
// The environment wins, and says so. A value in the Rune's manifest is applied on
|
||||
// every recreate, so a form that could quietly override it would make the manifest
|
||||
// a lie.
|
||||
func TestTheEnvironmentsGmailClientCannotBeOverwritten(t *testing.T) {
|
||||
ts := newTestServerWith(t, func(c *config.Config) {
|
||||
c.GmailClientID = "fra-miljøet.apps.googleusercontent.com"
|
||||
c.GmailClientSecret = "også-fra-miljøet"
|
||||
})
|
||||
ts.bootstrap(t)
|
||||
|
||||
_, shown := ts.do(t, "GET", "/api/v1/gmail/client", nil)
|
||||
if shown["from_env"] != true {
|
||||
t.Errorf("from_env = %v with the environment set", shown["from_env"])
|
||||
}
|
||||
if shown["client_id"] != "fra-miljøet.apps.googleusercontent.com" {
|
||||
t.Errorf("client_id = %v", shown["client_id"])
|
||||
}
|
||||
|
||||
resp, _ := ts.do(t, "PUT", "/api/v1/gmail/client", map[string]any{
|
||||
"client_id": "min-egen", "client_secret": "min-egen",
|
||||
})
|
||||
if resp.StatusCode != http.StatusConflict {
|
||||
t.Fatalf("overwriting the environment: status %d, want 409", resp.StatusCode)
|
||||
}
|
||||
|
||||
_, after := ts.do(t, "GET", "/api/v1/gmail/client", nil)
|
||||
if after["client_id"] != "fra-miljøet.apps.googleusercontent.com" {
|
||||
t.Errorf("the environment's client was replaced: %v", after["client_id"])
|
||||
}
|
||||
}
|
||||
|
||||
// Administrators only, and sessions only: this is the instance's identity to
|
||||
// Google, and a leaked token must not be able to read or replace it.
|
||||
func TestTheGmailClientIsAdminsOnlyAndSessionsOnly(t *testing.T) {
|
||||
ts := newTestServer(t)
|
||||
ts.bootstrap(t)
|
||||
other := ts.newUser(t, "anden@example.dk", "Anden")
|
||||
|
||||
for _, c := range []struct {
|
||||
method string
|
||||
body any
|
||||
}{{"GET", nil}, {"PUT", map[string]any{"client_id": "x"}}} {
|
||||
if resp, _ := other.do(t, c.method, "/api/v1/gmail/client", c.body); resp.StatusCode != http.StatusForbidden {
|
||||
t.Errorf("%s as an ordinary user: status %d, want 403", c.method, resp.StatusCode)
|
||||
}
|
||||
}
|
||||
|
||||
admin, err := ts.db.UserByEmail(t.Context(), "kristian@example.dk")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
token := ts.apiToken(t, admin.ID)
|
||||
req, _ := http.NewRequest("GET", ts.URL+"/api/v1/gmail/client", nil)
|
||||
req.Header.Set("Authorization", "Bearer "+token)
|
||||
resp, err := (&http.Client{}).Do(req)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
resp.Body.Close()
|
||||
if resp.StatusCode != http.StatusForbidden {
|
||||
t.Errorf("with an admin's API token: status %d, want 403", resp.StatusCode)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user