Uh oh!
There was an error while loading. Please reload this page.
Improve permission checking in DemographicsSources - #26
Conversation
labkey-jeckels
left a comment
There was a problem hiding this comment.
Thanks. This looks like it should cover the scenario. It might still be worth adding debug() calls when permission checks cause the code to bail out for opt-in troubleshooting in the future.
The scenario of concern is when tables using these customizers are referenced through a linked schema. The User object that's passed in now only has access to the linked schema's source container. Hence the inability to reference other containers referenced by the DemographicsSource's config.
| if (!target.hasPermission(u, ReadPermission.class)) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
This should be the key check to include.
There was a problem hiding this comment.
Sure, though technically there could be a study-level permission difference, right? On the whole, it seems like the customizer code needs to be more precise with permission checks (which had probably never in real-world use mattered before), so I tried to include checks at all levels, since in theory a situation could arise where the schema/table differ from the container itself.
There was a problem hiding this comment.
Indeed, there are other possible ways this could fail. In practice given the new linked schema permission handling and given the current logging on PRIMe, it's most likely to encounter a rejection at the container level.
bbimber
commented
May 26, 2022
@labkey-jeckels Is this an OK branch to target with this PR? do you want one of the release branches so ONPRC/PRIMe will pick it up sooner? |
labkey-jeckels
commented
May 26, 2022
It's fine to merge this as-is. Once it's finalized either you can run your scripts to merge into the LK fork or I can grab this specific commit as a separate PR. |
bbimber
commented
May 26, 2022
OK, is develop ok (please approve these together)? LabKey#126 |
labkey-jeckels
commented
May 26, 2022
Let's merge to release22.3-SNAPSHOT so we can quiet down that error logging on PRIMe. As you may know, they upgraded to 22.3 a couple of weeks ago. Is it easier to adjust the target of these PRs or create new ones? |
bbimber
commented
May 26, 2022
Sure. it is easier to merge those 3 to develop (since there's a lot of unrelated changes across the 3 repos), and do a targeted merge of LabDevKitModules into release-22.3-snapshot? There isnt a real reason to push the other two repos into release-22.3-snapshot. |
labkey-jeckels
commented
May 26, 2022
Sure, that sounds like a fine approach. I'll go ahead and approve |
@labkey-jeckels This is related to #124 and ticket 45210