From a0e6c1f7a8d278c6683ca6c9ca7344fc6fccdb22 Mon Sep 17 00:00:00 2001 From: Alex Ott Date: Tue, 4 Nov 2025 21:30:11 +0100 Subject: [PATCH] Alternative implementation of `databricks_permission` resource for managing permissions for individual principals. This resource provides fine-grained control over permissions by managing a single principal's access to a single object, unlike `databricks_permissions`, which manages all principals' access to an object at once. This is particularly useful for: - Managing permissions for different teams independently - Token and password authorization permissions that previously required all principals in one resource - Avoiding conflicts when multiple configurations manage different principals on the same object Caveat: Since we cannot remove an individual permission, the `Delete` operation is performed as `Read/Put`, so we need to use a lock around each object. --- NEXT_CHANGELOG.md | 5 +- docs/resources/permission.md | 90 +-- .../products/permissions/object_mutex.go | 54 +- .../products/permissions/object_mutex_test.go | 132 ++--- .../permissions/permission_level_validator.go | 37 +- .../permission_level_validator_test.go | 188 +++++- .../permissions/resource_permission.go | 557 ++++++------------ .../resource_permission_acc_test.go | 88 ++- .../permissions/resource_permission_test.go | 129 ++-- 9 files changed, 655 insertions(+), 625 deletions(-) rename {permissions => internal/providers/pluginfw/products/permissions}/resource_permission_acc_test.go (80%) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 0edf166409..21191a7c22 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -6,12 +6,10 @@ ### New Features and Improvements -* Add `provider_config` support for manual plugin framework resources and data sources ([#5127](https://github.com/databricks/terraform-provider-databricks/pull/5127)) -* Added `databricks_permission` resource for managing permissions on Databricks objects for individual principals ([#5161](https://github.com/databricks/terraform-provider-databricks/pull/5161)). +* Added `databricks_permission` resource for managing permissions on Databricks objects for individual principals ([#5186](https://github.com/databricks/terraform-provider-databricks/pull/5186)). ### Bug Fixes -* Remove unnecessary `SetSuppressDiff()` for `workload_size` in `databricks_model_serving` ([#5152](https://github.com/databricks/terraform-provider-databricks/pull/5152)). ### Documentation @@ -19,4 +17,3 @@ ### Internal Changes -* Caching group membership in `databricks_group_member` to improve performance ([#4581](https://github.com/databricks/terraform-provider-databricks/pull/4581)). diff --git a/docs/resources/permission.md b/docs/resources/permission.md index efb30d96ed..27c4a1fb69 100644 --- a/docs/resources/permission.md +++ b/docs/resources/permission.md @@ -10,6 +10,8 @@ This resource allows you to manage permissions for a single principal on a Datab ~> This resource is _authoritative_ for the specified object-principal pair. Configuring this resource will manage the permission for the specified principal only, without affecting permissions for other principals. +~> **Warning:** Do not use both `databricks_permission` and `databricks_permissions` resources for the same object. This will cause conflicts as both resources manage the same permissions. + -> Use `databricks_permissions` when you need to manage all permissions for an object in a single resource. Use `databricks_permission` (singular) when you want to manage permissions for individual principals independently. ## Example Usage @@ -35,14 +37,16 @@ resource "databricks_group" "data_engineers" { # Grant CAN_RESTART permission to a group resource "databricks_permission" "cluster_de" { - cluster_id = databricks_cluster.shared.id + object_type = "clusters" + object_id = databricks_cluster.shared.id group_name = databricks_group.data_engineers.display_name permission_level = "CAN_RESTART" } # Grant CAN_ATTACH_TO permission to a user resource "databricks_permission" "cluster_analyst" { - cluster_id = databricks_cluster.shared.id + object_type = "clusters" + object_id = databricks_cluster.shared.id user_name = "analyst@company.com" permission_level = "CAN_ATTACH_TO" } @@ -71,14 +75,16 @@ resource "databricks_job" "etl" { # Grant CAN_MANAGE to a service principal resource "databricks_permission" "job_sp" { - job_id = databricks_job.etl.id + object_type = "jobs" + object_id = databricks_job.etl.id service_principal_name = databricks_service_principal.automation.application_id permission_level = "CAN_MANAGE" } # Grant CAN_VIEW to a group resource "databricks_permission" "job_viewers" { - job_id = databricks_job.etl.id + object_type = "jobs" + object_id = databricks_job.etl.id group_name = "Data Viewers" permission_level = "CAN_VIEW" } @@ -99,14 +105,16 @@ resource "databricks_notebook" "analysis" { # Grant CAN_RUN to a user resource "databricks_permission" "notebook_user" { - notebook_path = databricks_notebook.analysis.path + object_type = "notebooks" + object_id = databricks_notebook.analysis.path user_name = "data.scientist@company.com" permission_level = "CAN_RUN" } # Grant CAN_EDIT to a group resource "databricks_permission" "notebook_editors" { - notebook_path = databricks_notebook.analysis.path + object_type = "notebooks" + object_id = databricks_notebook.analysis.path group_name = "Notebook Editors" permission_level = "CAN_EDIT" } @@ -119,19 +127,22 @@ This resource solves the limitation where all token permissions must be defined ```hcl # Multiple resources can now manage different principals independently resource "databricks_permission" "tokens_team_a" { - authorization = "tokens" + object_type = "authorization" + object_id = "tokens" group_name = "Team A" permission_level = "CAN_USE" } resource "databricks_permission" "tokens_team_b" { - authorization = "tokens" + object_type = "authorization" + object_id = "tokens" group_name = "Team B" permission_level = "CAN_USE" } resource "databricks_permission" "tokens_service_account" { - authorization = "tokens" + object_type = "authorization" + object_id = "tokens" service_principal_name = databricks_service_principal.ci_cd.application_id permission_level = "CAN_USE" } @@ -147,7 +158,8 @@ resource "databricks_sql_endpoint" "analytics" { } resource "databricks_permission" "warehouse_users" { - sql_endpoint_id = databricks_sql_endpoint.analytics.id + object_type = "sql/warehouses" + object_id = databricks_sql_endpoint.analytics.id group_name = "SQL Users" permission_level = "CAN_USE" } @@ -157,6 +169,29 @@ resource "databricks_permission" "warehouse_users" { The following arguments are required: +* `object_type` - (Required) The type of object to manage permissions for. Valid values include: + * `clusters` - For cluster permissions + * `cluster-policies` - For cluster policy permissions + * `instance-pools` - For instance pool permissions + * `jobs` - For job permissions + * `pipelines` - For Delta Live Tables pipeline permissions + * `notebooks` - For notebook permissions (use path as `object_id`) + * `directories` - For directory permissions (use path as `object_id`) + * `workspace-files` - For workspace file permissions (use path as `object_id`) + * `registered-models` - For registered model permissions + * `experiments` - For experiment permissions + * `sql-dashboards` - For legacy SQL dashboard permissions + * `sql/warehouses` - For SQL warehouse permissions + * `queries` - For query permissions + * `alerts` - For alert permissions + * `dashboards` - For Lakeview dashboard permissions + * `repos` - For repo permissions + * `authorization` - For authorization permissions (use `tokens` or `passwords` as `object_id`) + * `serving-endpoints` - For model serving endpoint permissions + * `vector-search-endpoints` - For vector search endpoint permissions + +* `object_id` - (Required) The ID or path of the object. For notebooks, directories, and workspace files, use the path (e.g., `/Shared/notebook`). For authorization, use `tokens` or `passwords`. For other objects, use the resource ID. + * `permission_level` - (Required) The permission level to grant. The available permission levels depend on the object type. Common values include `CAN_MANAGE`, `CAN_USE`, `CAN_VIEW`, `CAN_RUN`, `CAN_EDIT`, `CAN_READ`, `CAN_RESTART`, `CAN_ATTACH_TO`. Exactly one of the following principal identifiers must be specified: @@ -165,32 +200,6 @@ Exactly one of the following principal identifiers must be specified: * `group_name` - (Optional) Group name to grant permissions to. Conflicts with `user_name` and `service_principal_name`. * `service_principal_name` - (Optional) Application ID of the service principal. Conflicts with `user_name` and `group_name`. -Exactly one of the following object identifiers must be specified: - -* `cluster_id` - (Optional) ID of the [databricks_cluster](cluster.md). -* `cluster_policy_id` - (Optional) ID of the [databricks_cluster_policy](cluster_policy.md). -* `instance_pool_id` - (Optional) ID of the [databricks_instance_pool](instance_pool.md). -* `job_id` - (Optional) ID of the [databricks_job](job.md). -* `pipeline_id` - (Optional) ID of the [databricks_pipeline](pipeline.md). -* `notebook_id` - (Optional) ID of the [databricks_notebook](notebook.md). Can be used when the notebook is referenced by ID. -* `notebook_path` - (Optional) Path to the [databricks_notebook](notebook.md). -* `directory_id` - (Optional) ID of the [databricks_directory](directory.md). -* `directory_path` - (Optional) Path to the [databricks_directory](directory.md). -* `workspace_file_id` - (Optional) ID of the [databricks_workspace_file](workspace_file.md). -* `workspace_file_path` - (Optional) Path to the [databricks_workspace_file](workspace_file.md). -* `registered_model_id` - (Optional) ID of the [databricks_mlflow_model](mlflow_model.md). -* `experiment_id` - (Optional) ID of the [databricks_mlflow_experiment](mlflow_experiment.md). -* `sql_dashboard_id` - (Optional) ID of the legacy [databricks_sql_dashboard](sql_dashboard.md). -* `sql_endpoint_id` - (Optional) ID of the [databricks_sql_endpoint](sql_endpoint.md). -* `sql_query_id` - (Optional) ID of the [databricks_query](query.md). -* `sql_alert_id` - (Optional) ID of the [databricks_alert](alert.md). -* `dashboard_id` - (Optional) ID of the [databricks_dashboard](dashboard.md) (Lakeview). -* `repo_id` - (Optional) ID of the [databricks_repo](repo.md). -* `repo_path` - (Optional) Path to the [databricks_repo](repo.md). -* `authorization` - (Optional) Type of authorization. Currently supports `tokens` and `passwords`. -* `serving_endpoint_id` - (Optional) ID of the [databricks_model_serving](model_serving.md) endpoint. -* `vector_search_endpoint_id` - (Optional) ID of the [databricks_vector_search_endpoint](vector_search_endpoint.md). - ## Attribute Reference In addition to all arguments above, the following attributes are exported: @@ -249,13 +258,15 @@ resource "databricks_permissions" "cluster_all" { ```hcl resource "databricks_permission" "cluster_de" { - cluster_id = databricks_cluster.shared.id + object_type = "clusters" + object_id = databricks_cluster.shared.id group_name = "Data Engineers" permission_level = "CAN_RESTART" } resource "databricks_permission" "cluster_analyst" { - cluster_id = databricks_cluster.shared.id + object_type = "clusters" + object_id = databricks_cluster.shared.id user_name = "analyst@company.com" permission_level = "CAN_ATTACH_TO" } @@ -263,7 +274,8 @@ resource "databricks_permission" "cluster_analyst" { # Adding a third principal is a separate resource # No need to modify existing resources resource "databricks_permission" "cluster_viewer" { - cluster_id = databricks_cluster.shared.id + object_type = "clusters" + object_id = databricks_cluster.shared.id group_name = "Viewers" permission_level = "CAN_ATTACH_TO" } diff --git a/internal/providers/pluginfw/products/permissions/object_mutex.go b/internal/providers/pluginfw/products/permissions/object_mutex.go index 06a06ceba4..cd2a6a89e7 100644 --- a/internal/providers/pluginfw/products/permissions/object_mutex.go +++ b/internal/providers/pluginfw/products/permissions/object_mutex.go @@ -1,10 +1,11 @@ package permissions import ( + "fmt" "sync" ) -// objectMutexManager manages mutexes per object ID to prevent concurrent +// objectMutexManager manages mutexes per object to prevent concurrent // operations on the same Databricks object that could lead to race conditions. // // This is particularly important for Delete operations where multiple @@ -12,51 +13,46 @@ import ( // concurrently, each doing GET -> filter -> SET, which could result in // lost permission updates. type objectMutexManager struct { - mutexes map[string]*sync.Mutex - mapLock sync.Mutex + mutexes sync.Map // map[string]*sync.Mutex } // globalObjectMutexManager is the singleton instance used by all permission resources -var globalObjectMutexManager = &objectMutexManager{ - mutexes: make(map[string]*sync.Mutex), -} +var globalObjectMutexManager = &objectMutexManager{} -// Lock acquires a mutex for the given object ID. -// Each object ID gets its own mutex to allow concurrent operations on different objects +// Lock acquires a mutex for the given object type and ID. +// Each object gets its own mutex to allow concurrent operations on different objects // while serializing operations on the same object. -func (m *objectMutexManager) Lock(objectID string) { - m.mapLock.Lock() - mu, exists := m.mutexes[objectID] - if !exists { - mu = &sync.Mutex{} - m.mutexes[objectID] = mu - } - m.mapLock.Unlock() +func (m *objectMutexManager) Lock(objectType, objectID string) { + key := fmt.Sprintf("%s/%s", objectType, objectID) + + // LoadOrStore returns the existing value if present, otherwise stores and returns the given value + value, _ := m.mutexes.LoadOrStore(key, &sync.Mutex{}) + mu := value.(*sync.Mutex) - // Lock the object-specific mutex (outside the map lock to avoid deadlock) + // Lock the object-specific mutex mu.Lock() } -// Unlock releases the mutex for the given object ID. -func (m *objectMutexManager) Unlock(objectID string) { - m.mapLock.Lock() - mu, exists := m.mutexes[objectID] - m.mapLock.Unlock() +// Unlock releases the mutex for the given object type and ID. +func (m *objectMutexManager) Unlock(objectType, objectID string) { + key := fmt.Sprintf("%s/%s", objectType, objectID) - if exists { + value, ok := m.mutexes.Load(key) + if ok { + mu := value.(*sync.Mutex) mu.Unlock() } } -// lockObject acquires a lock for the given object ID. +// lockObject acquires a lock for the given object type and ID. // This should be called at the start of any operation that modifies permissions. -func lockObject(objectID string) { - globalObjectMutexManager.Lock(objectID) +func lockObject(objectType, objectID string) { + globalObjectMutexManager.Lock(objectType, objectID) } -// unlockObject releases the lock for the given object ID. +// unlockObject releases the lock for the given object type and ID. // This should be called at the end of any operation that modifies permissions. // Use defer to ensure it's always called even if the operation panics. -func unlockObject(objectID string) { - globalObjectMutexManager.Unlock(objectID) +func unlockObject(objectType, objectID string) { + globalObjectMutexManager.Unlock(objectType, objectID) } diff --git a/internal/providers/pluginfw/products/permissions/object_mutex_test.go b/internal/providers/pluginfw/products/permissions/object_mutex_test.go index cc58bab7a0..36d0400ce4 100644 --- a/internal/providers/pluginfw/products/permissions/object_mutex_test.go +++ b/internal/providers/pluginfw/products/permissions/object_mutex_test.go @@ -10,42 +10,40 @@ import ( ) func TestObjectMutexManager_LockUnlock(t *testing.T) { - manager := &objectMutexManager{ - mutexes: make(map[string]*sync.Mutex), - } + manager := &objectMutexManager{} - objectID := "/clusters/test-cluster" + objectType := "clusters" + objectID := "test-cluster" // First lock should succeed immediately - manager.Lock(objectID) + manager.Lock(objectType, objectID) - // Verify mutex was created - manager.mapLock.Lock() - _, exists := manager.mutexes[objectID] - manager.mapLock.Unlock() - assert.True(t, exists, "Mutex should be created for object ID") + // Verify mutex was created by checking we can load it + key := "clusters/test-cluster" + _, exists := manager.mutexes.Load(key) + assert.True(t, exists, "Mutex should be created for object") // Unlock - manager.Unlock(objectID) + manager.Unlock(objectType, objectID) } func TestObjectMutexManager_ConcurrentAccess(t *testing.T) { - manager := &objectMutexManager{ - mutexes: make(map[string]*sync.Mutex), - } + manager := &objectMutexManager{} - objectID := "/clusters/test-cluster" + objectType := "clusters" + objectID := "test-cluster" var counter int32 var wg sync.WaitGroup // Simulate 10 concurrent operations on the same object + // The mutex should ensure they execute serially for i := 0; i < 10; i++ { wg.Add(1) go func() { defer wg.Done() - manager.Lock(objectID) - defer manager.Unlock(objectID) + manager.Lock(objectType, objectID) + defer manager.Unlock(objectType, objectID) // Simulate some work with a counter // Without proper locking, this would cause race conditions @@ -62,12 +60,12 @@ func TestObjectMutexManager_ConcurrentAccess(t *testing.T) { } func TestObjectMutexManager_DifferentObjects(t *testing.T) { - manager := &objectMutexManager{ - mutexes: make(map[string]*sync.Mutex), - } + manager := &objectMutexManager{} - objectID1 := "/clusters/cluster-1" - objectID2 := "/clusters/cluster-2" + objectType1 := "clusters" + objectID1 := "cluster-1" + objectType2 := "clusters" + objectID2 := "cluster-2" var wg sync.WaitGroup results := make([]string, 0, 2) @@ -78,8 +76,8 @@ func TestObjectMutexManager_DifferentObjects(t *testing.T) { go func() { defer wg.Done() - manager.Lock(objectID1) - defer manager.Unlock(objectID1) + manager.Lock(objectType1, objectID1) + defer manager.Unlock(objectType1, objectID1) time.Sleep(50 * time.Millisecond) resultsMutex.Lock() @@ -89,8 +87,8 @@ func TestObjectMutexManager_DifferentObjects(t *testing.T) { go func() { defer wg.Done() - manager.Lock(objectID2) - defer manager.Unlock(objectID2) + manager.Lock(objectType2, objectID2) + defer manager.Unlock(objectType2, objectID2) time.Sleep(50 * time.Millisecond) resultsMutex.Lock() @@ -104,64 +102,44 @@ func TestObjectMutexManager_DifferentObjects(t *testing.T) { assert.Len(t, results, 2, "Both operations should complete") } -func TestObjectMutexManager_SerializeSameObject(t *testing.T) { - manager := &objectMutexManager{ - mutexes: make(map[string]*sync.Mutex), - } - - objectID := "/jobs/job-123" - var executionOrder []int - var orderMutex sync.Mutex - var wg sync.WaitGroup - - // Launch 3 goroutines that should execute serially - for i := 1; i <= 3; i++ { - wg.Add(1) - go func(num int) { - defer wg.Done() - - manager.Lock(objectID) - defer manager.Unlock(objectID) - - // Record execution order - orderMutex.Lock() - executionOrder = append(executionOrder, num) - orderMutex.Unlock() +func TestObjectMutexManager_DifferentObjectTypes(t *testing.T) { + manager := &objectMutexManager{} - time.Sleep(10 * time.Millisecond) // Simulate work - }(i) - } - - wg.Wait() + objectType1 := "clusters" + objectType2 := "jobs" + objectID := "same-id" // Same ID but different types - // All 3 operations should complete - assert.Len(t, executionOrder, 3, "All operations should complete") + var wg sync.WaitGroup + results := make([]string, 0, 2) + var resultsMutex sync.Mutex - // Operations should be serialized (no concurrent execution) - // We can't predict the exact order, but we can verify no race conditions occurred -} + // Operations on different object types should run concurrently even with same ID + wg.Add(2) -func TestGlobalObjectMutexManager(t *testing.T) { - // Test the global singleton - objectID := "/notebooks/notebook-456" + go func() { + defer wg.Done() + manager.Lock(objectType1, objectID) + defer manager.Unlock(objectType1, objectID) - lockObject(objectID) + time.Sleep(50 * time.Millisecond) + resultsMutex.Lock() + results = append(results, "clusters") + resultsMutex.Unlock() + }() - // Verify we can unlock without error - unlockObject(objectID) + go func() { + defer wg.Done() + manager.Lock(objectType2, objectID) + defer manager.Unlock(objectType2, objectID) - // Should be able to lock again after unlocking - lockObject(objectID) - unlockObject(objectID) -} + time.Sleep(50 * time.Millisecond) + resultsMutex.Lock() + results = append(results, "jobs") + resultsMutex.Unlock() + }() -func TestObjectMutexManager_UnlockNonexistent(t *testing.T) { - manager := &objectMutexManager{ - mutexes: make(map[string]*sync.Mutex), - } + wg.Wait() - // Unlocking a non-existent object should not panic - assert.NotPanics(t, func() { - manager.Unlock("/clusters/does-not-exist") - }, "Unlocking non-existent object should not panic") + // Both operations should complete (order doesn't matter) + assert.Len(t, results, 2, "Both operations on different object types should complete concurrently") } diff --git a/internal/providers/pluginfw/products/permissions/permission_level_validator.go b/internal/providers/pluginfw/products/permissions/permission_level_validator.go index b997734615..e44e3845aa 100644 --- a/internal/providers/pluginfw/products/permissions/permission_level_validator.go +++ b/internal/providers/pluginfw/products/permissions/permission_level_validator.go @@ -10,15 +10,18 @@ import ( "github.com/hashicorp/terraform-plugin-framework/types" ) -// permissionLevelValidator validates that the permission_level is valid for the configured object type +// permissionLevelValidator validates that the permission_level is valid for the configured object type. +// It uses a hybrid approach: +// 1. If the object type is known in permission_definitions.go, validate against allowed levels +// 2. If the object type is unknown (new type), skip validation and let the API handle it type permissionLevelValidator struct{} func (v permissionLevelValidator) Description(ctx context.Context) string { - return "validates that the permission level is valid for the configured object type" + return "validates that the permission level is valid for the configured object type when the object type is known" } func (v permissionLevelValidator) MarkdownDescription(ctx context.Context) string { - return "validates that the permission level is valid for the configured object type" + return "validates that the permission level is valid for the configured object type when the object type is known" } func (v permissionLevelValidator) ValidateString(ctx context.Context, req validator.StringRequest, resp *validator.StringResponse) { @@ -28,19 +31,23 @@ func (v permissionLevelValidator) ValidateString(ctx context.Context, req valida permissionLevel := req.ConfigValue.ValueString() - // Dynamically iterate through all permission definitions to find which object ID is set + // Get the object_type from the configuration + var objectType types.String + diags := req.Config.GetAttribute(ctx, path.Root("object_type"), &objectType) + if diags.HasError() || objectType.IsNull() || objectType.IsUnknown() { + // Can't validate without object_type, let the API handle it + return + } + + objectTypeValue := objectType.ValueString() + + // Try to find the permission mapping for this object type allPermissions := permissions.AllResourcePermissions() var mapping permissions.WorkspaceObjectPermissions var found bool for _, m := range allPermissions { - var attrValue types.String - diags := req.Config.GetAttribute(ctx, path.Root(m.GetField()), &attrValue) - if diags.HasError() { - continue // Attribute doesn't exist or has errors, try next - } - - if !attrValue.IsNull() && !attrValue.IsUnknown() && attrValue.ValueString() != "" { + if m.GetRequestObjectType() == objectTypeValue { mapping = m found = true break @@ -48,7 +55,8 @@ func (v permissionLevelValidator) ValidateString(ctx context.Context, req valida } if !found { - // If we can't determine the object type, let the ConflictsWith validators handle it + // Object type not found in our definitions - this might be a new object type + // Let the API handle validation return } @@ -71,14 +79,15 @@ func (v permissionLevelValidator) ValidateString(ctx context.Context, req valida fmt.Sprintf( "Permission level %q is not valid for object type %q. Allowed levels: %v", permissionLevel, - mapping.GetObjectType(), + objectTypeValue, allowedLevels, ), ) } } -// ValidatePermissionLevel returns a validator that checks if the permission level is valid for the object type +// ValidatePermissionLevel returns a validator that checks if the permission level is valid for the object type. +// Uses a hybrid approach: validates against known object types, lets API handle unknown ones. func ValidatePermissionLevel() validator.String { return permissionLevelValidator{} } diff --git a/internal/providers/pluginfw/products/permissions/permission_level_validator_test.go b/internal/providers/pluginfw/products/permissions/permission_level_validator_test.go index ce1d69f6c8..0c74a9871a 100644 --- a/internal/providers/pluginfw/products/permissions/permission_level_validator_test.go +++ b/internal/providers/pluginfw/products/permissions/permission_level_validator_test.go @@ -4,21 +4,187 @@ import ( "context" "testing" + "github.com/hashicorp/terraform-plugin-framework/path" + "github.com/hashicorp/terraform-plugin-framework/resource/schema" + "github.com/hashicorp/terraform-plugin-framework/schema/validator" + "github.com/hashicorp/terraform-plugin-framework/tfsdk" + "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-go/tftypes" "github.com/stretchr/testify/assert" ) -func TestPermissionLevelValidator_Description(t *testing.T) { - validator := ValidatePermissionLevel() +func TestPermissionLevelValidator_KnownObjectType(t *testing.T) { + v := permissionLevelValidator{} - desc := validator.Description(context.Background()) - assert.NotEmpty(t, desc) - assert.Contains(t, desc, "permission level") + tests := []struct { + name string + objectType string + permissionLevel string + expectError bool + errorContains string + }{ + { + name: "valid cluster permission", + objectType: "clusters", + permissionLevel: "CAN_ATTACH_TO", + expectError: false, + }, + { + name: "valid cluster permission - CAN_RESTART", + objectType: "clusters", + permissionLevel: "CAN_RESTART", + expectError: false, + }, + { + name: "invalid cluster permission", + objectType: "clusters", + permissionLevel: "INVALID_PERMISSION", + expectError: true, + errorContains: "not valid for object type", + }, + { + name: "valid job permission", + objectType: "jobs", + permissionLevel: "CAN_VIEW", + expectError: false, + }, + { + name: "valid authorization permission", + objectType: "authorization", + permissionLevel: "CAN_USE", + expectError: false, + }, + } - mdDesc := validator.MarkdownDescription(context.Background()) - assert.NotEmpty(t, mdDesc) - assert.Contains(t, mdDesc, "permission level") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a test config + configValues := map[string]tftypes.Value{ + "object_type": tftypes.NewValue(tftypes.String, tt.objectType), + "permission_level": tftypes.NewValue(tftypes.String, tt.permissionLevel), + } + + config := tfsdk.Config{ + Schema: schema.Schema{ + Attributes: map[string]schema.Attribute{ + "object_type": schema.StringAttribute{ + Required: true, + }, + "permission_level": schema.StringAttribute{ + Required: true, + }, + }, + }, + Raw: tftypes.NewValue(tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "object_type": tftypes.String, + "permission_level": tftypes.String, + }, + }, configValues), + } + + req := validator.StringRequest{ + Path: path.Root("permission_level"), + ConfigValue: types.StringValue(tt.permissionLevel), + Config: config, + } + + resp := &validator.StringResponse{} + + v.ValidateString(context.Background(), req, resp) + + if tt.expectError { + assert.True(t, resp.Diagnostics.HasError(), "Expected error but got none") + if tt.errorContains != "" { + assert.Contains(t, resp.Diagnostics.Errors()[0].Detail(), tt.errorContains) + } + } else { + assert.False(t, resp.Diagnostics.HasError(), "Expected no error but got: %v", resp.Diagnostics.Errors()) + } + }) + } +} + +func TestPermissionLevelValidator_UnknownObjectType(t *testing.T) { + v := permissionLevelValidator{} + + // Test with an unknown object type - should not error (let API handle it) + configValues := map[string]tftypes.Value{ + "object_type": tftypes.NewValue(tftypes.String, "new-unknown-type"), + "permission_level": tftypes.NewValue(tftypes.String, "SOME_PERMISSION"), + } + + config := tfsdk.Config{ + Schema: schema.Schema{ + Attributes: map[string]schema.Attribute{ + "object_type": schema.StringAttribute{ + Required: true, + }, + "permission_level": schema.StringAttribute{ + Required: true, + }, + }, + }, + Raw: tftypes.NewValue(tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "object_type": tftypes.String, + "permission_level": tftypes.String, + }, + }, configValues), + } + + req := validator.StringRequest{ + Path: path.Root("permission_level"), + ConfigValue: types.StringValue("SOME_PERMISSION"), + Config: config, + } + + resp := &validator.StringResponse{} + + v.ValidateString(context.Background(), req, resp) + + // Should NOT error - let API handle validation for unknown types + assert.False(t, resp.Diagnostics.HasError(), "Should not error for unknown object type") } -// Note: Full validation testing with config is done in acceptance tests -// The validator requires access to the full config to determine object type, -// which is complex to mock in unit tests but straightforward in integration tests +func TestPermissionLevelValidator_MissingObjectType(t *testing.T) { + v := permissionLevelValidator{} + + // Test with null object_type - should not error (let API handle it) + configValues := map[string]tftypes.Value{ + "object_type": tftypes.NewValue(tftypes.String, nil), + "permission_level": tftypes.NewValue(tftypes.String, "CAN_USE"), + } + + config := tfsdk.Config{ + Schema: schema.Schema{ + Attributes: map[string]schema.Attribute{ + "object_type": schema.StringAttribute{ + Required: true, + }, + "permission_level": schema.StringAttribute{ + Required: true, + }, + }, + }, + Raw: tftypes.NewValue(tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "object_type": tftypes.String, + "permission_level": tftypes.String, + }, + }, configValues), + } + + req := validator.StringRequest{ + Path: path.Root("permission_level"), + ConfigValue: types.StringValue("CAN_USE"), + Config: config, + } + + resp := &validator.StringResponse{} + + v.ValidateString(context.Background(), req, resp) + + // Should NOT error - let API handle validation when object_type is missing + assert.False(t, resp.Diagnostics.HasError(), "Should not error when object_type is null") +} diff --git a/internal/providers/pluginfw/products/permissions/resource_permission.go b/internal/providers/pluginfw/products/permissions/resource_permission.go index 0e7e9fd47c..158d63e6fe 100644 --- a/internal/providers/pluginfw/products/permissions/resource_permission.go +++ b/internal/providers/pluginfw/products/permissions/resource_permission.go @@ -5,12 +5,10 @@ import ( "fmt" "strings" - "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/apierr" "github.com/databricks/databricks-sdk-go/service/iam" "github.com/databricks/terraform-provider-databricks/common" pluginfwcommon "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/common" - "github.com/databricks/terraform-provider-databricks/permissions" "github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator" "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/path" @@ -36,12 +34,10 @@ type PermissionResource struct { } // PermissionResourceModel represents the Terraform resource model -// Note: Object identifiers are NOT defined in the struct - they are read/written dynamically -// using GetAttribute()/SetAttribute(). This eliminates the need for hardcoded fields -// and makes the code truly generic - new permission types require zero changes here. type PermissionResourceModel struct { ID types.String `tfsdk:"id"` ObjectType types.String `tfsdk:"object_type"` + ObjectID types.String `tfsdk:"object_id"` // Principal identifiers - exactly one required UserName types.String `tfsdk:"user_name"` @@ -50,10 +46,6 @@ type PermissionResourceModel struct { // Permission level PermissionLevel types.String `tfsdk:"permission_level"` - - // Note: Object identifiers (cluster_id, job_id, etc.) are NOT defined here. - // They are accessed dynamically using GetAttribute()/SetAttribute() based on - // the definitions in permissions/permission_definitions.go } func (r *PermissionResource) Metadata(ctx context.Context, req resource.MetadataRequest, resp *resource.MetadataResponse) { @@ -61,104 +53,81 @@ func (r *PermissionResource) Metadata(ctx context.Context, req resource.Metadata } func (r *PermissionResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) { - // Collect all object identifier field names for ConflictsWith validators - allPermissions := permissions.AllResourcePermissions() - objectFieldPaths := make([]path.Expression, 0, len(allPermissions)) - for _, mapping := range allPermissions { - objectFieldPaths = append(objectFieldPaths, path.MatchRoot(mapping.GetField())) - } - - attrs := map[string]schema.Attribute{ - "id": schema.StringAttribute{ - Computed: true, - PlanModifiers: []planmodifier.String{ - stringplanmodifier.UseStateForUnknown(), - }, - }, - "object_type": schema.StringAttribute{ - Computed: true, - }, - // Principal identifiers - exactly one required, mutually exclusive - "user_name": schema.StringAttribute{ - Optional: true, - Description: "User name of the principal. Conflicts with group_name and service_principal_name.", - Validators: []validator.String{ - stringvalidator.ConflictsWith( - path.MatchRoot("group_name"), - path.MatchRoot("service_principal_name"), - ), + resp.Schema = schema.Schema{ + Description: "Manages permissions for a single principal on a Databricks object. " + + "This resource is authoritative for the specified object-principal pair only. " + + "Use `databricks_permissions` for managing all principals on an object at once.", + Attributes: map[string]schema.Attribute{ + "id": schema.StringAttribute{ + Computed: true, + PlanModifiers: []planmodifier.String{ + stringplanmodifier.UseStateForUnknown(), + }, }, - PlanModifiers: []planmodifier.String{ - stringplanmodifier.RequiresReplace(), + "object_type": schema.StringAttribute{ + Required: true, + Description: "The type of object to manage permissions for (e.g., 'clusters', 'jobs', 'notebooks', 'authorization'). See the Databricks Permissions API documentation for the full list of supported object types.", + PlanModifiers: []planmodifier.String{ + stringplanmodifier.RequiresReplace(), + }, }, - }, - "group_name": schema.StringAttribute{ - Optional: true, - Description: "Group name of the principal. Conflicts with user_name and service_principal_name.", - Validators: []validator.String{ - stringvalidator.ConflictsWith( - path.MatchRoot("user_name"), - path.MatchRoot("service_principal_name"), - ), + "object_id": schema.StringAttribute{ + Required: true, + Description: "The ID of the object to manage permissions for. The format depends on the object type.", + PlanModifiers: []planmodifier.String{ + stringplanmodifier.RequiresReplace(), + }, }, - PlanModifiers: []planmodifier.String{ - stringplanmodifier.RequiresReplace(), + // Principal identifiers - exactly one required, mutually exclusive + "user_name": schema.StringAttribute{ + Optional: true, + Description: "User name of the principal. Conflicts with group_name and service_principal_name.", + Validators: []validator.String{ + stringvalidator.ConflictsWith( + path.MatchRoot("group_name"), + path.MatchRoot("service_principal_name"), + ), + }, + PlanModifiers: []planmodifier.String{ + stringplanmodifier.RequiresReplace(), + }, }, - }, - "service_principal_name": schema.StringAttribute{ - Optional: true, - Description: "Service principal name. Conflicts with user_name and group_name.", - Validators: []validator.String{ - stringvalidator.ConflictsWith( - path.MatchRoot("user_name"), - path.MatchRoot("group_name"), - ), + "group_name": schema.StringAttribute{ + Optional: true, + Description: "Group name of the principal. Conflicts with user_name and service_principal_name.", + Validators: []validator.String{ + stringvalidator.ConflictsWith( + path.MatchRoot("user_name"), + path.MatchRoot("service_principal_name"), + ), + }, + PlanModifiers: []planmodifier.String{ + stringplanmodifier.RequiresReplace(), + }, }, - PlanModifiers: []planmodifier.String{ - stringplanmodifier.RequiresReplace(), + "service_principal_name": schema.StringAttribute{ + Optional: true, + Description: "Service principal name. Conflicts with user_name and group_name.", + Validators: []validator.String{ + stringvalidator.ConflictsWith( + path.MatchRoot("user_name"), + path.MatchRoot("group_name"), + ), + }, + PlanModifiers: []planmodifier.String{ + stringplanmodifier.RequiresReplace(), + }, }, - }, - // Permission level - "permission_level": schema.StringAttribute{ - Required: true, - Description: "Permission level for the principal on the object (e.g., CAN_MANAGE, CAN_USE, CAN_VIEW).", - Validators: []validator.String{ - ValidatePermissionLevel(), + // Permission level + "permission_level": schema.StringAttribute{ + Required: true, + Description: "Permission level for the principal on the object (e.g., CAN_MANAGE, CAN_USE, CAN_VIEW). See the Databricks Permissions API documentation for valid permission levels for each object type.", + Validators: []validator.String{ + ValidatePermissionLevel(), + }, }, }, } - - // Dynamically add object identifier attributes from permission definitions - // Each object identifier is mutually exclusive with all others - for i, mapping := range allPermissions { - fieldName := mapping.GetField() - - // Build ConflictsWith list - all other object fields except this one - conflictPaths := make([]path.Expression, 0, len(objectFieldPaths)-1) - for j, p := range objectFieldPaths { - if i != j { - conflictPaths = append(conflictPaths, p) - } - } - - attrs[fieldName] = schema.StringAttribute{ - Optional: true, - Description: fmt.Sprintf("ID or path for %s object type. Conflicts with all other object identifier attributes.", mapping.GetObjectType()), - Validators: []validator.String{ - stringvalidator.ConflictsWith(conflictPaths...), - }, - PlanModifiers: []planmodifier.String{ - stringplanmodifier.RequiresReplace(), - }, - } - } - - resp.Schema = schema.Schema{ - Description: "Manages permissions for a single principal on a Databricks object. " + - "This resource is authoritative for the specified object-principal pair only. " + - "Use `databricks_permissions` for managing all principals on an object at once.", - Attributes: attrs, - } } func (r *PermissionResource) Configure(ctx context.Context, req resource.ConfigureRequest, resp *resource.ConfigureResponse) { @@ -177,87 +146,27 @@ func (r *PermissionResource) Configure(ctx context.Context, req resource.Configu } func (r *PermissionResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) { - // Read principal and permission_level using GetAttribute - var userName, groupName, servicePrincipalName, permissionLevel types.String - - resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("user_name"), &userName)...) - resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("group_name"), &groupName)...) - resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("service_principal_name"), &servicePrincipalName)...) - resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("permission_level"), &permissionLevel)...) - + var plan PermissionResourceModel + resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...) if resp.Diagnostics.HasError() { return } - w, err := r.Client.WorkspaceClient() - if err != nil { - resp.Diagnostics.AddError("Failed to get workspace client", err.Error()) - return - } - - // Get the object mapping and ID (reads object identifiers dynamically from plan) - mapping, objectID, objectFieldName, objectFieldValue, err := r.getObjectMappingAndID(ctx, w, req.Plan) - if err != nil { - resp.Diagnostics.AddError("Failed to get object mapping", err.Error()) - return - } - - // Lock the object to prevent concurrent modifications - lockObject(objectID) - defer unlockObject(objectID) - - // Determine principal identifier - var principal string - if !userName.IsNull() && !userName.IsUnknown() && userName.ValueString() != "" { - principal = userName.ValueString() - } else if !groupName.IsNull() && !groupName.IsUnknown() && groupName.ValueString() != "" { - principal = groupName.ValueString() - } else if !servicePrincipalName.IsNull() && !servicePrincipalName.IsUnknown() && servicePrincipalName.ValueString() != "" { - principal = servicePrincipalName.ValueString() - } else { - resp.Diagnostics.AddError("Invalid principal configuration", "exactly one of 'user_name', 'group_name', or 'service_principal_name' must be set") + r.upsertPermission(ctx, &plan, &resp.Diagnostics) + if resp.Diagnostics.HasError() { return } - // Create the permission update request - permLevel := iam.PermissionLevel(permissionLevel.ValueString()) - - // Use Update API (PATCH) to add permissions for this principal only - idParts := strings.Split(objectID, "/") - permID := idParts[len(idParts)-1] - - _, err = w.Permissions.Update(ctx, iam.UpdateObjectPermissions{ - RequestObjectId: permID, - RequestObjectType: mapping.GetRequestObjectType(), - AccessControlList: []iam.AccessControlRequest{ - { - UserName: userName.ValueString(), - GroupName: groupName.ValueString(), - ServicePrincipalName: servicePrincipalName.ValueString(), - PermissionLevel: permLevel, - }, - }, - }) - if err != nil { - resp.Diagnostics.AddError("Failed to create permission", err.Error()) - return - } + // Set computed fields + principal := r.getPrincipalFromModel(&plan) + plan.ID = types.StringValue(fmt.Sprintf("%s/%s/%s", plan.ObjectType.ValueString(), plan.ObjectID.ValueString(), principal)) - // Set the ID, object_type, and all other fields in state - resourceID := fmt.Sprintf("%s/%s", objectID, principal) - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("id"), types.StringValue(resourceID))...) - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("object_type"), types.StringValue(mapping.GetRequestObjectType()))...) - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root(objectFieldName), objectFieldValue)...) // Set the object identifier field - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("user_name"), userName)...) - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("group_name"), groupName)...) - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("service_principal_name"), servicePrincipalName)...) - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("permission_level"), permissionLevel)...) + resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...) } func (r *PermissionResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) { - // Read ID from state using GetAttribute - var id types.String - resp.Diagnostics.Append(req.State.GetAttribute(ctx, path.Root("id"), &id)...) + var state PermissionResourceModel + resp.Diagnostics.Append(req.State.Get(ctx, &state)...) if resp.Diagnostics.HasError() { return } @@ -268,27 +177,17 @@ func (r *PermissionResource) Read(ctx context.Context, req resource.ReadRequest, return } - // Parse the ID to get object ID and principal - objectID, principal, err := r.parseID(id.ValueString()) + // Parse the ID to get object type, object ID and principal + objectType, objectID, principal, err := r.parseID(state.ID.ValueString()) if err != nil { resp.Diagnostics.AddError("Failed to parse resource ID", err.Error()) return } - // Get the mapping from the ID - mapping, err := permissions.GetResourcePermissionsFromId(objectID) - if err != nil { - resp.Diagnostics.AddError("Failed to get resource permissions mapping", err.Error()) - return - } - // Read current permissions - idParts := strings.Split(objectID, "/") - permID := idParts[len(idParts)-1] - perms, err := w.Permissions.Get(ctx, iam.GetPermissionRequest{ - RequestObjectId: permID, - RequestObjectType: mapping.GetRequestObjectType(), + RequestObjectId: objectID, + RequestObjectType: objectType, }) if err != nil { // If the object or permissions are not found, remove from state to trigger recreation @@ -302,12 +201,11 @@ func (r *PermissionResource) Read(ctx context.Context, req resource.ReadRequest, // Filter for the specific principal found := false - var currentPermissionLevel types.String for _, acl := range perms.AccessControlList { if r.matchesPrincipal(acl, principal) { // Update the state with the current permission level if len(acl.AllPermissions) > 0 { - currentPermissionLevel = types.StringValue(string(acl.AllPermissions[0].PermissionLevel)) + state.PermissionLevel = types.StringValue(string(acl.AllPermissions[0].PermissionLevel)) found = true break } @@ -320,95 +218,27 @@ func (r *PermissionResource) Read(ctx context.Context, req resource.ReadRequest, return } - // Read the object identifier field from current state to preserve it - // (It should already be in state, but we need to make sure it stays there) - var objectFieldValue types.String - resp.Diagnostics.Append(req.State.GetAttribute(ctx, path.Root(mapping.GetField()), &objectFieldValue)...) - if resp.Diagnostics.HasError() { - return - } - - // Update state using SetAttribute - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root(mapping.GetField()), objectFieldValue)...) - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("permission_level"), currentPermissionLevel)...) + resp.Diagnostics.Append(resp.State.Set(ctx, &state)...) } func (r *PermissionResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) { - // Read ID, principals, and permission_level from plan using GetAttribute - var id, userName, groupName, servicePrincipalName, permissionLevel types.String - - resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("id"), &id)...) - resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("user_name"), &userName)...) - resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("group_name"), &groupName)...) - resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("service_principal_name"), &servicePrincipalName)...) - resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, path.Root("permission_level"), &permissionLevel)...) - + var plan PermissionResourceModel + resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...) if resp.Diagnostics.HasError() { return } - w, err := r.Client.WorkspaceClient() - if err != nil { - resp.Diagnostics.AddError("Failed to get workspace client", err.Error()) - return - } - - // Parse the ID to get object ID and principal - objectID, _, err := r.parseID(id.ValueString()) - if err != nil { - resp.Diagnostics.AddError("Failed to parse resource ID", err.Error()) - return - } - - // Lock the object to prevent concurrent modifications - lockObject(objectID) - defer unlockObject(objectID) - - // Get the mapping from the ID - mapping, err := permissions.GetResourcePermissionsFromId(objectID) - if err != nil { - resp.Diagnostics.AddError("Failed to get resource permissions mapping", err.Error()) - return - } - - // Update the permission using PATCH - permLevel := iam.PermissionLevel(permissionLevel.ValueString()) - idParts := strings.Split(objectID, "/") - permID := idParts[len(idParts)-1] - - _, err = w.Permissions.Update(ctx, iam.UpdateObjectPermissions{ - RequestObjectId: permID, - RequestObjectType: mapping.GetRequestObjectType(), - AccessControlList: []iam.AccessControlRequest{ - { - UserName: userName.ValueString(), - GroupName: groupName.ValueString(), - ServicePrincipalName: servicePrincipalName.ValueString(), - PermissionLevel: permLevel, - }, - }, - }) - if err != nil { - resp.Diagnostics.AddError("Failed to update permission", err.Error()) - return - } - - // Read the object identifier field from current state to preserve it - var objectFieldValue types.String - resp.Diagnostics.Append(req.State.GetAttribute(ctx, path.Root(mapping.GetField()), &objectFieldValue)...) + r.upsertPermission(ctx, &plan, &resp.Diagnostics) if resp.Diagnostics.HasError() { return } - // Update state using SetAttribute - preserve the object identifier field - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root(mapping.GetField()), objectFieldValue)...) - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("permission_level"), permissionLevel)...) + resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...) } func (r *PermissionResource) Delete(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) { - // Read ID from state using GetAttribute - var id types.String - resp.Diagnostics.Append(req.State.GetAttribute(ctx, path.Root("id"), &id)...) + var state PermissionResourceModel + resp.Diagnostics.Append(req.State.Get(ctx, &state)...) if resp.Diagnostics.HasError() { return } @@ -419,33 +249,20 @@ func (r *PermissionResource) Delete(ctx context.Context, req resource.DeleteRequ return } - // Parse the ID to get object ID and principal - objectID, principal, err := r.parseID(id.ValueString()) - if err != nil { - resp.Diagnostics.AddError("Failed to parse resource ID", err.Error()) - return - } + objectType := state.ObjectType.ValueString() + objectID := state.ObjectID.ValueString() + principal := r.getPrincipalFromModel(&state) // Lock the object to prevent concurrent modifications // This is CRITICAL for Delete to avoid race conditions when multiple // permission resources for the same object are deleted concurrently - lockObject(objectID) - defer unlockObject(objectID) - - // Get the mapping from the ID - mapping, err := permissions.GetResourcePermissionsFromId(objectID) - if err != nil { - resp.Diagnostics.AddError("Failed to get resource permissions mapping", err.Error()) - return - } + lockObject(objectType, objectID) + defer unlockObject(objectType, objectID) // Read current permissions to see what to remove - idParts := strings.Split(objectID, "/") - permID := idParts[len(idParts)-1] - currentPerms, err := w.Permissions.Get(ctx, iam.GetPermissionRequest{ - RequestObjectId: permID, - RequestObjectType: mapping.GetRequestObjectType(), + RequestObjectId: objectID, + RequestObjectType: objectType, }) if err != nil { // If the object or permissions are not found, the permission is already gone @@ -475,8 +292,8 @@ func (r *PermissionResource) Delete(ctx context.Context, req resource.DeleteRequ // Use Set to replace all permissions (effectively removing the specified principal) _, err = w.Permissions.Set(ctx, iam.SetObjectPermissions{ - RequestObjectId: permID, - RequestObjectType: mapping.GetRequestObjectType(), + RequestObjectId: objectID, + RequestObjectType: objectType, AccessControlList: remainingACLs, }) if err != nil { @@ -491,8 +308,8 @@ func (r *PermissionResource) Delete(ctx context.Context, req resource.DeleteRequ } func (r *PermissionResource) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) { - // Import ID format: /// - // Example: /clusters/cluster-123/user@example.com + // Import ID format: // + // Example: clusters/cluster-123/user@example.com w, err := r.Client.WorkspaceClient() if err != nil { @@ -501,29 +318,19 @@ func (r *PermissionResource) ImportState(ctx context.Context, req resource.Impor } // Parse the import ID - objectID, principal, err := r.parseID(req.ID) + objectType, objectID, principal, err := r.parseID(req.ID) if err != nil { resp.Diagnostics.AddError( "Invalid Import ID Format", - fmt.Sprintf("Expected format: ///. Error: %s", err.Error()), + fmt.Sprintf("Expected format: //. Error: %s", err.Error()), ) return } - // Get the mapping from the ID to determine object type and field - mapping, err := permissions.GetResourcePermissionsFromId(objectID) - if err != nil { - resp.Diagnostics.AddError("Failed to get resource permissions mapping", err.Error()) - return - } - // Read current permissions from Databricks - idParts := strings.Split(objectID, "/") - permID := idParts[len(idParts)-1] - perms, err := w.Permissions.Get(ctx, iam.GetPermissionRequest{ - RequestObjectId: permID, - RequestObjectType: mapping.GetRequestObjectType(), + RequestObjectId: objectID, + RequestObjectType: objectType, }) if err != nil { resp.Diagnostics.AddError("Failed to read permissions", err.Error()) @@ -532,16 +339,35 @@ func (r *PermissionResource) ImportState(ctx context.Context, req resource.Impor // Find the specific principal's permission var found bool - var permissionLevel string - var userName, groupName, servicePrincipalName string + var state PermissionResourceModel + state.ID = types.StringValue(req.ID) + state.ObjectType = types.StringValue(objectType) + state.ObjectID = types.StringValue(objectID) for _, acl := range perms.AccessControlList { if r.matchesPrincipal(acl, principal) { if len(acl.AllPermissions) > 0 { - permissionLevel = string(acl.AllPermissions[0].PermissionLevel) - userName = acl.UserName - groupName = acl.GroupName - servicePrincipalName = acl.ServicePrincipalName + state.PermissionLevel = types.StringValue(string(acl.AllPermissions[0].PermissionLevel)) + + // Set principal fields - use null for empty strings + if acl.UserName != "" { + state.UserName = types.StringValue(acl.UserName) + } else { + state.UserName = types.StringNull() + } + + if acl.GroupName != "" { + state.GroupName = types.StringValue(acl.GroupName) + } else { + state.GroupName = types.StringNull() + } + + if acl.ServicePrincipalName != "" { + state.ServicePrincipalName = types.StringValue(acl.ServicePrincipalName) + } else { + state.ServicePrincipalName = types.StringNull() + } + found = true break } @@ -551,84 +377,64 @@ func (r *PermissionResource) ImportState(ctx context.Context, req resource.Impor if !found { resp.Diagnostics.AddError( "Permission Not Found", - fmt.Sprintf("No permission found for principal %q on object %q", principal, objectID), + fmt.Sprintf("No permission found for principal %q on object %s/%s", principal, objectType, objectID), ) return } - // Set all attributes in state using SetAttribute - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("id"), types.StringValue(req.ID))...) - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("object_type"), types.StringValue(mapping.GetRequestObjectType()))...) - - // Set the object identifier field (e.g., cluster_id, job_id, etc.) - // Extract the configured value from the objectID - configuredValue := permID - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root(mapping.GetField()), types.StringValue(configuredValue))...) - - // Set principal fields - use null for empty strings to avoid ImportStateVerify failures - if userName != "" { - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("user_name"), types.StringValue(userName))...) - } else { - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("user_name"), types.StringNull())...) - } - - if groupName != "" { - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("group_name"), types.StringValue(groupName))...) - } else { - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("group_name"), types.StringNull())...) - } - - if servicePrincipalName != "" { - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("service_principal_name"), types.StringValue(servicePrincipalName))...) - } else { - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("service_principal_name"), types.StringNull())...) - } - - // Set permission level - resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("permission_level"), types.StringValue(permissionLevel))...) + resp.Diagnostics.Append(resp.State.Set(ctx, &state)...) } // Helper methods -// AttributeGetter is an interface for types that can get attributes (Plan, Config, State) -type AttributeGetter interface { - GetAttribute(ctx context.Context, path path.Path, target interface{}) diag.Diagnostics -} - -// PermissionMapping is an interface that abstracts the permissions mapping operations -type PermissionMapping interface { - GetRequestObjectType() string - GetObjectType() string - GetID(context.Context, *databricks.WorkspaceClient, string) (string, error) -} - -func (r *PermissionResource) getObjectMappingAndID(ctx context.Context, w *databricks.WorkspaceClient, getter AttributeGetter) (PermissionMapping, string, string, types.String, error) { - // Dynamically iterate through all permission definitions to find which object ID is set - allPermissions := permissions.AllResourcePermissions() +// upsertPermission creates or updates a permission for a principal on an object +func (r *PermissionResource) upsertPermission(ctx context.Context, model *PermissionResourceModel, diags *diag.Diagnostics) { + w, err := r.Client.WorkspaceClient() + if err != nil { + diags.AddError("Failed to get workspace client", err.Error()) + return + } - for _, mapping := range allPermissions { - var attrValue types.String - diags := getter.GetAttribute(ctx, path.Root(mapping.GetField()), &attrValue) - if diags.HasError() { - continue // Attribute doesn't exist or has errors, try next - } + objectType := model.ObjectType.ValueString() + objectID := model.ObjectID.ValueString() - if !attrValue.IsNull() && !attrValue.IsUnknown() && attrValue.ValueString() != "" { - configuredValue := attrValue.ValueString() + // Lock the object to prevent concurrent modifications + lockObject(objectType, objectID) + defer unlockObject(objectType, objectID) - // Get the object ID (may involve path resolution) - objectID, err := mapping.GetID(ctx, w, configuredValue) - if err != nil { - return nil, "", "", types.String{}, err - } + // Create the permission update request + permLevel := iam.PermissionLevel(model.PermissionLevel.ValueString()) - // Return mapping, objectID, field name, and field value - return mapping, objectID, mapping.GetField(), attrValue, nil - } + _, err = w.Permissions.Update(ctx, iam.UpdateObjectPermissions{ + RequestObjectId: objectID, + RequestObjectType: objectType, + AccessControlList: []iam.AccessControlRequest{ + { + UserName: model.UserName.ValueString(), + GroupName: model.GroupName.ValueString(), + ServicePrincipalName: model.ServicePrincipalName.ValueString(), + PermissionLevel: permLevel, + }, + }, + }) + if err != nil { + diags.AddError("Failed to create/update permission", err.Error()) + return } +} - // No object identifier was set - return nil, "", "", types.String{}, fmt.Errorf("at least one object identifier must be set") +// getPrincipalFromModel extracts the principal identifier from the model +func (r *PermissionResource) getPrincipalFromModel(model *PermissionResourceModel) string { + if !model.UserName.IsNull() && model.UserName.ValueString() != "" { + return model.UserName.ValueString() + } + if !model.GroupName.IsNull() && model.GroupName.ValueString() != "" { + return model.GroupName.ValueString() + } + if !model.ServicePrincipalName.IsNull() && model.ServicePrincipalName.ValueString() != "" { + return model.ServicePrincipalName.ValueString() + } + return "" } func (r *PermissionResource) matchesPrincipal(acl iam.AccessControlResponse, principal string) bool { @@ -637,16 +443,19 @@ func (r *PermissionResource) matchesPrincipal(acl iam.AccessControlResponse, pri acl.ServicePrincipalName == principal } -func (r *PermissionResource) parseID(id string) (objectID string, principal string, error error) { - // ID format: /// +func (r *PermissionResource) parseID(id string) (objectType string, objectID string, principal string, err error) { + // ID format: // + // Example: clusters/cluster-123/user@example.com parts := strings.Split(id, "/") - if len(parts) < 4 { - return "", "", fmt.Errorf("invalid ID format: expected ///, got %s", id) + if len(parts) < 3 { + return "", "", "", fmt.Errorf("invalid ID format: expected //, got %s", id) } - // Reconstruct object ID and get principal + // Handle cases where object_id might contain slashes (e.g., notebooks paths) + // The principal is always the last part principal = parts[len(parts)-1] - objectID = strings.Join(parts[:len(parts)-1], "/") + objectType = parts[0] + objectID = strings.Join(parts[1:len(parts)-1], "/") - return objectID, principal, nil + return objectType, objectID, principal, nil } diff --git a/permissions/resource_permission_acc_test.go b/internal/providers/pluginfw/products/permissions/resource_permission_acc_test.go similarity index 80% rename from permissions/resource_permission_acc_test.go rename to internal/providers/pluginfw/products/permissions/resource_permission_acc_test.go index 1c0158e999..08213db7b2 100644 --- a/permissions/resource_permission_acc_test.go +++ b/internal/providers/pluginfw/products/permissions/resource_permission_acc_test.go @@ -43,13 +43,15 @@ resource "databricks_group" "group2" { } resource "databricks_permission" "cluster_group1" { - cluster_id = databricks_cluster.this.id + object_type = "clusters" + object_id = databricks_cluster.this.id group_name = databricks_group.group1.display_name permission_level = "CAN_ATTACH_TO" } resource "databricks_permission" "cluster_group2" { - cluster_id = databricks_cluster.this.id + object_type = "clusters" + object_id = databricks_cluster.this.id group_name = databricks_group.group2.display_name permission_level = "CAN_RESTART" } @@ -107,13 +109,15 @@ resource "databricks_user" "test_user" { } resource "databricks_permission" "job_group" { - job_id = databricks_job.this.id + object_type = "jobs" + object_id = databricks_job.this.id group_name = databricks_group.viewers.display_name permission_level = "CAN_VIEW" } resource "databricks_permission" "job_user" { - job_id = databricks_job.this.id + object_type = "jobs" + object_id = databricks_job.this.id user_name = databricks_user.test_user.user_name permission_level = "CAN_MANAGE_RUN" } @@ -122,9 +126,12 @@ resource "databricks_permission" "job_user" { acceptance.WorkspaceLevel(t, acceptance.Step{ Template: template, Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("databricks_permission.job_group", "object_type", "jobs"), + resource.TestCheckResourceAttrSet("databricks_permission.job_group", "object_id"), resource.TestCheckResourceAttr("databricks_permission.job_group", "permission_level", "CAN_VIEW"), + resource.TestCheckResourceAttr("databricks_permission.job_user", "object_type", "jobs"), + resource.TestCheckResourceAttrSet("databricks_permission.job_user", "object_id"), resource.TestCheckResourceAttr("databricks_permission.job_user", "permission_level", "CAN_MANAGE_RUN"), - resource.TestCheckResourceAttr("databricks_permission.job_group", "object_type", "jobs"), ), }) } @@ -150,13 +157,15 @@ resource "databricks_group" "runners" { } resource "databricks_permission" "notebook_editors" { - notebook_path = databricks_notebook.this.path + object_type = "notebooks" + object_id = databricks_notebook.this.path group_name = databricks_group.editors.display_name permission_level = "CAN_EDIT" } resource "databricks_permission" "notebook_runners" { - notebook_path = databricks_notebook.this.path + object_type = "notebooks" + object_id = databricks_notebook.this.path group_name = databricks_group.runners.display_name permission_level = "CAN_RUN" } @@ -165,7 +174,11 @@ resource "databricks_permission" "notebook_runners" { acceptance.WorkspaceLevel(t, acceptance.Step{ Template: template, Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("databricks_permission.notebook_editors", "object_type", "notebooks"), + resource.TestCheckResourceAttrSet("databricks_permission.notebook_editors", "object_id"), resource.TestCheckResourceAttr("databricks_permission.notebook_editors", "permission_level", "CAN_EDIT"), + resource.TestCheckResourceAttr("databricks_permission.notebook_runners", "object_type", "notebooks"), + resource.TestCheckResourceAttrSet("databricks_permission.notebook_runners", "object_id"), resource.TestCheckResourceAttr("databricks_permission.notebook_runners", "permission_level", "CAN_RUN"), ), }) @@ -189,19 +202,22 @@ resource "databricks_group" "team_c" { # This demonstrates the key benefit: each team's token permissions # can be managed independently, unlike databricks_permissions resource "databricks_permission" "tokens_team_a" { - authorization = "tokens" + object_type = "authorization" + object_id = "tokens" group_name = databricks_group.team_a.display_name permission_level = "CAN_USE" } resource "databricks_permission" "tokens_team_b" { - authorization = "tokens" + object_type = "authorization" + object_id = "tokens" group_name = databricks_group.team_b.display_name permission_level = "CAN_USE" } resource "databricks_permission" "tokens_team_c" { - authorization = "tokens" + object_type = "authorization" + object_id = "tokens" group_name = databricks_group.team_c.display_name permission_level = "CAN_USE" } @@ -210,10 +226,13 @@ resource "databricks_permission" "tokens_team_c" { acceptance.WorkspaceLevel(t, acceptance.Step{ Template: template, Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("databricks_permission.tokens_team_a", "authorization", "tokens"), + resource.TestCheckResourceAttr("databricks_permission.tokens_team_a", "object_type", "authorization"), + resource.TestCheckResourceAttr("databricks_permission.tokens_team_a", "object_id", "tokens"), resource.TestCheckResourceAttr("databricks_permission.tokens_team_a", "permission_level", "CAN_USE"), - resource.TestCheckResourceAttr("databricks_permission.tokens_team_b", "authorization", "tokens"), - resource.TestCheckResourceAttr("databricks_permission.tokens_team_c", "authorization", "tokens"), + resource.TestCheckResourceAttr("databricks_permission.tokens_team_b", "object_type", "authorization"), + resource.TestCheckResourceAttr("databricks_permission.tokens_team_b", "object_id", "tokens"), + resource.TestCheckResourceAttr("databricks_permission.tokens_team_c", "object_type", "authorization"), + resource.TestCheckResourceAttr("databricks_permission.tokens_team_c", "object_id", "tokens"), func(s *terraform.State) error { w := databricks.Must(databricks.NewWorkspaceClient()) permissions, err := w.Permissions.GetByRequestObjectTypeAndRequestObjectId(context.Background(), "authorization", "tokens") @@ -257,13 +276,15 @@ resource "databricks_group" "team_c" { } resource "databricks_permission" "tokens_team_a" { - authorization = "tokens" + object_type = "authorization" + object_id = "tokens" group_name = databricks_group.team_a.display_name permission_level = "CAN_USE" } resource "databricks_permission" "tokens_team_c" { - authorization = "tokens" + object_type = "authorization" + object_id = "tokens" group_name = databricks_group.team_c.display_name permission_level = "CAN_USE" } @@ -309,7 +330,8 @@ resource "databricks_group" "test" { } resource "databricks_permission" "job_group" { - job_id = databricks_job.this.id + object_type = "jobs" + object_id = databricks_job.this.id group_name = databricks_group.test.display_name permission_level = "CAN_VIEW" } @@ -325,7 +347,8 @@ resource "databricks_group" "test" { } resource "databricks_permission" "job_group" { - job_id = databricks_job.this.id + object_type = "jobs" + object_id = databricks_job.this.id group_name = databricks_group.test.display_name permission_level = "CAN_MANAGE" } @@ -334,11 +357,15 @@ resource "databricks_permission" "job_group" { acceptance.WorkspaceLevel(t, acceptance.Step{ Template: template1, Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("databricks_permission.job_group", "object_type", "jobs"), + resource.TestCheckResourceAttrSet("databricks_permission.job_group", "object_id"), resource.TestCheckResourceAttr("databricks_permission.job_group", "permission_level", "CAN_VIEW"), ), }, acceptance.Step{ Template: template2, Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("databricks_permission.job_group", "object_type", "jobs"), + resource.TestCheckResourceAttrSet("databricks_permission.job_group", "object_id"), resource.TestCheckResourceAttr("databricks_permission.job_group", "permission_level", "CAN_MANAGE"), func(s *terraform.State) error { w := databricks.Must(databricks.NewWorkspaceClient()) @@ -375,7 +402,8 @@ resource "databricks_service_principal" "sp" { } resource "databricks_permission" "job_sp" { - job_id = databricks_job.this.id + object_type = "jobs" + object_id = databricks_job.this.id service_principal_name = databricks_service_principal.sp.application_id permission_level = "CAN_MANAGE" } @@ -384,6 +412,8 @@ resource "databricks_permission" "job_sp" { acceptance.WorkspaceLevel(t, acceptance.Step{ Template: template, Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("databricks_permission.job_sp", "object_type", "jobs"), + resource.TestCheckResourceAttrSet("databricks_permission.job_sp", "object_id"), resource.TestCheckResourceAttr("databricks_permission.job_sp", "permission_level", "CAN_MANAGE"), resource.TestCheckResourceAttrSet("databricks_permission.job_sp", "service_principal_name"), ), @@ -402,7 +432,8 @@ resource "databricks_group" "test" { } resource "databricks_permission" "job_group" { - job_id = databricks_job.this.id + object_type = "jobs" + object_id = databricks_job.this.id group_name = databricks_group.test.display_name permission_level = "CAN_VIEW" } @@ -437,7 +468,8 @@ resource "databricks_group" "sql_users" { } resource "databricks_permission" "warehouse_users" { - sql_endpoint_id = databricks_sql_endpoint.this.id + object_type = "sql/warehouses" + object_id = databricks_sql_endpoint.this.id group_name = databricks_group.sql_users.display_name permission_level = "CAN_USE" } @@ -446,6 +478,8 @@ resource "databricks_permission" "warehouse_users" { acceptance.WorkspaceLevel(t, acceptance.Step{ Template: template, Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("databricks_permission.warehouse_users", "object_type", "sql/warehouses"), + resource.TestCheckResourceAttrSet("databricks_permission.warehouse_users", "object_id"), resource.TestCheckResourceAttr("databricks_permission.warehouse_users", "permission_level", "CAN_USE"), ), }) @@ -471,7 +505,8 @@ resource "databricks_group" "pool_users" { } resource "databricks_permission" "pool_access" { - instance_pool_id = databricks_instance_pool.this.id + object_type = "instance-pools" + object_id = databricks_instance_pool.this.id group_name = databricks_group.pool_users.display_name permission_level = "CAN_ATTACH_TO" } @@ -480,6 +515,8 @@ resource "databricks_permission" "pool_access" { acceptance.WorkspaceLevel(t, acceptance.Step{ Template: template, Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("databricks_permission.pool_access", "object_type", "instance-pools"), + resource.TestCheckResourceAttrSet("databricks_permission.pool_access", "object_id"), resource.TestCheckResourceAttr("databricks_permission.pool_access", "permission_level", "CAN_ATTACH_TO"), ), }) @@ -503,15 +540,18 @@ resource "databricks_group" "policy_users" { } resource "databricks_permission" "policy_access" { - cluster_policy_id = databricks_cluster_policy.this.id - group_name = databricks_group.policy_users.display_name - permission_level = "CAN_USE" + object_type = "cluster-policies" + object_id = databricks_cluster_policy.this.id + group_name = databricks_group.policy_users.display_name + permission_level = "CAN_USE" } ` acceptance.WorkspaceLevel(t, acceptance.Step{ Template: template, Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("databricks_permission.policy_access", "object_type", "cluster-policies"), + resource.TestCheckResourceAttrSet("databricks_permission.policy_access", "object_id"), resource.TestCheckResourceAttr("databricks_permission.policy_access", "permission_level", "CAN_USE"), ), }) diff --git a/internal/providers/pluginfw/products/permissions/resource_permission_test.go b/internal/providers/pluginfw/products/permissions/resource_permission_test.go index fdeb1ea2a1..3a34be6115 100644 --- a/internal/providers/pluginfw/products/permissions/resource_permission_test.go +++ b/internal/providers/pluginfw/products/permissions/resource_permission_test.go @@ -35,15 +35,12 @@ func TestPermissionResource_Schema(t *testing.T) { _, ok = resp.Schema.Attributes["permission_level"] assert.True(t, ok, "permission_level should be in schema") - // Check some object identifiers - _, ok = resp.Schema.Attributes["cluster_id"] - assert.True(t, ok, "cluster_id should be in schema") - - _, ok = resp.Schema.Attributes["job_id"] - assert.True(t, ok, "job_id should be in schema") + // Check object identifier fields + _, ok = resp.Schema.Attributes["object_type"] + assert.True(t, ok, "object_type should be in schema") - _, ok = resp.Schema.Attributes["authorization"] - assert.True(t, ok, "authorization should be in schema") + _, ok = resp.Schema.Attributes["object_id"] + assert.True(t, ok, "object_id should be in schema") // Check computed fields idAttr, ok := resp.Schema.Attributes["id"] @@ -52,10 +49,18 @@ func TestPermissionResource_Schema(t *testing.T) { assert.True(t, stringAttr.Computed, "id should be computed") } + // Verify object_type is required objectTypeAttr, ok := resp.Schema.Attributes["object_type"] assert.True(t, ok, "object_type should be in schema") if stringAttr, ok := objectTypeAttr.(schema.StringAttribute); ok { - assert.True(t, stringAttr.Computed, "object_type should be computed") + assert.True(t, stringAttr.Required, "object_type should be required") + } + + // Verify object_id is required + objectIDAttr, ok := resp.Schema.Attributes["object_id"] + assert.True(t, ok, "object_id should be in schema") + if stringAttr, ok := objectIDAttr.(schema.StringAttribute); ok { + assert.True(t, stringAttr.Required, "object_id should be required") } } @@ -73,36 +78,48 @@ func TestPermissionResource_ParseID(t *testing.T) { r := &PermissionResource{} tests := []struct { - name string - id string - wantObjectID string - wantPrincipal string - wantError bool + name string + id string + wantObjectType string + wantObjectID string + wantPrincipal string + wantError bool }{ { - name: "cluster permission", - id: "/clusters/test-cluster-id/user@example.com", - wantObjectID: "/clusters/test-cluster-id", - wantPrincipal: "user@example.com", - wantError: false, + name: "cluster permission", + id: "clusters/test-cluster-id/user@example.com", + wantObjectType: "clusters", + wantObjectID: "test-cluster-id", + wantPrincipal: "user@example.com", + wantError: false, + }, + { + name: "job permission", + id: "jobs/123/test-group", + wantObjectType: "jobs", + wantObjectID: "123", + wantPrincipal: "test-group", + wantError: false, }, { - name: "job permission", - id: "/jobs/123/test-group", - wantObjectID: "/jobs/123", - wantPrincipal: "test-group", - wantError: false, + name: "authorization tokens", + id: "authorization/tokens/developers", + wantObjectType: "authorization", + wantObjectID: "tokens", + wantPrincipal: "developers", + wantError: false, }, { - name: "authorization tokens", - id: "/authorization/tokens/developers", - wantObjectID: "/authorization/tokens", - wantPrincipal: "developers", - wantError: false, + name: "notebook with path containing slashes", + id: "notebooks/Shared/Team/notebook.py/user@example.com", + wantObjectType: "notebooks", + wantObjectID: "Shared/Team/notebook.py", + wantPrincipal: "user@example.com", + wantError: false, }, { name: "invalid format - too few parts", - id: "/clusters/test-cluster-id", + id: "clusters/test-cluster-id", wantError: true, }, { @@ -114,11 +131,12 @@ func TestPermissionResource_ParseID(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotObjectID, gotPrincipal, err := r.parseID(tt.id) + gotObjectType, gotObjectID, gotPrincipal, err := r.parseID(tt.id) if tt.wantError { assert.Error(t, err) } else { assert.NoError(t, err) + assert.Equal(t, tt.wantObjectType, gotObjectType) assert.Equal(t, tt.wantObjectID, gotObjectID) assert.Equal(t, tt.wantPrincipal, gotPrincipal) } @@ -147,36 +165,40 @@ func TestPermissionResource_ImportState(t *testing.T) { r := &PermissionResource{} tests := []struct { - name string - importID string - expectedObjID string - expectedPrinc string - expectError bool + name string + importID string + expectedObjType string + expectedObjID string + expectedPrinc string + expectError bool }{ { - name: "valid cluster import", - importID: "/clusters/cluster-123/user@example.com", - expectedObjID: "/clusters/cluster-123", - expectedPrinc: "user@example.com", - expectError: false, + name: "valid cluster import", + importID: "clusters/cluster-123/user@example.com", + expectedObjType: "clusters", + expectedObjID: "cluster-123", + expectedPrinc: "user@example.com", + expectError: false, }, { - name: "valid job import", - importID: "/jobs/job-456/data-engineers", - expectedObjID: "/jobs/job-456", - expectedPrinc: "data-engineers", - expectError: false, + name: "valid job import", + importID: "jobs/job-456/data-engineers", + expectedObjType: "jobs", + expectedObjID: "job-456", + expectedPrinc: "data-engineers", + expectError: false, }, { - name: "valid authorization import", - importID: "/authorization/tokens/team-a", - expectedObjID: "/authorization/tokens", - expectedPrinc: "team-a", - expectError: false, + name: "valid authorization import", + importID: "authorization/tokens/team-a", + expectedObjType: "authorization", + expectedObjID: "tokens", + expectedPrinc: "team-a", + expectError: false, }, { name: "invalid format - too few parts", - importID: "/clusters/cluster-123", + importID: "clusters/cluster-123", expectError: true, }, { @@ -188,12 +210,13 @@ func TestPermissionResource_ImportState(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - objectID, principal, err := r.parseID(tt.importID) + objectType, objectID, principal, err := r.parseID(tt.importID) if tt.expectError { assert.Error(t, err, "Expected error for invalid import ID") } else { assert.NoError(t, err, "Expected no error for valid import ID") + assert.Equal(t, tt.expectedObjType, objectType, "Object type should match") assert.Equal(t, tt.expectedObjID, objectID, "Object ID should match") assert.Equal(t, tt.expectedPrinc, principal, "Principal should match") }