Skip to content

Commit

Permalink
Prevent possible infinite sync loops
Browse files Browse the repository at this point in the history
Signed-off-by: Rick Elrod <[email protected]>
  • Loading branch information
relrod committed Aug 7, 2024
1 parent a72f5cf commit c37d1e9
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 8 deletions.
14 changes: 10 additions & 4 deletions ansible_base/jwt_consumer/common/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from django.conf import settings
from django.contrib.auth import get_user_model
from django.core.exceptions import ObjectDoesNotExist
from django.db import transaction
from django.db.models import Model
from django.db.utils import IntegrityError
from rest_framework.authentication import BaseAuthentication
Expand Down Expand Up @@ -120,10 +121,15 @@ def parse_jwt_token(self, request):
logger.warn(f"New user {self.user.username} created from JWT auth")
except IntegrityError as exc:
logger.warning(f'Existing user {self.token["user_data"]} is a conflict with local user, error: {exc}')
self.user, created = get_user_model().objects.update_or_create(
username=self.token["user_data"]['username'],
defaults=user_defaults,
)

with transaction.atomic():
try:
self.user = get_user_model().objects.select_for_update().get(username=self.token["user_data"]['username'])
except get_user_model().DoesNotExist:
self.user = get_user_model()(**user_defaults)

self.user._is_from_resource_server = True
self.user.save()

setattr(self.user, "resource_api_actions", self.token.get("resource_api_actions", None))

Expand Down
13 changes: 10 additions & 3 deletions ansible_base/resource_registry/models/resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ def delete_resource(self):
raise ValidationError({"resource_type": _(f"Resource type: {self.content_type.resource_type.name} cannot be managed by Resources.")})

with transaction.atomic():
self.content_object._is_from_resource_server = True # Don't try to sync it back to the server
self.content_object.delete()
self.delete()

Expand All @@ -107,8 +108,11 @@ def create_resource(

with transaction.atomic():
ObjModel = c_type.model_class()
content_object = processor(ObjModel()).save(resource_data, is_new=True)
resource = cls.objects.get(object_id=content_object.pk, content_type=c_type)
content_object = processor(ObjModel())
content_object.instance._is_from_resource_server = True # Don't try to sync it back to the server
content_object.save(resource_data, is_new=True)
del content_object.instance._is_from_resource_server
resource = cls.objects.get(object_id=content_object.instance.pk, content_type=c_type)

if ansible_id:
resource.ansible_id = ansible_id
Expand All @@ -134,7 +138,10 @@ def update_resource(self, resource_data: dict, ansible_id=None, partial=False, s
self.service_id = service_id
self.save()

processor(self.content_object).save(resource_data)
content_object = processor(self.content_object)
content_object.instance._is_from_resource_server = True # Don't try to sync it back to the server
content_object.save(resource_data)
del content_object.instance._is_from_resource_server


# This is a separate function so that it can work with models from apps in the
Expand Down
2 changes: 1 addition & 1 deletion ansible_base/resource_registry/rest_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def _make_request(
kwargs["stream"] = stream

resp = requests.request(**kwargs)
logger.info(f"Response status code: {resp.status_code}")
logger.info(f"Response status code from {url}: {resp.status_code}")

if self.raise_if_bad_request:
resp.raise_for_status()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ def sync_to_resource_server(instance, action, ansible_id=None):
object. (For create, the resource is expected to exist before calling this
function.)
"""

# This gets set in Resource.create_resource() and friends (and jwt_consumer.common.auth...)
is_from_resource_server = getattr(instance, '_is_from_resource_server', False)
if is_from_resource_server:
# Avoid an infinite loop by not syncing resources that came from the resource server.
logger.info(f"Skipping sync of resource {instance}, it appears to have come from the resource server")
return

try:
if action != "delete" and ansible_id is not None:
raise Exception("ansible_id should not be provided for create/update actions")
Expand Down
12 changes: 12 additions & 0 deletions test_app/tests/resource_registry/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,18 @@ def test_sync_to_resource_server_exception_during_save(self, user, organization,

assert queries.captured_queries[-1]['sql'] == 'ROLLBACK'

@pytest.mark.django_db(transaction=True)
def test_sync_to_resource_server_from_resource_server(self, user, organization, connect_monkeypatch):
"""
If we try to sync a model that came from the resource server, we should bail out.
"""
with connect_monkeypatch():
with mock.patch(f'{utils_path}.get_resource_server_client') as get_resource_server_client:
organization._is_from_resource_server = True
organization.save()

get_resource_server_client.assert_not_called()

@pytest.mark.parametrize(
'new_settings,should_sync',
[
Expand Down

0 comments on commit c37d1e9

Please sign in to comment.