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

/op USER remove only works until USER reconnects #397

Closed
mikitsu opened this issue Jun 28, 2021 · 1 comment · Fixed by #398
Closed

/op USER remove only works until USER reconnects #397

mikitsu opened this issue Jun 28, 2021 · 1 comment · Fixed by #398

Comments

@mikitsu
Copy link
Contributor

mikitsu commented Jun 28, 2021

Describe the bug
Using the /op command to remove a user from being op only works until they reconnect again. Un-opping is done by adding an entry with 1ns expiry time, which is rejected by the Set (

if item.Value() == nil {
and
if item.Value() == nil {
) as expired (unless you're really, really fast, which never happened to me).
Upon reconnection, the public key is still in the op set, so a removed op regains op status.

Versions

  • Client version: any
  • Server version: at least the latest one, probably every one after /op with remove was added in 6070119

To Reproduce
Steps to reproduce the behavior:

  1. Start a chat server and connect as op
  2. run: /op (your name) remove
  3. you are no longer op (as seen with /whois and "must be op" for op commands)
  4. disconnect and reconnect
  5. you are op again (as seen with /whois and "must be op" for op commands)

This also works with two ops and one up-opping the other (probably a more typical case), but the above seems to be the simplest.

Expected behavior
op2 should not be op after being removed.

Additional context
I found this while writing a /whitelist for #270, so I'll just incorporate a fix for this there, if that's ok (doesn't seem extremely urgent to me).

The two possible solutions I can think of are:

  • Allow expired items to be added. This probably changes less code, but I haven't really checked if something depends on the current error. (Just grep ErrNil, which returned nothing outside of set/set.go)
  • Add DeWhitelist and DeOp methods to Auth that call Set.Remove. This requires more changes, but seems like The Right Way.

In any case, I'd probably also add some error logging or passing errors up where auth deals with Set.

@shazow
Copy link
Owner

shazow commented Jul 2, 2021

I thiiiiink the first solution should be fine. Might be easier to review and merge if it's in its own PR, if possible. :)

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 a pull request may close this issue.

2 participants