16
\$\begingroup\$

I have a function that checks if a user can perform an operation based on access to a specific URL.

The function parameters are the requested URL: url, and user level: lv. The conf object has stored access levels for comparison with the users access level.

function isAuthenticated(url, lv){
    let conf = config.getConfig();

    // Role Editor
    if((url === "/RoleEditor" || url === "/SaveRoles") && lv > conf.roleEditor)
        return true;
    // Queue
    if(url === "/WriteQueueData" && lv > conf.queuedata)
        return true;
    // Listener
    if((url === "/startListener" || url === "/stopListener" || url === "restartListener" || url === "/ListenerConfig" || url === "/SaveListenerConfig") && lv > conf.listener)
        return true;
    // Client details
    if(url === "/ClientInfo" && lv > conf.clientDetails)
        return true;
    // User information
    if(url === "/UserManagement" && lv > conf.userOverview)
        return true;
    // User Edit
    if((url === "/EditUser" || url === "/EditUserToDb") && lv > conf.userEdit)
        return true;

    //
    // Another 6 of this comparisons
    //
    return false;
}

This seems inefficient. For example, if the url is "/RoleEditor" but the power level is too low, all other comparisons are still being made. This could be fixed by changing to this:

if((url === "/RoleEditor" || url === "/SaveRoles"))
    if(lv > conf.roleEditor)
        return true;
    else
        return false;

But that feels even worse (more redundant).

Is there a better way to do this?

\$\endgroup\$
  • 3
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question? for examples, and revise the title accordingly. \$\endgroup\$ – Martin R Nov 18 at 9:55
  • \$\begingroup\$ @MartinR: Quality of phrasing aside; I don't think it's wrong for OP to specify what exactly they want reviewed (in this case the redundancy). Not every review has a universally correct answer - different priorities lead to different approaches. I do agree it's not a great title and can do with a rephrasing, but it seems to stay within range of acceptable content (stating what the code is + which specific problem OP wants to tackle) \$\endgroup\$ – Flater Nov 18 at 14:37
  • 4
    \$\begingroup\$ @Flater: From How to Ask: “State what your code does in your title, not your main concerns about it.” \$\endgroup\$ – Martin R Nov 18 at 15:26
  • \$\begingroup\$ Not exactly a review of your code, but a review of your design - your model has a single authorization path, with escalating levels of access. It does not contemplate that someone might need to be able to access "ClientInfo" but not be able to start/stop listeners, or vice versa, for example. This is sufficient for a simple application, but may eventually need re-working when users can have more complex role allocations. \$\endgroup\$ – Stobor Nov 20 at 3:20
  • \$\begingroup\$ url.trim('/') does not actually trim anything besides whitespace \$\endgroup\$ – Marc Nov 20 at 13:56
25
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ For the mappings, would there be a noticeable downside to just using the roles as keys and the level as values? I.e mappings = {"RoleEditor": 1, "WriteQueueData": 2} \$\endgroup\$ – Cain Nov 18 at 17:04
  • 2
    \$\begingroup\$ Instead of url.substring(1) isn't it better to do url.trim('/')? This way, if url contains '/RoleEditor' or '/RoleEditor/', you will be sure it will match with 'RoleEditor'. \$\endgroup\$ – Ismael Miguel Nov 18 at 18:38
  • \$\begingroup\$ @Cain: It somewhat irks me to have to write multiple entries for urls with the same level; but it would work on a technical level, yes. \$\endgroup\$ – Flater Nov 18 at 22:09
  • \$\begingroup\$ @IsmaelMiguel: Yes, it does seem to be a better approach. But if you're trying to parse any valid (partial) URL, then you could also start accounting for URL encoding (and probably other things I'm forgetting right now). Definitely a nice to have, but it depends on whether you need it or not and that isn't quite clear to me based on OP's example. \$\endgroup\$ – Flater Nov 20 at 9:28
  • \$\begingroup\$ @Flater I don't see it as a "nice to have". If you notice, under the // Listener comment, the 3rd test is url === "restartListener" (the slash is missing). Now, it could be a mistake from the O.P. or intentional. Whichever it is, it fails with your code, which will try to match with "estartListener" instead. The .trim function is made to remove unnecessary characters (taken from a list or characters) from the beginning and end of the string. You method blindly drops the first character, whichever it may be. \$\endgroup\$ – Ismael Miguel Nov 20 at 9:34
12
\$\begingroup\$

Although I don't disagree with the accepted answer, I just wanted to point out another option that is more similar to the OP's original code/logic but with less redundancy.

When one variable is continuously checked, that is a perfect case for a switch statement.

function isAuthenticated(url, lv){

    let conf = config.getConfig();

    switch(url){

        // Role Editor
        case "/RoleEditor":
        case "/SaveRoles":
            return lv > conf.roleEditor;

        // Queue
        case "/WriteQueueData":
            return lv > conf.queuedata;

        // Listener
        case "/startListener":
        case "/stopListener":
        case "restartListener":
        case "/ListenerConfig":
        case "/SaveListenerConfig":
            return lv > conf.listener;

        // Client details
        case "/ClientInfo":
            return lv > conf.clientDetails;

        // User information
        case "/UserManagement":
            return lv > conf.userOverview;

        // User Edit
        case "/EditUser":
        case "/EditUserToDb":
            return lv > conf.userEdit;

    }

    return false;
}

As can be seen in the code, replacing a bunch of if expressions with a switch allows you to keep your code comments pretty much as-is, and you can mimic the || by using fall-through on a case.

Some people prefer always having default case where you return false;, others prefer returning outside the switch.

This is definitely more verbose and arguably not as elegant as other solutions, but I just thought it would be good to round out possible solutions because I think it is a perfect example of when you could use a switch statement.

\$\endgroup\$
  • 4
    \$\begingroup\$ Although logically sound, this is the start of a path that will lead to source code storing config info, which is just terrible. Nobody's going to touch a giant case structure and will simply add to it, whereas the accepted answer is literally screaming move the mapping outside the source code. \$\endgroup\$ – Nelson Nov 19 at 4:01
  • 1
    \$\begingroup\$ I believe that the case "restartListener": may have been a mistake by the O.P.. Everything else starts with /, except that one. Also, in switch(url){, just like how I've suggested before, it would be better to use switch(url.trim("/")){ to remove the slashes, making it easier to write and write. Even if the url contains e.g.: "/Admin/DeleteUser/", it will be "Admin/DeleteUser" after the trim. \$\endgroup\$ – Ismael Miguel Nov 19 at 9:45
  • 1
    \$\begingroup\$ @IsmaelMiguel: "I believe that the case "restartListener": may have been a mistake by the O.P." I think so, too, but that's actually a good point in favor of the switch statement: That typo is now much more visible (and, thus, more likely to be found and fixed) than it was before. \$\endgroup\$ – Heinzi Nov 19 at 15:18
  • 1
    \$\begingroup\$ @Nelson: I used to think so, too, but after years of maintaining software where every deployment also necessitated deploying config files which are the same in the installation, I see things differently: If it is static configuration info which is not meant to be customizable, it should be in the source code (or at least as close to the source code as possible - surely in the same source code repository). Whether it is stored as as actual programming language code or as a text file (parsed at compile time or at run time) does not matter much. \$\endgroup\$ – Heinzi Nov 19 at 15:25
  • 1
    \$\begingroup\$ Personally, I wouldn't feel like I needed to to refactor this version of the code if I saw it (like I would with the OP's code) but I also wouldn't want to refactor to it absent other considerations. \$\endgroup\$ – Bobson Nov 20 at 22:59
9
\$\begingroup\$

According to your description either of if conditional branches should return immediately if url is matched and the returned value is then designated by access level check lv > conf.<some_access_level>.

To avoid falling into a mess on 12 conditionals a more flexible and performant way is to declare a predefined mapping: "User role name" --> "Access level name":

const accessRolesMap = {
    "RoleEditor": "roleEditor",
    "SaveRoles": "roleEditor",
    "WriteQueueData": "queuedata",
    "ClientInfo": "clientDetails",
    "UserManagement": "userOverview",
    "EditUser": "userEdit",
    "EditUserToDb": "userEdit",
    "startListener": "listener",
    "stopListener": "listener",
    "restartListener": "listener",
    "ListenerConfig": "listener",
    "SaveListenerConfig": "listener",
};

function isAuthenticated(url, accessLevel) {
    let conf = config.getConfig();
    url = url.replace(/^\//, '');
    return (accessRolesMap.hasOwnProperty(url) && accessLevel > conf[accessRolesMap[url]]);
}

accessRolesMap may seem de-normalized but it outweighs that with its fast membership check.

\$\endgroup\$
  • \$\begingroup\$ In the definition of the accessRolesMap, there should be exactly one entry per line of code, to keep the definition readable. Bonus points if you align the role names vertically. \$\endgroup\$ – Roland Illig Nov 18 at 22:59
  • \$\begingroup\$ @RolandIllig, Ok, readability is important. Check my update \$\endgroup\$ – RomanPerekhrest Nov 19 at 5:57
  • \$\begingroup\$ "/RoleEditor" is now the same as "RoleEditor". Nitpicking aside, this is probably the best solution. \$\endgroup\$ – JollyJoker Nov 19 at 8:01
  • \$\begingroup\$ I have 2 nitpicks with this answer: 1- As I've explained in other answers, use url.trim("/"). In this case, if you need a deeper level in an url (using examples from other comments I gave), like in "/Admin/DeleteUser/", Yours will return "AdminDeleteUser" instead of "Admin/DeleteUser". The O.P. may like this side effect of your answer, or be surprised by it. 2- It's always a good idea to use . hasOwnProperty() instead of in. Assuming url = "toString", your code will return true, which is simply wrong. [...] \$\endgroup\$ – Ismael Miguel Nov 19 at 9:52
  • \$\begingroup\$ [...] url in accessRolesMap && accessLevel > conf[accessRolesMap[url]] with url = "toString" will be the same as "toString" in accessRolesMap && accessLevel > conf["function toString() { [native code] }"], which will return true for any accessLevel above 0. "function toString() { [native code] }" is the string representation of accessRolesMap["toString"], to be used as an index in the conf object. Using accessRolesMap.hasOwnProperty(url) will solve this issue. \$\endgroup\$ – Ismael Miguel Nov 19 at 9:55
4
\$\begingroup\$

First of all. I suppose this is about authorization, not authentication. User is authenticated if you were able to verify their identity. What you do there is to check if they have access to particular resources, or if they have right to perform certain action. This is called authorization.

For your code:

This is very bad, because brackets are missing. I wouldn't be sure whether that else belongs to the outer or the inner if.

if((url === "/RoleEditor" || url === "/SaveRoles"))
    if(lv > conf.roleEditor)
        return true;
    else
        return false;

Other than that, there is not much you can do about this except two things:

1) merge everything together, but that would decrease readability wastly:

return ((url === "/RoleEditor" || url === "/SaveRoles") && lv > conf.roleEditor)
    || (url === "/WriteQueueData" && lv > conf.queuedata)
    || ...

2) increase level of abstraction by creating an authorizator object capable of configuring the authorization rules

const Authorizator = function (config) {
  this.config = config;
  this.rules = {};
}
Authorizator.prototype = {
  addRule : function (urls, check) {
    if (!urls instanceof Array) {
      urls = [urls];
    }
    for (let i=0; i<urls.length; ++i) {
      this.rules[url] = check;
    }
  },

  isAuthorized: function (url, lv) {
    return this.rules[url] && this.rules[url](this.config, lv);
  }
};
let auth = new Authorizator(config);
auth.addRule(["/RoleEditor", "/SaveRoles"], (lv, conf) => lv > conf.roleEditor);
auth.addRule("/WriteQueueData", (lv, conf) => conf.queuedata);
// ...

const authorized = auth.isAuthorized(url, lv);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.