Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature: add application filter to azuread_conditional_access_policy #1357

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Feature: add application filter to azuread_conditional_access_policy #1357

wants to merge 4 commits into from

Conversation

BrendanThompson
Copy link

@BrendanThompson BrendanThompson commented Apr 18, 2024 •
edited

Description

This PR adds the ability to have an application filter on azuread_conditional_access_policy.

resource "azuread_conditional_access_policy" "this" {
  display_name = "access-policy"
  state        = "disabled"

  conditions {
    client_app_types    = ["other"]

    applications {
      filter {
        mode = "include"
        rule = "CustomSecurityAttribute.AttributeName -eq \"AttributeValue\""
      }
    }

    users {
      included_roles = ["Contributor"]
    }
  }
}

Change Log

Related Issue(s)

Fixes #1318

@BrendanThompson
Copy link
Author

@tombuildsstuff — don't suppose you've time to give some feedback on this PR as well?

@BrendanThompson @manicminer
feat: add filter to applications in schema
cbe697b 
Signed-off-by: Brendan Thompson <github@blt.is>
@BrendanThompson @manicminer
feat: add expansion for applications filter
7fb5a79 
Signed-off-by: Brendan Thompson <github@blt.is>
@BrendanThompson @manicminer
feat: make flatten filter generic and use for device and app
7212c53 
Signed-off-by: Brendan Thompson <github@blt.is>
@BrendanThompson @manicminer
chore: ensure only one inclusion option is used
2b43824 
Signed-off-by: Brendan Thompson <github@blt.is>
@manicminer manicminer force-pushed the feature/application-conditional-filter branch from 84c762d to 2b43824 Compare May 8, 2024 19:36
@github-actions github-actions bot added size/S and removed size/XL labels May 8, 2024
manicminer
manicminer requested changes May 8, 2024
View reviewed changes
Copy link
Member

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @TomasKunka for this addition.

This is looking great - if you could add an acceptance test case that uses a filter for an application condition then this should be good to merge. Conditional Access Policies have shown to yield odd payload parsing bugs further down the line so having coverage is necessary to give us a good shot at spotting regressions.

Thanks!

@BrendanThompson
Copy link
Author

@manicminer thanks for reviewing this, I will try and get an acceptance test in today 😃

@manicminer
Copy link
Member

manicminer commented May 9, 2024 •
edited

@BrendanThompson Great thanks, and sorry for the mis-tag! 😊

@BrendanThompson
Copy link
Author

@manicminer — sorry it's taken me a few days to get back to you. I started writing the tests then I realised that we will actually require some attributes to be set on wherever the Acceptance Tests are run, otherwise the tests will fail. There is currently also no mechanism to deploy said attributes with Terraform ☹️.

https://learn.microsoft.com/en-us/entra/fundamentals/custom-security-attributes-overview

The above link are the attributes we need in-place. How would you like to proceed on this, if possible were you able to add an attribute and I can test with that. Then perhaps the next thing is to work on integrating the attribute creation into the provider? (Happy to work in this)

@manicminer
Copy link
Member

@BrendanThompson All good, appreciate you looking at this. Unfortunately these are not currently supported by the provider. I don't believe they are supported by the current SDK either so this would have to be added throughout in order to integrate custom security attributes. Is it not maybe possible to build a rule for testing without using custom attributes? Even if it's essentially a no-op that would be fine.

@BrendanThompson
Copy link
Author

I cannot think of any way to test it properly without the custom attributes being there to be honest. And testing it without an attribute would yield odd results due to the API. We could add a notice on the resource itself, like a warning around it. Buyer beware until the security attributes are available. What are your thoughts?

@manicminer
Copy link
Member

I would also love to ship this but unless we are able to demonstrate some level of testing, we unfortunately won't be able to add a property or block to a resource without it, which I guess means this will have to wait until we have a resource to support custom security attributes.

@BrendanThompson
Copy link
Author

Hey @manicminer,

I have opened a PR in Hamilton (manicminer/hamilton#281) to add support for both attribute sets and custom security attribute definitions once that has gone through I will add support for them in the provider and update the tests here 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manicminer manicminer manicminer requested changes

Assignees
No one assigned
Labels
dependencies feature/conditional-access size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

application_filter param missing from azuread_conditional_access_policy applications condition
2 participants
@BrendanThompson @manicminer