diff --git a/ee/api/test/test_organization.py b/ee/api/test/test_organization.py index cc48690395b..4d7cf41f21f 100644 --- a/ee/api/test/test_organization.py +++ b/ee/api/test/test_organization.py @@ -274,3 +274,20 @@ class TestOrganizationEnterpriseAPI(APILicensedTest): self.organization.refresh_from_db() self.assertFalse(self.organization.is_feature_available("whatever")) License.PLANS = current_plans + + def test_get_organization_restricted_teams_hidden(self): + self.organization_membership.level = OrganizationMembership.Level.MEMBER + self.organization_membership.save() + Team.objects.create( + organization=self.organization, + name="FORBIDDEN", + access_control=True, + ) + + response = self.client.get(f"/api/organizations/{self.organization.id}") + + self.assertEqual(response.status_code, 200) + self.assertListEqual( + [team["name"] for team in response.json()["teams"]], + [self.team.name], # "FORBIDDEN" excluded + ) diff --git a/ee/clickhouse/models/test/__snapshots__/test_property.ambr b/ee/clickhouse/models/test/__snapshots__/test_property.ambr index d27396834cf..cc8e77f83a0 100644 --- a/ee/clickhouse/models/test/__snapshots__/test_property.ambr +++ b/ee/clickhouse/models/test/__snapshots__/test_property.ambr @@ -57,7 +57,7 @@ --- # name: test_parse_groups_persons_edge_case_with_single_filter ( - 'AND ( has(%(vglobalperson_0)s, "pmat_email"))', + 'AND ( has(%(vglobalperson_0)s, replaceRegexpAll(JSONExtractRaw(person_props, %(kglobalperson_0)s), \'^"|"$\', \'\')))', { 'kglobalperson_0': 'email', 'vglobalperson_0': [ diff --git a/ee/clickhouse/views/test/__snapshots__/test_clickhouse_experiment_secondary_results.ambr b/ee/clickhouse/views/test/__snapshots__/test_clickhouse_experiment_secondary_results.ambr index 8fd35450587..2d1aa22e3be 100644 --- a/ee/clickhouse/views/test/__snapshots__/test_clickhouse_experiment_secondary_results.ambr +++ b/ee/clickhouse/views/test/__snapshots__/test_clickhouse_experiment_secondary_results.ambr @@ -1,6 +1,6 @@ # name: ClickhouseTestExperimentSecondaryResults.test_basic_secondary_metric_results ' - /* user_id:131 celery:posthog.celery.sync_insight_caching_state */ + /* user_id:132 celery:posthog.celery.sync_insight_caching_state */ SELECT team_id, date_diff('second', max(timestamp), now()) AS age FROM events diff --git a/posthog/api/organization.py b/posthog/api/organization.py index b0de79ff522..38e387e1eee 100644 --- a/posthog/api/organization.py +++ b/posthog/api/organization.py @@ -114,12 +114,8 @@ class OrganizationSerializer(serializers.ModelSerializer, UserPermissionsSeriali return membership.level if membership is not None else None def get_teams(self, instance: Organization) -> List[Dict[str, Any]]: - teams = cast( - List[Dict[str, Any]], - TeamBasicSerializer(instance.teams.all(), context=self.context, many=True).data, - ) - visible_team_ids = set(self.user_permissions.team_ids_visible_for_user) - return [team for team in teams if team["id"] in visible_team_ids] + visible_teams = instance.teams.filter(id__in=self.user_permissions.team_ids_visible_for_user) + return TeamBasicSerializer(visible_teams, context=self.context, many=True).data # type: ignore def get_metadata(self, instance: Organization) -> Dict[str, Union[str, int, object]]: return { diff --git a/posthog/test/test_user_permissions.py b/posthog/test/test_user_permissions.py index b0562dbca57..4d074d0a264 100644 --- a/posthog/test/test_user_permissions.py +++ b/posthog/test/test_user_permissions.py @@ -45,6 +45,22 @@ class TestUserTeamPermissions(BaseTest, WithPermissionsBase): with self.assertNumQueries(1): assert permissions.team(self.team).effective_membership_level is None + def test_team_effective_membership_level_membership_isolation(self): + self.team.access_control = True + self.team.save() + ExplicitTeamMembership.objects.create( + team=self.team, + parent_membership=self.organization_membership, + ) + forbidden_team = Team.objects.create( + organization=self.organization, + name="FORBIDDEN", + access_control=True, + ) + permissions = UserPermissions(user=self.user) + with self.assertNumQueries(2): + assert permissions.team(forbidden_team).effective_membership_level is None + def test_team_effective_membership_level_with_explicit_membership_returns_current_level(self): self.team.access_control = True self.team.save() diff --git a/posthog/user_permissions.py b/posthog/user_permissions.py index 4d1a13fa768..6630cded580 100644 --- a/posthog/user_permissions.py +++ b/posthog/user_permissions.py @@ -72,7 +72,6 @@ class UserPermissions: candidate_teams = Team.objects.filter(organization_id__in=self.organizations.keys()).only( "pk", "organization_id", "access_control" ) - return [team.pk for team in candidate_teams if self.team(team).effective_membership_level is not None] # Cached properties/functions for efficient lookups in other classes @@ -96,7 +95,7 @@ class UserPermissions: return {membership.organization_id: membership for membership in memberships} @cached_property - def explicit_team_memberships(self) -> Dict[UUID, Any]: + def explicit_team_memberships(self) -> Dict[int, Any]: try: from ee.models import ExplicitTeamMembership except ImportError: @@ -104,8 +103,8 @@ class UserPermissions: memberships = ExplicitTeamMembership.objects.filter( parent_membership_id__in=[membership.pk for membership in self.organization_memberships.values()] - ).only("parent_membership_id", "level") - return {membership.parent_membership_id: membership.level for membership in memberships} + ).only("parent_membership_id", "level", "team_id") + return {membership.team_id: membership.level for membership in memberships} @cached_property def dashboard_privileges(self) -> Dict[int, Dashboard.PrivilegeLevel]: @@ -168,7 +167,7 @@ class UserTeamPermissions: ): return organization_membership.level - explicit_membership_level = self.p.explicit_team_memberships.get(organization_membership.pk) + explicit_membership_level = self.p.explicit_team_memberships.get(self.team.id) if explicit_membership_level is not None: return max(explicit_membership_level, organization_membership.level) # Only organizations admins and above get implicit project membership