Bugfix CA Policies

[alexwilcox9]

Fixes #568

Whilst testing this I initially started by commenting out the setting of ApplicationEnforcedRestrictions and CloudAppSecurity in expandConditionalAccessSessionControls which bizarrely started causing panics in flattenConditionalAccessSessionControls

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xed80d0]

goroutine 691 [running]:
github.com/hashicorp/terraform-provider-azuread/internal/services/conditionalaccess.flattenConditionalAccessSessionControls(...)
        /home/alexwilcox/go/src/github.com/hashicorp/terraform-provider-azuread/internal/services/conditionalaccess/conditional_access_policy_resource.go:573
github.com/hashicorp/terraform-provider-azuread/internal/services/conditionalaccess.conditionalAccessPolicyResourceRead(0x13afeb8, 0xc000d0acc0, 0xc0009aa400, 0x10322a0, 0xc00089a1c0, 0xc000992d70, 0xc00075dae0, 0x0)
        /home/alexwilcox/go/src/github.com/hashicorp/terraform-provider-azuread/internal/services/conditionalaccess/conditional_access_policy_resource.go:425 +0x6d0
github.com/hashicorp/terraform-provider-azuread/internal/services/conditionalaccess.conditionalAccessPolicyResourceCreate(0x13afeb8, 0xc000d0acc0, 0xc0009aa400, 0x10322a0, 0xc00089a1c0, 0xc000992d60, 0x9059ea, 0xc00075cb60)

I believe I've solved this by adding some nil checks in that function

I found that not setting ApplicationEnforcedRestrictions and CloudAppSecurity at all in expandConditionalAccessSessionControls even if they weren't set in the terraform configuration got it working so I've now rearranged the logic in that function so that if the attributes aren't set in the terraform we don't instantiate those structs

I've tried updating the session_controls block in the complete test to the following and all are now passing

  session_controls {
    application_enforced_restrictions_enabled = true
    cloud_app_security_policy                 = "monitorOnly"
    sign_in_frequency                         = 10
    sign_in_frequency_period                  = "hours"
  }

  session_controls {
    cloud_app_security_policy                 = "monitorOnly"
    sign_in_frequency                         = 10
    sign_in_frequency_period                  = "hours"
  }

  session_controls {
    sign_in_frequency                         = 10
    sign_in_frequency_period                  = "hours"
  }

  session_controls {
    application_enforced_restrictions_enabled = true
    cloud_app_security_policy                 = "monitorOnly"
  }

Also when running all these tests I did hit the update issue that named locations were getting (and were solved by the refresh func) but I wasn't sure how best to go about adding this as I see in named locations you just compare each value in turn and there's a lot of values to check in this resource! If that is the way I'm happy to crack on, just wanted to check first

=== CONT  TestAccConditionalAccessPolicy_update
    testcase.go:60: Step 3/4 error: After applying this test step and performing a `terraform refresh`, the plan was not empty.
        stdout
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # azuread_conditional_access_policy.test will be updated in-place
          ~ resource "azuread_conditional_access_policy" "test" {
                id           = "84812c78-43fe-4338-b738-9c037623f6d3"
                # (2 unchanged attributes hidden)
        
              ~ conditions {
                  ~ client_app_types    = [
                      - "browser",
                      + "all",
                    ]
                  ~ sign_in_risk_levels = [
                      + "medium",
                    ]
                  ~ user_risk_levels    = [
                      + "medium",
                    ]
        
                  ~ applications {
                      ~ excluded_applications = [
                          + "00000004-0000-0ff1-ce00-000000000000",
                        ]
                        # (2 unchanged attributes hidden)
                    }
        
                  ~ locations {
                      ~ excluded_locations = [
                          + "AllTrusted",
                        ]
                        # (1 unchanged attribute hidden)
                    }
        
                  + platforms {
                      + excluded_platforms = [
                          + "iOS",
                        ]
                      + included_platforms = [
                          + "android",
                        ]
                    }
        
                    # (1 unchanged block hidden)
                }
        
              ~ grant_controls {
                  ~ built_in_controls             = [
                      - "block",
                      + "mfa",
                    ]
                    # (3 unchanged attributes hidden)
                }
        
              + session_controls {
                  + application_enforced_restrictions_enabled = true
                  + cloud_app_security_policy                 = "monitorOnly"
                  + sign_in_frequency                         = 10
                  + sign_in_frequency_period                  = "hours"
                }
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.
--- PASS: TestAccConditionalAccessPolicy_complete (45.17s)
--- PASS: TestAccConditionalAccessPolicy_basic (46.88s)
--- FAIL: TestAccConditionalAccessPolicy_update (48.58s)
FAIL
FAIL    github.com/hashicorp/terraform-provider-azuread/internal/services/conditionalaccess     48.591s

[kaovd]

Nice catch, do you think we need a bunch of tests to make sure we've got 100% coverage or this will be good?

[alexwilcox9]

@kaovd I'm not sure really, there are so many different combinations we could produce tests for there has to be a balance and the real world will always produce something we didn't plan for.
Will see what @manicminer thinks when they're back

[manicminer]

Hi @alexwilcox9 thanks for picking this up. This seems to be down to API bugs where it won't allow boolean values to be false, but insists they be omitted entirely to unset them!

The approach you've taken is good, my only suggestion would be for the flatten funcs, that they always set values even when those values are the default. Suggested approach inline below :)

We also seem to have missed some nil checks in the initial implementation which would account for the crashes you saw. I added nil checks for all nested pointers in my below suggestion.

In terms of tests, I think we're OK for these (API bug causing) cases to not litter the tests with all the different permutations as that will explode exponentially.

[alexwilcox9]

Thanks for the feedback @manicminer I've now added the requested changes
What do you think about the occasionally failing update test? Is it worth building a refresh func in?

[manicminer]

Note to reviewer: This change is technically breaking since some Optional fields are now Required, but those fields were required by the API anyway and omitting them would have failed to apply. The updated expandConditionalAccessSessionControls() works around some peculiar API behaviors, pasted below from the commit message for visibility.

---

Adjust the behaviour of the sessionControls field

• The API is very fussy about the field in POST requests
• Where it has no features enabled, it must be omitted
• Where a feature in this section is disabled, that feature must be omitted (isEnabled: false is not recognized in POST requests)
• This does not apply to PATCH requests, where sessionControls can be omitted when no contained features are enabled, but if any are enabled, all the features must be specified with their respective isEnabled field (which can be false for PATCH requests).

[manicminer]

Test results

authored some changes

[jackofallops]

This LGTM 🚀

[github-actions]

This functionality has been released in v2.3.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]

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.