diff --git a/base/oauth.go b/base/oauth.go index a61ad24b..b640bcfc 100644 --- a/base/oauth.go +++ b/base/oauth.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "strings" + "sync" "time" "github.com/keybase/go-keybase-chat-bot/kbchat" @@ -22,17 +23,26 @@ func (e OAuthRequiredError) Error() string { return "OAuth is required for this, permission requested." } -// ShouldRetryAuth checks if an error indicates OAuth credentials have failed -// and should be deleted to trigger re-authentication. This consolidates the -// retry logic used across meetbot, zoombot, and gcalbot. +// ShouldRetryAuth reports whether err means the user's OAuth credentials are +// permanently unusable and should be deleted. Transient token-fetch failures +// (network, 5xx) are not treated as credential errors. func ShouldRetryAuth(err error) bool { if err == nil { return false } - errMsg := err.Error() - return strings.Contains(errMsg, "cannot fetch token") || - strings.Contains(errMsg, "invalid_grant") || - strings.Contains(errMsg, "token expired and refresh token is not set") + var retr *oauth2.RetrieveError + if errors.As(err, &retr) { + switch strings.ToLower(retr.ErrorCode) { + case "invalid_grant", "invalid_token": + return true + } + body := string(retr.Body) + return strings.Contains(body, "invalid_grant") || + strings.Contains(body, "invalid_token") + } + msg := err.Error() + return strings.Contains(msg, "invalid_grant") || + strings.Contains(msg, "token expired and refresh token is not set") } type OAuthStorage interface { @@ -287,18 +297,58 @@ func GetOAuthClient( return nil, OAuthRequiredError{} } - // renew token - if token.Expiry.Before(time.Now()) { - newToken, err := config.TokenSource(ctx, token).Token() - if err != nil { - return nil, fmt.Errorf("unable to renew token: %s", err) - } - err = storage.PutToken(ctx, tokenIdentifier, newToken) - if err != nil { - return nil, fmt.Errorf("unable to update token: %s", err) - } - token = newToken + + src := PersistTokenSource(ctx, token, ConfigTokenSource(ctx, config, token), func(ctx context.Context, tok *oauth2.Token) error { + return storage.PutToken(ctx, tokenIdentifier, tok) + }) + if _, err := src.Token(); err != nil { + return nil, fmt.Errorf("unable to renew token: %w", err) + } + return oauth2.NewClient(ctx, src), nil +} + +// ConfigTokenSource is config.TokenSource, except tokens with a zero Expiry +// and a refresh token are treated as expired. oauth2.Token.Valid treats a +// zero Expiry as never-expired, which would skip refresh forever. +func ConfigTokenSource(ctx context.Context, config *oauth2.Config, token *oauth2.Token) oauth2.TokenSource { + if token != nil && token.Expiry.IsZero() && token.RefreshToken != "" { + cp := *token + cp.Expiry = time.Now().Add(-time.Minute) + token = &cp } + return config.TokenSource(ctx, token) +} + +// PersistTokenSource wraps src and writes the token whenever AccessToken, +// RefreshToken, or Expiry changes (including refresh-token rotation). +func PersistTokenSource(ctx context.Context, token *oauth2.Token, src oauth2.TokenSource, put func(context.Context, *oauth2.Token) error) oauth2.TokenSource { + // Token() is invoked on later HTTP refreshes; the caller context may + // already be done by then, so persist independently of it. + return &persistTokenSource{ctx: context.WithoutCancel(ctx), token: token, src: src, put: put} +} + +type persistTokenSource struct { + ctx context.Context + token *oauth2.Token + src oauth2.TokenSource + put func(context.Context, *oauth2.Token) error + mu sync.Mutex +} - return config.Client(ctx, token), nil +func (s *persistTokenSource) Token() (*oauth2.Token, error) { + s.mu.Lock() + defer s.mu.Unlock() + tok, err := s.src.Token() + if err != nil { + return nil, err + } + if tok.AccessToken != s.token.AccessToken || + tok.RefreshToken != s.token.RefreshToken || + !tok.Expiry.Equal(s.token.Expiry) { + *s.token = *tok + if err := s.put(s.ctx, tok); err != nil { + return nil, fmt.Errorf("unable to update token: %w", err) + } + } + return tok, nil } diff --git a/base/oauth_test.go b/base/oauth_test.go new file mode 100644 index 00000000..0d38bede --- /dev/null +++ b/base/oauth_test.go @@ -0,0 +1,160 @@ +package base + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" +) + +func TestShouldRetryAuth(t *testing.T) { + t.Parallel() + + t.Run("nil", func(t *testing.T) { + require.False(t, ShouldRetryAuth(nil)) + }) + + t.Run("invalid_grant RetrieveError", func(t *testing.T) { + err := &oauth2.RetrieveError{ErrorCode: "invalid_grant", Body: []byte(`{"error":"invalid_grant"}`)} + require.True(t, ShouldRetryAuth(err)) + require.True(t, ShouldRetryAuth(fmt.Errorf("unable to renew token: %w", err))) + }) + + t.Run("invalid_grant in body only", func(t *testing.T) { + err := &oauth2.RetrieveError{Body: []byte(`{"error":"invalid_grant"}`)} + require.True(t, ShouldRetryAuth(err)) + }) + + t.Run("missing refresh token", func(t *testing.T) { + require.True(t, ShouldRetryAuth(errors.New("oauth2: token expired and refresh token is not set"))) + }) + + t.Run("transient cannot fetch token", func(t *testing.T) { + err := &oauth2.RetrieveError{Body: []byte("connection reset"), ErrorCode: ""} + require.False(t, ShouldRetryAuth(err)) + require.False(t, ShouldRetryAuth(errors.New("oauth2: cannot fetch token: 500 Internal Server Error"))) + }) + + t.Run("unrelated", func(t *testing.T) { + require.False(t, ShouldRetryAuth(errors.New("calendar: 404 not found"))) + }) +} + +type stubTokenSource struct { + tok *oauth2.Token + err error +} + +func (s stubTokenSource) Token() (*oauth2.Token, error) { + return s.tok, s.err +} + +func TestPersistTokenSource(t *testing.T) { + t.Parallel() + baseTok := oauth2.Token{ + AccessToken: "old-access", + RefreshToken: "old-refresh", + Expiry: time.Now().Add(time.Hour), + } + + t.Run("no write when unchanged", func(t *testing.T) { + orig := baseTok + var puts int + src := PersistTokenSource(context.Background(), &orig, stubTokenSource{tok: &orig}, + func(context.Context, *oauth2.Token) error { + puts++ + return nil + }) + tok, err := src.Token() + require.NoError(t, err) + require.Equal(t, "old-access", tok.AccessToken) + require.Equal(t, 0, puts) + }) + + t.Run("writes on refresh token rotation", func(t *testing.T) { + stored := baseTok + rotated := &oauth2.Token{ + AccessToken: "new-access", + RefreshToken: "new-refresh", + Expiry: time.Now().Add(2 * time.Hour), + } + var got *oauth2.Token + src := PersistTokenSource(context.Background(), &stored, stubTokenSource{tok: rotated}, + func(_ context.Context, tok *oauth2.Token) error { + got = tok + return nil + }) + tok, err := src.Token() + require.NoError(t, err) + require.Equal(t, "new-refresh", tok.RefreshToken) + require.Equal(t, "new-refresh", stored.RefreshToken) + require.Equal(t, "new-refresh", got.RefreshToken) + }) + + t.Run("put uses uncancelled context", func(t *testing.T) { + orig := baseTok + ctx, cancel := context.WithCancel(context.Background()) + cancel() + rotated := &oauth2.Token{ + AccessToken: "new-access", + RefreshToken: "new-refresh", + Expiry: time.Now().Add(time.Hour), + } + src := PersistTokenSource(ctx, &orig, stubTokenSource{tok: rotated}, + func(putCtx context.Context, _ *oauth2.Token) error { + require.NoError(t, putCtx.Err()) + return nil + }) + _, err := src.Token() + require.NoError(t, err) + }) + + t.Run("put error", func(t *testing.T) { + orig := baseTok + rotated := &oauth2.Token{ + AccessToken: "new-access", + RefreshToken: "old-refresh", + Expiry: time.Now().Add(time.Hour), + } + src := PersistTokenSource(context.Background(), &orig, stubTokenSource{tok: rotated}, + func(context.Context, *oauth2.Token) error { + return errors.New("db down") + }) + _, err := src.Token() + require.ErrorContains(t, err, "unable to update token") + require.ErrorContains(t, err, "db down") + }) +} + +func TestConfigTokenSourceZeroExpiry(t *testing.T) { + t.Parallel() + token := &oauth2.Token{AccessToken: "a", RefreshToken: "r"} + require.True(t, token.Valid(), "zero expiry is Valid() in oauth2") + + //nolint:gosec // G101: False positive - TokenURL is a dummy loopback address, not credentials + cfg := &oauth2.Config{Endpoint: oauth2.Endpoint{TokenURL: "http://127.0.0.1:1"}} + src := ConfigTokenSource(context.Background(), cfg, token) + _, err := src.Token() + require.Error(t, err, "zero-expiry token with refresh token must be refreshed, not reused") +} + +func TestPersistTokenSourceConcurrent(t *testing.T) { + t.Parallel() + token := &oauth2.Token{AccessToken: "a", RefreshToken: "r", Expiry: time.Now().Add(time.Hour)} + rotated := &oauth2.Token{AccessToken: "b", RefreshToken: "r2", Expiry: time.Now().Add(2 * time.Hour)} + src := PersistTokenSource(context.Background(), token, stubTokenSource{tok: rotated}, + func(context.Context, *oauth2.Token) error { return nil }) + var wg sync.WaitGroup + for range 8 { + wg.Go(func() { + _, err := src.Token() + require.NoError(t, err) + }) + } + wg.Wait() +} diff --git a/gcalbot/gcalbot/account.go b/gcalbot/gcalbot/account.go index 54057be9..83723b0e 100644 --- a/gcalbot/gcalbot/account.go +++ b/gcalbot/gcalbot/account.go @@ -4,12 +4,13 @@ import ( "context" "fmt" "strings" - "time" + "sync" "golang.org/x/oauth2" "google.golang.org/api/googleapi" + "github.com/keybase/go-keybase-chat-bot/kbchat" "github.com/keybase/go-keybase-chat-bot/kbchat/types/chat1" "github.com/keybase/managed-bots/base" "google.golang.org/api/calendar/v3" @@ -90,9 +91,10 @@ func (h *Handler) deleteAccount(ctx context.Context, keybaseUsername, accountNic return fmt.Errorf("error getting account: %s", err) } - srv, err := h.GetCalendarServiceWithRetry(ctx, account) - if err == nil { - // Successfully got service, stop all channels before deleting + srv, err := getCalendarService(ctx, account, h.oauth, h.db) + if err != nil { + h.Debug("skipping channel cleanup for %s/%s: %s", keybaseUsername, accountNickname, err) + } else { channels, err := h.db.GetChannelListByAccount(ctx, account) if err != nil { return err @@ -107,7 +109,6 @@ func (h *Handler) deleteAccount(ctx context.Context, keybaseUsername, accountNic case nil: case *googleapi.Error: if err.Code == 404 { - // if the channel wasn't found, continue continue } return err @@ -115,68 +116,99 @@ func (h *Handler) deleteAccount(ctx context.Context, keybaseUsername, accountNic return err } } - } else if _, ok := err.(AccountAuthError); ok { - // Auth already failed, can't stop channels but continue with deletion - h.Debug("skipping channel cleanup for %s/%s due to auth failure", keybaseUsername, accountNickname) - } else { - // Unexpected error - return err } // cascading delete of account, oauth, subscriptions, channels and invites - err = h.db.DeleteAccount(ctx, keybaseUsername, accountNickname) - - return err + return h.db.DeleteAccount(ctx, keybaseUsername, accountNickname) } -func GetCalendarService(ctx context.Context, account *Account, config *oauth2.Config, db *DB) (srv *calendar.Service, err error) { - if account.Token.Expiry.Before(time.Now()) { - newToken, err := config.TokenSource(ctx, &account.Token).Token() - if err != nil { - return nil, err - } - account.Token = *newToken - err = db.InsertAccount(ctx, *account) - if err != nil { - return nil, fmt.Errorf("unable to update account token: %s", err) - } +func getCalendarService(ctx context.Context, account *Account, config *oauth2.Config, db *DB) (*calendar.Service, error) { + src := base.PersistTokenSource(ctx, &account.Token, base.ConfigTokenSource(ctx, config, &account.Token), + func(ctx context.Context, _ *oauth2.Token) error { + return db.InsertAccount(ctx, *account) + }) + if _, err := src.Token(); err != nil { + return nil, err } - client := config.Client(ctx, &account.Token) - return calendar.NewService(ctx, option.WithHTTPClient(client)) + return calendar.NewService(ctx, option.WithHTTPClient(oauth2.NewClient(ctx, src))) } -// GetCalendarServiceWithRetry wraps GetCalendarService and handles auth failures -// by deleting invalid credentials. Returns AccountAuthError if credentials were deleted. -func (h *Handler) GetCalendarServiceWithRetry(ctx context.Context, account *Account) (*calendar.Service, error) { - srv, err := GetCalendarService(ctx, account, h.oauth, h.db) - if err != nil && base.ShouldRetryAuth(err) { - h.Errorf("auth failed for %s/%s, deleting credentials: %v", account.KeybaseUsername, account.AccountNickname, err) - if delErr := h.db.DeleteAccount(ctx, account.KeybaseUsername, account.AccountNickname); delErr != nil { - h.Errorf("failed to delete account after auth error: %v", delErr) - } - return nil, AccountAuthError{ - Username: account.KeybaseUsername, - Nickname: account.AccountNickname, - } +const reconnectAccountMsg = "Your account '%s' needs to be reconnected. Please run `!gcal accounts connect %s` again." + +// CalendarAuth obtains a Calendar client and recovers from invalid OAuth credentials. +type CalendarAuth struct { + oauth *oauth2.Config + db *DB + debug *base.DebugOutput + kbc *kbchat.API + + mu sync.Mutex + invalidated map[string]struct{} +} + +func NewCalendarAuth(oauth *oauth2.Config, db *DB, debug *base.DebugOutput, kbc *kbchat.API) *CalendarAuth { + return &CalendarAuth{oauth: oauth, db: db, debug: debug, kbc: kbc, invalidated: make(map[string]struct{})} +} + +func accountKey(account *Account) string { + return account.KeybaseUsername + "\x00" + account.AccountNickname +} + +func (c *CalendarAuth) GetCalendarService(ctx context.Context, account *Account) (*calendar.Service, error) { + if err := c.alreadyInvalidated(account); err != nil { + return nil, err + } + srv, err := getCalendarService(ctx, account, c.oauth, c.db) + if err != nil { + return nil, c.InvalidateIfAuthError(ctx, account, err) } - return srv, err + return srv, nil } -// handleAuthError sends a reconnection message to the user for AccountAuthError -func (h *Handler) handleAuthError(err error, accountNickname string, convID chat1.ConvIDStr) error { - if _, ok := err.(AccountAuthError); !ok { - return err +func (c *CalendarAuth) alreadyInvalidated(account *Account) error { + c.mu.Lock() + defer c.mu.Unlock() + if _, ok := c.invalidated[accountKey(account)]; ok { + return AccountAuthError{Username: account.KeybaseUsername, Nickname: account.AccountNickname} } - h.ChatEcho(convID, "Your account '%s' needs to be reconnected. Please run `!gcal accounts connect %s` again.", accountNickname, accountNickname) return nil } -// handleAuthErrorDM sends a reconnection message via DM for AccountAuthError -func (h *Handler) handleAuthErrorDM(err error, account *Account) error { - if _, ok := err.(AccountAuthError); !ok { +// InvalidateIfAuthError deletes the account and DMs the user when err +// indicates invalid OAuth credentials. Returns AccountAuthError in that case, +// otherwise returns err unchanged. Safe to call repeatedly for the same account. +func (c *CalendarAuth) InvalidateIfAuthError(ctx context.Context, account *Account, err error) error { + if err == nil { + return nil + } + if IsAccountAuthError(err) { return err } - _, sendErr := h.kbc.SendMessageByTlfName(account.KeybaseUsername, - "Your account '%s' needs to be reconnected. Please run `!gcal accounts connect %s` again.", account.AccountNickname, account.AccountNickname) - return sendErr + if !base.ShouldRetryAuth(err) { + return err + } + authErr := AccountAuthError{Username: account.KeybaseUsername, Nickname: account.AccountNickname} + c.mu.Lock() + if _, seen := c.invalidated[accountKey(account)]; seen { + c.mu.Unlock() + return authErr + } + c.invalidated[accountKey(account)] = struct{}{} + c.mu.Unlock() + + c.debug.Errorf("auth failed for %s/%s, deleting credentials: %v", account.KeybaseUsername, account.AccountNickname, err) + if delErr := c.db.DeleteAccount(ctx, account.KeybaseUsername, account.AccountNickname); delErr != nil { + c.debug.Errorf("failed to delete account after auth error: %v", delErr) + } + if _, sendErr := c.kbc.SendMessageByTlfName(account.KeybaseUsername, reconnectAccountMsg, + account.AccountNickname, account.AccountNickname); sendErr != nil { + c.debug.Errorf("failed to DM user after auth error: %v", sendErr) + } + return authErr +} + +// WrapAuth invalidates the account on auth failure and returns nil in that +// case so callers can treat reconnect-needed as handled. +func (c *CalendarAuth) WrapAuth(ctx context.Context, account *Account, err error) error { + return IgnoreAccountAuthError(c.InvalidateIfAuthError(ctx, account, err)) } diff --git a/gcalbot/gcalbot/calendar.go b/gcalbot/gcalbot/calendar.go index c5d2cf34..4e5ee340 100644 --- a/gcalbot/gcalbot/calendar.go +++ b/gcalbot/gcalbot/calendar.go @@ -8,7 +8,7 @@ import ( "google.golang.org/api/calendar/v3" ) -func (h *Handler) handleCalendarsList(ctx context.Context, msg chat1.MsgSummary, args []string) error { +func (h *Handler) handleCalendarsList(ctx context.Context, msg chat1.MsgSummary, args []string) (err error) { if len(args) != 1 { h.ChatEcho(msg.ConvID, "Invalid number of arguments.") return nil @@ -25,9 +25,17 @@ func (h *Handler) handleCalendarsList(ctx context.Context, msg chat1.MsgSummary, return nil } - srv, err := h.GetCalendarServiceWithRetry(ctx, account) + defer func() { + err = h.InvalidateIfAuthError(ctx, account, err) + if IsAccountAuthError(err) { + h.ChatEcho(msg.ConvID, reconnectAccountMsg, accountNickname, accountNickname) + err = nil + } + }() + + srv, err := h.GetCalendarService(ctx, account) if err != nil { - return h.handleAuthError(err, accountNickname, msg.ConvID) + return err } calendarList, err := getCalendarList(ctx, srv) diff --git a/gcalbot/gcalbot/handler.go b/gcalbot/gcalbot/handler.go index e7ce3c1c..b63886c2 100644 --- a/gcalbot/gcalbot/handler.go +++ b/gcalbot/gcalbot/handler.go @@ -14,6 +14,7 @@ import ( type Handler struct { *base.DebugOutput + *CalendarAuth stats *base.StatsRegistry kbc *kbchat.API @@ -39,8 +40,10 @@ func NewHandler( tokenSecret string, httpPrefix string, ) *Handler { + debug := base.NewDebugOutput("Handler", debugConfig) return &Handler{ - DebugOutput: base.NewDebugOutput("Handler", debugConfig), + DebugOutput: debug, + CalendarAuth: NewCalendarAuth(oauth, db, debug, kbc), stats: stats.SetPrefix("Handler"), kbc: kbc, db: db, diff --git a/gcalbot/gcalbot/html.go b/gcalbot/gcalbot/html.go index 79893032..3d174b5b 100644 --- a/gcalbot/gcalbot/html.go +++ b/gcalbot/gcalbot/html.go @@ -289,17 +289,19 @@ const tmplLogin = `{{template "header" .}} {{template "footer" .}}` type ErrorPage struct { - Title string + Title string + Heading string + Body string } const tmplError = `{{template "header" .}}

- An error occurred :( + {{if .Heading}}{{.Heading}}{{else}}An error occurred :({{end}}

- Please try again! + {{if .Body}}{{.Body}}{{else}}Please try again!{{end}}

If the error is recurring, report the issue by messaging @gcalbot diff --git a/gcalbot/gcalbot/http.go b/gcalbot/gcalbot/http.go index 1ca6c9f6..00bc4a4b 100644 --- a/gcalbot/gcalbot/http.go +++ b/gcalbot/gcalbot/http.go @@ -84,7 +84,12 @@ func (h *HTTPSrv) healthCheckHandler(_ http.ResponseWriter, _ *http.Request) {} func (h *HTTPSrv) configHandler(w http.ResponseWriter, r *http.Request) { h.Stats.Count("config") var err error + var accountNickname string defer func() { + if IsAccountAuthError(err) { + h.showReconnect(w, accountNickname) + return + } if err != nil { h.Errorf("error in configHandler: %s", err) h.showConfigError(w) @@ -118,7 +123,7 @@ func (h *HTTPSrv) configHandler(w http.ResponseWriter, r *http.Request) { isPrivate := base.IsDirectPrivateMessage(h.kbc.GetUsername(), keybaseUsername, keybaseConv.Channel) - accountNickname := r.Form.Get("account") + accountNickname = r.Form.Get("account") calendarID := r.Form.Get("calendar") previousAccountNickname := r.Form.Get("previous_account") @@ -201,20 +206,14 @@ func (h *HTTPSrv) configHandler(w http.ResponseWriter, r *http.Request) { return } - srv, err := h.handler.GetCalendarServiceWithRetry(ctx, selectedAccount) + srv, err := h.handler.GetCalendarService(ctx, selectedAccount) if err != nil { - switch err.(type) { - case AccountAuthError: - h.Errorf("account auth failed for web UI: %v", err) - h.showConfigError(w) - default: - h.Errorf("error getting calendar service: %v", err) - } return } calendarList, err := srv.CalendarList.List().Do() if err != nil { + err = h.handler.InvalidateIfAuthError(ctx, selectedAccount, err) return } page.Calendars = calendarList.Items @@ -453,6 +452,18 @@ func (h *HTTPSrv) showConfigError(w http.ResponseWriter) { }) } +func (h *HTTPSrv) showReconnect(w http.ResponseWriter, accountNickname string) { + h.Stats.Count("configReconnect") + w.WriteHeader(http.StatusUnauthorized) + h.servePage(w, "error", ErrorPage{ + Title: "gcalbot | reconnect", + Heading: "Account needs to be reconnected", + Body: fmt.Sprintf( + "Your account '%s' needs to be reconnected. Message @gcalbot in the Keybase app with !gcal accounts connect %s.", + accountNickname, accountNickname), + }) +} + func (h *HTTPSrv) homeHandler(w http.ResponseWriter, _ *http.Request) { h.Stats.Count("home") homePage := `Google Calendar Bot is a Keybase chatbot diff --git a/gcalbot/gcalbot/invite.go b/gcalbot/gcalbot/invite.go index 59b7143e..91469840 100644 --- a/gcalbot/gcalbot/invite.go +++ b/gcalbot/gcalbot/invite.go @@ -2,6 +2,7 @@ package gcalbot import ( "context" + "errors" "fmt" "time" @@ -28,8 +29,9 @@ const ( ResponseStatusAccepted ResponseStatus = "accepted" ) -func (h *Handler) sendEventInvite(ctx context.Context, account *Account, channel *Channel, event *calendar.Event) error { +func (h *Handler) sendEventInvite(ctx context.Context, account *Account, channel *Channel, event *calendar.Event) (err error) { h.stats.Count("sendEventInvite") + defer func() { err = h.WrapAuth(ctx, account, err) }() message := `You've been invited to %s: %s Awaiting your response. *Are you going?*` @@ -41,9 +43,9 @@ Awaiting your response. *Are you going?*` eventType = "a recurring event" } - srv, err := h.GetCalendarServiceWithRetry(ctx, account) + srv, err := h.GetCalendarService(ctx, account) if err != nil { - return h.handleAuthErrorDM(err, account) + return err } timezone, err := GetUserTimezone(srv) if err != nil { @@ -87,8 +89,9 @@ Awaiting your response. *Are you going?*` return nil } -func (h *Handler) updateEventResponseStatus(ctx context.Context, invite *Invite, account *Account, reaction InviteReaction) error { +func (h *Handler) updateEventResponseStatus(ctx context.Context, invite *Invite, account *Account, reaction InviteReaction) (err error) { h.stats.Count("updateEventResponseStatus") + defer func() { err = h.WrapAuth(ctx, account, err) }() var responseStatus ResponseStatus var confirmationMessageStatus string @@ -107,28 +110,22 @@ func (h *Handler) updateEventResponseStatus(ctx context.Context, invite *Invite, return nil } - srv, err := h.GetCalendarServiceWithRetry(ctx, account) + srv, err := h.GetCalendarService(ctx, account) if err != nil { - return h.handleAuthErrorDM(err, account) + return err } // fetch event // TODO(marcel): check if event was deleted event, err := srv.Events.Get(invite.CalendarID, invite.EventID).Fields("attendees").Do() - switch typedErr := err.(type) { - case nil: - case *googleapi.Error: - if typedErr.Code == 404 { + if err != nil { + var gerr *googleapi.Error + if errors.As(err, &gerr) && gerr.Code == 404 { _, err = h.kbc.SendMessageByTlfName(account.KeybaseUsername, "I couldn't update your status. Are you sure this event still exists?") - if err != nil { - return err - } - return nil + return err } return fmt.Errorf("error getting event: %s", err) - default: - return fmt.Errorf("error getting event: %s", err) } // update response status on event @@ -173,12 +170,19 @@ func (h *Handler) updateEventResponseStatus(ctx context.Context, invite *Invite, func (h *Handler) syncAllInvites(account *Account, srv *calendar.Service, channelID, calendarID string) { syncStart := time.Now() + // context.Background() because syncAllInvites is a background goroutine that outlives the request context + ctx := context.Background() + var err error + defer func() { + if err = h.WrapAuth(ctx, account, err); err != nil { + h.Errorf("error syncing all invites: %s", err) + } + }() var nextSyncToken string var events []*calendar.Event - // context.Background() because syncAllInvites is a background goroutine that outlives the request context - err := srv.Events.List(calendarID). - Pages(context.Background(), func(page *calendar.Events) error { + err = srv.Events.List(calendarID). + Pages(ctx, func(page *calendar.Events) error { if page.NextPageToken == "" { // set the sync token when the page token is empty nextSyncToken = page.NextSyncToken @@ -187,7 +191,6 @@ func (h *Handler) syncAllInvites(account *Account, srv *calendar.Service, channe return nil }) if err != nil { - h.Errorf("error syncing all invites: %s", err) return } @@ -210,16 +213,18 @@ func (h *Handler) syncAllInvites(account *Account, srv *calendar.Service, channe continue } else if event.End.DateTime != "" { // this is a normal event - end, err = time.Parse(time.RFC3339, event.End.DateTime) - if err != nil { - h.Errorf("error parsing time: %s", err) + var parseErr error + end, parseErr = time.Parse(time.RFC3339, event.End.DateTime) + if parseErr != nil { + h.Errorf("error parsing time: %s", parseErr) continue } } else if event.End.Date != "" { // this is an all day event - end, err = time.Parse(AllDayDateFormat, event.End.Date) - if err != nil { - h.Errorf("error parsing time: %s", err) + var parseErr error + end, parseErr = time.Parse(AllDayDateFormat, event.End.Date) + if parseErr != nil { + h.Errorf("error parsing time: %s", parseErr) continue } end = end.Add(-24 * time.Hour) // the google API sets the end day to the day after, so compensate by one day @@ -236,21 +241,19 @@ func (h *Handler) syncAllInvites(account *Account, srv *calendar.Service, channe for _, attendee := range event.Attendees { responseStatus := ResponseStatus(attendee.ResponseStatus) if attendee.Self && !attendee.Organizer && responseStatus == ResponseStatusNeedsAction { - err = h.db.InsertInvite(context.Background(), account, Invite{ + if insErr := h.db.InsertInvite(ctx, account, Invite{ CalendarID: calendarID, EventID: event.Id, - }) - if err != nil { - h.Errorf("error inserting invite: %s", err) + }); insErr != nil { + h.Errorf("error inserting invite: %s", insErr) } break } } } - err = h.db.UpdateChannelNextSyncToken(context.Background(), channelID, nextSyncToken) + err = h.db.UpdateChannelNextSyncToken(ctx, channelID, nextSyncToken) if err != nil { - h.Errorf("unable to update sync token: %v", err) return } diff --git a/gcalbot/gcalbot/oauth.go b/gcalbot/gcalbot/oauth.go index 4d1827e8..47fee971 100644 --- a/gcalbot/gcalbot/oauth.go +++ b/gcalbot/gcalbot/oauth.go @@ -88,7 +88,7 @@ func (h *HTTPSrv) oauthHandler(w http.ResponseWriter, r *http.Request) { // if account was created in a 1on1 conv, create default subscription to invites & 5 minute reminder for primary calendar if base.IsDirectPrivateMessage(h.kbc.GetUsername(), req.KeybaseUsername, conv.Channel) { var srv *calendar.Service - srv, err = GetCalendarService(ctx, &account, h.oauth, h.db) + srv, err = h.handler.GetCalendarService(ctx, &account) if err != nil { return } diff --git a/gcalbot/gcalbot/reminderscheduler/main.go b/gcalbot/gcalbot/reminderscheduler/main.go index c0b1bf72..8b496599 100644 --- a/gcalbot/gcalbot/reminderscheduler/main.go +++ b/gcalbot/gcalbot/reminderscheduler/main.go @@ -3,6 +3,7 @@ package reminderscheduler import ( "sync" + "github.com/keybase/go-keybase-chat-bot/kbchat" "github.com/keybase/managed-bots/base" "github.com/keybase/managed-bots/gcalbot/gcalbot" "golang.org/x/oauth2" @@ -17,7 +18,7 @@ type ReminderScheduler struct { stats *base.StatsRegistry db *gcalbot.DB - oauth *oauth2.Config + cal *gcalbot.CalendarAuth subscriptionReminders *SubscriptionReminders eventReminders *EventReminders @@ -29,13 +30,15 @@ func NewReminderScheduler( debugConfig *base.ChatDebugOutputConfig, db *gcalbot.DB, oauth *oauth2.Config, + kbc *kbchat.API, ) *ReminderScheduler { + debug := base.NewDebugOutput("ReminderScheduler", debugConfig) return &ReminderScheduler{ stats: stats.SetPrefix("ReminderScheduler"), - DebugOutput: base.NewDebugOutput("ReminderScheduler", debugConfig), + DebugOutput: debug, shutdownCh: make(chan struct{}), db: db, - oauth: oauth, + cal: gcalbot.NewCalendarAuth(oauth, db, debug, kbc), subscriptionReminders: NewSubscriptionReminders(), eventReminders: NewEventReminders(), minuteReminders: NewMinuteReminders(), diff --git a/gcalbot/gcalbot/reminderscheduler/sync.go b/gcalbot/gcalbot/reminderscheduler/sync.go index c26f6780..af680512 100644 --- a/gcalbot/gcalbot/reminderscheduler/sync.go +++ b/gcalbot/gcalbot/reminderscheduler/sync.go @@ -5,9 +5,6 @@ import ( "context" "time" - "golang.org/x/oauth2" - - "github.com/keybase/managed-bots/base" "github.com/keybase/managed-bots/gcalbot/gcalbot" "google.golang.org/api/calendar/v3" ) @@ -48,14 +45,15 @@ func (r *ReminderScheduler) eventSyncLoop(shutdownCh chan struct{}) error { } func (r *ReminderScheduler) syncEvents(account *gcalbot.Account, subscription *gcalbot.Subscription) { - srv, err := gcalbot.GetCalendarService(context.Background(), account, r.oauth, r.db) - switch err.(type) { - case nil: - case *oauth2.RetrieveError: - r.Debug("error retrieving token: %s", err) - return - default: - r.Errorf("error getting calendar service: %s", err) + var err error + defer func() { + if err = r.cal.WrapAuth(context.Background(), account, err); err != nil { + r.Errorf("error syncing events: %s", err) + } + }() + + srv, err := r.cal.GetCalendarService(context.Background(), account) + if err != nil { return } @@ -72,19 +70,17 @@ func (r *ReminderScheduler) syncEvents(account *gcalbot.Account, subscription *g events = append(events, page.Items...) return nil }) - switch err := err.(type) { - case nil: - case *oauth2.RetrieveError: - base.LogOAuthError(r.DebugOutput, "error refreshing token API", err) - return - default: - r.Debug("error getting events from API: %s", err) + if err != nil { return } for _, event := range events { err = r.UpdateOrCreateReminderEvent(account, subscription, event) if err != nil { + if gcalbot.IsAccountAuthError(err) { + return + } r.Errorf("error updating or creating reminder event: %s", err) + err = nil } } } @@ -93,7 +89,8 @@ func (r *ReminderScheduler) UpdateOrCreateReminderEvent( account *gcalbot.Account, subscription *gcalbot.Subscription, event *calendar.Event, -) error { +) (err error) { + defer func() { err = r.cal.InvalidateIfAuthError(context.Background(), account, err) }() r.stats.Count("UpdateOrCreateReminderEvent") status := gcalbot.EventStatus(event.Status) if status == gcalbot.EventStatusCancelled { @@ -125,13 +122,8 @@ func (r *ReminderScheduler) UpdateOrCreateReminderEvent( } }) - srv, err := gcalbot.GetCalendarService(context.Background(), account, r.oauth, r.db) - switch err.(type) { - case nil: - case *oauth2.RetrieveError: - r.Debug("error retrieving token: %s", err) - return nil - default: + srv, err := r.cal.GetCalendarService(context.Background(), account) + if err != nil { return err } diff --git a/gcalbot/gcalbot/schedulescheduler/main.go b/gcalbot/gcalbot/schedulescheduler/main.go index fddf5c8e..8d5d7984 100644 --- a/gcalbot/gcalbot/schedulescheduler/main.go +++ b/gcalbot/gcalbot/schedulescheduler/main.go @@ -3,6 +3,7 @@ package schedulescheduler import ( "sync" + "github.com/keybase/go-keybase-chat-bot/kbchat" "github.com/keybase/managed-bots/base" "github.com/keybase/managed-bots/gcalbot/gcalbot" "golang.org/x/oauth2" @@ -16,7 +17,7 @@ type ScheduleScheduler struct { stats *base.StatsRegistry db *gcalbot.DB - oauth *oauth2.Config + cal *gcalbot.CalendarAuth } func NewScheduleScheduler( @@ -24,13 +25,15 @@ func NewScheduleScheduler( debugConfig *base.ChatDebugOutputConfig, db *gcalbot.DB, oauth *oauth2.Config, + kbc *kbchat.API, ) *ScheduleScheduler { + debug := base.NewDebugOutput("ScheduleScheduler", debugConfig) return &ScheduleScheduler{ stats: stats.SetPrefix("ScheduleScheduler"), - DebugOutput: base.NewDebugOutput("ScheduleScheduler", debugConfig), + DebugOutput: debug, shutdownCh: make(chan struct{}), db: db, - oauth: oauth, + cal: gcalbot.NewCalendarAuth(oauth, db, debug, kbc), } } diff --git a/gcalbot/gcalbot/schedulescheduler/send.go b/gcalbot/gcalbot/schedulescheduler/send.go index 7d6ab029..9765e8b8 100644 --- a/gcalbot/gcalbot/schedulescheduler/send.go +++ b/gcalbot/gcalbot/schedulescheduler/send.go @@ -8,8 +8,6 @@ import ( "strings" "time" - "golang.org/x/oauth2" - "google.golang.org/api/calendar/v3" "google.golang.org/api/googleapi" @@ -103,18 +101,17 @@ func (s *ScheduleScheduler) SendDailyScheduleMessage(sendMinute time.Time, subsc s.stats.Count("SendDailyScheduleMessage") s.stats.CountMult("SendDailyScheduleMessage - calendars", len(subscription.CalendarIDs)) - srv, err := gcalbot.GetCalendarService(context.Background(), &subscription.Account, s.oauth, s.db) - switch err.(type) { - case nil: - case *oauth2.RetrieveError: - s.Debug("error retrieving token: %s", err) - return - default: - if base.ShouldRetryAuth(err) { - s.Debug("auth error in scheduler (will not auto-delete): %s", err) - return + ctx := context.Background() + account := &subscription.Account + var err error + defer func() { + if err = s.cal.WrapAuth(ctx, account, err); err != nil { + s.Errorf("unable to send daily schedule: %s", err) } - s.Errorf("unable to get calendar service: %s", err) + }() + + srv, err := s.cal.GetCalendarService(ctx, account) + if err != nil { return } @@ -129,21 +126,24 @@ func (s *ScheduleScheduler) SendDailyScheduleMessage(sendMinute time.Time, subsc format24HourTime, err := gcalbot.GetUserFormat24HourTime(srv) if err != nil { - s.Errorf("unable to get user 24 hour time setting: %s", err) return } calendarSummaries := make([]string, len(subscription.CalendarIDs)) var events []*calendar.Event for index, calendarID := range subscription.CalendarIDs { - cal, err := srv.Calendars.Get(calendarID).Fields("summary").Do() - if err != nil { + cal, calErr := srv.Calendars.Get(calendarID).Fields("summary").Do() + if calErr != nil { + if base.ShouldRetryAuth(calErr) { + err = calErr + return + } var gerr *googleapi.Error - if errors.As(err, &gerr) && gerr.Code == 404 { + if errors.As(calErr, &gerr) && gerr.Code == 404 { // Calendar was deleted or user lost access; use ID as display name s.Debug("calendar no longer accessible (404): %s", calendarID) } else { - s.Errorf("error getting calendar summary from API: %s", err) + s.Errorf("error getting calendar summary from API: %s", calErr) } calendarSummaries[index] = calendarID // use the cal id if there is an error } else { @@ -155,12 +155,16 @@ func (s *ScheduleScheduler) SendDailyScheduleMessage(sendMinute time.Time, subsc TimeMax(maxTime.Format(time.RFC3339)). SingleEvents(true). OrderBy("startTime"). - Pages(context.Background(), func(page *calendar.Events) error { + Pages(ctx, func(page *calendar.Events) error { events = append(events, page.Items...) return nil }) if err != nil { + if base.ShouldRetryAuth(err) { + return + } s.Debug("error getting events from API: %s", err) + err = nil continue } } @@ -185,6 +189,7 @@ Calendars: %s formattedSchedule, err = gcalbot.FormatEventSchedule(events, subscription.Timezone, format24HourTime) if err != nil { s.Errorf("unable to format schedule: %s", err) + err = nil return } } diff --git a/gcalbot/gcalbot/type.go b/gcalbot/gcalbot/type.go index 0842a170..0b3c04ce 100644 --- a/gcalbot/gcalbot/type.go +++ b/gcalbot/gcalbot/type.go @@ -1,6 +1,7 @@ package gcalbot import ( + "errors" "fmt" "time" @@ -114,3 +115,15 @@ type AccountAuthError struct { func (e AccountAuthError) Error() string { return fmt.Sprintf("account '%s' for user '%s' requires re-authentication", e.Nickname, e.Username) } + +func IsAccountAuthError(err error) bool { + var e AccountAuthError + return errors.As(err, &e) +} + +func IgnoreAccountAuthError(err error) error { + if IsAccountAuthError(err) { + return nil + } + return err +} diff --git a/gcalbot/gcalbot/type_test.go b/gcalbot/gcalbot/type_test.go new file mode 100644 index 00000000..a4415607 --- /dev/null +++ b/gcalbot/gcalbot/type_test.go @@ -0,0 +1,18 @@ +package gcalbot + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestIgnoreAccountAuthError(t *testing.T) { + t.Parallel() + authErr := AccountAuthError{Username: "u", Nickname: "n"} + require.True(t, IsAccountAuthError(authErr)) + require.True(t, IsAccountAuthError(fmt.Errorf("wrap: %w", authErr))) + require.NoError(t, IgnoreAccountAuthError(authErr)) + require.EqualError(t, IgnoreAccountAuthError(fmt.Errorf("boom")), "boom") + require.False(t, IsAccountAuthError(fmt.Errorf("boom"))) +} diff --git a/gcalbot/gcalbot/webhook.go b/gcalbot/gcalbot/webhook.go index e471d080..35bb865d 100644 --- a/gcalbot/gcalbot/webhook.go +++ b/gcalbot/gcalbot/webhook.go @@ -2,6 +2,7 @@ package gcalbot import ( "context" + "errors" "fmt" "net/http" "sync" @@ -9,6 +10,7 @@ import ( "golang.org/x/oauth2" + "github.com/keybase/go-keybase-chat-bot/kbchat" "github.com/keybase/managed-bots/base" "google.golang.org/api/calendar/v3" @@ -17,7 +19,14 @@ import ( func (h *HTTPSrv) handleEventUpdateWebhook(w http.ResponseWriter, r *http.Request) { var err error + var account *Account + // WithoutCancel: Google sends the webhook and may close the connection + // immediately; DB writes and reminder scheduling must complete regardless. + ctx := context.WithoutCancel(r.Context()) defer func() { + if account != nil { + err = h.handler.WrapAuth(ctx, account, err) + } if err != nil { h.Errorf("error in event update webhook: %s", err) } @@ -28,13 +37,11 @@ func (h *HTTPSrv) handleEventUpdateWebhook(w http.ResponseWriter, r *http.Reques // sync header, safe to ignore return } - // WithoutCancel: Google sends the webhook and may close the connection - // immediately; DB writes and reminder scheduling must complete regardless. - ctx := context.WithoutCancel(r.Context()) channelID := r.Header.Get("X-Goog-Channel-ID") resourceID := r.Header.Get("X-Goog-Resource-ID") - channel, account, err := h.db.GetChannelAndAccountByID(ctx, channelID) + var channel *Channel + channel, account, err = h.db.GetChannelAndAccountByID(ctx, channelID) if err != nil { return } else if channel == nil { @@ -62,19 +69,8 @@ func (h *HTTPSrv) handleEventUpdateWebhook(w http.ResponseWriter, r *http.Reques return } - srv, err := GetCalendarService(ctx, account, h.oauth, h.db) - switch err.(type) { - case nil: - case *oauth2.RetrieveError: - h.Debug("error retrieving token: %s", err) - err = nil // clear error - return - default: - if base.ShouldRetryAuth(err) { - h.Debug("auth error in webhook (will not auto-delete): %s", err) - err = nil // clear error - return - } + srv, err := h.handler.GetCalendarService(ctx, account) + if err != nil { return } @@ -198,6 +194,9 @@ func (h *HTTPSrv) handleEventUpdateWebhook(w http.ResponseWriter, r *http.Reques break } } + if err != nil { + return + } } err = h.db.UpdateChannelNextSyncToken(ctx, channelID, nextSyncToken) @@ -235,8 +234,10 @@ func (h *Handler) createSubscription( func (h *Handler) removeSubscription( ctx context.Context, account *Account, subscription Subscription, -) error { - err := h.db.DeleteSubscription(ctx, account, subscription) +) (err error) { + defer func() { err = h.WrapAuth(ctx, account, err) }() // swallow auth errors; subscription is already removed + + err = h.db.DeleteSubscription(ctx, account, subscription) if err != nil { // if no error, subscription doesn't exist, short circuit return err @@ -251,34 +252,28 @@ func (h *Handler) removeSubscription( if subscriptionCount == 0 { // if there are no more subscriptions for this account + calendar, remove the channel - channel, err := h.db.GetChannel(ctx, account, subscription.CalendarID) + var channel *Channel + channel, err = h.db.GetChannel(ctx, account, subscription.CalendarID) if err != nil { return err } if channel != nil { - srv, err := GetCalendarService(ctx, account, h.oauth, h.db) + var srv *calendar.Service + srv, err = h.GetCalendarService(ctx, account) if err != nil { - if base.ShouldRetryAuth(err) { - h.Debug("auth error stopping channel, skipping channel.Stop: %s", err) - // Still delete from DB even if we can't stop the channel - } else { - return err - } - } else { - // Only try to stop if we got the service successfully - err = srv.Channels.Stop(&calendar.Channel{ - Id: channel.ChannelID, - ResourceId: channel.ResourceID, - }).Do() - switch err := err.(type) { - case nil: - case *googleapi.Error: - if err.Code != 404 { - return err - } + return err + } + err = srv.Channels.Stop(&calendar.Channel{ + Id: channel.ChannelID, + ResourceId: channel.ResourceID, + }).Do() + if err != nil { + var gerr *googleapi.Error + if errors.As(err, &gerr) && gerr.Code == 404 { // if the channel wasn't found, don't return - default: + err = nil + } else { return err } } @@ -293,12 +288,14 @@ func (h *Handler) removeSubscription( return nil } -func (h *Handler) createEventChannel(ctx context.Context, account *Account, calendarID string) error { - srv, err := GetCalendarService(ctx, account, h.oauth, h.db) +func (h *Handler) createEventChannel(ctx context.Context, account *Account, calendarID string) (err error) { + // Propagate AccountAuthError so callers do not insert subscriptions after + // credentials were deleted. Contrast removeSubscription, which uses WrapAuth + // because the subscription row is already gone. + defer func() { err = h.InvalidateIfAuthError(ctx, account, err) }() + + srv, err := h.GetCalendarService(ctx, account) if err != nil { - if base.ShouldRetryAuth(err) { - h.Debug("auth error creating channel, cannot create webhook: %s", err) - } return err } exists, err := h.db.ExistsChannelByAccountAndCalendar(ctx, account, calendarID) @@ -349,7 +346,7 @@ type RenewChannelScheduler struct { stats *base.StatsRegistry db *DB - config *oauth2.Config + cal *CalendarAuth httpPrefix string } @@ -358,13 +355,15 @@ func NewRenewChannelScheduler( debugConfig *base.ChatDebugOutputConfig, db *DB, config *oauth2.Config, + kbc *kbchat.API, httpPrefix string, ) *RenewChannelScheduler { + debug := base.NewDebugOutput("RenewChannelScheduler", debugConfig) return &RenewChannelScheduler{ stats: stats.SetPrefix("RenewChannelScheduler"), - DebugOutput: base.NewDebugOutput("RenewChannelScheduler", debugConfig), + DebugOutput: debug, db: db, - config: config, + cal: NewCalendarAuth(config, db, debug, kbc), httpPrefix: httpPrefix, shutdownCh: make(chan struct{}), } @@ -422,15 +421,13 @@ func (r *RenewChannelScheduler) renewScheduler(shutdownCh chan struct{}) { } } -func (r *RenewChannelScheduler) renewChannel(account *Account, channel *Channel) error { +func (r *RenewChannelScheduler) renewChannel(account *Account, channel *Channel) (err error) { r.stats.Count("renewChannel") - srv, err := GetCalendarService(context.Background(), account, r.config, r.db) - switch err.(type) { - case nil: - case *oauth2.RetrieveError: - r.Debug("error retrieving token: %s", err) - return nil - default: + ctx := context.Background() + defer func() { err = r.cal.WrapAuth(ctx, account, err) }() + + srv, err := r.cal.GetCalendarService(ctx, account) + if err != nil { return err } @@ -459,16 +456,10 @@ func (r *RenewChannelScheduler) renewChannel(account *Account, channel *Channel) Id: channel.ChannelID, ResourceId: channel.ResourceID, }).Do() - switch err := err.(type) { - case nil: - case *googleapi.Error: - if err.Code != 404 { - return err - } + var gerr *googleapi.Error + if errors.As(err, &gerr) && gerr.Code == 404 { // if the channel wasn't found, don't return an error - default: - return err + return nil } - - return nil + return err } diff --git a/gcalbot/main.go b/gcalbot/main.go index 5eef72eb..8c75cce1 100644 --- a/gcalbot/main.go +++ b/gcalbot/main.go @@ -217,9 +217,9 @@ func (s *BotServer) Go() (err error) { db := gcalbot.NewDB(sdb, debugConfig) stats = stats.SetPrefix(s.Name()) - renewScheduler := gcalbot.NewRenewChannelScheduler(stats, debugConfig, db, config, s.opts.HTTPPrefix) - reminderScheduler := reminderscheduler.NewReminderScheduler(stats, debugConfig, db, config) - scheduleScheduler := schedulescheduler.NewScheduleScheduler(stats, debugConfig, db, config) + renewScheduler := gcalbot.NewRenewChannelScheduler(stats, debugConfig, db, config, s.kbc, s.opts.HTTPPrefix) + reminderScheduler := reminderscheduler.NewReminderScheduler(stats, debugConfig, db, config, s.kbc) + scheduleScheduler := schedulescheduler.NewScheduleScheduler(stats, debugConfig, db, config, s.kbc) handler := gcalbot.NewHandler(stats, s.kbc, debugConfig, db, config, reminderScheduler, secret, s.opts.HTTPPrefix) httpSrv := gcalbot.NewHTTPSrv(stats, s.kbc, debugConfig, db, config, reminderScheduler, handler) eg := &errgroup.Group{}