-
-
Notifications
You must be signed in to change notification settings - Fork 725
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
Rewrite to signalr #670
base: master
Are you sure you want to change the base?
Rewrite to signalr #670
Conversation
Known Problems/Issues: - We wait 5s for each response from Node -> C#, this is a problem if we wait for a Window result (like a MessageBox) - After 5s everything from SignalrSerializeHelper will return null. - IpcMain is not working (committed out multiple lines) - Unsure if this is needed because we're able to use signalr/razor/blazor for this - Hybrid Support is forced to be true - Electron HostHook dint works (also unsure if needed) - Demos are not converted yet Expect a lot of bugs ;-)
…rong in socketio version) -> Should fix most areas with system.text.json
We have already received a PR #595 with a complete switch to another .NET library with Socket.IO support. Socket.IO runs natively in the backend and is better supported under Node.js than SignalR. We are happy to be open to pros and cons when deciding on SignalR. Currently there would be no reason. |
This PR is just to have a discussion :-) From my point of view it depends on the use case what is a better fit. In my current project I use Electron.NET as completely optional component/plugin and in that scenario it makes sense to use dotnet as "master", I also use Electron.NET as GUI of a remote application that runs on a webserver for a different project - Both cases are not possible with the current implementation because Electron.NET and the main assembly needs to be on the same system and electron need to be the entry application what is not the case with the signalr implementation. I also dislike the fact that we need to run 2 independent "server" implementations that use a separate port (dotnet backend, nodejs socket io) and there is no way to have a useful authorization - This problem dont exists with signalr/kaestrel as backend solution and is a show stopper in any real world scenario where security is a factor. Another thing that will make lazy people happy is that you dont need to attach a debugger - you can just debug from VS as you do in any other dotnet application :-) |
The two processes communicating via web sockets is ugly. I'm with you there. With SignalR we would have the same communication as well. I haven't noticed any difference in your implementation. As far as client application security is concerned, any desktop solution is easily vulnerable. Processes hosted in VMs can also be easily dismantled with a kernel debugger. But yes, an alternative for Electron.NET would be great. We just haven't found a simple solution to this. |
Thats true - In terms of communication it's pretty similar, at the end of the day signalr is also a socket connection. We need to run kaestrel (dotnet) in any scenario (with or without socketio) so the dotnet backend process will always be needed and with signalr it's just a additional endpoint on the same already existing backend and don't request a additional backend (socketio) that's hostet in a different process (electron) In my PR Electron.NET (UI/Electron itself) is optional and not a base requirement that bootstraps the dotnet entry assembly, it's exactly the way around - dotnet will start the electron process if needed and setup/provide the communication with signalr. It shifts the complete server logic to one process (dotnet). In terms of security concerns: It's way easier to exploit a application where I just need to send a packet to a socket without any kind of authorize than access the system memory. ATM every application that can communicate with localhost is able to send packets and use Electron.NET as puppet. Other exploits are for sure also possible but e.g. a memory injection would need at least elevated user rights and/or another level of knowledge. With one backend the coder is able to use any level of protection/authorization aspnetcore offers (and that's a lot^^) and don't need to take care about two backend services. Here a little sumup what the signalr implementation offers:
|
This looks like interesting solution to me ! But if it is very much different from current Socket implementation I understand it could take a lot of time to fully test this approach and make it production ready. And would it be possible to have like multiple different host models ? Users could choose if they wish to use the SocketHost library or the SignalRHost that could be the new one ? I'm using currently Electron with Blazor Server. Would it use the same SignalR in this scenario or it would have two layers of SignalR within SignalR ? :) And what about the third model that could work like BlazorWebView that Microsoft is shipping for MAUI and WPF ? I'm really enjoying so far the use of electron.net and hoping for the update soon ! |
Hi, the solution works for what I need to get done pretty good - I use it already in production: In my productive version the code is a little bit modified from the code in repo because I need to have authorization and multiple security checks. It should have implemented almost everything from the socketio solution but will break old code (if you have e.g. custom hooks/endpoints in electron JS). The different hosting models are hard to implement - In the way it's currently written up we need to maintain both JS implementations (signalr works different from the socketio version and all JS files are changed to reflect this) For Blazor: Thats exactly what I currently use in my productive solution too - Depending on what hosting model you use (Server/Clientside) it is easy to implement and you can directly bind blazor frontend to electron methods with the classes. I currently use a server side blazor hosting with electron.net as client side frontend. For BlazorWebView: IMHO this is currently out of scope - The way how this is implemented is based on MS edge, not electron. To have a solution like this I suggest to use chromly or CEFSharp :-) |
Hi Guys,
I did a rewrite from SocketIO to signalr because there are a lot of drawbacks if there are 2 "server" run concurrent (SocketIO from nodejs and Kaestrel from dotnet). Besides of that I dislike the fact that one program needs 2 ports ;-)
I use Electron.NET different, so in my scenario I start electron though dotnet and not dotnet though electron (electronize). This fulfill my needs better because the UI is not needed in all situations (I want to e.g. access Kaestrel from a remote computer and want the UI only if I work locally on the same system), or want to show a traymenu while the main application is running (as some kind of crossplatform UI). I also work 99,9% in c# code, and dont want to attach a debugger all the time. "electronize" method should still work in this version.
I also want to worry about only one side in terms of security, IMHO dotnet with Kaestrel contains a lot of useful stuff for authorize and we can make sure that only "verified" commands run though.
Please DONT merge this as it is - It's it basically working but have a lot of drawbacks and untested areas (besides of the fact that it completely breaks the SocketIO version), so please let me know if you consider to use this as idea/base and take a review. If there are issues (they are^^) just write me.
Known Problems/Issues:
We wait 5s for each response from Node -> C# (see SignalrSerializeHelper.cs) , this is a problem if we wait for a Window result (like a MessageBox) - After 5s everything from SignalrSerializeHelper will return null, should be easy to implement.
Hybrid Support is forced to be true
Electron HostHook did not work (also unsure if needed)
SignalrSerializeHelper is a bit messy and could get replaced in net7 with [Epic]: Support returning values from client invocations dotnet/aspnetcore#5280
- IpcMain is not working (committed out multiple lines) - Unsure if this is needed because we're able to use signalr/razor/blazor for this. Fixed
- Demos are not converted yet Fixed
Expect a lot of bugs ;-)
Thank you