Skip to content

Commit

Permalink
[Fix] Automatically assign IS_OWNER permission to sql warehouse if …
Browse files Browse the repository at this point in the history 
…not specified (#3829)

## Changes
- SQL warehouses supports specifying `IS_OWNER` permission and therefore
requires the same workaround as jobs & pipelines.
- Resolves #3730

## Tests
<!-- 
How is this tested? Please see the checklist below and also describe any
other relevant tests
-->

- [x] `make test` run locally
- [x] relevant change in `docs/` folder
- [x] covered with integration tests in `internal/acceptance`
- [x] relevant acceptance tests are passing
- [x] using Go SDK
  • Loading branch information
@nkvuong
nkvuong committed Aug 9, 2024
1 parent 7342fa5 commit fb7e4ef
Show file tree
Hide file tree
Showing 3 changed files with 181 additions and 31 deletions.
4 changes: 4 additions & 0 deletions docs/resources/permissions.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,10 @@ resource "databricks_permissions" "token_usage" {

[SQL warehouses](https://docs.databricks.com/sql/user/security/access-control/sql-endpoint-acl.html) have four possible permissions: `CAN_USE`, `CAN_MONITOR`, `CAN_MANAGE` and `IS_OWNER`:

- The creator of a warehouse has `IS_OWNER` permission. Destroying `databricks_permissions` resource for a warehouse would revert ownership to the creator.
- A warehouse must have exactly one owner. If a resource is changed and no owner is specified, the currently authenticated principal would become the new owner of the warehouse. Nothing would change, per se, if the warehouse was created through Terraform.
- A warehouse cannot have a group as an owner.

```hcl
data "databricks_current_user" "me" {}
Expand Down
100 changes: 75 additions & 25 deletions permissions/resource_permissions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,56 @@ func (a PermissionsAPI) shouldExplicitlyGrantCallingUserManagePermissions(object
return isDbsqlPermissionsWorkaroundNecessary(objectID)
}

func isOwnershipWorkaroundNecessary(objectID string) bool {
return strings.HasPrefix(objectID, "/jobs") || strings.HasPrefix(objectID, "/pipelines") || strings.HasPrefix(objectID, "/sql/warehouses")
}

// Suppress the error if it is 404
func ignoreNotFound(err error) error {
var apiErr *apierr.APIError
if !errors.As(err, &apiErr) {
return err
}
if apiErr.StatusCode == 404 {
return nil
}
if strings.Contains(apiErr.Message, "does not exist.") {
return nil
}
return err
}

func (a PermissionsAPI) getObjectCreator(objectID string) (string, error) {
w, err := a.client.WorkspaceClient()
if err != nil {
return "", err
}
if strings.HasPrefix(objectID, "/jobs") {
jobId, err := strconv.ParseInt(strings.ReplaceAll(objectID, "/jobs/", ""), 10, 64)
if err != nil {
return "", err
}
job, err := w.Jobs.GetByJobId(a.context, jobId)
if err != nil {
return "", ignoreNotFound(err)
}
return job.CreatorUserName, nil
} else if strings.HasPrefix(objectID, "/pipelines") {
pipeline, err := w.Pipelines.GetByPipelineId(a.context, strings.ReplaceAll(objectID, "/pipelines/", ""))
if err != nil {
return "", ignoreNotFound(err)
}
return pipeline.CreatorUserName, nil
} else if strings.HasPrefix(objectID, "/sql/warehouses") {
warehouse, err := w.Warehouses.GetById(a.context, strings.ReplaceAll(objectID, "/sql/warehouses/", ""))
if err != nil {
return "", ignoreNotFound(err)
}
return warehouse.CreatorName, nil
}
return "", nil
}

func (a PermissionsAPI) ensureCurrentUserCanManageObject(objectID string, objectACL AccessControlChangeList) (AccessControlChangeList, error) {
if !a.shouldExplicitlyGrantCallingUserManagePermissions(objectID) {
return objectACL, nil
Expand Down Expand Up @@ -168,6 +218,19 @@ func (a PermissionsAPI) put(objectID string, objectACL AccessControlChangeList)
return a.client.Put(a.context, urlPathForObjectID(objectID), objectACL)
}

// safePutWithOwner is a workaround for the limitation where warehouse without owners cannot have IS_OWNER set
func (a PermissionsAPI) safePutWithOwner(objectID string, objectACL AccessControlChangeList, originalAcl []AccessControlChange) error {
err := a.put(objectID, objectACL)
if err != nil {
if strings.Contains(err.Error(), "with no existing owner must provide a new owner") {
objectACL.AccessControlList = originalAcl
return a.put(objectID, objectACL)
}
return err
}
return nil
}

// Update updates object permissions. Technically, it's using method named SetOrDelete, but here we do more
func (a PermissionsAPI) Update(objectID string, objectACL AccessControlChangeList) error {
if objectID == "/authorization/tokens" || objectID == "/registered-models/root" || objectID == "/directories/0" {
Expand All @@ -177,7 +240,9 @@ func (a PermissionsAPI) Update(objectID string, objectACL AccessControlChangeLis
PermissionLevel: "CAN_MANAGE",
})
}
if strings.HasPrefix(objectID, "/jobs") || strings.HasPrefix(objectID, "/pipelines") {
originalAcl := make([]AccessControlChange, len(objectACL.AccessControlList))
_ = copy(originalAcl, objectACL.AccessControlList)
if isOwnershipWorkaroundNecessary(objectID) {
owners := 0
for _, acl := range objectACL.AccessControlList {
if acl.PermissionLevel == "IS_OWNER" {
Expand All @@ -200,7 +265,7 @@ func (a PermissionsAPI) Update(objectID string, objectACL AccessControlChangeLis
})
}
}
return a.put(objectID, objectACL)
return a.safePutWithOwner(objectID, objectACL, originalAcl)
}

// Delete gracefully removes permissions. Technically, it's using method named SetOrDelete, but here we do more
Expand All @@ -218,37 +283,22 @@ func (a PermissionsAPI) Delete(objectID string) error {
}
}
}
w, err := a.client.WorkspaceClient()
if err != nil {
return err
}
if strings.HasPrefix(objectID, "/jobs") {
jobId, err := strconv.ParseInt(strings.ReplaceAll(objectID, "/jobs/", ""), 10, 64)
originalAcl := make([]AccessControlChange, len(accl.AccessControlList))
_ = copy(originalAcl, accl.AccessControlList)
if isOwnershipWorkaroundNecessary(objectID) {
creator, err := a.getObjectCreator(objectID)
if err != nil {
return err
}
job, err := w.Jobs.GetByJobId(a.context, jobId)
if err != nil {
if strings.HasSuffix(err.Error(), " does not exist.") {
return nil
}
return err
}
accl.AccessControlList = append(accl.AccessControlList, AccessControlChange{
UserName: job.CreatorUserName,
PermissionLevel: "IS_OWNER",
})
} else if strings.HasPrefix(objectID, "/pipelines") {
job, err := w.Pipelines.GetByPipelineId(a.context, strings.ReplaceAll(objectID, "/pipelines/", ""))
if err != nil {
return err
if creator == "" {
return nil
}
accl.AccessControlList = append(accl.AccessControlList, AccessControlChange{
UserName: job.CreatorUserName,
UserName: creator,
PermissionLevel: "IS_OWNER",
})
}
return a.put(objectID, accl)
return a.safePutWithOwner(objectID, accl, originalAcl)
}

// Read gets all relevant permissions for the object, including inherited ones
Expand Down
108 changes: 102 additions & 6 deletions permissions/resource_permissions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ func TestResourcePermissionsRead_NotFound(t *testing.T) {
{
Method: http.MethodGet,
Resource: "/api/2.0/permissions/clusters/abc",
Response: common.APIErrorBody{
Response: apierr.APIError{
ErrorCode: "NOT_FOUND",
Message: "Cluster does not exist",
},
Expand All @@ -432,7 +432,7 @@ func TestResourcePermissionsRead_some_error(t *testing.T) {
{
Method: http.MethodGet,
Resource: "/api/2.0/permissions/clusters/abc",
Response: common.APIErrorBody{
Response: apierr.APIError{
ErrorCode: "INVALID_REQUEST",
Message: "Internal error happened",
},
Expand Down Expand Up @@ -498,7 +498,7 @@ func TestResourcePermissionsRead_ErrorOnScimMe(t *testing.T) {
{
Method: http.MethodGet,
Resource: "/api/2.0/preview/scim/v2/Me",
Response: common.APIErrorBody{
Response: apierr.APIError{
ErrorCode: "INVALID_REQUEST",
Message: "Internal error happened",
},
Expand Down Expand Up @@ -652,7 +652,7 @@ func TestResourcePermissionsDelete_error(t *testing.T) {
},
},
},
Response: common.APIErrorBody{
Response: apierr.APIError{
ErrorCode: "INVALID_REQUEST",
Message: "Internal error happened",
},
Expand Down Expand Up @@ -860,6 +860,10 @@ func TestResourcePermissionsCreate_SQLA_Endpoint(t *testing.T) {
UserName: TestingUser,
PermissionLevel: "CAN_USE",
},
{
UserName: TestingAdminUser,
PermissionLevel: "IS_OWNER",
},
{
UserName: TestingAdminUser,
PermissionLevel: "CAN_MANAGE",
Expand All @@ -878,6 +882,98 @@ func TestResourcePermissionsCreate_SQLA_Endpoint(t *testing.T) {
UserName: TestingUser,
PermissionLevel: "CAN_USE",
},
{
UserName: TestingAdminUser,
PermissionLevel: "IS_OWNER",
},
{
UserName: TestingAdminUser,
PermissionLevel: "CAN_MANAGE",
},
},
},
},
},
Resource: ResourcePermissions(),
State: map[string]any{
"sql_endpoint_id": "abc",
"access_control": []any{
map[string]any{
"user_name": TestingUser,
"permission_level": "CAN_USE",
},
},
},
Create: true,
}.Apply(t)
assert.NoError(t, err)
ac := d.Get("access_control").(*schema.Set)
require.Equal(t, 1, len(ac.List()))
firstElem := ac.List()[0].(map[string]any)
assert.Equal(t, TestingUser, firstElem["user_name"])
assert.Equal(t, "CAN_USE", firstElem["permission_level"])
}

func TestResourcePermissionsCreate_SQLA_Endpoint_WithOwnerError(t *testing.T) {
d, err := qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
me,
{
Method: "PUT",
Resource: "/api/2.0/permissions/sql/warehouses/abc",
ExpectedRequest: AccessControlChangeList{
AccessControlList: []AccessControlChange{
{
UserName: TestingUser,
PermissionLevel: "CAN_USE",
},
{
UserName: TestingAdminUser,
PermissionLevel: "IS_OWNER",
},
{
UserName: TestingAdminUser,
PermissionLevel: "CAN_MANAGE",
},
},
},
Response: apierr.APIError{
ErrorCode: "INVALID_PARAMETER_VALUE",
Message: "PUT requests for warehouse *** with no existing owner must provide a new owner.",
},
Status: 400,
},
{
Method: "PUT",
Resource: "/api/2.0/permissions/sql/warehouses/abc",
ExpectedRequest: AccessControlChangeList{
AccessControlList: []AccessControlChange{
{
UserName: TestingUser,
PermissionLevel: "CAN_USE",
},
{
UserName: TestingAdminUser,
PermissionLevel: "CAN_MANAGE",
},
},
},
},
{
Method: http.MethodGet,
Resource: "/api/2.0/permissions/sql/warehouses/abc",
Response: ObjectACL{
ObjectID: "dashboards/abc",
ObjectType: "dashboard",
AccessControlList: []AccessControl{
{
UserName: TestingUser,
PermissionLevel: "CAN_USE",
},
{
UserName: TestingAdminUser,
PermissionLevel: "IS_OWNER",
},
{
UserName: TestingAdminUser,
PermissionLevel: "CAN_MANAGE",
Expand Down Expand Up @@ -1003,7 +1099,7 @@ func TestResourcePermissionsCreate_NotebookPath_NotExists(t *testing.T) {
{
Method: http.MethodGet,
Resource: "/api/2.0/workspace/get-status?path=%2FDevelopment%2FInit",
Response: common.APIErrorBody{
Response: apierr.APIError{
ErrorCode: "INVALID_REQUEST",
Message: "Internal error happened",
},
Expand Down Expand Up @@ -1181,7 +1277,7 @@ func TestResourcePermissionsCreate_error(t *testing.T) {
{
Method: http.MethodPut,
Resource: "/api/2.0/permissions/clusters/abc",
Response: common.APIErrorBody{
Response: apierr.APIError{
ErrorCode: "INVALID_REQUEST",
Message: "Internal error happened",
},
Expand Down

0 comments on commit fb7e4ef

Please sign in to comment.