Restore ability of non-admins to delete workbooks

[bbimber]

@jryurkanin and/or @labkey-jeckels: I dont have a good understanding of when this behavior changed, and I suppose it is possible this never fully worked as intended for non-admins. However, this PR is a small change designed to ensure that non-admin users have the ability to delete workbook containers.

As background: workbooks are technically a labkey container, but function slightly differently. notably, users with delete permission in the parent container used to be able to delete them. You can see this in the code by the fact that Container.getPermissionNeededToDelete() exists, and WorkbookContainerType overrides this to only require DeletePermission.

I just has a user report this, but apparently non-admin users cannot delete workbooks anymore. I tracked this down in the code to the one line I'm changing in this PR.

The problem is that ContainerManager.getAllChildren() also returns the container itself (a workbook here). Even though WorkbookContainerType.getPermissionNeededToDelete() is DeletePermission, the result of ContainerManager.hasTreePermission(target, getUser(), AdminPermission.class) is to require the user to also have AdminPermission on the workbook itself.

If the change I propose here is acceptable, I'd like to also write an integration test to codify this behavior (which i can do in this PR). I thought it was worth feeling out the change first though. Let me know what you think.

[labkey-jeckels]

I didn't try testing against old versions but I didn't find anything that looked like it would have introduced a change anytime recently.

Your change looks fine. I'll hold PR approval for the automated test coverage.

[bbimber]

Hi @labkey-jeckels: I just added an integration test, but this exposed the need for a couple related changes, summarized below:

1) AbstractQueryUpdateService.java: truncate rows requires AdminPermission. Workbooks are kind of a weird case here, but I think my proposed change (deferring to container.getPermissionNeededToDelete()) is appropriate here.

2) TriggerConfigurationsTable.java has some weird preexisting code. It overrides hasPermission() to hard-code AdminPermission for everything. This took me a little bit to track down and understand, since the existing code ends up throwing a misleading "The user does not have read permission for table X" error, when in fact the user does have read permission. This hasPermission() override code completely ignores the permission being requested and always tests AdminPermission. Anyway, I am less sure on my proposal here because I dont exactly understand why that code exists, but I am proposing to also change this to Container.getPermissionNeededToDelete(), which will be AdminPermission for normal container types and therefore unchanged by this PR.

[labkey-jeckels]

@bbimber will the new test fail without those changes? I'm curious about the codepath that is triggering the problem. I can give it a try locally but wanted to confirm the repro scenario first.

[bbimber]

@labkey-jeckels: This code only got on my radar specifically because is was triggered by this test. I can look at it in more detail if you want, but my guess is that on any container delete certain tables are truncated (perhaps container scoped domains or something?). In any event, DeleteFolderAction definitely triggered the pathways in question here. It is possible I could figure out some other resolution that is higher upstream of what I'm touching here, though nothing was immediately obvious the first time through.

[labkey-jeckels]

@labkey-jeckels: This code only got on my radar specifically because is was triggered by this test. I can look at it in more detail if you want, but my guess is that on any container delete certain tables are truncated (perhaps container scoped domains or something?). In any event, DeleteFolderAction definitely triggered the pathways in question here. It is possible I could figure out some other resolution that is higher upstream of what I'm touching here, though nothing was immediately obvious the first time through.

Thanks. I repro'd locally and have a better handle on what's happening now. Your change to AbstractQueryUpdateService would probably be fine but I opted for an approach that makes TriggerConfigurationsTable slightly more normal. Unless you object, I'll verify it passes on TeamCity too and then approve/merge.

[bbimber]

hi @labkey-jeckels: no, i have no opinions whatsoever on how to implement whatever the pipeline trigger code is trying to do. if you think your approach fits it better, fine with me.