Workaround: auto-retry group creation without calling principal as ow…

…ner, partial workaround for microsoftgraph/msgraph-metadata#92
hashicorp · Feb 27, 2023 · 49587c9 · 49587c9
1 parent 5896f2d
commit 49587c9
Show file tree

Hide file tree

Showing 2 changed files with 94 additions and 7 deletions.
diff --git a/internal/services/groups/group_resource.go b/internal/services/groups/group_resource.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"log"
 	"net/http"
+	"regexp"
 	"strings"
 	"time"
 
@@ -22,7 +23,10 @@ import (
 	"github.com/manicminer/hamilton/msgraph"
 )
 
-const groupResourceName = "azuread_group"
+const (
+	groupResourceName        = "azuread_group"
+	groupDuplicateValueError = "Request contains a property with duplicate values"
+)
 
 func groupResource() *schema.Resource {
 	return &schema.Resource{
@@ -582,28 +586,84 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter
 	properties.Owners = &ownersFirst20
 
 	var group *msgraph.Group
+	var status int
 	var err error
 
 	if v, ok := d.GetOk("administrative_unit_ids"); ok {
 		administrativeUnitIds := tf.ExpandStringSlice(v.(*schema.Set).List())
 		for i, administrativeUnitId := range administrativeUnitIds {
 			// Create the group in the first administrative unit, as this requires fewer permissions than creating it at tenant level
 			if i == 0 {
-				group, _, err = administrativeUnitsClient.CreateGroup(ctx, administrativeUnitId, &properties)
+				group, status, err = administrativeUnitsClient.CreateGroup(ctx, administrativeUnitId, &properties)
 				if err != nil {
-					return tf.ErrorDiagF(err, "Creating group in administrative unit with ID %q, %q", administrativeUnitId, displayName)
+					if status == http.StatusBadRequest && regexp.MustCompile(groupDuplicateValueError).MatchString(err.Error()) {
+						// Retry the request, without the calling principal as owner
+						newOwners := make(msgraph.Owners, 0)
+						for _, o := range *properties.Owners {
+							if id := o.ID(); id != nil && *id != callerId {
+								newOwners = append(newOwners, o)
+							}
+						}
+
+						// No point in retrying if the caller wasn't specified
+						if len(newOwners) == len(*properties.Owners) {
+							log.Printf("[DEBUG] Not retrying group creation for %q within AU %q as owner was not specified", displayName, administrativeUnitId)
+							return tf.ErrorDiagF(err, "Creating group in administrative unit with ID %q, %q", administrativeUnitId, displayName)
+						}
+
+						// If the API is refusing the calling principal as owner, it will typically automatically append the caller in the background,
+						// and subsequent GETs for the group will include the calling principal as owner, as if it were specified when creating.
+						log.Printf("[DEBUG] Retrying group creation for %q within AU %q without calling principal as owner", displayName, administrativeUnitId)
+						properties.Owners = &newOwners
+						group, status, err = administrativeUnitsClient.CreateGroup(ctx, administrativeUnitId, &properties)
+						if err != nil {
+							return tf.ErrorDiagF(err, "Creating group in administrative unit with ID %q, %q", administrativeUnitId, displayName)
+						}
+					} else {
+						return tf.ErrorDiagF(err, "Creating group in administrative unit with ID %q, %q", administrativeUnitId, displayName)
+					}
 				}
 			} else {
-				err := addGroupToAdministrativeUnit(ctx, administrativeUnitsClient, administrativeUnitId, group)
+				err = addGroupToAdministrativeUnit(ctx, administrativeUnitsClient, administrativeUnitId, group)
 				if err != nil {
-					return tf.ErrorDiagF(err, "Could not add group %q to administrative unit with object ID: %q", *group.ID(), administrativeUnitId)
+					return tf.ErrorDiagF(err, "Adding group %q to administrative unit with object ID: %q", *group.ID(), administrativeUnitId)
 				}
 			}
 		}
 	} else {
-		group, _, err = client.Create(ctx, properties)
+		group, status, err = client.Create(ctx, properties)
 		if err != nil {
-			return tf.ErrorDiagF(err, "Creating group %q", displayName)
+			if status == http.StatusBadRequest && regexp.MustCompile(groupDuplicateValueError).MatchString(err.Error()) {
+				// Retry the request, without the calling principal as owner
+				newOwners := make(msgraph.Owners, 0)
+				for _, o := range *properties.Owners {
+					if id := o.ID(); id != nil && *id != callerId {
+						newOwners = append(newOwners, o)
+					}
+				}
+
+				// No point in retrying if the caller wasn't specified
+				if len(newOwners) == len(*properties.Owners) {
+					log.Printf("[DEBUG] Not retrying group creation for %q as owner was not specified", displayName)
+					return tf.ErrorDiagF(err, "Creating group %q", displayName)
+				}
+
+				// If the API is refusing the calling principal as owner, it will typically automatically append the caller in the background,
+				// and subsequent GETs for the group will include the calling principal as owner, as if it were specified when creating.
+				log.Printf("[DEBUG] Retrying group creation for %q without calling principal as owner", displayName)
+				if len(newOwners) == 0 {
+					properties.Owners = nil
+				} else {
+					properties.Owners = &newOwners
+				}
+
+				group, status, err = client.Create(ctx, properties)
+				if err != nil {
+					return tf.ErrorDiagF(err, "Creating group %q", displayName)
+				}
+			} else {
+				return tf.ErrorDiagF(err, "Creating group %q", displayName)
+			}
 		}
 	}
 

diff --git a/internal/services/groups/group_resource_test.go b/internal/services/groups/group_resource_test.go
@@ -159,6 +159,21 @@ func TestAccGroup_dynamicMembership(t *testing.T) {
 	})
 }
 
+func TestAccGroup_callerOwner(t *testing.T) {
+	data := acceptance.BuildTestData(t, "azuread_group", "test")
+	r := GroupResource{}
+
+	data.ResourceTest(t, r, []resource.TestStep{
+		{
+			Config: r.withCallerAsOwner(data),
+			Check: resource.ComposeTestCheckFunc(
+				check.That(data.ResourceName).ExistsInAzure(r),
+			),
+		},
+		data.ImportStep(),
+	})
+}
+
 func TestAccGroup_owners(t *testing.T) {
 	data := acceptance.BuildTestData(t, "azuread_group", "test")
 	r := GroupResource{}
@@ -714,6 +729,18 @@ resource "azuread_group" "test" {
 `, r.templateThreeUsers(data), data.RandomInteger)
 }
 
+func (GroupResource) withCallerAsOwner(data acceptance.TestData) string {
+	return fmt.Sprintf(`
+data "azuread_client_config" "test" {}
+
+resource "azuread_group" "test" {
+  display_name     = "acctestGroup-%[1]d"
+  security_enabled = true
+  owners           = [data.azuread_client_config.test.object_id]
+}
+`, data.RandomInteger)
+}
+
 func (GroupResource) withServicePrincipalOwner(data acceptance.TestData) string {
 	return fmt.Sprintf(`
 resource "azuread_application" "test" {