Skip to content

Commit a4d19ab

Browse files
authored
fix(backup): Update password comparator to handle SSO users (#68223)
SSO-only users have `last_password_change` date. When we autoassign the newly imported user a password, this date gets set from its previously null value. This change makes the comparator clever enough to pick up on this, ensuring that it doesn't report the difference between the left-side null value and the right-side set one.
1 parent 2251627 commit a4d19ab

File tree

3 files changed

+1786
-15
lines changed

3 files changed

+1786
-15
lines changed

src/sentry/backup/comparators.py

Lines changed: 48 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -104,8 +104,9 @@ def __scrub__(
104104
self,
105105
left: JSONData,
106106
right: JSONData,
107-
f: Callable[[list[str]], list[str]]
108-
| Callable[[list[str]], ScrubbedData] = lambda _: ScrubbedData(),
107+
f: (
108+
Callable[[list[str]], list[str]] | Callable[[list[str]], ScrubbedData]
109+
) = lambda _: ScrubbedData(),
109110
) -> None:
110111
"""Removes all of the fields compared by this comparator from the `fields` dict, so that the
111112
remaining fields may be compared for equality. Public callers should use the inheritance-safe wrapper, `scrub`, rather than using this internal method directly.
@@ -420,11 +421,27 @@ def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[Compa
420421
# Old user, password must remain constant.
421422
if not right["fields"].get("is_unclaimed"):
422423
findings.extend(super().compare(on, left, right))
424+
425+
# Ensure that `last_password_change` did not get mutated either.
426+
lv = left["fields"].get("last_password_change", None)
427+
rv = right["fields"].get("last_password_change", None)
428+
if lv != rv:
429+
findings.append(
430+
ComparatorFinding(
431+
kind=self.get_kind(),
432+
on=on,
433+
left_pk=left["pk"],
434+
right_pk=right["pk"],
435+
reason=f"""the left value ("{lv}") of `last_password_change` was not equal to the right value ("{rv}")""",
436+
)
437+
)
423438
return findings
424439

425440
# New user, password must change.
426441
left_password = left["fields"]["password"]
427442
right_password = right["fields"]["password"]
443+
left_lpc = left["fields"].get("last_password_change") or UNIX_EPOCH
444+
right_lpc = right["fields"].get("last_password_change") or UNIX_EPOCH
428445
if left_password == right_password:
429446
left_pw_truncated = self.truncate(
430447
[left_password] if not isinstance(left_password, list) else left_password
@@ -444,6 +461,18 @@ def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[Compa
444461
)
445462
)
446463

464+
# Ensure that the `last_password_change` field was not nulled or less than the left side.
465+
if parser.parse(left_lpc) > parser.parse(right_lpc):
466+
findings.append(
467+
ComparatorFinding(
468+
kind=self.get_kind(),
469+
on=on,
470+
left_pk=left["pk"],
471+
right_pk=right["pk"],
472+
reason=f"""the left value ({left_lpc}) of `last_password_change` was not less than or equal to the right value ({right_lpc})""",
473+
)
474+
)
475+
447476
return findings
448477

449478
def truncate(self, data: list[str]) -> list[str]:
@@ -691,11 +720,17 @@ def auto_assign_datetime_equality_comparators(comps: ComparatorMap) -> None:
691720
assign = set()
692721
for f in fields:
693722
if isinstance(f, models.DateTimeField) and name in comps:
694-
date_updated_comparator = next(
695-
filter(lambda e: isinstance(e, DateUpdatedComparator), comps[name]), None
696-
)
697-
if not date_updated_comparator or f.name not in date_updated_comparator.fields:
698-
assign.add(f.name)
723+
# Only auto assign the `DatetimeEqualityComparator` if this field is not mentioned
724+
# by a conflicting comparator.
725+
possibly_conflicting = [
726+
e
727+
for e in comps[name]
728+
if isinstance(e, DateUpdatedComparator) or isinstance(e, IgnoredComparator)
729+
]
730+
assign.add(f.name)
731+
for comp in possibly_conflicting:
732+
if f.name in comp.fields:
733+
assign.remove(f.name)
699734

700735
if len(assign):
701736
found = next(
@@ -747,7 +782,7 @@ def auto_assign_foreign_key_comparators(comps: ComparatorMap) -> None:
747782

748783
# No arguments, so we lazily cache the result after the first calculation.
749784
@lru_cache(maxsize=1)
750-
def get_default_comparators():
785+
def get_default_comparators() -> dict[str, list[JSONScrubbingComparator]]:
751786
"""Helper function executed at startup time which builds the static default comparators map."""
752787

753788
from sentry.models.actor import Actor
@@ -817,11 +852,12 @@ def get_default_comparators():
817852
],
818853
"sentry.user": [
819854
AutoSuffixComparator("username"),
820-
DateUpdatedComparator("last_active", "last_password_change"),
821-
# UserPasswordComparator handles `is_unclaimed` and `password` for us. Because of
822-
# this, we can ignore the `is_unclaimed` field otherwise and scrub it from the
855+
DateUpdatedComparator("last_active"),
856+
# `UserPasswordObfuscatingComparator` handles `last_password_change`,
857+
# `is_unclaimed`, and `password` for us. Because of this, we can ignore the
858+
# `last_password_change` and`is_unclaimed` fields otherwise and scrub them from the
823859
# comparison.
824-
IgnoredComparator("is_unclaimed"),
860+
IgnoredComparator("last_password_change", "is_unclaimed"),
825861
UserPasswordObfuscatingComparator(),
826862
],
827863
"sentry.useremail": [

0 commit comments

Comments
 (0)