Skip to content

Commit

Permalink
ref: prevent cross-db transactions for sending account emails (#56110)
Browse files Browse the repository at this point in the history 
prior to this fix:

```console
$ pytest tests/sentry/web/frontend/test_accounts.py::TestAccounts::test_leaking_recovery_hash
============================= test session starts ==============================
platform darwin -- Python 3.8.16, pytest-7.2.1, pluggy-0.13.1
rootdir: /Users/asottile/workspace/sentry, configfile: pyproject.toml
plugins: fail-slow-0.3.0, rerunfailures-11.0, sentry-0.1.11, xdist-3.0.2, cov-4.0.0, repeat-0.9.1, django-4.4.0
collected 1 item                                                               

tests/sentry/web/frontend/test_accounts.py F                             [100%]

=================================== FAILURES ===================================
___________________ TestAccounts.test_leaking_recovery_hash ____________________
tests/sentry/web/frontend/test_accounts.py:71: in test_leaking_recovery_hash
    resp = self.client.post(
.venv/lib/python3.8/site-packages/django/test/client.py:751: in post
    response = super().post(path, data=data, content_type=content_type, secure=secure, **extra)
.venv/lib/python3.8/site-packages/django/test/client.py:407: in post
    return self.generic('POST', path, post_data, content_type,
.venv/lib/python3.8/site-packages/django/test/client.py:473: in generic
    return self.request(**r)
.venv/lib/python3.8/site-packages/django/test/client.py:719: in request
    self.check_exception(response)
.venv/lib/python3.8/site-packages/django/test/client.py:580: in check_exception
    raise exc_value
.venv/lib/python3.8/site-packages/django/core/handlers/exception.py:47: in inner
    response = get_response(request)
.venv/lib/python3.8/site-packages/django/core/handlers/base.py:181: in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
src/sentry/web/decorators.py:51: in wrapped
    response = func(request, *args, **kwargs)
src/sentry/web/frontend/accounts.py:122: in recover_confirm
    if not user.has_2fa():
src/sentry/models/user.py:207: in has_2fa
    user_id=self.id, type__in=[a.type for a in available_authenticators(ignore_backup=True)]
src/sentry/auth/authenticators/__init__.py:23: in available_authenticators
    return [v for v in interfaces if not v.is_backup_interface and v.is_available]
src/sentry/auth/authenticators/__init__.py:23: in <listcomp>
    return [v for v in interfaces if not v.is_backup_interface and v.is_available]
src/sentry/utils/decorators.py:8: in __get__
    return self.fget(owner)
src/sentry/auth/authenticators/sms.py:46: in is_available
    return sms_available()
src/sentry/utils/sms.py:54: in sms_available
    return bool(options.get("sms.twilio-account"))
src/sentry/options/manager.py:285: in get
    result = self.store.get(opt, silent=silent)
src/sentry/options/store.py:96: in get
    result = self.get_store(key, silent=silent)
src/sentry/options/store.py:187: in get_store
    value = self.model.objects.get(key=key.name).value
src/sentry/db/models/manager/base.py:255: in get
    return super().get(*args, **kwargs)
.venv/lib/python3.8/site-packages/django/db/models/manager.py:85: in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
.venv/lib/python3.8/site-packages/django/db/models/query.py:431: in get
    num = len(clone)
.venv/lib/python3.8/site-packages/django/db/models/query.py:262: in __len__
    self._fetch_all()
.venv/lib/python3.8/site-packages/django/db/models/query.py:1324: in _fetch_all
    self._result_cache = list(self._iterable_class(self))
.venv/lib/python3.8/site-packages/django/db/models/query.py:51: in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
.venv/lib/python3.8/site-packages/django/db/models/sql/compiler.py:1175: in execute_sql
    cursor.execute(sql, params)
.venv/lib/python3.8/site-packages/django/db/backends/utils.py:98: in execute
    return super().execute(sql, params)
.venv/lib/python3.8/site-packages/sentry_sdk/integrations/django/__init__.py:610: in execute
    return real_execute(self, sql, params)
.venv/lib/python3.8/site-packages/django/db/backends/utils.py:66: in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
.venv/lib/python3.8/site-packages/django/db/backends/utils.py:75: in _execute_with_wrappers
    return executor(sql, params, many, context)
src/sentry/testutils/hybrid_cloud.py:116: in __call__
    assert (
E   AssertionError: Transaction opened for db {'control'}, but command running against db default
E   assert 'default' in {'control'}
E    +  where 'default' = <sentry.testutils.hybrid_cloud.EnforceNoCrossTransactionWrapper object at 0x12898c460>.alias
```

```console
$ pytest tests/sentry/mediators/sentry_app_installations/test_installation_notifier.py::TestInstallationNotifier::test_task_enqueued
============================= test session starts ==============================
platform darwin -- Python 3.8.16, pytest-7.2.1, pluggy-0.13.1
rootdir: /Users/asottile/workspace/sentry, configfile: pyproject.toml
plugins: fail-slow-0.3.0, rerunfailures-11.0, sentry-0.1.11, xdist-3.0.2, cov-4.0.0, repeat-0.9.1, django-4.4.0
collected 1 item                                                               

tests/sentry/mediators/sentry_app_installations/test_installation_notifier.py F [100%]

=================================== FAILURES ===================================
_________________ TestInstallationNotifier.test_task_enqueued __________________
tests/sentry/mediators/sentry_app_installations/test_installation_notifier.py:45: in test_task_enqueued
    InstallationNotifier.run(install=self.install, user=self.user, action="created")
src/sentry/mediators/mediator.py:155: in run
    return _inner()
src/sentry/mediators/mediator.py:147: in _inner
    result = obj.call()
src/sentry/mediators/sentry_app_installations/installation_notifier.py:20: in call
    self._send_webhook()
src/sentry/mediators/sentry_app_installations/installation_notifier.py:27: in _send_webhook
    return send_and_save_webhook_request(self.sentry_app, self.request)
src/sentry/utils/sentry_apps/webhooks.py:34: in wrapper
    return func(sentry_app, app_platform_event, url)
src/sentry/utils/sentry_apps/webhooks.py:126: in send_and_save_webhook_request
    timeout=options.get("sentry-apps.webhook.timeout.sec"),
src/sentry/options/manager.py:285: in get
    result = self.store.get(opt, silent=silent)
src/sentry/options/store.py:96: in get
    result = self.get_store(key, silent=silent)
src/sentry/options/store.py:187: in get_store
    value = self.model.objects.get(key=key.name).value
src/sentry/db/models/manager/base.py:255: in get
    return super().get(*args, **kwargs)
.venv/lib/python3.8/site-packages/django/db/models/manager.py:85: in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
.venv/lib/python3.8/site-packages/django/db/models/query.py:431: in get
    num = len(clone)
.venv/lib/python3.8/site-packages/django/db/models/query.py:262: in __len__
    self._fetch_all()
.venv/lib/python3.8/site-packages/django/db/models/query.py:1324: in _fetch_all
    self._result_cache = list(self._iterable_class(self))
.venv/lib/python3.8/site-packages/django/db/models/query.py:51: in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
.venv/lib/python3.8/site-packages/django/db/models/sql/compiler.py:1175: in execute_sql
    cursor.execute(sql, params)
.venv/lib/python3.8/site-packages/django/db/backends/utils.py:98: in execute
    return super().execute(sql, params)
.venv/lib/python3.8/site-packages/sentry_sdk/integrations/django/__init__.py:610: in execute
    return real_execute(self, sql, params)
.venv/lib/python3.8/site-packages/django/db/backends/utils.py:66: in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
.venv/lib/python3.8/site-packages/django/db/backends/utils.py:75: in _execute_with_wrappers
    return executor(sql, params, many, context)
src/sentry/testutils/hybrid_cloud.py:116: in __call__
    assert (
E   AssertionError: Transaction opened for db {'control'}, but command running against db default
E   assert 'default' in {'control'}
E    +  where 'default' = <sentry.testutils.hybrid_cloud.EnforceNoCrossTransactionWrapper object at 0x13b3e0d60>.alias
=========================== short test summary info ============================
FAILED tests/sentry/mediators/sentry_app_installations/test_installation_notifier.py::TestInstallationNotifier::test_task_enqueued - AssertionError: Transaction opened for db {'control'}, but command running ...
============================== 1 failed in 4.50s ============================
```

---------

Co-authored-by: Zachary Collins <zachary.collins@sentry.io>
Co-authored-by: Ryan Skonnord <ryan.skonnord@sentry.io>
  • Loading branch information
@asottile-sentry @corps @RyanSkonnord
3 people authored Sep 13, 2023
1 parent f8cf121 commit c2f0c36
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 4 deletions.
10 changes: 7 additions & 3 deletions src/sentry/auth/authenticators/u2f.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ def create_credential_object(registeredKey):
)


def _get_url_prefix() -> str:
return options.get("system.url-prefix")


class U2fInterface(AuthenticatorInterface):
type = 3
interface_id = "u2f"
Expand All @@ -51,7 +55,7 @@ class U2fInterface(AuthenticatorInterface):
allow_multi_enrollment = True
# rp is a relying party for webauthn, this would be sentry.io for SAAS
# and the prefix for self-hosted / dev environments
rp_id = urlparse(options.get("system.url-prefix")).hostname
rp_id = urlparse(_get_url_prefix()).hostname
rp = PublicKeyCredentialRpEntity(rp_id, "Sentry")
webauthn_registration_server = Fido2Server(rp)

Expand All @@ -71,12 +75,12 @@ def u2f_app_id(cls):
def u2f_facets(cls):
facets = options.get("u2f.facets")
if not facets:
return [options.get("system.url-prefix")]
return [_get_url_prefix()]
return [x.rstrip("/") for x in facets]

@classproperty
def is_available(cls):
url_prefix = options.get("system.url-prefix")
url_prefix = _get_url_prefix()
return url_prefix and url_prefix.startswith("https://")

def _get_kept_devices(self, key):
Expand Down
8 changes: 7 additions & 1 deletion src/sentry/options/store.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from django.db.utils import OperationalError, ProgrammingError
from django.utils import timezone

from sentry.db.postgres.transactions import in_test_hide_transaction_boundary
from sentry.options.manager import UpdateChannel

CACHE_FETCH_ERR = "Unable to fetch option cache for %s"
Expand Down Expand Up @@ -184,7 +185,12 @@ def get_store(self, key, silent=False):
is limited at the moment.
"""
try:
value = self.model.objects.get(key=key.name).value
# NOTE: To greatly reduce test bugs due to cache leakage, we don't enforce cross db constraints
# because in practice the option query is consistent with the process level silo mode.
# If you do change the way the option class model is picked, keep in mind it may not be deeply
# tested due to the core assumption it should be stable per process in practice.
with in_test_hide_transaction_boundary():
value = self.model.objects.get(key=key.name).value
except (self.model.DoesNotExist, ProgrammingError, OperationalError):
value = None
except Exception:
Expand Down

0 comments on commit c2f0c36

Please sign in to comment.