search

mozilla / testdaybot

Mozilla QA Test Day IRC Bot
9 stars 12 forks source link

Add user verification for admin commands #33

Closed galgeek closed 9 years ago

galgeek commented 9 years ago

hi @whimboo !

here's a patch for issue #28. I've run some quick tests on it, seems ok. this could perhaps still be tidied up some, but it is far better than the code it replaces in the first patch. I've added some error-checking and tweaked the adminhelp array to support all this.

galgeek commented 9 years ago

hi @whimboo !

thanks for your feedback. the updated commit addresses it, except as I've noted in response to your line notes above.

galgeek commented 9 years ago

Hi @whimboo !

Thanks for your feedback. I've updated this pull request. I've also responded to a couple of your line notes above.

Maybe this is ready to be merged?

galgeek commented 9 years ago

hi @whimboo !

I've added another commit, adding verification for arguments to :addAdmin and :addHelper, checking first that the to be added is signed on to the channel before verifying with client.whois. This code tests fine, but I'm sure it could be condensed...

I'm curious to know whether you're comfortable with these limits on an added nick.

galgeek commented 9 years ago

hi @whimboo

I've restructured this listener to decrease nesting.

I've also backed out the checks on arguments to :addAdmin and :addHelper. Since we already check for logged on user of a registered nick and membership in the admin list before we run any admin command, it doesn't add much security to check these arguments, too. Moreover, it's not at all clear how we might verify a nick that's not currently signed in; that should probably be a separate issue.

thank you for your feedback!

galgeek commented 9 years ago

hi @whimboo

I see some possibilities, in /help ... output from the Mozilla IRC server, for doing additional validation of arguments to :addAdmin/:addHelper, in particular, the /check command available to operators (though apparently not to all channel operators, so I can't test them with op on the #testdaybotTest channel), and there may be others. Maybe you know more?

I do think this should be a separate issue.

whimboo commented 9 years ago

Moreover, it's not at all clear how we might verify a nick that's not currently signed in; that should probably be a separate issue.

Right now I think we should not allow the action and abort it. Only identified users should be able to get added.

whimboo commented 9 years ago

Also not exactly sure what you mean with the /help output. Maybe you can clarify?

galgeek commented 9 years ago

Hi @whimboo

Mozilla's IRC server has an operator command, /check <nick> that looks like it might provide info on nicks that are not currently signed in, or not using our channel. If we want to allow :addAdmin to add a user with a registered nick who is not currently signed in, we need an alternative to node-irc's Client.whois to check, since that works only for signed in users.

galgeek commented 9 years ago

Hi @whimboo I’ve rebased on updated master, and removed one more level of nesting. I used just git rebase master but still had to push +branch. Thank you again for all your feedback and help!

whimboo commented 9 years ago

Hm, /check is really for operators of the IRC network. So no-one of us will have access to it.

whimboo commented 9 years ago

This looks fine to me. I will get it merged in a minute. Thanks Barbara!