From 203f3bd9ae36a3a551a29ca484b3611621b57874 Mon Sep 17 00:00:00 2001 From: Nir Ozery Date: Tue, 23 May 2023 14:57:49 +0300 Subject: [PATCH 1/5] Migrations: Revert remove ACL migration --- cmd/lakefs/cmd/migrate.go | 28 +- cmd/lakefs/cmd/migrate_test.go | 110 ++++++ pkg/auth/base.go | 19 + pkg/kv/migration.go | 5 +- pkg/kv/migrations/import_permissions.go | 15 +- pkg/kv/migrations/migrations_test.go | 298 +++++++++++++++- pkg/kv/migrations/rbac_to_acl.go | 448 ++++++++++++++++++++++++ pkg/kv/migrations/utils.go | 16 + 8 files changed, 897 insertions(+), 42 deletions(-) create mode 100644 cmd/lakefs/cmd/migrate_test.go create mode 100644 pkg/kv/migrations/rbac_to_acl.go create mode 100644 pkg/kv/migrations/utils.go diff --git a/cmd/lakefs/cmd/migrate.go b/cmd/lakefs/cmd/migrate.go index f6216a97dfd..c37feac1536 100644 --- a/cmd/lakefs/cmd/migrate.go +++ b/cmd/lakefs/cmd/migrate.go @@ -10,6 +10,7 @@ import ( "github.com/treeverse/lakefs/pkg/config" "github.com/treeverse/lakefs/pkg/kv" "github.com/treeverse/lakefs/pkg/kv/migrations" + "github.com/treeverse/lakefs/pkg/logging" ) // migrateCmd represents the migrate command @@ -77,6 +78,8 @@ var upCmd = &cobra.Command{ } defer kvStore.Close() + force, _ := cmd.Flags().GetBool("force") + _, err = kv.ValidateSchemaVersion(ctx, kvStore) switch { case err == nil: @@ -85,7 +88,7 @@ var upCmd = &cobra.Command{ _, _ = fmt.Fprintf(os.Stderr, "%s\n", err) os.Exit(1) case errors.Is(err, kv.ErrMigrationRequired): - err = doMigration(ctx, kvStore, cfg) + err = DoMigration(ctx, kvStore, cfg, force) if err != nil { _, _ = fmt.Fprintf(os.Stderr, "Migration failed: %s\n", err) os.Exit(1) @@ -98,7 +101,7 @@ var upCmd = &cobra.Command{ }, } -func doMigration(ctx context.Context, kvStore kv.Store, cfg *config.Config) error { +func DoMigration(ctx context.Context, kvStore kv.Store, cfg *config.Config, force bool) error { var ( version int err error @@ -109,22 +112,15 @@ func doMigration(ctx context.Context, kvStore kv.Store, cfg *config.Config) erro return err } switch { - case version < kv.ACLNoReposMigrateVersion || version >= kv.NextSchemaVersion: + case version >= kv.NextSchemaVersion || version < kv.ACLMigrateVersion: return fmt.Errorf("wrong starting version %d: %w", version, kv.ErrMigrationVersion) - + case version < kv.ACLNoReposMigrateVersion: + err = migrations.MigrateToACL(ctx, kvStore, cfg, logging.Default(), version, force) case version < kv.ACLImportMigrateVersion: - // skip migrate to ACL for users with External authorizations - if !cfg.IsAuthUISimplified() { - fmt.Println("skipping ACL migration - external Authorization") - err = kv.SetDBSchemaVersion(ctx, kvStore, kv.ACLImportMigrateVersion) - if err != nil { - return fmt.Errorf("failed to upgrade version, to fix this re-run migration: %w", err) - } - } else { - if err = migrations.MigrateImportPermissions(ctx, kvStore); err != nil { - return err - } - } + err = migrations.MigrateImportPermissions(ctx, kvStore, cfg) + } + if err != nil { + return err } } return nil diff --git a/cmd/lakefs/cmd/migrate_test.go b/cmd/lakefs/cmd/migrate_test.go new file mode 100644 index 00000000000..3beacce20cf --- /dev/null +++ b/cmd/lakefs/cmd/migrate_test.go @@ -0,0 +1,110 @@ +package cmd_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "github.com/treeverse/lakefs/cmd/lakefs/cmd" + "github.com/treeverse/lakefs/pkg/config" + "github.com/treeverse/lakefs/pkg/kv" + "github.com/treeverse/lakefs/pkg/kv/kvtest" +) + +func TestDoMigrate(t *testing.T) { + ctx := context.Background() + + t.Run("not_initialized", func(t *testing.T) { + kvStore := kvtest.GetStore(ctx, t) + err := cmd.DoMigration(ctx, kvStore, nil, false) + require.ErrorIs(t, err, kv.ErrNotFound) + _, err = kv.GetDBSchemaVersion(ctx, kvStore) + require.ErrorIs(t, err, kv.ErrNotFound) + }) + + t.Run("not_meets_required_version", func(t *testing.T) { + kvStore := kvtest.GetStore(ctx, t) + require.NoError(t, kv.SetDBSchemaVersion(ctx, kvStore, kv.InitialMigrateVersion)) + err := cmd.DoMigration(ctx, kvStore, nil, false) + require.ErrorIs(t, err, kv.ErrMigrationVersion) + version, err := kv.GetDBSchemaVersion(ctx, kvStore) + require.NoError(t, err) + require.Equal(t, kv.InitialMigrateVersion, version) + }) + + t.Run("from_acl_v1_no_force", func(t *testing.T) { + cfg := config.Config{} + cfg.Auth.UIConfig.RBAC = config.AuthRBACSimplified + kvStore := kvtest.GetStore(ctx, t) + require.NoError(t, kv.SetDBSchemaVersion(ctx, kvStore, kv.ACLMigrateVersion)) + err := cmd.DoMigration(ctx, kvStore, &cfg, false) + require.ErrorIs(t, err, kv.ErrMigrationVersion) + version, err := kv.GetDBSchemaVersion(ctx, kvStore) + require.NoError(t, err) + require.Equal(t, kv.ACLMigrateVersion, version) + }) + + t.Run("from_acl_v1_force", func(t *testing.T) { + cfg := config.Config{} + cfg.Auth.UIConfig.RBAC = config.AuthRBACSimplified + kvStore := kvtest.GetStore(ctx, t) + require.NoError(t, kv.SetDBSchemaVersion(ctx, kvStore, kv.ACLNoReposMigrateVersion)) + err := cmd.DoMigration(ctx, kvStore, &cfg, true) + require.NoError(t, err) + version, err := kv.GetDBSchemaVersion(ctx, kvStore) + require.NoError(t, err) + require.True(t, kv.IsLatestSchemaVersion(version)) + }) + + t.Run("from_acl_v2", func(t *testing.T) { + cfg := config.Config{} + cfg.Auth.UIConfig.RBAC = config.AuthRBACSimplified + startVer := kv.ACLNoReposMigrateVersion + for !kv.IsLatestSchemaVersion(startVer) { + kvStore := kvtest.GetStore(ctx, t) + require.NoError(t, kv.SetDBSchemaVersion(ctx, kvStore, uint(startVer))) + err := cmd.DoMigration(ctx, kvStore, &cfg, false) + require.NoError(t, err) + version, err := kv.GetDBSchemaVersion(ctx, kvStore) + require.NoError(t, err) + require.True(t, kv.IsLatestSchemaVersion(version)) + startVer += 1 + } + }) + + t.Run("latest_version", func(t *testing.T) { + cfg := config.Config{} + cfg.Auth.UIConfig.RBAC = config.AuthRBACSimplified + kvStore := kvtest.GetStore(ctx, t) + require.NoError(t, kv.SetDBSchemaVersion(ctx, kvStore, kv.NextSchemaVersion-1)) + err := cmd.DoMigration(ctx, kvStore, &cfg, false) + require.NoError(t, err) + version, err := kv.GetDBSchemaVersion(ctx, kvStore) + require.NoError(t, err) + require.True(t, kv.IsLatestSchemaVersion(version)) + }) + + t.Run("next_version", func(t *testing.T) { + cfg := config.Config{} + cfg.Auth.UIConfig.RBAC = config.AuthRBACSimplified + kvStore := kvtest.GetStore(ctx, t) + require.NoError(t, kv.SetDBSchemaVersion(ctx, kvStore, kv.NextSchemaVersion)) + err := cmd.DoMigration(ctx, kvStore, &cfg, false) + require.ErrorIs(t, err, kv.ErrMigrationVersion) + version, err := kv.GetDBSchemaVersion(ctx, kvStore) + require.NoError(t, err) + require.Equal(t, kv.NextSchemaVersion, version) + }) + + t.Run("invalid_version", func(t *testing.T) { + cfg := config.Config{} + cfg.Auth.UIConfig.RBAC = config.AuthRBACSimplified + kvStore := kvtest.GetStore(ctx, t) + require.NoError(t, kv.SetDBSchemaVersion(ctx, kvStore, 0)) + err := cmd.DoMigration(ctx, kvStore, &cfg, false) + require.ErrorIs(t, err, kv.ErrMigrationVersion) + version, err := kv.GetDBSchemaVersion(ctx, kvStore) + require.NoError(t, err) + require.Equal(t, 0, version) + }) +} diff --git a/pkg/auth/base.go b/pkg/auth/base.go index f6aa886b32e..2cd8428a69e 100644 --- a/pkg/auth/base.go +++ b/pkg/auth/base.go @@ -76,6 +76,25 @@ var statementByName = map[string]model.Statement{ }, } +// GetActionsForPolicyType returns the actions for police type typ. +func GetActionsForPolicyType(typ string) ([]string, error) { + statement, ok := statementByName[typ] + if !ok { + return nil, fmt.Errorf("%w: %s", ErrStatementNotFound, typ) + } + actions := make([]string, len(statement.Action)) + copy(actions, statement.Action) + return actions, nil +} + +func GetActionsForPolicyTypeOrDie(typ string) []string { + ret, err := GetActionsForPolicyType(typ) + if err != nil { + panic(err) + } + return ret +} + // MakeStatementForPolicyType returns statements for policy type typ, // limited to resources. func MakeStatementForPolicyType(typ string, resources []string) (model.Statements, error) { diff --git a/pkg/kv/migration.go b/pkg/kv/migration.go index ad3576050b0..ae1ebaa4d7a 100644 --- a/pkg/kv/migration.go +++ b/pkg/kv/migration.go @@ -58,8 +58,9 @@ func ValidateSchemaVersion(ctx context.Context, store Store) (int, error) { return kvVersion, fmt.Errorf("incompatible schema version. Latest: %d: %w", NextSchemaVersion-1, ErrMigrationVersion) case kvVersion < InitialMigrateVersion: return kvVersion, fmt.Errorf("migration to KV required. Did you migrate using version v0.80.x? https://docs.lakefs.io/reference/upgrade.html#lakefs-0800-or-greater-kv-migration: %w", ErrMigrationVersion) - case kvVersion < ACLNoReposMigrateVersion: - return kvVersion, fmt.Errorf("migration to ACL required. Did you migrate using version v0.99.x? https://docs.lakefs.io/reference/access-control-list.html#migrating-from-the-previous-version-of-acls: %w", ErrMigrationVersion) + case kvVersion < ACLMigrateVersion, + kvVersion < ACLNoReposMigrateVersion: + return kvVersion, fmt.Errorf("migration to ACL required. Please run 'lakefs migrate up': %w", ErrMigrationRequired) case kvVersion < ACLImportMigrateVersion: return kvVersion, fmt.Errorf("ACL migration required. Please run 'lakefs migrate up': %w", ErrMigrationRequired) } diff --git a/pkg/kv/migrations/import_permissions.go b/pkg/kv/migrations/import_permissions.go index 2dbb63fc486..6140c2a9de9 100644 --- a/pkg/kv/migrations/import_permissions.go +++ b/pkg/kv/migrations/import_permissions.go @@ -5,13 +5,20 @@ import ( "fmt" "github.com/treeverse/lakefs/pkg/auth/model" + "github.com/treeverse/lakefs/pkg/config" "github.com/treeverse/lakefs/pkg/kv" "github.com/treeverse/lakefs/pkg/permissions" "golang.org/x/exp/slices" "google.golang.org/protobuf/types/known/timestamppb" ) -func MigrateImportPermissions(ctx context.Context, kvStore kv.Store) error { +func MigrateImportPermissions(ctx context.Context, kvStore kv.Store, cfg *config.Config) error { + // skip migrate to ACL for users with External authorizations + if !cfg.IsAuthUISimplified() { + fmt.Println("skipping ACL migration - external Authorization") + return updateKVSchemaVersion(ctx, kvStore, kv.ACLImportMigrateVersion) + } + const action = "fs:Import*" it, err := kv.NewPrimaryIterator(ctx, kvStore, (&model.PolicyData{}).ProtoReflect().Type(), model.PartitionKey, model.PolicyPath(""), kv.IteratorOptionsFrom([]byte(""))) if err != nil { @@ -42,9 +49,5 @@ func MigrateImportPermissions(ctx context.Context, kvStore kv.Store) error { } } - err = kv.SetDBSchemaVersion(ctx, kvStore, kv.ACLImportMigrateVersion) - if err != nil { - return fmt.Errorf("migration succeeded but failed to upgrade version, to fix this re-run migration: %w", err) - } - return nil + return updateKVSchemaVersion(ctx, kvStore, kv.ACLImportMigrateVersion) } diff --git a/pkg/kv/migrations/migrations_test.go b/pkg/kv/migrations/migrations_test.go index 667d755844c..56f8f49f106 100644 --- a/pkg/kv/migrations/migrations_test.go +++ b/pkg/kv/migrations/migrations_test.go @@ -2,32 +2,287 @@ package migrations_test import ( "context" + "errors" "fmt" "strings" "testing" "time" + "github.com/go-test/deep" "github.com/stretchr/testify/require" "github.com/treeverse/lakefs/pkg/auth" + "github.com/treeverse/lakefs/pkg/auth/acl" "github.com/treeverse/lakefs/pkg/auth/model" - auth_testutil "github.com/treeverse/lakefs/pkg/auth/testutil" + "github.com/treeverse/lakefs/pkg/auth/setup" + authtestutil "github.com/treeverse/lakefs/pkg/auth/testutil" + "github.com/treeverse/lakefs/pkg/config" "github.com/treeverse/lakefs/pkg/kv/migrations" "github.com/treeverse/lakefs/pkg/permissions" "github.com/treeverse/lakefs/pkg/testutil" "golang.org/x/exp/slices" ) +// ##################################################################################### +// # +// rbac_to_acl # +// # +// ##################################################################################### + +func TestGetMinPermission(t *testing.T) { + tests := []struct { + Action string + Permission model.ACLPermission + }{ + {Action: permissions.ReadObjectAction, Permission: acl.ACLRead}, + {Action: "fs:Read*", Permission: acl.ACLRead}, + {Action: "fs:ReadO*", Permission: acl.ACLRead}, + {Action: permissions.ListObjectsAction, Permission: acl.ACLRead}, + {Action: "fs:List*", Permission: acl.ACLRead}, + {Action: permissions.ReadActionsAction, Permission: acl.ACLWrite}, + {Action: "fs:WriteO?ject", Permission: acl.ACLWrite}, + } + + mig := migrations.NewACLsMigrator(nil, false) + + for _, tt := range tests { + t.Run(tt.Action, func(t *testing.T) { + permission := mig.GetMinPermission(tt.Action) + if permission != tt.Permission { + t.Errorf("Got permission %s != %s", permission, tt.Permission) + } + }) + } +} + +func TestComputePermission(t *testing.T) { + tests := []struct { + Name string + Actions []string + + Permission model.ACLPermission + Err error + }{ + { + Name: "read-all", + Actions: auth.GetActionsForPolicyTypeOrDie("FSRead"), + Permission: acl.ACLRead, + }, { + Name: "read-one", + Actions: []string{permissions.ReadRepositoryAction}, + Permission: acl.ACLRead, + }, { + Name: "read-two", + Actions: []string{permissions.ListObjectsAction, permissions.ReadTagAction}, + Permission: acl.ACLRead, + }, { + Name: "only-own-credentials", + Actions: auth.GetActionsForPolicyTypeOrDie("AuthManageOwnCredentials"), + Permission: acl.ACLRead, + }, { + Name: "write-all", + Actions: auth.GetActionsForPolicyTypeOrDie("FSReadWrite"), + Permission: acl.ACLWrite, + }, { + Name: "write-one", + Actions: []string{permissions.WriteObjectAction}, + Permission: acl.ACLWrite, + }, { + Name: "write-one-read-one-create-one", + Actions: []string{permissions.CreateCommitAction, permissions.ReadObjectAction, permissions.CreateMetaRangeAction}, + Permission: acl.ACLWrite, + }, { + Name: "super-all", + Actions: auth.GetActionsForPolicyTypeOrDie("FSFullAccess"), + Permission: acl.ACLSuper, + }, { + Name: "super-one", + Actions: []string{permissions.AttachStorageNamespaceAction}, + Permission: acl.ACLSuper, + }, { + Name: "super-one-write-one-read-two", + Actions: []string{permissions.CreateTagAction, permissions.AttachStorageNamespaceAction, permissions.ReadConfigAction, permissions.ReadRepositoryAction}, + Permission: acl.ACLSuper, + }, { + Name: "admin-all", + Actions: auth.GetActionsForPolicyTypeOrDie("AllAccess"), + Permission: acl.ACLAdmin, + }, { + Name: "admin-one", + Actions: []string{permissions.SetGarbageCollectionRulesAction}, + Permission: acl.ACLAdmin, + }, + } + + ctx := context.Background() + + mig := migrations.NewACLsMigrator(nil, false) + + for _, tt := range tests { + t.Run(tt.Name, func(t *testing.T) { + permission, err := mig.ComputePermission(ctx, tt.Actions) + + if permission != tt.Permission { + t.Errorf("Got permission %s when expecting %s", permission, tt.Permission) + } + + if !errors.Is(err, tt.Err) { + t.Errorf("Got error %s but expected %s", err, tt.Err) + } + }) + } +} + +func TestBroaderPermission(t *testing.T) { + perms := []model.ACLPermission{"", acl.ACLRead, acl.ACLWrite, acl.ACLSuper, acl.ACLAdmin} + for i, a := range perms { + for j, b := range perms { + t.Run(fmt.Sprintf("%s:%s", a, b), func(t *testing.T) { + after := i > j + broader := migrations.BroaderPermission(a, b) + if after != broader { + if after { + t.Error("Expected broader permission") + } else { + t.Error("Expected not a broader permission") + } + } + }) + } + } +} + +func getPolicy(t *testing.T, ctx context.Context, svc auth.Service, name string) *model.Policy { + t.Helper() + policy, err := svc.GetPolicy(ctx, name) + if err != nil { + t.Fatal(err) + } + return policy +} + +func getPolicies(t *testing.T, ctx context.Context, svc auth.Service, name string) []*model.Policy { + t.Helper() + policies, _, err := svc.ListGroupPolicies(ctx, name, &model.PaginationParams{Amount: 1000}) + if err != nil { + t.Fatal(err) + } + return policies +} + +func TestNewACLForPolicies_Generator(t *testing.T) { + now := time.Now() + ctx := context.Background() + svc, _ := authtestutil.SetupService(t, ctx, []byte("shh...")) + + err := setup.SetupRBACBaseGroups(ctx, svc, now) + if err != nil { + t.Fatal(err) + } + + tests := []struct { + Name string + // Policies are passed to NewACLForPolicies + Policies []*model.Policy + // ACL is expected to be returned by NewACLForPolicies + ACL model.ACL + // Warnings is a slice of warning messages. Each message + // should be contained in some returned warning (but not + // every returned warning must be matched!). + Warnings []string + // Err, if set, is expected to be the error returned from NewACLForPolicies + Err error + }{ + { + Name: "ExactlyFSFullAccess", + Policies: []*model.Policy{getPolicy(t, ctx, svc, "FSFullAccess")}, + ACL: model.ACL{ + Permission: acl.ACLSuper, + }, + }, { + Name: "GroupSuperUsers", + Policies: getPolicies(t, ctx, svc, "SuperUsers"), + ACL: model.ACL{ + Permission: acl.ACLSuper, + }, + }, { + Name: "ExactlyFSReadAll", + Policies: []*model.Policy{getPolicy(t, ctx, svc, "FSReadAll")}, + ACL: model.ACL{ + Permission: acl.ACLRead, + }, + }, { + Name: "GroupViewers", + Policies: getPolicies(t, ctx, svc, "Viewers"), + ACL: model.ACL{ + Permission: acl.ACLRead, + }, + }, + } + + mig := migrations.NewACLsMigrator(svc, false) + + for _, tt := range tests { + t.Run(tt.Name, func(t *testing.T) { + acp, warn, err := mig.NewACLForPolicies(ctx, tt.Policies) + if !errors.Is(err, tt.Err) { + t.Errorf("Got error %s, expected %s", err, tt.Err) + } + if diffs := deep.Equal(acp, &tt.ACL); diffs != nil { + t.Errorf("Bad ACL: %s", diffs) + } + if len(tt.Warnings) > 0 { + if warn == nil { + t.Fatalf("No warnings returned when expecting %v", tt.Warnings) + } + fullWarning := warn.Error() + for _, expectedWarning := range tt.Warnings { + if !strings.Contains(fullWarning, expectedWarning) { + t.Errorf("Got warnings %v, which did not include expected warning %s", fullWarning, expectedWarning) + } + } + } + }) + } +} + +// ##################################################################################### +// +// # +// import_permissions # +// # +// +// ##################################################################################### func TestMigrateImportPermissions(t *testing.T) { ctx := context.Background() - authService, store := auth_testutil.SetupService(t, ctx, []byte("some secret")) + authService, store := authtestutil.SetupService(t, ctx, []byte("some secret")) + cfg := config.Config{} tests := []struct { - name string - policies []model.Policy + name string + policies []model.Policy + uiAuthType string }{ { - name: "empty", - policies: []model.Policy{}, + name: "eternal_auth_type", + policies: []model.Policy{ + { + CreatedAt: time.Now().Add(-time.Hour).UTC(), + DisplayName: "import_no_change", + Statement: []model.Statement{ + { + Effect: "allow", + Action: []string{permissions.ImportFromStorageAction, permissions.CreatePolicyAction}, + Resource: createARN("importFromStorage"), + }, + }, + }, + }, + uiAuthType: config.AuthRBACExternal, + }, + { + name: "empty", + policies: []model.Policy{}, + uiAuthType: config.AuthRBACSimplified, }, { name: "no_import", @@ -55,6 +310,7 @@ func TestMigrateImportPermissions(t *testing.T) { }, }, }, + uiAuthType: config.AuthRBACSimplified, }, { name: "basic", @@ -67,7 +323,8 @@ func TestMigrateImportPermissions(t *testing.T) { Effect: "allow", Action: []string{permissions.ImportFromStorageAction, permissions.CreatePolicyAction}, Resource: createARN("importFromStorage"), - }}, + }, + }, }, { CreatedAt: time.Now().Add(-time.Hour).UTC(), @@ -77,7 +334,8 @@ func TestMigrateImportPermissions(t *testing.T) { Effect: "deny", Action: []string{permissions.RevertBranchAction}, Resource: createARN("RevertBranchAction"), - }}, + }, + }, }, { CreatedAt: time.Now().Add(-time.Hour).UTC(), @@ -101,6 +359,7 @@ func TestMigrateImportPermissions(t *testing.T) { }, }, }, + uiAuthType: config.AuthRBACSimplified, }, } @@ -110,10 +369,11 @@ func TestMigrateImportPermissions(t *testing.T) { testutil.MustDo(t, "create Policy", authService.WritePolicy(ctx, &policy, false)) } // Run migrate - testutil.MustDo(t, "migrate", migrations.MigrateImportPermissions(ctx, store)) + cfg.Auth.UIConfig.RBAC = tt.uiAuthType + testutil.MustDo(t, "migrate", migrations.MigrateImportPermissions(ctx, store, &cfg)) // Verify - verifyMigration(t, ctx, authService, tt.policies) + verifyMigration(t, ctx, authService, tt.policies, cfg) }) } } @@ -121,22 +381,24 @@ func TestMigrateImportPermissions(t *testing.T) { func createARN(name string) string { return fmt.Sprintf("arn:%s:this:is:an:arn", name) } -func verifyMigration(t *testing.T, ctx context.Context, authService *auth.AuthService, policies []model.Policy) { +func verifyMigration(t *testing.T, ctx context.Context, authService *auth.AuthService, policies []model.Policy, cfg config.Config) { for _, prev := range policies { policy, err := authService.GetPolicy(ctx, prev.DisplayName) testutil.MustDo(t, "get policy", err) require.Equal(t, prev.ACL, policy.ACL) if strings.HasPrefix(policy.DisplayName, "import") { - require.Greater(t, policy.CreatedAt, prev.CreatedAt) expected := prev.Statement - for _, statement := range expected { - for { - idx := slices.Index(statement.Action, permissions.ImportFromStorageAction) - if idx < 0 { - break + if cfg.IsAuthUISimplified() { + require.Greater(t, policy.CreatedAt, prev.CreatedAt) + for _, statement := range expected { + for { + idx := slices.Index(statement.Action, permissions.ImportFromStorageAction) + if idx < 0 { + break + } + statement.Action[idx] = "fs:Import*" } - statement.Action[idx] = "fs:Import*" } } require.Equal(t, expected, policy.Statement) diff --git a/pkg/kv/migrations/rbac_to_acl.go b/pkg/kv/migrations/rbac_to_acl.go new file mode 100644 index 00000000000..9cf1f463075 --- /dev/null +++ b/pkg/kv/migrations/rbac_to_acl.go @@ -0,0 +1,448 @@ +package migrations + +import ( + "context" + "errors" + "fmt" + "strings" + "time" + + "github.com/hashicorp/go-multierror" + "github.com/treeverse/lakefs/pkg/auth" + "github.com/treeverse/lakefs/pkg/auth/acl" + "github.com/treeverse/lakefs/pkg/auth/crypt" + "github.com/treeverse/lakefs/pkg/auth/model" + authparams "github.com/treeverse/lakefs/pkg/auth/params" + "github.com/treeverse/lakefs/pkg/auth/wildcard" + "github.com/treeverse/lakefs/pkg/config" + "github.com/treeverse/lakefs/pkg/kv" + "github.com/treeverse/lakefs/pkg/logging" + "github.com/treeverse/lakefs/pkg/permissions" +) + +const ( + maxGroups = 1000 + pageSize = 1000 + maxGroupPolicies = 1000 +) + +var ( + // ErrTooMany is returned when this migration does not support a + // particular number of resources. It should not occur on any + // reasonably-sized installation. + ErrTooMany = errors.New("too many") + ErrTooManyPolicies = fmt.Errorf("%w policies", ErrTooMany) + ErrTooManyGroups = fmt.Errorf("%w groups", ErrTooMany) + ErrNotAllowed = fmt.Errorf("not allowed") + ErrAlreadyHasACL = errors.New("already has ACL") + ErrAddedActions = errors.New("added actions") + ErrEmpty = errors.New("empty") + ErrPolicyExists = errors.New("policy exists") + + // allPermissions lists all permissions, from most restrictive to + // most permissive. It includes "" for some edge cases. + allPermissions = []model.ACLPermission{"", acl.ACLRead, acl.ACLWrite, acl.ACLSuper, acl.ACLAdmin} +) + +func MigrateToACL(ctx context.Context, kvStore kv.Store, cfg *config.Config, logger logging.Logger, version int, force bool) error { + if cfg.IsAuthTypeAPI() { + fmt.Println("skipping ACL migration - external Authorization") + return updateKVSchemaVersion(ctx, kvStore, kv.ACLNoReposMigrateVersion) + } + + // handle migrate within ACLs + if version == kv.ACLMigrateVersion { + if force { + return updateKVSchemaVersion(ctx, kvStore, kv.ACLNoReposMigrateVersion) + } else { + return fmt.Errorf("migrating from previous version of ACL will leave repository level groups until the group is re-edited - please run migrate again with --force flag or contact services@treeverse.io: %w\n", kv.ErrMigrationVersion) + } + } + + type Warning struct { + GroupID string + ACL model.ACL + Warn error + } + var ( + groupReports []Warning + usersWithPolicies []string + ) + updateTime := time.Now() + authService := auth.NewAuthService( + kvStore, + crypt.NewSecretStore(cfg.AuthEncryptionSecret()), + nil, + authparams.ServiceCache(cfg.Auth.Cache), + logger.WithField("service", "auth_service"), + ) + usersWithPolicies, err := rbacToACL(ctx, authService, false, updateTime, func(groupID string, acl model.ACL, warn error) { + groupReports = append(groupReports, Warning{GroupID: groupID, ACL: acl, Warn: warn}) + }) + if err != nil { + return fmt.Errorf("failed to upgrade RBAC policies to ACLs: %w\n", err) + } + + for _, w := range groupReports { + fmt.Printf("GROUP %s\n\tACL: %s\n", w.GroupID, reportACL(w.ACL)) + if w.Warn != nil { + var multi *multierror.Error + if errors.As(w.Warn, &multi) { + multi.ErrorFormat = func(es []error) string { + points := make([]string, len(es)) + for i, err := range es { + points[i] = fmt.Sprintf("* %s", err) + } + plural := "s" + if len(es) == 1 { + plural = "" + } + return fmt.Sprintf( + "%d change%s:\n\t%s\n", + len(points), plural, strings.Join(points, "\n\t")) + } + } + fmt.Println(w.Warn) + } + fmt.Println() + } + for _, username := range usersWithPolicies { + fmt.Printf("USER (%s) detaching directly-attached policies\n", username) + } + + usersWithPolicies, err = rbacToACL(ctx, authService, true, updateTime, func(groupID string, acl model.ACL, warn error) { + groupReports = append(groupReports, Warning{GroupID: groupID, ACL: acl, Warn: warn}) + }) + if err != nil { + return fmt.Errorf("failed to upgrade RBAC policies to ACLs: %w\n", err) + } + + return updateKVSchemaVersion(ctx, kvStore, kv.ACLNoReposMigrateVersion) +} + +func reportACL(acl model.ACL) string { + return string(acl.Permission) + " on [ALL repositories]" +} + +// checkPolicyACLName fails if policy name is named as an ACL policy (start +// with ACLPolicyPrefix) but is not an ACL policy. +func checkPolicyACLName(ctx context.Context, svc auth.Service, name string) error { + if !acl.IsACLPolicyName(name) { + return nil + } + + _, err := svc.GetGroup(ctx, name) + switch { + case errors.Is(err, auth.ErrNotFound): + return nil + case err == nil: + return fmt.Errorf("%s: %w", name, ErrPolicyExists) + default: + return fmt.Errorf("check policy name %s: %w", name, err) + } +} + +// rbacToACL translates all groups on svc to use ACLs instead of RBAC +// policies. It updates svc only if doUpdate. It calls messageFunc to +// report increased permissions. +// returns a list of users with directly attached policies +func rbacToACL(ctx context.Context, svc auth.Service, doUpdate bool, creationTime time.Time, messageFunc func(string, model.ACL, error)) ([]string, error) { + mig := NewACLsMigrator(svc, doUpdate) + + groups, _, err := svc.ListGroups(ctx, &model.PaginationParams{Amount: maxGroups + 1}) + if err != nil { + return nil, fmt.Errorf("list groups: %w", err) + } + if len(groups) > maxGroups { + return nil, fmt.Errorf("%w (got %d)", ErrTooManyGroups, len(groups)) + } + for _, group := range groups { + var warnings error + + policies, _, err := svc.ListGroupPolicies(ctx, group.DisplayName, &model.PaginationParams{Amount: maxGroupPolicies + 1}) + if err != nil { + return nil, fmt.Errorf("list group %+v policies: %w", group, err) + } + if len(policies) > maxGroupPolicies { + return nil, fmt.Errorf("group %+v: %w (got %d)", group, ErrTooManyPolicies, len(policies)) + } + newACL, warn, err := mig.NewACLForPolicies(ctx, policies) + if err != nil { + return nil, fmt.Errorf("create ACL for group %+v: %w", group, err) + } + if warn != nil { + warnings = multierror.Append(warnings, warn) + } + + log := logging.FromContext(ctx) + log.WithFields(logging.Fields{ + "group": group.DisplayName, + "acl": fmt.Sprintf("%+v", newACL), + }).Info("Computed ACL") + + aclPolicyName := acl.ACLPolicyName(group.DisplayName) + err = checkPolicyACLName(ctx, svc, aclPolicyName) + if err != nil { + warnings = multierror.Append(warnings, warn) + } + policyExists := errors.Is(err, ErrPolicyExists) + if doUpdate { + err = acl.WriteGroupACL(ctx, svc, group.DisplayName, *newACL, creationTime, policyExists) + if errors.Is(err, auth.ErrAlreadyExists) { + warnings = multierror.Append(warnings, err) + } else if err != nil { + return nil, err + } + } + + messageFunc(group.DisplayName, *newACL, warnings) + } + var usersWithPolicies []string + hasMoreUser := true + afterUser := "" + for hasMoreUser { + // get membership groups to user + users, userPaginator, err := svc.ListUsers(ctx, &model.PaginationParams{ + After: afterUser, + Amount: pageSize, + }) + if err != nil { + return nil, err + } + for _, user := range users { + // get policies attracted to user + hasMoreUserPolicy := true + afterUserPolicy := "" + for hasMoreUserPolicy { + userPolicies, userPoliciesPaginator, err := svc.ListUserPolicies(ctx, user.Username, &model.PaginationParams{ + After: afterUserPolicy, + Amount: pageSize, + }) + if err != nil { + return nil, fmt.Errorf("list user policies: %w", err) + } + if len(userPolicies) > 0 { + usersWithPolicies = append(usersWithPolicies, user.Username) + } + if !doUpdate { + break + } + for _, policy := range userPolicies { + if err := svc.DetachPolicyFromUser(ctx, policy.DisplayName, user.Username); err != nil { + return nil, fmt.Errorf("failed detaching policy %s from user %s: %w", policy.DisplayName, user.Username, err) + } + } + afterUserPolicy = userPoliciesPaginator.NextPageToken + hasMoreUserPolicy = userPoliciesPaginator.NextPageToken != "" + } + afterUser = userPaginator.NextPageToken + hasMoreUser = userPaginator.NextPageToken != "" + } + } + return usersWithPolicies, nil +} + +// ACLsMigrator migrates from policies to ACLs. +type ACLsMigrator struct { + svc auth.Service + doUpdate bool + + Actions map[model.ACLPermission]map[string]struct{} +} + +func makeSet(allEls ...[]string) map[string]struct{} { + ret := make(map[string]struct{}, 0) + for _, els := range allEls { + for _, el := range els { + ret[el] = struct{}{} + } + } + return ret +} + +// NewACLsMigrator returns an ACLsMigrator. That ACLsMigrator will only +// check (change nothing) if doUpdate is false. +func NewACLsMigrator(svc auth.Service, doUpdate bool) *ACLsMigrator { + manageOwnCredentials := auth.GetActionsForPolicyTypeOrDie("AuthManageOwnCredentials") + ciRead := auth.GetActionsForPolicyTypeOrDie("RepoManagementRead") + return &ACLsMigrator{ + svc: svc, + doUpdate: doUpdate, + Actions: map[model.ACLPermission]map[string]struct{}{ + acl.ACLAdmin: makeSet(auth.GetActionsForPolicyTypeOrDie("AllAccess")), + acl.ACLSuper: makeSet(auth.GetActionsForPolicyTypeOrDie("FSFullAccess"), manageOwnCredentials, ciRead), + acl.ACLWrite: makeSet(auth.GetActionsForPolicyTypeOrDie("FSReadWrite"), manageOwnCredentials, ciRead), + acl.ACLRead: makeSet(auth.GetActionsForPolicyTypeOrDie("FSRead"), manageOwnCredentials), + }, + } +} + +// NewACLForPolicies converts policies of group name to an ACL. warn +// summarizes all losses in converting policies to ACL. err holds an error +// if conversion failed. +func (mig *ACLsMigrator) NewACLForPolicies(ctx context.Context, policies []*model.Policy) (acl *model.ACL, warn error, err error) { + warn = nil + acl = new(model.ACL) + + allAllowedActions := make(map[string]struct{}) + for _, p := range policies { + if p.ACL.Permission != "" { + warn = multierror.Append(warn, fmt.Errorf("policy %s: %w", p.DisplayName, ErrAlreadyHasACL)) + } + + for _, s := range p.Statement { + if s.Effect != model.StatementEffectAllow { + warn = multierror.Append(warn, fmt.Errorf("ignore policy %s statement %+v: %w", p.DisplayName, s, ErrNotAllowed)) + } + sp, err := mig.ComputePermission(ctx, s.Action) + if err != nil { + return nil, warn, fmt.Errorf("convert policy %s statement %+v: %w", p.DisplayName, s, err) + } + for _, allowedAction := range expandMatchingActions(s.Action) { + allAllowedActions[allowedAction] = struct{}{} + } + + if BroaderPermission(sp, acl.Permission) { + acl.Permission = sp + } + } + } + addedActions := mig.ComputeAddedActions(acl.Permission, allAllowedActions) + if len(addedActions) > 0 { + warn = multierror.Append(warn, fmt.Errorf("%w: %s", ErrAddedActions, strings.Join(addedActions, ", "))) + } + return acl, warn, err +} + +func expandMatchingActions(patterns []string) []string { + ret := make([]string, 0, len(patterns)) + for _, action := range permissions.Actions { + for _, pattern := range patterns { + if wildcard.Match(pattern, action) { + ret = append(ret, action) + } + } + } + return ret +} + +func someActionMatches(action string, availableActions map[string]struct{}) bool { + for availableAction := range availableActions { + if wildcard.Match(availableAction, action) { + return true + } + } + return false +} + +func (mig *ACLsMigrator) GetMinPermission(action string) model.ACLPermission { + if !strings.ContainsAny(action, "*?") { + for _, permission := range allPermissions { + if someActionMatches(action, mig.Actions[permission]) { + return permission + } + } + return "" + } + + // Try a wildcard match against all known actions: find the least + // permissions that allows all actions that the action pattern + // matches. + for _, permission := range allPermissions { + // This loop is reasonably efficient only for small numbers + // of ACL permissions. + actionsForPermission := mig.Actions[permission] + permissionOK := true + for _, a := range permissions.Actions { + if !wildcard.Match(action, a) { + // a does not include action. + continue + } + if someActionMatches(a, actionsForPermission) { + // a is allowed at permission. + continue + } + permissionOK = false + break + } + if permissionOK { + return permission + } + } + panic(fmt.Sprintf("Unknown action %s", action)) +} + +// ComputePermission returns ACL permission for actions and the actions that +// applying that permission will add to it. +func (mig *ACLsMigrator) ComputePermission(ctx context.Context, actions []string) (model.ACLPermission, error) { + log := logging.FromContext(ctx) + permission := model.ACLPermission("") + for _, action := range actions { + p := mig.GetMinPermission(action) + if BroaderPermission(p, permission) { + log.WithFields(logging.Fields{ + "action": action, + "permission": p, + "prev_permission": permission, + }).Debug("Increase permission") + permission = p + } else { + log.WithFields(logging.Fields{ + "action": action, + "permission": p, + }).Trace("Permission") + } + } + if permission == model.ACLPermission("") { + return permission, fmt.Errorf("%w actions", ErrEmpty) + } + + return permission, nil +} + +// ComputeAddedActions returns the list of actions that permission allows +// that are not in alreadyAllowedActions. +func (mig *ACLsMigrator) ComputeAddedActions(permission model.ACLPermission, alreadyAllowedActions map[string]struct{}) []string { + var ( + allAllowedActions map[string]struct{} + ) + switch permission { + case acl.ACLRead: + allAllowedActions = mig.Actions[acl.ACLRead] + case acl.ACLWrite: + allAllowedActions = mig.Actions[acl.ACLWrite] + case acl.ACLSuper: + allAllowedActions = mig.Actions[acl.ACLSuper] + case acl.ACLAdmin: + default: + allAllowedActions = mig.Actions[acl.ACLAdmin] + } + addedActions := make(map[string]struct{}, len(allAllowedActions)) + for _, action := range permissions.Actions { + if someActionMatches(action, allAllowedActions) && !someActionMatches(action, alreadyAllowedActions) { + addedActions[action] = struct{}{} + } + } + addedActionsSlice := make([]string, 0, len(addedActions)) + for action := range addedActions { + addedActionsSlice = append(addedActionsSlice, action) + } + return addedActionsSlice +} + +// BroaderPermission returns true if a offers strictly more permissions that b. +func BroaderPermission(a, b model.ACLPermission) bool { + switch a { + case "": + return false + case acl.ACLRead: + return b == "" + case acl.ACLWrite: + return b == "" || b == acl.ACLRead + case acl.ACLSuper: + return b == "" || b == acl.ACLRead || b == acl.ACLWrite + case acl.ACLAdmin: + return b == "" || b == acl.ACLRead || b == acl.ACLWrite || b == acl.ACLSuper + } + panic(fmt.Sprintf("impossible comparison %s and %s", a, b)) +} diff --git a/pkg/kv/migrations/utils.go b/pkg/kv/migrations/utils.go new file mode 100644 index 00000000000..4f452840743 --- /dev/null +++ b/pkg/kv/migrations/utils.go @@ -0,0 +1,16 @@ +package migrations + +import ( + "context" + "fmt" + + "github.com/treeverse/lakefs/pkg/kv" +) + +func updateKVSchemaVersion(ctx context.Context, kvStore kv.Store, version uint) error { + err := kv.SetDBSchemaVersion(ctx, kvStore, version) + if err != nil { + return fmt.Errorf("failed to upgrade version, to fix this re-run migration: %w", err) + } + return nil +} From c0e4f8e6265e81c6d9fd609117cc634bd1b8548a Mon Sep 17 00:00:00 2001 From: Nir Ozery Date: Wed, 24 May 2023 10:45:04 +0300 Subject: [PATCH 2/5] Gofmt --- pkg/kv/migrations/rbac_to_acl.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/kv/migrations/rbac_to_acl.go b/pkg/kv/migrations/rbac_to_acl.go index 9cf1f463075..c5f1d6a0069 100644 --- a/pkg/kv/migrations/rbac_to_acl.go +++ b/pkg/kv/migrations/rbac_to_acl.go @@ -55,7 +55,7 @@ func MigrateToACL(ctx context.Context, kvStore kv.Store, cfg *config.Config, log if force { return updateKVSchemaVersion(ctx, kvStore, kv.ACLNoReposMigrateVersion) } else { - return fmt.Errorf("migrating from previous version of ACL will leave repository level groups until the group is re-edited - please run migrate again with --force flag or contact services@treeverse.io: %w\n", kv.ErrMigrationVersion) + return fmt.Errorf("migrating from previous version of ACL will leave repository level groups until the group is re-edited - please run migrate again with --force flag or contact services@treeverse.io: %w", kv.ErrMigrationVersion) } } @@ -80,7 +80,7 @@ func MigrateToACL(ctx context.Context, kvStore kv.Store, cfg *config.Config, log groupReports = append(groupReports, Warning{GroupID: groupID, ACL: acl, Warn: warn}) }) if err != nil { - return fmt.Errorf("failed to upgrade RBAC policies to ACLs: %w\n", err) + return fmt.Errorf("failed to upgrade RBAC policies to ACLs: %w", err) } for _, w := range groupReports { @@ -110,11 +110,11 @@ func MigrateToACL(ctx context.Context, kvStore kv.Store, cfg *config.Config, log fmt.Printf("USER (%s) detaching directly-attached policies\n", username) } - usersWithPolicies, err = rbacToACL(ctx, authService, true, updateTime, func(groupID string, acl model.ACL, warn error) { + _, err = rbacToACL(ctx, authService, true, updateTime, func(groupID string, acl model.ACL, warn error) { groupReports = append(groupReports, Warning{GroupID: groupID, ACL: acl, Warn: warn}) }) if err != nil { - return fmt.Errorf("failed to upgrade RBAC policies to ACLs: %w\n", err) + return fmt.Errorf("failed to upgrade RBAC policies to ACLs: %w", err) } return updateKVSchemaVersion(ctx, kvStore, kv.ACLNoReposMigrateVersion) From 90b6fecd8fed64c5030eb036ed79d770529fcb9e Mon Sep 17 00:00:00 2001 From: Nir Ozery Date: Sun, 28 May 2023 11:57:06 +0300 Subject: [PATCH 3/5] CR Fixes --- pkg/kv/migrations/import_permissions.go | 2 +- pkg/kv/migrations/migrations_test.go | 64 +++++++------------------ pkg/kv/migrations/rbac_to_acl.go | 6 +-- 3 files changed, 21 insertions(+), 51 deletions(-) diff --git a/pkg/kv/migrations/import_permissions.go b/pkg/kv/migrations/import_permissions.go index 6140c2a9de9..cfbe177cfd1 100644 --- a/pkg/kv/migrations/import_permissions.go +++ b/pkg/kv/migrations/import_permissions.go @@ -13,7 +13,7 @@ import ( ) func MigrateImportPermissions(ctx context.Context, kvStore kv.Store, cfg *config.Config) error { - // skip migrate to ACL for users with External authorizations + // skip migrate for users with External authorizations if !cfg.IsAuthUISimplified() { fmt.Println("skipping ACL migration - external Authorization") return updateKVSchemaVersion(ctx, kvStore, kv.ACLImportMigrateVersion) diff --git a/pkg/kv/migrations/migrations_test.go b/pkg/kv/migrations/migrations_test.go index 56f8f49f106..1d4d8042b70 100644 --- a/pkg/kv/migrations/migrations_test.go +++ b/pkg/kv/migrations/migrations_test.go @@ -22,11 +22,7 @@ import ( "golang.org/x/exp/slices" ) -// ##################################################################################### -// # -// rbac_to_acl # -// # -// ##################################################################################### +// # rbac_to_acl test code func TestGetMinPermission(t *testing.T) { tests := []struct { @@ -115,19 +111,17 @@ func TestComputePermission(t *testing.T) { ctx := context.Background() - mig := migrations.NewACLsMigrator(nil, false) - for _, tt := range tests { t.Run(tt.Name, func(t *testing.T) { - permission, err := mig.ComputePermission(ctx, tt.Actions) - - if permission != tt.Permission { - t.Errorf("Got permission %s when expecting %s", permission, tt.Permission) - } + mig := migrations.NewACLsMigrator(nil, false) + permission, err := mig.ComputePermission(ctx, tt.Actions) if !errors.Is(err, tt.Err) { t.Errorf("Got error %s but expected %s", err, tt.Err) } + if permission != tt.Permission { + t.Errorf("Got permission %s when expecting %s", permission, tt.Permission) + } }) } } @@ -136,17 +130,15 @@ func TestBroaderPermission(t *testing.T) { perms := []model.ACLPermission{"", acl.ACLRead, acl.ACLWrite, acl.ACLSuper, acl.ACLAdmin} for i, a := range perms { for j, b := range perms { - t.Run(fmt.Sprintf("%s:%s", a, b), func(t *testing.T) { - after := i > j - broader := migrations.BroaderPermission(a, b) - if after != broader { - if after { - t.Error("Expected broader permission") - } else { - t.Error("Expected not a broader permission") - } + after := i > j + broader := migrations.BroaderPermission(a, b) + if after != broader { + if after { + t.Error("Expected broader permission") + } else { + t.Error("Expected not a broader permission") } - }) + } } } } @@ -185,10 +177,6 @@ func TestNewACLForPolicies_Generator(t *testing.T) { Policies []*model.Policy // ACL is expected to be returned by NewACLForPolicies ACL model.ACL - // Warnings is a slice of warning messages. Each message - // should be contained in some returned warning (but not - // every returned warning must be matched!). - Warnings []string // Err, if set, is expected to be the error returned from NewACLForPolicies Err error }{ @@ -223,38 +211,21 @@ func TestNewACLForPolicies_Generator(t *testing.T) { for _, tt := range tests { t.Run(tt.Name, func(t *testing.T) { - acp, warn, err := mig.NewACLForPolicies(ctx, tt.Policies) + acp, _, err := mig.NewACLForPolicies(ctx, tt.Policies) if !errors.Is(err, tt.Err) { t.Errorf("Got error %s, expected %s", err, tt.Err) } if diffs := deep.Equal(acp, &tt.ACL); diffs != nil { t.Errorf("Bad ACL: %s", diffs) } - if len(tt.Warnings) > 0 { - if warn == nil { - t.Fatalf("No warnings returned when expecting %v", tt.Warnings) - } - fullWarning := warn.Error() - for _, expectedWarning := range tt.Warnings { - if !strings.Contains(fullWarning, expectedWarning) { - t.Errorf("Got warnings %v, which did not include expected warning %s", fullWarning, expectedWarning) - } - } - } }) } } -// ##################################################################################### -// -// # -// import_permissions # -// # -// -// ##################################################################################### +// # import_permissions test code + func TestMigrateImportPermissions(t *testing.T) { ctx := context.Background() - authService, store := authtestutil.SetupService(t, ctx, []byte("some secret")) cfg := config.Config{} tests := []struct { @@ -365,6 +336,7 @@ func TestMigrateImportPermissions(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + authService, store := authtestutil.SetupService(t, ctx, []byte("some secret")) for _, policy := range tt.policies { testutil.MustDo(t, "create Policy", authService.WritePolicy(ctx, &policy, false)) } diff --git a/pkg/kv/migrations/rbac_to_acl.go b/pkg/kv/migrations/rbac_to_acl.go index c5f1d6a0069..a1527512d28 100644 --- a/pkg/kv/migrations/rbac_to_acl.go +++ b/pkg/kv/migrations/rbac_to_acl.go @@ -403,9 +403,7 @@ func (mig *ACLsMigrator) ComputePermission(ctx context.Context, actions []string // ComputeAddedActions returns the list of actions that permission allows // that are not in alreadyAllowedActions. func (mig *ACLsMigrator) ComputeAddedActions(permission model.ACLPermission, alreadyAllowedActions map[string]struct{}) []string { - var ( - allAllowedActions map[string]struct{} - ) + var allAllowedActions map[string]struct{} switch permission { case acl.ACLRead: allAllowedActions = mig.Actions[acl.ACLRead] @@ -430,7 +428,7 @@ func (mig *ACLsMigrator) ComputeAddedActions(permission model.ACLPermission, alr return addedActionsSlice } -// BroaderPermission returns true if a offers strictly more permissions that b. +// BroaderPermission returns true if a offers strictly more permissions than b. Unknown ACLPermission will panic. func BroaderPermission(a, b model.ACLPermission) bool { switch a { case "": From 7233b98745591360b6ef81f1c01f00788a59e1df Mon Sep 17 00:00:00 2001 From: Nir Ozery Date: Wed, 31 May 2023 11:36:02 +0300 Subject: [PATCH 4/5] CR Fixes --- pkg/kv/migrations/rbac_to_acl.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/kv/migrations/rbac_to_acl.go b/pkg/kv/migrations/rbac_to_acl.go index a1527512d28..46dcf474d44 100644 --- a/pkg/kv/migrations/rbac_to_acl.go +++ b/pkg/kv/migrations/rbac_to_acl.go @@ -38,6 +38,7 @@ var ( ErrAddedActions = errors.New("added actions") ErrEmpty = errors.New("empty") ErrPolicyExists = errors.New("policy exists") + ErrHasWarnings = errors.New("has warnings") // allPermissions lists all permissions, from most restrictive to // most permissive. It includes "" for some edge cases. @@ -83,9 +84,11 @@ func MigrateToACL(ctx context.Context, kvStore kv.Store, cfg *config.Config, log return fmt.Errorf("failed to upgrade RBAC policies to ACLs: %w", err) } + hasWarnings := false for _, w := range groupReports { fmt.Printf("GROUP %s\n\tACL: %s\n", w.GroupID, reportACL(w.ACL)) if w.Warn != nil { + hasWarnings = true var multi *multierror.Error if errors.As(w.Warn, &multi) { multi.ErrorFormat = func(es []error) string { @@ -110,6 +113,10 @@ func MigrateToACL(ctx context.Context, kvStore kv.Store, cfg *config.Config, log fmt.Printf("USER (%s) detaching directly-attached policies\n", username) } + if hasWarnings && !force { + return fmt.Errorf("warnings found. Please fix or run using --force flag: %w", ErrHasWarnings) + } + _, err = rbacToACL(ctx, authService, true, updateTime, func(groupID string, acl model.ACL, warn error) { groupReports = append(groupReports, Warning{GroupID: groupID, ACL: acl, Warn: warn}) }) From 85719c5799f2d7b02e1bc9717d46381fd7d6e50d Mon Sep 17 00:00:00 2001 From: Nir Ozery Date: Wed, 31 May 2023 11:37:56 +0300 Subject: [PATCH 5/5] CR Fixes 2 --- pkg/kv/migrations/migrations_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/kv/migrations/migrations_test.go b/pkg/kv/migrations/migrations_test.go index 1d4d8042b70..b8b63c24341 100644 --- a/pkg/kv/migrations/migrations_test.go +++ b/pkg/kv/migrations/migrations_test.go @@ -22,8 +22,6 @@ import ( "golang.org/x/exp/slices" ) -// # rbac_to_acl test code - func TestGetMinPermission(t *testing.T) { tests := []struct { Action string @@ -222,8 +220,6 @@ func TestNewACLForPolicies_Generator(t *testing.T) { } } -// # import_permissions test code - func TestMigrateImportPermissions(t *testing.T) { ctx := context.Background() cfg := config.Config{}