Fixes upload loophole (#517)

* fix: Uploads now require proposal membership

* fix: Minor log tweak

* fix: Jobn execution now checks project membership

* fix: Revert project check in Squonk - should be handled higher up

* fix: Job execution now requires project membership

* build: Fix build (local precommit problem)

* ci: Update GitHub action versions

* fix: Improved target experiment file handling

* fix: Internal function rename

* fix: Better control of security cache (new default of 2 minutes)

---------

Co-authored-by: Alan Christie <[email protected]>

.github/workflows/build-dev.yaml

@@ -81,7 +81,7 @@ jobs:
         pip install --requirement build-requirements.txt
         pre-commit run --all-files
     - name: Docker build
-      uses: docker/build-push-action@v4
+      uses: docker/build-push-action@v5
       with:
         context: .
         tags: ${{ steps.vars.outputs.BE_NAMESPACE }}/fragalysis-backend:${{ env.GITHUB_REF_SLUG }}
@@ -96,7 +96,7 @@ jobs:
         BE_IMAGE_TAG: ${{ env.GITHUB_REF_SLUG }}
     - name: Login to DockerHub
       if: steps.vars.outputs.push == 'true'
-      uses: docker/login-action@v2
+      uses: docker/login-action@v3
       with:
         username: ${{ secrets.DOCKERHUB_USERNAME }}
         password: ${{ secrets.DOCKERHUB_TOKEN }}

.github/workflows/build-production.yaml

@@ -128,7 +128,7 @@ jobs:
         pip install --requirement build-requirements.txt
         pre-commit run --all-files
     - name: Build
-      uses: docker/build-push-action@v4
+      uses: docker/build-push-action@v5
       with:
         context: .
         tags: |
@@ -145,7 +145,7 @@ jobs:
         BE_IMAGE_TAG: ${{ steps.vars.outputs.tag }}
     - name: Login to DockerHub
       if: steps.vars.outputs.push == 'true'
-      uses: docker/login-action@v2
+      uses: docker/login-action@v3
       with:
         username: ${{ secrets.DOCKERHUB_USERNAME }}
         password: ${{ secrets.DOCKERHUB_TOKEN }}

.github/workflows/build-staging.yaml

@@ -150,7 +150,7 @@ jobs:
         pip install --requirement build-requirements.txt
         pre-commit run --all-files
     - name: Docker build
-      uses: docker/build-push-action@v4
+      uses: docker/build-push-action@v5
       with:
         context: .
         tags: ${{ steps.vars.outputs.BE_NAMESPACE }}/fragalysis-backend:${{ steps.vars.outputs.tag }}

api/security.py

@@ -19,9 +19,11 @@
 
 logger: logging.Logger = logging.getLogger(__name__)
 
+# A list of cached security results.
+# Results use the key 'RESULTS' and the collection time uses the key 'TIMESTAMP'.
 USER_LIST_DICT: Dict[str, Any] = {}
-
-connector: str = os.environ.get('SECURITY_CONNECTOR', 'ispyb')
+# Period to cache user lists in seconds
+USER_LIST_CACHE_SECONDS: int = settings.SECURITY_CONNECTOR_CACHE_MINUTES * 60
 
 # example test:
 # from rest_framework.test import APIRequestFactory
@@ -109,9 +111,9 @@ def get_conn() -> Optional[Connector]:
 
 
 def get_configured_connector() -> Optional[Union[Connector, SSHConnector]]:
-    if connector == 'ispyb':
+    if settings.SECURITY_CONNECTOR == 'ispyb':
         return get_conn()
-    elif connector == 'ssh_ispyb':
+    elif settings.SECURITY_CONNECTOR == 'ssh_ispyb':
         return get_remote_conn()
     return None
 
@@ -122,9 +124,9 @@ def ping_configured_connector() -> bool:
     a connection can be made.
     """
     conn: Optional[Union[Connector, SSHConnector]] = None
-    if connector == 'ispyb':
+    if settings.SECURITY_CONNECTOR == 'ispyb':
         conn = get_conn()
-    elif connector == 'ssh_ispyb':
+    elif settings.SECURITY_CONNECTOR == 'ssh_ispyb':
         conn = get_remote_conn()
         if conn is not None:
             conn.stop()
@@ -175,17 +177,18 @@ def get_proposals_for_user_from_django(self, user):
             return prop_ids
 
     def needs_updating(self, user):
-        """Returns true of the data collected for a user is out of date.
-        It's simple, we just record the last collected timestamp and consider it
-        'out of date' (i.e. more than an hour old).
-        """
-        update_window = 3600
-        if user.username not in USER_LIST_DICT:
-            USER_LIST_DICT[user.username] = {"RESULTS": [], "TIMESTAMP": 0}
+        """Returns true of the data collected for a user is out of date."""
         current_time = time.time()
-        if current_time - USER_LIST_DICT[user.username]["TIMESTAMP"] > update_window:
+        if user.username not in USER_LIST_DICT:
+            USER_LIST_DICT[user.username] = {"RESULTS": [], "TIMESTAMP": current_time}
+        if (
+            current_time - USER_LIST_DICT[user.username]["TIMESTAMP"]
+            >= USER_LIST_CACHE_SECONDS
+        ):
+            # Clear the cache (using the current time as the new timestamp)
             USER_LIST_DICT[user.username]["TIMESTAMP"] = current_time
             return True
+        # Cached results are still valid...
         return False
 
     def run_query_with_connector(self, conn, user):
@@ -283,13 +286,26 @@ def get_proposals_for_user_from_ispyb(self, user):
             )
             return cached_prop_ids
 
-    def get_proposals_for_user(self, user):
-        """Returns a list of proposals (public and private) that the user has access to."""
+    def get_proposals_for_user(self, user, restrict_to_membership=False):
+        """Returns a list of proposals that the user has access to.
+
+        If 'restrict_to_membership' is set only those proposals/visits where the user
+        is a member of the visit will be returned. Otherwise the 'public'
+        proposals/visits will also be returned. Typically 'restrict_to_membership' is
+        used for uploads/changes - this allows us to implement logic that (say)
+        only permits explicit members of public proposals to add/load data for that
+        project (restrict_to_membership=True), but everyone can 'see' public data
+        (restrict_to_membership=False).
+        """
         assert user
 
         proposals = []
         ispyb_user = os.environ.get("ISPYB_USER")
-        logger.debug("ispyb_user=%s", ispyb_user)
+        logger.debug(
+            "ispyb_user=%s restrict_to_membership=%s",
+            ispyb_user,
+            restrict_to_membership,
+        )
         if ispyb_user:
             if user.is_authenticated:
                 logger.info("Getting proposals from ISPyB...")
@@ -301,11 +317,13 @@ def get_proposals_for_user(self, user):
             logger.info("Getting proposals from Django...")
             proposals = self.get_proposals_for_user_from_django(user)
 
-        # Now add in Target Access Strings that everyone has access to
-        # (unless we already have them)
-        for open_proposal in self.get_open_proposals():
-            if open_proposal not in proposals:
-                proposals.append(open_proposal)
+        # We have all the proposals where the user has membership.
+        if not restrict_to_membership:
+            # Here we're not restricting proposals to those where the user is a member,
+            # so we add those projects/proposals that everyone has access to.
+            for open_proposal in self.get_open_proposals():
+                if open_proposal not in proposals:
+                    proposals.append(open_proposal)
 
         return proposals
 

fragalysis/settings.py

@@ -11,6 +11,7 @@
 """
 
 import os
+import sys
 from datetime import timedelta
 
 import sentry_sdk
@@ -32,13 +33,6 @@
 PROPOSAL_SUPPORTED = True
 PROPOSAL_REQUIRED = True
 
-# Authentication check when uploading files.
-# This can be switched off to simplify development testing if required.
-# Should always be True on production.
-AUTHENTICATE_UPLOAD = True
-if os.environ.get("AUTHENTICATE_UPLOAD") == 'False':
-    AUTHENTICATE_UPLOAD = False
-
 # AnonymousUser should be the first record inserted into the auth_user table.
 ANONYMOUS_USER = 1
 
@@ -230,6 +224,15 @@
 OIDC_STORE_ACCESS_TOKEN = True
 OIDC_STORE_ID_TOKEN = True
 
+# Security/access control connector.
+# Currently one of 'ispyb' or 'ssh_ispyb'.
+SECURITY_CONNECTOR = os.environ.get('SECURITY_CONNECTOR', 'ispyb').lower()
+# Number of minutes to cache security information for a user.
+# Set to '0' to disable caching.
+SECURITY_CONNECTOR_CACHE_MINUTES = int(
+    os.environ.get('SECURITY_CONNECTOR_CACHE_MINUTES', '2')
+)
+
 # SessionRefresh configuration.
 # There's only one item - the token expiry period, with a default of 15 minutes.
 # The default is 15 minutes if you don't set this value.
@@ -243,6 +246,14 @@
 # see api.utils for the 'deployment_mode_is_production()' function.
 DEPLOYMENT_MODE = os.environ.get("DEPLOYMENT_MODE", "production").upper()
 
+# Authentication check when uploading files.
+# This can be switched off to simplify development testing if required.
+# It's asserted as True for 'production' mode.
+AUTHENTICATE_UPLOAD = True
+if os.environ.get("AUTHENTICATE_UPLOAD") == 'False':
+    assert DEPLOYMENT_MODE != "PRODUCTION"
+    AUTHENTICATE_UPLOAD = False
+
 ROOT_URLCONF = "fragalysis.urls"
 
 STATIC_ROOT = os.path.join(PROJECT_ROOT, "static")
@@ -442,6 +453,7 @@
             'console': {
                 'level': 'DEBUG',
                 'class': 'logging.StreamHandler',
+                'stream': sys.stdout,
                 'formatter': 'simple',
             },
             'rotating': {

viewer/squonk2_agent.py

@@ -746,11 +746,11 @@ def _verify_access(self, c_params: CommonParams) -> Squonk2AgentRv:
         target_access_string = self._get_target_access_string(access_id)
         assert target_access_string
         proposal_list: List[str] = self.__ispyb_safe_query_set.get_proposals_for_user(
-            user
+            user, restrict_to_membership=True
         )
         if not target_access_string in proposal_list:
             msg = (
-                f'The user ({user.username}) cannot access "{target_access_string}"'
+                f'The user ({user.username}) cannot modify "{target_access_string}"'
                 f' (access_id={access_id}). Only {proposal_list})'
             )
             _LOGGER.warning(msg)

viewer/target_loader.py

@@ -188,7 +188,7 @@ def _validate_bundle_against_mode(config_yaml: Dict[str, Any]) -> Optional[str]:
     # Initial concern - the loader's git information.
     # It must not be 'dirty' and must have a valid 'tag'.
     xca_git_info_key = "xca_git_info"
-    base_error_msg = "Stack is in PRODUCTION mode - and "
+    base_error_msg = "Stack is in PRODUCTION mode - and"
     try:
         xca_git_info = config_yaml[xca_git_info_key]
     except KeyError:
@@ -1826,11 +1826,17 @@ def load_target(
                 archive.extractall(target_loader.raw_data)
                 msg = f"Data extraction complete: {data_bundle}"
                 logger.info("%s%s", target_loader.report.task_id, msg)
-        except FileNotFoundError as exc:
-            msg = f"{data_bundle} file does not exist"
-            logger.exception("%s%s", target_loader.report.task_id, msg)
-            target_loader.experiment_upload.message = exc.args[0]
-            raise FileNotFoundError(msg) from exc
+        except Exception as exc:
+            # Handle _any_ underlying problem with the file.
+            logger.error('Got an exception opening the file: %s', str(exc))
+            target_loader.report.log(
+                logging.ERROR,
+                f"Decompression of '{bundle_filename}' has failed. Is it a Target Experiment file?",
+            )
+            target_loader.report.final(
+                f"Failed to decompress '{target_loader.data_bundle}'", success=False
+            )
+            return
 
         target_loader.report.log(logging.INFO, f"Decompressed '{bundle_filename}'")
 
@@ -1849,23 +1855,23 @@ def load_target(
             # Any problem with the underlying data is transmitted in the report.
             logger.debug(exc, exc_info=True)
             target_loader.report.final(
-                f"Uploading {target_loader.data_bundle} failed", success=False
+                f"Failed to process '{target_loader.data_bundle}'", success=False
             )
             return
-
         else:
-            # Move the uploaded file to its final location
-            target_loader.abs_final_path.mkdir(parents=True)
-            target_loader.raw_data.rename(target_loader.abs_final_path)
-            Path(target_loader.bundle_path).rename(
-                target_loader.abs_final_path.parent.joinpath(target_loader.data_bundle)
-            )
+            _move_and_save_target_experiment(target_loader)
 
-            set_directory_permissions(target_loader.abs_final_path, 0o755)
 
-            target_loader.report.final(
-                f"{target_loader.data_bundle} uploaded successfully"
-            )
-            target_loader.experiment_upload.message = target_loader.report.json()
+def _move_and_save_target_experiment(target_loader):
+    # Move the uploaded file to its final location
+    target_loader.abs_final_path.mkdir(parents=True)
+    target_loader.raw_data.rename(target_loader.abs_final_path)
+    Path(target_loader.bundle_path).rename(
+        target_loader.abs_final_path.parent.joinpath(target_loader.data_bundle)
+    )
+
+    set_directory_permissions(target_loader.abs_final_path, 0o755)
 
-            target_loader.experiment_upload.save()
+    target_loader.report.final(f"{target_loader.data_bundle} uploaded successfully")
+    target_loader.experiment_upload.message = target_loader.report.json()
+    target_loader.experiment_upload.save()