Is eval() always evil?

[Shawn Dowler]

I recently put extended a solution from Mark Stanger (Crossfuze) on his blog called http://www.servicenowguru.com/system-definition/group-managers-manage-group-members/. The difficulty I came across was that I needed to allow a manager of any parent group to edit the users in any child group below the one they were the manager for. This necessitated that I be able to go all the way up the hierarchy looking at:

current.group.manager

current.group.parent.manager

current.group.parent.parent.manager

current.group.parent.parent.parent.manager

current.group.parent.parent.parent.parent.manager …etc.

There were many ways I could have tackled that, but the most efficient way to me wound up being a loop that uses eval() with a String in a loop to keep adding ".parent" to itself and sticking it in between "current.group" and ".manager" to keep checking. I hate using eval(), but I kept it to only being used to check if something is true or not.

My question is: is this bad? Is there a better way that I'm missing? When is eval() not evil? Here's a sample of part of the solution (Check here for my full post with all the code😞

// sys_user_grmember Write and Delete ACLs 
var answer = false; //Restrict access by default
if (gs.hasRole('user_admin')) {
   answer = true; //Allow access if user has 'user_admin'
} else {
   var parentsStr = "";
   var hasParent = false;
   do {
   if (eval("current.group" + parentsStr + ".manager") == gs.getUserID()) {
   answer = true; //Allow access if the user is manager of the group or one of group's parents
   }
   parentsStr = parentsStr + ".parent";
   hasParent = JSUtil.notNil(eval("current.group" + parentsStr));
   }
   while (hasParent == true && answer == false);
}

[marcguy]

I believe eval can be evil when it's allowed to run client side (because something could be injected or tampered with), but server side is not too much of a problem (because the code can't be tampered with as easily by end user), that's what I understand anyway.

[andrew_venables · Accepted solution]

eval can be very evil in traditional web development but since the author of scripts in ServiceNow has access to write background code anyway i thinks its risk is limited.

There are a few situations where eval is necessary - but this isn't one of them. Try the below script instead (untested):

var answer = false;

if (gs.hasRole('user_admin')) {

         answer = true;

} else {

         var grp = current.group;

         do {

                   if (grp.manager == gs.getUserID()) {

                             answer = true;

                   }

                   grp = grp.parent;

         } while (!grp.nil());

}

[Shawn Dowler]

I didn't even think of it like that. Wow, talk about overcomplicating things. This will come in handy elsewhere, I'm sure. Here is what the tested version looks like:

var answer = false; //Restrict access by default

if (gs.hasRole('user_admin')) {

       answer = true; //Allow access if user has 'user_admin'

} else {

       var grp = current.group;

       do {

               if (grp.manager == gs.getUserID()) {

                       answer = true; //Allow access if the user is manager of the group or one of group's parents

               }

               grp = grp.parent;

       }

       while (!grp.nil() && answer == false);

}

[justin_drysdale]

Bonus points for the do-while loop. It is not used enough IMO.