From edf330094472e91b21b0253539b1410247be33be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simonyi=20Gerg=C5=91?= <28359278+gergosimonyi@users.noreply.github.com> Date: Mon, 14 Apr 2025 16:18:59 +0200 Subject: [PATCH] policies/reputation: limit reputation score (#14008) * add limits to reputation score * limit reputation score limits Upper to non-negative, Lower to non-positive * simplify tests * "fix" bandit false-positives * move magic numbers to constants Is it too much to ask for a world in which I can just import these straight from Python? --- authentik/policies/reputation/signals.py | 22 ++++++++-- authentik/policies/reputation/tests.py | 44 ++++++++++++------- authentik/tenants/api/settings.py | 2 + ..._tenant_reputation_lower_limit_and_more.py | 32 ++++++++++++++ authentik/tenants/models.py | 14 +++++- schema.yml | 33 ++++++++++++++ .../admin/admin-settings/AdminSettingsForm.ts | 23 ++++++++++ 7 files changed, 150 insertions(+), 20 deletions(-) create mode 100644 authentik/tenants/migrations/0005_tenant_reputation_lower_limit_and_more.py diff --git a/authentik/policies/reputation/signals.py b/authentik/policies/reputation/signals.py index 2092e3512d..802001af98 100644 --- a/authentik/policies/reputation/signals.py +++ b/authentik/policies/reputation/signals.py @@ -2,7 +2,6 @@ from django.contrib.auth.signals import user_logged_in from django.db import transaction -from django.db.models import F from django.dispatch import receiver from django.http import HttpRequest from structlog.stdlib import get_logger @@ -13,20 +12,29 @@ from authentik.events.context_processors.geoip import GEOIP_CONTEXT_PROCESSOR from authentik.policies.reputation.models import Reputation, reputation_expiry from authentik.root.middleware import ClientIPMiddleware from authentik.stages.identification.signals import identification_failed +from authentik.tenants.utils import get_current_tenant LOGGER = get_logger() +def clamp(value, min, max): + return sorted([min, value, max])[1] + + def update_score(request: HttpRequest, identifier: str, amount: int): """Update score for IP and User""" remote_ip = ClientIPMiddleware.get_client_ip(request) + tenant = get_current_tenant() + new_score = clamp(amount, tenant.reputation_lower_limit, tenant.reputation_upper_limit) with transaction.atomic(): reputation, created = Reputation.objects.select_for_update().get_or_create( ip=remote_ip, identifier=identifier, defaults={ - "score": amount, + "score": clamp( + amount, tenant.reputation_lower_limit, tenant.reputation_upper_limit + ), "ip_geo_data": GEOIP_CONTEXT_PROCESSOR.city_dict(remote_ip) or {}, "ip_asn_data": ASN_CONTEXT_PROCESSOR.asn_dict(remote_ip) or {}, "expires": reputation_expiry(), @@ -34,9 +42,15 @@ def update_score(request: HttpRequest, identifier: str, amount: int): ) if not created: - reputation.score = F("score") + amount + new_score = clamp( + reputation.score + amount, + tenant.reputation_lower_limit, + tenant.reputation_upper_limit, + ) + reputation.score = new_score reputation.save() - LOGGER.info("Updated score", amount=amount, for_user=identifier, for_ip=remote_ip) + + LOGGER.info("Updated score", amount=new_score, for_user=identifier, for_ip=remote_ip) @receiver(login_failed) diff --git a/authentik/policies/reputation/tests.py b/authentik/policies/reputation/tests.py index 50b7b5a196..1486d55574 100644 --- a/authentik/policies/reputation/tests.py +++ b/authentik/policies/reputation/tests.py @@ -6,9 +6,11 @@ from authentik.core.models import User from authentik.lib.generators import generate_id from authentik.policies.reputation.api import ReputationPolicySerializer from authentik.policies.reputation.models import Reputation, ReputationPolicy +from authentik.policies.reputation.signals import update_score from authentik.policies.types import PolicyRequest from authentik.stages.password import BACKEND_INBUILT from authentik.stages.password.stage import authenticate +from authentik.tenants.models import DEFAULT_REPUTATION_LOWER_LIMIT, DEFAULT_REPUTATION_UPPER_LIMIT class TestReputationPolicy(TestCase): @@ -17,36 +19,48 @@ class TestReputationPolicy(TestCase): def setUp(self): self.request_factory = RequestFactory() self.request = self.request_factory.get("/") - self.test_ip = "127.0.0.1" - self.test_username = "test" + self.ip = "127.0.0.1" + self.username = "username" + self.password = generate_id() # We need a user for the one-to-one in userreputation - self.user = User.objects.create(username=self.test_username) + self.user = User.objects.create(username=self.username) + self.user.set_password(self.password) self.backends = [BACKEND_INBUILT] def test_ip_reputation(self): """test IP reputation""" # Trigger negative reputation - authenticate( - self.request, self.backends, username=self.test_username, password=self.test_username - ) - self.assertEqual(Reputation.objects.get(ip=self.test_ip).score, -1) + authenticate(self.request, self.backends, username=self.username, password=self.username) + self.assertEqual(Reputation.objects.get(ip=self.ip).score, -1) def test_user_reputation(self): """test User reputation""" # Trigger negative reputation - authenticate( - self.request, self.backends, username=self.test_username, password=self.test_username - ) - self.assertEqual(Reputation.objects.get(identifier=self.test_username).score, -1) + authenticate(self.request, self.backends, username=self.username, password=self.username) + self.assertEqual(Reputation.objects.get(identifier=self.username).score, -1) def test_update_reputation(self): """test reputation update""" - Reputation.objects.create(identifier=self.test_username, ip=self.test_ip, score=43) + Reputation.objects.create(identifier=self.username, ip=self.ip, score=4) # Trigger negative reputation - authenticate( - self.request, self.backends, username=self.test_username, password=self.test_username + authenticate(self.request, self.backends, username=self.username, password=self.username) + self.assertEqual(Reputation.objects.get(identifier=self.username).score, 3) + + def test_reputation_lower_limit(self): + """test reputation lower limit""" + Reputation.objects.create(identifier=self.username, ip=self.ip) + update_score(self.request, identifier=self.username, amount=-1000) + self.assertEqual( + Reputation.objects.get(identifier=self.username).score, DEFAULT_REPUTATION_LOWER_LIMIT + ) + + def test_reputation_upper_limit(self): + """test reputation upper limit""" + Reputation.objects.create(identifier=self.username, ip=self.ip) + update_score(self.request, identifier=self.username, amount=1000) + self.assertEqual( + Reputation.objects.get(identifier=self.username).score, DEFAULT_REPUTATION_UPPER_LIMIT ) - self.assertEqual(Reputation.objects.get(identifier=self.test_username).score, 42) def test_policy(self): """Test Policy""" diff --git a/authentik/tenants/api/settings.py b/authentik/tenants/api/settings.py index 569995699d..d8d1624b8a 100644 --- a/authentik/tenants/api/settings.py +++ b/authentik/tenants/api/settings.py @@ -20,6 +20,8 @@ class SettingsSerializer(ModelSerializer): "default_user_change_email", "default_user_change_username", "event_retention", + "reputation_lower_limit", + "reputation_upper_limit", "footer_links", "gdpr_compliance", "impersonation", diff --git a/authentik/tenants/migrations/0005_tenant_reputation_lower_limit_and_more.py b/authentik/tenants/migrations/0005_tenant_reputation_lower_limit_and_more.py new file mode 100644 index 0000000000..95f72d47a6 --- /dev/null +++ b/authentik/tenants/migrations/0005_tenant_reputation_lower_limit_and_more.py @@ -0,0 +1,32 @@ +# Generated by Django 5.0.14 on 2025-04-14 07:50 + +import django.core.validators +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("authentik_tenants", "0004_tenant_impersonation_require_reason"), + ] + + operations = [ + migrations.AddField( + model_name="tenant", + name="reputation_lower_limit", + field=models.IntegerField( + default=-5, + help_text="Reputation cannot decrease lower than this value. Zero or negative.", + validators=[django.core.validators.MaxValueValidator(0)], + ), + ), + migrations.AddField( + model_name="tenant", + name="reputation_upper_limit", + field=models.IntegerField( + default=5, + help_text="Reputation cannot increase higher than this value. Zero or positive.", + validators=[django.core.validators.MinValueValidator(0)], + ), + ), + ] diff --git a/authentik/tenants/models.py b/authentik/tenants/models.py index 654009055b..bc3f9a464c 100644 --- a/authentik/tenants/models.py +++ b/authentik/tenants/models.py @@ -5,7 +5,7 @@ from uuid import uuid4 from django.apps import apps from django.core.exceptions import ValidationError -from django.core.validators import MinValueValidator +from django.core.validators import MaxValueValidator, MinValueValidator from django.db import models from django.db.utils import IntegrityError from django.dispatch import receiver @@ -25,6 +25,8 @@ VALID_SCHEMA_NAME = re.compile(r"^t_[a-z0-9]{1,61}$") DEFAULT_TOKEN_DURATION = "days=1" # nosec DEFAULT_TOKEN_LENGTH = 60 +DEFAULT_REPUTATION_LOWER_LIMIT = -5 +DEFAULT_REPUTATION_UPPER_LIMIT = 5 def _validate_schema_name(name): @@ -70,6 +72,16 @@ class Tenant(TenantMixin, SerializerModel): "Events will be deleted after this duration.(Format: weeks=3;days=2;hours=3,seconds=2)." ), ) + reputation_lower_limit = models.IntegerField( + help_text=_("Reputation cannot decrease lower than this value. Zero or negative."), + default=DEFAULT_REPUTATION_LOWER_LIMIT, + validators=[MaxValueValidator(0)], + ) + reputation_upper_limit = models.IntegerField( + help_text=_("Reputation cannot increase higher than this value. Zero or positive."), + default=DEFAULT_REPUTATION_UPPER_LIMIT, + validators=[MinValueValidator(0)], + ) footer_links = models.JSONField( help_text=_("The option configures the footer links on the flow executor pages."), default=list, diff --git a/schema.yml b/schema.yml index 73b2ecaf88..d98d1a6dec 100644 --- a/schema.yml +++ b/schema.yml @@ -53723,6 +53723,17 @@ components: type: string minLength: 1 description: 'Events will be deleted after this duration.(Format: weeks=3;days=2;hours=3,seconds=2).' + reputation_lower_limit: + type: integer + maximum: 0 + minimum: -2147483648 + description: Reputation cannot decrease lower than this value. Zero or negative. + reputation_upper_limit: + type: integer + maximum: 2147483647 + minimum: 0 + description: Reputation cannot increase higher than this value. Zero or + positive. footer_links: description: The option configures the footer links on the flow executor pages. @@ -57775,6 +57786,17 @@ components: event_retention: type: string description: 'Events will be deleted after this duration.(Format: weeks=3;days=2;hours=3,seconds=2).' + reputation_lower_limit: + type: integer + maximum: 0 + minimum: -2147483648 + description: Reputation cannot decrease lower than this value. Zero or negative. + reputation_upper_limit: + type: integer + maximum: 2147483647 + minimum: 0 + description: Reputation cannot increase higher than this value. Zero or + positive. footer_links: description: The option configures the footer links on the flow executor pages. @@ -57818,6 +57840,17 @@ components: type: string minLength: 1 description: 'Events will be deleted after this duration.(Format: weeks=3;days=2;hours=3,seconds=2).' + reputation_lower_limit: + type: integer + maximum: 0 + minimum: -2147483648 + description: Reputation cannot decrease lower than this value. Zero or negative. + reputation_upper_limit: + type: integer + maximum: 2147483647 + minimum: 0 + description: Reputation cannot increase higher than this value. Zero or + positive. footer_links: description: The option configures the footer links on the flow executor pages. diff --git a/web/src/admin/admin-settings/AdminSettingsForm.ts b/web/src/admin/admin-settings/AdminSettingsForm.ts index cfff45e783..f046084b73 100644 --- a/web/src/admin/admin-settings/AdminSettingsForm.ts +++ b/web/src/admin/admin-settings/AdminSettingsForm.ts @@ -23,6 +23,9 @@ import { AdminApi, FooterLink, Settings, SettingsRequest } from "@goauthentik/ap import "./AdminSettingsFooterLinks.js"; import { IFooterLinkInput, akFooterLinkInput } from "./AdminSettingsFooterLinks.js"; +const DEFAULT_REPUTATION_LOWER_LIMIT = -5; +const DEFAULT_REPUTATION_UPPER_LIMIT = 5; + @customElement("ak-admin-settings-form") export class AdminSettingsForm extends Form { // @@ -177,6 +180,26 @@ export class AdminSettingsForm extends Form { `} > + +