From 0a86cf68cba82cba38a76028c861f07e0420295a Mon Sep 17 00:00:00 2001 From: Alexander Pace <alexander.pace@ligo.org> Date: Tue, 26 Mar 2024 15:47:01 -0500 Subject: [PATCH 01/10] speed up unauthenticated access of public data products https://git.ligo.org/computing/gracedb/server/-/issues/302 --- gracedb/superevents/utils.py | 73 ++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/gracedb/superevents/utils.py b/gracedb/superevents/utils.py index d8cecdca4..24c6d89ee 100644 --- a/gracedb/superevents/utils.py +++ b/gracedb/superevents/utils.py @@ -6,6 +6,8 @@ from django.db.models import Q from django.http import Http404 from django.shortcuts import get_object_or_404 from django.contrib.auth.models import Group as DjangoGroup +from django.contrib.contenttypes.models import ContentType +from guardian.models import Permission from .models import Superevent, Log, Labelling, EMObservation, EMFootprint, \ VOEvent, Signoff @@ -38,6 +40,15 @@ SUPEREVENT_PERMS = { settings.PUBLIC_GROUP: ['view'], } +# Prefetch some group objects. +INTERNAL_USERS_GROUP = DjangoGroup.objects.get(name=settings.LVC_GROUP) +PUBLIC_USERS_GROUP = DjangoGroup.objects.get(name=settings.PUBLIC_GROUP) +LVEM_USERS_GROUP = DjangoGroup.objects.get(name=settings.LVEM_GROUP) + +# Prefetch some contenttype stuff: +SUPEREVENT_CTYPE = ContentType.objects.get_for_model(Superevent) +SUPEREVENT_LOG_CTYPE = ContentType.objects.get_for_model(Log) + def create_superevent(submitter, t_start, t_0, t_end, preferred_event, events=[], labels=[], category='P', add_log_message=True, @@ -1067,3 +1078,65 @@ def remove_pipeline_preferred_event_from_superevent(superevent, event, .issue_alerts() EventAlertIssuer(event, alert_type='removed_from_superevent') \ .issue_alerts() + + +#------------------------------------------------------------------------------- +# A faster (though less general) version of guardian.shortcuts +# get_objects_for_user. Right now it filters superevents and superevent logs for +# what is visible for a user based on their group affilition. +#------------------------------------------------------------------------------- +def fast_get_objects_for_user(user, perm_codename, queryset): + + # First things first, if a user in part of the internal_users + # group, they're authorized to view anything, so just return + # the queryset. Otherwise, get the relevant group for the + # groupobjectpermission (gop) query: + + if INTERNAL_USERS_GROUP in user.groups.all(): + return queryset + + elif PUBLIC_USERS_GROUP in user.groups.all(): + gop_group = PUBLIC_USERS_GROUP + + elif LVEM_USERS_GROUP in user.groups.all(): + gop_group = LVEM_USERS_GROUP + + else: + gop_group = None + + # Now get the corresponding permission object: + app_label, codename = perm_codename.split('.') + + # For now, this function only considers 'superevent' permissions, + # but maybe FIXME later to make it more general. + if app_label != 'superevents': + raise ValueError('Only superevent permissions are considered at ', + 'this time.') + + # Next get the permission object from the codename. This will utilize a + # ForeignKey relation to filter groupobjectpermissions instead of a + # scan of the codename column, which should be faster even though both are + # indexed. If it's not found, then it should raise a DoesNotExist error, which + # I'm going to allow throw to the user: + perm = Permission.objects.get(codename=codename) + + # Next we're going to change the query based on what model we have. + # We have to compare ContentType's because of some weird django model + # inheritance nuance: + + queryset_model = ContentType.objects.get_for_model(queryset.model) + + if queryset_model == SUPEREVENT_LOG_CTYPE: + col_name = 'loggroupobjectpermission' + elif queryset_model == SUPEREVENT_CTYPE: + col_name = 'supereventgroupobjectpermission' + else: + raise ValueError('Only superevents.model.superevent and ' + 'superevents.model.Log sets supported at this time.') + + # Construct the query kwarg: + query_kwarg = {f'{col_name}__permission': perm, + f'{col_name}__group': gop_group} + + # Do the query and return the results: + return queryset.filter(**query_kwarg) -- GitLab From 0c515f33310b46d0976862d986e268109e55f8e5 Mon Sep 17 00:00:00 2001 From: Alexander Pace <alexander.pace@ligo.org> Date: Tue, 26 Mar 2024 20:43:36 -0500 Subject: [PATCH 02/10] removed import-time queries it's supposedly bad practice, and they break tests anyway --- gracedb/superevents/utils.py | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/gracedb/superevents/utils.py b/gracedb/superevents/utils.py index 24c6d89ee..4cac7bd7e 100644 --- a/gracedb/superevents/utils.py +++ b/gracedb/superevents/utils.py @@ -40,15 +40,6 @@ SUPEREVENT_PERMS = { settings.PUBLIC_GROUP: ['view'], } -# Prefetch some group objects. -INTERNAL_USERS_GROUP = DjangoGroup.objects.get(name=settings.LVC_GROUP) -PUBLIC_USERS_GROUP = DjangoGroup.objects.get(name=settings.PUBLIC_GROUP) -LVEM_USERS_GROUP = DjangoGroup.objects.get(name=settings.LVEM_GROUP) - -# Prefetch some contenttype stuff: -SUPEREVENT_CTYPE = ContentType.objects.get_for_model(Superevent) -SUPEREVENT_LOG_CTYPE = ContentType.objects.get_for_model(Log) - def create_superevent(submitter, t_start, t_0, t_end, preferred_event, events=[], labels=[], category='P', add_log_message=True, @@ -1092,14 +1083,17 @@ def fast_get_objects_for_user(user, perm_codename, queryset): # the queryset. Otherwise, get the relevant group for the # groupobjectpermission (gop) query: - if INTERNAL_USERS_GROUP in user.groups.all(): + if user.groups.filter(name=settings.LVC_GROUP): return queryset - elif PUBLIC_USERS_GROUP in user.groups.all(): - gop_group = PUBLIC_USERS_GROUP + # Default to the public group. This should cover 100% of the use + # cases since lvem_users isn't used anymore, but let's leave it + # in as a backup. + elif user.groups.filter(name=settings.PUBLIC_GROUP): + gop_group = DjangoGroup.objects.get(name=settings.PUBLIC_GROUP) - elif LVEM_USERS_GROUP in user.groups.all(): - gop_group = LVEM_USERS_GROUP + elif user.groups.filter(name=settings.LVEM_OBSERVERS_GROUP): + gop_group = DjangoGroup.objects.get(name=settings.LVEM_OBSERVERS_GROUP) else: gop_group = None @@ -1126,9 +1120,9 @@ def fast_get_objects_for_user(user, perm_codename, queryset): queryset_model = ContentType.objects.get_for_model(queryset.model) - if queryset_model == SUPEREVENT_LOG_CTYPE: + if queryset_model == ContentType.objects.get_for_model(Log): col_name = 'loggroupobjectpermission' - elif queryset_model == SUPEREVENT_CTYPE: + elif queryset_model == ContentType.objects.get_for_model(Superevent): col_name = 'supereventgroupobjectpermission' else: raise ValueError('Only superevents.model.superevent and ' -- GitLab From 53654693e4d4830c31ad65c37b8012ae0304e783 Mon Sep 17 00:00:00 2001 From: Daniel Wysocki <daniel.wysocki@ligo.org> Date: Wed, 27 Mar 2024 22:51:12 +0000 Subject: [PATCH 03/10] Apply 1 suggestion(s) to 1 file(s) --- gracedb/superevents/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gracedb/superevents/utils.py b/gracedb/superevents/utils.py index 4cac7bd7e..5ee2f69fe 100644 --- a/gracedb/superevents/utils.py +++ b/gracedb/superevents/utils.py @@ -1078,7 +1078,7 @@ def remove_pipeline_preferred_event_from_superevent(superevent, event, #------------------------------------------------------------------------------- def fast_get_objects_for_user(user, perm_codename, queryset): - # First things first, if a user in part of the internal_users + # First things first, if a user is part of the internal_users # group, they're authorized to view anything, so just return # the queryset. Otherwise, get the relevant group for the # groupobjectpermission (gop) query: -- GitLab From 986f17a8f2a1faedcf443d3e1dd81f0dfc898797 Mon Sep 17 00:00:00 2001 From: Daniel Wysocki <daniel.wysocki@ligo.org> Date: Wed, 27 Mar 2024 23:01:26 +0000 Subject: [PATCH 04/10] Apply 1 suggestion(s) to 1 file(s) --- gracedb/superevents/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gracedb/superevents/utils.py b/gracedb/superevents/utils.py index 5ee2f69fe..469df0ae9 100644 --- a/gracedb/superevents/utils.py +++ b/gracedb/superevents/utils.py @@ -1083,7 +1083,7 @@ def fast_get_objects_for_user(user, perm_codename, queryset): # the queryset. Otherwise, get the relevant group for the # groupobjectpermission (gop) query: - if user.groups.filter(name=settings.LVC_GROUP): + if user.groups.filter(name=settings.LVC_GROUP).exists(): return queryset # Default to the public group. This should cover 100% of the use -- GitLab From 2b9e9eb89bd55ba9f158822995392d93cdea783d Mon Sep 17 00:00:00 2001 From: Daniel Wysocki <daniel.wysocki@ligo.org> Date: Wed, 27 Mar 2024 23:01:32 +0000 Subject: [PATCH 05/10] Apply 1 suggestion(s) to 1 file(s) --- gracedb/superevents/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gracedb/superevents/utils.py b/gracedb/superevents/utils.py index 469df0ae9..e45d2eadc 100644 --- a/gracedb/superevents/utils.py +++ b/gracedb/superevents/utils.py @@ -1089,7 +1089,7 @@ def fast_get_objects_for_user(user, perm_codename, queryset): # Default to the public group. This should cover 100% of the use # cases since lvem_users isn't used anymore, but let's leave it # in as a backup. - elif user.groups.filter(name=settings.PUBLIC_GROUP): + elif user.groups.filter(name=settings.PUBLIC_GROUP).exists(): gop_group = DjangoGroup.objects.get(name=settings.PUBLIC_GROUP) elif user.groups.filter(name=settings.LVEM_OBSERVERS_GROUP): -- GitLab From 0faf9cfebdbb62f7da3456b64e27e47dc6df80ca Mon Sep 17 00:00:00 2001 From: Daniel Wysocki <daniel.wysocki@ligo.org> Date: Wed, 27 Mar 2024 23:01:43 +0000 Subject: [PATCH 06/10] Apply 1 suggestion(s) to 1 file(s) --- gracedb/superevents/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gracedb/superevents/utils.py b/gracedb/superevents/utils.py index e45d2eadc..2476b9296 100644 --- a/gracedb/superevents/utils.py +++ b/gracedb/superevents/utils.py @@ -1092,7 +1092,7 @@ def fast_get_objects_for_user(user, perm_codename, queryset): elif user.groups.filter(name=settings.PUBLIC_GROUP).exists(): gop_group = DjangoGroup.objects.get(name=settings.PUBLIC_GROUP) - elif user.groups.filter(name=settings.LVEM_OBSERVERS_GROUP): + elif user.groups.filter(name=settings.LVEM_OBSERVERS_GROUP).exists(): gop_group = DjangoGroup.objects.get(name=settings.LVEM_OBSERVERS_GROUP) else: -- GitLab From f5430ce5200eed1199bc33822592fccacb571363 Mon Sep 17 00:00:00 2001 From: Alexander Pace <alexander.pace@ligo.org> Date: Thu, 28 Mar 2024 15:12:27 -0500 Subject: [PATCH 07/10] add conditional for anonymous user with no groups see tests in api/v1/superevents/test_access.py, and confirmed by hitting superevents and superevent file pages in the browser. --- gracedb/superevents/utils.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/gracedb/superevents/utils.py b/gracedb/superevents/utils.py index 2476b9296..22c216a28 100644 --- a/gracedb/superevents/utils.py +++ b/gracedb/superevents/utils.py @@ -1088,7 +1088,13 @@ def fast_get_objects_for_user(user, perm_codename, queryset): # Default to the public group. This should cover 100% of the use # cases since lvem_users isn't used anymore, but let's leave it - # in as a backup. + # in as a backup. The first condition is for the really AnonymousUser + # created by DRF (which has no group memberships) and the second is the + # AnonymousUser in the database that is a member of the public group. + + elif not user.groups.exists(): + gop_group = DjangoGroup.objects.get(name=settings.PUBLIC_GROUP) + elif user.groups.filter(name=settings.PUBLIC_GROUP).exists(): gop_group = DjangoGroup.objects.get(name=settings.PUBLIC_GROUP) -- GitLab From cf72b3398eb9a9dd3f8a7b10858eff76f8c60886 Mon Sep 17 00:00:00 2001 From: Alexander Pace <alexander.pace@ligo.org> Date: Thu, 28 Mar 2024 15:34:26 -0500 Subject: [PATCH 08/10] swap out get_objects_for_user for superevent-specific views --- gracedb/api/v1/superevents/views.py | 12 ++++++------ gracedb/superevents/views.py | 9 ++++----- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/gracedb/api/v1/superevents/views.py b/gracedb/api/v1/superevents/views.py index 4cdc7b558..eb52a7709 100644 --- a/gracedb/api/v1/superevents/views.py +++ b/gracedb/api/v1/superevents/views.py @@ -8,7 +8,6 @@ from django.core.exceptions import ValidationError from django.http import HttpResponse from django.shortcuts import get_object_or_404 -from guardian.shortcuts import get_objects_for_user from rest_framework import mixins, parsers, permissions, serializers, status, \ viewsets from rest_framework.decorators import action @@ -27,7 +26,8 @@ from superevents.utils import remove_tag_from_log, \ confirm_superevent_as_gw, get_superevent_by_date_id_or_404, \ get_superevent_by_sid_or_gwid_or_404, \ expose_superevent, hide_superevent, delete_signoff, \ - remove_pipeline_preferred_event_from_superevent + remove_pipeline_preferred_event_from_superevent, \ + fast_get_objects_for_user from .filters import SupereventSearchFilter, SupereventOrderingFilter from .paginators import CustomSupereventPagination from .permissions import SupereventModelPermissions, \ @@ -282,8 +282,8 @@ class SupereventLogTagViewSet(SafeCreateMixin, SafeDestroyMixin, # Pass full set of logs for parent superevent to get_objects_for_user, # which will filter based on view permissions - parent_log_queryset = get_objects_for_user(self.request.user, - 'superevents.view_log', klass=parent_superevent.log_set.all()) + parent_log_queryset = fast_get_objects_for_user(self.request.user, + 'superevents.view_log', parent_superevent.log_set.all()) # Get parent_log and cache it; return 404 if not found self._parent_log = get_object_or_404(parent_log_queryset, @@ -317,8 +317,8 @@ class SupereventFileViewSet(InheritDefaultPermissionsMixin, def filter_log_queryset(self, log_queryset): # Filter queryset based on the user's view permissions - return get_objects_for_user(self.request.user, - 'superevents.view_log', klass=log_queryset) + return fast_get_objects_for_user(self.request.user, + 'superevents.view_log', log_queryset) # This is a bandaid to overcome some very very # intermittent errors we've been seeing on AWS. diff --git a/gracedb/superevents/views.py b/gracedb/superevents/views.py index f380ea017..0ac8dd67d 100644 --- a/gracedb/superevents/views.py +++ b/gracedb/superevents/views.py @@ -14,8 +14,6 @@ from django.views.decorators.vary import vary_on_headers from django.views.generic.detail import DetailView from django.views.generic import ListView -from guardian.shortcuts import get_objects_for_user - from core.file_utils import get_file_list, flexible_skymap_to_png from core.time_utils import utc_datetime_decimal_seconds from core.utils import display_far_hz_to_yr @@ -30,7 +28,8 @@ from .mixins import ExposeHideMixin, OperatorSignoffMixin, \ from .models import Superevent, VOEvent, Log from search.constants import RUN_MAP from .utils import get_superevent_by_date_id_or_404, \ - get_superevent_by_sid_or_gwid_or_404 + get_superevent_by_sid_or_gwid_or_404, \ + fast_get_objects_for_user # Set up logger @@ -158,8 +157,8 @@ class SupereventFileList(SupereventDetailView): context = DetailView.get_context_data(self, **kwargs) # Get list of logs which are viewable by the user - viewable_logs = get_objects_for_user(self.request.user, - self.log_view_permission, klass=self.object.log_set.all()) + viewable_logs = fast_get_objects_for_user(self.request.user, + self.log_view_permission, self.object.log_set.all()) file_list = viewable_logs.exclude(filename='').order_by('filename') -- GitLab From 1546846d993d2cc4e7486dd2a4d5717f774b7c62 Mon Sep 17 00:00:00 2001 From: Alexander Pace <alexander.pace@ligo.org> Date: Mon, 1 Apr 2024 19:32:42 -0500 Subject: [PATCH 09/10] check if user.is_anonymous just like in here https://github.com/django-guardian/django-guardian/blob/devel/guardian/shortcuts.py#L548 --- gracedb/superevents/utils.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/gracedb/superevents/utils.py b/gracedb/superevents/utils.py index 22c216a28..173ac7988 100644 --- a/gracedb/superevents/utils.py +++ b/gracedb/superevents/utils.py @@ -1089,13 +1089,11 @@ def fast_get_objects_for_user(user, perm_codename, queryset): # Default to the public group. This should cover 100% of the use # cases since lvem_users isn't used anymore, but let's leave it # in as a backup. The first condition is for the really AnonymousUser - # created by DRF (which has no group memberships) and the second is the - # AnonymousUser in the database that is a member of the public group. + # created by DRF (which has no group memberships, no username, and no + # user.id), or the AnonymousUser in the database that is a member of + # the public group. - elif not user.groups.exists(): - gop_group = DjangoGroup.objects.get(name=settings.PUBLIC_GROUP) - - elif user.groups.filter(name=settings.PUBLIC_GROUP).exists(): + elif user.is_anonymous or user.groups.filter(name=settings.PUBLIC_GROUP).exists(): gop_group = DjangoGroup.objects.get(name=settings.PUBLIC_GROUP) elif user.groups.filter(name=settings.LVEM_OBSERVERS_GROUP).exists(): -- GitLab From 230a077c53a977f83f36c06671ab1c80e9c4b9bc Mon Sep 17 00:00:00 2001 From: Alexander Pace <alexander.pace@ligo.org> Date: Tue, 2 Apr 2024 09:36:15 -0500 Subject: [PATCH 10/10] assume yes for apt-get upgrade --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index f989fc2f3..a02809054 100644 --- a/Dockerfile +++ b/Dockerfile @@ -11,7 +11,7 @@ RUN echo 'deb http://deb.debian.org/debian bookworm-backports main' > /etc/apt/s RUN echo 'deb http://apt.postgresql.org/pub/repos/apt bookworm-pgdg main' > /etc/apt/sources.list.d/pgdg.list RUN wget --quiet -O - https://www.postgresql.org/media/keys/ACCC4CF8.asc | apt-key add - RUN apt-get update && \ - apt-get upgrade && \ + apt-get --assume-yes upgrade && \ apt-get install --install-recommends --assume-yes \ apache2 \ gcc \ -- GitLab