fix(gcloudiam): add check for duplicate roles

raystack · Jun 9, 2024 · ed0f6fa · ed0f6fa
1 parent faf5807
commit ed0f6fa
Show file tree

Hide file tree

Showing 5 changed files with 204 additions and 22 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,
 	}
 }