Makes RemoteAuthenticationContext serialize correctly

[lewishazell]

Makes RemoteAuthenticationContext serialize correctly

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

This PR ensures that RemoteAuthenticationContext property assignments are not optimized away when only used in serialization.

Description

The entire context is serialized for JSInterop and the State and InterativeRequest properties are lost during serialization. This seems to be because the only usage is for serialization and the property assignments are compiled out.

I think this may be the root cause of #53131, which I've also seen in our production application. The lack of the State property after serialization causes issues with the logout callback.

The following code sample demonstrates this behaviour:

protected override void OnInitialized()
{
    base.OnInitialized();

    var currentContext = new RemoteAuthenticationContext<RemoteAuthenticationState>();
    currentContext.State = new RemoteAuthenticationState() { ReturnUrl = "/" };
    Console.WriteLine($"currentContext: {JsonSerializer.Serialize(currentContext)}");

    var proposedContext = new DemoRemoteAuthenticationContextWithAttribute<RemoteAuthenticationState>();
    proposedContext.State = new RemoteAuthenticationState() { ReturnUrl = "/" };
    Console.WriteLine($"proposedContext: {JsonSerializer.Serialize(proposedContext)}");
}

// explicit member types as LinkerFlags is internal
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.PublicProperties)]
public class DemoRemoteAuthenticationContextWithAttribute<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.PublicProperties)] TRemoteAuthenticationState> where TRemoteAuthenticationState : RemoteAuthenticationState
{
    public string? Url { get; set; }

    public TRemoteAuthenticationState? State { get; set; }

    public InteractiveRequestOptions? InteractiveRequest { get; set; }
}

Which gives the following output in a browser:

currentContext: {}
proposedContext: {"Url":null,"State":{"ReturnUrl":"/"},"InteractiveRequest":null}

Fixes #53131

[ghost]

Thanks for your PR, @lewishazell. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[mkArtakMSFT]

Thanks for your PR, @lewishazell.
@halter73 can you please review this? Thanks!

[dotnet-policy-service]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

[azure-pipelines]

Commenter does not have sufficient privileges for PR 53819 in repo dotnet/aspnetcore

[halter73]

Thanks for the PR. I am going to close this in favor of #54225 because the type-level [DynamicallyAccessedMembers] only helps if something calls RemoteAuthenticationContext.GetType() which isn't the case here. A class-level [DynamicallyAccessedMembers] attribute does not tell the trimmer if any part of the type is preserved, preserve all parts of the type. You can look dotnet/linker#1196, dotnet/runtime#49465, and dotnet/linker#1929 for the original motivation and test cases that were added when [DynamicallyAccessedMembers] was first allowed to target classes.

The reason that it appeared to work with the DemoRemoteAuthenticationContextWithAttribute test case is because Blazor does not trim application assemblies during publish. It defaults to partial trim mode meaning that trims projects that have opten into trimming with "IsTrimmable" metadata like Microsoft.AspNetCore.Components.WebAssembly.Authentication. If you trim the application assembly by adding <TrimMode>full</TrimMode> to the csproj, you will see the same issue with DemoRemoteAuthenticationContextWithAttribute that you do with RemoteAuthenticationContext. If you try this, you'll probably want to move the sample code from OnInitialized() to Program.cs because full trimming prevents Blazor from being able to even activate components.

In #54225, I used [DynamicallyAccessedMembers] on a generic parameter instead, but I imagine adding a [DynamicDependency(DynamicallyAccessedMemberTypes..., typeof(RemoteAuthenticationContext<RemoteAuthenticationState>))] on the appropriate members would also work.

cc @MichalStrehovsky @eerhardt to see if it makes sense to make class-level [DynamicallyAccessedMembers] attributes behave the way the submitter of the PR thought it did where if any part of the type is preserved, all parts of the type are preserved. Or to see whether we can do anything to eliminate this confusion.

[MichalStrehovsky]

cc @MichalStrehovsky @eerhardt to see if it makes sense to make class-level [DynamicallyAccessedMembers] attributes behave the way the submitter of the PR thought it did where if any part of the type is preserved, all parts of the type are preserved. Or to see whether we can do anything to eliminate this confusion.

The purpose of these annotations is to make it possible to statically prove that some reflection done at runtime is safe. The fact that these annotations prevent things from being trimmed is secondary. Class-level [DynamicallyAccessedMembers] was introduced to make it possible to statically prove someType.GetType().GetField(...) etc. can be done safely and a build-time warning can be avoided when someType is typed as something that has these annotations on the class/interface.

I don't think there's a pattern that would allow us to prove something is trim-safe by simply keeping members on types that were kept (the proposed new semantic). It would only keep extra stuff, but there would still be trimming warnings at the point where this reflection is done.

So no plans for extending the semantic to indiscriminately keeping members.

[lewishazell]

@halter73 No problem, really glad it helped get to the root of the issue even if our fix isn't entirely correct. It was a difficult one to test for on our side. Will look over your PR with great interest.