Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/issue 551 azuread_groups security_enabled flag #552

Closed
wants to merge 19 commits into from
Closed

Feature/issue 551 azuread_groups security_enabled flag #552

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a016424
Added initial functionality
Threpio Sep 5, 2021
0f9b8fe
Added tests for security enabled on all groups
Threpio Sep 5, 2021
d98e165
Updated docs and added basic 0 response check
Threpio Sep 5, 2021
a6a9bc2
I will run fmt correctly one day
Threpio Sep 5, 2021
183f275
Final linting?
Threpio Sep 5, 2021
0b84113
Update internal/services/groups/groups_data_source.go
Threpio Sep 6, 2021
85d7176
Update internal/services/groups/groups_data_source.go
Threpio Sep 6, 2021
72bb465
Fmt cleanup
Threpio Sep 6, 2021
81a19e1
Not sure if this code is how you want the logic done. If you'd prefer…
Threpio Sep 8, 2021
3218bcf
Logic extracted to remove duplication
Threpio Sep 8, 2021
02ec9c5
Linting stuff
Threpio Sep 8, 2021
b7bb343
Added basic tests for mail_enabled
Threpio Sep 8, 2021
10e5567
I will fmt correctly one day
Threpio Sep 8, 2021
c58e7d6
Documentation updates
Threpio Sep 8, 2021
ba1ea96
Update docs/data-sources/groups.md
Threpio Sep 9, 2021
4d7d2ae
Update docs/data-sources/groups.md
Threpio Sep 9, 2021
db4f01c
Update internal/services/groups/groups_data_source_test.go
Threpio Sep 9, 2021
cfee55e
Refactored docs to make the arguments alphabetical
Threpio Sep 11, 2021
c1878fc
Updated tests to include both mail and security enabled tests
Threpio Sep 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions docs/data-sources/groups.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,29 @@ data "azuread_groups" "allGroups" {
}
```

*Look up all security groups*
Threpio marked this conversation as resolved.
Show resolved Hide resolved
```terraform
data "azuread_groups" "allGroups" {
return_all = true
security_enabled = true
}
```


## Argument Reference

The following arguments are supported:

* `display_names` - (Optional) The display names of the groups.
* `object_ids` - (Optional) The object IDs of the groups.
* `return_all` - (Optional) A flag to denote if all groups should be fetched and returned.
* `security_enabled` - (Optional) A flag to denote if only `security_enabled=true` groups should be returned.
Threpio marked this conversation as resolved.
Show resolved Hide resolved
* `mail_enabled` - (Optional) A flag to denote if only `mail_enabled=true` groups should be returned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this up a few lines to maintain alphabetic sorting?


~> One of `display_names`, `object_ids` or `return_all` should be specified. Either of the first two _may_ be specified as an empty list, in which case no results will be returned.

~> `security_enabled` and `mail_enabled` flags work with `return_all` and `display_names` but not `object_ids`
Threpio marked this conversation as resolved.
Show resolved Hide resolved

## Attributes Reference

The following attributes are exported:
Expand Down
62 changes: 55 additions & 7 deletions internal/services/groups/groups_data_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,22 @@ func groupsDataSource() *schema.Resource {
Optional: true,
ExactlyOneOf: []string{"display_names", "object_ids", "return_all"},
Threpio marked this conversation as resolved.
Show resolved Hide resolved
},

"security_enabled": {
Description: "Whether the groups are security_enabled",
Type: schema.TypeBool,
Optional: true,
Computed: true,
ConflictsWith: []string{"object_ids"},
},

"mail_enabled": {
Description: "Whether the groups are mail_enabled",
Type: schema.TypeBool,
Optional: true,
Computed: true,
ConflictsWith: []string{"object_ids"},
},
Threpio marked this conversation as resolved.
Show resolved Hide resolved
},
}
}
Expand All @@ -70,6 +86,8 @@ func groupsDataSourceRead(ctx context.Context, d *schema.ResourceData, meta inte
var groups []msgraph.Group
var expectedCount int
var returnAll = d.Get("return_all").(bool)
var securityEnabled = d.Get("security_enabled").(bool)
var mailEnabled = d.Get("mail_enabled").(bool)

var displayNames []interface{}
if v, ok := d.GetOk("display_names"); ok {
Expand All @@ -87,8 +105,7 @@ func groupsDataSourceRead(ctx context.Context, d *schema.ResourceData, meta inte
if len(*result) == 0 {
return tf.ErrorDiagPathF(err, "return_all", "No groups found")
}

groups = append(groups, *result...)
groups = filterResults(securityEnabled, mailEnabled, result)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we compose odata filters for this too, to minimise the number of unrelated results going over the wire? e.g. securityEnabled eq true and/or mailEnabled eq true

Copy link
Contributor Author

@Threpio Threpio Sep 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be being stupid and not understanding odata querying - But would this not lead to something like:

if securityEnabled && mailEnabled{
  query := odata.Query{
	  Filter: fmt.Sprintf("displayName eq '%s', securityEnabled eq true, mailEnabled eq true", displayName),
  }
} else if securityEnabled {
  query := odata.Query{
	  Filter: fmt.Sprintf("displayName eq '%s', securityEnabled eq true", displayName),
  }
} else if mailEnabled {
  query := odata.Query{
	  Filter: fmt.Sprintf("displayName eq '%s', mailEnabled eq true", displayName),
  }
} else {
  query := odata.Query{
	  Filter: fmt.Sprintf("displayName eq '%s'", displayName),
  }
}```
Or is there a way to construct the queries that will omit what we are not querying after? (I do 100% agree about the extra results and such)

} else if len(displayNames) > 0 {
expectedCount = len(displayNames)
for _, v := range displayNames {
Expand All @@ -102,13 +119,15 @@ func groupsDataSourceRead(ctx context.Context, d *schema.ResourceData, meta inte
}

count := len(*result)
if count > 1 {

//Could we make this a switch case?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If think this is fine as an if-else block for now. Maybe one more case would make it eligible IMO

if !mailEnabled && !securityEnabled && count > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we use odata filters, I think this would be better left as if count > 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - Using server side filtering would allow that to return correctly.

If we search for a group with displayname xyz-2 but there is a group xyz-23 does it return both groups or just the one with the exact matching display name? (I assume this is Hamilton and their server doing the clever querying.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several operators and functions. eq is a whole value check. there's also a startsWith function which does what you'd expect (docs)

return tf.ErrorDiagPathF(err, "display_names", "More than one group found with display name: %q", displayName)
} else if count == 0 {
return tf.ErrorDiagPathF(err, "display_names", "No group found with display name: %q", displayName)
} else {
groups = filterResults(securityEnabled, mailEnabled, result)
}

groups = append(groups, (*result)[0])
}
} else if objectIds, ok := d.Get("object_ids").([]interface{}); ok && len(objectIds) > 0 {
expectedCount = len(objectIds)
Expand All @@ -126,7 +145,7 @@ func groupsDataSourceRead(ctx context.Context, d *schema.ResourceData, meta inte
}
}

if !returnAll && len(groups) != expectedCount {
if !returnAll && !securityEnabled && !mailEnabled && len(groups) != expectedCount {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, with server-side filtering, this can be left as-is

return tf.ErrorDiagF(fmt.Errorf("Expected: %d, Actual: %d", expectedCount, len(groups)), "Unexpected number of groups returned")
}

Expand All @@ -139,10 +158,13 @@ func groupsDataSourceRead(ctx context.Context, d *schema.ResourceData, meta inte
if group.DisplayName == nil {
return tf.ErrorDiagF(errors.New("API returned group with nil displayName"), "Bad API response")
}

newObjectIds = append(newObjectIds, *group.ID)
newDisplayNames = append(newDisplayNames, *group.DisplayName)
}
// Check if securityEnabled/mailEnabled has caused the number of returned groups to be 0
if len(newObjectIds) == 0 && (securityEnabled || mailEnabled) {
return tf.ErrorDiagF(errors.New("No groups found with a filter flag applied"), "Unexpected Number of groups returned")
}
Comment on lines +164 to +167
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what scenario this is intended to catch as this would already have errored in the main if-else block above?


h := sha1.New()
if _, err := h.Write([]byte(strings.Join(newDisplayNames, "-"))); err != nil {
Expand All @@ -156,3 +178,29 @@ func groupsDataSourceRead(ctx context.Context, d *schema.ResourceData, meta inte

return nil
}

// Extracting the flag functionality to prevent duplication
func filterResults(securityEnabled, mailEnabled bool, results *[]msgraph.Group) (groups []msgraph.Group) {
if securityEnabled && !mailEnabled {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need a nil check for results here or we'll get a crash when dereferencing

for _, r := range *results {
if *r.SecurityEnabled {
groups = append(groups, r)
}
}
} else if mailEnabled && !securityEnabled {
for _, r := range *results {
if *r.MailEnabled {
groups = append(groups, r)
}
}
} else if mailEnabled && securityEnabled {
for _, r := range *results {
if *r.MailEnabled && *r.SecurityEnabled {
groups = append(groups, r)
}
}
} else {
groups = append(groups, *results...)
}
return groups
}
46 changes: 46 additions & 0 deletions internal/services/groups/groups_data_source_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,34 @@ func TestAccGroupsDataSource_returnAll(t *testing.T) {
})
}

func TestAccGroupsDataSource_securityEnabled(t *testing.T) {
data := acceptance.BuildTestData(t, "data.azuread_groups", "test")

data.DataSourceTest(t, []resource.TestStep{
{
Config: GroupsDataSource{}.securityEnabled(),
Check: resource.ComposeTestCheckFunc(
check.That(data.ResourceName).Key("display_names.#").Exists(),
check.That(data.ResourceName).Key("object_ids.#").Exists(),
),
},
})
}

func TestAccGroupsDataSource_mailEnabled(t *testing.T) {
data := acceptance.BuildTestData(t, "data.azuread_groups", "test")

data.DataSourceTest(t, []resource.TestStep{
{
Config: GroupsDataSource{}.mailEnabled(),
Check: resource.ComposeTestCheckFunc(
check.That(data.ResourceName).Key("display_names.#").Exists(),
check.That(data.ResourceName).Key("object_ids.#").Exists(),
),
},
})
}
Comment on lines +73 to +99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are good, can we also add a test for security_enabled + mail_enabled to make sure these work together as expected?


func (GroupsDataSource) template(data acceptance.TestData) string {
return fmt.Sprintf(`
resource "azuread_group" "testA" {
Expand Down Expand Up @@ -121,3 +149,21 @@ data "azuread_groups" "test" {
}
`
}

func (GroupsDataSource) securityEnabled() string {
return `
data "azuread_groups" "test" {
return_all = true
security_enabled = true
Comment on lines +170 to +171
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be mutually exclusive since return_all implies all groups? Alternatively we could update the docs to reflect that return_all excludes display_names and object_ids but can be used with security_enabled to return all security-enabled groups.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with the alternative. Otherwise we can look to apply a number of filters for other fields at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me!

}
`
}

func (GroupsDataSource) mailEnabled() string {
return `
data "azuread_groups" "test" {
return_all = true
main_enabled = true
Threpio marked this conversation as resolved.
Show resolved Hide resolved
}
`
}