-
Notifications
You must be signed in to change notification settings - Fork 167
Replace login keys with one-time passwords #2643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0f53e66
3790f49
de1d269
3765007
0321fea
1435b51
8110c51
60d537e
37a4f48
463952b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| # Generated by Django 5.2.11 on 2026-02-14 11:35 | ||
|
|
||
| import django.db.models.deletion | ||
| from django.conf import settings | ||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("evaluation", "0162_unified_questions_from_tmp_relation"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RemoveField( | ||
| model_name="userprofile", | ||
| name="login_key", | ||
| ), | ||
| migrations.RemoveField( | ||
| model_name="userprofile", | ||
| name="login_key_valid_until", | ||
| ), | ||
| migrations.CreateModel( | ||
| name="OtpHash", | ||
| fields=[ | ||
| ("id", models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), | ||
| ("otp_hash", models.CharField(max_length=256, unique=True, verbose_name="Hashed OTP")), | ||
| ("valid_until", models.DateTimeField(verbose_name="Valid until")), | ||
| ( | ||
| "user", | ||
| models.ForeignKey( | ||
| on_delete=django.db.models.deletion.CASCADE, | ||
| related_name="otp_hashes", | ||
| to=settings.AUTH_USER_MODEL, | ||
| ), | ||
| ), | ||
| ], | ||
| options={ | ||
| "verbose_name": "OTP hash", | ||
| "verbose_name_plural": "OTP hashes", | ||
| }, | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ | |
|
|
||
| from django.conf import settings | ||
| from django.contrib import messages | ||
| from django.contrib.auth.hashers import check_password, is_password_usable, make_password | ||
| from django.contrib.auth.hashers import PBKDF2PasswordHasher, check_password, is_password_usable, make_password | ||
| from django.contrib.auth.models import BaseUserManager, Group, PermissionsMixin | ||
| from django.contrib.auth.password_validation import validate_password | ||
| from django.contrib.postgres.fields import ArrayField | ||
|
|
@@ -1895,12 +1895,6 @@ class UserProfile(EvapBaseUser, PermissionsMixin): | |
| help_text=_("Technical user that represents a group of users."), | ||
| ) | ||
|
|
||
| # key for url based login of this user | ||
| MAX_LOGIN_KEY = 2**31 - 1 | ||
|
|
||
| login_key = models.IntegerField(verbose_name=_("Login Key"), unique=True, blank=True, null=True) | ||
| login_key_valid_until = models.DateField(verbose_name=_("Login Key Validity"), blank=True, null=True) | ||
|
|
||
| is_active = models.BooleanField(default=True, verbose_name=_("active")) | ||
|
|
||
| notes = models.TextField(verbose_name=_("notes"), blank=True, default="", max_length=1024 * 1024) | ||
|
|
@@ -2110,30 +2104,10 @@ def email_needs_login_key(email): | |
| def needs_login_key(self): | ||
| return UserProfile.email_needs_login_key(self.email) | ||
|
|
||
| def ensure_valid_login_key(self): | ||
| if self.login_key and self.login_key_valid_until > date.today(): | ||
| self.reset_login_key_validity() | ||
| return | ||
|
|
||
| while True: | ||
| key = secrets.choice(range(UserProfile.MAX_LOGIN_KEY)) | ||
| try: | ||
| self.login_key = key | ||
| self.reset_login_key_validity() | ||
| break | ||
| except IntegrityError: | ||
| # unique constraint failed, the login key was already in use. Generate another one. | ||
| continue | ||
|
|
||
| def reset_login_key_validity(self): | ||
| self.login_key_valid_until = date.today() + timedelta(settings.LOGIN_KEY_VALIDITY) | ||
| self.save() | ||
|
|
||
| @property | ||
| def login_url(self): | ||
| if not self.needs_login_key: | ||
| return "" | ||
| return settings.PAGE_URL + reverse("evaluation:login_key_authentication", args=[self.login_key]) | ||
| def generate_login_url(self, *, typeable: bool = False) -> str: | ||
| assert self.needs_login_key | ||
| otp = OtpHash.create(self, typeable=typeable) | ||
| return settings.PAGE_URL + reverse("evaluation:otp_authentication", kwargs={"otp": otp}) | ||
|
|
||
| def get_sorted_courses_responsible_for(self): | ||
| return self.courses_responsible_for.order_by("semester__created_at", "name_de") | ||
|
|
@@ -2157,6 +2131,80 @@ def get_sorted_due_evaluations(self): | |
| return sorted(evaluations_and_days_left, key=lambda tup: (tup[1], tup[0].full_name)) | ||
|
|
||
|
|
||
| class OtpHash(models.Model): | ||
| """Stores hashed one-time passwords (OTPs) for external user login via URL.""" | ||
|
|
||
| _hasher = PBKDF2PasswordHasher() | ||
|
|
||
| user = models.ForeignKey(UserProfile, on_delete=models.CASCADE, related_name="otp_hashes") | ||
| otp_hash = models.CharField(max_length=256, unique=True, verbose_name=_("Hashed OTP")) | ||
| valid_until = models.DateTimeField(verbose_name=_("Valid until")) | ||
|
|
||
| class Meta: | ||
| verbose_name = _("OTP hash") | ||
| verbose_name_plural = _("OTP hashes") | ||
|
|
||
| @classmethod | ||
| def enforce_otp_count_limit(cls, user: UserProfile) -> None: | ||
| """Keep only the newest (limit - 1) OTPs for the user, delete the rest.""" | ||
| ids_to_keep = cls.objects.filter(user=user).order_by("-valid_until")[: settings.MAX_OTPS_PER_USER - 1] | ||
| cls.objects.filter(user=user).exclude(id__in=ids_to_keep).delete() | ||
|
|
||
| @classmethod | ||
| def create(cls, user: UserProfile, *, typeable: bool = False) -> str: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we also have the |
||
| """Create a new OTP for the given user. Returns the raw OTP. | ||
|
|
||
| If typeable is True, uses the TYPEABLE variants of | ||
| OTP_ALPHABET, OTP_LENGTH, and OTP_VALIDITY. | ||
| """ | ||
| cls.enforce_otp_count_limit(user) | ||
|
|
||
| if typeable: | ||
| alphabet = settings.OTP_ALPHABET_TYPEABLE | ||
| length = settings.OTP_LENGTH_TYPEABLE | ||
| validity = settings.OTP_VALIDITY_TYPEABLE | ||
| else: | ||
| alphabet = settings.OTP_ALPHABET | ||
| length = settings.OTP_LENGTH | ||
| validity = settings.OTP_VALIDITY | ||
|
|
||
| for _i in range(10): | ||
| raw_otp = "".join(secrets.choice(alphabet) for _ in range(length)) | ||
| try: | ||
| cls.objects.create( | ||
| user=user, | ||
| otp_hash=cls.hash_otp(raw_otp), | ||
| valid_until=now() + validity, | ||
| ) | ||
| return raw_otp | ||
| except IntegrityError: | ||
| # unique constraint failed, this OTP is already in use. Generate another one. | ||
| continue | ||
| raise RuntimeError("Failed to create OTP after 10 attempts") | ||
|
|
||
| @classmethod | ||
| def hash_otp(cls, otp: str) -> str: | ||
| # fixed iterations so that hashes are stable and can be queried for directly | ||
| # the salt needs to be static for the same reason, thus providing no additional security. | ||
| return cls._hasher.encode(otp, salt="otp", iterations=settings.OTP_HASH_ITERATIONS) | ||
|
|
||
| @classmethod | ||
| def get(cls, otp: str) -> "OtpHash | None": | ||
| otp_hash = cls.hash_otp(otp) | ||
| try: | ||
| return cls.objects.select_related("user").get(otp_hash=otp_hash) | ||
| except cls.DoesNotExist: | ||
| return None | ||
|
|
||
| def is_valid(self) -> bool: | ||
| return self.valid_until >= now() | ||
|
|
||
| def invalidate(self) -> None: | ||
| # send it way back so enforce_otp_count_limit deletes it before any active OTPs | ||
| self.valid_until = now() - timedelta(days=365) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| self.save() | ||
|
|
||
|
|
||
| def validate_template(value): | ||
| """Field validator which ensures that the value can be compiled into a | ||
| Django Template.""" | ||
|
|
@@ -2306,9 +2354,8 @@ def send_to_user( | |
| send_separate_login_url = False | ||
| body_params["login_url"] = "" | ||
| if user.needs_login_key: | ||
| user.ensure_valid_login_key() | ||
| if not cc_addresses: | ||
| body_params["login_url"] = user.login_url | ||
| body_params["login_url"] = user.generate_login_url() | ||
| else: | ||
| send_separate_login_url = True | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we call this from the user merge code we want to keep
limit, notlimit - 1; should we make this a parameter here and call it withsettings.MAX_OTPS_PER_USER - 1increate, or should we always usesettings.MAX_OTPS_PER_USERhere and call this only after creating the new otp?