-
Notifications
You must be signed in to change notification settings - Fork 70
Add 'user' option for server #448
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
Conversation
osa1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @anarsoul, thanks for the patch.
I think it's not quite right, see my inline comment.
There are also CI failures.
crates/libtiny_wire/src/lib.rs
Outdated
| pub fn user(hostname: &str, realname: &str) -> String { | ||
| format!("USER {hostname} 8 * :{realname}\r\n") | ||
| pub fn user(user: &str, hostname: &str, realname: &str) -> String { | ||
| format!("USER {user} {hostname} * :{realname}\r\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This format doesn't seem right according to RFC 2812:
Lines 577 to 609 in c5e68d9
| 3.1.3 User message | |
| Command: USER | |
| Parameters: <user> <mode> <unused> <realname> | |
| The USER command is used at the beginning of connection to specify | |
| the username, hostname and realname of a new user. | |
| The <mode> parameter should be a numeric, and can be used to | |
| automatically set user modes when registering with the server. This | |
| parameter is a bitmask, with only 2 bits having any signification: if | |
| the bit 2 is set, the user mode 'w' will be set and if the bit 3 is | |
| set, the user mode 'i' will be set. (See Section 3.1.5 "User | |
| Modes"). | |
| The <realname> may contain space characters. | |
| Numeric Replies: | |
| ERR_NEEDMOREPARAMS ERR_ALREADYREGISTRED | |
| Example: | |
| USER guest 0 * :Ronnie Reagan ; User registering themselves with a | |
| username of "guest" and real name | |
| "Ronnie Reagan". | |
| USER guest 8 * :Ronnie Reagan ; User registering themselves with a | |
| username of "guest" and real name | |
| "Ronnie Reagan", and asking to be set | |
| invisible. | |
According to the RFC, USER message should have 4 parameters:
- User: this is currently the
hostnameargument - Mode: this is currently hard-coded as
8. I don't remember what this means, we should probably document. - Unused: hard-coded as
*, this is copied from the RFC. - Realname: the
realnameargument.
With this you make the mode parameter hostname, which doesn't seem right according to the RFC. RFC says "mode parameter should be numeric, and can be used to automatically set user modes ...".
Maybe you just want to pass different hostname argument to this function? Did you check if the hostname is configurable in the config file? (I don't remember the answer, it may already be configurable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, I referred to https://www.rfc-editor.org/rfc/rfc1459#section-4.1.3
hostname isn't configurable, current code just uses &self.nicks[0] for hostname. But anyway, the name is misleading, as per RFC2812 it should be user
I'll fix it up tonight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case it might make sense to follow one of the more recent specs. Maybe https://modern.ircdocs.horse/#user-message.
According to this spec there should be one user name and one real name. So the hostname can be dropped.
Could you try it and see if it works on some of the popular servers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Is it sufficient to test it on OFTC and Libera?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are the ones I use. Are there others that are popular these days? Idk tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I updated the patch to just replace hostname with user and to allow specifying it in the config.
I confirmed that the change works for me on OFTC and Libera and that it works correctly with ZNC as well.
9da9082 to
98aaa09
Compare
|
cargo-fmt CI failure should be fixed now |
osa1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @anarsoul , looks great.
Just one more thing -- could you update the default config file in crates/tiny/config.yml with the new field, with documentation? I think we should add this field right above realname:
# Servers to automatically connect.
servers:
- addr: irc.oftc.net
port: 6697
tls: true
# HERE
realname: yourname
nicks: [tiny_user]
To be backwards compatible with the current version it should be commented-out, with documentation.
d3b95a5 to
3a05cc5
Compare
ZNC uses 'user' as a way to specify what network to connect to if there are several networks configured. E.g. myspecialuser/oftc or myspecialuser/libera Replace 'hostname' in 'wire::user' with 'user' and allow specifying it in the config
ZNC uses 'user' as a way to specify what network to connect to if there are several networks configured. E.g. myspecialuser/oftc or myspecialuser/libera