From ac01dd173da0cecb72a5635fef7e5f034cde4aab Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Tue, 7 Nov 2023 13:58:16 -0300 Subject: [PATCH] Improve user permissions query (#3291) The query for checking a user permission (used to get users from a Slack usergroup, for example) is timing out (and generating retries, besides affecting some use cases: [logs](https://ops.grafana-ops.net/explore?panes=%7B%22FCQ%22:%7B%22datasource%22:%22c-R8UWvVk%22,%22queries%22:%5B%7B%22refId%22:%22A%22,%22expr%22:%22%7Bnamespace%3D%5C%22amixr-prod%5C%22,%20cluster%3D%5C%22prod-us-central-0%5C%22%7D%20%7C%3D%20%5C%22Timeout%20exceeded%20in%20regular%20expression%20match.%5C%22%22,%22queryType%22:%22range%22,%22datasource%22:%7B%22type%22:%22loki%22,%22uid%22:%22c-R8UWvVk%22%7D,%22editorMode%22:%22code%22%7D%5D,%22range%22:%7B%22from%22:%22now-6h%22,%22to%22:%22now%22%7D%7D%7D&schemaVersion=1&orgId=1)): `django.db.utils.OperationalError: (3699, 'Timeout exceeded in regular expression match.')` Change to a `contains` query except for SQLite (not supported), where a simplified version of the original regex query is used. --- CHANGELOG.md | 1 + engine/apps/slack/tests/test_user_group.py | 22 ++++++++++++++++++++++ engine/apps/user_management/models/user.py | 13 +++++++++++-- engine/settings/base.py | 1 + 4 files changed, 35 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b1fe8a6..2aedaafe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fix escalation policy importance going back to default by @vadimkerr ([#3282](https://github.com/grafana/oncall/pull/3282)) +- Improve user permissions query ([#3291](https://github.com/grafana/oncall/pull/3291)) ## v1.3.54 (2023-11-06) diff --git a/engine/apps/slack/tests/test_user_group.py b/engine/apps/slack/tests/test_user_group.py index 2f4d354d..0af070f2 100644 --- a/engine/apps/slack/tests/test_user_group.py +++ b/engine/apps/slack/tests/test_user_group.py @@ -154,3 +154,25 @@ def test_populate_slack_usergroups_for_team( assert usergroup.handle == "test_handle" assert usergroup.members == ["test_user_1", "test_user_2"] assert usergroup.is_active + + +@pytest.mark.django_db +def test_get_users_from_members_for_organization( + make_organization_with_slack_team_identity, + make_slack_user_group, + make_user_with_slack_user_identity, +): + organization, slack_team_identity = make_organization_with_slack_team_identity() + + user_1, slack_user_identity_1 = make_user_with_slack_user_identity( + slack_team_identity, organization, slack_id="slack_id_1" + ) + user_2, slack_user_identity_2 = make_user_with_slack_user_identity( + slack_team_identity, organization, slack_id="slack_id_2" + ) + user_group = make_slack_user_group(slack_team_identity) + user_group.members = ["slack_id_1", "slack_id_2"] + user_group.save(update_fields=["members"]) + + users = user_group.get_users_from_members_for_organization(organization) + assert set(users) == {user_1, user_2} diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index 92829216..d6dbbd71 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -1,6 +1,7 @@ import datetime import json import logging +import re import typing from urllib.parse import urljoin @@ -34,6 +35,10 @@ if typing.TYPE_CHECKING: logger = logging.getLogger(__name__) +class PermissionsQuery(typing.TypedDict): + permissions__contains: typing.Dict + + class PermissionsRegexQuery(typing.TypedDict): permissions__regex: str @@ -387,7 +392,7 @@ class User(models.Model): @staticmethod def build_permissions_query( permission: LegacyAccessControlCompatiblePermission, organization - ) -> typing.Union[PermissionsRegexQuery, RoleInQuery]: + ) -> typing.Union[PermissionsQuery, PermissionsRegexQuery, RoleInQuery]: """ This method returns a django query filter that is compatible with RBAC as well as legacy "basic" role based authorization. If a permission is provided we simply do @@ -399,7 +404,11 @@ class User(models.Model): """ if organization.is_rbac_permissions_enabled: # https://stackoverflow.com/a/50251879 - return PermissionsRegexQuery(permissions__regex=r".*{0}.*".format(permission.value)) + if settings.DATABASE_TYPE == settings.DATABASE_TYPES.SQLITE3: + # https://docs.djangoproject.com/en/4.2/topics/db/queries/#contains + return PermissionsRegexQuery(permissions__regex=re.escape(permission.value)) + required_permission = {"action": permission.value} + return PermissionsQuery(permissions__contains=[required_permission]) return RoleInQuery(role__lte=permission.fallback_role.value) def get_or_create_notification_policies(self, important=False): diff --git a/engine/settings/base.py b/engine/settings/base.py index cd58cb26..bbc14bb6 100644 --- a/engine/settings/base.py +++ b/engine/settings/base.py @@ -120,6 +120,7 @@ DATABASE_DEFAULTS = { }, } +DATABASE_TYPES = DatabaseTypes DATABASE_NAME = os.getenv("DATABASE_NAME") or os.getenv("MYSQL_DB_NAME") DATABASE_USER = os.getenv("DATABASE_USER") or os.getenv("MYSQL_USER") DATABASE_PASSWORD = os.getenv("DATABASE_PASSWORD") or os.getenv("MYSQL_PASSWORD")