search

PretendoNetwork / nex-protocols-common-go

Reusable implementations of NEX methods found in many servers
GNU Affero General Public License v3.0
19 stars 8 forks source link

feat(matchmake-extension): Lie about permission checks for CloseParticipation #32

Closed ashquarky closed 5 months ago

ashquarky commented 6 months ago

PUYOPUYOTETRIS has all users send this packet, which is silly. If we send an error as we should, it aborts the match. For users where it won't do anything, still claim success instead of sending an error.

I also considered introducing a configurable toggle on CommonProtocol to enable and disable this behaviour game-by-game, but couldn't find a clean way to do that.

The only weird behaviour I can think of is that OnAfterCloseParticipation still gets called even in a lying situation, so callers might have to account for participation not actually being closed.

jonbarrow commented 5 months ago

I'll have to dig around in dumps to prove this, but I remember seeing behaviour like this in other games during my testing before the shutdown. I believe this was a changed behaviour in a later NEX revision, where this only returned this error later on in NEX's life. IIRC older revisions expect SessionVoid as the error here? Try sending that, and see if the issues still happen.

Given that PUYOPUYOTETRIS is known to behave this way I think using it's NEX version as the check would be fine.

EDIT: It seems like PUYOPUYOTETRIS may actually be doing something custom here? I checked a few existing dumps (for the 3DS version) and it does seem to just send back a success no mater what (though the SessionVoid change does exist in other games, somewhere around 3.6/3.7 IIRC?). If PUYOPUYOTETRIS is doing something custom here then I wonder if it's better to just handle this on it's server specifically, and not in common? Just overwrite the common handler for this method in the PUYOPUYOTETRIS server to always return a success

I'm unsure about this but I can't think on a better solution anyway. @jonbarrow thoughts?

This is fine. It's just meant to be called after the function runs, and assuming I'm right about this being a NEX version difference then it would also be accurate

ashquarky commented 5 months ago

IIRC older revisions expect SessionVoid as the error here? Try sending that, and see if the issues still happen.

This kills the session and, on hardware, shows 106-0318 (SessionVoid, lmao). So I think this is custom for Puyo.

jonbarrow commented 5 months ago

IIRC older revisions expect SessionVoid as the error here? Try sending that, and see if the issues still happen.

This kills the session and, on hardware, shows 106-0318 (SessionVoid, lmao). So I think this is custom for Puyo.

I think it might be better to just overwrite the common handler then with your own in the Puyo server. Give that a go and let us know if there are any pain points? This is the first time we've had to deal with something like this so the common lib may not be super optimized for it. @DaniElectra this might be something we should consider in the future

ashquarky commented 5 months ago

The only pain point was that when I copied the signature: 

func (commonProtocol *CommonProtocol) closeParticipation(err error, packet nex.PacketInterface, callID uint32, gid *types.PrimitiveU32) (*nex.RMCMessage, *nex.Error) {

I had to remove (commonProtocol *CommonProtocol) since that's not exported. It was only used for the OnAfterCloseParticipation callback which I could obviously implement myself if I needed a callback so Whatever. Otherwise it worked just fine and I was able to override the behaviour.

Closing this PR as no longer needed (for Puyo at least)

ashquarky commented 5 months ago

Here's what that ended up looking like: https://github.com/ashquarky/puyo-wiiu/commit/69191f027ee351aa39793a673b53ad5beb430134