Initial mfa support (for internal users) #93591

Merged
Oleg-Komarov merged 46 commits from mfa into main 2024-08-29 11:44:06 +02:00
13 changed files with 45 additions and 27 deletions
Showing only changes of commit 585dc35274 - Show all commits

View File

@ -287,6 +287,7 @@ def construct_password_changed(user):
def construct_mfa_new_device(user, device_type): def construct_mfa_new_device(user, device_type):
context = { context = {
"device_type": device_type, "device_type": device_type,
"support_email": settings.SUPPORT_EMAIL,
"user": user, "user": user,
} }
email_body_txt = loader.render_to_string( email_body_txt = loader.render_to_string(
@ -311,6 +312,7 @@ def construct_mfa_disabled(user):
def construct_mfa_recovery_used(user): def construct_mfa_recovery_used(user):
context = { context = {
"support_email": settings.SUPPORT_EMAIL,
"user": user, "user": user,
} }
email_body_txt = loader.render_to_string( email_body_txt = loader.render_to_string(

View File

@ -316,7 +316,7 @@ class PasswordChangeForm(BootstrapModelFormMixin, auth_forms.PasswordChangeForm)
def save(self, *args, **kwargs): def save(self, *args, **kwargs):
user = super().save(*args, **kwargs) user = super().save(*args, **kwargs)
if user.has_confirmed_email: if user.has_confirmed_email:
bid_main.tasks.send_password_changed_email(user_pk=user.pk) bid_main.tasks.send_mail_password_changed(user_pk=user.pk)
return user return user
@ -335,7 +335,7 @@ class SetPasswordForm(auth_forms.SetPasswordForm):
def save(self, *args, **kwargs): def save(self, *args, **kwargs):
user = super().save(*args, **kwargs) user = super().save(*args, **kwargs)
if user.has_confirmed_email: if user.has_confirmed_email:
bid_main.tasks.send_password_changed_email(user_pk=user.pk) bid_main.tasks.send_mail_password_changed(user_pk=user.pk)
return user return user

View File

@ -39,7 +39,7 @@ def process_new_login(sender, request, user, **kwargs):
fields.update({"last_login_ip", "current_login_ip"}) fields.update({"last_login_ip", "current_login_ip"})
if user.has_confirmed_email: if user.has_confirmed_email:
bid_main.tasks.send_new_user_session_email( bid_main.tasks.send_mail_new_user_session(
user_pk=user.pk, user_pk=user.pk,
session_data={ session_data={
'device': str(user_session.device or 'Unknown'), 'device': str(user_session.device or 'Unknown'),
@ -83,7 +83,7 @@ def delete_orphaned_avatar_files(sender, instance, **kwargs):
@receiver(recovery_used) @receiver(recovery_used)
def send_mfa_recovery_used_email(sender, **kwargs): def send_mail_mfa_recovery_used(sender, **kwargs):
user = kwargs['device'].user user = kwargs['device'].user
if user.confirmed_email_at: if user.confirmed_email_at:
bid_main.tasks.send_mfa_recovery_used_email(user.pk) bid_main.tasks.send_mail_mfa_recovery_used(user.pk)

View File

@ -12,7 +12,7 @@ log = logging.getLogger(__name__)
@background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING})
def send_new_user_session_email(user_pk, session_data): def send_mail_new_user_session(user_pk, session_data):
user = User.objects.get(pk=user_pk) user = User.objects.get(pk=user_pk)
log.info("sending a new user session email for account %s", user.pk) log.info("sending a new user session email for account %s", user.pk)
@ -29,7 +29,7 @@ def send_new_user_session_email(user_pk, session_data):
@background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING})
def send_password_changed_email(user_pk): def send_mail_password_changed(user_pk):
user = User.objects.get(pk=user_pk) user = User.objects.get(pk=user_pk)
log.info("sending a password change email for account %s", user.pk) log.info("sending a password change email for account %s", user.pk)
@ -46,7 +46,7 @@ def send_password_changed_email(user_pk):
@background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING})
def send_mfa_new_device_email(user_pk, device_type): def send_mail_mfa_new_device(user_pk, device_type):
user = User.objects.get(pk=user_pk) user = User.objects.get(pk=user_pk)
log.info("sending a new mfa device email for account %s", user.pk) log.info("sending a new mfa device email for account %s", user.pk)
@ -63,7 +63,7 @@ def send_mfa_new_device_email(user_pk, device_type):
@background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING})
def send_mfa_disabled_email(user_pk): def send_mail_mfa_disabled(user_pk):
user = User.objects.get(pk=user_pk) user = User.objects.get(pk=user_pk)
log.info("sending an mfa disabled email for account %s", user.pk) log.info("sending an mfa disabled email for account %s", user.pk)
@ -80,7 +80,7 @@ def send_mfa_disabled_email(user_pk):
@background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING})
def send_mfa_recovery_used_email(user_pk): def send_mail_mfa_recovery_used(user_pk):
user = User.objects.get(pk=user_pk) user = User.objects.get(pk=user_pk)
log.info("sending an mfa recovery used email for account %s", user.pk) log.info("sending an mfa recovery used email for account %s", user.pk)

View File

@ -3,7 +3,7 @@ Dear {{ user.full_name|default:user.email }}!
A new {{ device_type }} multi-factor authenticator has been added to your Blender ID account {{ user.email }} A new {{ device_type }} multi-factor authenticator has been added to your Blender ID account {{ user.email }}
If this wasn't done by you, please reset your password immediately and contact blenderid@blender.org for support. If this wasn't done by you, please reset your password immediately and contact {{ support_email }} for support.
Oleg-Komarov marked this conversation as resolved Outdated

might be out of scope, but setting an ADMIN_EMAIL (like in DevFund) or SUPPORT_EMAIL (not to be confused with builtin settings.ADMINS) configuration variable and passing it to the templates that need it is more maintainable than hard-coding it in multiple files.

might be out of scope, but setting an `ADMIN_EMAIL` (like in DevFund) or `SUPPORT_EMAIL` (not to be confused with builtin `settings.ADMINS`) configuration variable and passing it to the templates that need it is more maintainable than hard-coding it in multiple files.
-- --
Kind regards, Kind regards,

View File

@ -3,7 +3,7 @@ Dear {{ user.full_name|default:user.email }}!
A recovery code was used to pass multi-factor authentication for your Blender ID account {{ user.email }} A recovery code was used to pass multi-factor authentication for your Blender ID account {{ user.email }}
If this wasn't done by you, please reset your password immediately, re-generate your MFA recovery codes, and contact blenderid@blender.org for support. If this wasn't done by you, please reset your password immediately, re-generate your MFA recovery codes, and contact {{ support_email }} for support.
-- --
Kind regards, Kind regards,

View File

@ -60,7 +60,10 @@ Multi-factor Authentication Setup
<div class="bid box mt-3"> <div class="bid box mt-3">
<h3>Security keys (U2F, WebAuthn, FIDO2)</h3> <h3>Security keys (U2F, WebAuthn, FIDO2)</h3>
<p> <p>
E.g. a yubikey. Hardware security keys, e.g. Yubikeys.
</p>
<p>
Blender ID supports these keys only as a second factor and <strong>does not</strong> provide a passwordless sign-in.
</p> </p>
<ul> <ul>
{% for d in devices_per_type.u2f %} {% for d in devices_per_type.u2f %}
@ -79,7 +82,7 @@ Multi-factor Authentication Setup
<p> <p>
Oleg-Komarov marked this conversation as resolved
Review

will become invalidated.

"will be invalided" or "will become invalid"

> will become invalidated. "will be invalided" or "will become invalid"
Store your recovery codes safely (e.g. in a password manager or use a printed copy) and don't share them. Store your recovery codes safely (e.g. in a password manager or use a printed copy) and don't share them.
Each code can be used only once. Each code can be used only once.
You can generate a new set of recovery codes at any time, any remaining old codes will become invalidated. You can generate a new set of recovery codes at any time, any remaining old codes will be invalidated.
</p> </p>
{% with recovery=devices_per_type.recovery.0 %} {% with recovery=devices_per_type.recovery.0 %}
{% if recovery %} {% if recovery %}

View File

@ -8,6 +8,14 @@ Multi-factor Authentication Setup
{% block body %} {% block body %}
<div class="bid box"> <div class="bid box">
<h2>New U2F device</h2> <h2>New U2F device</h2>
<div class="row">
<div class="col-md-12">
<p>Please watch <a href="https://www.youtube.com/watch?v=V6mxPS5O-sY">setup video</a> if you are not familiar with yubikeys.</p>
{% if first_device %}
<p>Since this is your first MFA device, you will be promted to use your security key immediately after setup to sign-in using MFA.</p>
{% endif %}
</div>
</div>
<div class="row"> <div class="row">
<div class="col-md-6"> <div class="col-md-6">
{% with form=form|add_form_classes %} {% with form=form|add_form_classes %}

View File

@ -12,8 +12,8 @@ import bid_main.tasks
@patch( @patch(
'bid_main.tasks.send_password_changed_email', 'bid_main.tasks.send_mail_password_changed',
new=bid_main.tasks.send_password_changed_email.task_function, new=bid_main.tasks.send_mail_password_changed.task_function,
) )
@patch( @patch(
'django.contrib.auth.base_user.AbstractBaseUser.check_password', 'django.contrib.auth.base_user.AbstractBaseUser.check_password',

View File

@ -50,8 +50,8 @@ class TestActiveSessions(TestCase):
class TestNewUserSessionEmail(TestCase): class TestNewUserSessionEmail(TestCase):
@patch( @patch(
'bid_main.tasks.send_new_user_session_email', 'bid_main.tasks.send_mail_new_user_session',
new=bid_main.tasks.send_new_user_session_email.task_function, new=bid_main.tasks.send_mail_new_user_session.task_function,
) )
@patch( @patch(
'django.contrib.auth.base_user.AbstractBaseUser.check_password', 'django.contrib.auth.base_user.AbstractBaseUser.check_password',

View File

@ -69,7 +69,7 @@ class DisableView(mixins.MfaRequiredMixin, FormView):
for device in devices_for_user(self.request.user): for device in devices_for_user(self.request.user):
device.delete() device.delete()
if self.request.user.confirmed_email_at: if self.request.user.confirmed_email_at:
bid_main.tasks.send_mfa_disabled_email(self.request.user.pk) bid_main.tasks.send_mail_mfa_disabled(self.request.user.pk)
Oleg-Komarov marked this conversation as resolved Outdated

DevFund and other services use send_mail_* for the most part: it's easier to parse visually

DevFund and other services use `send_mail_*` for the most part: it's easier to parse visually
return super().form_valid(form) return super().form_valid(form)
@ -82,7 +82,7 @@ class GenerateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View):
): ):
# Forbid setting up recovery codes unless the user already has some other method # Forbid setting up recovery codes unless the user already has some other method
return HttpResponseBadRequest("can't setup recovery codes before other methods") return HttpResponseBadRequest("can't setup recovery codes before other methods")
user.staticdevice_set.all().delete() EncryptedRecoveryDevice.objects.filter(user=user).delete()
Oleg-Komarov marked this conversation as resolved Outdated

From this line it's not clear that this is recovery codes that are being deleted

From this line it's not clear that this is recovery codes that are being deleted
device = EncryptedRecoveryDevice.objects.create(name='recovery', user=user) device = EncryptedRecoveryDevice.objects.create(name='recovery', user=user)
for _ in range(10): for _ in range(10):
# https://pages.nist.gov/800-63-3/sp800-63b.html#5122-look-up-secret-verifiers # https://pages.nist.gov/800-63-3/sp800-63b.html#5122-look-up-secret-verifiers
@ -91,10 +91,10 @@ class GenerateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View):
return redirect(reverse('bid_main:mfa') + '?display_recovery_codes=1#recovery-codes') return redirect(reverse('bid_main:mfa') + '?display_recovery_codes=1#recovery-codes')
class InvalidateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): class InvalidateRecoveryView(mixins.MfaRequiredMixin, View):
def post(self, request, *args, **kwargs): def post(self, request, *args, **kwargs):
user = self.request.user user = self.request.user
user.staticdevice_set.all().delete() EncryptedRecoveryDevice.objects.filter(user=user).delete()
Oleg-Komarov marked this conversation as resolved Outdated

same as above

same as above
return redirect('bid_main:mfa') return redirect('bid_main:mfa')
@ -132,7 +132,7 @@ class TotpRegisterView(mixins.MfaRequiredIfConfiguredMixin, FormView):
def form_valid(self, form): def form_valid(self, form):
form.save() form.save()
if self.request.user.confirmed_email_at: if self.request.user.confirmed_email_at:
bid_main.tasks.send_mfa_new_device_email(self.request.user.pk, 'totp') bid_main.tasks.send_mail_mfa_new_device(self.request.user.pk, 'totp')
return super().form_valid(form) return super().form_valid(form)
@ -141,6 +141,11 @@ class U2fRegisterView(mixins.MfaRequiredIfConfiguredMixin, FormView):
success_url = reverse_lazy('bid_main:mfa') success_url = reverse_lazy('bid_main:mfa')
template_name = "bid_main/mfa/u2f_register.html" template_name = "bid_main/mfa/u2f_register.html"
Oleg-Komarov marked this conversation as resolved
Review

is context['first_device'] = not devices_for_user(self.request.user) necessary here as well?

is `context['first_device'] = not devices_for_user(self.request.user)` necessary here as well?
def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
context['first_device'] = not devices_for_user(self.request.user)
return context
def get_form_kwargs(self): def get_form_kwargs(self):
credentials = [ credentials = [
AttestedCredentialData(d.credential) AttestedCredentialData(d.credential)
@ -161,7 +166,7 @@ class U2fRegisterView(mixins.MfaRequiredIfConfiguredMixin, FormView):
def form_valid(self, form): def form_valid(self, form):
form.save() form.save()
if self.request.user.confirmed_email_at: if self.request.user.confirmed_email_at:
bid_main.tasks.send_mfa_new_device_email(self.request.user.pk, 'u2f') bid_main.tasks.send_mail_mfa_new_device(self.request.user.pk, 'u2f')
return super().form_valid(form) return super().form_valid(form)

View File

@ -428,3 +428,6 @@ if os.environ.get('ADMINS') is not None:
ADMINS = [[_.strip() for _ in adm.split(':')] for adm in os.environ.get('ADMINS').split(',')] ADMINS = [[_.strip() for _ in adm.split(':')] for adm in os.environ.get('ADMINS').split(',')]
EMAIL_SUBJECT_PREFIX = f'[{ALLOWED_HOSTS[0]}]' EMAIL_SUBJECT_PREFIX = f'[{ALLOWED_HOSTS[0]}]'
SERVER_EMAIL = f'django@{ALLOWED_HOSTS[0]}' SERVER_EMAIL = f'django@{ALLOWED_HOSTS[0]}'
SUPPORT_EMAIL = 'blenderid@blender.org'

View File

@ -132,9 +132,6 @@ class U2fAuthenticateForm(OTPAgentFormMixin, forms.Form):
self.clean_agent() self.clean_agent()
return self.cleaned_data return self.cleaned_data
def save(self):
pass
Oleg-Komarov marked this conversation as resolved
Review

might not be necessary at all, since this isn't a ModelForm?

might not be necessary at all, since this isn't a `ModelForm`?
class DisableMfaForm(forms.Form): class DisableMfaForm(forms.Form):
disable_mfa_confirm = forms.BooleanField( disable_mfa_confirm = forms.BooleanField(