Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions docs/user/rest-api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,53 @@ Remove Email Address

DELETE /api/v1/users/user/{id}/email/{id}/

List Organization Memberships

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The new API documentation lists the paths but omits their authorization rules. A client cannot tell that superusers have global access, while organization managers need the relevant model permission, are limited to organizations they manage, and cannot manage superusers.

Please add a concise note documenting these permission and scope restrictions.

Severity: P3

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. code-block:: text

GET /api/v1/users/user/{id}/organization-membership/

Add Organization Membership
~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. code-block:: text

POST /api/v1/users/user/{id}/organization-membership/

.. note::

The organization manager flag is represented internally by the
``is_admin`` field in the payload.

Get Organization Membership
~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. code-block:: text

GET /api/v1/users/user/{id}/organization-membership/{org_id}/

Change Organization Membership
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. code-block:: text

PUT /api/v1/users/user/{id}/organization-membership/{org_id}/

Patch Organization Membership
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. code-block:: text

PATCH /api/v1/users/user/{id}/organization-membership/{org_id}/

Remove Organization Membership
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. code-block:: text

DELETE /api/v1/users/user/{id}/organization-membership/{org_id}/

Comment thread
BHARATH0153 marked this conversation as resolved.
List Organizations
~~~~~~~~~~~~~~~~~~

Expand Down
34 changes: 34 additions & 0 deletions openwisp_users/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,40 @@ def to_internal_value(self, data):
return super().to_internal_value(data)


class OrganizationMembershipSerializer(ValidatedModelSerializer):
exclude_validation = ("user", "organization")
id = serializers.UUIDField(read_only=True)
organization = OrgUserCustomPrimarykeyRelatedField()

class Meta:
model = OrganizationUser
fields = ("id", "organization", "is_admin", "created", "modified")

def validate(self, data):
data["user"] = self.context["user"]
if self.instance is None:
organization = data.get("organization")
if (
organization is not None
and self.Meta.model.objects.filter(
user=data["user"], organization=organization
).exists()
):
raise serializers.ValidationError(
{
"organization": _(
"The user is already a member of this organization."
Comment thread
BHARATH0153 marked this conversation as resolved.
)
}
)
return super().validate(data)


class OrganizationMembershipDetailSerializer(OrganizationMembershipSerializer):
class Meta(OrganizationMembershipSerializer.Meta):
extra_kwargs = {"organization": {"read_only": True}}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The detail serializer does not make organization read-only. DRF preserves the field explicitly declared on the parent serializer, so this extra_kwargs entry has no effect. I reproduced that a manager who manages both organizations can move an owner membership from org1 to org2. The OrganizationOwner row still names org1, while its linked membership belongs to org2, and the user is then treated as org2's owner.

Please explicitly declare the detail field as read-only, or keep it writable and reject a value that differs from the URL organization. Add a regression test for an owner membership as well.

Severity: P1



class BaseSuperUserSerializer(ValidatedModelSerializer):
_skip_validation_fields = [
"groups",
Expand Down
10 changes: 10 additions & 0 deletions openwisp_users/api/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ def get_view(name):
get_view("email_update"),
name="email_update",
),
path(
"users/user/<uuid:pk>/organization-membership/",
get_view("organization_membership_list"),
name="organization_membership_list",
),
path(
"users/user/<uuid:pk>/organization-membership/<uuid:org_id>/",
get_view("organization_membership_detail"),
name="organization_membership_detail",
),
path("users/group/", get_view("group_list"), name="group_list"),
path("users/group/<int:pk>/", get_view("group_detail"), name="group_detail"),
]
Expand Down
83 changes: 83 additions & 0 deletions openwisp_users/api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
from django.urls import reverse
from django.utils.translation import gettext_lazy as _
from drf_yasg.utils import swagger_auto_schema
from organizations.exceptions import OwnershipRequired
from rest_framework.authtoken.views import ObtainAuthToken
from rest_framework.exceptions import ValidationError
from rest_framework.generics import (
GenericAPIView,
ListCreateAPIView,
Expand All @@ -32,6 +34,8 @@
EmailAddressSerializer,
GroupSerializer,
OrganizationDetailSerializer,
OrganizationMembershipDetailSerializer,
OrganizationMembershipSerializer,
OrganizationSerializer,
PasswordChangeSerializer,
PasswordResetSerializer,
Expand Down Expand Up @@ -331,6 +335,83 @@ def get_object(self):
return obj


class BaseOrganizationMembershipView(ProtectedAPIMixin, FilterByParent, GenericAPIView):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The parent lookup and organization-scoping logic duplicates BaseEmailView. This is authorization code, so maintaining two copies risks a tenant-isolation fix landing in one endpoint but not the other.

Please move the shared parent-scoping and serializer-context behavior into a common base or mixin, then reuse it from both endpoint families.

Severity: P3

model = OrganizationUser
serializer_class = OrganizationMembershipSerializer

def get_queryset(self):
if getattr(self, "swagger_fake_view", False):
return OrganizationUser.objects.none()
qs = OrganizationUser.objects.select_related("organization", "user")
if not self.request.user.is_superuser:
qs = qs.filter(organization_id__in=self.request.user.organizations_managed)
return qs

def initial(self, *args, **kwargs):
super().initial(*args, **kwargs)
self.assert_parent_exists()

def get_parent_queryset(self):
qs = User.objects.filter(pk=self.kwargs["pk"])
if self.request.user.is_superuser:
return qs
return self.get_organization_queryset(qs)

def get_organization_queryset(self, qs):
orgs = self.request.user.organizations_managed
app_label = User._meta.app_config.label
filter_kwargs = {
"is_superuser": False,
f"{app_label}_organizationuser__organization_id__in": orgs,
}
return qs.filter(**filter_kwargs).distinct()
Comment thread
BHARATH0153 marked this conversation as resolved.

def get_serializer_context(self):
if getattr(self, "swagger_fake_view", False):
return None
context = super().get_serializer_context()
context["user"] = self.get_parent_queryset().first()
return context
Comment thread
coderabbitai[bot] marked this conversation as resolved.


class OrganizationMembershipListCreateView(
BaseOrganizationMembershipView, ListCreateAPIView
):
pagination_class = OpenWispPagination

def get_queryset(self):
if getattr(self, "swagger_fake_view", False):
return OrganizationUser.objects.none()
return super().get_queryset().filter(user_id=self.kwargs["pk"])
Comment thread
BHARATH0153 marked this conversation as resolved.


class OrganizationMembershipDetailView(
BaseOrganizationMembershipView, RetrieveUpdateDestroyAPIView
):
def get_serializer_class(self):
return OrganizationMembershipDetailSerializer

def get_object(self):
queryset = self.filter_queryset(self.get_queryset())
queryset = queryset.filter(user_id=self.kwargs["pk"])
filter_kwargs = {
"organization_id": self.kwargs["org_id"],
}
obj = get_object_or_404(queryset, **filter_kwargs)
self.check_object_permissions(self.request, obj)
return obj

def update(self, request, *args, **kwargs):
kwargs["partial"] = True
return super().update(request, *args, **kwargs)
Comment thread
BHARATH0153 marked this conversation as resolved.
Comment thread
BHARATH0153 marked this conversation as resolved.
Comment on lines +404 to +406

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This turns PUT into a partial update, so PUT accepts {"is_admin": true} without a complete representation. It differs from the other update endpoints and removes the route-body invariant needed for this membership resource.

Please restore standard DRF PUT semantics, keep PATCH for partial changes, and cover the distinction with a regression test.

Severity: P2


def destroy(self, request, *args, **kwargs):
try:
return super().destroy(request, *args, **kwargs)
except OwnershipRequired as error:
raise ValidationError(str(error))


obtain_auth_token = ObtainAuthTokenView.as_view()
password_reset = PasswordResetView.as_view()
password_reset_confirm = PasswordResetConfirmView.as_view()
Expand All @@ -344,3 +425,5 @@ def get_object(self):
password_change = PasswordChangeView.as_view()
email_update = EmailUpdateView.as_view()
email_list = EmailListCreateView.as_view()
organization_membership_list = OrganizationMembershipListCreateView.as_view()
organization_membership_detail = OrganizationMembershipDetailView.as_view()
Loading
Loading