Don't rely on having @ to check if it's user or SP (#2765)

* Fix: don't rely on having `@` to check if it's user or SP

There are some installations where user name isn't equal to email - for them our detection
of username vs SP ID is broken, and lead to some problems, like, generation of erroneous
`run_as` blocks in job definitions, etc.

This PR fixes this by validating a given string as UUID, and if it's matches, then uses it
as SP ID, and the rest as user name.  Changes affect following:

* `databricks_job` API - besides validation, it also doesn't fill the `RunAs` structure on
  read when creator user name == run as user name.
* `databricks_current_user` data source - should fix generation of `acl_principal_id`

* Update jobs/data_job_test.go

Co-authored-by: Miles Yucht <[email protected]>

* Add integration test

---------

Co-authored-by: Miles Yucht <[email protected]>

Author: alexott

common/util.go

@@ -0,0 +1,13 @@
+package common
+
+import (
+	"regexp"
+)
+
+var (
+	uuidRegex = regexp.MustCompile(`^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`)
+)
+
+func StringIsUUID(s string) bool {
+	return uuidRegex.MatchString(s)
+}

common/util_test.go

@@ -0,0 +1,12 @@
+package common
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestStringIsUUID(t *testing.T) {
+	assert.True(t, StringIsUUID("3f670caf-9a4b-4479-8143-1a0878da8f57"))
+	assert.False(t, StringIsUUID("abc"))
+}

exporter/importables.go

@@ -42,7 +42,6 @@ var (
 	nameNormalizationRegex       = regexp.MustCompile(`\W+`)
 	jobClustersRegex             = regexp.MustCompile(`^((job_cluster|task)\.[0-9]+\.new_cluster\.[0-9]+\.)`)
 	dltClusterRegex              = regexp.MustCompile(`^(cluster\.[0-9]+\.)`)
-	uuidRegex                    = regexp.MustCompile(`^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`)
 	predefinedClusterPolicies    = []string{"Personal Compute", "Job Compute", "Power User Compute", "Shared Compute"}
 	secretPathRegex              = regexp.MustCompile(`^\{\{secrets\/([^\/]+)\/([^}]+)\}\}$`)
 	sqlParentRegexp              = regexp.MustCompile(`^folders/(\d+)$`)

exporter/util.go

@@ -96,13 +96,15 @@ func (ic *importContext) emitUserOrServicePrincipal(userOrSPName string) {
 	if userOrSPName == "" {
 		return
 	}
+	// TODO: think about another way of checking for a user. ideally we need to check against the
+	// list of users/SPs obtained via SCIM API - this will be done in the refactoring requested by the SCIM team
 	if strings.Contains(userOrSPName, "@") {
 		ic.Emit(&resource{
 			Resource:  "databricks_user",
 			Attribute: "user_name",
 			Value:     userOrSPName,
 		})
-	} else if uuidRegex.MatchString(userOrSPName) {
+	} else if common.StringIsUUID(userOrSPName) {
 		ic.Emit(&resource{
 			Resource:  "databricks_service_principal",
 			Attribute: "application_id",
@@ -126,8 +128,10 @@ func (ic *importContext) IsUserOrServicePrincipalDirectory(path, prefix string)
 	}
 	parts := strings.SplitN(path, "/", 4)
 	if len(parts) == 3 || (len(parts) == 4 && parts[3] == "") {
+		// TODO: think about another way of checking for a user. ideally we need to check against the
+		// list of users/SPs obtained via SCIM API - this will be done in the refactoring requested by the SCIM team
 		userOrSPName := parts[2]
-		return strings.Contains(userOrSPName, "@") || uuidRegex.MatchString(userOrSPName)
+		return strings.Contains(userOrSPName, "@") || common.StringIsUUID(userOrSPName)
 	}
 	return false
 }

internal/acceptance/job_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/databricks/databricks-sdk-go"
 	"github.com/databricks/databricks-sdk-go/service/jobs"
 	"github.com/databricks/terraform-provider-databricks/common"
+	"github.com/databricks/terraform-provider-databricks/qa"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -216,3 +217,65 @@ func TestAccJobControlRunState(t *testing.T) {
 		Check:    resourceCheck("databricks_job.this", waitForRunToStart),
 	})
 }
+
+func runAsTemplate(runAs string) string {
+	return `
+	data "databricks_current_user" "me" {}
+	data "databricks_spark_version" "latest" {}
+	data "databricks_node_type" "smallest" {
+		local_disk = true
+	}
+
+	resource "databricks_notebook" "this" {
+		path     = "${data.databricks_current_user.me.home}/Terraform{var.RANDOM}"
+		language = "PYTHON"
+		content_base64 = base64encode(<<-EOT
+			# created from ${abspath(path.module)}
+			display(spark.range(10))
+			EOT
+		)
+	}
+
+	resource "databricks_job" "this" {
+		name = "{var.RANDOM}"
+
+		job_cluster {
+			job_cluster_key = "j"
+			new_cluster {
+				num_workers   = 20
+				spark_version = data.databricks_spark_version.latest.id
+				node_type_id  = data.databricks_node_type.smallest.id
+			}
+		}
+
+		task {
+			task_key = "c"
+			job_cluster_key = "j"
+			notebook_task {
+				notebook_path = databricks_notebook.this.path
+			}
+		}
+
+		run_as {
+			` + runAs + `
+		}
+	}`
+}
+
+func TestAccJobRunAsUser(t *testing.T) {
+	workspaceLevel(t, step{
+		Template: `
+		resource "databricks_user" "this" {
+			user_name = "` + qa.RandomEmail() + `"
+		}
+	` + runAsTemplate(`user_name = databricks_user.this.user_name`),
+	})
+}
+
+func TestAccJobRunAsServicePrincipal(t *testing.T) {
+	loadDebugEnvIfRunsFromIDE(t, "ucws")
+	spId := GetEnvOrSkipTest(t, "ACCOUNT_LEVEL_SERVICE_PRINCIPAL_ID")
+	unityWorkspaceLevel(t, step{
+		Template: runAsTemplate(`service_principal_name = "` + spId + `"`),
+	})
+}

jobs/data_job_test.go

@@ -84,6 +84,97 @@ func TestDataSourceQueryableJobMatchesId(t *testing.T) {
 	})
 }
 
+func TestDataSourceQueryableJobRunAsSP(t *testing.T) {
+	spID := "3f670caf-9a4b-4479-8143-1a0878da8f57"
+	qa.ResourceFixture{
+		Fixtures: []qa.HTTPFixture{
+			{
+				Method:   "GET",
+				Resource: "/api/2.0/jobs/get?job_id=234",
+				Response: Job{
+					JobID: 234,
+					Settings: &JobSettings{
+						Name: "Second",
+					},
+					CreatorUserName: "[email protected]",
+					RunAsUserName:   spID,
+				},
+			},
+		},
+		Resource:    DataSourceJob(),
+		Read:        true,
+		New:         true,
+		NonWritable: true,
+		HCL:         `job_id = "234"`,
+		ID:          "234",
+	}.ApplyAndExpectData(t, map[string]any{
+		"job_id":                         "234",
+		"id":                             "234",
+		"job_settings.0.settings.0.name": "Second",
+		"job_settings.0.settings.0.run_as.0.service_principal_name": spID,
+	})
+}
+
+func TestDataSourceQueryableJobRunAsSameUser(t *testing.T) {
+	qa.ResourceFixture{
+		Fixtures: []qa.HTTPFixture{
+			{
+				Method:   "GET",
+				Resource: "/api/2.0/jobs/get?job_id=234",
+				Response: Job{
+					JobID: 234,
+					Settings: &JobSettings{
+						Name: "Second",
+					},
+					CreatorUserName: "[email protected]",
+					RunAsUserName:   "[email protected]",
+				},
+			},
+		},
+		Resource:    DataSourceJob(),
+		Read:        true,
+		New:         true,
+		NonWritable: true,
+		HCL:         `job_id = "234"`,
+		ID:          "234",
+	}.ApplyAndExpectData(t, map[string]any{
+		"job_id":                             "234",
+		"id":                                 "234",
+		"job_settings.0.settings.0.name":     "Second",
+		"job_settings.0.settings.0.run_as.0": map[string]any{},
+	})
+}
+
+func TestDataSourceQueryableJobRunAsAnotherUser(t *testing.T) {
+	qa.ResourceFixture{
+		Fixtures: []qa.HTTPFixture{
+			{
+				Method:   "GET",
+				Resource: "/api/2.0/jobs/get?job_id=234",
+				Response: Job{
+					JobID: 234,
+					Settings: &JobSettings{
+						Name: "Second",
+					},
+					CreatorUserName: "[email protected]",
+					RunAsUserName:   "[email protected]",
+				},
+			},
+		},
+		Resource:    DataSourceJob(),
+		Read:        true,
+		New:         true,
+		NonWritable: true,
+		HCL:         `job_id = "234"`,
+		ID:          "234",
+	}.ApplyAndExpectData(t, map[string]any{
+		"job_id":                         "234",
+		"id":                             "234",
+		"job_settings.0.settings.0.name": "Second",
+		"job_settings.0.settings.0.run_as.0.user_name": "[email protected]",
+	})
+}
+
 func TestDataSourceQueryableJobMatchesName(t *testing.T) {
 	qa.ResourceFixture{
 		Fixtures:    commonFixtures("First"),

jobs/resource_job.go

@@ -572,16 +572,14 @@ func (a JobsAPI) Read(id string) (job Job, err error) {
 		job.Settings.sortWebhooksByID()
 	}
 
-	if job.RunAsUserName != "" && job.Settings != nil {
-		userNameIsEmail := strings.Contains(job.RunAsUserName, "@")
-
-		if userNameIsEmail {
+	if job.Settings != nil && job.RunAsUserName != "" && job.RunAsUserName != job.CreatorUserName {
+		if common.StringIsUUID(job.RunAsUserName) {
 			job.Settings.RunAs = &JobRunAs{
-				UserName: job.RunAsUserName,
+				ServicePrincipalName: job.RunAsUserName,
 			}
 		} else {
 			job.Settings.RunAs = &JobRunAs{
-				ServicePrincipalName: job.RunAsUserName,
+				UserName: job.RunAsUserName,
 			}
 		}
 	}

scim/data_current_user.go

@@ -59,10 +59,10 @@ func DataSourceCurrentUser() *schema.Resource {
 			d.Set("user_name", me.UserName)
 			d.Set("home", fmt.Sprintf("/Users/%s", me.UserName))
 			d.Set("repos", fmt.Sprintf("/Repos/%s", me.UserName))
-			if strings.Contains(me.UserName, "@") {
-				d.Set("acl_principal_id", fmt.Sprintf("users/%s", me.UserName))
-			} else {
+			if common.StringIsUUID(me.UserName) {
 				d.Set("acl_principal_id", fmt.Sprintf("servicePrincipals/%s", me.UserName))
+			} else {
+				d.Set("acl_principal_id", fmt.Sprintf("users/%s", me.UserName))
 			}
 			d.Set("external_id", me.ExternalId)
 			splits := strings.Split(me.UserName, "@")