Merge pull request #1382 from hashicorp/bugfix/conditional-access-ses…

…sion-controls

azuread_conditional_access_policy: improve handling of the `session_controls` block

go.mod

@@ -9,7 +9,7 @@ require (
 	github.com/hashicorp/go-uuid v1.0.3
 	github.com/hashicorp/terraform-plugin-sdk/v2 v2.29.0
 	github.com/hashicorp/terraform-plugin-testing v1.5.1
-	github.com/manicminer/hamilton v0.67.0
+	github.com/manicminer/hamilton v0.68.0
 	golang.org/x/text v0.14.0
 )
 

go.sum

@@ -111,8 +111,8 @@ github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc=
 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw=
-github.com/manicminer/hamilton v0.67.0 h1:hG3tPunQCGcgP2Nx0+lwW+Swu9MXOs4JGospakK79pY=
-github.com/manicminer/hamilton v0.67.0/go.mod h1:u80g9rPtJpCG7EC0iayttt8UfeAp6jknClixgZGE950=
+github.com/manicminer/hamilton v0.68.0 h1:2ZKQRTegktxJIoLBWxI43HczpTiwXl6brvwTXy75gPk=
+github.com/manicminer/hamilton v0.68.0/go.mod h1:u80g9rPtJpCG7EC0iayttt8UfeAp6jknClixgZGE950=
 github.com/mattn/go-colorable v0.1.9/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc=
 github.com/mattn/go-colorable v0.1.12/go.mod h1:u5H1YNBxpqRaxsYJYSkiCWKzEfiAb1Gb520KVy5xxl4=
 github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA=

internal/services/conditionalaccess/conditional_access_policy_resource.go

@@ -582,7 +582,7 @@ func conditionalAccessPolicyResource() *pluginsdk.Resource {
 						"sign_in_frequency_authentication_type": {
 							Type:     pluginsdk.TypeString,
 							Optional: true,
-							Default:  msgraph.ConditionalAccessAuthenticationTypePrimaryAndSecondaryAuthentication,
+							Computed: true,
 							ValidateFunc: validation.StringInSlice([]string{
 								msgraph.ConditionalAccessAuthenticationTypePrimaryAndSecondaryAuthentication,
 								msgraph.ConditionalAccessAuthenticationTypeSecondaryAuthentication,
@@ -592,7 +592,7 @@ func conditionalAccessPolicyResource() *pluginsdk.Resource {
 						"sign_in_frequency_interval": {
 							Type:     pluginsdk.TypeString,
 							Optional: true,
-							Default:  msgraph.ConditionalAccessFrequencyIntervalTimeBased,
+							Computed: true,
 							ValidateFunc: validation.StringInSlice([]string{
 								msgraph.ConditionalAccessFrequencyIntervalTimeBased,
 								msgraph.ConditionalAccessFrequencyIntervalEveryTime,
@@ -637,12 +637,14 @@ func conditionalAccessPolicyCustomizeDiff(_ context.Context, diff *pluginsdk.Res
 func conditionalAccessPolicyDiffSuppress(k, old, new string, d *pluginsdk.ResourceData) bool {
 	suppress := false
 
+	// When ineffectual `session_controls` are specified, you must send `sessionControls: null`, and when policy has ineffectual
+	// `sessionControls`, the API condenses it to `sessionControls: null` in the response.
 	if k == "session_controls.#" && old == "0" && new == "1" {
-		// When an ineffectual `session_controls` block is configured, the API just ignores it and returns
-		// sessionControls: null
 		sessionControlsRaw := d.Get("session_controls").([]interface{})
 		if len(sessionControlsRaw) == 1 && sessionControlsRaw[0] != nil {
 			sessionControls := sessionControlsRaw[0].(map[string]interface{})
+
+			// Suppress by default, but only if all the block properties have a non-default value
 			suppress = true
 			if v, ok := sessionControls["application_enforced_restrictions_enabled"]; ok && v.(bool) {
 				suppress = false
@@ -659,10 +661,10 @@ func conditionalAccessPolicyDiffSuppress(k, old, new string, d *pluginsdk.Resour
 			if v, ok := sessionControls["sign_in_frequency"]; ok && v.(int) > 0 {
 				suppress = false
 			}
-			if v, ok := sessionControls["sign_in_frequency_authentication_type"]; ok && v.(string) != msgraph.ConditionalAccessAuthenticationTypePrimaryAndSecondaryAuthentication {
+			if v, ok := sessionControls["sign_in_frequency_authentication_type"]; ok && v.(string) != "" {
 				suppress = false
 			}
-			if v, ok := sessionControls["sign_in_frequency_interval"]; ok && v.(string) != msgraph.ConditionalAccessFrequencyIntervalTimeBased {
+			if v, ok := sessionControls["sign_in_frequency_interval"]; ok && v.(string) != "" {
 				suppress = false
 			}
 			if v, ok := sessionControls["sign_in_frequency_period"]; ok && v.(string) != "" {

internal/services/conditionalaccess/conditional_access_policy_resource_test.go

@@ -162,16 +162,6 @@ func TestAccConditionalAccessPolicy_sessionControls(t *testing.T) {
 			),
 		},
 		data.ImportStep(),
-	})
-}
-
-func TestAccConditionalAccessPolicy_sessionControlsDisabled(t *testing.T) {
-	// This is testing the DiffSuppressFunc for the `session_controls` block
-
-	data := acceptance.BuildTestData(t, "azuread_conditional_access_policy", "test")
-	r := ConditionalAccessPolicyResource{}
-
-	data.ResourceTest(t, r, []acceptance.TestStep{
 		{
 			Config: r.sessionControlsDisabled(data),
 			Check: acceptance.ComposeTestCheckFunc(
@@ -199,6 +189,46 @@ func TestAccConditionalAccessPolicy_sessionControlsDisabled(t *testing.T) {
 			),
 		},
 		data.ImportStep(),
+		{
+			Config: r.sessionControlsApplicationEnforcedRestrictions(data),
+			Check: acceptance.ComposeTestCheckFunc(
+				check.That(data.ResourceName).ExistsInAzure(r),
+				check.That(data.ResourceName).Key("id").Exists(),
+				check.That(data.ResourceName).Key("display_name").HasValue(fmt.Sprintf("acctest-CONPOLICY-%d", data.RandomInteger)),
+				check.That(data.ResourceName).Key("state").HasValue("disabled"),
+			),
+		},
+		data.ImportStep(),
+		{
+			Config: r.sessionControlsCloudAppSecurityPolicy(data),
+			Check: acceptance.ComposeTestCheckFunc(
+				check.That(data.ResourceName).ExistsInAzure(r),
+				check.That(data.ResourceName).Key("id").Exists(),
+				check.That(data.ResourceName).Key("display_name").HasValue(fmt.Sprintf("acctest-CONPOLICY-%d", data.RandomInteger)),
+				check.That(data.ResourceName).Key("state").HasValue("disabled"),
+			),
+		},
+		data.ImportStep(),
+		{
+			Config: r.sessionControlsPersistentBrowserMode(data),
+			Check: acceptance.ComposeTestCheckFunc(
+				check.That(data.ResourceName).ExistsInAzure(r),
+				check.That(data.ResourceName).Key("id").Exists(),
+				check.That(data.ResourceName).Key("display_name").HasValue(fmt.Sprintf("acctest-CONPOLICY-%d", data.RandomInteger)),
+				check.That(data.ResourceName).Key("state").HasValue("disabled"),
+			),
+		},
+		data.ImportStep(),
+		{
+			Config: r.sessionControlsDisabled(data),
+			Check: acceptance.ComposeTestCheckFunc(
+				check.That(data.ResourceName).ExistsInAzure(r),
+				check.That(data.ResourceName).Key("id").Exists(),
+				check.That(data.ResourceName).Key("display_name").HasValue(fmt.Sprintf("acctest-CONPOLICY-%d", data.RandomInteger)),
+				check.That(data.ResourceName).Key("state").HasValue("disabled"),
+			),
+		},
+		data.ImportStep(),
 	})
 }
 
@@ -302,6 +332,11 @@ func TestAccConditionalAccessPolicy_guestsOrExternalUsers(t *testing.T) {
 }
 
 func (r ConditionalAccessPolicyResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) {
+	clients.ConditionalAccess.PoliciesClient.BaseClient.DisableRetries = true
+	defer func() {
+		clients.ConditionalAccess.PoliciesClient.BaseClient.DisableRetries = false
+	}()
+
 	var id *string
 
 	app, status, err := clients.ConditionalAccess.PoliciesClient.Get(ctx, state.ID, odata.Query{})
@@ -523,6 +558,129 @@ resource "azuread_conditional_access_policy" "test" {
 `, data.RandomInteger)
 }
 
+func (ConditionalAccessPolicyResource) sessionControlsApplicationEnforcedRestrictions(data acceptance.TestData) string {
+	return fmt.Sprintf(`
+provider "azuread" {}
+
+resource "azuread_conditional_access_policy" "test" {
+  display_name = "acctest-CONPOLICY-%[1]d"
+  state        = "disabled"
+
+  conditions {
+    client_app_types = ["browser"]
+
+    applications {
+      included_applications = ["All"]
+    }
+
+    locations {
+      included_locations = ["All"]
+    }
+
+    platforms {
+      included_platforms = ["all"]
+    }
+
+    users {
+      included_users = ["All"]
+      excluded_users = ["GuestsOrExternalUsers"]
+    }
+  }
+
+  grant_controls {
+    operator          = "OR"
+    built_in_controls = ["block"]
+  }
+
+  session_controls {
+    application_enforced_restrictions_enabled = true
+  }
+}
+`, data.RandomInteger)
+}
+
+func (ConditionalAccessPolicyResource) sessionControlsCloudAppSecurityPolicy(data acceptance.TestData) string {
+	return fmt.Sprintf(`
+provider "azuread" {}
+
+resource "azuread_conditional_access_policy" "test" {
+  display_name = "acctest-CONPOLICY-%[1]d"
+  state        = "disabled"
+
+  conditions {
+    client_app_types = ["browser"]
+
+    applications {
+      included_applications = ["All"]
+    }
+
+    locations {
+      included_locations = ["All"]
+    }
+
+    platforms {
+      included_platforms = ["all"]
+    }
+
+    users {
+      included_users = ["All"]
+      excluded_users = ["GuestsOrExternalUsers"]
+    }
+  }
+
+  grant_controls {
+    operator          = "OR"
+    built_in_controls = ["block"]
+  }
+
+  session_controls {
+    cloud_app_security_policy = "monitorOnly"
+  }
+}
+`, data.RandomInteger)
+}
+
+func (ConditionalAccessPolicyResource) sessionControlsPersistentBrowserMode(data acceptance.TestData) string {
+	return fmt.Sprintf(`
+provider "azuread" {}
+
+resource "azuread_conditional_access_policy" "test" {
+  display_name = "acctest-CONPOLICY-%[1]d"
+  state        = "disabled"
+
+  conditions {
+    client_app_types = ["browser"]
+
+    applications {
+      included_applications = ["All"]
+    }
+
+    locations {
+      included_locations = ["All"]
+    }
+
+    platforms {
+      included_platforms = ["all"]
+    }
+
+    users {
+      included_users = ["All"]
+      excluded_users = ["GuestsOrExternalUsers"]
+    }
+  }
+
+  grant_controls {
+    operator          = "OR"
+    built_in_controls = ["block"]
+  }
+
+  session_controls {
+    persistent_browser_mode = "always"
+  }
+}
+`, data.RandomInteger)
+}
+
 func (ConditionalAccessPolicyResource) clientApplicationsIncluded(data acceptance.TestData) string {
 	return fmt.Sprintf(`
 provider "azuread" {}

internal/services/conditionalaccess/conditionalaccess.go

@@ -493,19 +493,24 @@ func expandConditionalAccessSessionControls(in []interface{}) *msgraph.Condition
 		signInFrequency.IsEnabled = pointer.To(true)
 		signInFrequency.Type = pointer.To(config["sign_in_frequency_period"].(string))
 		signInFrequency.Value = pointer.To(int32(frequencyValue))
+
+		// AuthenticationType and FrequencyInterval must be set to default values here
+		signInFrequency.AuthenticationType = pointer.To(msgraph.ConditionalAccessAuthenticationTypePrimaryAndSecondaryAuthentication)
+		signInFrequency.FrequencyInterval = pointer.To(msgraph.ConditionalAccessFrequencyIntervalTimeBased)
 	}
 
-	if authenticationType, ok := config["sign_in_frequency_authentication_type"]; ok {
+	if authenticationType, ok := config["sign_in_frequency_authentication_type"]; ok && authenticationType.(string) != "" {
 		signInFrequency.AuthenticationType = pointer.To(authenticationType.(string))
 	}
 
-	if interval, ok := config["sign_in_frequency_interval"]; ok {
+	if interval, ok := config["sign_in_frequency_interval"]; ok && interval.(string) != "" {
 		signInFrequency.FrequencyInterval = pointer.To(interval.(string))
 	}
 
 	// API returns 400 error if signInFrequency is set with all default/zero values
-	if pointer.From(signInFrequency.IsEnabled) || pointer.From(signInFrequency.FrequencyInterval) != msgraph.ConditionalAccessFrequencyIntervalTimeBased ||
-		pointer.From(signInFrequency.AuthenticationType) != msgraph.ConditionalAccessAuthenticationTypePrimaryAndSecondaryAuthentication {
+	if (signInFrequency.IsEnabled != nil && *signInFrequency.IsEnabled) ||
+		(signInFrequency.FrequencyInterval != nil && *signInFrequency.FrequencyInterval != msgraph.ConditionalAccessFrequencyIntervalTimeBased) ||
+		(signInFrequency.AuthenticationType != nil && *signInFrequency.AuthenticationType != msgraph.ConditionalAccessAuthenticationTypePrimaryAndSecondaryAuthentication) {
 		result.SignInFrequency = &signInFrequency
 	}
 

vendor/modules.txt

@@ -209,7 +209,7 @@ github.com/hashicorp/terraform-svchost
 # github.com/hashicorp/yamux v0.1.1
 ## explicit; go 1.15
 github.com/hashicorp/yamux
-# github.com/manicminer/hamilton v0.67.0
+# github.com/manicminer/hamilton v0.68.0
 ## explicit; go 1.21
 github.com/manicminer/hamilton/errors
 github.com/manicminer/hamilton/internal/utils