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

Remove Debug and Release Entitlements plists and Replace with Custom Entitlements #16355

Closed
wants to merge 22 commits into from

Conversation

dustin-wojciechowski
Copy link
Contributor

@dustin-wojciechowski dustin-wojciechowski commented Jul 25, 2023

UPDATE: I've reopened this PR to keep everything together. I've added two of the entitlements that were in this PR to the iOS sdk, but we agreed that the issue with adding the com.network.client as a default could cause issues during notarizing in that not all MacCatalyst apps require that entitlement. This was a requirement that is strictly unique to MAUI Blazor applications, so I have left that entitlement in there.

But open to suggestions if there is a more favorable way to do this.

Description of Change

CustomEntitlements in csproj should replace need for adding Entitlement .plist files for Debug and Release for MacCatalyst templates.

Ideally we should try to move this away from the csproj in Maui project altogether and into iOS sdk(xamarin/xamarin-macios#18344). Also, unique configurations based on debug and publish intentions has been proposed for dotnet but doesn't seem to have any traction: dotnet/sdk#31918

@dustin-wojciechowski
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dustin-wojciechowski dustin-wojciechowski added area-templates Project templates, Item Templates for Blazor and MAUI platform/macOS 🍏 macOS / Mac Catalyst area-blazor Blazor Hybrid / Desktop, BlazorWebView Blazor ❤️ MAUI Issues in MAUI functionality that affect Blazor, but are not bugs in Blazor itself labels Jul 27, 2023
@dustin-wojciechowski dustin-wojciechowski changed the title Dev/update templates for blazor Remove Debug and Release Entitlements plists and Replace with Custom Entitlements Jul 27, 2023
@dustin-wojciechowski dustin-wojciechowski marked this pull request as ready for review July 27, 2023 18:45
@dustin-wojciechowski dustin-wojciechowski requested a review from a team as a code owner July 27, 2023 18:45
@dustin-wojciechowski
Copy link
Contributor Author

Still need to test this through TestFlight as well, but getting this out for visibility.

Copy link
Member

@Redth Redth left a comment

Choose a reason for hiding this comment

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

Ultimately I think these should go into the macios sdk workload. Android already does this for some debug and release config defaults.

@dalexsoto @rolfbjarne @jonathanpeppers

Thoughts?

@samhouts samhouts added this to the .NET 8 GA milestone Jul 31, 2023
@rolfbjarne
Copy link
Member

Ultimately I think these should go into the macios sdk workload. Android already does this for some debug and release config defaults.

@dalexsoto @rolfbjarne @jonathanpeppers

Thoughts?

I agree, this is not limited to MAUI so we can move this to defaults in xamarin-macios instead.

@dustin-wojciechowski
Copy link
Contributor Author

I am preparing a PR for the xamarin-macios repo to hopefully include the files there, so I will close this PR.

@dustin-wojciechowski dustin-wojciechowski deleted the dev/update-templates-for-blazor branch August 2, 2023 19:52
@Redth
Copy link
Member

Redth commented Aug 16, 2023

@dustin-wojciechowski dustin-wojciechowski restored the dev/update-templates-for-blazor branch August 28, 2023 22:15
@mattleibow
Copy link
Member

mattleibow commented Sep 8, 2023

Yeah, have a look and see if there is something we can do. We have the dotnetinternal class, so maybe we can add a appleinternal and have a build then check.

I did something for windows #17238

You can also skip tests where this won't work. Not sure if this will run in non mac.

@dustin-wojciechowski
Copy link
Contributor Author

dustin-wojciechowski commented Sep 14, 2023

Update: An integration test that uses codesign to verify entitlements was introduced separately in this PR: #17331. That passes in main because the entitlements are being added via the template.

The latest commit successfully adds the network.security.client entitlement to the executable, but the tests will still fail because the PR in xamarin-macios that adds the com.security.get-task-allow and com.security.app-sandbox entitlements (xamarin/xamarin-macios#18669) will be released in NET8.0.100 RC 2. Once that is released, and the version.props file is updated to consume the latest maccatalyst workload, all tests should pass and this PR should be good to be merged.

…. Also added entitlements that are missing and will be added at a later date to the ios sdk
@samhouts samhouts modified the milestones: .NET 8 GA, Under Consideration Sep 19, 2023
@dustin-wojciechowski
Copy link
Contributor Author

As we're closely approaching NET8 GA, I can add all the entitlements to the MAUI SDK now, and then take them out when maui is able to consume macios SDK NET8 RC2 successfully.

Two options:

  1. Get this PR in now (all tests pass) and then later PR to remove the entitlements that were added into the macios sdk (no real impact, just a redundancy
  2. Suspend this PR until we get the latest feed from macios.

Let me know what you think.

Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

This change doesn't sit quite right with me because I feel it's hard-coding too much Blazor Hybrid template functionality and hides it behind an assumption of how the project is structured. That's fine when it's within the template because the developer can tweak as needed, but here it's in some targets that can be hard to modify the behavior of.

<Project>

<PropertyGroup>
<EnableDefaultMauiBlazorEntitlements Condition="'$(EnableDefaultMauiBlazorEntitlements)'=='' And '$(UsingMicrosoftNETSdkRazor)'=='true'">true</EnableDefaultMauiBlazorEntitlements>
Copy link
Member

Choose a reason for hiding this comment

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

Hmm this essentially looks like it's hard-coding some Blazor Hybrid logic into this target, which seems a bit too specific. I'm not sure this logic is quite right, either, because I don't think the MAUI project itself has to necessarily use the Razor SDK, yet the app likely still wants to use these entitlements if it has Blazor usage in it (perhaps indirectly through a project reference).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I got the idea from the Microsoft.Maui.Sdk.ProjectCapabilities.targets file. It would have been more favorable to use the MauiBlazor ProjectCapability directly, but the condition checking for that wasn't working in the integration test.

I had not thought of an indirect reference without using the Razor SDK though. I will investigate and see if I can come up with an alternative means.

Copy link
Member

Choose a reason for hiding this comment

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

Oh weird... I don't really like that either. Looks like it's ancient. I think it's a reasonable for telemetry but I'm not sure I like it for functional purposes... Though it doesn't even necessarily look right for that because it doesn't actually check if it's MAUI SDK + Razor, though probably that's fine because this very target is from MAUI to begin with.

Copy link
Member

Choose a reason for hiding this comment

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

should this rather be in the maui blazor webview nuget? Then installing the webview nuget also sets this?

@dalexsoto
Copy link
Member

@Eilon @mattleibow if this doesn't belong to the targets what do you suggest we do here? Maybe the fact that this is already in the MacCat blazor templates is good enough and we can close this?

<EnableDefaultMauiBlazorEntitlements Condition="'$(EnableDefaultMauiBlazorEntitlements)'=='' And '$(UsingMicrosoftNETSdkRazor)'=='true'">true</EnableDefaultMauiBlazorEntitlements>
</PropertyGroup>

<ItemGroup Condition="'$(EnableDefaultMauiBlazorEntitlements)'=='true'And '$(Configuration)' == 'Release'">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<ItemGroup Condition="'$(EnableDefaultMauiBlazorEntitlements)'=='true'And '$(Configuration)' == 'Release'">
<ItemGroup Condition="'$(EnableDefaultMauiBlazorEntitlements)'=='true' And '$(Configuration)' == 'Release'">

<CustomEntitlements Include="com.apple.security.network.client" Type="boolean" Value="true" />
</ItemGroup>

<ItemGroup Condition="'$(EnableDefaultMauiBlazorEntitlements)'=='True'And '$(Configuration)' == 'Debug'">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<ItemGroup Condition="'$(EnableDefaultMauiBlazorEntitlements)'=='True'And '$(Configuration)' == 'Debug'">
<ItemGroup Condition="'$(EnableDefaultMauiBlazorEntitlements)'=='True' And '$(Configuration)' == 'Debug'">

@Eilon
Copy link
Member

Eilon commented Sep 19, 2023

@Eilon @mattleibow if this doesn't belong to the targets what do you suggest we do here? Maybe the fact that this is already in the MacCat blazor templates is good enough and we can close this?

Maybe do nothing for now? I don't like hiding project-specific settings in the SDK because then it becomes a huge challenge when someone wants to change the value for their project. The thing I don't know is whether this is just "always fine" and not really cause anyone problems in the future - I'm not familiar enough with these settings/behaviors to have a good sense of that.

If we're super duper sure that this conditional logic and behavior is almost certainly right for almost all apps, then maybe it's OK. If we're not so sure, then let's leave them as project template settings. (I don't like having junk in projects, either, but sometimes it's really not junk).

@mattleibow mattleibow modified the milestones: Under Consideration, .NET 8 GA Sep 28, 2023
@mattleibow
Copy link
Member

Moving this to GA as if we downgrade to net7 without removing the hardend runtime property, mac catalyst crashes. Once net8 ships, this will live in peoples' templates forever and it makes it look sad.

So fix soon, ship soon, happy forever!

@mattleibow mattleibow added the do-not-merge Don't merge this PR label Sep 28, 2023
@Eilon
Copy link
Member

Eilon commented Sep 28, 2023

Moving this to GA as if we downgrade to net7 without removing the hardend runtime property, mac catalyst crashes. Once net8 ships, this will live in peoples' templates forever and it makes it look sad.

So fix soon, ship soon, happy forever!

Not sure I understand what "downgrade to net7" means? Also, what is the harm of having "this" (not sure what "this" is?) live in peoples' templates? Is the alternative to put this in the SDK and then we break peoples' apps when we decide to change the defaults (that's part of what I'm worried about)?

@mattleibow
Copy link
Member

mattleibow commented Sep 28, 2023

this refers to the issue. downgrade refers to file new maui net8, and then changing the tfm to net7.

But I would expect the basic parts needed to make a blazor app work be in the box and not in the templates. If these are not needed, then why are they in the template at all.

We have thousands of defaults already in the box, both the sdk and in android/ios. So, if blazor needs these or works best with them then we should set it.

The comment says it is essential - and thus does not qualify as a basic "default".

<!--Essential for MAUI Blazor, as it allows your sandboxed app to connect to a server process.-->
<CustomEntitlements Include="com.apple.security.app-sandbox" Type="boolean" Value="true" />
<CustomEntitlements Include="com.apple.security.network.client" Type="boolean" Value="true" />

If they are not essential, then what are they for?

<Project>

<PropertyGroup>
<EnableDefaultMauiBlazorEntitlements Condition="'$(EnableDefaultMauiBlazorEntitlements)'=='' And '$(UsingMicrosoftNETSdkRazor)'=='true'">true</EnableDefaultMauiBlazorEntitlements>
Copy link
Member

Choose a reason for hiding this comment

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

should this rather be in the maui blazor webview nuget? Then installing the webview nuget also sets this?

</ItemGroup>

<ItemGroup Condition="'$(EnableDefaultMauiBlazorEntitlements)'=='True'And '$(Configuration)' == 'Debug'">
<CustomEntitlements Include="com.apple.security.get-task-allow" Type="boolean" Value="true" />
Copy link
Member

@mattleibow mattleibow Sep 28, 2023

Choose a reason for hiding this comment

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

This is in the iOS sdk now so we can leave it out? If this is just needed because main is net7, then we need an issue tracking this to be removed in the net8 branch asap and it needs to be marked for GA using the Milestones.

@dustin-wojciechowski
Copy link
Contributor Author

Per the discussion mentioned above:

  1. No entitlements will be added to either the macios or maui sdk's (entitilements currently in macios sdk will be reverted)
  2. Templates will be retained for maui-blazor and maui-mobile, with entitlements being consistent between debug and release configurations.

Please see current PR #17833 for adjustments to templates.

dalexsoto pushed a commit to xamarin/xamarin-macios that referenced this pull request Oct 12, 2023
rolfbjarne pushed a commit to xamarin/xamarin-macios that referenced this pull request Oct 13, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView area-templates Project templates, Item Templates for Blazor and MAUI Blazor ❤️ MAUI Issues in MAUI functionality that affect Blazor, but are not bugs in Blazor itself do-not-merge Don't merge this PR platform/macOS 🍏 macOS / Mac Catalyst
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants