Improve scoping checks for objects#7739
Conversation
|
WARNING: This PR appears to have the default title generated by GitHub. Please use something more descriptive. |
| if (data == null) | ||
| throw new NotFoundException("Error: Data object not found for the given LSID: " + lsid); | ||
| // The LSID could be from another container. If so, redirect there | ||
| ensureCorrectContainer(getContainer(), data, getViewContext()); |
There was a problem hiding this comment.
so if I understand ensureCorrectContainer correctly, it will redirect when a requested object is from a different container (and check that the user has read perm in that container). shouldn't some of the cases return "not found" in that case instead? for example, this usage of getting an attachment for a dataclass seems a bit odd to me that it redirects instead of telling the user the object wasn't found in the request container.
There was a problem hiding this comment.
Yes, it's been that way for a while (aside from the next check to make sure that the user can see that container). It was originally because we weren't always great at using the right container in the URL when you have a grid of rows that span multiple containers via a non-default ContainerFilter.
We could choose to be stricter, but this is a way to continue to support sloppy links while fixing the real bug.
|
|
||
| Container source = issue.getContainerFromId(); | ||
| // Moving an issue removes it from its source folder, so require Admin there. | ||
| if (source == null || !source.hasPermission(getUser(), AdminPermission.class)) |
There was a problem hiding this comment.
should this also be checking that the issue's source container is the same as the action/request container? I'm not seeing that check anywhere (but I'm not sure if this action is intended to support cross-container moves of issues).
There was a problem hiding this comment.
It could be that picky, but this approach supports a grid with a non-default ContainerFilter so it seems like most backwards compatible approach.
| assertNull("No study should have been created in the destination", StudyManager.getInstance().getStudy(dest)); | ||
| assertNotNull("The source study must be untouched", StudyManager.getInstance().getStudy(source)); | ||
|
|
||
| // Positive scenario covered by StudyPublishTest |
There was a problem hiding this comment.
is the "destination folder doesn't exist so check admin on the destination parent" check also new here? if so, should that have a test case?
| if (errors.hasErrors()) | ||
| return false; | ||
|
|
||
| if (cSrc == null || !cSrc.hasPermission(getUser(), AdminPermission.class)) |
There was a problem hiding this comment.
I like that the comment "//user must have admin perms on both source and destination container" was already included above
| return false; | ||
|
|
||
| if (cSrc == null || !cSrc.hasPermission(getUser(), AdminPermission.class)) | ||
| throw new NotFoundException("No source container found, or you do not have permission to copy from it."); |
There was a problem hiding this comment.
this check on cSrc throws NotFoundException but below if the user doesn't have AdminPermission to _cDest, it just skips the if block and returns true. Should that check on _cDest also throw NotFoundException?
2 participants
Rationale
We can improve our parameter validation
Changes
AbstractContainerScopingTest