From 205e59f32961440b7feb287654394aeca7776d95 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Fri, 21 Jun 2024 14:08:47 +0100 Subject: [PATCH 01/39] fix: Adds extra log to Targets view --- viewer/views.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/viewer/views.py b/viewer/views.py index 93d6b12c..10432ce4 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -199,21 +199,24 @@ class TargetView(ISpyBSafeQuerySet): filterset_fields = ("title",) def patch(self, request, pk): + logger.info("pk=%s", pk) try: target = self.queryset.get(pk=pk) except models.Target.DoesNotExist: + msg = f"Target pk={pk} does not exist" + logger.warning(msg) return Response( - {"message": f"Target pk={pk} does not exist"}, + {"message": msg}, status=status.HTTP_404_NOT_FOUND, ) serializer = self.serializer_class(target, data=request.data, partial=True) if serializer.is_valid(): - logger.debug("serializer data: %s", serializer.validated_data) + logger.info("serializer.validated_data=%s", serializer.validated_data) serializer.save() return Response(serializer.data, status=status.HTTP_200_OK) else: - logger.debug("serializer error: %s", serializer.errors) + logger.warning("serializer.errors=%s", serializer.errors) return Response( {"message": "wrong parameters"}, status=status.HTTP_400_BAD_REQUEST ) From fcd43c8ec1f5e8fac032dc8d836f087198ecfe6c Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Fri, 21 Jun 2024 14:41:12 +0100 Subject: [PATCH 02/39] fix: More target logging --- viewer/models.py | 7 ++++++- viewer/views.py | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/viewer/models.py b/viewer/models.py index 0d38914f..e35d41ac 100644 --- a/viewer/models.py +++ b/viewer/models.py @@ -123,7 +123,12 @@ def __str__(self) -> str: return f"{self.title}" def __repr__(self) -> str: - return "" % (self.id, self.title, self.project_id) + return "" % ( + self.id, + self.title, + self.display_name, + self.project_id, + ) class ExperimentUpload(models.Model): diff --git a/viewer/views.py b/viewer/views.py index 10432ce4..41dd8471 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -210,10 +210,16 @@ def patch(self, request, pk): status=status.HTTP_404_NOT_FOUND, ) + logger.info("request.data=%s", request.data) serializer = self.serializer_class(target, data=request.data, partial=True) if serializer.is_valid(): logger.info("serializer.validated_data=%s", serializer.validated_data) - serializer.save() + logger.info( + "serializer.validated_data.display_name=%s", + serializer.validated_data.get('display_name'), + ) + new_target = serializer.save() + logger.info("new_target=%s", new_target) return Response(serializer.data, status=status.HTTP_200_OK) else: logger.warning("serializer.errors=%s", serializer.errors) From 65eb128cc6f642dc5aa9d3a01e234d0757b939a7 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Fri, 21 Jun 2024 15:17:06 +0100 Subject: [PATCH 03/39] fix: safe query set now derived from ModelViewSet --- api/security.py | 2 +- viewer/views.py | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/api/security.py b/api/security.py index 7aab1b3b..64f60fc3 100644 --- a/api/security.py +++ b/api/security.py @@ -184,7 +184,7 @@ def ping_configured_connector() -> bool: return conn is not None -class ISpyBSafeQuerySet(viewsets.ReadOnlyModelViewSet): +class ISpyBSafeQuerySet(viewsets.ModelViewSet): def get_queryset(self): """ Optionally restricts the returned purchases to a given proposals diff --git a/viewer/views.py b/viewer/views.py index 41dd8471..c18ddf5c 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -210,16 +210,20 @@ def patch(self, request, pk): status=status.HTTP_404_NOT_FOUND, ) + logger.info("target=%s", repr(target)) logger.info("request.data=%s", request.data) serializer = self.serializer_class(target, data=request.data, partial=True) if serializer.is_valid(): logger.info("serializer.validated_data=%s", serializer.validated_data) + logger.info( + "serializer.validated_datakeys=%s", serializer.validated_data.keys() + ) logger.info( "serializer.validated_data.display_name=%s", serializer.validated_data.get('display_name'), ) new_target = serializer.save() - logger.info("new_target=%s", new_target) + logger.info("new_target=%s", repr(new_target)) return Response(serializer.data, status=status.HTTP_200_OK) else: logger.warning("serializer.errors=%s", serializer.errors) From b781a506ad9b25c21f479597be792f2cbfcf9dab Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Fri, 21 Jun 2024 15:59:50 +0100 Subject: [PATCH 04/39] feat: Experiment with IsProposalMember class --- api/security.py | 2 +- viewer/permissions.py | 27 +++++++++++++++++++++++++++ viewer/views.py | 2 ++ 3 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 viewer/permissions.py diff --git a/api/security.py b/api/security.py index 64f60fc3..7aab1b3b 100644 --- a/api/security.py +++ b/api/security.py @@ -184,7 +184,7 @@ def ping_configured_connector() -> bool: return conn is not None -class ISpyBSafeQuerySet(viewsets.ModelViewSet): +class ISpyBSafeQuerySet(viewsets.ReadOnlyModelViewSet): def get_queryset(self): """ Optionally restricts the returned purchases to a given proposals diff --git a/viewer/permissions.py b/viewer/permissions.py new file mode 100644 index 00000000..667a7a7a --- /dev/null +++ b/viewer/permissions.py @@ -0,0 +1,27 @@ +import logging + +from rest_framework import permissions + +from api.security import ISpyBSafeQuerySet + +_LOGGER: logging.Logger = logging.getLogger(__name__) +_ISPYB_SAFE_QUERY_SET: ISpyBSafeQuerySet = ISpyBSafeQuerySet() + + +class IsProposalMember(permissions.BasePermission): + """ + Custom permission to only allow owners of an object to edit it. + """ + + def has_object_permission(self, request, view, obj): + del view + + # Read permissions are allowed to any request, + # so we'll always allow GET, HEAD or OPTIONS requests. + if request.method in permissions.SAFE_METHODS: + return True + + _LOGGER.info("Checking %s", repr(obj)) + proposals = _ISPYB_SAFE_QUERY_SET.get_proposals_for_user(request.user) + _LOGGER.info("Proposals=%s", proposals) + return True diff --git a/viewer/views.py b/viewer/views.py index c18ddf5c..05a2f869 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -35,6 +35,7 @@ from api.utils import get_highlighted_diffs, get_params, pretty_request from service_status.models import Service from viewer import filters, models, serializers +from viewer.permissions import IsProposalMember from viewer.squonk2_agent import ( AccessParams, CommonParams, @@ -197,6 +198,7 @@ class TargetView(ISpyBSafeQuerySet): serializer_class = serializers.TargetSerializer filter_permissions = "project_id" filterset_fields = ("title",) + permission_classes = [IsProposalMember] def patch(self, request, pk): logger.info("pk=%s", pk) From 63f752c88d3782591c00ad8503e7e34d41a0a23b Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Fri, 21 Jun 2024 16:18:55 +0100 Subject: [PATCH 05/39] fix: Another permission tweak --- viewer/permissions.py | 5 ----- viewer/views.py | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index 667a7a7a..d977c3bb 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -16,11 +16,6 @@ class IsProposalMember(permissions.BasePermission): def has_object_permission(self, request, view, obj): del view - # Read permissions are allowed to any request, - # so we'll always allow GET, HEAD or OPTIONS requests. - if request.method in permissions.SAFE_METHODS: - return True - _LOGGER.info("Checking %s", repr(obj)) proposals = _ISPYB_SAFE_QUERY_SET.get_proposals_for_user(request.user) _LOGGER.info("Proposals=%s", proposals) diff --git a/viewer/views.py b/viewer/views.py index 05a2f869..d02c8e11 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -191,7 +191,7 @@ class ProjectView(ISpyBSafeQuerySet): filter_permissions = "" -class TargetView(ISpyBSafeQuerySet): +class TargetView(viewsets.ModelViewSet): """Targets (api/targets)""" queryset = models.Target.objects.filter() From 36a2992bd9051b0c95f7da066d4bd07be3a069f0 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Fri, 21 Jun 2024 16:37:18 +0100 Subject: [PATCH 06/39] fix: Another permission tweak --- viewer/permissions.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index d977c3bb..03347cb2 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -17,6 +17,9 @@ def has_object_permission(self, request, view, obj): del view _LOGGER.info("Checking %s", repr(obj)) - proposals = _ISPYB_SAFE_QUERY_SET.get_proposals_for_user(request.user) - _LOGGER.info("Proposals=%s", proposals) + _LOGGER.info("filter_permissions=%s", obj.filter_permissions) + proposals = _ISPYB_SAFE_QUERY_SET.get_proposals_for_user( + user=request.user, restrict_to_membership=True + ) + _LOGGER.info("proposals=%s", proposals) return True From a395f0189c14264ad63fe173f6aabbbb08d7cfc5 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Fri, 21 Jun 2024 16:48:55 +0100 Subject: [PATCH 07/39] fix: Typo --- viewer/permissions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index 03347cb2..bfecce20 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -17,7 +17,7 @@ def has_object_permission(self, request, view, obj): del view _LOGGER.info("Checking %s", repr(obj)) - _LOGGER.info("filter_permissions=%s", obj.filter_permissions) + _LOGGER.info("filter_permissions=%s", self.filter_permissions) proposals = _ISPYB_SAFE_QUERY_SET.get_proposals_for_user( user=request.user, restrict_to_membership=True ) From 1069392f78f4ad2d0cbb8f888d23c96699227d97 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Fri, 21 Jun 2024 16:58:50 +0100 Subject: [PATCH 08/39] fix: Fixed silly typo --- viewer/permissions.py | 1 - 1 file changed, 1 deletion(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index bfecce20..5c6d4fbb 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -17,7 +17,6 @@ def has_object_permission(self, request, view, obj): del view _LOGGER.info("Checking %s", repr(obj)) - _LOGGER.info("filter_permissions=%s", self.filter_permissions) proposals = _ISPYB_SAFE_QUERY_SET.get_proposals_for_user( user=request.user, restrict_to_membership=True ) From c6f89cd012da02652274edec29dee3d93591b36d Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Fri, 21 Jun 2024 17:09:08 +0100 Subject: [PATCH 09/39] fix: Display view filter_permissions --- viewer/permissions.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index 5c6d4fbb..1c0a6c58 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -14,9 +14,8 @@ class IsProposalMember(permissions.BasePermission): """ def has_object_permission(self, request, view, obj): - del view - _LOGGER.info("Checking %s", repr(obj)) + _LOGGER.info("view.filter_permissions=%s", view.filter_permissions) proposals = _ISPYB_SAFE_QUERY_SET.get_proposals_for_user( user=request.user, restrict_to_membership=True ) From 390d6ae8fa347c9e74fb1db06166a7826dd93b4a Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sat, 22 Jun 2024 06:37:16 +0100 Subject: [PATCH 10/39] fix: Experimental new has_permission --- viewer/permissions.py | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index 1c0a6c58..365da2cf 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -1,4 +1,5 @@ import logging +from typing import List from rest_framework import permissions @@ -14,10 +15,22 @@ class IsProposalMember(permissions.BasePermission): """ def has_object_permission(self, request, view, obj): - _LOGGER.info("Checking %s", repr(obj)) - _LOGGER.info("view.filter_permissions=%s", view.filter_permissions) - proposals = _ISPYB_SAFE_QUERY_SET.get_proposals_for_user( - user=request.user, restrict_to_membership=True - ) - _LOGGER.info("proposals=%s", proposals) - return True + # Here we check that the user has access to the proposal(s) + # the object belongs to. + # What proposals does the Object belong to? + # The object's proposal record ID is stored in the filter_permissions field. + object_proposals: List[str] = [ + p.title for p in getattr(obj, view.filter_permissions).all() + ] + _LOGGER.info("obj.proposals=%s", object_proposals) + # What proposals does the user have access to? + # We restrict the query to only those proposals + # where the user has explicit membership + # ensuring that the user is a member of an open proposal in order to access it. + user_proposals = _ISPYB_SAFE_QUERY_SET.get_proposals_for_user(user=request.user) + _LOGGER.info("user_proposals=%s", user_proposals) + # So - is any of the object's proposals in the user's list of proposals? + # Only one needs to match for permission to be granted. + permission = any(proposal in user_proposals for proposal in object_proposals) + _LOGGER.info("permission=%s", permission) + return permission From 705286749811f9490cd10c7ac0df378ee3dc5a9b Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sat, 22 Jun 2024 07:18:52 +0100 Subject: [PATCH 11/39] fix: Add user_is_member_of_any_given_proposals to security --- api/security.py | 8 ++++++++ viewer/permissions.py | 13 +++++-------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/api/security.py b/api/security.py index 7aab1b3b..1204aa15 100644 --- a/api/security.py +++ b/api/security.py @@ -327,6 +327,14 @@ def _get_proposals_from_connector(self, user, conn): ) CachedContent.set_content(user.username, prop_id_set) + def user_is_member_of_any_given_proposals(self, user, proposals): + """Returns true if the user has access to any proposal in the given + proposals list.Only one needs to match for permission to be granted. + We 'restrict_to_membership' to only consider proposals the user + has explicit membership.""" + user_proposals = self.get_proposals_for_user(user, restrict_to_membership=True) + return any(proposal in user_proposals for proposal in proposals) + def get_proposals_for_user(self, user, restrict_to_membership=False): """Returns a list of proposals that the user has access to. diff --git a/viewer/permissions.py b/viewer/permissions.py index 365da2cf..55f9a01e 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -14,6 +14,8 @@ class IsProposalMember(permissions.BasePermission): Custom permission to only allow owners of an object to edit it. """ + message: str = "Your authority to access this object has not been given" + def has_object_permission(self, request, view, obj): # Here we check that the user has access to the proposal(s) # the object belongs to. @@ -23,14 +25,9 @@ def has_object_permission(self, request, view, obj): p.title for p in getattr(obj, view.filter_permissions).all() ] _LOGGER.info("obj.proposals=%s", object_proposals) - # What proposals does the user have access to? - # We restrict the query to only those proposals - # where the user has explicit membership - # ensuring that the user is a member of an open proposal in order to access it. - user_proposals = _ISPYB_SAFE_QUERY_SET.get_proposals_for_user(user=request.user) - _LOGGER.info("user_proposals=%s", user_proposals) # So - is any of the object's proposals in the user's list of proposals? - # Only one needs to match for permission to be granted. - permission = any(proposal in user_proposals for proposal in object_proposals) + permission = _ISPYB_SAFE_QUERY_SET.user_is_member_of_any_given_proposals( + user=request.user, proposals=object_proposals + ) _LOGGER.info("permission=%s", permission) return permission From c17fac3facbed3f921809e9aee1ed46efa0879b6 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sat, 22 Jun 2024 07:20:23 +0100 Subject: [PATCH 12/39] fix: Change to filter class name --- viewer/permissions.py | 2 +- viewer/views.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index 55f9a01e..6100cb43 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -9,7 +9,7 @@ _ISPYB_SAFE_QUERY_SET: ISpyBSafeQuerySet = ISpyBSafeQuerySet() -class IsProposalMember(permissions.BasePermission): +class IsManyProposalMember(permissions.BasePermission): """ Custom permission to only allow owners of an object to edit it. """ diff --git a/viewer/views.py b/viewer/views.py index d02c8e11..2cdeb28d 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -35,7 +35,7 @@ from api.utils import get_highlighted_diffs, get_params, pretty_request from service_status.models import Service from viewer import filters, models, serializers -from viewer.permissions import IsProposalMember +from viewer.permissions import IsManyProposalMember from viewer.squonk2_agent import ( AccessParams, CommonParams, @@ -198,7 +198,7 @@ class TargetView(viewsets.ModelViewSet): serializer_class = serializers.TargetSerializer filter_permissions = "project_id" filterset_fields = ("title",) - permission_classes = [IsProposalMember] + permission_classes = [IsManyProposalMember] def patch(self, request, pk): logger.info("pk=%s", pk) From effeb12f9c85446ae89fa7da274f8f78934b503a Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sat, 22 Jun 2024 07:36:55 +0100 Subject: [PATCH 13/39] fix: Better has_object_permission --- viewer/permissions.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index 6100cb43..02acd664 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -2,6 +2,7 @@ from typing import List from rest_framework import permissions +from rest_framework.exceptions import PermissionDenied from api.security import ISpyBSafeQuerySet @@ -17,17 +18,17 @@ class IsManyProposalMember(permissions.BasePermission): message: str = "Your authority to access this object has not been given" def has_object_permission(self, request, view, obj): - # Here we check that the user has access to the proposal(s) - # the object belongs to. - # What proposals does the Object belong to? - # The object's proposal record ID is stored in the filter_permissions field. + # Here we check that the user has access to one of the proposal(s) + # the object belongs to. The object's proposal record IDs (Many) + # is stored in the reference defined in its 'filter_permissions' field. object_proposals: List[str] = [ p.title for p in getattr(obj, view.filter_permissions).all() ] - _LOGGER.info("obj.proposals=%s", object_proposals) # So - is any of the object's proposals in the user's list of proposals? - permission = _ISPYB_SAFE_QUERY_SET.user_is_member_of_any_given_proposals( + if not _ISPYB_SAFE_QUERY_SET.user_is_member_of_any_given_proposals( user=request.user, proposals=object_proposals - ) - _LOGGER.info("permission=%s", permission) - return permission + ): + raise PermissionDenied( + detail="Your authority to access this object has not been given" + ) + return True From bec00ace93ec4986aae1c16fcf53af143d288509 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sat, 22 Jun 2024 07:38:28 +0100 Subject: [PATCH 14/39] feat: Leaner has_object_permission --- viewer/permissions.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index 02acd664..be41ca8f 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -1,13 +1,9 @@ -import logging -from typing import List - from rest_framework import permissions from rest_framework.exceptions import PermissionDenied from api.security import ISpyBSafeQuerySet -_LOGGER: logging.Logger = logging.getLogger(__name__) -_ISPYB_SAFE_QUERY_SET: ISpyBSafeQuerySet = ISpyBSafeQuerySet() +_ISPYB_SAFE_QUERY_SET = ISpyBSafeQuerySet() class IsManyProposalMember(permissions.BasePermission): @@ -21,7 +17,7 @@ def has_object_permission(self, request, view, obj): # Here we check that the user has access to one of the proposal(s) # the object belongs to. The object's proposal record IDs (Many) # is stored in the reference defined in its 'filter_permissions' field. - object_proposals: List[str] = [ + object_proposals = [ p.title for p in getattr(obj, view.filter_permissions).all() ] # So - is any of the object's proposals in the user's list of proposals? From c769fe38b45a13b06197699f213ed7ac94e966e9 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sat, 22 Jun 2024 07:40:01 +0100 Subject: [PATCH 15/39] docs: Minor doc tweak --- viewer/permissions.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index be41ca8f..2f68f199 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -7,11 +7,8 @@ class IsManyProposalMember(permissions.BasePermission): - """ - Custom permission to only allow owners of an object to edit it. - """ - - message: str = "Your authority to access this object has not been given" + """Custom permission to only allow objects to be changed + by users who are members of the object's proposals.""" def has_object_permission(self, request, view, obj): # Here we check that the user has access to one of the proposal(s) From 4fb81d2a49f43aa845ca578776d8784eca4fe184 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sat, 22 Jun 2024 07:52:09 +0100 Subject: [PATCH 16/39] fix: Experiment with SafeQuerySet --- viewer/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/viewer/views.py b/viewer/views.py index 2cdeb28d..ac6960c1 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -191,7 +191,7 @@ class ProjectView(ISpyBSafeQuerySet): filter_permissions = "" -class TargetView(viewsets.ModelViewSet): +class TargetView(ISpyBSafeQuerySet): """Targets (api/targets)""" queryset = models.Target.objects.filter() From b2d0f11c8a7e1a7a990a1e8a3bfee853f4830d5e Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sat, 22 Jun 2024 08:04:10 +0100 Subject: [PATCH 17/39] fix: Experiment with ModelViewSet in ISPYB --- api/security.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/security.py b/api/security.py index 1204aa15..7cda53be 100644 --- a/api/security.py +++ b/api/security.py @@ -184,7 +184,7 @@ def ping_configured_connector() -> bool: return conn is not None -class ISpyBSafeQuerySet(viewsets.ReadOnlyModelViewSet): +class ISpyBSafeQuerySet(viewsets.ModelViewSet): def get_queryset(self): """ Optionally restricts the returned purchases to a given proposals @@ -199,7 +199,7 @@ def get_queryset(self): # Must have a foreign key to a Project for this filter to work. # get_q_filter() returns a Q expression for filtering - q_filter = self.get_q_filter(proposal_list) + q_filter = self._get_q_filter(proposal_list) return self.queryset.filter(q_filter).distinct() def _get_open_proposals(self): @@ -375,7 +375,7 @@ def get_proposals_for_user(self, user, restrict_to_membership=False): # Return the set() as a list() return list(proposals) - def get_q_filter(self, proposal_list): + def _get_q_filter(self, proposal_list): """Returns a Q expression representing a (potentially complex) table filter.""" if self.filter_permissions: # Q-filter is based on the filter_permissions string From 7b1df7d68e3c2312d634808a87a147cc07706814 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sat, 22 Jun 2024 08:18:52 +0100 Subject: [PATCH 18/39] fix: Back to read-only viewset (and reduced log) --- api/security.py | 2 +- viewer/views.py | 17 ++--------------- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/api/security.py b/api/security.py index 7cda53be..cf847a28 100644 --- a/api/security.py +++ b/api/security.py @@ -184,7 +184,7 @@ def ping_configured_connector() -> bool: return conn is not None -class ISpyBSafeQuerySet(viewsets.ModelViewSet): +class ISpyBSafeQuerySet(viewsets.ReadOnlyModelViewSet): def get_queryset(self): """ Optionally restricts the returned purchases to a given proposals diff --git a/viewer/views.py b/viewer/views.py index ac6960c1..c12a8839 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -191,7 +191,7 @@ class ProjectView(ISpyBSafeQuerySet): filter_permissions = "" -class TargetView(ISpyBSafeQuerySet): +class TargetView(viewsets.ModelViewSet): """Targets (api/targets)""" queryset = models.Target.objects.filter() @@ -201,7 +201,6 @@ class TargetView(ISpyBSafeQuerySet): permission_classes = [IsManyProposalMember] def patch(self, request, pk): - logger.info("pk=%s", pk) try: target = self.queryset.get(pk=pk) except models.Target.DoesNotExist: @@ -212,23 +211,11 @@ def patch(self, request, pk): status=status.HTTP_404_NOT_FOUND, ) - logger.info("target=%s", repr(target)) - logger.info("request.data=%s", request.data) serializer = self.serializer_class(target, data=request.data, partial=True) if serializer.is_valid(): - logger.info("serializer.validated_data=%s", serializer.validated_data) - logger.info( - "serializer.validated_datakeys=%s", serializer.validated_data.keys() - ) - logger.info( - "serializer.validated_data.display_name=%s", - serializer.validated_data.get('display_name'), - ) - new_target = serializer.save() - logger.info("new_target=%s", repr(new_target)) + _ = serializer.save() return Response(serializer.data, status=status.HTTP_200_OK) else: - logger.warning("serializer.errors=%s", serializer.errors) return Response( {"message": "wrong parameters"}, status=status.HTTP_400_BAD_REQUEST ) From 3965563b4e4d6b3959d57774539bb4143c9fc516 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sat, 22 Jun 2024 08:50:47 +0100 Subject: [PATCH 19/39] fix: User must be authenticated --- viewer/permissions.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/viewer/permissions.py b/viewer/permissions.py index 2f68f199..19a4aafc 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -11,6 +11,9 @@ class IsManyProposalMember(permissions.BasePermission): by users who are members of the object's proposals.""" def has_object_permission(self, request, view, obj): + # Users must be authenticated + if not request.user.is_authenticated: + return False # Here we check that the user has access to one of the proposal(s) # the object belongs to. The object's proposal record IDs (Many) # is stored in the reference defined in its 'filter_permissions' field. From 7cf186960f7f9c68a3992dff75a4109d18743d7d Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Sun, 23 Jun 2024 19:39:21 +0100 Subject: [PATCH 20/39] docs: Doc tweak --- viewer/permissions.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index 19a4aafc..6dd1e98d 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -14,17 +14,18 @@ def has_object_permission(self, request, view, obj): # Users must be authenticated if not request.user.is_authenticated: return False - # Here we check that the user has access to one of the proposal(s) - # the object belongs to. The object's proposal record IDs (Many) - # is stored in the reference defined in its 'filter_permissions' field. + # Here we check that the user has access to any proposal the object belongs to. + # The object's proposal records (many in this case) are stored in the reference + # defined in the view's 'filter_permissions' property, e.g. 'project_id'. object_proposals = [ p.title for p in getattr(obj, view.filter_permissions).all() ] - # So - is any of the object's proposals in the user's list of proposals? + # So - has the user been associated with any of the proposals for this object? if not _ISPYB_SAFE_QUERY_SET.user_is_member_of_any_given_proposals( user=request.user, proposals=object_proposals ): raise PermissionDenied( detail="Your authority to access this object has not been given" ) + # User is a member of at least one of the object's proposals... return True From 9b2fc8eaffb862e9346c27b598027c4dd22564cf Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 10:12:16 +0100 Subject: [PATCH 21/39] feat: Experiment with filter class for Target view --- viewer/filters.py | 11 +++++++++++ viewer/views.py | 3 ++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/viewer/filters.py b/viewer/filters.py index d5e62e03..0720e8af 100644 --- a/viewer/filters.py +++ b/viewer/filters.py @@ -1,6 +1,7 @@ import django_filters from django_filters import rest_framework as filters +from api.security import ISpyBSafeQuerySet from viewer.models import ( CanonSite, CanonSiteConf, @@ -12,6 +13,8 @@ XtalformSite, ) +_ISPYB_SAFE_QUERY_SET = ISpyBSafeQuerySet() + class SnapshotFilter(filters.FilterSet): session_project = django_filters.CharFilter( @@ -114,3 +117,11 @@ class AssemblyFilter(TargetFilterMixin): class Meta: model = QuatAssembly fields = ("target",) + + +class SecurityFilter(filters.BaseFilterBackend): + def filter_queryset(self, request, queryset, view): + print("SecurityFilter - filter_queryset %s %s %s", request, queryset, view) + qs = _ISPYB_SAFE_QUERY_SET.get_queryset() + print(f"SecurityFilter - got {len(qs)}") + return qs diff --git a/viewer/views.py b/viewer/views.py index c12a8839..763b18eb 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -194,9 +194,10 @@ class ProjectView(ISpyBSafeQuerySet): class TargetView(viewsets.ModelViewSet): """Targets (api/targets)""" - queryset = models.Target.objects.filter() + # queryset = models.Target.objects.filter() serializer_class = serializers.TargetSerializer filter_permissions = "project_id" + filter_class = filters.SecurityFilter filterset_fields = ("title",) permission_classes = [IsManyProposalMember] From f51be1ef6bd2bcdd11a9c5a6650ae228e5880623 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 10:39:22 +0100 Subject: [PATCH 22/39] fix: Attempt to fix build errors --- viewer/filters.py | 3 ++- viewer/views.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/viewer/filters.py b/viewer/filters.py index 0720e8af..d410236b 100644 --- a/viewer/filters.py +++ b/viewer/filters.py @@ -1,5 +1,6 @@ import django_filters from django_filters import rest_framework as filters +from rest_framework.filters import BaseFilterBackend from api.security import ISpyBSafeQuerySet from viewer.models import ( @@ -119,7 +120,7 @@ class Meta: fields = ("target",) -class SecurityFilter(filters.BaseFilterBackend): +class SecurityFilter(BaseFilterBackend): def filter_queryset(self, request, queryset, view): print("SecurityFilter - filter_queryset %s %s %s", request, queryset, view) qs = _ISPYB_SAFE_QUERY_SET.get_queryset() diff --git a/viewer/views.py b/viewer/views.py index 763b18eb..e74b93e3 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -197,7 +197,7 @@ class TargetView(viewsets.ModelViewSet): # queryset = models.Target.objects.filter() serializer_class = serializers.TargetSerializer filter_permissions = "project_id" - filter_class = filters.SecurityFilter + filterset_class = filters.SecurityFilter filterset_fields = ("title",) permission_classes = [IsManyProposalMember] From 81da348fe7bd1ab2d4e8e931ca5ed0e6e7400414 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 10:49:41 +0100 Subject: [PATCH 23/39] fix: Switch to filter_class --- viewer/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/viewer/views.py b/viewer/views.py index e74b93e3..763b18eb 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -197,7 +197,7 @@ class TargetView(viewsets.ModelViewSet): # queryset = models.Target.objects.filter() serializer_class = serializers.TargetSerializer filter_permissions = "project_id" - filterset_class = filters.SecurityFilter + filter_class = filters.SecurityFilter filterset_fields = ("title",) permission_classes = [IsManyProposalMember] From 21546916ce77a9c1e24062de8b5b7a1d8ac37714 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 11:20:50 +0100 Subject: [PATCH 24/39] fix: Attempt to fix filter logging --- viewer/filters.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/viewer/filters.py b/viewer/filters.py index d410236b..c576d2f7 100644 --- a/viewer/filters.py +++ b/viewer/filters.py @@ -1,3 +1,5 @@ +import logging + import django_filters from django_filters import rest_framework as filters from rest_framework.filters import BaseFilterBackend @@ -14,6 +16,8 @@ XtalformSite, ) +logger = logging.getLogger(__name__) + _ISPYB_SAFE_QUERY_SET = ISpyBSafeQuerySet() @@ -122,7 +126,9 @@ class Meta: class SecurityFilter(BaseFilterBackend): def filter_queryset(self, request, queryset, view): - print("SecurityFilter - filter_queryset %s %s %s", request, queryset, view) + logger.info( + "SecurityFilter - filter_queryset %s %s %s", request, queryset, view + ) qs = _ISPYB_SAFE_QUERY_SET.get_queryset() - print(f"SecurityFilter - got {len(qs)}") + logger.info("SecurityFilter - got %s", len(qs)) return qs From 0329202fb78b6fc0840ec56ec218700bb11f31cf Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 11:40:23 +0100 Subject: [PATCH 25/39] fix: Another tweak of filters --- viewer/filters.py | 13 ++++++------- viewer/views.py | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/viewer/filters.py b/viewer/filters.py index c576d2f7..9908d5e7 100644 --- a/viewer/filters.py +++ b/viewer/filters.py @@ -125,10 +125,9 @@ class Meta: class SecurityFilter(BaseFilterBackend): - def filter_queryset(self, request, queryset, view): - logger.info( - "SecurityFilter - filter_queryset %s %s %s", request, queryset, view - ) - qs = _ISPYB_SAFE_QUERY_SET.get_queryset() - logger.info("SecurityFilter - got %s", len(qs)) - return qs + @property + def qs(self): + logger.info("SecurityFilter - request=%s", self.request) + query_set = _ISPYB_SAFE_QUERY_SET.get_queryset() + logger.info("SecurityFilter - len(query_set)=%s", len(query_set)) + return query_set diff --git a/viewer/views.py b/viewer/views.py index 763b18eb..e74b93e3 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -197,7 +197,7 @@ class TargetView(viewsets.ModelViewSet): # queryset = models.Target.objects.filter() serializer_class = serializers.TargetSerializer filter_permissions = "project_id" - filter_class = filters.SecurityFilter + filterset_class = filters.SecurityFilter filterset_fields = ("title",) permission_classes = [IsManyProposalMember] From 82787dec28f86b586d328d2fae23822f408e0a9e Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 11:46:07 +0100 Subject: [PATCH 26/39] feat: Fix filerset typo --- viewer/filters.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/viewer/filters.py b/viewer/filters.py index 9908d5e7..5f9a8911 100644 --- a/viewer/filters.py +++ b/viewer/filters.py @@ -2,7 +2,6 @@ import django_filters from django_filters import rest_framework as filters -from rest_framework.filters import BaseFilterBackend from api.security import ISpyBSafeQuerySet from viewer.models import ( @@ -124,7 +123,7 @@ class Meta: fields = ("target",) -class SecurityFilter(BaseFilterBackend): +class SecurityFilter(django_filters.FilterSet): @property def qs(self): logger.info("SecurityFilter - request=%s", self.request) From b9c49f3c3a8fa01794300085185f418ea7b9cfb7 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 12:12:02 +0100 Subject: [PATCH 27/39] fix: Back to built-in (genric) views --- viewer/filters.py | 12 ------------ viewer/views.py | 12 ++++++------ 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/viewer/filters.py b/viewer/filters.py index 5f9a8911..d0df524a 100644 --- a/viewer/filters.py +++ b/viewer/filters.py @@ -3,7 +3,6 @@ import django_filters from django_filters import rest_framework as filters -from api.security import ISpyBSafeQuerySet from viewer.models import ( CanonSite, CanonSiteConf, @@ -17,8 +16,6 @@ logger = logging.getLogger(__name__) -_ISPYB_SAFE_QUERY_SET = ISpyBSafeQuerySet() - class SnapshotFilter(filters.FilterSet): session_project = django_filters.CharFilter( @@ -121,12 +118,3 @@ class AssemblyFilter(TargetFilterMixin): class Meta: model = QuatAssembly fields = ("target",) - - -class SecurityFilter(django_filters.FilterSet): - @property - def qs(self): - logger.info("SecurityFilter - request=%s", self.request) - query_set = _ISPYB_SAFE_QUERY_SET.get_queryset() - logger.info("SecurityFilter - len(query_set)=%s", len(query_set)) - return query_set diff --git a/viewer/views.py b/viewer/views.py index e74b93e3..e3e57549 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -24,7 +24,7 @@ from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse from django.views import View -from rest_framework import permissions, status, viewsets +from rest_framework import generics, permissions, status, viewsets from rest_framework.exceptions import ParseError from rest_framework.parsers import BaseParser from rest_framework.response import Response @@ -83,6 +83,7 @@ _SESSION_MESSAGE = 'session_message' _SQ2A: Squonk2Agent = get_squonk2_agent() +_ISPYB_SAFE_QUERY_SET: ISpyBSafeQuerySet = ISpyBSafeQuerySet() class CompoundIdentifierTypeView(viewsets.ModelViewSet): @@ -191,16 +192,15 @@ class ProjectView(ISpyBSafeQuerySet): filter_permissions = "" -class TargetView(viewsets.ModelViewSet): - """Targets (api/targets)""" - - # queryset = models.Target.objects.filter() +class TargetView(generics.RetrieveUpdateAPIView): serializer_class = serializers.TargetSerializer filter_permissions = "project_id" - filterset_class = filters.SecurityFilter filterset_fields = ("title",) permission_classes = [IsManyProposalMember] + def get_queryset(self): + return _ISPYB_SAFE_QUERY_SET.get_queryset() + def patch(self, request, pk): try: target = self.queryset.get(pk=pk) From 990972177cb8346f0916304def4f2b78991a5133 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 12:21:00 +0100 Subject: [PATCH 28/39] feat: Attempt to fix has no attribute 'get_extra_actions' --- api/urls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/urls.py b/api/urls.py index 8f4d38ea..8481ff25 100644 --- a/api/urls.py +++ b/api/urls.py @@ -13,7 +13,6 @@ router = DefaultRouter() # Register the basic data router.register("compounds", viewer_views.CompoundView, "compounds") -router.register("targets", viewer_views.TargetView, "targets") router.register("projects", viewer_views.ProjectView) router.register("session-projects", viewer_views.SessionProjectsView) router.register("snapshots", viewer_views.SnapshotsView) @@ -151,4 +150,5 @@ def schema_view(request): path("auth/", drf_views.obtain_auth_token, name="auth"), path("swagger/", schema_view), path("job_request/", viewer_views.JobRequestView.as_view(), name="job_request"), + path("targets/", viewer_views.TargetView.as_view(), "targets"), ] From 19a49205af3f179eb13323bfa40ea1164eeb15f2 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 12:30:22 +0100 Subject: [PATCH 29/39] fix: Silly typo --- api/urls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/urls.py b/api/urls.py index 8481ff25..b212889e 100644 --- a/api/urls.py +++ b/api/urls.py @@ -150,5 +150,5 @@ def schema_view(request): path("auth/", drf_views.obtain_auth_token, name="auth"), path("swagger/", schema_view), path("job_request/", viewer_views.JobRequestView.as_view(), name="job_request"), - path("targets/", viewer_views.TargetView.as_view(), "targets"), + path("targets/", viewer_views.TargetView.as_view(), name="targets"), ] From 9f53091a7d63828ccaedb5e7f23e750e0af8dacb Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 12:52:13 +0100 Subject: [PATCH 30/39] fix: Back to ispybsafequeryset --- api/urls.py | 2 +- viewer/views.py | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/api/urls.py b/api/urls.py index b212889e..8f4d38ea 100644 --- a/api/urls.py +++ b/api/urls.py @@ -13,6 +13,7 @@ router = DefaultRouter() # Register the basic data router.register("compounds", viewer_views.CompoundView, "compounds") +router.register("targets", viewer_views.TargetView, "targets") router.register("projects", viewer_views.ProjectView) router.register("session-projects", viewer_views.SessionProjectsView) router.register("snapshots", viewer_views.SnapshotsView) @@ -150,5 +151,4 @@ def schema_view(request): path("auth/", drf_views.obtain_auth_token, name="auth"), path("swagger/", schema_view), path("job_request/", viewer_views.JobRequestView.as_view(), name="job_request"), - path("targets/", viewer_views.TargetView.as_view(), name="targets"), ] diff --git a/viewer/views.py b/viewer/views.py index e3e57549..3a7f9591 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -24,7 +24,7 @@ from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse from django.views import View -from rest_framework import generics, permissions, status, viewsets +from rest_framework import permissions, status, viewsets from rest_framework.exceptions import ParseError from rest_framework.parsers import BaseParser from rest_framework.response import Response @@ -83,7 +83,6 @@ _SESSION_MESSAGE = 'session_message' _SQ2A: Squonk2Agent = get_squonk2_agent() -_ISPYB_SAFE_QUERY_SET: ISpyBSafeQuerySet = ISpyBSafeQuerySet() class CompoundIdentifierTypeView(viewsets.ModelViewSet): @@ -192,15 +191,13 @@ class ProjectView(ISpyBSafeQuerySet): filter_permissions = "" -class TargetView(generics.RetrieveUpdateAPIView): +class TargetView(ISpyBSafeQuerySet): + # queryset = models.Target.objects.filter() serializer_class = serializers.TargetSerializer filter_permissions = "project_id" filterset_fields = ("title",) permission_classes = [IsManyProposalMember] - def get_queryset(self): - return _ISPYB_SAFE_QUERY_SET.get_queryset() - def patch(self, request, pk): try: target = self.queryset.get(pk=pk) From f33dc60bb70d018eaf0d2ad567a6a4aa680e9f98 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 14:02:41 +0100 Subject: [PATCH 31/39] fix: Restore queryset --- viewer/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/viewer/views.py b/viewer/views.py index 3a7f9591..dc0cf655 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -192,7 +192,7 @@ class ProjectView(ISpyBSafeQuerySet): class TargetView(ISpyBSafeQuerySet): - # queryset = models.Target.objects.filter() + queryset = models.Target.objects.filter() serializer_class = serializers.TargetSerializer filter_permissions = "project_id" filterset_fields = ("title",) From 02171180d8289e3465a7d2824e17472bdd841873 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 14:31:21 +0100 Subject: [PATCH 32/39] fix: Experiment with mixins --- viewer/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/viewer/views.py b/viewer/views.py index dc0cf655..4854d8c9 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -24,7 +24,7 @@ from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse from django.views import View -from rest_framework import permissions, status, viewsets +from rest_framework import mixins, permissions, status, viewsets from rest_framework.exceptions import ParseError from rest_framework.parsers import BaseParser from rest_framework.response import Response @@ -191,7 +191,7 @@ class ProjectView(ISpyBSafeQuerySet): filter_permissions = "" -class TargetView(ISpyBSafeQuerySet): +class TargetView(mixins.UpdateModelMixin, ISpyBSafeQuerySet): queryset = models.Target.objects.filter() serializer_class = serializers.TargetSerializer filter_permissions = "project_id" From 4528006ea70a28e122a82622a117ec7a027299a6 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 15:58:03 +0100 Subject: [PATCH 33/39] docs: Doc tweaks --- api/security.py | 19 ++++++++++++++++--- viewer/permissions.py | 10 +++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/api/security.py b/api/security.py index cf847a28..cefa0b8c 100644 --- a/api/security.py +++ b/api/security.py @@ -185,9 +185,22 @@ def ping_configured_connector() -> bool: class ISpyBSafeQuerySet(viewsets.ReadOnlyModelViewSet): + """ + This ISpyBSafeQuerySet, which inherits from the DRF viewsets.ReadOnlyModelViewSet, + is used for all views that need to yield (filter) view objects based on a + user's proposal membership. This requires the view to define the property + "filter_permissions" to enable this class to navigate to the view object's Project + (proposal/visit). + + As the ISpyBSafeQuerySet is based on a ReadOnlyModelViewSet, which only provides + implementations for list() and retrieve() methods, the user will need to provide + "mixins" for any additional methods the view needs to support (PATCH, PUT, DELETE). + """ + def get_queryset(self): """ - Optionally restricts the returned purchases to a given proposals + Restricts the returned records to those that belong to proposals + the user has access to. Without a user only 'open' proposals are returned. """ # The list of proposals this user can have proposal_list = self.get_proposals_for_user(self.request.user) @@ -267,8 +280,8 @@ def _get_proposals_for_user_from_ispyb(self, user): return cached_prop_ids def _get_proposals_from_connector(self, user, conn): - """Updates the USER_LIST_DICT with the results of a query - and marks it as populated. + """ + Updates the user's proposal cache with the results of a query """ assert user assert conn diff --git a/viewer/permissions.py b/viewer/permissions.py index 6dd1e98d..39868266 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -7,8 +7,12 @@ class IsManyProposalMember(permissions.BasePermission): - """Custom permission to only allow objects to be changed - by users who are members of the object's proposals.""" + """ + Custom permission to only allow objects to be changed by users + who are members of the object's proposals. This permission class should be used + in any view that need to restrict object modifications to users who are members of + at least one of the object's proposals. + """ def has_object_permission(self, request, view, obj): # Users must be authenticated @@ -20,7 +24,7 @@ def has_object_permission(self, request, view, obj): object_proposals = [ p.title for p in getattr(obj, view.filter_permissions).all() ] - # So - has the user been associated with any of the proposals for this object? + # Has the user been associated (in IPSpyB) with any of the object's proposals? if not _ISPYB_SAFE_QUERY_SET.user_is_member_of_any_given_proposals( user=request.user, proposals=object_proposals ): From d9ad42ba39ed3e8af47ac15878f1ed703f83b285 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 16:12:12 +0100 Subject: [PATCH 34/39] docs: Doc tweak --- api/security.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/api/security.py b/api/security.py index cefa0b8c..9c95d86f 100644 --- a/api/security.py +++ b/api/security.py @@ -25,7 +25,8 @@ @cache class CachedContent: - """A static class managing caches proposals/visits for each user. + """ + A static class managing caches proposals/visits for each user. Proposals should be collected when has_expired() returns True. Content can be written (when the cache for the user has expired) and read using the set/get methods. @@ -341,15 +342,18 @@ def _get_proposals_from_connector(self, user, conn): CachedContent.set_content(user.username, prop_id_set) def user_is_member_of_any_given_proposals(self, user, proposals): - """Returns true if the user has access to any proposal in the given + """ + Returns true if the user has access to any proposal in the given proposals list.Only one needs to match for permission to be granted. We 'restrict_to_membership' to only consider proposals the user - has explicit membership.""" + has explicit membership. + """ user_proposals = self.get_proposals_for_user(user, restrict_to_membership=True) return any(proposal in user_proposals for proposal in proposals) def get_proposals_for_user(self, user, restrict_to_membership=False): - """Returns a list of proposals that the user has access to. + """ + 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' From f1ef712830787d3a66dd5902c635edc93f4a347e Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 16:14:00 +0100 Subject: [PATCH 35/39] fix: Switch to update() from patch() --- viewer/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/viewer/views.py b/viewer/views.py index 4854d8c9..4e45e3fc 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -198,7 +198,7 @@ class TargetView(mixins.UpdateModelMixin, ISpyBSafeQuerySet): filterset_fields = ("title",) permission_classes = [IsManyProposalMember] - def patch(self, request, pk): + def update(self, request, pk): try: target = self.queryset.get(pk=pk) except models.Target.DoesNotExist: From 2f938665023b20d0ea0d3c62dbe177c04bf33ea5 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 16:44:29 +0100 Subject: [PATCH 36/39] fix: Back to patch() --- viewer/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/viewer/views.py b/viewer/views.py index 4e45e3fc..4854d8c9 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -198,7 +198,7 @@ class TargetView(mixins.UpdateModelMixin, ISpyBSafeQuerySet): filterset_fields = ("title",) permission_classes = [IsManyProposalMember] - def update(self, request, pk): + def patch(self, request, pk): try: target = self.queryset.get(pk=pk) except models.Target.DoesNotExist: From da1a4bb63e1078d717577973690a7e11785f5909 Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Mon, 24 Jun 2024 17:03:32 +0100 Subject: [PATCH 37/39] refactor: Minor refactor --- viewer/views.py | 206 ++++++++++++++++++++++++------------------------ 1 file changed, 103 insertions(+), 103 deletions(-) diff --git a/viewer/views.py b/viewer/views.py index 4854d8c9..37baae69 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -85,6 +85,109 @@ _SQ2A: Squonk2Agent = get_squonk2_agent() +def react(request): + """We "START HERE". This is the first API call that the front-end calls.""" + + discourse_api_key = settings.DISCOURSE_API_KEY + + context = {} + + # Legacy URL (a n optional prior stack) + # May be blank ('') + context['legacy_url'] = settings.LEGACY_URL + + # Is the Squonk2 Agent configured? + logger.info("Checking whether Squonk2 is configured...") + sq2_rv = _SQ2A.configured() + if sq2_rv.success: + logger.info("Squonk2 is configured") + context['squonk_available'] = 'true' + else: + logger.info("Squonk2 is NOT configured") + context['squonk_available'] = 'false' + + if discourse_api_key: + context['discourse_available'] = 'true' + else: + context['discourse_available'] = 'false' + + user = request.user + if user.is_authenticated: + context['discourse_host'] = '' + context['user_present_on_discourse'] = 'false' + # If user is authenticated and a discourse api key is available, then check discourse to + # see if user is set up and set up flag in context. + if discourse_api_key: + context['discourse_host'] = settings.DISCOURSE_HOST + _, _, user_id = check_discourse_user(user) + if user_id: + context['user_present_on_discourse'] = 'true' + + # If user is authenticated Squonk can be called then return the Squonk host + # so the Frontend can navigate to it + context['squonk_ui_url'] = '' + if sq2_rv.success and check_squonk_active(request): + context['squonk_ui_url'] = _SQ2A.get_ui_url() + + return render(request, "viewer/react_temp.html", context) + + +def save_pdb_zip(pdb_file): + zf = zipfile.ZipFile(pdb_file) + zip_lst = zf.namelist() + zfile = {} + zfile_hashvals: Dict[str, str] = {} + print(zip_lst) + for filename in zip_lst: + # only handle pdb files + if filename.split('.')[-1] == 'pdb': + f = filename.split('/')[0] + save_path = os.path.join(settings.MEDIA_ROOT, 'tmp', f) + if default_storage.exists(f): + rand_str = uuid.uuid4().hex + pdb_path = default_storage.save( + save_path.replace('.pdb', f'-{rand_str}.pdb'), + ContentFile(zf.read(filename)), + ) + # Test if Protein object already exists + # code = filename.split('/')[-1].replace('.pdb', '') + # test_pdb_code = filename.split('/')[-1].replace('.pdb', '') + # test_prot_objs = Protein.objects.filter(code=test_pdb_code) + # + # if len(test_prot_objs) > 0: + # # make a unique pdb code as not to overwrite existing object + # rand_str = uuid.uuid4().hex + # test_pdb_code = f'{code}#{rand_str}' + # zfile_hashvals[code] = rand_str + # + # fn = test_pdb_code + '.pdb' + # + # pdb_path = default_storage.save('tmp/' + fn, + # ContentFile(zf.read(filename))) + else: + pdb_path = default_storage.save( + save_path, ContentFile(zf.read(filename)) + ) + test_pdb_code = pdb_path.split('/')[-1].replace('.pdb', '') + zfile[test_pdb_code] = pdb_path + + # Close the zip file + if zf: + zf.close() + + return zfile, zfile_hashvals + + +def save_tmp_file(myfile): + """Save file in temporary location for validation/upload processing""" + + name = myfile.name + path = default_storage.save('tmp/' + name, ContentFile(myfile.read())) + tmp_file = str(os.path.join(settings.MEDIA_ROOT, path)) + + return tmp_file + + class CompoundIdentifierTypeView(viewsets.ModelViewSet): queryset = models.CompoundIdentifierType.objects.all() serializer_class = serializers.CompoundIdentifierTypeSerializer @@ -228,109 +331,6 @@ class CompoundView(ISpyBSafeQuerySet): filterset_class = filters.CompoundFilter -def react(request): - """We "START HERE". This is the first API call that the front-end calls.""" - - discourse_api_key = settings.DISCOURSE_API_KEY - - context = {} - - # Legacy URL (a n optional prior stack) - # May be blank ('') - context['legacy_url'] = settings.LEGACY_URL - - # Is the Squonk2 Agent configured? - logger.info("Checking whether Squonk2 is configured...") - sq2_rv = _SQ2A.configured() - if sq2_rv.success: - logger.info("Squonk2 is configured") - context['squonk_available'] = 'true' - else: - logger.info("Squonk2 is NOT configured") - context['squonk_available'] = 'false' - - if discourse_api_key: - context['discourse_available'] = 'true' - else: - context['discourse_available'] = 'false' - - user = request.user - if user.is_authenticated: - context['discourse_host'] = '' - context['user_present_on_discourse'] = 'false' - # If user is authenticated and a discourse api key is available, then check discourse to - # see if user is set up and set up flag in context. - if discourse_api_key: - context['discourse_host'] = settings.DISCOURSE_HOST - _, _, user_id = check_discourse_user(user) - if user_id: - context['user_present_on_discourse'] = 'true' - - # If user is authenticated Squonk can be called then return the Squonk host - # so the Frontend can navigate to it - context['squonk_ui_url'] = '' - if sq2_rv.success and check_squonk_active(request): - context['squonk_ui_url'] = _SQ2A.get_ui_url() - - return render(request, "viewer/react_temp.html", context) - - -def save_pdb_zip(pdb_file): - zf = zipfile.ZipFile(pdb_file) - zip_lst = zf.namelist() - zfile = {} - zfile_hashvals: Dict[str, str] = {} - print(zip_lst) - for filename in zip_lst: - # only handle pdb files - if filename.split('.')[-1] == 'pdb': - f = filename.split('/')[0] - save_path = os.path.join(settings.MEDIA_ROOT, 'tmp', f) - if default_storage.exists(f): - rand_str = uuid.uuid4().hex - pdb_path = default_storage.save( - save_path.replace('.pdb', f'-{rand_str}.pdb'), - ContentFile(zf.read(filename)), - ) - # Test if Protein object already exists - # code = filename.split('/')[-1].replace('.pdb', '') - # test_pdb_code = filename.split('/')[-1].replace('.pdb', '') - # test_prot_objs = Protein.objects.filter(code=test_pdb_code) - # - # if len(test_prot_objs) > 0: - # # make a unique pdb code as not to overwrite existing object - # rand_str = uuid.uuid4().hex - # test_pdb_code = f'{code}#{rand_str}' - # zfile_hashvals[code] = rand_str - # - # fn = test_pdb_code + '.pdb' - # - # pdb_path = default_storage.save('tmp/' + fn, - # ContentFile(zf.read(filename))) - else: - pdb_path = default_storage.save( - save_path, ContentFile(zf.read(filename)) - ) - test_pdb_code = pdb_path.split('/')[-1].replace('.pdb', '') - zfile[test_pdb_code] = pdb_path - - # Close the zip file - if zf: - zf.close() - - return zfile, zfile_hashvals - - -def save_tmp_file(myfile): - """Save file in temporary location for validation/upload processing""" - - name = myfile.name - path = default_storage.save('tmp/' + name, ContentFile(myfile.read())) - tmp_file = str(os.path.join(settings.MEDIA_ROOT, path)) - - return tmp_file - - class UploadCSet(APIView): """Render and control viewer/upload-cset.html - a page allowing upload of computed sets. Validation and upload tasks are defined in `viewer.compound_set_upload`, `viewer.sdf_check` and `viewer.tasks` and the task From 23fcb97b63ccc23897ffca491f9d34a7149b512a Mon Sep 17 00:00:00 2001 From: Alan Christie Date: Tue, 25 Jun 2024 10:18:58 +0100 Subject: [PATCH 38/39] feat: Better permissions class for proposals --- viewer/permissions.py | 49 ++++++++++++++++++++++++++++++------------- viewer/views.py | 31 +++++++++++++-------------- 2 files changed, 48 insertions(+), 32 deletions(-) diff --git a/viewer/permissions.py b/viewer/permissions.py index 39868266..95c975e6 100644 --- a/viewer/permissions.py +++ b/viewer/permissions.py @@ -6,27 +6,46 @@ _ISPYB_SAFE_QUERY_SET = ISpyBSafeQuerySet() -class IsManyProposalMember(permissions.BasePermission): +class IsObjectProposalMember(permissions.BasePermission): """ - Custom permission to only allow objects to be changed by users - who are members of the object's proposals. This permission class should be used - in any view that need to restrict object modifications to users who are members of - at least one of the object's proposals. + Custom permissions to only allow write-access to objects (changes) by users + who are members of the object's proposals. This permissions class should be used + in any view that needs to restrict object modifications to users who are members of + at least one of the object's proposals. This class can be used for objects that + either have one proposal or many. + + If the object has no proposals, the user is granted access. """ def has_object_permission(self, request, view, obj): - # Users must be authenticated + # Here we check that the user has access to any proposal the object belongs to. + # Firstly, users must be authenticated if not request.user.is_authenticated: return False - # Here we check that the user has access to any proposal the object belongs to. - # The object's proposal records (many in this case) are stored in the reference - # defined in the view's 'filter_permissions' property, e.g. 'project_id'. - object_proposals = [ - p.title for p in getattr(obj, view.filter_permissions).all() - ] - # Has the user been associated (in IPSpyB) with any of the object's proposals? - if not _ISPYB_SAFE_QUERY_SET.user_is_member_of_any_given_proposals( - user=request.user, proposals=object_proposals + # Protect ourselves from views that do not (oddly) + # have a property called 'filter_permissions'... + if not hasattr(view, "filter_permissions"): + raise AttributeError( + "The view object must define a 'filter_permissions' property" + ) + # The object's proposal records (one or many) can be obtained via + # the view's 'filter_permissions' property. A standard + # django property reference, e.g. 'target__project_id'. + object_proposals = [] + attr_value = getattr(obj, view.filter_permissions) + if attr_value.__class__.__name__ == "ManyRelatedManager": + # Potential for many proposals... + object_proposals = [p.title for p in attr_value.all()] + else: + # Only one proposal... + object_proposals = [attr_value.title] + # Now we have the proposals the object belongs to + # has the user been associated (in IPSpyB) with any of them? + if ( + object_proposals + and not _ISPYB_SAFE_QUERY_SET.user_is_member_of_any_given_proposals( + user=request.user, proposals=object_proposals + ) ): raise PermissionDenied( detail="Your authority to access this object has not been given" diff --git a/viewer/views.py b/viewer/views.py index 37baae69..cb8e7fdf 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -35,7 +35,7 @@ from api.utils import get_highlighted_diffs, get_params, pretty_request from service_status.models import Service from viewer import filters, models, serializers -from viewer.permissions import IsManyProposalMember +from viewer.permissions import IsObjectProposalMember from viewer.squonk2_agent import ( AccessParams, CommonParams, @@ -84,17 +84,18 @@ _SQ2A: Squonk2Agent = get_squonk2_agent() +# --------------------------- +# ENTRYPOINT FOR THE FRONTEND +# --------------------------- + def react(request): """We "START HERE". This is the first API call that the front-end calls.""" discourse_api_key = settings.DISCOURSE_API_KEY - context = {} - - # Legacy URL (a n optional prior stack) - # May be blank ('') - context['legacy_url'] = settings.LEGACY_URL + # Start building the context that will be passed to the template + context = {'legacy_url': settings.LEGACY_URL} # Is the Squonk2 Agent configured? logger.info("Checking whether Squonk2 is configured...") @@ -106,11 +107,7 @@ def react(request): logger.info("Squonk2 is NOT configured") context['squonk_available'] = 'false' - if discourse_api_key: - context['discourse_available'] = 'true' - else: - context['discourse_available'] = 'false' - + context['discourse_available'] = 'true' if discourse_api_key else 'false' user = request.user if user.is_authenticated: context['discourse_host'] = '' @@ -123,8 +120,9 @@ def react(request): if user_id: context['user_present_on_discourse'] = 'true' - # If user is authenticated Squonk can be called then return the Squonk host - # so the Frontend can navigate to it + # User is authenticated, so if Squonk can be called + # return the Squonk UI URL + # so the f/e knows where to go. context['squonk_ui_url'] = '' if sq2_rv.success and check_squonk_active(request): context['squonk_ui_url'] = _SQ2A.get_ui_url() @@ -299,7 +297,7 @@ class TargetView(mixins.UpdateModelMixin, ISpyBSafeQuerySet): serializer_class = serializers.TargetSerializer filter_permissions = "project_id" filterset_fields = ("title",) - permission_classes = [IsManyProposalMember] + permission_classes = [IsObjectProposalMember] def patch(self, request, pk): try: @@ -987,6 +985,8 @@ class SessionProjectsView(viewsets.ModelViewSet): """ queryset = models.SessionProject.objects.filter() + filter_permissions = "target__project_id" + filterset_fields = '__all__' def get_serializer_class(self): """Determine which serializer to use based on whether the request is a GET or a POST, PUT or PATCH request @@ -1003,9 +1003,6 @@ def get_serializer_class(self): # (POST, PUT, PATCH) return serializers.SessionProjectWriteSerializer - filter_permissions = "target_id__project_id" - filterset_fields = '__all__' - class SessionActionsView(viewsets.ModelViewSet): """View to retrieve information about actions relating to sessions_project (GET). From ba8138511e153cf1dd2a601c40ea7e7cd298f80f Mon Sep 17 00:00:00 2001 From: "Alan B. Christie" <29806285+alanbchristie@users.noreply.github.com> Date: Tue, 25 Jun 2024 10:36:30 +0100 Subject: [PATCH 39/39] Align 1247 with latest staging (#611) * feat: incremental RHS upload (issue 1394) Allows upload. Currently adds unnecessary compound sets and overwrites old ones when it doesn't need to * feat: incremental RHS uploads Fully working, upload, delete, overwrite, etc. NB! migrations! * fix: code cleanup for merge --------- Co-authored-by: Kalev Takkis --- viewer/cset_upload.py | 267 ++++++++++++------- viewer/migrations/0056_compound_inchi_key.py | 18 ++ viewer/migrations/0057_auto_20240612_1348.py | 23 ++ viewer/migrations/0058_auto_20240614_1016.py | 35 +++ viewer/models.py | 27 +- viewer/target_loader.py | 28 +- viewer/tasks.py | 3 + viewer/utils.py | 27 +- viewer/views.py | 28 +- 9 files changed, 325 insertions(+), 131 deletions(-) create mode 100644 viewer/migrations/0056_compound_inchi_key.py create mode 100644 viewer/migrations/0057_auto_20240612_1348.py create mode 100644 viewer/migrations/0058_auto_20240614_1016.py diff --git a/viewer/cset_upload.py b/viewer/cset_upload.py index 0ccf7555..5db2ef7a 100644 --- a/viewer/cset_upload.py +++ b/viewer/cset_upload.py @@ -7,20 +7,21 @@ from pathlib import Path from typing import Any, Dict, List, Optional, Tuple +import numpy as np from openpyxl.utils import get_column_letter os.environ.setdefault("DJANGO_SETTINGS_MODULE", "fragalysis.settings") import django django.setup() -from django.conf import settings - -logger = logging.getLogger(__name__) +from django.conf import settings +from django.core.exceptions import MultipleObjectsReturned from django.core.files.base import ContentFile from django.core.files.storage import default_storage +from django.db.models import F, TextField, Value +from django.db.models.expressions import Func from rdkit import Chem -from rdkit.Chem import Crippen, Descriptors from viewer.models import ( Compound, @@ -34,7 +35,13 @@ TextScoreValues, User, ) -from viewer.utils import add_props_to_sdf_molecule, is_url, word_count +from viewer.utils import add_props_to_sdf_molecule, alphanumerator, is_url, word_count + +logger = logging.getLogger(__name__) + + +# maximum distance between corresponding atoms in poses +_DIST_LIMIT = 0.5 def dataType(a_str: str) -> str: @@ -132,6 +139,7 @@ def __init__( version, zfile, zfile_hashvals, + computed_set_name, ): self.user_id = user_id self.sdf_filename = sdf_filename @@ -141,6 +149,7 @@ def __init__( self.version = version self.zfile = zfile self.zfile_hashvals = zfile_hashvals + self.computed_set_name = computed_set_name def process_pdb(self, pdb_code, zfile, zfile_hashvals) -> str | None: for key in zfile_hashvals.keys(): @@ -254,41 +263,49 @@ def get_site_observation( return site_obvs - def create_mol(self, inchi, long_inchi=None, name=None) -> Compound: + def create_mol(self, inchi, target, name=None) -> Compound: # check for an existing compound, returning a Compound - if long_inchi: - cpd = Compound.objects.filter(long_inchi=long_inchi) - sanitized_mol = Chem.MolFromInchi(long_inchi, sanitize=True) - else: - cpd = Compound.objects.filter(inchi=inchi) - sanitized_mol = Chem.MolFromInchi(inchi, sanitize=True) - - new_mol = cpd[0] if len(cpd) != 0 else Compound() - new_mol.smiles = Chem.MolToSmiles(sanitized_mol) - new_mol.inchi = inchi - if long_inchi: - new_mol.long_inchi = long_inchi - new_mol.identifier = name - - # descriptors - new_mol.mol_log_p = Crippen.MolLogP(sanitized_mol) - new_mol.mol_wt = float(Chem.rdMolDescriptors.CalcExactMolWt(sanitized_mol)) - new_mol.heavy_atom_count = Chem.Lipinski.HeavyAtomCount(sanitized_mol) - new_mol.heavy_atom_mol_wt = float(Descriptors.HeavyAtomMolWt(sanitized_mol)) - new_mol.nhoh_count = Chem.Lipinski.NHOHCount(sanitized_mol) - new_mol.no_count = Chem.Lipinski.NOCount(sanitized_mol) - new_mol.num_h_acceptors = Chem.Lipinski.NumHAcceptors(sanitized_mol) - new_mol.num_h_donors = Chem.Lipinski.NumHDonors(sanitized_mol) - new_mol.num_het_atoms = Chem.Lipinski.NumHeteroatoms(sanitized_mol) - new_mol.num_rot_bonds = Chem.Lipinski.NumRotatableBonds(sanitized_mol) - new_mol.num_val_electrons = Descriptors.NumValenceElectrons(sanitized_mol) - new_mol.ring_count = Chem.Lipinski.RingCount(sanitized_mol) - new_mol.tpsa = Chem.rdMolDescriptors.CalcTPSA(sanitized_mol) - - # make sure there is an id so inspirations can be added - new_mol.save() - - return new_mol + + sanitized_mol = Chem.MolFromInchi(inchi, sanitize=True) + Chem.RemoveStereochemistry(sanitized_mol) + inchi = Chem.inchi.MolToInchi(sanitized_mol) + inchi_key = Chem.InchiToInchiKey(inchi) + + try: + # NB! Max said there could be thousands of compounds per + # target so this distinct() here may become a problem + + # fmt: off + cpd = Compound.objects.filter( + computedmolecule__computed_set__target=target, + ).distinct().get( + inchi_key=inchi_key, + ) + # fmt: on + except Compound.DoesNotExist: + cpd = Compound( + smiles=Chem.MolToSmiles(sanitized_mol), + inchi=inchi, + inchi_key=inchi_key, + current_identifier=name, + ) + cpd.save() + except MultipleObjectsReturned as exc: + # NB! when processing new uploads, Compound is always + # fetched by inchi_key, so this shouldn't ever create + # duplicates. Ands LHS uploads do not create inchi_keys, + # so under normal operations duplicates should never + # occur. However there's nothing in the db to prevent + # this, so adding a catch clause and writing a meaningful + # message + logger.error( + 'Duplicate compounds for target %s with inchi key %s.', + target.title, + inchi_key, + ) + raise MultipleObjectsReturned from exc + + return cpd def set_props(self, cpd, props, compound_set) -> List[ScoreDescription]: if 'ref_mols' and 'ref_pdb' not in list(props.keys()): @@ -322,13 +339,10 @@ def set_mol( smiles = Chem.MolToSmiles(mol) inchi = Chem.inchi.MolToInchi(mol) molecule_name = mol.GetProp('_Name') - long_inchi = None - if len(inchi) > 255: - long_inchi = inchi - inchi = inchi[:254] + version = mol.GetProp('version') compound: Compound = self.create_mol( - inchi, name=molecule_name, long_inchi=long_inchi + inchi, compound_set.target, name=molecule_name ) insp = mol.GetProp('ref_mols') @@ -353,12 +367,7 @@ def set_mol( 'No matching molecules found for inspiration frag ' + i ) - if qs.count() > 1: - ids = [m.cmpd.id for m in qs] - ind = ids.index(max(ids)) - ref = qs[ind] - elif qs.count() == 1: - ref = qs[0] + ref = qs.order_by('-cmpd_id').first() insp_frags.append(ref) @@ -385,11 +394,60 @@ def set_mol( # Need a ComputedMolecule before saving. # Check if anything exists already... - existing_computed_molecules = ComputedMolecule.objects.filter( - molecule_name=molecule_name, smiles=smiles, computed_set=compound_set + + # I think, realistically, I only need to check compound + # fmt: off + qs = ComputedMolecule.objects.filter( + compound=compound, + ).annotate( + # names come in format: + # target_name-sequential number-sequential letter, + # e.g. A71EV2A-1-a, hence grabbing the 3rd column + suffix=Func( + F('name'), + Value('-'), + Value(3), + function='split_part', + output_field=TextField(), + ), ) - computed_molecule: Optional[ComputedMolecule] = None + if qs.exists(): + suffix = next( + alphanumerator(start_from=qs.order_by('-suffix').first().suffix) + ) + else: + suffix = 'a' + + # distinct is ran on indexed field, so shouldn't be a problem + number = ComputedMolecule.objects.filter( + computed_set__target=compound_set.target, + ).values('id').distinct().count() + 1 + # fmt: on + + name = f'v{number}{suffix}' + + existing_computed_molecules = [] + for k in qs: + kmol = Chem.MolFromMolBlock(k.sdf_info) + if kmol: + # find distances between corresponding atoms of the + # two conformers. if any one exceeds the _DIST_LIMIT, + # consider it to be a new ComputedMolecule + _, _, atom_map = Chem.rdMolAlign.GetBestAlignmentTransform(mol, kmol) + molconf = mol.GetConformer() + kmolconf = kmol.GetConformer() + small_enough = True + for mol_atom, kmol_atom in atom_map: + molpos = np.array(molconf.GetAtomPosition(mol_atom)) + kmolpos = np.array(kmolconf.GetAtomPosition(kmol_atom)) + distance = np.linalg.norm(molpos - kmolpos) + if distance >= _DIST_LIMIT: + small_enough = False + break + if small_enough: + existing_computed_molecules.append(k) + if len(existing_computed_molecules) == 1: logger.info( 'Using existing ComputedMolecule %s', existing_computed_molecules[0] @@ -400,10 +458,10 @@ def set_mol( for exist in existing_computed_molecules: logger.info('Deleting ComputedMolecule %s', exist) exist.delete() - computed_molecule = ComputedMolecule() - if not computed_molecule: + computed_molecule = ComputedMolecule(name=name) + else: logger.info('Creating new ComputedMolecule') - computed_molecule = ComputedMolecule() + computed_molecule = ComputedMolecule(name=name) if isinstance(ref_so, SiteObservation): code = ref_so.code @@ -414,18 +472,20 @@ def set_mol( pdb_info = ref_so lhs_so = None - assert computed_molecule + # I don't quite understand why the overwrite of existing + # compmol.. but this is how it was, not touching it now + # update: I think it's about updating metadata. moving + # name attribute out so it won't get overwritten computed_molecule.compound = compound - computed_molecule.computed_set = compound_set computed_molecule.sdf_info = Chem.MolToMolBlock(mol) computed_molecule.site_observation_code = code computed_molecule.reference_code = code computed_molecule.molecule_name = molecule_name - computed_molecule.name = f"{target}-{computed_molecule.identifier}" computed_molecule.smiles = smiles computed_molecule.pdb = lhs_so # TODO: this is wrong computed_molecule.pdb_info = pdb_info + computed_molecule.version = version # Extract possible reference URL and Rationale # URLs have to be valid URLs and rationals must contain more than one word ref_url: Optional[str] = ( @@ -447,6 +507,8 @@ def set_mol( # Done computed_molecule.save() + compound_set.computed_molecules.add(computed_molecule) + # No update the molecule in the original file... add_props_to_sdf_molecule( sdf_file=filename, @@ -530,50 +592,51 @@ def task(self) -> ComputedSet: ) # Do we have any existing ComputedSets? - # Ones with the same method and upload date? - today: datetime.date = datetime.date.today() - existing_sets: List[ComputedSet] = ComputedSet.objects.filter( - method=truncated_submitter_method, upload_date=today - ).all() - # If so, find the one with the highest ordinal. - latest_ordinal: int = 0 - for exiting_set in existing_sets: - assert exiting_set.md_ordinal > 0 - if exiting_set.md_ordinal > latest_ordinal: - latest_ordinal = exiting_set.md_ordinal - if latest_ordinal: - logger.info( - 'Found existing ComputedSets for method "%s" on %s (%d) with ordinal=%d', - truncated_submitter_method, - str(today), - len(existing_sets), - latest_ordinal, + try: + computed_set = ComputedSet.objects.get(name=self.computed_set_name) + # refresh some attributes + computed_set.md_ordinal = F('md_ordinal') + 1 + computed_set.upload_date = datetime.date.today() + computed_set.save() + except ComputedSet.DoesNotExist: + # no, create new + + today: datetime.date = datetime.date.today() + new_ordinal: int = 1 + try: + target = Target.objects.get(title=self.target) + except Target.DoesNotExist as exc: + # probably wrong target name supplied + logger.error('Target %s does not exist', self.target) + raise Target.DoesNotExist from exc + + cs_name: str = ( + f'{truncated_submitter_method}-{str(today)}-' + + f'{get_column_letter(new_ordinal)}' ) - # ordinals are 1-based - new_ordinal: int = latest_ordinal + 1 - - # The computed set "name" consists of the "method", - # today's date and a 2-digit ordinal. The ordinal - # is used to distinguish between computed sets uploaded - # with the same method on the same day. - assert new_ordinal > 0 - cs_name: str = f"{truncated_submitter_method}-{str(today)}-{get_column_letter(new_ordinal)}" - logger.info('Creating new ComputedSet "%s"', cs_name) - - computed_set: ComputedSet = ComputedSet() - computed_set.name = cs_name - computed_set.md_ordinal = new_ordinal - computed_set.upload_date = today - computed_set.method = self.submitter_method[: ComputedSet.LENGTH_METHOD] - computed_set.target = Target.objects.get(title=self.target) - computed_set.spec_version = float(self.version.strip('ver_')) - if self.user_id: - computed_set.owner_user = User.objects.get(id=self.user_id) - else: - # The User ID may only be None if AUTHENTICATE_UPLOAD is False. - # Here the ComputedSet owner will take on a default (anonymous) value. - assert settings.AUTHENTICATE_UPLOAD is False - computed_set.save() + logger.info('Creating new ComputedSet "%s"', cs_name) + + computed_set = ComputedSet( + name=cs_name, + md_ordinal=new_ordinal, + upload_date=today, + method=self.submitter_method[: ComputedSet.LENGTH_METHOD], + target=target, + spec_version=float(self.version.strip('ver_')), + ) + if self.user_id: + try: + computed_set.owner_user = User.objects.get(id=self.user_id) + except User.DoesNotExist as exc: + logger.error('User %s does not exist', self.user_id) + raise User.DoesNotExist from exc + + else: + # The User ID may only be None if AUTHENTICATE_UPLOAD is False. + # Here the ComputedSet owner will take on a default (anonymous) value. + assert settings.AUTHENTICATE_UPLOAD is False + + computed_set.save() # check compound set folder exists. cmp_set_folder = os.path.join( diff --git a/viewer/migrations/0056_compound_inchi_key.py b/viewer/migrations/0056_compound_inchi_key.py new file mode 100644 index 00000000..d0591072 --- /dev/null +++ b/viewer/migrations/0056_compound_inchi_key.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.25 on 2024-06-11 13:10 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('viewer', '0055_merge_20240516_1003'), + ] + + operations = [ + migrations.AddField( + model_name='compound', + name='inchi_key', + field=models.CharField(blank=True, db_index=True, max_length=80), + ), + ] diff --git a/viewer/migrations/0057_auto_20240612_1348.py b/viewer/migrations/0057_auto_20240612_1348.py new file mode 100644 index 00000000..1dd8e5ac --- /dev/null +++ b/viewer/migrations/0057_auto_20240612_1348.py @@ -0,0 +1,23 @@ +# Generated by Django 3.2.25 on 2024-06-12 13:48 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('viewer', '0056_compound_inchi_key'), + ] + + operations = [ + migrations.AddField( + model_name='computedmolecule', + name='version', + field=models.PositiveSmallIntegerField(default=1), + ), + migrations.AlterField( + model_name='compound', + name='inchi_key', + field=models.CharField(blank=True, db_index=True, max_length=27), + ), + ] diff --git a/viewer/migrations/0058_auto_20240614_1016.py b/viewer/migrations/0058_auto_20240614_1016.py new file mode 100644 index 00000000..8e296ed6 --- /dev/null +++ b/viewer/migrations/0058_auto_20240614_1016.py @@ -0,0 +1,35 @@ +# Generated by Django 3.2.25 on 2024-06-14 10:16 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('viewer', '0057_auto_20240612_1348'), + ] + + operations = [ + migrations.CreateModel( + name='ComputedSetComputedMolecule', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('computed_molecule', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='viewer.computedmolecule')), + ('computed_set', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='viewer.computedset')), + ], + ), + migrations.AddConstraint( + model_name='computedsetcomputedmolecule', + constraint=models.UniqueConstraint(fields=('computed_set', 'computed_molecule'), name='unique_computedsetcomputedmolecule'), + ), + migrations.RemoveField( + model_name='computedmolecule', + name='computed_set', + ), + migrations.AddField( + model_name='computedset', + name='computed_molecules', + field=models.ManyToManyField(related_name='computed_set', through='viewer.ComputedSetComputedMolecule', to='viewer.ComputedMolecule'), + ), + ] diff --git a/viewer/models.py b/viewer/models.py index e35d41ac..85678ad0 100644 --- a/viewer/models.py +++ b/viewer/models.py @@ -261,6 +261,7 @@ class Compound(models.Model): ) description = models.TextField(blank=True, null=True) comments = models.TextField(blank=True, null=True) + inchi_key = models.CharField(db_index=True, max_length=27, blank=True) objects = models.Manager() filter_manager = CompoundDataManager() @@ -951,6 +952,12 @@ class ComputedSet(models.Model): upload_datetime = models.DateTimeField( null=True, help_text="The datetime the upload was completed" ) + computed_molecules = models.ManyToManyField( + "ComputedMolecule", + through="ComputedSetComputedMolecule", + through_fields=("computed_set", "computed_molecule"), + related_name="computed_set", + ) def __str__(self) -> str: target_title: str = self.target.title if self.target else "None" @@ -978,7 +985,6 @@ class ComputedMolecule(models.Model): null=True, blank=True, ) - computed_set = models.ForeignKey(ComputedSet, on_delete=models.CASCADE) name = models.CharField( max_length=50, help_text="A combination of Target and Identifier" ) @@ -1021,6 +1027,7 @@ class ComputedMolecule(models.Model): max_length=255, help_text="Link to pdb file; user-uploaded pdb or pdb.experiment.pdb_info", ) + version = models.PositiveSmallIntegerField(null=False, default=1) def __str__(self) -> str: return f"{self.smiles}" @@ -1035,6 +1042,24 @@ def __repr__(self) -> str: ) +class ComputedSetComputedMolecule(models.Model): + computed_set = models.ForeignKey(ComputedSet, null=False, on_delete=models.CASCADE) + computed_molecule = models.ForeignKey( + ComputedMolecule, null=False, on_delete=models.CASCADE + ) + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=[ + "computed_set", + "computed_molecule", + ], + name="unique_computedsetcomputedmolecule", + ), + ] + + class ScoreDescription(models.Model): """The names and descriptions of scores that the user uploads with each computed set molecule.""" diff --git a/viewer/target_loader.py b/viewer/target_loader.py index f0c01bb8..8a3cbb15 100644 --- a/viewer/target_loader.py +++ b/viewer/target_loader.py @@ -1,11 +1,9 @@ import contextlib import functools import hashlib -import itertools import logging import math import os -import string import tarfile import uuid from collections.abc import Callable @@ -13,7 +11,7 @@ from enum import Enum from pathlib import Path from tempfile import TemporaryDirectory -from typing import Any, Dict, Generator, Iterable, List, Optional, Tuple, TypeVar +from typing import Any, Dict, Iterable, List, Optional, Tuple, TypeVar import yaml from celery import Task @@ -46,6 +44,7 @@ XtalformQuatAssembly, XtalformSite, ) +from viewer.utils import alphanumerator logger = logging.getLogger(__name__) @@ -277,29 +276,6 @@ def calculate_sha256(filepath) -> str: return sha256_hash.hexdigest() -def alphanumerator(start_from: str = "") -> Generator[str, None, None]: - """Return alphabetic generator (A, B .. AA, AB...) starting from a specified point.""" - - # since product requries finite maximum return string length set - # to 10 characters. that should be enough for fragalysis (and to - # cause database issues) - generator = ( - "".join(word) - for word in itertools.chain.from_iterable( - itertools.product(string.ascii_lowercase, repeat=i) for i in range(1, 11) - ) - ) - - # Drop values until the starting point is reached - if start_from is not None and start_from != '': - start_from = start_from.lower() - generator = itertools.dropwhile(lambda x: x != start_from, generator) # type: ignore[assignment] - # and drop one more, then it starts from after the start from as it should - _ = next(generator) - - return generator - - def strip_version(s: str, separator: str = "/") -> Tuple[str, int]: # format something like XX01ZVNS2B-x0673/B/501/1 # remove tailing '1' diff --git a/viewer/tasks.py b/viewer/tasks.py index cd360587..2274fdd4 100644 --- a/viewer/tasks.py +++ b/viewer/tasks.py @@ -86,6 +86,7 @@ def process_compound_set(validate_output): logger.warning('process_compound_set() EXIT params=%s (not validated)', params) return process_stage, validate_dict, validated + computed_set_name = params.get('update', None) submitter_name, submitter_method, blank_version = blank_mol_vals(params['sdf']) zfile, zfile_hashvals = PdbOps().run(params) @@ -100,6 +101,7 @@ def process_compound_set(validate_output): version=blank_version, zfile=zfile, zfile_hashvals=zfile_hashvals, + computed_set_name=computed_set_name, ) compound_set = save_mols.task() @@ -186,6 +188,7 @@ def validate_compound_set(task_params): 'sdf': sdf_file, 'target': target, 'pdb_zip': zfile, + 'update': update, } # Protect ourselves from an empty, blank or missing SD file. diff --git a/viewer/utils.py b/viewer/utils.py index b7b72feb..e170a0ee 100644 --- a/viewer/utils.py +++ b/viewer/utils.py @@ -4,13 +4,15 @@ Collection of technical methods tidied up in one location. """ import fnmatch +import itertools import json import logging import os import shutil +import string import tempfile from pathlib import Path -from typing import Dict, Optional +from typing import Dict, Generator, Optional from urllib.parse import urlparse from django.conf import settings @@ -405,3 +407,26 @@ def restore_curated_tags(filename: str) -> None: except IntegrityError as exc: logger.error(exc) + + +def alphanumerator(start_from: str = "") -> Generator[str, None, None]: + """Return alphabetic generator (A, B .. AA, AB...) starting from a specified point.""" + + # since product requries finite maximum return string length set + # to 10 characters. that should be enough for fragalysis (and to + # cause database issues) + generator = ( + "".join(word) + for word in itertools.chain.from_iterable( + itertools.product(string.ascii_lowercase, repeat=i) for i in range(1, 11) + ) + ) + + # Drop values until the starting point is reached + if start_from is not None and start_from != '': + start_from = start_from.lower() + generator = itertools.dropwhile(lambda x: x != start_from, generator) # type: ignore[assignment] + # and drop one more, then it starts from after the start from as it should + _ = next(generator) + + return generator diff --git a/viewer/views.py b/viewer/views.py index cb8e7fdf..f1b58e35 100644 --- a/viewer/views.py +++ b/viewer/views.py @@ -457,7 +457,6 @@ def post(self, request): request.session[_SESSION_ERROR], ) return redirect('viewer:upload_cset') - # You cannot validate or upload a set # unless the user is part of the Target's project (proposal) # even if the target is 'open'. @@ -569,7 +568,34 @@ def post(self, request): assert selected_set written_sdf_filename = selected_set.written_sdf_filename selected_set_name = selected_set.name + + # related objects: + # - ComputedSetComputedMolecule + # - ComputedMolecule + # - NumericalScoreValues + # - TextScoreValues + # - ComputedMolecule_computed_inspirations + # - Compound + + # all but ComputedMolecule are handled automatically + # but (because of the m2m), have to delete those + # separately + + # select ComputedMolecule objects that are in this set + # and not in any other sets + # fmt: off + selected_set.computed_molecules.exclude( + pk__in=models.ComputedMolecule.objects.filter( + computed_set__in=models.ComputedSet.objects.filter( + target=selected_set.target, + ).exclude( + pk=selected_set.pk, + ), + ), + ).delete() + # fmt: on selected_set.delete() + # ...and the original (expected) file if os.path.isfile(written_sdf_filename): os.remove(written_sdf_filename)