shouldn't the currentLocation also be a dependency?

yes, true.
And currentUser dependancy should be [currentUser.id]

exactly :D and the currentLocation -> currentLocation?.id

@awalkowiak can approve this if he decides, but there's some tech debt here that we need to deal with in the future.

• Resolve all roles / permissions on the server-side and use the app context as the vehicle to deliver them. That means, if you're writing auth code on the client we need to refactor.
• Hard-coded roles should not exist anywhere in the application, but definitely not on the client-side
  • Use configuration properties where possible

As I mentioned on the tech huddle, it would be better to allow the server to resolve access rules on its own. The client shouldn't really be responsible for this. It should just ask "given what you know about me, do I have access to a certain resource". That should happen on-demand, for the most part (i.e. when accessign a URL / controller+action) but in the case where we need to determine whether we display a particular thing to a user, then it probably makes sense to have a have a permission in the appContext.

In this case we really just want to check if the user has the product manager "role" (i.e. permission) within the current context (i.e. location). In my opinion, I don't think the "admin" role should have anything to do with this. Some implementation out there probably wants to be able to allow managers to also be product managers. So we need to make that a configurable property.

It'll be similar to this permission in the App Context (which should also be configurable).

boolean isUserRequestApprover = userService.isUserInAllRoles(session?.user?.id, [RoleType.ROLE_REQUISITION_APPROVER], session.warehouse?.id)

so something like this, but configurable since we don't want roles to be hard-coded

List<Role> defaultRoleTypes = grailsApplication.config.openboxes.product.manager.defaultRoleTypes ?: [RoleType.ROLE_PRODUCT_MANAGER]
boolean defaultRoleTypes = userService.isUserInAllRoles(session?.user?.id, productManagerRoles, session.warehouse?.id)

and then runtime.groovy

openboxes.product.manager.defaultRoleTypes = [RoleType.ROLE_PRODUCT_MANAGER]

If PIH's requirements specify that we need to include the "admin" role, then the configurable code above will allow PIH to make a config change in their Ansible configuration to allow this to work.

Aside: You could also move the "authorization" logic to a service to ensure there is a centralized declaration of permission logic. In the case of purchase approval we added that to the OrderService.

    boolean canApproveOrder(Order order, User userInstance) {
        if (isApprovalRequired(order)) {
            List<RoleType> defaultRoleTypes = grailsApplication.config.openboxes.purchasing.approval.defaultRoleTypes
            return userService.hasAnyRoles(userInstance, defaultRoleTypes)
        }
        return Boolean.TRUE
    }

For this case we probably want to move it to the ProductService. I would also be ok with creating a new SecurityService but we can make that change when we migrate to a security framework like Spring Security or Shiro.

I created this ticket for this one https://pihemr.atlassian.net/browse/OBPIH-6254

If you don't end up moving the code to the server-side and pulling this permission from the app context, then we need to at least change the name of the variable to canManageProducts or something like that.

Convention: Variable names should describe the what, not the how.

Should this be a replacement for the permisionUtil that we already have? Is there a case that we want to have both? I think we should have only one thing if that's possible.

Let's change it to the suggested convention canManageProducts and let's stick to that version of variable names moving forward.