MODE between WHO and RPL_WHOREPLY is lost

half-duplex commented 1 year ago

Description

I recently encountered an issue where a channel op (+o) was not being treated as an op by the bot. Checking the raw log, I found this when they last joined the channel:

<<      ':User!user@user.host JOIN :#channel\r\n'
>>      'WHO User\r\n'
<<      ':ChanServ!ChanServ@services MODE #channel +o User\r\n'
<<      ':server 352 Sopel #channel user user.host foo.server User Hr :0 John Smith\r\n'
<<      ':server 315 Sopel User :End of /WHO list.\r\n'

I'm not really sure how to handle this correctly, or if it's even Sopel's responsibility.

Reproduction steps

Run this test:

def test_bot_who_mode_order(mockbot, ircfactory):
    irc = ircfactory(mockbot)
    irc.bot._isupport = isupport.ISupport(chanmodes=("o", "", "", "", tuple()))
    irc.bot.modeparser.chanmodes = irc.bot.isupport.CHANMODES
    irc.channel_joined("#test")
    mockbot.on_message(":Alex!alex@alex.local JOIN :#test")
    assert mockbot.backend.message_sent == rawlist("WHO Alex")
    mockbot.on_message(":ChanServ!ChanServ@services.local MODE #test +o Alex")
    mockbot.on_message(":server.local 352 Sopel #test alex alex.local specific.server.local Alex Hr :0 Alex")
    mockbot.on_message(":server.local 315 Sopel Alex :End of /WHO list.")
    assert mockbot.channels["#test"].privileges[Identifier("Alex")] == OP

test/test_coretasks.py:129: in test_bot_who_mode_order
    assert mockbot.channels["#test"].privileges[Identifier("Alex")] == OP
E   assert 0 == <AccessLevel.OP: 4>

Expected behavior

bot.channels["#channel"].privileges["User"] = OP

Sopel version

Roughly 51300a1ab854d6ec82d90df1bc876188c03335ff

Installation method

pip install

IRCd

Unreal3.2.10.6

half-duplex commented 1 year ago

I wonder if modern Unreal has the same interleaving behavior, considering 3.2 was released in 2004 and EOL in 2016... https://www.unrealircd.org/docs/UnrealIRCd_releases

dgw commented 1 year ago

Hmm, this smells race-y, but on the server side. I don't think a client should have to be in the business of deciding which messages from the server are trustworthy.

It's unfortunate in a way that Unreal 3.2.10 added support for away-notify, as Sopel's periodic polling of WHO information would certainly fix the permissions-state desync no more than max(120, len(bot.channels) * 30) seconds after the problematic WHO request if it weren't disabled by the availability of away-notify:

https://github.com/sopel-irc/sopel/blob/51300a1ab854d6ec82d90df1bc876188c03335ff/sopel/coretasks.py#L916-L925

I'm playing with some possible resolutions locally. The trick will be figuring out when to selectively ignore what the WHO response says about privileges.

dgw commented 1 year ago

I'm feeling pretty secure in at minimum leaving this until after 8.0.0 based on testing elsewhere (non-Unreal-3.2 IRCds). tfw is an alt I used for testing; GlaD0S is network services:

2023-11-15 14:29:42,523 <<      ':tfw!~qwebirc@my.hostname JOIN :#dgw\r\n'
2023-11-15 14:29:42,525 >>      'WHO tfw\r\n'
2023-11-15 14:29:42,525 <<      ':GlaD0S!EvilAI@I.Am.A.Potato MODE #dgw +qo tfw tfw\r\n'
2023-11-15 14:29:42,556 <<      ':irc.rizon.io 352 SopelTest #dgw ~qwebirc my.hostname * tfw Hr~@ :0 Rizon Web IRC\r\n'
2023-11-15 14:29:42,557 <<      ':irc.rizon.io 315 SopelTest tfw :End of /WHO list.\r\n'
2023-11-15 14:29:48,202 <<      ':dgw!~dgw@my.vhost PRIVMSG #dgw :,isop tfw\r\n'
2023-11-15 14:29:48,203 >>      'PRIVMSG #dgw :dgw: tfw is a chanop.\r\n'

This network CAP ACKed both multi-prefix and userhost-in-names, though only multi-prefix is relevant to this scenario. (It gives Sopel the ~@ in status flags instead of just ~.)

A quick testing plugin (isop.py) that I wrote to avoid dealing with ipython gave me those last two lines:

isop.py code

```py from __future__ import annotations from sopel import plugin @plugin.commands('isop') def isop(bot, trigger): if not trigger.group(3): bot.reply("You dum-dum. I need a nick!") return is_not = 'is not' if bot.channels[trigger.sender].privileges[trigger.group(3)] >= plugin.OP: is_not = 'is' bot.reply("{} {} a chanop.".format(trigger.group(3), is_not)) ```

We might need to consider allowing the periodic WHO polling to operate if multi-prefix isn't available too, even on servers supporting away-notify, because otherwise a nick with @+ will appear to Sopel as only @ in NAMES and WHO replies, and a MODE -o nick will leave Sopel thinking nick has no privileges when in fact it still has +v.

dgw commented 1 year ago

To follow up on the discussion here while bringing it back to the actual bug report: Maybe coretasks could keep track of pending WHO requests, but the User object shouldn't.

One possible solution: queue MODE commands for later processing if a target exists in the pending WHO queue.

Another, perhaps better, solution: Stop updating privileges from RPL_WHOREPLY / RPL_WHOSPCRPL. For both WHO and WHOX's c field, the channel included in the response when querying against a nickname is both arbitrary and optional (can be *), plus the inclusion of status flags for permissions on that channel is also optional.

For users already in the channel when Sopel joins, the initial RPL_NAMREPLY burst will give their current (or at least, highest) permissions. For a user joining after Sopel, the server has to send MODEs anyway.

I think Sopel pulls privileges from WHO responses as a fallback for servers not supporting multi-prefix.

The theory is that if somenick on the channel before Sopel joins has multiple privileges like +ao, but the server only sends the highest status character & (for +a) in NAMES/WHO replies, Sopel will only store the ADMIN privilege flag for somenick. If MODE -a somenick comes in, Sopel will think somenick's privileges are 0 (regular user) until the next WHO of that channel, because it's never seen the @ status prefix or a MODE +o somenick.

multi-prefix makes that situation easier, eliminating the period where Sopel thinks that somenick's privileges are 0, but not all servers support the extension. Plus if a non-multi-prefix server happens to support away-notify, even the fallback breaks: Sopel disables WHO polling with away-notify active.

Written, rewritten, and then deleted to focus on the bug: Ideas about reducing how often Sopel even sends WHO for a newly joined user it's already seen in another channel.

sopel-irc / sopel