breaking: Using Events instead of Message for connect and disconnect

[James-Frowen]

• Using events is simpler to understand than InvokeHandler
• Allows multiple objects to listen for connected events to add/remove message handlers to NetworkClient/Server
• Stops attackers from sending Connect/Disconnect Message

this change does cause client OnClientConnect in host mode to be called the same

BREAKING CHANGE: use events in NetworkServer/Client instead of handlers for Connect/DisconnectMessage

[James-Frowen]

3 things that keep doing

• fix tests
• find better names for OnConnectedEvent
• can we remove public bool InvokeHandler<T>(T msg, int channelId) where T : NetworkMessage

I dont like the name of the events OnConnectedEvent. but OnConnected is already a method inside NetworkServer. One option is to rename the methods as they are private/internal

        public static event ConnectionEvent OnConnected;

        public static event ConnectionEvent OnDisconnect;

the public bool InvokeHandler<T>(T msg, int channelId) where T : NetworkMessage method seems to only be used to invoke message locally. I think this is un-needed as you could just call a method instead of invoking it indirectly.

[MrGadget1024]

Since you're obsoleting the messages, I don't think this is a breaking change, unless I missed something else.

[James-Frowen]

Since you're obsoleting the messages, I don't think this is a breaking change, unless I missed something else.

they are not longer being invoked, so if someone manually replaced the handlers this would break

edit: changes to NetworkManager have been done in this PR, so users using the NetworkManager should have no changes to make. the breaking part would be for fully custom Managers

[miwarnec]

as far as I can tell, UNET team did it this way so that there is only one way for all events (message + handler).
with this change there are now two ways.
let me think about it a bit. it's not necessarily good or bad either way.

[miwarnec]

I think this is one of the few cases where even though your PR is more simple, we should keep the previous solution with the big picture in mind.
this way there is only one way to react to messages.
it will also allow us to fully decouple the authenticator form core mirror later, where we can set authenticated = true in the default connectmessage handler, and authenticator can overwrite the handler and do it's own authentication method.
I do that in DOTSNET too and I think it's worth it.

[James-Frowen]

It isnt added a 2nd way to react to message, it is removing the ability to invoke message locally. The 2 main benefits are being more obvious what is calling what (hard to know what InvokeHandler is really invoking), and allows multiple things to listen for the events.

reopening as discussed on discord

[miwarnec]

@James-Frowen please rebase, fix sonar errors and switch from draft to live :)

[James-Frowen]

I'm in the process of fixing the tests, it seems some of them aren't clean up after themselves, I will make it live PR once I sort it out

[James-Frowen]

This change also stops attackers from sending Connect/Disconnect Message

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

93.3% Coverage
0.0% Duplication

[miwarnec]

This change also stops attackers from sending Connect/Disconnect Message

Please elaborate :)

[James-Frowen]

This change also stops attackers from sending Connect/Disconnect Message

Please elaborate :)

An attacker can send the ConnectMessage and Mirror will treat it the same as if it came from conn.InvokeHandler(new ConnectMessage(), -1);

I'm not sure exactly how NetworkManager would handle receiving multiple of these from the same client but It will likely call the OnServerConnect virtual method that user's override.

[miwarnec]

reading through changes again:

• the PR does too much. like last time, please don't rename the internal functions from OnConnected to OnConnectedTransport etc.
• wasn't the point to remove the internal invokehandler function? seems like it's still there?

[James-Frowen]

• the PR does too much. like last time, please don't rename the internal functions from OnConnected to OnConnectedTransport etc.

They have to be renamed so that the events can be called OnConnected, what name do you think would be better?

I'm not sure what less this this PR can do? the main point is now to remove ConnectMessage and DisconnectMessage

• wasn't the point to remove the internal invokehandler function? seems like it's still there?

InvokeHandler has been marked with Obsolete, we don't need to fully remove the function now. The InvokeHandler itself isnt a security risk, it is that message handlers get registered so that an attacker could invoke them (when they should only be called locally on the server).

Its been about a month since I started hat and since then we have found that ConnectMessage and DisconnectMessage can be used by attacker to break the server. so the original idea of the PR is slightly different.

[miwarnec]

closing in favor of 52747a4