[PM-25913] Fix owners unable to rename provider-managed organization

[eliykat]

🎟️ Tracking

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

📔 Objective

Fix a bug where the owner of an organization managed by a provider could not update the organization name.

Root cause: changing an organization name triggers a Stripe update, which checks whether the owner has permission to manage billing. Owners cannot manage billing if the organization is managed by a provider (it's managed by the provider), so the change fails.

Solution: separate authorization checks from business logic. Previously this was tightly coupled because shouldUpdateBilling was evaluated in the controller and passed into the service method. With this PR, the service method is moved to a command, and the command logic is separated from authorization checks in the controller. This allows owners to change the name, even though this will trigger a Stripe update, and even though they do not have permission to make Stripe changes otherwise.

Other changes: There was some accumulated tech debt here I kept tripping over:

• the business logic was still in OrganizationService rather than a command, and was a mix of AC and Billing logic. AC logic has been moved into a command and Billing logic into one of their services (as discussed with @amorask-bitwarden)
• deprecated/unused properties in the request model. These have been removed.
• OrganizationsController did not use SutProvider, requiring manual updates to dependencies every time. This has been updated.
• this flow has a lot of optionality to it: you can update any combination of the organization name, billing email, and public and private keys on cloud, or only some of these on self-host. This was not well documented. I have updated the models and the private API to use nullable properties because this is a simpler API to communicate "no changes required for this property". The flow of the command also makes the self-host/cloud split clear.
• no tests, which have now been added.

Web PR: bitwarden/clients#17482

📸 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

[claude]

Claude finished @eliykat's task —— View job

---

Code Review Complete

This is a well-executed refactoring that successfully separates authorization from business logic, addressing the root cause where organization owners couldn't rename provider-managed organizations.

---

Summary of Changes Since Last Review

This PR has been through multiple review cycles, with previous reviewers (cyprain-okeke and JimmyVo16) having approved. The latest commits addressed their feedback about using extension methods instead of modifying input parameters.

Core architectural improvements:

• Extracted organization update logic from OrganizationService into OrganizationUpdateCommand (command pattern)
• Moved Stripe billing updates to OrganizationBillingService
• Separated authorization checks from business logic in the controller
• Made request model properties nullable to clearly communicate optional updates
• Added comprehensive test coverage (unit + integration tests)

---

Findings

Finding 1: Billing email trimming inconsistency
💭 In OrganizationUpdateExtensions.cs:22, the billing email is trimmed and lowercased. However, in the controller authorization check (OrganizationsController.cs:234), we check if model.BillingEmail is not null to determine if billing permission is required. If a user sends an empty string or whitespace-only string for BillingEmail, it will pass the authorization check (requiring billing permissions) but after trimming might become empty. Consider whether empty/whitespace strings should be normalized to null in the request model binding or validation layer.

Example scenario

// User sends: { "BillingEmail": "   " }
// Controller: requiresBillingPermission = true (requires EditSubscription)
// Extension: result is "   ".ToLowerInvariant().Trim() = ""
// Organization ends up with empty billing email

This may be intentional, but worth verifying the desired behavior.

Finding 2: Missing validation for organization name length
⚠️ The request model has [StringLength(50)] validation on the Name property (OrganizationUpdateRequestModel.cs:10), but the Stripe custom field value is truncated to 30 characters in OrganizationBillingService.cs:200-202. The 50-character limit for organization names should be documented or validated earlier in the flow to set proper expectations. The truncation is correctly handled, but users may not realize their 50-character name will be shortened in Stripe invoices.

Finding 3: Authorization logic correctness
The authorization separation correctly addresses the bug:

• Owners can change name (only requires OrganizationOwner permission) - OrganizationsController.cs:237
• Changing billing email requires EditSubscription permission - OrganizationsController.cs:234-236
• The command handles Stripe updates internally without checking permissions

This properly allows owners of provider-managed orgs to rename, since name changes trigger Stripe updates but don't require billing permissions anymore. Well done.

---

Good Practices Observed

• Command pattern properly separates concerns
• Nullable properties clearly document optional parameters
• Comprehensive test coverage including edge cases (self-hosted, no gateway customer, provider scenarios)
• Extension methods provide clean separation of concerns
• Integration tests verify real-world authorization scenarios
• Clear documentation in comments explaining self-host behavior

---

Suggestions

Finding 4: Consider extracting authorization logic to a separate method
🎨 The authorization check in OrganizationsController.cs:234-242 could be extracted to a private method like IsAuthorizedToUpdate(Guid organizationId, OrganizationUpdateRequestModel model) for better testability and readability. Currently, this logic is inline and would be difficult to unit test in isolation.

Suggested refactoring

private async Task<bool> IsAuthorizedToUpdate(Guid organizationId, OrganizationUpdateRequestModel model)
{
    var requiresBillingPermission = model.BillingEmail is not null;
    return requiresBillingPermission
        ? await _currentContext.EditSubscription(organizationId)
        : await _currentContext.OrganizationOwner(organizationId);
}

public async Task<IResult> Put(Guid organizationId, [FromBody] OrganizationUpdateRequestModel model)
{
    if (!await IsAuthorizedToUpdate(organizationId, model))
    {
        return TypedResults.Unauthorized();
    }
    // ...
}

Finding 5: Self-host documentation could be more prominent
💭 The self-host behavior (only updates keys, ignores name/billing) is well-documented in OrganizationUpdateCommand.cs:52-56, but this constraint isn't visible at the API layer. Consider adding this to the XML documentation on the controller endpoint or the request model so API consumers understand the limitation.

---

Security & Compliance

✅ Zero-knowledge principles maintained: Private keys remain encrypted
✅ Authorization properly enforced: Billing changes require appropriate permissions
✅ No PII logging concerns: No sensitive data exposed in new code
✅ Nullable reference types used: ADR 0024 compliance

---

Test Coverage

Excellent coverage across:

• Unit tests for command logic (414 lines)
• Unit tests for billing service updates (94 new lines)
• Integration tests for authorization scenarios (196 lines)
• Edge cases: self-host, null values, gateway customer ID scenarios

Codecov report shows 98.21% patch coverage - the 2 missing lines are acceptable and likely in auto-generated code or edge cases.

---

Action Items

1. Optional: Review Finding 1 regarding empty string handling for billing email
2. Optional: Consider Finding 4's suggestion to extract authorization logic for better testability
3. Ready to merge after addressing or acknowledging these optional improvements

---

This is high-quality work that successfully resolves the issue while improving code organization and test coverage. The separation of concerns is well-executed, and the nullable properties API is cleaner than the previous approach. The previous reviewers' feedback has been adequately addressed.

[codecov]

Codecov Report

❌ Patch coverage is 98.21429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.25%. Comparing base (3559759) to head (7470685).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...st/Organizations/OrganizationUpdateRequestModel.cs	90.00%	0 Missing and 1 partial ⚠️
.../Organizations/Update/OrganizationUpdateRequest.cs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6599      +/-   ##
==========================================
+ Coverage   53.10%   53.25%   +0.15%     
==========================================
  Files        1903     1906       +3     
  Lines       84877    84955      +78     
  Branches     7633     7639       +6     
==========================================
+ Hits        45071    45247     +176     
+ Misses      38055    37954     -101     
- Partials     1751     1754       +3     

☔ 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.

[cyprain-okeke]

Looks Good for billing

[JimmyVo16]

Not a dealbreaker, but I’m always uneasy about modifying input parameters. It isn’t obvious from looking at the method, and it can lead to race conditions.

Based on what you’re trying to do, I think using an extension method would make more sense here and provide more idiomatic meaning.

[JimmyVo16]

With that said, the impact here is pretty small if the scope is only within this class.

[eliykat]

Done

[JimmyVo16]

same

[eliykat]

Done

[JimmyVo16]

Also non-blocker: same but with a different name for this case.