Skip to content

Commit

Permalink
Conditional Access: support sign_in_frequency_authentication_type a…
Browse files Browse the repository at this point in the history 
…nd `sign_in_frequency_interval`

Additionally, remove the conditional ForceNew for `session_controls` and
`devices` blocks, now that that the API consistently accepts `null` values
for these objects.

Also add plan-time validation for an ineffectual `session_controls`
block when `grant_controls` has not been set.

And fix up some tests.
  • Loading branch information
@manicminer
manicminer committed Oct 26, 2023
1 parent bbab633 commit ea3561b
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 166 deletions.
8 changes: 5 additions & 3 deletions docs/resources/conditional_access_policy.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.

---

Expand Down Expand Up @@ -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.

---

Expand Down
4 changes: 4 additions & 0 deletions internal/services/conditionalaccess/authentication_strength_policy_resource_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down
57 changes: 43 additions & 14 deletions internal/services/conditionalaccess/conditional_access_policy_resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
},
},
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
159 changes: 24 additions & 135 deletions internal/services/conditionalaccess/conditional_access_policy_resource_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand All @@ -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"]
Expand All @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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)"
}
Expand Down Expand Up @@ -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)"
}
Expand Down
Loading

0 comments on commit ea3561b

Please sign in to comment.