Skip to content
Open
164 changes: 156 additions & 8 deletions contentcuration/contentcuration/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@
from contentcuration.constants import feedback
from contentcuration.constants import user_history
from contentcuration.constants.contentnode import kind_activity_map
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_VIEWER
from contentcuration.constants.organization_roles import organization_role_choices
from contentcuration.constants.organization_roles import (
organization_role_status_choices,
Expand Down Expand Up @@ -818,7 +822,7 @@ def file_on_disk_name(instance, filename):


def generate_file_on_disk_name(checksum, filename):
""" Separated from file_on_disk_name to allow for simple way to check if has already exists """
"""Separated from file_on_disk_name to allow for simple way to check if has already exists"""
h = checksum
basename, ext = os.path.splitext(filename)
directory = os.path.join(settings.STORAGE_ROOT, h[0], h[1])
Expand All @@ -845,7 +849,7 @@ def object_storage_name(instance, filename):


def generate_object_storage_name(checksum, filename, default_ext=""):
""" Separated from file_on_disk_name to allow for simple way to check if has already exists """
"""Separated from file_on_disk_name to allow for simple way to check if has already exists"""
h = checksum
basename, actual_ext = os.path.splitext(filename)
ext = actual_ext if actual_ext else default_ext
Expand Down Expand Up @@ -1061,7 +1065,7 @@ class ChannelModelManager(models.Manager.from_queryset(ChannelModelQuerySet)):


class Channel(models.Model):
""" Permissions come from association with organizations """
"""Permissions come from association with organizations"""

id = UUIDField(primary_key=True, default=uuid.uuid4)
name = models.CharField(max_length=200, blank=True)
Expand Down Expand Up @@ -1225,11 +1229,55 @@ def filter_edit_queryset(cls, queryset, user):
user_id=user_id, channel_id=OuterRef("id")
)
)
queryset = queryset.annotate(edit=edit)
organization_edit = Exists(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: no coverage for this grant — tests/viewsets/test_organization.py never references Channel, and tests/viewsets/test_channel.py never references organizations. #5967 requires automated tests verifying permission enforcement, and lists channel access for all three roles.

Minimum cases: admin and editor can update an org-owned channel they have no m2m share on; viewer cannot; a non-member cannot; an ORGANIZATION_ROLE_STATUS_PENDING role grants nothing.

OrganizationRole.objects.filter(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: filters status but not organization__deleted, so roles keep conferring channel edit after OrganizationViewSet.perform_destroy soft-deletes the org. Every other org queryset in this PR excludes deleted orgs (1929, 1947, 2018, 2038). Same gap in the organization_view subquery at 1272. Intended?

user_id=user_id,
organization_id=OuterRef("organization_id"),
status=ORGANIZATION_ROLE_STATUS_ACTIVE,
role__in=(
ORGANIZATION_ADMIN,
ORGANIZATION_EDITOR,
),
)
)
queryset = queryset.annotate(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: ChannelViewSet.get_queryset (viewsets/channel.py:499-517) re-annotates edit/view from the m2m tables alone, and organization_edit/organization_view are surfaced nowhere. An org admin listing channels therefore sees edit: false on channels the write endpoints will accept from them. Consider folding the org Exists into the edit annotation there so read and write agree.

edit=edit,
organization_edit=organization_edit,
)
if user.is_admin:
return queryset

return queryset.filter(edit=True)
return queryset.filter(Q(edit=True) | Q(organization_edit=True))

@rtibblesbot rtibblesbot Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved — addressed in the current code.

blocking: this grant is the one I asked for last round, but it is one role too wide on the delete path. get_edit_queryset (viewsets/base.py:597) dispatches here, and ChannelViewSet.destroy (viewsets/channel.py:484) resolves its target through get_edit_object() — so an active ORGANIZATION_EDITOR can soft-delete any channel their organization owns. #5967 gives delete to Admin only; Editor is scoped to "Create and update organization-owned channels and content".

Split edit from delete: annotate the role alongside the boolean and check it in ChannelViewSet.destroy, or add a filter_delete_queryset restricted to ORGANIZATION_ADMIN and call it from destroy.


@classmethod
def filter_delete_queryset(cls, queryset, user):

@rtibblesbot rtibblesbot Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved — addressed in the current code.

blocking: no callers; grep finds only this definition. Delete goes through get_edit_queryset() (base.py:591) from ChannelViewSet.destroy (channel.py:484) and delete_from_changes (base.py:750); filter_edit_queryset admits ORGANIZATION_EDITOR, so org editors still delete org channels — #5967 says admin only.

My last-round ask, unwired. Add get_delete_queryset() beside get_edit_queryset() in BaseValuesViewset, wire both delete paths, test editor 403 / admin 204 — or drop until wired.

user_id = not user.is_anonymous and user.id

if not user_id:
return queryset.none()

edit = Exists(
User.editable_channels.through.objects.filter(
user_id=user_id, channel_id=OuterRef("id")
)
)
organization_delete = Exists(
OrganizationRole.objects.filter(
user_id=user_id,
organization_id=OuterRef("organization_id"),
status=ORGANIZATION_ROLE_STATUS_ACTIVE,
role=ORGANIZATION_ADMIN,
)
)
queryset = queryset.annotate(
edit=edit,
organization_delete=organization_delete,
)

if user.is_admin:
return queryset

return queryset.filter(Q(edit=True) | Q(organization_delete=True))

@classmethod
def filter_view_queryset(cls, queryset, user):
Expand All @@ -1238,35 +1286,60 @@ def filter_view_queryset(cls, queryset, user):

if user_id:
filters = dict(user_id=user_id, channel_id=OuterRef("id"))

edit = Exists(
User.editable_channels.through.objects.filter(**filters).values(
"user_id"
)
)

view = Exists(
User.view_only_channels.through.objects.filter(**filters).values(
"user_id"
)
)

organization_view = Exists(

@rtibblesbot rtibblesbot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved — addressed in the current code.

blocking: org roles now grant channel view but never channel edit. Channel.filter_edit_queryset (models.py:1246) is unchanged and still consults only editable_channels, so an organization admin or editor cannot create, update, or delete a channel their organization owns. #5967 lists both as role permissions ("Create, update, and delete organization-owned channels" / "Create and update organization-owned channels and content").

Either mirror this annotation into filter_edit_queryset restricted to ORGANIZATION_ADMIN/ORGANIZATION_EDITOR, or say in the PR body that channel-level enforcement lands in a follow-up so the criterion is not silently dropped.

The new view grant is also untested — test_channel.py never mentions organizations. A channel visible to an org viewer and invisible to a non-member is the assertion to add.

OrganizationRole.objects.filter(
user_id=user_id,
organization_id=OuterRef("organization_id"),
status=ORGANIZATION_ROLE_STATUS_ACTIVE,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: this filters OrganizationRole.status but not organization__deleted. Organization.filter_view_queryset (1941) and OrganizationRole.filter_view_queryset (2031) both exclude soft-deleted orgs, and perform_destroy only sets deleted=True — so after deleting an organization its channels stay visible to former members while the org itself disappears. Adding organization__deleted=False here keeps the three filters consistent.

role__in=(
ORGANIZATION_ADMIN,
ORGANIZATION_EDITOR,
ORGANIZATION_VIEWER,
),
)
)
else:
edit = boolean_val(False)
view = boolean_val(False)
organization_view = boolean_val(False)

queryset = queryset.annotate(
edit=edit,
view=view,
organization_view=organization_view,
)

if user_id and user.is_admin:
return queryset

permission_filter = Q()

if user_id:
pending_channels = Invitation.objects.filter(
email=user_email, revoked=False, declined=False, accepted=False
email=user_email,
revoked=False,
declined=False,
accepted=False,
).values_list("channel_id", flat=True)

permission_filter = (
Q(view=True) | Q(edit=True) | Q(deleted=False, id__in=pending_channels)
Q(view=True)
| Q(edit=True)
| Q(organization_view=True)
| Q(deleted=False, id__in=pending_channels)
)

return queryset.filter(permission_filter | Q(deleted=False, public=True))
Expand Down Expand Up @@ -1881,6 +1954,40 @@ class Organization(models.Model):

objects = CustomManager()

@classmethod
def filter_view_queryset(cls, queryset, user):
queryset = queryset.filter(deleted=False)

if user.is_anonymous:
return queryset.filter(public=True)

if user.is_admin:
return queryset

return queryset.filter(
Q(public=True)
| Q(
user_roles__user=user,
user_roles__status=ORGANIZATION_ROLE_STATUS_ACTIVE,
)
).distinct()

@classmethod
def filter_edit_queryset(cls, queryset, user):
queryset = queryset.filter(deleted=False)

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()

class Meta:
verbose_name = "Organization"
verbose_name_plural = "Organizations"
Expand Down Expand Up @@ -1936,6 +2043,47 @@ class OrganizationRole(models.Model):
)
updated_at = models.DateTimeField(auto_now=True, help_text="Last update timestamp")

@classmethod
def filter_view_queryset(cls, queryset, user):
queryset = queryset.filter(
organization__deleted=False,
).select_related(
"organization",
"user",
)

if user.is_anonymous:
return queryset.none()

if user.is_admin:
return queryset

return queryset.filter(
organization__user_roles__user=user,
organization__user_roles__status=ORGANIZATION_ROLE_STATUS_ACTIVE,
).distinct()

@classmethod
def filter_edit_queryset(cls, queryset, user):
queryset = queryset.filter(
organization__deleted=False,
).select_related(
"organization",
"user",
)

if user.is_anonymous:
return queryset.none()

if user.is_admin:
return queryset

return queryset.filter(
organization__user_roles__user=user,
organization__user_roles__role=ORGANIZATION_ADMIN,
organization__user_roles__status=ORGANIZATION_ROLE_STATUS_ACTIVE,
).distinct()

class Meta:
unique_together = ("user", "organization")
verbose_name = "Organization Role"
Expand Down Expand Up @@ -3705,7 +3853,7 @@ def save(self, *args, **kwargs):


class Invitation(models.Model):
""" Invitation to edit channel """
"""Invitation to edit channel"""

id = UUIDField(primary_key=True, default=uuid.uuid4)
accepted = models.BooleanField(default=False)
Expand Down
Loading