The Mod Protocol API, aka better validation of mod presence of both-side mods with extra protocol version syncing #4011

Patbox · 2024-08-07T19:08:12Z

This api implements Mod Protocols, a simple way for a mods to quickly check if they are present on client/server, while also allowing for prevention of joining if it's missing on either side (through not being forced to). It can be also used for syncing compatible protocol versions for general networking api usage or feature toggling, without making the server/client incompatible with vanilla clients/servers.

Example screenshots of mismatch screen:

Vanilla client joining modded server:

Modded client with some mismatched mods joining modded server:

Modded client (with server required entries) joining vanilla server:

Vanillish clients get disconnected by the server, while rest is validated client side.

(Also see https://github.com/Patbox/fabric/blob/mod-protocol/fabric-mod-protocol-api-v1/src/main/java/net/fabricmc/fabric/api/modprotocol/v1/package-info.java for info)

Worth noting for people who might get confused, this is not an "anticheat"! This api allows any mod to register any protocol names and even then cheat clients could easily work around it. This api is purely to improve experience of regular users / limit amount of reports for mod devs or pack makers.

One thing to do with this pr would be bit more testing + maybe writing some unit tests, through from what I've checked everything should work. Otherwise I think it should be all done.

...d-protocol-api-v1/src/main/java/net/fabricmc/fabric/impl/modprotocol/ModProtocolLocator.java

…ads, use id to get the protocol from the map

...1/src/client/java/net/fabricmc/fabric/api/client/modprotocol/v1/ClientModProtocolLookup.java

fabric-mod-protocol-api-v1/src/testmod/resources/fabric.mod.json

fabric-mod-protocol-api-v1/src/main/resources/fabric.mod.json

apple502j · 2024-08-08T17:02:39Z

fabric-mod-protocol-api-v1/src/main/resources/fabric.mod.json

@@ -0,0 +1,42 @@
+{


Indent appears to be messed up.

apple502j · 2024-08-08T17:04:16Z

fabric-mod-protocol-api-v1/src/main/resources/assets/fabric-mod-protocol-api-v1/lang/en_us.json

@@ -0,0 +1,8 @@
+{
+  "text.fabric.mod_protocol.mismatched.title": "Required protocols are mismatched!",


I think this title is unclear, but I haven't come up with a better one 🤷🏼

Tbf it's best one I could think of too

apple502j · 2024-08-08T17:06:12Z

...rc/main/java/net/fabricmc/fabric/impl/modprotocol/payload/ModProtocolResponseC2SPayload.java

+			PacketCodecs.map(ModProtocolResponseC2SPayload::createMap, Identifier.PACKET_CODEC, PacketCodecs.INTEGER)
+			.xmap(ModProtocolResponseC2SPayload::new, ModProtocolResponseC2SPayload::supported).cast();
+
+	private static Object2IntMap<Identifier> createMap(int i) {


Maybe there should be a cap on the number of protocols clients can send.

Would require writing a custom lazy codec since the class is initialized on init, but should be doable. Client should never sent more than amount server did.

Can be a hardcoded cap, just so that bad clients won't request a map of size 1M.

There is no hardcap on api side, so ideally it would be dynamic

Co-authored-by: apple502j <[email protected]>

…scription

apple502j · 2024-08-09T13:52:22Z

...pi-v1/src/client/java/net/fabricmc/fabric/impl/modprotocol/client/ClientModProtocolInit.java

-public final class ClientModProtocolInit {
-	public static void clientInit() {
+public final class ClientModProtocolInit implements ClientModInitializer {
+	public void onInitializeClient() {
 		ClientConfigurationNetworking.registerGlobalReceiver(ModProtocolRequestS2CPayload.ID, (payload, context) -> {


Please use @Override.

christopherplaysminecraft · 2024-08-09T21:12:56Z

Just wondering, wouldn't this allow any server to continuously annoy users by requesting them to install a mod?

Also wondering about the wording in the original message

through not being forced to

Does that mean that one can simply tell it (click a button) to ignore any missing mods?

modmuss50 · 2024-08-09T21:30:26Z

It has always been possible (and quite easy) for a server mod to write code that disconnects the player if the corresponding client side mod isn't present. This PR also won't allow server owners to request any client mod to be installed.

christopherplaysminecraft · 2024-08-09T21:35:50Z

Okay
Tbh I understood this whole PR to basically just do that... whoops.

modmuss50

This looks like a great start, just got a few points/questions regarding the API and networking changes before looking at the impl in detail.

I think the protocols should be attached to the ModContainer/modid in a stronger fashion, I dont think a general ID has much benefit.

I am still a little bit skeptical of using int ids as apposed to a semver version, I worry people wont remeber to change their protocol id. My inital thoughts about this problem were that we would re-use the same versioning system as used by loading when determining mod compatibility. I would expect most mod developers will just do a strict match of the specific versions, so its a setup and forget deal. I think we should discuss this and agree on the spec before spending ages perfecting the impl. See #463 for some prior thoughts on this topic.

I like having an API to query the client/server branding, I think this would be better off as a seperate PR so its not lost within this one, and could likely go in quite quickly. I would strongly suggest splitting that into another PR.

...tocol-api-v1/src/client/java/net/fabricmc/fabric/api/client/modprotocol/v1/package-info.java

modmuss50 · 2024-08-09T21:47:21Z

...c-mod-protocol-api-v1/src/main/java/net/fabricmc/fabric/api/modprotocol/v1/package-info.java

+ *     		<dt>"fabric:mod_protocol": {
+ *     		 "protocol": [1, 2],
+ *     		 "id: "custom:id",
+ *     		 "name": "Mod Name",
+ *     		 "version": "v1.2.3",
+ *     		 "require_client": false,
+ *     		 "require_server": true
+ *     		}</dt>


This should be versioned, e.g fabric:mod_protocol_v1

Yeah fair, through I imagine ideally it would never need incompatible format change, but can do it.

modmuss50 · 2024-08-09T21:47:49Z

...c-mod-protocol-api-v1/src/main/java/net/fabricmc/fabric/api/modprotocol/v1/package-info.java

+ *      	<p>"id" is the protocols identifier, which can have any namespace and path, as long as it's valid.
+ *      	It is optional and defaults to an ID with "mod" namespace and path equal to mod's id.
+ *      	</p>
+ *     		<p>"name" is a name displayed if protocol doesn't match. It's optional and by default it uses one from mod's metadata.</p>


Im not sure this is that useful tbh, why would a mod have 2 names?

The protocol array support (and naming/etc) is mostly done to mirror "provides", allowing people to check against old ids when they change/include other mods protocol easily.

Being able to change other mods protocols seems like a massive recipe for confusion. Is this not what the dependency system already handles?

This is not really for changing other mods protocol. It's mostly to mirror the "provides" functionality (your own mod changing it's id, but still being network compatible). Through to be fair it could be moved to allow this in code only

modmuss50 · 2024-08-09T21:48:01Z

...c-mod-protocol-api-v1/src/main/java/net/fabricmc/fabric/api/modprotocol/v1/package-info.java

+ *      	It is optional and defaults to an ID with "mod" namespace and path equal to mod's id.
+ *      	</p>
+ *     		<p>"name" is a name displayed if protocol doesn't match. It's optional and by default it uses one from mod's metadata.</p>
+ *          <p>"version" is a version displayed if protocol doesn't match. It's optional and by default it uses one from mod's metadata.</p>


Again, why duplicate data?

Same as above

modmuss50 · 2024-08-09T21:49:09Z

...c-mod-protocol-api-v1/src/main/java/net/fabricmc/fabric/api/modprotocol/v1/package-info.java

+ *     		<p>"name" is a name displayed if protocol doesn't match. It's optional and by default it uses one from mod's metadata.</p>
+ *          <p>"version" is a version displayed if protocol doesn't match. It's optional and by default it uses one from mod's metadata.</p>
+ *          <p>"require_client" controls if clients without this protocol can join the server, defaults to true, preventing joining</p>
+ *          <p>"require_server" controls if clients can join servers without this mod, defaults to true, preventing joining</p>


Is true the correct default here? I wonder if most mods would be happy to join a server without their mod 🤔 Maybe there should be no default for these values?

Wasn't sure myself, but though it makes sense to default to something since the just number inline does. But can be changed

modmuss50 · 2024-08-09T22:01:43Z

...col-api-v1/src/main/java/net/fabricmc/fabric/api/modprotocol/v1/ServerModProtocolLookup.java

+	 * @param protocolId protocol's id
+	 * @return Protocol version supported by the player
+	 */
+	public static int getSupportedProtocol(ServerPlayerEntity player, Identifier protocolId) {


Whats the usecase for getting this later on? I do agree this doesnt seem that helpful.

Im also not too keen of APIs that return -1, I think it should throw (name the method to suggest it throws) and also provide one that returns an OptionalInt

modmuss50 · 2024-08-09T22:05:15Z

...i-v1/src/client/java/net/fabricmc/fabric/impl/networking/client/ClientLoginNetworkAddon.java

@@ -103,6 +103,11 @@ protected void handleRegistration(Identifier channelName) {
 	protected void handleUnregistration(Identifier channelName) {
 	}

+	@Override
+	public @Nullable String getBrand() {


Ideally this wouldnt exist on the login addon, could the root method go in ClientCommonNetworkAddon? Im not too worried about this as its impl, and can easily be refactored later. Or even sooner before this PR goes in as its own thing.

Originally I wanted to use it purely for changing the message (through ended up using the same one for everyone), so yeah I can drop this for now

modmuss50 · 2024-08-09T22:06:00Z

...pi-v1/src/main/java/net/fabricmc/fabric/api/networking/v1/ServerConfigurationNetworking.java

+	/**
+	 * Gets the brand of the client the player connected with.
+	 *
+	 * @param handler the network handler, representing the connection to the player/client
+	 * @return client's brand, or {@code null} if not sent
+	 */
+	@Nullable
+	public static String getBrand(ServerConfigurationNetworkHandler handler) {


What are your toughts on moving this out into its own PR? Will make things a bit easier to review, this change can likely go in quite quickly.

It would also be nice to have a way to get the servers brand.

modmuss50 · 2024-08-09T22:08:45Z

...-api-v1/src/main/java/net/fabricmc/fabric/impl/networking/AbstractChanneledNetworkAddon.java

@@ -87,6 +90,10 @@ public boolean handle(CustomPayload payload) {
 			}
 		}

+		if (payload instanceof BrandCustomPayload brand) {
+			this.brand = brand.brand();


I'd return false after this point. False becuase we only need to "peek" at this packet and not consume it, allowing vanilla or any exisitng mod to also read it.

modmuss50 · 2024-08-09T22:09:39Z

...api-v1/src/main/java/net/fabricmc/fabric/mixin/networking/ServerPlayNetworkHandlerMixin.java

+		if (connection.getPacketListener() instanceof NetworkHandlerExtensions extension) {
+			this.addon.setBrand(extension.getAddon().getBrand());
+		}


A comment explaing this may help.

modmuss50 · 2024-08-09T22:57:45Z

In its current form I dont believe this supports the use case where you have an optional mod but want to have a requiment when it does exist.

E.g you have a mod that is optional on the server, but the protocol must match if it does exist on the client. Care should be taken to allow a vanilla client to join in this example.

Patbox · 2024-08-10T05:46:09Z

This looks like a great start, just got a few points/questions regarding the API and networking changes before looking at the impl in detail.

I think the protocols should be attached to the ModContainer/modid in a stronger fashion, I dont think a general ID has much benefit.

I am still a little bit skeptical of using int ids as apposed to a semver version, I worry people wont remeber to change their protocol id. My inital thoughts about this problem were that we would re-use the same versioning system as used by loading when determining mod compatibility. I would expect most mod developers will just do a strict match of the specific versions, so its a setup and forget deal. I think we should discuss this and agree on the spec before spending ages perfecting the impl. See #463 for some prior thoughts on this topic.

I like having an API to query the client/server branding, I think this would be better off as a seperate PR so its not lost within this one, and could likely go in quite quickly. I would strongly suggest splitting that into another PR.

Protocols are dettached from containers due to in api usage, which can be useful for other tools. This is why it's namespaced.

Using strings would be bit more annoying when trying to use the getSupportedProtocol api's for any checks, since you might need to do a bit more matching. Aside of not all mods using valid semver in the first place. Ints are simplest way to check against version compat (which most software, including Minecraft does for protocol). My idea for improving visibility, is to put it alongside other values in gradle.properties, which makes it as simple of a change as version one. Having dependency style matching won't work for more complex cases or will make people more likely to make it way stricter than it needs to be, which could end up being annoying for modpack server makers which might want to get a hotfix for their server quickly without needing to wait for full modpack update.

modmuss50 · 2024-08-12T09:36:12Z

Protocols are dettached from containers due to in api usage, which can be useful for other tools. This is why it's namespaced.

Can you expand on this? I dont see what issues attaching them to containers/modids would cause.

Using strings would be bit more annoying when trying to use the getSupportedProtocol api's for any checks, since you might need to do a bit more matching.

I am not a massive fan of getSupportedProtocol I dont think this whole PR should be used for versioning packets, the main goal is to check that the client is compatible. Versioning packets is better done by using canSend imo.

Aside of not all mods using valid semver in the first place.

We could easily have a fallback to support basic comparsions, such as the version being identical. Mods are highly encouraged to use semver, you already miss out on a lot of stuff and get a warning from loader if you dont.

Ints are simplest way to check against version compat

The code for comparing versions already exists in loader, we need to make this easy for most people to use in their mods. I really dont see people updating an int every time they break protocol, it gets even more complex when you have 2 or more branches for differing minecraft versions. These problems have all been solved with semver.

it alongside other values in gradle.properties

I dont think we can do this, processResources is not suited to this. One solution may be to move everything to the fmj but this is another discussion.

Having dependency style matching won't work for more complex cases

Such as? There are complex cases that this PR in its current state doesnt handle.

will make people more likely to make it way stricter than it needs to be

I agree with this, I would expect the default to allow patch version changes. We need to find a good middle ground as like I said I dont see people every changing this int value, I know I will certainly forget.

apple502j · 2024-08-12T10:06:33Z

Can you expand on this? I dont see what issues attaching them to containers/modids would cause.

String ID would allow mods to define multiple protocols, though I am unsure why you need them.

re: canSend - I agree this module should only be used for the automatic disconnection, fail-soft protocol checks should continue to use canSend.

Patbox · 2024-08-12T11:40:58Z

Can you expand on this? I dont see what issues attaching them to containers/modids would cause.
A single mod can for example change vanilla protocol depending on it's configuration, which then could just use mod protocol to inform about it / enforce it without requiring it's own syncing (or ending up with bad errors). Other thing this could be used for is a mod to register configurable protocol (for example for the modpack), to inform player that they run on outdated version. In both cases you want custom name and version.

I am not a massive fan of getSupportedProtocol I dont think this whole PR should be used for versioning packets, the main goal is to check that the client is compatible. Versioning packets is better done by using canSend imo.

Can send is a just a boolean, which doesn't indicate packet version, just whatever that identifier is handled (which by itself is bit different concept, through it would require some extensions to be more useful with fabric's net api). It's pretty much similar to how network api has c:version packet, just more generic. I have similar system in polymer and I can say it's pretty useful.

The code for comparing versions already exists in loader, we need to make this easy for most people to use in their mods. I really dont see people updating an int every time they break protocol, it gets even more complex when you have 2 or more branches for differing minecraft versions. These problems have all been solved with semver.

"Semver" doesn't really tell much about network compatibility, as Mojang themselves break it in minor versions, but also keeps it the same if they can. I can imagine modders doing things similarly to that. To be fair I myself don't use semver correctly, just have mods that use semver-like in format.

Patbox added 4 commits August 6, 2024 11:40

Start working on Mod Protocol API

605d858

Implement error messages, move ids to Identifier

5d1f970

Implement apis and document them

dbcdcaa

Prevent joining to vanilla clients if server mods are required.

685051a

modmuss50 added the enhancement New feature or request label Aug 7, 2024

modmuss50 requested review from modmuss50 and a team August 7, 2024 19:09

maityyy reviewed Aug 7, 2024

View reviewed changes

...d-protocol-api-v1/src/main/java/net/fabricmc/fabric/impl/modprotocol/ModProtocolLocator.java Show resolved Hide resolved

Patbox added 4 commits August 7, 2024 21:19

Fix style check

d16dbc4

Actually fix the checkstyle

781a8da

Check-styles aren't my passion

822bdb2

Prevent mutating static array that can be accessed from multiple thre…

73affdf

…ads, use id to get the protocol from the map

apple502j suggested changes Aug 8, 2024

View reviewed changes

Patbox and others added 4 commits August 8, 2024 19:21

Apply suggestions from code review

a770fb7

Co-authored-by: apple502j <[email protected]>

Merge branch '1.21.1' into mod-protocol

175d392

Java docs on the UNSUPPORTED, use (Client)ModInitializer, also set de…

a08bffa

…scription

Set version of the module to 1.0.0

ae341a5

apple502j reviewed Aug 9, 2024

View reviewed changes

modmuss50 reviewed Aug 9, 2024

View reviewed changes

		@@ -0,0 +1,8 @@
		{
		"text.fabric.mod_protocol.mismatched.title": "Required protocols are mismatched!",

The Mod Protocol API, aka better validation of mod presence of both-side mods with extra protocol version syncing #4011

Are you sure you want to change the base?

The Mod Protocol API, aka better validation of mod presence of both-side mods with extra protocol version syncing #4011

Conversation

Patbox commented Aug 7, 2024

Vanilla client joining modded server:

Modded client with some mismatched mods joining modded server:

Modded client (with server required entries) joining vanilla server:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apple502j Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherplaysminecraft commented Aug 9, 2024

modmuss50 commented Aug 9, 2024

christopherplaysminecraft commented Aug 9, 2024

modmuss50 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modmuss50 commented Aug 9, 2024 • edited Loading

Patbox commented Aug 10, 2024

modmuss50 commented Aug 12, 2024

apple502j commented Aug 12, 2024

Patbox commented Aug 12, 2024

apple502j Aug 9, 2024 •

edited

Loading

modmuss50 commented Aug 9, 2024 •

edited

Loading