From 2249b9307e9cbcb6f523c10ece54c73a3c29e808 Mon Sep 17 00:00:00 2001 From: Marc 'risson' Schmitt Date: Tue, 15 Oct 2024 14:15:24 +0200 Subject: [PATCH 1/3] core: expiring model: don't synchronously delete Signed-off-by: Marc 'risson' Schmitt --- authentik/core/models.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/authentik/core/models.py b/authentik/core/models.py index 4c8e247e7226..e4ad6cdac8fa 100644 --- a/authentik/core/models.py +++ b/authentik/core/models.py @@ -10,7 +10,7 @@ from django.contrib.auth.models import AbstractUser from django.contrib.auth.models import UserManager as DjangoUserManager from django.db import models -from django.db.models import Q, QuerySet, options +from django.db.models import QuerySet, options from django.db.models.constants import LOOKUP_SEP from django.http import HttpRequest from django.utils.functional import SimpleLazyObject, cached_property @@ -805,9 +805,7 @@ def expire_action(self, *args, **kwargs): def filter_not_expired(cls, **kwargs) -> QuerySet["Token"]: """Filer for tokens which are not expired yet or are not expiring, and match filters in `kwargs`""" - for obj in cls.objects.filter(**kwargs).filter(Q(expires__lt=now(), expiring=True)): - obj.delete() - return cls.objects.filter(**kwargs) + return cls.objects.filter(expires__gte=now(), expiring=True).filter(**kwargs) @property def is_expired(self) -> bool: From 863958b4d6af1d01c68713a32c252007317100fe Mon Sep 17 00:00:00 2001 From: Marc 'risson' Schmitt Date: Tue, 15 Oct 2024 14:22:25 +0200 Subject: [PATCH 2/3] make sure we don't break something else Signed-off-by: Marc 'risson' Schmitt --- authentik/core/models.py | 12 +++++++++++- authentik/core/tasks.py | 7 +------ authentik/stages/consent/stage.py | 2 ++ 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/authentik/core/models.py b/authentik/core/models.py index e4ad6cdac8fa..3848436439a3 100644 --- a/authentik/core/models.py +++ b/authentik/core/models.py @@ -805,7 +805,17 @@ def expire_action(self, *args, **kwargs): def filter_not_expired(cls, **kwargs) -> QuerySet["Token"]: """Filer for tokens which are not expired yet or are not expiring, and match filters in `kwargs`""" - return cls.objects.filter(expires__gte=now(), expiring=True).filter(**kwargs) + return cls.objects.filter(expires__gt=now(), expiring=True).filter(**kwargs) + + @classmethod + def delete_expired(cls) -> int: + objects = ( + cls.objects.all().exclude(expiring=False).exclude(expiring=True, expires__gt=now()) + ) + amount = objects.count() + for obj in objects: + obj.expire_action() + return amount @property def is_expired(self) -> bool: diff --git a/authentik/core/tasks.py b/authentik/core/tasks.py index 1a565d8f7f21..9ec5bdc60217 100644 --- a/authentik/core/tasks.py +++ b/authentik/core/tasks.py @@ -30,12 +30,7 @@ def clean_expired_models(self: SystemTask): messages = [] for cls in ExpiringModel.__subclasses__(): cls: ExpiringModel - objects = ( - cls.objects.all().exclude(expiring=False).exclude(expiring=True, expires__gt=now()) - ) - amount = objects.count() - for obj in objects: - obj.expire_action() + amount = cls.delete_expired() LOGGER.debug("Expired models", model=cls, amount=amount) messages.append(f"Expired {amount} {cls._meta.verbose_name_plural}") # Special case diff --git a/authentik/stages/consent/stage.py b/authentik/stages/consent/stage.py index 36648c899a1e..43702753b915 100644 --- a/authentik/stages/consent/stage.py +++ b/authentik/stages/consent/stage.py @@ -96,6 +96,8 @@ def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: if PLAN_CONTEXT_PENDING_USER in self.executor.plan.context: user = self.executor.plan.context[PLAN_CONTEXT_PENDING_USER] + # Remove expired consents to prevent database unique constraints errors + UserConsent.delete_expired() consent: UserConsent | None = UserConsent.filter_not_expired( user=user, application=application ).first() From 929e42d3f2cc393b4d20ac6d3336d1aa817e9087 Mon Sep 17 00:00:00 2001 From: Marc 'risson' Schmitt Date: Tue, 15 Oct 2024 14:51:20 +0200 Subject: [PATCH 3/3] fix other issues Signed-off-by: Marc 'risson' Schmitt --- authentik/core/models.py | 21 +++++++++++++-------- authentik/outposts/models.py | 30 +++++++++++++----------------- authentik/stages/consent/stage.py | 3 +-- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/authentik/core/models.py b/authentik/core/models.py index 3848436439a3..0c556348a76f 100644 --- a/authentik/core/models.py +++ b/authentik/core/models.py @@ -10,7 +10,7 @@ from django.contrib.auth.models import AbstractUser from django.contrib.auth.models import UserManager as DjangoUserManager from django.db import models -from django.db.models import QuerySet, options +from django.db.models import Q, QuerySet, options from django.db.models.constants import LOOKUP_SEP from django.http import HttpRequest from django.utils.functional import SimpleLazyObject, cached_property @@ -802,19 +802,24 @@ def expire_action(self, *args, **kwargs): return self.delete(*args, **kwargs) @classmethod - def filter_not_expired(cls, **kwargs) -> QuerySet["Token"]: + def _not_expired_filter(cls): + return Q(expires__gt=now(), expiring=True) | Q(expiring=False) + + @classmethod + def filter_not_expired(cls, delete_expired=False, **kwargs) -> QuerySet["ExpiringModel"]: """Filer for tokens which are not expired yet or are not expiring, and match filters in `kwargs`""" - return cls.objects.filter(expires__gt=now(), expiring=True).filter(**kwargs) + if delete_expired: + cls.delete_expired(**kwargs) + return cls.objects.filter(cls._not_expired_filter()).filter(**kwargs) @classmethod - def delete_expired(cls) -> int: - objects = ( - cls.objects.all().exclude(expiring=False).exclude(expiring=True, expires__gt=now()) - ) - amount = objects.count() + def delete_expired(cls, **kwargs) -> int: + objects = cls.objects.all().exclude(cls._not_expired_filter()).filter(**kwargs) + amount = 0 for obj in objects: obj.expire_action() + amount += 1 return amount @property diff --git a/authentik/outposts/models.py b/authentik/outposts/models.py index 4032892fe870..71459ec7e1dc 100644 --- a/authentik/outposts/models.py +++ b/authentik/outposts/models.py @@ -9,7 +9,7 @@ from dacite.core import from_dict from django.contrib.auth.models import Permission from django.core.cache import cache -from django.db import IntegrityError, models, transaction +from django.db import models, transaction from django.db.models.base import Model from django.utils.translation import gettext_lazy as _ from guardian.models import UserObjectPermission @@ -380,26 +380,22 @@ def token(self) -> Token: """Get/create token for auto-generated user""" managed = f"goauthentik.io/outpost/{self.token_identifier}" tokens = Token.filter_not_expired( + delete_expired=True, identifier=self.token_identifier, intent=TokenIntents.INTENT_API, managed=managed, ) - if tokens.exists(): - return tokens.first() - try: - return Token.objects.create( - user=self.user, - identifier=self.token_identifier, - intent=TokenIntents.INTENT_API, - description=f"Autogenerated by authentik for Outpost {self.name}", - expiring=False, - managed=managed, - ) - except IntegrityError: - # Integrity error happens mostly when managed is reused - Token.objects.filter(managed=managed).delete() - Token.objects.filter(identifier=self.token_identifier).delete() - return self.token + token: Token | None = tokens.first() + if token: + return token + return Token.objects.create( + user=self.user, + identifier=self.token_identifier, + intent=TokenIntents.INTENT_API, + description=f"Autogenerated by authentik for Outpost {self.name}", + expiring=False, + managed=managed, + ) def get_required_objects(self) -> Iterable[models.Model | str]: """Get an iterator of all objects the user needs read access to""" diff --git a/authentik/stages/consent/stage.py b/authentik/stages/consent/stage.py index 43702753b915..8bbe2f4416d2 100644 --- a/authentik/stages/consent/stage.py +++ b/authentik/stages/consent/stage.py @@ -97,9 +97,8 @@ def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: user = self.executor.plan.context[PLAN_CONTEXT_PENDING_USER] # Remove expired consents to prevent database unique constraints errors - UserConsent.delete_expired() consent: UserConsent | None = UserConsent.filter_not_expired( - user=user, application=application + delete_expired=True, user=user, application=application ).first() self.executor.plan.context[PLAN_CONTEXT_CONSENT] = consent