OBPIH-3983 Manager cannot delete a pending shipment

[kchelstowski]

What needs to be discussed and I am not sure how to do it is how to distinguish outbound sm (Create outbound movement) from stock request (Create Stock Request) on the backend side, because it uses the same def remove and in the rules for now I followed what was supposed to be done for regular shipments so:

[controller: 'stockMovement', actions: ['remove'], accessRules: [ minimumRequiredRole: RoleType.ROLE_ASSISTANT ]]

but the problem is that we don't want to change the behavior in requests, so it should still remain > Admin.
I've hidden Delete button on the frontend side checking if requsition.sourceType == ELECTRONIC:

stockMovement?.requisition?.sourceType == RequisitionSourceType.ELECTRONIC

, so if you are not > Admin you won't be able to see the button to delete, but I assume we would like to protect it also on the backend side for some circumstances (for example we change a user's role from Admin to Manager while he is on the request's view page and he does see the button (because he rendered it when he had Admin), but he shouldn't be already able to delete). I know it's not high probable to happen, so we need to discuss whether there is some way to distinguish outbound sm and requests in the security rules.
My only idea was to make some additional check in def remove in StockMovementController to check

if (stockMovement?.requisition?.sourceType == RequisitionSourceType.ELECTRONIC)

(if it's a request) and if so, then check the user role, but isn't it an ugly idea?

As for the redirect stuff - it was discussed with Katarzyna, that it is rather not expected to be redirected to orders' list page when the location has enabled purchasing, so I fixed it on the fly.

cc @awalkowiak @jmiranda

[kchelstowski]

I added a commit with the thing that was discussed during tech huddle - I changed some minor things, for example here:

// figure out whether the URL points to stock request
String [] urlParts = parseURL(request.requestURL)
boolean isRequestedUrlForStockRequest = urlParts[0] == "stockRequest" 
if (request.getAttribute("expectedSourceType") == RequisitionSourceType.ELECTRONIC && !isRequestedUrlForStockRequest {
    throw IllegalAccessException("You are trying to do something malicious, please stop")
}

in my opinion (obviously I might be wrong) it wasn't good approach in 100%, because if it was supposed to prevent user from pasting the url and trying to delete stockRequest using stockMovement in the url (/stockMovement/remove/$id), it wouldn't, because here:

"/stockRequest/$action/$id**?" {    
    controller = "stockMovement"
    isStockRequest = true
    expectedSourceType = RequisitionSourceType.ELECTRONIC
}

anytime user would use /stockMovement url, he wouldn't get the expectedSourceType attribute so the if would return false, because expectedSourceType wouldn't be == RequisitionSourceType.ELECTRONIC .

I also assumed that the user could try to delete the stockMovement using stockRequest url (it wouldn't actually break anything at this point, because permissions to remove the stockMovement using stockRequest are higher (>= Admin), so if the user is allowed to delete stockRequest, he/she is also allowed to delete stockMovement (because to remove stockMovement you have to be (>= Assistant), but it might change in the future, so to avoid it, I changed the if to this:

 StockMovement stockMovement = stockMovementService.getStockMovement(params.id)
        boolean isRequestedUrlForStockRequest = params.isStockRequest ?: false

        // Check if URL is /stockRequest and if the stockMovement we are trying to delete is a request
        // OR check if url is /stockMovement and the stockMovement we are trying to delete is not request to prevent user from trying to delete request using /stockMovement URL
        if ((params.expectedSourceType == RequisitionSourceType.ELECTRONIC && (!isRequestedUrlForStockRequest || !stockMovement.isElectronicType())) ||
            params.expectedSourceType != RequisitionSourceType.ELECTRONIC && !isRequestedUrlForStockRequest && stockMovement.isElectronicType()) {
                throw new IllegalAccessException("You are trying to do something malicious, please stop")
        }

I assumed that the user could also try to delete stockMovement using stockRequest url and add the params (expectedSourceType and isStockRequest) by hand and "pretend" that the thing the user is deleting is stockRequest, so to prevent this from happening I added a check if the thing we are actually deleting is the stockRequest or not:

!stockMovement.isElectronicType()

so by this we are pretending any of those scenarios:

• user wants to delete stockMovement using stockRequest url (for example typing url by hand in the browser (/stockRequest/remove/$id) even if he passes required params (expectedSourceType and isStockRequest) to url
• user wants to delete stockRequest using stockMovement url
• checked both for user with permission >= admin and < admin

As you proposed @jmiranda I made a generic findAccessRules method to ConfigHelper, but had (by "had" I mean that I didn't have any other idea) to add one more argument to it (rules) where we pass current rbac.rules. I needed to do it to make tests for this method, because I didn't have an idea how to mock current rbac.rules to check some different scenarios (if I hadn't done it, in all tests I would have to use the same rbac.rules (because they don't change in Config.groovy), but I don't think it's a big "issue", is it?

Let me know what you think guys @jmiranda @awalkowiak about this stuff, I'm mostly curious about my approach to test the findAccessRules method, as those were first tests I wrote in this project

[jmiranda]

Sorry I think I've added enough for now. If you need some more help with this let me know and I'll try to take a swipe at it.

[jmiranda]

This seems like a bad idea since the roles are configurable. We probably want to use the configured role, right? I guess it doesn't matter since we're just displaying the delete action and the RoleFilters will determine whether the stock movement can actually be removed by the user. In that case it would be better to just specify RoleType.ROLE_ASSISTANT since the higher roles will be included.

[jmiranda]

Honestly, the easiest solution would either to always show the link (which is my preferred approach) or show for users with at least RoleType.ROLE_ASSISTANT.

The better solution would be to override the LinkTagLib and use the ConfigHelper.findAccessRule() to determine whether the link is disabled or not.

[kchelstowski]

yep, you're right, I had blind guess that it would be enough to write just

<g:isUserInRole roles="[RoleType.ROLE_ASSISTANT]">

or create a new <g:isUserAssistant>, but I kept all roles thinking, that maybe the order might change in the future.
I have mixed feelings if the button should be visible for all, even if they don't have permissions - I mean, I followed the logic which was implemented in many places where we don't see something if we don't have enough permissions.
I wasn't feeling good with the fact, that when you don't have permission (blocked by "new" logic in RoleFilters) and you click the button, you are just redirected to blank page and you don't know what is going on (none message displays for the user), so as you probably say, it would be the best to modify LinkTagLib, so after clicking the button I'd have "Access Denied" (the button would be disabled), but I'll need to figure this out.

[jmiranda]

I have mixed feelings if the button should be visible for all

I've debated this with Kelsey a bunch and I think you're both probably right in this case. For users that will never have access to this button we should probably hide it. If the button is visible under certain situations (i.e. status changed) then it should probably be displayed but disabled with a message popup for why the user can't click the button.

So if we agree that the button needs to be hidden we can accomplish that either in the LinkTagLib itself or in a new wrapper taglib in the AuthTagLib. You could actually even keep using g:isUserInRole, but don't provide any roles just the controller and action that the . In the taglib you'd retrieve role(s) from the ConfigHelper.

<g:isUserInRole controller="${controllerName}" action="${actionName}">

or a new taglib if you don't want to mess around with (read: possibly break) the existing taglib that is used everywhere

<g:isUserInMinimumRequiredRole controller="${controllerName}" action="${actionName}">

If there's some way to do this without passing the controller and action explicitly (i.e. do these get passed as attributes implicitly)?

def isUserInRole = { attrs, body ->
    if (session.user) {
        // Ask the config helper to return the default role(s). In most cases it'll be a 
        def defaultRoles = ConfigHelper.findDefaultRoles(attrs.controller, attrs.params)
        def requiredRoles = attrs.roles ?: defaultRoles
        def isUserInRole = userService.isUserInRole(session?.user?.id, requiredRoles)
        if (isUserInRole)
            out << body()
    }
}

Sources for UX discussion
https://ux.stackexchange.com/questions/107513/what-is-better-for-ux-hiding-or-disabling-irrelevant-buttons
https://ux.stackexchange.com/questions/24386/inactive-state-of-buttons-to-hide-or-not-to-hide-that-is-the-question
https://blogs.sap.com/2019/11/13/do-disabled-buttons-need-to-be-hidden/

[jmiranda]

Sorry I didn't mean for you to use this verbatim. I was just showing you how to include parameters in the UrlMapping.

anytime user would use /stockMovement url, he wouldn't get the expectedSourceType attribute so the if would return false, because expectedSourceType wouldn't be == RequisitionSourceType.ELECTRONIC

Sorry for the confusion. I was not trying to write the code for you. I was just trying to paint broad strokes to help you head in the right direction. The idea is that we could set up our own UrlMappings, pass in parameters, and do the auth logic in the controller. Whether you can make that work is up to you.

I think you're probably correct so this might need some rethinking. Or you might be able to pass expectedSourceType = RequisitionSourceType.PAPER in the stockMovement UrlMappings and maybe that solves the problem. But you'll need to think through that to see if it would actually work.

[jmiranda]

I assumed that the user could also try to delete stockMovement using stockRequest url and add the params (expectedSourceType and isStockRequest) by hand and "pretend" that the thing the user is deleting is stockRequest,

This is a great point.

[kchelstowski]

I don't think adding expectedSourceType == RequisitionSourceType.PAPER in the stockMovement URL would help, because again, we might come to situation when user tries to delete stockRequest using stockMovement's URL and just passes expectedSourceType "by hand", so I don't know what would be the best solution there.
With that said I realized that my approach also wasn't the best, because without checking actual URL I also allowed user to delete stockRequest using stockMovement URL - the user would just need to pass isStockRequest and expectedSourceType and it would work (I think nothing wrong would actually happen, but the user should get the message, that he is trying to do something bad).

[jmiranda]

we might come to situation when user tries to delete stockRequest using stockMovement's URL and just passes expectedSourceType "by hand"

Correct, but there's a possibility that hardcoding the parameter in the UrlMapping would override the value passed in the request parameters. We should test that out.

With that said I realized that my approach also wasn't the best,

Correct. We need to inspect the URL. Which is why I want to push this logic into the filters (it doesn't really belong up at the controller level).

[jmiranda]

I think I wanted this to be based off the URL.

[jmiranda]

String [] urlParts = parseURL(request.requestURL)
boolean isRequestedUrlForStockRequest = urlParts[0] == "stockRequest" 

This is important because unlike parameters, the request URL can't be tampered with. The user can change the URl in the browser from /openboxes/stockRequest/remove/:id to /openboxes/stockMovement/remove/:id but the system will delegate that to the right controller.

As you correctly pointed out the user can modify isStockRequest parameter of the URL, so they could do something like /openboxes/stockMovement/remove/:id?isStockRequest=true and we'd be taking their word for it. Whereas if we base it off the URL then we can be confident that the value has not been tampered with.

[jmiranda]

Also, if we decide to go with a separate StockRequestController then you can just use params.controller.

[kchelstowski]

Ok, I will change the logic to check the url without those unnecessary params at this point

[jmiranda]

This logic is getting complicated enough that it probably needs its own method on StockMovement or perhaps in the Controller. But honestly, now that I'm seeing this, it's probably time to move the logic to a more centralized place like the RoleFilters.groovy as explained in this comment.

And one more note. We could potentially move the authorization logic to one of the filters (SecurityFilters, RoleFilters) if it makes more sense. That way we can include the code in one place rather than throughout the controller code.

For example, someone might say “hey i don’t want browsers or managers to view my stock requests” so we’d have to add the same logic to the stockMovement:show action.

[jmiranda]

Again, I don't mean for you to copy-pasta the pseudocode I'm writing in the ticket. This is just demonstrating the general approach I would take. The exception type and message should be something you should figure out on your own or ask Kelsey for help.

[jmiranda]

Again exception type and message should not be what I wrote in the pseudocode. At the very least, "Admin" needs to be substituted for the minimum required role. But I'd like the general message to follow whatever convention we have for access exceptions.

[jmiranda]

I think we could (should) pull the rules in via the ConfigurationHolder.

List rules = ConfigurationHolder.config.openboxes.security.rbac.rules?:[]

Can't remember if the rules are a list or a map so I'll leave that for you to figure out.

[jmiranda]

Oh I remember why you said you needed to do this ... tests. Even unit tests can mock configuration properties.

class UserTests extends GrailsUnitTestCase {
    @Before
    void setUp() {
        super.setUp()
        mockConfig("openboxes.anonymize.enabled = false")
    }

[kchelstowski]

thanks

[jmiranda]

This is a good thought, but this could lead to some bad things and might happen frequently. Perhaps instead of an exception we should talk about the precedence rules. What wins by default when there's a conflict like this? My hunch is that the more specific rule should win.

[jmiranda]

I think the easiest way to do this would be to use cascading logic

Map rule = rules.find {
    if (it.controller == controllerName && it.actions.contains(actionName)) return it
    else if (it.controller == controllerName && it.actions.contains("*")) return it
    else if (it.controller == "*" && it.actions.contains("*")) return it
    else return null
}

That way you don't have to go back through the list of rules to figure out which one wins using precedence rules.

[jmiranda]

And one thing to note ... your idea about throwing an exception when there are multiple rules makes more sense if there were more than one rule for the matching criteria.

For example, if we had two rules for "stockRequest:remove" that had different minimum required roles (one was superuser, the other was admin), then we could potentially throw the exception. However, I think it would be better to choose the higher role in this case to avoid the exception.

Exceptions would be bad because the user didn't do anything wrong so we shouldn't punish them for a sysadmin error. We should just try to limit the damage by taking the most conservative approach i.e. use superuser instead of admin.

[kchelstowski]

yeah, I just thought that it didn't have sense to have more than one rule specified for a controller and an action, but you are completely right that it is not user's fault.
I'm wondering how to determine which rule wins, because does this:

Map rule = rules.find {
    if (it.controller == controllerName && it.actions.contains(actionName)) return it
    else if (it.controller == controllerName && it.actions.contains("*")) return it
    else if (it.controller == "*" && it.actions.contains("*")) return it
    else return null
}

guarantee you that it would find the rule with Superuser instead of Manager if we have something like this (in this order)?

 [controller: 'order', actions: ['remove'], accessRules: [ minimumRequiredRole: RoleType.ROLE_MANAGER ]],
 [controller: 'order', actions: ['remove'], accessRules: [ minimumRequiredRole: RoleType.ROLE_SUPERUSER ]]

[jmiranda]

This is awesome that you added tests for this. We're going to want to write the tests in Spock if possible. I'm not sure how to

With that said, you can add configuration properties to a Junit test like this

    @Before
    void setUp() {
        super.setUp()
        mockConfig("openboxes.anonymize.enabled = false")
    }

Might be more complicated for this but I think we can just use Groovy DSL. The following might not be correct so you might have to massage it to get it to work.

String rbacRulesConfig = """
openboxes { 
    security {
        rbac { 
            rules {
                controller = "order"
                action = "remove"
                accessRules: [ minimumRequiredRole: RoleType.ROLE_MANAGER ]
            }
        }    
    }    
}
"""
mockConfig(rbacRulesConfig)

The other option would just be to copy the code

String rbacRulesConfig = """
[
  [controller: 'order', actions: ['remove'], accessRules: [ minimumRequiredRole: RoleType.ROLE_MANAGER ]],
  [controller: 'order', actions: ['removeOrderItem'], accessRules: [ minimumRequiredRole: RoleType.ROLE_MANAGER ]],
]

[kchelstowski]

@jmiranda I've added my fixes. Like before I will add my comments to what I've done later

[kchelstowski]

@jmiranda comments as promised,
so starting from the top:

I removed those params from the UrlMapping and am looking on URL now:

String[] urlParts = request.request.requestURI.split("/")
boolean isRequestedUrlForStockRequest = urlParts[2] == "stockRequest"

having done that, the if below looks now a bit less complexed:

// Check if URL is /stockRequest and if the stockMovement we are trying to delete is a request
 // OR check if url is /stockMovement and the stockMovement we are trying to delete is not request to prevent user from trying to delete request using /stockMovement URL
if ((isRequestedUrlForStockRequest && !stockMovement.electronicType) ||
            (!isRequestedUrlForStockRequest && stockMovement.electronicType)) {
                throw new IllegalAccessException("You can't delete the stock movement: ${stockMovement.name} using this URL")
}

as you can see I also changed the message in exception (you said that I should follow the convention when throwing IllegalAccessException, but there isn't any convention, I only found two usages of this exception and the messages look like this:
Unable to locate domain ${className}
and
No domain class ${className} could be found

so let me know if you are fine with the message I figured out or I should change/ask Kelsey for help with that unless you have an idea?

I changed LinkTagLib as I was supposed to bring back the visibility of the button, so inside LinkTagLib I determine whether the button should be disabled or not doing this (I wasn't sure whether I could delete those: missManager, missAdmin etc. at this point without breaking anything, so I decided to keep it for now as it doesn't do anything "bad" here.

 def rule = ConfigHelper.findAccessRule(controllerName as String, actionName as String)
 def missRule = rule ? !userService.isUserInRole(session.user, rule?.accessRules?.minimumRequiredRole) : false

I added isElectronicType to transients:

static transients = [
            "electronicType"
    ]

I know you said that I should make the method getIsElectronicType and in transients do isElectronicType, but I saw that the same thing had been done in OutboundStockMovement and OutboundStockMovementItem with electronicType as transient, so I wanted to follow already existing convention.

I also changed the message which you didn't like (in second exception) from:

 throw new IllegalAccessException("User must be in role Admin to delete stock request")

to:

throw new IllegalAccessException("You don't have minimum required role to perform this action")

Now, talking about findAccessRule method, you proposed using find instead of findAll as I did and get the rule, where the role is "more specified", but according to my comment which I made above:

I'm wondering how to determine which rule wins, because does this:

Map rule = rules.find {
    if (it.controller == controllerName && it.actions.contains(actionName)) return it
    else if (it.controller == controllerName && it.actions.contains("*")) return it
    else if (it.controller == "*" && it.actions.contains("*")) return it
    else return null
 }

guarantee you that it would find the rule with Superuser instead of Manager if we have something like this (in this order)?

[controller: 'order', actions: ['remove'], accessRules: [ minimumRequiredRole: RoleType.ROLE_MANAGER ]],
[controller: 'order', actions: ['remove'], accessRules: [ minimumRequiredRole: RoleType.ROLE_SUPERUSER ]]

and I need to say, that my worries were correct - if we have the rules specified like above, the find method finds the first rule, so in this example it would find the one with Manager as minimumRequiredRole.
I figured out the solution by keeping findAll and just sorting the result by the sortOrder of role enum. By doing it, according to the example from above, the rule with the Superuser moves to the top (it's the first element of List), as Superuser has the least sortOrder in enum.

 ArrayList rule = rules.findAll {
            (it.controller == controllerName && it.actions.contains(actionName)) ||
            (it.controller == controllerName && it.actions.contains("*")) ||
            (it.controller == "*" && it.actions.contains("*"))
 }.sort { it?.accessRules?.minimumRequiredRole?.sortOrder }

 return rule?.size() > 0 ? rule?.first() : null

I also remove argument rules from findAccessRule as you told me how to mock rules in tests, so the tests are also changed and I made the mockConfig working. Thanks for that tip.

@jmiranda That would be all from me, looking forward to know what you think about those changes and if there is anything more I could fix (I know you were talking about moving some logic to Filters, but I didn't have an idea how you would see that, so for now I kept it as it is, if you decide I have to do it, then I would be grateful for the tips).

cc @awalkowiak