From 16cbd0def85f432c90cc099278f8abe054f94b26 Mon Sep 17 00:00:00 2001 From: tiwood Date: Wed, 13 Mar 2019 18:01:09 +0100 Subject: [PATCH 01/25] New resource 'azuread_group_member' --- azuread/provider.go | 1 + azuread/resource_group_member.go | 156 +++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+) create mode 100644 azuread/resource_group_member.go diff --git a/azuread/provider.go b/azuread/provider.go index 1daa673133..a7b743b6a2 100644 --- a/azuread/provider.go +++ b/azuread/provider.go @@ -85,6 +85,7 @@ func Provider() terraform.ResourceProvider { "azuread_application": resourceApplication(), "azuread_application_password": resourceApplicationPassword(), "azuread_group": resourceGroup(), + "azuread_group_member": resourceGroupMember(), "azuread_service_principal": resourceServicePrincipal(), "azuread_service_principal_password": resourceServicePrincipalPassword(), "azuread_user": resourceUser(), diff --git a/azuread/resource_group_member.go b/azuread/resource_group_member.go new file mode 100644 index 0000000000..a3db16e079 --- /dev/null +++ b/azuread/resource_group_member.go @@ -0,0 +1,156 @@ +package azuread + +import ( + "fmt" + "log" + "strings" + + "github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac" + "github.com/hashicorp/terraform/helper/schema" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/ar" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/validate" +) + +func resourceGroupMember() *schema.Resource { + return &schema.Resource{ + Create: resourceGroupMemberCreate, + Read: resourceGroupMemberRead, + Delete: resourceGroupMemberDelete, + Importer: &schema.ResourceImporter{ + State: schema.ImportStatePassthrough, + }, + + Schema: map[string]*schema.Schema{ + "group_object_id": { + Type: schema.TypeString, + Required: true, + ForceNew: true, + ValidateFunc: validate.UUID, + }, + "member_object_id": { + Type: schema.TypeString, + Required: true, + ForceNew: true, + ValidateFunc: validate.UUID, + }, + }, + } +} + +func resourceGroupMemberCreate(d *schema.ResourceData, meta interface{}) error { + client := meta.(*ArmClient).groupsClient + ctx := meta.(*ArmClient).StopContext + + groupID := d.Get("group_object_id").(string) + memberID := d.Get("member_object_id").(string) + tenantID := client.TenantID + + memberGraphURL := fmt.Sprintf("https://graph.windows.net/%s/directoryObjects/%s", tenantID, memberID) + + properties := graphrbac.GroupAddMemberParameters{ + URL: &memberGraphURL, + } + + _, err := client.AddMember(ctx, groupID, properties) + if err != nil { + return err + } + + id := fmt.Sprintf("%s/%s", groupID, memberID) + d.SetId(id) + + return resourceGroupMemberRead(d, meta) +} + +func resourceGroupMemberRead(d *schema.ResourceData, meta interface{}) error { + client := meta.(*ArmClient).groupsClient + ctx := meta.(*ArmClient).StopContext + + id := strings.Split(d.Id(), "/") + if len(id) != 2 { + return fmt.Errorf("ID should be in the format {groupObjectId}/{memberObjectId} - but got %q", d.Id()) + } + + groupID := id[0] + memberID := id[1] + + members, err := client.GetGroupMembersComplete(ctx, groupID) + if err != nil { + return fmt.Errorf("Error retrieving Azure AD Group members (groupObjectId: %q): %+v", groupID, err) + } + + var memberObjectID string + for members.NotDone() { + // possible members are users, groups or service principals + // we try to 'cast' each result as the correspondig type and diff + // if we found the object we're looking for + user, _ := members.Value().AsUser() + if user != nil { + if *user.ObjectID == memberID { + memberObjectID = *user.ObjectID + // we successfully found the directory object we're looking for, we can stop looping + // through the results + break + } + } + + group, _ := members.Value().AsADGroup() + if group != nil { + if *group.ObjectID == memberID { + memberObjectID = *group.ObjectID + // we successfully found the directory object we're looking for, we can stop looping + // through the results + break + } + } + + servicePrincipal, _ := members.Value().AsServicePrincipal() + if servicePrincipal != nil { + if *servicePrincipal.ObjectID == memberID { + memberObjectID = *servicePrincipal.ObjectID + // we successfully found the directory object we're looking for, we can stop looping + // through the results + break + } + } + + err = members.NextWithContext(ctx) + if err != nil { + return fmt.Errorf("Error listing Azure AD Group Members: %s", err) + } + } + + if memberObjectID == "" { + log.Printf("[DEBUG] Azure AD Group Member was not found (groupObjectId:%q / memberObjectId:%q ) - removing from state!", groupID, memberID) + d.SetId("") + return fmt.Errorf("Azure AD Group Member not found - groupObjectId:%q / memberObjectId:%q", groupID, memberID) + } + + d.Set("group_object_id", groupID) + d.Set("member_object_id", memberObjectID) + + return nil +} + +func resourceGroupMemberDelete(d *schema.ResourceData, meta interface{}) error { + client := meta.(*ArmClient).groupsClient + ctx := meta.(*ArmClient).StopContext + + id := strings.Split(d.Id(), "/") + if len(id) != 2 { + return fmt.Errorf("ID should be in the format {groupObjectId}/{memberObjectId} - but got %q", d.Id()) + } + + groupID := id[0] + memberID := id[1] + + resp, err := client.RemoveMember(ctx, groupID, memberID) + + if err != nil { + if !ar.ResponseWasNotFound(resp) { + return fmt.Errorf("Error removing Member (memberObjectId: %q) from Azure AD Group (groupObjectId: %q): %+v", memberID, groupID, err) + } + } + + return nil +} From 61312b7005ce2565b51a03264f9a4afc7d406563 Mon Sep 17 00:00:00 2001 From: tiwood Date: Wed, 13 Mar 2019 18:01:44 +0100 Subject: [PATCH 02/25] Tests for 'azuread_group_member' --- azuread/resource_group_member_test.go | 224 ++++++++++++++++++++++++++ 1 file changed, 224 insertions(+) create mode 100644 azuread/resource_group_member_test.go diff --git a/azuread/resource_group_member_test.go b/azuread/resource_group_member_test.go new file mode 100644 index 0000000000..1cc095a3be --- /dev/null +++ b/azuread/resource_group_member_test.go @@ -0,0 +1,224 @@ +package azuread + +import ( + "fmt" + "testing" + + "github.com/hashicorp/terraform/helper/acctest" + "github.com/hashicorp/terraform/helper/resource" + "github.com/hashicorp/terraform/terraform" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/ar" +) + +func TestAccAzureADGroupMember_User(t *testing.T) { + resourceName := "azuread_group_member.test" + id := acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) + password := id + "p@$$wR2" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupMemberDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAzureADGroupMember_User(id, password), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet(resourceName, "group_object_id"), + resource.TestCheckResourceAttrSet(resourceName, "member_object_id"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccAzureADGroupMember_Group(t *testing.T) { + resourceName := "azuread_group_member.test" + id := acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupMemberDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAzureADGroupMember_Group(id), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet(resourceName, "group_object_id"), + resource.TestCheckResourceAttrSet(resourceName, "member_object_id"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccAzureADGroupMember_ServicePrincipal(t *testing.T) { + resourceName := "azuread_group_member.test" + id := acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupMemberDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAzureADGroupMember_ServicePrincipal(id), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet(resourceName, "group_object_id"), + resource.TestCheckResourceAttrSet(resourceName, "member_object_id"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func testCheckAzureADGroupMemberDestroy(s *terraform.State) error { + for _, rs := range s.RootModule().Resources { + if rs.Type != "azuread_group_member" { + continue + } + + client := testAccProvider.Meta().(*ArmClient).groupsClient + ctx := testAccProvider.Meta().(*ArmClient).StopContext + + groupID := rs.Primary.Attributes["group_object_id"] + memberID := rs.Primary.Attributes["member_object_id"] + + members, err := client.GetGroupMembersComplete(ctx, groupID) + if err != nil { + if ar.ResponseWasNotFound(members.Response().Response) { + return nil + } + + return err + } + + var memberObjectID string + for members.NotDone() { + // possible members are users, groups or service principals + // we try to 'cast' each result as the correspondig type and diff + // if we found the object we're looking for + user, _ := members.Value().AsUser() + if user != nil { + if *user.ObjectID == memberID { + memberObjectID = *user.ObjectID + // we successfully found the directory object we're looking for, we can stop looping + // through the results + break + } + } + + group, _ := members.Value().AsADGroup() + if group != nil { + if *group.ObjectID == memberID { + memberObjectID = *group.ObjectID + // we successfully found the directory object we're looking for, we can stop looping + // through the results + break + } + } + + servicePrincipal, _ := members.Value().AsServicePrincipal() + if servicePrincipal != nil { + if *servicePrincipal.ObjectID == memberID { + memberObjectID = *servicePrincipal.ObjectID + // we successfully found the directory object we're looking for, we can stop looping + // through the results + break + } + } + + err = members.NextWithContext(ctx) + if err != nil { + return fmt.Errorf("Error listing Azure AD Group Members: %s", err) + } + } + + if memberObjectID != "" { + return fmt.Errorf("Azure AD group member still exists:\n%#v", memberObjectID) + } + } + + return nil +} + +func testAccAzureADGroupMember_User(id string, password string) string { + return fmt.Sprintf(` + +data "azuread_domains" "tenant_domain" { + only_initial = true +} + +resource "azuread_user" "test" { + user_principal_name = "acctestA%[1]s@${data.azuread_domains.tenant_domain.domains.0.domain_name}" + display_name = "acctestA%[1]s" + password = "%[2]s" +} + +resource "azuread_group" "test" { + name = "acctest%[1]s" +} + +resource "azuread_group_member" "test" { + group_object_id = "${azuread_group.test.id}" + member_object_id = "${azuread_user.test.id}" +} + +`, id, password) +} + +func testAccAzureADGroupMember_Group(id string) string { + return fmt.Sprintf(` + +resource "azuread_group" "testA" { + name = "acctestA%[1]s" +} + +resource "azuread_group" "testB" { + name = "acctestB%[1]s" +} + +resource "azuread_group_member" "test" { + group_object_id = "${azuread_group.testA.id}" + member_object_id = "${azuread_group.testB.id}" +} + +`, id) +} + +func testAccAzureADGroupMember_ServicePrincipal(id string) string { + return fmt.Sprintf(` + +resource "azuread_application" "test" { + name = "acctest%[1]s" +} + +resource "azuread_service_principal" "test" { + application_id = "${azuread_application.test.application_id}" +} + +resource "azuread_group" "test" { + name = "acctestA%[1]s" +} + +resource "azuread_group_member" "test" { + group_object_id = "${azuread_group.test.id}" + member_object_id = "${azuread_service_principal.test.id}" +} + +`, id) +} From 80a03d3b98aea8074e237a410ad7b3322b3f8f9b Mon Sep 17 00:00:00 2001 From: tiwood Date: Wed, 13 Mar 2019 18:02:22 +0100 Subject: [PATCH 03/25] Documentation for new resource 'azuread_group_member' --- website/azuread.erb | 4 ++ website/docs/r/group_member.markdown | 55 ++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 website/docs/r/group_member.markdown diff --git a/website/azuread.erb b/website/azuread.erb index 0fc53de4d5..204f3b3c8d 100644 --- a/website/azuread.erb +++ b/website/azuread.erb @@ -89,6 +89,10 @@ azuread_group + > + azuread_group_member + + > azuread_service_principal diff --git a/website/docs/r/group_member.markdown b/website/docs/r/group_member.markdown new file mode 100644 index 0000000000..c5e1abd54b --- /dev/null +++ b/website/docs/r/group_member.markdown @@ -0,0 +1,55 @@ +--- +layout: "azuread" +page_title: "Azure Active Directory: azuread_group_member" +sidebar_current: "docs-azuread-resource-azuread-group-member" +description: |- + Manages a Group Membership within Azure Active Directory. + +--- + +# azuread_group_member + +Manages a Group Membership within Azure Active Directory. + +## Example Usage + +```hcl + +data "azuread_user" "my_user" { + user_principal_name = "johndoe@hashicorp.com" +} + +resource "azuread_group" "my_group" { + name = "my_group" +} + +resource "azuread_group_member" "test" { + group_object_id = "${azuread_group.my_group.id}" + member_object_id = "${data.azuread_user.my_user.id}" +} +``` + +## Argument Reference + +The following arguments are supported: + +* `group_object_id` - (Required) The Object ID of the Azure AD Group you want to add the Member to. +* `member_object_id` - (Required) The Object ID of the Azure AD Object you want to add as a Member to the Group. Supported Object types are Users, Groups or Service Principals. + +-> **NOTE:** The Member object has to be present in your Azure Active Directory, either as a Member or a Guest. + +## Attributes Reference + +The following attributes are exported: + +* `id` - The ID of the Azure AD Group Member. + +## Import + +Azure Active Directory Group Members can be imported using the `object id`, e.g. + +```shell +terraform import azuread_group_member.test 00000000-0000-0000-0000-000000000000/11111111-1111-1111-1111-111111111111 +``` + +-> **NOTE:** This ID format is unique to Terraform and is composed of the Azure AD Group Object ID and the target Member Object ID in the format `{GroupObjectID}/{MemberObjectID}`. From 68c6caee7780345746f4944831703930b6d57bf6 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Wed, 5 Jun 2019 09:37:58 +0200 Subject: [PATCH 04/25] resource_group_member: fix review comments --- azuread/resource_group_member.go | 11 +++-------- website/docs/r/group_member.markdown | 4 ++-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/azuread/resource_group_member.go b/azuread/resource_group_member.go index a3db16e079..0aef3e6210 100644 --- a/azuread/resource_group_member.go +++ b/azuread/resource_group_member.go @@ -2,7 +2,6 @@ package azuread import ( "fmt" - "log" "strings" "github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac" @@ -51,8 +50,7 @@ func resourceGroupMemberCreate(d *schema.ResourceData, meta interface{}) error { URL: &memberGraphURL, } - _, err := client.AddMember(ctx, groupID, properties) - if err != nil { + if _, err := client.AddMember(ctx, groupID, properties); err != nil { return err } @@ -82,7 +80,7 @@ func resourceGroupMemberRead(d *schema.ResourceData, meta interface{}) error { var memberObjectID string for members.NotDone() { // possible members are users, groups or service principals - // we try to 'cast' each result as the correspondig type and diff + // we try to 'cast' each result as the corresponding type and diff // if we found the object we're looking for user, _ := members.Value().AsUser() if user != nil { @@ -114,14 +112,12 @@ func resourceGroupMemberRead(d *schema.ResourceData, meta interface{}) error { } } - err = members.NextWithContext(ctx) - if err != nil { + if err = members.NextWithContext(ctx); err != nil { return fmt.Errorf("Error listing Azure AD Group Members: %s", err) } } if memberObjectID == "" { - log.Printf("[DEBUG] Azure AD Group Member was not found (groupObjectId:%q / memberObjectId:%q ) - removing from state!", groupID, memberID) d.SetId("") return fmt.Errorf("Azure AD Group Member not found - groupObjectId:%q / memberObjectId:%q", groupID, memberID) } @@ -145,7 +141,6 @@ func resourceGroupMemberDelete(d *schema.ResourceData, meta interface{}) error { memberID := id[1] resp, err := client.RemoveMember(ctx, groupID, memberID) - if err != nil { if !ar.ResponseWasNotFound(resp) { return fmt.Errorf("Error removing Member (memberObjectId: %q) from Azure AD Group (groupObjectId: %q): %+v", memberID, groupID, err) diff --git a/website/docs/r/group_member.markdown b/website/docs/r/group_member.markdown index c5e1abd54b..62206ee06d 100644 --- a/website/docs/r/group_member.markdown +++ b/website/docs/r/group_member.markdown @@ -33,8 +33,8 @@ resource "azuread_group_member" "test" { The following arguments are supported: -* `group_object_id` - (Required) The Object ID of the Azure AD Group you want to add the Member to. -* `member_object_id` - (Required) The Object ID of the Azure AD Object you want to add as a Member to the Group. Supported Object types are Users, Groups or Service Principals. +* `group_object_id` - (Required) The Object ID of the Azure AD Group you want to add the Member to. Changing this forces a new resource to be created. +* `member_object_id` - (Required) The Object ID of the Azure AD Object you want to add as a Member to the Group. Supported Object types are Users, Groups or Service Principals. Changing this forces a new resource to be created. -> **NOTE:** The Member object has to be present in your Azure Active Directory, either as a Member or a Guest. From b1a030c7eea41bed4fdae018b1dba5a0beb15605 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Wed, 5 Jun 2019 23:09:53 +0200 Subject: [PATCH 05/25] resource_group: add members --- azuread/helpers/slices/slices.go | 16 +++++ azuread/resource_group.go | 106 +++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 azuread/helpers/slices/slices.go diff --git a/azuread/helpers/slices/slices.go b/azuread/helpers/slices/slices.go new file mode 100644 index 0000000000..f0b7ca972c --- /dev/null +++ b/azuread/helpers/slices/slices.go @@ -0,0 +1,16 @@ +package slices + +// difference returns the elements in `a` that aren't in `b`. +func Difference(a, b []string) []string { + mb := make(map[string]struct{}, len(b)) + for _, x := range b { + mb[x] = struct{}{} + } + var diff []string + for _, x := range a { + if _, found := mb[x]; !found { + diff = append(diff, x) + } + } + return diff +} diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 66ff3d6794..5d7737f359 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -1,9 +1,14 @@ package azuread import ( + "context" "fmt" "log" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/slices" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/tf" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/validate" + "github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac" "github.com/google/uuid" "github.com/hashicorp/terraform/helper/schema" @@ -17,6 +22,7 @@ func resourceGroup() *schema.Resource { return &schema.Resource{ Create: resourceGroupCreate, Read: resourceGroupRead, + Update: resourceGroupUpdate, Delete: resourceGroupDelete, Importer: &schema.ResourceImporter{ @@ -35,6 +41,17 @@ func resourceGroup() *schema.Resource { Type: schema.TypeString, Computed: true, }, + + "members": { + Type: schema.TypeSet, + Optional: true, + Set: schema.HashString, + ForceNew: false, + Elem: &schema.Schema{ + Type: schema.TypeString, + ValidateFunc: validate.UUID, + }, + }, }, } } @@ -61,6 +78,17 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error { } d.SetId(*group.ObjectID) + // Add members if specified + if v, ok := d.GetOk("members"); ok { + members := tf.ExpandStringSlicePtr(v.(*schema.Set).List()) + + for _, memberUuid := range *members { + if err := addMember(*group.ObjectID, memberUuid, client, ctx); err != nil { + return err + } + } + } + _, err = graph.WaitForReplication(func() (interface{}, error) { return client.Get(ctx, *group.ObjectID) }) @@ -88,9 +116,50 @@ func resourceGroupRead(d *schema.ResourceData, meta interface{}) error { d.Set("name", resp.DisplayName) d.Set("object_id", resp.ObjectID) + + members, err := fetchAllMembers(d.Id(), client, ctx) + if err != nil { + return err + } + + d.Set("members", members) + return nil } +func resourceGroupUpdate(d *schema.ResourceData, meta interface{}) error { + client := meta.(*ArmClient).groupsClient + ctx := meta.(*ArmClient).StopContext + + if v, ok := d.GetOk("members"); ok && d.HasChange("members") { + existingMembers, err := fetchAllMembers(d.Id(), client, ctx) + if err != nil { + return err + } + + desiredMembers := *tf.ExpandStringSlicePtr(v.(*schema.Set).List()) + membersForRemoval := slices.Difference(existingMembers, desiredMembers) + membersToAdd := slices.Difference(desiredMembers, existingMembers) + + for _, existingMember := range membersForRemoval { + log.Printf("[DEBUG] Removing member with id %q from Azure AD group with id %q", existingMember, d.Id()) + if resp, err := client.RemoveMember(ctx, d.Id(), existingMember); err != nil { + if !ar.ResponseWasNotFound(resp) { + return fmt.Errorf("Error Deleting group member %q from Azure AD Group with ID %q: %+v", existingMember, d.Id(), err) + } + } + } + + for _, newMember := range membersToAdd { + if err := addMember(d.Id(), newMember, client, ctx); err != nil { + return err + } + } + } + + return resourceGroupRead(d, meta) +} + func resourceGroupDelete(d *schema.ResourceData, meta interface{}) error { client := meta.(*ArmClient).groupsClient ctx := meta.(*ArmClient).StopContext @@ -103,3 +172,40 @@ func resourceGroupDelete(d *schema.ResourceData, meta interface{}) error { return nil } + +func fetchAllMembers(groupId string, client graphrbac.GroupsClient, ctx context.Context) ([]string, error) { + it, err := client.GetGroupMembersComplete(ctx, groupId) + + if err != nil { + return nil, fmt.Errorf("Error listing existing group members from Azure AD Group with ID %q: %+v", groupId, err) + } + + existingMembers := make([]string, 0) + + for it.NotDone() { + currUser, _ := it.Value().AsUser() + existingMembers = append(existingMembers, *currUser.ObjectID) + if err := it.NextWithContext(ctx); err != nil { + return nil, fmt.Errorf("Error during pagination of group members from Azure AD Group with ID %q: %+v", groupId, err) + } + } + + log.Printf("[DEBUG] %d members in Azure AD group with ID: %q", len(existingMembers), groupId) + + return existingMembers, nil +} + +func addMember(groupId string, member string, client graphrbac.GroupsClient, ctx context.Context) error { + memberGraphURL := fmt.Sprintf("https://graph.windows.net/%s/directoryObjects/%s", client.TenantID, member) + + properties := graphrbac.GroupAddMemberParameters{ + URL: &memberGraphURL, + } + + log.Printf("[DEBUG] Adding member with id %q to Azure AD group with id %q", member, groupId) + if _, err := client.AddMember(ctx, groupId, properties); err != nil { + return err + } + + return nil +} From 192c04b946f47306cc66393c05c59457c7fe8fcb Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Thu, 6 Jun 2019 15:18:19 +0200 Subject: [PATCH 06/25] resource_group: documentation --- website/docs/r/group.markdown | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/website/docs/r/group.markdown b/website/docs/r/group.markdown index 6a55abaa87..19929b8bec 100644 --- a/website/docs/r/group.markdown +++ b/website/docs/r/group.markdown @@ -15,17 +15,35 @@ Manages a Group within Azure Active Directory. ## Example Usage +*Basic example* + ```hcl resource "azuread_group" "my_group" { name = "MyGroup" } ``` +*A group with members* + +```hcl +resource "azuread_user" "my_user" { + display_name = "John Doe" + password = "notSecure123" + user_principal_name = "john.doe@terraform.onmicrosoft.com" +} + +resource "azuread_group" "my_group" { + name = "MyGroup" + members = [ azuread_user.my_user.id /*, more users */ ] +} +``` + ## Argument Reference The following arguments are supported: -* `name` - (Required) The display name for the Group. +* `name` - (Required) The display name for the Group. Changing this forces a new resource to be created. +* `members` (Optional) A set of users who should be members of this Group. -> **NOTE:** Group names are not unique within Azure Active Directory. @@ -37,6 +55,8 @@ The following attributes are exported: * `name` - The Display Name of the Group. +* `members` - The Group Members in the Group. + ## Import Azure Active Directory Groups can be imported using the `object id`, e.g. From 6f0be2901417df094d8d2a930d9c7f02ba97011e Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Thu, 6 Jun 2019 15:35:40 +0200 Subject: [PATCH 07/25] resource_group: add test for members --- azuread/resource_group_test.go | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/azuread/resource_group_test.go b/azuread/resource_group_test.go index e210c2e666..734a370b05 100644 --- a/azuread/resource_group_test.go +++ b/azuread/resource_group_test.go @@ -2,6 +2,7 @@ package azuread import ( "fmt" + "strings" "testing" "github.com/hashicorp/go-uuid" @@ -29,6 +30,49 @@ func TestAccAzureADGroup_basic(t *testing.T) { testCheckAzureADGroupExists(resourceName), resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctest%s", id)), resource.TestCheckResourceAttrSet(resourceName, "object_id"), + resource.TestCheckNoResourceAttr(resourceName, "members"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccAzureADGroup_members(t *testing.T) { + resourceName := "azuread_group.test" + id, err := uuid.GenerateUUID() + if err != nil { + t.Fatal(err) + } + + members := make([]string, 0) + + for i := 0; i < 5; i++ { + memberUuid, err := uuid.GenerateUUID() + if err != nil { + t.Fatal(err) + } + members = append(members, "\""+memberUuid+"\"") + } + + config := testAccAzureADGroupWithMembers(id, members) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupDestroy, + Steps: []resource.TestStep{ + { + Config: config, + Check: resource.ComposeTestCheckFunc( + testCheckAzureADGroupExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctest%s", id)), + resource.TestCheckResourceAttrSet(resourceName, "object_id"), + resource.TestCheckResourceAttr(resourceName, "members", fmt.Sprintf("[ %s ]", strings.Join(members, ", "))), ), }, { @@ -123,3 +167,12 @@ resource "azuread_group" "test" { } `, id) } + +func testAccAzureADGroupWithMembers(id string, members []string) string { + return fmt.Sprintf(` +resource "azuread_group" "test" { + name = "acctest%s" + members = [ %s ] +} +`, id, strings.Join(members, ", ")) +} From b1dd10d446c20b867d4444943e253770d44e6209 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Thu, 6 Jun 2019 20:37:28 +0200 Subject: [PATCH 08/25] fix lint issues --- azuread/resource_group_member_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azuread/resource_group_member_test.go b/azuread/resource_group_member_test.go index 1cc095a3be..6cab41b829 100644 --- a/azuread/resource_group_member_test.go +++ b/azuread/resource_group_member_test.go @@ -110,7 +110,7 @@ func testCheckAzureADGroupMemberDestroy(s *terraform.State) error { var memberObjectID string for members.NotDone() { // possible members are users, groups or service principals - // we try to 'cast' each result as the correspondig type and diff + // we try to 'cast' each result as the corresponding type and diff // if we found the object we're looking for user, _ := members.Value().AsUser() if user != nil { From fd1b4b72bd83c9810991ed186e9c63ee2ef59c0b Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Thu, 13 Jun 2019 14:34:36 +0200 Subject: [PATCH 09/25] resource_group: extract fetchAllMembers into graph helper --- azuread/helpers/graph/group.go | 31 +++++++++++++++++++++++++++++++ azuread/resource_group.go | 26 ++------------------------ 2 files changed, 33 insertions(+), 24 deletions(-) create mode 100644 azuread/helpers/graph/group.go diff --git a/azuread/helpers/graph/group.go b/azuread/helpers/graph/group.go new file mode 100644 index 0000000000..2c4947e01b --- /dev/null +++ b/azuread/helpers/graph/group.go @@ -0,0 +1,31 @@ +package graph + +import ( + "context" + "fmt" + "log" + + "github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac" +) + +func GroupAllMembers(groupId string, client graphrbac.GroupsClient, ctx context.Context) ([]string, error) { + it, err := client.GetGroupMembersComplete(ctx, groupId) + + if err != nil { + return nil, fmt.Errorf("Error listing existing group members from Azure AD Group with ID %q: %+v", groupId, err) + } + + existingMembers := make([]string, 0) + + for it.NotDone() { + currUser, _ := it.Value().AsUser() + existingMembers = append(existingMembers, *currUser.ObjectID) + if err := it.NextWithContext(ctx); err != nil { + return nil, fmt.Errorf("Error during pagination of group members from Azure AD Group with ID %q: %+v", groupId, err) + } + } + + log.Printf("[DEBUG] %d members in Azure AD group with ID: %q", len(existingMembers), groupId) + + return existingMembers, nil +} diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 5d7737f359..8d77649c83 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -117,7 +117,7 @@ func resourceGroupRead(d *schema.ResourceData, meta interface{}) error { d.Set("name", resp.DisplayName) d.Set("object_id", resp.ObjectID) - members, err := fetchAllMembers(d.Id(), client, ctx) + members, err := graph.GroupAllMembers(d.Id(), client, ctx) if err != nil { return err } @@ -132,7 +132,7 @@ func resourceGroupUpdate(d *schema.ResourceData, meta interface{}) error { ctx := meta.(*ArmClient).StopContext if v, ok := d.GetOk("members"); ok && d.HasChange("members") { - existingMembers, err := fetchAllMembers(d.Id(), client, ctx) + existingMembers, err := graph.GroupAllMembers(d.Id(), client, ctx) if err != nil { return err } @@ -173,28 +173,6 @@ func resourceGroupDelete(d *schema.ResourceData, meta interface{}) error { return nil } -func fetchAllMembers(groupId string, client graphrbac.GroupsClient, ctx context.Context) ([]string, error) { - it, err := client.GetGroupMembersComplete(ctx, groupId) - - if err != nil { - return nil, fmt.Errorf("Error listing existing group members from Azure AD Group with ID %q: %+v", groupId, err) - } - - existingMembers := make([]string, 0) - - for it.NotDone() { - currUser, _ := it.Value().AsUser() - existingMembers = append(existingMembers, *currUser.ObjectID) - if err := it.NextWithContext(ctx); err != nil { - return nil, fmt.Errorf("Error during pagination of group members from Azure AD Group with ID %q: %+v", groupId, err) - } - } - - log.Printf("[DEBUG] %d members in Azure AD group with ID: %q", len(existingMembers), groupId) - - return existingMembers, nil -} - func addMember(groupId string, member string, client graphrbac.GroupsClient, ctx context.Context) error { memberGraphURL := fmt.Sprintf("https://graph.windows.net/%s/directoryObjects/%s", client.TenantID, member) From 5d32e56d41ae9d59b6bf51d86fe8cacfc6b7e672 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Thu, 13 Jun 2019 14:36:41 +0200 Subject: [PATCH 10/25] resource_group: extract addMember into graph helper --- azuread/helpers/graph/group.go | 15 +++++++++++++++ azuread/resource_group.go | 20 ++------------------ 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/azuread/helpers/graph/group.go b/azuread/helpers/graph/group.go index 2c4947e01b..bcd74f0691 100644 --- a/azuread/helpers/graph/group.go +++ b/azuread/helpers/graph/group.go @@ -29,3 +29,18 @@ func GroupAllMembers(groupId string, client graphrbac.GroupsClient, ctx context. return existingMembers, nil } + +func GroupAddMember(groupId string, member string, client graphrbac.GroupsClient, ctx context.Context) error { + memberGraphURL := fmt.Sprintf("https://graph.windows.net/%s/directoryObjects/%s", client.TenantID, member) + + properties := graphrbac.GroupAddMemberParameters{ + URL: &memberGraphURL, + } + + log.Printf("[DEBUG] Adding member with id %q to Azure AD group with id %q", member, groupId) + if _, err := client.AddMember(ctx, groupId, properties); err != nil { + return err + } + + return nil +} diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 8d77649c83..34f9f8ce88 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -1,7 +1,6 @@ package azuread import ( - "context" "fmt" "log" @@ -83,7 +82,7 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error { members := tf.ExpandStringSlicePtr(v.(*schema.Set).List()) for _, memberUuid := range *members { - if err := addMember(*group.ObjectID, memberUuid, client, ctx); err != nil { + if err := graph.GroupAddMember(*group.ObjectID, memberUuid, client, ctx); err != nil { return err } } @@ -151,7 +150,7 @@ func resourceGroupUpdate(d *schema.ResourceData, meta interface{}) error { } for _, newMember := range membersToAdd { - if err := addMember(d.Id(), newMember, client, ctx); err != nil { + if err := graph.GroupAddMember(d.Id(), newMember, client, ctx); err != nil { return err } } @@ -172,18 +171,3 @@ func resourceGroupDelete(d *schema.ResourceData, meta interface{}) error { return nil } - -func addMember(groupId string, member string, client graphrbac.GroupsClient, ctx context.Context) error { - memberGraphURL := fmt.Sprintf("https://graph.windows.net/%s/directoryObjects/%s", client.TenantID, member) - - properties := graphrbac.GroupAddMemberParameters{ - URL: &memberGraphURL, - } - - log.Printf("[DEBUG] Adding member with id %q to Azure AD group with id %q", member, groupId) - if _, err := client.AddMember(ctx, groupId, properties); err != nil { - return err - } - - return nil -} From 500b2a9d449878d19f5ea841c5b2a4bba31325e6 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Thu, 13 Jun 2019 15:25:42 +0200 Subject: [PATCH 11/25] resource_group: bugfix for empty set of members --- azuread/resource_group.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 34f9f8ce88..5ae131ecaf 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -130,7 +130,7 @@ func resourceGroupUpdate(d *schema.ResourceData, meta interface{}) error { client := meta.(*ArmClient).groupsClient ctx := meta.(*ArmClient).StopContext - if v, ok := d.GetOk("members"); ok && d.HasChange("members") { + if v, ok := d.GetOkExists("members"); ok && d.HasChange("members") { existingMembers, err := graph.GroupAllMembers(d.Id(), client, ctx) if err != nil { return err From 4d6e3f9f5add75f0eb719779f7422735b06f195c Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Mon, 17 Jun 2019 09:47:31 +0200 Subject: [PATCH 12/25] Misc fixes --- azuread/helpers/graph/group.go | 22 ++++++++++-- azuread/resource_group.go | 4 ++- azuread/resource_group_member.go | 52 ++++----------------------- azuread/resource_group_member_test.go | 12 +++---- azuread/resource_group_test.go | 2 +- website/docs/r/group.markdown | 6 ++-- website/docs/r/group_member.markdown | 4 +-- 7 files changed, 42 insertions(+), 60 deletions(-) diff --git a/azuread/helpers/graph/group.go b/azuread/helpers/graph/group.go index bcd74f0691..337dc65151 100644 --- a/azuread/helpers/graph/group.go +++ b/azuread/helpers/graph/group.go @@ -17,9 +17,27 @@ func GroupAllMembers(groupId string, client graphrbac.GroupsClient, ctx context. existingMembers := make([]string, 0) + var memberObjectID string for it.NotDone() { - currUser, _ := it.Value().AsUser() - existingMembers = append(existingMembers, *currUser.ObjectID) + // possible members are users, groups or service principals + // we try to 'cast' each result as the corresponding type and diff + // if we found the object we're looking for + user, _ := it.Value().AsUser() + if user != nil { + memberObjectID = *user.ObjectID + } + + group, _ := it.Value().AsADGroup() + if group != nil { + memberObjectID = *group.ObjectID + } + + servicePrincipal, _ := it.Value().AsServicePrincipal() + if servicePrincipal != nil { + memberObjectID = *servicePrincipal.ObjectID + } + + existingMembers = append(existingMembers, memberObjectID) if err := it.NextWithContext(ctx); err != nil { return nil, fmt.Errorf("Error during pagination of group members from Azure AD Group with ID %q: %+v", groupId, err) } diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 5ae131ecaf..745b306ab8 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -82,7 +82,9 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error { members := tf.ExpandStringSlicePtr(v.(*schema.Set).List()) for _, memberUuid := range *members { - if err := graph.GroupAddMember(*group.ObjectID, memberUuid, client, ctx); err != nil { + err := graph.GroupAddMember(*group.ObjectID, memberUuid, client, ctx) + + if err != nil { return err } } diff --git a/azuread/resource_group_member.go b/azuread/resource_group_member.go index 0aef3e6210..d0c289af13 100644 --- a/azuread/resource_group_member.go +++ b/azuread/resource_group_member.go @@ -4,7 +4,8 @@ import ( "fmt" "strings" - "github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/graph" + "github.com/hashicorp/terraform/helper/schema" "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/ar" "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/validate" @@ -42,15 +43,8 @@ func resourceGroupMemberCreate(d *schema.ResourceData, meta interface{}) error { groupID := d.Get("group_object_id").(string) memberID := d.Get("member_object_id").(string) - tenantID := client.TenantID - - memberGraphURL := fmt.Sprintf("https://graph.windows.net/%s/directoryObjects/%s", tenantID, memberID) - properties := graphrbac.GroupAddMemberParameters{ - URL: &memberGraphURL, - } - - if _, err := client.AddMember(ctx, groupID, properties); err != nil { + if err := graph.GroupAddMember(groupID, memberID, client, ctx); err != nil { return err } @@ -72,48 +66,16 @@ func resourceGroupMemberRead(d *schema.ResourceData, meta interface{}) error { groupID := id[0] memberID := id[1] - members, err := client.GetGroupMembersComplete(ctx, groupID) + members, err := graph.GroupAllMembers(groupID, client, ctx) if err != nil { return fmt.Errorf("Error retrieving Azure AD Group members (groupObjectId: %q): %+v", groupID, err) } var memberObjectID string - for members.NotDone() { - // possible members are users, groups or service principals - // we try to 'cast' each result as the corresponding type and diff - // if we found the object we're looking for - user, _ := members.Value().AsUser() - if user != nil { - if *user.ObjectID == memberID { - memberObjectID = *user.ObjectID - // we successfully found the directory object we're looking for, we can stop looping - // through the results - break - } - } - - group, _ := members.Value().AsADGroup() - if group != nil { - if *group.ObjectID == memberID { - memberObjectID = *group.ObjectID - // we successfully found the directory object we're looking for, we can stop looping - // through the results - break - } - } - - servicePrincipal, _ := members.Value().AsServicePrincipal() - if servicePrincipal != nil { - if *servicePrincipal.ObjectID == memberID { - memberObjectID = *servicePrincipal.ObjectID - // we successfully found the directory object we're looking for, we can stop looping - // through the results - break - } - } - if err = members.NextWithContext(ctx); err != nil { - return fmt.Errorf("Error listing Azure AD Group Members: %s", err) + for _, objectID := range members { + if objectID == memberID { + memberObjectID = objectID } } diff --git a/azuread/resource_group_member_test.go b/azuread/resource_group_member_test.go index 6cab41b829..8b8ac21fa6 100644 --- a/azuread/resource_group_member_test.go +++ b/azuread/resource_group_member_test.go @@ -174,8 +174,8 @@ resource "azuread_group" "test" { } resource "azuread_group_member" "test" { - group_object_id = "${azuread_group.test.id}" - member_object_id = "${azuread_user.test.id}" + group_object_id = "${azuread_group.test.object_id}" + member_object_id = "${azuread_user.test.object_id}" } `, id, password) @@ -193,8 +193,8 @@ resource "azuread_group" "testB" { } resource "azuread_group_member" "test" { - group_object_id = "${azuread_group.testA.id}" - member_object_id = "${azuread_group.testB.id}" + group_object_id = "${azuread_group.testA.object_id}" + member_object_id = "${azuread_group.testB.object_id}" } `, id) @@ -216,8 +216,8 @@ resource "azuread_group" "test" { } resource "azuread_group_member" "test" { - group_object_id = "${azuread_group.test.id}" - member_object_id = "${azuread_service_principal.test.id}" + group_object_id = "${azuread_group.test.object_id}" + member_object_id = "${azuread_service_principal.test.object_id}" } `, id) diff --git a/azuread/resource_group_test.go b/azuread/resource_group_test.go index 734a370b05..df196d19bb 100644 --- a/azuread/resource_group_test.go +++ b/azuread/resource_group_test.go @@ -171,7 +171,7 @@ resource "azuread_group" "test" { func testAccAzureADGroupWithMembers(id string, members []string) string { return fmt.Sprintf(` resource "azuread_group" "test" { - name = "acctest%s" + name = "acctest%s" members = [ %s ] } `, id, strings.Join(members, ", ")) diff --git a/website/docs/r/group.markdown b/website/docs/r/group.markdown index 19929b8bec..aca1381966 100644 --- a/website/docs/r/group.markdown +++ b/website/docs/r/group.markdown @@ -34,7 +34,7 @@ resource "azuread_user" "my_user" { resource "azuread_group" "my_group" { name = "MyGroup" - members = [ azuread_user.my_user.id /*, more users */ ] + members = [ azuread_user.my_user.object_id /*, more users */ ] } ``` @@ -43,7 +43,7 @@ resource "azuread_group" "my_group" { The following arguments are supported: * `name` - (Required) The display name for the Group. Changing this forces a new resource to be created. -* `members` (Optional) A set of users who should be members of this Group. +* `members` (Optional) A set of members who should be present in this Group. Supported Object types are Users, Groups or Service Principals. Do not use `azuread_group_member` at the same time as this argument. -> **NOTE:** Group names are not unique within Azure Active Directory. @@ -55,7 +55,7 @@ The following attributes are exported: * `name` - The Display Name of the Group. -* `members` - The Group Members in the Group. +* `members` - The Members of the Group. ## Import diff --git a/website/docs/r/group_member.markdown b/website/docs/r/group_member.markdown index 62206ee06d..3b54999d95 100644 --- a/website/docs/r/group_member.markdown +++ b/website/docs/r/group_member.markdown @@ -3,13 +3,13 @@ layout: "azuread" page_title: "Azure Active Directory: azuread_group_member" sidebar_current: "docs-azuread-resource-azuread-group-member" description: |- - Manages a Group Membership within Azure Active Directory. + Manages a single Group Membership within Azure Active Directory. --- # azuread_group_member -Manages a Group Membership within Azure Active Directory. +Manages a single Group Membership within Azure Active Directory. Do not use this resource at the same time as `azuread_group.members`. ## Example Usage From cc51f5038e66a1b85441e1d673a2661b1ece1ab4 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Wed, 19 Jun 2019 14:30:17 +0200 Subject: [PATCH 13/25] resource_group: fix broken acceptance tests --- azuread/resource_group_test.go | 44 +++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/azuread/resource_group_test.go b/azuread/resource_group_test.go index df196d19bb..83a7e55277 100644 --- a/azuread/resource_group_test.go +++ b/azuread/resource_group_test.go @@ -30,7 +30,7 @@ func TestAccAzureADGroup_basic(t *testing.T) { testCheckAzureADGroupExists(resourceName), resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctest%s", id)), resource.TestCheckResourceAttrSet(resourceName, "object_id"), - resource.TestCheckNoResourceAttr(resourceName, "members"), + resource.TestCheckResourceAttr(resourceName, "members.#", "0"), ), }, { @@ -44,6 +44,7 @@ func TestAccAzureADGroup_basic(t *testing.T) { func TestAccAzureADGroup_members(t *testing.T) { resourceName := "azuread_group.test" + id, err := uuid.GenerateUUID() if err != nil { t.Fatal(err) @@ -52,11 +53,8 @@ func TestAccAzureADGroup_members(t *testing.T) { members := make([]string, 0) for i := 0; i < 5; i++ { - memberUuid, err := uuid.GenerateUUID() - if err != nil { - t.Fatal(err) - } - members = append(members, "\""+memberUuid+"\"") + memberUuid, _ := uuid.GenerateUUID() + members = append(members, memberUuid) } config := testAccAzureADGroupWithMembers(id, members) @@ -72,7 +70,7 @@ func TestAccAzureADGroup_members(t *testing.T) { testCheckAzureADGroupExists(resourceName), resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctest%s", id)), resource.TestCheckResourceAttrSet(resourceName, "object_id"), - resource.TestCheckResourceAttr(resourceName, "members", fmt.Sprintf("[ %s ]", strings.Join(members, ", "))), + resource.TestCheckResourceAttr(resourceName, "members.#", "5"), ), }, { @@ -103,6 +101,7 @@ func TestAccAzureADGroup_complete(t *testing.T) { testCheckAzureADGroupExists(resourceName), resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctest%s", id)), resource.TestCheckResourceAttrSet(resourceName, "object_id"), + resource.TestCheckResourceAttr(resourceName, "members.#", "0"), ), }, { @@ -169,10 +168,37 @@ resource "azuread_group" "test" { } func testAccAzureADGroupWithMembers(id string, members []string) string { - return fmt.Sprintf(` + var sb strings.Builder + + sb.WriteString(` +data "azuread_domains" "tenant_domain" { + only_initial = true +}`) + + for _, member := range members { + fmt.Fprintf(&sb, ` +resource "azuread_user" "acctest_user_%[1]s" { + user_principal_name = "acctest.%[1]s@${data.azuread_domains.tenant_domain.domains.0.domain_name}" + display_name = "acctest-%[1]s" + password = "%[1]s" +} +`, member) + } + + fmt.Fprintf(&sb, ` resource "azuread_group" "test" { name = "acctest%s" members = [ %s ] } -`, id, strings.Join(members, ", ")) +`, id, strings.Join(formatMembersAsUser(members), ", ")) + + return sb.String() +} + +func formatMembersAsUser(members []string) []string { + vsm := make([]string, len(members)) + for i, v := range members { + vsm[i] = "azuread_user.acctest_user_" + v + ".id" + } + return vsm } From 953508dfbeba84911f3d318744df3de315ba9824 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Wed, 19 Jun 2019 15:31:32 +0200 Subject: [PATCH 14/25] resource_group: mark 'members' as computed This satisfies resource_group_member_test --- azuread/resource_group.go | 1 + 1 file changed, 1 insertion(+) diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 745b306ab8..5e68444070 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -44,6 +44,7 @@ func resourceGroup() *schema.Resource { "members": { Type: schema.TypeSet, Optional: true, + Computed: true, Set: schema.HashString, ForceNew: false, Elem: &schema.Schema{ From 61b953674b63ce0f488a249e4bac83667195ed1a Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Mon, 24 Jun 2019 12:02:31 +0200 Subject: [PATCH 15/25] resource_group: more acceptance tests --- azuread/resource_group_test.go | 172 +++++++++++++++++++++++++++++---- 1 file changed, 154 insertions(+), 18 deletions(-) diff --git a/azuread/resource_group_test.go b/azuread/resource_group_test.go index 83a7e55277..1e9dd82603 100644 --- a/azuread/resource_group_test.go +++ b/azuread/resource_group_test.go @@ -26,12 +26,7 @@ func TestAccAzureADGroup_basic(t *testing.T) { Steps: []resource.TestStep{ { Config: config, - Check: resource.ComposeTestCheckFunc( - testCheckAzureADGroupExists(resourceName), - resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctest%s", id)), - resource.TestCheckResourceAttrSet(resourceName, "object_id"), - resource.TestCheckResourceAttr(resourceName, "members.#", "0"), - ), + Check: assertResourceWithMemberCount(id, resourceName, "0"), }, { ResourceName: resourceName, @@ -66,12 +61,7 @@ func TestAccAzureADGroup_members(t *testing.T) { Steps: []resource.TestStep{ { Config: config, - Check: resource.ComposeTestCheckFunc( - testCheckAzureADGroupExists(resourceName), - resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctest%s", id)), - resource.TestCheckResourceAttrSet(resourceName, "object_id"), - resource.TestCheckResourceAttr(resourceName, "members.#", "5"), - ), + Check: assertResourceWithMemberCount(id, resourceName, "5"), }, { ResourceName: resourceName, @@ -97,12 +87,7 @@ func TestAccAzureADGroup_complete(t *testing.T) { Steps: []resource.TestStep{ { Config: config, - Check: resource.ComposeTestCheckFunc( - testCheckAzureADGroupExists(resourceName), - resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctest%s", id)), - resource.TestCheckResourceAttrSet(resourceName, "object_id"), - resource.TestCheckResourceAttr(resourceName, "members.#", "0"), - ), + Check: assertResourceWithMemberCount(id, resourceName, "0"), }, { ResourceName: resourceName, @@ -113,6 +98,93 @@ func TestAccAzureADGroup_complete(t *testing.T) { }) } +func TestAccAzureADGroup_diverse(t *testing.T) { + resourceName := "azuread_group.test" + id, err := uuid.GenerateUUID() + if err != nil { + t.Fatal(err) + } + config := testAccAzureADGroupWithDiverseMembers(id) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupDestroy, + Steps: []resource.TestStep{ + { + Config: config, + Check: assertResourceWithMemberCount(id, resourceName, "3"), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccAzureADGroup_progression(t *testing.T) { + resourceName := "azuread_group.test" + id, err := uuid.GenerateUUID() + if err != nil { + t.Fatal(err) + } + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupDestroy, + Steps: []resource.TestStep{ + // Empty group with 0 members + { + Config: groupWithMembers(id, ""), + Check: assertResourceWithMemberCount(id, resourceName, "0"), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + // Group with 1 member + { + Config: user(id) + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.id", id)), + Check: assertResourceWithMemberCount(id, resourceName, "1"), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + // Group with multiple members + { + Config: user(id+"a") + user(id+"b") + user(id+"c") + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.id, azuread_user.acctest_user_%[2]s.id, azuread_user.acctest_user_%[3]s.id", id+"a", id+"b", id+"c")), + Check: assertResourceWithMemberCount(id, resourceName, "3"), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + // Group with a different member + { + Config: servicePrincipal(id) + groupWithMembers(id, fmt.Sprintf("azuread_service_principal.test_sp_%[1]s.id", id)), + Check: assertResourceWithMemberCount(id, resourceName, "1"), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + // Empty group with 0 members + { + Config: groupWithMembers(id, ""), + Check: assertResourceWithMemberCount(id, resourceName, "0"), + }, + }, + }) +} + func testCheckAzureADGroupExists(name string) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[name] @@ -159,6 +231,15 @@ func testCheckAzureADGroupDestroy(s *terraform.State) error { return nil } +func assertResourceWithMemberCount(id string, resourceName string, memberCount string) resource.TestCheckFunc { + return resource.ComposeTestCheckFunc( + testCheckAzureADGroupExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctest%s", id)), + resource.TestCheckResourceAttrSet(resourceName, "object_id"), + resource.TestCheckResourceAttr(resourceName, "members.#", memberCount), + ) +} + func testAccAzureADGroup(id string) string { return fmt.Sprintf(` resource "azuread_group" "test" { @@ -167,6 +248,17 @@ resource "azuread_group" "test" { `, id) } +func testAccAzureADGroupWithDiverseMembers(id string) string { + var sb strings.Builder + + sb.WriteString(servicePrincipal(id)) + sb.WriteString(group(id)) + sb.WriteString(user(id)) + sb.WriteString(groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.id, azuread_group.test_g_%[1]s.id, azuread_service_principal.test_sp_%[1]s.id", id))) + + return sb.String() +} + func testAccAzureADGroupWithMembers(id string, members []string) string { var sb strings.Builder @@ -202,3 +294,47 @@ func formatMembersAsUser(members []string) []string { } return vsm } + +func servicePrincipal(id string) string { + return fmt.Sprintf(` +resource "azuread_application" "test_app_%[1]s" { + name = "app%[1]s" +} + +resource "azuread_service_principal" "test_sp_%[1]s" { + application_id = azuread_application.test_app_%[1]s.application_id +} + +`, id) +} + +func group(id string) string { + return fmt.Sprintf(` +resource "azuread_group" "test_g_%[1]s" { + name = "acctest%[1]s" +} +`, id) +} + +func groupWithMembers(id string, hclMemberString string) string { + return fmt.Sprintf(` +data "azuread_domains" "tenant_domain" { + only_initial = true +} + +resource "azuread_group" "test" { + name = "acctest%[1]s" + members = [ %[2]s ] +} +`, id, hclMemberString) +} + +func user(id string) string { + return fmt.Sprintf(` +resource "azuread_user" "acctest_user_%[1]s" { + user_principal_name = "acctest.%[1]s@${data.azuread_domains.tenant_domain.domains.0.domain_name}" + display_name = "acctest-%[1]s" + password = "%[1]s" +} +`, id) +} From b6fdf5f6fe616841a019a75a5150cee580f97132 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Mon, 24 Jun 2019 13:32:40 +0200 Subject: [PATCH 16/25] resource_group: fix linter warning in acceptance test --- azuread/resource_group_test.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/azuread/resource_group_test.go b/azuread/resource_group_test.go index 1e9dd82603..6be960bc65 100644 --- a/azuread/resource_group_test.go +++ b/azuread/resource_group_test.go @@ -26,7 +26,7 @@ func TestAccAzureADGroup_basic(t *testing.T) { Steps: []resource.TestStep{ { Config: config, - Check: assertResourceWithMemberCount(id, resourceName, "0"), + Check: assertResourceWithMemberCount(id, "0"), }, { ResourceName: resourceName, @@ -61,7 +61,7 @@ func TestAccAzureADGroup_members(t *testing.T) { Steps: []resource.TestStep{ { Config: config, - Check: assertResourceWithMemberCount(id, resourceName, "5"), + Check: assertResourceWithMemberCount(id, "5"), }, { ResourceName: resourceName, @@ -87,7 +87,7 @@ func TestAccAzureADGroup_complete(t *testing.T) { Steps: []resource.TestStep{ { Config: config, - Check: assertResourceWithMemberCount(id, resourceName, "0"), + Check: assertResourceWithMemberCount(id, "0"), }, { ResourceName: resourceName, @@ -113,7 +113,7 @@ func TestAccAzureADGroup_diverse(t *testing.T) { Steps: []resource.TestStep{ { Config: config, - Check: assertResourceWithMemberCount(id, resourceName, "3"), + Check: assertResourceWithMemberCount(id, "3"), }, { ResourceName: resourceName, @@ -139,7 +139,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { // Empty group with 0 members { Config: groupWithMembers(id, ""), - Check: assertResourceWithMemberCount(id, resourceName, "0"), + Check: assertResourceWithMemberCount(id, "0"), }, { ResourceName: resourceName, @@ -149,7 +149,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { // Group with 1 member { Config: user(id) + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.id", id)), - Check: assertResourceWithMemberCount(id, resourceName, "1"), + Check: assertResourceWithMemberCount(id, "1"), }, { ResourceName: resourceName, @@ -159,7 +159,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { // Group with multiple members { Config: user(id+"a") + user(id+"b") + user(id+"c") + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.id, azuread_user.acctest_user_%[2]s.id, azuread_user.acctest_user_%[3]s.id", id+"a", id+"b", id+"c")), - Check: assertResourceWithMemberCount(id, resourceName, "3"), + Check: assertResourceWithMemberCount(id, "3"), }, { ResourceName: resourceName, @@ -169,7 +169,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { // Group with a different member { Config: servicePrincipal(id) + groupWithMembers(id, fmt.Sprintf("azuread_service_principal.test_sp_%[1]s.id", id)), - Check: assertResourceWithMemberCount(id, resourceName, "1"), + Check: assertResourceWithMemberCount(id, "1"), }, { ResourceName: resourceName, @@ -179,7 +179,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { // Empty group with 0 members { Config: groupWithMembers(id, ""), - Check: assertResourceWithMemberCount(id, resourceName, "0"), + Check: assertResourceWithMemberCount(id, "0"), }, }, }) @@ -231,7 +231,9 @@ func testCheckAzureADGroupDestroy(s *terraform.State) error { return nil } -func assertResourceWithMemberCount(id string, resourceName string, memberCount string) resource.TestCheckFunc { +func assertResourceWithMemberCount(id string, memberCount string) resource.TestCheckFunc { + resourceName := "azuread_group.test" + return resource.ComposeTestCheckFunc( testCheckAzureADGroupExists(resourceName), resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctest%s", id)), From c86d8c6c2e27865359e90dd38dd42289f4cc7e19 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Mon, 24 Jun 2019 14:51:23 +0200 Subject: [PATCH 17/25] resource_group: id -> object_id in tests --- azuread/resource_group_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/azuread/resource_group_test.go b/azuread/resource_group_test.go index 6be960bc65..e39d7f223a 100644 --- a/azuread/resource_group_test.go +++ b/azuread/resource_group_test.go @@ -148,7 +148,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { }, // Group with 1 member { - Config: user(id) + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.id", id)), + Config: user(id) + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.object_id", id)), Check: assertResourceWithMemberCount(id, "1"), }, { @@ -158,7 +158,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { }, // Group with multiple members { - Config: user(id+"a") + user(id+"b") + user(id+"c") + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.id, azuread_user.acctest_user_%[2]s.id, azuread_user.acctest_user_%[3]s.id", id+"a", id+"b", id+"c")), + Config: user(id+"a") + user(id+"b") + user(id+"c") + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.object_id, azuread_user.acctest_user_%[2]s.object_id, azuread_user.acctest_user_%[3]s.object_id", id+"a", id+"b", id+"c")), Check: assertResourceWithMemberCount(id, "3"), }, { @@ -168,7 +168,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { }, // Group with a different member { - Config: servicePrincipal(id) + groupWithMembers(id, fmt.Sprintf("azuread_service_principal.test_sp_%[1]s.id", id)), + Config: servicePrincipal(id) + groupWithMembers(id, fmt.Sprintf("azuread_service_principal.test_sp_%[1]s.object_id", id)), Check: assertResourceWithMemberCount(id, "1"), }, { @@ -256,7 +256,7 @@ func testAccAzureADGroupWithDiverseMembers(id string) string { sb.WriteString(servicePrincipal(id)) sb.WriteString(group(id)) sb.WriteString(user(id)) - sb.WriteString(groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.id, azuread_group.test_g_%[1]s.id, azuread_service_principal.test_sp_%[1]s.id", id))) + sb.WriteString(groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.object_id, azuread_group.test_g_%[1]s.object_id, azuread_service_principal.test_sp_%[1]s.object_id", id))) return sb.String() } @@ -292,7 +292,7 @@ resource "azuread_group" "test" { func formatMembersAsUser(members []string) []string { vsm := make([]string, len(members)) for i, v := range members { - vsm[i] = "azuread_user.acctest_user_" + v + ".id" + vsm[i] = "azuread_user.acctest_user_" + v + ".object_id" } return vsm } From 6c097b1d4cf530e5d0d528b3c7e2a8b0801b6346 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Tue, 25 Jun 2019 08:47:06 +0200 Subject: [PATCH 18/25] graph/group: change method signatures --- azuread/helpers/graph/group.go | 4 ++-- azuread/resource_group.go | 8 ++++---- azuread/resource_group_member.go | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/azuread/helpers/graph/group.go b/azuread/helpers/graph/group.go index 337dc65151..d0dadb791d 100644 --- a/azuread/helpers/graph/group.go +++ b/azuread/helpers/graph/group.go @@ -8,7 +8,7 @@ import ( "github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac" ) -func GroupAllMembers(groupId string, client graphrbac.GroupsClient, ctx context.Context) ([]string, error) { +func GroupAllMembers(client graphrbac.GroupsClient, ctx context.Context, groupId string) ([]string, error) { it, err := client.GetGroupMembersComplete(ctx, groupId) if err != nil { @@ -48,7 +48,7 @@ func GroupAllMembers(groupId string, client graphrbac.GroupsClient, ctx context. return existingMembers, nil } -func GroupAddMember(groupId string, member string, client graphrbac.GroupsClient, ctx context.Context) error { +func GroupAddMember(client graphrbac.GroupsClient, ctx context.Context, groupId string, member string) error { memberGraphURL := fmt.Sprintf("https://graph.windows.net/%s/directoryObjects/%s", client.TenantID, member) properties := graphrbac.GroupAddMemberParameters{ diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 5e68444070..eaca920abb 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -83,7 +83,7 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error { members := tf.ExpandStringSlicePtr(v.(*schema.Set).List()) for _, memberUuid := range *members { - err := graph.GroupAddMember(*group.ObjectID, memberUuid, client, ctx) + err := graph.GroupAddMember(client, ctx, *group.ObjectID, memberUuid) if err != nil { return err @@ -119,7 +119,7 @@ func resourceGroupRead(d *schema.ResourceData, meta interface{}) error { d.Set("name", resp.DisplayName) d.Set("object_id", resp.ObjectID) - members, err := graph.GroupAllMembers(d.Id(), client, ctx) + members, err := graph.GroupAllMembers(client, ctx, d.Id()) if err != nil { return err } @@ -134,7 +134,7 @@ func resourceGroupUpdate(d *schema.ResourceData, meta interface{}) error { ctx := meta.(*ArmClient).StopContext if v, ok := d.GetOkExists("members"); ok && d.HasChange("members") { - existingMembers, err := graph.GroupAllMembers(d.Id(), client, ctx) + existingMembers, err := graph.GroupAllMembers(client, ctx, d.Id()) if err != nil { return err } @@ -153,7 +153,7 @@ func resourceGroupUpdate(d *schema.ResourceData, meta interface{}) error { } for _, newMember := range membersToAdd { - if err := graph.GroupAddMember(d.Id(), newMember, client, ctx); err != nil { + if err := graph.GroupAddMember(client, ctx, d.Id(), newMember); err != nil { return err } } diff --git a/azuread/resource_group_member.go b/azuread/resource_group_member.go index d0c289af13..0970cba372 100644 --- a/azuread/resource_group_member.go +++ b/azuread/resource_group_member.go @@ -44,7 +44,7 @@ func resourceGroupMemberCreate(d *schema.ResourceData, meta interface{}) error { groupID := d.Get("group_object_id").(string) memberID := d.Get("member_object_id").(string) - if err := graph.GroupAddMember(groupID, memberID, client, ctx); err != nil { + if err := graph.GroupAddMember(client, ctx, groupID, memberID); err != nil { return err } @@ -66,7 +66,7 @@ func resourceGroupMemberRead(d *schema.ResourceData, meta interface{}) error { groupID := id[0] memberID := id[1] - members, err := graph.GroupAllMembers(groupID, client, ctx) + members, err := graph.GroupAllMembers(client, ctx, groupID) if err != nil { return fmt.Errorf("Error retrieving Azure AD Group members (groupObjectId: %q): %+v", groupID, err) } From 646fa01f39f228fdfc1c1025ea9a8d8cbd04fe33 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Tue, 25 Jun 2019 08:57:31 +0200 Subject: [PATCH 19/25] graph/group: introduce GroupAddMembers --- azuread/helpers/graph/group.go | 12 ++++++++++++ azuread/resource_group.go | 8 ++------ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/azuread/helpers/graph/group.go b/azuread/helpers/graph/group.go index d0dadb791d..ff5215dd4c 100644 --- a/azuread/helpers/graph/group.go +++ b/azuread/helpers/graph/group.go @@ -62,3 +62,15 @@ func GroupAddMember(client graphrbac.GroupsClient, ctx context.Context, groupId return nil } + +func GroupAddMembers(client graphrbac.GroupsClient, ctx context.Context, groupId string, members []string) error { + for _, memberUuid := range members { + err := GroupAddMember(client, ctx, groupId, memberUuid) + + if err != nil { + return err + } + } + + return nil +} diff --git a/azuread/resource_group.go b/azuread/resource_group.go index eaca920abb..74c7860080 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -82,12 +82,8 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error { if v, ok := d.GetOk("members"); ok { members := tf.ExpandStringSlicePtr(v.(*schema.Set).List()) - for _, memberUuid := range *members { - err := graph.GroupAddMember(client, ctx, *group.ObjectID, memberUuid) - - if err != nil { - return err - } + if err := graph.GroupAddMembers(client, ctx, *group.ObjectID, *members); err != nil { + return err } } From bbdc1a56679e049dde9601291b755cc3861acdc3 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Tue, 25 Jun 2019 09:36:52 +0200 Subject: [PATCH 20/25] resource_group: use `member` as an ID separator --- azuread/resource_group_member.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/azuread/resource_group_member.go b/azuread/resource_group_member.go index 0970cba372..0afc87a2c9 100644 --- a/azuread/resource_group_member.go +++ b/azuread/resource_group_member.go @@ -48,7 +48,7 @@ func resourceGroupMemberCreate(d *schema.ResourceData, meta interface{}) error { return err } - id := fmt.Sprintf("%s/%s", groupID, memberID) + id := fmt.Sprintf("%s/member/%s", groupID, memberID) d.SetId(id) return resourceGroupMemberRead(d, meta) @@ -58,9 +58,9 @@ func resourceGroupMemberRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*ArmClient).groupsClient ctx := meta.(*ArmClient).StopContext - id := strings.Split(d.Id(), "/") + id := strings.Split(d.Id(), "/member/") if len(id) != 2 { - return fmt.Errorf("ID should be in the format {groupObjectId}/{memberObjectId} - but got %q", d.Id()) + return fmt.Errorf("ID should be in the format {groupObjectId}/member/{memberObjectId} - but got %q", d.Id()) } groupID := id[0] @@ -94,9 +94,9 @@ func resourceGroupMemberDelete(d *schema.ResourceData, meta interface{}) error { client := meta.(*ArmClient).groupsClient ctx := meta.(*ArmClient).StopContext - id := strings.Split(d.Id(), "/") + id := strings.Split(d.Id(), "/member/") if len(id) != 2 { - return fmt.Errorf("ID should be in the format {groupObjectId}/{memberObjectId} - but got %q", d.Id()) + return fmt.Errorf("ID should be in the format {groupObjectId}/member/{memberObjectId} - but got %q", d.Id()) } groupID := id[0] From 3c17afc5f4816d4cfd3103f7c24cc6159d132234 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Tue, 25 Jun 2019 10:28:16 +0200 Subject: [PATCH 21/25] update documentation --- website/docs/r/group.markdown | 4 +++- website/docs/r/group_member.markdown | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/website/docs/r/group.markdown b/website/docs/r/group.markdown index aca1381966..f178629c93 100644 --- a/website/docs/r/group.markdown +++ b/website/docs/r/group.markdown @@ -43,10 +43,12 @@ resource "azuread_group" "my_group" { The following arguments are supported: * `name` - (Required) The display name for the Group. Changing this forces a new resource to be created. -* `members` (Optional) A set of members who should be present in this Group. Supported Object types are Users, Groups or Service Principals. Do not use `azuread_group_member` at the same time as this argument. +* `members` (Optional) A set of members who should be present in this Group. Supported Object types are Users, Groups or Service Principals. -> **NOTE:** Group names are not unique within Azure Active Directory. +-> **NOTE:** Do not use `azuread_group_member` at the same time as the `members` argument. + ## Attributes Reference The following attributes are exported: diff --git a/website/docs/r/group_member.markdown b/website/docs/r/group_member.markdown index 3b54999d95..48ddd11f98 100644 --- a/website/docs/r/group_member.markdown +++ b/website/docs/r/group_member.markdown @@ -9,7 +9,9 @@ description: |- # azuread_group_member -Manages a single Group Membership within Azure Active Directory. Do not use this resource at the same time as `azuread_group.members`. +Manages a single Group Membership within Azure Active Directory. + +-> **NOTE:** Do not use this resource at the same time as `azuread_group.members`. ## Example Usage From 702e8696a3f05380d234d2aff1c6ab574ac1ae4b Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Tue, 25 Jun 2019 10:43:44 +0200 Subject: [PATCH 22/25] resource_group: test review comments --- azuread/resource_group_test.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/azuread/resource_group_test.go b/azuread/resource_group_test.go index e39d7f223a..b5ac1ac059 100644 --- a/azuread/resource_group_test.go +++ b/azuread/resource_group_test.go @@ -148,7 +148,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { }, // Group with 1 member { - Config: user(id) + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.object_id", id)), + Config: groupWithOneMember(id), Check: assertResourceWithMemberCount(id, "1"), }, { @@ -158,7 +158,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { }, // Group with multiple members { - Config: user(id+"a") + user(id+"b") + user(id+"c") + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.object_id, azuread_user.acctest_user_%[2]s.object_id, azuread_user.acctest_user_%[3]s.object_id", id+"a", id+"b", id+"c")), + Config: groupWithThreeMembers(id), Check: assertResourceWithMemberCount(id, "3"), }, { @@ -168,7 +168,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { }, // Group with a different member { - Config: servicePrincipal(id) + groupWithMembers(id, fmt.Sprintf("azuread_service_principal.test_sp_%[1]s.object_id", id)), + Config: groupWithServicePrincipal(id), Check: assertResourceWithMemberCount(id, "1"), }, { @@ -340,3 +340,15 @@ resource "azuread_user" "acctest_user_%[1]s" { } `, id) } + +func groupWithOneMember(id string) string { + return user(id) + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.object_id", id)) +} + +func groupWithThreeMembers(id string) string { + return user(id+"a") + user(id+"b") + user(id+"c") + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.object_id, azuread_user.acctest_user_%[2]s.object_id, azuread_user.acctest_user_%[3]s.object_id", id+"a", id+"b", id+"c")) +} + +func groupWithServicePrincipal(id string) string { + return servicePrincipal(id) + groupWithMembers(id, fmt.Sprintf("azuread_service_principal.test_sp_%[1]s.object_id", id)) +} From 28ac563a0cac183667441f93934f032920afc4c0 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Wed, 26 Jun 2019 09:56:55 +0200 Subject: [PATCH 23/25] resource_group: revert to plain HCL for tests --- azuread/resource_group_test.go | 136 +++++++++++++++------------------ 1 file changed, 62 insertions(+), 74 deletions(-) diff --git a/azuread/resource_group_test.go b/azuread/resource_group_test.go index b5ac1ac059..baf0ebfd1a 100644 --- a/azuread/resource_group_test.go +++ b/azuread/resource_group_test.go @@ -2,7 +2,6 @@ package azuread import ( "fmt" - "strings" "testing" "github.com/hashicorp/go-uuid" @@ -45,14 +44,7 @@ func TestAccAzureADGroup_members(t *testing.T) { t.Fatal(err) } - members := make([]string, 0) - - for i := 0; i < 5; i++ { - memberUuid, _ := uuid.GenerateUUID() - members = append(members, memberUuid) - } - - config := testAccAzureADGroupWithMembers(id, members) + config := testAccAzureADGroupWithThreeMembers(id) resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -61,7 +53,7 @@ func TestAccAzureADGroup_members(t *testing.T) { Steps: []resource.TestStep{ { Config: config, - Check: assertResourceWithMemberCount(id, "5"), + Check: assertResourceWithMemberCount(id, "3"), }, { ResourceName: resourceName, @@ -138,7 +130,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { Steps: []resource.TestStep{ // Empty group with 0 members { - Config: groupWithMembers(id, ""), + Config: testAccAzureADGroup(id), Check: assertResourceWithMemberCount(id, "0"), }, { @@ -148,7 +140,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { }, // Group with 1 member { - Config: groupWithOneMember(id), + Config: testAccAzureADGroupWithOneMember(id), Check: assertResourceWithMemberCount(id, "1"), }, { @@ -158,7 +150,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { }, // Group with multiple members { - Config: groupWithThreeMembers(id), + Config: testAccAzureADGroupWithThreeMembers(id), Check: assertResourceWithMemberCount(id, "3"), }, { @@ -168,7 +160,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { }, // Group with a different member { - Config: groupWithServicePrincipal(id), + Config: testAccAzureADGroupWithServicePrincipal(id), Check: assertResourceWithMemberCount(id, "1"), }, { @@ -178,7 +170,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { }, // Empty group with 0 members { - Config: groupWithMembers(id, ""), + Config: testAccAzureADGroup(id), Check: assertResourceWithMemberCount(id, "0"), }, }, @@ -246,109 +238,105 @@ func testAccAzureADGroup(id string) string { return fmt.Sprintf(` resource "azuread_group" "test" { name = "acctest%s" + members = [] } `, id) } func testAccAzureADGroupWithDiverseMembers(id string) string { - var sb strings.Builder - - sb.WriteString(servicePrincipal(id)) - sb.WriteString(group(id)) - sb.WriteString(user(id)) - sb.WriteString(groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.object_id, azuread_group.test_g_%[1]s.object_id, azuread_service_principal.test_sp_%[1]s.object_id", id))) + return fmt.Sprintf(` +data "azuread_domains" "tenant_domain" { + only_initial = true +} - return sb.String() +resource "azuread_application" "test_app_%[1]s" { + name = "app%[1]s" } -func testAccAzureADGroupWithMembers(id string, members []string) string { - var sb strings.Builder +resource "azuread_service_principal" "test_sp_%[1]s" { + application_id = azuread_application.test_app_%[1]s.application_id +} - sb.WriteString(` -data "azuread_domains" "tenant_domain" { - only_initial = true -}`) +resource "azuread_group" "test_g_%[1]s" { + name = "acctest%[1]s" +} - for _, member := range members { - fmt.Fprintf(&sb, ` resource "azuread_user" "acctest_user_%[1]s" { user_principal_name = "acctest.%[1]s@${data.azuread_domains.tenant_domain.domains.0.domain_name}" display_name = "acctest-%[1]s" password = "%[1]s" } -`, member) - } - fmt.Fprintf(&sb, ` resource "azuread_group" "test" { - name = "acctest%s" - members = [ %s ] -} -`, id, strings.Join(formatMembersAsUser(members), ", ")) - - return sb.String() + name = "acctest%[1]s" + members = [ azuread_user.acctest_user_%[1]s.object_id, azuread_group.test_g_%[1]s.object_id, azuread_service_principal.test_sp_%[1]s.object_id ] } - -func formatMembersAsUser(members []string) []string { - vsm := make([]string, len(members)) - for i, v := range members { - vsm[i] = "azuread_user.acctest_user_" + v + ".object_id" - } - return vsm +`, id) } -func servicePrincipal(id string) string { +func testAccAzureADGroupWithOneMember(id string) string { return fmt.Sprintf(` -resource "azuread_application" "test_app_%[1]s" { - name = "app%[1]s" -} - -resource "azuread_service_principal" "test_sp_%[1]s" { - application_id = azuread_application.test_app_%[1]s.application_id +data "azuread_domains" "tenant_domain" { + only_initial = true } -`, id) +resource "azuread_user" "acctest_user_%[1]s" { + user_principal_name = "acctest.%[1]s@${data.azuread_domains.tenant_domain.domains.0.domain_name}" + display_name = "acctest-%[1]s" + password = "%[1]s" } -func group(id string) string { - return fmt.Sprintf(` -resource "azuread_group" "test_g_%[1]s" { +resource "azuread_group" "test" { name = "acctest%[1]s" + members = [ azuread_user.acctest_user_%[1]s.object_id ] } `, id) } -func groupWithMembers(id string, hclMemberString string) string { +func testAccAzureADGroupWithThreeMembers(id string) string { return fmt.Sprintf(` data "azuread_domains" "tenant_domain" { only_initial = true } +resource "azuread_user" "acctest_user_%[2]s" { + user_principal_name = "acctest.%[2]s@${data.azuread_domains.tenant_domain.domains.0.domain_name}" + display_name = "acctest-%[2]s" + password = "%[2]s" +} + +resource "azuread_user" "acctest_user_%[3]s" { + user_principal_name = "acctest.%[3]s@${data.azuread_domains.tenant_domain.domains.0.domain_name}" + display_name = "acctest-%[3]s" + password = "%[3]s" +} + +resource "azuread_user" "acctest_user_%[4]s" { + user_principal_name = "acctest.%[4]s@${data.azuread_domains.tenant_domain.domains.0.domain_name}" + display_name = "acctest-%[4]s" + password = "%[4]s" +} + resource "azuread_group" "test" { name = "acctest%[1]s" - members = [ %[2]s ] + members = [ azuread_user.acctest_user_%[2]s.object_id, azuread_user.acctest_user_%[3]s.object_id, azuread_user.acctest_user_%[4]s.object_id ] } -`, id, hclMemberString) +`, id, id+"a", id+"b", id+"c") } -func user(id string) string { +func testAccAzureADGroupWithServicePrincipal(id string) string { return fmt.Sprintf(` -resource "azuread_user" "acctest_user_%[1]s" { - user_principal_name = "acctest.%[1]s@${data.azuread_domains.tenant_domain.domains.0.domain_name}" - display_name = "acctest-%[1]s" - password = "%[1]s" -} -`, id) +resource "azuread_application" "test_app_%[1]s" { + name = "app%[1]s" } -func groupWithOneMember(id string) string { - return user(id) + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.object_id", id)) +resource "azuread_service_principal" "test_sp_%[1]s" { + application_id = azuread_application.test_app_%[1]s.application_id } -func groupWithThreeMembers(id string) string { - return user(id+"a") + user(id+"b") + user(id+"c") + groupWithMembers(id, fmt.Sprintf("azuread_user.acctest_user_%[1]s.object_id, azuread_user.acctest_user_%[2]s.object_id, azuread_user.acctest_user_%[3]s.object_id", id+"a", id+"b", id+"c")) +resource "azuread_group" "test" { + name = "acctest%[1]s" + members = [ azuread_service_principal.test_sp_%[1]s.object_id ] } - -func groupWithServicePrincipal(id string) string { - return servicePrincipal(id) + groupWithMembers(id, fmt.Sprintf("azuread_service_principal.test_sp_%[1]s.object_id", id)) +`, id) } From aeba0ad26683f9e76236cc195adb97ad4339bfed Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Fri, 28 Jun 2019 09:06:51 +0200 Subject: [PATCH 24/25] resource_group: Use GroupAddMembers helper --- azuread/resource_group.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 74c7860080..ba170ddc91 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -148,10 +148,8 @@ func resourceGroupUpdate(d *schema.ResourceData, meta interface{}) error { } } - for _, newMember := range membersToAdd { - if err := graph.GroupAddMember(client, ctx, d.Id(), newMember); err != nil { - return err - } + if err := graph.GroupAddMembers(client, ctx, d.Id(), membersToAdd); err != nil { + return err } } From dbde7fb10ff140f6116fcd96bd3e271646955638 Mon Sep 17 00:00:00 2001 From: Even Holthe Date: Fri, 28 Jun 2019 09:07:12 +0200 Subject: [PATCH 25/25] graph/group: wrap errors --- azuread/helpers/graph/group.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/azuread/helpers/graph/group.go b/azuread/helpers/graph/group.go index ff5215dd4c..68308e4159 100644 --- a/azuread/helpers/graph/group.go +++ b/azuread/helpers/graph/group.go @@ -57,7 +57,7 @@ func GroupAddMember(client graphrbac.GroupsClient, ctx context.Context, groupId log.Printf("[DEBUG] Adding member with id %q to Azure AD group with id %q", member, groupId) if _, err := client.AddMember(ctx, groupId, properties); err != nil { - return err + return fmt.Errorf("Error adding group member %q to Azure AD Group with ID %q: %+v", member, groupId, err) } return nil @@ -68,7 +68,7 @@ func GroupAddMembers(client graphrbac.GroupsClient, ctx context.Context, groupId err := GroupAddMember(client, ctx, groupId, memberUuid) if err != nil { - return err + return fmt.Errorf("Error while adding members to Azure AD Group with ID %q: %+v", groupId, err) } }