How to check for all privileges with hasPrivilege tag

Hey there :slight_smile:

I am currently struggling with privilege checks in the radiology module. As far as I know (feel free to correct me) there are 2 tags, which are able to check privileges on JSP level:

  1. RequireTag openmrs:require --> this throws an error if a privilege is not set

  2. PrivilegeTag openmrs:hasPrivilege --> this just “hides” the fields it contains

require provides the “allPrivileges” section, but hasPrivilege has no option for this. I was wondering if there is another way to hide code if one or more of several required privileges are missing. The following is ugly&no option for me:

<openmrs:hasPrivilege privilege="Privilege 1">
<openmrs:hasPrivilege privilege="Privilege 2">
Sample Code

I would volunteer to make a PR to add the “allPrivileges” functionality to the hasPrivilegeTag

PrivilegeTag already supports a list of comma-separated privileges, but the comma is an OR not an AND. How are you going to indicate this without breaking compatibility ?

What about adding a boolean attribute like “require all” which is false by default (behavior equivalent to now) and if true all privileges in the comma separated list are required?

It’s a feasible option. Actually the “inverse” attribute means require none (all must be false). So the inverse of require all would be at least one false :slight_smile:

As @lluismf mentioned, the privilege attribute of the hasPrivilege tag can be a comma delimited list of several privileges that behaves just like RequireTag.allPrivileges

@lluismf pointed out that the privilege attribute “supports a list of comma-separated privileges, but the comma is an OR not an AND” so you just need to have one of the privileges in the list for it to be true, thus it does not behave like RequireTag.allPrivileges. it should rather be called hasAnyPrivilege

Changing the attribute name will break the Jsp using the tag unless they are refactored. Related to this, how will the new angular UI deal with this (conditional rendering of dom elements based on privileges) ? Server side rendering in this case seems easier.

opened a feature request am happy to work on it if you think its ok :slight_smile: PS: to convince you has no test, this new feature will come with a great set of tests :wink:

@bgevam & @teleivo, this sounds like a great addition. I commented on the ticket that the attribute name “hasAll” is probably better than “requireAll” (and to make sure we support false values – e.g., “false”, “0”, “no” – for using the default behavior (to support, for example, someone conditionally setting this attribute).

@burke am on it :slight_smile: also commented back. attributes that are considered boolean are never checked against “no”, “0”. so I suggest we dont start now, yes?