inspircd / inspircd-contrib

Third-party InspIRCd module repository.
https://www.inspircd.org
66 stars 72 forks source link

Fix a bunch of issues with roleplay. #247

Closed Elizafox closed 3 years ago

Elizafox commented 3 years ago

EDIT: mention the new fixes for this.

Okay so @SadieCat gave me a good way to fix this. I took some pointers (and code) from the `m_ircv3_msgid`` module as well as her suggestions.

There are issues with the roleplay module as it stands with remote propagation: messages get duplicated, and history isn't played back correctly to clients; all roleplay commands look as if they came from the person who executed them. The reason for this is m_spanningtree hooks OnUserPostMesage to propagate messages and m_chanhistory uses this to store history. There are probably countless other things I missed, but those were the big two.

My first shake at this was to make a fake client class, but this could potentially cause issues as inspircd was not really designed to do this and it was a bit of a hack.

The latest commit in this branch rectifies the issues I mentioned above in another way, by using a special inspircd.org/roleplay-src tag that lets us rewrite things as we need to. This tag will never be seen by clients, it is simply for internal use. When we send out a user message, we now simply call msg.SetSource() on it to make it appear it came from us.

Previously, for posterity: ~~This is in a simlar vein to FakeUser, but inherits directly from User since this isn't a server (although it pretends to be one, it's ultimately just... us). This is good enough to fool most modules like the history module and spanningtree module.

This fixes a bug where spanningtree would erroneously propagate roleplay messages as if they came from the person who used the command. This also happens to fix an issue where the chanhistory module would not save the logs correctly from roleplay.~~

SadieCat commented 3 years ago

This could break places that assume that a fake user can be cast to FakeUser* like SplitCommand::Handle or IS_SERVER.

Elizafox commented 3 years ago

This could break places that assume that a fake user can be cast to FakeUser* like SplitCommand::Handle or IS_SERVER.

As mentioned on IRC, here for posterity:

Ideas:

I think I'm going to go with the last approach as it's simplest and it's pretty unlikely to break anything. In theory someone checking IS_LOCAL and IS_REMOTE could assume then IS_SERVER will hold true or some permutation thereof, but I doubt this will crop up in practise (there are no instances in the InspIRCd codebase I could find that would be affected by this) and code should be doing explicit checks anyway.

SadieCat commented 3 years ago

It's not ideal but for now I feel like a better option would be to send S2S as the real sender but mark it with an internal-only (i.e. one that always returns false for ShouldSendTag) tag like inspircd.org/roleplay-source=foo!bar@baz and then hook OnUserWrite and if that tag is set then unset it and call SetSource to set the source.

Elizafox commented 3 years ago

It's not ideal but for now I feel like a better option would be to send S2S as the real sender but mark it with an internal-only (i.e. one that always returns false for ShouldSendTag) tag like inspircd.org/roleplay-source=foo!bar@baz and then hook OnUserWrite and if that tag is set then unset it and call SetSource to set the source.

This would work I think.

Elizafox commented 3 years ago

I had to do some extra crap with copying tags to make this work properly. I looked at the m_ircv3_msgid module and stole the logic from there.