Skip to content

Commit

Permalink
[Fix] Fixed reading of permissions for SQL objects (#3800)
Browse files Browse the repository at this point in the history
## Changes
<!-- Summary of your changes that are easy to understand -->

The `object_id` for legacy SQL objects didn't have a correct format, and
now requires a special handling.

The error wasn't caught because we didn't have integration tests for SQL
objects permissions, and object IDs in the unit test weren't matching to
actual payload.

Fixes #3799 

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

- [x] `make test` run locally
- [ ] ~relevant change in `docs/` folder~
- [x] tested manually import of resources
- [x] covered with integration tests in `internal/acceptance`
- [x] relevant acceptance tests are passing
- [ ] ~using Go SDK~

Co-authored-by: Tanmay Rustagi <88379306+tanmay-db@users.noreply.github.com>
  • Loading branch information
alexott and tanmay-db authored Jul 23, 2024
1 parent 8fb39fb commit 165d129
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 10 deletions.
4 changes: 2 additions & 2 deletions docs/resources/permissions.md
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,7 @@ resource "databricks_group" "eng" {
display_name = "Engineering"
}
resource "databricks_permissions" "endpoint_usage" {
resource "databricks_permissions" "query_usage" {
sql_query_id = "3244325"
access_control {
Expand Down Expand Up @@ -767,7 +767,7 @@ resource "databricks_group" "eng" {
display_name = "Engineering"
}
resource "databricks_permissions" "endpoint_usage" {
resource "databricks_permissions" "alert_usage" {
sql_alert_id = "3244325"
access_control {
Expand Down
16 changes: 16 additions & 0 deletions internal/acceptance/sql_alert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@ func TestAccAlert(t *testing.T) {
query = "SELECT 1 AS p1, 2 as p2"
}
resource "databricks_permissions" "alert_usage" {
sql_alert_id = databricks_sql_alert.alert.id
access_control {
group_name = "users"
permission_level = "CAN_RUN"
}
}
resource "databricks_sql_alert" "alert" {
query_id = databricks_sql_query.this.id
name = "tf-alert-{var.RANDOM}"
Expand All @@ -31,6 +39,14 @@ func TestAccAlert(t *testing.T) {
query = "SELECT 1 AS p1, 2 as p2"
}
resource "databricks_permissions" "alert_usage" {
sql_alert_id = databricks_sql_alert.alert.id
access_control {
group_name = "users"
permission_level = "CAN_RUN"
}
}
resource "databricks_sql_alert" "alert" {
query_id = databricks_sql_query.this.id
name = "tf-alert-{var.RANDOM}"
Expand Down
16 changes: 16 additions & 0 deletions internal/acceptance/sql_dashboard_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,22 @@ func TestAccDashboard(t *testing.T) {
}
}
resource "databricks_permissions" "sql_dashboard_usage" {
sql_dashboard_id = databricks_sql_dashboard.d1.id
access_control {
group_name = "users"
permission_level = "CAN_RUN"
}
}
resource "databricks_permissions" "query_usage" {
sql_query_id = databricks_sql_query.q1.id
access_control {
group_name = "users"
permission_level = "CAN_RUN"
}
}
resource "databricks_sql_query" "q1" {
data_source_id = "{env.TEST_DEFAULT_WAREHOUSE_DATASOURCE_ID}"
name = "tf-{var.RANDOM}-query"
Expand Down
20 changes: 18 additions & 2 deletions permissions/resource_permissions.go
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,23 @@ type PermissionsEntity struct {
AccessControlList []AccessControlChange `json:"access_control" tf:"slice_set"`
}

func (oa *ObjectACL) isMatchingMapping(mapping permissionsIDFieldMapping) bool {
if mapping.objectType != oa.ObjectType {
return false
}
if oa.ObjectID != "" && oa.ObjectID[0] == '/' {
return strings.HasPrefix(oa.ObjectID[1:], mapping.resourceType)
}
if strings.HasPrefix(oa.ObjectID, "dashboards/") || strings.HasPrefix(oa.ObjectID, "alerts/") || strings.HasPrefix(oa.ObjectID, "queries/") {
idx := strings.Index(oa.ObjectID, "/")
if idx != -1 {
return mapping.resourceType == "sql/"+oa.ObjectID[:idx]
}
}

return false
}

func (oa *ObjectACL) ToPermissionsEntity(d *schema.ResourceData, me string) (PermissionsEntity, error) {
entity := PermissionsEntity{}
for _, accessControl := range oa.AccessControlList {
Expand All @@ -342,10 +359,9 @@ func (oa *ObjectACL) ToPermissionsEntity(d *schema.ResourceData, me string) (Per
}
}
for _, mapping := range permissionsResourceIDFields() {
if mapping.objectType != oa.ObjectType || !strings.HasPrefix(oa.ObjectID[1:], mapping.resourceType) {
if !oa.isMatchingMapping(mapping) {
continue
}
log.Printf("[DEBUG] mapping %v for object %v", mapping, oa)
entity.ObjectType = mapping.objectType
var pathVariant any
if mapping.objectType == "file" {
Expand Down
12 changes: 6 additions & 6 deletions permissions/resource_permissions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ func TestResourcePermissionsRead_SQLA_Asset(t *testing.T) {
Method: http.MethodGet,
Resource: "/api/2.0/preview/sql/permissions/dashboards/abc",
Response: ObjectACL{
ObjectID: "/sql/dashboards/abc",
ObjectID: "dashboards/abc",
ObjectType: "dashboard",
AccessControlList: []AccessControl{
{
Expand Down Expand Up @@ -812,7 +812,7 @@ func TestResourcePermissionsCreate_SQLA_Asset(t *testing.T) {
Method: http.MethodGet,
Resource: "/api/2.0/preview/sql/permissions/dashboards/abc",
Response: ObjectACL{
ObjectID: "/sql/dashboards/abc",
ObjectID: "dashboards/abc",
ObjectType: "dashboard",
AccessControlList: []AccessControl{
{
Expand Down Expand Up @@ -871,7 +871,7 @@ func TestResourcePermissionsCreate_SQLA_Endpoint(t *testing.T) {
Method: http.MethodGet,
Resource: "/api/2.0/permissions/sql/warehouses/abc",
Response: ObjectACL{
ObjectID: "/sql/dashboards/abc",
ObjectID: "dashboards/abc",
ObjectType: "dashboard",
AccessControlList: []AccessControl{
{
Expand Down Expand Up @@ -934,7 +934,7 @@ func TestResourcePermissionsCreate_SQLA_Endpoint_WithOwner(t *testing.T) {
Method: http.MethodGet,
Resource: "/api/2.0/permissions/sql/warehouses/abc",
Response: ObjectACL{
ObjectID: "/sql/dashboards/abc",
ObjectID: "dashboards/abc",
ObjectType: "dashboard",
AccessControlList: []AccessControl{
{
Expand Down Expand Up @@ -1651,7 +1651,7 @@ func TestResourcePermissionsCreate_Sql_Queries(t *testing.T) {
Method: http.MethodGet,
Resource: "/api/2.0/preview/sql/permissions/queries/id111",
Response: ObjectACL{
ObjectID: "/sql/queries/id111",
ObjectID: "queries/id111",
ObjectType: "query",
AccessControlList: []AccessControl{
{
Expand Down Expand Up @@ -1711,7 +1711,7 @@ func TestResourcePermissionsUpdate_Sql_Queries(t *testing.T) {
Method: http.MethodGet,
Resource: "/api/2.0/preview/sql/permissions/queries/id111",
Response: ObjectACL{
ObjectID: "/sql/queries/id111",
ObjectID: "queries/id111",
ObjectType: "query",
AccessControlList: []AccessControl{
{
Expand Down

0 comments on commit 165d129

Please sign in to comment.