Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send all enabled capabilities on CAP LIST #67

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

ToxicFrog
Copy link
Contributor

Notably, this fixes an issue with weechat (and possibly some other clients) where if you ever run /cap or /cap list after connection registration, the client will see the "CAP * LIST sasl" reply as downgrading the connection to drop all capabilities except sasl.

@ToxicFrog ToxicFrog marked this pull request as draft February 12, 2024 20:40
@ToxicFrog
Copy link
Contributor Author

Just realized not all the internal capability tags match up to the on-the-wire ones even if you correct for _, so this is not fully baked.

@progval
Copy link
Owner

progval commented Feb 12, 2024

Oops, good catch.

You should be a global hashmap with @capability_names = @capabilities |> Enum.map(fn {name, {atom, _value}} -> {atom, name}) |> Map.new and use that to lookup the name for each enable capability.

@progval
Copy link
Owner

progval commented Feb 12, 2024

Only CAP LS 302 should have values in the reply, see https://ircv3.net/specs/extensions/capability-negotiation

@ToxicFrog
Copy link
Contributor Author

Only CAP LS 302 should have values in the reply, see https://ircv3.net/specs/extensions/capability-negotiation

Yep, that's what the conditional on 312-316 is for -- if it doesn't get the 302 it'll call it with is_302 = false and you don't get the values.

17:46 <-- matrix cap LS 302
17:46 --> matrix :server. CAP * LS :account-tag batch draft/account-registration=before-connect draft/channel-rename draft/chathistory draft/message-redaction draft/multiline=max-bytes=8192 draft/no-implicit-names draft/sasl-ir echo-message extended-join labeled-response message-tags sasl=PLAIN server-time soju.im/account-required standard-replies userhost-in-names
17:46 <-- matrix cap LS
17:46 --> matrix :server. CAP * LS :account-tag batch draft/account-registration draft/channel-rename draft/chathistory draft/message-redaction draft/multiline draft/no-implicit-names draft/sasl-ir echo-message extended-join labeled-response message-tags sasl server-time soju.im/account-required standard-replies userhost-in-names

@progval
Copy link
Owner

progval commented Feb 12, 2024

Ah right, i got confused because you added another pair of cases for "CAP LS" next to "CAP LIST"; those won't ever run so you don't really need the new function either.

@progval
Copy link
Owner

progval commented Feb 12, 2024

ah but clients can still call them outside registration, nvm

@ToxicFrog ToxicFrog marked this pull request as ready for review February 12, 2024 23:38
@progval
Copy link
Owner

progval commented Feb 12, 2024

Could you add tests for each of these cases?

@ToxicFrog
Copy link
Contributor Author

ah but clients can still call them outside registration, nvm

Yeah. There's not a lot of use for being able to do that until post hoc CAP REQ is also implemented (which I haven't touched here), but I wanted it for the sake of completeness and it wasn't hard to add.

Could you add tests for each of these cases?

Sure thing. Might not get to it until tomorrow though.

Notably, this fixes an issue with weechat (and possibly some other
clients) where if you ever run /cap or /cap list after connection
registration, the client will see the "CAP * LIST sasl" reply as
downgrading the connection to drop all capabilities except sasl.

This also enables the use of CAP LS after connection registration,
although without post-hoc CAP REQ support this is of limited use.
@ToxicFrog
Copy link
Contributor Author

Done, and writing the unit tests also revealed a bug that weechat was fine with but which could have confused other clients, which is now fixed.

@progval progval merged commit 64ff97c into progval:main Feb 13, 2024
8 checks passed
@ToxicFrog ToxicFrog deleted the cap-list branch February 13, 2024 17:09
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.

2 participants