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.

Sign up for GitHub

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

Jump to bottom

Westinm/trimming #2210

Merged
merged 16 commits into from
May 13, 2023
Merged

Westinm/trimming #2210

merged 16 commits into from
May 13, 2023

Conversation

westin-m
Copy link
Contributor

@westin-m westin-m commented Apr 26, 2023 •
edited
Loading

Make IdWeb trimmable. Where possible, change implementations to be trim-friendly. Otherwise, bubble-up the warnings to Apps calling our library. More info on trimming is here.

Published File Size in KB Using the 4-1-MyOrg Sample (.net7)

Filename Before After % Change
Microsoft.Identity.Web.Certificate.dll 28 22 21%
Microsoft.Identity.Web.Certificateless.dll 12 12 0%
Microsoft.Identity.Web.Diagnostics.dll 13.5 9 33%
Microsoft.Identity.Web.dll 149 149 0%
Microsoft.Identity.Web.DownstreamApi.dll 53.5 53.5 0%
Microsoft.Identity.Web.TokenAcquisition.dll 122 88.5 27%
Microsoft.Identity.Web.TokenCache.dll 46 18.5 60%
Microsoft.Identity.Web.UI.dll 15 15 0%
Microsoft.Identity.Web.UI.Views.dll 17.5 17.5 0%

bgavrilMS
bgavrilMS reviewed May 1, 2023
View reviewed changes
Directory.Build.props Outdated Show resolved Hide resolved
@westin-m westin-m marked this pull request as ready for review May 12, 2023 18:34
@westin-m westin-m linked an issue May 12, 2023 that may be closed by this pull request
Microsoft.Identity.Web failing to validate audience with PublishTrimmed=true #2098
Closed
@westin-m westin-m requested review from jmprieur and jennyf19 May 12, 2023 21:53
jennyf19
jennyf19 approved these changes May 13, 2023
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit:

jmprieur
jmprieur approved these changes May 13, 2023
View reviewed changes
Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM
I left a question

We can address it in a later iteration (with .NET 8). But would want to capture that in a work item.

src/Microsoft.Identity.Web/AuthorizeForScopesAttribute.cs
@@ -93,7 +90,7 @@ public override void OnException(ExceptionContext context)
IDWebErrorMessage.ScopeKeySectionIsProvidedButNotPresentInTheServicesCollection,
nameof(ScopeKeySection)));
}
string? scopeKeySectionValue = configuration.GetValue<string>(ScopeKeySection);
string? scopeKeySectionValue = configuration[ScopeKeySection];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the section be a single property value? Did you test it?

src/Microsoft.Identity.Web.TokenAcquisition/MicrosoftIdentityBaseAuthenticationBuilder.cs
@@ -22,6 +23,9 @@ public class MicrosoftIdentityBaseAuthenticationBuilder
/// </summary>
/// <param name="services">The services being configured.</param>
/// <param name="configurationSection">Optional configuration section.</param>
#if NET6_0_OR_GREATER
[RequiresUnreferencedCode("Calls Microsoft.Extensions.Configuration.ConfigurationBinder.Bind(IConfiguration, Object).")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do better in .NET 8? by enabling EnableMicrosoftExtensionsConfigurationBinderSourceGenerator ?
https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-8#source-generator-for-configuration-binding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I read, probably. I do want to try this and test it when we add .NET 8.

Directory.Build.props
@@ -29,6 +29,10 @@
<EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
</PropertyGroup>

<PropertyGroup Condition="'$(TargetFramework)' == 'net6.0' or '$(TargetFramework)' == 'net7.0'">
<IsTrimmable>true</IsTrimmable>
</PropertyGroup>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we also want to try

<EnableMicrosoftExtensionsConfigurationBinderSourceGenerator>true</EnableMicrosoftExtensionsConfigurationBinderSourceGenerator>

if we use Microsoft.Extensions.Configuration.Binder NuGet package 8.*?

and then remove the attributes which are due to the binding.

Shall we do that when we enable .NET 8?
@jennyf19 @westin-m ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do that as an increment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I want to try this with .NET 8.

@westin-m westin-m merged commit e86d3c0 into master May 13, 2023
@westin-m westin-m deleted the westinm/trimming branch June 15, 2023 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennyf19 jennyf19 jennyf19 approved these changes

@bgavrilMS bgavrilMS bgavrilMS left review comments

@jmprieur jmprieur jmprieur approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Microsoft.Identity.Web failing to validate audience with PublishTrimmed=true
4 participants
@westin-m @bgavrilMS @jmprieur @jennyf19