Skip to content

[PM-29555] Add self-revoke endpoint for declining organization data ownership policy#6739

Merged
r-tome merged 16 commits intomainfrom
ac/pm-29555/self-revoke-endpoint
Jan 6, 2026
Merged

[PM-29555] Add self-revoke endpoint for declining organization data ownership policy#6739
r-tome merged 16 commits intomainfrom
ac/pm-29555/self-revoke-endpoint

Conversation

@r-tome
Copy link
Copy Markdown
Contributor

@r-tome r-tome commented Dec 16, 2025

🎟️ Tracking

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

📔 Objective

Adds PUT /organizations/{orgId}/users/revoke-self endpoint that allows organization users to self-revoke when declining the OrganizationDataOwnership policy.

The endpoint verifies that the policy is enabled, requires the user to be in a Confirmed state, and blocks Owners and Admins from self-revoking. Adds a new OrganizationUser_SelfRevoked event for audit logging.

⏰ 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

… self-revocation logic, including success scenarios and various failure conditions.
…rviceCollectionExtensions for user self-revocation functionality
…h new endpoint for user-initiated revocation
…ersController, covering scenarios for eligible users, non-members, and users with owner/admin roles.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 16, 2025

Logo
Checkmarx One – Scan Summary & Details672139d5-518d-45b9-81dd-a16a31cc8223

New Issues (3)

Checkmarx found the following issues in this Pull Request

# Severity Issue Source File / Package Checkmarx Insight
1 MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1511
detailsMethod at line 1511 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ...
Attack Vector
2 MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 1387
detailsMethod at line 1387 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ...
Attack Vector
3 MEDIUM CSRF /src/Api/Vault/Controllers/CiphersController.cs: 245
detailsMethod at line 245 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows t...
Attack Vector
Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs: 98

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 16, 2025

Codecov Report

❌ Patch coverage is 94.11765% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.96%. Comparing base (35868c2) to head (d6ceb19).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...Console/Controllers/OrganizationUsersController.cs 72.72% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6739      +/-   ##
==========================================
+ Coverage   54.93%   54.96%   +0.03%     
==========================================
  Files        1927     1930       +3     
  Lines       85457    85507      +50     
  Branches     7648     7654       +6     
==========================================
+ Hits        46949    47003      +54     
+ Misses      36723    36717       -6     
- Partials     1785     1787       +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.

…ior when a user attempts to self-revoke without confirmation. This test checks for a BadRequestException with an appropriate message.
@r-tome r-tome marked this pull request as ready for review December 16, 2025 15:25
@r-tome r-tome requested a review from a team as a code owner December 16, 2025 15:25
@r-tome r-tome requested a review from eliykat December 16, 2025 15:25
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Dec 16, 2025

Claude finished @r-tome's task in 2m 23s —— View job


Code Review Complete

  • Starting code review
  • Analyzing changes
  • Posting feedback

The review has been completed. Please check the inline comments for specific feedback.

Copy link
Copy Markdown
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

Looks good! Here are a few suggestions, but I can be flexible on them if timing is an issue because Vault need it.

Comment thread src/Api/AdminConsole/Controllers/OrganizationUsersController.cs Outdated
- Implemented MemberRequirement to check if a user is a member of the organization.
- Added unit tests for MemberRequirement to validate authorization logic for different user types.
…egration test for provider users

- Changed authorization attribute from MemberOrProviderRequirement to MemberRequirement in the RevokeSelfAsync method.
- Added a new integration test to verify that provider users who are not members receive a forbidden response when attempting to revoke themselves.
…quirement

- Implemented the EligibleForSelfRevoke method to determine if a user can self-revoke their data ownership based on their membership status and policy state.
- Added unit tests to validate the eligibility logic for confirmed, invited, and non-policy users, as well as for different organization IDs.
- Updated the SelfRevokeOrganizationUserCommand to utilize policy requirements for determining user eligibility for self-revocation.
- Implemented checks to prevent the last owner from revoking themselves, ensuring organizational integrity.
- Modified unit tests to reflect changes in eligibility logic and added scenarios for confirmed owners and admins.
- Removed deprecated policy checks and streamlined the command's dependencies.
@r-tome r-tome requested a review from a team as a code owner December 22, 2025 13:36
@r-tome r-tome requested a review from eliykat December 22, 2025 14:24
eliykat
eliykat previously approved these changes Dec 30, 2025
Copy link
Copy Markdown
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

Some non-blocking comments on comments. 😀

Comment on lines +76 to +77
/// <summary>
/// Determines if a user is eligible for self-revocation under the Organization Data Ownership policy.
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.

Worth noting something like: "Self-revoke is used to opt out of migrating the user's personal vault to the organization pursuant to this policy." Just to explain how self-revoke relates to this policy.

Comment on lines +13 to +14
/// Validates the OrganizationDataOwnership policy is enabled and applies to the user (currently Owners/Admins are exempt),
/// the user is a confirmed member, and prevents the last owner from revoking themselves.
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.

I worry about including this much implementation detail in the xmldoc; generally callers don't need to know what checks are done internally. However if you think they're useful to include, I think the <remarks> section is better for this.

Copy link
Copy Markdown
Contributor

@AlexRubik AlexRubik left a comment

Choose a reason for hiding this comment

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

dirt code lgtm

@r-tome r-tome merged commit 1b17d99 into main Jan 6, 2026
65 of 66 checks passed
@r-tome r-tome deleted the ac/pm-29555/self-revoke-endpoint branch January 6, 2026 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants