Add user verification for admin commands #33

galgeek · 2014-12-23T19:25:52Z

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.

whimboo · 2014-12-29T15:31:26Z

bot.js

+                  ":addHelper" : ":addHelper <nickname> as a Test Day helper",
+                  ":next" : ":next <start as YYYY-MM-DDThh:mmZ> <end as YYYY-MM-DDThh:mmZ> <etherpad> <topic> as next Test Day",
+                  ":stats" : ":stats display Test Day stats",
+                  ":stop" : ":stop Test Day early"


I don't see why we need all those changes. Especially not on this PR, which should only cover additional admin checks. Please revert that and if really necessary file a new issue and PR.

Well, lets forget that. I see now why it was used for.

galgeek · 2014-12-30T03:58:44Z

hi @whimboo !

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

whimboo · 2014-12-30T09:09:48Z

bot.js

+              }
+              break;
+            case ":addAdmin":
+              if (cmdLen >= 2){


For now lets compare to exactly 2. We do not handle anything else than the first argument to the command. The same applies to the other commands.

Done.

My logic was that Javascript functions generally ignore extra parameters, and we do report what we've done.

Currently we pass in only a single argument, but my point is that the user has to make use of the correct command definition. We might have to do extra checks in the future, but for now that length check should be enough.

galgeek · 2014-12-30T19:36:03Z

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 · 2014-12-31T07:21:40Z

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.

whimboo · 2015-01-05T11:47:06Z

bot.js

@@ -126,72 +127,95 @@ client.addListener('message', function(from, to, message){

 client.addListener('pm', function(from, message){ // private messages to bot
  checkTestDay();
-  if (message.search(':adminhelp') === 0){
+  var command = message.split(" ");
+  if (command[0] in adminhelp){ // admin command
    if (testDayAdmins.indexOf(from) >= 0){


Lets negate this check and return early so that we can reduce the nested if/else conditions.

galgeek · 2015-01-06T07:57:00Z

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 · 2015-01-06T17:52:21Z

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 · 2015-01-06T19:12:07Z

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 · 2015-01-06T19:12:48Z

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

galgeek · 2015-01-06T20:42:22Z

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 · 2015-01-06T21:15:44Z

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 · 2015-01-07T14:16:15Z

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

whimboo · 2015-01-07T14:22:16Z

bot.js

+
+  client.whois(from, function(whoisinfo){
+    if (!(whoisinfo.accountinfo && whoisinfo.accountinfo.search('is logged in as') >= 0)){
+      client.say(from, "Sorry! You're not logged in with a registered nick.");


You can also return early here.

nit: something I have seen a lot and which might be helpful to you... opening { brackets will also get a blank before. You might want to have a look at our general Mozilla coding styles: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

whimboo · 2015-01-07T22:23:40Z

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

galgeek changed the title ~~issue 28: adding user verification per admin command~~ issue 28: adding user verification per admin command (#28) Dec 24, 2014

galgeek mentioned this pull request Dec 24, 2014

add user verification for each admin command #28

Closed

whimboo reviewed Dec 29, 2014
View reviewed changes

galgeek force-pushed the i28-add-admin-verification-per-command branch from ad520f8 to a80b734 Compare December 30, 2014 02:57

galgeek added a commit to galgeek/testdaybot that referenced this pull request Dec 30, 2014

Add user verification for admin commands (mozilla#33)

a80b734

galgeek changed the title ~~issue 28: adding user verification per admin command (#28)~~ Add user verification for admin commands (#33) Dec 30, 2014

galgeek changed the title ~~Add user verification for admin commands (#33)~~ Add user verification for admin commands Dec 30, 2014

galgeek mentioned this pull request Dec 30, 2014

issue 22: advertise/broadcast message #35

Closed

whimboo reviewed Dec 30, 2014
View reviewed changes

galgeek force-pushed the i28-add-admin-verification-per-command branch from a80b734 to d224041 Compare December 30, 2014 19:31

galgeek added a commit to galgeek/testdaybot that referenced this pull request Dec 30, 2014

Add user verification for admin commands (mozilla#33)

201352f

whimboo reviewed Jan 5, 2015
View reviewed changes

galgeek force-pushed the i28-add-admin-verification-per-command branch from 5e7debf to e7b943f Compare January 6, 2015 07:43

galgeek force-pushed the i28-add-admin-verification-per-command branch from e7b943f to 340f07b Compare January 6, 2015 21:06

galgeek added a commit to galgeek/testdaybot that referenced this pull request Jan 6, 2015

Add user verification for admin commands (mozilla#33)

7de0e66

whimboo reviewed Jan 7, 2015
View reviewed changes

Add user verification for admin commands (mozilla#33)

18f5b1d

galgeek force-pushed the i28-add-admin-verification-per-command branch from 340f07b to 18f5b1d Compare January 7, 2015 22:11

whimboo merged commit 18f5b1d into mozilla:master Jan 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user verification for admin commands #33

Add user verification for admin commands #33

galgeek commented Dec 23, 2014

whimboo Dec 29, 2014

whimboo Dec 29, 2014

galgeek commented Dec 30, 2014

whimboo Dec 30, 2014

galgeek Dec 30, 2014

whimboo Dec 30, 2014

galgeek commented Dec 30, 2014

galgeek commented Dec 31, 2014

whimboo Jan 5, 2015

galgeek commented Jan 6, 2015

galgeek commented Jan 6, 2015

whimboo commented Jan 6, 2015

whimboo commented Jan 6, 2015

galgeek commented Jan 6, 2015

galgeek commented Jan 6, 2015

whimboo commented Jan 7, 2015

whimboo Jan 7, 2015

whimboo commented Jan 7, 2015

Add user verification for admin commands #33

Add user verification for admin commands #33

Conversation

galgeek commented Dec 23, 2014

whimboo Dec 29, 2014

Choose a reason for hiding this comment

whimboo Dec 29, 2014

Choose a reason for hiding this comment

galgeek commented Dec 30, 2014

whimboo Dec 30, 2014

Choose a reason for hiding this comment

galgeek Dec 30, 2014

Choose a reason for hiding this comment

whimboo Dec 30, 2014

Choose a reason for hiding this comment

galgeek commented Dec 30, 2014

galgeek commented Dec 31, 2014

whimboo Jan 5, 2015

Choose a reason for hiding this comment

galgeek commented Jan 6, 2015

galgeek commented Jan 6, 2015

whimboo commented Jan 6, 2015

whimboo commented Jan 6, 2015

galgeek commented Jan 6, 2015

galgeek commented Jan 6, 2015

whimboo commented Jan 7, 2015

whimboo Jan 7, 2015

Choose a reason for hiding this comment

whimboo commented Jan 7, 2015