Enigma server cleanup (#175)

* Convert packet classes to records where possible

* Remove Packet#read and no-args packet constructors

* Fix assumption for swing in enigma server

* Separate packets by c2s/s2c

* Don't send disconnect messages for logging-in clients

* Add a dev option to log client packets

* Update dev menu with the new option

* Reorder imports

* Add an explanation for the protocol version

* Handle logins after validating the packet

* Reject changes from unapproved clients

* Log actual port on server start

* Only notify disconnections of approved clients

* Fix some server logging

* Set up a network test

* Validate usernames

* Add a test for the username validation

* Update protocol.md

* More login tests

* Add a test plugin

* Ignore messages from unapproved clients

* Fix invalid packets crashing the packet handling thread

* Add a username field to the server creation dialog

* Update protocol file

* Don't send packets to unapproved clients

* Reset the mapping to the server state on invalid changes

* Improve invalid username message

* Change protocol version

enigma-server/build.gradle

@@ -7,6 +7,8 @@ plugins {
 dependencies {
 	shadow(implementation project(':enigma'))
 	implementation libs.jopt
+
+	testImplementation testFixtures(project(':enigma'))
 }
 
 mainClassName = 'org.quiltmc.enigma.network.DedicatedEnigmaServer'

enigma-server/docs/protocol.md

@@ -11,58 +11,61 @@ Strings, see below.
 
 ## Login protocol
 ```
-Client     Server
-|               |
-|     Login     |
-| >>>>>>>>>>>>> |
-|               |
-| SyncMappings  |
-| <<<<<<<<<<<<< |
-|               |
-| ConfirmChange |
-| >>>>>>>>>>>>> |
+Client        Server
+|                  |
+|     LoginC2S     |
+| >>>>>>>>>>>>>>>> |
+|                  |
+| SyncMappingsS2C  |
+| <<<<<<<<<<<<<<<< |
+|                  |
+| ConfirmChangeC2S |
+| >>>>>>>>>>>>>>>> |
 ```
 1. On connect, the client sends a login packet to the server. This allows the server to test the validity of the client,
    as well as allowing the client to declare metadata about itself, such as the username.
-1. After validating the login packet, the server sends all its mappings to the client, and the client will apply them.
-1. Upon receiving the mappings, the client sends a `ConfirmChange` packet with `sync_id` set to 0, to confirm that it
+2. After validating the login packet, the server sends all its mappings to the client, and the client will apply them.
+   - Just before the mappings are sent, the server sends the new user list to every connected client
+3. Upon receiving the mappings, the client sends a `ConfirmChangeC2S` packet with `sync_id` set to 0, to confirm that it
    has received the mappings and is in sync with the server. Once the server receives this packet, the client will be
    allowed to modify mappings.
 
-The server will not accept any other packets from the client until this entire exchange has been completed.
+The server will ignore any other packets from the client until this entire exchange has been completed, and may kick the
+client if any other packet is received during this stage.
 
 ## Kicking clients
 When the server kicks a client, it may optionally send a `Kick` packet immediately before closing the connection, which
 contains the reason why the client was kicked (so the client can display it to the user). This is not required though -
-the server may simply terminate the connection.
+the server may simply terminate the connection. After the connection is closed, the server should send the new user list
+to the other connected clients.
 
 ## Changing mappings
-This section uses the example of renaming, but the same pattern applies to all mapping changes.
 ```
-Client A   Server    Client B
-|           |               |
-| RenameC2S |               |
-| >>>>>>>>> |               |
-|           |               |
-|           |   RenameS2C   |
-|           | >>>>>>>>>>>>> |
-|           |               |
-|           | ConfirmChange |
-|           | <<<<<<<<<<<<< |
+Client A       Server        Client B
+|                |                  |
+| EntryChangeC2S |                  |
+| >>>>>>>>>>>>>> |                  |
+|                |                  |
+|                |  EntryChangeS2C  |
+|                | >>>>>>>>>>>>>>>> |
+|                |                  |
+|                | ConfirmChangeC2S |
+|                | <<<<<<<<<<<<<<<< |
 ```
 
 1. Client A validates the name and updates the mapping client-side to give the impression there is no latency >:)
-1. Client A sends a rename packet to the server, notifying it of the rename.
-1. The server assesses the validity of the rename. If it is invalid for whatever reason (e.g. the mapping was locked or
-   the name contains invalid characters), then the server sends an appropriate packet back to client A to revert the
-   change, with `sync_id` set to 0. The server will ignore any `ConfirmChange` packets it receives in response to this.
-1. If the rename was valid, the server will lock all clients except client A from being able to modify this mapping, and
-   then send an appropriate packet to all clients except client A notifying them of this rename. The `sync_id` will be a
+2. Client A sends an entry change packet to the server, notifying it of the change.
+3. The server assesses the validity of the change. If it is invalid for whatever reason (e.g. the mapping was locked or
+   the name contains invalid characters), then the server sends an appropriate packet back to client A to reset the
+   mapping back to the same state as the server, with `sync_id` set to 0. The server will ignore any `ConfirmChangeC2S`
+   packets it receives in response to this.
+4. If the change was valid, the server will lock all clients except client A from being able to modify this mapping, and
+   then send an appropriate packet to all clients except client A notifying them of this change. The `sync_id` will be a
    unique non-zero value identifying this change.
-1. Each client responds to this packet by updating their mappings locally to reflect this change, then sending a
-   `ConfirmChange` packet with the same `sync_id` as the one in the packet they received, to confirm that they have
+5. Each client responds to this packet by updating their mappings locally to reflect this change, then sending a
+   `ConfirmChangeC2S` packet with the same `sync_id` as the one in the packet they received, to confirm that they have
    received the change.
-1. When the server receives the `ConfirmChange` packet, and another change to that mapping hasn't occurred since, the
+6. When the server receives the `ConfirmChangeC2S` packet, and another change to that mapping hasn't occurred since, the
    server will unlock that mapping for that client and allow them to make changes again.
 
 ## Packets
@@ -130,7 +133,7 @@ struct Entry {
 - `index`: The index of the local variable in the local variable table.
 - `parameter`: Whether the local variable is a parameter.
 
-### The Message struct
+### The ServerMessage struct
 ```c
 enum MessageType {
     MESSAGE_CHAT = 0,
@@ -143,7 +146,7 @@ enum MessageType {
 };
 typedef unsigned byte message_type_t;
 
-struct Message {
+struct ServerMessage {
     message_type_t type;
     union { // Note that the size of this varies depending on type, it is not constant size
         struct {
@@ -198,10 +201,12 @@ typedef enum tristate_change {
     TRISTATE_CHANGE_SET = 2
 } tristate_change_t;
 
-// Contains 2 packed values:
+// Contains 4 packed values:
 // bitmask   type
-// 0011  tristate_change_t deobf_name_change;
-// 1100  tristate_change_t javadoc_change;
+// 00000011  tristate_change_t deobf_name_change;
+// 00001100  tristate_change_t javadoc_change;
+// 00110000  tristate_change_t token_type_change;
+// 11000000  tristate_change_t source_plugin_id_change;
 typedef uint8_t entry_change_flags;
 
 struct entry_change {
@@ -239,9 +244,9 @@ struct LoginC2SPacket {
 }
 ```
 - `protocol_version`: the version of the protocol. If the version does not match on the server, then the client will be
-                      kicked immediately. Currently always equal to 0.
-- `checksum`: the SHA-1 hash of the JAR file the client has open. If this does not match the SHA-1 hash of the JAR file
-              the server has open, the client will be kicked.
+                      kicked immediately.
+- `checksum`: the SHA-1 hash of the sorted class files in the JAR file the client has open. If this does not match the
+              SHA-1 hash of the JAR file the server has open, the client will be kicked.
 - `password`: the password needed to log into the server. Note that each `char` is 2 bytes, as per the Java data type.
               If this password is incorrect, the client will be kicked.
 - `username`: the username of the user logging in. If the username is not unique, the client will be kicked.
@@ -306,7 +311,7 @@ typedef { Entry but without the has_parent or parent fields } NoParentEntry;
 ### Message (server-to-client)
 ```c
 struct MessageS2CPacket {
-    Message message;
+    ServerMessage message;
 }
 ```
 

enigma-server/src/main/java/org/quiltmc/enigma/network/DedicatedEnigmaServer.java

@@ -1,19 +1,19 @@
 package org.quiltmc.enigma.network;
 
+import joptsimple.OptionParser;
+import joptsimple.OptionSet;
+import joptsimple.OptionSpec;
+import joptsimple.ValueConverter;
 import org.quiltmc.enigma.api.Enigma;
 import org.quiltmc.enigma.api.EnigmaProfile;
 import org.quiltmc.enigma.api.EnigmaProject;
 import org.quiltmc.enigma.api.ProgressListener;
 import org.quiltmc.enigma.api.class_provider.ClasspathClassProvider;
-import org.quiltmc.enigma.api.translation.mapping.serde.MappingParseException;
 import org.quiltmc.enigma.api.translation.mapping.EntryRemapper;
 import org.quiltmc.enigma.api.translation.mapping.serde.MappingFormat;
+import org.quiltmc.enigma.api.translation.mapping.serde.MappingParseException;
 import org.quiltmc.enigma.api.translation.mapping.tree.HashEntryTree;
 import org.quiltmc.enigma.util.Utils;
-import joptsimple.OptionParser;
-import joptsimple.OptionSet;
-import joptsimple.OptionSpec;
-import joptsimple.ValueConverter;
 import org.tinylog.Logger;
 
 import java.io.IOException;

enigma-server/src/main/java/org/quiltmc/enigma/network/EnigmaClient.java

@@ -4,7 +4,6 @@
 import org.quiltmc.enigma.network.packet.PacketRegistry;
 import org.tinylog.Logger;
 
-import javax.swing.SwingUtilities;
 import java.io.DataInput;
 import java.io.DataInputStream;
 import java.io.DataOutput;
@@ -14,16 +13,18 @@
 import java.net.Socket;
 import java.net.SocketException;
 
-public class EnigmaClient {
-	private final ClientPacketHandler controller;
+public abstract class EnigmaClient {
+	protected boolean logPackets = false;
+
+	private final ClientPacketHandler handler;
 
 	private final String ip;
 	private final int port;
 	private Socket socket;
 	private DataOutput output;
 
-	public EnigmaClient(ClientPacketHandler controller, String ip, int port) {
-		this.controller = controller;
+	public EnigmaClient(ClientPacketHandler handler, String ip, int port) {
+		this.handler = handler;
 		this.ip = ip;
 		this.port = port;
 	}
@@ -42,16 +43,25 @@ public void connect() throws IOException {
 						break;
 					}
 
-					Packet<ClientPacketHandler> packet = PacketRegistry.createS2CPacket(packetId);
+					Packet<ClientPacketHandler> packet = PacketRegistry.readS2CPacket(packetId, input);
 					if (packet == null) {
 						throw new IOException("Received invalid packet id " + packetId);
 					}
 
-					packet.read(input);
-					SwingUtilities.invokeLater(() -> packet.handle(this.controller));
+					if (this.logPackets) {
+						Logger.info("Received packet {} (id {})", packet, packetId);
+					}
+
+					this.runOnThread(() -> {
+						try {
+							packet.handle(this.handler);
+						} catch (Exception e) {
+							Logger.error(e, "Failed to handle packet!");
+						}
+					});
 				}
 			} catch (IOException e) {
-				this.controller.disconnectIfConnected(e.toString());
+				this.handler.disconnectIfConnected(e.toString());
 			}
 		});
 		thread.setName("Client I/O thread");
@@ -73,8 +83,14 @@ public void sendPacket(Packet<ServerPacketHandler> packet) {
 		try {
 			this.output.writeByte(PacketRegistry.getC2SId(packet));
 			packet.write(this.output);
+
+			if (this.logPackets) {
+				Logger.info("Sent packet {}", packet);
+			}
 		} catch (IOException e) {
-			this.controller.disconnectIfConnected(e.toString());
+			this.handler.disconnectIfConnected(e.toString());
 		}
 	}
+
+	protected abstract void runOnThread(Runnable task);
 }