diff --git a/docs/resources/conditional_access_policy.md b/docs/resources/conditional_access_policy.md index a69e06343..67218ce1a 100644 --- a/docs/resources/conditional_access_policy.md +++ b/docs/resources/conditional_access_policy.md @@ -187,7 +187,7 @@ The following arguments are supported: `devices` block supports the following: -* `filter` - (Optional) A `filter` block as described below. A `filter` block can be added to an existing policy, but removing the `filter` block forces a new resource to be created. +* `filter` - (Optional) A `filter` block as described below. --- @@ -246,8 +246,10 @@ 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`. * `disable_resilience_defaults` - (Optional) Disables [resilience defaults](https://learn.microsoft.com/en-us/azure/active-directory/conditional-access/resilience-defaults). Defaults to `false`. * `persistent_browser_mode` - (Optional) Session control to define whether to persist cookies. Possible values are: `always` or `never`. -* `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. +* `sign_in_frequency` - (Optional) Number of days or hours to enforce sign-in frequency. Required when `sign_in_frequency_period` is specified. +* `sign_in_frequency_authentication_type` - (Optional) Authentication type for enforcing sign-in frequency. Possible values are: `primaryAndSecondaryAuthentication` or `secondaryAuthentication`. Defaults to `primaryAndSecondaryAuthentication`. +* `sign_in_frequency_interval` - (Optional) The interval to apply to sign-in frequency control. Possible values are: `timeBased` or `everyTime`. Defaults to `timeBased`. +* `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. --- diff --git a/internal/services/conditionalaccess/authentication_strength_policy_resource_test.go b/internal/services/conditionalaccess/authentication_strength_policy_resource_test.go index c8a13e53e..b741d5e42 100644 --- a/internal/services/conditionalaccess/authentication_strength_policy_resource_test.go +++ b/internal/services/conditionalaccess/authentication_strength_policy_resource_test.go @@ -95,6 +95,8 @@ func (r AuthenticationStrengthPolicyResource) Exists(ctx context.Context, client func (AuthenticationStrengthPolicyResource) basic(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azuread" {} + resource "azuread_authentication_strength_policy" "test" { display_name = "acctestASP-%[1]d" description = "test" @@ -105,6 +107,8 @@ resource "azuread_authentication_strength_policy" "test" { func (AuthenticationStrengthPolicyResource) complete(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azuread" {} + resource "azuread_authentication_strength_policy" "test" { display_name = "acctestASP-%[1]d" description = "test" diff --git a/internal/services/conditionalaccess/conditional_access_policy_resource.go b/internal/services/conditionalaccess/conditional_access_policy_resource.go index d3e631a65..e7c181a6d 100644 --- a/internal/services/conditionalaccess/conditional_access_policy_resource.go +++ b/internal/services/conditionalaccess/conditional_access_policy_resource.go @@ -476,11 +476,34 @@ func conditionalAccessPolicyResource() *pluginsdk.Resource { ValidateFunc: validation.IntAtLeast(0), }, + "sign_in_frequency_authentication_type": { + Type: pluginsdk.TypeString, + Optional: true, + Default: msgraph.ConditionalAccessAuthenticationTypePrimaryAndSecondaryAuthentication, + ValidateFunc: validation.StringInSlice([]string{ + msgraph.ConditionalAccessAuthenticationTypePrimaryAndSecondaryAuthentication, + msgraph.ConditionalAccessAuthenticationTypeSecondaryAuthentication, + }, false), + }, + + "sign_in_frequency_interval": { + Type: pluginsdk.TypeString, + Optional: true, + Default: msgraph.ConditionalAccessFrequencyIntervalTimeBased, + ValidateFunc: validation.StringInSlice([]string{ + msgraph.ConditionalAccessFrequencyIntervalTimeBased, + msgraph.ConditionalAccessFrequencyIntervalEveryTime, + }, false), + }, + "sign_in_frequency_period": { Type: pluginsdk.TypeString, Optional: true, RequiredWith: []string{"session_controls.0.sign_in_frequency"}, - ValidateFunc: validation.StringInSlice([]string{"days", "hours"}, false), + ValidateFunc: validation.StringInSlice([]string{ + msgraph.ConditionalAccessFrequencyTypeDays, + msgraph.ConditionalAccessFrequencyTypeHours, + }, false), }, }, }, @@ -489,20 +512,20 @@ func conditionalAccessPolicyResource() *pluginsdk.Resource { } } -func conditionalAccessPolicyCustomizeDiff(ctx context.Context, diff *pluginsdk.ResourceDiff, meta interface{}) error { - // See https://github.com/microsoftgraph/msgraph-metadata/issues/93 - if old, new := diff.GetChange("session_controls.0.sign_in_frequency"); old.(int) > 0 && new.(int) == 0 { - diff.ForceNew("session_controls.0.sign_in_frequency") - } - if old, new := diff.GetChange("session_controls.0.sign_in_frequency_period"); old.(string) != "" && new.(string) == "" { - diff.ForceNew("session_controls.0.sign_in_frequency") - } - - if old, new := diff.GetChange("conditions.0.devices.#"); old.(int) > 0 && new.(int) == 0 { - diff.ForceNew("conditions.0.devices") +func conditionalAccessPolicyCustomizeDiff(_ context.Context, diff *pluginsdk.ResourceDiff, _ interface{}) error { + // The API does not like sessionControls being set with ineffectual properties, so this additional validation complements + // AtLeastOneOf: []string{"grant_controls", "session_controls"} by helping to ensure that either `grant_controls` or a + // useful `session_controls` block has been set in the configuration. + var sessionControlsSetButIneffective bool + if diff.Get("session_controls.#").(int) == 1 && !diff.Get("session_controls.0.application_enforced_restrictions_enabled").(bool) && + diff.Get("session_controls.0.cloud_app_security_policy").(string) == "" && !diff.Get("session_controls.0.disable_resilience_defaults").(bool) && + diff.Get("session_controls.0.persistent_browser_mode").(string) == "" && diff.Get("session_controls.0.sign_in_frequency").(int) == 0 && + diff.Get("session_controls.0.sign_in_frequency_authentication_type").(string) == msgraph.ConditionalAccessAuthenticationTypePrimaryAndSecondaryAuthentication && + diff.Get("session_controls.0.sign_in_frequency_interval").(string) == msgraph.ConditionalAccessFrequencyIntervalTimeBased { + sessionControlsSetButIneffective = true } - if old, new := diff.GetChange("conditions.0.devices.0.filter.#"); old.(int) > 0 && new.(int) == 0 { - diff.ForceNew("conditions.0.devices.0.filter") + if diff.Get("grant_controls.#").(int) == 0 && sessionControlsSetButIneffective { + return fmt.Errorf("when specifying `session_controls` but not `grant_controls`, one of the properties in the `session_controls` block must be set to an effective value in order for session controls to work") } return nil @@ -533,6 +556,12 @@ 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 { + suppress = false + } + if v, ok := sessionControls["sign_in_frequency_interval"]; ok && v.(string) != msgraph.ConditionalAccessFrequencyIntervalTimeBased { + suppress = false + } if v, ok := sessionControls["sign_in_frequency_period"]; ok && v.(string) != "" { suppress = false } diff --git a/internal/services/conditionalaccess/conditional_access_policy_resource_test.go b/internal/services/conditionalaccess/conditional_access_policy_resource_test.go index 7aa573eb5..1c4e6d944 100644 --- a/internal/services/conditionalaccess/conditional_access_policy_resource_test.go +++ b/internal/services/conditionalaccess/conditional_access_policy_resource_test.go @@ -84,55 +84,6 @@ func TestAccConditionalAccessPolicy_update(t *testing.T) { }) } -func TestAccConditionalAccessPolicy_deviceFilter(t *testing.T) { - // This is a separate test for two reasons: - // 1. To accommodate future properties included_devices/excluded_devices which both conflict with devices.0.filter - // 2. Because devices.0.filter is conditionally ForceNew, as the API ignores removal of this property - - data := acceptance.BuildTestData(t, "azuread_conditional_access_policy", "test") - r := ConditionalAccessPolicyResource{} - - data.ResourceTest(t, r, []acceptance.TestStep{ - { - Config: r.deviceFilter(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.complete(data), - Check: acceptance.ComposeTestCheckFunc( - check.That(data.ResourceName).ExistsInAzure(r), - ), - }, - data.ImportStep(), - { - Config: r.deviceFilter(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.deviceFilterUpdate(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(), - }) -} - func TestAccConditionalAccessPolicy_includedUserActions(t *testing.T) { data := acceptance.BuildTestData(t, "azuread_conditional_access_policy", "test") r := ConditionalAccessPolicyResource{} @@ -332,6 +283,8 @@ func (r ConditionalAccessPolicyResource) Exists(ctx context.Context, clients *cl func (ConditionalAccessPolicyResource) basic(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azuread" {} + resource "azuread_conditional_access_policy" "test" { display_name = "acctest-CONPOLICY-%[1]d" state = "disabled" @@ -359,6 +312,8 @@ resource "azuread_conditional_access_policy" "test" { func (ConditionalAccessPolicyResource) complete(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azuread" {} + resource "azuread_conditional_access_policy" "test" { display_name = "acctest-CONPOLICY-%[1]d" state = "enabledForReportingButNotEnforced" @@ -373,6 +328,13 @@ resource "azuread_conditional_access_policy" "test" { excluded_applications = [] } + devices { + filter { + mode = "exclude" + rule = "device.operatingSystem eq \"Doors\"" + } + } + locations { included_locations = ["All"] excluded_locations = ["AllTrusted"] @@ -396,103 +358,22 @@ resource "azuread_conditional_access_policy" "test" { session_controls { application_enforced_restrictions_enabled = true - disable_resilience_defaults = false cloud_app_security_policy = "blockDownloads" + disable_resilience_defaults = false persistent_browser_mode = "always" sign_in_frequency = 2 + sign_in_frequency_authentication_type = "primaryAndSecondaryAuthentication" + sign_in_frequency_interval = "timeBased" sign_in_frequency_period = "days" } } `, data.RandomInteger) } -func (ConditionalAccessPolicyResource) deviceFilter(data acceptance.TestData) string { - return fmt.Sprintf(` -resource "azuread_conditional_access_policy" "test" { - display_name = "acctest-CONPOLICY-%[1]d" - state = "disabled" - - conditions { - client_app_types = ["browser"] - - applications { - included_applications = ["All"] - } - - devices { - filter { - mode = "exclude" - rule = "device.operatingSystem eq \"Doors\"" - } - } - - locations { - included_locations = ["All"] - } - - platforms { - included_platforms = ["all"] - } - - users { - included_users = ["All"] - excluded_users = ["GuestsOrExternalUsers"] - } - } - - grant_controls { - operator = "OR" - built_in_controls = ["block"] - } -} -`, data.RandomInteger) -} - -func (ConditionalAccessPolicyResource) deviceFilterUpdate(data acceptance.TestData) string { - return fmt.Sprintf(` -resource "azuread_conditional_access_policy" "test" { - display_name = "acctest-CONPOLICY-%[1]d" - state = "disabled" - - conditions { - client_app_types = ["browser"] - - applications { - included_applications = ["All"] - } - - devices { - filter { - mode = "exclude" - rule = "device.model eq \"yPhone Z\"" - } - } - - locations { - included_locations = ["All"] - } - - platforms { - included_platforms = ["all"] - } - - users { - included_users = ["All"] - excluded_users = ["GuestsOrExternalUsers"] - } - } - - grant_controls { - operator = "OR" - built_in_controls = ["block"] - } - -} -`, data.RandomInteger) -} - func (ConditionalAccessPolicyResource) includedUserActions(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azuread" {} + resource "azuread_conditional_access_policy" "test" { display_name = "acctest-CONPOLICY-%[1]d" state = "disabled" @@ -527,6 +408,8 @@ resource "azuread_conditional_access_policy" "test" { func (ConditionalAccessPolicyResource) sessionControls(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azuread" {} + resource "azuread_conditional_access_policy" "test" { display_name = "acctest-CONPOLICY-%[1]d" state = "disabled" @@ -566,6 +449,8 @@ resource "azuread_conditional_access_policy" "test" { func (ConditionalAccessPolicyResource) sessionControlsDisabled(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azuread" {} + resource "azuread_conditional_access_policy" "test" { display_name = "acctest-CONPOLICY-%[1]d" state = "disabled" @@ -605,6 +490,8 @@ resource "azuread_conditional_access_policy" "test" { func (ConditionalAccessPolicyResource) clientApplicationsIncluded(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azuread" {} + data "azuread_service_principal" "test" { display_name = "Terraform Acceptance Tests (Single Tenant)" } @@ -641,6 +528,8 @@ resource "azuread_conditional_access_policy" "test" { func (ConditionalAccessPolicyResource) clientApplicationsExcluded(data acceptance.TestData) string { return fmt.Sprintf(` +provider "azuread" {} + data "azuread_service_principal" "test" { display_name = "Terraform Acceptance Tests (Single Tenant)" } diff --git a/internal/services/conditionalaccess/conditionalaccess.go b/internal/services/conditionalaccess/conditionalaccess.go index 375237665..c67a9d42c 100644 --- a/internal/services/conditionalaccess/conditionalaccess.go +++ b/internal/services/conditionalaccess/conditionalaccess.go @@ -140,12 +140,12 @@ func flattenConditionalAccessSessionControls(in *msgraph.ConditionalAccessSessio applicationEnforceRestrictions := false if in.ApplicationEnforcedRestrictions != nil { - applicationEnforceRestrictions = *in.ApplicationEnforcedRestrictions.IsEnabled + applicationEnforceRestrictions = pointer.From(in.ApplicationEnforcedRestrictions.IsEnabled) } cloudAppSecurity := "" - if in.CloudAppSecurity != nil && in.CloudAppSecurity.CloudAppSecurityType != nil { - cloudAppSecurity = *in.CloudAppSecurity.CloudAppSecurityType + if in.CloudAppSecurity != nil { + cloudAppSecurity = pointer.From(in.CloudAppSecurity.CloudAppSecurityType) } disableResilienceDefaults := false @@ -154,15 +154,22 @@ func flattenConditionalAccessSessionControls(in *msgraph.ConditionalAccessSessio } signInFrequency := 0 + signInFrequencyAuthenticationType := "" + signInFrequencyInterval := "" signInFrequencyPeriod := "" - if in.SignInFrequency != nil && in.SignInFrequency.Value != nil && in.SignInFrequency.Type != nil { - signInFrequency = int(*in.SignInFrequency.Value) - signInFrequencyPeriod = *in.SignInFrequency.Type + if in.SignInFrequency != nil { + if in.SignInFrequency.Value != nil && in.SignInFrequency.Type != nil { + signInFrequency = int(*in.SignInFrequency.Value) + signInFrequencyPeriod = *in.SignInFrequency.Type + } + + signInFrequencyAuthenticationType = pointer.From(in.SignInFrequency.AuthenticationType) + signInFrequencyInterval = pointer.From(in.SignInFrequency.FrequencyInterval) } persistentBrowserMode := "" - if in.PersistentBrowser != nil && in.PersistentBrowser.Mode != nil { - persistentBrowserMode = *in.PersistentBrowser.Mode + if in.PersistentBrowser != nil { + persistentBrowserMode = pointer.From(in.PersistentBrowser.Mode) } return []interface{}{ @@ -172,6 +179,8 @@ func flattenConditionalAccessSessionControls(in *msgraph.ConditionalAccessSessio "disable_resilience_defaults": disableResilienceDefaults, "persistent_browser_mode": persistentBrowserMode, "sign_in_frequency": signInFrequency, + "sign_in_frequency_authentication_type": signInFrequencyAuthenticationType, + "sign_in_frequency_interval": signInFrequencyInterval, "sign_in_frequency_period": signInFrequencyPeriod, }, } @@ -448,12 +457,32 @@ func expandConditionalAccessSessionControls(in []interface{}) *msgraph.Condition } } - if signInFrequency := config["sign_in_frequency"].(int); signInFrequency > 0 { - result.SignInFrequency = &msgraph.SignInFrequencySessionControl{ - IsEnabled: pointer.To(true), - Type: pointer.To(config["sign_in_frequency_period"].(string)), - Value: pointer.To(int32(signInFrequency)), - } + signInFrequency := msgraph.SignInFrequencySessionControl{} + if frequencyValue := config["sign_in_frequency"].(int); frequencyValue > 0 { + signInFrequency.IsEnabled = pointer.To(true) + signInFrequency.Type = pointer.To(config["sign_in_frequency_period"].(string)) + signInFrequency.Value = pointer.To(int32(frequencyValue)) + } + + if authenticationType, ok := config["sign_in_frequency_authentication_type"]; ok { + signInFrequency.AuthenticationType = pointer.To(authenticationType.(string)) + } + + if interval, ok := config["sign_in_frequency_interval"]; ok { + 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 { + result.SignInFrequency = &signInFrequency + } + + // API does not accept ineffectual and sessionControls object, and it will not remove any existing sessionControls unless the entire object is set to null + if (result.ApplicationEnforcedRestrictions == nil || !pointer.From(result.ApplicationEnforcedRestrictions.IsEnabled)) && + result.CloudAppSecurity == nil && !pointer.From(result.DisableResilienceDefaults) && + result.PersistentBrowser == nil && result.SignInFrequency == nil { + return nil } return &result