-
Notifications
You must be signed in to change notification settings - Fork 517
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
Fix Intune device enrollment platform restrictions #4362
Conversation
e0a2cbd
to
f89f448
Compare
f89f448
to
07afeaa
Compare
Hi @FabienTschanz, I see that you have changed the Key parameter from the DisplayName to the Identity property. What is the reasoning behind this? We have tried to do that last year of many of the Intune resources, but this caused a lot of issues with users. (Reviewed the PR you are mentioning. Will merge shortly) |
@ykuijs That is because IntuneEnrollmentPlatformRestrictions policies, specifically for Android enrollment policies, there can two resources be created: One for the --> Each row creates a separate object under the same "abc" policy in the UI. If we use the display name as the key, compiling the M365TenantConfig will unfortunately result in a conflict because two resources have the same display name. Switching to Key resolves that issue because then multiple policies can have the same display name and thus be merged in the Intune portal UI under the same policy. I'm sure that must sound like bogus to some, but unfortunately that's how it's implemented. If I was to create a Android policy right now with both of these settings set to Allow, and then I wanted to export and compile that, it wouldn't work because the display names of these policies were the same... |
Hi @FabienTschanz, thanks for this explanation. That indeed is a problem. Just not sure if making the Identity parameter a Key parameter is the best solution here. When looking at the resource, it current has two different properties for Android and AndroidForWork: If so, shouldn't we update the resource to include all platform restrictions for the same policy into a single resource? I have some doubts on the change you are suggesting: If I can create two instances for the same policy but with different settings, how does that work: When applying the settings for the second instance, what will have to happen with the settings of the first instance? Do they have to be overwritten or not? This all is solved if the settings are all included into one instance per policy: That is the single version of the truth and DSC can simply apply that. |
@ykuijs As far as I remember, originally, the enrollment platform restrictions were policies, where one would specify the settings for all platforms in the same policy, like the default policy that spans over all Android, AndroidForWork iOS, Windows etc. That approach was changed in 2023 to splitting all platforms into separate resources. The I agree with you that the better solution is to map both Android platforms into the same policy object. Now I also understand why there was such a complex logic in the DSC resource to map those Do you want me to adapt the proposed logic to my change? I don't think it's too much trouble, given that pretty much everything is already there and only needs to be grouped + verified in a proper way. |
@William-Francillette My lack of Intune expertise prevents me from saying something sensible here. Can you please share your view here? |
I'm not sure what are we trying to fix in that PR because the issue mentioned has already been fixed by @ricmestre in #4247. The singlePlatform resource was already implemented and the resource will work for both the default policy (multi-platform) and single platform. |
Now that the other PR #4247 was merged I'll have to open a new one with the fix for the issue I reported on #4082, but yeah I was also having an hard time trying to figure out what this was trying to fix but I tested it and now I know what's going on. Someone decided that the Android restrictions should be placed under the same umbrella in the Intune admin portal, nevertheless when exporting with M365DSC it creates 2 separate entries for the same restriction, see below, and of course this configuration cannot be compiled since their DisplayName is the same and it should be unique since is set as Key. The other platform restrictions don't seem to have this problem only Android so I'd say the PR should be changed to place the two Android restrictions under a single resource (of course you can still have multiple resources/policies with different restrictions). IntuneDeviceEnrollmentPlatformRestriction "IntuneDeviceEnrollmentPlatformRestriction-test"
{
AndroidForWorkRestriction = MSFT_DeviceEnrollmentPlatformRestriction{
platformBlocked = $True
personalDeviceEnrollmentBlocked = $False
};
Credential = $Credscredential;
Description = "";
DeviceEnrollmentConfigurationType = "singlePlatformRestriction";
DisplayName = "test";
Ensure = "Present";
Identity = "50761f20-24c7-4850-bb84-d82d44b96d4e_SinglePlatformRestriction";
Priority = 1;
}
IntuneDeviceEnrollmentPlatformRestriction "IntuneDeviceEnrollmentPlatformRestriction-test-2"
{
AndroidRestriction = MSFT_DeviceEnrollmentPlatformRestriction{
platformBlocked = $True
personalDeviceEnrollmentBlocked = $False
};
Credential = $Credscredential;
Description = "";
DeviceEnrollmentConfigurationType = "singlePlatformRestriction";
DisplayName = "test";
Ensure = "Present";
Identity = "f3d09e8d-494c-42e7-92b5-693db5974948_SinglePlatformRestriction";
Priority = 1;
} |
Thank you @ricmestre for the explanation. That's exactly what I intended to say, seems like my point didn't get across well enough. Sorry about that. |
OK, just gave it a quick glance and I don't need to open a new PR for fixing the Assignments this one already fixes my issue #4082 . |
okie dokie, so the issue is with the key parameter in the schema - Can we just removed the key as having displayName as a key is not correct in that case? we are actually in the same dilemma of not aligning with the Graph API and considering the displayname as a key when the Intune portal doesn't. |
They have different Identity so one could also argue that they're separate restrictions, which they are but just happen to have the same name, so it'd be a matter of setting it as key and remove it from DisplayName, I just did that with the current code and it seems to work without issues. Cloning to a new tenant would be just a matter of keeping something in the Identity field, can be the same ones used from the source tenant, and ensure the DisplayName is the same for both restrictions so that they appear in the Intune portal, BUT this now would bring a few other issues. For some reason it took 2 deployments to actually create both policies, probably a fluke but now basically whenever you want to make a change to the policies in the target tenant since the Identity is from the source it searches the policies by name right? Guess what happens? Now it finds 2 different policies with the same name and returns that array instead of a single restriction so some filtering on the platform must be done when searching by DisplayName, additionally the Identity property must also be excluded from being tested in Test-TargetResource and probably something else I'm forgetting about right now. |
@ricmestre I fixed the searching by displayname you mentioned if the Identity is not found by additionally querying the -FilterScript { `
$_.AdditionalProperties.'@odata.type' -like "#microsoft.graph.deviceEnrollmentPlatformRestriction*Configuration" -and `
$(if ($null -ne $_.AdditionalProperties.platformType) { $_.AdditionalProperties.platformType -eq $PlatformType } else { $true }) `
} Buuuut the caveat is that nobody stops you from creating two or more policies with the same name for the same restriction type... We would have to deal with that too. |
Wasn't looking at your code, just figuring out the issues I found with the current code but yeah that would fix it and yes we need to ensure that only a single policy is returned, if more than one than it should throw an error message like it's already done in some other DSC resources. |
Okayyy, so let's sum things up for the moment:
What are the next steps? I suggest the following:
In this PR, two of those fixes are already present. If you all agree, I'd update the PR to not include such drastic changes that I currently would introduce (@William-Francillette you confirmed it works for the single platform approach, and I understand now that it does exactly that), but additionally introduce behavior checks for the Android platforms, such that they are exported as a union and their handling is consistent. What do you think? Can we move forward with that? |
yes they are 2 different policies in the backend, if you create one using the Graph API it won't show in the Intune portal until the other one exist: those 2 Android policies must exist and have the same name if you want to have a consistent behaviour when configuring device restriction policies - I've tested those different scenarios a while ago and the way displayname is handled is not really consistent for this policy so having them in separate instances was easier to maintain. Here, we should have an agreement on how we want to handle this Graph policy and then implement the resource accordingly - if we judge that the current one is not correct then we need to agree on the best course of action. |
We can't apply that logic here because if you want to apply the restriction for any Android both must exist with the same name |
That's a good point that we must encounter for |
Not exactly. We can distinguish the Android policies by their platform type, so if we get two policies as a result with the same name, and both platform types are present, we can be sure that they're ok. My filtering should already apply for that case and only select the appropriate one. It needs a bit of tweaking, but checking for both types and having the same name isn't that difficult. |
Don't want to stop anyone here, I have a lot on my plate already as of now, but clearly 1. is the quickest fix, as for 2 that I suggested earlier now I'm thinking should we really do that? I'm not even taking in the fact that it's much more work than 1. but what would you use for Identity in that case? Those Android restrictions are 2 separate policies with their own Identity, so how do you differentiate them under the same resource, always search by DisplayName if platform is Android? Join the string of both Identities and split them when searching? Just choose one of the 2? |
If 2 is chosen, we should ditch retrieving the graph policy using Identity and only use displayName If the Graph cmdlet retrieve 2 policies Set should create and update both android and androidforwork Export should combine the android and androidforwork Same here @ricmestre, I'm a bit worried about the amount of work and test required for that |
I understand both ways and I'm fine to implement both variations, whichever option is selected. In my opinion, the overhead won't be as much as what's currently estimated, but we will see when it's time to implement. I understand both variations and both have dis-/advantages for themselves. We must look at what the customers might think: Do they understand that if option 1 is selected, they must create two Android resources, one for each platform type, before a policy in Intune will exist? Or if option 2 is specified, do they know that specifying both resources in the same object is necessary only for the Android policy? Or will they specify even more resources? Personally, option 2 in terms of customer usability is probably the preferred way, but I don't have nearly enough experience as some of you have. In terms of engineering and maintainability, option 1 is much more preferred. |
I just read through the whole conversation and am now trying to put two and two together. There is one thing I'm wondering about. Maybe it is just my lack of in-depth knowledge in this area of Intune: Is there any possible way that a restriction for ANDROID might be different from another ANDROID FOR WORK one? |
@andikrueger Intune portal joins together both Android restrictions into a single one if they have the same DisplayName but they are effectively 2 independent Android policies. On top of that then you can have multiple more Android restrictions with other names, priorities and assignments. Edit: So yes, in the same policy/restriction you can block Android and allow Android for Work since they are independent, just shown together in the portal. |
@ricmestre Thanks! With you Edit in mind: Wouldn't we break this kind of option then? |
yeah breaking the policy would be option 1 and we would only need to remove the key |
Conclusion: Option 1 would be a simpler solution and the one with more options for configuring restrictions? |
Want to chip in a few thoughts as well:
|
Windows, iOS and MacOS are also separated resource but they only create one resource per policy |
Ok, then I am not sure if I get it. The current situation is one resource that is able to create policies with restrictions for all platforms. What is the proposed solution?? |
@ykuijs Currently, one resource in DSC creates one policy in Intune, except for Android, there are two resources necessary with the same display name. That's also what option 1 would suggest to continue using. Option 2 would be one DSC resource per policy, even for the Android platform. There, the DSC resource would manage both Android platforms. |
Where do we stand on this PR? I haven't been keeping up with all the messages in this thread, but are we moving forward with the proposed changes? |
If I understand it correctly, option 1 is the preferred option. Is that correct? Let's go ahead and implement that. Just to get it clear on my end:
|
@FabienTschanz, would it be possible to complete this PR before the end of this week? Then we can merge and include it in next weeks Breaking Change release. |
@ykuijs Should be possible, although it will most likely take up to Friday evening before it's complete. I will revert some changes and reuse the previously existing code, alongside the necessary changes for option 1.
|
Implementation pending - Stay tuned. Do not merge yet. |
Perfect. Please make sure the Readme.md for both resources include the information that you should use both resources to have that appear correctly in the Intune portal. |
8412af9
to
359110e
Compare
Done, all is working now. Requesting review please. |
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.
One small comment
[Write, Description("Identity of the device enrollment platform restriction.")] String Identity; | ||
[Key, Description("Display name of the device enrollment platform restriction.")] String DisplayName; | ||
[Key, Description("Identity of the device enrollment platform restriction.")] String Identity; | ||
[Write, Description("Display name of the device enrollment platform restriction.")] String DisplayName; |
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.
I think this should be Required. We need a DisplayName if the ID does not exist.
@FabienTschanz, could you please resolve the comment asap? @NikCharlebois, can this PR be included in todays release? It is a breaking change, so don't want to push this one to October. |
[System.String] | ||
$Identity, | ||
|
||
[Parameter(Mandatory = $true)] | ||
[Parameter()] |
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.
If the parameter is required, this one also needs to stay Mandatory=$true (and in the other methods as well)
@ykuijs Done, please have another look. Should all be good now. |
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.
LGTM
Pull Request (PR) description
This pull request addresses multiple issues in the
IntuneDeviceEnrollmentPlatformRestriction
resource:Attention: The implementation uses code that is not yet merged --> #4247 . That pull request must first be merged before this one can be merged.
This Pull Request (PR) fixes the following issues