Skip to content

Commit

Permalink
Merge pull request #1076 from hashicorp/bugfix/group-regressions
Browse files Browse the repository at this point in the history 
azuread_group: regression fixes
  • Loading branch information
@manicminer
manicminer committed Apr 19, 2023
2 parents 02f8c6e + 7b775a4 commit 9246ef8
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 58 deletions.
2 changes: 1 addition & 1 deletion docs/resources/group.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
25 changes: 13 additions & 12 deletions internal/services/groups/group_data_source.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}

Expand Down
84 changes: 40 additions & 44 deletions internal/services/groups/group_resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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(),
},
Expand All @@ -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
}
Expand Down Expand Up @@ -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())
}
Expand All @@ -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())
}
Expand All @@ -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())
}
Expand All @@ -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())
}
Expand Down Expand Up @@ -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(),
Expand All @@ -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(),
Expand All @@ -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(),
Expand All @@ -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(),
Expand All @@ -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())
}
Expand Down Expand Up @@ -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
}
}
}

Expand Down
7 changes: 7 additions & 0 deletions internal/services/groups/group_resource_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
})
}

Expand Down
9 changes: 8 additions & 1 deletion internal/services/groups/groups.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"math/rand"
"net/http"
"time"

"github.com/hashicorp/go-azure-sdk/sdk/odata"
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 9246ef8

Please sign in to comment.