From cf2a01edc627cca6c46f24f766dbbb26c1928089 Mon Sep 17 00:00:00 2001 From: yazzylazy Date: Sun, 12 Jul 2026 18:36:23 -0400 Subject: [PATCH 01/10] 6009: add organizations to invitation querysets and sync Extends the channel-invitation flow to support organization invitations, per #6009 (following #6008's Invitation.organization field): - Add Organization.filter_edit_queryset/filter_view_queryset based on active OrganizationRole membership, and extend Invitation's equivalents to grant org admins edit access and org members view access. - Allow InvitationSerializer to accept an organization in place of (or alongside) a channel, requiring at least one of the two. - Teach the /sync endpoint's permission gate to recognize organization- scoped changes, since org admins acting on another user's invitation don't fit the existing channel-scoped or self-scoped checks. --- contentcuration/contentcuration/models.py | 50 ++++- .../contentcuration/tests/test_models.py | 170 +++++++++++++++ .../contentcuration/tests/testdata.py | 16 ++ .../tests/viewsets/test_invitation.py | 195 ++++++++++++++++++ .../contentcuration/viewsets/invitation.py | 31 ++- .../contentcuration/viewsets/sync/endpoint.py | 33 +++ 6 files changed, 493 insertions(+), 2 deletions(-) diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index 74e3287904..7de43e3dc7 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -1894,6 +1894,38 @@ class Meta: def __str__(self): return self.name + @classmethod + def filter_edit_queryset(cls, queryset, user): + if user.is_anonymous: + return queryset.none() + + if user.is_admin: + return queryset + + return queryset.filter( + user_roles__user=user, + user_roles__role=ORGANIZATION_ADMIN, + user_roles__status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ).distinct() + + @classmethod + def filter_view_queryset(cls, queryset, user): + if user.is_anonymous: + return queryset.none() + + if user.is_admin: + return queryset + + return queryset.filter( + user_roles__user=user, + user_roles__role__in=[ + ORGANIZATION_ADMIN, + ORGANIZATION_EDITOR, + ORGANIZATION_VIEWER, + ], + user_roles__status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ).distinct() + class OrganizationRole(models.Model): """ @@ -3788,7 +3820,14 @@ def filter_edit_queryset(cls, queryset, user): return queryset return queryset.filter( - Q(email__iexact=user.email) | Q(sender=user) | Q(channel__editors=user) + Q(email__iexact=user.email) + | Q(sender=user) + | Q(channel__editors=user) + | Q( + organization__user_roles__user=user, + organization__user_roles__role=ORGANIZATION_ADMIN, + organization__user_roles__status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ) ).distinct() @classmethod @@ -3803,6 +3842,15 @@ def filter_view_queryset(cls, queryset, user): | Q(sender=user) | Q(channel__editors=user) | Q(channel__viewers=user) + | Q( + organization__user_roles__user=user, + organization__user_roles__role__in=[ + ORGANIZATION_ADMIN, + ORGANIZATION_EDITOR, + ORGANIZATION_VIEWER, + ], + organization__user_roles__status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ) ).distinct() diff --git a/contentcuration/contentcuration/tests/test_models.py b/contentcuration/contentcuration/tests/test_models.py index eaf4e0370b..bf891fc48a 100644 --- a/contentcuration/contentcuration/tests/test_models.py +++ b/contentcuration/contentcuration/tests/test_models.py @@ -16,6 +16,15 @@ from contentcuration.constants import channel_history from contentcuration.constants import community_library_submission from contentcuration.constants import user_history +from contentcuration.constants.organization_roles import ORGANIZATION_ADMIN +from contentcuration.constants.organization_roles import ORGANIZATION_EDITOR +from contentcuration.constants.organization_roles import ( + ORGANIZATION_ROLE_STATUS_ACTIVE, +) +from contentcuration.constants.organization_roles import ( + ORGANIZATION_ROLE_STATUS_PENDING, +) +from contentcuration.constants.organization_roles import ORGANIZATION_VIEWER from contentcuration.models import AssessmentItem from contentcuration.models import AuditedSpecialPermissionsLicense from contentcuration.models import Change @@ -34,6 +43,8 @@ from contentcuration.models import Language from contentcuration.models import License from contentcuration.models import object_storage_name +from contentcuration.models import Organization +from contentcuration.models import OrganizationRole from contentcuration.models import RecommendationsEvent from contentcuration.models import RecommendationsInteractionEvent from contentcuration.models import User @@ -309,6 +320,165 @@ def create_change(server_rev, applied): self.assertEqual(channel.get_server_rev(), 2) +class OrganizationTestCase(PermissionQuerysetTestCase): + @property + def base_queryset(self): + return Organization.objects.all() + + def test_filter_edit_queryset__admin_role(self): + organization = testdata.organization() + user = testdata.user() + + queryset = Organization.filter_edit_queryset(self.base_queryset, user=user) + self.assertQuerysetDoesNotContain(queryset, pk=organization.id) + + OrganizationRole.objects.create( + user=user, + organization=organization, + role=ORGANIZATION_ADMIN, + status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ) + queryset = Organization.filter_edit_queryset(self.base_queryset, user=user) + self.assertQuerysetContains(queryset, pk=organization.id) + + def test_filter_edit_queryset__editor_role_cannot_edit(self): + organization = testdata.organization() + user = testdata.user() + OrganizationRole.objects.create( + user=user, + organization=organization, + role=ORGANIZATION_EDITOR, + status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ) + + queryset = Organization.filter_edit_queryset(self.base_queryset, user=user) + self.assertQuerysetDoesNotContain(queryset, pk=organization.id) + + def test_filter_edit_queryset__pending_admin_cannot_edit(self): + organization = testdata.organization() + user = testdata.user() + OrganizationRole.objects.create( + user=user, + organization=organization, + role=ORGANIZATION_ADMIN, + status=ORGANIZATION_ROLE_STATUS_PENDING, + ) + + queryset = Organization.filter_edit_queryset(self.base_queryset, user=user) + self.assertQuerysetDoesNotContain(queryset, pk=organization.id) + + def test_filter_edit_queryset__anonymous(self): + organization = testdata.organization() + + queryset = Organization.filter_edit_queryset( + self.base_queryset, user=self.anonymous_user + ) + self.assertQuerysetDoesNotContain(queryset, pk=organization.id) + + def test_filter_view_queryset__viewer_role(self): + organization = testdata.organization() + user = testdata.user() + + queryset = Organization.filter_view_queryset(self.base_queryset, user=user) + self.assertQuerysetDoesNotContain(queryset, pk=organization.id) + + OrganizationRole.objects.create( + user=user, + organization=organization, + role=ORGANIZATION_VIEWER, + status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ) + queryset = Organization.filter_view_queryset(self.base_queryset, user=user) + self.assertQuerysetContains(queryset, pk=organization.id) + + def test_filter_view_queryset__anonymous(self): + organization = testdata.organization() + + queryset = Organization.filter_view_queryset( + self.base_queryset, user=self.anonymous_user + ) + self.assertQuerysetDoesNotContain(queryset, pk=organization.id) + + +class InvitationOrganizationTestCase(PermissionQuerysetTestCase): + @property + def base_queryset(self): + return Invitation.objects.all() + + def _make_org_invitation(self): + organization = testdata.organization() + invitee = testdata.user(email="org-invitee@le.com") + invitation = Invitation.objects.create( + email=invitee.email, organization=organization + ) + return organization, invitation + + def test_filter_edit_queryset__organization_admin(self): + organization, invitation = self._make_org_invitation() + user = testdata.user() + + queryset = Invitation.filter_edit_queryset(self.base_queryset, user=user) + self.assertQuerysetDoesNotContain(queryset, pk=invitation.id) + + OrganizationRole.objects.create( + user=user, + organization=organization, + role=ORGANIZATION_ADMIN, + status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ) + queryset = Invitation.filter_edit_queryset(self.base_queryset, user=user) + self.assertQuerysetContains(queryset, pk=invitation.id) + + def test_filter_edit_queryset__organization_editor_cannot_edit(self): + organization, invitation = self._make_org_invitation() + user = testdata.user() + OrganizationRole.objects.create( + user=user, + organization=organization, + role=ORGANIZATION_EDITOR, + status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ) + + queryset = Invitation.filter_edit_queryset(self.base_queryset, user=user) + self.assertQuerysetDoesNotContain(queryset, pk=invitation.id) + + def test_filter_view_queryset__organization_editor(self): + organization, invitation = self._make_org_invitation() + user = testdata.user() + + queryset = Invitation.filter_view_queryset(self.base_queryset, user=user) + self.assertQuerysetDoesNotContain(queryset, pk=invitation.id) + + OrganizationRole.objects.create( + user=user, + organization=organization, + role=ORGANIZATION_EDITOR, + status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ) + queryset = Invitation.filter_view_queryset(self.base_queryset, user=user) + self.assertQuerysetContains(queryset, pk=invitation.id) + + def test_filter_view_queryset__organization_viewer(self): + organization, invitation = self._make_org_invitation() + user = testdata.user() + OrganizationRole.objects.create( + user=user, + organization=organization, + role=ORGANIZATION_VIEWER, + status=ORGANIZATION_ROLE_STATUS_ACTIVE, + ) + + queryset = Invitation.filter_view_queryset(self.base_queryset, user=user) + self.assertQuerysetContains(queryset, pk=invitation.id) + + def test_filter_view_queryset__unrelated_user(self): + organization, invitation = self._make_org_invitation() + user = testdata.user() + + queryset = Invitation.filter_view_queryset(self.base_queryset, user=user) + self.assertQuerysetDoesNotContain(queryset, pk=invitation.id) + + class ContentNodeTestCase(PermissionQuerysetTestCase): @property def base_queryset(self): diff --git a/contentcuration/contentcuration/tests/testdata.py b/contentcuration/contentcuration/tests/testdata.py index 962d2e0a5b..52aa11de5a 100644 --- a/contentcuration/contentcuration/tests/testdata.py +++ b/contentcuration/contentcuration/tests/testdata.py @@ -19,6 +19,10 @@ from contentcuration.constants import ( community_library_submission as community_library_submission_constants, ) +from contentcuration.constants.organization_roles import ORGANIZATION_ADMIN +from contentcuration.constants.organization_roles import ( + ORGANIZATION_ROLE_STATUS_ACTIVE, +) from contentcuration.tests.utils import mixer @@ -253,6 +257,18 @@ def channel(name="testchannel"): return channel +def organization(name="Test Organization"): + return cc.Organization.objects.create(name=name) + + +def organization_role( + user, organization, role=ORGANIZATION_ADMIN, status=ORGANIZATION_ROLE_STATUS_ACTIVE +): + return cc.OrganizationRole.objects.create( + user=user, organization=organization, role=role, status=status + ) + + def random_string(chars=10): """ Generate a random string diff --git a/contentcuration/contentcuration/tests/viewsets/test_invitation.py b/contentcuration/contentcuration/tests/viewsets/test_invitation.py index e07d52cb59..98a82c3ede 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_invitation.py +++ b/contentcuration/contentcuration/tests/viewsets/test_invitation.py @@ -3,6 +3,11 @@ from django.urls import reverse from contentcuration import models +from contentcuration.constants.organization_roles import ORGANIZATION_ADMIN +from contentcuration.constants.organization_roles import ORGANIZATION_EDITOR +from contentcuration.constants.organization_roles import ( + ORGANIZATION_ROLE_STATUS_ACTIVE, +) from contentcuration.tests import testdata from contentcuration.tests.base import StudioAPITestCase from contentcuration.tests.viewsets.base import generate_create_event @@ -346,6 +351,196 @@ def test_delete_invitations(self): pass +class OrganizationInvitationSyncTestCase(SyncTestMixin, StudioAPITestCase): + @property + def invitation_metadata(self): + return { + "id": uuid.uuid4().hex, + "organization": self.organization.id, + "email": self.invited_user.email, + } + + def setUp(self): + super(OrganizationInvitationSyncTestCase, self).setUp() + self.organization = testdata.organization() + self.org_admin = testdata.user("org-admin@inc.com") + testdata.organization_role(self.org_admin, self.organization) + self.invited_user = testdata.user("org-invitee@inc.com") + self.client.force_authenticate(user=self.org_admin) + + def test_create_organization_invitation(self): + invitation = self.invitation_metadata + response = self.sync_changes( + [ + generate_create_event( + invitation["id"], + INVITATION, + invitation, + organization_id=self.organization.id, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + try: + models.Invitation.objects.get(id=invitation["id"]) + except models.Invitation.DoesNotExist: + self.fail("Organization invitation was not created") + + def test_create_organization_invitation_by_non_admin_rejected(self): + editor = testdata.user("org-editor@inc.com") + testdata.organization_role(editor, self.organization, role=ORGANIZATION_EDITOR) + self.client.force_authenticate(user=editor) + + invitation = self.invitation_metadata + response = self.sync_changes( + [ + generate_create_event( + invitation["id"], + INVITATION, + invitation, + organization_id=self.organization.id, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + try: + models.Invitation.objects.get(id=invitation["id"]) + self.fail("Organization invitation was created by a non-admin") + except models.Invitation.DoesNotExist: + pass + + def test_create_invitation_requires_channel_or_organization(self): + self.client.force_authenticate(user=self.invited_user) + invitation = { + "id": uuid.uuid4().hex, + "email": self.invited_user.email, + } + response = self.sync_changes( + [ + generate_create_event( + invitation["id"], + INVITATION, + invitation, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + try: + models.Invitation.objects.get(id=invitation["id"]) + self.fail("Invitation without channel or organization was created") + except models.Invitation.DoesNotExist: + pass + + def test_accept_organization_invitation_creates_role(self): + invitation = models.Invitation.objects.create( + id=uuid.uuid4().hex, + organization=self.organization, + email=self.invited_user.email, + sender=self.org_admin, + ) + self.client.force_authenticate(user=self.invited_user) + response = self.sync_changes( + [ + generate_update_event( + invitation.id, + INVITATION, + {"accepted": True}, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + invitation.refresh_from_db() + self.assertTrue(invitation.accepted) + role = models.OrganizationRole.objects.get( + user=self.invited_user, organization=self.organization + ) + self.assertEqual(role.role, ORGANIZATION_EDITOR) + self.assertEqual(role.status, ORGANIZATION_ROLE_STATUS_ACTIVE) + + def test_revoke_organization_invitation_by_admin(self): + invitation = models.Invitation.objects.create( + id=uuid.uuid4().hex, + organization=self.organization, + email=self.invited_user.email, + sender=self.org_admin, + ) + response = self.sync_changes( + [ + generate_update_event( + invitation.id, + INVITATION, + {"revoked": True}, + organization_id=self.organization.id, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + invitation.refresh_from_db() + self.assertTrue(invitation.revoked) + + def test_revoke_organization_invitation_by_non_admin_rejected(self): + editor = testdata.user("org-editor2@inc.com") + testdata.organization_role(editor, self.organization, role=ORGANIZATION_EDITOR) + self.client.force_authenticate(user=editor) + + invitation = models.Invitation.objects.create( + id=uuid.uuid4().hex, + organization=self.organization, + email=self.invited_user.email, + sender=self.org_admin, + ) + response = self.sync_changes( + [ + generate_update_event( + invitation.id, + INVITATION, + {"revoked": True}, + organization_id=self.organization.id, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + invitation.refresh_from_db() + self.assertFalse(invitation.revoked) + + def test_channel_invitation_with_organization_admin_role(self): + channel = testdata.channel() + channel.editors.add(self.org_admin) + invitation = models.Invitation.objects.create( + id=uuid.uuid4().hex, + channel=channel, + organization=self.organization, + email=self.invited_user.email, + sender=self.org_admin, + share_mode="admin", + ) + self.client.force_authenticate(user=self.invited_user) + response = self.sync_changes( + [ + generate_update_event( + invitation.id, + INVITATION, + {"accepted": True}, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + invitation.refresh_from_db() + self.assertTrue(invitation.accepted) + self.assertTrue(channel.editors.filter(pk=self.invited_user.id).exists()) + role = models.OrganizationRole.objects.get( + user=self.invited_user, organization=self.organization + ) + self.assertEqual(role.role, ORGANIZATION_ADMIN) + + class CRUDTestCase(StudioAPITestCase): @property def invitation_metadata(self): diff --git a/contentcuration/contentcuration/viewsets/invitation.py b/contentcuration/contentcuration/viewsets/invitation.py index 75f40149cf..e26b2f2be9 100644 --- a/contentcuration/contentcuration/viewsets/invitation.py +++ b/contentcuration/contentcuration/viewsets/invitation.py @@ -10,6 +10,7 @@ from contentcuration.models import Change from contentcuration.models import Channel from contentcuration.models import Invitation +from contentcuration.models import Organization from contentcuration.viewsets.base import BulkListSerializer from contentcuration.viewsets.base import BulkModelSerializer from contentcuration.viewsets.base import ValuesViewset @@ -25,7 +26,12 @@ class InvitationSerializer(BulkModelSerializer): accepted = serializers.BooleanField(read_only=True) declined = serializers.BooleanField(read_only=True) - channel = UserFilteredPrimaryKeyRelatedField(queryset=Channel.objects.all()) + channel = UserFilteredPrimaryKeyRelatedField( + queryset=Channel.objects.all(), required=False + ) + organization = UserFilteredPrimaryKeyRelatedField( + queryset=Organization.objects.all(), required=False + ) class Meta: model = Invitation @@ -36,12 +42,24 @@ class Meta: "revoked", "email", "channel", + "organization", "share_mode", "first_name", "last_name", ) list_serializer_class = BulkListSerializer + def validate(self, data): + channel = data.get("channel", getattr(self.instance, "channel", None)) + organization = data.get( + "organization", getattr(self.instance, "organization", None) + ) + if not channel and not organization: + raise serializers.ValidationError( + "Invitation must specify either a channel or an organization." + ) + return data + def create(self, validated_data): # Need to remove default values for these non-model fields here if "request" in self.context: @@ -88,12 +106,14 @@ def get_fields(self): class InvitationFilter(FilterSet): invited = CharFilter(method="filter_invited") channel = CharFilter(method="filter_channel") + organization = CharFilter(method="filter_organization") class Meta: model = Invitation fields = ( "invited", "channel", + "organization", ) def filter_invited(self, queryset, name, value): @@ -102,6 +122,9 @@ def filter_invited(self, queryset, name, value): def filter_channel(self, queryset, name, value): return queryset.filter(channel_id=value) + def filter_organization(self, queryset, name, value): + return queryset.filter(organization_id=value) + def get_sender_name(item): return "{} {}".format(item.get("sender__first_name"), item.get("sender__last_name")) @@ -124,8 +147,10 @@ class InvitationViewSet(ValuesViewset): "sender__first_name", "sender__last_name", "channel_id", + "organization_id", "share_mode", "channel__name", + "organization__name", ) field_map = { "first_name": "invited__first_name", @@ -133,6 +158,8 @@ class InvitationViewSet(ValuesViewset): "sender_name": get_sender_name, "channel_name": "channel__name", "channel": "channel_id", + "organization_name": "organization__name", + "organization": "organization_id", } def perform_update(self, serializer): @@ -163,6 +190,7 @@ def accept(self, request, pk=None): INVITATION, {"accepted": True}, channel_id=invitation.channel_id, + user_id=request.user.id, ), applied=True, created_by_id=request.user.id, @@ -181,6 +209,7 @@ def decline(self, request, pk=None): INVITATION, {"declined": True}, channel_id=invitation.channel_id, + user_id=request.user.id, ), applied=True, created_by_id=request.user.id, diff --git a/contentcuration/contentcuration/viewsets/sync/endpoint.py b/contentcuration/contentcuration/viewsets/sync/endpoint.py index 33ff296623..dbf4b4be6e 100644 --- a/contentcuration/contentcuration/viewsets/sync/endpoint.py +++ b/contentcuration/contentcuration/viewsets/sync/endpoint.py @@ -16,6 +16,8 @@ from contentcuration.models import Change from contentcuration.models import Channel from contentcuration.models import CustomTaskMetadata +from contentcuration.models import Organization +from contentcuration.models import User from contentcuration.tasks import apply_channel_changes_task from contentcuration.tasks import apply_user_changes_task from contentcuration.viewsets.sync.constants import CHANNEL @@ -65,6 +67,17 @@ def handle_changes(self, request): .values_list("id", flat=True) .distinct() ).union(created_channel_ids) + change_organization_ids = set( + x.get("organization_id") for x in changes if x.get("organization_id") + ) + allowed_org_ids = set( + Organization.filter_edit_queryset( + Organization.objects.filter(id__in=change_organization_ids), + request.user, + ) + .values_list("id", flat=True) + .distinct() + ) # Allow changes that are either: # Not related to a channel and instead related to the user if the user is the current user. user_only_changes = [] @@ -81,6 +94,17 @@ def handle_changes(self, request): user_only_changes.append(c) elif c.get("channel_id") in allowed_ids: channel_changes.append(c) + elif ( + c.get("channel_id") is None + and c.get("organization_id") in allowed_org_ids + ): + # Organization-scoped changes (e.g. organization invitations) have + # no channel, and are frequently made by an org admin on behalf of + # another user, so they can't rely on the user-self check above. + # They're routed through the same per-user change queue instead of + # a dedicated per-organization one, since they otherwise have the + # same "no channel" shape as user-only changes. + user_only_changes.append(c) else: disallowed_changes.append(c) change_models = Change.create_changes( @@ -92,6 +116,15 @@ def handle_changes(self, request): apply_user_changes_task.fetch_or_enqueue( request.user, user_id=request.user.id ) + other_target_user_ids = set( + c.get("user_id") + for c in user_only_changes + if c.get("user_id") and c.get("user_id") != request.user.id + ) + for target_user in User.objects.filter(id__in=other_target_user_ids): + apply_user_changes_task.fetch_or_enqueue( + target_user, user_id=target_user.id + ) for channel_id in allowed_ids: apply_channel_changes_task.fetch_or_enqueue( request.user, channel_id=channel_id From 2045c38946ea28037f15c01fcb35086c60960d64 Mon Sep 17 00:00:00 2001 From: yazzylazy Date: Mon, 13 Jul 2026 12:05:06 -0400 Subject: [PATCH 02/10] fix: add organization_id support to sync event builders, fix test fixtures - generate_create_event/generate_update_event/generate_delete_event (viewsets/sync/utils.py) never gained an organization_id parameter when handle_changes() was taught to read it, causing a TypeError in every organization invitation test that passed organization_id. - Two OrganizationInvitationSyncTestCase fixtures were missing invited=self.invited_user, which InvitationSerializer.get_fields() requires to unlock the accepted field for sync-based updates, matching the existing SyncTestCase fixture pattern. --- .../tests/viewsets/test_invitation.py | 2 ++ .../contentcuration/viewsets/sync/utils.py | 28 ++++++++++++++----- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_invitation.py b/contentcuration/contentcuration/tests/viewsets/test_invitation.py index 98a82c3ede..5d27d4b354 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_invitation.py +++ b/contentcuration/contentcuration/tests/viewsets/test_invitation.py @@ -439,6 +439,7 @@ def test_accept_organization_invitation_creates_role(self): id=uuid.uuid4().hex, organization=self.organization, email=self.invited_user.email, + invited=self.invited_user, sender=self.org_admin, ) self.client.force_authenticate(user=self.invited_user) @@ -517,6 +518,7 @@ def test_channel_invitation_with_organization_admin_role(self): channel=channel, organization=self.organization, email=self.invited_user.email, + invited=self.invited_user, sender=self.org_admin, share_mode="admin", ) diff --git a/contentcuration/contentcuration/viewsets/sync/utils.py b/contentcuration/contentcuration/viewsets/sync/utils.py index 8f5ce98a05..18e90863e2 100644 --- a/contentcuration/contentcuration/viewsets/sync/utils.py +++ b/contentcuration/contentcuration/viewsets/sync/utils.py @@ -23,7 +23,7 @@ def validate_table(table): raise ValueError("{} is not a valid table name".format(table)) -def _generate_event(key, table, event_type, channel_id, user_id): +def _generate_event(key, table, event_type, channel_id, user_id, organization_id=None): validate_table(table) event = { "key": key, @@ -34,23 +34,37 @@ def _generate_event(key, table, event_type, channel_id, user_id): event["channel_id"] = channel_id if user_id: event["user_id"] = user_id + if organization_id: + event["organization_id"] = organization_id return event -def generate_create_event(key, table, obj, channel_id=None, user_id=None): - event = _generate_event(key, table, CREATED, channel_id, user_id) +def generate_create_event( + key, table, obj, channel_id=None, user_id=None, organization_id=None +): + event = _generate_event( + key, table, CREATED, channel_id, user_id, organization_id=organization_id + ) event["obj"] = obj return event -def generate_update_event(key, table, mods, channel_id=None, user_id=None): - event = _generate_event(key, table, UPDATED, channel_id, user_id) +def generate_update_event( + key, table, mods, channel_id=None, user_id=None, organization_id=None +): + event = _generate_event( + key, table, UPDATED, channel_id, user_id, organization_id=organization_id + ) event["mods"] = mods return event -def generate_delete_event(key, table, channel_id=None, user_id=None): - return _generate_event(key, table, DELETED, channel_id, user_id) +def generate_delete_event( + key, table, channel_id=None, user_id=None, organization_id=None +): + return _generate_event( + key, table, DELETED, channel_id, user_id, organization_id=organization_id + ) def generate_move_event(key, table, target, position, channel_id=None, user_id=None): From 475ba8e058028febbceeb29282b28432517a929c Mon Sep 17 00:00:00 2001 From: yazzylazy Date: Mon, 13 Jul 2026 16:08:26 -0400 Subject: [PATCH 03/10] 6009: enforce channel/organization mutual exclusivity on Invitation Per the "Organizations XOR channels" requirement from #5971 and rtibbles' comment on #6009 confirming this was deliberately deferred to the API layer: InvitationSerializer.validate() now rejects an invitation that has both channel and organization set, not just one that has neither. This invalidates the "co-owner" scenario (one invitation with both fields set) the earlier test exercised, so that test is replaced with one asserting the combination is rejected. --- .../tests/viewsets/test_invitation.py | 39 ++++++++----------- .../contentcuration/viewsets/invitation.py | 4 ++ 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/test_invitation.py b/contentcuration/contentcuration/tests/viewsets/test_invitation.py index 5d27d4b354..b8bac2934d 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_invitation.py +++ b/contentcuration/contentcuration/tests/viewsets/test_invitation.py @@ -3,7 +3,6 @@ from django.urls import reverse from contentcuration import models -from contentcuration.constants.organization_roles import ORGANIZATION_ADMIN from contentcuration.constants.organization_roles import ORGANIZATION_EDITOR from contentcuration.constants.organization_roles import ( ORGANIZATION_ROLE_STATUS_ACTIVE, @@ -510,37 +509,33 @@ def test_revoke_organization_invitation_by_non_admin_rejected(self): invitation.refresh_from_db() self.assertFalse(invitation.revoked) - def test_channel_invitation_with_organization_admin_role(self): + def test_invitation_with_channel_and_organization_is_rejected(self): channel = testdata.channel() channel.editors.add(self.org_admin) - invitation = models.Invitation.objects.create( - id=uuid.uuid4().hex, - channel=channel, - organization=self.organization, - email=self.invited_user.email, - invited=self.invited_user, - sender=self.org_admin, - share_mode="admin", - ) - self.client.force_authenticate(user=self.invited_user) + invitation = { + "id": uuid.uuid4().hex, + "channel": channel.id, + "organization": self.organization.id, + "email": self.invited_user.email, + } response = self.sync_changes( [ - generate_update_event( - invitation.id, + generate_create_event( + invitation["id"], INVITATION, - {"accepted": True}, + invitation, + channel_id=channel.id, + organization_id=self.organization.id, user_id=self.invited_user.id, ) ], ) self.assertEqual(response.status_code, 200, response.content) - invitation.refresh_from_db() - self.assertTrue(invitation.accepted) - self.assertTrue(channel.editors.filter(pk=self.invited_user.id).exists()) - role = models.OrganizationRole.objects.get( - user=self.invited_user, organization=self.organization - ) - self.assertEqual(role.role, ORGANIZATION_ADMIN) + try: + models.Invitation.objects.get(id=invitation["id"]) + self.fail("Invitation with both channel and organization was created") + except models.Invitation.DoesNotExist: + pass class CRUDTestCase(StudioAPITestCase): diff --git a/contentcuration/contentcuration/viewsets/invitation.py b/contentcuration/contentcuration/viewsets/invitation.py index e26b2f2be9..57ad34afce 100644 --- a/contentcuration/contentcuration/viewsets/invitation.py +++ b/contentcuration/contentcuration/viewsets/invitation.py @@ -58,6 +58,10 @@ def validate(self, data): raise serializers.ValidationError( "Invitation must specify either a channel or an organization." ) + if channel and organization: + raise serializers.ValidationError( + "Invitation cannot specify both a channel and an organization." + ) return data def create(self, validated_data): From adf18304c7595bee5c04be09165ce1a72a8b9e23 Mon Sep 17 00:00:00 2001 From: yazzylazy Date: Sun, 19 Jul 2026 19:17:43 -0400 Subject: [PATCH 04/10] 6009: add full organization_id broadcast parity to Change/sync Extends the Change/sync mechanism to give organization_id the same three roles channel_id already has, rather than reusing the per-user change queue as a stopgap: - Change.organization: new FK field + migration, so organization scope is a real, queryable column instead of being buried in the opaque kwargs JSON blob. - apply_organization_changes_task: dedicated Celery task mirroring apply_channel_changes_task, draining pending organization-scoped changes. apply_user_changes_task now excludes organization-tagged rows so a change is only ever processed by one task. - handle_changes(): organization-scoped changes route through their own permission-gated bucket via Organization.filter_edit_queryset, rather than being folded into user-only changes. The self-only check stays first in priority, since a user must be able to act on their own invitation even before holding any org role (e.g. accepting it). Also covers organization_ids that end up on self-only changes, so a self-accept carrying organization_id can't get stranded unapplied. - organization_revs: full broadcast support in return_changes(), mirroring channel_revs, so any org admin sees invitation changes live, not just the acting user. - generate_create_event/update_event/delete_event: gained an organization_id parameter so events can actually carry it. - accept/decline actions and the sync-based accept path now tag organization_id on their Change events. Also drops organization_name/organization__name from the invitation viewset's values/field_map - unused and not requested; organization_id is what's actually needed by the API consumer today. --- .../migrations/0170_change_organization.py | 23 +++++ contentcuration/contentcuration/models.py | 10 +++ contentcuration/contentcuration/tasks.py | 25 +++++- .../contentcuration/tests/viewsets/base.py | 8 +- .../tests/viewsets/test_invitation.py | 85 +++++++++++++++++++ .../contentcuration/viewsets/invitation.py | 5 +- .../contentcuration/viewsets/sync/endpoint.py | 77 ++++++++++++----- 7 files changed, 208 insertions(+), 25 deletions(-) create mode 100644 contentcuration/contentcuration/migrations/0170_change_organization.py diff --git a/contentcuration/contentcuration/migrations/0170_change_organization.py b/contentcuration/contentcuration/migrations/0170_change_organization.py new file mode 100644 index 0000000000..2a5db45f69 --- /dev/null +++ b/contentcuration/contentcuration/migrations/0170_change_organization.py @@ -0,0 +1,23 @@ +import django.db.models.deletion +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + + dependencies = [ + ("contentcuration", "0169_invitation_organization"), + ] + + operations = [ + migrations.AddField( + model_name="change", + name="organization", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="contentcuration.organization", + ), + ), + ] diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index 7de43e3dc7..4086b9f35c 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -3873,6 +3873,13 @@ class Change(models.Model): channel = models.ForeignKey( Channel, null=True, blank=True, on_delete=models.CASCADE ) + # For changes related to an organization rather than a channel (e.g. organization + # invitations), so that they can be broadcast to anyone watching that organization, + # the same way channel-scoped changes are broadcast via `channel` above. + # Indexed by default because it's a ForeignKey field. + organization = models.ForeignKey( + "Organization", null=True, blank=True, on_delete=models.CASCADE + ) # For those changes related to users, store a user value instead of channel # this may be different to created_by, as changes to invitations affect individual users. # Indexed by default because it's a ForeignKey field. @@ -3916,6 +3923,7 @@ def _create_from_change( cls, created_by_id=None, channel_id=None, + organization_id=None, user_id=None, session_key=None, applied=False, @@ -3944,6 +3952,7 @@ def _create_from_change( session_id=session_key, created_by_id=created_by_id, channel_id=channel_id, + organization_id=organization_id, user_id=user_id, client_rev=rev, table=table, @@ -4005,6 +4014,7 @@ def serialize(cls, change): "table": get_attribute(change, ["table"]), "type": get_attribute(change, ["change_type"]), "channel_id": get_attribute(change, ["channel_id"]), + "organization_id": get_attribute(change, ["organization_id"]), "user_id": get_attribute(change, ["user_id"]), "created_by_id": get_attribute(change, ["created_by_id"]), "unpublishable": get_attribute(change, ["unpublishable"]), diff --git a/contentcuration/contentcuration/tasks.py b/contentcuration/contentcuration/tasks.py index 5ab9497e1b..61f10aac98 100644 --- a/contentcuration/contentcuration/tasks.py +++ b/contentcuration/contentcuration/tasks.py @@ -34,7 +34,11 @@ def apply_user_changes_task(self, user_id): from contentcuration.viewsets.sync.base import apply_changes changes_qs = Change.objects.filter( - applied=False, errored=False, user_id=user_id, channel__isnull=True + applied=False, + errored=False, + user_id=user_id, + channel__isnull=True, + organization__isnull=True, ) apply_changes(changes_qs) if changes_qs.exists(): @@ -57,6 +61,25 @@ def apply_channel_changes_task(self, channel_id): self.requeue() +@app.task(bind=True, name="apply_organization_changes") +def apply_organization_changes_task(self, organization_id): + """ + :type self: contentcuration.utils.celery.tasks.CeleryTask + :param organization_id: The organization ID for which to process changes + """ + from contentcuration.viewsets.sync.base import apply_changes + + changes_qs = Change.objects.filter( + applied=False, + errored=False, + organization_id=organization_id, + channel__isnull=True, + ) + apply_changes(changes_qs) + if changes_qs.exists(): + self.requeue() + + class CustomEmailMessage(EmailMessage): """ jayoshih: There's an issue with the django postmark backend where diff --git a/contentcuration/contentcuration/tests/viewsets/base.py b/contentcuration/contentcuration/tests/viewsets/base.py index c75b7f9549..155d6dec1b 100644 --- a/contentcuration/contentcuration/tests/viewsets/base.py +++ b/contentcuration/contentcuration/tests/viewsets/base.py @@ -106,7 +106,7 @@ class SyncTestMixin(EagerTasksTestMixin): def sync_url(self): return reverse("sync") - def sync_changes(self, changes): + def sync_changes(self, changes, organization_revs=None): channel_ids = set(c.get("channel_id") for c in changes) channel_revs = {} for channel_id in channel_ids: @@ -114,7 +114,11 @@ def sync_changes(self, changes): channel_revs[channel_id] = 0 return self.client.post( self.sync_url, - {"changes": changes, "channel_revs": channel_revs}, + { + "changes": changes, + "channel_revs": channel_revs, + "organization_revs": organization_revs or {}, + }, format="json", ) diff --git a/contentcuration/contentcuration/tests/viewsets/test_invitation.py b/contentcuration/contentcuration/tests/viewsets/test_invitation.py index b8bac2934d..215e619d7b 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_invitation.py +++ b/contentcuration/contentcuration/tests/viewsets/test_invitation.py @@ -1,6 +1,7 @@ import uuid from django.urls import reverse +from rest_framework.test import APIClient from contentcuration import models from contentcuration.constants.organization_roles import ORGANIZATION_EDITOR @@ -537,6 +538,90 @@ def test_invitation_with_channel_and_organization_is_rejected(self): except models.Invitation.DoesNotExist: pass + def test_organization_invitation_broadcast_to_other_admin(self): + other_admin = testdata.user("org-admin-2@inc.com") + testdata.organization_role(other_admin, self.organization) + + invitation = self.invitation_metadata + response = self.sync_changes( + [ + generate_create_event( + invitation["id"], + INVITATION, + invitation, + organization_id=self.organization.id, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + + other_client = APIClient() + other_client.force_authenticate(user=other_admin) + response = other_client.post( + self.sync_url, + { + "changes": [], + "channel_revs": {}, + "organization_revs": {self.organization.id: 0}, + }, + format="json", + ) + self.assertEqual(response.status_code, 200, response.content) + # force_authenticate() (used by both test clients here) bypasses Django's + # real session machinery, so every force_authenticate'd request ends up + # with session_key=None - which makes return_changes() misclassify this + # as "successes" (same session) rather than "changes" (a different + # session), even though these are two distinct users/clients. That + # session-identity distinction isn't reproducible with force_authenticate + # and isn't what this test is checking; what matters here is that + # organization_revs correctly delivered the change to this observer at + # all, so check both buckets rather than relying on that classification. + payload = response.json() + returned_keys = [ + c.get("key") for c in payload["changes"] + payload["successes"] + ] + self.assertIn(invitation["id"], returned_keys) + + def test_organization_invitation_not_broadcast_to_unrelated_user(self): + unrelated_user = testdata.user("unrelated-org-user@inc.com") + + invitation = self.invitation_metadata + response = self.sync_changes( + [ + generate_create_event( + invitation["id"], + INVITATION, + invitation, + organization_id=self.organization.id, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + + other_client = APIClient() + other_client.force_authenticate(user=unrelated_user) + response = other_client.post( + self.sync_url, + { + "changes": [], + "channel_revs": {}, + "organization_revs": {self.organization.id: 0}, + }, + format="json", + ) + self.assertEqual(response.status_code, 200, response.content) + # organization_revs is filtered to organizations the requester can view, + # so an unrelated user's request for this org's revs is dropped entirely + # and no invitation changes come back for it, in either bucket. + payload = response.json() + returned_keys = [ + c.get("key") + for c in payload.get("changes", []) + payload.get("successes", []) + ] + self.assertNotIn(invitation["id"], returned_keys) + class CRUDTestCase(StudioAPITestCase): @property diff --git a/contentcuration/contentcuration/viewsets/invitation.py b/contentcuration/contentcuration/viewsets/invitation.py index 57ad34afce..1484abc099 100644 --- a/contentcuration/contentcuration/viewsets/invitation.py +++ b/contentcuration/contentcuration/viewsets/invitation.py @@ -87,6 +87,7 @@ def update(self, instance, validated_data): "accepted": True, }, channel_id=instance.channel_id, + organization_id=instance.organization_id, ) ) @@ -154,7 +155,6 @@ class InvitationViewSet(ValuesViewset): "organization_id", "share_mode", "channel__name", - "organization__name", ) field_map = { "first_name": "invited__first_name", @@ -162,7 +162,6 @@ class InvitationViewSet(ValuesViewset): "sender_name": get_sender_name, "channel_name": "channel__name", "channel": "channel_id", - "organization_name": "organization__name", "organization": "organization_id", } @@ -194,6 +193,7 @@ def accept(self, request, pk=None): INVITATION, {"accepted": True}, channel_id=invitation.channel_id, + organization_id=invitation.organization_id, user_id=request.user.id, ), applied=True, @@ -213,6 +213,7 @@ def decline(self, request, pk=None): INVITATION, {"declined": True}, channel_id=invitation.channel_id, + organization_id=invitation.organization_id, user_id=request.user.id, ), applied=True, diff --git a/contentcuration/contentcuration/viewsets/sync/endpoint.py b/contentcuration/contentcuration/viewsets/sync/endpoint.py index dbf4b4be6e..e3b9f389e4 100644 --- a/contentcuration/contentcuration/viewsets/sync/endpoint.py +++ b/contentcuration/contentcuration/viewsets/sync/endpoint.py @@ -17,8 +17,8 @@ from contentcuration.models import Channel from contentcuration.models import CustomTaskMetadata from contentcuration.models import Organization -from contentcuration.models import User from contentcuration.tasks import apply_channel_changes_task +from contentcuration.tasks import apply_organization_changes_task from contentcuration.tasks import apply_user_changes_task from contentcuration.viewsets.sync.constants import CHANNEL from contentcuration.viewsets.sync.constants import CREATED @@ -83,6 +83,9 @@ def handle_changes(self, request): user_only_changes = [] # Related to a channel that the user is an editor for. channel_changes = [] + # Not related to a channel, but related to an organization the user + # can edit (e.g. an org admin managing another user's invitation). + organization_changes = [] # Changes that cannot be made disallowed_changes = [] for c in changes: @@ -91,6 +94,9 @@ def handle_changes(self, request): elif ( c.get("channel_id") is None and c.get("user_id") == request.user.id ): + # A user can always act on their own behalf, even if e.g. they + # don't yet have edit rights on the organization a change also + # references (accepting an org invitation is exactly this case). user_only_changes.append(c) elif c.get("channel_id") in allowed_ids: channel_changes.append(c) @@ -98,17 +104,11 @@ def handle_changes(self, request): c.get("channel_id") is None and c.get("organization_id") in allowed_org_ids ): - # Organization-scoped changes (e.g. organization invitations) have - # no channel, and are frequently made by an org admin on behalf of - # another user, so they can't rely on the user-self check above. - # They're routed through the same per-user change queue instead of - # a dedicated per-organization one, since they otherwise have the - # same "no channel" shape as user-only changes. - user_only_changes.append(c) + organization_changes.append(c) else: disallowed_changes.append(c) change_models = Change.create_changes( - user_only_changes + channel_changes, + user_only_changes + channel_changes + organization_changes, created_by_id=request.user.id, session_key=session_key, ) @@ -116,19 +116,27 @@ def handle_changes(self, request): apply_user_changes_task.fetch_or_enqueue( request.user, user_id=request.user.id ) - other_target_user_ids = set( - c.get("user_id") - for c in user_only_changes - if c.get("user_id") and c.get("user_id") != request.user.id - ) - for target_user in User.objects.filter(id__in=other_target_user_ids): - apply_user_changes_task.fetch_or_enqueue( - target_user, user_id=target_user.id - ) for channel_id in allowed_ids: apply_channel_changes_task.fetch_or_enqueue( request.user, channel_id=channel_id ) + # A change can end up with organization_id set even when it was routed + # via the self-only check above (e.g. a user accepting their own org + # invitation, who isn't an org member yet and so isn't in + # allowed_org_ids). apply_user_changes_task excludes anything with + # organization_id set, so without this such a change would never be + # picked up by any task. Cover every organization_id that actually + # ended up on a change, not just the ones routed via the dedicated + # organization_changes bucket. + all_organization_ids = allowed_org_ids.union( + c.get("organization_id") + for c in user_only_changes + organization_changes + if c.get("organization_id") + ) + for organization_id in all_organization_ids: + apply_organization_changes_task.fetch_or_enqueue( + request.user, organization_id=organization_id + ) allowed_changes = [ {"rev": c.client_rev, "server_rev": c.server_rev} for c in change_models ] @@ -150,7 +158,24 @@ def get_channel_revs(self, request): } return channel_revs - def return_changes(self, request, channel_revs): + def get_organization_revs(self, request): + organization_revs = request.data.get("organization_revs", {}) + if organization_revs: + # Filter to only the organizations that the user has permissions to view. + organization_ids = ( + Organization.filter_view_queryset( + Organization.objects.all(), request.user + ) + .filter(id__in=organization_revs.keys()) + .values_list("id", flat=True) + ) + organization_revs = { + organization_id: organization_revs[organization_id] + for organization_id in organization_ids + } + return organization_revs + + def return_changes(self, request, channel_revs, organization_revs): user_rev = request.data.get("user_rev") or 0 unapplied_revs = request.data.get("unapplied_revs", []) session_key = request.session.session_key @@ -175,12 +200,20 @@ def return_changes(self, request, channel_revs): & relevant_to_session_filter ) + for organization_id, rev in organization_revs.items(): + change_filter |= ( + Q(organization_id=organization_id) + & (unapplied_revs_filter | Q(server_rev__gt=rev)) + & relevant_to_session_filter + ) + changes_to_return = list( Change.objects.filter(change_filter) .values( "server_rev", "session_id", "channel_id", + "organization_id", "user_id", "created_by_id", "applied", @@ -228,6 +261,7 @@ def return_tasks(self, request, channel_revs): task_name__in=[ apply_channel_changes_task.name, apply_user_changes_task.name, + apply_organization_changes_task.name, ] ) .annotate( @@ -265,10 +299,13 @@ def post(self, request): } channel_revs = self.get_channel_revs(request) + organization_revs = self.get_organization_revs(request) response_payload.update(self.handle_changes(request)) - response_payload.update(self.return_changes(request, channel_revs)) + response_payload.update( + self.return_changes(request, channel_revs, organization_revs) + ) response_payload.update(self.return_tasks(request, channel_revs)) From 683d1ca689c06ddae00785251ac1e082e2661e75 Mon Sep 17 00:00:00 2001 From: yazzylazy Date: Sun, 19 Jul 2026 19:26:43 -0400 Subject: [PATCH 05/10] fix: resolve migration leaf conflict after merging unstable 0170_change_organization collided with unstable's own new 0170_merge_20260717_0136 (both depended on prior leaves). Renumbered to 0171 and repointed its dependency at the new merge migration. --- ...{0170_change_organization.py => 0171_change_organization.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename contentcuration/contentcuration/migrations/{0170_change_organization.py => 0171_change_organization.py} (89%) diff --git a/contentcuration/contentcuration/migrations/0170_change_organization.py b/contentcuration/contentcuration/migrations/0171_change_organization.py similarity index 89% rename from contentcuration/contentcuration/migrations/0170_change_organization.py rename to contentcuration/contentcuration/migrations/0171_change_organization.py index 2a5db45f69..d6e9954ff2 100644 --- a/contentcuration/contentcuration/migrations/0170_change_organization.py +++ b/contentcuration/contentcuration/migrations/0171_change_organization.py @@ -6,7 +6,7 @@ class Migration(migrations.Migration): dependencies = [ - ("contentcuration", "0169_invitation_organization"), + ("contentcuration", "0170_merge_20260717_0136"), ] operations = [ From 22b1f51db1877bb34b0cb55e6ccb779f7a687610 Mon Sep 17 00:00:00 2001 From: yazzylazy Date: Mon, 20 Jul 2026 10:56:52 -0400 Subject: [PATCH 06/10] 6009: revert Change/sync organization_id broadcast, per rtibbles review rtibbles pointed out on PR review that channel_id/user_id exist on Change specifically to decide where a change propagates to, and there was no established need for organizations to have that same live cross-admin broadcast - permission checks belong at the viewset layer (which Organization.filter_edit_queryset via UserFilteredPrimaryKeyRelatedField already handles), not as a new routing/broadcast concept on the shared Change model. Reverts: - Change.organization field and its migration - apply_organization_changes_task, and the organization exclusion on apply_user_changes_task - The dedicated organization_changes bucket, organization_revs, and get_organization_revs() in handle_changes()/return_changes() - organization_id tagging on accept/decline Change events Keeps: - The handle_changes() permission gate for organization-scoped changes (still needed - an org admin acting on someone else's invitation doesn't fit the channel-scoped or self-scoped checks), now routing through the existing per-user change queue instead of a dedicated one - Organization.filter_edit_queryset/filter_view_queryset (used by both the permission gate and the serializer's UserFilteredPrimaryKeyRelatedField) - All queryset, validation (including XOR), and read-path behavior --- .../migrations/0171_change_organization.py | 23 ----- contentcuration/contentcuration/models.py | 10 --- contentcuration/contentcuration/tasks.py | 25 +----- .../contentcuration/tests/viewsets/base.py | 8 +- .../tests/viewsets/test_invitation.py | 85 ------------------- .../contentcuration/viewsets/invitation.py | 3 - .../contentcuration/viewsets/sync/endpoint.py | 79 +++++------------ 7 files changed, 25 insertions(+), 208 deletions(-) delete mode 100644 contentcuration/contentcuration/migrations/0171_change_organization.py diff --git a/contentcuration/contentcuration/migrations/0171_change_organization.py b/contentcuration/contentcuration/migrations/0171_change_organization.py deleted file mode 100644 index d6e9954ff2..0000000000 --- a/contentcuration/contentcuration/migrations/0171_change_organization.py +++ /dev/null @@ -1,23 +0,0 @@ -import django.db.models.deletion -from django.db import migrations -from django.db import models - - -class Migration(migrations.Migration): - - dependencies = [ - ("contentcuration", "0170_merge_20260717_0136"), - ] - - operations = [ - migrations.AddField( - model_name="change", - name="organization", - field=models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.CASCADE, - to="contentcuration.organization", - ), - ), - ] diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index 68088899dd..482afa63de 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -3892,13 +3892,6 @@ class Change(models.Model): channel = models.ForeignKey( Channel, null=True, blank=True, on_delete=models.CASCADE ) - # For changes related to an organization rather than a channel (e.g. organization - # invitations), so that they can be broadcast to anyone watching that organization, - # the same way channel-scoped changes are broadcast via `channel` above. - # Indexed by default because it's a ForeignKey field. - organization = models.ForeignKey( - "Organization", null=True, blank=True, on_delete=models.CASCADE - ) # For those changes related to users, store a user value instead of channel # this may be different to created_by, as changes to invitations affect individual users. # Indexed by default because it's a ForeignKey field. @@ -3942,7 +3935,6 @@ def _create_from_change( cls, created_by_id=None, channel_id=None, - organization_id=None, user_id=None, session_key=None, applied=False, @@ -3971,7 +3963,6 @@ def _create_from_change( session_id=session_key, created_by_id=created_by_id, channel_id=channel_id, - organization_id=organization_id, user_id=user_id, client_rev=rev, table=table, @@ -4033,7 +4024,6 @@ def serialize(cls, change): "table": get_attribute(change, ["table"]), "type": get_attribute(change, ["change_type"]), "channel_id": get_attribute(change, ["channel_id"]), - "organization_id": get_attribute(change, ["organization_id"]), "user_id": get_attribute(change, ["user_id"]), "created_by_id": get_attribute(change, ["created_by_id"]), "unpublishable": get_attribute(change, ["unpublishable"]), diff --git a/contentcuration/contentcuration/tasks.py b/contentcuration/contentcuration/tasks.py index 61f10aac98..5ab9497e1b 100644 --- a/contentcuration/contentcuration/tasks.py +++ b/contentcuration/contentcuration/tasks.py @@ -34,11 +34,7 @@ def apply_user_changes_task(self, user_id): from contentcuration.viewsets.sync.base import apply_changes changes_qs = Change.objects.filter( - applied=False, - errored=False, - user_id=user_id, - channel__isnull=True, - organization__isnull=True, + applied=False, errored=False, user_id=user_id, channel__isnull=True ) apply_changes(changes_qs) if changes_qs.exists(): @@ -61,25 +57,6 @@ def apply_channel_changes_task(self, channel_id): self.requeue() -@app.task(bind=True, name="apply_organization_changes") -def apply_organization_changes_task(self, organization_id): - """ - :type self: contentcuration.utils.celery.tasks.CeleryTask - :param organization_id: The organization ID for which to process changes - """ - from contentcuration.viewsets.sync.base import apply_changes - - changes_qs = Change.objects.filter( - applied=False, - errored=False, - organization_id=organization_id, - channel__isnull=True, - ) - apply_changes(changes_qs) - if changes_qs.exists(): - self.requeue() - - class CustomEmailMessage(EmailMessage): """ jayoshih: There's an issue with the django postmark backend where diff --git a/contentcuration/contentcuration/tests/viewsets/base.py b/contentcuration/contentcuration/tests/viewsets/base.py index 155d6dec1b..c75b7f9549 100644 --- a/contentcuration/contentcuration/tests/viewsets/base.py +++ b/contentcuration/contentcuration/tests/viewsets/base.py @@ -106,7 +106,7 @@ class SyncTestMixin(EagerTasksTestMixin): def sync_url(self): return reverse("sync") - def sync_changes(self, changes, organization_revs=None): + def sync_changes(self, changes): channel_ids = set(c.get("channel_id") for c in changes) channel_revs = {} for channel_id in channel_ids: @@ -114,11 +114,7 @@ def sync_changes(self, changes, organization_revs=None): channel_revs[channel_id] = 0 return self.client.post( self.sync_url, - { - "changes": changes, - "channel_revs": channel_revs, - "organization_revs": organization_revs or {}, - }, + {"changes": changes, "channel_revs": channel_revs}, format="json", ) diff --git a/contentcuration/contentcuration/tests/viewsets/test_invitation.py b/contentcuration/contentcuration/tests/viewsets/test_invitation.py index 215e619d7b..b8bac2934d 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_invitation.py +++ b/contentcuration/contentcuration/tests/viewsets/test_invitation.py @@ -1,7 +1,6 @@ import uuid from django.urls import reverse -from rest_framework.test import APIClient from contentcuration import models from contentcuration.constants.organization_roles import ORGANIZATION_EDITOR @@ -538,90 +537,6 @@ def test_invitation_with_channel_and_organization_is_rejected(self): except models.Invitation.DoesNotExist: pass - def test_organization_invitation_broadcast_to_other_admin(self): - other_admin = testdata.user("org-admin-2@inc.com") - testdata.organization_role(other_admin, self.organization) - - invitation = self.invitation_metadata - response = self.sync_changes( - [ - generate_create_event( - invitation["id"], - INVITATION, - invitation, - organization_id=self.organization.id, - user_id=self.invited_user.id, - ) - ], - ) - self.assertEqual(response.status_code, 200, response.content) - - other_client = APIClient() - other_client.force_authenticate(user=other_admin) - response = other_client.post( - self.sync_url, - { - "changes": [], - "channel_revs": {}, - "organization_revs": {self.organization.id: 0}, - }, - format="json", - ) - self.assertEqual(response.status_code, 200, response.content) - # force_authenticate() (used by both test clients here) bypasses Django's - # real session machinery, so every force_authenticate'd request ends up - # with session_key=None - which makes return_changes() misclassify this - # as "successes" (same session) rather than "changes" (a different - # session), even though these are two distinct users/clients. That - # session-identity distinction isn't reproducible with force_authenticate - # and isn't what this test is checking; what matters here is that - # organization_revs correctly delivered the change to this observer at - # all, so check both buckets rather than relying on that classification. - payload = response.json() - returned_keys = [ - c.get("key") for c in payload["changes"] + payload["successes"] - ] - self.assertIn(invitation["id"], returned_keys) - - def test_organization_invitation_not_broadcast_to_unrelated_user(self): - unrelated_user = testdata.user("unrelated-org-user@inc.com") - - invitation = self.invitation_metadata - response = self.sync_changes( - [ - generate_create_event( - invitation["id"], - INVITATION, - invitation, - organization_id=self.organization.id, - user_id=self.invited_user.id, - ) - ], - ) - self.assertEqual(response.status_code, 200, response.content) - - other_client = APIClient() - other_client.force_authenticate(user=unrelated_user) - response = other_client.post( - self.sync_url, - { - "changes": [], - "channel_revs": {}, - "organization_revs": {self.organization.id: 0}, - }, - format="json", - ) - self.assertEqual(response.status_code, 200, response.content) - # organization_revs is filtered to organizations the requester can view, - # so an unrelated user's request for this org's revs is dropped entirely - # and no invitation changes come back for it, in either bucket. - payload = response.json() - returned_keys = [ - c.get("key") - for c in payload.get("changes", []) + payload.get("successes", []) - ] - self.assertNotIn(invitation["id"], returned_keys) - class CRUDTestCase(StudioAPITestCase): @property diff --git a/contentcuration/contentcuration/viewsets/invitation.py b/contentcuration/contentcuration/viewsets/invitation.py index 1484abc099..b3b50369f6 100644 --- a/contentcuration/contentcuration/viewsets/invitation.py +++ b/contentcuration/contentcuration/viewsets/invitation.py @@ -87,7 +87,6 @@ def update(self, instance, validated_data): "accepted": True, }, channel_id=instance.channel_id, - organization_id=instance.organization_id, ) ) @@ -193,7 +192,6 @@ def accept(self, request, pk=None): INVITATION, {"accepted": True}, channel_id=invitation.channel_id, - organization_id=invitation.organization_id, user_id=request.user.id, ), applied=True, @@ -213,7 +211,6 @@ def decline(self, request, pk=None): INVITATION, {"declined": True}, channel_id=invitation.channel_id, - organization_id=invitation.organization_id, user_id=request.user.id, ), applied=True, diff --git a/contentcuration/contentcuration/viewsets/sync/endpoint.py b/contentcuration/contentcuration/viewsets/sync/endpoint.py index e3b9f389e4..538846426a 100644 --- a/contentcuration/contentcuration/viewsets/sync/endpoint.py +++ b/contentcuration/contentcuration/viewsets/sync/endpoint.py @@ -17,8 +17,8 @@ from contentcuration.models import Channel from contentcuration.models import CustomTaskMetadata from contentcuration.models import Organization +from contentcuration.models import User from contentcuration.tasks import apply_channel_changes_task -from contentcuration.tasks import apply_organization_changes_task from contentcuration.tasks import apply_user_changes_task from contentcuration.viewsets.sync.constants import CHANNEL from contentcuration.viewsets.sync.constants import CREATED @@ -83,9 +83,6 @@ def handle_changes(self, request): user_only_changes = [] # Related to a channel that the user is an editor for. channel_changes = [] - # Not related to a channel, but related to an organization the user - # can edit (e.g. an org admin managing another user's invitation). - organization_changes = [] # Changes that cannot be made disallowed_changes = [] for c in changes: @@ -94,9 +91,6 @@ def handle_changes(self, request): elif ( c.get("channel_id") is None and c.get("user_id") == request.user.id ): - # A user can always act on their own behalf, even if e.g. they - # don't yet have edit rights on the organization a change also - # references (accepting an org invitation is exactly this case). user_only_changes.append(c) elif c.get("channel_id") in allowed_ids: channel_changes.append(c) @@ -104,11 +98,19 @@ def handle_changes(self, request): c.get("channel_id") is None and c.get("organization_id") in allowed_org_ids ): - organization_changes.append(c) + # Organization-scoped changes (e.g. an org admin creating or + # revoking another user's invitation) have no channel, and the + # actor isn't necessarily the target user, so they can't rely + # on the self-only check above. There's no dedicated broadcast + # mechanism for organizations (see PR discussion) - this just + # needs the change to be accepted and applied, so it's routed + # through the existing per-user queue, tagging the target user + # rather than the actor. + user_only_changes.append(c) else: disallowed_changes.append(c) change_models = Change.create_changes( - user_only_changes + channel_changes + organization_changes, + user_only_changes + channel_changes, created_by_id=request.user.id, session_key=session_key, ) @@ -116,27 +118,19 @@ def handle_changes(self, request): apply_user_changes_task.fetch_or_enqueue( request.user, user_id=request.user.id ) + other_target_user_ids = set( + c.get("user_id") + for c in user_only_changes + if c.get("user_id") and c.get("user_id") != request.user.id + ) + for target_user in User.objects.filter(id__in=other_target_user_ids): + apply_user_changes_task.fetch_or_enqueue( + target_user, user_id=target_user.id + ) for channel_id in allowed_ids: apply_channel_changes_task.fetch_or_enqueue( request.user, channel_id=channel_id ) - # A change can end up with organization_id set even when it was routed - # via the self-only check above (e.g. a user accepting their own org - # invitation, who isn't an org member yet and so isn't in - # allowed_org_ids). apply_user_changes_task excludes anything with - # organization_id set, so without this such a change would never be - # picked up by any task. Cover every organization_id that actually - # ended up on a change, not just the ones routed via the dedicated - # organization_changes bucket. - all_organization_ids = allowed_org_ids.union( - c.get("organization_id") - for c in user_only_changes + organization_changes - if c.get("organization_id") - ) - for organization_id in all_organization_ids: - apply_organization_changes_task.fetch_or_enqueue( - request.user, organization_id=organization_id - ) allowed_changes = [ {"rev": c.client_rev, "server_rev": c.server_rev} for c in change_models ] @@ -158,24 +152,7 @@ def get_channel_revs(self, request): } return channel_revs - def get_organization_revs(self, request): - organization_revs = request.data.get("organization_revs", {}) - if organization_revs: - # Filter to only the organizations that the user has permissions to view. - organization_ids = ( - Organization.filter_view_queryset( - Organization.objects.all(), request.user - ) - .filter(id__in=organization_revs.keys()) - .values_list("id", flat=True) - ) - organization_revs = { - organization_id: organization_revs[organization_id] - for organization_id in organization_ids - } - return organization_revs - - def return_changes(self, request, channel_revs, organization_revs): + def return_changes(self, request, channel_revs): user_rev = request.data.get("user_rev") or 0 unapplied_revs = request.data.get("unapplied_revs", []) session_key = request.session.session_key @@ -200,20 +177,12 @@ def return_changes(self, request, channel_revs, organization_revs): & relevant_to_session_filter ) - for organization_id, rev in organization_revs.items(): - change_filter |= ( - Q(organization_id=organization_id) - & (unapplied_revs_filter | Q(server_rev__gt=rev)) - & relevant_to_session_filter - ) - changes_to_return = list( Change.objects.filter(change_filter) .values( "server_rev", "session_id", "channel_id", - "organization_id", "user_id", "created_by_id", "applied", @@ -261,7 +230,6 @@ def return_tasks(self, request, channel_revs): task_name__in=[ apply_channel_changes_task.name, apply_user_changes_task.name, - apply_organization_changes_task.name, ] ) .annotate( @@ -299,13 +267,10 @@ def post(self, request): } channel_revs = self.get_channel_revs(request) - organization_revs = self.get_organization_revs(request) response_payload.update(self.handle_changes(request)) - response_payload.update( - self.return_changes(request, channel_revs, organization_revs) - ) + response_payload.update(self.return_changes(request, channel_revs)) response_payload.update(self.return_tasks(request, channel_revs)) From eb4a8b63b55abb01ac8c407bde4bcb0af6604e8c Mon Sep 17 00:00:00 2001 From: yazzylazy Date: Mon, 27 Jul 2026 17:23:42 -0400 Subject: [PATCH 07/10] 6009: fix org invitation review findings (rtibblesbot) - Route org-scoped changes through the actor's own queue instead of trusting a client-supplied user_id: fixes a silent-drop when user_id is omitted (endpoint.py:97) and a change-injection vector where an org admin could tag an arbitrary user as the target (endpoint.py:121). Scoped to the invitation table specifically, mirroring how the created_channel_ids exception is scoped to the channel table. - InvitationSerializer.get_fields(): unlock `revoked` for any active org admin of the invitation's organization, not just the original sender - org-admin edit rights on the queryset didn't actually enable revoke without this. - InvitationSerializer.get_fields(): gate accepted/declined on email match (mirroring _ensure_invitee) instead of the `invited` FK, which is never populated for invitations created through the sync API - the real invitee of a real org invitation couldn't accept/decline. - InvitationSerializer.validate(): use channel_id/organization_id instead of dereferencing the related objects, avoiding up to 2N extra queries per sync batch. - Organization.filter_edit_queryset/filter_view_queryset: exclude soft-deleted organizations. - New tests: org change with no user_id still applies; a client- supplied user_id is not trusted as the routing target; cross-org isolation; revoke by a non-sender org admin; org invitation delete via sync; accept of an invitation created through the real sync create path (not hand-constructed with `invited` set); organization filter on the list endpoint. --- contentcuration/contentcuration/models.py | 2 + .../tests/viewsets/test_invitation.py | 188 ++++++++++++++++++ .../contentcuration/viewsets/invitation.py | 20 +- .../contentcuration/viewsets/sync/endpoint.py | 33 ++- 4 files changed, 222 insertions(+), 21 deletions(-) diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index 482afa63de..888777ce47 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -1907,6 +1907,7 @@ def filter_edit_queryset(cls, queryset, user): user_roles__user=user, user_roles__role=ORGANIZATION_ADMIN, user_roles__status=ORGANIZATION_ROLE_STATUS_ACTIVE, + deleted=False, ).distinct() @classmethod @@ -1925,6 +1926,7 @@ def filter_view_queryset(cls, queryset, user): ORGANIZATION_VIEWER, ], user_roles__status=ORGANIZATION_ROLE_STATUS_ACTIVE, + deleted=False, ).distinct() diff --git a/contentcuration/contentcuration/tests/viewsets/test_invitation.py b/contentcuration/contentcuration/tests/viewsets/test_invitation.py index b8bac2934d..a184bb2771 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_invitation.py +++ b/contentcuration/contentcuration/tests/viewsets/test_invitation.py @@ -537,6 +537,194 @@ def test_invitation_with_channel_and_organization_is_rejected(self): except models.Invitation.DoesNotExist: pass + def test_create_organization_invitation_without_user_id(self): + # A client is not required to (and, per _routes_to_actor below, cannot + # meaningfully) supply user_id for an org-scoped change - routing is + # derived server-side from the actor. This must not silently drop the + # change. + invitation = self.invitation_metadata + response = self.sync_changes( + [ + generate_create_event( + invitation["id"], + INVITATION, + invitation, + organization_id=self.organization.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + try: + models.Invitation.objects.get(id=invitation["id"]) + except models.Invitation.DoesNotExist: + self.fail( + "Organization invitation without a client-supplied user_id " + "was not created" + ) + + def test_organization_invitation_change_routes_to_actor(self): + # A client-supplied user_id must not be trusted as the routing + # target, since that would let an org admin inject a change into an + # arbitrary user's sync feed. The Change row should always be tagged + # with the actor's own id. + unrelated_user = testdata.user("unrelated-target@inc.com") + invitation = self.invitation_metadata + response = self.sync_changes( + [ + generate_create_event( + invitation["id"], + INVITATION, + invitation, + organization_id=self.organization.id, + user_id=unrelated_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + change = models.Change.objects.get( + table=INVITATION, kwargs__key=invitation["id"] + ) + self.assertEqual(change.user_id, self.org_admin.id) + self.assertNotEqual(change.user_id, unrelated_user.id) + + def test_create_organization_invitation_for_different_org_is_rejected(self): + other_organization = testdata.organization() + invitation = self.invitation_metadata + invitation["organization"] = other_organization.id + response = self.sync_changes( + [ + generate_create_event( + invitation["id"], + INVITATION, + invitation, + organization_id=other_organization.id, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + try: + models.Invitation.objects.get(id=invitation["id"]) + self.fail( + "Invitation was created for an organization the admin doesn't manage" + ) + except models.Invitation.DoesNotExist: + pass + + def test_revoke_organization_invitation_by_different_admin(self): + other_admin = testdata.user("org-admin-3@inc.com") + testdata.organization_role(other_admin, self.organization) + + invitation = models.Invitation.objects.create( + id=uuid.uuid4().hex, + organization=self.organization, + email=self.invited_user.email, + sender=self.org_admin, + ) + self.client.force_authenticate(user=other_admin) + response = self.sync_changes( + [ + generate_update_event( + invitation.id, + INVITATION, + {"revoked": True}, + organization_id=self.organization.id, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + invitation.refresh_from_db() + self.assertTrue(invitation.revoked) + + def test_delete_organization_invitation(self): + invitation = models.Invitation.objects.create( + id=uuid.uuid4().hex, + organization=self.organization, + email=self.invited_user.email, + sender=self.org_admin, + ) + response = self.sync_changes( + [ + generate_delete_event( + invitation.id, + INVITATION, + organization_id=self.organization.id, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + try: + models.Invitation.objects.get(id=invitation.id) + self.fail("Organization invitation was not deleted") + except models.Invitation.DoesNotExist: + pass + + def test_accept_organization_invitation_created_via_sync(self): + # Unlike the fixtures above (which set `invited` directly via the + # ORM), an invitation created through the sync API - the real + # creation path - never gets `invited` populated. The real invitee + # must still be able to accept it. + invitation = self.invitation_metadata + response = self.sync_changes( + [ + generate_create_event( + invitation["id"], + INVITATION, + invitation, + organization_id=self.organization.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + created = models.Invitation.objects.get(id=invitation["id"]) + self.assertIsNone(created.invited) + + self.client.force_authenticate(user=self.invited_user) + response = self.sync_changes( + [ + generate_update_event( + invitation["id"], + INVITATION, + {"accepted": True}, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + created.refresh_from_db() + self.assertTrue(created.accepted) + self.assertTrue( + models.OrganizationRole.objects.filter( + user=self.invited_user, organization=self.organization + ).exists() + ) + + def test_list_invitations_filtered_by_organization(self): + invitation = models.Invitation.objects.create( + id=uuid.uuid4().hex, + organization=self.organization, + email=self.invited_user.email, + sender=self.org_admin, + ) + other_organization = testdata.organization() + other_invitation = models.Invitation.objects.create( + id=uuid.uuid4().hex, + organization=other_organization, + email=self.invited_user.email, + sender=self.org_admin, + ) + response = self.client.get( + reverse("invitation-list"), {"organization": self.organization.id} + ) + self.assertEqual(response.status_code, 200, response.content) + payload = response.json() + results = payload["results"] if isinstance(payload, dict) else payload + returned_ids = [item["id"] for item in results] + self.assertIn(invitation.id, returned_ids) + self.assertNotIn(other_invitation.id, returned_ids) + class CRUDTestCase(StudioAPITestCase): @property diff --git a/contentcuration/contentcuration/viewsets/invitation.py b/contentcuration/contentcuration/viewsets/invitation.py index b3b50369f6..9d0cad92cc 100644 --- a/contentcuration/contentcuration/viewsets/invitation.py +++ b/contentcuration/contentcuration/viewsets/invitation.py @@ -50,9 +50,9 @@ class Meta: list_serializer_class = BulkListSerializer def validate(self, data): - channel = data.get("channel", getattr(self.instance, "channel", None)) + channel = data.get("channel", getattr(self.instance, "channel_id", None)) organization = data.get( - "organization", getattr(self.instance, "organization", None) + "organization", getattr(self.instance, "organization_id", None) ) if not channel and not organization: raise serializers.ValidationError( @@ -98,11 +98,25 @@ def get_fields(self): # allow invitation state to be modified under the right conditions if request and request.user and self.instance: - if self.instance.invited == request.user: + # Match on email rather than the `invited` FK, since `invited` is + # only ever populated by the channel email-invite flow - it's + # never set for invitations created through the sync API, which + # would otherwise leave the real invitee unable to accept/decline. + if (request.user.email or "").lower() == ( + self.instance.email or "" + ).lower(): fields["accepted"].read_only = self.instance.revoked fields["declined"].read_only = False if self.instance.sender == request.user: fields["revoked"].read_only = False + if ( + self.instance.organization_id + and Organization.filter_edit_queryset( + Organization.objects.filter(id=self.instance.organization_id), + request.user, + ).exists() + ): + fields["revoked"].read_only = False return fields diff --git a/contentcuration/contentcuration/viewsets/sync/endpoint.py b/contentcuration/contentcuration/viewsets/sync/endpoint.py index 538846426a..8a6ff2c75f 100644 --- a/contentcuration/contentcuration/viewsets/sync/endpoint.py +++ b/contentcuration/contentcuration/viewsets/sync/endpoint.py @@ -17,11 +17,11 @@ from contentcuration.models import Channel from contentcuration.models import CustomTaskMetadata from contentcuration.models import Organization -from contentcuration.models import User from contentcuration.tasks import apply_channel_changes_task from contentcuration.tasks import apply_user_changes_task from contentcuration.viewsets.sync.constants import CHANNEL from contentcuration.viewsets.sync.constants import CREATED +from contentcuration.viewsets.sync.constants import INVITATION from contentcuration.viewsets.sync.constants import SERVER_ONLY_CHANGES @@ -96,16 +96,22 @@ def handle_changes(self, request): channel_changes.append(c) elif ( c.get("channel_id") is None + and c.get("table") == INVITATION and c.get("organization_id") in allowed_org_ids ): - # Organization-scoped changes (e.g. an org admin creating or - # revoking another user's invitation) have no channel, and the - # actor isn't necessarily the target user, so they can't rely - # on the self-only check above. There's no dedicated broadcast - # mechanism for organizations (see PR discussion) - this just - # needs the change to be accepted and applied, so it's routed - # through the existing per-user queue, tagging the target user - # rather than the actor. + # Organization-scoped invitation changes (e.g. an org admin + # creating or revoking another user's invitation) have no + # channel, and the actor isn't necessarily the target user, + # so they can't rely on the self-only check above. Trusting + # a client-supplied user_id here would let an org admin + # inject a change into an arbitrary user's sync feed (or + # silently drop it if user_id is omitted), so the routing + # target is derived server-side from the actor instead - + # apply_changes() already runs under the actor's + # permissions via sync_initial(change.created_by). Scoped + # to the invitation table specifically, since nothing else + # currently understands organization_id. + c["user_id"] = request.user.id user_only_changes.append(c) else: disallowed_changes.append(c) @@ -118,15 +124,6 @@ def handle_changes(self, request): apply_user_changes_task.fetch_or_enqueue( request.user, user_id=request.user.id ) - other_target_user_ids = set( - c.get("user_id") - for c in user_only_changes - if c.get("user_id") and c.get("user_id") != request.user.id - ) - for target_user in User.objects.filter(id__in=other_target_user_ids): - apply_user_changes_task.fetch_or_enqueue( - target_user, user_id=target_user.id - ) for channel_id in allowed_ids: apply_channel_changes_task.fetch_or_enqueue( request.user, channel_id=channel_id From 4f4a2b9dcc80402fb9533e7e163d281814bbc572 Mon Sep 17 00:00:00 2001 From: yazzylazy Date: Wed, 29 Jul 2026 18:58:25 -0400 Subject: [PATCH 08/10] 6009: tag user_id on the sync-based accept echo event InvitationSerializer.update()'s echo Change event (generated when an invitation is accepted via /sync) only carried channel_id, so for org invitations (channel_id=None) it had neither routing key and was dead weight in the Change table. Tags it with the accepting user's id, mirroring how the accept/decline REST actions already do this. --- contentcuration/contentcuration/viewsets/invitation.py | 1 + 1 file changed, 1 insertion(+) diff --git a/contentcuration/contentcuration/viewsets/invitation.py b/contentcuration/contentcuration/viewsets/invitation.py index 9d0cad92cc..7b93dad6ea 100644 --- a/contentcuration/contentcuration/viewsets/invitation.py +++ b/contentcuration/contentcuration/viewsets/invitation.py @@ -87,6 +87,7 @@ def update(self, instance, validated_data): "accepted": True, }, channel_id=instance.channel_id, + user_id=self.context["request"].user.id, ) ) From 1400e4a52de7927aa13a8409779e83d14e807a8c Mon Sep 17 00:00:00 2001 From: yazzylazy Date: Wed, 29 Jul 2026 19:09:14 -0400 Subject: [PATCH 09/10] 6009: drop unused organization_id param, add org CRUD 405 test Per rtibblesbot review: - Removed organization_id from generate_create_event/update_event/ delete_event and _generate_event in sync/utils.py - no production code calls these with organization_id (real clients send it as a plain dict key; handle_changes() reads it straight off the incoming JSON, not through this Python helper). The signature no longer implies routing support that doesn't exist. - The test-side wrappers in tests/viewsets/base.py now pop organization_id out of kwargs before forwarding to the production helper, and attach it to the resulting dict afterward, so existing test call sites (which build raw payloads to exercise the organization permission branch) didn't need to change individually. - Added CRUDTestCase.test_create_organization_invitation, asserting POST with an organization payload is also blocked with 405, mirroring the existing channel-payload version of that test. --- .../contentcuration/tests/viewsets/base.py | 14 ++++++++++ .../tests/viewsets/test_invitation.py | 17 +++++++++++ .../contentcuration/viewsets/sync/utils.py | 28 +++++-------------- 3 files changed, 38 insertions(+), 21 deletions(-) diff --git a/contentcuration/contentcuration/tests/viewsets/base.py b/contentcuration/contentcuration/tests/viewsets/base.py index c75b7f9549..61864e6b50 100644 --- a/contentcuration/contentcuration/tests/viewsets/base.py +++ b/contentcuration/contentcuration/tests/viewsets/base.py @@ -40,19 +40,33 @@ def generate_copy_event(*args, **kwargs): def generate_create_event(*args, **kwargs): + # organization_id isn't a parameter of the production event builders (no + # production code emits it - real clients send it as a plain dict key, + # not through this Python function), but tests need to be able to attach + # it to simulate what a real client payload would contain, since + # handle_changes() reads it directly off the raw incoming dict. + organization_id = kwargs.pop("organization_id", None) event = base_generate_create_event(*args, **kwargs) + if organization_id: + event["organization_id"] = organization_id event["rev"] = random.randint(1, 10000000) return event def generate_delete_event(*args, **kwargs): + organization_id = kwargs.pop("organization_id", None) event = base_generate_delete_event(*args, **kwargs) + if organization_id: + event["organization_id"] = organization_id event["rev"] = random.randint(1, 10000000) return event def generate_update_event(*args, **kwargs): + organization_id = kwargs.pop("organization_id", None) event = base_generate_update_event(*args, **kwargs) + if organization_id: + event["organization_id"] = organization_id event["rev"] = random.randint(1, 10000000) return event diff --git a/contentcuration/contentcuration/tests/viewsets/test_invitation.py b/contentcuration/contentcuration/tests/viewsets/test_invitation.py index a184bb2771..a9d0c618d3 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_invitation.py +++ b/contentcuration/contentcuration/tests/viewsets/test_invitation.py @@ -762,6 +762,23 @@ def test_create_invitation(self): ) self.assertEqual(response.status_code, 405, response.content) + def test_create_organization_invitation(self): + organization = testdata.organization() + org_admin = testdata.user("crud-org-admin@inc.com") + testdata.organization_role(org_admin, organization) + self.client.force_authenticate(user=org_admin) + invitation = { + "id": uuid.uuid4().hex, + "organization": organization.id, + "email": self.invited_user.email, + } + response = self.client.post( + reverse("invitation-list"), + invitation, + format="json", + ) + self.assertEqual(response.status_code, 405, response.content) + def test_update_invitation_accept(self): invitation = models.Invitation.objects.create(**self.invitation_db_metadata) diff --git a/contentcuration/contentcuration/viewsets/sync/utils.py b/contentcuration/contentcuration/viewsets/sync/utils.py index 18e90863e2..8f5ce98a05 100644 --- a/contentcuration/contentcuration/viewsets/sync/utils.py +++ b/contentcuration/contentcuration/viewsets/sync/utils.py @@ -23,7 +23,7 @@ def validate_table(table): raise ValueError("{} is not a valid table name".format(table)) -def _generate_event(key, table, event_type, channel_id, user_id, organization_id=None): +def _generate_event(key, table, event_type, channel_id, user_id): validate_table(table) event = { "key": key, @@ -34,37 +34,23 @@ def _generate_event(key, table, event_type, channel_id, user_id, organization_id event["channel_id"] = channel_id if user_id: event["user_id"] = user_id - if organization_id: - event["organization_id"] = organization_id return event -def generate_create_event( - key, table, obj, channel_id=None, user_id=None, organization_id=None -): - event = _generate_event( - key, table, CREATED, channel_id, user_id, organization_id=organization_id - ) +def generate_create_event(key, table, obj, channel_id=None, user_id=None): + event = _generate_event(key, table, CREATED, channel_id, user_id) event["obj"] = obj return event -def generate_update_event( - key, table, mods, channel_id=None, user_id=None, organization_id=None -): - event = _generate_event( - key, table, UPDATED, channel_id, user_id, organization_id=organization_id - ) +def generate_update_event(key, table, mods, channel_id=None, user_id=None): + event = _generate_event(key, table, UPDATED, channel_id, user_id) event["mods"] = mods return event -def generate_delete_event( - key, table, channel_id=None, user_id=None, organization_id=None -): - return _generate_event( - key, table, DELETED, channel_id, user_id, organization_id=organization_id - ) +def generate_delete_event(key, table, channel_id=None, user_id=None): + return _generate_event(key, table, DELETED, channel_id, user_id) def generate_move_event(key, table, target, position, channel_id=None, user_id=None): From fbaf00e8a11cf8af8acf021ce9744de5a1bd26fd Mon Sep 17 00:00:00 2001 From: yazzylazy Date: Wed, 29 Jul 2026 19:18:34 -0400 Subject: [PATCH 10/10] 6009: add test for channel invitation co-owner (admin) share_mode Confirmed with the team: the organization "co-owner" acceptance criterion in #6009 documents existing behavior rather than requiring new code - _accept_channel_invitation only special-cases VIEW_ACCESS, so share_mode="admin" already grants the same editor access as "edit". Adds a test locking that in, closing the last open review thread. --- .../tests/viewsets/test_invitation.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/contentcuration/contentcuration/tests/viewsets/test_invitation.py b/contentcuration/contentcuration/tests/viewsets/test_invitation.py index a9d0c618d3..95d33b77d8 100644 --- a/contentcuration/contentcuration/tests/viewsets/test_invitation.py +++ b/contentcuration/contentcuration/tests/viewsets/test_invitation.py @@ -7,6 +7,7 @@ from contentcuration.constants.organization_roles import ( ORGANIZATION_ROLE_STATUS_ACTIVE, ) +from contentcuration.models import ADMIN_ACCESS from contentcuration.tests import testdata from contentcuration.tests.base import StudioAPITestCase from contentcuration.tests.viewsets.base import generate_create_event @@ -145,6 +146,32 @@ def test_update_invitation_accept(self): ) self.assertTrue(models.Change.objects.filter(channel=self.channel).exists()) + def test_update_invitation_accept_admin_share_mode_grants_editor_access(self): + # A channel invitation's "co-owner" share_mode (admin) documents + # existing behavior rather than introducing a new access tier: + # _accept_channel_invitation only special-cases VIEW_ACCESS, so + # anything else - including "admin" - grants the same editor access + # as "edit". This locks that in as intended, confirmed behavior. + invitation = models.Invitation.objects.create( + share_mode=ADMIN_ACCESS, **self.invitation_db_metadata + ) + + self.client.force_authenticate(user=self.invited_user) + response = self.sync_changes( + [ + generate_update_event( + invitation.id, + INVITATION, + {"accepted": True}, + user_id=self.invited_user.id, + ) + ], + ) + self.assertEqual(response.status_code, 200, response.content) + invitation.refresh_from_db() + self.assertTrue(invitation.accepted) + self.assertTrue(self.channel.editors.filter(pk=self.invited_user.id).exists()) + def test_update_invitation_revoke(self): invitation = models.Invitation.objects.create(**self.invitation_db_metadata)