From 38f042567023e771002c8c9d317c3a32b5cb7c27 Mon Sep 17 00:00:00 2001 From: James Houlahan Date: Thu, 16 Apr 2020 16:27:57 +0200 Subject: [PATCH] refactor: make sentry report its own package --- pkg/config/logs.go | 11 ++++--- pkg/pmapi/auth.go | 5 ++- pkg/pmapi/auth_test.go | 7 +--- pkg/pmapi/client.go | 2 +- pkg/pmapi/mocks/mocks.go | 28 ++++++++-------- pkg/{pmapi/sentry.go => sentry/report.go} | 32 +++++++++++-------- .../sentry_test.go => sentry/report_test.go} | 13 +++++--- test/fakeapi/auth.go | 4 +++ 8 files changed, 56 insertions(+), 46 deletions(-) rename pkg/{pmapi/sentry.go => sentry/report.go} (86%) rename pkg/{pmapi/sentry_test.go => sentry/report_test.go} (86%) diff --git a/pkg/config/logs.go b/pkg/config/logs.go index 1a554144..ddc4e4dc 100644 --- a/pkg/config/logs.go +++ b/pkg/config/logs.go @@ -19,6 +19,7 @@ package config import ( "bytes" + "errors" "fmt" "io/ioutil" "os" @@ -31,6 +32,7 @@ import ( "time" "github.com/ProtonMail/proton-bridge/pkg/pmapi" + "github.com/ProtonMail/proton-bridge/pkg/sentry" "github.com/sirupsen/logrus" ) @@ -57,11 +59,10 @@ var logCrashRgx = regexp.MustCompile("^v.*_crash_.*\\.log$") //nolint[gochecknog // HandlePanic reports the crash to sentry or local file when sentry fails. func HandlePanic(cfg *Config, output string) { if !cfg.IsDevMode() { - // TODO: Is it okay to just create a throwaway client like this? - c := pmapi.NewClientManager(cfg.GetAPIConfig()).GetAnonymousClient() - defer c.Logout() - - if err := c.ReportSentryCrash(fmt.Errorf(output)); err != nil { + apiCfg := cfg.GetAPIConfig() + clientID := apiCfg.ClientID + appVersion := apiCfg.AppVersion + if err := sentry.ReportSentryCrash(clientID, appVersion, pmapi.CurrentUserAgent, errors.New(output)); err != nil { log.Error("Sentry crash report failed: ", err) } } diff --git a/pkg/pmapi/auth.go b/pkg/pmapi/auth.go index 6b3976d5..25a2ba6b 100644 --- a/pkg/pmapi/auth.go +++ b/pkg/pmapi/auth.go @@ -494,7 +494,10 @@ func (c *client) DeleteAuth() (err error) { return } -// TODO: Need a method like IsConnected() to be able to detect whether a client is logged in or not. +// IsConnected returns whether the client is authorized to access the API. +func (c *client) IsConnected() bool { + return c.uid != "" && c.accessToken != "" +} // ClearData clears sensitive data from the client. func (c *client) ClearData() { diff --git a/pkg/pmapi/auth_test.go b/pkg/pmapi/auth_test.go index e3a8bece..7a602cec 100644 --- a/pkg/pmapi/auth_test.go +++ b/pkg/pmapi/auth_test.go @@ -336,12 +336,7 @@ func TestClient_Logout(t *testing.T) { c.Logout() r.Eventually(t, func() bool { - // TODO: Use a method like IsConnected() which returns whether the client was logged out or not. - return c.accessToken == "" && - c.uid == "" && - c.kr == nil && - c.addresses == nil && - c.user == nil + return c.IsConnected() == false && c.kr == nil && c.addresses == nil && c.user == nil }, 10*time.Second, 10*time.Millisecond) } diff --git a/pkg/pmapi/client.go b/pkg/pmapi/client.go index 5cbf41eb..e371d03c 100644 --- a/pkg/pmapi/client.go +++ b/pkg/pmapi/client.go @@ -98,6 +98,7 @@ type Client interface { Auth2FA(twoFactorCode string, auth *Auth) (*Auth2FA, error) Logout() DeleteAuth() error + IsConnected() bool ClearData() CurrentUser() (*User, error) @@ -131,7 +132,6 @@ type Client interface { ReportBugWithEmailClient(os, osVersion, title, description, username, email, emailClient string) error SendSimpleMetric(category, action, label string) error - ReportSentryCrash(reportErr error) (err error) GetMailSettings() (MailSettings, error) GetContactEmailByEmail(string, int, int) ([]ContactEmail, error) diff --git a/pkg/pmapi/mocks/mocks.go b/pkg/pmapi/mocks/mocks.go index b06cc0e3..be46bd1f 100644 --- a/pkg/pmapi/mocks/mocks.go +++ b/pkg/pmapi/mocks/mocks.go @@ -418,6 +418,20 @@ func (mr *MockClientMockRecorder) Import(arg0 interface{}) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Import", reflect.TypeOf((*MockClient)(nil).Import), arg0) } +// IsConnected mocks base method +func (m *MockClient) IsConnected() bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsConnected") + ret0, _ := ret[0].(bool) + return ret0 +} + +// IsConnected indicates an expected call of IsConnected +func (mr *MockClientMockRecorder) IsConnected() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsConnected", reflect.TypeOf((*MockClient)(nil).IsConnected)) +} + // KeyRingForAddressID mocks base method func (m *MockClient) KeyRingForAddressID(arg0 string) *crypto.KeyRing { m.ctrl.T.Helper() @@ -531,20 +545,6 @@ func (mr *MockClientMockRecorder) ReportBugWithEmailClient(arg0, arg1, arg2, arg return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReportBugWithEmailClient", reflect.TypeOf((*MockClient)(nil).ReportBugWithEmailClient), arg0, arg1, arg2, arg3, arg4, arg5, arg6) } -// ReportSentryCrash mocks base method -func (m *MockClient) ReportSentryCrash(arg0 error) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ReportSentryCrash", arg0) - ret0, _ := ret[0].(error) - return ret0 -} - -// ReportSentryCrash indicates an expected call of ReportSentryCrash -func (mr *MockClientMockRecorder) ReportSentryCrash(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReportSentryCrash", reflect.TypeOf((*MockClient)(nil).ReportSentryCrash), arg0) -} - // SendMessage mocks base method func (m *MockClient) SendMessage(arg0 string, arg1 *pmapi.SendMessageReq) (*pmapi.Message, *pmapi.Message, error) { m.ctrl.T.Helper() diff --git a/pkg/pmapi/sentry.go b/pkg/sentry/report.go similarity index 86% rename from pkg/pmapi/sentry.go rename to pkg/sentry/report.go index 528950c7..a5bc478e 100644 --- a/pkg/pmapi/sentry.go +++ b/pkg/sentry/report.go @@ -15,7 +15,7 @@ // You should have received a copy of the GNU General Public License // along with ProtonMail Bridge. If not, see . -package pmapi +package sentry import ( "fmt" @@ -26,19 +26,20 @@ import ( "strings" "github.com/getsentry/raven-go" + log "github.com/sirupsen/logrus" ) const fileParseError = "[file parse error]" var isGoroutine = regexp.MustCompile("^goroutine [[:digit:]]+.*") //nolint[gochecknoglobals] -// SentryThreads implements standard sentry thread report. -type SentryThreads struct { +// Threads implements standard sentry thread report. +type Threads struct { Values []Thread `json:"values"` } // Class specifier. -func (s *SentryThreads) Class() string { return "threads" } +func (s *Threads) Class() string { return "threads" } // Thread wraps a single stacktrace. type Thread struct { @@ -49,7 +50,7 @@ type Thread struct { } // TraceAllRoutines traces all goroutines and saves them to the current object. -func (s *SentryThreads) TraceAllRoutines() { +func (s *Threads) TraceAllRoutines() { s.Values = []Thread{} goroutines := &strings.Builder{} _ = pprof.Lookup("goroutine").WriteTo(goroutines, 2) @@ -109,7 +110,7 @@ func (s *SentryThreads) TraceAllRoutines() { s.Values = append(s.Values, thread) } -func findPanicSender(s *SentryThreads, err error) string { +func findPanicSender(s *Threads, err error) string { out := "error nil" if err != nil { out = err.Error() @@ -146,28 +147,31 @@ func findPanicSender(s *SentryThreads, err error) string { } // ReportSentryCrash reports a sentry crash with stacktrace from all goroutines. -func (c *client) ReportSentryCrash(reportErr error) (err error) { +func ReportSentryCrash(clientID, appVersion, userAgent string, reportErr error) (err error) { if reportErr == nil { return } + tags := map[string]string{ "OS": runtime.GOOS, - "Client": c.cm.config.ClientID, - "Version": c.cm.config.AppVersion, - "UserAgent": CurrentUserAgent, - "UserID": c.userID, + "Client": clientID, + "Version": appVersion, + "UserAgent": userAgent, + "UserID": "", } - threads := &SentryThreads{} + threads := &Threads{} threads.TraceAllRoutines() errorWithFile := findPanicSender(threads, reportErr) packet := raven.NewPacket(errorWithFile, threads) eventID, ch := raven.Capture(packet, tags) + if err = <-ch; err == nil { - c.log.Warn("Reported error with id: ", eventID) + log.WithField("errorID", eventID).Warn("Reported sentry error") } else { - c.log.Errorf("Can not report `%s` due to `%s`", reportErr.Error(), err.Error()) + log.WithField("error", reportErr).WithError(err).Error("Failed to report sentry error") } + return err } diff --git a/pkg/pmapi/sentry_test.go b/pkg/sentry/report_test.go similarity index 86% rename from pkg/pmapi/sentry_test.go rename to pkg/sentry/report_test.go index efe946bd..4c902ee4 100644 --- a/pkg/pmapi/sentry_test.go +++ b/pkg/sentry/report_test.go @@ -15,7 +15,7 @@ // You should have received a copy of the GNU General Public License // along with ProtonMail Bridge. If not, see . -package pmapi +package sentry import ( "errors" @@ -25,14 +25,17 @@ import ( ) func TestSentryCrashReport(t *testing.T) { - cm := NewClientManager(testClientConfig) - c := cm.GetClient("bridgetest") - if err := c.ReportSentryCrash(errors.New("Testing crash report - api proxy; goroutines with threads, find origin")); err != nil { + if err := ReportSentryCrash( + "clientID", + "appVersion", + "useragent", + errors.New("Testing crash report - api proxy; goroutines with threads, find origin"), + ); err != nil { t.Fatal("Expected no error while report, but have", err) } } -func (s *SentryThreads) TraceAllRoutinesTest() { +func (s *Threads) TraceAllRoutinesTest() { s.Values = []Thread{ { ID: 0, diff --git a/test/fakeapi/auth.go b/test/fakeapi/auth.go index 428f51b7..2e42bef6 100644 --- a/test/fakeapi/auth.go +++ b/test/fakeapi/auth.go @@ -154,6 +154,10 @@ func (api *FakePMAPI) Logout() { api.ClearData() } +func (api *FakePMAPI) IsConnected() bool { + return api.uid != "" && api.lastToken != "" +} + func (api *FakePMAPI) DeleteAuth() error { if err := api.checkAndRecordCall(DELETE, "/auth", nil); err != nil { return err