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

Add password_command for sasl and nick_password #583

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

4e554c4c
Copy link
Contributor

This commit builds off of #552 and allows the user to supply a command instead of a password file anywhere that a password file is used. If the password command ends unsuccessfully, then an error is displayed.

Trailing whitespace is now trimmed from password commands, as it is commonplace for commands to leave a trailing newline after the password they output.

Resolves #580

This commit builds off of squidowl#552 and allows the user to supply a command
instead of a password file anywhere that a password file is used. If the
password command ends unsuccessfully, then an error is displayed.

Trailing whitespace is now trimmed from password commands, as it is
commonplace for commands to leave a trailing newline after the password
they output.

Resolves squidowl#580
@4e554c4c 4e554c4c changed the title Add password_command for sasl and nick_password` Add password_command for sasl and nick_password Sep 20, 2024
@casperstorm
Copy link
Member

I think this deserves a entry in CHANGELOG. I'll do proper review tomorrow.

data/src/config.rs Outdated Show resolved Hide resolved
Copy link
Member

@casperstorm casperstorm left a comment

Choose a reason for hiding this comment

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

A single nit regarding naming, but overall it looks great.

Copy link
Member

@casperstorm casperstorm left a comment

Choose a reason for hiding this comment

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

Looks good to me. @tarkah do you have anything for this?

data/src/server.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
#[error("Exactly one of sasl.plain.password, sasl.plain.password_file or sasl.plain.password_command must be set.")]
DuplicateSaslPassword,
#[error("Config does not exist")]
ConfigMissing { has_yaml_config: bool },
}

impl From<std::io::Error> for Error {
Copy link
Contributor Author

@4e554c4c 4e554c4c Sep 26, 2024

Choose a reason for hiding this comment

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

I tried to refactor this bit out (the custom From impl) for this PR, but it turns out that every single Error in Halloy has to be Clone

im not a huge fan but also fixing that is out of scope for this PR

Copy link
Member

@tarkah tarkah left a comment

Choose a reason for hiding this comment

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

Changes look great! I think that cleaned it up very nicely. Thanks for tackling this

@casperstorm casperstorm merged commit 1b334aa into squidowl:main Sep 27, 2024
1 check passed
@casperstorm
Copy link
Member

Thanks for yet another good PR.

@4e554c4c 4e554c4c deleted the password_command_sasl branch September 27, 2024 09:39
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.

Password command not working for SASL
3 participants