From d7b425c514af041f0bf1e7e99521996773d82693 Mon Sep 17 00:00:00 2001 From: jbhusson <jonah@elixxir.io> Date: Mon, 10 Feb 2020 15:31:40 -0800 Subject: [PATCH] Fixes from ben's code review --- cmd/root.go | 67 +++++++++++++++++------------ firebase/fcm.go | 26 ++++++----- firebase/fcm_test.go | 5 +-- notifications/notifications.go | 57 ++++++++++++------------ notifications/notifications_test.go | 45 +++++++------------ notifications/updateNDF.go | 4 +- 6 files changed, 99 insertions(+), 105 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index bd6a917..5daf98b 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -11,6 +11,7 @@ package cmd import ( "fmt" "github.com/mitchellh/go-homedir" + "github.com/pkg/errors" "github.com/spf13/cobra" jww "github.com/spf13/jwalterweatherman" "github.com/spf13/viper" @@ -18,9 +19,11 @@ import ( "gitlab.com/elixxir/notifications-bot/notifications" "gitlab.com/elixxir/notifications-bot/storage" "gitlab.com/elixxir/primitives/id" + "gitlab.com/elixxir/primitives/ndf" "gitlab.com/elixxir/primitives/utils" "os" "path" + "strings" ) var ( @@ -54,19 +57,20 @@ var rootCmd = &cobra.Command{ certPath := viper.GetString("certPath") keyPath := viper.GetString("keyPath") localAddress := fmt.Sprintf("0.0.0.0:%d", viper.GetInt("port")) + fbCreds := viper.GetString("firebaseCredentialsPath") // Populate params NotificationParams = notifications.Params{ Address: localAddress, CertPath: certPath, KeyPath: keyPath, + FBCreds: fbCreds, } jww.INFO.Println("Starting Notifications...") - impl, err := notifications.StartNotifications(NotificationParams, noTLS) + impl, err := notifications.StartNotifications(NotificationParams, noTLS, false) if err != nil { - err = fmt.Errorf("Failed to start notifications server: %+v", err) - panic(err) + jww.FATAL.Panicf("Failed to start notifications server: %+v", err) } impl.Storage = storage.NewDatabase( @@ -76,41 +80,48 @@ var rootCmd = &cobra.Command{ viper.GetString("dbAddress"), ) - permissioningAddr := viper.GetString("permissioningAddress") - permissioningCertPath := viper.GetString("permissioningCertPath") - cert, err := utils.ReadFile(permissioningCertPath) + err = setupConnection(impl, viper.GetString("permissioningCertPath"), viper.GetString("permissioningAddress")) if err != nil { - jww.FATAL.Panicf("Could not read permissioning cert: %+v", err) - } - - _, err = impl.Comms.AddHost(id.PERMISSIONING, permissioningAddr, cert, true, true) - if err != nil { - jww.FATAL.Panicf("Failed to Create permissioning host: %+v", err) - } - Setup: - ndf, err := notifications.PollNdf(nil, impl.Comms) - if err != nil { - jww.FATAL.Panicf("Failed to get NDF: %+v", err) - } - - err = impl.UpdateNdf(ndf) - if err != nil { - jww.FATAL.Panicf("Failed to update impl's NDF: %+v", err) + jww.FATAL.Panicf("Failed to set up connections: %+v", err) } // Start notification loop killChan := make(chan struct{}) errChan := make(chan error) - go impl.RunNotificationLoop(viper.GetString("firebaseCredentialsPath"), - loopDelay, killChan, errChan) + go impl.RunNotificationLoop(loopDelay, killChan, errChan) // Wait forever to prevent process from ending err = <-errChan - jww.ERROR.Println(err) - goto Setup + panic(err) }, } +func setupConnection(impl *notifications.Impl, permissioningCertPath, permissioningAddr string) error { + cert, err := utils.ReadFile(permissioningCertPath) + if err != nil { + jww.FATAL.Panicf("Could not read permissioning cert: %+v", err) + } + + _, err = impl.Comms.AddHost(id.PERMISSIONING, permissioningAddr, cert, true, true) + if err != nil { + jww.FATAL.Panicf("Failed to Create permissioning host: %+v", err) + } + + var def *ndf.NetworkDefinition + for def == nil { + def, err = notifications.PollNdf(nil, impl.Comms) + if err != nil && !strings.Contains(err.Error(), ndf.NO_NDF) { + return errors.Wrap(err, "Failed to get NDF") + } + } + + err = impl.UpdateNdf(def) + if err != nil { + return errors.Wrap(err, "Failed to update impl's NDF") + } + return nil +} + // Execute adds all child commands to the root command and sets flags // appropriately. This is called by main.main(). It only needs to // happen once to the rootCmd. @@ -142,8 +153,8 @@ func init() { rootCmd.Flags().BoolVar(&noTLS, "noTLS", false, "Runs without TLS enabled") - rootCmd.Flags().IntVarP(&loopDelay, "loopDelay", "", 5, - "Set the delay between notification loops (in seconds)") + rootCmd.Flags().IntVarP(&loopDelay, "loopDelay", "", 500, + "Set the delay between notification loops (in milliseconds)") // Bind config and command line flags of the same name err := viper.BindPFlag("verbose", rootCmd.Flags().Lookup("verbose")) diff --git a/firebase/fcm.go b/firebase/fcm.go index 5a05e3f..665e50a 100644 --- a/firebase/fcm.go +++ b/firebase/fcm.go @@ -19,13 +19,12 @@ import ( // function types for use in notificationsbot struct type SetupFunc func(string) (*messaging.Client, context.Context, error) -type SendFunc func(FBSender, context.Context, string) (string, error) +type SendFunc func(FBSender, string) (string, error) // FirebaseComm is a struct which holds the functions to setup the messaging app and sending notifications // Using a struct in this manner allows us to properly unit test the NotifyUser function type FirebaseComm struct { - SetupMessagingApp SetupFunc - SendNotification SendFunc + SendNotification SendFunc } // This interface matches the send function in the messaging app, allowing us to unit test sendNotification @@ -36,44 +35,43 @@ type FBSender interface { // Set up a notificationbot object with the proper setup and send functions func NewFirebaseComm() *FirebaseComm { return &FirebaseComm{ - SetupMessagingApp: setupMessagingApp, - SendNotification: sendNotification, + SendNotification: sendNotification, } } // FOR TESTING USE ONLY: setup a notificationbot object with mocked setup and send funcs -func NewMockFirebaseComm(t *testing.T, setupFunc SetupFunc, sendFunc SendFunc) *FirebaseComm { +func NewMockFirebaseComm(t *testing.T, sendFunc SendFunc) *FirebaseComm { if t == nil { panic("This method should only be used in tests") } return &FirebaseComm{ - SetupMessagingApp: setupFunc, - SendNotification: sendFunc, + SendNotification: sendFunc, } } // setupApp is a helper function which sets up a connection with firebase // It returns a messaging client, a context object and an error -func setupMessagingApp(serviceKeyPath string) (*messaging.Client, context.Context, error) { +func SetupMessagingApp(serviceKeyPath string) (*messaging.Client, error) { ctx := context.Background() opt := option.WithCredentialsFile(serviceKeyPath) app, err := firebase.NewApp(context.Background(), nil, opt) if err != nil { - return nil, nil, errors.Errorf("Error initializing app: %v", err) + return nil, errors.Errorf("Error initializing app: %v", err) } client, err := app.Messaging(ctx) if err != nil { - return nil, nil, errors.Errorf("Error getting Messaging app: %+v", err) + return nil, errors.Errorf("Error getting Messaging app: %+v", err) } - return client, ctx, nil + return client, nil } // SendNotification accepts a registration token and service account file // It gets the proper infrastructure, then builds & sends a notification through the firebase admin API // returns string, error (string is of dubious use, but is returned for the time being) -func sendNotification(app FBSender, ctx context.Context, token string) (string, error) { +func sendNotification(app FBSender, token string) (string, error) { + ctx := context.Background() message := &messaging.Message{ Notification: &messaging.Notification{ Title: "xx Messenger", @@ -84,7 +82,7 @@ func sendNotification(app FBSender, ctx context.Context, token string) (string, resp, err := app.Send(ctx, message) if err != nil { - return "", errors.Errorf("Failed to send notification: %+v", err) + return "", errors.Wrap(err, "Failed to send notification") } return resp, nil } diff --git a/firebase/fcm_test.go b/firebase/fcm_test.go index c594362..dc9ec36 100644 --- a/firebase/fcm_test.go +++ b/firebase/fcm_test.go @@ -22,10 +22,9 @@ func (MockSender) Send(ctx context.Context, app *messaging.Message) (string, err // This tests the function which sends a notification to firebase. // Note: this requires you to have a valid token & service credentials func TestSendNotification(t *testing.T) { - ctx := context.Background() app := MockSender{} - _, err := sendNotification(app, ctx, token) + _, err := sendNotification(app, token) if err != nil { t.Error(err.Error()) } @@ -34,7 +33,7 @@ func TestSendNotification(t *testing.T) { // Unit test the NewFirebaseComm method func TestNewFirebaseComm(t *testing.T) { comm := NewFirebaseComm() - if comm.SendNotification == nil || comm.SetupMessagingApp == nil { + if comm.SendNotification == nil { t.Error("Failed to set functions in comm") } } diff --git a/notifications/notifications.go b/notifications/notifications.go index e329989..4715d94 100644 --- a/notifications/notifications.go +++ b/notifications/notifications.go @@ -10,6 +10,7 @@ package notifications import ( "crypto/x509" + "firebase.google.com/go/messaging" "github.com/pkg/errors" jww "github.com/spf13/jwalterweatherman" "gitlab.com/elixxir/comms/connect" @@ -27,7 +28,7 @@ import ( // Function type definitions for the main operations (poll and notify) type PollFunc func(*Impl) ([]string, error) -type NotifyFunc func(string, string, *firebase.FirebaseComm, storage.Storage) (string, error) +type NotifyFunc func(*messaging.Client, string, *firebase.FirebaseComm, storage.Storage) (string, error) // Params struct holds info passed in for configuration type Params struct { @@ -35,6 +36,7 @@ type Params struct { CertPath string KeyPath string PublicAddress string + FBCreds string } // Local impl for notifications; holds comms, storage object, creds and main functions @@ -47,6 +49,7 @@ type Impl struct { ndf *ndf.NetworkDefinition pollFunc PollFunc notifyFunc NotifyFunc + fcm *messaging.Client } // We use an interface here inorder to allow us to mock the getHost and RequestNDF in the notifcationsBot.Comms for testing @@ -59,36 +62,34 @@ type NotificationComms interface { // Main function for this repo accepts credentials and an impl // loops continuously, polling for notifications and notifying the relevant users -func (nb *Impl) RunNotificationLoop(fbCreds string, loopDuration int, killChan chan struct{}, errChan chan error) { +func (nb *Impl) RunNotificationLoop(loopDuration int, killChan chan struct{}, errChan chan error) { fc := firebase.NewFirebaseComm() for { // Stop execution if killed by channel select { case <-killChan: return - default: + case <-time.After(time.Millisecond * time.Duration(loopDuration)): } UIDs, err := nb.pollFunc(nb) if err != nil { - errChan <- errors.Errorf("Failed to poll gateway for users to notify: %+v", err) + errChan <- errors.Wrap(err, "Failed to poll gateway for users to notify") return } for _, id := range UIDs { - _, err := nb.notifyFunc(id, fbCreds, fc, nb.Storage) + _, err := nb.notifyFunc(nb.fcm, id, fc, nb.Storage) if err != nil { - errChan <- errors.Errorf("Failed to notify user with ID %+v: %+v", id, err) + errChan <- errors.Wrapf(err, "Failed to notify user with ID %+v", id) return } } - - time.Sleep(time.Second * time.Duration(loopDuration)) } } // StartNotifications creates an Impl from the information passed in -func StartNotifications(params Params, noTLS bool) (*Impl, error) { +func StartNotifications(params Params, noTLS, noFirebase bool) (*Impl, error) { impl := &Impl{} var cert, key []byte @@ -97,28 +98,25 @@ func StartNotifications(params Params, noTLS bool) (*Impl, error) { // Read in private key key, err = utils.ReadFile(params.KeyPath) if err != nil { - return nil, errors.Errorf("failed to read key at %+v: %+v", params.KeyPath, err) + return nil, errors.Wrapf(err, "failed to read key at %+v", params.KeyPath) } impl.notificationKey, err = rsa.LoadPrivateKeyFromPem(key) if err != nil { - return nil, errors.Errorf("Failed to parse notifications server key: %+v. "+ - "NotificationsKey is %+v", - err, impl.notificationKey) + return nil, errors.Wrapf(err, "Failed to parse notifications server key (%+v)", impl.notificationKey) } if !noTLS { // Read in TLS keys from files cert, err = utils.ReadFile(params.CertPath) if err != nil { - return nil, errors.Errorf("failed to read certificate at %+v: %+v", params.CertPath, err) + return nil, errors.Wrapf(err, "failed to read certificate at %+v", params.CertPath) } // Set globals for notification server impl.certFromFile = string(cert) impl.notificationCert, err = tls.LoadCertificate(string(cert)) if err != nil { - return nil, errors.Errorf("Failed to parse notifications server cert: %+v. "+ - "Notifications cert is %+v", - err, impl.notificationCert) + return nil, errors.Wrapf(err, "Failed to parse notifications server cert. "+ + "Notifications cert is %+v", impl.notificationCert) } } @@ -129,6 +127,14 @@ func StartNotifications(params Params, noTLS bool) (*Impl, error) { impl.Comms = notificationBot.StartNotificationBot(id.NOTIFICATION_BOT, params.PublicAddress, handler, cert, key) + if !noFirebase { + app, err := firebase.SetupMessagingApp(params.FBCreds) + if err != nil { + return nil, errors.Wrap(err, "Failed to setup firebase messaging app") + } + impl.fcm = app + } + return impl, nil } @@ -149,19 +155,14 @@ func NewImplementation(instance *Impl) *notificationBot.Implementation { // NotifyUser accepts a UID and service key file path. // It handles the logic involved in retrieving a user's token and sending the notification -func notifyUser(uid string, serviceKeyPath string, fc *firebase.FirebaseComm, db storage.Storage) (string, error) { +func notifyUser(fcm *messaging.Client, uid string, fc *firebase.FirebaseComm, db storage.Storage) (string, error) { u, err := db.GetUser(uid) if err != nil { jww.DEBUG.Printf("No registration found for user with ID %+v", uid) return "", nil } - app, ctx, err := fc.SetupMessagingApp(serviceKeyPath) - if err != nil { - return "", errors.Errorf("Failed to setup messaging app: %+v", err) - } - - resp, err := fc.SendNotification(app, ctx, u.Token) + resp, err := fc.SendNotification(fcm, u.Token) if err != nil { return "", errors.Errorf("Failed to send notification to user with ID %+v: %+v", uid, err) } @@ -178,7 +179,7 @@ func pollForNotifications(nb *Impl) (strings []string, e error) { users, err := nb.Comms.RequestNotifications(h) if err != nil { - return nil, errors.Errorf("Failed to retrieve notifications from gateway: %+v", err) + return nil, errors.Wrap(err, "Failed to retrieve notifications from gateway") } return users.IDs, nil @@ -193,7 +194,7 @@ func (nb *Impl) RegisterForNotifications(clientToken []byte, auth *connect.Auth) } err := nb.Storage.UpsertUser(u) if err != nil { - return errors.Errorf("Failed to register user with notifications: %+v", err) + return errors.Wrap(err, "Failed to register user with notifications") } return nil } @@ -202,7 +203,7 @@ func (nb *Impl) RegisterForNotifications(clientToken []byte, auth *connect.Auth) func (nb *Impl) UnregisterForNotifications(auth *connect.Auth) error { err := nb.Storage.DeleteUser(auth.Sender.GetId()) if err != nil { - return errors.Errorf("Failed to unregister user with notifications: %+v", err) + return errors.Wrap(err, "Failed to unregister user with notifications") } return nil } @@ -211,7 +212,7 @@ func (nb *Impl) UpdateNdf(ndf *ndf.NetworkDefinition) error { gw := ndf.Gateways[len(ndf.Gateways)-1] _, err := nb.Comms.AddHost("gw", gw.Address, []byte(gw.TlsCertificate), false, true) if err != nil { - return errors.Errorf("Failed to add gateway host from NDF: %+v", err) + return errors.Wrap(err, "Failed to add gateway host from NDF") } nb.ndf = ndf diff --git a/notifications/notifications_test.go b/notifications/notifications_test.go index 4ef079e..009d832 100644 --- a/notifications/notifications_test.go +++ b/notifications/notifications_test.go @@ -6,7 +6,6 @@ package notifications import ( - "context" "firebase.google.com/go/messaging" "github.com/pkg/errors" "gitlab.com/elixxir/comms/connect" @@ -28,7 +27,7 @@ func TestRunNotificationLoop(t *testing.T) { impl.pollFunc = func(*Impl) (i []string, e error) { return []string{"test1", "test2"}, nil } - impl.notifyFunc = func(s3 string, s2 string, comm *firebase.FirebaseComm, storage storage.Storage) (s string, e error) { + impl.notifyFunc = func(fcm *messaging.Client, s3 string, comm *firebase.FirebaseComm, storage storage.Storage) (s string, e error) { return "good", nil } killChan := make(chan struct{}) @@ -37,42 +36,27 @@ func TestRunNotificationLoop(t *testing.T) { time.Sleep(10 * time.Second) killChan <- struct{}{} }() - impl.RunNotificationLoop("", 3, killChan, errChan) + impl.RunNotificationLoop(3, killChan, errChan) } // Test notificationbot's notifyuser function // this mocks the setup and send functions, and only tests the core logic of this function func TestNotifyUser(t *testing.T) { - badsetup := func(string) (*messaging.Client, context.Context, error) { - ctx := context.Background() - return &messaging.Client{}, ctx, errors.New("Failed") - } - setup := func(string) (*messaging.Client, context.Context, error) { - ctx := context.Background() - return &messaging.Client{}, ctx, nil - } - badsend := func(firebase.FBSender, context.Context, string) (string, error) { + badsend := func(firebase.FBSender, string) (string, error) { return "", errors.New("Failed") } - send := func(firebase.FBSender, context.Context, string) (string, error) { + send := func(firebase.FBSender, string) (string, error) { return "", nil } - fc_badsetup := firebase.NewMockFirebaseComm(t, badsetup, send) - fc_badsend := firebase.NewMockFirebaseComm(t, setup, badsend) - fc := firebase.NewMockFirebaseComm(t, setup, send) - - _, err := notifyUser("test", "testpath", fc_badsetup, testutil.MockStorage{}) - if err == nil { - t.Error("Should have returned an error") - return - } + fc_badsend := firebase.NewMockFirebaseComm(t, badsend) + fc := firebase.NewMockFirebaseComm(t, send) - _, err = notifyUser("test", "testpath", fc_badsend, testutil.MockStorage{}) + _, err := notifyUser(nil, "test", fc_badsend, testutil.MockStorage{}) if err == nil { t.Errorf("Should have returned an error") } - _, err = notifyUser("test", "testpath", fc, testutil.MockStorage{}) + _, err = notifyUser(nil, "test", fc, testutil.MockStorage{}) if err != nil { t.Errorf("Failed to notify user properly") } @@ -92,31 +76,31 @@ func TestStartNotifications(t *testing.T) { PublicAddress: "0.0.0.0:42010", } - n, err := StartNotifications(params, false) + n, err := StartNotifications(params, false, true) if err == nil || !strings.Contains(err.Error(), "failed to read key at") { t.Errorf("Should have thrown an error for no key path") } params.KeyPath = wd + "/../testutil/badkey" - n, err = StartNotifications(params, false) + n, err = StartNotifications(params, false, true) if err == nil || !strings.Contains(err.Error(), "Failed to parse notifications server key") { t.Errorf("Should have thrown an error bad key") } params.KeyPath = wd + "/../testutil/cmix.rip.key" - n, err = StartNotifications(params, false) + n, err = StartNotifications(params, false, true) if err == nil || !strings.Contains(err.Error(), "failed to read certificate at") { t.Errorf("Should have thrown an error for no cert path") } params.CertPath = wd + "/../testutil/badkey" - n, err = StartNotifications(params, false) + n, err = StartNotifications(params, false, true) if err == nil || !strings.Contains(err.Error(), "Failed to parse notifications server cert") { t.Errorf("Should have thrown an error for bad certificate") } params.CertPath = wd + "/../testutil/cmix.rip.crt" - n, err = StartNotifications(params, false) + n, err = StartNotifications(params, false, true) if err != nil { t.Errorf("Failed to start notifications successfully: %+v", err) } @@ -256,7 +240,8 @@ func getNewImpl() *Impl { KeyPath: wd + "/../testutil/cmix.rip.key", CertPath: wd + "/../testutil/cmix.rip.crt", PublicAddress: "0.0.0.0:0", + FBCreds: "", } - instance, _ := StartNotifications(params, false) + instance, _ := StartNotifications(params, false, true) return instance } diff --git a/notifications/updateNDF.go b/notifications/updateNDF.go index 770e199..d858898 100644 --- a/notifications/updateNDF.go +++ b/notifications/updateNDF.go @@ -32,7 +32,7 @@ func PollNdf(currentDef *ndf.NetworkDefinition, comms NotificationComms) (*ndf.N } //Put the hash in a message - msg := &pb.NDFHash{Hash: ndfHash} + msg := &pb.NDFHash{Hash: ndfHash} // TODO: this should be a helper somewhere regHost, ok := comms.GetHost(id.PERMISSIONING) if !ok { @@ -42,7 +42,7 @@ func PollNdf(currentDef *ndf.NetworkDefinition, comms NotificationComms) (*ndf.N //Send the hash to registration response, err := comms.RequestNdf(regHost, msg) if err != nil { - errMsg := errors.Errorf("Failed to get ndf from permissioning: %v", err) + errMsg := errors.Wrap(err, "Failed to get ndf from permissioning") if strings.Contains(errMsg.Error(), noNDFErr.Error()) { jww.WARN.Println("Continuing without an updated NDF") return nil, nil -- GitLab