zilencer: Don't allow syncing mobile_pushes_received::day count.

This commit is contained in:
Mateusz Mandera 2023-10-23 14:01:49 +02:00 committed by Tim Abbott
parent 183c775603
commit b7117d51b2
3 changed files with 34 additions and 2 deletions

View File

@ -855,5 +855,10 @@ def get_count_stats(realm: Optional[Realm] = None) -> Dict[str, CountStat]:
return OrderedDict((stat.property, stat) for stat in count_stats_) return OrderedDict((stat.property, stat) for stat in count_stats_)
# These properties are tracked by the bouncer itself and therefore syncing them
# from a remote server should not be allowed - or the server would be able to interfere
# with our data.
BOUNCER_ONLY_REMOTE_COUNT_STAT_PROPERTIES = ["mobile_pushes_received::day"]
# To avoid refactoring for now COUNT_STATS can be used as before # To avoid refactoring for now COUNT_STATS can be used as before
COUNT_STATS = get_count_stats() COUNT_STATS = get_count_stats()

View File

@ -1017,6 +1017,26 @@ class AnalyticsBouncerTest(BouncerTestCase):
send_analytics_to_push_bouncer() send_analytics_to_push_bouncer()
check_counts(7, 5, 3, 2, 2) check_counts(7, 5, 3, 2, 2)
# Now create an InstallationCount with a property that's not supposed
# to be tracked by the remote server - since the bouncer itself tracks
# the RemoteInstallationCount with this property. We want to verify
# that the remote server will fail at sending analytics to the bouncer
# with such an InstallationCount - since syncing it should not be allowed.
forbidden_installation_count = InstallationCount.objects.create(
property="mobile_pushes_received::day",
end_time=end_time,
value=5,
)
with self.assertLogs(level="WARNING") as warn_log:
send_analytics_to_push_bouncer()
self.assertEqual(
warn_log.output, ["WARNING:root:Invalid property mobile_pushes_received::day"]
)
# The analytics endpoint call counts increase by 1, but the actual RemoteCounts remain unchanged,
# since syncing the data failed.
check_counts(8, 6, 3, 2, 2)
forbidden_installation_count.delete()
(realm_count_data, installation_count_data, realmauditlog_data) = build_analytics_data( (realm_count_data, installation_count_data, realmauditlog_data) = build_analytics_data(
RealmCount.objects.all(), InstallationCount.objects.all(), RealmAuditLog.objects.all() RealmCount.objects.all(), InstallationCount.objects.all(), RealmAuditLog.objects.all()
) )

View File

@ -17,7 +17,11 @@ from django.utils.translation import gettext as err_
from django.views.decorators.csrf import csrf_exempt from django.views.decorators.csrf import csrf_exempt
from pydantic import BaseModel, ConfigDict from pydantic import BaseModel, ConfigDict
from analytics.lib.counts import COUNT_STATS, do_increment_logging_stat from analytics.lib.counts import (
BOUNCER_ONLY_REMOTE_COUNT_STAT_PROPERTIES,
COUNT_STATS,
do_increment_logging_stat,
)
from corporate.lib.stripe import do_deactivate_remote_server from corporate.lib.stripe import do_deactivate_remote_server
from zerver.decorator import require_post from zerver.decorator import require_post
from zerver.lib.exceptions import JsonableError from zerver.lib.exceptions import JsonableError
@ -449,7 +453,10 @@ def validate_incoming_table_data(
) -> None: ) -> None:
last_id = get_last_id_from_server(server, model) last_id = get_last_id_from_server(server, model)
for row in rows: for row in rows:
if is_count_stat and row["property"] not in COUNT_STATS: if is_count_stat and (
row["property"] not in COUNT_STATS
or row["property"] in BOUNCER_ONLY_REMOTE_COUNT_STAT_PROPERTIES
):
raise JsonableError(_("Invalid property {property}").format(property=row["property"])) raise JsonableError(_("Invalid property {property}").format(property=row["property"]))
if row.get("id") is None: if row.get("id") is None:
# This shouldn't be possible, as submitting data like this should be # This shouldn't be possible, as submitting data like this should be