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 an option to stop the connection when SASL fails. #514

Merged
merged 3 commits into from Dec 21, 2016

Conversation

Projects
None yet
4 participants
@LemonBoy
Copy link
Member

commented Jul 12, 2016

No description provided.

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2016

I guess it's fine, I'm a bit sceptic of the internal flag and related code duplication but whatever. @dequis ?

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2016

(should update abi constant)

@@ -79,6 +79,7 @@ struct _IRC_SERVER_REC {
int cap_complete:1; /* We've done the initial CAP negotiation */

guint sasl_timeout; /* Holds the source id of the running timeout */
int sasl_success:1; /* Did we authenticate successfully ? */

This comment has been minimized.

Copy link
@dequis

dequis Aug 11, 2016

Member

Put this after cap_complete:1, otherwise it won't take advantage of the bitfield.

@dequis

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

That seems ok, but i'm a bit worried about that server_disconnect call. Should check that the code that runs after the signal is emitted doesn't depend on the connection still existing.

@ahf

This comment has been minimized.

Copy link
Member

commented Oct 17, 2016

sasl_disconnect_on_fail sounds wrong in my head. I would at least change it to something like sasl_disconnect_on_failure or _on_error.

Should the policy also be enforced on a global "per-client" level?

@dequis

This comment has been minimized.

Copy link
Member

commented Oct 17, 2016

Renaming to _on_failure seems ok (but i don't feel too strongly about it)

Should the policy also be enforced on a global "per-client" level?

What do you mean? It looks like a global setting to me. I think that's fine.

@ahf

This comment has been minimized.

Copy link
Member

commented Oct 17, 2016

Ack. I think I wrote the wrong question: should it be possible to control this on a per connection level.

@LemonBoy LemonBoy force-pushed the LemonBoy:sasl_fail branch from 81e950f to 27c388b Oct 20, 2016

@dequis dequis removed the needs changes label Dec 6, 2016

@dequis

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

I don't think we need to have this as a per-connection setting. It's more of a personal preference, similar to "do i care about appearing decloaked or not"

@LemonBoy LemonBoy force-pushed the LemonBoy:sasl_fail branch from 27c388b to 4ccffd8 Dec 12, 2016

@dequis

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

Should this also set server->no_reconnect = TRUE too?

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2016

Maybe the sasl failure is temporary

@ailin-nemui ailin-nemui merged commit 77ff8f5 into irssi:master Dec 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.