Skip to content

Conversation

@eliykat
Copy link
Member

@eliykat eliykat commented Jan 16, 2026

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-23768

📔 Objective

Add endpoints to the public API to restore and revoke members.

Implementation decisions:

  • added explicit restore and revoke endpoints, instead of allowing PUT to update a member's status. This is because there are other statuses as well (invited, accepted, confirmed) and you cannot transition from any status to any other status. Allowing arbitrary writes to the member's status would be likely to cause confusion.

  • used POST instead of PUT. From RFC9110:

    The fundamental difference between the POST and PUT methods is highlighted by the different intent for the enclosed representation. The target resource in a POST request is intended to handle the enclosed representation according to the resource's own semantics, whereas the enclosed representation in a PUT request is defined as replacing the state of the target resource. Hence, the intent of PUT is idempotent and visible to intermediaries, even though the exact effect is only known by the origin server.

    In this case, the operation is not idempotent: revoking a user once returns a 200 OK result, revoking a user twice returns a 400 Bad Request (already revoked). Also, the request is not replacing the resource with the resource in the request - it is telling the server to perform an action according to its own semantics. This suggests that POST is the better choice.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@eliykat eliykat requested a review from a team as a code owner January 16, 2026 04:50
@eliykat eliykat requested review from BTreston and jrmccannon and removed request for BTreston January 16, 2026 04:50
@eliykat eliykat added the ai-review Request a Claude code review label Jan 16, 2026
@claude
Copy link
Contributor

claude bot commented Jan 16, 2026

Claude finished @eliykat's task in 2m 34s —— View job


Code Review: Approved

Reviewed: All changes including incremental updates since 2026-01-16

Summary

This PR adds restore and revoke endpoints to the Public API for organization members. The implementation is clean and follows established patterns.

Changes reviewed:

  • MembersController.cs - New POST /public/members/{id}/revoke and POST /public/members/{id}/restore endpoints
  • ErrorResponseModel.cs - Added [JsonConstructor] attribute and improved nullability (removed #nullable disable, changed to immutable properties)
  • MembersControllerTests.cs - Comprehensive integration tests for both endpoints

Security - Organization ownership is properly validated before performing operations. Both endpoints verify organizationUser.OrganizationId == _currentContext.OrganizationId before proceeding.

Design Decisions - The choice of POST over PUT is well-justified per RFC9110 since these are non-idempotent operations (revoking twice returns different status codes).

Error Handling - Properly leverages the existing error handling infrastructure:

  • Revoke uses the v2 command pattern with Match<IActionResult> for explicit error handling
  • Restore relies on BadRequestException caught by the global ExceptionHandlerFilterAttribute

Test Coverage - Comprehensive integration tests covering:

  • Success cases for both revoke and restore
  • Already revoked/active error cases
  • Not found cases
  • Cross-organization authorization checks

Recent fixes verified:

  • [JsonConstructor] attribute correctly enables test deserialization
  • Restore_Member_Success test properly verifies that invited users are restored to Invited status

No issues found.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 16, 2026

Logo
Checkmarx One – Scan Summary & Details00f26b2e-14df-45e5-b8da-c698f6b1e208

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Jan 16, 2026

Codecov Report

❌ Patch coverage is 90.90909% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.10%. Comparing base (d8379d3) to head (7599b73).

Files with missing lines Patch % Lines
...minConsole/Public/Controllers/MembersController.cs 93.33% 0 Missing and 2 partials ⚠️
...c/Api/Models/Public/Response/ErrorResponseModel.cs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6859      +/-   ##
==========================================
+ Coverage   56.07%   56.10%   +0.03%     
==========================================
  Files        1968     1968              
  Lines       86927    86956      +29     
  Branches     7742     7744       +2     
==========================================
+ Hits        48740    48783      +43     
+ Misses      36386    36370      -16     
- Partials     1801     1803       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants