diff --git a/assets/app/vue/defines.ts b/assets/app/vue/defines.ts index ff00494a..7ab07c5f 100644 --- a/assets/app/vue/defines.ts +++ b/assets/app/vue/defines.ts @@ -1,5 +1,5 @@ -export const APP_PASSWORD_SUPPORT_URL = 'https://support.tb.pro/hc/articles/46805020320275-App-password-support'; export const OTHER_APPS_SUPPORT_URL = 'https://support.tb.pro/hc/articles/45584951087635-Using-Thundermail-with-other-clients'; +export const APP_PASSWORD_SUPPORT_URL = 'https://support.tb.pro/hc/articles/46805020320275-Create-an-app-password'; export const WHAT_IS_IMAP_SUPPORT_URL = 'https://support.tb.pro/hc/articles/46108465880467-What-is-IMAP'; export const WHAT_IS_JMAP_SUPPORT_URL = 'https://support.tb.pro/hc/articles/46109214513939-What-is-JMAP'; export const WHAT_IS_SMTP_SUPPORT_URL = 'https://support.tb.pro/hc/articles/46106891841043-What-is-SMTP'; diff --git a/src/thunderbird_accounts/authentication/api.py b/src/thunderbird_accounts/authentication/api.py index 16a16b65..b681f443 100644 --- a/src/thunderbird_accounts/authentication/api.py +++ b/src/thunderbird_accounts/authentication/api.py @@ -5,6 +5,7 @@ from thunderbird_accounts.authentication.permissions import CanCreateTestAllowListEntries from enum import StrEnum from urllib.parse import quote +import logging from django.conf import settings from mozilla_django_oidc.contrib.drf import OIDCAuthentication @@ -311,7 +312,12 @@ def sign_up(request: Request): status=400, ) except ImportUserError as ex: - sentry_sdk.capture_exception(ex) + if ex.is_already_exists and not can_register_with_username(partial_username): + logging.info( + f'Dupe signup suppressed (status={ex.status_code or "none"}, error={ex.error_code or "unknown"})', + ) + else: + sentry_sdk.capture_exception(ex) return Response( { 'error': ex.error_desc if ex.error_desc else _('There was an unknown error, please try again later.'), diff --git a/src/thunderbird_accounts/authentication/clients.py b/src/thunderbird_accounts/authentication/clients.py index cefda02b..35c1882e 100644 --- a/src/thunderbird_accounts/authentication/clients.py +++ b/src/thunderbird_accounts/authentication/clients.py @@ -339,22 +339,14 @@ def import_user( }, ) except RequestException as exc: - sentry_sdk.capture_exception(exc) - - try: - error_data: dict = json.loads(exc.response.content.decode()) - except (TypeError, JSONDecodeError): - # Could not determine explicit error information - error_data = {} - # Note: status_code == 409 means the user already exists on keycloak which we should have caught before this # function call. - raise ImportUserError( - username=username, - error=f'Error<{exc.response.status_code}>: {exc.response.content.decode()}', - error_code=error_data.get('error', f'status-{exc.response.status_code}'), - error_desc=error_data.get('error_description', error_data.get('errorMessage')), - ) + error = ImportUserError.from_request_exception(exc, username=username) + + if not error.is_already_exists: + sentry_sdk.capture_exception(exc) + + raise error # Request returns an empty body on a 201 success, so retrieve pkid from location. # ex/ {'Location': 'http://keycloak:8999/admin/realms/tbpro/users/39a7b5e8-7a64-45e3-acf1-ca7d314bfcec', ... } diff --git a/src/thunderbird_accounts/authentication/exceptions.py b/src/thunderbird_accounts/authentication/exceptions.py index 35178bee..f676f081 100644 --- a/src/thunderbird_accounts/authentication/exceptions.py +++ b/src/thunderbird_accounts/authentication/exceptions.py @@ -1,5 +1,10 @@ +import json from typing import Optional +from requests.exceptions import RequestException + +POSTGRES_DUPLICATE_KEY_MARKER = 'duplicate key value violates unique constraint' + class KeycloakError(RuntimeError): """Generic error""" @@ -31,6 +36,7 @@ def __str__(self): class ImportUserError(KeycloakError): username: str error: str + """Format an error usefully if the user already exists in Keycloak""" def __init__( self, @@ -38,6 +44,7 @@ def __init__( username: Optional[str] = None, error_code: Optional[str] = None, error_desc: Optional[str] = None, + status_code: Optional[int] = None, *args, **kwargs, ): @@ -48,6 +55,44 @@ def __init__( # Structured error from keycloak self.error_code = error_code self.error_desc = error_desc + self.status_code = status_code + + @classmethod + def from_request_exception(cls, exc: RequestException, *, username: Optional[str] = None): + response = exc.response + status_code = None + response_content = '' + error_data = {} + + if response is not None: + status_code = response.status_code + response_content = response.content.decode(errors='replace') + try: + error_data = json.loads(response_content) + except (TypeError, json.JSONDecodeError): + error_data = {} + + if response_content: + error = f'Error<{status_code}>: {response_content}' + else: + error = f'Error<{exc}>: No response!' + + return cls( + username=username, + error=error, + error_code=error_data.get('error', f'status-{status_code}' if status_code is not None else None), + error_desc=error_data.get('error_description', error_data.get('errorMessage')), + status_code=status_code, + ) + + @property + def is_already_exists(self): + # HTTP 409 means "Conflict" and is returned by Keycloak when a resource already exists. + if self.status_code == 409: + return True + + error_text = ' '.join(filter(None, [self.error_code, self.error_desc, self.error])).lower() + return POSTGRES_DUPLICATE_KEY_MARKER in error_text def __str__(self): return f'ImportUserError: {self.error} for {self.username}' diff --git a/src/thunderbird_accounts/authentication/tests/test_api.py b/src/thunderbird_accounts/authentication/tests/test_api.py index 0b7c0636..db99591b 100644 --- a/src/thunderbird_accounts/authentication/tests/test_api.py +++ b/src/thunderbird_accounts/authentication/tests/test_api.py @@ -221,6 +221,52 @@ def test_import_errors_propagate(self, mock_import_user: MagicMock): self.assertEqual(resp_data.get('type'), mock_test_type, msg=assert_fail_msg) self.assertEqual(resp_data.get('error'), mock_test_error, msg=assert_fail_msg) + def test_import_already_exists_error_does_not_capture_sentry_when_local_user_exists( + self, mock_import_user: MagicMock + ): + def create_local_user_and_raise(username, email, **_kwargs): + User.objects.create(username=username, email=email, recovery_email=email) + raise ImportUserError( + 'Error<409>: User exists with same username', + username=username, + error_code='status-409', + error_desc='User exists with same username', + status_code=409, + ) + + mock_import_user.side_effect = create_local_user_and_raise + + with patch('thunderbird_accounts.authentication.api.sentry_sdk.capture_exception') as mock_capture_exception: + response = self.client.post( + '/api/v1/auth/sign-up/', + self.make_sign_up_data(email=self.wait_list[0]), + ) + + assert_fail_msg = self.get_messages(response) + self.assertEqual(response.status_code, 400, msg=assert_fail_msg) + self.assertEqual(response.json()['type'], 'status-409', msg=assert_fail_msg) + mock_capture_exception.assert_not_called() + + def test_import_already_exists_error_captures_sentry_without_local_user(self, mock_import_user: MagicMock): + mock_import_user.side_effect = ImportUserError( + 'Error<409>: User exists with same username', + username='hello', + error_code='status-409', + error_desc='User exists with same username', + status_code=409, + ) + + with patch('thunderbird_accounts.authentication.api.sentry_sdk.capture_exception') as mock_capture_exception: + response = self.client.post( + '/api/v1/auth/sign-up/', + self.make_sign_up_data(email=self.wait_list[0]), + ) + + assert_fail_msg = self.get_messages(response) + self.assertEqual(response.status_code, 400, msg=assert_fail_msg) + self.assertEqual(response.json()['type'], 'status-409', msg=assert_fail_msg) + mock_capture_exception.assert_called_once() + @override_settings(USE_ALLOW_LIST=True) class CanISignUpTestcase(APITestCase): diff --git a/src/thunderbird_accounts/mail/views.py b/src/thunderbird_accounts/mail/views.py index a770006e..a2f49bf5 100644 --- a/src/thunderbird_accounts/mail/views.py +++ b/src/thunderbird_accounts/mail/views.py @@ -489,10 +489,17 @@ def add_email_alias(request: HttpRequest): ) if not created: + logging.info('Alias creation found an existing local email record for the requested address') return JsonResponse( {'success': False, 'error': _('This email address is not available.')}, status=400, ) + except IntegrityError: + logging.info('Alias creation hit a local duplicate-address race') + return JsonResponse( + {'success': False, 'error': _('This email address is not available.')}, + status=400, + ) except Exception as e: logging.error(f'Error creating email alias: {e}') return JsonResponse(