azuread_group: regression fixes

docs/resources/group.md

@@ -141,7 +141,7 @@ The following arguments are supported:
 
 !> **Warning** Do not use the `members` property at the same time as the [azuread_group_member](https://registry.terraform.io/providers/hashicorp/azuread/latest/docs/resources/group_member) resource for the same group. Doing so will cause a conflict and group members will be removed.
 
-* `onpremises_group_type` - (Optional) The on-premises group type that the AAD group will be written as, when writeback is enabled. Possible values are `UniversalDistributionGroup`, `UniversalMailEnabledSecurityGroup`, or `UniversalSecurityGroup`. Once set, unsetting this property forces a new resource to be created.
+* `onpremises_group_type` - (Optional) The on-premises group type that the AAD group will be written as, when writeback is enabled. Possible values are `UniversalDistributionGroup`, `UniversalMailEnabledSecurityGroup`, or `UniversalSecurityGroup`.
 * `owners` - (Optional) A set of object IDs of principals that will be granted ownership of the group. Supported object types are users or service principals. By default, the principal being used to execute Terraform is assigned as the sole owner. Groups cannot be created with no owners or have all their owners removed.
 
 -> **Group Ownership**  It's recommended to always specify one or more group owners, including the principal being used to execute Terraform, such as in the example above. When removing group owners, if a user principal has been assigned ownership, the last user cannot be removed as an owner. Microsoft 365 groups are required to always have at least one owner which _must be a user_ (i.e. not a service principal).

internal/services/groups/group_data_source.go

@@ -370,18 +370,19 @@ func groupDataSourceRead(ctx context.Context, d *schema.ResourceData, meta inter
 		if err != nil {
 			return tf.ErrorDiagF(err, "Could not retrieve group with object ID %q", d.Id())
 		}
-
-		if groupExtra != nil && groupExtra.AllowExternalSenders != nil {
-			allowExternalSenders = *groupExtra.AllowExternalSenders
-		}
-		if groupExtra != nil && groupExtra.AutoSubscribeNewMembers != nil {
-			autoSubscribeNewMembers = *groupExtra.AutoSubscribeNewMembers
-		}
-		if groupExtra != nil && groupExtra.HideFromAddressLists != nil {
-			hideFromAddressLists = *groupExtra.HideFromAddressLists
-		}
-		if groupExtra != nil && groupExtra.HideFromOutlookClients != nil {
-			hideFromOutlookClients = *groupExtra.HideFromOutlookClients
+		if groupExtra != nil {
+			if groupExtra.AllowExternalSenders != nil {
+				allowExternalSenders = *groupExtra.AllowExternalSenders
+			}
+			if groupExtra.AutoSubscribeNewMembers != nil {
+				autoSubscribeNewMembers = *groupExtra.AutoSubscribeNewMembers
+			}
+			if groupExtra.HideFromAddressLists != nil {
+				hideFromAddressLists = *groupExtra.HideFromAddressLists
+			}
+			if groupExtra.HideFromOutlookClients != nil {
+				hideFromOutlookClients = *groupExtra.HideFromOutlookClients
+			}
 		}
 	}
 

internal/services/groups/group_resource.go

@@ -183,6 +183,7 @@ func groupResource() *schema.Resource {
 				Description: "Indicates the target on-premise group type the group will be written back as",
 				Type:        schema.TypeString,
 				Optional:    true,
+				Computed:    true,
 				ValidateFunc: validation.StringInSlice([]string{
 					msgraph.UniversalDistributionGroup,
 					msgraph.UniversalSecurityGroup,
@@ -363,12 +364,6 @@ func groupResourceCustomizeDiff(ctx context.Context, diff *schema.ResourceDiff,
 		}
 	}
 
-	// onPremisesGroupType can't be unset
-	oldOnPremisesGroupType, newOnPremisesGroupType := diff.GetChange("onpremises_group_type")
-	if oldOnPremisesGroupType.(string) != "" && newOnPremisesGroupType == "" {
-		diff.ForceNew("onpremises_group_type")
-	}
-
 	mailEnabled := diff.Get("mail_enabled").(bool)
 	securityEnabled := diff.Get("security_enabled").(bool)
 	groupTypes := make([]msgraph.GroupType, 0)
@@ -747,19 +742,18 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter
 
 	if hasGroupType(groupTypes, msgraph.GroupTypeUnified) {
 		// Newly created Unified groups now get a description added out-of-band, so we'll wait a couple of minutes to see if this appears and then clear it
+		// See https://github.com/microsoftgraph/msgraph-metadata/issues/331
 		if description == "" {
 			// Ignoring the error result here because the description might not be updated out of band, in which case we skip over this
-			updated, _ := helpers.WaitForUpdateWithTimeout(ctx, 2*time.Minute, func(ctx context.Context) (*bool, error) {
+			if updated, _ := helpers.WaitForUpdateWithTimeout(ctx, 2*time.Minute, func(ctx context.Context) (*bool, error) {
 				client.BaseClient.DisableRetries = true
 				group, _, err := client.Get(ctx, *group.ID(), odata.Query{})
 				if err != nil {
 					return nil, err
 				}
 				return utils.Bool(group.Description != nil && *group.Description != ""), nil
-			})
-
-			if updated {
-				status, err := client.Update(ctx, msgraph.Group{
+			}); updated {
+				status, err = client.Update(ctx, msgraph.Group{
 					DirectoryObject: msgraph.DirectoryObject{
 						Id: group.ID(),
 					},
@@ -773,9 +767,9 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter
 				}
 
 				// Wait for Description to be removed
-				if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) {
+				if err = helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) {
 					client.BaseClient.DisableRetries = true
-					group, _, err := client.Get(ctx, *group.ID(), odata.Query{})
+					group, _, err = client.Get(ctx, *group.ID(), odata.Query{})
 					if err != nil {
 						return nil, err
 					}
@@ -805,11 +799,11 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter
 			// Wait for AllowExternalSenders to be updated
 			if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) {
 				client.BaseClient.DisableRetries = true
-				group, err := groupGetAdditional(ctx, client, *group.ID())
+				groupExtra, err := groupGetAdditional(ctx, client, *group.ID())
 				if err != nil {
 					return nil, err
 				}
-				return utils.Bool(group.AllowExternalSenders != nil && *group.AllowExternalSenders == allowExternalSenders), nil
+				return utils.Bool(groupExtra != nil && groupExtra.AllowExternalSenders != nil && *groupExtra.AllowExternalSenders == allowExternalSenders), nil
 			}); err != nil {
 				return tf.ErrorDiagF(err, "Waiting for update of `external_senders_allowed` for group with object ID %q", *group.ID())
 			}
@@ -829,11 +823,11 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter
 			// Wait for AutoSubscribeNewMembers to be updated
 			if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) {
 				client.BaseClient.DisableRetries = true
-				group, err := groupGetAdditional(ctx, client, *group.ID())
+				groupExtra, err := groupGetAdditional(ctx, client, *group.ID())
 				if err != nil {
 					return nil, err
 				}
-				return utils.Bool(group.AutoSubscribeNewMembers != nil && *group.AutoSubscribeNewMembers == autoSubscribeNewMembers), nil
+				return utils.Bool(groupExtra != nil && groupExtra.AutoSubscribeNewMembers != nil && *groupExtra.AutoSubscribeNewMembers == autoSubscribeNewMembers), nil
 			}); err != nil {
 				return tf.ErrorDiagF(err, "Waiting for update of `auto_subscribe_new_members` for group with object ID %q", *group.ID())
 			}
@@ -853,11 +847,11 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter
 			// Wait for HideFromAddressLists to be updated
 			if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) {
 				client.BaseClient.DisableRetries = true
-				group, err := groupGetAdditional(ctx, client, *group.ID())
+				groupExtra, err := groupGetAdditional(ctx, client, *group.ID())
 				if err != nil {
 					return nil, err
 				}
-				return utils.Bool(group.HideFromAddressLists != nil && *group.HideFromAddressLists == hideFromAddressList), nil
+				return utils.Bool(groupExtra != nil && groupExtra.HideFromAddressLists != nil && *groupExtra.HideFromAddressLists == hideFromAddressList), nil
 			}); err != nil {
 				return tf.ErrorDiagF(err, "Waiting for update of `hide_from_address_lists` for group with object ID %q", *group.ID())
 			}
@@ -877,11 +871,11 @@ func groupResourceCreate(ctx context.Context, d *schema.ResourceData, meta inter
 			// Wait for HideFromOutlookClients to be updated
 			if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) {
 				client.BaseClient.DisableRetries = true
-				group, err := groupGetAdditional(ctx, client, *group.ID())
+				groupExtra, err := groupGetAdditional(ctx, client, *group.ID())
 				if err != nil {
 					return nil, err
 				}
-				return utils.Bool(group.HideFromOutlookClients != nil && *group.HideFromOutlookClients == hideFromOutlookClients), nil
+				return utils.Bool(groupExtra != nil && groupExtra.HideFromOutlookClients != nil && *groupExtra.HideFromOutlookClients == hideFromOutlookClients), nil
 			}); err != nil {
 				return tf.ErrorDiagF(err, "Waiting for update of `hide_from_outlook_clients` for group with object ID %q", *group.ID())
 			}
@@ -1026,7 +1020,7 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter
 		}
 
 		// AllowExternalSenders can only be set in its own PATCH request; including other properties returns a 400
-		if v, ok := d.GetOkExists("external_senders_allowed"); ok && (extra.AllowExternalSenders == nil || *extra.AllowExternalSenders != v.(bool)) { //nolint:staticcheck
+		if v, ok := d.GetOkExists("external_senders_allowed"); ok && (extra == nil || extra.AllowExternalSenders == nil || *extra.AllowExternalSenders != v.(bool)) { //nolint:staticcheck
 			if _, err := client.Update(ctx, msgraph.Group{
 				DirectoryObject: msgraph.DirectoryObject{
 					Id: group.ID(),
@@ -1039,18 +1033,18 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter
 			// Wait for AllowExternalSenders to be updated
 			if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) {
 				client.BaseClient.DisableRetries = true
-				group, err := groupGetAdditional(ctx, client, *group.ID())
+				groupExtra, err := groupGetAdditional(ctx, client, *group.ID())
 				if err != nil {
 					return nil, err
 				}
-				return utils.Bool(group.AllowExternalSenders != nil && *group.AllowExternalSenders == v.(bool)), nil
+				return utils.Bool(groupExtra != nil && groupExtra.AllowExternalSenders != nil && *groupExtra.AllowExternalSenders == v.(bool)), nil
 			}); err != nil {
 				return tf.ErrorDiagF(err, "Waiting for update of `external_senders_allowed` for group with object ID %q", *group.ID())
 			}
 		}
 
 		// AutoSubscribeNewMembers can only be set in its own PATCH request; including other properties returns a 400
-		if v, ok := d.GetOkExists("auto_subscribe_new_members"); ok && (extra.AutoSubscribeNewMembers == nil || *extra.AutoSubscribeNewMembers != v.(bool)) { //nolint:staticcheck
+		if v, ok := d.GetOkExists("auto_subscribe_new_members"); ok && (extra == nil || extra.AutoSubscribeNewMembers == nil || *extra.AutoSubscribeNewMembers != v.(bool)) { //nolint:staticcheck
 			if _, err := client.Update(ctx, msgraph.Group{
 				DirectoryObject: msgraph.DirectoryObject{
 					Id: group.ID(),
@@ -1063,18 +1057,18 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter
 			// Wait for AutoSubscribeNewMembers to be updated
 			if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) {
 				client.BaseClient.DisableRetries = true
-				group, err := groupGetAdditional(ctx, client, *group.ID())
+				groupExtra, err := groupGetAdditional(ctx, client, *group.ID())
 				if err != nil {
 					return nil, err
 				}
-				return utils.Bool(group.AutoSubscribeNewMembers != nil && *group.AutoSubscribeNewMembers == v.(bool)), nil
+				return utils.Bool(groupExtra != nil && groupExtra.AutoSubscribeNewMembers != nil && *groupExtra.AutoSubscribeNewMembers == v.(bool)), nil
 			}); err != nil {
 				return tf.ErrorDiagF(err, "Waiting for update of `auto_subscribe_new_members` for group with object ID %q", *group.ID())
 			}
 		}
 
 		// HideFromAddressLists can only be set in its own PATCH request; including other properties returns a 400
-		if v, ok := d.GetOkExists("hide_from_address_lists"); ok && (extra.HideFromAddressLists == nil || *extra.HideFromAddressLists != v.(bool)) { //nolint:staticcheck
+		if v, ok := d.GetOkExists("hide_from_address_lists"); ok && (extra == nil || extra.HideFromAddressLists == nil || *extra.HideFromAddressLists != v.(bool)) { //nolint:staticcheck
 			if _, err := client.Update(ctx, msgraph.Group{
 				DirectoryObject: msgraph.DirectoryObject{
 					Id: group.ID(),
@@ -1087,18 +1081,18 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter
 			// Wait for HideFromAddressLists to be updated
 			if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) {
 				client.BaseClient.DisableRetries = true
-				group, err := groupGetAdditional(ctx, client, *group.ID())
+				groupExtra, err := groupGetAdditional(ctx, client, *group.ID())
 				if err != nil {
 					return nil, err
 				}
-				return utils.Bool(group.HideFromAddressLists != nil && *group.HideFromAddressLists == v.(bool)), nil
+				return utils.Bool(groupExtra != nil && groupExtra.HideFromAddressLists != nil && *groupExtra.HideFromAddressLists == v.(bool)), nil
 			}); err != nil {
 				return tf.ErrorDiagF(err, "Waiting for update of `hide_from_address_lists` for group with object ID %q", *group.ID())
 			}
 		}
 
 		// HideFromOutlookClients can only be set in its own PATCH request; including other properties returns a 400
-		if v, ok := d.GetOkExists("hide_from_outlook_clients"); ok && (extra.HideFromOutlookClients == nil || *extra.HideFromOutlookClients != v.(bool)) { //nolint:staticcheck
+		if v, ok := d.GetOkExists("hide_from_outlook_clients"); ok && (extra == nil || extra.HideFromOutlookClients == nil || *extra.HideFromOutlookClients != v.(bool)) { //nolint:staticcheck
 			if _, err := client.Update(ctx, msgraph.Group{
 				DirectoryObject: msgraph.DirectoryObject{
 					Id: group.ID(),
@@ -1111,11 +1105,11 @@ func groupResourceUpdate(ctx context.Context, d *schema.ResourceData, meta inter
 			// Wait for HideFromOutlookClients to be updated
 			if err := helpers.WaitForUpdate(ctx, func(ctx context.Context) (*bool, error) {
 				client.BaseClient.DisableRetries = true
-				group, err := groupGetAdditional(ctx, client, *group.ID())
+				groupExtra, err := groupGetAdditional(ctx, client, *group.ID())
 				if err != nil {
 					return nil, err
 				}
-				return utils.Bool(group.HideFromOutlookClients != nil && *group.HideFromOutlookClients == v.(bool)), nil
+				return utils.Bool(groupExtra != nil && groupExtra.HideFromOutlookClients != nil && *groupExtra.HideFromOutlookClients == v.(bool)), nil
 			}); err != nil {
 				return tf.ErrorDiagF(err, "Waiting for update of `hide_from_outlook_clients` for group with object ID %q", *group.ID())
 			}
@@ -1313,17 +1307,19 @@ func groupResourceRead(ctx context.Context, d *schema.ResourceData, meta interfa
 			return tf.ErrorDiagF(err, "Could not retrieve group with object UID %q", d.Id())
 		}
 
-		if groupExtra != nil && groupExtra.AllowExternalSenders != nil {
-			allowExternalSenders = *groupExtra.AllowExternalSenders
-		}
-		if groupExtra != nil && groupExtra.AutoSubscribeNewMembers != nil {
-			autoSubscribeNewMembers = *groupExtra.AutoSubscribeNewMembers
-		}
-		if groupExtra != nil && groupExtra.HideFromAddressLists != nil {
-			hideFromAddressLists = *groupExtra.HideFromAddressLists
-		}
-		if groupExtra != nil && groupExtra.HideFromOutlookClients != nil {
-			hideFromOutlookClients = *groupExtra.HideFromOutlookClients
+		if groupExtra != nil {
+			if groupExtra.AllowExternalSenders != nil {
+				allowExternalSenders = *groupExtra.AllowExternalSenders
+			}
+			if groupExtra.AutoSubscribeNewMembers != nil {
+				autoSubscribeNewMembers = *groupExtra.AutoSubscribeNewMembers
+			}
+			if groupExtra.HideFromAddressLists != nil {
+				hideFromAddressLists = *groupExtra.HideFromAddressLists
+			}
+			if groupExtra.HideFromOutlookClients != nil {
+				hideFromOutlookClients = *groupExtra.HideFromOutlookClients
+			}
 		}
 	}
 

internal/services/groups/group_resource_test.go

@@ -516,6 +516,13 @@ func TestAccGroup_writebackUpdate(t *testing.T) {
 			),
 		},
 		data.ImportStep(),
+		{
+			Config: r.basic(data),
+			Check: resource.ComposeTestCheckFunc(
+				check.That(data.ResourceName).ExistsInAzure(r),
+			),
+		},
+		data.ImportStep(),
 	})
 }
 

internal/services/groups/groups.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"math/rand"
+	"net/http"
 	"time"
 
 	"github.com/hashicorp/go-azure-sdk/sdk/odata"
@@ -44,8 +45,14 @@ func groupFindByName(ctx context.Context, client *msgraph.GroupsClient, displayN
 
 func groupGetAdditional(ctx context.Context, client *msgraph.GroupsClient, id string) (*msgraph.Group, error) {
 	query := odata.Query{Select: []string{"allowExternalSenders", "autoSubscribeNewMembers", "hideFromAddressLists", "hideFromOutlookClients"}}
-	groupExtra, _, err := client.Get(ctx, id, query)
+	groupExtra, status, err := client.Get(ctx, id, query)
 	if err != nil {
+		if status == http.StatusNotFound {
+			// API returns 404 when these M365-only fields are requested for a group in a non-M365 tenant, so we
+			// don't raise an error in this case and proceed as if they are not set.
+			// See https://github.com/microsoftgraph/msgraph-metadata/issues/333
+			return nil, nil
+		}
 		return nil, fmt.Errorf("retrieving additional fields: %+v", err)
 	}
 	return groupExtra, nil