-
Notifications
You must be signed in to change notification settings - Fork 300
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.
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
Feature: add application filter to azuread_conditional_access_policy
#1357
base: main
Are you sure you want to change the base?
Feature: add application filter to azuread_conditional_access_policy
#1357
Conversation
@tombuildsstuff — don't suppose you've time to give some feedback on this PR as well? |
Signed-off-by: Brendan Thompson <[email protected]>
Signed-off-by: Brendan Thompson <[email protected]>
Signed-off-by: Brendan Thompson <[email protected]>
Signed-off-by: Brendan Thompson <[email protected]>
84c762d
to
2b43824
Compare
There was a problem hiding this 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!
@manicminer thanks for reviewing this, I will try and get an acceptance test in today 😃 |
@BrendanThompson Great thanks, and sorry for the mis-tag! 😊 |
@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) |
@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. |
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? |
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. |
Hey @manicminer, I have opened a PR in |
@BrendanThompson The hamilton version has been updated in the provider, hopefully this unblocks your work on this 🙂 |
…stroy Co-authored-by: KenSpur <[email protected]>
…`app_role`, `oauth2_permission_scope`, `identifier_uris`, `optional_claims`, and `required_resource_access`
…_application_registration
…om template The `/instantiate` operation can return a 404 whlst also processing the request completely normally. This leads to orphaned objects in the directory, and a resource that cannot successfully Create. Work around this by polling for the application and service principal that you'd expect to see created out-of-band, whenever a 404 is received. Also set a temporary `displayName` for the application, as this is the only means we have to identify the resulting object is the one we are looking for. Unfortunately this means that this workaround cannot be implemented for the `azuread_application_from_template` resource, since that resource intentionally avoids changing the implicitly created `user_impersonation` scope - this will get created with nonsensical display names / descriptions in the consent flow. Since the whole point of the standalone resource is to inherit scopes from the template, this makes it infeasible to add this workaround there.
…ner/hamilton` - depends on manicminer/hamilton#285
I'm really hoping to use I have nothing valuable to add... but I just wanted to cheer you guys on as I'm hoping to leverage this appFilter as well lol thanks for your work on it! |
Description
This PR adds the ability to have an application filter on
azuread_conditional_access_policy
.Change Log
azuread_conditional_access_policy
– add thefilter
attribute to theapplications
block allowing for applications to be filtered [Feature: add application filter toazuread_conditional_access_policy
#1357]Related Issue(s)
Fixes #1318