From b03af24338dae9887ef3cef3085cddcc3bef9bc6 Mon Sep 17 00:00:00 2001 From: Spitap Date: Wed, 21 Jun 2023 13:45:39 +0200 Subject: [PATCH] Added password check to modify TFA setup --- frontend/src/api/account.js | 8 +- .../src/components/user/TwoFactorAuthForm.vue | 89 ++++++++++++++++--- frontend/src/plugins/vee-validate.js | 7 ++ modoboa/core/api/v1/serializers.py | 22 +++++ modoboa/core/api/v1/viewsets.py | 22 ++++- modoboa/core/api/v2/tests.py | 38 ++++++-- modoboa/core/api/v2/viewsets.py | 2 +- .../core/templates/core/user_security.html | 83 ++++++++++++++++- modoboa/static/js/twocols_nav.js | 24 ----- 9 files changed, 243 insertions(+), 52 deletions(-) diff --git a/frontend/src/api/account.js b/frontend/src/api/account.js index e9617d813..b0c54554e 100644 --- a/frontend/src/api/account.js +++ b/frontend/src/api/account.js @@ -23,11 +23,11 @@ export default { const payload = { pin_code: pinCode } return repository.post(`${resource}/tfa/setup/check/`, payload) }, - disableTFA () { - return repository.post(`${resource}/tfa/disable/`) + disableTFA (data) { + return repository.post(`${resource}/tfa/disable/`, data) }, - resetRecoveryCodes () { - return repository.post(`${resource}/tfa/reset_codes/`) + resetRecoveryCodes (data) { + return repository.post(`${resource}/tfa/reset_codes/`, data) }, getForward () { return repository.get(`${resource}/forward/`) diff --git a/frontend/src/components/user/TwoFactorAuthForm.vue b/frontend/src/components/user/TwoFactorAuthForm.vue index 48d66b4ec..8c4db43ef 100644 --- a/frontend/src/components/user/TwoFactorAuthForm.vue +++ b/frontend/src/components/user/TwoFactorAuthForm.vue @@ -11,7 +11,7 @@ - Click here to copy the key + + Click here to copy the key + - mdi-check-all - + mdi-check-all + @@ -41,7 +43,7 @@ @@ -115,6 +133,7 @@ import account from '@/api/account' import Cookies from 'js-cookie' import RecoveryCodesResetDialog from './RecoveryCodesResetDialog' import QrcodeVue from 'qrcode.vue' +import { bus } from '@/main' export default { components: { @@ -132,7 +151,11 @@ export default { qrURL: null, clicked: false, showCodesResetDialog: false, - tokens: [] + loadingDisable: false, + loadingReset: false, + password: '', + tokens: [], + errors: {} } }, methods: { @@ -164,15 +187,52 @@ export default { Cookies.set('refreshToken', resp.data.refresh, { sameSite: 'strict' }) this.$store.dispatch('auth/initialize') }, - disableTFA () { - account.disableTFA().then(resp => { + async disableTFA () { + const isValid = await this.$refs.observer.validate() + if (!isValid) { + return + } + const payload = { + password: this.password + } + this.loadingDisable = true + account.disableTFA(payload).then(resp => { + this.loadingDisable = false this.$store.dispatch('auth/fetchUser') + this.$refs.password.reset() + this.$refs.observer.reset() + }).catch(err => { + this.loadingDisable = false + if (err.response.status === 400) { + this.$refs.observer.setErrors(err.response.data) + } else { + bus.$emit('notification', { msg: this.$gettext(err.response.data), type: 'error' }) + } }) }, - resetRecoveryCodes () { - account.resetRecoveryCodes().then(resp => { + async resetRecoveryCodes () { + const isValid = await this.$refs.observer.validate() + if (!isValid) { + return + } + const payload = { + password: this.password + } + this.loadingReset = true + + account.resetRecoveryCodes(payload).then(resp => { + this.loadingReset = false this.newTokens = resp.data.tokens this.showCodesResetDialog = true + this.$refs.password.reset() + this.$refs.observer.reset() + }).catch(err => { + this.loadingReset = false + if (err.response.status === 400) { + this.$refs.observer.setErrors(err.response.data) + } else { + bus.$emit('notification', { msg: this.$gettext(err.response.data), type: 'error' }) + } }) }, closeRecoveryCodesResetDialog () { @@ -191,9 +251,10 @@ export default { diff --git a/frontend/src/plugins/vee-validate.js b/frontend/src/plugins/vee-validate.js index 3d753f006..bf78b912b 100644 --- a/frontend/src/plugins/vee-validate.js +++ b/frontend/src/plugins/vee-validate.js @@ -15,6 +15,13 @@ extend('numeric', { ...numeric, message: () => $gettext('This field must be a valid number') }) +extend('minmax', { + validate: async (value, args) => { + return value.length >= args.length && value.length <= args.length + }, + params: ['length'], + message: $gettext('This field must have a length of {length} characters') +}) extend('required', { ...required, message: () => $gettext('This field is required') diff --git a/modoboa/core/api/v1/serializers.py b/modoboa/core/api/v1/serializers.py index 39e9ff6e1..c3f115530 100644 --- a/modoboa/core/api/v1/serializers.py +++ b/modoboa/core/api/v1/serializers.py @@ -1,9 +1,14 @@ """Core API serializers.""" +import logging + +from django.utils.html import escape from django.utils.translation import gettext as _ from rest_framework import serializers +logger = logging.getLogger("modoboa.auth") + class CheckTFASetupSerializer(serializers.Serializer): """Serializer used to finalize 2FA setup.""" @@ -21,3 +26,20 @@ class CheckTFASetupSerializer(serializers.Serializer): if user.tfa_enabled or user.staticdevice_set.exists(): raise serializers.ValidationError(_("2FA is already enabled")) return data + + +class CheckPasswordTFASerializer(serializers.Serializer): + """Serializer used to check user password when modifying TFA.""" + + password = serializers.CharField() + + def validate_password(self, value): + user = self.context["user"] + if not user.check_password(value): + logger.warning( + _("Failed TFA settings editing attempt from '%s' as user '%s'"), + self.context["remote_addr"], + escape(user.username) + ) + raise serializers.ValidationError(_("Invalid password")) + return value diff --git a/modoboa/core/api/v1/viewsets.py b/modoboa/core/api/v1/viewsets.py index 31a44fa25..8ea9e2ca9 100644 --- a/modoboa/core/api/v1/viewsets.py +++ b/modoboa/core/api/v1/viewsets.py @@ -57,8 +57,17 @@ class AccountViewSet(GetThrottleViewsetMixin, viewsets.ViewSet): @action(methods=["post"], detail=False, url_path="tfa/disable") def tfa_disable(self, request): """Disable TFA.""" + serializer = serializers.CheckPasswordTFASerializer( + data=request.data, + context={ + "user": request.user, + "remote_addr": request.META["REMOTE_ADDR"] + } + ) + serializer.is_valid(raise_exception=True) if not request.user.tfa_enabled: - return response.Response({"error": _("2FA is not enabled")}, + # We include it as "password" to display the error + return response.Response({"password": _("2FA is not enabled")}, status=400) request.user.totpdevice_set.all().delete() request.user.staticdevice_set.all().delete() @@ -70,9 +79,18 @@ class AccountViewSet(GetThrottleViewsetMixin, viewsets.ViewSet): @action(methods=["post"], detail=False, url_path="tfa/reset_codes") def tfa_reset_codes(self, request, *args, **kwargs): """Reset recovery codes.""" + serializer = serializers.CheckPasswordTFASerializer( + data=request.data, + context={ + "user": request.user, + "remote_addr": request.META["REMOTE_ADDR"] + } + ) + serializer.is_valid(raise_exception=True) device = request.user.staticdevice_set.first() if device is None: - return response.Response(status=403) + return response.Response({"password": _("2FA is not enabled")}, + status=400) device.token_set.all().delete() for cpt in range(10): token = StaticToken.random_token() diff --git a/modoboa/core/api/v2/tests.py b/modoboa/core/api/v2/tests.py index 079437e92..cc0ccf019 100644 --- a/modoboa/core/api/v2/tests.py +++ b/modoboa/core/api/v2/tests.py @@ -146,8 +146,9 @@ class AccountViewSetTestCase(ModoAPITestCase): self.assertEqual(resp.status_code, 200) self.assertIn("access", resp.json()) - def test_tfa_setup_get_qr_code(self): - url = reverse("v2:account-tfa-setup-get-qr-code") + def test_tfa_setup(self): + # Setup TFA + url = reverse("v2:account-tfa-setup-get-key") resp = self.client.get(url) self.assertEqual(resp.status_code, 404) @@ -156,7 +157,7 @@ class AccountViewSetTestCase(ModoAPITestCase): user.totpdevice_set.create(name="Device") resp = self.client.get(url) self.assertEqual(resp.status_code, 200) - self.assertEqual(resp.content_type, "application/xml") + self.assertEqual(resp.content_type, "application/json") user.tfa_enabled = True user.save() @@ -164,7 +165,11 @@ class AccountViewSetTestCase(ModoAPITestCase): self.assertEqual(resp.status_code, 404) @mock.patch("django_otp.plugins.otp_totp.models.TOTPDevice.verify_token") - def test_tfa_setup_check(self, verify_mock): + def test_tfa_setup_modify(self, + verify_mock, + password_ko="Toto1234", + password_ok="password" + ): user = models.User.objects.get(username="admin") user.totpdevice_set.create(name="Device") @@ -183,6 +188,30 @@ class AccountViewSetTestCase(ModoAPITestCase): resp = self.client.post(url, data, format="json") self.assertEqual(resp.status_code, 400) + url_reset = reverse("v2:account-tfa-reset-codes") + url_disable = reverse("v2:account-tfa-disable") + data = {"password": password_ko} + # Try regenerate TFA backup code with wrong password + resp = self.client.post(url_reset, data, format="json") + self.assertEqual(resp.status_code, 400) + # Try disable with wrong password + resp = self.client.post(url_disable, data, format="json") + self.assertEqual(resp.status_code, 400) + + # Test with password ok + data = {"password": password_ok} + # Try regenerate TFA backup codes with good password + resp = self.client.post(url_reset, data, format="json") + self.assertEqual(resp.status_code, 200) + self.assertIn("tokens", resp.json()) + + # Try disable with good password + resp = self.client.post(url_disable, data, format="json") + self.assertEqual(resp.status_code, 200) + + user.refresh_from_db() + self.assertEqual(user.tfa_enabled, False) + def test_api_token(self): # 1. Obtain a JWT token so we can safely play with basic token url = reverse("v2:token_obtain_pair") @@ -415,7 +444,6 @@ class PasswordResetTestCase(AccountViewSetTestCase): # TODO: See why user doesn't update it's password --> self.test_me_password(password_ok="MyHardenedPass1!") - class AuthenticationTestCase(ModoAPITestCase): @mock.patch("django_otp.match_token") diff --git a/modoboa/core/api/v2/viewsets.py b/modoboa/core/api/v2/viewsets.py index 1732ef67d..7cba4fdf6 100644 --- a/modoboa/core/api/v2/viewsets.py +++ b/modoboa/core/api/v2/viewsets.py @@ -99,7 +99,7 @@ class AccountViewSet(core_v1_viewsets.AccountViewSet): @action(methods=["get"], detail=False, url_path="tfa/setup/key") def tfa_setup_get_key(self, request): - """Get a QR code to finalize the setup process.""" + """Get a key and url to finalize the setup process.""" if request.user.tfa_enabled: return response.Response(status=404) device = request.user.totpdevice_set.first() diff --git a/modoboa/core/templates/core/user_security.html b/modoboa/core/templates/core/user_security.html index 5a31cc418..3541799f7 100644 --- a/modoboa/core/templates/core/user_security.html +++ b/modoboa/core/templates/core/user_security.html @@ -54,10 +54,10 @@ Two-Factor Authentication is enabled for your account. {% endblocktrans %} - + {% trans "Disable 2FA" %} - + {% trans "Reset recovery codes" %} {% else %} @@ -101,3 +101,82 @@ + + + diff --git a/modoboa/static/js/twocols_nav.js b/modoboa/static/js/twocols_nav.js index d04b3e3e8..e3574a13e 100644 --- a/modoboa/static/js/twocols_nav.js +++ b/modoboa/static/js/twocols_nav.js @@ -166,12 +166,6 @@ TwocolsNav.prototype = { this.update_content(data); $('#start-tfa-setup').click(this.startTFASetup); $('#finalize-tfa-setup').click(this.finalizeTFASetup); - $('#deactivate-tfa').confirm({ - question: gettext('Are you sure?'), - warning: gettext('This will invalidate your registered application.'), - method: 'POST' - }); - $('#tfa-refresh-codes').click(this.resetTFARecoveryCodes); }, startTFASetup: function (evt) { @@ -202,24 +196,6 @@ TwocolsNav.prototype = { var errors = JSON.parse(jqxhr.responseText); display_field_error($('#pin-code'), errors.pin_code.join(' ')); }); - }, - - resetTFARecoveryCodes: function (evt) { - evt.preventDefault(); - $.ajax({ - url: $(this).attr('href'), - type: 'post', - contentType: 'application/json' - }).done(function (resp) { - var $container = $('#tfa-codes-area'); - var $ul = $('
    '); - $.each(resp.tokens, function (index, value) { - var $li = $('
  • ', { html: value }); - $ul.append($li); - }); - $container.append($ul); - $('#codes-modal').modal(); - }); } };