From dd760d0a0b92de60f2e7423632ba74e27afa692b Mon Sep 17 00:00:00 2001 From: kt Date: Thu, 18 Jul 2019 19:35:54 -0700 Subject: [PATCH 1/6] added owners to azuread_group resource and datasource --- azuread/data_group.go | 25 +++++++ azuread/data_group_test.go | 36 ++++++++++ azuread/data_users_test.go | 4 +- azuread/helpers/graph/group.go | 89 +++++++++++++++++++----- azuread/resource_group.go | 52 +++++++++++++- azuread/resource_group_member.go | 1 + azuread/resource_group_test.go | 116 ++++++++++++++++++++++--------- azuread/resource_user_test.go | 24 ++++--- 8 files changed, 282 insertions(+), 65 deletions(-) diff --git a/azuread/data_group.go b/azuread/data_group.go index 5e3ccdccb5..64c12ae94a 100644 --- a/azuread/data_group.go +++ b/azuread/data_group.go @@ -34,6 +34,18 @@ func dataGroup() *schema.Resource { ValidateFunc: validate.NoEmptyStrings, ConflictsWith: []string{"object_id"}, }, + + "members": { + Type: schema.TypeList, + Computed: true, + Elem: &schema.Schema{Type: schema.TypeString}, + }, + + "owners": { + Type: schema.TypeList, + Computed: true, + Elem: &schema.Schema{Type: schema.TypeString}, + }, }, } } @@ -73,5 +85,18 @@ func dataSourceActiveDirectoryGroupRead(d *schema.ResourceData, meta interface{} d.Set("object_id", group.ObjectID) d.Set("name", group.DisplayName) + + members, err := graph.GroupAllMembers(client, ctx, d.Id()) + if err != nil { + return err + } + d.Set("members", members) + + owners, err := graph.GroupAllOwners(client, ctx, d.Id()) + if err != nil { + return err + } + d.Set("members", owners) + return nil } diff --git a/azuread/data_group_test.go b/azuread/data_group_test.go index 3953ab0b80..1cbc80d160 100644 --- a/azuread/data_group_test.go +++ b/azuread/data_group_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/tf" ) @@ -54,6 +55,31 @@ func TestAccDataSourceAzureADGroup_byObjectId(t *testing.T) { }) } +func TestAccDataSourceAzureADGroup_members(t *testing.T) { + dsn := "data.azuread_group.test" + id := tf.AccRandTimeInt() + pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupDestroy, + Steps: []resource.TestStep{ + { + Config: testAccDataSourceAzureADGroup_members(id, pw), + }, + { + Config: testAccDataSourceAzureADGroup_objectId(id), + Check: resource.ComposeTestCheckFunc( + testCheckAzureADGroupExists(dsn), + resource.TestCheckResourceAttr(dsn, "name", fmt.Sprintf("acctestGroup-%d", id)), + resource.TestCheckResourceAttr(dsn, "members.#", "3"), + ), + }, + }, + }) +} + func testAccDataSourceAzureADGroup_name(id int) string { return fmt.Sprintf(` %s @@ -73,3 +99,13 @@ data "azuread_group" "test" { } `, testAccAzureADGroup_basic(id)) } + +func testAccDataSourceAzureADGroup_members(id int, password string) string { + return fmt.Sprintf(` +%s + +data "azuread_group" "test" { + object_id = "${azuread_group.test.object_id}" +} +`, testAccAzureADGroupWithThreeMembers(id, password)) +} diff --git a/azuread/data_users_test.go b/azuread/data_users_test.go index b24f12c71d..13ba9b7d3c 100644 --- a/azuread/data_users_test.go +++ b/azuread/data_users_test.go @@ -57,7 +57,7 @@ func testAccAzureADUsersDataSource_byUserPrincipalNames(id int, password string) data "azuread_users" "test" { user_principal_names = ["${azuread_user.testA.user_principal_name}", "${azuread_user.testB.user_principal_name}"] } -`, testAccADUser_multiple(id, password)) +`, testAccADUser_threeUsersABC(id, password)) } func testAccAzureADUsersDataSource_byObjectIds(id int, password string) string { @@ -67,5 +67,5 @@ func testAccAzureADUsersDataSource_byObjectIds(id int, password string) string { data "azuread_users" "test" { object_ids = ["${azuread_user.testA.object_id}", "${azuread_user.testB.object_id}"] } -`, testAccADUser_multiple(id, password)) +`, testAccADUser_threeUsersABC(id, password)) } diff --git a/azuread/helpers/graph/group.go b/azuread/helpers/graph/group.go index 222373f903..b24eb49fbf 100644 --- a/azuread/helpers/graph/group.go +++ b/azuread/helpers/graph/group.go @@ -39,41 +39,49 @@ func GroupGetByDisplayName(client *graphrbac.GroupsClient, ctx context.Context, return &group, nil } -func GroupAllMembers(client graphrbac.GroupsClient, ctx context.Context, groupId string) ([]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) +func DirectoryObjectListToIDs(objects graphrbac.DirectoryObjectListResultIterator, ctx context.Context) ([]string, error) { + ids := make([]string, 0) + for objects.NotDone() { + v := objects.Value() - var memberObjectID string - for it.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, _ := it.Value().AsUser() + user, _ := v.AsUser() if user != nil { - memberObjectID = *user.ObjectID + ids = append(ids, *user.ObjectID) } - group, _ := it.Value().AsADGroup() + group, _ := v.AsADGroup() if group != nil { - memberObjectID = *group.ObjectID + ids = append(ids, *group.ObjectID) } - servicePrincipal, _ := it.Value().AsServicePrincipal() + servicePrincipal, _ := v.AsServicePrincipal() if servicePrincipal != nil { - memberObjectID = *servicePrincipal.ObjectID + ids = append(ids, *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) + if err := objects.NextWithContext(ctx); err != nil { + return nil, fmt.Errorf("Error during pagination of directory objects: %+v", err) } } + return ids, nil +} + +func GroupAllMembers(client graphrbac.GroupsClient, ctx context.Context, groupId string) ([]string, error) { + members, 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, err := DirectoryObjectListToIDs(members, ctx) + if err != nil { + return nil, fmt.Errorf("Error getting objects IDs of group members for 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 @@ -105,3 +113,46 @@ func GroupAddMembers(client graphrbac.GroupsClient, ctx context.Context, groupId return nil } + +func GroupAllOwners(client graphrbac.GroupsClient, ctx context.Context, groupId string) ([]string, error) { + owners, err := client.ListOwnersComplete(ctx, groupId) + + if err != nil { + return nil, fmt.Errorf("Error listing existing group owners from Azure AD Group with ID %q: %+v", groupId, err) + } + + existingMembers, err := DirectoryObjectListToIDs(owners, ctx) + if err != nil { + return nil, fmt.Errorf("Error getting objects IDs of group owners for 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 GroupAddOwner(client graphrbac.GroupsClient, ctx context.Context, groupId string, owner string) error { + ownerGraphURL := fmt.Sprintf("https://graph.windows.net/%s/directoryObjects/%s", client.TenantID, owner) + + properties := graphrbac.AddOwnerParameters{ + URL: &ownerGraphURL, + } + + log.Printf("[DEBUG] Adding owner with id %q to Azure AD group with id %q", owner, groupId) + if _, err := client.AddOwner(ctx, groupId, properties); err != nil { + return fmt.Errorf("Error adding group owner %q to Azure AD Group with ID %q: %+v", owner, groupId, err) + } + + return nil +} + +func GroupAddOwners(client graphrbac.GroupsClient, ctx context.Context, groupId string, owner []string) error { + for _, ownerUuid := range owner { + err := GroupAddOwner(client, ctx, groupId, ownerUuid) + + if err != nil { + return fmt.Errorf("Error while adding owners to Azure AD Group with ID %q: %+v", groupId, err) + } + } + + return nil +} diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 980d355dd1..336e6557f0 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -47,7 +47,17 @@ func resourceGroup() *schema.Resource { Optional: true, Computed: true, Set: schema.HashString, - ForceNew: false, + Elem: &schema.Schema{ + Type: schema.TypeString, + ValidateFunc: validate.UUID, + }, + }, + + "owners": { + Type: schema.TypeSet, + Optional: true, + Computed: true, + Set: schema.HashString, Elem: &schema.Schema{ Type: schema.TypeString, ValidateFunc: validate.UUID, @@ -88,6 +98,15 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error { } } + // Add owners if specified + if v, ok := d.GetOk("owners"); ok { + members := tf.ExpandStringSlicePtr(v.(*schema.Set).List()) + + if err := graph.GroupAddOwners(client, ctx, *group.ObjectID, *members); err != nil { + return err + } + } + _, err = graph.WaitForReplication(func() (interface{}, error) { return client.Get(ctx, *group.ObjectID) }) @@ -120,9 +139,14 @@ func resourceGroupRead(d *schema.ResourceData, meta interface{}) error { if err != nil { return err } - d.Set("members", members) + owners, err := graph.GroupAllOwners(client, ctx, d.Id()) + if err != nil { + return err + } + d.Set("owners", owners) + return nil } @@ -154,6 +178,30 @@ func resourceGroupUpdate(d *schema.ResourceData, meta interface{}) error { } } + if v, ok := d.GetOkExists("owners"); ok && d.HasChange("owners") { + existingOwners, err := graph.GroupAllOwners(client, ctx, d.Id()) + if err != nil { + return err + } + + desiredOwners := *tf.ExpandStringSlicePtr(v.(*schema.Set).List()) + ownersForRemoval := slices.Difference(existingOwners, desiredOwners) + ownersToAdd := slices.Difference(desiredOwners, existingOwners) + + for _, ownerToDelete := range ownersForRemoval { + log.Printf("[DEBUG] Removing member with id %q from Azure AD group with id %q", ownerToDelete, d.Id()) + if resp, err := client.RemoveOwner(ctx, d.Id(), ownerToDelete); err != nil { + if !ar.ResponseWasNotFound(resp) { + return fmt.Errorf("Error Deleting group member %q from Azure AD Group with ID %q: %+v", ownerToDelete, d.Id(), err) + } + } + } + + if err := graph.GroupAddOwners(client, ctx, d.Id(), ownersToAdd); err != nil { + return err + } + } + return resourceGroupRead(d, meta) } diff --git a/azuread/resource_group_member.go b/azuread/resource_group_member.go index 0afc87a2c9..1b83eb16ca 100644 --- a/azuread/resource_group_member.go +++ b/azuread/resource_group_member.go @@ -27,6 +27,7 @@ func resourceGroupMember() *schema.Resource { ForceNew: true, ValidateFunc: validate.UUID, }, + "member_object_id": { Type: schema.TypeString, Required: true, diff --git a/azuread/resource_group_test.go b/azuread/resource_group_test.go index 34a121b723..c1370befbc 100644 --- a/azuread/resource_group_test.go +++ b/azuread/resource_group_test.go @@ -23,7 +23,52 @@ func TestAccAzureADGroup_basic(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccAzureADGroup_basic(id), - Check: testCheckAzureAdGroupBasic(id, "0"), + Check: testCheckAzureAdGroupBasic(id, "0", "0"), + }, + { + ResourceName: rn, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccAzureADGroup_complete(t *testing.T) { + rn := "azuread_group.test" + id := tf.AccRandTimeInt() + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAzureADGroup_basic(id), + Check: testCheckAzureAdGroupBasic(id, "0", "0"), + }, + { + ResourceName: rn, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccAzureADGroup_owners(t *testing.T) { + rn := "azuread_group.test" + id := tf.AccRandTimeInt() + pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAzureADGroupWithThreeOwners(id, pw), + Check: testCheckAzureAdGroupBasic(id, "0", "3"), }, { ResourceName: rn, @@ -46,7 +91,7 @@ func TestAccAzureADGroup_members(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccAzureADGroupWithThreeMembers(id, pw), - Check: testCheckAzureAdGroupBasic(id, "3"), + Check: testCheckAzureAdGroupBasic(id, "3", "0"), }, { ResourceName: rn, @@ -57,9 +102,10 @@ func TestAccAzureADGroup_members(t *testing.T) { }) } -func TestAccAzureADGroup_complete(t *testing.T) { +func TestAccAzureADGroup_membersAndOwners(t *testing.T) { rn := "azuread_group.test" id := tf.AccRandTimeInt() + pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -67,8 +113,8 @@ func TestAccAzureADGroup_complete(t *testing.T) { CheckDestroy: testCheckAzureADGroupDestroy, Steps: []resource.TestStep{ { - Config: testAccAzureADGroup_basic(id), - Check: testCheckAzureAdGroupBasic(id, "0"), + Config: testAccAzureADGroupWithOwnersAndMembers(id, pw), + Check: testCheckAzureAdGroupBasic(id, "2", "1"), }, { ResourceName: rn, @@ -79,7 +125,7 @@ func TestAccAzureADGroup_complete(t *testing.T) { }) } -func TestAccAzureADGroup_diverse(t *testing.T) { +func TestAccAzureADGroup_membersDiverse(t *testing.T) { rn := "azuread_group.test" id := tf.AccRandTimeInt() pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) @@ -91,7 +137,7 @@ func TestAccAzureADGroup_diverse(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccAzureADGroupWithDiverseMembers(id, pw), - Check: testCheckAzureAdGroupBasic(id, "3"), + Check: testCheckAzureAdGroupBasic(id, "3", "0"), }, { ResourceName: rn, @@ -102,7 +148,7 @@ func TestAccAzureADGroup_diverse(t *testing.T) { }) } -func TestAccAzureADGroup_progression(t *testing.T) { +func TestAccAzureADGroup_membersUpdate(t *testing.T) { rn := "azuread_group.test" id := tf.AccRandTimeInt() pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) @@ -115,7 +161,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { // Empty group with 0 members { Config: testAccAzureADGroup_basic(id), - Check: testCheckAzureAdGroupBasic(id, "0"), + Check: testCheckAzureAdGroupBasic(id, "0", "0"), }, { ResourceName: rn, @@ -125,7 +171,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { // Group with 1 member { Config: testAccAzureADGroupWithOneMember(id, pw), - Check: testCheckAzureAdGroupBasic(id, "1"), + Check: testCheckAzureAdGroupBasic(id, "1", "0"), }, { ResourceName: rn, @@ -135,7 +181,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { // Group with multiple members { Config: testAccAzureADGroupWithThreeMembers(id, pw), - Check: testCheckAzureAdGroupBasic(id, "3"), + Check: testCheckAzureAdGroupBasic(id, "3", "0"), }, { ResourceName: rn, @@ -145,7 +191,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { // Group with a different member { Config: testAccAzureADGroupWithServicePrincipal(id), - Check: testCheckAzureAdGroupBasic(id, "1"), + Check: testCheckAzureAdGroupBasic(id, "1", "0"), }, { ResourceName: rn, @@ -155,7 +201,7 @@ func TestAccAzureADGroup_progression(t *testing.T) { // Empty group with 0 members { Config: testAccAzureADGroup_basic(id), - Check: testCheckAzureAdGroupBasic(id, "0"), + Check: testCheckAzureAdGroupBasic(id, "0", "0"), }, }, }) @@ -207,7 +253,7 @@ func testCheckAzureADGroupDestroy(s *terraform.State) error { return nil } -func testCheckAzureAdGroupBasic(id int, memberCount string) resource.TestCheckFunc { +func testCheckAzureAdGroupBasic(id int, memberCount, ownerCount string) resource.TestCheckFunc { resourceName := "azuread_group.test" return resource.ComposeTestCheckFunc( @@ -215,6 +261,7 @@ func testCheckAzureAdGroupBasic(id int, memberCount string) resource.TestCheckFu resource.TestCheckResourceAttr(resourceName, "name", fmt.Sprintf("acctestGroup-%d", id)), resource.TestCheckResourceAttrSet(resourceName, "object_id"), resource.TestCheckResourceAttr(resourceName, "members.#", memberCount), + resource.TestCheckResourceAttr(resourceName, "owners.#", ownerCount), ) } @@ -279,33 +326,36 @@ resource "azuread_group" "test" { func testAccAzureADGroupWithThreeMembers(id int, password string) string { return fmt.Sprintf(` -data "azuread_domains" "tenant_domain" { - only_initial = true -} +%[1]s -resource "azuread_user" "testA" { - user_principal_name = "acctestUser.%[1]d.A@${data.azuread_domains.tenant_domain.domains.0.domain_name}" - display_name = "acctestUser-%[1]dA" - password = "%[2]s" +resource "azuread_group" "test" { + name = "acctestGroup-%[2]d" + members = [ azuread_user.testA.object_id, azuread_user.testB.object_id, azuread_user.testC.object_id ] } - -resource "azuread_user" "testB" { - user_principal_name = "acctestUser.%[1]d.B@${data.azuread_domains.tenant_domain.domains.0.domain_name}" - display_name = "acctestUser-%[1]d-B" - password = "%[2]s" +`, testAccADUser_threeUsersABC(id, password), id) } -resource "azuread_user" "testC" { - user_principal_name = "acctestUser.%[1]dC@${data.azuread_domains.tenant_domain.domains.0.domain_name}" - display_name = "acctestUser-%[1]dC" - password = "%[2]s" +func testAccAzureADGroupWithThreeOwners(id int, password string) string { + return fmt.Sprintf(` +%[1]s + +resource "azuread_group" "test" { + name = "acctestGroup-%[2]d" + owners = [ azuread_user.testA.object_id, azuread_user.testB.object_id, azuread_user.testC.object_id ] +} +`, testAccADUser_threeUsersABC(id, password), id) } +func testAccAzureADGroupWithOwnersAndMembers(id int, password string) string { + return fmt.Sprintf(` +%[1]s + resource "azuread_group" "test" { - name = "acctestGroup-%[1]d" - members = [ azuread_user.testA.object_id, azuread_user.testB.object_id, azuread_user.testC.object_id ] + name = "acctestGroup-%[2]d" + owners = [ azuread_user.testA.object_id ] + members = [ azuread_user.testB.object_id, azuread_user.testC.object_id ] } -`, id, password) +`, testAccADUser_threeUsersABC(id, password), id) } func testAccAzureADGroupWithServicePrincipal(id int) string { diff --git a/azuread/resource_user_test.go b/azuread/resource_user_test.go index 34f6470938..60e82c7993 100644 --- a/azuread/resource_user_test.go +++ b/azuread/resource_user_test.go @@ -114,7 +114,7 @@ func TestAccAzureADUser_update(t *testing.T) { ), }, { - Config: testAccADUser_multiple(id, pw1), + Config: testAccADUser_threeUsersABC(id, pw1), Check: resource.ComposeTestCheckFunc( testCheckADUserExists("azuread_user.testA"), testCheckADUserExists("azuread_user.testB"), @@ -207,23 +207,29 @@ resource "azuread_user" "test" { `, id, password) } -func testAccADUser_multiple(id int, password string) string { +func testAccADUser_threeUsersABC(id int, password string) string { return fmt.Sprintf(` data "azuread_domains" "tenant_domain" { only_initial = true } resource "azuread_user" "testA" { - user_principal_name = "acctestUser.%[1]d.A@${data.azuread_domains.tenant_domain.domains.0.domain_name}" - display_name = "acctestUser-%[1]d-A" - password = "%[2]s" + user_principal_name = "acctestUser.%[1]d.A@${data.azuread_domains.tenant_domain.domains.0.domain_name}" + display_name = "acctestUser-%[1]d-A" + password = "%[2]s" } resource "azuread_user" "testB" { - user_principal_name = "acctestUser.%[1]d.B@${data.azuread_domains.tenant_domain.domains.0.domain_name}" - display_name = "acctestUser-%[1]d-B" - mail_nickname = "acctestUser-%[1]d-B" - password = "%[2]s" + user_principal_name = "acctestUser.%[1]d.B@${data.azuread_domains.tenant_domain.domains.0.domain_name}" + display_name = "acctestUser-%[1]d-B" + mail_nickname = "acctestUser-%[1]d-B" + password = "%[2]s" +} + +resource "azuread_user" "testC" { + user_principal_name = "acctestUser.%[1]d.C@${data.azuread_domains.tenant_domain.domains.0.domain_name}" + display_name = "acctestUser-%[1]d-C" + password = "%[2]s" } `, id, password) } From 7ee0a24854c1d3d584463e4757e16429dc70fab8 Mon Sep 17 00:00:00 2001 From: kt Date: Sun, 21 Jul 2019 15:35:04 -0700 Subject: [PATCH 2/6] add azuread_group_owner resource --- azuread/data_group.go | 2 +- azuread/data_group_test.go | 39 +++- azuread/helpers/ar/response.go | 4 +- azuread/helpers/graph/credentials.go | 2 +- azuread/helpers/graph/group.go | 54 +++++ azuread/helpers/graph/object_resource.go | 58 +++++ azuread/helpers/tf/locks.go | 15 ++ azuread/locks.go | 10 - azuread/provider.go | 5 +- azuread/resource_application_password.go | 8 +- azuread/resource_group.go | 2 + azuread/resource_group_member.go | 49 ++--- azuread/resource_group_member_test.go | 169 ++++++++------ azuread/resource_group_owner.go | 114 ++++++++++ azuread/resource_group_owner_test.go | 206 ++++++++++++++++++ azuread/resource_group_test.go | 145 ++++++++++-- .../resource_service_principal_password.go | 8 +- 17 files changed, 754 insertions(+), 136 deletions(-) create mode 100644 azuread/helpers/graph/object_resource.go create mode 100644 azuread/helpers/tf/locks.go delete mode 100644 azuread/locks.go create mode 100644 azuread/resource_group_owner.go create mode 100644 azuread/resource_group_owner_test.go diff --git a/azuread/data_group.go b/azuread/data_group.go index 64c12ae94a..779139724d 100644 --- a/azuread/data_group.go +++ b/azuread/data_group.go @@ -96,7 +96,7 @@ func dataSourceActiveDirectoryGroupRead(d *schema.ResourceData, meta interface{} if err != nil { return err } - d.Set("members", owners) + d.Set("owners", owners) return nil } diff --git a/azuread/data_group_test.go b/azuread/data_group_test.go index 1cbc80d160..b4e22f3180 100644 --- a/azuread/data_group_test.go +++ b/azuread/data_group_test.go @@ -18,9 +18,6 @@ func TestAccDataSourceAzureADGroup_byName(t *testing.T) { Providers: testAccProviders, CheckDestroy: testCheckAzureADGroupDestroy, Steps: []resource.TestStep{ - { - Config: testAccAzureADGroup_basic(id), - }, { Config: testAccDataSourceAzureADGroup_name(id), Check: resource.ComposeTestCheckFunc( @@ -41,9 +38,6 @@ func TestAccDataSourceAzureADGroup_byObjectId(t *testing.T) { Providers: testAccProviders, CheckDestroy: testCheckAzureADGroupDestroy, Steps: []resource.TestStep{ - { - Config: testAccAzureADGroup_basic(id), - }, { Config: testAccDataSourceAzureADGroup_objectId(id), Check: resource.ComposeTestCheckFunc( @@ -67,13 +61,32 @@ func TestAccDataSourceAzureADGroup_members(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccDataSourceAzureADGroup_members(id, pw), + Check: resource.ComposeTestCheckFunc( + testCheckAzureADGroupExists(dsn), + resource.TestCheckResourceAttr(dsn, "name", fmt.Sprintf("acctestGroup-%d", id)), + resource.TestCheckResourceAttr(dsn, "members.#", "3"), + ), }, + }, + }) +} + +func TestAccDataSourceAzureADGroup_owners(t *testing.T) { + dsn := "data.azuread_group.test" + id := tf.AccRandTimeInt() + pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupDestroy, + Steps: []resource.TestStep{ { - Config: testAccDataSourceAzureADGroup_objectId(id), + Config: testAccDataSourceAzureADGroup_owners(id, pw), Check: resource.ComposeTestCheckFunc( testCheckAzureADGroupExists(dsn), resource.TestCheckResourceAttr(dsn, "name", fmt.Sprintf("acctestGroup-%d", id)), - resource.TestCheckResourceAttr(dsn, "members.#", "3"), + resource.TestCheckResourceAttr(dsn, "owners.#", "3"), ), }, }, @@ -109,3 +122,13 @@ data "azuread_group" "test" { } `, testAccAzureADGroupWithThreeMembers(id, password)) } + +func testAccDataSourceAzureADGroup_owners(id int, password string) string { + return fmt.Sprintf(` +%s + +data "azuread_group" "test" { + object_id = "${azuread_group.test.object_id}" +} +`, testAccAzureADGroupWithThreeOwners(id, password)) +} diff --git a/azuread/helpers/ar/response.go b/azuread/helpers/ar/response.go index 9dc805c874..1f557af964 100644 --- a/azuread/helpers/ar/response.go +++ b/azuread/helpers/ar/response.go @@ -9,7 +9,7 @@ import ( //todo move to azure helpers func ResponseWasNotFound(resp autorest.Response) bool { - return responseWasStatusCode(resp, http.StatusNotFound) + return ResponseWasStatusCode(resp, http.StatusNotFound) } func ResponseErrorIsRetryable(err error) bool { @@ -27,7 +27,7 @@ func ResponseErrorIsRetryable(err error) bool { return false } -func responseWasStatusCode(resp autorest.Response, statusCode int) bool { // nolint: unparam +func ResponseWasStatusCode(resp autorest.Response, statusCode int) bool { // nolint: unparam if r := resp.Response; r != nil { if r.StatusCode == statusCode { return true diff --git a/azuread/helpers/graph/credentials.go b/azuread/helpers/graph/credentials.go index ca7a744568..16c6cc87bf 100644 --- a/azuread/helpers/graph/credentials.go +++ b/azuread/helpers/graph/credentials.go @@ -90,7 +90,7 @@ func ParsePasswordCredentialId(id string) (PasswordCredentialId, error) { } if _, err := uuid.ParseUUID(parts[1]); err != nil { - return PasswordCredentialId{}, fmt.Errorf("Object ID isn't a valid UUID (%q): %+v", id[1], err) + return PasswordCredentialId{}, fmt.Errorf("Credential ID isn't a valid UUID (%q): %+v", id[1], err) } return PasswordCredentialId{ diff --git a/azuread/helpers/graph/group.go b/azuread/helpers/graph/group.go index b24eb49fbf..db06b472d5 100644 --- a/azuread/helpers/graph/group.go +++ b/azuread/helpers/graph/group.go @@ -8,6 +8,60 @@ import ( "github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac" ) +type GroupOwnerId struct { + ObjectSubResourceId + GroupId string + OwnerId string +} + +func GroupOwnerIdFrom(groupId, ownerId string) GroupOwnerId { + return GroupOwnerId{ + ObjectSubResourceId: ObjectSubResourceIdFrom(groupId, "owner", ownerId), + GroupId: groupId, + OwnerId: ownerId, + } +} + +func ParseGroupOwnerId(idString string) (GroupOwnerId, error) { + id, err := ParseObjectSubResourceId(idString, "owner") + if err != nil { + return GroupOwnerId{}, fmt.Errorf("Unable to parse Owner ID: %v", err) + } + + return GroupOwnerId{ + ObjectSubResourceId: id, + GroupId: id.objectId, + OwnerId: id.subId, + }, nil +} + +type GroupMemberId struct { + ObjectSubResourceId + GroupId string + MemberId string +} + +func GroupMemberIdFrom(groupId, memberId string) GroupMemberId { + return GroupMemberId{ + ObjectSubResourceId: ObjectSubResourceIdFrom(groupId, "member", memberId), + GroupId: groupId, + MemberId: memberId, + } +} + +func ParseGroupMemberId(idString string) (GroupMemberId, error) { + id, err := ParseObjectSubResourceId(idString, "member") + if err != nil { + return GroupMemberId{}, fmt.Errorf("Unable to parse Member ID: %v", err) + } + + return GroupMemberId{ + ObjectSubResourceId: id, + GroupId: id.objectId, + MemberId: id.subId, + }, nil +} + func GroupGetByDisplayName(client *graphrbac.GroupsClient, ctx context.Context, displayName string) (*graphrbac.ADGroup, error) { filter := fmt.Sprintf("displayName eq '%s'", displayName) diff --git a/azuread/helpers/graph/object_resource.go b/azuread/helpers/graph/object_resource.go new file mode 100644 index 0000000000..7e562171b4 --- /dev/null +++ b/azuread/helpers/graph/object_resource.go @@ -0,0 +1,58 @@ +package graph + +import ( + "fmt" + "strings" + + "github.com/hashicorp/go-uuid" +) + +type ObjectSubResourceId struct { + objectId string + subId string + Type string +} + +func (id ObjectSubResourceId) String() string { + return id.objectId + "/" + id.Type + "/" + id.subId +} + +func ParseObjectSubResourceId(idString, expectedType string) (ObjectSubResourceId, error) { + parts := strings.Split(idString, "/") + if len(parts) != 3 { + return ObjectSubResourceId{}, fmt.Errorf("Object Resource ID should be in the format {objectId}/{keyId} - but got %q", idString) + } + + id := ObjectSubResourceId{ + objectId: parts[0], + Type: parts[1], + subId: parts[2], + } + + if _, err := uuid.ParseUUID(id.objectId); err != nil { + return ObjectSubResourceId{}, fmt.Errorf("Object ID isn't a valid UUID (%q): %+v", id.objectId, err) + } + + if id.Type == "" { + return ObjectSubResourceId{}, fmt.Errorf("Type in {objectID}/{type}/{subID} should not blank") + } + + if id.Type != expectedType { + return ObjectSubResourceId{}, fmt.Errorf("Type in {objectID}/{type}/{subID} was expected to be %s, got %s", expectedType, parts[2]) + } + + if _, err := uuid.ParseUUID(id.subId); err != nil { + return ObjectSubResourceId{}, fmt.Errorf("Object Sub Resource ID isn't a valid UUID (%q): %+v", id.subId, err) + } + + return id, nil + +} + +func ObjectSubResourceIdFrom(objectId, typeId, subId string) ObjectSubResourceId { + return ObjectSubResourceId{ + objectId: objectId, + Type: typeId, + subId: subId, + } +} diff --git a/azuread/helpers/tf/locks.go b/azuread/helpers/tf/locks.go new file mode 100644 index 0000000000..3419b365f9 --- /dev/null +++ b/azuread/helpers/tf/locks.go @@ -0,0 +1,15 @@ +package tf + +import "github.com/hashicorp/terraform/helper/mutexkv" + +// mutex is the instance of MutexKV for ARM resources +var mutex = mutexkv.NewMutexKV() + +// handles the case of using the same name for different kinds of resources +func LockByName(resourceType string, name string) { + mutex.Lock(resourceType + "." + name) +} + +func UnlockByName(resourceType string, name string) { + mutex.Unlock(resourceType + "." + name) +} diff --git a/azuread/locks.go b/azuread/locks.go deleted file mode 100644 index f62b745b0f..0000000000 --- a/azuread/locks.go +++ /dev/null @@ -1,10 +0,0 @@ -package azuread - -// handles the case of using the same name for different kinds of resources -func azureADLockByName(resourceType string, name string) { - armMutexKV.Lock(resourceType + "." + name) -} - -func azureADUnlockByName(resourceType string, name string) { - armMutexKV.Unlock(resourceType + "." + name) -} diff --git a/azuread/provider.go b/azuread/provider.go index 04f6298144..14bf9a05ee 100644 --- a/azuread/provider.go +++ b/azuread/provider.go @@ -4,14 +4,10 @@ import ( "fmt" "github.com/hashicorp/go-azure-helpers/authentication" - "github.com/hashicorp/terraform/helper/mutexkv" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/terraform" ) -// armMutexKV is the instance of MutexKV for ARM resources -var armMutexKV = mutexkv.NewMutexKV() - // Provider returns a terraform.ResourceProvider. func Provider() terraform.ResourceProvider { p := &schema.Provider{ @@ -88,6 +84,7 @@ func Provider() terraform.ResourceProvider { "azuread_application_password": resourceApplicationPassword(), "azuread_group": resourceGroup(), "azuread_group_member": resourceGroupMember(), + "azuread_group_owner": resourceGroupOwner(), "azuread_service_principal": resourceServicePrincipal(), "azuread_service_principal_password": resourceServicePrincipalPassword(), "azuread_user": resourceUser(), diff --git a/azuread/resource_application_password.go b/azuread/resource_application_password.go index bd20ab2e39..09ad01d221 100644 --- a/azuread/resource_application_password.go +++ b/azuread/resource_application_password.go @@ -108,8 +108,8 @@ func resourceApplicationPasswordCreate(d *schema.ResourceData, meta interface{}) } id := graph.PasswordCredentialIdFrom(objectId, *cred.KeyID) - azureADLockByName(resourceApplicationName, id.ObjectId) - defer azureADUnlockByName(resourceApplicationName, id.ObjectId) + tf.LockByName(resourceApplicationName, id.ObjectId) + defer tf.UnlockByName(resourceApplicationName, id.ObjectId) existingCreds, err := client.ListPasswordCredentials(ctx, id.ObjectId) if err != nil { @@ -194,8 +194,8 @@ func resourceApplicationPasswordDelete(d *schema.ResourceData, meta interface{}) return fmt.Errorf("Error parsing Application Password ID: %v", err) } - azureADLockByName(resourceApplicationName, id.ObjectId) - defer azureADUnlockByName(resourceApplicationName, id.ObjectId) + tf.LockByName(resourceApplicationName, id.ObjectId) + defer tf.UnlockByName(resourceApplicationName, id.ObjectId) // ensure the parent Application exists app, err := client.Get(ctx, id.ObjectId) diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 336e6557f0..7acfd68b3a 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -93,6 +93,7 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error { if v, ok := d.GetOk("members"); ok { members := tf.ExpandStringSlicePtr(v.(*schema.Set).List()) + // we could lock here against the group ember resource, but the should not be used together (todo conflicts with at a resource level?) if err := graph.GroupAddMembers(client, ctx, *group.ObjectID, *members); err != nil { return err } @@ -102,6 +103,7 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error { if v, ok := d.GetOk("owners"); ok { members := tf.ExpandStringSlicePtr(v.(*schema.Set).List()) + // we could lock here against the group owner resource, but the should not be used together (todo conflicts with at a resource level?) if err := graph.GroupAddOwners(client, ctx, *group.ObjectID, *members); err != nil { return err } diff --git a/azuread/resource_group_member.go b/azuread/resource_group_member.go index 1b83eb16ca..f8507279db 100644 --- a/azuread/resource_group_member.go +++ b/azuread/resource_group_member.go @@ -2,20 +2,22 @@ package azuread import ( "fmt" - "strings" - - "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/graph" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/tf" "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/validate" ) +const groupMemberResourceName = "azuread_group_member" + func resourceGroupMember() *schema.Resource { return &schema.Resource{ Create: resourceGroupMemberCreate, Read: resourceGroupMemberRead, Delete: resourceGroupMemberDelete, + Importer: &schema.ResourceImporter{ State: schema.ImportStatePassthrough, }, @@ -45,13 +47,14 @@ func resourceGroupMemberCreate(d *schema.ResourceData, meta interface{}) error { groupID := d.Get("group_object_id").(string) memberID := d.Get("member_object_id").(string) + tf.LockByName(groupMemberResourceName, groupID) + defer tf.UnlockByName(groupMemberResourceName, groupID) + if err := graph.GroupAddMember(client, ctx, groupID, memberID); err != nil { return err } - id := fmt.Sprintf("%s/member/%s", groupID, memberID) - d.SetId(id) - + d.SetId(graph.GroupMemberIdFrom(groupID, memberID).String()) return resourceGroupMemberRead(d, meta) } @@ -59,33 +62,29 @@ func resourceGroupMemberRead(d *schema.ResourceData, meta interface{}) error { client := meta.(*ArmClient).groupsClient ctx := meta.(*ArmClient).StopContext - id := strings.Split(d.Id(), "/member/") - if len(id) != 2 { - return fmt.Errorf("ID should be in the format {groupObjectId}/member/{memberObjectId} - but got %q", d.Id()) + id, err := graph.ParseGroupMemberId(d.Id()) + if err != nil { + return fmt.Errorf("Unable to parse ID: %v", err) } - groupID := id[0] - memberID := id[1] - - members, err := graph.GroupAllMembers(client, ctx, groupID) + members, err := graph.GroupAllMembers(client, ctx, id.GroupId) if err != nil { - return fmt.Errorf("Error retrieving Azure AD Group members (groupObjectId: %q): %+v", groupID, err) + return fmt.Errorf("Error retrieving Azure AD Group members (groupObjectId: %q): %+v", id.GroupId, err) } var memberObjectID string - for _, objectID := range members { - if objectID == memberID { + if objectID == id.MemberId { memberObjectID = objectID } } if memberObjectID == "" { d.SetId("") - return fmt.Errorf("Azure AD Group Member not found - groupObjectId:%q / memberObjectId:%q", groupID, memberID) + return fmt.Errorf("Azure AD Group Member not found - groupObjectId:%q / memberObjectId:%q", id.GroupId, id.MemberId) } - d.Set("group_object_id", groupID) + d.Set("group_object_id", id.GroupId) d.Set("member_object_id", memberObjectID) return nil @@ -95,18 +94,18 @@ func resourceGroupMemberDelete(d *schema.ResourceData, meta interface{}) error { client := meta.(*ArmClient).groupsClient ctx := meta.(*ArmClient).StopContext - id := strings.Split(d.Id(), "/member/") - if len(id) != 2 { - return fmt.Errorf("ID should be in the format {groupObjectId}/member/{memberObjectId} - but got %q", d.Id()) + id, err := graph.ParseGroupMemberId(d.Id()) + if err != nil { + return fmt.Errorf("Unable to parse ID: %v", err) } - groupID := id[0] - memberID := id[1] + tf.LockByName(groupMemberResourceName, id.GroupId) + defer tf.UnlockByName(groupMemberResourceName, id.GroupId) - resp, err := client.RemoveMember(ctx, groupID, memberID) + resp, err := client.RemoveMember(ctx, id.GroupId, id.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 fmt.Errorf("Error removing Member (memberObjectId: %q) from Azure AD Group (groupObjectId: %q): %+v", id.MemberId, id.GroupId, err) } } diff --git a/azuread/resource_group_member_test.go b/azuread/resource_group_member_test.go index 4cc76762b2..bb2c98b049 100644 --- a/azuread/resource_group_member_test.go +++ b/azuread/resource_group_member_test.go @@ -8,11 +8,12 @@ import ( "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/ar" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/graph" "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/tf" ) -func TestAccAzureADGroupMember_User(t *testing.T) { - rn := "azuread_group_member.test" +func TestAccAzureADGroupMember_user(t *testing.T) { + rn := "azuread_group_member.testA" id := tf.AccRandTimeInt() pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) @@ -22,7 +23,7 @@ func TestAccAzureADGroupMember_User(t *testing.T) { CheckDestroy: testCheckAzureADGroupMemberDestroy, Steps: []resource.TestStep{ { - Config: testAccAzureADGroupMember_User(id, pw), + Config: testAccAzureADGroupMember_oneUser(id, pw), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttrSet(rn, "group_object_id"), resource.TestCheckResourceAttrSet(rn, "member_object_id"), @@ -37,7 +38,68 @@ func TestAccAzureADGroupMember_User(t *testing.T) { }) } -func TestAccAzureADGroupMember_Group(t *testing.T) { +func TestAccAzureADGroupMember_multipleUser(t *testing.T) { + rna := "azuread_group_member.testA" + rnb := "azuread_group_member.testB" + id := tf.AccRandTimeInt() + pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupMemberDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAzureADGroupMember_oneUser(id, pw), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet(rna, "group_object_id"), + resource.TestCheckResourceAttrSet(rna, "member_object_id"), + ), + }, + { + ResourceName: rna, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccAzureADGroupMember_twoUsers(id, pw), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet(rna, "group_object_id"), + resource.TestCheckResourceAttrSet(rna, "member_object_id"), + resource.TestCheckResourceAttrSet(rnb, "group_object_id"), + resource.TestCheckResourceAttrSet(rnb, "member_object_id"), + ), + }, + // we rerun the config so the group resource updates with the number of members + { + Config: testAccAzureADGroupMember_twoUsers(id, pw), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("azuread_group.test", "members.#", "2"), + ), + }, + { + ResourceName: rna, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccAzureADGroupMember_oneUser(id, pw), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet(rna, "group_object_id"), + resource.TestCheckResourceAttrSet(rna, "member_object_id"), + ), + }, + { + Config: testAccAzureADGroupMember_oneUser(id, pw), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("azuread_group.test", "members.#", "1"), + ), + }, + }, + }) +} + +func TestAccAzureADGroupMember_group(t *testing.T) { rn := "azuread_group_member.test" id := tf.AccRandTimeInt() @@ -47,7 +109,7 @@ func TestAccAzureADGroupMember_Group(t *testing.T) { CheckDestroy: testCheckAzureADGroupMemberDestroy, Steps: []resource.TestStep{ { - Config: testAccAzureADGroupMember_Group(id), + Config: testAccAzureADGroupMember_group(id), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttrSet(rn, "group_object_id"), resource.TestCheckResourceAttrSet(rn, "member_object_id"), @@ -62,7 +124,7 @@ func TestAccAzureADGroupMember_Group(t *testing.T) { }) } -func TestAccAzureADGroupMember_ServicePrincipal(t *testing.T) { +func TestAccAzureADGroupMember_servicePrincipal(t *testing.T) { rn := "azuread_group_member.test" id := tf.AccRandTimeInt() @@ -72,7 +134,7 @@ func TestAccAzureADGroupMember_ServicePrincipal(t *testing.T) { CheckDestroy: testCheckAzureADGroupMemberDestroy, Steps: []resource.TestStep{ { - Config: testAccAzureADGroupMember_ServicePrincipal(id), + Config: testAccAzureADGroupMember_servicePrincipal(id), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttrSet(rn, "group_object_id"), resource.TestCheckResourceAttrSet(rn, "member_object_id"), @@ -99,53 +161,24 @@ func testCheckAzureADGroupMemberDestroy(s *terraform.State) error { 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 + // see if group exists + if resp, err := client.Get(ctx, groupID); err != nil { + if ar.ResponseWasNotFound(resp.Response) { + continue } - return err + return fmt.Errorf("Error retrieving Azure AD Group with ID %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 - } - } + members, err := graph.GroupAllMembers(client, ctx, groupID) + if err != nil { + return fmt.Errorf("Error retrieving Azure AD Group members (groupObjectId: %q): %+v", groupID, err) + } - err = members.NextWithContext(ctx) - if err != nil { - return fmt.Errorf("Error listing Azure AD Group Members: %s", err) + var memberObjectID string + for _, objectID := range members { + if objectID == memberID { + memberObjectID = objectID } } @@ -157,32 +190,44 @@ func testCheckAzureADGroupMemberDestroy(s *terraform.State) error { return nil } -func testAccAzureADGroupMember_User(id int, password string) string { +func testAccAzureADGroupMember_oneUser(id int, password string) string { return fmt.Sprintf(` +%[1]s + +resource "azuread_group" "test" { + name = "acctestGroup-%[2]d" +} -data "azuread_domains" "tenant_domain" { - only_initial = true +resource "azuread_group_member" "testA" { + group_object_id = "${azuread_group.test.object_id}" + member_object_id = "${azuread_user.testA.object_id}" } -resource "azuread_user" "test" { - user_principal_name = "acctestUser.%[1]d.A@${data.azuread_domains.tenant_domain.domains.0.domain_name}" - display_name = "acctestUser-%[1]d-A" - password = "%[2]s" +`, testAccADUser_threeUsersABC(id, password), id) } + +func testAccAzureADGroupMember_twoUsers(id int, password string) string { + return fmt.Sprintf(` +%[1]s resource "azuread_group" "test" { - name = "acctestGroup-%[1]d" + name = "acctestGroup-%[2]d" } -resource "azuread_group_member" "test" { +resource "azuread_group_member" "testA" { + group_object_id = "${azuread_group.test.object_id}" + member_object_id = "${azuread_user.testA.object_id}" +} + +resource "azuread_group_member" "testB" { group_object_id = "${azuread_group.test.object_id}" - member_object_id = "${azuread_user.test.object_id}" + member_object_id = "${azuread_user.testB.object_id}" } -`, id, password) +`, testAccADUser_threeUsersABC(id, password), id) } -func testAccAzureADGroupMember_Group(id int) string { +func testAccAzureADGroupMember_group(id int) string { return fmt.Sprintf(` resource "azuread_group" "test" { @@ -201,7 +246,7 @@ resource "azuread_group_member" "test" { `, id) } -func testAccAzureADGroupMember_ServicePrincipal(id int) string { +func testAccAzureADGroupMember_servicePrincipal(id int) string { return fmt.Sprintf(` resource "azuread_application" "test" { diff --git a/azuread/resource_group_owner.go b/azuread/resource_group_owner.go new file mode 100644 index 0000000000..9c42a83c1e --- /dev/null +++ b/azuread/resource_group_owner.go @@ -0,0 +1,114 @@ +package azuread + +import ( + "fmt" + + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/graph" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/tf" + + "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" +) + +const groupOwnberResourceName = "azuread_group_owner" + +func resourceGroupOwner() *schema.Resource { + return &schema.Resource{ + Create: resourceGroupOwnerCreate, + Read: resourceGroupOwnerRead, + Delete: resourceGroupOwnerDelete, + + Importer: &schema.ResourceImporter{ + State: schema.ImportStatePassthrough, + }, + + Schema: map[string]*schema.Schema{ + "group_object_id": { + Type: schema.TypeString, + Required: true, + ForceNew: true, + ValidateFunc: validate.UUID, + }, + + "owner_object_id": { + Type: schema.TypeString, + Required: true, + ForceNew: true, + ValidateFunc: validate.UUID, + }, + }, + } +} + +func resourceGroupOwnerCreate(d *schema.ResourceData, meta interface{}) error { + client := meta.(*ArmClient).groupsClient + ctx := meta.(*ArmClient).StopContext + + groupID := d.Get("group_object_id").(string) + ownerID := d.Get("owner_object_id").(string) + + tf.LockByName(groupOwnberResourceName, groupID) + defer tf.UnlockByName(groupOwnberResourceName, groupID) + + if err := graph.GroupAddOwner(client, ctx, groupID, ownerID); err != nil { + return err + } + + d.SetId(graph.GroupOwnerIdFrom(groupID, ownerID).String()) + return resourceGroupOwnerRead(d, meta) +} + +func resourceGroupOwnerRead(d *schema.ResourceData, meta interface{}) error { + client := meta.(*ArmClient).groupsClient + ctx := meta.(*ArmClient).StopContext + + id, err := graph.ParseGroupOwnerId(d.Id()) + if err != nil { + return fmt.Errorf("Unable to parse ID: %v", err) + } + + owners, err := graph.GroupAllOwners(client, ctx, id.GroupId) + if err != nil { + return fmt.Errorf("Error retrieving Azure AD Group owners (groupObjectId: %q): %+v", id.GroupId, err) + } + + var ownerObjectID string + for _, objectID := range owners { + if objectID == id.OwnerId { + ownerObjectID = objectID + } + } + + if ownerObjectID == "" { + d.SetId("") + return fmt.Errorf("Azure AD Group Owner not found - groupObjectId:%q / ownerObjectId:%q", id.GroupId, id.OwnerId) + } + + d.Set("group_object_id", id.GroupId) + d.Set("owner_object_id", id.OwnerId) + + return nil +} + +func resourceGroupOwnerDelete(d *schema.ResourceData, meta interface{}) error { + client := meta.(*ArmClient).groupsClient + ctx := meta.(*ArmClient).StopContext + + id, err := graph.ParseGroupOwnerId(d.Id()) + if err != nil { + return fmt.Errorf("Unable to parse ID: %v", err) + } + + tf.LockByName(groupOwnberResourceName, id.GroupId) + defer tf.UnlockByName(groupOwnberResourceName, id.GroupId) + + resp, err := client.RemoveOwner(ctx, id.GroupId, id.OwnerId) + if err != nil { + if !ar.ResponseWasNotFound(resp) { + return fmt.Errorf("Error removing Owner (ownerObjectId: %q) from Azure AD Group (groupObjectId: %q): %+v", id.OwnerId, id.GroupId, err) + } + } + + return nil +} diff --git a/azuread/resource_group_owner_test.go b/azuread/resource_group_owner_test.go new file mode 100644 index 0000000000..7ceb2a29c4 --- /dev/null +++ b/azuread/resource_group_owner_test.go @@ -0,0 +1,206 @@ +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" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/graph" + "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/tf" +) + +func TestAccAzureADGroupOwner_user(t *testing.T) { + rna := "azuread_group_owner.testA" + rnb := "azuread_group_owner.testB" + id := tf.AccRandTimeInt() + pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupOwnerDestroy(rna), + Steps: []resource.TestStep{ + { + Config: testAccAzureADGroupOwner_oneUser(id, pw), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet(rna, "group_object_id"), + resource.TestCheckResourceAttrSet(rna, "owner_object_id"), + ), + }, + { + Config: testAccAzureADGroupOwner_twoUsers(id, pw), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet(rna, "group_object_id"), + resource.TestCheckResourceAttrSet(rna, "owner_object_id"), + resource.TestCheckResourceAttrSet(rnb, "group_object_id"), + resource.TestCheckResourceAttrSet(rnb, "owner_object_id"), + ), + }, + // we rerun the config so the group resource updates with the number of members + { + Config: testAccAzureADGroupOwner_twoUsers(id, pw), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("azuread_group.test", "owners.#", "2"), + ), + }, + { + ResourceName: rnb, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccAzureADGroupOwner_oneUser(id, pw), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet(rna, "group_object_id"), + resource.TestCheckResourceAttrSet(rna, "owner_object_id"), + ), + }, + { + Config: testAccAzureADGroupOwner_twoUsers(id, pw), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("azuread_group.test", "owners.#", "1"), + ), + }, + { + ResourceName: rna, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccAzureADGroupOwner_ServicePrincipal(t *testing.T) { + rn := "azuread_group_owner.test" + id := tf.AccRandTimeInt() + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupOwnerDestroy(rn), + Steps: []resource.TestStep{ + { + Config: testAccAzureADGroupOwner_ServicePrincipal(id), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet(rn, "group_object_id"), + resource.TestCheckResourceAttrSet(rn, "owner_object_id"), + ), + }, + { + ResourceName: rn, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func testCheckAzureADGroupOwnerDestroy(ignoreResource string) func(s *terraform.State) error { + return func(s *terraform.State) error { + for _, rs := range s.RootModule().Resources { + if rs.Type != "azuread_group_owner" { + continue + } + + if rs.Primary.ID == ignoreResource { + continue // there always has to be one owner remaining so lets ignore it + } + + client := testAccProvider.Meta().(*ArmClient).groupsClient + ctx := testAccProvider.Meta().(*ArmClient).StopContext + + groupID := rs.Primary.Attributes["group_object_id"] + ownerID := rs.Primary.Attributes["owner_object_id"] + + // see if group exists + if resp, err := client.Get(ctx, groupID); err != nil { + if ar.ResponseWasNotFound(resp.Response) { + continue + } + + return fmt.Errorf("Error retrieving Azure AD Group with ID %q: %+v", groupID, err) + } + + owners, err := graph.GroupAllOwners(client, ctx, groupID) + if err != nil { + return fmt.Errorf("Error retrieving Azure AD Group owners (groupObjectId: %q): %+v", groupID, err) + } + + var ownerObjectID string + for _, objectID := range owners { + if objectID == ownerID { + ownerObjectID = objectID + } + } + + if ownerObjectID != "" { + return fmt.Errorf("Azure AD group owner still exists:\n%#v", ownerObjectID) + } + } + + return nil + } +} + +func testAccAzureADGroupOwner_oneUser(id int, password string) string { + return fmt.Sprintf(` +%[1]s + +resource "azuread_group" "test" { + name = "acctestGroup-%[2]d" +} + +resource "azuread_group_owner" "testA" { + group_object_id = "${azuread_group.test.object_id}" + owner_object_id = "${azuread_user.testA.object_id}" +} + +`, testAccADUser_threeUsersABC(id, password), id) +} + +func testAccAzureADGroupOwner_twoUsers(id int, password string) string { + return fmt.Sprintf(` +%[1]s + +resource "azuread_group" "test" { + name = "acctestGroup-%[2]d" +} + +resource "azuread_group_owner" "testA" { + group_object_id = "${azuread_group.test.object_id}" + owner_object_id = "${azuread_user.testA.object_id}" +} + +resource "azuread_group_owner" "testB" { + group_object_id = "${azuread_group.test.object_id}" + owner_object_id = "${azuread_user.testB.object_id}" +} + +`, testAccADUser_threeUsersABC(id, password), id) +} + +func testAccAzureADGroupOwner_ServicePrincipal(id int) string { + return fmt.Sprintf(` + +resource "azuread_application" "test" { + name = "acctestApp-%[1]d" +} + +resource "azuread_service_principal" "test" { + application_id = "${azuread_application.test.application_id}" +} + +resource "azuread_group" "test" { + name = "acctestGroup-%[1]d" +} + +resource "azuread_group_owner" "test" { + group_object_id = "${azuread_group.test.object_id}" + owner_object_id = "${azuread_service_principal.test.object_id}" +} + +`, id) +} diff --git a/azuread/resource_group_test.go b/azuread/resource_group_test.go index c1370befbc..a86fb321bb 100644 --- a/azuread/resource_group_test.go +++ b/azuread/resource_group_test.go @@ -148,6 +148,29 @@ func TestAccAzureADGroup_membersDiverse(t *testing.T) { }) } +func TestAccAzureADGroup_ownersDiverse(t *testing.T) { + rn := "azuread_group.test" + id := tf.AccRandTimeInt() + pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAzureADGroupWithDiverseOwners(id, pw), + Check: testCheckAzureAdGroupBasic(id, "0", "2"), + }, + { + ResourceName: rn, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + func TestAccAzureADGroup_membersUpdate(t *testing.T) { rn := "azuread_group.test" id := tf.AccRandTimeInt() @@ -190,7 +213,7 @@ func TestAccAzureADGroup_membersUpdate(t *testing.T) { }, // Group with a different member { - Config: testAccAzureADGroupWithServicePrincipal(id), + Config: testAccAzureADGroupWithServicePrincipalMember(id), Check: testCheckAzureAdGroupBasic(id, "1", "0"), }, { @@ -207,6 +230,61 @@ func TestAccAzureADGroup_membersUpdate(t *testing.T) { }) } +func TestAccAzureADGroup_ownersUpdate(t *testing.T) { + rn := "azuread_group.test" + id := tf.AccRandTimeInt() + pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureADGroupDestroy, + Steps: []resource.TestStep{ + // Empty group with 0 owners + { + Config: testAccAzureADGroup_basic(id), + Check: testCheckAzureAdGroupBasic(id, "0", "0"), + }, + { + ResourceName: rn, + ImportState: true, + ImportStateVerify: true, + }, + { + ResourceName: rn, + ImportState: true, + ImportStateVerify: true, + }, + // Group with multiple owners + { + Config: testAccAzureADGroupWithThreeOwners(id, pw), + Check: testCheckAzureAdGroupBasic(id, "0", "3"), + }, + // Group with 1 owners + { + Config: testAccAzureADGroupWithOneOwners(id, pw), + Check: testCheckAzureAdGroupBasic(id, "0", "1"), + }, + { + ResourceName: rn, + ImportState: true, + ImportStateVerify: true, + }, + // Group with a different owners + { + Config: testAccAzureADGroupWithServicePrincipalOwner(id), + Check: testCheckAzureAdGroupBasic(id, "0", "1"), + }, + { + ResourceName: rn, + ImportState: true, + ImportStateVerify: true, + }, + // Empty group with 0 owners is not possible + }, + }) +} + func testCheckAzureADGroupExists(name string) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[name] @@ -274,7 +352,7 @@ resource "azuread_group" "test" { `, id) } -func testAccAzureADGroupWithDiverseMembers(id int, password string) string { +func testAccAzureADDiverseDirectoryObjects(id int, password string) string { return fmt.Sprintf(` data "azuread_domains" "tenant_domain" { only_initial = true @@ -297,31 +375,51 @@ resource "azuread_user" "test" { display_name = "acctestUser-%[1]d" password = "%[2]s" } +`, id, password) +} + +func testAccAzureADGroupWithDiverseMembers(id int, password string) string { + return fmt.Sprintf(` +%[1]s resource "azuread_group" "test" { - name = "acctestGroup-%[1]d" + name = "acctestGroup-%[2]d" members = [ azuread_user.test.object_id, azuread_group.member.object_id, azuread_service_principal.test.object_id ] } -`, id, password) +`, testAccAzureADDiverseDirectoryObjects(id, password), id) } -func testAccAzureADGroupWithOneMember(id int, password string) string { +func testAccAzureADGroupWithDiverseOwners(id int, password string) string { return fmt.Sprintf(` -data "azuread_domains" "tenant_domain" { - only_initial = true -} +%[1]s -resource "azuread_user" "test" { - user_principal_name = "acctestUser.%[1]d@${data.azuread_domains.tenant_domain.domains.0.domain_name}" - display_name = "acctestUser-%[1]d" - password = "%[2]s" +resource "azuread_group" "test" { + name = "acctestGroup-%[2]d" + owners = [ azuread_user.test.object_id, azuread_service_principal.test.object_id ] +} +`, testAccAzureADDiverseDirectoryObjects(id, password), id) } +func testAccAzureADGroupWithOneMember(id int, password string) string { + return fmt.Sprintf(` +%[1]s + resource "azuread_group" "test" { - name = "acctestGroup-%[1]d" + name = "acctestGroup-%[2]d" members = [ azuread_user.test.object_id ] } -`, id, password) +`, testAccADUser_basic(id, password), id) +} + +func testAccAzureADGroupWithOneOwners(id int, password string) string { + return fmt.Sprintf(` +%[1]s + +resource "azuread_group" "test" { + name = "acctestGroup-%[2]d" + owners = [ azuread_user.test.object_id ] +} +`, testAccADUser_basic(id, password), id) } func testAccAzureADGroupWithThreeMembers(id int, password string) string { @@ -358,7 +456,7 @@ resource "azuread_group" "test" { `, testAccADUser_threeUsersABC(id, password), id) } -func testAccAzureADGroupWithServicePrincipal(id int) string { +func testAccAzureADGroupWithServicePrincipalMember(id int) string { return fmt.Sprintf(` resource "azuread_application" "test" { name = "acctestApp-%[1]d" @@ -374,3 +472,20 @@ resource "azuread_group" "test" { } `, id) } + +func testAccAzureADGroupWithServicePrincipalOwner(id int) string { + return fmt.Sprintf(` +resource "azuread_application" "test" { + name = "acctestApp-%[1]d" +} + +resource "azuread_service_principal" "test" { + application_id = azuread_application.test.application_id +} + +resource "azuread_group" "test" { + name = "acctestGroup-%[1]d" + owners = [ azuread_service_principal.test.object_id ] +} +`, id) +} diff --git a/azuread/resource_service_principal_password.go b/azuread/resource_service_principal_password.go index 9c7d57a204..abe8ba5048 100644 --- a/azuread/resource_service_principal_password.go +++ b/azuread/resource_service_principal_password.go @@ -39,8 +39,8 @@ func resourceServicePrincipalPasswordCreate(d *schema.ResourceData, meta interfa } id := graph.PasswordCredentialIdFrom(objectId, *cred.KeyID) - azureADLockByName(servicePrincipalResourceName, id.ObjectId) - defer azureADUnlockByName(servicePrincipalResourceName, id.ObjectId) + tf.LockByName(servicePrincipalResourceName, id.ObjectId) + defer tf.UnlockByName(servicePrincipalResourceName, id.ObjectId) existingCreds, err := client.ListPasswordCredentials(ctx, id.ObjectId) if err != nil { @@ -125,8 +125,8 @@ func resourceServicePrincipalPasswordDelete(d *schema.ResourceData, meta interfa return fmt.Errorf("Error parsing Application Password ID: %v", err) } - azureADLockByName(servicePrincipalResourceName, id.ObjectId) - defer azureADUnlockByName(servicePrincipalResourceName, id.ObjectId) + tf.LockByName(servicePrincipalResourceName, id.ObjectId) + defer tf.UnlockByName(servicePrincipalResourceName, id.ObjectId) // ensure the parent Service Principal exists servicePrincipal, err := client.Get(ctx, id.ObjectId) From fad8579de462474544ddc16c598cd538912ba3e3 Mon Sep 17 00:00:00 2001 From: kt Date: Sun, 21 Jul 2019 20:09:50 -0700 Subject: [PATCH 3/6] updated docs --- website/docs/r/group.markdown | 12 ++++-- website/docs/r/group_member.markdown | 2 +- website/docs/r/group_owner.markdown | 57 ++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 4 deletions(-) create mode 100644 website/docs/r/group_owner.markdown diff --git a/website/docs/r/group.markdown b/website/docs/r/group.markdown index f178629c93..360a3e4fdc 100644 --- a/website/docs/r/group.markdown +++ b/website/docs/r/group.markdown @@ -27,14 +27,15 @@ resource "azuread_group" "my_group" { ```hcl resource "azuread_user" "my_user" { - display_name = "John Doe" + display_name = "J Doe" password = "notSecure123" - user_principal_name = "john.doe@terraform.onmicrosoft.com" + user_principal_name = "j.doe@terraform.onmicrosoft.com" } resource "azuread_group" "my_group" { - name = "MyGroup" + name = "MyGroup" members = [ azuread_user.my_user.object_id /*, more users */ ] + owners = [ azuread_user.my_user.object_id /*, more users */ ] } ``` @@ -44,11 +45,14 @@ 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. +* `owners` (Optional) A set of owners who own this Group. Supported Object types are Users 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. +-> **NOTE:** Do not use `azuread_group_owner` at the same time as the `owners` argument. + ## Attributes Reference The following attributes are exported: @@ -59,6 +63,8 @@ The following attributes are exported: * `members` - The Members of the Group. +* `owners` - The Members of the Group. + ## Import Azure Active Directory Groups can be imported using the `object id`, e.g. diff --git a/website/docs/r/group_member.markdown b/website/docs/r/group_member.markdown index 48ddd11f98..1d87487072 100644 --- a/website/docs/r/group_member.markdown +++ b/website/docs/r/group_member.markdown @@ -18,7 +18,7 @@ Manages a single Group Membership within Azure Active Directory. ```hcl data "azuread_user" "my_user" { - user_principal_name = "johndoe@hashicorp.com" + user_principal_name = "jdoe@hashicorp.com" } resource "azuread_group" "my_group" { diff --git a/website/docs/r/group_owner.markdown b/website/docs/r/group_owner.markdown new file mode 100644 index 0000000000..543d0dec99 --- /dev/null +++ b/website/docs/r/group_owner.markdown @@ -0,0 +1,57 @@ +--- +layout: "azuread" +page_title: "Azure Active Directory: azuread_group_owner" +sidebar_current: "docs-azuread-resource-azuread-group-owner" +description: |- + Manages a single Group Ownership within Azure Active Directory. + +--- + +# azuread_group_owner + +Manages a single Group Ownership within Azure Active Directory. + +-> **NOTE:** Do not use this resource at the same time as `azuread_group.owners`. + +## Example Usage + +```hcl + +data "azuread_user" "my_user" { + user_principal_name = "jdoe@hashicorp.com" +} + +resource "azuread_group" "my_group" { + name = "my_group" +} + +resource "azuread_group_owner" "test" { + group_object_id = "${azuread_group.my_group.id}" + owner_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 Owner to. Changing this forces a new resource to be created. +* `owner_object_id` - (Required) The Object ID of the Azure AD Object you want to add as a Owner to the Group. Supported Object types are Users or Service Principals. Changing this forces a new resource to be created. + +-> **NOTE:** The Owner object has to be present in your Azure Active Directory, either as a Owner or a Guest. + +## Attributes Reference + +The following attributes are exported: + +* `id` - The ID of the Azure AD Group Owner. + +## Import + +Azure Active Directory Group Owners can be imported using the `object id`, e.g. + +```shell +terraform import azuread_group_owner.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 Owner Object ID in the format `{GroupObjectID}/{OwnerObjectID}`. From b44597a93294e134cf5815ab499cd4a0ae8ca9a8 Mon Sep 17 00:00:00 2001 From: kt Date: Sun, 21 Jul 2019 20:28:42 -0700 Subject: [PATCH 4/6] update test --- azuread/helpers/validate/strings_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/azuread/helpers/validate/strings_test.go b/azuread/helpers/validate/strings_test.go index 0827dae7a5..5d6a51627a 100644 --- a/azuread/helpers/validate/strings_test.go +++ b/azuread/helpers/validate/strings_test.go @@ -105,17 +105,17 @@ func TestStringIsEmailAddress(t *testing.T) { ErrCount int }{ { - Value: "john.doe@hashicorp.com", + Value: "j.doe@hashicorp.com", TestName: "Valid_EmailAddress", ErrCount: 0, }, { - Value: "john.doehashicorp.com", + Value: "j.doehashicorp.com", TestName: "Invalid_EmailAddress_NoAtChar", ErrCount: 1, }, { - Value: "john/doe@ha$hicorp.com", + Value: "j/doe@ha$hicorp.com", TestName: "Invalid_EmailAddress_InvalidChars", ErrCount: 1, }, From 325f5c8a39a7ebe4397a1870294eae95d9ed3d48 Mon Sep 17 00:00:00 2001 From: kt Date: Tue, 23 Jul 2019 08:13:47 -0700 Subject: [PATCH 5/6] removed azuread_group_owner resource --- azuread/helpers/graph/group.go | 27 ---- azuread/provider.go | 1 - azuread/resource_group_owner.go | 114 --------------- azuread/resource_group_owner_test.go | 206 --------------------------- website/docs/r/group_owner.markdown | 57 -------- 5 files changed, 405 deletions(-) delete mode 100644 azuread/resource_group_owner.go delete mode 100644 azuread/resource_group_owner_test.go delete mode 100644 website/docs/r/group_owner.markdown diff --git a/azuread/helpers/graph/group.go b/azuread/helpers/graph/group.go index db06b472d5..abbe90a97a 100644 --- a/azuread/helpers/graph/group.go +++ b/azuread/helpers/graph/group.go @@ -8,33 +8,6 @@ import ( "github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac" ) -type GroupOwnerId struct { - ObjectSubResourceId - GroupId string - OwnerId string -} - -func GroupOwnerIdFrom(groupId, ownerId string) GroupOwnerId { - return GroupOwnerId{ - ObjectSubResourceId: ObjectSubResourceIdFrom(groupId, "owner", ownerId), - GroupId: groupId, - OwnerId: ownerId, - } -} - -func ParseGroupOwnerId(idString string) (GroupOwnerId, error) { - id, err := ParseObjectSubResourceId(idString, "owner") - if err != nil { - return GroupOwnerId{}, fmt.Errorf("Unable to parse Owner ID: %v", err) - } - - return GroupOwnerId{ - ObjectSubResourceId: id, - GroupId: id.objectId, - OwnerId: id.subId, - }, nil -} - type GroupMemberId struct { ObjectSubResourceId GroupId string diff --git a/azuread/provider.go b/azuread/provider.go index 14bf9a05ee..534670a9f0 100644 --- a/azuread/provider.go +++ b/azuread/provider.go @@ -84,7 +84,6 @@ func Provider() terraform.ResourceProvider { "azuread_application_password": resourceApplicationPassword(), "azuread_group": resourceGroup(), "azuread_group_member": resourceGroupMember(), - "azuread_group_owner": resourceGroupOwner(), "azuread_service_principal": resourceServicePrincipal(), "azuread_service_principal_password": resourceServicePrincipalPassword(), "azuread_user": resourceUser(), diff --git a/azuread/resource_group_owner.go b/azuread/resource_group_owner.go deleted file mode 100644 index 9c42a83c1e..0000000000 --- a/azuread/resource_group_owner.go +++ /dev/null @@ -1,114 +0,0 @@ -package azuread - -import ( - "fmt" - - "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/graph" - "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/tf" - - "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" -) - -const groupOwnberResourceName = "azuread_group_owner" - -func resourceGroupOwner() *schema.Resource { - return &schema.Resource{ - Create: resourceGroupOwnerCreate, - Read: resourceGroupOwnerRead, - Delete: resourceGroupOwnerDelete, - - Importer: &schema.ResourceImporter{ - State: schema.ImportStatePassthrough, - }, - - Schema: map[string]*schema.Schema{ - "group_object_id": { - Type: schema.TypeString, - Required: true, - ForceNew: true, - ValidateFunc: validate.UUID, - }, - - "owner_object_id": { - Type: schema.TypeString, - Required: true, - ForceNew: true, - ValidateFunc: validate.UUID, - }, - }, - } -} - -func resourceGroupOwnerCreate(d *schema.ResourceData, meta interface{}) error { - client := meta.(*ArmClient).groupsClient - ctx := meta.(*ArmClient).StopContext - - groupID := d.Get("group_object_id").(string) - ownerID := d.Get("owner_object_id").(string) - - tf.LockByName(groupOwnberResourceName, groupID) - defer tf.UnlockByName(groupOwnberResourceName, groupID) - - if err := graph.GroupAddOwner(client, ctx, groupID, ownerID); err != nil { - return err - } - - d.SetId(graph.GroupOwnerIdFrom(groupID, ownerID).String()) - return resourceGroupOwnerRead(d, meta) -} - -func resourceGroupOwnerRead(d *schema.ResourceData, meta interface{}) error { - client := meta.(*ArmClient).groupsClient - ctx := meta.(*ArmClient).StopContext - - id, err := graph.ParseGroupOwnerId(d.Id()) - if err != nil { - return fmt.Errorf("Unable to parse ID: %v", err) - } - - owners, err := graph.GroupAllOwners(client, ctx, id.GroupId) - if err != nil { - return fmt.Errorf("Error retrieving Azure AD Group owners (groupObjectId: %q): %+v", id.GroupId, err) - } - - var ownerObjectID string - for _, objectID := range owners { - if objectID == id.OwnerId { - ownerObjectID = objectID - } - } - - if ownerObjectID == "" { - d.SetId("") - return fmt.Errorf("Azure AD Group Owner not found - groupObjectId:%q / ownerObjectId:%q", id.GroupId, id.OwnerId) - } - - d.Set("group_object_id", id.GroupId) - d.Set("owner_object_id", id.OwnerId) - - return nil -} - -func resourceGroupOwnerDelete(d *schema.ResourceData, meta interface{}) error { - client := meta.(*ArmClient).groupsClient - ctx := meta.(*ArmClient).StopContext - - id, err := graph.ParseGroupOwnerId(d.Id()) - if err != nil { - return fmt.Errorf("Unable to parse ID: %v", err) - } - - tf.LockByName(groupOwnberResourceName, id.GroupId) - defer tf.UnlockByName(groupOwnberResourceName, id.GroupId) - - resp, err := client.RemoveOwner(ctx, id.GroupId, id.OwnerId) - if err != nil { - if !ar.ResponseWasNotFound(resp) { - return fmt.Errorf("Error removing Owner (ownerObjectId: %q) from Azure AD Group (groupObjectId: %q): %+v", id.OwnerId, id.GroupId, err) - } - } - - return nil -} diff --git a/azuread/resource_group_owner_test.go b/azuread/resource_group_owner_test.go deleted file mode 100644 index 7ceb2a29c4..0000000000 --- a/azuread/resource_group_owner_test.go +++ /dev/null @@ -1,206 +0,0 @@ -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" - "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/graph" - "github.com/terraform-providers/terraform-provider-azuread/azuread/helpers/tf" -) - -func TestAccAzureADGroupOwner_user(t *testing.T) { - rna := "azuread_group_owner.testA" - rnb := "azuread_group_owner.testB" - id := tf.AccRandTimeInt() - pw := "p@$$wR2" + acctest.RandStringFromCharSet(7, acctest.CharSetAlphaNum) - - resource.ParallelTest(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, - Providers: testAccProviders, - CheckDestroy: testCheckAzureADGroupOwnerDestroy(rna), - Steps: []resource.TestStep{ - { - Config: testAccAzureADGroupOwner_oneUser(id, pw), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrSet(rna, "group_object_id"), - resource.TestCheckResourceAttrSet(rna, "owner_object_id"), - ), - }, - { - Config: testAccAzureADGroupOwner_twoUsers(id, pw), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrSet(rna, "group_object_id"), - resource.TestCheckResourceAttrSet(rna, "owner_object_id"), - resource.TestCheckResourceAttrSet(rnb, "group_object_id"), - resource.TestCheckResourceAttrSet(rnb, "owner_object_id"), - ), - }, - // we rerun the config so the group resource updates with the number of members - { - Config: testAccAzureADGroupOwner_twoUsers(id, pw), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("azuread_group.test", "owners.#", "2"), - ), - }, - { - ResourceName: rnb, - ImportState: true, - ImportStateVerify: true, - }, - { - Config: testAccAzureADGroupOwner_oneUser(id, pw), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrSet(rna, "group_object_id"), - resource.TestCheckResourceAttrSet(rna, "owner_object_id"), - ), - }, - { - Config: testAccAzureADGroupOwner_twoUsers(id, pw), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("azuread_group.test", "owners.#", "1"), - ), - }, - { - ResourceName: rna, - ImportState: true, - ImportStateVerify: true, - }, - }, - }) -} - -func TestAccAzureADGroupOwner_ServicePrincipal(t *testing.T) { - rn := "azuread_group_owner.test" - id := tf.AccRandTimeInt() - - resource.ParallelTest(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, - Providers: testAccProviders, - CheckDestroy: testCheckAzureADGroupOwnerDestroy(rn), - Steps: []resource.TestStep{ - { - Config: testAccAzureADGroupOwner_ServicePrincipal(id), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrSet(rn, "group_object_id"), - resource.TestCheckResourceAttrSet(rn, "owner_object_id"), - ), - }, - { - ResourceName: rn, - ImportState: true, - ImportStateVerify: true, - }, - }, - }) -} - -func testCheckAzureADGroupOwnerDestroy(ignoreResource string) func(s *terraform.State) error { - return func(s *terraform.State) error { - for _, rs := range s.RootModule().Resources { - if rs.Type != "azuread_group_owner" { - continue - } - - if rs.Primary.ID == ignoreResource { - continue // there always has to be one owner remaining so lets ignore it - } - - client := testAccProvider.Meta().(*ArmClient).groupsClient - ctx := testAccProvider.Meta().(*ArmClient).StopContext - - groupID := rs.Primary.Attributes["group_object_id"] - ownerID := rs.Primary.Attributes["owner_object_id"] - - // see if group exists - if resp, err := client.Get(ctx, groupID); err != nil { - if ar.ResponseWasNotFound(resp.Response) { - continue - } - - return fmt.Errorf("Error retrieving Azure AD Group with ID %q: %+v", groupID, err) - } - - owners, err := graph.GroupAllOwners(client, ctx, groupID) - if err != nil { - return fmt.Errorf("Error retrieving Azure AD Group owners (groupObjectId: %q): %+v", groupID, err) - } - - var ownerObjectID string - for _, objectID := range owners { - if objectID == ownerID { - ownerObjectID = objectID - } - } - - if ownerObjectID != "" { - return fmt.Errorf("Azure AD group owner still exists:\n%#v", ownerObjectID) - } - } - - return nil - } -} - -func testAccAzureADGroupOwner_oneUser(id int, password string) string { - return fmt.Sprintf(` -%[1]s - -resource "azuread_group" "test" { - name = "acctestGroup-%[2]d" -} - -resource "azuread_group_owner" "testA" { - group_object_id = "${azuread_group.test.object_id}" - owner_object_id = "${azuread_user.testA.object_id}" -} - -`, testAccADUser_threeUsersABC(id, password), id) -} - -func testAccAzureADGroupOwner_twoUsers(id int, password string) string { - return fmt.Sprintf(` -%[1]s - -resource "azuread_group" "test" { - name = "acctestGroup-%[2]d" -} - -resource "azuread_group_owner" "testA" { - group_object_id = "${azuread_group.test.object_id}" - owner_object_id = "${azuread_user.testA.object_id}" -} - -resource "azuread_group_owner" "testB" { - group_object_id = "${azuread_group.test.object_id}" - owner_object_id = "${azuread_user.testB.object_id}" -} - -`, testAccADUser_threeUsersABC(id, password), id) -} - -func testAccAzureADGroupOwner_ServicePrincipal(id int) string { - return fmt.Sprintf(` - -resource "azuread_application" "test" { - name = "acctestApp-%[1]d" -} - -resource "azuread_service_principal" "test" { - application_id = "${azuread_application.test.application_id}" -} - -resource "azuread_group" "test" { - name = "acctestGroup-%[1]d" -} - -resource "azuread_group_owner" "test" { - group_object_id = "${azuread_group.test.object_id}" - owner_object_id = "${azuread_service_principal.test.object_id}" -} - -`, id) -} diff --git a/website/docs/r/group_owner.markdown b/website/docs/r/group_owner.markdown deleted file mode 100644 index 543d0dec99..0000000000 --- a/website/docs/r/group_owner.markdown +++ /dev/null @@ -1,57 +0,0 @@ ---- -layout: "azuread" -page_title: "Azure Active Directory: azuread_group_owner" -sidebar_current: "docs-azuread-resource-azuread-group-owner" -description: |- - Manages a single Group Ownership within Azure Active Directory. - ---- - -# azuread_group_owner - -Manages a single Group Ownership within Azure Active Directory. - --> **NOTE:** Do not use this resource at the same time as `azuread_group.owners`. - -## Example Usage - -```hcl - -data "azuread_user" "my_user" { - user_principal_name = "jdoe@hashicorp.com" -} - -resource "azuread_group" "my_group" { - name = "my_group" -} - -resource "azuread_group_owner" "test" { - group_object_id = "${azuread_group.my_group.id}" - owner_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 Owner to. Changing this forces a new resource to be created. -* `owner_object_id` - (Required) The Object ID of the Azure AD Object you want to add as a Owner to the Group. Supported Object types are Users or Service Principals. Changing this forces a new resource to be created. - --> **NOTE:** The Owner object has to be present in your Azure Active Directory, either as a Owner or a Guest. - -## Attributes Reference - -The following attributes are exported: - -* `id` - The ID of the Azure AD Group Owner. - -## Import - -Azure Active Directory Group Owners can be imported using the `object id`, e.g. - -```shell -terraform import azuread_group_owner.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 Owner Object ID in the format `{GroupObjectID}/{OwnerObjectID}`. From 33e920d944c7921624048f848ede4662db81021a Mon Sep 17 00:00:00 2001 From: kt Date: Tue, 23 Jul 2019 08:39:51 -0700 Subject: [PATCH 6/6] comment typo --- azuread/resource_group.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/azuread/resource_group.go b/azuread/resource_group.go index 7acfd68b3a..c155363444 100644 --- a/azuread/resource_group.go +++ b/azuread/resource_group.go @@ -93,7 +93,7 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error { if v, ok := d.GetOk("members"); ok { members := tf.ExpandStringSlicePtr(v.(*schema.Set).List()) - // we could lock here against the group ember resource, but the should not be used together (todo conflicts with at a resource level?) + // we could lock here against the group member resource, but they should not be used together (todo conflicts with at a resource level?) if err := graph.GroupAddMembers(client, ctx, *group.ObjectID, *members); err != nil { return err } @@ -102,8 +102,6 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error { // Add owners if specified if v, ok := d.GetOk("owners"); ok { members := tf.ExpandStringSlicePtr(v.(*schema.Set).List()) - - // we could lock here against the group owner resource, but the should not be used together (todo conflicts with at a resource level?) if err := graph.GroupAddOwners(client, ctx, *group.ObjectID, *members); err != nil { return err }