From 701fb4e23c7af15433c1218dba9af98ad2cff6f1 Mon Sep 17 00:00:00 2001 From: Viraat Chandra Date: Fri, 3 Feb 2023 13:28:22 -0800 Subject: [PATCH 01/72] view survey responses from Project detail view --- .../project/templates/project/project_detail.html | 10 ++++++++++ coldfront/core/project/views.py | 11 ++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/coldfront/core/project/templates/project/project_detail.html b/coldfront/core/project/templates/project/project_detail.html index ac50576e4..f36a836b8 100644 --- a/coldfront/core/project/templates/project/project_detail.html +++ b/coldfront/core/project/templates/project/project_detail.html @@ -138,6 +138,15 @@

{% endif %}

+ + + {% if survey_answers %} +

+ Survey Responses: + +

+ {% endif %} + @@ -466,4 +475,5 @@

+{% include 'project/project_request/savio/project_request_survey_modal.html' with survey_form=survey_answers %} {% endblock %} diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py index 6f1cc08e5..3c3037231 100644 --- a/coldfront/core/project/views.py +++ b/coldfront/core/project/views.py @@ -34,12 +34,14 @@ ProjectSearchForm, ProjectUpdateForm, ProjectUserUpdateForm) +from coldfront.core.project.forms_.new_project_forms.request_forms import SavioProjectSurveyForm from coldfront.core.project.models import (Project, ProjectReview, ProjectReviewStatusChoice, ProjectStatusChoice, ProjectUser, ProjectUserRoleChoice, ProjectUserStatusChoice, - ProjectUserRemovalRequest) + ProjectUserRemovalRequest, + SavioProjectAllocationRequest) from coldfront.core.project.utils import (annotate_queryset_with_cluster_name, is_primary_cluster_project) from coldfront.core.project.utils_.addition_utils import can_project_purchase_service_units @@ -269,6 +271,13 @@ def get_context_data(self, **kwargs): context['can_request_sec_dir'] = \ pi_eligible_to_request_secure_dir(self.request.user) + # show survey responses if available + allocation_request = SavioProjectAllocationRequest.objects.filter( + project=self.object).first() + if allocation_request: + context['survey_answers'] = SavioProjectSurveyForm( + initial=allocation_request.survey_answers, disable_fields=True) + context['user_agreement_signed'] = \ access_agreement_signed(self.request.user) From 8c4f709670fb46a991b13c42adf1e86baf2ae981 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 8 Feb 2023 11:41:23 -0800 Subject: [PATCH 02/72] Add first pass at hiding multiple email address-related functionality in the User Profile --- coldfront/core/user/views.py | 105 +++++++++++++++++++++++------------ 1 file changed, 71 insertions(+), 34 deletions(-) diff --git a/coldfront/core/user/views.py b/coldfront/core/user/views.py index cdd7cad9a..d7737cb1e 100644 --- a/coldfront/core/user/views.py +++ b/coldfront/core/user/views.py @@ -60,6 +60,10 @@ class UserProfile(TemplateView): template_name = 'user/user_profile.html' + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._get_flag_states() + def dispatch(self, request, *args, viewed_username=None, **kwargs): # viewing another user profile requires permissions if viewed_username: @@ -96,59 +100,92 @@ def get_context_data(self, viewed_username=None, **kwargs): requester_is_viewed_user = viewed_user == self.request.user if requester_is_viewed_user: - self.update_context_with_identity_linking_request_data(context) + self._update_context_with_identity_linking_request_data(context) context['help_email'] = import_from_settings('CENTER_HELP_EMAIL') - # Only display the "Other Email Addresses" section for - # coldfront.core.user.models.EmailAddress if basic auth. is enabled. - is_basic_auth_enabled = flag_enabled('BASIC_AUTH_ENABLED') context['requester_is_viewed_user'] = requester_is_viewed_user - context['primary_address_updatable'] = ( - is_basic_auth_enabled and requester_is_viewed_user) + + self._update_context_with_email_and_account_data(context, viewed_user) + + if self._flag_lrc_only: + self._update_context_with_billing_data(context, viewed_user) + + context['is_lbl_employee'] = is_lbl_employee(viewed_user) + + return context + + def _get_flag_states(self): + """Store the states of various flags needed by the class.""" + self._flag_basic_auth_enabled = flag_enabled('BASIC_AUTH_ENABLED') + self._flag_lrc_only = flag_enabled('LRC_ONLY') + self._flag_multiple_email_addresses_allowed = flag_enabled( + 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED') + self._flag_sso_enabled = flag_enabled('SSO_ENABLED') + + @staticmethod + def _update_context_with_billing_data(context, viewed_user): + """Update the given context dictionary with fields relating to + billing IDs. Take the currently-viewed User object as an input + to make determinations.""" + billing_id = 'N/A' + try: + user_profile = viewed_user.userprofile + except UserProfileModel.DoesNotExist: + message = ( + f'User {viewed_user.username} unexpectedly has no ' + f'UserProfile.') + logger.error(message) + else: + billing_activity = user_profile.billing_activity + if billing_activity: + billing_id = billing_activity.full_id() + context['monthly_user_account_fee_billing_id'] = billing_id + + def _update_context_with_email_and_account_data(self, context, + viewed_user): + """Update the given context directory with fields relating to + user emails, passwords, and third-party accounts. Take the + currently-viewed User object as an input to make + determinations.""" + requester_is_viewed_user = viewed_user == self.request.user + context['change_password_enabled'] = ( - is_basic_auth_enabled and requester_is_viewed_user) - context['core_user_email_addresses_visible'] = is_basic_auth_enabled + self._flag_basic_auth_enabled and requester_is_viewed_user) + + context['primary_address_updatable'] = ( + self._flag_basic_auth_enabled and + self._flag_multiple_email_addresses_allowed and + requester_is_viewed_user) + + # Use coldfront.core.user.models.EmailAddress if basic auth. is + # enabled. + context['core_user_email_addresses_visible'] = ( + self._flag_basic_auth_enabled and + self._flag_multiple_email_addresses_allowed) if context['core_user_email_addresses_visible']: context['other_emails'] = EmailAddress.objects.filter( user=viewed_user, is_primary=False).order_by('email') context['core_user_email_addresses_updatable'] = \ requester_is_viewed_user - # Only display the "Other Email Addresses" section for - # allauth.account.models.EmailAddress if SSO is enabled. - is_sso_enabled = flag_enabled('SSO_ENABLED') - context['allauth_email_addresses_visible'] = is_sso_enabled + # Use allauth.account.models.EmailAddress if SSO is enabled. + context['allauth_email_addresses_visible'] = ( + self._flag_sso_enabled and + self._flag_multiple_email_addresses_allowed) if context['allauth_email_addresses_visible']: context['allauth_email_addresses_updatable'] = \ requester_is_viewed_user - # Only display the "Third-Party Accounts" section if SSO is enabled. - context['third_party_accounts_visible'] = is_sso_enabled + # Display the "Third-Party Accounts" section if SSO is enabled. + context['third_party_accounts_visible'] = ( + self._flag_sso_enabled and + self._flag_multiple_email_addresses_allowed) if context['third_party_accounts_visible']: context['third_party_accounts_updatable'] = \ requester_is_viewed_user - if flag_enabled('LRC_ONLY'): - billing_id = 'N/A' - try: - user_profile = viewed_user.userprofile - except UserProfileModel.DoesNotExist: - message = ( - f'User {viewed_user.username} unexpectedly has no ' - f'UserProfile.') - logger.error(message) - else: - billing_activity = user_profile.billing_activity - if billing_activity: - billing_id = billing_activity.full_id() - context['monthly_user_account_fee_billing_id'] = billing_id - - context['is_lbl_employee'] = is_lbl_employee(viewed_user) - - return context - - def update_context_with_identity_linking_request_data(self, context): + def _update_context_with_identity_linking_request_data(self, context): """Update the given context dictionary with fields relating to IdentityLinkingRequests. From fed0684249e8a7eaff741d614d6a2603e3633a03 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Thu, 9 Feb 2023 10:35:15 -0800 Subject: [PATCH 03/72] Remove admin view for old EmailAddress model; integrate its functionality into admin view for new one --- coldfront/core/account/admin.py | 83 +++++++++++++++++++++++++++++++++ coldfront/core/user/admin.py | 81 -------------------------------- 2 files changed, 83 insertions(+), 81 deletions(-) create mode 100644 coldfront/core/account/admin.py diff --git a/coldfront/core/account/admin.py b/coldfront/core/account/admin.py new file mode 100644 index 000000000..c68a52a75 --- /dev/null +++ b/coldfront/core/account/admin.py @@ -0,0 +1,83 @@ +from django.contrib import admin +from django.contrib import messages +from django.core.exceptions import ValidationError + +from allauth.account.models import EmailAddress + +from coldfront.core.account.utils.queries import update_user_primary_email_address + +import logging + + +logger = logging.getLogger(__name__) + + +admin.site.unregister(EmailAddress) + + +@admin.register(EmailAddress) +class EmailAddressAdmin(admin.ModelAdmin): + + actions = ('make_primary', 'make_verified', ) + readonly_fields = ('primary', 'verified', ) + + def delete_model(self, request, obj): + if obj.primary: + raise ValidationError( + 'Cannot delete primary email. Unset primary status in list ' + 'display before deleting.') + else: + super().delete_model(request, obj) + + @admin.action(description='Make selected primary') + def make_primary(self, request, queryset): + """Set the EmailAddresses in the given queryset as the primary + addresses of their respective users. + + Currently, admins are limited to setting at most one address at + a time.""" + if queryset.count() > 1: + raise ValidationError( + 'Cannot set more than one primary email address at a time.') + for email_address in queryset: + user = email_address.user + try: + update_user_primary_email_address(email_address) + except ValueError: + raise ValidationError( + 'Cannot set an unverified email address as primary.') + except Exception as e: + message = ( + f'Encountered unexpected exception when updating User ' + f'{user.pk}\'s primary EmailAddress to ' + f'{email_address.pk}. Details:') + logger.error(message) + logger.exception(e) + raise ValidationError( + f'Failed to set {email_address.pk} as primary. See the ' + f'log for details.') + else: + message = ( + f'Set User {user.pk}\'s primary EmailAddress to ' + f'{email_address.email}.') + messages.success(request, message) + + def delete_queryset(self, request, queryset): + """Delete EmailAddresses in the given queryset, skipping those + that are primary.""" + num_primary, num_non_primary = 0, 0 + for email_address in queryset: + if email_address.primary: + num_primary = num_primary + 1 + else: + email_address.delete() + num_non_primary = num_non_primary + 1 + + success_message = ( + f'Deleted {num_non_primary} non-primary EmailAddresses.') + messages.success(request, success_message) + + if num_primary > 0: + error_message = ( + f'Skipped deleting {num_primary} primary EmailAddresses.') + messages.error(request, error_message) \ No newline at end of file diff --git a/coldfront/core/user/admin.py b/coldfront/core/user/admin.py index be0ae38a6..7f4a7c342 100644 --- a/coldfront/core/user/admin.py +++ b/coldfront/core/user/admin.py @@ -1,14 +1,6 @@ from django.contrib import admin -from django.contrib import messages -from django.core.exceptions import ValidationError from coldfront.core.user.models import UserProfile, EmailAddress -from coldfront.core.user.utils import update_user_primary_email_address - -import logging - - -logger = logging.getLogger(__name__) @admin.register(UserProfile) @@ -28,79 +20,6 @@ def last_name(self, obj): return obj.user.last_name -@admin.register(EmailAddress) -class EmailAddressAdmin(admin.ModelAdmin): - list_display = ('user', 'email', 'is_primary', 'is_verified', ) - ordering = ('user', '-is_primary', '-is_verified', 'email', ) - list_filter = ('is_primary', 'is_verified', ) - search_fields = [ - 'user__username', 'user__first_name', 'user__last_name', 'email'] - fields = ('email', 'user', 'is_primary', 'is_verified', ) - actions = ('make_primary', ) - readonly_fields = ('is_primary', 'is_verified', ) - - def delete_model(self, request, obj): - if obj.is_primary: - raise ValidationError( - 'Cannot delete primary email. Unset primary status in list ' - 'display before deleting.') - else: - super().delete_model(request, obj) - - @admin.action(description='Make selected primary') - def make_primary(self, request, queryset): - """Set the EmailAddresses in the given queryset as the primary - addresses of their respective users. - - Currently, admins are limited to setting at most one address at - a time.""" - if queryset.count() > 1: - raise ValidationError( - 'Cannot set more than one primary email address at a time.') - for email_address in queryset: - user = email_address.user - try: - update_user_primary_email_address(email_address) - except ValueError: - raise ValidationError( - 'Cannot set an unverified email address as primary.') - except Exception as e: - message = ( - f'Encountered unexpected exception when updating User ' - f'{user.pk}\'s primary EmailAddress to ' - f'{email_address.pk}. Details:') - logger.error(message) - logger.exception(e) - raise ValidationError( - f'Failed to set {email_address.pk} as primary. See the ' - f'log for details.') - else: - message = ( - f'Set User {user.pk}\'s primary EmailAddress to ' - f'{email_address.email}.') - messages.success(request, message) - - def delete_queryset(self, request, queryset): - """Delete EmailAddresses in the given queryset, skipping those - that are primary.""" - num_primary, num_non_primary = 0, 0 - for email_address in queryset: - if email_address.is_primary: - num_primary = num_primary + 1 - else: - email_address.delete() - num_non_primary = num_non_primary + 1 - - success_message = ( - f'Deleted {num_non_primary} non-primary EmailAddresses.') - messages.success(request, success_message) - - if num_primary > 0: - error_message = ( - f'Skipped deleting {num_primary} primary EmailAddresses.') - messages.error(request, error_message) - - class EmailAddressInline(admin.TabularInline): model = EmailAddress extra = 0 From 3987136596bdc0f5b29c27979d3dbb7d64b88184 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Thu, 9 Feb 2023 10:38:19 -0800 Subject: [PATCH 04/72] Remove redundant email views already provided by allauth; remove references to old EmailAddress in user views --- coldfront/core/user/forms.py | 29 --- .../user/user_add_email_address.html | 31 --- .../user/templates/user/user_profile.html | 65 ------ .../user_update_primary_email_address.html | 38 --- coldfront/core/user/urls.py | 18 -- coldfront/core/user/views.py | 221 +----------------- 6 files changed, 5 insertions(+), 397 deletions(-) delete mode 100644 coldfront/core/user/templates/user/user_add_email_address.html delete mode 100644 coldfront/core/user/templates/user/user_update_primary_email_address.html diff --git a/coldfront/core/user/forms.py b/coldfront/core/user/forms.py index ae742651d..ee29a014e 100644 --- a/coldfront/core/user/forms.py +++ b/coldfront/core/user/forms.py @@ -240,40 +240,11 @@ def set_acknowledgement_help_text(self): field.help_text = template.format('LBNL', 'LBNL IT Division') -class EmailAddressAddForm(forms.Form): - - email = forms.EmailField(max_length=100, required=True) - - def clean_email(self): - cleaned_data = super().clean() - email = cleaned_data['email'].lower() - if (User.objects.filter(email=email).exists() or - EmailAddress.objects.filter(email=email).exists()): - raise forms.ValidationError( - f'Email address {email} is already in use.') - return email - - class UserReactivateForm(forms.Form): email = forms.EmailField(max_length=100, required=True) -class PrimaryEmailAddressSelectionForm(forms.Form): - - email_address = forms.ModelChoiceField( - label='New Primary Email Address', - queryset=EmailAddress.objects.none(), - required=True, - widget=forms.RadioSelect()) - - def __init__(self, *args, **kwargs): - user = kwargs.pop('user') - super().__init__(*args, **kwargs) - self.fields['email_address'].queryset = EmailAddress.objects.filter( - user=user, is_verified=True, is_primary=False) - - class VerifiedEmailAddressPasswordResetForm(PasswordResetForm): """A subclass of django.contrib.auth.forms.PasswordResetForm that uses EmailAddress.""" diff --git a/coldfront/core/user/templates/user/user_add_email_address.html b/coldfront/core/user/templates/user/user_add_email_address.html deleted file mode 100644 index aae512d65..000000000 --- a/coldfront/core/user/templates/user/user_add_email_address.html +++ /dev/null @@ -1,31 +0,0 @@ -{% extends "common/base.html" %} -{% load crispy_forms_tags %} -{% load static %} - - -{% block title %} -Add Email Address -{% endblock %} - - -{% block content %} - -

Add an Email Address

-
- -

Associate another email address with your account. Once verified, you may authenticate to this portal using this address.

- -
-
-
- {% csrf_token %} - {{ form|crispy }} - - - Cancel - -
-
-
- -{% endblock %} diff --git a/coldfront/core/user/templates/user/user_profile.html b/coldfront/core/user/templates/user/user_profile.html index 82090b203..6576a708b 100644 --- a/coldfront/core/user/templates/user/user_profile.html +++ b/coldfront/core/user/templates/user/user_profile.html @@ -185,71 +185,6 @@


- {% if core_user_email_addresses_visible %} -
-
- - Other Email Addresses{% if not requester_is_viewed_user %}: {{ viewed_user.username }}{% endif %} - {% if core_user_email_addresses_updatable %} -
- - - Add Email Address - -
- {% endif %} -
-
-

Below are other email addresses associated with your account. You may authenticate to this portal using any verified addresses.

-

You can verify an address by clicking the “Verify” button, which will send a verification email to the address. The address will remain unverified until you have clicked the link provided in that email.

-
- - - - - - - - {% for email in other_emails %} - - - - - - {% endfor %} - -
Email AddressStatusActions
{{ email.email }} - {% if email.is_verified %} - Verified - {% else %} - Verification Pending - {% endif %} - - {% if core_user_email_addresses_updatable %} - {% if not email.is_verified %} -
- {% csrf_token %} - -
- {% endif %} -
- {% csrf_token %} - -
- {% else %} - N/A - {% endif %} -
-
-
-
-
- {% endif %} - {% if allauth_email_addresses_visible %}
diff --git a/coldfront/core/user/templates/user/user_update_primary_email_address.html b/coldfront/core/user/templates/user/user_update_primary_email_address.html deleted file mode 100644 index a1e9be1f6..000000000 --- a/coldfront/core/user/templates/user/user_update_primary_email_address.html +++ /dev/null @@ -1,38 +0,0 @@ -{% extends "common/base.html" %} -{% load crispy_forms_tags %} -{% load static %} - - -{% block title %} -Update Primary Email Address -{% endblock %} - - -{% block content %} - -

Update Primary Email Address

-
- -

Select a verified email address below to set as your new primary address, which receives email notifications and may be used to reset your password.

-

Your current primary email address is: {{ request.user.email }}.

- -
-
- {% if has_verified_non_primary_emails %} -
- {% csrf_token %} - {{ form|crispy }} - - - Cancel - -
- {% else %} -
- You have no other verified email addresses. Please add one before proceeding. -
- {% endif %} -
-
- -{% endblock %} diff --git a/coldfront/core/user/urls.py b/coldfront/core/user/urls.py index c4b7c46a2..265c816a4 100644 --- a/coldfront/core/user/urls.py +++ b/coldfront/core/user/urls.py @@ -78,24 +78,6 @@ PasswordResetCompleteView.as_view( template_name='user/passwords/password_reset_complete.html'), name='password-reset-complete'), - - # Email views - f_path('add-email-address', - user_views.EmailAddressAddView.as_view(), - name='add-email-address'), - f_path('verify-email-address////', - user_views.verify_email_address, - name='verify-email-address'), - f_path('send-email-verification-email/', - user_views.SendEmailAddressVerificationEmailView.as_view(), - name='send-email-verification-email'), - f_path('remove-email-address/', - user_views.RemoveEmailAddressView.as_view(), - name='remove-email-address'), - f_path('update-primary-email-address', - user_views.UpdatePrimaryEmailAddressView.as_view(), - name='update-primary-email-address'), - ] diff --git a/coldfront/core/user/views.py b/coldfront/core/user/views.py index cdd7cad9a..4faff26e0 100644 --- a/coldfront/core/user/views.py +++ b/coldfront/core/user/views.py @@ -8,7 +8,6 @@ from django.contrib.auth.views import PasswordChangeView from django.core.exceptions import ImproperlyConfigured from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator -from django.db import IntegrityError from django.db.models import BooleanField, Prefetch from django.db.models.expressions import ExpressionWrapper, Q from django.db.models.functions import Lower @@ -24,24 +23,21 @@ from django.views.generic import CreateView, ListView, TemplateView from django.views.generic.edit import FormView +from allauth.account.models import EmailAddress + +from coldfront.core.account.utils.queries import update_user_primary_email_address from coldfront.core.allocation.utils import has_cluster_access from coldfront.core.project.models import Project, ProjectUser from coldfront.core.user.models import IdentityLinkingRequest, IdentityLinkingRequestStatusChoice from coldfront.core.user.models import UserProfile as UserProfileModel -from coldfront.core.user.forms import EmailAddressAddForm from coldfront.core.user.forms import UserReactivateForm -from coldfront.core.user.forms import PrimaryEmailAddressSelectionForm from coldfront.core.user.forms import UserAccessAgreementForm from coldfront.core.user.forms import UserProfileUpdateForm from coldfront.core.user.forms import UserRegistrationForm from coldfront.core.user.forms import UserSearchForm, UserSearchListForm -from coldfront.core.user.models import EmailAddress from coldfront.core.user.utils import CombinedUserSearch -from coldfront.core.user.utils import ExpiringTokenGenerator from coldfront.core.user.utils import send_account_activation_email from coldfront.core.user.utils import send_account_already_active_email -from coldfront.core.user.utils import send_email_verification_email -from coldfront.core.user.utils import update_user_primary_email_address from coldfront.core.user.utils_.host_user_utils import is_lbl_employee from coldfront.core.utils.common import (import_from_settings, utc_now_offset_aware) @@ -100,21 +96,6 @@ def get_context_data(self, viewed_username=None, **kwargs): context['help_email'] = import_from_settings('CENTER_HELP_EMAIL') - # Only display the "Other Email Addresses" section for - # coldfront.core.user.models.EmailAddress if basic auth. is enabled. - is_basic_auth_enabled = flag_enabled('BASIC_AUTH_ENABLED') - context['requester_is_viewed_user'] = requester_is_viewed_user - context['primary_address_updatable'] = ( - is_basic_auth_enabled and requester_is_viewed_user) - context['change_password_enabled'] = ( - is_basic_auth_enabled and requester_is_viewed_user) - context['core_user_email_addresses_visible'] = is_basic_auth_enabled - if context['core_user_email_addresses_visible']: - context['other_emails'] = EmailAddress.objects.filter( - user=viewed_user, is_primary=False).order_by('email') - context['core_user_email_addresses_updatable'] = \ - requester_is_viewed_user - # Only display the "Other Email Addresses" section for # allauth.account.models.EmailAddress if SSO is enabled. is_sso_enabled = flag_enabled('SSO_ENABLED') @@ -426,7 +407,7 @@ def get_queryset(self): users = users.filter(username__icontains=data.get('username')) if data.get('email'): - _users = EmailAddress.objects.filter(is_primary=False, email__icontains=data.get('email'))\ + _users = EmailAddress.objects.filter(primary=False, email__icontains=data.get('email'))\ .order_by('user').values_list('user__id') users = users.filter(Q(email__icontains=data.get('email')) | Q(id__in=_users)) else: @@ -642,7 +623,7 @@ def activate_user_account(request, uidb64=None, token=None): logger.info( f'Created EmailAddress {email_address.pk} for User ' f'{user.pk} and email {email}.') - email_address.is_verified = True + email_address.verified = True email_address.save() update_user_primary_email_address(email_address) except Exception as e: @@ -708,198 +689,6 @@ def user_access_agreement(request): return render(request, template_name, context={'form': form}) -class EmailAddressAddView(LoginRequiredMixin, FormView): - form_class = EmailAddressAddForm - template_name = 'user/user_add_email_address.html' - - logger = logging.getLogger(__name__) - - def form_valid(self, form): - form_data = form.cleaned_data - email = form_data['email'] - try: - email_address = EmailAddress.objects.create( - user=self.request.user, email=email, is_verified=False, - is_primary=False) - except IntegrityError: - self.logger.error( - f'EmailAddress {email} unexpectedly already exists.') - message = ( - 'Unexpected server error. Please contact an administrator.') - messages.error(self.request, message) - else: - self.logger.info( - f'Created EmailAddress {email_address.pk} for User ' - f'{self.request.user.pk}.') - try: - send_email_verification_email(email_address) - except Exception as e: - message = 'Failed to send verification email. Details:' - logger.error(message) - logger.exception(e) - message = ( - f'Added {email_address.email} to your account, but failed ' - f'to send verification email. You may try to resend it ' - f'from the User Profile.') - messages.warning(self.request, message) - else: - message = ( - f'Added {email_address.email} to your account. Please ' - f'verify it by clicking the link sent to your email.') - messages.success(self.request, message) - return super().form_valid(form) - - def get_success_url(self): - return reverse('user-profile') - - -class SendEmailAddressVerificationEmailView(LoginRequiredMixin, View): - - def dispatch(self, request, *args, **kwargs): - pk = self.kwargs.get('pk') - self.email_address = get_object_or_404(EmailAddress, pk=pk) - if self.email_address.user != request.user: - message = ( - 'You may not send a verification email to an email address ' - 'not associated with your account.') - messages.error(request, message) - return HttpResponseRedirect(reverse('user-profile')) - if self.email_address.is_verified: - logger.error( - f'EmailAddress {self.email_address.pk} is unexpectedly ' - f'already verified.') - message = f'{self.email_address.email} is already verified.' - messages.warning(request, message) - return HttpResponseRedirect(reverse('user-profile')) - return super().dispatch(request, *args, **kwargs) - - def post(self, request, *args, **kwargs): - try: - send_email_verification_email(self.email_address) - except Exception as e: - message = 'Failed to send verification email. Details:' - logger.error(message) - logger.exception(e) - message = ( - f'Failed to send verification email to ' - f'{self.email_address.email}. Please contact an administrator ' - f'if the problem persists.') - messages.error(request, message) - else: - message = ( - f'Please click on the link sent to {self.email_address.email} ' - f'to verify it.') - messages.success(request, message) - return HttpResponseRedirect(reverse('user-profile')) - - -def verify_email_address(request, uidb64=None, eaidb64=None, token=None): - try: - user_pk = int(force_text(urlsafe_base64_decode(uidb64))) - email_pk = int(force_text(urlsafe_base64_decode(eaidb64))) - email_address = EmailAddress.objects.get(pk=email_pk) - user = User.objects.get(pk=user_pk) - if email_address.user != user: - user = None - except: - user = None - if user and token: - if ExpiringTokenGenerator().check_token(user, token): - email_address.is_verified = True - email_address.save() - logger.info(f'EmailAddress {email_address.pk} has been verified.') - message = f'{email_address.email} has been verified.' - messages.success(request, message) - else: - message = ( - 'Invalid verification token. Please try again, or contact an ' - 'administrator if the problem persists.') - messages.error(request, message) - else: - message = ( - f'Failed to activate account. Please contact an administrator.') - messages.error(request, message) - return redirect(reverse('user-profile')) - - -class RemoveEmailAddressView(LoginRequiredMixin, View): - - def dispatch(self, request, *args, **kwargs): - pk = self.kwargs.get('pk') - self.email_address = get_object_or_404(EmailAddress, pk=pk) - if self.email_address.user != request.user: - message = ( - 'You may not remove an email address not associated with your ' - 'account.') - messages.error(request, message) - return HttpResponseRedirect(reverse('user-profile')) - return super().dispatch(request, *args, **kwargs) - - def post(self, request, *args, **kwargs): - self.email_address.delete() - message = ( - f'{self.email_address.email} has been removed from your account.') - messages.success(request, message) - return HttpResponseRedirect(reverse('user-profile')) - - -class UpdatePrimaryEmailAddressView(LoginRequiredMixin, FormView): - - form_class = PrimaryEmailAddressSelectionForm - template_name = 'user/user_update_primary_email_address.html' - login_url = '/' - - error_message = 'Unexpected failure. Please contact an administrator.' - - def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) - context['has_verified_non_primary_emails'] = \ - EmailAddress.objects.filter( - user=self.request.user, is_verified=True, is_primary=False) - return context - - def form_valid(self, form): - user = self.request.user - form_data = form.cleaned_data - new_primary = form_data['email_address'] - - try: - update_user_primary_email_address(new_primary) - except TypeError: - message = ( - f'New primary EmailAddress {new_primary} has unexpected type: ' - f'{type(new_primary)}.') - logger.error(message) - messages.error(self.request, self.error_message) - except ValueError: - message = ( - f'New primary EmailAddress {new_primary.pk} for User ' - f'{user.pk} is unexpectedly not verified.') - logger.error(message) - messages.error(self.request, self.error_message) - except Exception as e: - message = ( - f'Encountered unexpected exception when updating User ' - f'{user.pk}\'s primary EmailAddress to {new_primary.pk}. ' - f'Details:') - logger.error(message) - logger.exception(e) - messages.error(self.request, self.error_message) - else: - message = f'{new_primary.email} is your new primary email address.' - messages.success(self.request, message) - - return super().form_valid(form) - - def get_form_kwargs(self): - kwargs = super().get_form_kwargs() - kwargs['user'] = self.request.user - return kwargs - - def get_success_url(self): - return reverse('user-profile') - - class EmailAddressExistsView(View): def get(self, request, *args, **kwargs): From f51b39d454cadd74db6f8590f807a77a86f6c30a Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Thu, 9 Feb 2023 10:39:37 -0800 Subject: [PATCH 05/72] Move helper method for making an address a user's primary --- coldfront/core/account/utils/__init__.py | 0 coldfront/core/account/utils/queries.py | 64 ++++++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 coldfront/core/account/utils/__init__.py create mode 100644 coldfront/core/account/utils/queries.py diff --git a/coldfront/core/account/utils/__init__.py b/coldfront/core/account/utils/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/coldfront/core/account/utils/queries.py b/coldfront/core/account/utils/queries.py new file mode 100644 index 000000000..7fe087e0d --- /dev/null +++ b/coldfront/core/account/utils/queries.py @@ -0,0 +1,64 @@ +from django.db import transaction + +from allauth.account.models import EmailAddress + +import logging + + +logger = logging.getLogger(__name__) + + +def update_user_primary_email_address(email_address): + """Given an EmailAddress, which must be verified, perform the + following: + - If the user's current email field does not have a + corresponding EmailAddress, create one (verified); + - Set the user's email field to it; + - Set it as the primary EmailAddress of the user; and + - Set the user's other EmailAddress objects to be non-primary. + + Perform the updates in a transaction so that they all fail together + or all succeed together. + + Parameters: + - email_address (EmailAddress): the EmailAddress object to set + as the new primary + + Returns: + - None + + Raises: + - TypeError, if the provided address has an invalid type + - ValueError, if the provided address is not verified + """ + if not isinstance(email_address, EmailAddress): + raise TypeError(f'Invalid EmailAddress {email_address}.') + if not email_address.verified: + raise ValueError(f'EmailAddress {email_address} is unverified.') + + user = email_address.user + with transaction.atomic(): + + old_primary, created = EmailAddress.objects.get_or_create( + user=user, email=user.email.lower()) + if created: + message = ( + f'Created EmailAddress {old_primary.pk} for User {user.pk}\'s ' + f'old primary address {old_primary.email}, which unexpectedly ' + f'did not exist.') + logger.warning(message) + old_primary.verified = True + old_primary.primary = False + old_primary.save() + + # TODO: Hide behind feature flag? This seems relevant no matter what. + for ea in EmailAddress.objects.filter( + user=user, primary=True).exclude(pk=email_address.pk): + ea.primary = False + ea.save() + + user.email = email_address.email + user.save() + + email_address.primary = True + email_address.save() From 928a41a0dbca0ab3d3c038bcd4c804aa51c686ec Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Thu, 9 Feb 2023 15:10:23 -0800 Subject: [PATCH 06/72] Replace references to old EmailAddress with new one --- .../test_models => account/tests}/__init__.py | 0 .../core/account/tests/test_admin/__init__.py | 0 .../test_admin/test_email_address_admin.py | 48 +++---- .../core/account/tests/test_utils/__init__.py | 0 .../tests/test_utils/test_queries/__init__.py | 0 .../test_update_user_primary_email_address.py | 52 ++++---- .../load_allocation_renewal_requests.py | 39 +----- coldfront/core/socialaccount/signals.py | 2 +- coldfront/core/user/admin.py | 4 +- coldfront/core/user/auth.py | 9 +- coldfront/core/user/forms.py | 7 +- .../commands/create_email_addresses.py | 25 +--- .../management/commands/lower_email_case.py | 4 +- .../tests/test_models/test_email_address.py | 118 ------------------ .../test_views/test_activate_user_account.py | 32 ++--- .../test_update_primary_emailaddress_view.py | 77 ------------ coldfront/core/user/utils.py | 58 +-------- 17 files changed, 97 insertions(+), 378 deletions(-) rename coldfront/core/{user/tests/test_models => account/tests}/__init__.py (100%) create mode 100644 coldfront/core/account/tests/test_admin/__init__.py rename coldfront/core/{user => account}/tests/test_admin/test_email_address_admin.py (90%) create mode 100644 coldfront/core/account/tests/test_utils/__init__.py create mode 100644 coldfront/core/account/tests/test_utils/test_queries/__init__.py rename coldfront/core/{user/tests/test_utils => account/tests/test_utils/test_queries}/test_update_user_primary_email_address.py (79%) delete mode 100644 coldfront/core/user/tests/test_models/test_email_address.py delete mode 100644 coldfront/core/user/tests/test_views/test_update_primary_emailaddress_view.py diff --git a/coldfront/core/user/tests/test_models/__init__.py b/coldfront/core/account/tests/__init__.py similarity index 100% rename from coldfront/core/user/tests/test_models/__init__.py rename to coldfront/core/account/tests/__init__.py diff --git a/coldfront/core/account/tests/test_admin/__init__.py b/coldfront/core/account/tests/test_admin/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/coldfront/core/user/tests/test_admin/test_email_address_admin.py b/coldfront/core/account/tests/test_admin/test_email_address_admin.py similarity index 90% rename from coldfront/core/user/tests/test_admin/test_email_address_admin.py rename to coldfront/core/account/tests/test_admin/test_email_address_admin.py index 39ab635f6..1662a7da3 100644 --- a/coldfront/core/user/tests/test_admin/test_email_address_admin.py +++ b/coldfront/core/account/tests/test_admin/test_email_address_admin.py @@ -1,15 +1,17 @@ -from django.core.exceptions import ValidationError -from django.contrib.auth.models import User from django.contrib.admin.sites import AdminSite -from coldfront.core.user.models import EmailAddress -from coldfront.core.user.admin import EmailAddressAdmin -from coldfront.core.user.tests.utils import TestUserBase -from django.http import HttpRequest -from django.contrib.messages.storage import default_storage from django.contrib.messages import get_messages +from django.contrib.messages.storage import default_storage +from django.contrib.auth.models import User +from django.core.exceptions import ValidationError +from django.http import HttpRequest + +from allauth.account.models import EmailAddress + +from coldfront.core.account.admin import EmailAddressAdmin +from coldfront.core.user.tests.utils import TestUserBase -class EmailAddressAdminTest(TestUserBase): +class TestEmailAddressAdmin(TestUserBase): """ Class for testing methods in EmailAddressAdmin """ @@ -35,32 +37,32 @@ def setUp(self): self.email1 = EmailAddress.objects.create( user=self.user1, email='email1@email.com', - is_verified=True, - is_primary=True) + verified=True, + primary=True) self.email2 = EmailAddress.objects.create( user=self.user1, email='email2@email.com', - is_verified=True, - is_primary=False) + verified=True, + primary=False) self.email3 = EmailAddress.objects.create( user=self.user2, email='email3@email.com', - is_verified=True, - is_primary=True) + verified=True, + primary=True) self.email4 = EmailAddress.objects.create( user=self.user2, email='email4@email.com', - is_verified=True, - is_primary=False) + verified=True, + primary=False) self.email5 = EmailAddress.objects.create( user=self.user2, email='email5@email.com', - is_verified=False, - is_primary=False) + verified=False, + primary=False) self.request = HttpRequest() setattr(self.request, 'session', 'session') @@ -121,7 +123,7 @@ def test_make_primary_single_primary_email(self): query_set = EmailAddress.objects.filter(pk=self.email1.pk) self.app_admin.make_primary(self.request, query_set) self.email1.refresh_from_db() - self.assertTrue(self.email1.is_primary) + self.assertTrue(self.email1.primary) storage = list(get_messages(self.request)) self.assertEqual(len(storage), 1) @@ -136,10 +138,10 @@ def test_make_primary_single_non_primary_email(self): query_set = EmailAddress.objects.filter(pk=self.email2.pk) self.app_admin.make_primary(self.request, query_set) self.email1.refresh_from_db() - self.assertFalse(self.email1.is_primary) + self.assertFalse(self.email1.primary) self.email2.refresh_from_db() - self.assertTrue(self.email2.is_primary) + self.assertTrue(self.email2.primary) storage = list(get_messages(self.request)) self.assertEqual(len(storage), 1) @@ -152,7 +154,7 @@ def test_make_delete_queryset_only_primary(self): Testing EmailAddressAdmin delete_queryset method with only primary emails """ - query_set = EmailAddress.objects.filter(is_primary=True) + query_set = EmailAddress.objects.filter(primary=True) self.app_admin.delete_queryset(self.request, query_set) self.assertTrue(EmailAddress.objects.filter(pk=self.email1.pk).exists()) @@ -171,7 +173,7 @@ def test_make_delete_queryset_only_non_primary(self): primary emails """ - query_set = EmailAddress.objects.filter(is_primary=False) + query_set = EmailAddress.objects.filter(primary=False) self.app_admin.delete_queryset(self.request, query_set) self.assertFalse(EmailAddress.objects.filter(pk=self.email2.pk).exists()) diff --git a/coldfront/core/account/tests/test_utils/__init__.py b/coldfront/core/account/tests/test_utils/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/coldfront/core/account/tests/test_utils/test_queries/__init__.py b/coldfront/core/account/tests/test_utils/test_queries/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/coldfront/core/user/tests/test_utils/test_update_user_primary_email_address.py b/coldfront/core/account/tests/test_utils/test_queries/test_update_user_primary_email_address.py similarity index 79% rename from coldfront/core/user/tests/test_utils/test_update_user_primary_email_address.py rename to coldfront/core/account/tests/test_utils/test_queries/test_update_user_primary_email_address.py index 82c35d490..e1a3240ae 100644 --- a/coldfront/core/user/tests/test_utils/test_update_user_primary_email_address.py +++ b/coldfront/core/account/tests/test_utils/test_queries/test_update_user_primary_email_address.py @@ -1,10 +1,12 @@ -from coldfront.core.user.models import EmailAddress -from coldfront.core.user.tests.utils import TestUserBase -from coldfront.core.user.utils import update_user_primary_email_address from django.contrib.auth.models import User +from allauth.account.models import EmailAddress -class TestUpdateUserPrimaryEmailAddress(TestUserBase): +from coldfront.core.account.utils.queries import update_user_primary_email_address +from coldfront.core.utils.tests.test_base import TestBase + + +class TestUpdateUserPrimaryEmailAddress(TestBase): """A class for testing the utility method update_user_primary_email_address.""" @@ -26,8 +28,8 @@ def test_creates_email_address_for_old_user_email_if_nonexistent(self): new_primary = EmailAddress.objects.create( user=self.user, email='new@email.com', - is_verified=True, - is_primary=False) + verified=True, + primary=False) self.assertEqual(EmailAddress.objects.count(), 1) with self.assertLogs('', 'WARNING') as cm: @@ -41,8 +43,8 @@ def test_creates_email_address_for_old_user_email_if_nonexistent(self): f'An EmailAddress for User {self.user} and email {old_email} ' f'should have been created.') else: - self.assertTrue(email_address.is_verified) - self.assertFalse(email_address.is_primary) + self.assertTrue(email_address.verified) + self.assertFalse(email_address.primary) # Assert that a warning was logged. self.assertEqual(len(cm.output), 1) @@ -65,13 +67,13 @@ def test_raises_type_error_for_bad_input(self): self.fail('A TypeError should have been raised.') def test_raises_value_error_for_unverified_input(self): - """Test that, if the input is an EmailAddress with is_verified + """Test that, if the input is an EmailAddress with verified set to False, a ValueError is raised.""" email_address = EmailAddress.objects.create( user=self.user, email=self.user.email, - is_verified=False, - is_primary=False) + verified=False, + primary=False) try: update_user_primary_email_address(email_address) except ValueError as e: @@ -86,14 +88,14 @@ def test_sets_email_address_to_primary(self): new_primary = EmailAddress.objects.create( user=self.user, email='new@email.com', - is_verified=True, - is_primary=False) - self.assertFalse(new_primary.is_primary) + verified=True, + primary=False) + self.assertFalse(new_primary.primary) update_user_primary_email_address(new_primary) new_primary.refresh_from_db() - self.assertTrue(new_primary.is_primary) + self.assertTrue(new_primary.primary) def test_sets_user_email_field(self): """Test that the method sets the User's "email" field to that of @@ -103,8 +105,8 @@ def test_sets_user_email_field(self): new_primary = EmailAddress.objects.create( user=self.user, email='new@email.com', - is_verified=True, - is_primary=False) + verified=True, + primary=False) update_user_primary_email_address(new_primary) @@ -112,32 +114,32 @@ def test_sets_user_email_field(self): self.assertEqual(self.user.email, new_primary.email) def test_unsets_other_primary_email_addresses(self): - """Test that the method unsets the "is_primary" fields of other + """Test that the method unsets the "primary" fields of other EmailAddresses belonging to the User.""" kwargs = { 'user': self.user, - 'is_verified': True, - 'is_primary': False, + 'verified': True, + 'primary': False, } for i in range(3): kwargs['email'] = f'{i}@email.com' EmailAddress.objects.create(**kwargs) # Bypass the "save" method, which prevents multiple primary addresses, # by using the "update" method. - EmailAddress.objects.filter(user=self.user).update(is_primary=True) + EmailAddress.objects.filter(user=self.user).update(primary=True) user_primary_emails = EmailAddress.objects.filter( - user=self.user, is_primary=True) + user=self.user, primary=True) self.assertEqual(user_primary_emails.count(), 3) new_primary = EmailAddress.objects.create( user=self.user, email='new@email.com', - is_verified=True, - is_primary=False) + verified=True, + primary=False) update_user_primary_email_address(new_primary) user_primary_emails = EmailAddress.objects.filter( - user=self.user, is_primary=True) + user=self.user, primary=True) self.assertEqual(user_primary_emails.count(), 1) self.assertEqual(user_primary_emails.first().pk, new_primary.pk) diff --git a/coldfront/core/allocation/management/commands/load_allocation_renewal_requests.py b/coldfront/core/allocation/management/commands/load_allocation_renewal_requests.py index cf233f6c1..ecc7598d8 100644 --- a/coldfront/core/allocation/management/commands/load_allocation_renewal_requests.py +++ b/coldfront/core/allocation/management/commands/load_allocation_renewal_requests.py @@ -9,7 +9,8 @@ from django.core.management.base import BaseCommand from django.core.management.base import CommandError from django.db import transaction -from django.utils.module_loading import import_string + +from allauth.account.models import EmailAddress from coldfront.core.allocation.models import AllocationPeriod from coldfront.core.allocation.models import AllocationRenewalRequest @@ -40,10 +41,6 @@ class Command(BaseCommand): logger = logging.getLogger(__name__) - def __init__(self, *args, **kwargs): - super().__init__(*args, *kwargs) - self.email_module_dict = {} - def add_arguments(self, parser): parser.add_argument( 'json', @@ -58,15 +55,6 @@ def add_arguments(self, parser): 'allocation_period_name', help='The name of the AllocationPeriod the renewals are under.', type=str) - # TODO: Remove this once all emails are transitioned to - # TODO: allauth.account.models.EmailAddress. - parser.add_argument( - 'email_module', - choices=['allauth.account.models', 'coldfront.core.user.models'], - help=( - 'There are temporarily two EmailAddress models, until all can ' - 'be transitioned under allauth.account.models.'), - type=str) parser.add_argument( '--process', action='store_true', @@ -92,16 +80,6 @@ def handle(self, *args, **options): raise CommandError( f'Invalid AllocationPeriod {allocation_period_name}.') - email_module = options['email_module'] - if email_module == 'allauth.account.models': - verified_field, primary_field = 'verified', 'primary' - else: - verified_field, primary_field = 'is_verified', 'is_primary' - self.email_module_dict['model'] = import_string( - f'{email_module}.EmailAddress') - self.email_module_dict['verified_field'] = verified_field - self.email_module_dict['primary_field'] = primary_field - valid, already_renewed, invalid = self.parse_input_file( file_path, allocation_period) self.process_valid_objects( @@ -347,10 +325,6 @@ def _update_or_create_user_and_email_address(self, email, first_name='', email. If provided (and not already set), set the given first, middle, and last names. Also update UserProfile.is_pi if requested.""" - EmailAddress = self.email_module_dict['model'] - email_verified_field = self.email_module_dict['verified_field'] - email_primary_field = self.email_module_dict['primary_field'] - user = self._get_user_with_email(email) if isinstance(user, User): user.first_name = user.first_name or first_name @@ -368,8 +342,8 @@ def _update_or_create_user_and_email_address(self, email, first_name='', user=user, email=email, defaults={ - email_verified_field: True, - email_primary_field: True}) + 'verified': True, + 'primary': True}) if created: message = ( f'Created EmailAddress {email_address.pk} for User ' @@ -390,8 +364,8 @@ def _update_or_create_user_and_email_address(self, email, first_name='', kwargs = { 'user': user, 'email': email, - email_verified_field: True, - email_primary_field: True, + 'verified': True, + 'primary': True, } email_address = EmailAddress.objects.create(**kwargs) message = ( @@ -418,7 +392,6 @@ def _get_first_middle_last_names(full_name): def _get_user_with_email(self, email): """Return the User associated with the given email, or None.""" - EmailAddress = self.email_module_dict['model'] try: return User.objects.get(email__iexact=email) except User.DoesNotExist: diff --git a/coldfront/core/socialaccount/signals.py b/coldfront/core/socialaccount/signals.py index 25d363c64..662529647 100644 --- a/coldfront/core/socialaccount/signals.py +++ b/coldfront/core/socialaccount/signals.py @@ -6,7 +6,7 @@ from django.db import transaction from django.dispatch import receiver -from allauth.socialaccount.models import EmailAddress +from allauth.account.models import EmailAddress from allauth.socialaccount.models import SocialLogin from allauth.socialaccount.providers.base import AuthProcess from allauth.socialaccount.signals import social_account_added diff --git a/coldfront/core/user/admin.py b/coldfront/core/user/admin.py index 7f4a7c342..40bff30ca 100644 --- a/coldfront/core/user/admin.py +++ b/coldfront/core/user/admin.py @@ -1,6 +1,8 @@ from django.contrib import admin -from coldfront.core.user.models import UserProfile, EmailAddress +from allauth.account.models import EmailAddress + +from coldfront.core.user.models import UserProfile @admin.register(UserProfile) diff --git a/coldfront/core/user/auth.py b/coldfront/core/user/auth.py index 4e1022c0d..f5a79e8d3 100644 --- a/coldfront/core/user/auth.py +++ b/coldfront/core/user/auth.py @@ -1,7 +1,10 @@ -from coldfront.core.user.models import EmailAddress -from coldfront.core.user.utils import send_email_verification_email from django.contrib.auth.backends import BaseBackend from django.contrib.auth.models import User + +from allauth.account.models import EmailAddress + +from coldfront.core.user.utils import send_email_verification_email + import logging @@ -24,7 +27,7 @@ def authenticate(self, request, username=None, password=None, **kwargs): # If the EmailAddress exists, but is not verified, send a verification # email to it. Only do this if the user is already active; otherwise, # a separate account activation email will handle email verification. - if user.is_active and not email_address.is_verified: + if user.is_active and not email_address.verified: try: send_email_verification_email(email_address) except Exception as e: diff --git a/coldfront/core/user/forms.py b/coldfront/core/user/forms.py index ee29a014e..88bc01bff 100644 --- a/coldfront/core/user/forms.py +++ b/coldfront/core/user/forms.py @@ -8,15 +8,16 @@ from django.contrib.auth.models import User from django.contrib.auth.tokens import default_token_generator from django.contrib.sites.shortcuts import get_current_site -from django.core.exceptions import ImproperlyConfigured from django.urls import reverse from django.utils.encoding import force_bytes from django.utils.html import mark_safe from django.utils.http import urlsafe_base64_encode from django.utils.translation import gettext_lazy as _ +from allauth.account.models import EmailAddress + from coldfront.core.user.utils import send_account_activation_email -from coldfront.core.user.models import UserProfile, EmailAddress +from coldfront.core.user.models import UserProfile from coldfront.core.utils.mail import dummy_email_address from phonenumber_field.formfields import PhoneNumberField @@ -255,7 +256,7 @@ def get_email_address(email): if one exists, else None.""" try: return EmailAddress.objects.select_related('user').get( - email=email, is_verified=True, user__is_active=True) + email=email, verified=True, user__is_active=True) except EmailAddress.DoesNotExist: return None diff --git a/coldfront/core/user/management/commands/create_email_addresses.py b/coldfront/core/user/management/commands/create_email_addresses.py index 357217f14..f5a71f065 100644 --- a/coldfront/core/user/management/commands/create_email_addresses.py +++ b/coldfront/core/user/management/commands/create_email_addresses.py @@ -1,5 +1,8 @@ from django.contrib.auth.models import User from django.core.management.base import BaseCommand + +from allauth.account.models import EmailAddress + import logging @@ -15,27 +18,9 @@ class Command(BaseCommand): 'one-time use for existing users loaded in from spreadsheets.') logger = logging.getLogger(__name__) - def add_arguments(self, parser): - # TODO: Remove this once all emails are transitioned to - # TODO: allauth.account.models.EmailAddress. - parser.add_argument( - 'module', - choices=['allauth.account.models', 'coldfront.core.user.models'], - help=( - 'There are temporarily two EmailAddress models, until all can ' - 'be transitioned under allauth.account.models.'), - type=str) - def handle(self, *args, **options): """For each User that has no EmailAddress, create a verified, primary instance using the User's email field.""" - if options['module'] == 'allauth.account.models': - from allauth.account.models import EmailAddress - verified_field, primary_field = 'verified', 'primary' - else: - from coldfront.core.user.models import EmailAddress - verified_field, primary_field = 'is_verified', 'is_primary' - user_pks_with_emails = EmailAddress.objects.values_list( 'user', flat=True) users_without_emails = User.objects.exclude( @@ -46,8 +31,8 @@ def handle(self, *args, **options): kwargs = { 'user': user, 'email': email, - verified_field: True, - primary_field: True, + 'verified': True, + 'primary': True, } if not email: message = f'User {user.pk} email is empty.' diff --git a/coldfront/core/user/management/commands/lower_email_case.py b/coldfront/core/user/management/commands/lower_email_case.py index c467a5291..521079712 100644 --- a/coldfront/core/user/management/commands/lower_email_case.py +++ b/coldfront/core/user/management/commands/lower_email_case.py @@ -1,6 +1,8 @@ -from coldfront.core.user.models import EmailAddress from django.contrib.auth.models import User from django.core.management.base import BaseCommand + +from allauth.account.models import EmailAddress + import logging diff --git a/coldfront/core/user/tests/test_models/test_email_address.py b/coldfront/core/user/tests/test_models/test_email_address.py deleted file mode 100644 index 43d1e2e1a..000000000 --- a/coldfront/core/user/tests/test_models/test_email_address.py +++ /dev/null @@ -1,118 +0,0 @@ -from coldfront.core.user.models import EmailAddress -from coldfront.core.user.tests.utils import TestUserBase -from django.contrib.auth.models import User -from django.core.exceptions import ValidationError - - -class TestEmailAddress(TestUserBase): - """A class for testing the EmailAddress model.""" - - def setUp(self): - """Set up test data.""" - super().setUp() - - self.user = User.objects.create( - email='user@email.com', - first_name='First', - last_name='Last', - username='user') - - def test_save_nonexistent_to_primary(self): - """Test that saving an EmailAddress as a primary one when it did - not previously exist succeeds.""" - kwargs = { - 'user': self.user, - 'email': self.user.email, - 'is_verified': True, - 'is_primary': True, - } - try: - email_address = EmailAddress.objects.create(**kwargs) - except ValidationError: - self.fail('A ValidationError should not have been raised.') - self.assertTrue(email_address.is_primary) - - def test_save_non_primary_to_primary_no_others(self): - """Test that saving an EmailAddress that was not previously - primary as primary, when there are no other primary - EmailAddresses, succeeds.""" - kwargs = { - 'user': self.user, - 'email': self.user.email, - 'is_verified': True, - 'is_primary': False, - } - try: - email_address = EmailAddress.objects.create(**kwargs) - except ValidationError: - self.fail('A ValidationError should not have been raised.') - self.assertFalse(email_address.is_primary) - - try: - email_address.is_primary = True - email_address.save() - except ValidationError: - self.fail('A ValidationError should not have been raised.') - self.assertTrue(email_address.is_primary) - - def test_save_non_primary_to_primary_others(self): - """Test that saving an EmailAddress that was not previously - primary as primary, when there are other primary - EmailAddresses, fails.""" - kwargs = { - 'user': self.user, - 'email': self.user.email, - 'is_verified': True, - 'is_primary': True, - } - EmailAddress.objects.create(**kwargs) - - kwargs['email'] = 'new@email.com' - try: - EmailAddress.objects.create(**kwargs) - except ValidationError as e: - self.assertIn('User already has a primary email address', str(e)) - else: - self.fail('A ValidationError should have been raised.') - - def test_save_unverified_as_primary(self): - """Test that saving an unverified EmailAddress as primary - fails.""" - kwargs = { - 'user': self.user, - 'email': self.user.email, - 'is_verified': False, - 'is_primary': True, - } - try: - EmailAddress.objects.create(**kwargs) - except ValidationError as e: - self.assertIn( - 'Only verified emails may be set to primary.', str(e)) - else: - self.fail('A ValidationError should have been raised.') - - def test_save_updates_user_email_field_if_primary(self): - """Test that saving a primary EmailAddress updates the "email" - field of the User.""" - self.assertEqual(self.user.email, 'user@email.com') - - # The field should be updated if the address goes from non-primary to - # primary. - kwargs = { - 'user': self.user, - 'email': 'new0@email.com', - 'is_verified': True, - 'is_primary': True, - } - email_address = EmailAddress.objects.create(**kwargs) - self.user.refresh_from_db() - self.assertEqual(self.user.email, kwargs['email']) - - # The field should be updated if the address goes from primary to - # primary. - kwargs['email'] = 'new1@email.com' - email_address.email = kwargs['email'] - email_address.save() - self.user.refresh_from_db() - self.assertEqual(self.user.email, kwargs['email']) diff --git a/coldfront/core/user/tests/test_views/test_activate_user_account.py b/coldfront/core/user/tests/test_views/test_activate_user_account.py index 1124edc2a..8f7080475 100644 --- a/coldfront/core/user/tests/test_views/test_activate_user_account.py +++ b/coldfront/core/user/tests/test_views/test_activate_user_account.py @@ -1,12 +1,12 @@ +from django.contrib.auth.models import User +from django.contrib.messages import get_messages +from django.test import Client + +from allauth.account.models import EmailAddress from flags.state import enable_flag -from coldfront.core.user.models import EmailAddress from coldfront.core.user.tests.utils import TestUserBase from coldfront.core.user.utils import account_activation_url -from django.contrib.auth.models import User -from django.contrib.messages import get_messages -from django.test import Client -from django.urls import reverse class TestActivateUserAccount(TestUserBase): @@ -56,8 +56,8 @@ def test_creates_verified_primary_email_address(self): email_address = EmailAddress.objects.get(email=self.user.email) self.assertEqual(email_address.user, self.user) - self.assertTrue(email_address.is_verified) - self.assertTrue(email_address.is_primary) + self.assertTrue(email_address.verified) + self.assertTrue(email_address.primary) def test_updates_existing_email_addresses(self): """Test that account activation updates EmailAddresses so that @@ -65,18 +65,18 @@ def test_updates_existing_email_addresses(self): # Create an unverified, non-primary EmailAddress for the User. kwargs = { 'user': self.user, - 'is_verified': False, - 'is_primary': False, + 'verified': False, + 'primary': False, } email_address = EmailAddress.objects.create( user=self.user, email=self.user.email, - is_verified=False, - is_primary=False) + verified=False, + primary=False) # Create other primary EmailAddresses. other_email_addresses = [] - kwargs['is_verified'] = True + kwargs['verified'] = True for i in range(3): kwargs['email'] = f'{i}@email.com' other_email_addresses.append( @@ -85,7 +85,7 @@ def test_updates_existing_email_addresses(self): # by using the "update" method. EmailAddress.objects.filter( pk__in=[ea.pk for ea in other_email_addresses]).update( - is_primary=True) + primary=True) url = account_activation_url(self.user) response = self.client.get(url) @@ -95,11 +95,11 @@ def test_updates_existing_email_addresses(self): email_address.refresh_from_db() self.assertEqual(email_address.user, self.user) - self.assertTrue(email_address.is_verified) - self.assertTrue(email_address.is_primary) + self.assertTrue(email_address.verified) + self.assertTrue(email_address.primary) for ea in other_email_addresses: ea.refresh_from_db() - self.assertFalse(ea.is_primary) + self.assertFalse(ea.primary) # TODO diff --git a/coldfront/core/user/tests/test_views/test_update_primary_emailaddress_view.py b/coldfront/core/user/tests/test_views/test_update_primary_emailaddress_view.py deleted file mode 100644 index 94a7ac9a4..000000000 --- a/coldfront/core/user/tests/test_views/test_update_primary_emailaddress_view.py +++ /dev/null @@ -1,77 +0,0 @@ -from flags.state import enable_flag - -from coldfront.core.user.models import EmailAddress -from coldfront.core.user.tests.utils import TestUserBase -from django.contrib.auth.models import User -from django.contrib.messages import get_messages -from django.test import Client -from django.urls import reverse - - -class TestUpdateUserPrimaryEmailAddress(TestUserBase): - """ - A class for testing the view for updating a user's - primary email address'. - """ - - def setUp(self): - """Set up test data.""" - enable_flag('BASIC_AUTH_ENABLED') - super().setUp() - - self.password = 'password' - - self.user1 = User.objects.create( - email='user1@email.com', - first_name='First', - last_name='Last', - username='user1') - self.user1.set_password(self.password) - self.user1.save() - - self.email1 = EmailAddress.objects.create( - user=self.user1, - email='email1@email.com', - is_verified=True, - is_primary=True) - - self.email2 = EmailAddress.objects.create( - user=self.user1, - email='email2@email.com', - is_verified=True, - is_primary=False) - - self.client = Client() - - def test_update_primary_emailaddress_view(self): - """ - Testing UpdatePrimaryEmailAddressView - """ - - self.client.login(username=self.user1.username, password=self.password) - url = reverse( - 'update-primary-email-address') - data = {'email_address': str(self.email2.pk)} - response = self.client.get(url) - self.assertEqual(response.status_code, 200) - self.assertContains(response, f'Your current primary email address ' - f'is: {self.email1.email}.') - self.assertContains(response, self.email2.email) - - response = self.client.post(url, data) - self.assertEqual(response.status_code, 302) - - self.email1.refresh_from_db() - self.email2.refresh_from_db() - - self.assertTrue(self.email2.is_primary) - self.assertFalse(self.email1.is_primary) - - self.assertRedirects(response, reverse('user-profile')) - - messages = [str(m) for m in get_messages(response.wsgi_request)] - self.assertTrue(len(messages), 1) - self.assertEqual(messages[0], f'{self.email2.email} is your new ' - f'primary email address.') - - self.client.logout() diff --git a/coldfront/core/user/utils.py b/coldfront/core/user/utils.py index cc0ab43a8..47bede5d1 100644 --- a/coldfront/core/user/utils.py +++ b/coldfront/core/user/utils.py @@ -7,7 +7,6 @@ from django.conf import settings from django.contrib.auth.models import User from django.contrib.auth.tokens import PasswordResetTokenGenerator -from django.db import transaction from django.db.models import Q from django.urls import reverse from django.utils.crypto import constant_time_compare @@ -16,7 +15,6 @@ from django.utils.http import urlsafe_base64_encode from django.utils.module_loading import import_string -from coldfront.core.user.models import EmailAddress from coldfront.core.utils.common import import_from_settings from coldfront.core.utils.mail import send_email_template @@ -24,6 +22,7 @@ logger = logging.getLogger(__name__) + class UserSearch(abc.ABC): def __init__(self, user_search_string, search_by): @@ -285,58 +284,3 @@ def send_email_verification_email(email_address): receiver_list = [email_address.email, ] send_email_template(subject, template_name, context, sender, receiver_list) - - -def update_user_primary_email_address(email_address): - """Given an EmailAddress, which must be verified, perform the - following: - - If the user's current email field does not have a - corresponding EmailAddress, create one (verified); - - Set the user's email field to it; - - Set it as the primary EmailAddress of the user; and - - Set the user's other EmailAddress objects to be non-primary. - - Perform the updates in a transaction so that they all fail together - or all succeed together. - - Parameters: - - email_address (EmailAddress): the EmailAddress object to set - as the new primary - - Returns: - - None - - Raises: - - TypeError, if the provided address has an invalid type - - ValueError, if the provided address is not verified - """ - if not isinstance(email_address, EmailAddress): - raise TypeError(f'Invalid EmailAddress {email_address}.') - if not email_address.is_verified: - raise ValueError(f'EmailAddress {email_address} is unverified.') - - user = email_address.user - with transaction.atomic(): - - old_primary, created = EmailAddress.objects.get_or_create( - user=user, email=user.email.lower()) - if created: - message = ( - f'Created EmailAddress {old_primary.pk} for User {user.pk}\'s ' - f'old primary address {old_primary.email}, which unexpectedly ' - f'did not exist.') - logger.warning(message) - old_primary.is_verified = True - old_primary.is_primary = False - old_primary.save() - - for ea in EmailAddress.objects.filter( - user=user, is_primary=True).exclude(pk=email_address.pk): - ea.is_primary = False - ea.save() - - user.email = email_address.email - user.save() - - email_address.is_primary = True - email_address.save() From accc27f3bfa01e860dc43b2aef9275753e46c35d Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Thu, 9 Feb 2023 16:15:28 -0800 Subject: [PATCH 07/72] Create second version of SSO login page for BRC, with CalNet login prioritized; make login button text on home page dynamic --- .../templates/portal/nonauthorized_home.html | 5 + .../user/templates/user/sso_login_brc.html | 122 ++++++++++++++++++ .../{sso_login.html => sso_login_lrc.html} | 0 coldfront/core/user/views.py | 11 +- 4 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 coldfront/core/user/templates/user/sso_login_brc.html rename coldfront/core/user/templates/user/{sso_login.html => sso_login_lrc.html} (100%) diff --git a/coldfront/core/portal/templates/portal/nonauthorized_home.html b/coldfront/core/portal/templates/portal/nonauthorized_home.html index dbea8e85a..c0835f63f 100644 --- a/coldfront/core/portal/templates/portal/nonauthorized_home.html +++ b/coldfront/core/portal/templates/portal/nonauthorized_home.html @@ -38,7 +38,12 @@

Welcome to {{ PORTAL_NAME }}

{% if sso_enabled %} + {% flag_enabled 'BRC_ONLY' as brc_only %} + {% if brc_only %} + CalNet: Log In + {% else %} Berkeley Lab: Log In + {% endif %} diff --git a/coldfront/core/user/templates/user/sso_login_brc.html b/coldfront/core/user/templates/user/sso_login_brc.html new file mode 100644 index 000000000..94a8a2717 --- /dev/null +++ b/coldfront/core/user/templates/user/sso_login_brc.html @@ -0,0 +1,122 @@ +{% extends "common/base.html" %} +{% load feature_flags %} +{% load socialaccount %} + +{% load common_tags %} +{% load static %} + +{% block title %} +Log In +{% endblock %} + +{% block content %} + +

Log In: I am a...

+ +
+
+
+
+ uc_berkeley_logo_words +
+
+

UC Berkeley User

+

+ I have a CalNet ID. +

+

+

+ + + Log In + +

+
+
+
+
+
+
+
+ berkeley_lab_logo +
+
+

Berkeley Lab Collaborator

+

+ I do not have a CalNet ID, but I do have a Berkeley Lab Identity. +

+

+ + + Log In + +

+
+
+
+
+
+
+
+ cilogon_logo +
+
+

External Collaborator

+

+ I do not have a CalNet ID, and I do not have a Berkeley Lab Identity. +

+

+ + + Log In + +

+
+
+
+
+
+ +
+
+
+ + + Hints + +
+
+
+
    +
  • + Authentication is managed by + CILogon. You will be + redirected there, where you will select a provider to authenticate + with (University of California, Berkeley for most users). +
    • +
  • + If you have a CalNet ID, please authenticate using the first option + above. UC Berkeley users who do not use their CalNet ID may + experience delays when requesting cluster access. +
    • +
  • + External collaborators should select the provider of their home + institution. If your provider is not listed, select the + Google provider. +
    • +
+
+
+
+
+ +{% endblock %} + + +{% block javascript %} +{{ block.super }} + +{% endblock %} diff --git a/coldfront/core/user/templates/user/sso_login.html b/coldfront/core/user/templates/user/sso_login_lrc.html similarity index 100% rename from coldfront/core/user/templates/user/sso_login.html rename to coldfront/core/user/templates/user/sso_login_lrc.html diff --git a/coldfront/core/user/views.py b/coldfront/core/user/views.py index 4faff26e0..3b8301ccf 100644 --- a/coldfront/core/user/views.py +++ b/coldfront/core/user/views.py @@ -544,13 +544,22 @@ def dispatch(self, request, *args, **kwargs): class SSOLoginView(TemplateView): """Display the template for SSO login. If the user is authenticated, redirect to the home page.""" - template_name = 'user/sso_login.html' def dispatch(self, request, *args, **kwargs): if request.user.is_authenticated: return redirect(reverse('home')) return super().dispatch(request, *args, **kwargs) + def get_template_names(self): + if flag_enabled('BRC_ONLY'): + return ['user/sso_login_brc.html'] + elif flag_enabled('LRC_ONLY'): + return ['user/sso_login_lrc.html'] + else: + raise ImproperlyConfigured( + 'One of the following flags must be enabled: BRC_ONLY, ' + 'LRC_ONLY.') + class UserRegistrationView(CreateView): From ba675c4b1a71d966275d5d476fa98c1f77867395 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 15 Feb 2023 13:51:53 -0800 Subject: [PATCH 08/72] Add first-pass implementation of short-lived link authentication --- coldfront/config/local_settings.py.sample | 11 +++ coldfront/core/user/forms_/__init__.py | 0 .../core/user/forms_/link_login_forms.py | 10 ++ .../templates/user/request_login_link.html | 40 ++++++++ coldfront/core/user/urls.py | 13 ++- coldfront/core/user/utils.py | 32 ++++++ .../core/user/views_/link_login_views.py | 99 +++++++++++++++++++ coldfront/templates/email/login_link.txt | 14 +++ requirements.txt | 1 + 9 files changed, 219 insertions(+), 1 deletion(-) create mode 100644 coldfront/core/user/forms_/__init__.py create mode 100644 coldfront/core/user/forms_/link_login_forms.py create mode 100644 coldfront/core/user/templates/user/request_login_link.html create mode 100644 coldfront/core/user/views_/link_login_views.py create mode 100644 coldfront/templates/email/login_link.txt diff --git a/coldfront/config/local_settings.py.sample b/coldfront/config/local_settings.py.sample index d5164bebb..7df9a119e 100644 --- a/coldfront/config/local_settings.py.sample +++ b/coldfront/config/local_settings.py.sample @@ -438,6 +438,17 @@ SOCIALACCOUNT_PROVIDERS = { # Always request the 'email' scope. SOCIALACCOUNT_QUERY_EMAIL = True +#------------------------------------------------------------------------------ +# Django Sesame settings +#------------------------------------------------------------------------------ + +EXTRA_AUTHENTICATION_BACKENDS += [ + 'sesame.backends.ModelBackend', +] + +# The number of seconds a login token is valid for. +SESAME_MAX_AGE = 300 + #------------------------------------------------------------------------------ # Data import settings #------------------------------------------------------------------------------ diff --git a/coldfront/core/user/forms_/__init__.py b/coldfront/core/user/forms_/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/coldfront/core/user/forms_/link_login_forms.py b/coldfront/core/user/forms_/link_login_forms.py new file mode 100644 index 000000000..b5367eb62 --- /dev/null +++ b/coldfront/core/user/forms_/link_login_forms.py @@ -0,0 +1,10 @@ +from django import forms + + +class RequestLoginLinkForm(forms.Form): + + email = forms.EmailField() + + def clean_email(self): + email = self.cleaned_data['email'] + return email.lower() diff --git a/coldfront/core/user/templates/user/request_login_link.html b/coldfront/core/user/templates/user/request_login_link.html new file mode 100644 index 000000000..a94405a47 --- /dev/null +++ b/coldfront/core/user/templates/user/request_login_link.html @@ -0,0 +1,40 @@ +{% extends "common/base.html" %} +{% load static %} +{% load common_tags %} +{% load crispy_forms_tags %} + +{% block title %} +Request Login Link +{% endblock %} + +{% block content %} + +
+ +
+
+ + Request Login Link +
+
+

+ If your institution is not listed in CILogon, you may use this form to + request a short-lived link to log in to the portal. +

+

+ All new users must select an institution listed in CILogon. +

+
+ {% csrf_token %} +

+ {{ form|crispy }} + +

+
+
+
+
+{% endblock %} diff --git a/coldfront/core/user/urls.py b/coldfront/core/user/urls.py index 265c816a4..81edea4f7 100644 --- a/coldfront/core/user/urls.py +++ b/coldfront/core/user/urls.py @@ -5,11 +5,11 @@ from django.contrib.auth.views import PasswordResetDoneView from django.contrib.auth.views import PasswordResetView from django.urls import path, reverse_lazy -from django.views.generic import TemplateView from flags.urls import flagged_paths import coldfront.core.user.views as user_views +import coldfront.core.user.views_.link_login_views as link_login_views import coldfront.core.user.views_.request_hub_views as request_hub_views from coldfront.core.user.forms import VerifiedEmailAddressPasswordResetForm from coldfront.core.user.forms import UserLoginForm @@ -81,6 +81,17 @@ ] +with flagged_paths('LINK_LOGIN_ENABLED') as f_path: + urlpatterns += [ + f_path('request-login-link/', + link_login_views.RequestLoginLinkView.as_view(), + name='request-login-link'), + f_path('link-login/', + link_login_views.LinkLoginView.as_view(), + name='link-login'), + ] + + with flagged_paths('SSO_ENABLED') as f_path: urlpatterns += [ f_path('sso_login/', diff --git a/coldfront/core/user/utils.py b/coldfront/core/user/utils.py index 47bede5d1..2e3e45312 100644 --- a/coldfront/core/user/utils.py +++ b/coldfront/core/user/utils.py @@ -18,6 +18,7 @@ from coldfront.core.utils.common import import_from_settings from coldfront.core.utils.mail import send_email_template +from sesame.utils import get_query_string from urllib.parse import urljoin logger = logging.getLogger(__name__) @@ -211,6 +212,13 @@ def __email_verification_url(email_address): return urljoin(domain, view) +def login_token_url(user): + """Return a Django Sesame login link for the given User.""" + domain = import_from_settings('CENTER_BASE_URL') + path = reverse('link-login') + get_query_string(user) + return urljoin(domain, path) + + def send_account_activation_email(user): """Send an activation email to the given User, who has just created an account, providing a link to activate the account.""" @@ -284,3 +292,27 @@ def send_email_verification_email(email_address): receiver_list = [email_address.email, ] send_email_template(subject, template_name, context, sender, receiver_list) + + +def send_login_link_email(email_address): + """Send an email containing a login link to the given + EmailAddress.""" + email_enabled = import_from_settings('EMAIL_ENABLED', False) + if not email_enabled: + return + + subject = 'Login Link' + template_name = 'email/login_link.txt' + context = { + 'PORTAL_NAME': settings.PORTAL_NAME, + 'center_name': import_from_settings('CENTER_NAME', ''), + 'login_url': login_token_url(email_address.user), + 'login_link_max_age_minutes': ( + import_from_settings('SESAME_MAX_AGE') // 60), + 'signature': import_from_settings('EMAIL_SIGNATURE', ''), + } + + sender = import_from_settings('EMAIL_SENDER') + receiver_list = [email_address.email, ] + + send_email_template(subject, template_name, context, sender, receiver_list) diff --git a/coldfront/core/user/views_/link_login_views.py b/coldfront/core/user/views_/link_login_views.py new file mode 100644 index 000000000..f771e63ee --- /dev/null +++ b/coldfront/core/user/views_/link_login_views.py @@ -0,0 +1,99 @@ +import logging + +from django.contrib import messages +from django.shortcuts import redirect +from django.urls import reverse +from django.views.generic.edit import FormView + +from allauth.account.models import EmailAddress +from sesame.views import LoginView + +from coldfront.core.user.forms_.link_login_forms import RequestLoginLinkForm +from coldfront.core.user.utils import send_login_link_email +from coldfront.core.utils.common import import_from_settings + + +logger = logging.getLogger(__name__) + + +class RequestLoginLinkView(FormView): + """A view that sends a login link to the user with the provided + email address, if any.""" + + form_class = RequestLoginLinkForm + template_name = 'user/request_login_link.html' + + def dispatch(self, request, *args, **kwargs): + if request.user.is_authenticated: + return redirect(reverse('home')) + return super().dispatch(request, *args, **kwargs) + + def form_valid(self, form): + """If the email address belongs to a user, send a login link to + it.""" + email = form.cleaned_data.get('email') + email_address = self._validate_email_address(email) + if email_address: + send_login_link_email(email_address) + self._send_success_message() + return super().form_valid(form) + + def get_success_url(self): + return reverse('request-login-link') + + def _send_success_message(self): + """Send a success message to the user explaining that a link was + (conditionally) sent.""" + login_link_max_age_minutes = ( + import_from_settings('SESAME_MAX_AGE') // 60) + message = ( + f'If the email address you entered corresponds to an existing ' + f'user, please check the address for a login link. Note that this ' + f'link will expire in {login_link_max_age_minutes} minutes.') + messages.success(self.request, message) + + def _validate_email_address(self, email): + """Return an EmailAddress object corresponding to the given + address (str) if one exists. Otherwise, return None. Write user + and log messages as needed.""" + email_address = None + try: + email_address = EmailAddress.objects.get(email=email) + except EmailAddress.DoesNotExist: + pass + except EmailAddress.MultipleObjectsReturned: + logger.error( + f'Unexpectedly found multiple EmailAddresses for email ' + f'{email}.') + message = ( + 'Unexpected server error. Please contact an administrator.') + messages.error(self.request, message) + return email_address + + +class LinkLoginView(LoginView): + """A subclass of Django Sesame's login view, with custom logic.""" + + def login_failed(self): + """Send an error message to the user and write to the log before + deferring to parent logic.""" + message = 'Invalid or expired login link.' + messages.error(self.request, message) + logger.warning( + 'A user failed to log in using an invalid or expired login link.') + return super().login_failed() + + def login_success(self): + """Activate the user if needed, send a success message to the + user, and write to the log before deferring to parent logic.""" + user = self.request.user + if not user.is_active: + user.is_active = True + user.save() + + message = f'Successfully signed in as {user.username}.' + messages.success(self.request, message) + logger.warning( + f'User {user.pk} ({user.username}) logged in using a login link.') + + return super().login_success() diff --git a/coldfront/templates/email/login_link.txt b/coldfront/templates/email/login_link.txt new file mode 100644 index 000000000..f42781534 --- /dev/null +++ b/coldfront/templates/email/login_link.txt @@ -0,0 +1,14 @@ +Dear {{ PORTAL_NAME }} user, + +You requested a link to log in to the portal. + +Below is the link, which will expire in {{ login_link_max_age_minutes }} minutes. + +{{ login_url }} + +Do not share this link with anyone else. + +Reminder: If your institution is listed in CILogon, you should use it to log in. + +Thank you, +{{ signature }} \ No newline at end of file diff --git a/requirements.txt b/requirements.txt index 20420398e..1b118b503 100644 --- a/requirements.txt +++ b/requirements.txt @@ -17,6 +17,7 @@ django-model-utils==4.1.1 django-phonenumber-field==5.1.0 django-picklefield==2.0 django-q==1.0.1 +django-sesame==3.1 django-settings-export==1.2.1 django-simple-history==2.12.0 django-sslserver==0.20 From a52441387633ff9f992147dbc897d295a0134b68 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 15 Feb 2023 13:52:29 -0800 Subject: [PATCH 09/72] Link to view for requesting login link in SSO login page --- coldfront/core/user/templates/user/sso_login_brc.html | 8 ++++++++ coldfront/core/user/templates/user/sso_login_lrc.html | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/coldfront/core/user/templates/user/sso_login_brc.html b/coldfront/core/user/templates/user/sso_login_brc.html index 94a8a2717..5c32aa3f0 100644 --- a/coldfront/core/user/templates/user/sso_login_brc.html +++ b/coldfront/core/user/templates/user/sso_login_brc.html @@ -104,6 +104,14 @@

External Collaborator

institution. If your provider is not listed, select the Google provider. + {% flag_enabled 'LINK_LOGIN_ENABLED' as link_login_enabled %} + {% if link_login_enabled %} +
  • + If you are an existing user whose institution is not listed, you + may request a short-lived login link + here. +
    • + {% endif %}
    diff --git a/coldfront/core/user/templates/user/sso_login_lrc.html b/coldfront/core/user/templates/user/sso_login_lrc.html index d1f18fb02..f181229b9 100644 --- a/coldfront/core/user/templates/user/sso_login_lrc.html +++ b/coldfront/core/user/templates/user/sso_login_lrc.html @@ -104,6 +104,14 @@

    External Collaborator

    institution. If your provider is not listed, select the Google provider. + {% flag_enabled 'LINK_LOGIN_ENABLED' as link_login_enabled %} + {% if link_login_enabled %} +
  • + If you are an existing user whose institution is not listed, you + may request a short-lived login link + here. +
    • + {% endif %} From ec4a8fc810e06e1e3c955686e6dad6ea488fbcd4 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 15 Feb 2023 14:24:08 -0800 Subject: [PATCH 10/72] Update wording --- .../core/user/templates/user/request_login_link.html | 7 ++++--- coldfront/core/user/templates/user/sso_login_brc.html | 10 +++++----- coldfront/core/user/templates/user/sso_login_lrc.html | 10 +++++----- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/coldfront/core/user/templates/user/request_login_link.html b/coldfront/core/user/templates/user/request_login_link.html index a94405a47..0599d9a2f 100644 --- a/coldfront/core/user/templates/user/request_login_link.html +++ b/coldfront/core/user/templates/user/request_login_link.html @@ -18,11 +18,12 @@

    - If your institution is not listed in CILogon, you may use this form to - request a short-lived link to log in to the portal. + If you have previously used BRC resources, but your institution is not + listed in CILogon, you may use this form to request a short-lived link + to log in to the portal.

    - All new users must select an institution listed in CILogon. + New users must select an institution listed in CILogon.

    {% csrf_token %} diff --git a/coldfront/core/user/templates/user/sso_login_brc.html b/coldfront/core/user/templates/user/sso_login_brc.html index 5c32aa3f0..83430a5a8 100644 --- a/coldfront/core/user/templates/user/sso_login_brc.html +++ b/coldfront/core/user/templates/user/sso_login_brc.html @@ -100,15 +100,15 @@

    External Collaborator

    experience delays when requesting cluster access.
  • - External collaborators should select the provider of their home - institution. If your provider is not listed, select the - Google provider. + External collaborators should select their home institution as the + provider. If you are a new user and your institution is not listed, + select the Google provider.
    • {% flag_enabled 'LINK_LOGIN_ENABLED' as link_login_enabled %} {% if link_login_enabled %}
  • - If you are an existing user whose institution is not listed, you - may request a short-lived login link + If you are an existing external collaborator whose institution is + not listed, you may request a short-lived login link here.
    • {% endif %} diff --git a/coldfront/core/user/templates/user/sso_login_lrc.html b/coldfront/core/user/templates/user/sso_login_lrc.html index f181229b9..d8d97bd97 100644 --- a/coldfront/core/user/templates/user/sso_login_lrc.html +++ b/coldfront/core/user/templates/user/sso_login_lrc.html @@ -100,15 +100,15 @@

    External Collaborator

    experience delays when requesting cluster access.
  • - External collaborators should select the provider of their home - institution. If your provider is not listed, select the - Google provider. + External collaborators should select their home institution as the + provider. If you are a new user and your institution is not listed, + select the Google provider.
    • {% flag_enabled 'LINK_LOGIN_ENABLED' as link_login_enabled %} {% if link_login_enabled %}
  • - If you are an existing user whose institution is not listed, you - may request a short-lived login link + If you are an existing external collaborator whose institution is + not listed, you may request a short-lived login link here.
    • {% endif %} From 62906dad1260838c6212cccb3c942f15dfe554f1 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 15 Feb 2023 14:30:40 -0800 Subject: [PATCH 11/72] Update wording --- coldfront/core/user/templates/user/request_login_link.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/coldfront/core/user/templates/user/request_login_link.html b/coldfront/core/user/templates/user/request_login_link.html index 0599d9a2f..8cf24dd0c 100644 --- a/coldfront/core/user/templates/user/request_login_link.html +++ b/coldfront/core/user/templates/user/request_login_link.html @@ -18,9 +18,9 @@

    - If you have previously used BRC resources, but your institution is not - listed in CILogon, you may use this form to request a short-lived link - to log in to the portal. + If you are an existing user, but your institution is not listed in + CILogon, you may use this form to request a short-lived link to log in + to the portal.

    New users must select an institution listed in CILogon. From 4d7e409675cd2345425e8414ec0bfa7d0a6be0d6 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 8 Mar 2023 14:44:56 -0800 Subject: [PATCH 12/72] Correct typo in PI role name: 'Principle' -> 'Principal' in request hub; refactor sorting icons out into template that can be included and optionally hidden; hide it in the request hub --- .../project_removal_request_list_table.html | 19 +++++++------------ .../core/user/views_/request_hub_views.py | 13 +++++++------ coldfront/templates/common/table_sorter.html | 8 ++++++++ 3 files changed, 22 insertions(+), 18 deletions(-) create mode 100644 coldfront/templates/common/table_sorter.html diff --git a/coldfront/core/project/templates/project/project_removal/project_removal_request_list_table.html b/coldfront/core/project/templates/project/project_removal/project_removal_request_list_table.html index 909ed5808..de61baf87 100644 --- a/coldfront/core/project/templates/project/project_removal/project_removal_request_list_table.html +++ b/coldfront/core/project/templates/project/project_removal/project_removal_request_list_table.html @@ -3,37 +3,32 @@ # - - + {% include 'common/table_sorter.html' with table_sorter_field='id' %} {% if request_filter == 'pending' or adj == 'pending' %} Date Requested + {% include 'common/table_sorter.html' with table_sorter_field='request_time' %} {% else %} Date Completed + {% include 'common/table_sorter.html' with table_sorter_field='completion_time' %} {% endif %} - - User Email - - + {% include 'common/table_sorter.html' with table_sorter_field='project_user__user__email' %} User - - + {% include 'common/table_sorter.html' with table_sorter_field='project_user__user__username' %} Requester - - + {% include 'common/table_sorter.html' with table_sorter_field='requester__username' %} Project - - + {% include 'common/table_sorter.html' with table_sorter_field='project_user__project__name' %} Status diff --git a/coldfront/core/user/views_/request_hub_views.py b/coldfront/core/user/views_/request_hub_views.py index 85dc6e94e..5f36368fd 100644 --- a/coldfront/core/user/views_/request_hub_views.py +++ b/coldfront/core/user/views_/request_hub_views.py @@ -406,7 +406,7 @@ def get_secure_dir_join_request(self): request_pks = [request.pk for request in secure_dir_join_pending if request.allocation.project.projectuser_set.filter( user=user, - role__name='Principle Investigator', + role__name='Principal Investigator', status__name='Active' ).exists()] pi_cond = Q(pk__in=request_pks) @@ -416,7 +416,7 @@ def get_secure_dir_join_request(self): request_pks = [request.pk for request in secure_dir_join_complete if request.allocation.project.projectuser_set.filter( user=user, - role__name='Principle Investigator', + role__name='Principal Investigator', status__name='Active' ).exists()] pi_cond = Q(pk__in=request_pks) @@ -469,7 +469,7 @@ def get_secure_dir_remove_request(self): request_pks = [request.pk for request in secure_dir_remove_pending if request.allocation.project.projectuser_set.filter( user=user, - role__name='Principle Investigator', + role__name='Principal Investigator', status__name='Active' ).exists()] pi_cond = Q(pk__in=request_pks) @@ -479,7 +479,7 @@ def get_secure_dir_remove_request(self): request_pks = [request.pk for request in secure_dir_remove_complete if request.allocation.project.projectuser_set.filter( user=user, - role__name='Principle Investigator', + role__name='Principal Investigator', status__name='Active' ).exists()] pi_cond = Q(pk__in=request_pks) @@ -533,7 +533,7 @@ def get_secure_dir_request(self): request_pks = [request.pk for request in secure_dir_pending if request.project.projectuser_set.filter( user=user, - role__name='Principle Investigator', + role__name='Principal Investigator', status__name='Active' ).exists()] pi_cond = Q(pk__in=request_pks) @@ -543,7 +543,7 @@ def get_secure_dir_request(self): request_pks = [request.pk for request in secure_dir_complete if request.project.projectuser_set.filter( user=user, - role__name='Principle Investigator', + role__name='Principal Investigator', status__name='Active' ).exists()] pi_cond = Q(pk__in=request_pks) @@ -604,5 +604,6 @@ def get_context_data(self, **kwargs): context['admin_staff'] = (self.request.user.is_superuser or self.request.user.is_staff) + context['hide_table_sorter'] = True return context diff --git a/coldfront/templates/common/table_sorter.html b/coldfront/templates/common/table_sorter.html new file mode 100644 index 000000000..907e21d04 --- /dev/null +++ b/coldfront/templates/common/table_sorter.html @@ -0,0 +1,8 @@ +{% if not hide_table_sorter %} + + + + + + +{% endif %} From c43eb04090f374b72cf0179ac3149a8abe912c7c Mon Sep 17 00:00:00 2001 From: Viraat Chandra Date: Sat, 11 Mar 2023 22:49:20 -0800 Subject: [PATCH 13/72] complete tabbed ui --- .../templates/project/project_detail.html | 2 +- .../savio/project_request_surveys_modal.html | 73 +++++++++++++++++++ coldfront/core/project/views.py | 4 + 3 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 coldfront/core/project/templates/project/project_request/savio/project_request_surveys_modal.html diff --git a/coldfront/core/project/templates/project/project_detail.html b/coldfront/core/project/templates/project/project_detail.html index f36a836b8..31d8a0ac4 100644 --- a/coldfront/core/project/templates/project/project_detail.html +++ b/coldfront/core/project/templates/project/project_detail.html @@ -475,5 +475,5 @@

    -{% include 'project/project_request/savio/project_request_survey_modal.html' with survey_form=survey_answers %} +{% include 'project/project_request/savio/project_request_surveys_modal.html' with survey_answers=survey_answers %} {% endblock %} diff --git a/coldfront/core/project/templates/project/project_request/savio/project_request_surveys_modal.html b/coldfront/core/project/templates/project/project_request/savio/project_request_surveys_modal.html new file mode 100644 index 000000000..55f78cd36 --- /dev/null +++ b/coldfront/core/project/templates/project/project_request/savio/project_request_surveys_modal.html @@ -0,0 +1,73 @@ +{% load crispy_forms_tags %} + + + + diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py index 3c3037231..91da52edc 100644 --- a/coldfront/core/project/views.py +++ b/coldfront/core/project/views.py @@ -278,6 +278,10 @@ def get_context_data(self, **kwargs): context['survey_answers'] = SavioProjectSurveyForm( initial=allocation_request.survey_answers, disable_fields=True) + allocation_requests = SavioProjectAllocationRequest.objects.filter(project=self.object) + survey_answers_list = list(map(lambda x: SavioProjectSurveyForm(initial=x.survey_answers, disable_fields=True), allocation_requests)) + context['survey_answers'] = list(zip(allocation_requests, survey_answers_list)) + context['user_agreement_signed'] = \ access_agreement_signed(self.request.user) From b7b1dd388979da2756dbd84519677ece9bf207aa Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 22 Mar 2023 10:09:29 -0700 Subject: [PATCH 14/72] Disallow superusers, staff from logging in using a link for security reasons --- coldfront/core/user/utils.py | 22 ++++++++++++++++++- .../core/user/views_/link_login_views.py | 22 ++++++++++++++++++- .../templates/email/login_link_ineligible.txt | 12 ++++++++++ 3 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 coldfront/templates/email/login_link_ineligible.txt diff --git a/coldfront/core/user/utils.py b/coldfront/core/user/utils.py index 2e3e45312..2f3484de5 100644 --- a/coldfront/core/user/utils.py +++ b/coldfront/core/user/utils.py @@ -305,7 +305,6 @@ def send_login_link_email(email_address): template_name = 'email/login_link.txt' context = { 'PORTAL_NAME': settings.PORTAL_NAME, - 'center_name': import_from_settings('CENTER_NAME', ''), 'login_url': login_token_url(email_address.user), 'login_link_max_age_minutes': ( import_from_settings('SESAME_MAX_AGE') // 60), @@ -316,3 +315,24 @@ def send_login_link_email(email_address): receiver_list = [email_address.email, ] send_email_template(subject, template_name, context, sender, receiver_list) + + +def send_login_link_ineligible_email(email_address, reason): + """Send an email containing a reason explaining why the User with + the given EmailAddress is ineligible to receive a login link.""" + email_enabled = import_from_settings('EMAIL_ENABLED', False) + if not email_enabled: + return + + subject = 'Ineligible for Login Link' + template_name = 'email/login_link_ineligible.txt' + context = { + 'PORTAL_NAME': settings.PORTAL_NAME, + 'reason': reason, + 'signature': import_from_settings('EMAIL_SIGNATURE', ''), + } + + sender = import_from_settings('EMAIL_SENDER') + receiver_list = [email_address.email, ] + + send_email_template(subject, template_name, context, sender, receiver_list) diff --git a/coldfront/core/user/views_/link_login_views.py b/coldfront/core/user/views_/link_login_views.py index f771e63ee..7c6343b3c 100644 --- a/coldfront/core/user/views_/link_login_views.py +++ b/coldfront/core/user/views_/link_login_views.py @@ -10,6 +10,7 @@ from coldfront.core.user.forms_.link_login_forms import RequestLoginLinkForm from coldfront.core.user.utils import send_login_link_email +from coldfront.core.user.utils import send_login_link_ineligible_email from coldfront.core.utils.common import import_from_settings @@ -23,6 +24,9 @@ class RequestLoginLinkView(FormView): form_class = RequestLoginLinkForm template_name = 'user/request_login_link.html' + class UserIneligibleException(Exception): + pass + def dispatch(self, request, *args, **kwargs): if request.user.is_authenticated: return redirect(reverse('home')) @@ -34,7 +38,13 @@ def form_valid(self, form): email = form.cleaned_data.get('email') email_address = self._validate_email_address(email) if email_address: - send_login_link_email(email_address) + try: + self._validate_user_eligible(email_address.user) + except self.UserIneligibleException as e: + reason = str(e) + send_login_link_ineligible_email(email_address, reason) + else: + send_login_link_email(email_address) self._send_success_message() return super().form_valid(form) @@ -70,6 +80,16 @@ def _validate_email_address(self, email): messages.error(self.request, message) return email_address + def _validate_user_eligible(self, user): + """Return None if the given User is eligible to log in using + this method. Otherwise, raise an exception with a user-facing + message explaining why the user is ineligible.""" + # Staff users and superusers + if user.is_staff or user.is_superuser: + raise self.UserIneligibleException( + 'For security reasons, portal staff are disallowed from ' + 'logging in using a link.') + class LinkLoginView(LoginView): """A subclass of Django Sesame's login view, with custom logic.""" diff --git a/coldfront/templates/email/login_link_ineligible.txt b/coldfront/templates/email/login_link_ineligible.txt new file mode 100644 index 000000000..7908590eb --- /dev/null +++ b/coldfront/templates/email/login_link_ineligible.txt @@ -0,0 +1,12 @@ +Dear {{ PORTAL_NAME }} user, + +You requested a link to log in to the portal. + +You are ineligible to receive a link for the following reason: + +{{ reason }} + +Please contact a system administrator if you have any questions. + +Thank you, +{{ signature }} \ No newline at end of file From 75675741f50630edb554e1690bea1c69023e3c5b Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 22 Mar 2023 10:40:12 -0700 Subject: [PATCH 15/72] Configure link-login settings in Ansible; raise error if flag combinations are invalid --- bootstrap/ansible/main.copyme | 7 ++++--- bootstrap/ansible/settings_template.tmpl | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/bootstrap/ansible/main.copyme b/bootstrap/ansible/main.copyme index cd0c670e8..af16b66a3 100644 --- a/bootstrap/ansible/main.copyme +++ b/bootstrap/ansible/main.copyme @@ -81,9 +81,10 @@ cilogon_app_client_id: "" cilogon_app_secret: "" # Django Flags settings. -# TODO: For LRC, disable basic auth. and enable SSO. -flag_basic_auth_enabled: True -flag_sso_enabled: False +# TODO: For LRC, disable link login. +flag_basic_auth_enabled: False +flag_sso_enabled: True +flag_link_login_enabled: True # TODO: For LRC, disable BRC and enable LRC. flag_brc_enabled: True flag_lrc_enabled: False diff --git a/bootstrap/ansible/settings_template.tmpl b/bootstrap/ansible/settings_template.tmpl index 21b2ddfb1..4f1557231 100644 --- a/bootstrap/ansible/settings_template.tmpl +++ b/bootstrap/ansible/settings_template.tmpl @@ -162,12 +162,28 @@ FLAGS = { ], 'BASIC_AUTH_ENABLED': [{'condition': 'boolean', 'value': {{ flag_basic_auth_enabled }}}], 'BRC_ONLY': [{'condition': 'boolean', 'value': {{ flag_brc_enabled }}}], + 'LINK_LOGIN_ENABLED': [{'condition': 'boolean', 'value': {{ flag_link_login_enabled }}}], 'LRC_ONLY': [{'condition': 'boolean', 'value': {{ flag_lrc_enabled }}}], 'SECURE_DIRS_REQUESTABLE': [{'condition': 'boolean', 'value': {{ flag_brc_enabled }}}], 'SERVICE_UNITS_PURCHASABLE': [{'condition': 'boolean', 'value': {{ flag_brc_enabled }}}], 'SSO_ENABLED': [{'condition': 'boolean', 'value': {{ flag_sso_enabled }}}], } +# Enforce that boolean flags are consistent with each other. +from django.core.exceptions import ImproperlyConfigured +if not (FLAGS['BRC_ONLY'][0]['value'] ^ FLAGS['LRC_ONLY'][0]['value']): + raise ImproperlyConfigured( + 'Exactly one of BRC_ONLY, LRC_ONLY should be enabled.') +if not ( + FLAGS['BASIC_AUTH_ENABLED'][0]['value'] ^ + FLAGS['SSO_ENABLED'][0]['value']): + raise ImproperlyConfigured( + 'Exactly one of BASIC_AUTH_ENABLED, SSO_ENABLED should be enabled.') +if not FLAGS['SSO_ENABLED'][0]['value'] and FLAGS['LINK_LOGIN_ENABLED']: + raise ImproperlyConfigured( + 'LINK_LOGIN_ENABLED should only be enabled when SSO_ENABLED is ' + 'enabled.') + #------------------------------------------------------------------------------ # django-q settings #------------------------------------------------------------------------------ From 416c982a63bd47f1f53391a5c602e33521380c75 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 22 Mar 2023 11:19:07 -0700 Subject: [PATCH 16/72] Do not send login link emails to unverified addresses --- coldfront/core/user/views_/link_login_views.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/coldfront/core/user/views_/link_login_views.py b/coldfront/core/user/views_/link_login_views.py index 7c6343b3c..c070ab823 100644 --- a/coldfront/core/user/views_/link_login_views.py +++ b/coldfront/core/user/views_/link_login_views.py @@ -64,13 +64,12 @@ def _send_success_message(self): def _validate_email_address(self, email): """Return an EmailAddress object corresponding to the given - address (str) if one exists. Otherwise, return None. Write user - and log messages as needed.""" - email_address = None + address (str) if one exists and is verified. Otherwise, return + None. Write user and log messages as needed.""" try: email_address = EmailAddress.objects.get(email=email) except EmailAddress.DoesNotExist: - pass + return None except EmailAddress.MultipleObjectsReturned: logger.error( f'Unexpectedly found multiple EmailAddresses for email ' @@ -78,7 +77,11 @@ def _validate_email_address(self, email): message = ( 'Unexpected server error. Please contact an administrator.') messages.error(self.request, message) - return email_address + return None + else: + if not email_address.verified: + return None + return email_address def _validate_user_eligible(self, user): """Return None if the given User is eligible to log in using From 4cb03a66f8a217cb2928c09a50141a614163d325 Mon Sep 17 00:00:00 2001 From: Viraat Chandra Date: Thu, 23 Mar 2023 16:06:16 -0700 Subject: [PATCH 17/72] refactor sorting and initial ordering in request hub view --- ...on_cluster_account_request_list_table.html | 24 +++----- ...re_dir_manage_user_request_list_table.html | 6 +- .../secure_dir_request_list_table.html | 12 ++-- .../request_list_table.html | 28 ++------- .../project_join_request_list_table.html | 3 +- .../project_renewal_request_list_table.html | 35 ++--------- .../savio/project_request_list_table.html | 15 ++--- .../vector/project_request_list_table.html | 15 ++--- .../core/user/views_/request_hub_views.py | 58 ++++++++++--------- 9 files changed, 66 insertions(+), 130 deletions(-) diff --git a/coldfront/core/allocation/templates/allocation/allocation_cluster_account_request_list_table.html b/coldfront/core/allocation/templates/allocation/allocation_cluster_account_request_list_table.html index de0017a7d..e8c315971 100644 --- a/coldfront/core/allocation/templates/allocation/allocation_cluster_account_request_list_table.html +++ b/coldfront/core/allocation/templates/allocation/allocation_cluster_account_request_list_table.html @@ -4,34 +4,28 @@ # - - + {% include 'common/table_sorter.html' with table_sorter_field='id' %} - {% if request_filter == 'completed' %} - Completion Time - - - {% else %} + {% if request_filter == 'pending' or adj == 'pending' %} Request Time - - + {% include 'common/table_sorter.html' with table_sorter_field='request_time' %} + {% else %} + Completion Time + {% include 'common/table_sorter.html' with table_sorter_field='completion_time' %} {% endif %} User Email - - + {% include 'common/table_sorter.html' with table_sorter_field='project_user__user__email' %} Cluster Username - - + {% include 'common/table_sorter.html' with table_sorter_field='project_user__user__username' %} Project - - + {% include 'common/table_sorter.html' with table_sorter_field='project_user__project__name' %} Allocation diff --git a/coldfront/core/allocation/templates/secure_dir/secure_dir_manage_user_request_list_table.html b/coldfront/core/allocation/templates/secure_dir/secure_dir_manage_user_request_list_table.html index 366075a51..3e20d1508 100644 --- a/coldfront/core/allocation/templates/secure_dir/secure_dir_manage_user_request_list_table.html +++ b/coldfront/core/allocation/templates/secure_dir/secure_dir_manage_user_request_list_table.html @@ -3,8 +3,7 @@ # - - + {% include 'common/table_sorter.html' with table_sorter_field='id' %} {% if request_filter == 'pending' or adj == 'pending' %} @@ -12,8 +11,7 @@ {% else %} Date Completed {% endif %} - - + {% include 'common/table_sorter.html' with table_sorter_field='created' %} User diff --git a/coldfront/core/allocation/templates/secure_dir/secure_dir_request/secure_dir_request_list_table.html b/coldfront/core/allocation/templates/secure_dir/secure_dir_request/secure_dir_request_list_table.html index 44d7f543a..d6c694152 100644 --- a/coldfront/core/allocation/templates/secure_dir/secure_dir_request/secure_dir_request_list_table.html +++ b/coldfront/core/allocation/templates/secure_dir/secure_dir_request/secure_dir_request_list_table.html @@ -3,23 +3,19 @@ # - - + {% include 'common/table_sorter.html' with table_sorter_field='id' %} Requested - - + {% include 'common/table_sorter.html' with table_sorter_field='request_time' %} Requester - - + {% include 'common/table_sorter.html' with table_sorter_field='requester__email' %} Project - - + {% include 'common/table_sorter.html' with table_sorter_field='project' %} Status diff --git a/coldfront/core/project/templates/project/project_allocation_addition/request_list_table.html b/coldfront/core/project/templates/project/project_allocation_addition/request_list_table.html index 3a1fae7d9..b6538d302 100644 --- a/coldfront/core/project/templates/project/project_allocation_addition/request_list_table.html +++ b/coldfront/core/project/templates/project/project_allocation_addition/request_list_table.html @@ -3,39 +3,19 @@ # - - - - - - + {% include 'common/table_sorter.html' with table_sorter_field='id' %} Date Requested/
    Last Modified - - - - - - + {% include 'common/table_sorter.html' with table_sorter_field='modified' %} Requester Email - - - - - - + {% include 'common/table_sorter.html' with table_sorter_field='requester__email' %} Project - - - - - - + {% include 'common/table_sorter.html' with table_sorter_field='project' %} Number of Service Units Status diff --git a/coldfront/core/project/templates/project/project_join_request_list_table.html b/coldfront/core/project/templates/project/project_join_request_list_table.html index 20fcb94e3..e5e806bc2 100644 --- a/coldfront/core/project/templates/project/project_join_request_list_table.html +++ b/coldfront/core/project/templates/project/project_join_request_list_table.html @@ -15,8 +15,7 @@ Date Requested - - + {% include 'common/table_sorter.html' with table_sorter_field='created' %} Reason diff --git a/coldfront/core/project/templates/project/project_renewal/project_renewal_request_list_table.html b/coldfront/core/project/templates/project/project_renewal/project_renewal_request_list_table.html index a13811d24..f31a1d9fc 100644 --- a/coldfront/core/project/templates/project/project_renewal/project_renewal_request_list_table.html +++ b/coldfront/core/project/templates/project/project_renewal/project_renewal_request_list_table.html @@ -3,48 +3,23 @@ # - - - - - - + {% include 'common/table_sorter.html' with table_sorter_field='id' %} Requested - - - - - - + {% include 'common/table_sorter.html' with table_sorter_field='request_time' %} Requester - - - - - - + {% include 'common/table_sorter.html' with table_sorter_field='requester__email' %} Project - - - - - - + {% include 'common/table_sorter.html' with table_sorter_field='project' %} PI - - - - - - + {% include 'common/table_sorter.html' with table_sorter_field='pi' %} Status diff --git a/coldfront/core/project/templates/project/project_request/savio/project_request_list_table.html b/coldfront/core/project/templates/project/project_request/savio/project_request_list_table.html index 3291afe1b..bc87e515f 100644 --- a/coldfront/core/project/templates/project/project_request/savio/project_request_list_table.html +++ b/coldfront/core/project/templates/project/project_request/savio/project_request_list_table.html @@ -3,28 +3,23 @@ # - - + {% include 'common/table_sorter.html' with table_sorter_field='id' %} Requested - - + {% include 'common/table_sorter.html' with table_sorter_field='request_time' %} Requester - - + {% include 'common/table_sorter.html' with table_sorter_field='requester__email' %} Project - - + {% include 'common/table_sorter.html' with table_sorter_field='project' %} PI - - + {% include 'common/table_sorter.html' with table_sorter_field='pi' %} Status diff --git a/coldfront/core/project/templates/project/project_request/vector/project_request_list_table.html b/coldfront/core/project/templates/project/project_request/vector/project_request_list_table.html index 7f9ce2da7..0525a5ef7 100644 --- a/coldfront/core/project/templates/project/project_request/vector/project_request_list_table.html +++ b/coldfront/core/project/templates/project/project_request/vector/project_request_list_table.html @@ -3,28 +3,23 @@ # - - + {% include 'common/table_sorter.html' with table_sorter_field='id' %} Date Requested/
    Last Modified - - + {% include 'common/table_sorter.html' with table_sorter_field='modified' %} Requester Email - - + {% include 'common/table_sorter.html' with table_sorter_field='requester__email' %} Project - - + {% include 'common/table_sorter.html' with table_sorter_field='project' %} PI - - + {% include 'common/table_sorter.html' with table_sorter_field='pi' %} Status diff --git a/coldfront/core/user/views_/request_hub_views.py b/coldfront/core/user/views_/request_hub_views.py index 5f36368fd..6d8e14167 100644 --- a/coldfront/core/user/views_/request_hub_views.py +++ b/coldfront/core/user/views_/request_hub_views.py @@ -27,6 +27,7 @@ class RequestListItem: Object to keep track of all variables used in for each request type in the request hub """ + def __init__(self): num = None title = None @@ -89,12 +90,12 @@ def get_cluster_account_request(self): cluster_account_list_complete = \ ClusterAccessRequest.objects.filter( status__name__in=['Denied', 'Complete'], **kwargs).order_by( - 'modified') + '-modified') cluster_account_list_pending = \ ClusterAccessRequest.objects.filter( status__name__in=['Pending - Add', 'Processing'], **kwargs).order_by( - 'modified') + '-modified') cluster_request_object.num = self.paginators cluster_request_object.pending_queryset = \ @@ -130,12 +131,12 @@ def get_project_removal_request(self): removal_request_pending = \ ProjectUserRemovalRequest.objects.filter( status__name__in=['Pending', 'Processing'], *args).order_by( - 'modified') + '-modified') removal_request_complete = \ ProjectUserRemovalRequest.objects.filter( status__name='Complete', *args).order_by( - 'modified') + '-modified') removal_request_object.num = self.paginators removal_request_object.pending_queryset = \ @@ -174,7 +175,7 @@ def get_savio_project_request(self): annotate_queryset_with_allocation_period_not_started_bool( SavioProjectAllocationRequest.objects.filter( status__name__in=pending_status_names, *args - ).order_by('request_time')) + ).order_by('-request_time')) complete_status_names = [ 'Approved - Complete', 'Approved - Scheduled', 'Denied'] @@ -182,7 +183,7 @@ def get_savio_project_request(self): annotate_queryset_with_allocation_period_not_started_bool( SavioProjectAllocationRequest.objects.filter( status__name__in=complete_status_names, *args - ).order_by('request_time')) + ).order_by('-request_time')) savio_proj_request_object.num = self.paginators savio_proj_request_object.pending_queryset = \ @@ -219,14 +220,12 @@ def get_vector_project_request(self): project_request_pending = \ VectorProjectAllocationRequest.objects.filter( status__name__in=['Under Review', 'Approved - Processing'], - *args).order_by( - 'modified') + *args).order_by('-modified') project_request_complete = \ VectorProjectAllocationRequest.objects.filter( status__name__in=['Approved - Complete', 'Denied'], - *args).order_by( - 'modified') + *args).order_by('-modified') vector_proj_request_object.num = self.paginators vector_proj_request_object.pending_queryset = \ @@ -263,12 +262,12 @@ def get_project_join_request(self): project_join_request_pending = \ ProjectUserJoinRequest.objects.filter( project_user__status__name='Pending - Add', - *args).order_by('modified') + *args).order_by('-modified') project_join_request_complete = \ ProjectUserJoinRequest.objects.filter( project_user__status__name__in=['Active', 'Denied'], - *args).order_by('modified') + *args).order_by('-modified') proj_join_request_object.num = self.paginators proj_join_request_object.pending_queryset = \ @@ -308,14 +307,14 @@ def get_project_renewal_request(self): annotate_queryset_with_allocation_period_not_started_bool( AllocationRenewalRequest.objects.filter( status__name__in=pending_status_names, *args - ).order_by('request_time')) + ).order_by('-request_time')) complete_status_names = ['Approved', 'Complete', 'Denied'] project_renewal_request_complete = \ annotate_queryset_with_allocation_period_not_started_bool( AllocationRenewalRequest.objects.filter( status__name__in=complete_status_names, *args - ).order_by('request_time')) + ).order_by('-request_time')) proj_renewal_request_object.num = self.paginators proj_renewal_request_object.pending_queryset = \ @@ -347,10 +346,10 @@ def get_su_purchase_request(self): user = self.request.user su_purchase_request_pending = AllocationAdditionRequest.objects.filter( - status__name__in=['Under Review']).order_by('modified') + status__name__in=['Under Review']).order_by('-modified') su_purchase_request_complete = AllocationAdditionRequest.objects.filter( - status__name__in=['Complete', 'Denied']).order_by('modified') + status__name__in=['Complete', 'Denied']).order_by('-modified') if not self.show_all_requests: request_ids = [ @@ -395,10 +394,10 @@ def get_secure_dir_join_request(self): user = self.request.user secure_dir_join_pending = SecureDirAddUserRequest.objects.filter( - status__name__in=['Pending', 'Processing']).order_by('modified') + status__name__in=['Pending', 'Processing']).order_by('-modified') secure_dir_join_complete = SecureDirAddUserRequest.objects.filter( - status__name__in=['Complete', 'Denied']).order_by('modified') + status__name__in=['Complete', 'Denied']).order_by('-modified') if not self.show_all_requests: # limit secure_dir_requests to objects user is a PI of or user has @@ -411,7 +410,8 @@ def get_secure_dir_join_request(self): ).exists()] pi_cond = Q(pk__in=request_pks) - secure_dir_join_pending = secure_dir_join_pending.filter(user_cond | pi_cond) + secure_dir_join_pending = secure_dir_join_pending.filter( + user_cond | pi_cond) request_pks = [request.pk for request in secure_dir_join_complete if request.allocation.project.projectuser_set.filter( @@ -421,7 +421,8 @@ def get_secure_dir_join_request(self): ).exists()] pi_cond = Q(pk__in=request_pks) - secure_dir_join_complete = secure_dir_join_complete.filter(user_cond | pi_cond) + secure_dir_join_complete = secure_dir_join_complete.filter( + user_cond | pi_cond) secure_dir_join_request_object.num = self.paginators secure_dir_join_request_object.pending_queryset = \ @@ -458,10 +459,10 @@ def get_secure_dir_remove_request(self): user = self.request.user secure_dir_remove_pending = SecureDirRemoveUserRequest.objects.filter( - status__name__in=['Pending', 'Processing']).order_by('modified') + status__name__in=['Pending', 'Processing']).order_by('-modified') secure_dir_remove_complete = SecureDirRemoveUserRequest.objects.filter( - status__name__in=['Complete', 'Denied']).order_by('modified') + status__name__in=['Complete', 'Denied']).order_by('-modified') if not self.show_all_requests: # limit secure_dir_requests to objects user is a PI of or user has @@ -474,7 +475,8 @@ def get_secure_dir_remove_request(self): ).exists()] pi_cond = Q(pk__in=request_pks) - secure_dir_remove_pending = secure_dir_remove_pending.filter(user_cond | pi_cond) + secure_dir_remove_pending = secure_dir_remove_pending.filter( + user_cond | pi_cond) request_pks = [request.pk for request in secure_dir_remove_complete if request.allocation.project.projectuser_set.filter( @@ -484,7 +486,8 @@ def get_secure_dir_remove_request(self): ).exists()] pi_cond = Q(pk__in=request_pks) - secure_dir_remove_complete = secure_dir_remove_complete.filter(user_cond | pi_cond) + secure_dir_remove_complete = secure_dir_remove_complete.filter( + user_cond | pi_cond) secure_dir_remove_request_object.num = self.paginators secure_dir_remove_request_object.pending_queryset = \ @@ -522,10 +525,10 @@ def get_secure_dir_request(self): user = self.request.user secure_dir_pending = SecureDirRequest.objects.filter( - status__name__in=['Under Review', 'Approved - Processing']).order_by('modified') + status__name__in=['Under Review', 'Approved - Processing']).order_by('-modified') secure_dir_complete = SecureDirRequest.objects.filter( - status__name__in=['Approved - Complete', 'Denied']).order_by('modified') + status__name__in=['Approved - Complete', 'Denied']).order_by('-modified') if not self.show_all_requests: # limit secure_dir_requests to objects user is a PI of or user has @@ -548,7 +551,8 @@ def get_secure_dir_request(self): ).exists()] pi_cond = Q(pk__in=request_pks) - secure_dir_complete = secure_dir_complete.filter(user_cond | pi_cond) + secure_dir_complete = secure_dir_complete.filter( + user_cond | pi_cond) secure_dir_request_object.num = self.paginators secure_dir_request_object.pending_queryset = \ From 4acb6c79a69785c4fa47b4a1261e9afe594715c0 Mon Sep 17 00:00:00 2001 From: Viraat Chandra Date: Thu, 23 Mar 2023 16:36:18 -0700 Subject: [PATCH 18/72] update initial order for individual requests list views --- coldfront/core/allocation/views_/secure_dir_views.py | 4 ++-- .../core/project/views_/addition_views/approval_views.py | 2 +- coldfront/core/project/views_/join_views/approval_views.py | 4 ++-- .../core/project/views_/new_project_views/approval_views.py | 4 ++-- coldfront/core/project/views_/removal_views.py | 2 +- coldfront/core/project/views_/renewal_views/approval_views.py | 3 ++- 6 files changed, 10 insertions(+), 9 deletions(-) diff --git a/coldfront/core/allocation/views_/secure_dir_views.py b/coldfront/core/allocation/views_/secure_dir_views.py index f53e07374..9ee0e7cea 100644 --- a/coldfront/core/allocation/views_/secure_dir_views.py +++ b/coldfront/core/allocation/views_/secure_dir_views.py @@ -353,7 +353,7 @@ def get_queryset(self): direction = '-' order_by = direction + order_by else: - order_by = 'id' + order_by = '-modified' pending_status = self.request_status_obj.objects.filter( Q(name__icontains='Pending') | Q(name__icontains='Processing')) @@ -1091,7 +1091,7 @@ def get_queryset(self): direction = '-' order_by = direction + order_by else: - order_by = 'id' + order_by = '-modified' return SecureDirRequest.objects.order_by(order_by) diff --git a/coldfront/core/project/views_/addition_views/approval_views.py b/coldfront/core/project/views_/addition_views/approval_views.py index 8eaf4da98..8fdaaebc6 100644 --- a/coldfront/core/project/views_/addition_views/approval_views.py +++ b/coldfront/core/project/views_/addition_views/approval_views.py @@ -240,7 +240,7 @@ def get_order_by(self): direction = '-' order_by = direction + order_by else: - order_by = 'id' + order_by = '-modified' return order_by def test_func(self): diff --git a/coldfront/core/project/views_/join_views/approval_views.py b/coldfront/core/project/views_/join_views/approval_views.py index 0e1f25a96..89bd92935 100644 --- a/coldfront/core/project/views_/join_views/approval_views.py +++ b/coldfront/core/project/views_/join_views/approval_views.py @@ -244,9 +244,9 @@ def get_queryset(self): direction = '' else: direction = '-' - order_by = direction + 'created' + order_by = direction + order_by else: - order_by = '-created' + order_by = '-modified' project_join_requests = \ ProjectUserJoinRequest.objects.filter( diff --git a/coldfront/core/project/views_/new_project_views/approval_views.py b/coldfront/core/project/views_/new_project_views/approval_views.py index 9d42339bb..0b5d1b5c0 100644 --- a/coldfront/core/project/views_/new_project_views/approval_views.py +++ b/coldfront/core/project/views_/new_project_views/approval_views.py @@ -69,7 +69,7 @@ def get_queryset(self): direction = '-' order_by = direction + order_by else: - order_by = 'id' + order_by = '-request_time' return annotate_queryset_with_allocation_period_not_started_bool( SavioProjectAllocationRequest.objects.order_by(order_by)) @@ -910,7 +910,7 @@ def get_queryset(self): direction = '-' order_by = direction + order_by else: - order_by = 'id' + order_by = '-modified' return VectorProjectAllocationRequest.objects.order_by(order_by) def get_context_data(self, **kwargs): diff --git a/coldfront/core/project/views_/removal_views.py b/coldfront/core/project/views_/removal_views.py index ffba9b603..62bfcb307 100644 --- a/coldfront/core/project/views_/removal_views.py +++ b/coldfront/core/project/views_/removal_views.py @@ -217,7 +217,7 @@ def get_queryset(self): direction = '-' order_by = direction + order_by else: - order_by = 'id' + order_by = '-modified' project_removal_status_complete, _ = \ ProjectUserRemovalRequestStatusChoice.objects.get_or_create( diff --git a/coldfront/core/project/views_/renewal_views/approval_views.py b/coldfront/core/project/views_/renewal_views/approval_views.py index a331f6a19..447361b80 100644 --- a/coldfront/core/project/views_/renewal_views/approval_views.py +++ b/coldfront/core/project/views_/renewal_views/approval_views.py @@ -55,7 +55,8 @@ def get_queryset(self): direction = '-' order_by = direction + order_by else: - order_by = 'id' + order_by = '-request_time' + return annotate_queryset_with_allocation_period_not_started_bool( AllocationRenewalRequest.objects.order_by(order_by)) From 2e944fbf4d76a644f727f690fc21bcf4aa0c5091 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Tue, 28 Mar 2023 00:44:42 -0500 Subject: [PATCH 19/72] initial docker-compose --- Dockerfile | 66 +++++++++++++++--------- bootstrap/ansible/main.copyme | 2 + bootstrap/ansible/settings_template.tmpl | 6 +-- docker-compose.yml | 60 +++++++++++++++++++++ 4 files changed, 106 insertions(+), 28 deletions(-) create mode 100644 docker-compose.yml diff --git a/Dockerfile b/Dockerfile index b56ea76b9..041b6079c 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,34 +1,50 @@ -FROM centos:8 +FROM centos/python-38-centos7 LABEL description="coldfront" + # install dependencies -RUN yum -y install epel-release -RUN yum -y update -RUN yum -y install python36 python36-devel git memcached redis +# RUN yum -y install epel-release +# RUN yum -y update +# RUN yum -y install python36 python36-devel git memcached redis +USER root WORKDIR /root # install coldfront -RUN mkdir /opt/coldfront_app - -WORKDIR /opt/coldfront_app - -RUN cd /opt/coldfront_app -RUN git clone https://github.com/ubccr/coldfront.git -RUN python3.6 -mvenv venv -RUN source venv/bin/activate - -WORKDIR /opt/coldfront_app/coldfront - -RUN cd /opt/coldfront_app/coldfront -RUN pip3 install wheel -RUN pip3 install -r requirements.txt -RUN cp coldfront/config/local_settings.py.sample coldfront/config/local_settings.py -RUN cp coldfront/config/local_strings.py.sample coldfront/config/local_strings.py - -RUN python3 ./manage.py initial_setup -RUN python3 ./manage.py load_test_data - -EXPOSE 8000 +RUN mkdir -p /vagrant/coldfront_app +WORKDIR /vagrant/coldfront_app/ +RUN git clone -b issue_521 https://github.com/ucb-rit/coldfront.git +WORKDIR ./coldfront +COPY main.yml ./main.yml + +RUN pip3 install wheel jinja2-cli pyyaml \ + && pip3 install -r requirements.txt + +RUN cp coldfront/config/local_settings.py.sample \ + coldfront/config/local_settings.py \ + && cp coldfront/config/local_strings.py.sample \ + coldfront/config/local_strings.py \ + && python -c \ +"from jinja2 import Template, Environment, FileSystemLoader; \ +import yaml; \ +env = Environment(loader=FileSystemLoader('bootstrap/ansible/')); \ +env.filters['bool'] = lambda x: str(x).lower() in ['true', 'yes', 'on', '1']; \ +options = yaml.safe_load(open('main.yml').read()); \ +options.update({'redis_host': 'redis', 'db_host': 'db', \ + 'debug_toolbar_ips':['host.docker.internal']}); \ +print(env.get_template('settings_template.tmpl').render(options))" \ + > coldfront/config/dev_settings.py + +RUN mkdir -p /var/log/user_portals/cf_mybrc \ + && touch /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ + && chmod 775 /var/log/user_portals/cf_mybrc \ + && chmod 664 /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ + && chmod +x ./manage.py + + +CMD ./manage.py initial_setup \ + && ./manage.py runserver + +EXPOSE 8080 STOPSIGNAL SIGINT diff --git a/bootstrap/ansible/main.copyme b/bootstrap/ansible/main.copyme index cd0c670e8..a3ac645c1 100644 --- a/bootstrap/ansible/main.copyme +++ b/bootstrap/ansible/main.copyme @@ -21,6 +21,7 @@ djangoprojname: coldfront # The name of the PostgreSQL database. # TODO: For LRC, set this to 'cf_lrc_db'. db_name: cf_brc_db +db_host: localhost # The credentials for the database admin user. # TODO: Replace the username and password. @@ -30,6 +31,7 @@ db_admin_passwd: '' # The password for Redis. # TODO: Replace the password. redis_passwd: '' +redis_host: localhost # Log paths. # TODO: For LRC, use the substring 'cf_mylrc'. diff --git a/bootstrap/ansible/settings_template.tmpl b/bootstrap/ansible/settings_template.tmpl index 21b2ddfb1..818c6abfe 100644 --- a/bootstrap/ansible/settings_template.tmpl +++ b/bootstrap/ansible/settings_template.tmpl @@ -46,7 +46,7 @@ DATABASES = { 'NAME': '{{ db_name }}', 'USER': '{{ db_admin_user }}', 'PASSWORD': '{{ db_admin_passwd }}', - 'HOST': 'localhost', + 'HOST': '{{ db_host }}', 'PORT': '5432', }, } @@ -135,7 +135,7 @@ CONSTANCE_CONFIG = { 'LAUNCH_DATE': (date(1970, 1, 1), 'The date the portal was launched.'), } CONSTANCE_REDIS_CONNECTION = { - 'host': '127.0.0.1', + 'host': '{{ redis_host }}', 'port': 6379, 'db': 0, 'password': '{{ redis_passwd }}', @@ -174,7 +174,7 @@ FLAGS = { Q_CLUSTER = { 'redis': { - 'host': '127.0.0.1', + 'host': '{{ redis_host }}', 'port': 6379, 'db': 0, 'password': '{{ redis_passwd }}', diff --git a/docker-compose.yml b/docker-compose.yml new file mode 100644 index 000000000..e6e4af424 --- /dev/null +++ b/docker-compose.yml @@ -0,0 +1,60 @@ +# Decompoe the stack into microservices running in Docker, orchestrated with Docker Compose. +# Use cases: + +# People with MacBooks with M1 chips aren’t able to use VirtualBox right now, so we need an alternative for them. +# The group is bringing up an internal Kubernetes cluster. It’d be nice to have multiple staging instances that we can easily bring up so that we can stage multiple branches at the same time. +# This should be more lightweight/faster than a VM. +# The deliverable would be a set of Docker images (Postgres, Redis, one to run the web service, etc.) and a Docker Compose file to run them. +# I’m not exactly sure how Ansible is going to fit into this picture. Maybe Ansible just creates configuration files that get injected into containers. Let me know if you have thoughts on / experience with this.# Docker Compose + +services: + db: + image: postgres:15 + ports: + - 5432:5432 + volumes: + - db_data:/var/lib/postgresql/data + environment: + POSTGRES_DB: cf_brc_db + POSTGRES_USER: admin + POSTGRES_PASSWORD: root + healthcheck: + test: ["CMD", "pg_isready", "-U", "admin"] + interval: 10s + timeout: 5s + retries: 5 + restart: unless-stopped + + redis: + image: redis:7 + ports: + - 6379:6379 + volumes: + - redis_data:/data + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 10s + timeout: 5s + retries: 5 + restart: unless-stopped + + coldfront: + image: coldfront:latest + build: + context: . + dockerfile: Dockerfile + ports: + - 8880:8000 + depends_on: + db: + condition: service_healthy + redis: + condition: service_healthy + restart: unless-stopped + # network_mode: host + +volumes: + db_data: + external: false + redis_data: + external: false From 1c50483469dddf2904f75f6d1585bf8868859b6e Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Tue, 28 Mar 2023 00:54:57 -0500 Subject: [PATCH 20/72] cleaned up dockerfile --- Dockerfile | 8 -------- 1 file changed, 8 deletions(-) diff --git a/Dockerfile b/Dockerfile index 041b6079c..806617d67 100644 --- a/Dockerfile +++ b/Dockerfile @@ -2,16 +2,9 @@ FROM centos/python-38-centos7 LABEL description="coldfront" - -# install dependencies -# RUN yum -y install epel-release -# RUN yum -y update -# RUN yum -y install python36 python36-devel git memcached redis - USER root WORKDIR /root -# install coldfront RUN mkdir -p /vagrant/coldfront_app WORKDIR /vagrant/coldfront_app/ RUN git clone -b issue_521 https://github.com/ucb-rit/coldfront.git @@ -42,7 +35,6 @@ RUN mkdir -p /var/log/user_portals/cf_mybrc \ && chmod 664 /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ && chmod +x ./manage.py - CMD ./manage.py initial_setup \ && ./manage.py runserver From 015c42dabad3d806ef521309502b5d3891140976 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Tue, 28 Mar 2023 16:54:36 -0500 Subject: [PATCH 21/72] more changes for docker --- Dockerfile | 64 ++++++++----------- coldfront/config/local_settings.py.sample | 5 ++ docker/Dockerfile | 43 +++++++++++++ .../docker-compose.yml | 9 +-- 4 files changed, 81 insertions(+), 40 deletions(-) create mode 100644 docker/Dockerfile rename docker-compose.yml => docker/docker-compose.yml (91%) diff --git a/Dockerfile b/Dockerfile index 806617d67..b56ea76b9 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,42 +1,34 @@ -FROM centos/python-38-centos7 +FROM centos:8 LABEL description="coldfront" -USER root +# install dependencies +RUN yum -y install epel-release +RUN yum -y update +RUN yum -y install python36 python36-devel git memcached redis + WORKDIR /root -RUN mkdir -p /vagrant/coldfront_app -WORKDIR /vagrant/coldfront_app/ -RUN git clone -b issue_521 https://github.com/ucb-rit/coldfront.git -WORKDIR ./coldfront -COPY main.yml ./main.yml - -RUN pip3 install wheel jinja2-cli pyyaml \ - && pip3 install -r requirements.txt - -RUN cp coldfront/config/local_settings.py.sample \ - coldfront/config/local_settings.py \ - && cp coldfront/config/local_strings.py.sample \ - coldfront/config/local_strings.py \ - && python -c \ -"from jinja2 import Template, Environment, FileSystemLoader; \ -import yaml; \ -env = Environment(loader=FileSystemLoader('bootstrap/ansible/')); \ -env.filters['bool'] = lambda x: str(x).lower() in ['true', 'yes', 'on', '1']; \ -options = yaml.safe_load(open('main.yml').read()); \ -options.update({'redis_host': 'redis', 'db_host': 'db', \ - 'debug_toolbar_ips':['host.docker.internal']}); \ -print(env.get_template('settings_template.tmpl').render(options))" \ - > coldfront/config/dev_settings.py - -RUN mkdir -p /var/log/user_portals/cf_mybrc \ - && touch /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ - && chmod 775 /var/log/user_portals/cf_mybrc \ - && chmod 664 /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ - && chmod +x ./manage.py - -CMD ./manage.py initial_setup \ - && ./manage.py runserver - -EXPOSE 8080 +# install coldfront +RUN mkdir /opt/coldfront_app + +WORKDIR /opt/coldfront_app + +RUN cd /opt/coldfront_app +RUN git clone https://github.com/ubccr/coldfront.git +RUN python3.6 -mvenv venv +RUN source venv/bin/activate + +WORKDIR /opt/coldfront_app/coldfront + +RUN cd /opt/coldfront_app/coldfront +RUN pip3 install wheel +RUN pip3 install -r requirements.txt +RUN cp coldfront/config/local_settings.py.sample coldfront/config/local_settings.py +RUN cp coldfront/config/local_strings.py.sample coldfront/config/local_strings.py + +RUN python3 ./manage.py initial_setup +RUN python3 ./manage.py load_test_data + +EXPOSE 8000 STOPSIGNAL SIGINT diff --git a/coldfront/config/local_settings.py.sample b/coldfront/config/local_settings.py.sample index d5164bebb..d4af5a466 100644 --- a/coldfront/config/local_settings.py.sample +++ b/coldfront/config/local_settings.py.sample @@ -385,6 +385,11 @@ INTERNAL_IPS = [ '127.0.0.1' ] +if DEBUG: + import socket + hostname, _, ips = socket.gethostbyname_ex(socket.gethostname()) + INTERNAL_IPS = [ip[: ip.rfind('.')] + '.1' for ip in ips] + ['10.0.2.2'] + # The number of hours for which a newly created authentication token will be # valid. TOKEN_EXPIRATION_HOURS = 24 diff --git a/docker/Dockerfile b/docker/Dockerfile new file mode 100644 index 000000000..45019a954 --- /dev/null +++ b/docker/Dockerfile @@ -0,0 +1,43 @@ +FROM centos/python-38-centos7 + +LABEL description="coldfront" + +USER root +WORKDIR /root + +RUN mkdir -p /vagrant/coldfront_app +WORKDIR /vagrant/coldfront_app/ +RUN git clone -b issue_521 https://github.com/ucb-rit/coldfront.git +WORKDIR ./coldfront +COPY main.yml ./main.yml + +RUN pip3 install wheel jinja2-cli pyyaml \ + && pip3 install -r requirements.txt + +RUN cp coldfront/config/local_settings.py.sample \ + coldfront/config/local_settings.py \ + && cp coldfront/config/local_strings.py.sample \ + coldfront/config/local_strings.py \ + && python -c \ +"from jinja2 import Template, Environment, FileSystemLoader; \ +import yaml; \ +env = Environment(loader=FileSystemLoader('bootstrap/ansible/')); \ +env.filters['bool'] = lambda x: str(x).lower() in ['true', 'yes', 'on', '1']; \ +options = yaml.safe_load(open('main.yml').read()); \ +options.update({'redis_host': 'redis', 'db_host': 'db', \ + 'debug_toolbar_ips': \ + [type(str('c'), (), {'__contains__': lambda *a: True})()]}); \ +print(env.get_template('settings_template.tmpl').render(options))" \ + > coldfront/config/dev_settings.py + +RUN mkdir -p /var/log/user_portals/cf_mybrc \ + && touch /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ + && chmod 775 /var/log/user_portals/cf_mybrc \ + && chmod 664 /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ + && chmod +x ./manage.py + +CMD ./manage.py initial_setup \ + && ./manage.py runserver 0.0.0.0:80 + +EXPOSE 80 +STOPSIGNAL SIGINT diff --git a/docker-compose.yml b/docker/docker-compose.yml similarity index 91% rename from docker-compose.yml rename to docker/docker-compose.yml index e6e4af424..b1afefd80 100644 --- a/docker-compose.yml +++ b/docker/docker-compose.yml @@ -19,7 +19,7 @@ services: POSTGRES_USER: admin POSTGRES_PASSWORD: root healthcheck: - test: ["CMD", "pg_isready", "-U", "admin"] + test: ["CMD", "pg_isready", "-U", "admin", "-d", "cf_brc_db"] interval: 10s timeout: 5s retries: 5 @@ -44,17 +44,18 @@ services: context: . dockerfile: Dockerfile ports: - - 8880:8000 + - 8880:80 + extra_hosts: + - "host.docker.internal:host-gateway" depends_on: db: condition: service_healthy redis: condition: service_healthy restart: unless-stopped - # network_mode: host volumes: db_data: external: false redis_data: - external: false + external: false \ No newline at end of file From 6bcc066fb295aa11412dbe7dc623641271eefc71 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Tue, 28 Mar 2023 17:04:06 -0500 Subject: [PATCH 22/72] removed redundant setting --- docker/Dockerfile | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docker/Dockerfile b/docker/Dockerfile index 45019a954..660ef0a25 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -24,9 +24,7 @@ import yaml; \ env = Environment(loader=FileSystemLoader('bootstrap/ansible/')); \ env.filters['bool'] = lambda x: str(x).lower() in ['true', 'yes', 'on', '1']; \ options = yaml.safe_load(open('main.yml').read()); \ -options.update({'redis_host': 'redis', 'db_host': 'db', \ - 'debug_toolbar_ips': \ - [type(str('c'), (), {'__contains__': lambda *a: True})()]}); \ +options.update({'redis_host': 'redis', 'db_host': 'db'}); \ print(env.get_template('settings_template.tmpl').render(options))" \ > coldfront/config/dev_settings.py From 6f50b6e76d0c65fa2641f4ac70f804fcaae510e0 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Tue, 28 Mar 2023 17:06:04 -0500 Subject: [PATCH 23/72] newline at end of file --- docker/docker-compose.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index b1afefd80..817d823a6 100644 --- a/docker/docker-compose.yml +++ b/docker/docker-compose.yml @@ -58,4 +58,5 @@ volumes: db_data: external: false redis_data: - external: false \ No newline at end of file + external: false + From 48148acfe8a0f12b2c4f3f28738c153853f296f6 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Wed, 29 Mar 2023 17:03:22 -0500 Subject: [PATCH 24/72] finished pt. 1 --- Dockerfile | 72 +++++++++++-------- .../docker-compose.yml => docker-compose.yml | 2 + docker/Dockerfile | 41 ----------- 3 files changed, 45 insertions(+), 70 deletions(-) rename docker/docker-compose.yml => docker-compose.yml (96%) delete mode 100644 docker/Dockerfile diff --git a/Dockerfile b/Dockerfile index b56ea76b9..ff9355e1b 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,34 +1,48 @@ -FROM centos:8 +FROM centos/python-38-centos7 LABEL description="coldfront" -# install dependencies -RUN yum -y install epel-release -RUN yum -y update -RUN yum -y install python36 python36-devel git memcached redis - +USER root WORKDIR /root - -# install coldfront -RUN mkdir /opt/coldfront_app - -WORKDIR /opt/coldfront_app - -RUN cd /opt/coldfront_app -RUN git clone https://github.com/ubccr/coldfront.git -RUN python3.6 -mvenv venv -RUN source venv/bin/activate - -WORKDIR /opt/coldfront_app/coldfront - -RUN cd /opt/coldfront_app/coldfront -RUN pip3 install wheel -RUN pip3 install -r requirements.txt -RUN cp coldfront/config/local_settings.py.sample coldfront/config/local_settings.py -RUN cp coldfront/config/local_strings.py.sample coldfront/config/local_strings.py - -RUN python3 ./manage.py initial_setup -RUN python3 ./manage.py load_test_data - -EXPOSE 8000 +COPY requirements.txt ./ +RUN pip install -r requirements.txt \ + && pip install jinja2 pyyaml && rm requirements.txt + +RUN mkdir -p /vagrant/coldfront_app +WORKDIR /vagrant/coldfront_app/ +RUN git clone -b issue_521 https://github.com/ucb-rit/coldfront.git +WORKDIR ./coldfront +COPY main.yml ./main.yml + +RUN mkdir -p /var/log/user_portals/cf_mybrc \ + && touch /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ + && chmod 775 /var/log/user_portals/cf_mybrc \ + && chmod 664 /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ + && chmod +x ./manage.py + +RUN cp coldfront/config/local_settings.py.sample \ + coldfront/config/local_settings.py \ + && cp coldfront/config/local_strings.py.sample \ + coldfront/config/local_strings.py \ + && python -c \ +"from jinja2 import Template, Environment, FileSystemLoader; \ +import yaml; \ +env = Environment(loader=FileSystemLoader('bootstrap/ansible/')); \ +env.filters['bool'] = lambda x: str(x).lower() in ['true', 'yes', 'on', '1']; \ +options = yaml.safe_load(open('main.yml').read()); \ +options.update({'redis_host': 'redis', 'db_host': 'db'}); \ +print(env.get_template('settings_template.tmpl').render(options))" \ + > coldfront/config/dev_settings.py + +CMD ./manage.py initial_setup \ + && ./manage.py migrate \ + && ./manage.py add_accounting_defaults \ + && ./manage.py add_allowance_defaults \ + && ./manage.py add_directory_defaults \ + && ./manage.py create_allocation_periods \ + && ./manage.py create_staff_group \ + && ./manage.py collectstatic \ + && ./manage.py runserver 0.0.0.0:80 + +EXPOSE 80 STOPSIGNAL SIGINT diff --git a/docker/docker-compose.yml b/docker-compose.yml similarity index 96% rename from docker/docker-compose.yml rename to docker-compose.yml index 817d823a6..bf6450e72 100644 --- a/docker/docker-compose.yml +++ b/docker-compose.yml @@ -45,6 +45,8 @@ services: dockerfile: Dockerfile ports: - 8880:80 + volumes: + - ./coldfront:/vagrant/coldfront_app/coldfront/coldfront extra_hosts: - "host.docker.internal:host-gateway" depends_on: diff --git a/docker/Dockerfile b/docker/Dockerfile deleted file mode 100644 index 660ef0a25..000000000 --- a/docker/Dockerfile +++ /dev/null @@ -1,41 +0,0 @@ -FROM centos/python-38-centos7 - -LABEL description="coldfront" - -USER root -WORKDIR /root - -RUN mkdir -p /vagrant/coldfront_app -WORKDIR /vagrant/coldfront_app/ -RUN git clone -b issue_521 https://github.com/ucb-rit/coldfront.git -WORKDIR ./coldfront -COPY main.yml ./main.yml - -RUN pip3 install wheel jinja2-cli pyyaml \ - && pip3 install -r requirements.txt - -RUN cp coldfront/config/local_settings.py.sample \ - coldfront/config/local_settings.py \ - && cp coldfront/config/local_strings.py.sample \ - coldfront/config/local_strings.py \ - && python -c \ -"from jinja2 import Template, Environment, FileSystemLoader; \ -import yaml; \ -env = Environment(loader=FileSystemLoader('bootstrap/ansible/')); \ -env.filters['bool'] = lambda x: str(x).lower() in ['true', 'yes', 'on', '1']; \ -options = yaml.safe_load(open('main.yml').read()); \ -options.update({'redis_host': 'redis', 'db_host': 'db'}); \ -print(env.get_template('settings_template.tmpl').render(options))" \ - > coldfront/config/dev_settings.py - -RUN mkdir -p /var/log/user_portals/cf_mybrc \ - && touch /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ - && chmod 775 /var/log/user_portals/cf_mybrc \ - && chmod 664 /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ - && chmod +x ./manage.py - -CMD ./manage.py initial_setup \ - && ./manage.py runserver 0.0.0.0:80 - -EXPOSE 80 -STOPSIGNAL SIGINT From 392c0cbc9c8527f6a2d3db0fa565e52baf91b2b1 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Wed, 29 Mar 2023 17:15:08 -0500 Subject: [PATCH 25/72] using mounts instead of images --- Dockerfile | 21 ++------------------- docker-compose.yml | 2 +- gen_config.py | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 20 deletions(-) create mode 100644 gen_config.py diff --git a/Dockerfile b/Dockerfile index ff9355e1b..69587dc49 100644 --- a/Dockerfile +++ b/Dockerfile @@ -8,11 +8,8 @@ COPY requirements.txt ./ RUN pip install -r requirements.txt \ && pip install jinja2 pyyaml && rm requirements.txt -RUN mkdir -p /vagrant/coldfront_app -WORKDIR /vagrant/coldfront_app/ -RUN git clone -b issue_521 https://github.com/ucb-rit/coldfront.git -WORKDIR ./coldfront -COPY main.yml ./main.yml +COPY . /vagrant/coldfront_app/coldfront/ +WORKDIR /vagrant/coldfront_app/coldfront/ RUN mkdir -p /var/log/user_portals/cf_mybrc \ && touch /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ @@ -20,20 +17,6 @@ RUN mkdir -p /var/log/user_portals/cf_mybrc \ && chmod 664 /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ && chmod +x ./manage.py -RUN cp coldfront/config/local_settings.py.sample \ - coldfront/config/local_settings.py \ - && cp coldfront/config/local_strings.py.sample \ - coldfront/config/local_strings.py \ - && python -c \ -"from jinja2 import Template, Environment, FileSystemLoader; \ -import yaml; \ -env = Environment(loader=FileSystemLoader('bootstrap/ansible/')); \ -env.filters['bool'] = lambda x: str(x).lower() in ['true', 'yes', 'on', '1']; \ -options = yaml.safe_load(open('main.yml').read()); \ -options.update({'redis_host': 'redis', 'db_host': 'db'}); \ -print(env.get_template('settings_template.tmpl').render(options))" \ - > coldfront/config/dev_settings.py - CMD ./manage.py initial_setup \ && ./manage.py migrate \ && ./manage.py add_accounting_defaults \ diff --git a/docker-compose.yml b/docker-compose.yml index bf6450e72..fe6b83adc 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -46,7 +46,7 @@ services: ports: - 8880:80 volumes: - - ./coldfront:/vagrant/coldfront_app/coldfront/coldfront + - ./:/vagrant/coldfront_app/coldfront/ extra_hosts: - "host.docker.internal:host-gateway" depends_on: diff --git a/gen_config.py b/gen_config.py new file mode 100644 index 000000000..7382379ed --- /dev/null +++ b/gen_config.py @@ -0,0 +1,14 @@ +#!/usr/bin/env bash +cp coldfront/config/local_settings.py.sample \ + coldfront/config/local_settings.py +cp coldfront/config/local_strings.py.sample \ + coldfront/config/local_strings.py +python -c \ +"from jinja2 import Template, Environment, FileSystemLoader; \ +import yaml; \ +env = Environment(loader=FileSystemLoader('bootstrap/ansible/')); \ +env.filters['bool'] = lambda x: str(x).lower() in ['true', 'yes', 'on', '1']; \ +options = yaml.safe_load(open('main.yml').read()); \ +options.update({'redis_host': 'redis', 'db_host': 'db'}); \ +print(env.get_template('settings_template.tmpl').render(options))" \ + > coldfront/config/dev_settings.py From 7a639bac66fc7261e3bf22006d347d9a9be66726 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Wed, 29 Mar 2023 17:20:47 -0500 Subject: [PATCH 26/72] --noinput for collectstatic --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index 69587dc49..d0a9c4185 100644 --- a/Dockerfile +++ b/Dockerfile @@ -24,7 +24,7 @@ CMD ./manage.py initial_setup \ && ./manage.py add_directory_defaults \ && ./manage.py create_allocation_periods \ && ./manage.py create_staff_group \ - && ./manage.py collectstatic \ + && ./manage.py collectstatic --noinput \ && ./manage.py runserver 0.0.0.0:80 EXPOSE 80 From 65cc3123f4af47efb57b044bbd7978742beb64a9 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 31 Mar 2023 11:58:16 -0700 Subject: [PATCH 27/72] Correct bug in flag consistency check --- bootstrap/ansible/settings_template.tmpl | 3 ++- coldfront/templates/email/login/login_link.txt | 14 ++++++++++++++ .../email/login/login_link_ineligible.txt | 12 ++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 coldfront/templates/email/login/login_link.txt create mode 100644 coldfront/templates/email/login/login_link_ineligible.txt diff --git a/bootstrap/ansible/settings_template.tmpl b/bootstrap/ansible/settings_template.tmpl index 4f1557231..1d4798366 100644 --- a/bootstrap/ansible/settings_template.tmpl +++ b/bootstrap/ansible/settings_template.tmpl @@ -179,7 +179,8 @@ if not ( FLAGS['SSO_ENABLED'][0]['value']): raise ImproperlyConfigured( 'Exactly one of BASIC_AUTH_ENABLED, SSO_ENABLED should be enabled.') -if not FLAGS['SSO_ENABLED'][0]['value'] and FLAGS['LINK_LOGIN_ENABLED']: +if (not FLAGS['SSO_ENABLED'][0]['value'] and + FLAGS['LINK_LOGIN_ENABLED'][0]['value']): raise ImproperlyConfigured( 'LINK_LOGIN_ENABLED should only be enabled when SSO_ENABLED is ' 'enabled.') diff --git a/coldfront/templates/email/login/login_link.txt b/coldfront/templates/email/login/login_link.txt new file mode 100644 index 000000000..f42781534 --- /dev/null +++ b/coldfront/templates/email/login/login_link.txt @@ -0,0 +1,14 @@ +Dear {{ PORTAL_NAME }} user, + +You requested a link to log in to the portal. + +Below is the link, which will expire in {{ login_link_max_age_minutes }} minutes. + +{{ login_url }} + +Do not share this link with anyone else. + +Reminder: If your institution is listed in CILogon, you should use it to log in. + +Thank you, +{{ signature }} \ No newline at end of file diff --git a/coldfront/templates/email/login/login_link_ineligible.txt b/coldfront/templates/email/login/login_link_ineligible.txt new file mode 100644 index 000000000..7908590eb --- /dev/null +++ b/coldfront/templates/email/login/login_link_ineligible.txt @@ -0,0 +1,12 @@ +Dear {{ PORTAL_NAME }} user, + +You requested a link to log in to the portal. + +You are ineligible to receive a link for the following reason: + +{{ reason }} + +Please contact a system administrator if you have any questions. + +Thank you, +{{ signature }} \ No newline at end of file From 517ef7dca58a78c3bb7a9566ef1a9f2000394ae0 Mon Sep 17 00:00:00 2001 From: Viraat Chandra Date: Mon, 3 Apr 2023 01:33:06 -0700 Subject: [PATCH 28/72] address comments --- .../savio/project_request_surveys_modal.html | 58 ++++++++++--------- coldfront/core/project/views.py | 25 +++++--- 2 files changed, 47 insertions(+), 36 deletions(-) diff --git a/coldfront/core/project/templates/project/project_request/savio/project_request_surveys_modal.html b/coldfront/core/project/templates/project/project_request/savio/project_request_surveys_modal.html index 55f78cd36..66d020416 100644 --- a/coldfront/core/project/templates/project/project_request/savio/project_request_surveys_modal.html +++ b/coldfront/core/project/templates/project/project_request/savio/project_request_surveys_modal.html @@ -20,11 +20,13 @@
    @@ -33,31 +35,33 @@
    {% for allocation_request, survey_form in survey_answers %} - {% if forloop.counter == 1 %} -
    -
    -
    -

    Requester: {{ allocation_request.requester }}

    -

    Principal Investigator: {{ allocation_request.pi }}

    -

    Requested: {{ allocation_request.request_time|date:"M. d, Y"}}

    -
    -
    +
    +
    +
    +

    + Requester: + {{ allocation_request.requester.first_name }} + {{ allocation_request.requester.last_name }} + ({{ allocation_request.requester.username }}) +

    - {{ survey_form|crispy }} -
    - {% else %} -
    -
    -
    -

    Requester: {{ allocation_request.requester }}

    -

    Principal Investigator: {{ allocation_request.pi }}

    -

    Requested: {{ allocation_request.request_time|date:"M. d, Y"}}

    -
    -
    +

    + Principal Investigator: + {{ allocation_request.pi.first_name }} + {{ allocation_request.pi.last_name }} + ({{ allocation_request.pi.username }}) +

    - {{ survey_form|crispy }} +

    + Requested: + {{ allocation_request.request_time|date:"M. d, Y"}} +

    +
    - {% endif %} + + {{ survey_form|crispy }} +
    {% endfor %}
    diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py index 91da52edc..468c29e8b 100644 --- a/coldfront/core/project/views.py +++ b/coldfront/core/project/views.py @@ -272,15 +272,22 @@ def get_context_data(self, **kwargs): pi_eligible_to_request_secure_dir(self.request.user) # show survey responses if available - allocation_request = SavioProjectAllocationRequest.objects.filter( - project=self.object).first() - if allocation_request: - context['survey_answers'] = SavioProjectSurveyForm( - initial=allocation_request.survey_answers, disable_fields=True) - - allocation_requests = SavioProjectAllocationRequest.objects.filter(project=self.object) - survey_answers_list = list(map(lambda x: SavioProjectSurveyForm(initial=x.survey_answers, disable_fields=True), allocation_requests)) - context['survey_answers'] = list(zip(allocation_requests, survey_answers_list)) + allocation_requests = SavioProjectAllocationRequest.objects.filter( + project=self.object, status__name='Approved - Complete').order_by('request_time') + + if allocation_requests.exists(): + survey_answers_list = list(map( + lambda x: SavioProjectSurveyForm( + initial=x.survey_answers, + disable_fields=True + ), + allocation_requests + )) + + context['survey_answers'] = list(zip( + allocation_requests, + survey_answers_list + )) context['user_agreement_signed'] = \ access_agreement_signed(self.request.user) From d28feb79ca28e5ec18198a201e07739218bcd8ca Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Mon, 3 Apr 2023 09:33:04 -0700 Subject: [PATCH 29/72] moved around commands --- Dockerfile | 11 ++++++----- gen_config.py => bootstrap/development/gen_config.sh | 0 2 files changed, 6 insertions(+), 5 deletions(-) rename gen_config.py => bootstrap/development/gen_config.sh (100%) diff --git a/Dockerfile b/Dockerfile index d0a9c4185..ab13a4223 100644 --- a/Dockerfile +++ b/Dockerfile @@ -8,14 +8,15 @@ COPY requirements.txt ./ RUN pip install -r requirements.txt \ && pip install jinja2 pyyaml && rm requirements.txt -COPY . /vagrant/coldfront_app/coldfront/ -WORKDIR /vagrant/coldfront_app/coldfront/ - RUN mkdir -p /var/log/user_portals/cf_mybrc \ && touch /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ && chmod 775 /var/log/user_portals/cf_mybrc \ - && chmod 664 /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ - && chmod +x ./manage.py + && chmod 664 /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log + +COPY . /vagrant/coldfront_app/coldfront/ +WORKDIR /vagrant/coldfront_app/coldfront/ + +RUN chmod +x ./manage.py CMD ./manage.py initial_setup \ && ./manage.py migrate \ diff --git a/gen_config.py b/bootstrap/development/gen_config.sh similarity index 100% rename from gen_config.py rename to bootstrap/development/gen_config.sh From 3a5be9caeeb1b3092439f2ecb999d572c82a237e Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 5 Apr 2023 11:29:17 -0700 Subject: [PATCH 30/72] Add class to send verification email to confirm login activity --- .../core/account/utils/login_activity.py | 95 +++++++++++++++++++ coldfront/core/user/utils.py | 4 +- coldfront/core/utils/common.py | 12 ++- .../email/login/verify_login_activity.txt | 18 ++++ coldfront/templates/email/login_link.txt | 14 --- .../templates/email/login_link_ineligible.txt | 12 --- requirements.txt | 1 + 7 files changed, 125 insertions(+), 31 deletions(-) create mode 100644 coldfront/core/account/utils/login_activity.py create mode 100644 coldfront/templates/email/login/verify_login_activity.txt delete mode 100644 coldfront/templates/email/login_link.txt delete mode 100644 coldfront/templates/email/login_link_ineligible.txt diff --git a/coldfront/core/account/utils/login_activity.py b/coldfront/core/account/utils/login_activity.py new file mode 100644 index 000000000..6fbf880a3 --- /dev/null +++ b/coldfront/core/account/utils/login_activity.py @@ -0,0 +1,95 @@ +import pytz + +from django.conf import settings +from django.http.request import HttpRequest +from django.urls import reverse + +from allauth.account.models import EmailAddress +from allauth.account.models import EmailConfirmationHMAC +from user_agents import parse + +from coldfront.core.utils.common import build_absolute_url +from coldfront.core.utils.common import import_from_settings +from coldfront.core.utils.common import utc_now_offset_aware +from coldfront.core.utils.mail import send_email_template + + +class LoginActivityVerifier(object): + """A class that can email an unverified EmailAddress, notifying the + user that a blocked login attempt was made using it and asking the + user to confirm that the attempt was made by them, which will verify + the address.""" + + def __init__(self, request, email_address, request_login_method_str): + """Validate arguments. + + Parameters + - request (HttpRequest): the request object for the login + - email_address (EmailAddress): an unverified EmailAddress + instance + - request_login_method_str (str): A str describing the + method that was used to attempt to log in (e.g., + 'CILogon - Lawrence Berkeley National Laboratory' or + 'Login Link Request') + """ + assert isinstance(request, HttpRequest) + assert isinstance(email_address, EmailAddress) + assert not email_address.verified + assert isinstance(request_login_method_str, str) + + self._request = request + self._email_address = email_address + self._request_time_str = self.get_display_timezone_now_str() + self._request_user_agent_str = self.get_request_user_agent_str() + self._request_login_method_str = request_login_method_str + + @staticmethod + def get_display_timezone_now_str(): + """Return a time string for the current time in the display time + zone (e.g., 'January 1, 1970 at 12:00 PM (UTC)').""" + display_time_zone = pytz.timezone(settings.DISPLAY_TIME_ZONE) + utc_now = utc_now_offset_aware() + display_format = '%B %-d, %Y at %-I:%M %p (%Z)' + return display_time_zone.normalize(utc_now).strftime(display_format) + + def get_request_user_agent_str(self): + """Given an HTTP request, return a str describing the user agent + that made the request (e.g., 'Chrome on Mac OS X').""" + user_agent = parse(self._request.headers.get('user-agent')) + return f'{user_agent.browser.family} on {user_agent.os.family}' + + def login_activity_verification_url(self): + """Return an absolute URL to the view for verifying the + EmailAddress. + + This is adapted from allauth.account.adapter. + DefaultAccountAdapter.get_email_confirmation_url.""" + hmac = EmailConfirmationHMAC(self._email_address) + return build_absolute_url( + reverse('account_confirm_email', args=[hmac.key])) + + def send_email(self): + """Email the user, asking them to confirm the login attempt by + clicking on a link, which will verify the address.""" + email_enabled = import_from_settings('EMAIL_ENABLED', False) + if not email_enabled: + return + + subject = 'Verify Login Activity' + template_name = 'email/login/verify_login_activity.txt' + context = { + 'PORTAL_NAME': settings.PORTAL_NAME, + 'email_address': self._email_address.email, + 'request_time_str': self._request_time_str, + 'request_user_agent_str': self._request_user_agent_str, + 'request_login_method_str': self._request_login_method_str, + 'support_email': settings.CENTER_HELP_EMAIL, + 'verification_url': self.login_activity_verification_url(), + 'signature': import_from_settings('EMAIL_SIGNATURE', ''), + } + + sender = settings.EMAIL_SENDER + receiver_list = [self._email_address.email] + + send_email_template( + subject, template_name, context, sender, receiver_list) diff --git a/coldfront/core/user/utils.py b/coldfront/core/user/utils.py index 2f3484de5..f205a5987 100644 --- a/coldfront/core/user/utils.py +++ b/coldfront/core/user/utils.py @@ -302,7 +302,7 @@ def send_login_link_email(email_address): return subject = 'Login Link' - template_name = 'email/login_link.txt' + template_name = 'email/login/login_link.txt' context = { 'PORTAL_NAME': settings.PORTAL_NAME, 'login_url': login_token_url(email_address.user), @@ -325,7 +325,7 @@ def send_login_link_ineligible_email(email_address, reason): return subject = 'Ineligible for Login Link' - template_name = 'email/login_link_ineligible.txt' + template_name = 'email/login/login_link_ineligible.txt' context = { 'PORTAL_NAME': settings.PORTAL_NAME, 'reason': reason, diff --git a/coldfront/core/utils/common.py b/coldfront/core/utils/common.py index df0e06eb4..41a52b90a 100644 --- a/coldfront/core/utils/common.py +++ b/coldfront/core/utils/common.py @@ -40,9 +40,8 @@ def get_domain_url(request): def project_detail_url(project): - domain = import_from_settings('CENTER_BASE_URL') - view = reverse('project-detail', kwargs={'pk': project.pk}) - return urljoin(domain, view) + return build_absolute_url( + reverse('project-detail', kwargs={'pk': project.pk})) class Echo: @@ -86,6 +85,13 @@ def assert_obj_type(obj, expected_obj_type, null_allowed=False): assert isinstance(obj, expected_obj_type), message +def build_absolute_url(path): + """Return the given URL path (e.g., "/users/1/") with the domain + (CENTER_BASE_URL) prepended.""" + domain = import_from_settings('CENTER_BASE_URL') + return urljoin(domain, path) + + def delete_scheduled_tasks(func, *args): """Delete scheduled tasks (Schedule objects) for running the given function with the given arguments. Write a message to the log.""" diff --git a/coldfront/templates/email/login/verify_login_activity.txt b/coldfront/templates/email/login/verify_login_activity.txt new file mode 100644 index 000000000..d5011f4c5 --- /dev/null +++ b/coldfront/templates/email/login/verify_login_activity.txt @@ -0,0 +1,18 @@ +Dear {{ PORTAL_NAME }} user, + +You are receiving this email because someone attempted to log in to your {{ PORTAL_NAME }} account using this email address ({{ email_address }}). + +This login attempt was blocked because this email address has not been verified. + +Below are some details about the login attempt: + +When: {{ request_time_str }} +What: {{ request_user_agent_str }} +How: {{ request_login_method_str }} + +If this was you, please confirm by clicking the link below. Doing so will verify this email address, allowing you to log in to the portal using it. If not, please contact us at {{ support_email }}. + +{{ verification_url }} + +Thank you, +{{ signature }} \ No newline at end of file diff --git a/coldfront/templates/email/login_link.txt b/coldfront/templates/email/login_link.txt deleted file mode 100644 index f42781534..000000000 --- a/coldfront/templates/email/login_link.txt +++ /dev/null @@ -1,14 +0,0 @@ -Dear {{ PORTAL_NAME }} user, - -You requested a link to log in to the portal. - -Below is the link, which will expire in {{ login_link_max_age_minutes }} minutes. - -{{ login_url }} - -Do not share this link with anyone else. - -Reminder: If your institution is listed in CILogon, you should use it to log in. - -Thank you, -{{ signature }} \ No newline at end of file diff --git a/coldfront/templates/email/login_link_ineligible.txt b/coldfront/templates/email/login_link_ineligible.txt deleted file mode 100644 index 7908590eb..000000000 --- a/coldfront/templates/email/login_link_ineligible.txt +++ /dev/null @@ -1,12 +0,0 @@ -Dear {{ PORTAL_NAME }} user, - -You requested a link to log in to the portal. - -You are ineligible to receive a link for the following reason: - -{{ reason }} - -Please contact a system administrator if you have any questions. - -Thank you, -{{ signature }} \ No newline at end of file diff --git a/requirements.txt b/requirements.txt index 047252814..a7570005c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -53,4 +53,5 @@ sqlparse==0.3.0 text-unidecode==1.3 tqdm==4.62.3 urllib3==1.24.2 +user-agents==2.2.0 wcwidth==0.1.7 From e78a2dc23aad0890b22663c69481b0e6e66b55b1 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 5 Apr 2023 11:32:17 -0700 Subject: [PATCH 31/72] Incorporate login activity verification in SSO and link-login flows --- coldfront/core/socialaccount/adapter.py | 94 ++++++++++++------- .../core/user/utils_/link_login_utils.py | 57 +++++++++++ .../core/user/views_/link_login_views.py | 56 ++++++----- 3 files changed, 151 insertions(+), 56 deletions(-) create mode 100644 coldfront/core/user/utils_/link_login_utils.py diff --git a/coldfront/core/socialaccount/adapter.py b/coldfront/core/socialaccount/adapter.py index ce9f9d3eb..e8a438a7a 100644 --- a/coldfront/core/socialaccount/adapter.py +++ b/coldfront/core/socialaccount/adapter.py @@ -1,10 +1,11 @@ from allauth.account.models import EmailAddress -from allauth.account.utils import user_email +from allauth.account.utils import user_email as user_email_func from allauth.account.utils import user_field from allauth.account.utils import user_username from allauth.exceptions import ImmediateHttpResponse from allauth.socialaccount.adapter import DefaultSocialAccountAdapter from allauth.utils import valid_email_or_none +from coldfront.core.account.utils.login_activity import LoginActivityVerifier from collections import defaultdict from django.conf import settings from django.http import HttpResponseBadRequest @@ -45,12 +46,12 @@ def populate_user(self, request, sociallogin, data): f'Provider {provider} did not provide an email address ' f'for User with UID {user_uid}.') logger.error(log_message) - self.raise_server_error(self.get_auth_error_message()) + self._raise_server_error(self._get_auth_error_message()) validated_email = validated_email.lower() user = sociallogin.user user_username(user, validated_email) - user_email(user, validated_email) + user_email_func(user, validated_email) user_field(user, 'first_name', first_name) user_field(user, 'last_name', last_name) return user @@ -98,7 +99,7 @@ def pre_social_login(self, request, sociallogin): f'Provider {provider} did not provide any email addresses for ' f'User with email {user_email} and UID {user_uid}.') logger.error(log_message) - self.raise_server_error(self.get_auth_error_message()) + self._raise_server_error(self._get_auth_error_message()) # SOCIALACCOUNT_PROVIDERS['cilogon']['VERIFIED_EMAIL'] should be True, # so all provider-given addresses should be interpreted as verified. @@ -110,7 +111,7 @@ def pre_social_login(self, request, sociallogin): f'None of the email addresses in ' f'[{", ".join(provider_addresses)}] are verified.') logger.error(log_message) - self.raise_server_error(self.get_auth_error_message()) + self._raise_server_error(self._get_auth_error_message()) # Fetch EmailAddresses matching those given by the provider, divided by # the associated User. @@ -129,44 +130,73 @@ def pre_social_login(self, request, sociallogin): if not matching_addresses_by_user: return elif len(matching_addresses_by_user) == 1: - # If at least one address was verified, connect the two accounts. - # Otherwise, raise an error with instructions on how to proceed. user = next(iter(matching_addresses_by_user)) addresses = matching_addresses_by_user[user] if any([a.verified for a in addresses]): - log_message = ( - f'Attempting to connect data for User with email ' - f'{user_email} and UID {user_uid} from provider ' - f'{provider} to local User {user.pk}.') - logger.info(log_message) - sociallogin.connect(request, user) - log_message = f'Successfully connected data to User {user.pk}.' - logger.info(log_message) + self._activate_and_connect_user( + request, sociallogin, provider, user, user_email, user_uid) else: - log_message = ( - f'Found only unverified email addresses associated with ' - f'local User {user.pk} matching those given by provider ' - f'{provider} for User with email {user_email} and UID ' - f'{user_uid}.') - logger.warning(log_message) - message = ( - 'You are attempting to login using an email address ' - 'associated with an existing User, but it is unverified. ' - 'Please login to that account using a different provider, ' - 'verify the email address, and try again.') - self.raise_client_error(message) + self._block_login_for_verification( + request, sociallogin, provider, user, user_email, user_uid, + addresses) else: - user_pks = [user.pk for user in matching_addresses_by_user] + user_pks = sorted([user.pk for user in matching_addresses_by_user]) log_message = ( f'Unexpectedly found multiple Users ([{", ".join(user_pks)}]) ' f'that had email addresses matching those provided by ' f'provider {provider} for User with email {user_email} and ' f'UID {user_uid}.') logger.error(log_message) - self.raise_server_error(self.get_auth_error_message()) + self._raise_server_error(self._get_auth_error_message()) + + @staticmethod + def _activate_and_connect_user(request, sociallogin, provider, user, + user_email, user_uid): + """Activate the User if needed and connect the provider account + to the User's account in the database.""" + if not user.is_active: + user.is_active = True + user.save() + log_message = f'Activated User {user.pk}.' + logger.info(log_message) + + sociallogin.connect(request, user) + log_message = ( + f'Successfully connected data for User with email {user_email} and ' + f'UID {user_uid} from provider {provider} to local User {user.pk}.') + logger.info(log_message) + + def _block_login_for_verification(self, request, sociallogin, provider, + user, user_email, user_uid, + email_addresses): + """Block the login attempt and send verification emails to the + given EmailAddresses.""" + log_message = ( + f'Found only unverified email addresses associated with local User ' + f'{user.pk} matching those given by provider {provider} for User ' + f'with email {user_email} and UID {user_uid}.') + logger.warning(log_message) + + try: + cilogon_idp = sociallogin.serialize()[ + 'account']['extra_data']['idp_name'] + request_login_method_str = f'CILogon - {cilogon_idp}' + except Exception as e: + logger.exception(f'Failed to determine CILogon IDP. Details:\n{e}') + request_login_method_str = 'CILogon' + for email_address in email_addresses: + verifier = LoginActivityVerifier( + request, email_address, request_login_method_str) + verifier.send_email() + + message = ( + 'You are attempting to login using an email address associated ' + 'with an existing user, but it is unverified. Please check the ' + 'address for a verification email.') + self._raise_client_error(message) @staticmethod - def get_auth_error_message(): + def _get_auth_error_message(): """Return the generic message the user should receive if authentication-related errors occur.""" return ( @@ -174,7 +204,7 @@ def get_auth_error_message(): f'{settings.CENTER_HELP_EMAIL} for further assistance.') @staticmethod - def raise_client_error(message): + def _raise_client_error(message): """Raise an ImmediateHttpResponse with a client error and the given message.""" template = 'error_with_message.html' @@ -183,7 +213,7 @@ def raise_client_error(message): raise ImmediateHttpResponse(response) @staticmethod - def raise_server_error(message): + def _raise_server_error(message): """Raise an ImmediateHttpResponse with a server error and the given message.""" template = 'error_with_message.html' diff --git a/coldfront/core/user/utils_/link_login_utils.py b/coldfront/core/user/utils_/link_login_utils.py new file mode 100644 index 000000000..7bb1f786e --- /dev/null +++ b/coldfront/core/user/utils_/link_login_utils.py @@ -0,0 +1,57 @@ +from django.conf import settings +from django.urls import reverse + +from sesame.utils import get_query_string + +from coldfront.core.utils.common import build_absolute_url +from coldfront.core.utils.common import import_from_settings +from coldfront.core.utils.mail import send_email_template + + +def login_token_url(user): + """Return a Django Sesame login link for the given User.""" + return build_absolute_url(reverse('link-login') + get_query_string(user)) + + +def send_login_link_email(email_address): + """Send an email containing a login link to the given + EmailAddress.""" + email_enabled = import_from_settings('EMAIL_ENABLED', False) + if not email_enabled: + return + + subject = 'Login Link' + template_name = 'email/login/login_link.txt' + context = { + 'PORTAL_NAME': settings.PORTAL_NAME, + 'login_url': login_token_url(email_address.user), + 'login_link_max_age_minutes': ( + import_from_settings('SESAME_MAX_AGE') // 60), + 'signature': import_from_settings('EMAIL_SIGNATURE', ''), + } + + sender = import_from_settings('EMAIL_SENDER') + receiver_list = [email_address.email, ] + + send_email_template(subject, template_name, context, sender, receiver_list) + + +def send_login_link_ineligible_email(email_address, reason): + """Send an email containing a reason explaining why the User with + the given EmailAddress is ineligible to receive a login link.""" + email_enabled = import_from_settings('EMAIL_ENABLED', False) + if not email_enabled: + return + + subject = 'Ineligible for Login Link' + template_name = 'email/login/login_link_ineligible.txt' + context = { + 'PORTAL_NAME': settings.PORTAL_NAME, + 'reason': reason, + 'signature': import_from_settings('EMAIL_SIGNATURE', ''), + } + + sender = import_from_settings('EMAIL_SENDER') + receiver_list = [email_address.email, ] + + send_email_template(subject, template_name, context, sender, receiver_list) diff --git a/coldfront/core/user/views_/link_login_views.py b/coldfront/core/user/views_/link_login_views.py index c070ab823..94788467c 100644 --- a/coldfront/core/user/views_/link_login_views.py +++ b/coldfront/core/user/views_/link_login_views.py @@ -8,9 +8,10 @@ from allauth.account.models import EmailAddress from sesame.views import LoginView +from coldfront.core.account.utils.login_activity import LoginActivityVerifier from coldfront.core.user.forms_.link_login_forms import RequestLoginLinkForm -from coldfront.core.user.utils import send_login_link_email -from coldfront.core.user.utils import send_login_link_ineligible_email +from coldfront.core.user.utils_.link_login_utils import send_login_link_email +from coldfront.core.user.utils_.link_login_utils import send_login_link_ineligible_email from coldfront.core.utils.common import import_from_settings @@ -33,41 +34,52 @@ def dispatch(self, request, *args, **kwargs): return super().dispatch(request, *args, **kwargs) def form_valid(self, form): - """If the email address belongs to a user, send a login link to - it.""" + """If a corresponding EmailAddress exists, send an email (with a + login link, further instructions, or an explanation) to it. + + In all cases, display an acknowledging message with the same + text, to avoid leaking information.""" email = form.cleaned_data.get('email') email_address = self._validate_email_address(email) if email_address: - try: - self._validate_user_eligible(email_address.user) - except self.UserIneligibleException as e: - reason = str(e) - send_login_link_ineligible_email(email_address, reason) + if not email_address.verified: + request_login_method_str = 'Link Login Request' + verifier = LoginActivityVerifier( + self.request, email_address, request_login_method_str) + verifier.send_email() else: - send_login_link_email(email_address) - self._send_success_message() + try: + self._validate_user_eligible(email_address.user) + except self.UserIneligibleException as e: + reason = str(e) + send_login_link_ineligible_email(email_address, reason) + else: + send_login_link_email(email_address) + self._send_ack_message() return super().form_valid(form) - def get_success_url(self): + @staticmethod + def get_success_url(): return reverse('request-login-link') - def _send_success_message(self): - """Send a success message to the user explaining that a link was - (conditionally) sent.""" + def _send_ack_message(self): + """Send an acknowledging message to the user explaining that a + link or further instructions were (conditionally) sent.""" login_link_max_age_minutes = ( import_from_settings('SESAME_MAX_AGE') // 60) message = ( f'If the email address you entered corresponds to an existing ' - f'user, please check the address for a login link. Note that this ' - f'link will expire in {login_link_max_age_minutes} minutes.') + f'user, please check the address for a login link or further ' + f'instructions. Note that this link will expire in ' + f'{login_link_max_age_minutes} minutes.') messages.success(self.request, message) def _validate_email_address(self, email): """Return an EmailAddress object corresponding to the given - address (str) if one exists and is verified. Otherwise, return - None. Write user and log messages as needed.""" + address (str) if one exists. Otherwise, return None. Write user + and log messages as needed.""" try: - email_address = EmailAddress.objects.get(email=email) + return EmailAddress.objects.get(email=email) except EmailAddress.DoesNotExist: return None except EmailAddress.MultipleObjectsReturned: @@ -78,10 +90,6 @@ def _validate_email_address(self, email): 'Unexpected server error. Please contact an administrator.') messages.error(self.request, message) return None - else: - if not email_address.verified: - return None - return email_address def _validate_user_eligible(self, user): """Return None if the given User is eligible to log in using From 2722a25efabbfb8af5e1c088d876ddf571944069 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 5 Apr 2023 11:40:13 -0700 Subject: [PATCH 32/72] Add log message about user activation from link login --- coldfront/core/user/views_/link_login_views.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coldfront/core/user/views_/link_login_views.py b/coldfront/core/user/views_/link_login_views.py index 94788467c..8653d75e5 100644 --- a/coldfront/core/user/views_/link_login_views.py +++ b/coldfront/core/user/views_/link_login_views.py @@ -121,6 +121,8 @@ def login_success(self): if not user.is_active: user.is_active = True user.save() + log_message = f'Activated User {user.pk}.' + logger.info(log_message) message = f'Successfully signed in as {user.username}.' messages.success(self.request, message) From 51d6f92fffbf19f0a0fe5ba665a20fa85cb887d2 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 7 Apr 2023 14:25:47 -0700 Subject: [PATCH 33/72] Create EmailAddress for newly-created PI user during new project request so it exists when PI attempts to log in --- .../views_/new_project_views/request_views.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/coldfront/core/project/views_/new_project_views/request_views.py b/coldfront/core/project/views_/new_project_views/request_views.py index d38fdd7bd..aa7019c8f 100644 --- a/coldfront/core/project/views_/new_project_views/request_views.py +++ b/coldfront/core/project/views_/new_project_views/request_views.py @@ -1,3 +1,5 @@ +from allauth.account.models import EmailAddress + from coldfront.core.allocation.models import Allocation from coldfront.core.allocation.models import AllocationStatusChoice from coldfront.core.billing.forms import BillingIDValidationForm @@ -465,8 +467,8 @@ def __handle_pi_data(self, form_data): # Create a new User object intended to be a new PI. step_number = self.step_numbers_by_form_name['new_pi'] data = form_data[step_number] + email = data['email'] try: - email = data['email'] pi = User.objects.create( username=email, first_name=data['first_name'], @@ -488,6 +490,18 @@ def __handle_pi_data(self, form_data): pi_profile.upgrade_request = utc_now_offset_aware() pi_profile.save() + # Create an unverified, primary EmailAddress for the new User object. + try: + EmailAddress.objects.create( + user=pi, + email=email, + verified=False, + primary=True) + except IntegrityError as e: + self.logger.error( + f'EmailAddress {email} unexpectedly already exists.') + raise e + return pi def __handle_recharge_allowance(self, form_data, From 3f308f6442f2658742d6a4c18c8d9c1a18c5b2d9 Mon Sep 17 00:00:00 2001 From: Viraat Chandra Date: Sun, 9 Apr 2023 23:10:34 -0700 Subject: [PATCH 34/72] address comments and fix bugs in table sorting --- .../allocation_cluster_account_request_list_table.html | 6 +++--- .../templates/project/project_join_request_list_table.html | 3 +++ .../project_renewal/project_renewal_request_list_table.html | 4 ++-- .../project_request/savio/project_request_list_table.html | 2 +- .../project_request/vector/project_request_list_table.html | 2 +- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/coldfront/core/allocation/templates/allocation/allocation_cluster_account_request_list_table.html b/coldfront/core/allocation/templates/allocation/allocation_cluster_account_request_list_table.html index e8c315971..5328157d7 100644 --- a/coldfront/core/allocation/templates/allocation/allocation_cluster_account_request_list_table.html +++ b/coldfront/core/allocation/templates/allocation/allocation_cluster_account_request_list_table.html @@ -17,15 +17,15 @@ User Email - {% include 'common/table_sorter.html' with table_sorter_field='project_user__user__email' %} + {% include 'common/table_sorter.html' with table_sorter_field='allocation_user__user__email' %} Cluster Username - {% include 'common/table_sorter.html' with table_sorter_field='project_user__user__username' %} + {% include 'common/table_sorter.html' with table_sorter_field='allocation_user__user__username' %} Project - {% include 'common/table_sorter.html' with table_sorter_field='project_user__project__name' %} + {% include 'common/table_sorter.html' with table_sorter_field='allocation_user__allocation__project__name' %} Allocation diff --git a/coldfront/core/project/templates/project/project_join_request_list_table.html b/coldfront/core/project/templates/project/project_join_request_list_table.html index e5e806bc2..28293a9b3 100644 --- a/coldfront/core/project/templates/project/project_join_request_list_table.html +++ b/coldfront/core/project/templates/project/project_join_request_list_table.html @@ -6,12 +6,15 @@ Username + {% include 'common/table_sorter.html' with table_sorter_field='project_user__user__username' %} User Email + {% include 'common/table_sorter.html' with table_sorter_field='project_user__user__email' %} Project + {% include 'common/table_sorter.html' with table_sorter_field='project_user__project' %} Date Requested diff --git a/coldfront/core/project/templates/project/project_renewal/project_renewal_request_list_table.html b/coldfront/core/project/templates/project/project_renewal/project_renewal_request_list_table.html index f31a1d9fc..7045cf82e 100644 --- a/coldfront/core/project/templates/project/project_renewal/project_renewal_request_list_table.html +++ b/coldfront/core/project/templates/project/project_renewal/project_renewal_request_list_table.html @@ -15,11 +15,11 @@ Project - {% include 'common/table_sorter.html' with table_sorter_field='project' %} + {% include 'common/table_sorter.html' with table_sorter_field='post_project__name' %} PI - {% include 'common/table_sorter.html' with table_sorter_field='pi' %} + {% include 'common/table_sorter.html' with table_sorter_field='pi__email' %} Status diff --git a/coldfront/core/project/templates/project/project_request/savio/project_request_list_table.html b/coldfront/core/project/templates/project/project_request/savio/project_request_list_table.html index bc87e515f..771816cec 100644 --- a/coldfront/core/project/templates/project/project_request/savio/project_request_list_table.html +++ b/coldfront/core/project/templates/project/project_request/savio/project_request_list_table.html @@ -19,7 +19,7 @@ PI - {% include 'common/table_sorter.html' with table_sorter_field='pi' %} + {% include 'common/table_sorter.html' with table_sorter_field='pi__email' %} Status diff --git a/coldfront/core/project/templates/project/project_request/vector/project_request_list_table.html b/coldfront/core/project/templates/project/project_request/vector/project_request_list_table.html index 0525a5ef7..37cf6ca8a 100644 --- a/coldfront/core/project/templates/project/project_request/vector/project_request_list_table.html +++ b/coldfront/core/project/templates/project/project_request/vector/project_request_list_table.html @@ -19,7 +19,7 @@ PI - {% include 'common/table_sorter.html' with table_sorter_field='pi' %} + {% include 'common/table_sorter.html' with table_sorter_field='pi__email' %} Status From 35b6f5581ccfad1f8c16dc3107072c37fc2fec49 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 10 Apr 2023 08:48:16 -0700 Subject: [PATCH 35/72] Display default text when request user agent unknown --- coldfront/core/account/utils/login_activity.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/coldfront/core/account/utils/login_activity.py b/coldfront/core/account/utils/login_activity.py index 6fbf880a3..c33c376a1 100644 --- a/coldfront/core/account/utils/login_activity.py +++ b/coldfront/core/account/utils/login_activity.py @@ -55,7 +55,10 @@ def get_display_timezone_now_str(): def get_request_user_agent_str(self): """Given an HTTP request, return a str describing the user agent that made the request (e.g., 'Chrome on Mac OS X').""" - user_agent = parse(self._request.headers.get('user-agent')) + user_agent_header = self._request.headers.get('user-agent', '') + if not user_agent_header: + return 'Unknown Browser and OS' + user_agent = parse(user_agent_header) return f'{user_agent.browser.family} on {user_agent.os.family}' def login_activity_verification_url(self): From 406845f81b8c3bf335316e8045dcfe66b18d4c5d Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 10 Apr 2023 08:53:26 -0700 Subject: [PATCH 36/72] Refactor login link eligibility logic for use in custom auth backend --- coldfront/core/user/auth.py | 23 ++++++++++- .../core/user/utils_/link_login_utils.py | 19 +++++++++ .../core/user/views_/link_login_views.py | 39 ++++++++----------- 3 files changed, 57 insertions(+), 24 deletions(-) diff --git a/coldfront/core/user/auth.py b/coldfront/core/user/auth.py index f5a79e8d3..8616118d9 100644 --- a/coldfront/core/user/auth.py +++ b/coldfront/core/user/auth.py @@ -2,18 +2,23 @@ from django.contrib.auth.models import User from allauth.account.models import EmailAddress +from sesame.backends import ModelBackend as BaseSesameBackend from coldfront.core.user.utils import send_email_verification_email +from coldfront.core.user.utils_.link_login_utils import UserLoginLinkIneligible +from coldfront.core.user.utils_.link_login_utils import validate_user_eligible_for_login_link import logging +logger = logging.getLogger(__name__) + + class EmailAddressBackend(BaseBackend): """An authentication backend that allows a user to authenticate using any of their verified EmailAddress objects.""" def authenticate(self, request, username=None, password=None, **kwargs): - logger = logging.getLogger(__name__) if username is None: return None username = username.lower() @@ -45,3 +50,19 @@ def get_user(self, user_id): return User.objects.get(id=user_id) except User.DoesNotExist: return None + + +class SesameBackend(BaseSesameBackend): + """A subclass of django-sesame's ModelBackend that limits who is + eligible to log in using tokens.""" + + def user_can_authenticate(self, user): + try: + validate_user_eligible_for_login_link(user) + except UserLoginLinkIneligible as e: + message = ( + f'User {user.username} was blocked from Sesame authentication ' + f'because: {str(e)}') + logger.info(message) + return False + return True diff --git a/coldfront/core/user/utils_/link_login_utils.py b/coldfront/core/user/utils_/link_login_utils.py index 7bb1f786e..74c67ff50 100644 --- a/coldfront/core/user/utils_/link_login_utils.py +++ b/coldfront/core/user/utils_/link_login_utils.py @@ -8,6 +8,10 @@ from coldfront.core.utils.mail import send_email_template +class UserLoginLinkIneligible(Exception): + pass + + def login_token_url(user): """Return a Django Sesame login link for the given User.""" return build_absolute_url(reverse('link-login') + get_query_string(user)) @@ -55,3 +59,18 @@ def send_login_link_ineligible_email(email_address, reason): receiver_list = [email_address.email, ] send_email_template(subject, template_name, context, sender, receiver_list) + + +def validate_user_eligible_for_login_link(user): + """Return None if the given User is eligible to log in using a link. + Otherwise, raise an exception with a user-facing message explaining + why the user is ineligible.""" + # Inactive users + if not user.is_active: + raise UserLoginLinkIneligible( + 'Inactive users are disallowed from logging in using a link.') + # Staff users and superusers + if user.is_staff or user.is_superuser: + raise UserLoginLinkIneligible( + 'For security reasons, portal staff are disallowed from logging in ' + 'using a link.') diff --git a/coldfront/core/user/views_/link_login_views.py b/coldfront/core/user/views_/link_login_views.py index 8653d75e5..aa691551d 100644 --- a/coldfront/core/user/views_/link_login_views.py +++ b/coldfront/core/user/views_/link_login_views.py @@ -12,6 +12,8 @@ from coldfront.core.user.forms_.link_login_forms import RequestLoginLinkForm from coldfront.core.user.utils_.link_login_utils import send_login_link_email from coldfront.core.user.utils_.link_login_utils import send_login_link_ineligible_email +from coldfront.core.user.utils_.link_login_utils import UserLoginLinkIneligible +from coldfront.core.user.utils_.link_login_utils import validate_user_eligible_for_login_link from coldfront.core.utils.common import import_from_settings @@ -25,9 +27,6 @@ class RequestLoginLinkView(FormView): form_class = RequestLoginLinkForm template_name = 'user/request_login_link.html' - class UserIneligibleException(Exception): - pass - def dispatch(self, request, *args, **kwargs): if request.user.is_authenticated: return redirect(reverse('home')) @@ -49,8 +48,8 @@ def form_valid(self, form): verifier.send_email() else: try: - self._validate_user_eligible(email_address.user) - except self.UserIneligibleException as e: + validate_user_eligible_for_login_link(email_address.user) + except UserLoginLinkIneligible as e: reason = str(e) send_login_link_ineligible_email(email_address, reason) else: @@ -59,20 +58,24 @@ def form_valid(self, form): return super().form_valid(form) @staticmethod - def get_success_url(): - return reverse('request-login-link') - - def _send_ack_message(self): - """Send an acknowledging message to the user explaining that a - link or further instructions were (conditionally) sent.""" + def ack_message(): + """Return an acknowledging message explaining that a link or + further instructions were (conditionally) sent.""" login_link_max_age_minutes = ( import_from_settings('SESAME_MAX_AGE') // 60) - message = ( + return ( f'If the email address you entered corresponds to an existing ' f'user, please check the address for a login link or further ' f'instructions. Note that this link will expire in ' f'{login_link_max_age_minutes} minutes.') - messages.success(self.request, message) + + @staticmethod + def get_success_url(): + return reverse('request-login-link') + + def _send_ack_message(self): + """Send an acknowledging message to the user.""" + messages.success(self.request, self.ack_message()) def _validate_email_address(self, email): """Return an EmailAddress object corresponding to the given @@ -91,16 +94,6 @@ def _validate_email_address(self, email): messages.error(self.request, message) return None - def _validate_user_eligible(self, user): - """Return None if the given User is eligible to log in using - this method. Otherwise, raise an exception with a user-facing - message explaining why the user is ineligible.""" - # Staff users and superusers - if user.is_staff or user.is_superuser: - raise self.UserIneligibleException( - 'For security reasons, portal staff are disallowed from ' - 'logging in using a link.') - class LinkLoginView(LoginView): """A subclass of Django Sesame's login view, with custom logic.""" From b5083e10c06e4005ee66babd8b370d6d9d4882dd Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 10 Apr 2023 09:07:42 -0700 Subject: [PATCH 37/72] Do not activate users who get through SSO/link login flows so that blocked users stay blocked --- coldfront/core/socialaccount/adapter.py | 30 ++++++++----------- .../core/user/views_/link_login_views.py | 14 ++++----- 2 files changed, 18 insertions(+), 26 deletions(-) diff --git a/coldfront/core/socialaccount/adapter.py b/coldfront/core/socialaccount/adapter.py index e8a438a7a..c1b81817b 100644 --- a/coldfront/core/socialaccount/adapter.py +++ b/coldfront/core/socialaccount/adapter.py @@ -133,7 +133,7 @@ def pre_social_login(self, request, sociallogin): user = next(iter(matching_addresses_by_user)) addresses = matching_addresses_by_user[user] if any([a.verified for a in addresses]): - self._activate_and_connect_user( + self._connect_user( request, sociallogin, provider, user, user_email, user_uid) else: self._block_login_for_verification( @@ -149,23 +149,6 @@ def pre_social_login(self, request, sociallogin): logger.error(log_message) self._raise_server_error(self._get_auth_error_message()) - @staticmethod - def _activate_and_connect_user(request, sociallogin, provider, user, - user_email, user_uid): - """Activate the User if needed and connect the provider account - to the User's account in the database.""" - if not user.is_active: - user.is_active = True - user.save() - log_message = f'Activated User {user.pk}.' - logger.info(log_message) - - sociallogin.connect(request, user) - log_message = ( - f'Successfully connected data for User with email {user_email} and ' - f'UID {user_uid} from provider {provider} to local User {user.pk}.') - logger.info(log_message) - def _block_login_for_verification(self, request, sociallogin, provider, user, user_email, user_uid, email_addresses): @@ -195,6 +178,17 @@ def _block_login_for_verification(self, request, sociallogin, provider, 'address for a verification email.') self._raise_client_error(message) + @staticmethod + def _connect_user(request, sociallogin, provider, user, user_email, + user_uid): + """Connect the provider account to the User's account in the + database.""" + sociallogin.connect(request, user) + log_message = ( + f'Successfully connected data for User with email {user_email} and ' + f'UID {user_uid} from provider {provider} to local User {user.pk}.') + logger.info(log_message) + @staticmethod def _get_auth_error_message(): """Return the generic message the user should receive if diff --git a/coldfront/core/user/views_/link_login_views.py b/coldfront/core/user/views_/link_login_views.py index aa691551d..4e3a518fc 100644 --- a/coldfront/core/user/views_/link_login_views.py +++ b/coldfront/core/user/views_/link_login_views.py @@ -108,15 +108,13 @@ def login_failed(self): return super().login_failed() def login_success(self): - """Activate the user if needed, send a success message to the - user, and write to the log before deferring to parent logic.""" - user = self.request.user - if not user.is_active: - user.is_active = True - user.save() - log_message = f'Activated User {user.pk}.' - logger.info(log_message) + """Send a success message to the user and write to the log + before deferring to parent logic. + Note: Users should not be activated, so that explicitly blocked + users do not have a path to reactivation. + """ + user = self.request.user message = f'Successfully signed in as {user.username}.' messages.success(self.request, message) logger.warning( From 996cb00c92d436d4de3b69f6bb8659d9041a67f4 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 10 Apr 2023 09:12:21 -0700 Subject: [PATCH 38/72] Use custom subclass of django-sesame authentication backend to block ineligible users --- coldfront/config/local_settings.py.sample | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coldfront/config/local_settings.py.sample b/coldfront/config/local_settings.py.sample index 7df9a119e..d574160cb 100644 --- a/coldfront/config/local_settings.py.sample +++ b/coldfront/config/local_settings.py.sample @@ -443,7 +443,7 @@ SOCIALACCOUNT_QUERY_EMAIL = True #------------------------------------------------------------------------------ EXTRA_AUTHENTICATION_BACKENDS += [ - 'sesame.backends.ModelBackend', + 'coldfront.core.user.auth.SesameBackend', ] # The number of seconds a login token is valid for. From 8481281578dbdeffa099cbe7ef9fed6859d10745 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 14 Apr 2023 13:05:59 -0700 Subject: [PATCH 39/72] Add command for fixing erroneously-charged free QoS jobs; refactor logic as needed --- coldfront/api/statistics/utils.py | 28 ++- coldfront/api/statistics/views.py | 89 +------ .../core/statistics/management/__init__.py | 0 .../management/commands/__init__.py | 0 .../management/commands/free_qos_jobs.py | 238 ++++++++++++++++++ .../statistics/utils_/accounting_utils.py | 154 ++++++++++++ 6 files changed, 414 insertions(+), 95 deletions(-) create mode 100644 coldfront/core/statistics/management/__init__.py create mode 100644 coldfront/core/statistics/management/commands/__init__.py create mode 100644 coldfront/core/statistics/management/commands/free_qos_jobs.py create mode 100644 coldfront/core/statistics/utils_/accounting_utils.py diff --git a/coldfront/api/statistics/utils.py b/coldfront/api/statistics/utils.py index 878ad9363..07568fc47 100644 --- a/coldfront/api/statistics/utils.py +++ b/coldfront/api/statistics/utils.py @@ -225,14 +225,26 @@ def get_accounting_allocation_objects(project, user=None, if not isinstance(user, User): raise TypeError(f'User {user} is not a User object.') - # Check that there is an active association between the user and project. - active_status = ProjectUserStatusChoice.objects.get(name='Active') - ProjectUser.objects.get(user=user, project=project, status=active_status) - - # Check that the user is an active member of the allocation. - active_status = AllocationUserStatusChoice.objects.get(name='Active') - allocation_user = AllocationUser.objects.get( - allocation=allocation, user=user, status=active_status) + project_user_kwargs = { + 'user': user, + 'project': project, + } + if enforce_allocation_active: + # Check that there is an active association between the user and + # project. + project_user_kwargs['status'] = ProjectUserStatusChoice.objects.get( + name='Active') + ProjectUser.objects.get(**project_user_kwargs) + + allocation_user_kwargs = { + 'allocation': allocation, + 'user': user, + } + if enforce_allocation_active: + # Check that the user is an active member of the allocation. + allocation_user_kwargs['status'] = \ + AllocationUserStatusChoice.objects.get(name='Active') + allocation_user = AllocationUser.objects.get(**allocation_user_kwargs) # Check that the allocation user has an attribute for Service Units # and an associated usage. diff --git a/coldfront/api/statistics/views.py b/coldfront/api/statistics/views.py index 9981bd579..506ee60ff 100644 --- a/coldfront/api/statistics/views.py +++ b/coldfront/api/statistics/views.py @@ -2,7 +2,6 @@ import pytz from collections import OrderedDict -from datetime import date from datetime import datetime from datetime import timedelta from decimal import Decimal, InvalidOperation @@ -36,6 +35,7 @@ from coldfront.core.resource.utils_.allowance_utils.computing_allowance import ComputingAllowance from coldfront.core.resource.utils_.allowance_utils.interface import ComputingAllowanceInterface from coldfront.core.statistics.models import Job +from coldfront.core.statistics.utils_.accounting_utils import validate_job_dates from coldfront.core.user.models import UserProfile from coldfront.core.utils.common import display_time_zone_date_to_utc_datetime @@ -263,7 +263,7 @@ def create(self, request, *args, **kwargs): logger.warning(f'Job {jobslurmid} has no amount.') try: - job_dates_valid = self.validate_job_dates( + job_dates_valid = validate_job_dates( serializer.validated_data, allocation_objects.allocation, end_date_expected=False) except Exception as e: @@ -441,91 +441,6 @@ def update(self, request, *args, **kwargs): return Response(serializer.data) - @staticmethod - def validate_job_dates(job_data, allocation, end_date_expected=False): - """Given a dictionary representing a Job, its corresponding - Allocation, and whether the Job is expected to include an end - date, return whether: - (a) The Job has the expected dates, - (b) The Job's corresponding Allocation has the expected - dates, and - (c) The Job started and ended within the Allocation's dates. - - Write errors or warnings to the log if not.""" - logger = logging.getLogger(__name__) - - jobslurmid = job_data['jobslurmid'] - account_name = job_data['accountid'].name - - # The Job should have submit, start, and, if applicable, end dates. - expected_date_keys = ['submitdate', 'startdate'] - if end_date_expected: - expected_date_keys.append('enddate') - expected_dates = { - key: job_data.get(key, None) for key in expected_date_keys} - for key, expected_date in expected_dates.items(): - if not isinstance(expected_date, datetime): - logger.error(f'Job {jobslurmid} does not have a {key}.') - return False - - # The Job's corresponding Allocation should have a start date. - allocation_start_date = allocation.start_date - if not isinstance(allocation_start_date, date): - logger.error( - f'Allocation {allocation.pk} (Project {account_name}) does ' - f'not have a start date.') - return False - - # The Job should not have started before its corresponding Allocation's - # start date. - job_start_dt_utc = expected_dates['startdate'] - allocation_start_dt_utc = display_time_zone_date_to_utc_datetime( - allocation_start_date) - if job_start_dt_utc < allocation_start_dt_utc: - logger.warning( - f'Job {jobslurmid} start date ' - f'({job_start_dt_utc.strftime(DATE_FORMAT)}) is before ' - f'Allocation {allocation.pk} (Project {account_name}) start ' - f'date ({allocation_start_dt_utc.strftime(DATE_FORMAT)}).') - return False - - if not end_date_expected: - return True - - # The Job's corresponding Allocation may have an end date. (Compare - # against the maximum date if not.) - computing_allowance_interface = ComputingAllowanceInterface() - periodic_project_name_prefixes = tuple([ - computing_allowance_interface.code_from_name(allowance.name) - for allowance in computing_allowance_interface.allowances() - if ComputingAllowance(allowance).is_periodic()]) - if account_name.startswith(periodic_project_name_prefixes): - allocation_end_date = allocation.end_date - if not isinstance(allocation_end_date, date): - logger.error( - f'Allocation {allocation.pk} (Project {account_name}) ' - f'does not have an end date.') - return False - allocation_end_dt_utc = ( - display_time_zone_date_to_utc_datetime(allocation_end_date) + - timedelta(hours=24) - - timedelta(microseconds=1)) - else: - allocation_end_dt_utc = datetime.max.replace(tzinfo=pytz.utc) - - # The Job should not have ended after the last microsecond of its - # corresponding Allocation's end date. - job_end_dt_utc = expected_dates['enddate'] - if job_end_dt_utc > allocation_end_dt_utc: - logger.warning( - f'Job {jobslurmid} end date ' - f'({job_end_dt_utc.strftime(DATE_FORMAT)}) is after ' - f'Allocation {allocation.pk} (Project {account_name}) end ' - f'date ({allocation_end_dt_utc.strftime(DATE_FORMAT)}).') - return False - - return True - job_cost_parameter = openapi.Parameter( 'job_cost', diff --git a/coldfront/core/statistics/management/__init__.py b/coldfront/core/statistics/management/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/coldfront/core/statistics/management/commands/__init__.py b/coldfront/core/statistics/management/commands/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/coldfront/core/statistics/management/commands/free_qos_jobs.py b/coldfront/core/statistics/management/commands/free_qos_jobs.py new file mode 100644 index 000000000..ebdcc2967 --- /dev/null +++ b/coldfront/core/statistics/management/commands/free_qos_jobs.py @@ -0,0 +1,238 @@ +import json +import logging + +from decimal import Decimal + +from django.core.management.base import BaseCommand + +from coldfront.core.allocation.utils import get_project_compute_allocation +from coldfront.core.project.models import Project +from coldfront.core.statistics.models import Job +from coldfront.core.statistics.utils_.accounting_utils import set_job_amount +from coldfront.core.statistics.utils_.accounting_utils import validate_job_dates +from coldfront.core.resource.utils_.allowance_utils.computing_allowance import ComputingAllowance +from coldfront.core.resource.utils_.allowance_utils.interface import ComputingAllowanceInterface +from coldfront.core.resource.utils_.allowance_utils.interface import ComputingAllowanceInterfaceError +from coldfront.core.utils.common import add_argparse_dry_run_argument + + +"""An admin command for listing and updating jobs under free QoSes that +have non-zero amounts.""" + + +class Command(BaseCommand): + + help = ( + 'Manage jobs under free QoSes that have non-zero service unit amounts. ' + 'List statistics about them or reset their amounts to zero, updating ' + 'the associated usages.') + + logger = logging.getLogger(__name__) + + def add_arguments(self, parser): + subparsers = parser.add_subparsers( + dest='subcommand', + help='The subcommand to run.', + title='subcommands') + subparsers.required = True + self._add_reset_subparser(subparsers) + self._add_summary_subparser(subparsers) + add_argparse_dry_run_argument(parser) + + def handle(self, *args, **options): + subcommand = options['subcommand'] + if subcommand == 'reset': + self._handle_reset(*args, **options) + elif subcommand == 'summary': + self._handle_summary(*args, **options) + + @staticmethod + def _add_reset_subparser(parsers): + """Add a subparser for the 'reset' subcommand.""" + parser = parsers.add_parser( + 'reset', + help=( + 'Set amounts for relevant jobs to zero, and update associated ' + 'usages.')) + parser.add_argument( + 'qos_names', + help='A space-separated list of free QoS names.', + nargs='+', + type=str) + parser.add_argument( + '--project', + help='The name of a specific project to perform the reset for.', + type=str) + add_argparse_dry_run_argument(parser) + + @staticmethod + def _add_summary_subparser(parsers): + """Add a subparser for the 'summary' subcommand.""" + parser = parsers.add_parser( + 'summary', help='Get a JSON summary of relevant jobs.') + parser.add_argument( + 'qos_names', + help='A space-separated list of free QoS names.', + nargs='+', + type=str) + + def _handle_reset(self, *args, **options): + """Handle the 'reset' subcommand.""" + if options['project'] is not None: + project = Project.objects.get(name=options['project']) + else: + project = None + self._zero_out_free_qos_jobs( + options['qos_names'], project=project, dry_run=options['dry_run']) + + def _handle_summary(self, *args, **options): + """Handle the 'summary' subcommand.""" + output_json = self._summary_json(options['qos_names']) + self.stdout.write(json.dumps(output_json, indent=4, sort_keys=True)) + + @staticmethod + def _summary_json(qos_names): + """Return a dictionary detailing the number of jobs with the + given QoSes that have non-zero amounts, as well as the total + associated usage.""" + zero = Decimal('0.00') + + num_jobs = 0 + total_by_project_id = {} + for job in Job.objects.filter( + qos__in=qos_names, amount__gt=zero).iterator(): + num_jobs += 1 + # Use accountid_id to avoid a foreign key lookup. + project_id = job.accountid_id + if project_id not in total_by_project_id: + total_by_project_id[project_id] = zero + total_by_project_id[project_id] += job.amount + + total_by_project_name = {} + total_by_allowance = {} + for project_id, amount in total_by_project_id.items(): + project = Project.objects.get(id=project_id) + total_by_project_name[project.name] = str(amount) + if '_' in project.name: + allowance_type = project.name.split('_')[0] + else: + allowance_type = project.name + if allowance_type not in total_by_allowance: + total_by_allowance[allowance_type] = zero + total_by_allowance[allowance_type] += amount + + for allowance_name, amount in total_by_allowance.items(): + total_by_allowance[allowance_name] = str(amount) + + return { + 'num_jobs': num_jobs, + 'total_by_allowance': total_by_allowance, + 'total_by_project': total_by_project_name, + } + + def _zero_out_free_qos_jobs(self, qos_names, project=None, dry_run=False): + """For each job with one of the given QoSes, reset the job's + amount to zero, and update the associated usages if + appropriate. Optionally only consider jobs under the given + Project. Optionally display updates instead of performing + them.""" + computing_allowance_interface = ComputingAllowanceInterface() + periodic_project_name_prefixes = tuple([ + computing_allowance_interface.code_from_name(allowance.name) + for allowance in computing_allowance_interface.allowances() + if ComputingAllowance(allowance).is_periodic()]) + + total_by_project_name = {} + project_cache = {} + allocation_cache = {} + + zero = Decimal('0.00') + num_jobs = 0 + kwargs = { + 'qos__in': qos_names, + 'amount__gt': zero, + } + if project is not None: + kwargs['accountid'] = project + for job in Job.objects.filter(**kwargs).iterator(): + num_jobs += 1 + jobslurmid = job.jobslurmid + + project_id = job.accountid_id + if project_id in project_cache: + project = project_cache[project_id] + else: + project = job.accountid + project_cache[project_id] = project + + # Skip updating usages for any job that is outside its allocation's + # allowance period. Some projects don't have meaningful periods; + # avoid expensive lookups for them. + try: + computing_allowance_interface.allowance_from_project(project) + except ComputingAllowanceInterfaceError: + # Non-primary cluster project --> no allowance period + update_usages = True + else: + if project.name.startswith(periodic_project_name_prefixes): + # Has a periodic allowance --> defined allowance period + # Only update usages if the job is within the current + # period. + if project_id in allocation_cache: + allocation = allocation_cache[project_id] + else: + allocation = get_project_compute_allocation(project) + allocation_cache[project_id] = allocation + job_data = job.__dict__ + job_data['accountid'] = project + update_usages = validate_job_dates( + job_data, allocation, end_date_expected=True) + else: + # Does not have a periodic allowance --> no allowance period + update_usages = True + + if project.name not in total_by_project_name: + total_by_project_name[project.name] = { + 'num_jobs': 0, + 'usage': zero, + } + total_by_project_name[project.name]['num_jobs'] += 1 + if update_usages: + total_by_project_name[project.name]['usage'] += job.amount + else: + message = ( + f'Job {jobslurmid} outside of allowance period. Skipping ' + f'usage update.') + self.stdout.write(self.style.WARNING(message)) + + if not dry_run: + try: + set_job_amount( + jobslurmid, zero, update_usages=update_usages) + except Exception as e: + self.logger.exception(e) + message = ( + f'Failed to update amount for Job {jobslurmid}. ' + f'Details:\n{e}') + self.stderr.write(self.style.ERROR(message)) + + for project_name in total_by_project_name: + usage_str = str( + total_by_project_name[project_name]['usage']) + total_by_project_name[project_name]['usage'] = usage_str + result_json = json.dumps( + total_by_project_name, indent=4, sort_keys=True) + + message = ( + f'Corrected amounts for {num_jobs} jobs under free QoSes ' + f'{", ".join(sorted(qos_names))} to zero and associated usages. ' + f'Summary:\n{result_json}') + self.stdout.write(message) + + if not dry_run: + compact_result_json = json.dumps( + total_by_project_name, sort_keys=True) + self.logger.info( + f'Corrected amounts for {num_jobs} jobs under free QoSes ' + f'{", ".join(sorted(qos_names))} to zero and associated ' + f'usages. Summary: {compact_result_json}') diff --git a/coldfront/core/statistics/utils_/accounting_utils.py b/coldfront/core/statistics/utils_/accounting_utils.py new file mode 100644 index 000000000..e09128846 --- /dev/null +++ b/coldfront/core/statistics/utils_/accounting_utils.py @@ -0,0 +1,154 @@ +import logging +import pytz + +from datetime import date +from datetime import datetime +from datetime import timedelta +from decimal import Decimal + +from django.db import transaction + +from coldfront.api.statistics.utils import get_accounting_allocation_objects +from coldfront.core.allocation.models import AllocationAttributeUsage +from coldfront.core.allocation.models import AllocationUserAttributeUsage +from coldfront.core.resource.utils_.allowance_utils.computing_allowance import ComputingAllowance +from coldfront.core.resource.utils_.allowance_utils.interface import ComputingAllowanceInterface +from coldfront.core.statistics.models import Job +from coldfront.core.utils.common import display_time_zone_date_to_utc_datetime + + +def set_job_amount(jobslurmid, amount, update_usages=True): + """Set the number of service units for the Job with the given Slurm + ID to the given amount. Optionally update associated usages. + + Parameters: + - jobslurmid (str) + - amount (Decimal) + + Returns: + - None + + Raises: + + """ + assert isinstance(jobslurmid, str) + assert isinstance(amount, Decimal) + + with transaction.atomic(): + job = Job.objects.select_for_update().get(jobslurmid=jobslurmid) + + if update_usages: + account = job.accountid + user = job.userid + allocation_objects = get_accounting_allocation_objects( + account, user=user) + + account_usage = ( + AllocationAttributeUsage.objects.select_for_update().get( + pk=allocation_objects.allocation_attribute_usage.pk)) + user_account_usage = ( + AllocationUserAttributeUsage.objects.select_for_update().get( + pk=allocation_objects.allocation_user_attribute_usage.pk)) + + difference = amount - job.amount + + new_account_usage = max( + account_usage.value + difference, Decimal('0.00')) + account_usage.value = new_account_usage + account_usage.save() + + new_user_account_usage = max( + user_account_usage.value + difference, Decimal('0.00')) + user_account_usage.value = new_user_account_usage + user_account_usage.save() + + # Do not update the job.amount before calculating the difference. + job.amount = amount + job.save() + + +def validate_job_dates(job_data, allocation, end_date_expected=False): + """Given a dictionary representing a Job, its corresponding + Allocation, and whether the Job is expected to include an end date, + return whether: + (a) The Job has the expected dates, + (b) The Job's corresponding Allocation has the expected dates, + and + (c) The Job started and ended within the Allocation's dates. + + Write errors or warnings to the log if not.""" + logger = logging.getLogger(__name__) + + date_format = '%Y-%m-%d %H:%M:%SZ' + + jobslurmid = job_data['jobslurmid'] + account_name = job_data['accountid'].name + + # The Job should have submit, start, and, if applicable, end dates. + expected_date_keys = ['submitdate', 'startdate'] + if end_date_expected: + expected_date_keys.append('enddate') + expected_dates = { + key: job_data.get(key, None) for key in expected_date_keys} + for key, expected_date in expected_dates.items(): + if not isinstance(expected_date, datetime): + logger.error(f'Job {jobslurmid} does not have a {key}.') + return False + + # The Job's corresponding Allocation should have a start date. + allocation_start_date = allocation.start_date + if not isinstance(allocation_start_date, date): + logger.error( + f'Allocation {allocation.pk} (Project {account_name}) does not ' + f'have a start date.') + return False + + # The Job should not have started before its corresponding Allocation's + # start date. + job_start_dt_utc = expected_dates['startdate'] + allocation_start_dt_utc = display_time_zone_date_to_utc_datetime( + allocation_start_date) + if job_start_dt_utc < allocation_start_dt_utc: + logger.warning( + f'Job {jobslurmid} start date ' + f'({job_start_dt_utc.strftime(date_format)}) is before Allocation ' + f'{allocation.pk} (Project {account_name}) start date ' + f'({allocation_start_dt_utc.strftime(date_format)}).') + return False + + if not end_date_expected: + return True + + # The Job's corresponding Allocation may have an end date. (Compare + # against the maximum date if not.) + computing_allowance_interface = ComputingAllowanceInterface() + periodic_project_name_prefixes = tuple([ + computing_allowance_interface.code_from_name(allowance.name) + for allowance in computing_allowance_interface.allowances() + if ComputingAllowance(allowance).is_periodic()]) + if account_name.startswith(periodic_project_name_prefixes): + allocation_end_date = allocation.end_date + if not isinstance(allocation_end_date, date): + logger.error( + f'Allocation {allocation.pk} (Project {account_name}) does not ' + f'have an end date.') + return False + allocation_end_dt_utc = ( + display_time_zone_date_to_utc_datetime(allocation_end_date) + + timedelta(hours=24) - + timedelta(microseconds=1)) + else: + allocation_end_dt_utc = datetime.max.replace(tzinfo=pytz.utc) + + # The Job should not have ended after the last microsecond of its + # corresponding Allocation's end date. + job_end_dt_utc = expected_dates['enddate'] + if job_end_dt_utc > allocation_end_dt_utc: + logger.warning( + f'Job {jobslurmid} end date ' + f'({job_end_dt_utc.strftime(date_format)}) is after Allocation ' + f'{allocation.pk} (Project {account_name}) end date ' + f'({allocation_end_dt_utc.strftime(date_format)}).') + return False + + return True From 4c3783e90daae520f4ea9d5653a8945833dfbf47 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 14 Apr 2023 13:22:59 -0700 Subject: [PATCH 40/72] Allow the summary to be filtered by project --- .../management/commands/free_qos_jobs.py | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/coldfront/core/statistics/management/commands/free_qos_jobs.py b/coldfront/core/statistics/management/commands/free_qos_jobs.py index ebdcc2967..dc2f0ef18 100644 --- a/coldfront/core/statistics/management/commands/free_qos_jobs.py +++ b/coldfront/core/statistics/management/commands/free_qos_jobs.py @@ -75,6 +75,10 @@ def _add_summary_subparser(parsers): help='A space-separated list of free QoS names.', nargs='+', type=str) + parser.add_argument( + '--project', + help='The name of a specific project to get a summary for.', + type=str) def _handle_reset(self, *args, **options): """Handle the 'reset' subcommand.""" @@ -87,20 +91,30 @@ def _handle_reset(self, *args, **options): def _handle_summary(self, *args, **options): """Handle the 'summary' subcommand.""" - output_json = self._summary_json(options['qos_names']) + if options['project'] is not None: + project = Project.objects.get(name=options['project']) + else: + project = None + output_json = self._summary_json(options['qos_names'], project=project) self.stdout.write(json.dumps(output_json, indent=4, sort_keys=True)) @staticmethod - def _summary_json(qos_names): + def _summary_json(qos_names, project=None): """Return a dictionary detailing the number of jobs with the given QoSes that have non-zero amounts, as well as the total - associated usage.""" + associated usage. Optionally only consider jobs under the given + Project. """ zero = Decimal('0.00') num_jobs = 0 total_by_project_id = {} - for job in Job.objects.filter( - qos__in=qos_names, amount__gt=zero).iterator(): + kwargs = { + 'qos__in': qos_names, + 'amount__gt': zero, + } + if project is not None: + kwargs['accountid'] = project + for job in Job.objects.filter(**kwargs).iterator(): num_jobs += 1 # Use accountid_id to avoid a foreign key lookup. project_id = job.accountid_id From c6f8719ab52fa74e85a885e63d7c81b2ef90fbc1 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 14 Apr 2023 15:50:06 -0700 Subject: [PATCH 41/72] Fix failing tests --- coldfront/api/statistics/tests/test_job_view_set.py | 4 ++-- coldfront/api/statistics/views.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/coldfront/api/statistics/tests/test_job_view_set.py b/coldfront/api/statistics/tests/test_job_view_set.py index 0f85fcfe7..fbf54694a 100644 --- a/coldfront/api/statistics/tests/test_job_view_set.py +++ b/coldfront/api/statistics/tests/test_job_view_set.py @@ -1154,7 +1154,7 @@ def test_post_allocation_missing_dates(self): self.assertFalse( Job.objects.filter(jobslurmid=data['jobslurmid']).exists()) - @patch('coldfront.api.statistics.views.JobViewSet.validate_job_dates') + @patch('coldfront.api.statistics.views.validate_job_dates') def test_post_handles_exception_when_validating_dates(self, mock_method): """Test that a POST (create) request does not fail to create a job even if an exception is raised when determining whether @@ -1396,7 +1396,7 @@ def test_put_allocation_missing_dates(self): self.assertFalse( Job.objects.filter(jobslurmid=data['jobslurmid']).exists()) - @patch('coldfront.api.statistics.views.JobViewSet.validate_job_dates') + @patch('coldfront.api.statistics.views.validate_job_dates') def test_put_handles_exception_when_validating_dates(self, mock_method): """Test that a PUT (update) request does not fail to create a job even if an exception is raised when determining whether diff --git a/coldfront/api/statistics/views.py b/coldfront/api/statistics/views.py index 506ee60ff..abf867a70 100644 --- a/coldfront/api/statistics/views.py +++ b/coldfront/api/statistics/views.py @@ -374,7 +374,7 @@ def update(self, request, *args, **kwargs): logger.warning(f'Job {jobslurmid} has no amount.') try: - job_dates_valid = self.validate_job_dates( + job_dates_valid = validate_job_dates( serializer.validated_data, allocation_objects.allocation, end_date_expected=True) except Exception as e: From 7acce1a277b474e203ceb3a44b3be4c809dd85e0 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Mon, 17 Apr 2023 05:42:14 -0700 Subject: [PATCH 42/72] mylrc support v1 --- Dockerfile | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Dockerfile b/Dockerfile index ab13a4223..d42b56de4 100644 --- a/Dockerfile +++ b/Dockerfile @@ -8,10 +8,12 @@ COPY requirements.txt ./ RUN pip install -r requirements.txt \ && pip install jinja2 pyyaml && rm requirements.txt -RUN mkdir -p /var/log/user_portals/cf_mybrc \ - && touch /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log \ - && chmod 775 /var/log/user_portals/cf_mybrc \ - && chmod 664 /var/log/user_portals/cf_mybrc/cf_mybrc_{portal,api}.log +# mybrc or mylrc +ARG portal="mybrc" +RUN mkdir -p /var/log/user_portals/cf_${portal} \ + && touch /var/log/user_portals/cf_${portal}/cf_${portal}_{portal,api}.log \ + && chmod 775 /var/log/user_portals/cf_${portal} \ + && chmod 664 /var/log/user_portals/cf_${portal}/cf_${portal}_{portal,api}.log COPY . /vagrant/coldfront_app/coldfront/ WORKDIR /vagrant/coldfront_app/coldfront/ From 112aac08896d3a18707a8f1135b5fa38dddeb377 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Mon, 17 Apr 2023 08:08:04 -0700 Subject: [PATCH 43/72] added database loading support --- Dockerfile | 10 ++--- Dockerfile.db | 5 +++ .../docker_load_database_backup.sh | 4 ++ bootstrap/development/load_database_backup.sh | 42 ++++++++++++------- docker-compose.yml | 11 +++-- 5 files changed, 48 insertions(+), 24 deletions(-) create mode 100644 Dockerfile.db create mode 100644 bootstrap/development/docker_load_database_backup.sh diff --git a/Dockerfile b/Dockerfile index d42b56de4..a2a01b973 100644 --- a/Dockerfile +++ b/Dockerfile @@ -5,8 +5,8 @@ LABEL description="coldfront" USER root WORKDIR /root COPY requirements.txt ./ -RUN pip install -r requirements.txt \ - && pip install jinja2 pyyaml && rm requirements.txt +RUN pip install -r requirements.txt && rm requirements.txt +RUN pip install jinja2 pyyaml # mybrc or mylrc ARG portal="mybrc" @@ -15,13 +15,13 @@ RUN mkdir -p /var/log/user_portals/cf_${portal} \ && chmod 775 /var/log/user_portals/cf_${portal} \ && chmod 664 /var/log/user_portals/cf_${portal}/cf_${portal}_{portal,api}.log -COPY . /vagrant/coldfront_app/coldfront/ -WORKDIR /vagrant/coldfront_app/coldfront/ +ARG base_dir="/vagrant/coldfront_app" +COPY . ${base_dir}/coldfront/ +WORKDIR ${base_dir}/coldfront/ RUN chmod +x ./manage.py CMD ./manage.py initial_setup \ - && ./manage.py migrate \ && ./manage.py add_accounting_defaults \ && ./manage.py add_allowance_defaults \ && ./manage.py add_directory_defaults \ diff --git a/Dockerfile.db b/Dockerfile.db new file mode 100644 index 000000000..5af3ba0ab --- /dev/null +++ b/Dockerfile.db @@ -0,0 +1,5 @@ +FROM postgres:15 + +LABEL description="coldfront db" + +RUN apt update && apt install -y sudo postgresql-client \ No newline at end of file diff --git a/bootstrap/development/docker_load_database_backup.sh b/bootstrap/development/docker_load_database_backup.sh new file mode 100644 index 000000000..6ea06111f --- /dev/null +++ b/bootstrap/development/docker_load_database_backup.sh @@ -0,0 +1,4 @@ +#!/bin/bash +# $1 = database name +# $2 = dump file +docker exec -i coldfront-db-1 pg_restore --verbose --clean -U admin -d $1 < $2 diff --git a/bootstrap/development/load_database_backup.sh b/bootstrap/development/load_database_backup.sh index ef1a43dab..4d6199310 100644 --- a/bootstrap/development/load_database_backup.sh +++ b/bootstrap/development/load_database_backup.sh @@ -1,18 +1,27 @@ -#!/bin/sh +#!/bin/bash # This script clears out the PostgreSQL database with the given name and # loads in the data from the given dump file generated by pg_dump. It # skips loading entries from the Job table. # Store second last and last arguments. -DB_NAME=${@:(-2):1} +if [ "$POSTGRES_DB" != "" ]; then + DB_NAME=$POSTGRES_DB + if [ "$LOAD_DUMP" = true ]; then + DUMP_FILE=/db.dump + else + exit 0 + fi +else + DB_NAME=${@:(-2):1} + DUMP_FILE=${@: -1} +fi DB_OWNER=admin -DUMP_FILE=${@: -1} BIN=/usr/pgsql-15/bin/pg_restore if [[ "$1" == "--help" ]] || [[ "$1" == "-h" ]] ; then - echo "Usage: `basename $0` [OPTION] name-of-database absolute-path-to-dump-file" + echo "Usage: sudo -u postgres `basename $0` [OPTION] name-of-database absolute-path-to-dump-file" echo $' -k, --kill-connections\t\tterminates all connections to the database so it can be dropped' exit 0 elif ! [[ "$DUMP_FILE" = /* ]] ; then @@ -24,18 +33,19 @@ elif ! [[ -f "$DUMP_FILE" ]] ; then fi if [[ "$1" == "-k" || "$1" == "--kill-connections" ]] ; then - echo "TERMINATING DATABASE CONNECTIONS..." - sudo -u postgres psql -c "SELECT pg_terminate_backend(pid) + echo "TERMINATING DATABASE CONNECTIONS..." + psql -U $DB_OWNER -c "SELECT pg_terminate_backend(pid) FROM pg_stat_activity - WHERE pid <> pg_backend_pid() AND datname = '$DB_NAME';" + WHERE pid <> pg_backend_pid() AND datname = '$DB_NAME';" $DB_NAME fi -sudo -u postgres /bin/bash -c "echo DROPPING DATABASE... \ - && psql -c 'DROP DATABASE $DB_NAME;'; \ - \ - echo RECREATING DATABASE... \ - && psql -c 'CREATE DATABASE $DB_NAME OWNER $DB_OWNER;' \ - \ - && echo LOADING DATABASE... \ - && $BIN -d $DB_NAME $DUMP_FILE; \ - psql -c 'ALTER SCHEMA public OWNER TO $DB_OWNER;' $DB_NAME;" +echo DROPPING DATABASE... \ +&& psql -U $DB_OWNER -c "DROP DATABASE $DB_NAME;" $DB_NAME + +echo RECREATING DATABASE... \ +&& psql -U $DB_OWNER -c "CREATE DATABASE $DB_NAME OWNER $DB_OWNER;" $DB_NAME + +echo LOADING DATABASE... \ +&& $BIN -d $DB_NAME $DUMP_FILE + +psql -U $DB_OWNER -c "ALTER SCHEMA public OWNER TO $DB_OWNER;" $DB_NAME diff --git a/docker-compose.yml b/docker-compose.yml index fe6b83adc..baa90b4e7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -9,15 +9,19 @@ services: db: - image: postgres:15 + image: coldfront_db:latest + build: + context: . + dockerfile: Dockerfile.db ports: - 5432:5432 - volumes: - - db_data:/var/lib/postgresql/data environment: POSTGRES_DB: cf_brc_db POSTGRES_USER: admin POSTGRES_PASSWORD: root + LOAD_DUMP: true + volumes: + - db_data:/var/lib/postgresql/data healthcheck: test: ["CMD", "pg_isready", "-U", "admin", "-d", "cf_brc_db"] interval: 10s @@ -31,6 +35,7 @@ services: - 6379:6379 volumes: - redis_data:/data + command: --requirepass root healthcheck: test: ["CMD", "redis-cli", "ping"] interval: 10s From c086db4243a44c8f9529a87fc8c78b36a86f049a Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Mon, 17 Apr 2023 08:51:05 -0700 Subject: [PATCH 44/72] initial docker README section --- README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/README.md b/README.md index 3433d8423..cd78d3cc8 100644 --- a/README.md +++ b/README.md @@ -207,6 +207,14 @@ multiple files or directories to omit. - Open `htmlcov/index.html` in a browser to view which lines of code were covered by the tests and which were not. +## Docker - Quick Install (Recommend) +1. Generate configuration (`dev_settings.py`): have Python with the `jinja2` and `pyyaml` libraries installed, and then run `bootstrap/development/gen_config.sh` +2. Build Images: In the base directory, run `docker build . -t coldfront` and `docker build . -f Dockerfile.db -t coldfront_db` +3. To run: In the base directory, run `docker compose up` +4. To enter the coldfront container (similar to `vagrant ssh`): run `docker exec -it coldfront-coldfront-1 bash` +5. To load a database backup: run `bootstrap/development/docker_load_database_backup.sh ${DB_NAME} ${PATH_TO_DUMP}` +6. To start from scratch (delete volumes): In the base directory, run `docker compose down --volumes` + ## Local Machine - Quick Install (Not Recommended) 1. ColdFront requires Python 3.6, memcached, and redis. From 6653d859f2cc8d33673f4fb63819ff613c8f83c3 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Mon, 17 Apr 2023 09:38:02 -0700 Subject: [PATCH 45/72] added environment file --- docker-compose.yml | 2 +- env.sh | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 env.sh diff --git a/docker-compose.yml b/docker-compose.yml index baa90b4e7..1eb6bd5ad 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -49,7 +49,7 @@ services: context: . dockerfile: Dockerfile ports: - - 8880:80 + - ${COLDFRONT_PORT}:80 volumes: - ./:/vagrant/coldfront_app/coldfront/ extra_hosts: diff --git a/env.sh b/env.sh new file mode 100644 index 000000000..b85d8eed0 --- /dev/null +++ b/env.sh @@ -0,0 +1 @@ +export COLDFRONT_PORT=8880 \ No newline at end of file From be24f6b35fa5d1b8ea1d1d3362caffd9ecaa03d5 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Mon, 17 Apr 2023 09:42:08 -0700 Subject: [PATCH 46/72] env.sh -> .env --- env.sh => .env | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename env.sh => .env (100%) diff --git a/env.sh b/.env similarity index 100% rename from env.sh rename to .env From cc5f994be6e0536fbf3559ddb59018ef5709b66f Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 19 Apr 2023 13:37:17 -0700 Subject: [PATCH 47/72] Update SSO template displayed to inactive users; add comments about inactive users to pre_social_login --- coldfront/core/socialaccount/adapter.py | 8 +++++++- coldfront/templates/account/account_inactive.html | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/coldfront/core/socialaccount/adapter.py b/coldfront/core/socialaccount/adapter.py index c1b81817b..58536da37 100644 --- a/coldfront/core/socialaccount/adapter.py +++ b/coldfront/core/socialaccount/adapter.py @@ -66,6 +66,9 @@ def pre_social_login(self, request, sociallogin): EmailAddress matching one of those given by the provider, connect the two accounts. + Note that login is blocked outside (after) this method if the + user is inactive. + Adapted from: https://github.com/pennersr/django-allauth/issues/418#issuecomment-137259550 """ @@ -133,6 +136,9 @@ def pre_social_login(self, request, sociallogin): user = next(iter(matching_addresses_by_user)) addresses = matching_addresses_by_user[user] if any([a.verified for a in addresses]): + # After this, allauth.account.adapter.pre_login blocks login if + # the user is inactive. Regardless of that, connect the user + # (and trigger signals for creating EmailAddresses). self._connect_user( request, sociallogin, provider, user, user_email, user_uid) else: @@ -173,7 +179,7 @@ def _block_login_for_verification(self, request, sociallogin, provider, verifier.send_email() message = ( - 'You are attempting to login using an email address associated ' + 'You are attempting to log in using an email address associated ' 'with an existing user, but it is unverified. Please check the ' 'address for a verification email.') self._raise_client_error(message) diff --git a/coldfront/templates/account/account_inactive.html b/coldfront/templates/account/account_inactive.html index c15e892ec..b5fd33e23 100644 --- a/coldfront/templates/account/account_inactive.html +++ b/coldfront/templates/account/account_inactive.html @@ -13,7 +13,7 @@

    Account Inactive

    {% endblock %} From 9bd07086d6ce78bb753c5244113cfc0df322ec8b Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 21 Apr 2023 10:17:56 -0700 Subject: [PATCH 48/72] Remove redundant utils; add tests for link login views --- .../tests/test_views/test_link_login_views.py | 360 ++++++++++++++++++ coldfront/core/user/utils.py | 52 --- coldfront/core/utils/tests/test_base.py | 7 + 3 files changed, 367 insertions(+), 52 deletions(-) create mode 100644 coldfront/core/user/tests/test_views/test_link_login_views.py diff --git a/coldfront/core/user/tests/test_views/test_link_login_views.py b/coldfront/core/user/tests/test_views/test_link_login_views.py new file mode 100644 index 000000000..481816b8c --- /dev/null +++ b/coldfront/core/user/tests/test_views/test_link_login_views.py @@ -0,0 +1,360 @@ +from copy import deepcopy +from http import HTTPStatus +from urllib.parse import urlparse + +from django.conf import settings +from django.contrib.auth import get_user +from django.contrib.auth.models import User +from django.core import mail +from django.test import override_settings +from django.urls import reverse + +from allauth.account.models import EmailAddress + +from flags.state import disable_flag +from flags.state import enable_flag +from flags.state import flag_enabled + +from sesame import settings as sesame_settings + +from coldfront.core.user.utils_.link_login_utils import login_token_url +from coldfront.core.user.views_.link_login_views import RequestLoginLinkView +from coldfront.core.utils.tests.test_base import TestBase + + +class TestLoginLinkViews(TestBase): + """A class for testing the views for requesting a short-lived login + link and for authenticating using such links.""" + + def setUp(self): + """Set up test data.""" + enable_flag('BASIC_AUTH_ENABLED') + enable_flag('LINK_LOGIN_ENABLED') + + super().setUp() + + self.user = User.objects.create( + email='user@email.com', + first_name='First', + last_name='Last', + username='user', + is_active=True) + self.user.set_password(self.password) + self.user.save() + + self.email_address = EmailAddress.objects.create( + email=self.user.email, + user=self.user, + primary=True, + verified=True) + + self.assertEqual(len(mail.outbox), 0) + + def _assert_ack_message_sent(self, response): + """Assert that a message was sent as part of the given response + noting that an email may have been sent.""" + messages = self.get_message_strings(response) + self.assertTrue(messages) + self.assertIn(RequestLoginLinkView.ack_message(), messages) + + def _assert_bad_link_message_sent(self, response): + """Assert that a message was sent as part of the given response + noting that the provided login link is invalid or expired.""" + messages = self.get_message_strings(response) + self.assertTrue(messages) + self.assertIn('Invalid or expired login link.', messages) + + def _assert_email_confirmed_message_sent(self, response, email): + """Assert that a message was sent as part of the given response + noting that the given email address (str) has been confirmed.""" + messages = self.get_message_strings(response) + self.assertTrue(messages) + self.assertIn(f'You have confirmed {email}.', messages) + + def _assert_signed_in_message_sent(self, response, username): + """Assert that a message was sent as part of the given response + noting that the user with the given username (str) has + successfully been signed in.""" + messages = self.get_message_strings(response) + self.assertTrue(messages) + self.assertIn(f'Successfully signed in as {username}.', messages) + + def _request_login_link(self, email, client=None): + """Make a POST request to the view to request a login link for + the given email.""" + if client is None: + client = self.client + url = self._view_url() + data = {'email': email} + return client.post(url, data, format='json') + + @staticmethod + def _view_url(): + return reverse('request-login-link') + + def test_expected_flags_enabled(self): + """Test that, in this test (and by extension every other test in + the class), the expected authentication-related feature flags + are enabled.""" + self.assertTrue(flag_enabled('BASIC_AUTH_ENABLED')) + self.assertTrue(flag_enabled('LINK_LOGIN_ENABLED')) + + def test_views_inaccessible_if_flag_disabled(self): + """Test that the views are inaccessible if the + LINK_LOGIN_ENABLED flag is disabled.""" + flag_name = 'LINK_LOGIN_ENABLED' + enable_flag(flag_name) + + response = self.client.get(self._view_url()) + self.assertEqual(response.status_code, HTTPStatus.OK) + + response = self.client.get(login_token_url(self.user)) + self.assertRedirects(response, reverse('home')) + client_user = get_user(self.client) + self.assertTrue(client_user.is_authenticated) + self._assert_signed_in_message_sent(response, self.user.username) + + self.client.logout() + + flags_copy = deepcopy(settings.FLAGS) + flags_copy[flag_name] = {'condition': 'boolean', 'value': False} + with override_settings(FLAGS=flags_copy): + disable_flag(flag_name) + self.assertFalse(flag_enabled(flag_name)) + response = self.client.get(self._view_url()) + self.assertEqual(response.status_code, HTTPStatus.NOT_FOUND) + response = self.client.get(login_token_url(self.user)) + self.assertEqual(response.status_code, HTTPStatus.NOT_FOUND) + + def test_authenticated_user_redirected(self): + """Test that an authenticated user attempting to access the view + is redirected.""" + self.client.login(username=self.user.username, password=self.password) + + response = self.client.get(self._view_url()) + self.assertRedirects(response, reverse('home')) + + response = self._request_login_link(self.user.email) + self.assertRedirects(response, reverse('home')) + + def test_nonexistent_email_address(self): + """Test that, when the input email does not correspond to an + entry in the database: + - No email is sent. + - The user receives an acknowledgement. + """ + self.email_address.delete() + + response = self._request_login_link(self.email_address.email) + self.assertEqual(len(mail.outbox), 0) + self._assert_ack_message_sent(response) + + def test_unverified_email_address(self): + """Test that, when the input email exists but is unverified: + - A verification email is sent. + - The user receives an acknowledgement. + + If the user verifies their email and tries again, they should be + authenticated. + """ + self.email_address.verified = False + self.email_address.save() + + response = self._request_login_link(self.email_address.email) + self._assert_ack_message_sent(response) + + # A verification link should is sent to the address. + self.assertEqual(len(mail.outbox), 1) + urls = self.parse_urls_from_str(mail.outbox[0].body) + self.assertTrue(urls) + verification_url = urls[0] + + # The user clicks on the link, verifying the address. + self.client.post(verification_url) + self.email_address.refresh_from_db() + self.assertTrue(self.email_address.verified) + + # The user tries again. + response = self._request_login_link(self.email_address.email) + self._assert_ack_message_sent(response) + + # A login link is sent to the address. + self.assertEqual(len(mail.outbox), 2) + urls = self.parse_urls_from_str(mail.outbox[1].body) + self.assertTrue(urls) + login_url = urls[0] + + # The user, who is active, clicks on the link, logging them in. + self.assertTrue(self.user.is_active) + response = self.client.get(login_url) + self.assertRedirects(response, reverse('home')) + client_user = get_user(self.client) + self.assertTrue(client_user.is_authenticated) + self._assert_email_confirmed_message_sent( + response, self.email_address.email) + self._assert_signed_in_message_sent(response, self.user.username) + + def test_verified_email_address_user_ineligible(self): + """Test that when the input email exists and is verified, but + the user is ineligible to login using a link: + - An explanatory email is sent. + - The user receives an acknowledgement. + """ + self.user.is_staff = False + self.user.is_superuser = False + self.user.save() + + user_fields = ( + ('is_staff', True), + ('is_superuser', True), + ('is_active', False), + ) + expected_reason_strs = ( + 'portal staff are disallowed', + 'portal staff are disallowed', + 'Inactive users are disallowed', + ) + for i, user_field in enumerate(user_fields): + user_field_key, user_field_value = user_field + setattr(self.user, user_field_key, user_field_value) + self.user.save() + + response = self._request_login_link(self.email_address.email) + self._assert_ack_message_sent(response) + + # An email explaining why the user is ineligible is sent to the + # address. + self.assertEqual(len(mail.outbox), i + 1) + body = mail.outbox[i].body + self.assertIn('ineligible to receive a link', body) + self.assertIn(expected_reason_strs[i], body) + + setattr(self.user, user_field_key, not user_field_value) + self.user.save() + + # All causes of ineligibility are removed. + response = self._request_login_link(self.email_address.email) + self._assert_ack_message_sent(response) + + # A login link is sent to the address. + self.assertEqual(len(mail.outbox), len(user_fields) + 1) + urls = self.parse_urls_from_str(mail.outbox[-1].body) + self.assertTrue(urls) + login_url = urls[0] + + # The user clicks on the link, logging them in. + response = self.client.get(login_url) + self.assertRedirects(response, reverse('home')) + client_user = get_user(self.client) + self.assertTrue(client_user.is_authenticated) + self._assert_signed_in_message_sent(response, self.user.username) + + def test_verified_email_address_user_eligible(self): + """Test that when the input email exists and is verified, and + the user is eligible to login using a link: + - A login link email is sent. + - The user receives an acknowledgement. + """ + response = self._request_login_link(self.email_address.email) + self._assert_ack_message_sent(response) + + # A login link is sent to the address. + self.assertEqual(len(mail.outbox), 1) + urls = self.parse_urls_from_str(mail.outbox[0].body) + self.assertTrue(urls) + login_url = urls[0] + + # The user, who is active, clicks on the link, logging them in. + self.assertTrue(self.user.is_active) + response = self.client.get(login_url) + self.assertRedirects(response, reverse('home')) + client_user = get_user(self.client) + self.assertTrue(client_user.is_authenticated) + self._assert_signed_in_message_sent(response, self.user.username) + + def test_login_link_expired(self): + """Test that, if the login link is expired, authentication + fails.""" + # Set the max age of a token to 0 seconds. + with override_settings(SESAME_MAX_AGE=0): + sesame_settings.load() + + response = self._request_login_link(self.email_address.email) + self._assert_ack_message_sent(response) + + # A login link is sent to the address. + self.assertEqual(len(mail.outbox), 1) + urls = self.parse_urls_from_str(mail.outbox[0].body) + self.assertTrue(urls) + login_url = urls[0] + + # The user clicks on the link, but is not authenticated. + response = self.client.get(login_url) + self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN) + client_user = get_user(self.client) + self.assertFalse(client_user.is_authenticated) + self._assert_bad_link_message_sent(response) + + # Revert the max age of a token for subsequent tests. + sesame_settings.load() + + def test_login_link_invalid(self): + """Test that, if the login link is invalid, authentication + fails.""" + response = self._request_login_link(self.email_address.email) + self._assert_ack_message_sent(response) + + # A login link is sent to the address. + self.assertEqual(len(mail.outbox), 1) + urls = self.parse_urls_from_str(mail.outbox[0].body) + self.assertTrue(urls) + login_url = urls[0] + + # Reverse the token in the URL so that it is invalid. + parsed_url = urlparse(login_url) + token = parsed_url.query.split('=')[1] + modified_token = ''.join(list(reversed(token))) + modified_url = parsed_url._replace( + query=f'sesame={modified_token}').geturl() + + # The user clicks on the link, but is not authenticated. + response = self.client.get(modified_url) + self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN) + client_user = get_user(self.client) + self.assertFalse(client_user.is_authenticated) + self._assert_bad_link_message_sent(response) + + def test_login_blocked_for_ineligible_user(self): + """Test that a login link for a user who is ineligible to login + using a link does not authenticate the user.""" + login_url = login_token_url(self.user) + + self.user.is_staff = False + self.user.is_superuser = False + self.user.save() + + user_fields = ( + ('is_staff', True), + ('is_superuser', True), + ('is_active', False), + ) + for i, user_field in enumerate(user_fields): + user_field_key, user_field_value = user_field + setattr(self.user, user_field_key, user_field_value) + self.user.save() + + response = self.client.get(login_url) + client_user = get_user(self.client) + self.assertFalse(client_user.is_authenticated) + self._assert_bad_link_message_sent(response) + + setattr(self.user, user_field_key, not user_field_value) + self.user.save() + + # All causes of ineligibility are removed. The user clicks on the link, + # logging them in. + response = self.client.get(login_url) + self.assertRedirects(response, reverse('home')) + client_user = get_user(self.client) + self.assertTrue(client_user.is_authenticated) + self._assert_signed_in_message_sent(response, self.user.username) diff --git a/coldfront/core/user/utils.py b/coldfront/core/user/utils.py index f205a5987..47bede5d1 100644 --- a/coldfront/core/user/utils.py +++ b/coldfront/core/user/utils.py @@ -18,7 +18,6 @@ from coldfront.core.utils.common import import_from_settings from coldfront.core.utils.mail import send_email_template -from sesame.utils import get_query_string from urllib.parse import urljoin logger = logging.getLogger(__name__) @@ -212,13 +211,6 @@ def __email_verification_url(email_address): return urljoin(domain, view) -def login_token_url(user): - """Return a Django Sesame login link for the given User.""" - domain = import_from_settings('CENTER_BASE_URL') - path = reverse('link-login') + get_query_string(user) - return urljoin(domain, path) - - def send_account_activation_email(user): """Send an activation email to the given User, who has just created an account, providing a link to activate the account.""" @@ -292,47 +284,3 @@ def send_email_verification_email(email_address): receiver_list = [email_address.email, ] send_email_template(subject, template_name, context, sender, receiver_list) - - -def send_login_link_email(email_address): - """Send an email containing a login link to the given - EmailAddress.""" - email_enabled = import_from_settings('EMAIL_ENABLED', False) - if not email_enabled: - return - - subject = 'Login Link' - template_name = 'email/login/login_link.txt' - context = { - 'PORTAL_NAME': settings.PORTAL_NAME, - 'login_url': login_token_url(email_address.user), - 'login_link_max_age_minutes': ( - import_from_settings('SESAME_MAX_AGE') // 60), - 'signature': import_from_settings('EMAIL_SIGNATURE', ''), - } - - sender = import_from_settings('EMAIL_SENDER') - receiver_list = [email_address.email, ] - - send_email_template(subject, template_name, context, sender, receiver_list) - - -def send_login_link_ineligible_email(email_address, reason): - """Send an email containing a reason explaining why the User with - the given EmailAddress is ineligible to receive a login link.""" - email_enabled = import_from_settings('EMAIL_ENABLED', False) - if not email_enabled: - return - - subject = 'Ineligible for Login Link' - template_name = 'email/login/login_link_ineligible.txt' - context = { - 'PORTAL_NAME': settings.PORTAL_NAME, - 'reason': reason, - 'signature': import_from_settings('EMAIL_SIGNATURE', ''), - } - - sender = import_from_settings('EMAIL_SENDER') - receiver_list = [email_address.email, ] - - send_email_template(subject, template_name, context, sender, receiver_list) diff --git a/coldfront/core/utils/tests/test_base.py b/coldfront/core/utils/tests/test_base.py index 359b5d731..9ee3bfa46 100644 --- a/coldfront/core/utils/tests/test_base.py +++ b/coldfront/core/utils/tests/test_base.py @@ -2,6 +2,7 @@ from http import HTTPStatus from io import StringIO import os +import re import sys from django.conf import settings @@ -150,6 +151,12 @@ def get_message_strings(response): strings.""" return [str(m) for m in get_messages(response.wsgi_request)] + @staticmethod + def parse_urls_from_str(s): + """Return a list of URLs parsed from the given string, in the + order that they appear.""" + return re.findall('(?Phttps?://[^\s]+)', s) + @staticmethod def sign_user_access_agreement(user): """Simulate the given User signing the access agreement at the From f9c89cd709f4f119c8b0645f7c6fd2ee8c8c9025 Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Mon, 24 Apr 2023 03:28:07 -0700 Subject: [PATCH 49/72] Addressed comments on issue_521 --- .env | 4 +++- Dockerfile | 15 +++++++-------- README.md | 11 ++++++----- bootstrap/ansible/settings_template.tmpl | 6 ++++++ coldfront/config/local_settings.py.sample | 5 ----- docker-compose.yml | 13 ++----------- 6 files changed, 24 insertions(+), 30 deletions(-) diff --git a/.env b/.env index b85d8eed0..ff24258b3 100644 --- a/.env +++ b/.env @@ -1 +1,3 @@ -export COLDFRONT_PORT=8880 \ No newline at end of file +# Environment variables used by Docker (specifically for docker-compose.yml) +DB_NAME=cf_brc_db +COLDFRONT_PORT=8880 \ No newline at end of file diff --git a/Dockerfile b/Dockerfile index a2a01b973..be14a05d7 100644 --- a/Dockerfile +++ b/Dockerfile @@ -9,15 +9,14 @@ RUN pip install -r requirements.txt && rm requirements.txt RUN pip install jinja2 pyyaml # mybrc or mylrc -ARG portal="mybrc" -RUN mkdir -p /var/log/user_portals/cf_${portal} \ - && touch /var/log/user_portals/cf_${portal}/cf_${portal}_{portal,api}.log \ - && chmod 775 /var/log/user_portals/cf_${portal} \ - && chmod 664 /var/log/user_portals/cf_${portal}/cf_${portal}_{portal,api}.log +ARG PORTAL="mybrc" +RUN mkdir -p /var/log/user_portals/cf_${PORTAL} \ + && touch /var/log/user_portals/cf_${PORTAL}/cf_${PORTAL}_{portal,api}.log \ + && chmod 775 /var/log/user_portals/cf_${PORTAL} \ + && chmod 664 /var/log/user_portals/cf_${PORTAL}/cf_${PORTAL}_{portal,api}.log -ARG base_dir="/vagrant/coldfront_app" -COPY . ${base_dir}/coldfront/ -WORKDIR ${base_dir}/coldfront/ +COPY . /vagrant/coldfront_app/coldfront/ +WORKDIR /vagrant/coldfront_app/coldfront/ RUN chmod +x ./manage.py diff --git a/README.md b/README.md index cd78d3cc8..010e29a87 100644 --- a/README.md +++ b/README.md @@ -209,11 +209,12 @@ multiple files or directories to omit. ## Docker - Quick Install (Recommend) 1. Generate configuration (`dev_settings.py`): have Python with the `jinja2` and `pyyaml` libraries installed, and then run `bootstrap/development/gen_config.sh` -2. Build Images: In the base directory, run `docker build . -t coldfront` and `docker build . -f Dockerfile.db -t coldfront_db` -3. To run: In the base directory, run `docker compose up` -4. To enter the coldfront container (similar to `vagrant ssh`): run `docker exec -it coldfront-coldfront-1 bash` -5. To load a database backup: run `bootstrap/development/docker_load_database_backup.sh ${DB_NAME} ${PATH_TO_DUMP}` -6. To start from scratch (delete volumes): In the base directory, run `docker compose down --volumes` +2. Build Images: In the base directory, run `docker build . -t coldfront` and `docker build . -f Dockerfile.db -t coldfront_db`. `--build-arg PORTAL=mylrc` can be added to the build command to build for mylrc. +3. If needed, modify `.env` to customize the web server port and the database name (e.g from `cf_mybrc_db` to `cf_mylrc_db`) +4. To run: In the base directory, run `docker compose up` +5. To enter the coldfront container (similar to `vagrant ssh`): run `docker exec -it coldfront-coldfront-1 bash` +6. To load a database backup: run `bootstrap/development/docker_load_database_backup.sh ${DB_NAME} ${PATH_TO_DUMP}` +7. To start from scratch (delete volumes): In the base directory, run `docker compose down --volumes` ## Local Machine - Quick Install (Not Recommended) diff --git a/bootstrap/ansible/settings_template.tmpl b/bootstrap/ansible/settings_template.tmpl index 818c6abfe..4f96934d2 100644 --- a/bootstrap/ansible/settings_template.tmpl +++ b/bootstrap/ansible/settings_template.tmpl @@ -152,6 +152,12 @@ EXTRA_INTERNAL_IPS = [ {% endfor %} ] +if DEBUG: + # For Docker support + import socket + hostname, _, ips = socket.gethostbyname_ex(socket.gethostname()) + INTERNAL_IPS = [ip[: ip.rfind('.')] + '.1' for ip in ips] + ['10.0.2.2'] + #------------------------------------------------------------------------------ # django-flags settings #------------------------------------------------------------------------------ diff --git a/coldfront/config/local_settings.py.sample b/coldfront/config/local_settings.py.sample index d4af5a466..d5164bebb 100644 --- a/coldfront/config/local_settings.py.sample +++ b/coldfront/config/local_settings.py.sample @@ -385,11 +385,6 @@ INTERNAL_IPS = [ '127.0.0.1' ] -if DEBUG: - import socket - hostname, _, ips = socket.gethostbyname_ex(socket.gethostname()) - INTERNAL_IPS = [ip[: ip.rfind('.')] + '.1' for ip in ips] + ['10.0.2.2'] - # The number of hours for which a newly created authentication token will be # valid. TOKEN_EXPIRATION_HOURS = 24 diff --git a/docker-compose.yml b/docker-compose.yml index 1eb6bd5ad..60c6d84d8 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,12 +1,3 @@ -# Decompoe the stack into microservices running in Docker, orchestrated with Docker Compose. -# Use cases: - -# People with MacBooks with M1 chips aren’t able to use VirtualBox right now, so we need an alternative for them. -# The group is bringing up an internal Kubernetes cluster. It’d be nice to have multiple staging instances that we can easily bring up so that we can stage multiple branches at the same time. -# This should be more lightweight/faster than a VM. -# The deliverable would be a set of Docker images (Postgres, Redis, one to run the web service, etc.) and a Docker Compose file to run them. -# I’m not exactly sure how Ansible is going to fit into this picture. Maybe Ansible just creates configuration files that get injected into containers. Let me know if you have thoughts on / experience with this.# Docker Compose - services: db: image: coldfront_db:latest @@ -16,14 +7,14 @@ services: ports: - 5432:5432 environment: - POSTGRES_DB: cf_brc_db + POSTGRES_DB: ${DB_NAME} POSTGRES_USER: admin POSTGRES_PASSWORD: root LOAD_DUMP: true volumes: - db_data:/var/lib/postgresql/data healthcheck: - test: ["CMD", "pg_isready", "-U", "admin", "-d", "cf_brc_db"] + test: ["CMD", "pg_isready", "-U", "admin", "-d", "${DB_NAME}"] interval: 10s timeout: 5s retries: 5 From aca0d48b552d9532abd35b78045776068fdf066e Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Thu, 27 Apr 2023 14:08:04 -0700 Subject: [PATCH 50/72] Add command for migrating old EmailAddress model to new one --- .../commands/migrate_email_address_model.py | 235 ++++++++++++++++++ 1 file changed, 235 insertions(+) create mode 100644 coldfront/core/user/management/commands/migrate_email_address_model.py diff --git a/coldfront/core/user/management/commands/migrate_email_address_model.py b/coldfront/core/user/management/commands/migrate_email_address_model.py new file mode 100644 index 000000000..3593dca8b --- /dev/null +++ b/coldfront/core/user/management/commands/migrate_email_address_model.py @@ -0,0 +1,235 @@ +import logging +import sys +import traceback + +from django.contrib.auth.models import User +from django.core.management.base import BaseCommand + +from allauth.account.models import EmailAddress as NewEmailAddress + +from coldfront.core.user.models import EmailAddress as OldEmailAddress +from coldfront.core.utils.common import add_argparse_dry_run_argument + + +"""An admin command that creates or updates corresponding instances of +allauth.account.models.EmailAddress for existing emails in the User +model and coldfront.core.user.models.EmailAddress model.""" + + +class Command(BaseCommand): + + help = ( + 'Create or update instances of allauth.account.models.EmailAddress ' + 'based on emails stored in the User model and the deprecated ' + 'coldfront.core.users.models.EmailAddress model. This command strictly ' + 'updates whether an email is primary or verified from False to True, ' + 'and never the other way around (i.e., it respects existing values in ' + 'the new model). It is intended for a one-time migration, and may not ' + 'be suited for general reuse.') + + logger = logging.getLogger(__name__) + + def add_arguments(self, parser): + add_argparse_dry_run_argument(parser) + + def handle(self, *args, **options): + dry_run = options['dry_run'] + if not dry_run: + user_confirmation = input( + 'Are you sure you wish to proceed? [Y/y/N/n]: ') + if user_confirmation.strip().lower() != 'y': + self.stdout.write(self.style.WARNING('Migration aborted.')) + sys.exit(0) + for user in User.objects.iterator(): + try: + old_email_data = self._get_old_email_data(user) + self._process_emails_for_user( + user, old_email_data, dry_run=dry_run) + except Exception: + message = ( + f'Failed to process User {user.pk}. Details:\n' + f'{traceback.format_exc()}') + self.stdout.write(self.style.ERROR(message)) + + def _create_address_for_user(self, email, user, primary=False, + verified=False, dry_run=True): + """Create an instance of the new EmailAddress model for the + given email (str) and User, setting its primary and verified + fields. Optionally display the update instead of performing it. + + If the email already belongs to a different user, raise an + error. + + This method makes the following assumptions: + - The User does not already have a corresponding instance. + - If primary is True, the User does not already have a + primary instance. + + It should not be called if the assumptions are not true. + """ + try: + other_user_address = NewEmailAddress.objects.get(email=email) + except NewEmailAddress.DoesNotExist: + if dry_run: + phrase = 'Would create' + pk = 'PK' + style = self.style.WARNING + else: + email_address = NewEmailAddress.objects.create( + user=user, + email=email, + primary=primary, + verified=verified) + phrase = 'Created' + pk = email_address.pk + style = self.style.SUCCESS + + message = ( + f'{phrase} EmailAddress {pk} for User {user.pk} ' + f'({user.username}) with primary={primary} and ' + f'verified={verified}.') + self.stdout.write(style(message)) + if not dry_run: + self.logger.info(message) + else: + message = ( + f'A different User {other_user_address.user.pk} already has ' + f'email "{email}".') + self.stdout.write(self.style.WARNING(message)) + if not dry_run: + self.logger.warning(message) + + @staticmethod + def _get_old_email_data(user): + """Return a dict mapping emails (str) for the given User from + the old model and the User instance to a dict with keys + 'primary' and 'verified', denoting whether the address was + primary and verified. + + The email stored in the User instance is interpreted as being + primary. + + Raise an error if the User does not have exactly one primary + email under the old model. + """ + old_addresses = OldEmailAddress.objects.filter(user=user) + old_emails = {} + + for old_address in old_addresses: + email_str = old_address.email.strip().lower() + old_emails[email_str] = { + 'verified': old_address.is_verified, + 'primary': old_address.is_primary, + } + user_email_str = user.email.strip().lower() + if user_email_str: + if user_email_str in old_emails: + old_emails[user_email_str]['primary'] = True + else: + old_emails[user_email_str] = { + 'verified': False, + 'primary': True, + } + + old_primaries = [] + for email, attributes in old_emails.items(): + if attributes['primary']: + old_primaries.append(email) + + num_primaries = len(old_primaries) + if num_primaries == 0: + raise Exception( + f'Found no old primary emails for User {user.pk} ' + f'({user.username}).') + elif num_primaries == 1: + pass + else: + raise Exception( + f'Found multiple old primary emails for User {user.pk} ' + f'({user.username}): {", ".join(old_primaries)}.') + + return old_emails + + def _process_emails_for_user(self, user, old_email_data, dry_run=True): + """Given a User and a dict containing information about emails + in the old model, create or update emails in the new model. + Optionally display updates instead of performing them. + + old_email_data is assumed to contain exactly one primary + address. + """ + new_addresses = NewEmailAddress.objects.filter(user=user) + new_address_lower_strs = set( + [email.lower() + for email in new_addresses.values_list('email', flat=True)]) + + try: + new_primary = new_addresses.get(primary=True) + except NewEmailAddress.DoesNotExist: + new_primary = None + except NewEmailAddress.MultipleObjectsReturned as e: + raise e + + for old_email, attributes in old_email_data.items(): + # If this was a primary address, but the user already has a + # different primary, do not set this as primary. + if attributes['primary']: + if (new_primary is not None and + new_primary.email.lower() != old_email): + attributes['primary'] = False + if old_email in new_address_lower_strs: + email_address = new_addresses.get(email=old_email) + self._update_address_for_user( + email_address, user, primary=attributes['primary'], + verified=attributes['verified'], dry_run=dry_run) + else: + self._create_address_for_user( + old_email, user, primary=attributes['primary'], + verified=attributes['verified'], dry_run=dry_run) + + def _update_address_for_user(self, email_address, user, primary=False, + verified=False, dry_run=True): + """Update the given EmailAddress instance (new model) for the + given User, setting its primary and verified fields. + + It only sets fields if they would go from False to True. + + This method makes the following assumptions: + - If primary is True, the User does not already have a + primary instance other than the given instance. + + It should not be called if the assumptions are not true. + """ + # Only update "primary" if it would go from False to True, not + # the other way around. + primary_updated = not email_address.primary and primary + if primary_updated: + email_address.primary = True + # Only update "verified" if it would go from False to True, not + # the other way around. + verified_updated = not email_address.verified and verified + if verified_updated: + email_address.verified = True + + if primary_updated or verified_updated: + if dry_run: + phrase = 'Would update' + style = self.style.WARNING + else: + email_address.save() + phrase = 'Updated' + style = self.style.SUCCESS + + updates = [] + if primary_updated: + updates.append('set primary to True') + if verified_updated: + updates.append('set verified to True') + + message = ( + f'{phrase} EmailAddress {email_address.pk} ' + f'({email_address.email}) for User {user.pk} ' + f'({user.username}): {", ".join(updates)}.') + self.stdout.write(style(message)) + if not dry_run: + self.logger.info(message) From 7d1de921a5e026366ea9d1c308276c697fa64ebb Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 28 Apr 2023 09:25:27 -0700 Subject: [PATCH 51/72] Add admin interface action for activating selected users --- coldfront/core/utils/admin.py | 36 +++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/coldfront/core/utils/admin.py b/coldfront/core/utils/admin.py index 2f9840a80..a2aec98e8 100644 --- a/coldfront/core/utils/admin.py +++ b/coldfront/core/utils/admin.py @@ -1,9 +1,16 @@ from django.contrib import admin -from django.contrib.auth.admin import GroupAdmin, UserAdmin +from django.contrib import messages +from django.contrib.auth.admin import GroupAdmin as BaseGroupAdmin +from django.contrib.auth.admin import UserAdmin as BaseUserAdmin from django.contrib.auth.models import Group, User from coldfront.core.user.admin import EmailAddressInline +import logging + + +logger = logging.getLogger(__name__) + class UserInLine(admin.TabularInline): model = User.groups.through @@ -12,18 +19,35 @@ class UserInLine(admin.TabularInline): can_delete = False -class GroupsAdmin(GroupAdmin): +class GroupAdmin(BaseGroupAdmin): list_display = ["name", "pk"] inlines = [UserInLine] -class UsersAdmin(UserAdmin): - list_display = ["username", "email", "first_name", "last_name", "is_staff"] +class UserAdmin(BaseUserAdmin): + list_display = ["username", "email", "first_name", "last_name", "is_active"] inlines = [EmailAddressInline] + actions = ('activate', ) + + @admin.action(description='Activate selected users') + def activate(self, request, queryset): + user_pks = set() + for user in queryset: + if not user.is_active: + user.is_active = True + user.save() + user_pks.add(user.pk) + n = len(user_pks) + messages.success(request, f'Activated {n} users.') + if n > 0: + user_pk_str = ", ".join([str(pk) for pk in sorted(user_pks)]) + logger.info( + f'User {request.user.username} activated the following users: ' + f'{user_pk_str}.') admin.site.unregister(Group) -admin.site.register(Group, GroupsAdmin) +admin.site.register(Group, GroupAdmin) admin.site.unregister(User) -admin.site.register(User, UsersAdmin) \ No newline at end of file +admin.site.register(User, UserAdmin) From 48c44572382a0a9acdacf1d2fd33dfbc521a0fbe Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 28 Apr 2023 10:22:55 -0700 Subject: [PATCH 52/72] Forcefully disable SSO in test expecting Basic Auth --- .../user/tests/test_views/test_activate_user_account.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/coldfront/core/user/tests/test_views/test_activate_user_account.py b/coldfront/core/user/tests/test_views/test_activate_user_account.py index 8f7080475..ec18454b2 100644 --- a/coldfront/core/user/tests/test_views/test_activate_user_account.py +++ b/coldfront/core/user/tests/test_views/test_activate_user_account.py @@ -1,6 +1,10 @@ +from copy import deepcopy + +from django.conf import settings from django.contrib.auth.models import User from django.contrib.messages import get_messages from django.test import Client +from django.test import override_settings from allauth.account.models import EmailAddress from flags.state import enable_flag @@ -9,6 +13,11 @@ from coldfront.core.user.utils import account_activation_url +FLAGS_COPY = deepcopy(settings.FLAGS) +FLAGS_COPY['SSO_ENABLED'] = {'condition': 'boolean', 'value': False} + + +@override_settings(FLAGS=FLAGS_COPY) class TestActivateUserAccount(TestUserBase): """A class for testing the view for activating a user's account.""" From c85cf7ce6b0285e6b0b6686393d9b543ce9f615f Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 28 Apr 2023 10:42:54 -0700 Subject: [PATCH 53/72] Create new PIs in new project requests as active --- .../core/project/views_/new_project_views/request_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coldfront/core/project/views_/new_project_views/request_views.py b/coldfront/core/project/views_/new_project_views/request_views.py index aa7019c8f..e4bfa52d8 100644 --- a/coldfront/core/project/views_/new_project_views/request_views.py +++ b/coldfront/core/project/views_/new_project_views/request_views.py @@ -474,7 +474,7 @@ def __handle_pi_data(self, form_data): first_name=data['first_name'], last_name=data['last_name'], email=email, - is_active=False) + is_active=True) except IntegrityError as e: self.logger.error(f'User {email} unexpectedly exists.') raise e From a9a3b3dfb15104f6085cdeec30365b8f04353bf9 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 28 Apr 2023 14:21:21 -0700 Subject: [PATCH 54/72] Include new flag in test settings --- coldfront/config/test_settings.py.sample | 1 + 1 file changed, 1 insertion(+) diff --git a/coldfront/config/test_settings.py.sample b/coldfront/config/test_settings.py.sample index 4d92b5511..b69c3f4be 100644 --- a/coldfront/config/test_settings.py.sample +++ b/coldfront/config/test_settings.py.sample @@ -88,6 +88,7 @@ FLAGS = { ], 'BASIC_AUTH_ENABLED': [{'condition': 'boolean', 'value': True}], 'BRC_ONLY': [{'condition': 'boolean', 'value': True}], + 'LINK_LOGIN_ENABLED': [{'condition': 'boolean', 'value': False}], 'LRC_ONLY': [{'condition': 'boolean', 'value': False}], 'SECURE_DIRS_REQUESTABLE': [{'condition': 'boolean', 'value': True}], 'SERVICE_UNITS_PURCHASABLE': [{'condition': 'boolean', 'value': True}], From 61dcdfeea6f90416724f69655963e026bc97d5ec Mon Sep 17 00:00:00 2001 From: Hamza Kundi Date: Mon, 1 May 2023 04:50:12 -0700 Subject: [PATCH 55/72] added is_active=False check --- .../core/project/forms_/new_project_forms/request_forms.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coldfront/core/project/forms_/new_project_forms/request_forms.py b/coldfront/core/project/forms_/new_project_forms/request_forms.py index c18198347..bf767125f 100644 --- a/coldfront/core/project/forms_/new_project_forms/request_forms.py +++ b/coldfront/core/project/forms_/new_project_forms/request_forms.py @@ -216,9 +216,9 @@ def disable_pi_choices(self): def exclude_pi_choices(self): """Exclude certain Users from being displayed as PI options.""" - # Exclude any user that does not have an email address. + # Exclude any user that does not have an email address or is inactive. self.fields['PI'].queryset = User.objects.exclude( - Q(email__isnull=True) | Q(email__exact='')) + Q(email__isnull=True) | Q(email__exact='') | Q(is_active=False)) class SavioProjectNewPIForm(forms.Form): From ef8fb82977dce1f05b8c290c2eb57ad85ac1f3f5 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 1 May 2023 09:48:27 -0700 Subject: [PATCH 56/72] Properly test if identity linking request button is enabled --- .../user/templates/user/user_profile.html | 2 +- .../test_link_personal_account.py | 32 ++++++++++++++----- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/coldfront/core/user/templates/user/user_profile.html b/coldfront/core/user/templates/user/user_profile.html index 6576a708b..13fb20d2c 100644 --- a/coldfront/core/user/templates/user/user_profile.html +++ b/coldfront/core/user/templates/user/user_profile.html @@ -240,7 +240,7 @@

    Request Linking Email {% else %} - + {% csrf_token %} {% else %} - {% url 'account_email' as email_url %} + {% flag_enabled 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED' as multiple_email_addresses_allowed %}

    - This email confirmation link expired or is invalid. Please - issue a new email confirmation request. + This email confirmation link expired or is invalid. + {% if multiple_email_addresses_allowed %} + Please issue a new email confirmation request. + {% endif %}

    {% endif %} From e62f2302635f389316cbea0c9b8442e89f21cf52 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 5 May 2023 14:26:54 -0700 Subject: [PATCH 61/72] Skip social account connection for existing user if multiple addresses disabled; pass missing context variables to error page --- coldfront/core/socialaccount/adapter.py | 64 ++++++++++++++++----- coldfront/templates/error_with_message.html | 2 +- 2 files changed, 50 insertions(+), 16 deletions(-) diff --git a/coldfront/core/socialaccount/adapter.py b/coldfront/core/socialaccount/adapter.py index b7310459c..84f3f9e35 100644 --- a/coldfront/core/socialaccount/adapter.py +++ b/coldfront/core/socialaccount/adapter.py @@ -4,13 +4,17 @@ from allauth.account.utils import user_username from allauth.exceptions import ImmediateHttpResponse from allauth.socialaccount.adapter import DefaultSocialAccountAdapter +from allauth.socialaccount.models import SocialAccount from allauth.utils import valid_email_or_none from coldfront.core.account.utils.login_activity import LoginActivityVerifier +from coldfront.core.utils.context_processors import portal_and_program_names from collections import defaultdict from django.conf import settings from django.http import HttpResponseBadRequest from django.http import HttpResponseServerError from django.template.loader import render_to_string +from django.urls import reverse +from flags.state import flag_enabled import logging @@ -20,6 +24,11 @@ class CILogonAccountAdapter(DefaultSocialAccountAdapter): """An adapter that adjusts handling for the CILogon provider.""" + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._flag_multiple_email_addresses_allowed = flag_enabled( + 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED') + def populate_user(self, request, sociallogin, data): """Handle logins using the CILogon provider differently. In particular, use the given email address as the username; raise @@ -58,6 +67,17 @@ def populate_user(self, request, sociallogin, data): return super().populate_user(request, sociallogin, data) + def get_connect_redirect_url(self, request, socialaccount): + """ + Returns the default URL to redirect to after successfully + connecting a social account. + """ + if self._flag_multiple_email_addresses_allowed: + url = reverse('socialaccount_connections') + else: + url = reverse('home') + return url + def pre_social_login(self, request, sociallogin): """At this point, the user is authenticated by a provider. If the provider is not CILogon, do nothing. Otherwise, if this @@ -137,8 +157,9 @@ def pre_social_login(self, request, sociallogin): addresses = matching_addresses_by_user[user] if any([a.verified for a in addresses]): # After this, allauth.account.adapter.pre_login blocks login if - # the user is inactive. Regardless of that, connect the user - # (and trigger signals for creating EmailAddresses). + # the user is inactive. Regardless of that, (conditionally) + # connect the user (and trigger signals for creating + # EmailAddresses). self._connect_user( request, sociallogin, provider, user, user_email, user_uid) else: @@ -184,11 +205,21 @@ def _block_login_for_verification(self, request, sociallogin, provider, 'address for a verification email.') self._raise_client_error(message) - @staticmethod - def _connect_user(request, sociallogin, provider, user, user_email, + def _connect_user(self, request, sociallogin, provider, user, user_email, user_uid): """Connect the provider account to the User's account in the - database.""" + database. + + If users are not allowed to have multiple email addresses, and + the user already has a SocialAccount, raise an error. + """ + if not self._flag_multiple_email_addresses_allowed: + if SocialAccount.objects.filter(user=user).exists(): + message = ( + 'You may not connect more than one third-party account to ' + 'your portal account.') + self._raise_client_error(message) + sociallogin.connect(request, user) log_message = ( f'Successfully connected data for User with email {user_email} and ' @@ -203,20 +234,23 @@ def _get_auth_error_message(): f'Unexpected authentication error. Please contact ' f'{settings.CENTER_HELP_EMAIL} for further assistance.') - @staticmethod - def _raise_client_error(message): + def _raise_client_error(self, message): """Raise an ImmediateHttpResponse with a client error and the given message.""" + self._raise_error(HttpResponseBadRequest, message) + + @staticmethod + def _raise_error(response_class, message): + """Raise an ImmediateHttpResponse with an error HttpResponse + class (e.g., HttpResponseBadRequest or HttpResponseServerError) + error and the given message.""" template = 'error_with_message.html' - html = render_to_string(template, context={'message': message}) - response = HttpResponseBadRequest(html) + context = {'message': message, **portal_and_program_names(None)} + html = render_to_string(template, context=context) + response = response_class(html) raise ImmediateHttpResponse(response) - @staticmethod - def _raise_server_error(message): + def _raise_server_error(self, message): """Raise an ImmediateHttpResponse with a server error and the given message.""" - template = 'error_with_message.html' - html = render_to_string(template, context={'message': message}) - response = HttpResponseServerError(html) - raise ImmediateHttpResponse(response) + self._raise_error(HttpResponseServerError, message) diff --git a/coldfront/templates/error_with_message.html b/coldfront/templates/error_with_message.html index 19d80b5ae..1b7240c00 100644 --- a/coldfront/templates/error_with_message.html +++ b/coldfront/templates/error_with_message.html @@ -2,7 +2,7 @@ {% load static %} {% block title %} -MyBRC - Error +{{ PORTAL_NAME }} - Error {% endblock %} From 31833a0304c6298569f592ee330b5ae4412980a2 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 5 May 2023 14:51:45 -0700 Subject: [PATCH 62/72] Opt to block the allauth 'connect' process entirely --- coldfront/core/socialaccount/adapter.py | 32 ++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/coldfront/core/socialaccount/adapter.py b/coldfront/core/socialaccount/adapter.py index 84f3f9e35..819edb003 100644 --- a/coldfront/core/socialaccount/adapter.py +++ b/coldfront/core/socialaccount/adapter.py @@ -5,6 +5,7 @@ from allauth.exceptions import ImmediateHttpResponse from allauth.socialaccount.adapter import DefaultSocialAccountAdapter from allauth.socialaccount.models import SocialAccount +from allauth.socialaccount.providers.base import AuthProcess from allauth.utils import valid_email_or_none from coldfront.core.account.utils.login_activity import LoginActivityVerifier from coldfront.core.utils.context_processors import portal_and_program_names @@ -110,6 +111,16 @@ def pre_social_login(self, request, sociallogin): if provider != 'cilogon': return + # If users are not allowed to have multiple emails, and the user is + # attempting to connect another SocialAccount to their account (as + # opposed to logging in with an existing one), raise an error. + if not self._flag_multiple_email_addresses_allowed: + if sociallogin.state.get('process', None) == AuthProcess.CONNECT: + message = ( + 'You may not connect more than one third-party account to ' + 'your portal account.') + self._raise_client_error(message) + # If a SocialAccount already exists, meaning the provider account is # connected to a local account, proceed with login. if sociallogin.is_existing: @@ -157,9 +168,8 @@ def pre_social_login(self, request, sociallogin): addresses = matching_addresses_by_user[user] if any([a.verified for a in addresses]): # After this, allauth.account.adapter.pre_login blocks login if - # the user is inactive. Regardless of that, (conditionally) - # connect the user (and trigger signals for creating - # EmailAddresses). + # the user is inactive. Regardless of that, connect the user + # (and trigger signals for creating EmailAddresses). self._connect_user( request, sociallogin, provider, user, user_email, user_uid) else: @@ -205,21 +215,11 @@ def _block_login_for_verification(self, request, sociallogin, provider, 'address for a verification email.') self._raise_client_error(message) - def _connect_user(self, request, sociallogin, provider, user, user_email, + @staticmethod + def _connect_user(request, sociallogin, provider, user, user_email, user_uid): """Connect the provider account to the User's account in the - database. - - If users are not allowed to have multiple email addresses, and - the user already has a SocialAccount, raise an error. - """ - if not self._flag_multiple_email_addresses_allowed: - if SocialAccount.objects.filter(user=user).exists(): - message = ( - 'You may not connect more than one third-party account to ' - 'your portal account.') - self._raise_client_error(message) - + database.""" sociallogin.connect(request, user) log_message = ( f'Successfully connected data for User with email {user_email} and ' From 5a9b94f7e20dd28b59d6010dde503cd0128abadf Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 5 May 2023 15:12:44 -0700 Subject: [PATCH 63/72] Log a warning when provider gives multiple addresses --- coldfront/core/socialaccount/adapter.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/coldfront/core/socialaccount/adapter.py b/coldfront/core/socialaccount/adapter.py index 819edb003..5c2345f67 100644 --- a/coldfront/core/socialaccount/adapter.py +++ b/coldfront/core/socialaccount/adapter.py @@ -126,14 +126,25 @@ def pre_social_login(self, request, sociallogin): if sociallogin.is_existing: return - # If the provider does not provide any addresses, raise an error. provider_addresses = sociallogin.email_addresses - if not provider_addresses: + num_provider_addresses = len(provider_addresses) + # If the provider does not provide any addresses, raise an error. + if num_provider_addresses == 0: log_message = ( f'Provider {provider} did not provide any email addresses for ' f'User with email {user_email} and UID {user_uid}.') logger.error(log_message) self._raise_server_error(self._get_auth_error_message()) + # In general, it is expected that a provider will only give one address. + # If multiple are given, allow all of them to be associated with the + # user (agnostic of whether users are allowed to have multiple), but log + # a warning. + elif num_provider_addresses > 1: + log_message = ( + f'Provider {provider} provided more than one email address for ' + f'User with email {user_email} and UID {user_uid}: ' + f'{", ".join(provider_addresses)}.') + logger.warning(log_message) # SOCIALACCOUNT_PROVIDERS['cilogon']['VERIFIED_EMAIL'] should be True, # so all provider-given addresses should be interpreted as verified. From 6868b1fa9d071053e7c2353cdbdef1ec976a499f Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 8 May 2023 08:31:35 -0700 Subject: [PATCH 64/72] Cast flagged path pattern to string --- coldfront/core/account/urls.py | 4 ++-- coldfront/core/socialaccount/urls.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/coldfront/core/account/urls.py b/coldfront/core/account/urls.py index 602cb6a87..6474ba91e 100644 --- a/coldfront/core/account/urls.py +++ b/coldfront/core/account/urls.py @@ -28,5 +28,5 @@ if pattern.name in names_to_include_if_multiple_emails_allowed: urlpatterns.append( f_path( - pattern.pattern, pattern.callback, pattern.default_args, - pattern.name)) + str(pattern.pattern), pattern.callback, + pattern.default_args, pattern.name)) diff --git a/coldfront/core/socialaccount/urls.py b/coldfront/core/socialaccount/urls.py index e3361809d..d3080f8bc 100644 --- a/coldfront/core/socialaccount/urls.py +++ b/coldfront/core/socialaccount/urls.py @@ -26,5 +26,5 @@ if pattern.name in names_to_include_if_multiple_emails_allowed: urlpatterns.append( f_path( - pattern.pattern, pattern.callback, pattern.default_args, - pattern.name)) + str(pattern.pattern), pattern.callback, + pattern.default_args, pattern.name)) From 3c5c36070b47973e34f20617ee8c9bbd78218565 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 10 May 2023 10:20:58 -0700 Subject: [PATCH 65/72] Require Django SECRET_KEY to be configured for each deployment --- README.md | 16 +++++++++++----- bootstrap/ansible/main.copyme | 4 ++++ bootstrap/ansible/settings_template.tmpl | 2 ++ coldfront/config/local_settings.py.sample | 10 ++++++---- 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 010e29a87..d7fa75d44 100644 --- a/README.md +++ b/README.md @@ -46,9 +46,15 @@ of variables used by Ansible to configure the system. ``` cp bootstrap/ansible/main.copyme main.yml ``` -7. Customize `main.yml`. In particular, uncomment everything under the `dev_settings` section, and fill in the below variables. Note +7. Generate a key to be used as the `SECRET_KEY` for Django. + ``` + # This produces two lines: condense them into one. + openssl rand -base64 64 + ``` +8. Customize `main.yml`. In particular, uncomment everything under the `dev_settings` section, and fill in the below variables. Note that quotes need not be provided, except in the list variable. ``` + django_secret_key: secret_key_from_previous_step db_admin_passwd: password_here redis_passwd: password_here from_email: you@email.com @@ -56,18 +62,18 @@ that quotes need not be provided, except in the list variable. email_admin_list: ["you@email.com"] request_approval_cc_list: ["you@email.com"] ``` -8. Provision the VM. This should run the Ansible playbook. Expect this to take +9. Provision the VM. This should run the Ansible playbook. Expect this to take a few minutes on the first run. ``` vagrant up ``` -9. SSH into the VM. +10. SSH into the VM. ``` vagrant ssh ``` -10. On the host machine, navigate to `http://localhost:8880`, where the +11. On the host machine, navigate to `http://localhost:8880`, where the application should be served. -11. (Optional) Load data from a database dump file. +12. (Optional) Load data from a database dump file. ``` # Clear the Django database to avoid conflicts. python manage.py sqlflush | python manage.py dbshell diff --git a/bootstrap/ansible/main.copyme b/bootstrap/ansible/main.copyme index 62b9bb693..4410012fe 100644 --- a/bootstrap/ansible/main.copyme +++ b/bootstrap/ansible/main.copyme @@ -18,6 +18,10 @@ djangoprojname: coldfront # BRC/LRC Config ############################################################################### +# The secret key used by Django for cryptographic signing. +# TODO: Generate one using: `openssl rand -base64 64`. +django_secret_key: + # The name of the PostgreSQL database. # TODO: For LRC, set this to 'cf_lrc_db'. db_name: cf_brc_db diff --git a/bootstrap/ansible/settings_template.tmpl b/bootstrap/ansible/settings_template.tmpl index f86d9bba5..3b1026e56 100644 --- a/bootstrap/ansible/settings_template.tmpl +++ b/bootstrap/ansible/settings_template.tmpl @@ -3,6 +3,8 @@ import os # SECURITY WARNING: don't run with debug turned on in production! DEBUG = {{ debug }} +SECRET_KEY = '{{ django_secret_key }}' + ALLOWED_HOSTS = ['0.0.0.0', '{{ hostname }}', '{{ host_ip }}'] PORTAL_NAME = '{{ portal_name }}' diff --git a/coldfront/config/local_settings.py.sample b/coldfront/config/local_settings.py.sample index d574160cb..92a69c701 100644 --- a/coldfront/config/local_settings.py.sample +++ b/coldfront/config/local_settings.py.sample @@ -8,7 +8,9 @@ configure logging, and ColdFront plugins. # Secret Key -- Generate new key using: https://www.miniwebtool.com/django-secret-key-generator/ # and replace #------------------------------------------------------------------------------ -SECRET_KEY = 'vtri&lztlbinerr4+yg1yzm23ez@+ub6=4*63z1%d!)fg(g4x$' # REPLACE +# Instead of defining this here, it is imported from the deployment-specific +# settings file below. +# SECRET_KEY = 'vtri&lztlbinerr4+yg1yzm23ez@+ub6=4*63z1%d!)fg(g4x$' # REPLACE #------------------------------------------------------------------------------ # Enable debugging. WARNING: These should be set to False in production @@ -16,9 +18,9 @@ SECRET_KEY = 'vtri&lztlbinerr4+yg1yzm23ez@+ub6=4*63z1%d!)fg(g4x$' # REPLACE DEBUG = True DEVELOP = True -if DEBUG is False and SECRET_KEY == 'vtri&lztlbinerr4+yg1yzm23ez@+ub6=4*63z1%d!)fg(g4x$': - from django.core.exceptions import ImproperlyConfigured - raise ImproperlyConfigured("The SECRET_KEY setting is using the preset value. Please update it!") +# if DEBUG is False and SECRET_KEY == 'vtri&lztlbinerr4+yg1yzm23ez@+ub6=4*63z1%d!)fg(g4x$': +# from django.core.exceptions import ImproperlyConfigured +# raise ImproperlyConfigured("The SECRET_KEY setting is using the preset value. Please update it!") #------------------------------------------------------------------------------ # Session settings From 81d052ea7d211fdcc06257b749d1f9fac14725f9 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 10 May 2023 12:58:27 -0700 Subject: [PATCH 66/72] Prefer non-rendered context variable to hardcoded portal name in 400 template --- coldfront/templates/400.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coldfront/templates/400.html b/coldfront/templates/400.html index 57b754fe6..077ef2fe2 100644 --- a/coldfront/templates/400.html +++ b/coldfront/templates/400.html @@ -2,7 +2,7 @@ {% load static %} {% block title %} -MyBRC - Bad Request +{{ PORTAL_NAME }} - Bad Request {% endblock %} From 26afc2e466b76729b1f38ecab4eb1746d45bdf42 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 10 May 2023 15:32:29 -0700 Subject: [PATCH 67/72] Test that UserProfile sections and underlying views are hidden if flag is disabled --- .../test_user_profile/test_email_addresses.py | 52 +++++++++++++++++++ .../test_third_party_accounts.py | 52 +++++++++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 coldfront/core/user/tests/test_views/test_user_profile/test_email_addresses.py create mode 100644 coldfront/core/user/tests/test_views/test_user_profile/test_third_party_accounts.py diff --git a/coldfront/core/user/tests/test_views/test_user_profile/test_email_addresses.py b/coldfront/core/user/tests/test_views/test_user_profile/test_email_addresses.py new file mode 100644 index 000000000..ee5108c5a --- /dev/null +++ b/coldfront/core/user/tests/test_views/test_user_profile/test_email_addresses.py @@ -0,0 +1,52 @@ +from copy import deepcopy +from http import HTTPStatus + +from django.conf import settings +from django.test import override_settings +from django.urls import reverse + +from flags.state import disable_flag +from flags.state import enable_flag +from flags.state import flag_enabled + +from coldfront.core.utils.tests.test_base import TestBase + + +class TestEmailAddresses(TestBase): + """A class for testing the "Other Email Addresses" section of the + User Profile.""" + + def setUp(self): + """Set up test data.""" + super().setUp() + self.create_test_user() + self.client.login(username=self.user.username, password=self.password) + + @staticmethod + def user_profile_url(): + """Return the URL to the User Profile.""" + return reverse('user-profile') + + def test_section_disabled_if_multiple_addresses_disallowed(self): + """Test that, if the 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED' feature + flag is disabled, the section is hidden.""" + flag_name = 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED' + enable_flag(flag_name) + + response = self.client.get(self.user_profile_url()) + self.assertContains(response, 'Other Email Addresses') + section_url = reverse('account_email') + self.assertContains(response, section_url) + response = self.client.get(section_url) + self.assertEqual(response.status_code, HTTPStatus.OK) + + flags_copy = deepcopy(settings.FLAGS) + flags_copy[flag_name] = {'condition': 'boolean', 'value': False} + with override_settings(FLAGS=flags_copy): + disable_flag(flag_name) + self.assertFalse(flag_enabled(flag_name)) + response = self.client.get(self.user_profile_url()) + self.assertNotContains(response, 'Other Email Addresses') + self.assertNotContains(response, section_url) + response = self.client.get(section_url) + self.assertEqual(response.status_code, HTTPStatus.NOT_FOUND) diff --git a/coldfront/core/user/tests/test_views/test_user_profile/test_third_party_accounts.py b/coldfront/core/user/tests/test_views/test_user_profile/test_third_party_accounts.py new file mode 100644 index 000000000..a87256fb3 --- /dev/null +++ b/coldfront/core/user/tests/test_views/test_user_profile/test_third_party_accounts.py @@ -0,0 +1,52 @@ +from copy import deepcopy +from http import HTTPStatus + +from django.conf import settings +from django.test import override_settings +from django.urls import reverse + +from flags.state import disable_flag +from flags.state import enable_flag +from flags.state import flag_enabled + +from coldfront.core.utils.tests.test_base import TestBase + + +class TestThirdPartyAccounts(TestBase): + """A class for testing the "Third-Party Accounts" section of the + User Profile.""" + + def setUp(self): + """Set up test data.""" + super().setUp() + self.create_test_user() + self.client.login(username=self.user.username, password=self.password) + + @staticmethod + def user_profile_url(): + """Return the URL to the User Profile.""" + return reverse('user-profile') + + def test_section_disabled_if_multiple_addresses_disallowed(self): + """Test that, if the 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED' feature + flag is disabled, the section is hidden.""" + flag_name = 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED' + enable_flag(flag_name) + + response = self.client.get(self.user_profile_url()) + self.assertContains(response, 'Third-Party Accounts') + section_url = reverse('socialaccount_connections') + self.assertContains(response, section_url) + response = self.client.get(section_url) + self.assertEqual(response.status_code, HTTPStatus.OK) + + flags_copy = deepcopy(settings.FLAGS) + flags_copy[flag_name] = {'condition': 'boolean', 'value': False} + with override_settings(FLAGS=flags_copy): + disable_flag(flag_name) + self.assertFalse(flag_enabled(flag_name)) + response = self.client.get(self.user_profile_url()) + self.assertNotContains(response, 'Third-Party Accounts') + self.assertNotContains(response, section_url) + response = self.client.get(section_url) + self.assertEqual(response.status_code, HTTPStatus.NOT_FOUND) From 7e281bd25cd4b943411fd44a5e41d8527e05e3f2 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 12 May 2023 08:54:38 -0700 Subject: [PATCH 68/72] Set SECRET_KEY via env. variable in Actions-run tests --- .github/workflows/django_testing_ci.yml | 1 + coldfront/config/test_settings.py.sample | 2 ++ 2 files changed, 3 insertions(+) diff --git a/.github/workflows/django_testing_ci.yml b/.github/workflows/django_testing_ci.yml index cc2e33cce..1899439ad 100644 --- a/.github/workflows/django_testing_ci.yml +++ b/.github/workflows/django_testing_ci.yml @@ -79,6 +79,7 @@ jobs: # Get setting configuration from samples cp coldfront/config/local_strings.py.sample coldfront/config/local_strings.py cp coldfront/config/local_settings.py.sample coldfront/config/local_settings.py + export django_secret_key=`openssl rand -base64 64` cp coldfront/config/test_settings.py.sample coldfront/config/test_settings.py - name: Run Tests diff --git a/coldfront/config/test_settings.py.sample b/coldfront/config/test_settings.py.sample index b69c3f4be..063becaa6 100644 --- a/coldfront/config/test_settings.py.sample +++ b/coldfront/config/test_settings.py.sample @@ -3,6 +3,8 @@ import os # SECURITY WARNING: don't run with debug turned on in production! DEBUG = True +SECRET_KEY = os.environ.get('django_secret_key', '') + ALLOWED_HOSTS = ['0.0.0.0', 'localhost', 'localhost'] PORTAL_NAME = 'MyBRC' From edc64f762a51ba8d7a8d7e9d42b20c60314b5ff9 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 12 May 2023 09:06:54 -0700 Subject: [PATCH 69/72] Move definition of env. var. to where it is needed --- .github/workflows/django_testing_ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/django_testing_ci.yml b/.github/workflows/django_testing_ci.yml index 1899439ad..b172a6585 100644 --- a/.github/workflows/django_testing_ci.yml +++ b/.github/workflows/django_testing_ci.yml @@ -79,12 +79,12 @@ jobs: # Get setting configuration from samples cp coldfront/config/local_strings.py.sample coldfront/config/local_strings.py cp coldfront/config/local_settings.py.sample coldfront/config/local_settings.py - export django_secret_key=`openssl rand -base64 64` cp coldfront/config/test_settings.py.sample coldfront/config/test_settings.py - name: Run Tests run: | source ~/venv/bin/activate + export django_secret_key=`openssl rand -base64 64` python manage.py migrate python manage.py test From a80573ae7ae90744a0fefed6c8c3266fbba78fc7 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Fri, 12 May 2023 14:15:33 -0700 Subject: [PATCH 70/72] Block SSO views when flag disabled; update test for third-party accounts section accordingly --- coldfront/core/socialaccount/urls.py | 46 ++++++++------ .../test_third_party_accounts.py | 60 ++++++++++++++++--- 2 files changed, 80 insertions(+), 26 deletions(-) diff --git a/coldfront/core/socialaccount/urls.py b/coldfront/core/socialaccount/urls.py index d3080f8bc..06e59322a 100644 --- a/coldfront/core/socialaccount/urls.py +++ b/coldfront/core/socialaccount/urls.py @@ -8,23 +8,33 @@ urlpatterns = [] -names_to_include = { - 'socialaccount_login_cancelled', -} -for pattern in all_patterns: - if pattern.name in names_to_include: - urlpatterns.append(pattern) - - -# Only include view for connecting additional social accounts if users are -# allowed to have multiple emails. -names_to_include_if_multiple_emails_allowed = { - 'socialaccount_connections', -} -with flagged_paths('MULTIPLE_EMAIL_ADDRESSES_ALLOWED') as f_path: +# TODO: Come up with a more elegant solution for dealing with views protected by +# multiple flags. +with flagged_paths('SSO_ENABLED') as sso_f_path: + names_to_include = { + 'socialaccount_login_cancelled', + } for pattern in all_patterns: - if pattern.name in names_to_include_if_multiple_emails_allowed: - urlpatterns.append( - f_path( + if pattern.name in names_to_include: + urlpatterns.append(sso_f_path( + str(pattern.pattern), pattern.callback, + pattern.default_args, pattern.name) + ) + + # Only include the view for connecting additional social accounts if users + # are allowed to have multiple emails. + names_to_include_if_multiple_emails_allowed = { + 'socialaccount_connections', + } + with flagged_paths('MULTIPLE_EMAIL_ADDRESSES_ALLOWED') as multi_email_f_path: + for pattern in all_patterns: + if pattern.name in names_to_include_if_multiple_emails_allowed: + # The URL is not correctly disabled unless passed through both + # context managers. + tmp_pattern = multi_email_f_path( str(pattern.pattern), pattern.callback, - pattern.default_args, pattern.name)) + pattern.default_args, pattern.name) + final_pattern = sso_f_path( + str(tmp_pattern.pattern), tmp_pattern.callback, + tmp_pattern.default_args, tmp_pattern.name) + urlpatterns.append(final_pattern) diff --git a/coldfront/core/user/tests/test_views/test_user_profile/test_third_party_accounts.py b/coldfront/core/user/tests/test_views/test_user_profile/test_third_party_accounts.py index a87256fb3..1a474bb32 100644 --- a/coldfront/core/user/tests/test_views/test_user_profile/test_third_party_accounts.py +++ b/coldfront/core/user/tests/test_views/test_user_profile/test_third_party_accounts.py @@ -27,12 +27,16 @@ def user_profile_url(): """Return the URL to the User Profile.""" return reverse('user-profile') - def test_section_disabled_if_multiple_addresses_disallowed(self): - """Test that, if the 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED' feature - flag is disabled, the section is hidden.""" - flag_name = 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED' - enable_flag(flag_name) + def test_section_disabled_if_flags_disabled(self): + """Test that, if either the 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED' + feature flag or the 'SSO_ENABLED' feature flag is disabled, the + section is hidden.""" + email_flag_name = 'MULTIPLE_EMAIL_ADDRESSES_ALLOWED' + sso_flag_name = 'SSO_ENABLED' + enable_flag(email_flag_name) + enable_flag(sso_flag_name) + # Both enabled response = self.client.get(self.user_profile_url()) self.assertContains(response, 'Third-Party Accounts') section_url = reverse('socialaccount_connections') @@ -40,13 +44,53 @@ def test_section_disabled_if_multiple_addresses_disallowed(self): response = self.client.get(section_url) self.assertEqual(response.status_code, HTTPStatus.OK) + disabled_dict = {'condition': 'boolean', 'value': False} + enabled_dict = {'condition': 'boolean', 'value': True} + + # One enabled, one disabled flags_copy = deepcopy(settings.FLAGS) - flags_copy[flag_name] = {'condition': 'boolean', 'value': False} + flags_copy[email_flag_name] = disabled_dict + flags_copy[sso_flag_name] = enabled_dict + with override_settings(FLAGS=flags_copy): + disable_flag(email_flag_name) + enable_flag(sso_flag_name) + self.assertFalse(flag_enabled(email_flag_name)) + self.assertTrue(flag_enabled(sso_flag_name)) + # The section should be hidden. + response = self.client.get(self.user_profile_url()) + self.assertNotContains(response, 'Third-Party Accounts') + self.assertNotContains(response, section_url) + # The underlying view should be inaccessible. + response = self.client.get(section_url) + self.assertEqual(response.status_code, HTTPStatus.NOT_FOUND) + + flags_copy[email_flag_name] = enabled_dict + flags_copy[sso_flag_name] = disabled_dict + with override_settings(FLAGS=flags_copy): + enable_flag(email_flag_name) + disable_flag(sso_flag_name) + self.assertTrue(flag_enabled(email_flag_name)) + self.assertFalse(flag_enabled(sso_flag_name)) + # The section should be hidden. + response = self.client.get(self.user_profile_url()) + self.assertNotContains(response, 'Third-Party Accounts') + self.assertNotContains(response, section_url) + # The underlying view should not be accessible. + response = self.client.get(section_url) + self.assertEqual(response.status_code, HTTPStatus.NOT_FOUND) + + # Both disabled + flags_copy[email_flag_name] = disabled_dict + flags_copy[sso_flag_name] = disabled_dict with override_settings(FLAGS=flags_copy): - disable_flag(flag_name) - self.assertFalse(flag_enabled(flag_name)) + disable_flag(email_flag_name) + disable_flag(sso_flag_name) + self.assertFalse(flag_enabled(email_flag_name)) + self.assertFalse(flag_enabled(sso_flag_name)) + # The section should be hidden. response = self.client.get(self.user_profile_url()) self.assertNotContains(response, 'Third-Party Accounts') self.assertNotContains(response, section_url) + # The underlying view should not be accessible. response = self.client.get(section_url) self.assertEqual(response.status_code, HTTPStatus.NOT_FOUND) From 2848bd3b35f7d1754f3b4b6b26d9747f703c1cb5 Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Mon, 15 May 2023 10:12:58 -0700 Subject: [PATCH 71/72] Add (temporary) explanatory alert to BRC SSO login page --- .../user/templates/user/sso_login_brc.html | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/coldfront/core/user/templates/user/sso_login_brc.html b/coldfront/core/user/templates/user/sso_login_brc.html index 83430a5a8..7d286f2b6 100644 --- a/coldfront/core/user/templates/user/sso_login_brc.html +++ b/coldfront/core/user/templates/user/sso_login_brc.html @@ -13,6 +13,28 @@

    Log In: I am a...

    + +
    +
    +
    +
    + +
    +
    +
    From 21548a8b44e067210caccf51f801ba738bca0d2a Mon Sep 17 00:00:00 2001 From: Matthew Li Date: Wed, 17 May 2023 11:36:20 -0700 Subject: [PATCH 72/72] Send 10% of errors, 0.1% of transactions to Sentry --- bootstrap/ansible/settings_template.tmpl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bootstrap/ansible/settings_template.tmpl b/bootstrap/ansible/settings_template.tmpl index 00735585a..279b0b7fc 100644 --- a/bootstrap/ansible/settings_template.tmpl +++ b/bootstrap/ansible/settings_template.tmpl @@ -110,7 +110,8 @@ if SENTRY_DSN: sentry_sdk.init( dsn=SENTRY_DSN, integrations=[DjangoIntegration()], - traces_sample_rate=0.01, + sample_rate=0.1, + traces_sample_rate=0.001, send_default_pii=True) # Ignore noisy loggers. ignore_logger('coldfront.api')