From ab3b00c5aad1723c76382ab1c74e4d3772286ea4 Mon Sep 17 00:00:00 2001 From: Peter Thomassen Date: Fri, 4 Sep 2026 18:12:34 +0200 Subject: [PATCH] fix(webapp): don't mistake every 403 for an MFA challenge Report the error code alongside the detail, and redirect to the 2FA dialog only when the API asks for a second factor. --- api/desecapi/exception_handlers.py | 12 +++++++++++- api/desecapi/permissions.py | 12 ++++++++++++ api/desecapi/tests/test_authentication.py | 2 +- api/desecapi/tests/test_domain_limits.py | 2 ++ api/desecapi/tests/test_totp.py | 12 +++++++++--- api/desecapi/views/domains.py | 2 +- api/desecapi/views/records.py | 2 +- api/desecapi/views/tokens.py | 6 +++--- api/desecapi/views/users.py | 2 +- docs/auth/account.rst | 3 ++- www/webapp/src/utils.js | 14 +++++--------- 11 files changed, 48 insertions(+), 21 deletions(-) diff --git a/api/desecapi/exception_handlers.py b/api/desecapi/exception_handlers.py index 0a1b477d4..8b31feb5f 100644 --- a/api/desecapi/exception_handlers.py +++ b/api/desecapi/exception_handlers.py @@ -46,4 +46,14 @@ def _500(): metrics.get("desecapi_exception").labels(class_path).inc() return handler() - return drf_exception_handler(exc, context) + response = drf_exception_handler(exc, context) + + # DRF renders only the human-readable detail, dropping the machine-readable + # code that tells denials apart (e.g. an MFA challenge from a used-up domain + # limit). Clients need it to know what to do next, so put it on the wire. + if response is not None and isinstance(response.data, dict): + code = getattr(response.data.get("detail"), "code", None) + if code is not None: + response.data["code"] = code + + return response diff --git a/api/desecapi/permissions.py b/api/desecapi/permissions.py index b4e8b51b8..c34b26437 100644 --- a/api/desecapi/permissions.py +++ b/api/desecapi/permissions.py @@ -58,6 +58,18 @@ def has_permission(self, request, view): ) +class MFARequiredIfEnabledOrAPIToken(permissions.OR): + """ + Allows access to API tokens, and to human tokens as per MFARequiredIfEnabled. + """ + + message = MFARequiredIfEnabled.message + code = MFARequiredIfEnabled.code + + def __init__(self): + super().__init__(IsAPIToken(), MFARequiredIfEnabled()) + + class IsOwner(permissions.BasePermission): """ Custom permission to only allow owners of an object to view or edit it. diff --git a/api/desecapi/tests/test_authentication.py b/api/desecapi/tests/test_authentication.py index 9a4b25daa..96b2d7441 100644 --- a/api/desecapi/tests/test_authentication.py +++ b/api/desecapi/tests/test_authentication.py @@ -78,7 +78,7 @@ def assertAuthenticationStatus(self, code, plain=None, expired=False, **kwargs): response = self.client.get(self.reverse("v1:root"), **kwargs) body = ( - json.dumps({"detail": "Invalid token."}) + json.dumps({"detail": "Invalid token.", "code": "authentication_failed"}) if code == HTTP_401_UNAUTHORIZED else None ) diff --git a/api/desecapi/tests/test_domain_limits.py b/api/desecapi/tests/test_domain_limits.py index d7007e2c6..83a1379fe 100644 --- a/api/desecapi/tests/test_domain_limits.py +++ b/api/desecapi/tests/test_domain_limits.py @@ -168,6 +168,8 @@ def test_securing_domains_raises_the_limit(self): self.assertContains( response, "Domain limit", status_code=status.HTTP_403_FORBIDDEN ) + # Clients need to tell this apart from an MFA challenge + self.assertEqual(response.data["code"], "domain_limit_exceeded") # Two secure domains pay for their own slots, and the headroom floor # of 2 sits on top: 2 + max(2, round(sqrt(2))) = 4. diff --git a/api/desecapi/tests/test_totp.py b/api/desecapi/tests/test_totp.py index e289087ac..cb91b7e9e 100644 --- a/api/desecapi/tests/test_totp.py +++ b/api/desecapi/tests/test_totp.py @@ -34,6 +34,12 @@ def _decrement_timestep(self, offset): factor.save() def _test_MFA_permission_status(self, assertion): + def assert_status(response): + assertion(response.status_code, status.HTTP_403_FORBIDDEN) + if response.status_code == status.HTTP_403_FORBIDDEN: + # Clients need to tell this apart from other denials + self.assertEqual(response.data["code"], "mfa_required") + for method, view_names in { self.client.get: [ "v1:account", @@ -48,19 +54,19 @@ def _test_MFA_permission_status(self, assertion): }.items(): for view_name in view_names: response = method(self.reverse(view_name)) - assertion(response.status_code, status.HTTP_403_FORBIDDEN) + assert_status(response) for view_name in [ "v1:domain-detail", "v1:rrsets", ]: for method in [self.client.get, self.client.post]: response = method(self.reverse(view_name, name=self.my_domain)) - assertion(response.status_code, status.HTTP_403_FORBIDDEN) + assert_status(response) for method in [self.client.get, self.client.post]: response = method( self.reverse("v1:rrset@", name=self.my_domain, subname="", type="NS") ) - assertion(response.status_code, status.HTTP_403_FORBIDDEN) + assert_status(response) def test_workflow(self): # Request setting up TOTP factor diff --git a/api/desecapi/views/domains.py b/api/desecapi/views/domains.py index 4e81ec495..54b20a684 100644 --- a/api/desecapi/views/domains.py +++ b/api/desecapi/views/domains.py @@ -39,7 +39,7 @@ class DomainViewSet( def permission_classes(self): ret = [ IsAuthenticated, - permissions.IsAPIToken | permissions.MFARequiredIfEnabled, + permissions.MFARequiredIfEnabledOrAPIToken, permissions.IsOwner, ] if self.request.method not in SAFE_METHODS: diff --git a/api/desecapi/views/records.py b/api/desecapi/views/records.py index 491e6c466..5c570506c 100644 --- a/api/desecapi/views/records.py +++ b/api/desecapi/views/records.py @@ -38,7 +38,7 @@ class RRsetView(DomainViewMixin): serializer_class = RRsetSerializer permission_classes = ( IsAuthenticated, - permissions.IsAPIToken | permissions.MFARequiredIfEnabled, + permissions.MFARequiredIfEnabledOrAPIToken, permissions.IsDomainOwner, ) diff --git a/api/desecapi/views/tokens.py b/api/desecapi/views/tokens.py index 275db9158..8b1899d66 100644 --- a/api/desecapi/views/tokens.py +++ b/api/desecapi/views/tokens.py @@ -24,7 +24,7 @@ class TokenViewSet(IdempotentDestroyMixin, viewsets.ModelViewSet): def permission_classes(self): ret = [ IsAuthenticated, - permissions.IsAPIToken | permissions.MFARequiredIfEnabled, + permissions.MFARequiredIfEnabledOrAPIToken, permissions.HasManageTokensPermission, ] # The effective user may manage the token; its owner can only delete it @@ -86,7 +86,7 @@ class TokenPoliciesRoot(RetrieveAPIView): throttle_scope = "account_management_passive" permission_classes = [ IsAuthenticated, - permissions.IsAPIToken | permissions.MFARequiredIfEnabled, + permissions.MFARequiredIfEnabledOrAPIToken, permissions.HasManageTokensPermission | permissions.AuthTokenCorrespondsToViewToken, ] @@ -113,7 +113,7 @@ class TokenDomainPolicyViewSet(IdempotentDestroyMixin, viewsets.ModelViewSet): def permission_classes(self): ret = [ IsAuthenticated, - permissions.IsAPIToken | permissions.MFARequiredIfEnabled, + permissions.MFARequiredIfEnabledOrAPIToken, ] if self.request.method in SAFE_METHODS: ret.append( diff --git a/api/desecapi/views/users.py b/api/desecapi/views/users.py index 39a2d309e..78ec20699 100644 --- a/api/desecapi/views/users.py +++ b/api/desecapi/views/users.py @@ -72,7 +72,7 @@ def create(self, request, *args, **kwargs): class AccountView(generics.RetrieveUpdateAPIView): permission_classes = ( IsAuthenticated, - permissions.IsAPIToken | permissions.MFARequiredIfEnabled, + permissions.MFARequiredIfEnabledOrAPIToken, permissions.HasManageTokensPermission, ) serializer_class = serializers.UserSerializer diff --git a/docs/auth/account.rst b/docs/auth/account.rst index ba4b9b2b2..0a5ac131a 100644 --- a/docs/auth/account.rst +++ b/docs/auth/account.rst @@ -108,7 +108,8 @@ Request``:: HTTP/1.1 400 Bad Request { - "detail": "Registration denied. If you believe this is an error, please contact support." + "detail": "Registration denied. If you believe this is an error, please contact support.", + "code": "registration_denied" } diff --git a/www/webapp/src/utils.js b/www/webapp/src/utils.js index 4294c4890..7756894db 100644 --- a/www/webapp/src/utils.js +++ b/www/webapp/src/utils.js @@ -48,15 +48,11 @@ async function _digestError(error, app) { } else { return ['You are not logged in.']; } - } else if (error.response.status === 403) { - if (useUserStore().authenticated && !['change-email', 'delete-account'].includes(app.$route.name)) { // MFA - if (app.$route.name !== 'mfa') { - app.$router.push({name: 'mfa', query: {redirect: app.$route.fullPath}}); - } - return []; - } else { // unauthenticated 403, i.e. login failure - return [error.response.data.detail] - } + } else if (error.response.status === 403 && error.response.data?.code === 'mfa_required' && app !== undefined) { + if (app.$route.name !== 'mfa') { + app.$router.push({name: 'mfa', query: {redirect: app.$route.fullPath}}); + } + return []; } else if (error.response.status === 413) { return ['Too much data. Try to reduce the length of your inputs.']; } else if ('data' in error.response) {