Feature/issue 551 azuread_groups security_enabled flag #552

Threpio · 2021-09-05T19:03:49Z

Fixes #551

Added a security_enabled=true flag to azuread_groups datasource. It currently does not implement any logic when security_enabled=false which some people might want in the future.

This checks to ensure that atleast 1 result is returned - If one non security_enabled group is found but then dropped then an error suggesting the flag will be returned.

The test functionality for this and the return_all flag needs to be revisited by someone that understands this testing framwork more than me. There are single cases

manicminer

Hi @Threpio, thanks for this PR, this is off to a great start!

If we're adding a security_enabled flag, could we also add a mail_enabled flag? M365 groups can be secuyrity-enabled so it's probably desired to be able to specify both security_enabled = true and mail_enabled = false.

For the implementation, I think it would be better to first build up a filter based on security_enabled (and mail_enabled) and include that in the main if-else block. We'd also need to explicitly check the results since the filter is useful for server-side filtering but we also need client-side checks.

docs/data-sources/groups.md

internal/services/groups/groups_data_source.go

manicminer · 2021-09-06T09:31:40Z

internal/services/groups/groups_data_source.go

+			"security_enabled": {
+				Description: "Retrieve only groups that are security_enabled groups",
+				Type:        schema.TypeBool,
+				Optional:    true,


I think this should be mutually exclusive with the return_all property?

Suggested change

Optional: true,

Optional: true,

ConflictsWith: []string{"return_all"},

I think it should be possible to query for all groups that are security_enabled ?

I know it is a complication with regards to logic but the ability to just filter out 365 groups could be a useful tool.

(worth noting that I doubt I will be using the functionality defined here so if there is an end-user feel free to override me)

Yeah the more I've thought about it, I agree it should be possible to specify return_all and filter out mail and security groups.

Filter-out or filter in?

Sorry filter in, as in I should have written "filter for" :D

I suppose what we should think about aswell is the oppertunity for future work - Perhaps people will want to specify mail_enabled=false and only get non-mail_enabled groups

Right, that's sort of what I was trying to get at with the inclusion of both properties. Someone may want to specify security_enabled=true and mail_enabled=false to get security groups that aren't mail enabled (e.g. effectively only get classic security groups and not potential M365 groups)

(You should be able to use d.GetOk() to test if the property was specified versus just having it's default (unset) value)

internal/services/groups/groups_data_source.go

manicminer · 2021-09-06T09:36:09Z

internal/services/groups/groups_data_source_test.go

+  return_all       = true
+  security_enabled = true


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.

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

Sounds good to me!

internal/services/groups/groups_data_source.go

Co-authored-by: Tom Bamford <[email protected]>

Threpio · 2021-09-06T09:58:13Z

Would you like me to do the mail_enabled flag logic in this PR aswell or to have it's own?

Also I have noted that I have missed the documentation for logic of the conflict between specified object_ids and this flag - I am right in thinking they would conflict right?

Co-authored-by: Tom Bamford <[email protected]>

manicminer · 2021-09-06T10:58:22Z

Would you like me to do the mail_enabled flag logic in this PR aswell or to have it's own?

In this PR would be great since they are very related - and noting that M365 groups can be security enabled, having the mail_enabled property will be important.

It would be good to also add the types property to this and the singular azuread_group data sources (with similar logic to how the supports_services field works in the azuread_domains DS), but that can wait for another PR.

Also I have noted that I have missed the documentation for logic of the conflict between specified object_ids and this flag - I am right in thinking they would conflict right?

Correct, object_ids would be exclusive to security_enabled/mail_enabled, but display_names will be complementary/compatible.

… the logic be extracted then let me know

Threpio · 2021-09-08T08:39:26Z

Just to check - is mail_enabled and security_enabled conflict? Can a group retain both of these flags to true

manicminer · 2021-09-08T08:45:31Z

Just to check - is mail_enabled and security_enabled conflict? Can a group retain both of these flags to true

A group (depending on its type) can be either mail_enabled / security_enabled or both.

manicminer

@Threpio Thanks for the updates, this is looking mostly good. Just a few comments on the implementation, mainly the use of odata filters to get server-side filtering (we'll still keep the client-side filtering too). If you can take a look at these, this should be good to merge!

manicminer · 2021-09-09T10:15:52Z

internal/services/groups/groups_data_source.go

@@ -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)


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

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)

manicminer · 2021-09-09T10:16:47Z

internal/services/groups/groups_data_source.go

@@ -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?


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

manicminer · 2021-09-09T10:19:39Z

internal/services/groups/groups_data_source.go

-			if count > 1 {
+
+			//Could we make this a switch case?
+			if !mailEnabled && !securityEnabled && count > 1 {


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

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.

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)

manicminer · 2021-09-09T10:20:02Z

internal/services/groups/groups_data_source.go

@@ -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 {


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

manicminer · 2021-09-09T10:22:11Z

internal/services/groups/groups_data_source.go

+	// 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")
+	}


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

manicminer · 2021-09-09T10:33:08Z

internal/services/groups/groups_data_source_test.go

+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(),
+			),
+		},
+	})
+}


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

internal/services/groups/groups_data_source_test.go

docs/data-sources/groups.md

manicminer · 2021-09-09T10:34:58Z

docs/data-sources/groups.md

 ## 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.
+* `mail_enabled` - (Optional) A flag to denote if only `mail_enabled=true` groups should be returned.


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

docs/data-sources/groups.md

Co-authored-by: Tom Bamford <[email protected]>

Threpio · 2021-09-13T11:03:27Z

Sorry @manicminer - My priorities at work have been changed and I will no longer be able to finish up this PR. Do you want me to close it or?

manicminer · 2021-09-13T11:54:40Z

@Threpio No problem, thanks for your work on it and for letting us know. Go ahead and close it, I'm sure it will be picked up again at some point :)

github-actions · 2021-09-30T20:56:50Z

This functionality has been released in v2.5.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2021-10-31T02:08:52Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Threpio added 3 commits September 5, 2021 20:45

Added initial functionality

a016424

Added tests for security enabled on all groups

0f9b8fe

Updated docs and added basic 0 response check

d98e165

Threpio changed the title ~~Feature/issue 551 security groups~~ Feature/issue 551 azuread_groups security_enabled flag Sep 5, 2021

github-actions bot added the size/S label Sep 5, 2021

Threpio added 2 commits September 5, 2021 21:11

I will run fmt correctly one day

a6a9bc2

Final linting?

183f275

manicminer suggested changes Sep 6, 2021

View reviewed changes

manicminer added enhancement feature/groups waiting-response labels Sep 6, 2021

manicminer added this to the v2.2.0 milestone Sep 6, 2021

Update internal/services/groups/groups_data_source.go

0b84113

Co-authored-by: Tom Bamford <[email protected]>

github-actions bot removed the waiting-response label Sep 6, 2021

Threpio and others added 2 commits September 6, 2021 12:01

Update internal/services/groups/groups_data_source.go

85d7176

Co-authored-by: Tom Bamford <[email protected]>

Fmt cleanup

72bb465

manicminer added the waiting-response label Sep 7, 2021

Not sure if this code is how you want the logic done. If you'd prefer…

81a19e1

… the logic be extracted then let me know

github-actions bot added size/M and removed size/S labels Sep 8, 2021

github-actions bot removed the waiting-response label Sep 8, 2021

Threpio added 5 commits September 8, 2021 11:13

Logic extracted to remove duplication

3218bcf

Linting stuff

02ec9c5

Added basic tests for mail_enabled

b7bb343

I will fmt correctly one day

10e5567

Documentation updates

c58e7d6

manicminer suggested changes Sep 9, 2021

View reviewed changes

Threpio and others added 3 commits September 9, 2021 12:54

Update docs/data-sources/groups.md

ba1ea96

Co-authored-by: Tom Bamford <[email protected]>

Update docs/data-sources/groups.md

4d7d2ae

Co-authored-by: Tom Bamford <[email protected]>

Update internal/services/groups/groups_data_source_test.go

db4f01c

Co-authored-by: Tom Bamford <[email protected]>

manicminer modified the milestones: v2.2.0, v2.3.0 Sep 9, 2021

Threpio added 2 commits September 11, 2021 06:25

Refactored docs to make the arguments alphabetical

cfee55e

Updated tests to include both mail and security enabled tests

c1878fc

Threpio closed this Sep 14, 2021

Threpio deleted the feature/issue-551-security_groups branch September 18, 2021 14:43

manicminer modified the milestones: v2.3.0, v2.5.0 Sep 21, 2021

manicminer mentioned this pull request Sep 28, 2021

data.azuread_groups: support the mail_enabled and security_enabled properties #603

Merged

hashicorp deleted a comment from github-actions bot Sep 30, 2021

github-actions bot locked as resolved and limited conversation to collaborators Oct 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/issue 551 azuread_groups security_enabled flag #552

Feature/issue 551 azuread_groups security_enabled flag #552

Threpio commented Sep 5, 2021

manicminer left a comment

manicminer Sep 6, 2021

Threpio Sep 6, 2021 •

edited

Loading

manicminer Sep 6, 2021

Threpio Sep 8, 2021

manicminer Sep 9, 2021

Threpio Sep 9, 2021

manicminer Sep 9, 2021

manicminer Sep 9, 2021

manicminer Sep 6, 2021

Threpio Sep 8, 2021

manicminer Sep 9, 2021

Threpio commented Sep 6, 2021 •

edited

Loading

manicminer commented Sep 6, 2021

Threpio commented Sep 8, 2021

manicminer commented Sep 8, 2021

manicminer left a comment

manicminer Sep 9, 2021

Threpio Sep 9, 2021 •

edited

Loading

manicminer Sep 9, 2021

manicminer Sep 9, 2021

Threpio Sep 9, 2021

manicminer Sep 9, 2021

manicminer Sep 9, 2021

manicminer Sep 9, 2021

manicminer Sep 9, 2021

manicminer Sep 9, 2021

Threpio commented Sep 13, 2021

manicminer commented Sep 13, 2021

github-actions bot commented Sep 30, 2021

github-actions bot commented Oct 31, 2021

	Optional: true,
	Optional: true,
	ConflictsWith: []string{"return_all"},

Feature/issue 551 azuread_groups security_enabled flag #552

Feature/issue 551 azuread_groups security_enabled flag #552

Conversation

Threpio commented Sep 5, 2021

manicminer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Threpio Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Threpio commented Sep 6, 2021 • edited Loading

manicminer commented Sep 6, 2021

Threpio commented Sep 8, 2021

manicminer commented Sep 8, 2021

manicminer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Threpio Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Threpio commented Sep 13, 2021

manicminer commented Sep 13, 2021

github-actions bot commented Sep 30, 2021

github-actions bot commented Oct 31, 2021

Threpio Sep 6, 2021 •

edited

Loading

Threpio commented Sep 6, 2021 •

edited

Loading

Threpio Sep 9, 2021 •

edited

Loading