[LMS-1985] Screening: access rights to a dataset are checked 3 times

(merged from S99.x)

SVN: 19853

datastore_server/source/java/ch/systemsx/cisd/openbis/dss/generic/server/AbstractDssServiceRpc.java

@@ -169,48 +169,6 @@ public abstract class AbstractDssServiceRpc<T> extends AbstractServiceWithLogger
         return access;
     }
 
-    /**
-     * Asserts that specified data sets are all accessible by the user of the specified session.
-     */
-    protected void checkDatasetsAuthorization(String sessionToken, Set<String> dataSetCodes)
-    {
-        if (isSessionAuthorizedForDatasets(sessionToken, dataSetCodes) == false)
-        {
-            throw new IllegalArgumentException(
-                    "User is not allowed to access at least one of the following data sets: "
-                            + dataSetCodes);
-        }
-    }
-
-    /**
-     * Check with openBIS and return a collection of the data sets the user with the given
-     * sessionToken is allowed to access.
-     * 
-     * @param sessionToken The session token for the user.
-     * @param dataSetCodes The data set codes we want to check access for.
-     * @return True if all the data sets are accessible, false if one or more are not accessible.
-     */
-    protected boolean isSessionAuthorizedForDatasets(String sessionToken, Set<String> dataSetCodes)
-    {
-        boolean access;
-        if (operationLog.isInfoEnabled())
-        {
-            operationLog.info(String.format(
-                    "Check access to the data sets '%s' on openBIS server.", dataSetCodes));
-        }
-
-        try
-        {
-            openBISService.checkDataSetCollectionAccess(sessionToken, new ArrayList<String>(dataSetCodes));
-            access = true;
-        } catch (UserFailureException ex)
-        {
-            access = false;
-        }
-
-        return access;
-    }
-    
     protected File getRootDirectory(String datasetCode)
     {
         List<ExternalData> list = getOpenBISService().listDataSetsByCode(Arrays.asList(datasetCode));
@@ -245,14 +203,9 @@ public abstract class AbstractDssServiceRpc<T> extends AbstractServiceWithLogger
     /**
      * Return a map keyed by data set code with value root directory for that data set.
      */
-    protected Map<String, File> checkAccessAndGetRootDirectories(String sessionToken,
+    protected Map<String, File> getRootDirectories(String sessionToken,
             Set<String> dataSetCodes) throws IllegalArgumentException
     {
-        if (isSessionAuthorizedForDatasets(sessionToken, dataSetCodes) == false)
-        {
-            throw new IllegalArgumentException("Path does not exist.");
-        }
-
         HashMap<String, File> rootDirectories = new HashMap<String, File>();
         List<ExternalData> dataSets =
                 openBISService.listDataSetsByCode(new ArrayList<String>(dataSetCodes));

openbis/source/java/ch/systemsx/cisd/openbis/generic/shared/AbstractServerLogger.java

@@ -113,7 +113,7 @@ public abstract class AbstractServerLogger implements IServer
         if (sessionManagerOrNull == null
                 || sessionManagerOrNull.isAWellFormedSessionToken(sessionToken) == false)
         {
-            return null;
+            return "[SESSION:" + sessionToken + "]";
         }
         try
         {
@@ -122,7 +122,7 @@ public abstract class AbstractServerLogger implements IServer
         } catch (InvalidSessionException e)
         {
             // ignore the situation when session is not available
-            return null;
+            return "[NO SESSION]";
         }
     }
 

screening/source/java/ch/systemsx/cisd/openbis/dss/screening/server/DssServiceRpcScreening.java

@@ -24,7 +24,6 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -56,7 +55,7 @@ import ch.systemsx.cisd.openbis.dss.generic.shared.ServiceProvider;
 import ch.systemsx.cisd.openbis.dss.generic.shared.dto.Size;
 import ch.systemsx.cisd.openbis.dss.generic.shared.utils.CodeAndLabelUtil;
 import ch.systemsx.cisd.openbis.dss.generic.shared.utils.ImageUtil;
-import ch.systemsx.cisd.openbis.dss.screening.shared.api.v1.IDssServiceRpcScreeningInternal;
+import ch.systemsx.cisd.openbis.dss.screening.shared.api.v1.IDssServiceRpcScreening;
 import ch.systemsx.cisd.openbis.dss.shared.DssScreeningUtils;
 import ch.systemsx.cisd.openbis.generic.shared.basic.dto.ExternalData;
 import ch.systemsx.cisd.openbis.generic.shared.dto.identifier.SampleIdentifier;
@@ -94,8 +93,8 @@ import ch.systemsx.cisd.openbis.plugin.screening.shared.imaging.dataaccess.Trans
  * 
  * @author Tomasz Pylak
  */
-public class DssServiceRpcScreening extends AbstractDssServiceRpc<IDssServiceRpcScreeningInternal>
-        implements IDssServiceRpcScreeningInternal
+public class DssServiceRpcScreening extends AbstractDssServiceRpc<IDssServiceRpcScreening>
+        implements IDssServiceRpcScreening
 {
 
     /**
@@ -158,7 +157,7 @@ public class DssServiceRpcScreening extends AbstractDssServiceRpc<IDssServiceRpc
         return listAvailableFeatureCodes(sessionToken, featureDatasets);
     }
 
-    public IDssServiceRpcScreeningInternal createLogger(IInvocationLoggerContext context)
+    public IDssServiceRpcScreening createLogger(IInvocationLoggerContext context)
     {
         return new DssServiceRpcScreeningLogger(context);
     }
@@ -166,7 +165,6 @@ public class DssServiceRpcScreening extends AbstractDssServiceRpc<IDssServiceRpc
     public List<String> listAvailableFeatureCodes(String sessionToken,
             List<? extends IFeatureVectorDatasetIdentifier> featureDatasets)
     {
-        checkDatasetsAuthorizationForIDatasetIdentifier(sessionToken, featureDatasets);
         List<String> result = new ArrayList<String>(); // keep the order
         for (IFeatureVectorDatasetIdentifier identifier : featureDatasets)
         {
@@ -187,14 +185,12 @@ public class DssServiceRpcScreening extends AbstractDssServiceRpc<IDssServiceRpc
     public List<ImageDatasetMetadata> listImageMetadata(String sessionToken,
             List<? extends IImageDatasetIdentifier> imageDatasets)
     {
-        checkDatasetsAuthorizationForIDatasetIdentifier(sessionToken, imageDatasets);
         Set<String> datasetCodes = new HashSet<String>();
         for (IImageDatasetIdentifier dataset : imageDatasets)
         {
             datasetCodes.add(dataset.getDatasetCode());
         }
-        Map<String, File> datasetRoots =
-                checkAccessAndGetRootDirectories(sessionToken, datasetCodes);
+        Map<String, File> datasetRoots = getRootDirectories(sessionToken, datasetCodes);
         List<ImageDatasetMetadata> result = new ArrayList<ImageDatasetMetadata>();
         for (IImageDatasetIdentifier dataset : imageDatasets)
         {
@@ -317,7 +313,6 @@ public class DssServiceRpcScreening extends AbstractDssServiceRpc<IDssServiceRpc
     public List<FeatureVectorDataset> loadFeatures(String sessionToken,
             List<FeatureVectorDatasetReference> featureDatasets, List<String> featureNames)
     {
-        checkDatasetsAuthorizationForIDatasetIdentifier(sessionToken, featureDatasets);
         List<String> codes = normalize(featureNames);
         List<FeatureVectorDataset> result = new ArrayList<FeatureVectorDataset>();
         for (FeatureVectorDatasetReference dataset : featureDatasets)
@@ -383,7 +378,6 @@ public class DssServiceRpcScreening extends AbstractDssServiceRpc<IDssServiceRpc
             String sessionToken, List<FeatureVectorDatasetWellReference> datasetWellReferences,
             List<String> featureNames)
     {
-        checkDatasetsAuthorizationForIDatasetIdentifier(sessionToken, datasetWellReferences);
         WellFeatureCollection<FeatureTableRow> features =
                 FeatureVectorLoader.fetchWellFeatures(datasetWellReferences, featureNames, dao,
                         createMetadataProvider());
@@ -438,7 +432,6 @@ public class DssServiceRpcScreening extends AbstractDssServiceRpc<IDssServiceRpc
     private InputStream loadImages(String sessionToken, List<PlateImageReference> imageReferences,
             final Size sizeOrNull, final boolean convertToPng)
     {
-        checkDatasetsAuthorizationForIDatasetIdentifier(sessionToken, imageReferences);
         final Map<String, IImagingDatasetLoader> imageLoadersMap =
                 getImageDatasetsMap(sessionToken, imageReferences);
         return loadImages(imageReferences, sizeOrNull, convertToPng, imageLoadersMap);
@@ -1022,17 +1015,6 @@ public class DssServiceRpcScreening extends AbstractDssServiceRpc<IDssServiceRpc
         return getDAO().listFeatureDefsByDataSetId(dataSet.getId());
     }
 
-    public void checkDatasetsAuthorizationForIDatasetIdentifier(String sessionToken,
-            List<? extends IDatasetIdentifier> identifiers)
-    {
-        Set<String> dataSetCodes = new LinkedHashSet<String>();
-        for (IDatasetIdentifier identifier : identifiers)
-        {
-            dataSetCodes.add(identifier.getDatasetCode());
-        }
-        checkDatasetsAuthorization(sessionToken, dataSetCodes);
-    }
-
     private IImagingReadonlyQueryDAO getDAO()
     {
         synchronized (this)

screening/source/java/ch/systemsx/cisd/openbis/dss/screening/server/DssServiceRpcScreeningLogger.java

@@ -21,7 +21,7 @@ import java.util.List;
 
 import ch.systemsx.cisd.base.image.IImageTransformerFactory;
 import ch.systemsx.cisd.common.spring.IInvocationLoggerContext;
-import ch.systemsx.cisd.openbis.dss.screening.shared.api.v1.IDssServiceRpcScreeningInternal;
+import ch.systemsx.cisd.openbis.dss.screening.shared.api.v1.IDssServiceRpcScreening;
 import ch.systemsx.cisd.openbis.generic.shared.AbstractServerLogger;
 import ch.systemsx.cisd.openbis.plugin.screening.shared.api.v1.dto.FeatureVectorDataset;
 import ch.systemsx.cisd.openbis.plugin.screening.shared.api.v1.dto.FeatureVectorDatasetReference;
@@ -40,7 +40,7 @@ import ch.systemsx.cisd.openbis.plugin.screening.shared.api.v1.dto.WellPosition;
  * @author Franz-Josef Elmer
  */
 public class DssServiceRpcScreeningLogger extends AbstractServerLogger implements
-        IDssServiceRpcScreeningInternal
+        IDssServiceRpcScreening
 {
 
     DssServiceRpcScreeningLogger(IInvocationLoggerContext context)

screening/source/java/ch/systemsx/cisd/openbis/dss/screening/shared/api/v1/DatasetIdentifierPredicate.java

@@ -16,10 +16,17 @@
 
 package ch.systemsx.cisd.openbis.dss.screening.shared.api.v1;
 
+import java.util.ArrayList;
 import java.util.List;
 
+import org.apache.log4j.Logger;
+
 import ch.systemsx.cisd.common.exceptions.Status;
 import ch.systemsx.cisd.common.exceptions.UserFailureException;
+import ch.systemsx.cisd.common.logging.LogCategory;
+import ch.systemsx.cisd.common.logging.LogFactory;
+import ch.systemsx.cisd.openbis.dss.generic.shared.IEncapsulatedOpenBISService;
+import ch.systemsx.cisd.openbis.dss.generic.shared.ServiceProvider;
 import ch.systemsx.cisd.openbis.dss.generic.shared.api.authorization.IAuthorizationGuardPredicate;
 import ch.systemsx.cisd.openbis.plugin.screening.shared.api.v1.dto.IDatasetIdentifier;
 
@@ -30,21 +37,41 @@ import ch.systemsx.cisd.openbis.plugin.screening.shared.api.v1.dto.IDatasetIdent
  */
 public class DatasetIdentifierPredicate
         implements
-        IAuthorizationGuardPredicate<IDssServiceRpcScreeningInternal, List<? extends IDatasetIdentifier>>
+        IAuthorizationGuardPredicate<IDssServiceRpcScreening, List<? extends IDatasetIdentifier>>
 {
 
-    public Status evaluate(IDssServiceRpcScreeningInternal receiver, String sessionToken,
+    static protected final Logger operationLog = LogFactory.getLogger(LogCategory.OPERATION,
+            DatasetIdentifierPredicate.class);
+
+    public Status evaluate(IDssServiceRpcScreening receiver, String sessionToken,
             List<? extends IDatasetIdentifier> datasetIdentifiers) throws UserFailureException
     {
+        final IEncapsulatedOpenBISService openBISService = ServiceProvider.getOpenBISService();
+        if (operationLog.isInfoEnabled())
+        {
+            operationLog.info(String.format(
+                    "Check access to the data sets '%s' on openBIS server.", datasetIdentifiers));
+        }
+
         try
         {
-            receiver.checkDatasetsAuthorizationForIDatasetIdentifier(sessionToken,
-                    datasetIdentifiers);
-        } catch (IllegalArgumentException e)
+            openBISService.checkDataSetCollectionAccess(sessionToken,
+                    getDatasetCodes(datasetIdentifiers));
+            return Status.OK;
+        } catch (UserFailureException ex)
+        {
+            return Status.createError(ex.getMessage());
+        }
+    }
+
+    private List<String> getDatasetCodes(List<? extends IDatasetIdentifier> datasetIdentifiers)
+    {
+        final List<String> result = new ArrayList<String>();
+        for (IDatasetIdentifier id : datasetIdentifiers)
         {
-            return Status.createError(e.getMessage());
+            result.add(id.getDatasetCode());
         }
-        return Status.OK;
+        return result;
     }
 
 }

screening/source/java/ch/systemsx/cisd/openbis/dss/screening/shared/api/v1/IDssServiceRpcScreeningInternal.java

-/*
- * Copyright 2010 ETH Zuerich, CISD
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package ch.systemsx.cisd.openbis.dss.screening.shared.api.v1;
-
-import java.util.List;
-
-import ch.systemsx.cisd.openbis.plugin.screening.shared.api.v1.dto.IDatasetIdentifier;
-
-/**
- * Interface containing internal methods for the screening RPC interface.
- * 
- * @author Chandrasekhar Ramakrishnan
- */
-public interface IDssServiceRpcScreeningInternal extends IDssServiceRpcScreening
-{
-    void checkDatasetsAuthorizationForIDatasetIdentifier(String sessionToken,
-            List<? extends IDatasetIdentifier> featureDatasets);
-}

screening/source/java/ch/systemsx/cisd/openbis/dss/screening/shared/api/v1/SingleDataSetIdentifierPredicate.java

@@ -24,17 +24,17 @@ import ch.systemsx.cisd.openbis.dss.generic.shared.api.authorization.IAuthorizat
 import ch.systemsx.cisd.openbis.plugin.screening.shared.api.v1.dto.IDatasetIdentifier;
 
 /**
- * 
+ * A predicate for testing a single data set identifier.
  *
  * @author Franz-Josef Elmer
  */
 public class SingleDataSetIdentifierPredicate implements
-        IAuthorizationGuardPredicate<IDssServiceRpcScreeningInternal, IDatasetIdentifier>
+        IAuthorizationGuardPredicate<IDssServiceRpcScreening, IDatasetIdentifier>
 
 {
     private static final DatasetIdentifierPredicate PREDICATE = new DatasetIdentifierPredicate();
 
-    public Status evaluate(IDssServiceRpcScreeningInternal receiver, String sessionToken,
+    public Status evaluate(IDssServiceRpcScreening receiver, String sessionToken,
             IDatasetIdentifier datasetIdentifier) throws UserFailureException
     {
         return PREDICATE.evaluate(receiver, sessionToken,

screening/source/java/screening-dssApplicationContext.xml

@@ -28,12 +28,13 @@
             <bean class="org.springframework.aop.framework.ProxyFactoryBean">
                 <property name="proxyInterfaces">
                     <list>
-                        <value>ch.systemsx.cisd.openbis.dss.screening.shared.api.v1.IDssServiceRpcScreeningInternal</value>
+                        <value>ch.systemsx.cisd.openbis.dss.screening.shared.api.v1.IDssServiceRpcScreening</value>
                     </list>
                  </property>
                 <property name="interceptorNames">
                     <list>
                         <value>screening-rpc-authorization-advisor</value>
+                        <value>screening-rpc-log-advisor</value>
                     </list>
                 </property>
                 <property name="target">
@@ -46,12 +47,6 @@
         <property name="serviceInterface" value="ch.systemsx.cisd.openbis.dss.screening.shared.api.v1.IDssServiceRpcScreening" />
     </bean>
     
-    <!-- 
-        // This discovers on-the-fly all <code>org.springframework.aop.Advisor</code> implementations.
-    -->
-    <bean id="bean-post-processor"
-        class="org.springframework.aop.framework.autoproxy.DefaultAdvisorAutoProxyCreator" />
-        
     <!-- 
         // Authorization
     -->
@@ -62,5 +57,5 @@
         // Logging
     -->
 
-    <bean class="ch.systemsx.cisd.common.spring.LogAdvisor" />
+    <bean id="screening-rpc-log-advisor" class="ch.systemsx.cisd.common.spring.LogAdvisor" />
 </beans>
\ No newline at end of file

screening/sourceTest/java/ch/systemsx/cisd/openbis/dss/screening/server/DssServiceRpcScreeningTest.java

@@ -55,7 +55,7 @@ import ch.systemsx.cisd.openbis.dss.generic.server.images.dto.RequestedImageSize
 import ch.systemsx.cisd.openbis.dss.generic.shared.IEncapsulatedOpenBISService;
 import ch.systemsx.cisd.openbis.dss.generic.shared.dto.Size;
 import ch.systemsx.cisd.openbis.dss.generic.shared.utils.ImageUtil;
-import ch.systemsx.cisd.openbis.dss.screening.shared.api.v1.IDssServiceRpcScreeningInternal;
+import ch.systemsx.cisd.openbis.dss.screening.shared.api.v1.IDssServiceRpcScreening;
 import ch.systemsx.cisd.openbis.generic.shared.basic.dto.DatabaseInstance;
 import ch.systemsx.cisd.openbis.generic.shared.basic.dto.Experiment;
 import ch.systemsx.cisd.openbis.generic.shared.basic.dto.ExternalData;
@@ -247,7 +247,7 @@ public class DssServiceRpcScreeningTest extends AssertJUnit
             });
 
         TestMethodInterceptor interceptor = new TestMethodInterceptor();
-        IDssServiceRpcScreeningInternal serviceInternal = getAdvisedService(interceptor);
+        IDssServiceRpcScreening serviceInternal = getAdvisedService(interceptor);
 
         List<String> names =
                 serviceInternal.listAvailableFeatureCodes(SESSION_TOKEN, Arrays.asList(
@@ -640,13 +640,13 @@ public class DssServiceRpcScreeningTest extends AssertJUnit
         }
     }
 
-    private IDssServiceRpcScreeningInternal getAdvisedService(
+    private IDssServiceRpcScreening getAdvisedService(
             TestMethodInterceptor testMethodInterceptor)
     {
         ProxyFactory pf = new ProxyFactory();
         pf.addAdvisor(new DssServiceRpcAuthorizationAdvisor(testMethodInterceptor));
         pf.setTarget(screeningService);
-        pf.addInterface(IDssServiceRpcScreeningInternal.class);
-        return (IDssServiceRpcScreeningInternal) pf.getProxy();
+        pf.addInterface(IDssServiceRpcScreening.class);
+        return (IDssServiceRpcScreening) pf.getProxy();
     }
 }