From ed0f6fa072dc447dd16f317ff0d844077d1ec439 Mon Sep 17 00:00:00 2001 From: Chief-Rishab Date: Sun, 9 Jun 2024 21:19:13 +0530 Subject: [PATCH] fix(gcloudiam): add check for duplicate roles --- internal/server/services.go | 2 +- plugins/providers/gcloudiam/config.go | 10 ++ plugins/providers/gcloudiam/config_test.go | 147 +++++++++++++++++++ plugins/providers/gcloudiam/provider.go | 5 +- plugins/providers/gcloudiam/provider_test.go | 62 +++++--- 5 files changed, 204 insertions(+), 22 deletions(-) diff --git a/internal/server/services.go b/internal/server/services.go index 5dc5ba311..9b4410900 100644 --- a/internal/server/services.go +++ b/internal/server/services.go @@ -111,7 +111,7 @@ func InitServices(deps ServiceDeps) (*Services, error) { metabase.NewProvider(domain.ProviderTypeMetabase, deps.Crypto, deps.Logger), grafana.NewProvider(domain.ProviderTypeGrafana, deps.Crypto), tableau.NewProvider(domain.ProviderTypeTableau, deps.Crypto), - gcloudiam.NewProvider(domain.ProviderTypeGCloudIAM, deps.Crypto), + gcloudiam.NewProvider(domain.ProviderTypeGCloudIAM, deps.Crypto, deps.Logger), noop.NewProvider(domain.ProviderTypeNoOp, deps.Logger), gcs.NewProvider(domain.ProviderTypeGCS, deps.Crypto), dataplex.NewProvider(domain.ProviderTypePolicyTag, deps.Crypto), diff --git a/plugins/providers/gcloudiam/config.go b/plugins/providers/gcloudiam/config.go index f075339fb..46e674275 100644 --- a/plugins/providers/gcloudiam/config.go +++ b/plugins/providers/gcloudiam/config.go @@ -127,6 +127,16 @@ func (c *Config) parseAndValidate() error { if len(rc.Roles) == 0 { validationErrors = append(validationErrors, ErrRolesShouldNotBeEmpty) } + + // check for duplicates in roles + rolesMap := make(map[string]bool, 0) + for _, role := range rc.Roles { + if val, ok := rolesMap[role.ID]; ok && val { + validationErrors = append(validationErrors, fmt.Errorf("duplicate role: %q", role.ID)) + } else { + rolesMap[role.ID] = true + } + } } if len(validationErrors) > 0 { diff --git a/plugins/providers/gcloudiam/config_test.go b/plugins/providers/gcloudiam/config_test.go index 390cd8354..25846b0e6 100644 --- a/plugins/providers/gcloudiam/config_test.go +++ b/plugins/providers/gcloudiam/config_test.go @@ -5,7 +5,9 @@ import ( "errors" "testing" + "github.com/raystack/guardian/domain" "github.com/raystack/guardian/mocks" + "github.com/raystack/guardian/pkg/crypto" "github.com/raystack/guardian/plugins/providers/gcloudiam" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -93,3 +95,148 @@ func TestCredentials(t *testing.T) { }) }) } + +func TestConfig_ParseAndValidate(t *testing.T) { + + t.Run("should return error if resource config is nil", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: getBase64EncodedString(), + ResourceName: "projects/test-project", + }, + Resources: nil, + } + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.EqualError(t, actualErr, "empty resource config") + }) + + t.Run("should return error if service account key is not base64", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: "test-service-account-key", + ResourceName: "projects/test-project", + }, + Resources: nil, + } + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.NotNil(t, actualErr) + }) + t.Run("should return error if duplicate resource type is present", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: getBase64EncodedString(), + ResourceName: "projects/test-project", + }, + Resources: []*domain.ResourceConfig{ + { + Type: "project", + Roles: []*domain.Role{ + {ID: "test-roleA", Name: "test role A", Permissions: []interface{}{"test-permission-a"}}, + }, + }, + { + Type: "project", + Roles: []*domain.Role{ + {ID: "test-roleB", Name: "test role B", Permissions: []interface{}{"test-permission-b"}}, + }}, + }, + } + expectedErr := "duplicate resource type: \"project\"" + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.Equal(t, expectedErr, actualErr.Error()) + }) + + t.Run("should return error for invalid resource type", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: getBase64EncodedString(), + ResourceName: "projects/test-project", + }, + Resources: []*domain.ResourceConfig{ + {Type: "invalid-resource-type"}, + }, + } + expectedErr := "invalid resource type: \"invalid-resource-type\"\ngcloud_iam provider should not have empty roles" + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.Equal(t, expectedErr, actualErr.Error()) + }) + + t.Run("should return nil if config is valid", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: getBase64EncodedString(), + ResourceName: "projects/test-project", + }, + Resources: []*domain.ResourceConfig{ + { + Type: "project", + Roles: []*domain.Role{ + {ID: "test-role", Name: "test role", Permissions: []interface{}{"test-permission"}}, + }, + }, + }, + } + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.Nil(t, actualErr) + }) + + t.Run("should return error if duplicate role is configured", func(t *testing.T) { + crypo := crypto.NewAES("encryption_key") + providerConfig := domain.ProviderConfig{ + Type: "gcloudiam", + URN: "test-urn", + Credentials: gcloudiam.Credentials{ + ServiceAccountKey: getBase64EncodedString(), + ResourceName: "projects/test-project", + }, + Resources: []*domain.ResourceConfig{ + { + Type: "project", + Roles: []*domain.Role{ + {ID: "test-role1", Name: "test role 1", Permissions: []interface{}{"test-permission-1"}}, + {ID: "test-role2", Name: "test role 2", Permissions: []interface{}{"test-permission-2"}}, + {ID: "test-role1", Name: "test role 1", Permissions: []interface{}{"test-permission-11"}}, + {ID: "test-role3", Name: "test role 3", Permissions: []interface{}{"test-permission-3"}}, + }, + }, + }, + } + + expectedErr := "duplicate role: \"test-role1\"" + + config := gcloudiam.NewConfig(&providerConfig, crypo) + + actualErr := config.ParseAndValidate() + assert.EqualError(t, actualErr, expectedErr) + }) + +} + +func getBase64EncodedString() string { + return base64.StdEncoding.EncodeToString([]byte("test-service-account-key")) +} diff --git a/plugins/providers/gcloudiam/provider.go b/plugins/providers/gcloudiam/provider.go index 84dbb958e..8ab3b425c 100644 --- a/plugins/providers/gcloudiam/provider.go +++ b/plugins/providers/gcloudiam/provider.go @@ -7,6 +7,7 @@ import ( "github.com/mitchellh/mapstructure" "github.com/raystack/guardian/core/provider" "github.com/raystack/guardian/domain" + "github.com/raystack/salt/log" "golang.org/x/net/context" "google.golang.org/api/iam/v1" ) @@ -34,13 +35,15 @@ type Provider struct { typeName string Clients map[string]GcloudIamClient crypto encryptor + logger log.Logger } -func NewProvider(typeName string, crypto encryptor) *Provider { +func NewProvider(typeName string, crypto encryptor, logger log.Logger) *Provider { return &Provider{ typeName: typeName, Clients: map[string]GcloudIamClient{}, crypto: crypto, + logger: logger, } } diff --git a/plugins/providers/gcloudiam/provider_test.go b/plugins/providers/gcloudiam/provider_test.go index 53210bd73..7400dc996 100644 --- a/plugins/providers/gcloudiam/provider_test.go +++ b/plugins/providers/gcloudiam/provider_test.go @@ -6,6 +6,8 @@ import ( "errors" "testing" + "github.com/raystack/salt/log" + "github.com/raystack/guardian/domain" "github.com/raystack/guardian/plugins/providers/gcloudiam" "github.com/raystack/guardian/plugins/providers/gcloudiam/mocks" @@ -19,7 +21,8 @@ func TestCreateConfig(t *testing.T) { providerURN := "test-URN" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -92,7 +95,8 @@ func TestCreateConfig(t *testing.T) { providerURN := "test-URN" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -182,7 +186,8 @@ func TestCreateConfig(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -230,7 +235,8 @@ func TestCreateConfig(t *testing.T) { providerURN := "test-URN" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -279,7 +285,8 @@ func TestGetType(t *testing.T) { t.Run("should return provider type name", func(t *testing.T) { expectedTypeName := domain.ProviderTypeMetabase crypto := new(mocks.Encryptor) - p := gcloudiam.NewProvider(expectedTypeName, crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider(expectedTypeName, crypto, l) actualTypeName := p.GetType() @@ -290,7 +297,8 @@ func TestGetType(t *testing.T) { func TestGetResources(t *testing.T) { t.Run("should error when credentials are invalid", func(t *testing.T) { crypto := new(mocks.Encryptor) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) pc := &domain.ProviderConfig{ Type: domain.ProviderTypeGCloudIAM, URN: "test-project-id", @@ -309,7 +317,8 @@ func TestGetResources(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -518,7 +527,8 @@ func TestGetResources(t *testing.T) { func TestGrantAccess(t *testing.T) { t.Run("should return error if credentials is invalid", func(t *testing.T) { crypto := new(mocks.Encryptor) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) pc := &domain.ProviderConfig{ Credentials: "invalid-credentials", @@ -542,7 +552,8 @@ func TestGrantAccess(t *testing.T) { t.Run("should return error if resource type is unknown", func(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) crypto.On("Decrypt", "c2VydmljZS1hY2NvdW50LWtleS1qc29u").Return(`{"type":"service_account"}`, nil) // tests the newIamClient when p.Clients is not initialised in the provider config expectedError := errors.New("invalid resource type") @@ -615,7 +626,8 @@ func TestGrantAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -663,11 +675,12 @@ func TestGrantAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) + l := log.NewNoop() expectedRole := "role-1" expectedAccountType := "user" expectedAccountID := "test@email.com" expectedPermission := "roles/bigquery.admin" - p := gcloudiam.NewProvider("", crypto) + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -715,7 +728,8 @@ func TestGrantAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -759,7 +773,8 @@ func TestRevokeAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -830,7 +845,8 @@ func TestRevokeAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -882,7 +898,8 @@ func TestRevokeAccess(t *testing.T) { expectedPermission := "roles/bigquery.admin" expectedAccountType := "user" expectedAccountID := "test@email.com" - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -929,7 +946,8 @@ func TestRevokeAccess(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -974,7 +992,8 @@ func TestGetRoles(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -997,6 +1016,7 @@ func TestGetRoles(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) + l := log.NewNoop() expectedRoles := []*domain.Role{ { ID: "role-1", @@ -1005,7 +1025,7 @@ func TestGetRoles(t *testing.T) { }, } - p := gcloudiam.NewProvider("", crypto) + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -1039,9 +1059,10 @@ func TestGetPermissions(t *testing.T) { providerURN := "test-provider-urn" crypto := new(mocks.Encryptor) client := new(mocks.GcloudIamClient) + l := log.NewNoop() expectedPermissions := []interface{}{"roles/bigquery.admin"} - p := gcloudiam.NewProvider("", crypto) + p := gcloudiam.NewProvider("", crypto, l) p.Clients = map[string]gcloudiam.GcloudIamClient{ providerURN: client, } @@ -1074,7 +1095,8 @@ func TestGetAccountTypes(t *testing.T) { t.Run("should return the supported account types: user, serviceAccount", func(t *testing.T) { expectedAccountTypes := []string{"user", "serviceAccount", "group"} crypto := new(mocks.Encryptor) - p := gcloudiam.NewProvider("", crypto) + l := log.NewNoop() + p := gcloudiam.NewProvider("", crypto, l) actualAccountTypes := p.GetAccountTypes()