diff --git a/docs/resources/group.md b/docs/resources/group.md index 84b2c740ae..4c841d4db6 100644 --- a/docs/resources/group.md +++ b/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). diff --git a/internal/services/groups/group_data_source.go b/internal/services/groups/group_data_source.go index 98d264eeef..c034792313 100644 --- a/internal/services/groups/group_data_source.go +++ b/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 + } } } diff --git a/internal/services/groups/group_resource.go b/internal/services/groups/group_resource.go index 672d93ab86..e7a9eee11a 100644 --- a/internal/services/groups/group_resource.go +++ b/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 + } } } diff --git a/internal/services/groups/group_resource_test.go b/internal/services/groups/group_resource_test.go index c1c709c14d..1b0d2232a3 100644 --- a/internal/services/groups/group_resource_test.go +++ b/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(), }) } diff --git a/internal/services/groups/groups.go b/internal/services/groups/groups.go index c149255042..78702e12f2 100644 --- a/internal/services/groups/groups.go +++ b/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