From 27dce7e736282c4554b89c05b0396e9414bfed8f Mon Sep 17 00:00:00 2001 From: Hugh Enxing Date: Tue, 22 Aug 2023 10:49:51 -0400 Subject: [PATCH 1/3] Resets `User.failed_login_count` on password reset from anywhere --- api/main/models.py | 112 ++++++++++++++++++------------------- api/main/notify.py | 25 ++++----- api/main/rest/media.py | 1 - api/main/rest/transcode.py | 1 - api/main/views.py | 63 ++++++++------------- 5 files changed, 89 insertions(+), 113 deletions(-) diff --git a/api/main/models.py b/api/main/models.py index bdc75b062..abb31063a 100644 --- a/api/main/models.py +++ b/api/main/models.py @@ -372,36 +372,32 @@ def get_description(self): ) -@receiver(post_save, sender=User) -def user_save(sender, instance, created, **kwargs): +def block_user_save_email(instance, method, *args, **kwargs): # Create random attribute name with static prefix for determining if this is the root trigger of # this signal attr_prefix = "_saving_" random_attr = f"{attr_prefix}{''.join(random.sample(string.ascii_lowercase, 16))}" + # Adds random attribute to suppress email from save during creation, then removes it + setattr(instance, random_attr, True) + getattr(instance, method)(*args, **kwargs) + delattr(instance, random_attr) + + +@receiver(post_save, sender=User) +def user_save(sender, instance, created, **kwargs): if os.getenv("COGNITO_ENABLED") == "TRUE": if created: - # Adds random attribute to suppress email from save during creation, then removes it - setattr(instance, random_attr, True) - instance.move_to_cognito() - delattr(instance, random_attr) + block_user_save_email(instance, "move_to_cognito") else: TatorCognito().update_attributes(instance) if created: if instance.username: instance.username = instance.username.strip() - - # Adds random attribute to suppress email from save during creation, then removes it - setattr(instance, random_attr, True) - instance.save() - delattr(instance, random_attr) + block_user_save_email(instance, "save") if settings.SAML_ENABLED and not instance.email: instance.email = instance.username - - # Adds random attribute to suppress email from save during creation, then removes it - setattr(instance, random_attr, True) - instance.save() - delattr(instance, random_attr) + block_user_save_email(instance, "save") invites = Invitation.objects.filter(email=instance.email, status="Pending") if (invites.count() == 0) and (os.getenv("AUTOCREATE_ORGANIZATIONS")): organization = Organization.objects.create(name=f"{instance}'s Team") @@ -413,49 +409,53 @@ def user_pre_save(sender, instance, **kwargs): # Prefix for random attribute name to determine if this is the root trigger of this signal attr_prefix = "_saving_" user_desc = instance.get_description() - is_root = all(not attr.startswith(attr_prefix) for attr in dir(instance)) - created = not instance.pk - - if created: - msg = ( - f"You are being notified that a new user {instance} (username {instance.username}, " - f"email {instance.email}) has been added to the Tator deployment with the following " - f"attributes:\n\n{user_desc}" - ) - is_monitored = True - else: - msg = ( - f"You are being notified that an existing user {instance} been modified with the " - f"following values:\n\n{user_desc}" - ) - - # Only send an email if this is the root `post_save` trigger, i.e. does not have a random - # attribute added to it, and is a modification of a monitored field. - original_instance = type(instance).objects.get(pk=instance.id) - monitored_fields = [ - "username", - "first_name", - "last_name", - "email", - "is_staff", - "profile", - "password", - ] - is_monitored = any( - getattr(instance, fieldname, None) != getattr(original_instance, fieldname, None) - for fieldname in monitored_fields - ) + if all(not attr.startswith(attr_prefix) for attr in dir(instance)): + created = not instance.pk + if created: + msg = ( + f"You are being notified that a new user {instance} (username {instance.username}, " + f"email {instance.email}) has been added to the Tator deployment with the " + f"following attributes:\n\n{user_desc}" + ) + is_monitored = True + password_modified = False + else: + msg = ( + f"You are being notified that an existing user {instance} been modified with the " + f"following values:\n\n{user_desc}" + ) - if is_root and is_monitored: - logger.info(msg) - email_service = get_email_service() - if email_service: - email_service.email_staff( - sender=settings.TATOR_EMAIL_SENDER, - title=f"{'Created' if created else 'Modified'} user", - text=msg, + # Only send an email if this is the root `pre_save` trigger, i.e. does not have a random + # attribute added to it, and is a modification of a monitored field. + original_instance = type(instance).objects.get(pk=instance.id) + monitored_fields = [ + "username", + "first_name", + "last_name", + "email", + "is_staff", + "profile", + "password", + ] + password_modified = instance.password != original_instance.password + is_monitored = password_modified or any( + getattr(instance, fieldname, None) != getattr(original_instance, fieldname, None) + for fieldname in monitored_fields ) + if is_monitored: + if password_modified: + instance.last_failed_login = 0 + block_user_save_email(instance, "save") + logger.info(msg) + email_service = get_email_service() + if email_service: + email_service.email_staff( + sender=settings.TATOR_EMAIL_SENDER, + title=f"{'Created' if created else 'Modified'} user", + text=msg, + ) + @receiver(post_delete, sender=User) def user_post_delete(sender, instance, **kwargs): diff --git a/api/main/notify.py b/api/main/notify.py index e71d2bdba..6cf1ffcb4 100644 --- a/api/main/notify.py +++ b/api/main/notify.py @@ -12,38 +12,35 @@ class Notify: + @staticmethod def notification_enabled(): """Returns true if notification is enabled""" return settings.TATOR_SLACK_TOKEN and settings.TATOR_SLACK_CHANNEL - def notify_admin_msg(msg): + @classmethod + def notify_admin_msg(cls, msg): """Sends a given message to administrators""" try: - if Notify.notification_enabled(): + if cls.notification_enabled(): client = slack.WebClient(token=settings.TATOR_SLACK_TOKEN) response = client.chat_postMessage(channel=settings.TATOR_SLACK_CHANNEL, text=msg) - if response["ok"]: - return True - else: - return False + return bool(response["ok"]) except: - logger.warning("Slack Comms failed") + logger.warning("Slack Comms failed", exc_info=True) return False - def notify_admin_file(title, content): + @classmethod + def notify_admin_file(cls, title, content): """Send a given file to administrators""" try: - if Notify.notification_enabled(): + if cls.notification_enabled(): client = slack.WebClient(token=settings.TATOR_SLACK_TOKEN) response = client.files_upload( channels=settings.TATOR_SLACK_CHANNEL, content=content, title=title ) - if response["ok"]: - return True - else: - return False + return bool(response["ok"]) except: - logger.warning("Slack Comms failed") + logger.warning("Slack Comms failed", exc_info=True) return False diff --git a/api/main/rest/media.py b/api/main/rest/media.py index 2f1e53533..aa29c6fb1 100644 --- a/api/main/rest/media.py +++ b/api/main/rest/media.py @@ -32,7 +32,6 @@ ) from ..schema import MediaListSchema, MediaDetailSchema, parse from ..schema.components import media as media_schema -from ..notify import Notify from ..download import download_file from ..store import get_tator_store, get_storage_lookup from ..cache import TatorCache diff --git a/api/main/rest/transcode.py b/api/main/rest/transcode.py index 8c8bbd9a0..826d4ca82 100644 --- a/api/main/rest/transcode.py +++ b/api/main/rest/transcode.py @@ -15,7 +15,6 @@ from ..models import Media from ..schema import TranscodeListSchema from ..schema import TranscodeDetailSchema -from ..notify import Notify from .media import _create_media from ._util import url_to_key diff --git a/api/main/views.py b/api/main/views.py index 706163202..264eebb4c 100644 --- a/api/main/views.py +++ b/api/main/views.py @@ -7,23 +7,17 @@ from django.core.exceptions import PermissionDenied from django.http import HttpResponse from django.http import JsonResponse -from rest_framework.authtoken.models import Token from django.contrib.auth.models import AnonymousUser from django.conf import settings from django.template.response import TemplateResponse from rest_framework.authentication import TokenAuthentication -import yaml from .models import Project from .models import Membership -from .models import Affiliation -from .models import Invitation -from .models import User from .notify import Notify from .cache import TatorCache -import os import logging import sys @@ -34,10 +28,7 @@ def check_login(request): - if request.user.is_authenticated: - return JsonResponse({"is_authenticated": True}) - else: - return JsonResponse({"is_authenticated": False}) + return JsonResponse({"is_authenticated": bool(request.user.is_authenticated)}) class LoginRedirect(View): @@ -65,7 +56,7 @@ def get_context_data(self, **kwargs): # Check if user is part of project. if not project.has_user(self.request.user.pk): - raise PermissionDenied + raise PermissionDenied(f"User {self.request.user} does not have access to {project.id}") return context @@ -132,16 +123,14 @@ def dispatch(self, request, *args, **kwargs): user = request.user if isinstance(user, AnonymousUser): try: - (user, token) = TokenAuthentication().authenticate(request) - except Exception as e: + user, _ = TokenAuthentication().authenticate(request) + except Exception: msg = "*Security Alert:* " msg += f"Bad credentials presented for '{original_url}' ({user})" Notify.notify_admin_msg(msg) - logger.warn(msg) + logger.warn(msg, exc_info=True) return HttpResponse(status=403) - filename = os.path.basename(original_url) - project = None try: comps = original_url.split("/") @@ -150,21 +139,19 @@ def dispatch(self, request, *args, **kwargs): project_id = comps[3] project = Project.objects.get(pk=project_id) authorized = validate_project(user, project) - except Exception as e: - logger.info(f"ERROR: {e}") + except Exception: + logger.info("Could not validate project access", exc_info=True) authorized = False if authorized: return HttpResponse(status=200) - else: - # Files that aren't in the whitelist or database are forbidden - msg = f"({user}/{user.id}): " - msg += f"Attempted to access unauthorized file '{original_url}'" - msg += f". " - msg += f"Does not have access to '{project}'" - Notify.notify_admin_msg(msg) - return HttpResponse(status=403) + # Files that aren't in the whitelist or database are forbidden + msg = ( + f"({user}/{user.id}): Attempted to access unauthorized file '{original_url}'; does not " + f"have access to '{project}'" + ) + Notify.notify_admin_msg(msg) return HttpResponse(status=403) @@ -183,23 +170,18 @@ def dispatch(self, request, *args, **kwargs): user = request.user if isinstance(user, AnonymousUser): try: - (user, token) = TokenAuthentication().authenticate(request) - except Exception as e: - msg = "*Security Alert:* " - msg += f"Bad credentials presented for '{original_url}'" + user, _ = TokenAuthentication().authenticate(request) + except Exception: + msg = f"*Security Alert:* Bad credentials presented for '{original_url}'" Notify.notify_admin_msg(msg) return HttpResponse(status=403) if user.is_staff: return HttpResponse(status=200) - else: - # Files that aren't in the whitelist or database are forbidden - msg = f"({user}/{user.id}): " - msg += f"Attempted to access unauthorized URL '{original_url}'" - msg += f"." - Notify.notify_admin_msg(msg) - return HttpResponse(status=403) + # Files that aren't in the whitelist or database are forbidden + msg = f"({user}/{user.id}): Attempted to access unauthorized URL '{original_url}'." + Notify.notify_admin_msg(msg) return HttpResponse(status=403) @@ -214,11 +196,10 @@ def ErrorNotifierView(request, code, message, details=None): # Generate slack message if Notify.notification_enabled(): - msg = f"{request.get_host()}:" - msg += f" ({request.user}/{request.user.id})" - msg += f" caused {code} at {request.get_full_path()}" + user = request.user + msg = f"{request.get_host()}: ({user}/{user.id}) caused {code} at {request.get_full_path()}" if details: - Notify.notify_admin_file(msg, msg + "\n" + details) + Notify.notify_admin_file(msg, f"{msg}\n{details}") else: if code == 404 and isinstance(request.user, AnonymousUser): logger.warn(msg) From 10bbd50efb3dfdb4b3520c5c5ffffe8e70913d76 Mon Sep 17 00:00:00 2001 From: Hugh Enxing Date: Tue, 22 Aug 2023 10:50:32 -0400 Subject: [PATCH 2/3] Updates `tator-py` submodule --- api/main/models.py | 2 +- scripts/packages/tator-py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/main/models.py b/api/main/models.py index abb31063a..0f3e125ab 100644 --- a/api/main/models.py +++ b/api/main/models.py @@ -445,7 +445,7 @@ def user_pre_save(sender, instance, **kwargs): if is_monitored: if password_modified: - instance.last_failed_login = 0 + instance.failed_login_count = 0 block_user_save_email(instance, "save") logger.info(msg) email_service = get_email_service() diff --git a/scripts/packages/tator-py b/scripts/packages/tator-py index fd277af3f..ea2360099 160000 --- a/scripts/packages/tator-py +++ b/scripts/packages/tator-py @@ -1 +1 @@ -Subproject commit fd277af3f83383341266daf412dfdda87b0ff174 +Subproject commit ea236009939d9b55859fd0bc3d26f8b707b63a26 From 1bac2bd05be28beefae4119a5db93e724375c3eb Mon Sep 17 00:00:00 2001 From: Hugh Enxing Date: Tue, 22 Aug 2023 16:13:44 -0400 Subject: [PATCH 3/3] Minor refactoring for clarity --- api/main/auth.py | 57 +++++++++++++++++---------------- api/main/rest/password_reset.py | 30 ++++++++--------- 2 files changed, 45 insertions(+), 42 deletions(-) diff --git a/api/main/auth.py b/api/main/auth.py index 93f14d86c..9a8d75087 100644 --- a/api/main/auth.py +++ b/api/main/auth.py @@ -46,10 +46,11 @@ def authenticate(self, request, username=None, password=None, **kwargs): user.save() lockout_lifted = user.last_failed_login + LOCKOUT_TIME time_left = lockout_lifted - now - msg = f" *SECURITY ALERT:* Attempt to login during lockout" - msg += f" User={user}/{user.id}" - msg += f" Attempt count = {user.failed_login_count}" - msg += f" Lockout will be lifted in '{time_left}' at '{lockout_lifted}'" + msg = ( + f" *SECURITY ALERT:* Attempt to login during lockout: User={user}/{user.id}, " + f" Attempt count = {user.failed_login_count}. Lockout will be lifted in " + f"'{time_left}' at '{lockout_lifted}'" + ) Notify.notify_admin_msg(msg) # Run the default password hasher once to reduce the timing # difference (#20760). @@ -59,30 +60,32 @@ def authenticate(self, request, username=None, password=None, **kwargs): if user.check_password(password) and self.user_can_authenticate(user): user.last_login = datetime.now(timezone.utc) if user.failed_login_count >= LOCKOUT_LIMIT: - msg = "Login proceeded after lock expiry" - msg += f" User={user}/{user.id}" + msg = "Login proceeded after lock expiry User={user}/{user.id}" Notify.notify_admin_msg(msg) user.failed_login_count = 0 user.save() return user - else: - user.last_failed_login = datetime.now(timezone.utc) - user.failed_login_count += 1 - user.save() - if user.failed_login_count >= LOCKOUT_LIMIT: - msg = f"*SECURITY ALERT:* Bad Login Attempt for {user}/{user.id}" - msg += f" Attempt count = {user.failed_login_count}" - Notify.notify_admin_msg(msg) - # Send an email if the failed login count has been reached. - if (user.failed_login_count == LOCKOUT_LIMIT) and settings.TATOR_EMAIL_ENABLED: - get_email_service().email( - sender=settings.TATOR_EMAIL_SENDER, - recipients=[user.email], - title=f"Tator account has been locked", - text="This message is to notify you that your Tator account (username " - f"{user.username}) has been locked due to {LOCKOUT_LIMIT} failed logins. " - "Your account will be unlocked automatically after 10 minutes, or you " - "can unlock your account now by resetting your password. To reset your " - "password, follow the procedure described here:\n\n" - "https://tator.io/tutorials/2021-06-11-reset-your-password/", - ) + + user.last_failed_login = datetime.now(timezone.utc) + user.failed_login_count += 1 + user.save() + if user.failed_login_count >= LOCKOUT_LIMIT: + msg = ( + f"*SECURITY ALERT:* Bad Login Attempt for {user}/{user.id}. Attempt count = " + f"{user.failed_login_count}" + ) + Notify.notify_admin_msg(msg) + # Send an email if the failed login count has been reached. + email_service = get_email_service() + if user.failed_login_count == LOCKOUT_LIMIT and email_service: + email_service.email( + sender=settings.TATOR_EMAIL_SENDER, + recipients=[user.email], + title=f"Tator account has been locked", + text="This message is to notify you that your Tator account (username " + f"{user.username}) has been locked due to {LOCKOUT_LIMIT} failed logins. " + "Your account will be unlocked automatically after 10 minutes, or you " + "can unlock your account now by resetting your password. To reset your " + "password, follow the procedure described here:\n\n" + "https://tator.io/tutorials/2021-06-11-reset-your-password/", + ) diff --git a/api/main/rest/password_reset.py b/api/main/rest/password_reset.py index 5ef86bd9b..d37079a54 100644 --- a/api/main/rest/password_reset.py +++ b/api/main/rest/password_reset.py @@ -1,15 +1,11 @@ -import uuid -import os import logging +import uuid -from django.db import transaction from django.conf import settings -from django.http import Http404 -from ..models import PasswordReset -from ..models import User -from ..schema import PasswordResetListSchema from ..mail import get_email_service +from ..models import PasswordReset, User +from ..schema import PasswordResetListSchema from ._base_views import BaseListView @@ -31,17 +27,21 @@ def _post(self, params): raise RuntimeError(f"Email {email} is in use by multiple users!") user = users[0] reset = PasswordReset(user=user, reset_token=uuid.uuid1()) - url = f"{os.getenv('MAIN_HOST')}/password-reset?reset_token={reset.reset_token}&user={user.id}" - if settings.TATOR_EMAIL_ENABLED: - get_email_service().email( + url = f"{settings.PROTO}://{settings.MAIN_HOST}/password-reset?reset_token={reset.reset_token}&user={user.id}" + email_service = get_email_service() + if email_service: + text = ( + f"A password reset has been requested for this email address ({email}). If you did " + f"not initiate the reset this message can be ignored. To reset your password, " + f"please visit: \n\n{url}\n\nThis URL will expire in 24 hours." + ) + failure_msg = f"Unable to send email to {email}! Password reset creation failed." + email_service.email( sender=settings.TATOR_EMAIL_SENDER, recipients=[email], title=f"Tator password reset", - text=f"A password reset has been requested for this email address ({email}). " - f"If you did not initiate the reset this message can be ignored. " - f"To reset your password, please visit: \n\n{url}\n\n" - "This URL will expire in 24 hours.", - raise_on_failure=f"Unable to send email to {email}! Password reset creation failed.", + text=text, + raise_on_failure=failure_msg, ) else: raise RuntimeError(