search

microsoft / Microsoft365DSC

Manages, configures, extracts and monitors Microsoft 365 tenant configurations
https://aka.ms/M365DSC
MIT License
1.61k stars 500 forks source link

AADConditionalAccessPolicy: Add possibility to specify Include-/ExcludeApplications by name instead of GUID #4011

Open salbeck-sit opened 11 months ago

salbeck-sit commented 11 months ago

Description of the issue

Today we specify IncludeApplications and ExcludeApplications in Conditional Access policies by their GUID. The exception is built-in names such as 'All', 'Office365' and likely others that I haven't had use for so far. Groups and Users are specified by their name - so why not Apps ? The update I'm suggesting will allow applications to be specified by their DisplayName in the same way that the Entra portal allows you to do while at the same time keeping the door open to GUIDs being used in order to keep the resource backwards-compatible. It may be an issue to keep on top of additional built-in names added. I haven't been able to find a list other than what I can infer from https://learn.microsoft.com/en-us/entra/identity/conditional-access/concept-conditional-access-cloud-apps It doesn't make any real difference if 'built-in names' are updated in the resource or not as it is somewhat lenient in regards to 'translating' a name to a GUID. If an SPN doesn't exist, the name is used as-is. This may cause runtime-errors, ie if a name is missspelled, but errors should be easy enough to catch. The resource can also be updated to provide logging of such errors

I have a working copy of the resource and also expanded unit-tests. It'd be great if someone can provide a longer list of the above mentioned built-in names - or provide it as an add-on PR.

Microsoft 365 DSC Version

DEV

Which workloads are affected

Azure Active Directory

The DSC configuration

No response

Verbose logs showing the problem

No response

Environment Information + PowerShell Version

Built and tested using Windows Powershell 5.1 and Pester 5.4.0
salbeck-sit commented 11 months ago

PS I've also noticed a bug (or rather, VSCode did) in AADCOnditionalAccessPolicy wrt TermsOfUse due to a missspelled variable-name. I haven't included it as a separate issue but it is included in the PR I have ready

andikrueger commented 11 months ago

I guess, this was done to mitigate the risk of having applications with the same display name. Does you code handle this as well? User Principal Names for groups and users are unique.

ricmestre commented 11 months ago

Also, why do you need to keep up with the names of the apps from the website? I suspect you are creating a list of these apps directly in the module right? Wouldn't it be better to go through the array of apps and accept GUIDs by default but if it's a name then just call Get-MgApplication -Filter "displayName eq '$CurrentAppNameInTheArray'"? Then if it doesn't find it or if it finds more than one just error out.

This would make it work for cloning scenarios if those include/exclude apps are custom to each customer and don't have well-known GUIDs or names.

salbeck-sit commented 11 months ago

Ouch, I hadn't considered duplicate app displaynames. There is not really any quickfix. One option is for get/set/test/export to refer to unique SPN's by name and ambiguous ones by GUID and have configurations do the same. Use GUIDs everywhere if that's you thing but exports would then show names for unique app-names and GUIDs for others. If a config only refers to GUIDs then this would cause 'false drift'. For Test-/Set-TargetResource, there is only one option I can see for an app name that is found to be ambigous, that is to throw an error. In a configuration that means that such an app must be specified by GUID. Today this scenario is possible and also not covered and it will result in an error when the MOF is applied.

salbeck-sit commented 11 months ago

@ricmestre my idea about updating 'built-in names' is that such names are the ones defined in the GUI that doesn't refer to a GUID. Ie the names are logical like 'Office365' or 'All' because they don't refer to a 'real' app but to a group of apps. That means that these names cannot be represented by a GUID. In regards to the above, that means that on export, these names cannot be coverted to GUIDs and in configs, they also need to be specified by name only.

The reason I'd like a table of these names is that it would make it easier to figure out how to manage translation of apps between name and GUID, specifically which ones not to attempt to translate.

An interesting option turns up here, because what if a tenant contains a real app named 'Office365'. If a configuration refers to it by name without any 'filtering for built-in names' then it will be converted to a GUID. If the intention is to target the entire group of Office 365 apps covered by the same logical name that would fail. Conversely, if the intention is to target that specific app, it needs to be specified by GUID. If 'filtering for built-in names' is implemented then specifying such an app-name would always result in referring to the 'group of apps'. What I have so far is a table of just two names, 'Office365' and 'All' but it should be expanded. It will be simple to do it manually but not very effective. That's why I throwing it out here in the hope that someone knows of an official list.

salbeck-sit commented 11 months ago

At the moment only 3 'logical apps' exist: All, Office365 and MicrosoftAdminPortals

salbeck-sit commented 11 months ago

I've put in a PR. Let me know what you think.

ricmestre commented 11 months ago

Won't be able to test it in the near future but it looks like you integrated what you said initially plus what I've mentioned so basically it looks first for known names (currently only office365 and all), then for guid, then for name (which it translates to guid internally) and lastly if everything fails keeps the name as-is.

It looks good to me!