If return true, else return false
if(lv > conf.roleEditor)
return true;
else
return false;
Before we delve into the contextual considerations, an if that returns true or false is always a redundant if. This can at all times be refactored to:
return lv > conf.roleEditor;
Performance
I see that, if the url is "/RoleEditor" but the power level is too low, all other comparisons will be made. A fix for this would be changing all to this:
The first question is always "does this performance need to be optimized?" As these are simple value comparisons, the performance cost is likely negligible. Assuming this method isn't being called many thousands of times in a given page load.
When performance cost is negligible, favor readability over (irrelevant) optimizations. In that regard, you're free to not avoid the additional checks if it makes the code more readable and doesn't meaningfully impact performance.
That being said, the code can be improved here, which I will get to. The main takeaway here is that you are not forced to optimize it the way you currently wanted to if there is no proven performance issue.
Authentication vs authorization
As others have already remarked, you're dealing with authorization here, not authentication.
- Authentication = Who is this? (it's Bob)
- Authorization = Can Bob delete users? (yes)
Secure authorization
You are showing a javascript method. Presumably, this runs in the client's browser. By putting the logic in the browser, your users are able to find it and alter it if they so choose. This is a bad idea for security concerns.
It's not necessarily wrong to perform these checks client-side (e.g. for a fast response and thus good user experience) if they are also backed by server-side security checks (to ensure that users fiddling with the authorization logic client-side are unable to perform actions server-side).
Optimizing the algorithm
With all these sidenotes out of the way, we move on to the actual issue you were asking about, optimizing the ifs. Reading the code, there is a general pattern to be spotted:
if(url == "MyString" && lv > conf.MyStringMinimumLevel)
Sometimes there is more than one allowed string, so we'll account for that, but the repeating pattern is otherwise always the same. I'm going to refer to these two values as "names" and "levels" in the rest of the answer for brevity's sake.
This immediately opens the door to abstraction by mapping these names and levels. In fact, you've already sort of done this with all your conf.roleEditor, conf.queuedata, ... values. You just haven't done it reusably, which is forcing you to manually write all your if checks. If you write it reusable, you can reduce this to a very simple process:
- Fetch the level based on the
url name
- Check if the given user level is greater than the fetched level
var mappings =
{
{
"level" : 1
"names" : [ "RoleEditor", "SaveRoles" ]
},
{
"level" : 2
"names" : [ "WriteQueueData" ]
}
};
function isAuthorized(url, lv) {
var mapping = mappings.find(m => m.names.includes(url.trim('/'));
return mapping !== undefined
&& lv > mapping.level;
}
To explain:
.find() gives us the first item which conforms to the selection logic (or undefined if no such item exists). - The selection logic (
.includes()) checks if a mapping contains a name that matches the url parameter.
- By using
url.trim('/'), you omit the first and last character (the /) from the URL when it is a / (but will leave any other characters). This just makes it easier for use to not have to constantly put the / in the mapping names.
- The
!== undefined check ensures that we return false if no mapping exists for the current URL. Your intended behavior may be different here - adjust it as you see fit.
You'll probably want to store this mapping in your conf object, but I'll leave the finer points up to you. This is just a basic example of how you can reduce the method.
Edit: Sidenote
If the conf object should not be altered (for whatever reason), you can work around this by indirectly using the values in the mappings:
var mappings =
{
{
"level" : conf.roleEditor,
"names" : [ "RoleEditor", "SaveRoles" ]
},
{
"level" : conf.queuedata,
"names" : [ "WriteQueueData" ]
}
};
// The function is unchanged
This is less ideal (it's better to just change the conf object) but if you can't change it, you can at least still work around it.
