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.

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 Android Device Administrator Basic Wi-Fi Policy #3989

Closed

Conversation

FabienTschanz
Copy link
Contributor

@FabienTschanz FabienTschanz commented Dec 1, 2023

Pull Request (PR) description

This pull request updates the DSC resource for the Android Device Administrator Wi-Fi profile, specifically the "Basic" version of that Wi-Fi profile.

This change most likely results in a breaking change because the old DSC resource no longer exists.

More of these pull requests will be created soon, where the management of the Enterprise and Basic Wi-Fi profiles for Android systems will change.

  • Renames the IntuneWiFiConfigurationPolicyAndroidDeviceAdministrator to [...]Basic, making clear that this is the "Basic" Wi-Fi profile. Management of the "Enterprise" variant will be implemented with a separate pull request.
  • Updates the modifiable properties and allowed values. For a "Basic" Wi-Fi profile, the WiFiSecurityType is always open. Only Enterprise profiles have the wpaEnterprise resp. wpa2Enterprise possibilities.

This Pull Request (PR) fixes the following issues

@FabienTschanz FabienTschanz force-pushed the issue-3975/fix-policy branch 2 times, most recently from 5d50f5a to 4b67412 Compare December 1, 2023 08:15
@ricmestre
Copy link
Contributor

Not sure if I'll be able to test this out today but at first glance LGTM, thank you.

@ricmestre
Copy link
Contributor

@FabienTschanz Giving a little bit more of though on this, not sure if IntuneWifi* policies ever worked except for being able to be exported and this would be considered a breaking change because of that, but if it is a breaking change then the actual fix to the issue I reported, which is the condition in Test-TargetResource, would only be applied along with the rest of the changes next April, so it's not good, maybe just create a PR for now that fixes that and the bigger change it's done next year?

Another thing is that if you're splitting them into Basic(open)/Enterprise(wpa) then you need to change Export-TargetResource and add a filter in Get-MgBetaDeviceManagementDeviceConfiguration looking for { ... -and $_.WiFiSecurityType -eq 'open' }

@FabienTschanz
Copy link
Contributor Author

@ricmestre Didn't know about the breaking change in April, sorry about that. I can create a separate pull request though, but fixing something that isn't really working anyways and not make the whole package good is a bit troublesome 😕

The other point you mentioned: According to my testing, that's already handled with the @odata.type. You see, the Basic Wi-Fi profiles all have the type #microsoft.graph.androidWiFiConfiguration, whereas the Enterprise profiles all are of type #microsoft.graph.androidEnterpriseWiFiConfiguration.

@ricmestre
Copy link
Contributor

Yeah, it's a bummer if it's considered a breaking change even if these policies never worked before, @NikCharlebois @andikrueger @ykuijs could you please chime in here and let us know if such a change is considered as a breaking change even if it never worked except for exporting?

Didn't notice about the @odata.type but you're right if the security types are already differentiated by their type then yeah the filter won't be needed after all.

@ykuijs
Copy link
Member

ykuijs commented Dec 1, 2023

A rename of the resource would indeed mean a breaking change, unless it never could have worked anyways. In that case existing configurations couldn't have used the resource and therefore renaming it will never break existing configurations.

I don't have much Intune expertise, so can't say much about the original resource. @William-Francillette Can you share your insights into this issue? Would the original resource have worked and is this a breaking change or not?

@andikrueger
Copy link
Collaborator

The discussion reminds me of #2794 (comment) This is somehow similar and still different. If I understand it correctly, this PR includes more than the actual fix to the part of the resource that did never work.

Is there any chance to split this PR or unify both (basic and enterprise) profiles in one resource? If this makes any sense.
If not, I would suggest to fix the original issue now, still allow all previous parameters and write warnings about them being deprecated. The rename of this resource to basic could be done with the BR release in April; The new resource for Enterprise could still be added now as an additional resource.

@FabienTschanz
Copy link
Contributor Author

@andikrueger Splitting the PR is possible, but I strongly discourage to unify both of the possible profiles (Basic / Enterprise) into the same. As already mentioned in #3993, because if either of the types is chosen, they cannot be converted to the other type without creating a new policy.

Pretty much all of the Android Wi-Fi DSC resources are bugged as they are now and can not work in any way that makes sense for what DSC was intended to do... Either the export works, or the import works, but the patch would create a new one and so on.

Maybe you want to discuss the greater impact of the multiple PRs I made today with your colleagues in a bigger round and not on an individual case, and let me know how you would like to proceed. If you want to have the raised issues of @ricmestre fixed by the PRs, that's fine by me.

What is the process for updating a resource with a breaking change? Do I have to wait until April, postponing all the changes until then and only at that point create the PRs to address the issues with a profounder impact on M365DSC?

@FabienTschanz
Copy link
Contributor Author

@andikrueger Any updates on the current state? Can you answer my questions and let me know the next steps you would like me to do?

@William-Francillette
Copy link
Contributor

too many discussion threads for this 🤷‍♂️
have a look at my last comment on #3993
imo breaking change + changing the base of the resource is not a repeatable process.
we need a baseline so that we can generate those resources down the line

@FabienTschanz FabienTschanz deleted the issue-3975/fix-policy branch April 26, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants