Refactored job access checks pr

RdcRmodulesGrailsPlugin.groovy

@@ -22,7 +22,7 @@ import org.transmartproject.core.users.User
 
 class RdcRmodulesGrailsPlugin {
     // the plugin version
-    def version = "16.2-SNAPSHOT"
+    def version = "16.2"
     // the version or versions of Grails the plugin is designed for
     def grailsVersion = "2.2.4 > *"
     // the other plugins this plugin depends on

grails-app/conf/BuildConfig.groovy

@@ -62,7 +62,7 @@ grails.project.dependency.resolution = {
 
         /* serializable ImmutableMap only on guava 16 */
         compile group: 'com.google.guava', name: 'guava', version: '16.0-dev-20140115-68c8348'
-        compile 'org.transmartproject:transmart-core-api:16.2-SNAPSHOT'
+        compile 'org.transmartproject:transmart-core-api:16.2'
 
         /* compile instead of test due to technical limitations
          * (referenced from resources.groovy) */

grails-app/controllers/com/recomdata/transmart/rmodules/AnalysisFilesController.groovy

@@ -1,24 +1,19 @@
 package com.recomdata.transmart.rmodules
 
-import org.transmartproject.core.exceptions.InvalidRequestException
-
-import java.util.regex.Matcher
-import java.util.regex.Pattern
+import org.transmartproject.jobs.access.JobsAccessChecksService
 
 class AnalysisFilesController {
 
-    public static final String ROLE_ADMIN = 'ROLE_ADMIN'
-
     def sendFileService
 
-    def springSecurityService
-
     def RModulesOutputRenderService
 
+    JobsAccessChecksService jobsAccessChecksService
+
     def download() {
         String jobName = params.analysisName
 
-        if (!checkPermissions(jobName)) {
+        if (!jobsAccessChecksService.canDownload(jobName)) {
             render status: 403
             return
         }
@@ -64,44 +59,7 @@ class AnalysisFilesController {
         sendFileService.sendFile servletContext, request, response, targetFile
     }
 
-    private boolean isAdmin() {
-        springSecurityService.principal.authorities.any {
-            it.authority == ROLE_ADMIN
-        }
-    }
-
     private File getJobsDirectory() {
         new File(RModulesOutputRenderService.tempFolderDirectory)
     }
-
-    private boolean checkPermissions(String jobName) {
-        String userName = extractUserFromJobName(jobName)
-
-        def loggedInUser = springSecurityService.principal?.username
-        if (!loggedInUser) {
-            log.error 'Could not determine current logged in user\'s name'
-            return false
-        }
-
-        if (userName == loggedInUser || admin) {
-            return true
-        }
-
-        log.warn "User $loggedInUser has no access for job $jobName; refusing " +
-                "request for job $jobName"
-        false
-    }
-
-    private String extractUserFromJobName(String jobName) {
-        Pattern pattern = ~/(.+)-[a-zA-Z]+-\d+/
-        Matcher matcher = pattern.matcher(jobName)
-
-        if (!matcher.matches()) {
-            //should never happen due to url mapping
-            throw new InvalidRequestException('Invalid job name')
-        }
-
-        matcher.group(1)
-    }
-
 }

grails-app/services/org/transmartproject/jobs/access/JobsAccessChecksService.groovy

@@ -0,0 +1,51 @@
+package org.transmartproject.jobs.access
+
+import org.transmartproject.core.exceptions.InvalidRequestException
+
+import java.util.regex.Matcher
+import java.util.regex.Pattern
+
+class JobsAccessChecksService {
+
+    static transactional = false
+
+    public static final String ROLE_ADMIN = 'ROLE_ADMIN'
+
+    def springSecurityService
+
+    boolean canDownload(String jobName) {
+        String userName = extractUserFromJobName(jobName)
+
+        def loggedInUser = springSecurityService.principal?.username
+        if (!loggedInUser) {
+            log.error 'Could not determine current logged in user\'s name'
+            return false
+        }
+
+        if (userName == loggedInUser || admin) {
+            return true
+        }
+
+        log.warn "User $loggedInUser has no access for job $jobName; refusing " +
+                "request for job $jobName"
+        false
+    }
+
+    private boolean isAdmin() {
+        springSecurityService.principal.authorities.any {
+            it.authority == ROLE_ADMIN
+        }
+    }
+
+    private static String extractUserFromJobName(String jobName) {
+        Pattern pattern = ~/(.+)-[a-zA-Z]+-\d+/
+        Matcher matcher = pattern.matcher(jobName)
+
+        if (!matcher.matches()) {
+            //should never happen due to url mapping
+            throw new InvalidRequestException('Invalid job name')
+        }
+
+        matcher.group(1)
+    }
+}

test/unit/com/recomdata/transmart/rmodules/AnalysisFilesControllerTests.groovy

@@ -7,6 +7,7 @@ import org.junit.After
 import org.junit.Before
 import org.junit.Test
 import org.transmartproject.core.exceptions.InvalidRequestException
+import org.transmartproject.jobs.access.JobsAccessChecksService
 
 import javax.servlet.ServletContext
 import javax.servlet.http.HttpServletRequest
@@ -19,18 +20,14 @@ import static org.hamcrest.Matchers.*
 @WithGMock
 class AnalysisFilesControllerTests {
 
-    private static final String USER_NAME = 'user'
-    private static final String OTHER_USER_NAME = 'other_user'
-    private static final String ADMIN_NAME = 'admin'
     private static final String EXISTING_FILE_NAME = 'file_that_exists'
     private static final String FILE_CONTENTS = 'file contents\n'
-    private static final String ANALYSIS_NAME = "$USER_NAME-Analysis-100"
+    private static final String ANALYSIS_NAME = "user-Analysis-100"
 
     File temporaryDirectory
     File analysisDirectory
     File targetFile
     def sendFileServiceMock
-    def mockGrailsUser
 
     @Before
     void before() {
@@ -45,28 +42,11 @@ class AnalysisFilesControllerTests {
         sendFileServiceMock = mock()
         controller.sendFileService = sendFileServiceMock
 
-        mockGrailsUser = mock()
-        controller.springSecurityService = mock()
-        controller.springSecurityService.principal.
-                returns(mockGrailsUser).stub()
+        controller.jobsAccessChecksService = mock JobsAccessChecksService
 
         params.analysisName = ANALYSIS_NAME
     }
 
-    void setTestUsername(String username) {
-        mockGrailsUser.username.returns username
-    }
-
-    void setAdmin(boolean value) {
-        if (value) {
-            def adminAuthority = mock()
-            adminAuthority.authority.returns AnalysisFilesController.ROLE_ADMIN
-            mockGrailsUser.authorities.returns([adminAuthority])
-        } else {
-            mockGrailsUser.authorities.returns([])
-        }
-    }
-
     void setFile(String filename) {
         targetFile = new File(analysisDirectory, filename)
         targetFile << FILE_CONTENTS
@@ -82,8 +62,8 @@ class AnalysisFilesControllerTests {
     @Test
     void basicTest() {
         // test the normal circumstances (file exists and is allowed)
-        testUsername = USER_NAME
-        file         = EXISTING_FILE_NAME
+        controller.jobsAccessChecksService.canDownload(ANALYSIS_NAME).returns(true)
+        file = EXISTING_FILE_NAME
 
         sendFileServiceMock.sendFile(isA(ServletContext),
                 isA(HttpServletRequest), isA(HttpServletResponse),
@@ -98,8 +78,7 @@ class AnalysisFilesControllerTests {
 
     @Test
     void testNoPermission() {
-        testUsername = OTHER_USER_NAME
-        admin        = false
+        controller.jobsAccessChecksService.canDownload(ANALYSIS_NAME).returns(false)
 
         play {
             controller.download()
@@ -108,38 +87,10 @@ class AnalysisFilesControllerTests {
         assertThat response.status, is(403)
     }
 
-    @Test
-    void testAdminAlwaysHasPermission() {
-        testUsername = ADMIN_NAME
-        admin        = true
-        file         = EXISTING_FILE_NAME
-
-        sendFileServiceMock.sendFile(isA(ServletContext),
-                isA(HttpServletRequest), isA(HttpServletResponse),
-                is(equalTo(targetFile)))
-
-        play {
-            controller.download()
-        }
-
-        assertThat response.status, is(200)
-    }
-
-    @Test
-    void testBadAnalysisName() {
-        params.analysisName = 'not_a_valid_analysis_name'
-
-        play {
-            shouldFail InvalidRequestException, {
-                controller.download()
-            }
-        }
-    }
-
     @Test
     void testInexistingAnalysisName() {
-        testUsername        = USER_NAME
         params.analysisName = ANALYSIS_NAME + '1'
+        controller.jobsAccessChecksService.canDownload(params.analysisName).returns(true)
 
         play {
             controller.download()
@@ -150,19 +101,19 @@ class AnalysisFilesControllerTests {
 
     @Test
     void testAccessToExternalFilesNotAllowed() {
-        testUsername        = USER_NAME
-
+        controller.jobsAccessChecksService.canDownload(ANALYSIS_NAME).returns(true)
         file = '../test'
+
         play {
             controller.download()
         }
+
         assertThat response.status, is(404)
     }
 
     @Test
     void testNonExistingFile() {
-        testUsername = USER_NAME
-
+        controller.jobsAccessChecksService.canDownload(ANALYSIS_NAME).returns(true)
         params.path = 'file_that_does_not_exist'
 
         play {

test/unit/org/transmartproject/jobs/access/JobsAccessChecksServiceTests.groovy

@@ -0,0 +1,87 @@
+package org.transmartproject.jobs.access
+
+import grails.test.mixin.TestFor
+import org.gmock.WithGMock
+import org.junit.Before
+import org.junit.Test
+import org.transmartproject.core.exceptions.InvalidRequestException
+
+@TestFor(JobsAccessChecksService)
+@WithGMock
+class JobsAccessChecksServiceTests {
+
+    @Test
+    void testOwnerCanReadData() {
+        testUsername = 'test_user'
+        admin = false
+
+        play {
+            assertTrue service.canDownload('test_user-Analysis-100')
+        }
+    }
+
+    @Test
+    void testOtherUserCanNotReadData() {
+        testUsername = 'other_user'
+        admin = false
+
+        play {
+            assertFalse service.canDownload('test_user-Analysis-100')
+        }
+    }
+
+    @Test
+    void testAdminCanReadData() {
+        testUsername = 'admin_user'
+        admin = true
+
+        play {
+            assertTrue service.canDownload('test_user-Analysis-100')
+        }
+    }
+
+    @Test
+    void testNotLoggedInUserCanNotReadData() {
+        service.springSecurityService = mock()
+        service.springSecurityService.principal.returns(null).stub()
+
+        play {
+            assertFalse service.canDownload('test_user-Analysis-100')
+        }
+    }
+
+    @Test
+    void testBadAnalysisName() {
+        testUsername = 'test_user'
+        admin = false
+
+        play {
+            shouldFail InvalidRequestException, {
+                assertFalse service.canDownload('not_a_valid_analysis_name')
+            }
+        }
+    }
+
+    def mockGrailsUser
+
+    @Before
+    void before() {
+        mockGrailsUser = mock()
+        service.springSecurityService = mock()
+        service.springSecurityService.principal.returns(mockGrailsUser).stub()
+    }
+
+    void setTestUsername(String username) {
+        mockGrailsUser.username.returns(username).stub()
+    }
+
+    void setAdmin(boolean value) {
+        def authorities = []
+        if (value) {
+            def adminAuthority = mock()
+            adminAuthority.authority.returns JobsAccessChecksService.ROLE_ADMIN
+            authorities.add(adminAuthority)
+        }
+        mockGrailsUser.authorities.returns(authorities).stub()
+    }
+}

web-app/Rscripts/Heatmap/HClusteredHeatmapLoader.R

@@ -124,8 +124,8 @@ zscore.file = "zscores.txt"
             sd_rows_mRNA<-apply (mRNAData,1,sd,na.rm=T)
             mRNAData<-mRNAData[!is.na(sd_rows_mRNA),]                                       # remove markers where sd is NA
             sd_rows_mRNA<-sd_rows_mRNA[!is.na(sd_rows_mRNA)]
-            cutoff_sd<- sd_rows_mRNA[order(sd_rows_mRNA,decreasing = T)][maxDrawNumber+1]   # filter by SD, draw only the top maxDrawNumber
-            mRNAData<-mRNAData[sd_rows_mRNA>cutoff_sd,]
+            indices_to_include <- order(sd_rows_mRNA, decreasing = T)[1:maxDrawNumber]      # filter by SD, keep only the top maxDrawNumber
+            mRNAData <- mRNAData[indices_to_include, ]
     }
 
     colcolor<-colnames(mRNAData)                                                            # assign colors for different subset

web-app/Rscripts/Heatmap/KMeansHeatmap.R

@@ -115,8 +115,8 @@ zscore.file = "zscores.txt"
             sd_rows_matrix<-apply (matrixData,1,sd,na.rm=T)
             matrixData<-matrixData[!is.na(sd_rows_matrix),]                                 # remove markers where sd is NA
             sd_rows_matrix<-sd_rows_matrix[!is.na(sd_rows_matrix)]
-            cutoff_sd<- sd_rows_matrix[order(sd_rows_matrix,decreasing = T)][maxDrawNumber+1] # filter by SD, draw only the top maxDrawNumber
-            matrixData<-matrixData[sd_rows_matrix>cutoff_sd,]
+            indices_to_include <- order(sd_rows_matrix, decreasing = T)[1:maxDrawNumber]      # filter by SD, keep only the top maxDrawNumber
+            matrixData <- matrixData[indices_to_include, ]
     }
 
     #Transpose the matrix to put the sample column into a row.