Conditional access policy - support persistent browser session control #677

stawik-mesa · 2021-11-24T10:22:12Z

Improvements for azuread_conditional_access:

support the persistent_browser_mode field

manicminer

Hi @stawik-mesa, thanks so much for this contribution! It looks like we missed persistent browser session controls when adding this resource, and it'd be great to add this.

This mostly LGTM, I have a few comments below mainly on style/ordering, and a typo in the test config. Could you also look at adding this property in the sessionControls / sessionControlsDisabled test configs? We have to test these separately at the moment to ensure we have test coverage for an API bug workaround.

If you can take a look at these, and the tests pass, then this should be good to merge. Thanks!

manicminer · 2021-11-24T12:56:11Z

internal/services/conditionalaccess/conditional_access_policy_resource.go

+
+						"persistent_browser_mode": {
+							Type:         schema.TypeString,
+							Optional:     true,
+							ValidateFunc: validation.StringInSlice([]string{"always", "never"}, false),
+						},


This should go inside the session_controls block, e.g. alongside cloud_app_security_policy

internal/services/conditionalaccess/conditional_access_policy_resource_test.go

manicminer · 2021-11-24T12:58:02Z

internal/services/conditionalaccess/conditionalaccess.go

 	return []interface{}{
 		map[string]interface{}{
 			"application_enforced_restrictions_enabled": applicationEnforceRestrictions,
 			"cloud_app_security_policy":                 cloudAppSecurity,
 			"sign_in_frequency":                         signInFrequency,
 			"sign_in_frequency_period":                  signInFrequencyPeriod,
+			"persistent_browser_mode":                   persistentBrowserMode,


Could these be alphabetically ordered?

manicminer · 2021-11-24T12:58:35Z

internal/services/conditionalaccess/conditionalaccess.go

+		result.PersistentBrowser = &msgraph.PersistentBrowserSessionControl{
+			IsEnabled: utils.Bool(false),
+		}


As above, could these be ordered?

manicminer · 2021-11-24T13:01:14Z

docs/resources/conditional_access_policy.md

@@ -148,6 +148,7 @@ The following arguments are supported:
 * `cloud_app_security_policy` - (Optional) Enables cloud app security and specifies the cloud app security policy to use. Possible values are: `blockDownloads`, `mcasConfigured`, `monitorOnly` or `unknownFutureValue`.
 * `sign_in_frequency` - (Optional) Number of days or hours to enforce sign-in frequency. Required when `sign_in_frequency_period` is specified. Due to an API issue, removing this property forces a new resource to be created.
 * `sign_in_frequency_period` - (Optional) The time period to enforce sign-in frequency. Possible values are: `hours` or `days`. Required when `sign_in_frequency_period` is specified. Due to an API issue, removing this property forces a new resource to be created.
+* `persistent_browser_mode` - (Optional) Session control to define whether to persist cookies or not. Possible values are: `always` or `never`.


These will need to be ordered to pass documentation checks :)

Co-authored-by: Tom Bamford <tom@bamford.io>

stawik-mesa · 2021-11-24T17:43:48Z

Hi @stawik-mesa, thanks so much for this contribution! It looks like we missed persistent browser session controls when adding this resource, and it'd be great to add this.

This mostly LGTM, I have a few comments below mainly on style/ordering, and a typo in the test config. Could you also look at adding this property in the sessionControls / sessionControlsDisabled test configs? We have to test these separately at the moment to ensure we have test coverage for an API bug workaround.

If you can take a look at these, and the tests pass, then this should be good to merge. Thanks!

Hi @manicminer,

Thanks for the quick response on my PR. How should I integrate the test?
I haven't seen anything related to the other session control fields? How do you test them?

manicminer · 2021-11-25T12:09:29Z

@stawik-mesa You can run the tests by running make testacc TEST=./internal/services/conditionalaccess, however we're also happy to run them.

When I try to run this and enable persistent browser mode, I'm getting a 400 error from the API:

{
  "error": {
    "code": "BadRequest",
    "message": "1032: ConditionalActionPolicy validation failed due to InvalidConditionsForPersistentBrowserSessionMode.",
    "innerError": {
      "date": "2021-11-25T11:59:20",
      "request-id": "551250b2-b961-44fd-ad83-f6b1cf6b32dc",
      "client-request-id": "551250b2-b961-44fd-ad83-f6b1cf6b32dc"
    }
  }
}

I looked back at earlier PRs when we added Conditional Access support, and I see that persistent browser mode was initially added but removed before the resource was merged. Whilst this wasn't discussed in the original PR, it might be that the API doesn't work for this property.

Since we unfortunately can't merge until setting this property works, I'll dig into it a little further and reach out to the service team to find out why we're getting this error.

stawik-mesa · 2021-11-25T14:53:49Z

is it maybe related to: https://docs.microsoft.com/en-us/answers/questions/502950/conditional-access-exclude-application.html

…r-mode

stawik-mesa · 2021-11-29T07:46:04Z

@manicminer Locally all tests passed

stawik-mesa · 2021-11-29T12:40:01Z

@manicminer I hopefully fixed the format error on the test source code

…for `session_controls`

manicminer · 2021-12-02T13:30:33Z

@stawik-mesa Thanks for resolving the merge! When running the tests I noticed that changes to session_controls were not being picked up and tracked it down to the DiffSuppressFunc (previously added by me!) incorrectly discarding changes to that block, causing it to be omitted from API requests and silently ignored by Terraform, in turn allowing the tests to falsely pass. I believe that's now fixed.

I also merged your additional tests in with the tests for the session_controls block, so that's now tested with the other sibling properties and it appears to be working well.

I'll open this up for review from another contributor and we should be able to get this merged for this week's release. Thanks again!

manicminer · 2021-12-02T14:06:16Z

Test results

katbyte

aside from 1 minor comment LGTM 💠

katbyte · 2021-12-02T21:52:50Z

internal/services/conditionalaccess/conditional_access_policy_resource.go

+						"persistent_browser_mode": {
+							Type:     schema.TypeString,
+							Optional: true,
+							ValidateFunc: validation.StringInSlice([]string{
+								msgraph.PersistentBrowserSessionModeAlways,
+								msgraph.PersistentBrowserSessionModeNever,
+							}, false),
+						},


this seems to be a boolean so could become a bool? maybe

Suggested change

"persistent_browser_mode": {

Type: schema.TypeString,

Optional: true,

ValidateFunc: validation.StringInSlice([]string{

msgraph.PersistentBrowserSessionModeAlways,

msgraph.PersistentBrowserSessionModeNever,

}, false),

},

"persistent_browser_cookies_enabled": {

Type: schema.TypeString,

Optional: true,

ValidateFunc: validation.StringInSlice([]string{

msgraph.PersistentBrowserSessionModeAlways,

msgraph.PersistentBrowserSessionModeNever,

}, false),

},

It's more of a triple-value setting (there's an underlying bool which we infer the value of) so we'll probably have to stick with enum-like strings

github-actions · 2021-12-03T01:06:25Z

This functionality has been released in v2.12.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 · 2022-01-02T02:13:38Z

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.

Conditional access policy - support persitent browser session control

8a39397

github-actions bot added the size/XS label Nov 24, 2021

stawik-mesa changed the title ~~Conditional access policy - support persitent browser session control~~ Conditional access policy - support persistent browser session control Nov 24, 2021

manicminer suggested changes Nov 24, 2021

View reviewed changes

manicminer added enhancement feature/conditional-access waiting-response labels Nov 24, 2021

stawik-mesa and others added 2 commits November 24, 2021 18:36

Apply suggestions from code review

0091613

Co-authored-by: Tom Bamford <tom@bamford.io>

Put properties into correct order

b789c4e

github-actions bot removed the waiting-response label Nov 24, 2021

Merge branch 'main' into feature/conditional-access-persistent-browse…

c1b36f6

…r-mode

stawik-mesa marked this pull request as draft November 29, 2021 06:43

Add test case for persisten browser mode

6829346

github-actions bot added size/M and removed size/XS labels Nov 29, 2021

Fix test case

6003ea8

github-actions bot added size/L and removed size/M labels Nov 29, 2021

stawik-mesa marked this pull request as ready for review November 29, 2021 07:45

stawik-mesa requested a review from manicminer November 29, 2021 07:58

Fix terraform code format

fc013a8

azuread_conditional_access_policy: fix bad DiffSuppress, merge tests …

5621959

…for `session_controls`

github-actions bot added size/M and removed size/L labels Dec 2, 2021

manicminer requested a review from a team December 2, 2021 14:06

manicminer added this to the v2.12.0 milestone Dec 2, 2021

katbyte approved these changes Dec 2, 2021

View reviewed changes

manicminer merged commit 0cd59c7 into hashicorp:main Dec 3, 2021

manicminer added a commit that referenced this pull request Dec 3, 2021

Changelog for #677

f3373f0

github-actions bot locked as resolved and limited conversation to collaborators Jan 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditional access policy - support persistent browser session control #677

Conditional access policy - support persistent browser session control #677

stawik-mesa commented Nov 24, 2021

manicminer left a comment

manicminer Nov 24, 2021

manicminer Nov 24, 2021

manicminer Nov 24, 2021

manicminer Nov 24, 2021

stawik-mesa commented Nov 24, 2021

manicminer commented Nov 25, 2021 •

edited

Loading

stawik-mesa commented Nov 25, 2021 •

edited

Loading

stawik-mesa commented Nov 29, 2021

stawik-mesa commented Nov 29, 2021

manicminer commented Dec 2, 2021

manicminer commented Dec 2, 2021

katbyte left a comment

katbyte Dec 2, 2021

manicminer Dec 3, 2021

github-actions bot commented Dec 3, 2021

github-actions bot commented Jan 2, 2022

Conditional access policy - support persistent browser session control #677

Conditional access policy - support persistent browser session control #677

Conversation

stawik-mesa commented Nov 24, 2021

manicminer left a comment

Choose a reason for hiding this comment

manicminer Nov 24, 2021

Choose a reason for hiding this comment

manicminer Nov 24, 2021

Choose a reason for hiding this comment

manicminer Nov 24, 2021

Choose a reason for hiding this comment

manicminer Nov 24, 2021

Choose a reason for hiding this comment

stawik-mesa commented Nov 24, 2021

manicminer commented Nov 25, 2021 • edited Loading

stawik-mesa commented Nov 25, 2021 • edited Loading

stawik-mesa commented Nov 29, 2021

stawik-mesa commented Nov 29, 2021

manicminer commented Dec 2, 2021

manicminer commented Dec 2, 2021

katbyte left a comment

Choose a reason for hiding this comment

katbyte Dec 2, 2021

Choose a reason for hiding this comment

manicminer Dec 3, 2021

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2021

github-actions bot commented Jan 2, 2022

manicminer commented Nov 25, 2021 •

edited

Loading

stawik-mesa commented Nov 25, 2021 •

edited

Loading