Skip to content

Add +T no-CTCP usermode#1123

Merged
slingamn merged 1 commit intoergochat:masterfrom
clukawski:no-ctcp-user-mode
Jun 28, 2020
Merged

Add +T no-CTCP usermode#1123
slingamn merged 1 commit intoergochat:masterfrom
clukawski:no-ctcp-user-mode

Conversation

@clukawski
Copy link
Copy Markdown
Contributor

Adds no-CTCP usermode as per #1007

@slingamn
Copy link
Copy Markdown
Member

slingamn commented Jun 8, 2020

This looks great. I'll leave it open a little longer before merging.

@slingamn slingamn linked an issue Jun 8, 2020 that may be closed by this pull request
@slingamn
Copy link
Copy Markdown
Member

slingamn commented Jun 8, 2020

@ajaspers points out that this should prevent receiving CTCP messages, not sending them.

@KoraggKnightWolf
Copy link
Copy Markdown

Sending CTCPs should indeed still be possible if the user has +T also what an optional oper-override option? Either just a config option or an oper priv? It might be useful for opers to know what client and version a user has (regarding the client parsing issue Insp 3 has revealed for example).

@slingamn
Copy link
Copy Markdown
Member

I think I'm uncomfortable with this as an operator privilege. (It also presents a UI problem: normally we make operators exercise their privileges explicitly, e.g., with SAMODE or SAJOIN, but there's no SAPRIVMSG.)

irc/handlers.go Outdated

if rb.session.isTor && utils.IsRestrictedCTCPMessage(message) {
isCTCP := utils.IsRestrictedCTCPMessage(message)
if isCTCP && client.modes.HasMode(modes.UserNoCTCP) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still checking the mode on the sender, not the recipient?

@slingamn
Copy link
Copy Markdown
Member

This looks pretty good. Could you:

  1. Remove ERR_CANTSENDTOUSER
  2. Squash this to a single commit on top of master?

irc/handlers.go Outdated
}

// Restrict CTCP message for target user with +T
if message.IsRestrictedCTCPMessage() && user.modes.HasMode(modes.UserNoCTCP) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional performance-related feedback: either reverse the order of these checks, or cache the result of IsRestrictedCTCPMessage for use lower down with Tor sessions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@clukawski clukawski force-pushed the no-ctcp-user-mode branch from 2a7435f to 14cb8fc Compare June 28, 2020 04:01
@clukawski
Copy link
Copy Markdown
Contributor Author

Squashed and removed the unused numeric.

@clukawski clukawski force-pushed the no-ctcp-user-mode branch from 14cb8fc to fca2900 Compare June 28, 2020 04:04
@clukawski
Copy link
Copy Markdown
Contributor Author

Ok I reversed those checks as well, all squashed again

@slingamn slingamn merged commit d7a6222 into ergochat:master Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add no-CTCP user mode

3 participants