Multiple permission checks in web app automated tests

Question

I am writing automated tests for a web app and at one point I need to make sure that a user can only engage in activities allowed by a set permission. I have written all the standalone methods that perform the required tests and now I need to group all of them together and test. I came up with something that looks like this:

public boolean hasPermissions(String permissions, File file) {

    if ("Read Only".equals(permissions)) {

        return !canCreateNewFolder() && !canAddFiles() && !canAddLink() && canCopyMultipleItems() && !canMoveMultipleItems()
                && !canDeleteMultipleItems() && canDownloadMultipleItems() && canDownload(file) && canViewDetails(file)
                && !canEdit(file) && canCopy(file) && !canMove(file) && !canHide(file) && !canDelete(file);

    } else if ("Read and Add Only".equals(permissions)) {

        return canCreateNewFolder() && canAddFiles() && canAddLink() && canCopyMultipleItems() && !canMoveMultipleItems()
                && !canDeleteMultipleItems() && canDownloadMultipleItems() && canDownload(file) && canViewDetails(file)
                && !canEdit(file) && canCopy(file) && !canMove(file) && !canHide(file) && !canDelete(file);

    } else if ("Full Access".equals(permissions)) {

        return canCreateNewFolder() && canAddFiles() && canAddLink() && canCopyMultipleItems() && canMoveMultipleItems()
                && canDeleteMultipleItems() && canDownloadMultipleItems() && canDownload(file) && canViewDetails(file) && canEdit(file)
                && canCopy(file) && canMove(file) && canHide(file) && canDelete(file);

    } else {
        throw new UnsupportedOperationException("Unsupposrted permission");
    }

}

While this solution works with no issues, it somehow doesn't look right.

So just wanted to ask for another opinion on this design.

Answer 1

So, thing is, it's GREAT that you made the individual tests. Imagine this method with all those tests inlined. That'd be... really bad. So that part is good. But like you say, something is off.

Now, one way you could look at this is that you don't really need to test everything. That depends on what an implementation means, but someone has read rights if they can read a file. Whether or not they can write... it doesn't matter. But, that depends on your business requirements, so such a simplification may not apply.

Another thing you could do is apply semantic grouping. Combine checks that are in the same category.

There seems to be 3 groups:

Read access, which is the ability to read any file (canCopyMultipleItems, canDownloadMultipleItems, canDownload, canViewDetails, canCopy).

Create+Read access, which is read + append to directory. (canCreateNewFolder, canAddLink)

Write access, which is read + create + write. (canMoveMultipleItems, canDeleteMultipleItems, canEdit, canMove, canHide, canDelete).

If you are only interested in verifying that someone has "at least" these rights, this is easy, you can make three groups of checks (each of them including the previous) and return the appropriate value.

If, however, you're only allowed to have the exact matching set of rights, then the check becomes harder, I'd dare say that what you have right now is the fastest code you could have; any faster is going to be faster because of a redesign.

From a readability perspective, all I'd do is this:

Wrap each grouping we just mentioned in two separate functions:

hasAny<group>Right and hasAll<group>Rights.

Then you can use them like so:

public boolean hasPermissions(String permissions, File file) {

    if ("Read Only".equals(permissions)) {
        return hasAllReadRights(file) && !hasAnyCreateRight(file) && !hasAnyWriteRight(file);
    } else if ("Read and Add Only".equals(permissions)) {
        return hasAllReadRights(file) && hasAllCreateRights(file) && !hasAnyWriteRight(file);
    } else if ("Full Access".equals(permissions)) {
        return hasAllReadRights(file) && hasAllCreateRights(file) && hasAllWriteRights(file);
    } else {
        throw new UnsupportedOperationException("Unsupposrted permission");
    }
}

Perhaps even name "create" "add" instead.

---

That all said, you... clearly have a design issue going on. A string goes in, and based on that you're doing checks? Huh?

First, stringly typed code should be replaced with Enums. Second, having to check every single time if the user has a set of general rights is ... weird. These should be migrated to a different place; then all you're interested in is if the user has generic read rights, + the few things that are file specific. If instead you're relying on class variables in these "generic" checks (like canDownloadMultipleItems), then... well... then I still think it's worth splitting these checks up into "generic" and "file-specific" checks; if you're ever going to be looping over a directory, you'll want to be checking the generic parts only once, and not all the time.

---

Also, there's a typo in your exception. "Unsupposrted permission" -> "Unsupported permission"