From 75a5777e363aff5bd43de47820443bff4ff07c58 Mon Sep 17 00:00:00 2001 From: juanicabanas Date: Thu, 13 Oct 2022 11:32:32 -0300 Subject: [PATCH] PublicDashboards: Validate access token not to be duplicated and add retries. (#56755) PublicDashboards: Validate access token not to be duplicated and add retries. --- .../publicdashboards/database/database.go | 33 ++++++++++++++++++ .../publicdashboards/models/models.go | 4 +-- .../public_dashboard_store_mock.go | 34 ++++++++++++++++--- .../publicdashboards/publicdashboard.go | 1 + .../publicdashboards/service/service.go | 3 +- .../publicdashboards/service/service_test.go | 32 +++++++++++++++++ .../dashboard/api/publicDashboardApi.ts | 1 + 7 files changed, 99 insertions(+), 9 deletions(-) diff --git a/pkg/services/publicdashboards/database/database.go b/pkg/services/publicdashboards/database/database.go index a04d86250de..cbcfe6fd660 100644 --- a/pkg/services/publicdashboards/database/database.go +++ b/pkg/services/publicdashboards/database/database.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/publicdashboards" + "github.com/grafana/grafana/pkg/services/publicdashboards/internal/tokens" . "github.com/grafana/grafana/pkg/services/publicdashboards/models" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/sqlstore/migrator" @@ -137,6 +138,38 @@ func (d *PublicDashboardStoreImpl) GenerateNewPublicDashboardUid(ctx context.Con return uid, nil } +// Generates a new unique access token for a new public dashboard +func (d *PublicDashboardStoreImpl) GenerateNewPublicDashboardAccessToken(ctx context.Context) (string, error) { + var accessToken string + + err := d.sqlStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { + for i := 0; i < 3; i++ { + var err error + accessToken, err = tokens.GenerateAccessToken() + if err != nil { + continue + } + + exists, err := sess.Get(&PublicDashboard{AccessToken: accessToken}) + if err != nil { + return err + } + + if !exists { + return nil + } + } + + return ErrPublicDashboardFailedGenerateAccessToken + }) + + if err != nil { + return "", err + } + + return accessToken, nil +} + // Retrieves public dashboard configuration by Uid func (d *PublicDashboardStoreImpl) GetPublicDashboardByUid(ctx context.Context, uid string) (*PublicDashboard, error) { if uid == "" { diff --git a/pkg/services/publicdashboards/models/models.go b/pkg/services/publicdashboards/models/models.go index 68dedd7315a..79adb594b37 100644 --- a/pkg/services/publicdashboards/models/models.go +++ b/pkg/services/publicdashboards/models/models.go @@ -34,8 +34,8 @@ var ( Reason: "failed to generate unique public dashboard id", StatusCode: 500, } - ErrPublicDashboardFailedGenerateAccesstoken = PublicDashboardErr{ - Reason: "failed to public dashboard access token", + ErrPublicDashboardFailedGenerateAccessToken = PublicDashboardErr{ + Reason: "failed to create public dashboard", StatusCode: 500, } ErrPublicDashboardNotFound = PublicDashboardErr{ diff --git a/pkg/services/publicdashboards/public_dashboard_store_mock.go b/pkg/services/publicdashboards/public_dashboard_store_mock.go index 3daf07021b7..4cb15bef0fd 100644 --- a/pkg/services/publicdashboards/public_dashboard_store_mock.go +++ b/pkg/services/publicdashboards/public_dashboard_store_mock.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.12.1. DO NOT EDIT. +// Code generated by mockery v2.14.0. DO NOT EDIT. package publicdashboards @@ -9,8 +9,6 @@ import ( mock "github.com/stretchr/testify/mock" publicdashboardsmodels "github.com/grafana/grafana/pkg/services/publicdashboards/models" - - testing "testing" ) // FakePublicDashboardStore is an autogenerated mock type for the Store type @@ -39,6 +37,27 @@ func (_m *FakePublicDashboardStore) AccessTokenExists(ctx context.Context, acces return r0, r1 } +// GenerateNewPublicDashboardAccessToken provides a mock function with given fields: ctx +func (_m *FakePublicDashboardStore) GenerateNewPublicDashboardAccessToken(ctx context.Context) (string, error) { + ret := _m.Called(ctx) + + var r0 string + if rf, ok := ret.Get(0).(func(context.Context) string); ok { + r0 = rf(ctx) + } else { + r0 = ret.Get(0).(string) + } + + var r1 error + if rf, ok := ret.Get(1).(func(context.Context) error); ok { + r1 = rf(ctx) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // GenerateNewPublicDashboardUid provides a mock function with given fields: ctx func (_m *FakePublicDashboardStore) GenerateNewPublicDashboardUid(ctx context.Context) (string, error) { ret := _m.Called(ctx) @@ -254,8 +273,13 @@ func (_m *FakePublicDashboardStore) UpdatePublicDashboardConfig(ctx context.Cont return r0 } -// NewFakePublicDashboardStore creates a new instance of FakePublicDashboardStore. It also registers the testing.TB interface on the mock and a cleanup function to assert the mocks expectations. -func NewFakePublicDashboardStore(t testing.TB) *FakePublicDashboardStore { +type mockConstructorTestingTNewFakePublicDashboardStore interface { + mock.TestingT + Cleanup(func()) +} + +// NewFakePublicDashboardStore creates a new instance of FakePublicDashboardStore. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func NewFakePublicDashboardStore(t mockConstructorTestingTNewFakePublicDashboardStore) *FakePublicDashboardStore { mock := &FakePublicDashboardStore{} mock.Mock.Test(t) diff --git a/pkg/services/publicdashboards/publicdashboard.go b/pkg/services/publicdashboards/publicdashboard.go index 5e6f420b319..2da091d4cb2 100644 --- a/pkg/services/publicdashboards/publicdashboard.go +++ b/pkg/services/publicdashboards/publicdashboard.go @@ -32,6 +32,7 @@ type Store interface { AccessTokenExists(ctx context.Context, accessToken string) (bool, error) GenerateNewPublicDashboardUid(ctx context.Context) (string, error) GetDashboard(ctx context.Context, dashboardUid string) (*models.Dashboard, error) + GenerateNewPublicDashboardAccessToken(ctx context.Context) (string, error) GetPublicDashboard(ctx context.Context, accessToken string) (*PublicDashboard, *models.Dashboard, error) GetPublicDashboardByUid(ctx context.Context, uid string) (*PublicDashboard, error) GetPublicDashboardConfig(ctx context.Context, orgId int64, dashboardUid string) (*PublicDashboard, error) diff --git a/pkg/services/publicdashboards/service/service.go b/pkg/services/publicdashboards/service/service.go index 7e483bcf5ee..a298ec6528f 100644 --- a/pkg/services/publicdashboards/service/service.go +++ b/pkg/services/publicdashboards/service/service.go @@ -11,7 +11,6 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/publicdashboards" - "github.com/grafana/grafana/pkg/services/publicdashboards/internal/tokens" . "github.com/grafana/grafana/pkg/services/publicdashboards/models" "github.com/grafana/grafana/pkg/services/publicdashboards/queries" "github.com/grafana/grafana/pkg/services/publicdashboards/validation" @@ -153,7 +152,7 @@ func (pd *PublicDashboardServiceImpl) savePublicDashboardConfig(ctx context.Cont return "", err } - accessToken, err := tokens.GenerateAccessToken() + accessToken, err := pd.store.GenerateNewPublicDashboardAccessToken(ctx) if err != nil { return "", err } diff --git a/pkg/services/publicdashboards/service/service_test.go b/pkg/services/publicdashboards/service/service_test.go index af98c3687c5..42d4979963a 100644 --- a/pkg/services/publicdashboards/service/service_test.go +++ b/pkg/services/publicdashboards/service/service_test.go @@ -216,6 +216,38 @@ func TestSavePublicDashboard(t *testing.T) { _, err := service.SavePublicDashboardConfig(context.Background(), SignedInUser, dto) require.Error(t, err) }) + + t.Run("Pubdash access token generation throws an error and pubdash is not persisted", func(t *testing.T) { + dashboard := models.NewDashboard("testDashie") + + publicDashboardStore := &FakePublicDashboardStore{} + publicDashboardStore.On("GetDashboard", mock.Anything, mock.Anything).Return(dashboard, nil) + publicDashboardStore.On("GetPublicDashboardByUid", mock.Anything, mock.Anything).Return(nil, nil) + publicDashboardStore.On("GenerateNewPublicDashboardUid", mock.Anything).Return("an-uid", nil) + publicDashboardStore.On("GenerateNewPublicDashboardAccessToken", mock.Anything).Return("", ErrPublicDashboardFailedGenerateAccessToken) + + service := &PublicDashboardServiceImpl{ + log: log.New("test.logger"), + store: publicDashboardStore, + } + + dto := &SavePublicDashboardConfigDTO{ + DashboardUid: "an-id", + OrgId: 8, + UserId: 7, + PublicDashboard: &PublicDashboard{ + IsEnabled: true, + DashboardUid: "NOTTHESAME", + OrgId: 9999999, + }, + } + + _, err := service.SavePublicDashboardConfig(context.Background(), SignedInUser, dto) + + require.Error(t, err) + require.Equal(t, err, ErrPublicDashboardFailedGenerateAccessToken) + publicDashboardStore.AssertNotCalled(t, "SavePublicDashboardConfig") + }) } func TestUpdatePublicDashboard(t *testing.T) { diff --git a/public/app/features/dashboard/api/publicDashboardApi.ts b/public/app/features/dashboard/api/publicDashboardApi.ts index 92ced69c517..c61ccdf7695 100644 --- a/public/app/features/dashboard/api/publicDashboardApi.ts +++ b/public/app/features/dashboard/api/publicDashboardApi.ts @@ -38,6 +38,7 @@ export const publicDashboardApi = createApi({ method: 'POST', data: params.payload, }), + extraOptions: { maxRetries: 0 }, async onQueryStarted({ dashboard, payload }, { dispatch, queryFulfilled }) { const { data } = await queryFulfilled; dispatch(notifyApp(createSuccessNotification('Dashboard sharing configuration saved')));