diff --git a/issues/src/org/labkey/issue/IssuesController.java b/issues/src/org/labkey/issue/IssuesController.java index 38e68a5cbdd..1ff445b4132 100644 --- a/issues/src/org/labkey/issue/IssuesController.java +++ b/issues/src/org/labkey/issue/IssuesController.java @@ -113,6 +113,7 @@ import org.labkey.api.security.roles.OwnerRole; import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.roles.RoleManager; +import org.labkey.api.security.roles.SubmitterRole; import org.labkey.api.util.ButtonBuilder; import org.labkey.api.util.CSRFUtil; import org.labkey.api.util.DOM; @@ -2564,4 +2565,59 @@ private void deleteProjects() _projectA = null; } } + + /** + * GitHub Issue 1317 regression test. + */ + public static class GetIssuePermissionTestCase extends AbstractContainerScopingTest + { + private static final String ISSUE_TITLE = "getIssue() permission test issue"; + + @Test + public void testSubmitterCannotReadIssue() throws Exception + { + Container c = createIssuesContainer("Submitter"); + int issueId = createIssue(c); + + // A reader should be able to access the issue + User reader = createUserInRole(c, ReaderRole.class); + IssueObject asReader = IssueManager.getIssue(c, reader, issueId); + assertNotNull("Reader should be able to read the issue", asReader); + assertEquals(ISSUE_TITLE, asReader.getTitle()); + + // A submitter holds InsertPermission but not ReadPermission, so the issue shouldn't be accessible. + User submitter = createUserInRole(c, SubmitterRole.class); + assertNull("Submitter should not be able to read an issue", IssueManager.getIssue(c, submitter, issueId)); + assertNull("Submitter should not be able to read an issue in an unspecified container", IssueManager.getIssue(null, submitter, issueId)); + } + + private Container createIssuesContainer(String name) + { + Container c = createContainer(name, ModuleLoader.getInstance().getModule(IssuesModule.NAME)); + + IssueListDef def = new IssueListDef(); + def.setName(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + def.setLabel(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + def.setKind(IssueDefDomainKind.NAME); + def.beforeInsert(getAdmin(), c.getId()); + def.save(getAdmin()); + + return c; + } + + private int createIssue(Container c) + { + User admin = getAdmin(); + IssueObject issue = new IssueObject(); + issue.open(c, admin); + issue.setAssignedTo(admin.getUserId()); + issue.setTitle(ISSUE_TITLE); + issue.setPriority("3"); + issue.setIssueDefName(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + ObjectFactory.Registry.getFactory(IssueObject.class).toMap(issue, issue.getProperties()); + IssueManager.saveIssue(admin, c, issue); + + return issue.getIssueId(); + } + } } diff --git a/issues/src/org/labkey/issue/IssuesModule.java b/issues/src/org/labkey/issue/IssuesModule.java index 1b06988a612..1c872133166 100644 --- a/issues/src/org/labkey/issue/IssuesModule.java +++ b/issues/src/org/labkey/issue/IssuesModule.java @@ -193,7 +193,8 @@ public ActionURL getTabURL(Container c, User user) return Set.of( org.labkey.issue.model.IssueManager.TestCase.class, org.labkey.issue.IssuesController.MoveActionContainerScopingTestCase.class, - org.labkey.issue.IssuesController.GetUsersForGroupScopingTestCase.class + org.labkey.issue.IssuesController.GetUsersForGroupScopingTestCase.class, + org.labkey.issue.IssuesController.GetIssuePermissionTestCase.class ); } diff --git a/issues/src/org/labkey/issue/model/IssueManager.java b/issues/src/org/labkey/issue/model/IssueManager.java index d598e313e94..bca543f3d5a 100644 --- a/issues/src/org/labkey/issue/model/IssueManager.java +++ b/issues/src/org/labkey/issue/model/IssueManager.java @@ -122,9 +122,9 @@ import java.util.TreeSet; import java.util.function.Consumer; -import static org.labkey.api.util.IntegerUtils.asInteger; import static org.labkey.api.search.SearchService.PROPERTY.categories; import static org.labkey.api.security.UserManager.USER_DISPLAY_NAME_COMPARATOR; +import static org.labkey.api.util.IntegerUtils.asInteger; public class IssueManager { @@ -240,14 +240,21 @@ public static IssueObject getIssue( private static IssueObject _getIssue(@Nullable Container c, User user, int issueId) { IssueObject issue = _getRawIssue(c, issueId); + if (issue == null) + return null; - if (issue != null && issue.getIssueDefId() != null) - { - // container may initially be null if we don't care about a specific folder, but we need the - // correct domain for the provisioned table properties associated with the issue - if (c == null) - c = ContainerManager.getForId(issue.getContainerId()); + // container may initially be null if we don't care about a specific folder, but we need the + // correct domain for the provisioned table properties associated with the issue + if (c == null) + c = ContainerManager.getForId(issue.getContainerId()); + // GitHub Issue 1317: explicitly check for read access on the target container before querying to + // avoid Submitter roles from accessing the table and then hitting an exception during the read. + if (c == null || !c.hasPermission(user, ReadPermission.class)) + return null; + + if (issue.getIssueDefId() != null) + { IssueListDef issueListDef = getIssueListDef(issue.getContainerFromId(), issue.getIssueDefId()); UserSchema userSchema = QueryService.get().getUserSchema(user, c, IssuesQuerySchema.SCHEMA_NAME); TableInfo table = userSchema.getTable(issueListDef.getName());