Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENG-6682] Add reply-to and cc-ing features to Institutional Access #10841

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion api/users/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,15 @@ class UserMessageSerializer(JSONAPISerializer):
related_view_kwargs={'institution_id': '<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(
Expand Down Expand Up @@ -756,7 +765,7 @@ 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(
Expand All @@ -765,4 +774,6 @@ def create(self, validated_data: Dict[str, Any]) -> UserMessage:
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'],
)
96 changes: 94 additions & 2 deletions api_tests/users/views/test_user_message_institutional_access.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
AuthUserFactory,
InstitutionFactory
)
from website.mails import USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST


@pytest.mark.django_db
class TestUserMessageInstitutionalAccess:
Expand Down Expand Up @@ -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],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be handled later on, but in the case of a bcc, both the sender and the recipient should be bcc'd, otherwise the sender will be able to see the recipient's email address.

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,
)
136 changes: 93 additions & 43 deletions framework/email/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand All @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check with product: How do they want the To and CC handled to avoid giving someone's email address away? The way this appears to be implemented, the recipients will both see the other's email address. In the case of a cc:, it might be that you want to bcc: both recipients.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah Product agrees BCC is better.


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


Expand All @@ -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(attachment_content).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):
Expand Down
4 changes: 3 additions & 1 deletion osf/migrations/0025_usermessage.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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)),
Expand Down
13 changes: 12 additions & 1 deletion osf/models/user_message.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -70,14 +71,24 @@ 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.
"""
send_mail(
mail=MessageTypes.get_template(self.message_type),
to_addr=self.recipient.username,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, in the case of BCC'ing the sender, the recipient also needs to be BCC'd. There would be no to_address in that case. Without the BCC, however, it makes sense to have the recipient be in the to_addr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a wrapper for the the two different email utilities that implement the BBC in different ways, so it's hard to say which behavior is actually "the best".

But anyway, if there's no to_addr and just bbc-ing, yes that can just BBC I just have to that meld with the two utility functions, seems reasonable from a developer experience perceptive to adapt that behavior to SendGrid and simple SMTP, but it's just making the send_email wrapper around SMTP and Sendgrid better if I understand correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not super-concerned where it's changed, just that all recipients of the email are bcc'd if the bcc option is chosen.

Copy link
Contributor Author

@Johnetordoff Johnetordoff Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I dealt with that here:

recipients = [to_addr] + (bcc_addr or [])

https://github.com/CenterForOpenScience/osf.io/pull/10841/files#diff-78b71751c095a6eb9a92be212c4acf17f402fc34266030b5440596cf2a57d243R112 sorry probably got lost with the clean-up, but that's new. Sendgrid has it's own way of handling it.

SMTP BCCs by just not CCing, so no additional action is necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool cool

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,
user=self.recipient,
**{
'sender': self.sender,
Expand Down
Loading
Loading