aronson / discord-irc

Connects Discord and IRC channels by sending messages back and forth.
MIT License
4 stars 2 forks source link

Feature Request: Pluralkit message proxying #52

Closed KeiroD closed 9 months ago

KeiroD commented 10 months ago

So I know I was mentioning Pluralkit support in #29, but I think I'd like to expand on that request and explain what exactly I mean by this.

Can we get discord-irc to evaluate if a message is being sent by a user in a given role by Pluralkit or tupper? If the user is in that role, buffer the message internally inside the bot, and then if a delete event happens with a subsequent second message from a bot, do not display it. If there is no delete event, display the original message, as Pluralkit/Tupper currently does?

For example, see image:

moo

Some users have stated that seeing this is undesired and I don't know if it can be done. Theoretically it should be possible to do?

aronson commented 10 months ago

Looks like there's a library for real PluralKit support out there. I can instead query the PK API to determine if a message is supposed to be proxied or not, and skip the non-proxied message if it is.

I'm not sure if there's a library or API for Tupper I can leverage. I'm going to start with PluralKit since they've made it easy. I can't find developer resources for Tupper, just end-user resources.

I'm hesitant to add anything that buffers messages outside of the queue used for the flood delay due to potential race conditions. Messages could be sent out of order received.

Uncertain, but you may need to generate and provide a PluralKit API key to the bot when this support is enabled. We'll see, I think read-only access doesn't require a key. I'm pretty close to testing the PK support code now.

KeiroD commented 10 months ago

Tupper is probably OK to leave out in that case. Most servers I've been in seem to use PluralKit by default.

Should an API key be required, I'll add it to my config. That being said, yeah, that's the only thing I had concern about when I was asked if we could get PK support in this for that reason, the race condition, and why I asked if it was possible to do something like that but wasn't entirely certain if it was something that could be managed in this way. In theory, anyway, heh.

aronson commented 10 months ago

Hmm... the API isn't behaving as I expected. This may require the role setup with delay you mentioned.

While I wanted to avoid adding it, I can try implementing some kind of queueing structure internally that tracks the PluralKit interaction and avoids printing the original message. It might take me some time. It may also allow denial-of-service attacks from your users with the role enabled. I hope you trust your users 😉

aronson commented 10 months ago

OK I got it working without any hacks.

Here's the process:

It seems to work in my initial tests. I have a work day ahead of me but when that's done I can get this in a state ready to push to others.

image image
aronson commented 10 months ago

OK, I have a new testing build out. You can run it with the original docker commands for upgrade, but change the image name like so: docker run -v /path/to/config.json:/app/config.json ghcr.io/aronson/discord-irc:sha-d3ce64e

Note the :tag format is how one manually specifies which version of an image to use. Not sure I covered that in my guide.

If you need to go back, instead run using: ghcr.io/aronson/discord-irc:5.0.0

I've tested it a fair amount on my end, but I'm just one guy. If you're able to test this and can confirm it's working for you, we can get this out for everyone.

You'll need to add this snippet to your config as well, no API key necessary:

{
  // Other bot properties...
  "pluralKit": true,
}

Make sure to remove it if you roll back.

I believe I've implemented it in a way that will prevent any potential attacks without adding undue lag to messages.

KeiroD commented 10 months ago

Seems to work? At least, it's not crashing, but I am still seeing relayed repeated messages from PK.

I stuck the "pluralKit": true, option under ircOptions, but not sure if that's the appropriate location for it.

aronson commented 10 months ago

It goes under the main scope. It'll be ignored if it's in ircOptions as it's not a Deno-IRC parameter, it's a Discord-IRC parameter. ircOptions is just what's passed to the underlying IRC library. The flood protection was actually a feature I added to the library upstream, hence the last config feature going in ircOptions, apologies for any confusion.

My testing config for the feature:

{
  "nickname": "CheemsBot",
  "server": "irc.libera.chat",
  "port": 6697,
  "tls": true,
  "discordToken": "[REDACTED]",
  "channelMapping": {
    "#irc-test": "#cheems-test",
    "12345454325452345": "#cheems-test2"
  },
  "pluralKit": true,
  "ircOptions": {
    "nick": "CheemsBot",
    "username": "CheemsBot",
    "realname": "cheems",
    "password": "[REDACTED]",
    "floodDelay": 1000
  },
  "format": {
    "commandPrelude": "{$nickname} said:",
    "ircText": "<{$displayUsername} [@{$discordUsername}]> {$text}",
    "urlAttachment": "<{$displayUsername}> {$attachmentURL}",
    "discord": "<@**{$author}**> {$withMentions}"
  },
  "ircNickColor": true,
  "commandCharacters": ["?", ">", "!"],
  "ircStatusNotices": false,
  "webhooks": {
    "#irc-test": "https://discord.com/api/webhooks/[REDACTED]"
  }
}
KeiroD commented 10 months ago

Updated mine to match yours:

[
  {
    "nickname": "Example",
    "server": "irc.example.com",
    "tls": true,
    "port": 6697,
    "autoSendCommands": [
      ["PRIVMSG", "NickServ", "IDENTIFY password"]
    ],
    "discordToken": "redacted",
    "channelMapping": {
      "#redacted": "#redacted",
      "redacted1": "#redacted1",
      "redacted2": "#redacted2",
    },
    "pluralkit": true,
    "format": {
      "commandPrelude": "Command sent by {$nickname}",
      "ircText": "<{$displayUsername}> {$text}",
      "urlAttachment": "<{$displayUsername}> {$attachmentURL}",
      "discord": "**<{$author}>** {$withMentions}",
      "webhookAvatarURL": "https://robohash.org/{$nickname}"
    },
     "ircStatusNotices": true,
    "ignoreUsers": {
      "irc": ["DCB", "discord-irc"],
      "discordIds": ["211682893293027330", "1040073034373005324"],
  "ircOptions": {
    "password": "redacted",
    "floodDelay": 1000,
    },
  }
 }
]

Still seeing proxied messages coming through while using discord-irc:sha-d3ce64e.

aronson commented 10 months ago

You have a lowercase k in pluralKit. Strange that should've been caught by the schema validator.... Testing the docker image now.

EDIT: works fine on my end in docker as well as local binaries

KeiroD commented 10 months ago

... Dammit, I shoulda caught that. That did the trick. Such a tiny little change!!! aaaaaaaahahaha.

Sorry about that. Looks good here so far. Relays correctly!

As an aside, what do the command prefixes actually do here for the bot? I don't think I've ever seen what's documented for that.

aronson commented 10 months ago

Command characters/prefixes get the bot to skip the normal prefix (e.g. <displayName> @username) on the text sent to IRC or Discord and instead split it into two messages. This allows bots to parse the command more easily or even at all in some cases.

Example:

image image
KeiroD commented 10 months ago

Aha, gotcha. Thanks!

So far, everything is working for me. I've so far not seen any blips or any odd issues with regards to proxying PK at this time so I'd say we're pretty much good to go.

aronson commented 10 months ago

Excellent! One last thought...

I could add a caching feature to reduce how many API requests the bot sends to the PK API (it's 2 per message right now and the limit is 2 per second IIRC), but that would mean your users might have to wait to see their changes reflected in the bot. You'd set some config.json option to X seconds/minutes and it would use the old result for each user for that long before refreshing. This would reduce network traffic and be somewhat courteous to the PluralKit team (though your and my bots are a drop in the bucket in terms of their total traffic).

Would you be interested in that feature? I think you mentioned your floodDelay is 1 second so it might not matter at all, really, as you'll never outpace the PK API rate limit with outgoing messages to IRC.

KeiroD commented 10 months ago

I'd be definitely interested in that feature! I think making sure that PK doesn't get abused by the bot is an excellent idea.

Your suggestion sounds good to me. And yeah, haha, definitely a drop in the bucket but imo any courteous behaviour we can do to ensure that things work well is an excellent way to go.

aronson commented 10 months ago

Caching is now available with image sha-3b07580.

No config changes required if you plan to use the default of 5 minutes.

If you want to override, add a property next to "pluralKit": true:

{
  // Other bot properties...
  "pluralKit": true,
  "pkCacheSeconds": 3600, // one hour
  // Other bot properties...
}
KeiroD commented 10 months ago

Updated, I didn't change the delay option. So far so good, no issues as of yet.

aronson commented 10 months ago

Well it's been over 24 hours and it sounds like it hasn't had any issues!

I think we're good to release this feature if you're satisfied with it.

KeiroD commented 10 months ago

Yup, it's good to go! I was also able to help my friend who was also working on updating to this bot so they could get the same support and they've not reported any further issues since either. :D

KeiroD commented 10 months ago

Seems I spoke too soon. It seems like certain tags aren't quite working? For example:

[11:07:25] <AGNPH> <Samui Shiiba ♎> Ah, could that be a reason for it not working with certain tags?
[11:08:26] <AGNPH> <Omni Lyansuria> Might be? Theoretically it should cache all tags the moment it sees PK presence for all members in a Discord server but not entirely sure.
[11:09:04] <AGNPH> <Samui Shiiba ♎> [testing]
[11:09:05] <AGNPH> <Pyrova Shiiba ♎> testing
[11:09:08] <AGNPH> <Omni Lyansuria> I just know that there's an internal cache but it could also be that it needs to know more tags? I don't exactly have a direct view into what it's doing.
[11:09:13] <AGNPH> <Omni Lyansuria> hmm.
[11:09:14] <AGNPH> <Samui Shiiba ♎> Let me know if you see both.
[11:09:17] <AGNPH> <Omni Lyansuria> yeah I do
[11:09:36] <AGNPH> <Mizu Shiiba ♎> Checking
[11:09:57] <~Seoladan> That one works as expected, only one.
[11:10:11] <AGNPH> <Samui Shiiba ♎> Let us try something for science. Hold on.
[11:10:39] <~Seoladan> yup
[11:11:21] <AGNPH> <Samui Shiiba ♎> py.test
[11:11:21] <AGNPH> <Pyrova Shiiba ♎> test
[11:11:32] <~Seoladan> Double.
[11:11:42] <AGNPH> <Samui Shiiba ♎> Probably hasn't cached the new tag.
[11:11:45] <~Seoladan> fair
[11:11:49] <~Seoladan> try again?
[11:11:55] <AGNPH> <Samui Shiiba ♎> py.testing again
[11:11:57] <AGNPH> <Pyrova Shiiba ♎> testing again
[11:12:00] <~Seoladan> Double.
[11:12:05] <AGNPH> <Samui Shiiba ♎> Huh
[11:12:06] <~Seoladan> Hmmm.
[11:12:14] <~Seoladan> I'ma ping back the dev.

I hadn't touched the pkCacheSeconds functionality, at least until now. That didn't seem to make any difference.

Edit: Wait... is this pkCacheSeconds delay controlling how long it takes for the tags to be detected and properly used? Or is it just how long the bot caches the messages?

aronson commented 10 months ago

It's pretty simple

Quite interesting that tag didn't parse. There's nothing exceptional about it. If the user added it to their account within five minutes though it wouldn't be caught. Don't think that happened here.

You can set it to zero seconds to get the old check-every-time behavior as a test.

KeiroD commented 10 months ago

Having the user test and see if that's the issue. Will report back.

aronson commented 10 months ago

Problem identified and fixed.

Sometimes certain special characters in tags were being interpreted as pattern-matching control characters (I use Regular Expressions) and messing with detection in general. Some of these characters include . and []. The ones I tested with weren't control characters so I didn't catch this.

Pushing a build now.

KeiroD commented 10 months ago

Heh. Alright. :p I've also seen users make use of emojis and other tags for identifying proxied characters and the like, so I'll report if I see any that don't get caught.

aronson commented 10 months ago

Image tag sha-18a62c2 should fix the issue.

aronson commented 10 months ago

Emoji... oh no. Oh no no no. That's either gonna work instantly or be a REALLY fun one to write hahahaha

KeiroD commented 10 months ago

Emoji... oh no. Oh no no no. That's either gonna work instantly or be a REALLY fun one to write hahahaha

Hahahahah ohhh... yes. It'll likely be the latter, knowing our luck. But yes, fully agreed. :p

aronson commented 10 months ago

OK so... emoji like 😇 work if copy/pasted. They don't work if selected through the discord emoji picker, but nor does pluralkit proxy them either, so we're good!

KeiroD commented 10 months ago

Heh. That sounds good! I'll continue having people test these and we should hear back soon... hopefully. It's somewhat quiet for the server I'm testing this on at the moment, so may be a bit of time.

KeiroD commented 10 months ago

Huh. Interestingly a PK user doesn't have a tag but they do proxy correctly... except the messages are posted double. Example:

[04:20:55] <AGNPH> <AA System> Mood
[04:20:56] <AGNPH> <Ari> Mood
[04:22:10] <~Seoladan> ... huh, that one didn't get proxied.
[04:24:24] <AGNPH> <AA System> IRC can't handle my snekness
[04:24:24] <AGNPH> <Ari> IRC can't handle my snekness
[04:25:02] <~Seoladan> You don't have a tag do you?
[04:25:09] <AGNPH> <AA System> I don't
[04:25:10] <AGNPH> <Ari> I don't
[04:25:14] <~Seoladan> That's why, then.
[04:25:39] <AGNPH> <AA System> pk;s servertag | AA
[04:25:39] <AGNPH> <PluralKit> ✅ System server tag changed (using 4/79 characters). Member names will now end with | AA when proxied in the current server '🦊 Kitsunet 🌈'.
[04:25:47] <AGNPH> <AA System> I have one now
[04:25:47] <AGNPH> <Ari | AA> I have one now
aronson commented 10 months ago

A tag is required.

Adding the tag live means they have to wait 5 minutes for it to be refreshed.

aronson commented 10 months ago

Wait, I see there's something called autoProxySettings that might have the information I need to do it tagless

aronson commented 10 months ago

Oh, ha, they added a server tag, not a message proxy tag. When I say tags I specifically mean the prefixes and suffixes applied in the PK web UI.

KeiroD commented 10 months ago

Yep, I assumed that's what you meant by way of the tags as obviously the prefixes would be required and it does work like that.

But yeah the autoProxySettings might do the trick, I've seen people using PK to autolatch their proxies so they don't have to deal with proxying every message.

aronson commented 10 months ago

The PluralKit API does not seem to inform me that the user has a latch or front autoProxy enabled.

This is the output from the API from my users with latch mode enabled.

autoProxySettings is undefined, and none of the members have autoproxy_enabled set.

It sounds like I can't solve this on my own. I'll need to reach out to PK.

System {
  id: "idoay",
  uuid: "092ea041-a605-4b19-ac2f-405553acdc89",
  name: "Luigilike",
  description: null,
  tag: null,
  pronouns: null,
  avatar_url: null,
  banner: null,
  color: null,
  created: 2023-10-12T01:02:39.061Z,
  privacy: null,
  members: undefined,
  groups: undefined,
  fronters: undefined,
  switches: undefined,
  settings: undefined,
  config: undefined,
  autoproxySettings: undefined
}
[
  Member {
    id: "ongyd",
    uuid: "b54d1112-7567-4e52-b96c-b37fb19d38c9",
    system: "",
    name: "Super Luigi",
    display_name: null,
    color: null,
    birthday: null,
    pronouns: null,
    avatar_url: "https://i.redd.it/rs7ew3qulzny.png",
    webhook_avatar_url: null,
    banner: null,
    description: null,
    created: 2023-10-12T01:02:56.698Z,
    proxy_tags: [ { prefix: "super:", suffix: null } ],
    keep_proxy: false,
    tts: false,
    autoproxy_enabled: null,
    message_count: 2,
    last_message_timestamp: 2023-10-12T01:10:30.384Z,
    privacy: null,
    groups: undefined,
    settings: undefined
  },
  Member {
    id: "ylxjs",
    uuid: "267bbd0f-e62a-44f9-a503-2b80db5f2c94",
    system: "",
    name: "Fire Luigi",
    display_name: null,
    color: null,
    birthday: null,
    pronouns: null,
    avatar_url: "https://static.wikia.nocookie.net/luigi-mansionfanon/images/a/ab/Fire_Luigi.png/revision/latest?cb=2"... 13 more characters,
    webhook_avatar_url: null,
    banner: null,
    description: null,
    created: 2023-10-12T01:03:03.925Z,
    proxy_tags: [ { prefix: "fire:", suffix: null } ],
    keep_proxy: false,
    tts: false,
    autoproxy_enabled: null,
    message_count: 109,
    last_message_timestamp: 2023-10-24T09:43:22.483Z,
    privacy: null,
    groups: undefined,
    settings: undefined
  },
  Member {
    id: "graef",
    uuid: "726d3041-06bb-4e25-a281-d7e82abc7f90",
    system: "",
    name: "Mr. L",
    display_name: "Mr. L",
    color: null,
    birthday: null,
    pronouns: null,
    avatar_url: "https://images-wixmp-ed30a86b8c4ca887773594c2.wixmp.com/f/944f7b7e-7f19-4e70-a7d6-2c542fd1a80d/d6uml"... 156 more characters,
    webhook_avatar_url: null,
    banner: null,
    description: null,
    created: 2023-10-24T07:41:15.427Z,
    proxy_tags: [
      { prefix: "[", suffix: "]" },
      { prefix: "py.", suffix: null },
      { prefix: ".😇", suffix: null }
    ],
    keep_proxy: false,
    tts: false,
    autoproxy_enabled: null,
    message_count: 4,
    last_message_timestamp: 2023-10-24T07:59:11.234Z,
    privacy: null,
    groups: undefined,
    settings: undefined
  }
]
KeiroD commented 10 months ago

Probably for the best. I can probably yeet this into another server that makes heavy use of PK and has lots of members that use auto-latching. I'll arrange to make it happen at some point within the next day or so.

aronson commented 10 months ago

I've reached out to PK on their official support discord. Hopefully we'll get a solution as we work through it.

aronson commented 10 months ago

The PK API only allows users to look up their own autoProxy settings with an API key. It's not possible to publicly query this information.

As a result, autoProxy setups are not supported until something changes on their end, or someone solves the problem of "is a message about to be proxied" in a different way.

aronson commented 10 months ago

You could just add the user to the ignoreUsers.

KeiroD commented 10 months ago

Oof, that's unfortunate, but understandable, I suppose. I'd say that's probably about as good as we'll get at this point, methinks. Not encountered any other issues or anything like that, though.

aronson commented 10 months ago

OK, I've got it, the solution:

in #welcome on a server using the bot: "Hi, if you're using PK autoproxy support please react to this message to get a role! It'll help the bot know to ignore your messages and only send the proxy message."

We can add to ignoreUsers and have an ignoreRoles option. When users want to autoProxy they react to get the ignore role. Simple as that. (This is of course handled by another bot, there's many out there.)

Would that approach work for you?

KeiroD commented 10 months ago

It sure would work for me, yup!

aronson commented 10 months ago

OK I've implemented it on image tag sha-5a3a9cd

You set it up like so:

{
  // ...
  "ignoreUsers": {
    "roles": ["1144086714797793400", "114408954499793702"]
  },
  // ...
}

If you have other entries in ignoreUsers like discord it goes under them

"ignoreUsers": {
  "irc": ["irc_nick1", "irc_nick2"], // Ignore specified IRC nicks and do not send their messages to Discord.
  "discord": ["discord_nick1", "discord_nick2"], // Ignore specified Discord nicks and do not send their messages to IRC.
  "discordIds": ["198528216523210752"], // Ignore specified Discord ids and do not send their messages to IRC.
  "roles": ["1144086714797793400", "114408954499793702"]
}

It should support both role IDs as well as the name of the role, but I haven't tested if you need to include the "@" in the role name.

Hopefully this works 🙂

KeiroD commented 10 months ago

At work, but will deploy tomorrow and get back to you on test results. :D

KeiroD commented 10 months ago

Updated but still testing. Will let ya know of results.

KeiroD commented 10 months ago

Seems bot is crashing with this error on sha-5a3a9cd. Previous revision, sha-18a62c2 is running fine, but not using the new options for obvious reasons:

error: Uncaught (in promise) TypeError: this.findDiscordChannel is not a function
    const discordChannel = this.findDiscordChannel(channel);
                                ^
    at CustomIrcClient.sendExactToDiscord (file:///app/lib/mediator.ts:391:33)
    at CustomIrcClient.onJoin (file:///app/lib/ircClient.ts:137:16)
    at CustomIrcClient.emit (https://deno.land/x/irc@v0.15.0/core/events.ts:22:7)
    at https://deno.land/x/irc@v0.15.0/plugins/join.ts:27:12
    at CustomIrcClient.emit (https://deno.land/x/irc@v0.15.0/core/events.ts:22:7)
    at CustomIrcClient.loop (https://deno.land/x/irc@v0.15.0/core/client.ts:86:14)
    at eventLoopTick (ext:core/01_core.js:183:11)
aronson commented 10 months ago

I bound the method wrong, should be fixed in image sha-290271a with the feature you're looking for

KeiroD commented 10 months ago

I bound the method wrong, should be fixed in image sha-290271a with the feature you're looking for

All good. Testing again! Will post back update.

Edit: Looks like crash isn't happening anymore. Testing with a larger server with this.

KeiroD commented 10 months ago

Looks like the server I've been testing this on still hasn't added the roles I asked... still waiting.

Edit: But does seem to work otherwise.

aronson commented 10 months ago

Image sha-bb9ad77 now features transparent 30s caching on Discord Members, Channels, and Roles.

I found it made messages send faster.

This also fixes mentions of Discord bots that include the discriminator from IRC->Discord.

KeiroD commented 10 months ago

Nice! Updating to that version now. Will report back.