feat(api): Email non-compliant members when an organization is 2fa enforced #6877
Conversation
Security concerns found
Generated by 🚫 danger |
|
|
|
|
benvinegar
changed the title
|
@lauryndbrown what is the behavior when you click any of the links below the button? Do Notification Settings and Unsubscribe --> personal account settings, while Home goes to the dashboard of the last project they were looking at (or back to the 2FA setup page if the project was part of the 2FA-mandatory org)? |
src/sentry/models/organization.py
Outdated
| from sentry.models import Authenticator | ||
|
|
||
| # TODO(dcramer): pull in enum library | ||
|
|
There was a problem hiding this comment.
nit: remove these extra empty lines, not sure why they got added.
| data=organization.get_audit_log_data(), | ||
| ) | ||
|
|
||
| result = serializer.object |
There was a problem hiding this comment.
Do we want to trigger this from the endpoint itself?
My concern with this is it's potentially noisy? Like, if a user toggled it on/off/on, we'd be blasting out emails to everyone multiple times.
It's probably fine for now though until someone complains or abuses it.
We should mention in the UI what will happen when this is enabled though. Something like what GitHub says:
Members, billing managers, and outside collaborators who do not have two-factor authentication enabled for their personal account will be removed from the organization and will receive an email notifying them about the change.
Just so it's clear that we'll be emailing users when this happens and they will be locked out.
There was a problem hiding this comment.
Side note, it feels like this should go under OrganizationSerializer.save() instead.
There was a problem hiding this comment.
@mattrobenolt I was wondering about that. I noticed a few places in the code where something similar was done. That said, I can put it anywhere. Is the ideal place the serializer save? I hadn't seen any places do it there.
src/sentry/models/organization.py
Outdated
| from sentry import options | ||
| from sentry.utils.email import MessageBuilder | ||
|
|
||
| for member in self.member_set.all(): |
There was a problem hiding this comment.
Let's change this queryset a bit:
users = User.objects.filter(
is_active=True,
sentry_orgmember_set__organization=self,
)So this does a few things:
- We're querying directly for User instead of OrganizationMember, then dipping into member.user.
- Not all OrganizationMember rows are actually active. Some of them have
NULLforuser_idwhich would result in anObjectDoesNotExistbeing raised when you accessedmember.user. This is the case when there are outstanding pending user invites. - Limits it to actually active user accounts, and not disabled ones.
There was a problem hiding this comment.
That's really good insight! Thanks! Can do.
| projects under the {{ organization.name }} organization unless you enable at | ||
| least one form of two-factor authentication. | ||
|
|
||
| Click the link below to enable two-factor authentication. |
There was a problem hiding this comment.
For plain text, there is no "click".
There was a problem hiding this comment.
Also not sure why the entire thing is padded out 4 spaces? Do we have a precedent for this somewhere? I'd just drop that.
|
@saragilford I'm not sure I understand the question. The images you are seeing above are what the emails will look like when sent. The button below is a link to the 2fa configuration page. |
|
What happens when you click any of these links ^ ? @lauryndbrown |
|
@saragilford this is the debug view, and it has some default behaviors. Those links on debug view don't do much. In a real email, I'm not sure. I believe anyone on the growth or perhaps workflow could answer your questions. Those links are not part of this pull request. |
|
👍 |
|
@mattrobenolt I should have addressed all your previous concerns. Please let me know what you think. |
When a manager/owner enforces 2FA in an organization, the members without 2FA enabled are notified via email. Currently this emails all active members without 2FA and is triggered by setting the organization's require_2fa settings flag to True via the api endpoint.