From 1767ac647ae0876ceb44232519111fc2dd5726bf Mon Sep 17 00:00:00 2001 From: "grafana-delivery-bot[bot]" <132647405+grafana-delivery-bot[bot]@users.noreply.github.com> Date: Wed, 3 Jan 2024 14:16:03 +0100 Subject: [PATCH] [v10.2.x] AccessControl: Add safety valve truncation for long user defined scopes (#79879) AccessControl: Add safety valve truncation for long user defined scopes (#79854) * fix migrator bootloop by invalidating permissions * add test for scope truncation * lint * fix max size scope (cherry picked from commit 3bcde852acf91739a41014fe3179603540d89247) Co-authored-by: Jo --- .../accesscontrol/migrator/migrator.go | 17 +++++++++ .../accesscontrol/migrator/migrator_test.go | 35 +++++++++++++++---- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/pkg/services/accesscontrol/migrator/migrator.go b/pkg/services/accesscontrol/migrator/migrator.go index 0baaf2dc945..8ecc20ad553 100644 --- a/pkg/services/accesscontrol/migrator/migrator.go +++ b/pkg/services/accesscontrol/migrator/migrator.go @@ -15,6 +15,10 @@ var ( batchSize = 1000 ) +const ( + maxLen = 40 +) + func MigrateScopeSplit(db db.DB, log log.Logger) error { t := time.Now() ctx := context.Background() @@ -49,6 +53,12 @@ func MigrateScopeSplit(db db.DB, log log.Logger) error { for i := start; i < end; i++ { kind, attribute, identifier := permissions[i].SplitScope() + // Trim to max length to avoid bootloop. + // too long scopes will be truncated and the permission will become invalid. + kind = trimToMaxLen(kind, maxLen) + attribute = trimToMaxLen(attribute, maxLen) + identifier = trimToMaxLen(identifier, maxLen) + delQuery += "?," delArgs = append(delArgs, permissions[i].ID) @@ -109,3 +119,10 @@ func batch(count, batchSize int, eachFn func(start, end int) error) error { return nil } + +func trimToMaxLen(s string, maxLen int) string { + if len(s) > maxLen { + return s[:maxLen] + } + return s +} diff --git a/pkg/services/accesscontrol/migrator/migrator_test.go b/pkg/services/accesscontrol/migrator/migrator_test.go index d09962e7709..779bc399fa3 100644 --- a/pkg/services/accesscontrol/migrator/migrator_test.go +++ b/pkg/services/accesscontrol/migrator/migrator_test.go @@ -3,14 +3,17 @@ package migrator import ( "context" "fmt" + "strings" "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/stretchr/testify/require" ) func batchInsertPermissions(cnt int, sqlStore db.DB) error { @@ -35,27 +38,45 @@ func batchInsertPermissions(cnt int, sqlStore db.DB) error { }) } -func TestMigrateScopeSplit(t *testing.T) { +// TestIntegrationMigrateScopeSplit tests the scope split migration +// also tests the scope split truncation logic +func TestIntegrationMigrateScopeSplitTruncation(t *testing.T) { sqlStore := db.InitTestDB(t) logger := log.New("accesscontrol.migrator.test") + batchSize = 20 // Populate permissions require.NoError(t, batchInsertPermissions(3*batchSize, sqlStore), "could not insert permissions") + // Insert a permission with a scope longer than 240 characters + longScope := strings.Repeat("a", 60) + ":" + strings.Repeat("b", 60) + ":" + strings.Repeat("c", 60) + permission := ac.Permission{ + RoleID: 1, + Action: "action", + Scope: longScope, + Created: time.Now(), + Updated: time.Now(), + } + require.NoError(t, sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + _, err := sess.Insert(permission) + return err + }), "could not insert permission with long scope") + // Migrate require.NoError(t, MigrateScopeSplit(sqlStore, logger)) // Check migration result - permissions := make([]ac.Permission, 0, 3*batchSize) + permissions := make([]ac.Permission, 0, 3*batchSize+1) errFind := sqlStore.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error { return sess.Find(&permissions) }) require.NoError(t, errFind, "could not find permissions in store") for i := range permissions { - require.Equal(t, fmt.Sprintf("resource:uid:%v", i+1), permissions[i].Scope, "scope should have been preserved") - require.Equal(t, "resource", permissions[i].Kind) - require.Equal(t, "uid", permissions[i].Attribute) - require.Equal(t, fmt.Sprintf("%v", i+1), permissions[i].Identifier) + if permissions[i].Scope == longScope { + assert.Equal(t, strings.Repeat("a", 40), permissions[i].Kind) + assert.Equal(t, strings.Repeat("b", 40), permissions[i].Attribute) + assert.Equal(t, strings.Repeat("c", 40), permissions[i].Identifier) + } } }