From 23dcf21ab026174fa5634590ad5147d290c83149 Mon Sep 17 00:00:00 2001 From: Alex Zaslavsky Date: Mon, 2 Oct 2023 09:05:15 -0700 Subject: [PATCH] fix(backup): Fix unique collision behavior There were bugs and inadequacies in the `unique` collision resolution for imports - this change fixes them. Changes made here: - ApiToken re-use is disallowed - this is dangerous! Instead, whenever public keys or secrets collide, the _importer_ gets a fresh new key. - There were no collision tests for the following models with "collidable" unique sets: RelayUsage, Monitor, and UserPermission. These have now been added. A follow-on PR will add a check to `test_coverage` to ensure that all such models continue to be tested going forward. - `UserEmail` resolution was incorrect for the `--merge_users` case. When merging users, we DO NOT want the email to be overwritten by the incoming user! - OrgAuthToken, ProjectKey, and ProjectOption have all had their importers improved to make them a bit less confusing. Besides this, some other code has been cleaned up a bit. Issue: getsentry/team-ospo#203 --- fixtures/backup/fresh-install.json | 27 +++ src/sentry/backup/dependencies.py | 26 +++ src/sentry/backup/mixins.py | 54 +++--- src/sentry/models/apitoken.py | 3 - src/sentry/models/options/project_option.py | 11 +- src/sentry/models/orgauthtoken.py | 14 +- src/sentry/models/projectkey.py | 7 +- src/sentry/models/relay.py | 2 +- src/sentry/models/useremail.py | 22 ++- src/sentry/models/userip.py | 4 +- src/sentry/models/userpermission.py | 20 +- tests/sentry/backup/test_imports.py | 204 ++++++++++++++------ 12 files changed, 278 insertions(+), 116 deletions(-) diff --git a/fixtures/backup/fresh-install.json b/fixtures/backup/fresh-install.json index 4a01d09feb0345..9b832d80680077 100644 --- a/fixtures/backup/fresh-install.json +++ b/fixtures/backup/fresh-install.json @@ -116,6 +116,17 @@ "is_internal": true } }, +{ + "model":"sentry.authenticator", + "pk":1, + "fields": { + "user":1, + "created_at": "2023-06-22T22:59:55.602Z", + "last_used_at": null, + "type": 1, + "config": "\"\"" + } +}, { "model": "sentry.useremail", "pk": 1, @@ -285,5 +296,21 @@ "key": "sentry:origins", "value": "[\"*\"]" } +}, +{ + "model": "sentry.apiapplication", + "pk": 1, + "fields": { + "client_id": "e3bc5ddfe2d6c48115efa3c70bbeb0ef938f37a124a1e3a1275d804d15d111c9","client_secret": "bdcec56c5c1b3763afc292883dc0e7e6a0e7164d20435b9f4799e1df513ff940", + "owner":1, + "name": "uniquely-named-application", + "status": 0, + "allowed_origins": "", + "redirect_uris": "", + "homepage_url": null, + "privacy_url": null, + "terms_url": null, + "date_added": "2023-06-22T21:50:27.787Z" + } } ] diff --git a/src/sentry/backup/dependencies.py b/src/sentry/backup/dependencies.py index b48b0da02a5c01..f2d3bda0e78ec2 100644 --- a/src/sentry/backup/dependencies.py +++ b/src/sentry/backup/dependencies.py @@ -146,6 +146,32 @@ def get_possible_relocation_scopes(self) -> set[RelocationScope]: return self.model.get_possible_relocation_scopes() return set() + def get_uniques_without_foreign_keys(self) -> list[frozenset[str]]: + """ + Gets all unique sets (that is, either standalone fields that are marked `unique=True`, or + groups of fields listed in `Meta.unique_together`) for a model, as long as those sets do not + include any fields that are foreign keys. Note that the `id` field would be trivially + included in this list for every model, and is therefore ignored. + """ + + out = [] + for u in self.uniques: + # Exclude unique sets that are just {"id"}, since this is true for every model and not + # very useful when searching for potential collisions. + if u == {"id"}: + continue + + has_foreign_key = False + for field in u: + if self.foreign_keys.get(field): + has_foreign_key = True + break + + if not has_foreign_key: + out.append(u) + + return out + def get_model_name(model: type[models.Model] | models.Model) -> NormalizedModelName: return NormalizedModelName(f"{model._meta.app_label}.{model._meta.object_name}") diff --git a/src/sentry/backup/mixins.py b/src/sentry/backup/mixins.py index fb794dd7ea2f63..84c6b0d3cd3215 100644 --- a/src/sentry/backup/mixins.py +++ b/src/sentry/backup/mixins.py @@ -2,7 +2,7 @@ from typing import Optional, Tuple -from sentry.backup.dependencies import ImportKind +from sentry.backup.dependencies import ImportKind, dependencies, get_model_name from sentry.backup.helpers import ImportFlags from sentry.backup.scopes import ImportScope, RelocationScope @@ -17,34 +17,36 @@ def write_relocation_import( ) -> Optional[Tuple[int, ImportKind]]: # TODO(getsentry/team-ospo#190): Clean up the type checking here. if self.get_relocation_scope() == RelocationScope.Config: # type: ignore - # Get all fields with `unique=True` for this model. - cls = self.__class__ - uniq_fields = [ - f.name - for f in cls._meta.get_fields() # type: ignore - if getattr(f, "unique", False) and f.name != "id" - ] - - # Don't use this mixin for models with multiple `unique=True` fields; write custom logic - # instead. - if len(uniq_fields) > 1: + # Get all unique sets that will potentially cause collisions. + uniq_sets = dependencies()[get_model_name(self)].get_uniques_without_foreign_keys() # type: ignore + + # Don't use this mixin for models with multiple unique sets; write custom logic instead. + if len(uniq_sets) > 1: raise ValueError( - "Cannot use `OverwritableConfigMixin` on model with multiple unique fields" + "Cannot use `OverwritableConfigMixin` on model with multiple unique sets" ) - if len(uniq_fields) == 1 and getattr(self, uniq_fields[0], None) is not None: + + if len(uniq_sets) == 1: + uniq_set = uniq_sets[0] query = dict() - query[uniq_fields[0]] = getattr(self, uniq_fields[0]) - existing = self.__class__.objects.filter(**query).first() # type: ignore - if existing: - # Re-use the existing data if config overwrite is disabled. - if not flags.overwrite_configs: - return (existing.pk, ImportKind.Existing) - - # We are performing an overwrite (ie, keeping the old pk, but using all of the - # imported values). - self.pk = existing.pk - self.save() # type: ignore - return (self.pk, ImportKind.Overwrite) + for uniq_field_name in uniq_set: + if getattr(self, uniq_field_name, None) is not None: + query[uniq_field_name] = getattr(self, uniq_field_name) + + # If all of the fields in the unique set are NULL, we'll avoid a collision, so go + # ahead and write a new entry. + if len(query) > 0: + existing = self.__class__.objects.filter(**query).first() # type: ignore + if existing: + # Re-use the existing data if config overwrite is disabled. + if not flags.overwrite_configs: + return (existing.pk, ImportKind.Existing) + + # We are performing an overwrite (ie, keeping the old pk, but using all of + # the imported values). + self.pk = existing.pk + self.save() # type: ignore + return (self.pk, ImportKind.Overwrite) # Does not have a single colliding unique field - write as usual. return super().write_relocation_import(scope, flags) # type: ignore diff --git a/src/sentry/models/apitoken.py b/src/sentry/models/apitoken.py index b524c382722e35..ae99b1db0d4082 100644 --- a/src/sentry/models/apitoken.py +++ b/src/sentry/models/apitoken.py @@ -96,12 +96,9 @@ def write_relocation_import( query = models.Q(token=self.token) | models.Q(refresh_token=self.refresh_token) existing = self.__class__.objects.filter(query).first() if existing: - self.pk = existing.pk self.expires_at = timezone.now() + DEFAULT_EXPIRATION self.token = generate_token() self.refresh_token = generate_token() - self.save() - return (self.pk, ImportKind.Overwrite) return super().write_relocation_import(scope, flags) diff --git a/src/sentry/models/options/project_option.py b/src/sentry/models/options/project_option.py index 0d956f8f847814..298d4e05e5c570 100644 --- a/src/sentry/models/options/project_option.py +++ b/src/sentry/models/options/project_option.py @@ -165,11 +165,14 @@ class Meta: def write_relocation_import( self, _s: ImportScope, _f: ImportFlags ) -> Optional[Tuple[int, ImportKind]]: - (key, created) = self.__class__.objects.get_or_create( + # Some `ProjectOption`s for the project are automatically generated at insertion time via a + # `post_save()` hook, so they should already exist with autogenerated data. We simply need + # to update them with the correct, imported values here. + (option, _) = self.__class__.objects.get_or_create( project=self.project, key=self.key, defaults={"value": self.value} ) - if key: - self.pk = key.pk + if option: + self.pk = option.pk self.save() - return (self.pk, ImportKind.Inserted if created else ImportKind.Existing) + return (self.pk, ImportKind.Inserted) diff --git a/src/sentry/models/orgauthtoken.py b/src/sentry/models/orgauthtoken.py index 797f0ed3283560..26713bce339c76 100644 --- a/src/sentry/models/orgauthtoken.py +++ b/src/sentry/models/orgauthtoken.py @@ -4,7 +4,6 @@ from django.core.exceptions import ValidationError from django.db import models -from django.forms import model_to_dict from django.utils import timezone from django.utils.encoding import force_str @@ -83,7 +82,7 @@ def is_active(self) -> bool: return self.date_deactivated is None def write_relocation_import( - self, _s: ImportScope, _f: ImportFlags + self, scope: ImportScope, flags: ImportFlags ) -> Optional[Tuple[int, ImportKind]]: # TODO(getsentry/team-ospo#190): Prevents a circular import; could probably split up the # source module in such a way that this is no longer an issue. @@ -112,16 +111,7 @@ def write_relocation_import( self.token_hashed = hash_token(token_str) self.token_last_characters = token_str[-4:] - (key, created) = OrgAuthToken.objects.get_or_create( - token_hashed=self.token_hashed, - token_last_characters=self.token_last_characters, - defaults=model_to_dict(self), - ) - if key: - self.pk = key.pk - self.save() - - return (self.pk, ImportKind.Inserted if created else ImportKind.Existing) + return super().write_relocation_import(scope, flags) def is_org_auth_token_auth(auth: object) -> bool: diff --git a/src/sentry/models/projectkey.py b/src/sentry/models/projectkey.py index da0277f6d4e8de..d25ddde4928947 100644 --- a/src/sentry/models/projectkey.py +++ b/src/sentry/models/projectkey.py @@ -296,11 +296,14 @@ def write_relocation_import( if not self.secret_key or matching_secret_key: self.secret_key = self.generate_api_key() - (key, created) = ProjectKey.objects.get_or_create( + # ProjectKeys for the project are automatically generated at insertion time via a + # `post_save()` hook, so the keys for the project should already exist. We simply need to + # update them with the correct values here. + (key, _) = ProjectKey.objects.get_or_create( project=self.project, defaults=model_to_dict(self) ) if key: self.pk = key.pk self.save() - return (self.pk, ImportKind.Inserted if created else ImportKind.Existing) + return (self.pk, ImportKind.Inserted) diff --git a/src/sentry/models/relay.py b/src/sentry/models/relay.py index 3906dab1c15f4c..a1d0e179bfa1d3 100644 --- a/src/sentry/models/relay.py +++ b/src/sentry/models/relay.py @@ -9,7 +9,7 @@ @region_silo_only_model -class RelayUsage(Model): +class RelayUsage(OverwritableConfigMixin, Model): __relocation_scope__ = RelocationScope.Config relay_id = models.CharField(max_length=64) diff --git a/src/sentry/models/useremail.py b/src/sentry/models/useremail.py index 70047eaa6a4494..9f41cba6558120 100644 --- a/src/sentry/models/useremail.py +++ b/src/sentry/models/useremail.py @@ -96,17 +96,22 @@ def normalize_before_relocation_import( ) -> Optional[int]: from sentry.models.user import User - # If we are merging users, ignore this import and use the merged user's data. - if pk_map.get_kind(get_model_name(User), self.user_id) == ImportKind.Existing: - return None - + old_user_id = self.user_id old_pk = super().normalize_before_relocation_import(pk_map, scope, flags) if old_pk is None: return None - # Only preserve validation hashes in backup/restore scopes - in all others, have the user + # If we are merging users, ignore this import and use the merged user's data. + if pk_map.get_kind(get_model_name(User), old_user_id) == ImportKind.Existing: + # TODO(getsentry/team-ospo#190): Mutating `pk_map` here is a bit hacky, and we probably + # shouldn't do it. + useremail = self.__class__.objects.get(user_id=self.user_id) + pk_map.insert(get_model_name(self), self.pk, useremail.pk, ImportKind.Existing) + return None + + # Only preserve validation hashes in the backup/restore scope - in all others, have the user # verify their email again. - if scope not in {ImportScope.Config, ImportScope.Global}: + if scope != ImportScope.Global: self.is_verified = False self.validation_hash = get_secure_token() self.date_hash_added = timezone.now() @@ -116,10 +121,13 @@ def normalize_before_relocation_import( def write_relocation_import( self, _s: ImportScope, _f: ImportFlags ) -> Optional[Tuple[int, ImportKind]]: + # The `UserEmail` was automatically generated `post_save()`. We just need to update it with + # the data being imported. Note that if we've reached this point, we cannot be merging into + # an existing user, and are instead modifying the just-created `UserEmail` for a new one. useremail = self.__class__.objects.get(user=self.user, email=self.email) for f in self._meta.fields: if f.name not in ["id", "pk"]: setattr(useremail, f.name, getattr(self, f.name)) useremail.save() - return (useremail.pk, ImportKind.Existing) + return (useremail.pk, ImportKind.Inserted) diff --git a/src/sentry/models/userip.py b/src/sentry/models/userip.py index a030c7e413ca60..baa9bfbc749cde 100644 --- a/src/sentry/models/userip.py +++ b/src/sentry/models/userip.py @@ -63,8 +63,8 @@ def normalize_before_relocation_import( self.country_code = None self.region_code = None - # Only preserve the submitted timing data in backup/restore scopes. - if scope not in {ImportScope.Config, ImportScope.Global}: + # Only preserve the submitted timing data in the backup/restore scope. + if scope != ImportScope.Global: self.first_seen = self.last_seen = timezone.now() return old_pk diff --git a/src/sentry/models/userpermission.py b/src/sentry/models/userpermission.py index 2d569258283b1a..2d16f3996be117 100644 --- a/src/sentry/models/userpermission.py +++ b/src/sentry/models/userpermission.py @@ -1,10 +1,12 @@ from __future__ import annotations -from typing import FrozenSet, List +from typing import FrozenSet, List, Optional, Tuple from django.db import models -from sentry.backup.scopes import RelocationScope +from sentry.backup.dependencies import ImportKind +from sentry.backup.helpers import ImportFlags +from sentry.backup.scopes import ImportScope, RelocationScope from sentry.db.models import FlexibleForeignKey, control_silo_only_model, sane_repr from sentry.db.models.outboxes import ControlOutboxProducingModel from sentry.models.outbox import ControlOutboxBase, OutboxCategory @@ -49,3 +51,17 @@ def outboxes_for_update(self, shard_identifier: int | None = None) -> List[Contr object_identifier=self.user_id, ) ] + + def write_relocation_import( + self, _s: ImportScope, _f: ImportFlags + ) -> Optional[Tuple[int, ImportKind]]: + # Ensure that we never attempt to duplicate `UserPermission` entries, even when the + # `merge_users` flag is enabled, as they must always be globally unique per user. + (up, created) = self.__class__.objects.get_or_create( + user_id=self.user_id, permission=self.permission + ) + if up: + self.pk = up.pk + self.save() + + return (self.pk, ImportKind.Inserted if created else ImportKind.Existing) diff --git a/tests/sentry/backup/test_imports.py b/tests/sentry/backup/test_imports.py index 6e11f7a258fb0f..3633965acbd27f 100644 --- a/tests/sentry/backup/test_imports.py +++ b/tests/sentry/backup/test_imports.py @@ -31,12 +31,13 @@ from sentry.models.orgauthtoken import OrgAuthToken from sentry.models.project import Project from sentry.models.projectkey import ProjectKey -from sentry.models.relay import Relay +from sentry.models.relay import Relay, RelayUsage from sentry.models.user import User from sentry.models.useremail import UserEmail from sentry.models.userip import UserIP from sentry.models.userpermission import UserPermission from sentry.models.userrole import UserRole, UserRoleUser +from sentry.monitors.models import Monitor from sentry.snuba.models import QuerySubscription, SnubaQuery from sentry.testutils.factories import get_fixture_path from sentry.testutils.helpers.backups import ( @@ -200,13 +201,13 @@ def test_users_unsanitized_in_config_scope(self): # Every user except `max_user` shares an email. assert Email.objects.count() == 2 - # All `UserEmail`s must keep their imported verification status reset in this scope. + # All `UserEmail`s must have their verification status reset in this scope. assert UserEmail.objects.count() == 4 - assert UserEmail.objects.filter(is_verified=True).count() == 4 - assert UserEmail.objects.filter(date_hash_added__lt=datetime(2023, 7, 1, 0, 0)).count() == 4 + assert UserEmail.objects.filter(is_verified=True).count() == 0 + assert UserEmail.objects.filter(date_hash_added__lt=datetime(2023, 7, 1, 0, 0)).count() == 0 assert ( UserEmail.objects.filter(validation_hash="mCnWesSVvYQcq7qXQ36AZHwosAd6cghE").count() - == 4 + == 0 ) # 1 from `max_user`, 1 from `permission_user`. @@ -239,7 +240,7 @@ def test_users_unsanitized_in_global_scope(self): # Every user except `max_user` shares an email. assert Email.objects.count() == 2 - # All `UserEmail`s must keep their imported verification status reset in this scope. + # All `UserEmail`s must have their imported verification status reset in this scope. assert UserEmail.objects.count() == 4 assert UserEmail.objects.filter(is_verified=True).count() == 4 assert UserEmail.objects.filter(date_hash_added__lt=datetime(2023, 7, 1, 0, 0)).count() == 4 @@ -699,7 +700,7 @@ def test_colliding_api_token(self): # Take note of the `ApiTokens` that were created by the exhaustive organization - this is # the one we'll be importing. - colliding_no_refresh = ApiToken.objects.create( + colliding_no_refresh_set = ApiToken.objects.create( user=owner, token=generate_token(), expires_at=expires_at ) colliding_same_refresh_only = ApiToken.objects.create( @@ -726,8 +727,8 @@ def test_colliding_api_token(self): owner = self.create_exhaustive_user(username="owner") # Re-insert colliding tokens, pointed at the new user. - colliding_no_refresh.user_id = owner.id - colliding_no_refresh.save() + colliding_no_refresh_set.user_id = owner.id + colliding_no_refresh_set.save() colliding_same_refresh_only.token = generate_token() colliding_same_refresh_only.user_id = owner.id @@ -741,7 +742,7 @@ def test_colliding_api_token(self): colliding_same_both.save() assert ApiToken.objects.count() == 4 - assert ApiToken.objects.filter(token=colliding_no_refresh.token).count() == 1 + assert ApiToken.objects.filter(token=colliding_no_refresh_set.token).count() == 1 assert ( ApiToken.objects.filter( refresh_token=colliding_same_refresh_only.refresh_token @@ -759,16 +760,47 @@ def test_colliding_api_token(self): with open(tmp_path) as tmp_file: import_in_global_scope(tmp_file, printer=NOOP_PRINTER) - # Ensure that old tokens have been replaced. - assert ApiToken.objects.count() == 4 - assert not ApiToken.objects.filter(token=colliding_no_refresh.token).exists() - assert not ApiToken.objects.filter( - refresh_token=colliding_same_refresh_only.refresh_token - ).exists() - assert not ApiToken.objects.filter(token=colliding_same_token_only.token).exists() - assert not ApiToken.objects.filter( - token=colliding_same_both.token, refresh_token=colliding_same_both.refresh_token - ).exists() + # Ensure that old tokens have not been mutated. + assert ApiToken.objects.count() == 8 + assert ApiToken.objects.filter(token=colliding_no_refresh_set.token).count() == 1 + assert ( + ApiToken.objects.filter(refresh_token=colliding_same_refresh_only.refresh_token).count() + == 1 + ) + assert ApiToken.objects.filter(token=colliding_same_token_only.token).count() == 1 + assert ( + ApiToken.objects.filter( + token=colliding_same_both.token, refresh_token=colliding_same_both.refresh_token + ).count() + == 1 + ) + + def test_colliding_monitor(self): + owner = self.create_exhaustive_user("owner") + invited = self.create_exhaustive_user("invited") + self.create_exhaustive_organization("some-org", owner, invited) + + # Take note of a `Monitor` that was created by the exhaustive organization - this is the + # one we'll be importing. + colliding = Monitor.objects.filter().first() + + with tempfile.TemporaryDirectory() as tmp_dir: + tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir) + + # After exporting and clearing the database, insert a copy of the same `Monitor` as + # the one found in the import. + colliding.organization_id = self.create_organization().id + colliding.project_id = self.create_project().id + colliding.save() + + assert Monitor.objects.count() == 1 + assert Monitor.objects.filter(guid=colliding.guid).count() == 1 + + with open(tmp_path) as tmp_file: + import_in_organization_scope(tmp_file, printer=NOOP_PRINTER) + + assert Monitor.objects.count() == 2 + assert Monitor.objects.filter(guid=colliding.guid).count() == 1 def test_colliding_org_auth_token(self): owner = self.create_exhaustive_user("owner") @@ -817,8 +849,8 @@ def test_colliding_project_key(self): member = self.create_exhaustive_user("member") self.create_exhaustive_organization("some-org", owner, invited, [member]) - # Take note of the `ProjectKey` that was created by the exhaustive organization - this is - # the one we'll be importing. + # Take note of a `ProjectKey` that was created by the exhaustive organization - this is the + # one we'll be importing. colliding = ProjectKey.objects.filter().first() with tempfile.TemporaryDirectory() as tmp_dir: @@ -895,8 +927,11 @@ def test_colliding_configs_overwrite_configs_enabled_in_config_scope(self): colliding_option = Option.objects.all().first() colliding_control_option = ControlOption.objects.all().first() colliding_relay = Relay.objects.all().first() + colliding_relay_usage = RelayUsage.objects.all().first() colliding_user_role = UserRole.objects.all().first() + old_relay_public_key = colliding_relay.public_key + old_relay_usage_public_key = colliding_relay_usage.public_key old_user_role_permissions = colliding_user_role.permissions with tempfile.TemporaryDirectory() as tmp_dir: @@ -904,16 +939,23 @@ def test_colliding_configs_overwrite_configs_enabled_in_config_scope(self): colliding_option.value = "y" colliding_option.save() + colliding_control_option.value = "z" colliding_control_option.save() + colliding_relay.public_key = "invalid" colliding_relay.save() + + colliding_relay_usage.public_key = "invalid" + colliding_relay_usage.save() + colliding_user_role.permissions = ["other.admin"] colliding_user_role.save() assert Option.objects.count() == 1 assert ControlOption.objects.count() == 1 assert Relay.objects.count() == 1 + assert RelayUsage.objects.count() == 1 assert UserRole.objects.count() == 1 with open(tmp_path) as tmp_file: @@ -923,15 +965,15 @@ def test_colliding_configs_overwrite_configs_enabled_in_config_scope(self): assert Option.objects.count() == 1 assert Option.objects.filter(value__exact="a").exists() - assert not Option.objects.filter(value__exact="y").exists() assert ControlOption.objects.count() == 1 assert ControlOption.objects.filter(value__exact="b").exists() - assert not ControlOption.objects.filter(value__exact="z").exists() assert Relay.objects.count() == 1 assert Relay.objects.filter(public_key__exact=old_relay_public_key).exists() - assert not Relay.objects.filter(public_key__exact="invalid").exists() + + assert RelayUsage.objects.count() == 1 + assert RelayUsage.objects.filter(public_key__exact=old_relay_usage_public_key).exists() actual_user_role = UserRole.objects.first() assert len(actual_user_role.permissions) == len(old_user_role_permissions) @@ -946,24 +988,31 @@ def test_colliding_configs_overwrite_configs_disabled_in_config_scope(self): colliding_option = Option.objects.all().first() colliding_control_option = ControlOption.objects.all().first() colliding_relay = Relay.objects.all().first() + colliding_relay_usage = RelayUsage.objects.all().first() colliding_user_role = UserRole.objects.all().first() - old_relay_public_key = colliding_relay.public_key with tempfile.TemporaryDirectory() as tmp_dir: tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir) colliding_option.value = "y" colliding_option.save() + colliding_control_option.value = "z" colliding_control_option.save() + colliding_relay.public_key = "invalid" colliding_relay.save() + + colliding_relay_usage.public_key = "invalid" + colliding_relay_usage.save() + colliding_user_role.permissions = ["other.admin"] colliding_user_role.save() assert Option.objects.count() == 1 assert ControlOption.objects.count() == 1 assert Relay.objects.count() == 1 + assert RelayUsage.objects.count() == 1 assert UserRole.objects.count() == 1 with open(tmp_path) as tmp_file: @@ -972,17 +1021,17 @@ def test_colliding_configs_overwrite_configs_disabled_in_config_scope(self): ) assert Option.objects.count() == 1 - assert not Option.objects.filter(value__exact="a").exists() assert Option.objects.filter(value__exact="y").exists() assert ControlOption.objects.count() == 1 - assert not ControlOption.objects.filter(value__exact="b").exists() assert ControlOption.objects.filter(value__exact="z").exists() assert Relay.objects.count() == 1 - assert not Relay.objects.filter(public_key__exact=old_relay_public_key).exists() assert Relay.objects.filter(public_key__exact="invalid").exists() + assert RelayUsage.objects.count() == 1 + assert RelayUsage.objects.filter(public_key__exact="invalid").exists() + assert UserRole.objects.count() == 1 actual_user_role = UserRole.objects.first() assert len(actual_user_role.permissions) == 1 @@ -996,8 +1045,11 @@ def test_colliding_configs_overwrite_configs_enabled_in_global_scope(self): colliding_option = Option.objects.all().first() colliding_control_option = ControlOption.objects.all().first() colliding_relay = Relay.objects.all().first() + colliding_relay_usage = RelayUsage.objects.all().first() colliding_user_role = UserRole.objects.all().first() + old_relay_public_key = colliding_relay.public_key + old_relay_usage_public_key = colliding_relay_usage.public_key old_user_role_permissions = colliding_user_role.permissions with tempfile.TemporaryDirectory() as tmp_dir: @@ -1005,16 +1057,23 @@ def test_colliding_configs_overwrite_configs_enabled_in_global_scope(self): colliding_option.value = "y" colliding_option.save() + colliding_control_option.value = "z" colliding_control_option.save() + colliding_relay.public_key = "invalid" colliding_relay.save() + + colliding_relay_usage.public_key = "invalid" + colliding_relay_usage.save() + colliding_user_role.permissions = ["other.admin"] colliding_user_role.save() assert Option.objects.count() == 1 assert ControlOption.objects.count() == 1 assert Relay.objects.count() == 1 + assert RelayUsage.objects.count() == 1 assert UserRole.objects.count() == 1 with open(tmp_path) as tmp_file: @@ -1024,15 +1083,15 @@ def test_colliding_configs_overwrite_configs_enabled_in_global_scope(self): assert Option.objects.count() == 1 assert Option.objects.filter(value__exact="a").exists() - assert not Option.objects.filter(value__exact="y").exists() assert ControlOption.objects.count() == 1 assert ControlOption.objects.filter(value__exact="b").exists() - assert not ControlOption.objects.filter(value__exact="z").exists() assert Relay.objects.count() == 1 assert Relay.objects.filter(public_key__exact=old_relay_public_key).exists() - assert not Relay.objects.filter(public_key__exact="invalid").exists() + + assert RelayUsage.objects.count() == 1 + assert RelayUsage.objects.filter(public_key__exact=old_relay_usage_public_key).exists() actual_user_role = UserRole.objects.first() assert len(actual_user_role.permissions) == len(old_user_role_permissions) @@ -1047,24 +1106,31 @@ def test_colliding_configs_overwrite_configs_disabled_in_global_scope(self): colliding_option = Option.objects.all().first() colliding_control_option = ControlOption.objects.all().first() colliding_relay = Relay.objects.all().first() + colliding_relay_usage = RelayUsage.objects.all().first() colliding_user_role = UserRole.objects.all().first() - old_relay_public_key = colliding_relay.public_key with tempfile.TemporaryDirectory() as tmp_dir: tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir) colliding_option.value = "y" colliding_option.save() + colliding_control_option.value = "z" colliding_control_option.save() + colliding_relay.public_key = "invalid" colliding_relay.save() + + colliding_relay_usage.public_key = "invalid" + colliding_relay_usage.save() + colliding_user_role.permissions = ["other.admin"] colliding_user_role.save() assert Option.objects.count() == 1 assert ControlOption.objects.count() == 1 assert Relay.objects.count() == 1 + assert RelayUsage.objects.count() == 1 assert UserRole.objects.count() == 1 with open(tmp_path) as tmp_file: @@ -1073,29 +1139,29 @@ def test_colliding_configs_overwrite_configs_disabled_in_global_scope(self): ) assert Option.objects.count() == 1 - assert not Option.objects.filter(value__exact="a").exists() assert Option.objects.filter(value__exact="y").exists() assert ControlOption.objects.count() == 1 - assert not ControlOption.objects.filter(value__exact="b").exists() assert ControlOption.objects.filter(value__exact="z").exists() assert Relay.objects.count() == 1 - assert not Relay.objects.filter(public_key__exact=old_relay_public_key).exists() assert Relay.objects.filter(public_key__exact="invalid").exists() + assert RelayUsage.objects.count() == 1 + assert RelayUsage.objects.filter(public_key__exact="invalid").exists() + assert UserRole.objects.count() == 1 actual_user_role = UserRole.objects.first() assert len(actual_user_role.permissions) == 1 assert actual_user_role.permissions[0] == "other.admin" def test_colliding_user_with_merging_enabled_in_user_scope(self): - self.create_exhaustive_user(username="owner", email="owner@example.com") + self.create_exhaustive_user(username="owner", email="importing@example.com") with tempfile.TemporaryDirectory() as tmp_dir: tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir) with open(tmp_path) as tmp_file: - self.create_exhaustive_user(username="owner", email="owner@example.com") + self.create_exhaustive_user(username="owner", email="existing@example.com") import_in_user_scope( tmp_file, flags=ImportFlags(merge_users=True), @@ -1104,9 +1170,9 @@ def test_colliding_user_with_merging_enabled_in_user_scope(self): assert User.objects.count() == 1 assert UserIP.objects.count() == 1 - assert UserEmail.objects.count() == 1 + assert UserEmail.objects.count() == 1 # UserEmail gets overwritten assert Authenticator.objects.count() == 1 - assert Email.objects.count() == 1 + assert Email.objects.count() == 2 assert User.objects.filter(username__iexact="owner").exists() assert not User.objects.filter(username__iexact="owner-").exists() @@ -1114,13 +1180,16 @@ def test_colliding_user_with_merging_enabled_in_user_scope(self): assert User.objects.filter(is_unclaimed=True).count() == 0 assert User.objects.filter(is_unclaimed=False).count() == 1 + assert UserEmail.objects.filter(email__icontains="existing@").exists() + assert not UserEmail.objects.filter(email__icontains="importing@").exists() + def test_colliding_user_with_merging_disabled_in_user_scope(self): - self.create_exhaustive_user(username="owner", email="owner@example.com") + self.create_exhaustive_user(username="owner", email="importing@example.com") with tempfile.TemporaryDirectory() as tmp_dir: tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir) with open(tmp_path) as tmp_file: - self.create_exhaustive_user(username="owner", email="owner@example.com") + self.create_exhaustive_user(username="owner", email="existing@example.com") import_in_user_scope( tmp_file, flags=ImportFlags(merge_users=False), @@ -1131,7 +1200,7 @@ def test_colliding_user_with_merging_disabled_in_user_scope(self): assert UserIP.objects.count() == 2 assert UserEmail.objects.count() == 2 assert Authenticator.objects.count() == 1 # Only imported in global scope - assert Email.objects.count() == 1 # The two users still share the same email + assert Email.objects.count() == 2 assert User.objects.filter(username__iexact="owner").exists() assert User.objects.filter(username__icontains="owner-").exists() @@ -1139,14 +1208,17 @@ def test_colliding_user_with_merging_disabled_in_user_scope(self): assert User.objects.filter(is_unclaimed=True).count() == 1 assert User.objects.filter(is_unclaimed=False).count() == 1 + assert UserEmail.objects.filter(email__icontains="existing@").exists() + assert UserEmail.objects.filter(email__icontains="importing@").exists() + def test_colliding_user_with_merging_enabled_in_organization_scope(self): - owner = self.create_exhaustive_user(username="owner", email="owner@example.com") + owner = self.create_exhaustive_user(username="owner", email="importing@example.com") self.create_organization("some-org", owner=owner) with tempfile.TemporaryDirectory() as tmp_dir: tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir) with open(tmp_path) as tmp_file: - owner = self.create_exhaustive_user(username="owner", email="owner@example.com") + owner = self.create_exhaustive_user(username="owner", email="existing@example.com") self.create_organization("some-org", owner=owner) import_in_organization_scope( tmp_file, @@ -1156,9 +1228,9 @@ def test_colliding_user_with_merging_enabled_in_organization_scope(self): assert User.objects.count() == 1 assert UserIP.objects.count() == 1 - assert UserEmail.objects.count() == 1 + assert UserEmail.objects.count() == 1 # UserEmail gets overwritten assert Authenticator.objects.count() == 1 # Only imported in global scope - assert Email.objects.count() == 1 # Same email + assert Email.objects.count() == 2 assert User.objects.filter(username__iexact="owner").exists() assert not User.objects.filter(username__icontains="owner-").exists() @@ -1166,6 +1238,9 @@ def test_colliding_user_with_merging_enabled_in_organization_scope(self): assert User.objects.filter(is_unclaimed=True).count() == 0 assert User.objects.filter(is_unclaimed=False).count() == 1 + assert UserEmail.objects.filter(email__icontains="existing@").exists() + assert not UserEmail.objects.filter(email__icontains="importing@").exists() + assert Organization.objects.count() == 2 assert OrganizationMapping.objects.count() == 2 assert OrganizationMember.objects.count() == 2 # Same user in both orgs @@ -1190,13 +1265,13 @@ def test_colliding_user_with_merging_enabled_in_organization_scope(self): ) def test_colliding_user_with_merging_disabled_in_organization_scope(self): - owner = self.create_exhaustive_user(username="owner", email="owner@example.com") + owner = self.create_exhaustive_user(username="owner", email="importing@example.com") self.create_organization("some-org", owner=owner) with tempfile.TemporaryDirectory() as tmp_dir: tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir) with open(tmp_path) as tmp_file: - owner = self.create_exhaustive_user(username="owner", email="owner@example.com") + owner = self.create_exhaustive_user(username="owner", email="existing@example.com") self.create_organization("some-org", owner=owner) import_in_organization_scope( tmp_file, @@ -1208,7 +1283,7 @@ def test_colliding_user_with_merging_disabled_in_organization_scope(self): assert UserIP.objects.count() == 2 assert UserEmail.objects.count() == 2 assert Authenticator.objects.count() == 1 # Only imported in global scope - assert Email.objects.count() == 1 # Same email + assert Email.objects.count() == 2 assert User.objects.filter(username__iexact="owner").exists() assert User.objects.filter(username__icontains="owner-").exists() @@ -1216,6 +1291,9 @@ def test_colliding_user_with_merging_disabled_in_organization_scope(self): assert User.objects.filter(is_unclaimed=True).count() == 1 assert User.objects.filter(is_unclaimed=False).count() == 1 + assert UserEmail.objects.filter(email__icontains="existing@").exists() + assert UserEmail.objects.filter(email__icontains="importing@").exists() + assert Organization.objects.count() == 2 assert OrganizationMapping.objects.count() == 2 assert OrganizationMember.objects.count() == 2 @@ -1251,12 +1329,14 @@ def test_colliding_user_with_merging_disabled_in_organization_scope(self): ) def test_colliding_user_with_merging_enabled_in_config_scope(self): - self.create_exhaustive_user(username="owner", email="owner@example.com") + self.create_exhaustive_user(username="owner", email="importing@example.com", is_admin=True) with tempfile.TemporaryDirectory() as tmp_dir: tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir) with open(tmp_path) as tmp_file: - self.create_exhaustive_user(username="owner", email="owner@example.com") + self.create_exhaustive_user( + username="owner", email="existing@example.com", is_admin=True + ) import_in_config_scope( tmp_file, flags=ImportFlags(merge_users=True), @@ -1265,9 +1345,10 @@ def test_colliding_user_with_merging_enabled_in_config_scope(self): assert User.objects.count() == 1 assert UserIP.objects.count() == 1 - assert UserEmail.objects.count() == 1 + assert UserEmail.objects.count() == 1 # UserEmail gets overwritten + assert UserPermission.objects.count() == 1 assert Authenticator.objects.count() == 1 - assert Email.objects.count() == 1 + assert Email.objects.count() == 2 assert User.objects.filter(username__iexact="owner").exists() assert not User.objects.filter(username__iexact="owner-").exists() @@ -1275,13 +1356,18 @@ def test_colliding_user_with_merging_enabled_in_config_scope(self): assert User.objects.filter(is_unclaimed=True).count() == 0 assert User.objects.filter(is_unclaimed=False).count() == 1 + assert UserEmail.objects.filter(email__icontains="existing@").exists() + assert not UserEmail.objects.filter(email__icontains="importing@").exists() + def test_colliding_user_with_merging_disabled_in_config_scope(self): - self.create_exhaustive_user(username="owner", email="owner@example.com") + self.create_exhaustive_user(username="owner", email="importing@example.com", is_admin=True) with tempfile.TemporaryDirectory() as tmp_dir: tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir) with open(tmp_path) as tmp_file: - self.create_exhaustive_user(username="owner", email="owner@example.com") + self.create_exhaustive_user( + username="owner", email="existing@example.com", is_admin=True + ) import_in_config_scope( tmp_file, flags=ImportFlags(merge_users=False), @@ -1291,11 +1377,15 @@ def test_colliding_user_with_merging_disabled_in_config_scope(self): assert User.objects.count() == 2 assert UserIP.objects.count() == 2 assert UserEmail.objects.count() == 2 + assert UserPermission.objects.count() == 2 assert Authenticator.objects.count() == 1 # Only imported in global scope - assert Email.objects.count() == 1 # The two users still share the same email + assert Email.objects.count() == 2 assert User.objects.filter(username__iexact="owner").exists() assert User.objects.filter(username__icontains="owner-").exists() assert User.objects.filter(is_unclaimed=True).count() == 1 assert User.objects.filter(is_unclaimed=False).count() == 1 + + assert UserEmail.objects.filter(email__icontains="existing@").exists() + assert UserEmail.objects.filter(email__icontains="importing@").exists()