Skip to content

Changing msgpack options #20031

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

Merged
merged 16 commits into from
Apr 3, 2020
Merged

Changing msgpack options #20031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3587eec
MessagePackHubProtocolOptions now expose a property with the used ins…
tebeco Mar 20, 2020
3b0d679
Changed the static cached list of default Resolvers from IList to IRe…
tebeco Mar 20, 2020
7ff6c3b
Update the test to use a CompositeResolver between default SignalRRes…
tebeco Mar 20, 2020
71b7aeb
Rename CreateDefaultFormatterResolvers to CreateDefaultMessagePackSer…
tebeco Mar 20, 2020
75a3406
Adding summary for SerializerOptions so that customer can get a bette…
tebeco Apr 1, 2020
0b2dd88
Update src/SignalR/common/Protocols.MessagePack/src/MessagePackHubPro…
tebeco Apr 2, 2020
62fa142
Update src/SignalR/common/Protocols.MessagePack/src/MessagePackHubPro…
tebeco Apr 2, 2020
2a433a8
Update src/SignalR/common/Protocols.MessagePack/src/MessagePackHubPro…
tebeco Apr 2, 2020
1ce3786
Update src/SignalR/common/Protocols.MessagePack/src/MessagePackHubPro…
tebeco Apr 2, 2020
b0a3284
fixing typo in summary
tebeco Apr 2, 2020
5188f5c
Update src/SignalR/common/Protocols.MessagePack/src/MessagePackHubPro…
tebeco Apr 2, 2020
e4217c6
Update src/SignalR/common/Protocols.MessagePack/src/MessagePackHubPro…
tebeco Apr 2, 2020
de9e7a7
Update src/SignalR/common/Protocols.MessagePack/src/MessagePackHubPro…
tebeco Apr 2, 2020
38b9369
Update src/SignalR/common/Protocols.MessagePack/src/MessagePackHubPro…
tebeco Apr 2, 2020
96095a7
Update src/SignalR/common/Protocols.MessagePack/src/MessagePackHubPro…
tebeco Apr 2, 2020
f3fdbdc
As the "<code>" section render a newline, reverting the "." at the en…
tebeco Apr 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions src/SignalR/common/Protocols.MessagePack/src/MessagePackHubProtocolOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,31 @@ namespace Microsoft.AspNetCore.SignalR
{
public class MessagePackHubProtocolOptions
{
private IList<IFormatterResolver> _formatterResolvers;
private MessagePackSerializerOptions _messagePackSerializerOptions;

public IList<IFormatterResolver> FormatterResolvers
/// <summary>
/// <para>Gets or sets the <see cref="MessagePackSerializerOptions"/> used internally by the <see cref="MessagePackSerializer" />.</para>
/// <para>If you override the default value, we strongly recommend that you set <see cref="MessagePackSecurity" /> to <see cref="MessagePackSecurity.UntrustedData"/> by calling:</para>
/// <code>customMessagePackSerializerOptions = customMessagePackSerializerOptions.WithSecurity(MessagePackSecurity.UntrustedData)</code>
/// If you modify the default options you must also assign the updated options back to the <see cref="SerializerOptions" /> property:
/// <code>options.SerializerOptions = options.SerializerOptions.WithResolver(new CustomResolver());</code>
/// </summary>
public MessagePackSerializerOptions SerializerOptions
Copy link
Member

Choose a reason for hiding this comment

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

Lets add xml comments.

  • Recommend setting .WithSecurity(MessagePackSecurity.UntrustedData) if they override the options
  • If you want to modify to the default options you need to assign the options back to the SerializerOptions after modifications:
SerializerOptions = SerializerOptions.WithResolver(new CustomResolver());

Copy link
Contributor Author

@tebeco tebeco Apr 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

quick question, as the API has been approuved, and thinking about the summary, i have a terrible idea....
This would make another PR i guess since this API Changed has been approuved already ?

(i have no idea if it's possible) but does adding this makes sense:
(pseudo code, i have no idea if it's something good or bad since it's intrusive and also how properly speak english and being both nice and clear to the customer)

if(env.IsDevelopment() && options.serializerOptions.MessagePackSecurity != MessagePackSecurity.UntrustedData)
{
  if(options.EnableWarningForUntrustedData)
  {
    logger.LogWarning("MessagePackSerializerOptions have been overriden with a ... security settings ...blablabla if intended, you can turn this warning off by settings EnableWarningForUntrustedData to false")

   could even points to a github issue explaining that API

  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The options.serializerOptions.MessagePackSecurity != MessagePackSecurity.UntrustedData would need some massaging because UntrustedData is a copy-on-write object so if they've 'enhanced' it at all, you wouldn't want that to reactivate the failure. You'd want to look into the object to see what settings it has to determine whether it's a secure instance. But let's first wait to see what the ASP.NET Core folks think of your overall idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as said it just something that crossed my mind while trying to find an idea for the summary (TBH I'm pretty bad at it)
Also i have no idea if it's even possible

Copy link
Contributor Author

@tebeco tebeco Apr 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

(previous rendering deleted)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

9F4F2CC0-8025-4A38-8E41-B157D003F475

Copy link
Contributor Author

Choose a reason for hiding this comment

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

welllll
I should remove the . since the <code> seems to create a paragraph right
@anurse ?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(pushed a revert commit to this) :

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Yep, drop the . after the <code>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(done, do i mark this thread as resolved if summary if ok for you ?)

{
get
{
if (_formatterResolvers == null)
if (_messagePackSerializerOptions == null)
{
// The default set of resolvers trigger a static constructor that throws on AOT environments.
// This gives users the chance to use an AOT friendly formatter.
_formatterResolvers = MessagePackHubProtocol.CreateDefaultFormatterResolvers();
_messagePackSerializerOptions = MessagePackHubProtocol.CreateDefaultMessagePackSerializerOptions();
}

return _formatterResolvers;
return _messagePackSerializerOptions;
}
set
{
_formatterResolvers = value;
_messagePackSerializerOptions = value;
}
}
}
Expand Down
45 changes: 8 additions & 37 deletions src/SignalR/common/Protocols.MessagePack/src/Protocol/MessagePackHubProtocol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public class MessagePackHubProtocol : IHubProtocol
private const int NonVoidResult = 3;

private readonly MessagePackSerializerOptions _msgPackSerializerOptions;

private static readonly string ProtocolName = "messagepack";
private static readonly int ProtocolVersion = 1;

Expand All @@ -52,37 +53,7 @@ public MessagePackHubProtocol()
/// <param name="options">The options used to initialize the protocol.</param>
public MessagePackHubProtocol(IOptions<MessagePackHubProtocolOptions> options)
{
var msgPackOptions = options.Value;
var resolver = SignalRResolver.Instance;
var hasCustomFormatterResolver = false;

// if counts don't match then we know users customized resolvers so we set up the options with the provided resolvers
if (msgPackOptions.FormatterResolvers.Count != SignalRResolver.Resolvers.Count)
{
hasCustomFormatterResolver = true;
}
else
{
// Compare each "reference" in the FormatterResolvers IList<> against the default "SignalRResolver.Resolvers" IList<>
for (var i = 0; i < msgPackOptions.FormatterResolvers.Count; i++)
{
// check if the user customized the resolvers
if (msgPackOptions.FormatterResolvers[i] != SignalRResolver.Resolvers[i])
{
hasCustomFormatterResolver = true;
break;
}
}
}

if (hasCustomFormatterResolver)
{
resolver = CompositeResolver.Create(Array.Empty<IMessagePackFormatter>(), (IReadOnlyList<IFormatterResolver>)msgPackOptions.FormatterResolvers);
}

_msgPackSerializerOptions = MessagePackSerializerOptions.Standard
.WithResolver(resolver)
.WithSecurity(MessagePackSecurity.UntrustedData);
_msgPackSerializerOptions = options.Value.SerializerOptions;
}

/// <inheritdoc />
Expand Down Expand Up @@ -656,17 +627,17 @@ private static object DeserializeObject(ref MessagePackReader reader, Type type,
}
}

internal static List<IFormatterResolver> CreateDefaultFormatterResolvers()
{
// Copy to allow users to add/remove resolvers without changing the static SignalRResolver list
return new List<IFormatterResolver>(SignalRResolver.Resolvers);
}
internal static MessagePackSerializerOptions CreateDefaultMessagePackSerializerOptions() =>
MessagePackSerializerOptions
.Standard
.WithResolver(SignalRResolver.Instance)
.WithSecurity(MessagePackSecurity.UntrustedData);

internal class SignalRResolver : IFormatterResolver
{
public static readonly IFormatterResolver Instance = new SignalRResolver();

public static readonly IList<IFormatterResolver> Resolvers = new IFormatterResolver[]
public static readonly IReadOnlyList<IFormatterResolver> Resolvers = new IFormatterResolver[]
{
DynamicEnumAsStringResolver.Instance,
ContractlessStandardResolver.Instance,
Expand Down
3 changes: 2 additions & 1 deletion src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using System.Threading.Tasks;
using MessagePack;
using MessagePack.Formatters;
using MessagePack.Resolvers;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Connections;
using Microsoft.AspNetCore.Http;
Expand Down Expand Up @@ -2370,7 +2371,7 @@ public async Task HubOptionsCanUseCustomMessagePackSettings()
services.AddSignalR()
.AddMessagePackProtocol(options =>
{
options.FormatterResolvers.Insert(0, new CustomFormatter());
options.SerializerOptions = MessagePackSerializerOptions.Standard.WithResolver(CompositeResolver.Create(new CustomFormatter(), options.SerializerOptions.Resolver));
});
}, LoggerFactory);

Expand Down