From 4f21ecf982c91959672e148f87664eb9b43aef48 Mon Sep 17 00:00:00 2001 From: Matheus Macabu Date: Tue, 17 Sep 2024 08:59:47 +0200 Subject: [PATCH] CloudMigrations: improve nil handling (#93257) * CloudMigrations: fail token decryption if session is not found or without a token * CloudMigrations: do not report event if session is nil --- .../cloudmigrationimpl/cloudmigration.go | 17 ++- .../cloudmigrationimpl/cloudmigration_test.go | 100 +++++++++++++++++- .../cloudmigrationimpl/xorm_store.go | 18 +++- .../cloudmigrationimpl/xorm_store_test.go | 47 ++++++++ 4 files changed, 172 insertions(+), 10 deletions(-) diff --git a/pkg/services/cloudmigration/cloudmigrationimpl/cloudmigration.go b/pkg/services/cloudmigration/cloudmigrationimpl/cloudmigration.go index 6a8fdfeb02e..0ffe63e1fe1 100644 --- a/pkg/services/cloudmigration/cloudmigrationimpl/cloudmigration.go +++ b/pkg/services/cloudmigration/cloudmigrationimpl/cloudmigration.go @@ -351,10 +351,10 @@ func (s *Service) GetSession(ctx context.Context, uid string) (*cloudmigration.C func (s *Service) GetSessionList(ctx context.Context) (*cloudmigration.CloudMigrationSessionListResponse, error) { values, err := s.store.GetCloudMigrationSessionList(ctx) if err != nil { - return nil, err + return nil, fmt.Errorf("retrieving session list from store: %w", err) } - migrations := make([]cloudmigration.CloudMigrationSessionResponse, 0) + migrations := make([]cloudmigration.CloudMigrationSessionResponse, 0, len(values)) for _, v := range values { migrations = append(migrations, cloudmigration.CloudMigrationSessionResponse{ UID: v.UID, @@ -473,7 +473,7 @@ func (s *Service) DeleteSession(ctx context.Context, sessionUID string) (*cloudm session, snapshots, err := s.store.DeleteMigrationSessionByUID(ctx, sessionUID) if err != nil { s.report(ctx, session, gmsclient.EventDisconnect, 0, err) - return nil, fmt.Errorf("deleting migration from db: %w", err) + return nil, fmt.Errorf("deleting migration from db for session %v: %w", sessionUID, err) } err = s.deleteLocalFiles(snapshots) @@ -755,6 +755,17 @@ func (s *Service) report( return } + if sess == nil { + errMessage := "session not found" + if evtErr != nil { + errMessage = evtErr.Error() + } + + s.log.Error("failed to report event", "type", t, "error", errMessage) + + return + } + e := gmsclient.EventRequestDTO{ Event: t, LocalID: id, diff --git a/pkg/services/cloudmigration/cloudmigrationimpl/cloudmigration_test.go b/pkg/services/cloudmigration/cloudmigrationimpl/cloudmigration_test.go index 5b11be81dd1..dbfb688f5cd 100644 --- a/pkg/services/cloudmigration/cloudmigrationimpl/cloudmigration_test.go +++ b/pkg/services/cloudmigration/cloudmigrationimpl/cloudmigration_test.go @@ -137,8 +137,15 @@ func Test_GetSnapshotStatusFromGMS(t *testing.T) { s.gmsClient = gmsClientMock // Insert a session and snapshot into the database before we start - sess, err := s.store.CreateMigrationSession(context.Background(), cloudmigration.CloudMigrationSession{}) + createTokenResp, err := s.CreateToken(context.Background()) + assert.NoError(t, err) + assert.NotEmpty(t, createTokenResp.Token) + + sess, err := s.store.CreateMigrationSession(context.Background(), cloudmigration.CloudMigrationSession{ + AuthToken: createTokenResp.Token, + }) require.NoError(t, err) + uid, err := s.store.CreateSnapshot(context.Background(), cloudmigration.CloudMigrationSnapshot{ UID: "test uid", SessionUID: sess.UID, @@ -306,8 +313,15 @@ func Test_OnlyQueriesStatusFromGMSWhenRequired(t *testing.T) { s.gmsClient = gmsClientMock // Insert a snapshot into the database before we start - sess, err := s.store.CreateMigrationSession(context.Background(), cloudmigration.CloudMigrationSession{}) + createTokenResp, err := s.CreateToken(context.Background()) + assert.NoError(t, err) + assert.NotEmpty(t, createTokenResp.Token) + + sess, err := s.store.CreateMigrationSession(context.Background(), cloudmigration.CloudMigrationSession{ + AuthToken: createTokenResp.Token, + }) require.NoError(t, err) + uid, err := s.store.CreateSnapshot(context.Background(), cloudmigration.CloudMigrationSnapshot{ UID: uuid.NewString(), SessionUID: sess.UID, @@ -416,7 +430,13 @@ func Test_NonCoreDataSourcesHaveWarning(t *testing.T) { s := setUpServiceTest(t, false).(*Service) // Insert a processing snapshot into the database before we start so we query GMS - sess, err := s.store.CreateMigrationSession(context.Background(), cloudmigration.CloudMigrationSession{}) + createTokenResp, err := s.CreateToken(context.Background()) + assert.NoError(t, err) + assert.NotEmpty(t, createTokenResp.Token) + + sess, err := s.store.CreateMigrationSession(context.Background(), cloudmigration.CloudMigrationSession{ + AuthToken: createTokenResp.Token, + }) require.NoError(t, err) snapshotUid, err := s.store.CreateSnapshot(context.Background(), cloudmigration.CloudMigrationSnapshot{ UID: uuid.NewString(), @@ -528,6 +548,80 @@ func Test_NonCoreDataSourcesHaveWarning(t *testing.T) { assert.Equal(t, uninstalledAltered.Error, "Only core data sources are supported. Please ensure the plugin is installed on the cloud stack.") } +func TestDeleteSession(t *testing.T) { + s := setUpServiceTest(t, false).(*Service) + + t.Run("when deleting a session that does not exist in the database, it returns an error", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + session, err := s.DeleteSession(ctx, "invalid-session-uid") + require.Nil(t, session) + require.Error(t, err) + }) + + t.Run("when deleting an existing session, it returns the deleted session and no error", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + createTokenResp, err := s.CreateToken(ctx) + require.NoError(t, err) + require.NotEmpty(t, createTokenResp.Token) + + cmd := cloudmigration.CloudMigrationSessionRequest{ + AuthToken: createTokenResp.Token, + } + + createResp, err := s.CreateSession(ctx, cmd) + require.NoError(t, err) + require.NotEmpty(t, createResp.UID) + require.NotEmpty(t, createResp.Slug) + + deletedSession, err := s.DeleteSession(ctx, createResp.UID) + require.NoError(t, err) + require.NotNil(t, deletedSession) + require.Equal(t, deletedSession.UID, createResp.UID) + + notFoundSession, err := s.GetSession(ctx, deletedSession.UID) + require.ErrorIs(t, err, cloudmigration.ErrMigrationNotFound) + require.Nil(t, notFoundSession) + }) +} + +func TestReportEvent(t *testing.T) { + t.Run("when the session is nil, it does not report the event", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + gmsMock := &gmsClientMock{} + + s := setUpServiceTest(t, false).(*Service) + s.gmsClient = gmsMock + + require.NotPanics(t, func() { + s.report(ctx, nil, gmsclient.EventConnect, time.Minute, nil) + }) + + require.Zero(t, gmsMock.reportEventCalled) + }) + + t.Run("when the session is not nil, it reports the event", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + gmsMock := &gmsClientMock{} + + s := setUpServiceTest(t, false).(*Service) + s.gmsClient = gmsMock + + require.NotPanics(t, func() { + s.report(ctx, &cloudmigration.CloudMigrationSession{}, gmsclient.EventConnect, time.Minute, nil) + }) + + require.Equal(t, 1, gmsMock.reportEventCalled) + }) +} + func ctxWithSignedInUser() context.Context { c := &contextmodel.ReqContext{ SignedInUser: &user.SignedInUser{OrgID: 1}, diff --git a/pkg/services/cloudmigration/cloudmigrationimpl/xorm_store.go b/pkg/services/cloudmigration/cloudmigrationimpl/xorm_store.go index d1a07ed5aac..093c43ad8bc 100644 --- a/pkg/services/cloudmigration/cloudmigrationimpl/xorm_store.go +++ b/pkg/services/cloudmigration/cloudmigrationimpl/xorm_store.go @@ -45,7 +45,7 @@ func (ss *sqlStore) GetMigrationSessionByUID(ctx context.Context, uid string) (* } if err := ss.decryptToken(ctx, &cm); err != nil { - return &cm, err + return nil, fmt.Errorf("decrypting token: %w", err) } return &cm, err @@ -69,7 +69,7 @@ func (ss *sqlStore) CreateMigrationRun(ctx context.Context, cmr cloudmigration.C func (ss *sqlStore) CreateMigrationSession(ctx context.Context, migration cloudmigration.CloudMigrationSession) (*cloudmigration.CloudMigrationSession, error) { if err := ss.encryptToken(ctx, &migration); err != nil { - return nil, err + return nil, fmt.Errorf("encrypting token: %w", err) } err := ss.db.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { @@ -101,8 +101,9 @@ func (ss *sqlStore) GetCloudMigrationSessionList(ctx context.Context) ([]*cloudm for i := 0; i < len(migrations); i++ { m := migrations[i] + if err := ss.decryptToken(ctx, m); err != nil { - return migrations, err + return nil, fmt.Errorf("decrypting token: %w", err) } } @@ -169,7 +170,7 @@ func (ss *sqlStore) DeleteMigrationSessionByUID(ctx context.Context, uid string) } if err := ss.decryptToken(ctx, &c); err != nil { - return &c, snapshots, err + return nil, nil, fmt.Errorf("decrypting token: %w", err) } return &c, snapshots, nil @@ -474,6 +475,14 @@ func (ss *sqlStore) encryptToken(ctx context.Context, cm *cloudmigration.CloudMi } func (ss *sqlStore) decryptToken(ctx context.Context, cm *cloudmigration.CloudMigrationSession) error { + if cm == nil { + return cloudmigration.ErrMigrationNotFound + } + + if len(cm.AuthToken) == 0 { + return cloudmigration.ErrTokenNotFound + } + decoded, err := base64.StdEncoding.DecodeString(cm.AuthToken) if err != nil { return fmt.Errorf("token could not be decoded") @@ -483,6 +492,7 @@ func (ss *sqlStore) decryptToken(ctx context.Context, cm *cloudmigration.CloudMi if err != nil { return fmt.Errorf("decrypting auth token: %w", err) } + cm.AuthToken = string(t) return nil diff --git a/pkg/services/cloudmigration/cloudmigrationimpl/xorm_store_test.go b/pkg/services/cloudmigration/cloudmigrationimpl/xorm_store_test.go index c1a12e2d584..6f21c20dab2 100644 --- a/pkg/services/cloudmigration/cloudmigrationimpl/xorm_store_test.go +++ b/pkg/services/cloudmigration/cloudmigrationimpl/xorm_store_test.go @@ -325,6 +325,53 @@ func TestGetSnapshotList(t *testing.T) { }) } +func TestDecryptToken(t *testing.T) { + t.Parallel() + + _, s := setUpTest(t) + ctx := context.Background() + + t.Run("with an nil session, it returns a `migration not found` error", func(t *testing.T) { + t.Parallel() + + var cm *cloudmigration.CloudMigrationSession + + require.ErrorIs(t, s.decryptToken(ctx, cm), cloudmigration.ErrMigrationNotFound) + }) + + t.Run("with an empty auth token, it returns a `token not found` error", func(t *testing.T) { + t.Parallel() + + var cm cloudmigration.CloudMigrationSession + + require.ErrorIs(t, s.decryptToken(ctx, &cm), cloudmigration.ErrTokenNotFound) + }) + + t.Run("with an invalid base64 auth token, it returns an error", func(t *testing.T) { + t.Parallel() + + cm := cloudmigration.CloudMigrationSession{ + AuthToken: "invalid-base64-", + } + + require.Error(t, s.decryptToken(ctx, &cm)) + }) + + t.Run("with a valid base64 auth token, it decrypts it and overrides the auth token field", func(t *testing.T) { + t.Parallel() + + rawAuthToken := "raw-and-fake" + encodedAuthToken := base64.StdEncoding.EncodeToString([]byte(rawAuthToken)) + + cm := cloudmigration.CloudMigrationSession{ + AuthToken: encodedAuthToken, + } + + require.NoError(t, s.decryptToken(ctx, &cm)) + require.Equal(t, rawAuthToken, cm.AuthToken) + }) +} + func setUpTest(t *testing.T) (*sqlstore.SQLStore, *sqlStore) { testDB := db.InitTestDB(t) s := &sqlStore{