diff --git a/api/users/serializers.py b/api/users/serializers.py index 6f2cfc7e1cd..85c8ed3bc72 100644 --- a/api/users/serializers.py +++ b/api/users/serializers.py @@ -696,6 +696,15 @@ class UserMessageSerializer(JSONAPISerializer): related_view_kwargs={'institution_id': ''}, help_text='The institution associated with this message.', ) + cc = ser.BooleanField( + required=False, + default=False, + help_text='If true, CCs the sender.', + ) + reply_to = ser.BooleanField( + default=False, + help_text='Whether to set the sender\'s username as the `Reply-To` header in the email.', + ) def get_absolute_url(self, obj: UserMessage) -> str: return api_v2_url( @@ -756,13 +765,17 @@ def create(self, validated_data: Dict[str, Any]) -> UserMessage: if not recipient.is_affiliated_with_institution(institution): raise Conflict( - {'user': 'Can not send to recipient that is not affiliated with the provided institution.'}, + {'user': 'Cannot send to a recipient that is not affiliated with the provided institution.'}, ) - return UserMessage.objects.create( + user_message = UserMessage.objects.create( sender=sender, recipient=recipient, institution=institution, message_type=MessageTypes.INSTITUTIONAL_REQUEST, message_text=validated_data['message_text'], + is_sender_CCed=validated_data['cc'], + reply_to=validated_data['reply_to'], ) + user_message.save() + return user_message diff --git a/api_tests/users/views/test_user_message_institutional_access.py b/api_tests/users/views/test_user_message_institutional_access.py index 2da69424f76..79966be9adc 100644 --- a/api_tests/users/views/test_user_message_institutional_access.py +++ b/api_tests/users/views/test_user_message_institutional_access.py @@ -6,6 +6,8 @@ AuthUserFactory, InstitutionFactory ) +from website.mails import USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST + @pytest.mark.django_db class TestUserMessageInstitutionalAccess: @@ -151,5 +153,95 @@ def test_admin_cannot_message_user_outside_institution( """ res = app.post_json_api(url_without_affiliation, payload, auth=institutional_admin.auth, expect_errors=True) assert res.status_code == 409 - assert 'Can not send to recipient that is not affiliated with the provided institution.'\ - in res.json['errors'][0]['detail']['user'] + assert ('Cannot send to a recipient that is not affiliated with the provided institution.' + in res.json['errors'][0]['detail']['user']) + + @mock.patch('osf.models.user_message.send_mail') + def test_cc_institutional_admin( + self, + mock_send_mail, + app, + institutional_admin, + institution, + url_with_affiliation, + user_with_affiliation, + payload + ): + """ + Ensure CC option works as expected, sending messages to all institutional admins except the sender. + """ + mock_send_mail.return_value = mock.MagicMock() + + # Enable CC in the payload + payload['data']['attributes']['cc'] = True + + # Perform the API request + res = app.post_json_api( + url_with_affiliation, + payload, + auth=institutional_admin.auth, + ) + assert res.status_code == 201 + user_message = UserMessage.objects.get() + + assert user_message.is_sender_CCed + # Two emails are sent during the CC but this is how the mock works `send_email` is called once. + mock_send_mail.assert_called_once_with( + to_addr=user_with_affiliation.username, + cc_addr=[institutional_admin.username], + reply_to=None, + message_text='Requesting user access for collaboration', + mail=USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST, + user=user_with_affiliation, + sender=institutional_admin, + recipient=user_with_affiliation, + institution=institution, + ) + + @mock.patch('osf.models.user_message.send_mail') + def test_cc_field_defaults_to_false(self, mock_send_mail, app, institutional_admin, url_with_affiliation, user_with_affiliation, institution, payload): + """ + Ensure the `cc` field defaults to `false` when not provided in the payload. + """ + res = app.post_json_api(url_with_affiliation, payload, auth=institutional_admin.auth) + assert res.status_code == 201 + + user_message = UserMessage.objects.get(sender=institutional_admin) + assert user_message.message_text == payload['data']['attributes']['message_text'] + mock_send_mail.assert_called_once_with( + to_addr=user_with_affiliation.username, + cc_addr=None, + reply_to=None, + message_text='Requesting user access for collaboration', + mail=USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST, + user=user_with_affiliation, + sender=institutional_admin, + recipient=user_with_affiliation, + institution=institution, + ) + + @mock.patch('osf.models.user_message.send_mail') + def test_reply_to_header_set(self, mock_send_mail, app, institutional_admin, user_with_affiliation, institution, url_with_affiliation, payload): + """ + Ensure that the 'Reply-To' header is correctly set to the sender's email address. + """ + payload['data']['attributes']['reply_to'] = True + + res = app.post_json_api( + url_with_affiliation, + payload, + auth=institutional_admin.auth, + ) + assert res.status_code == 201 + + mock_send_mail.assert_called_once_with( + to_addr=user_with_affiliation.username, + cc_addr=None, + reply_to=institutional_admin.username, + message_text='Requesting user access for collaboration', + mail=USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST, + user=user_with_affiliation, + sender=institutional_admin, + recipient=user_with_affiliation, + institution=institution, + ) diff --git a/framework/email/tasks.py b/framework/email/tasks.py index bb2528fd1d3..f6087f19b71 100644 --- a/framework/email/tasks.py +++ b/framework/email/tasks.py @@ -20,8 +20,10 @@ def send_email( to_addr: str, subject: str, message: str, + reply_to: bool = False, ttls: bool = True, login: bool = True, + cc_addr: [] = None, username: str = None, password: str = None, categories=None, @@ -57,6 +59,8 @@ def send_email( categories=categories, attachment_name=attachment_name, attachment_content=attachment_content, + reply_to=reply_to, + cc_addr=cc_addr, ) else: return _send_with_smtp( @@ -68,35 +72,61 @@ def send_email( login=login, username=username, password=password, + reply_to=reply_to, + cc_addr=cc_addr, ) -def _send_with_smtp(from_addr, to_addr, subject, message, ttls=True, login=True, username=None, password=None): +def _send_with_smtp( + from_addr, + to_addr, + subject, + message, + ttls=True, + login=True, + username=None, + password=None, + cc_addr=None, + reply_to=None, +): username = username or settings.MAIL_USERNAME password = password or settings.MAIL_PASSWORD if login and (username is None or password is None): logger.error('Mail username and password not set; skipping send.') - return + return False - msg = MIMEText(message, 'html', _charset='utf-8') + msg = MIMEText( + message, + 'html', + _charset='utf-8', + ) msg['Subject'] = subject msg['From'] = from_addr msg['To'] = to_addr - s = smtplib.SMTP(settings.MAIL_SERVER) - s.ehlo() - if ttls: - s.starttls() - s.ehlo() - if login: - s.login(username, password) - s.sendmail( - from_addr=from_addr, - to_addrs=[to_addr], - msg=msg.as_string(), - ) - s.quit() + if cc_addr: + msg['Cc'] = ', '.join(cc_addr) + + if reply_to: + msg['Reply-To'] = reply_to + + # Combine recipients for SMTP + recipients = [to_addr] + (cc_addr or []) + + # Establish SMTP connection and send the email + with smtplib.SMTP(settings.MAIL_SERVER) as server: + server.ehlo() + if ttls: + server.starttls() + server.ehlo() + if login: + server.login(username, password) + server.sendmail( + from_addr=from_addr, + to_addrs=recipients, + msg=msg.as_string() + ) return True @@ -108,39 +138,59 @@ def _send_with_sendgrid( categories=None, attachment_name: str = None, attachment_content=None, + cc_addr=None, + reply_to=None, client=None, ): - if ( - settings.SENDGRID_WHITELIST_MODE and to_addr in settings.SENDGRID_EMAIL_WHITELIST - ) or settings.SENDGRID_WHITELIST_MODE is False: - client = client or SendGridAPIClient(settings.SENDGRID_API_KEY) - mail = Mail(from_email=from_addr, html_content=message, to_emails=to_addr, subject=subject) - if categories: - mail.category = [Category(x) for x in categories] - if attachment_name and attachment_content: - content_bytes = _content_to_bytes(attachment_content) - content_bytes = FileContent(b64encode(content_bytes).decode()) - attachment = Attachment(file_content=content_bytes, file_name=attachment_name) - mail.add_attachment(attachment) - - response = client.send(mail) - if response.status_code >= 400: - sentry.log_message( - f'{response.status_code} error response from sendgrid.' - f'from_addr: {from_addr}\n' - f'to_addr: {to_addr}\n' - f'subject: {subject}\n' - 'mimetype: html\n' - f'message: {response.body[:30]}\n' - f'categories: {categories}\n' - f'attachment_name: {attachment_name}\n' - ) - return response.status_code < 400 - else: + in_allowed_list = to_addr in settings.SENDGRID_EMAIL_WHITELIST + if settings.SENDGRID_WHITELIST_MODE and not in_allowed_list: sentry.log_message( f'SENDGRID_WHITELIST_MODE is True. Failed to send emails to non-whitelisted recipient {to_addr}.' ) + return False + + client = client or SendGridAPIClient(settings.SENDGRID_API_KEY) + mail = Mail( + from_email=from_addr, + html_content=message, + to_emails=to_addr, + subject=subject + ) + + if reply_to: # Add Reply-To header if provided + mail.reply_to = [{'email': reply_to}] + + if cc_addr: # Add CC header if CC addresses exist + mail.add_cc([{'email': email} for email in cc_addr]) + if categories: + mail.category = [Category(x) for x in categories] + + if attachment_name and attachment_content: + attachment = Attachment( + file_content=_content_to_bytes( + FileContent( + b64encode(content_bytes).decode() + ) + ), + file_name=attachment_name + ) + mail.add_attachment(attachment) + + response = client.send(mail) + if response.status_code not in (200, 201): + sentry.log_message( + f'{response.status_code} error response from sendgrid.' + f'from_addr: {from_addr}\n' + f'to_addr: {to_addr}\n' + f'subject: {subject}\n' + 'mimetype: html\n' + f'message: {response.body[:30]}\n' + f'categories: {categories}\n' + f'attachment_name: {attachment_name}\n' + ) + else: + return True def _content_to_bytes(attachment_content: BytesIO | str | bytes) -> bytes: if isinstance(attachment_content, bytes): diff --git a/osf/migrations/0025_usermessage.py b/osf/migrations/0025_usermessage.py index 4d92830df2d..456d77b555d 100644 --- a/osf/migrations/0025_usermessage.py +++ b/osf/migrations/0025_usermessage.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.13 on 2024-12-06 21:31 +# Generated by Django 4.2.13 on 2024-12-09 16:34 from django.conf import settings from django.db import migrations, models @@ -23,6 +23,8 @@ class Migration(migrations.Migration): ('_id', models.CharField(db_index=True, default=osf.models.base.generate_object_id, max_length=24, unique=True)), ('message_text', models.TextField(help_text='The content of the message. The custom text of a formatted email.')), ('message_type', models.CharField(choices=[('institutional_request', 'INSTITUTIONAL_REQUEST')], help_text='The type of message being sent, as defined in MessageTypes.', max_length=50)), + ('is_sender_CCed', models.BooleanField(default=False, help_text='The boolean value that indicates whether other institutional admins were CCed')), + ('reply_to', models.BooleanField(default=False, help_text="Whether to set the sender's username as the `Reply-To` header in the email.")), ('institution', models.ForeignKey(help_text='The institution associated with this message.', on_delete=django.db.models.deletion.CASCADE, to='osf.institution')), ('recipient', models.ForeignKey(help_text='The recipient of this message.', on_delete=django.db.models.deletion.CASCADE, related_name='received_user_messages', to=settings.AUTH_USER_MODEL)), ('sender', models.ForeignKey(help_text='The user who sent this message.', on_delete=django.db.models.deletion.CASCADE, related_name='sent_user_messages', to=settings.AUTH_USER_MODEL)), diff --git a/osf/models/user_message.py b/osf/models/user_message.py index f6543c102ee..68ad2250bed 100644 --- a/osf/models/user_message.py +++ b/osf/models/user_message.py @@ -2,6 +2,7 @@ from django.db import models from django.db.models.signals import post_save from django.dispatch import receiver + from .base import BaseModel, ObjectIDMixin from website.mails import send_mail, USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST @@ -70,14 +71,21 @@ class UserMessage(BaseModel, ObjectIDMixin): on_delete=models.CASCADE, help_text='The institution associated with this message.' ) + is_sender_CCed = models.BooleanField( + default=False, + help_text='The boolean value that indicates whether other institutional admins were CCed', + ) + reply_to = models.BooleanField( + default=False, + help_text='Whether to set the sender\'s username as the `Reply-To` header in the email.' + ) - def send_institution_request(self) -> None: - """ - Sends an institutional access request email to the recipient of the message. - """ + def send_institution_request(self): send_mail( to_addr=self.recipient.username, - mail=MessageTypes.get_template(MessageTypes.INSTITUTIONAL_REQUEST), + cc_addr=[self.sender.username] if self.is_sender_CCed else None, + reply_to=self.sender.username if self.reply_to else None, + mail=MessageTypes.get_template(self.message_type), user=self.recipient, **{ 'sender': self.sender, diff --git a/website/mails/mails.py b/website/mails/mails.py index 2e468acbf81..631b28d956c 100644 --- a/website/mails/mails.py +++ b/website/mails/mails.py @@ -76,17 +76,29 @@ def render_message(tpl_name, **context): def send_mail( - to_addr, mail, from_addr=None, mailer=None, celery=True, - username=None, password=None, callback=None, attachment_name=None, - attachment_content=None, **context): - """Send an email from the OSF. - Example: :: - + to_addr, + mail, + from_addr=None, + cc_addr=None, + reply_to=None, + mailer=None, + celery=True, + username=None, + password=None, + callback=None, + attachment_name=None, + attachment_content=None, + **context): + """ + Send an email from the OSF. + Example: from website import mails mails.send_email('foo@bar.com', mails.TEST, name="Foo") :param str to_addr: The recipient's email address + :param str cc_addr: The CC senders's email address (or list of addresses) + :param str reply_to: The sender's email address will appear in the reply-to header :param Mail mail: The mail object :param str mimetype: Either 'plain' or 'html' :param function callback: celery task to execute after send_mail completes @@ -119,6 +131,8 @@ def send_mail( categories=mail.categories, attachment_name=attachment_name, attachment_content=attachment_content, + cc_addr=cc_addr, + reply_to=reply_to, ) logger.debug('Preparing to send...')