Closed gianluca-nitti closed 7 years ago
Thank you for the very in-depth review.
I fixed most of the issues and left some comments on the ones I'm not sure how to improve.
Summing up:
ClientMessage
(in websocket
package) which deserializes using a separate gson instance (as I said before, in my understanding, using the shared one would result in a loop), then performs the checks and throws a JsonParseException
with an InvalidClientMessageException
as cause if the message is not valid, andAuthenticationHandshakeHandlerImpl
before doing the operations that could throw an NPE and throw a custom exception instead, since writing a validating deserializer for ClientAuthenticationMessage
would be tricky due to the required custom type adapters as discussed before.What do you think about it?
Overview
Here is the code which implements user authentication via HTTP and WebSocket as described in the devlog updates for weeks 2-3.
Here is a little summary of the updates (more details in the posts at the link above, and in the recent messages on Slack#server-api):
How to test
There is a simple Javascript WebSocket client as a proof-of-concept that the RSA part can be handled by a browser (with the right libraries) here. For HTTP I tested manually using Postman.
Note that the BigIntegers in the certificates are serialized as a base64 string representing their binary representation; the BigIntegerBase64Serializer class implemented in MovingBlocks/Terasology#2965, thus this branch will not compile if the root repository (where this facade is located as a nested repository in facades/Server) is not at that branch or that PR is merged. The reasons behind this is that handling very large numbers in JSON is unhandy, for example in Javascript external libraries must be used, and that by using base64 the messages are shorter (a little bandwidth is saved, but probably this isn't very relevant). This also means that directly pasting the "normal" configuartion file in the simple client linked above won't work, it needs to be re-encoded with the BigIntegers in the correct format. The hack I used was to start the regular client in debug mode with a breakpoint on the code that saves the configuration, and then using the IntelliJ debugger's "evaluate expression" feature to add the correct gson type adapter and obtaining a JSON string of the configuration with the numbers in the right format. Since the Java's BigInteger binary representation is simply the two's complement representation of the number manually converting them with a tool like this (pasting a number in the DEC field and getting the base64) should also work but I have not tried it yet. Any suggestion is welcome, maybe another version of
BigIntegerBase64Serializer
in this repository should be made so that the server could handle both the representations?