Bugfix: UPN single quote escaping

[daramir]

This PR addresses #639.

We need to escape User Principal Names before querying Microsoft Graph.

I'm unable to run acceptance tests at the moment, but I have included a test.

[manicminer]

Hi @daramir, thanks for the PR!

Overall this looks good, I have one suggestion for approaching the test configuration. If you can consider this, and the tests pass, then this should be good to merge. Thanks!

[manicminer]

@daramir Thanks for the updates! This is almost good to go, there are a couple of test fixes to make but unfortunately I don't have permission to push to your branch. I've made one suggestion below, and there are a couple of other changes:

diff --git a/internal/services/users/user_resource_test.go b/internal/services/users/user_resource_test.go
index b96e9ab4..e28395a8 100644
--- a/internal/services/users/user_resource_test.go
+++ b/internal/services/users/user_resource_test.go
@@ -149,7 +149,7 @@ data "azuread_domains" "test" {
 }

 resource "azuread_user" "test" {
-  user_principal_name = "acctestUser.%[1]d@${data.azuread_domains.test.domains.0.domain_name}"
+  user_principal_name = "acctestUser'%[1]d@${data.azuread_domains.test.domains.0.domain_name}"
   display_name        = "acctestUser-%[1]d"
   password            = "%[2]s"
 }
@@ -171,7 +171,7 @@ resource "azuread_user" "manager" {
 }

 resource "azuread_user" "test" {
-  user_principal_name = "acct'estUser.%[1]d@${data.azuread_domains.test.domains.0.domain_name}"
+  user_principal_name = "acctestUser'%[1]d@${data.azuread_domains.test.domains.0.domain_name}"
   mail                = "acctestUser.%[1]d@hashicorp.biz"
   mail_nickname       = "acctestUser-%[1]d-MailNickname"
   other_mails         = ["acctestUser.%[1]d@hashicorp.net", "acctestUser.%[1]d@hashicorp.org"]
@@ -223,7 +223,7 @@ data "azuread_domains" "test" {
 }

 resource "azuread_user" "testA" {
-  user_principal_name = "acctestUser.%[1]d.A@${data.azuread_domains.test.domains.0.domain_name}"
+  user_principal_name = "acctestUser'%[1]d.A@${data.azuread_domains.test.domains.0.domain_name}"
   display_name        = "acctestUser-%[1]d-A"
   password            = "%[2]s"
 }

These are because some of our tests use several configs to update resources in place.

If you can update the tests as per the above, this will be good to merge. Thanks!

[manicminer]

Thanks @daramir, this LGTM!

One thing I forgot to mention is why I suggested moving the apostrophe around - we try to prefix the primary text field of test resources with the string acctest (case doesn't matter) as this makes it easier to clean up after failed tests.

[manicminer]

@daramir Sorry, it looks like we also need to ensure single quotes are escape for mailNicknames too. I'm still unable to push to your fork, not sure why as GH only returns "access denied". Could you add this too?

diff --git a/internal/services/users/user_data_source.go b/internal/services/users/user_data_source.go
index 3e2ed884..58bab1c7 100644
--- a/internal/services/users/user_data_source.go
+++ b/internal/services/users/user_data_source.go
@@ -347,7 +347,7 @@ func userDataSourceRead(ctx context.Context, d *schema.ResourceData, meta interf
                user = *u
        } else if mailNickname, ok := d.Get("mail_nickname").(string); ok && mailNickname != "" {
                query := odata.Query{
-                       Filter: fmt.Sprintf("mailNickname eq '%s'", mailNickname),
+                       Filter: fmt.Sprintf("mailNickname eq '%s'", utils.EscapeSingleQuote(mailNickname)),
                }
                users, _, err := client.List(ctx, query)
                if err != nil {
diff --git a/internal/services/users/users_data_source.go b/internal/services/users/users_data_source.go
index fee72eff..087e8f47 100644
--- a/internal/services/users/users_data_source.go
+++ b/internal/services/users/users_data_source.go
@@ -223,7 +223,7 @@ func usersDataSourceRead(ctx context.Context, d *schema.ResourceData, meta inter
                        expectedCount = len(mailNicknames)
                        for _, v := range mailNicknames {
                                query := odata.Query{
-                                       Filter: fmt.Sprintf("mailNickname eq '%s'", v),
+                                       Filter: fmt.Sprintf("mailNickname eq '%s'", utils.EscapeSingleQuote(v.(string))),
                                }
                                result, _, err := client.List(ctx, query)
                                if err != nil {

Thanks!

[daramir]

@daramir Sorry, it looks like we also need to ensure single quotes are escape for mailNicknames too. I'm still unable to push to your fork, not sure why as GH only returns "access denied". Could you add this too?
(...)
Thanks!

Hi @manicminer , the above has been included. Please have a look!

[manicminer]

Thanks @daramir, this LGTM and tests are passing 🚀

[github-actions]

This functionality has been released in v2.9.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.