openwisp / openwisp/openwisp-users

[bug] Resending an unchanged is_admin value deletes the OrganizationUser membership

Open
#552 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
179
Forks
96
Avg merge
13h 40m
Merged PRs (30d)
16

Description

Describe the bug

SuperUserDetailSerializer.update() in openwisp_users/api/serializers.py uses a non-obvious rule for organization_users: if the payload includes an existing membership and the submitted is_admin value is the same as the current value, the membership is deleted instead of being left unchanged.

This behavior is implemented here:

if org_user.is_admin != org_user_data["is_admin"]:
    org_user.is_admin = org_user_data["is_admin"]
    org_user.full_clean()
    org_user.save()
else:
    org_user.delete()

As a result, API clients that resend the current representation of a user on PATCH can unintentionally delete organization memberships. The only way to preserve an existing membership is to omit it entirely from the payload, or omit is_admin from that specific entry.

Why this is a problem

  • The delete-vs-update behavior is not documented in the REST API docs.
  • The serializer response representation includes organization_users, so a naive PATCH round-trip can be destructive.
  • This is easy to miss for API consumers who expect unchanged fields to remain unchanged.

Relevant code

  • openwisp_users/api/serializers.py
    • BaseSuperUserSerializer.to_representation() serializes organization_users with organization and is_admin.
    • SuperUserDetailSerializer.update() deletes the membership when is_admin matches the current value.
  • docs/user/rest-api.rst currently notes that is_admin represents the organization manager flag, but does not document the delete-on-unchanged behavior.
  • Existing tests such as test_remove_org_user_api and test_make_user_org_admin_api cover parts of this flow, but do not make the contract explicit in the docs.

Proposed resolution

Please make a product decision on one of these options:

  1. Document the current behavior explicitly, or
  2. Replace it with an explicit removal signal instead of overloading "unchanged" to mean "delete".

Option 2 would be a breaking API change for existing consumers, so it likely needs a migration plan or versioning strategy.

Notes

This is unrelated to the disabled-organization write protection work. The behavior predates that effort and appears to come from the original REST API implementation.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with SuperUserDetailSerializer.update() and BaseSuperUserSerializer.to_representation() in openwisp_users/api/serializers.py, then read docs/user/rest-api.rst and the tests test_remove_org_user_api and test_make_user_org_admin_api. The issue requires a product decision between documenting the current behavior and changing the removal signal; done means the chosen contract is explicit and covered by the relevant documentation and tests, with migration or versioning considered for a breaking change.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
api, backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.