Skip to content

[PM-35253] Add organization ability UseInviteLinks#7489

Open
r-tome wants to merge 14 commits intomainfrom
ac/pm-35253/add-useinvitelinks-organization-ability
Open

[PM-35253] Add organization ability UseInviteLinks#7489
r-tome wants to merge 14 commits intomainfrom
ac/pm-35253/add-useinvitelinks-organization-ability

Conversation

@r-tome
Copy link
Copy Markdown
Contributor

@r-tome r-tome commented Apr 17, 2026

🎟️ Tracking

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

📔 Objective

Add the new organization ability UseInviteLinks across the server (schema/migrations, domain + licensing, API models, and tests).

A database data migration is included to enable UseInviteLinks for existing Enterprise organizations.

Related Pricing Service changes: https://github.com/bitwarden/billing-pricing/pull/103
Related Clients changes: bitwarden/clients#20227

@r-tome r-tome added the ai-review-vnext Request a Claude code review using the vNext workflow label Apr 17, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 17, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds the UseInviteLinks organization ability end-to-end across the server: MSSQL schema (table, stored procedures, views), EF migrations for MySQL/Postgres/SQLite, the Organization entity, licensing claims and verification, API response models, plan features, and the OrganizationAbility projection. It includes a batched MSSQL data migration (with EF equivalents) to enable the feature for existing Enterprise organizations across all historical plan types (4, 5, 10, 11, 14, 15, 19, 20), and adds integration tests validating the migration applies correctly to Enterprise plans and not to non-Enterprise plans, with idempotency coverage.

Code Review Details

No actionable findings. The implementation follows the established pattern for similar recently-added abilities (UseMyItems, UsePhishingBlocker, UseDisableSmAdsForUsers):

  • License hash exclusion is correctly added in OrganizationLicense.GetDataBytes alongside the other post-v15 claims, preserving backward compatibility with older license files.
  • VerifyData uses the HasClaim guard pattern so older licenses without the UseInviteLinks claim still validate.
  • The SQL data migration is batched (TOP (@BatchSize) with a self-converging UseInviteLinks = 0 predicate) and the reviewer has already validated runtime (~1 minute against backup).
  • Column ordering in Organization.sql was addressed in the prior review thread (now placed after ExemptFromBillingAutomation).
  • PlanType integer literals in the data migrations match the PlanType enum for all Enterprise variants.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 17, 2026

Logo
Checkmarx One – Scan Summary & Details74b800eb-94ed-4a93-98b1-53e082716e0f


New Issues (1) Checkmarx found the following issues in this Pull Request
# Severity Issue Source File / Package Checkmarx Insight
1 MEDIUM CSRF src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 512
detailsMethod at line 512 of /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs gets a parameter from a user request from orgId. This par...
Attack Vector

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

❌ Patch coverage is 97.29730% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.62%. Comparing base (dbd0d52) to head (3ab5864).

Files with missing lines Patch % Lines
.../Core/AdminConsole/Services/OrganizationFactory.cs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7489      +/-   ##
==========================================
+ Coverage   59.13%   63.62%   +4.48%     
==========================================
  Files        2077     2077              
  Lines       91848    91881      +33     
  Branches     8175     8175              
==========================================
+ Hits        54315    58455    +4140     
+ Misses      35601    31408    -4193     
- Partials     1932     2018      +86     

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

@r-tome r-tome marked this pull request as ready for review April 17, 2026 13:34
@r-tome r-tome requested review from a team as code owners April 17, 2026 13:34
Comment thread src/Sql/dbo/Tables/Organization.sql Outdated
[UsePhishingBlocker] BIT NOT NULL CONSTRAINT [DF_Organization_UsePhishingBlocker] DEFAULT (0),
[UseDisableSmAdsForUsers] BIT NOT NULL CONSTRAINT [DF_Organization_UseDisableSmAdsForUsers] DEFAULT (0),
[UseMyItems] BIT NOT NULL CONSTRAINT [DF_Organization_UseMyItems] DEFAULT (0),
[UseInviteLinks] BIT NOT NULL CONSTRAINT [DF_Organization_UseInviteLinks] DEFAULT (0),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ PR 7438 has /util/Migrator/DbScripts/2026-04-10_02_AddExemptFromBillingAutomation.sql, which adds the ExemptFromBillingAutomation column to this table. If this PR is deployed with the same release (or later) as PR 7438, the columns will be out of order. Do you know if this deployment will happen before 7438? If not, then the UseInviteLinks column should be placed after the ExemptFromBillingAutomation, and the other objects should be adjusted as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mkincaid-bw well observed! I have fixed the order of the columns by putting UseInviteLinks after ExemptFromBillingAutomation

JimmyVo16
JimmyVo16 previously approved these changes Apr 17, 2026
Copy link
Copy Markdown
Contributor

@mkincaid-bw mkincaid-bw left a comment

Choose a reason for hiding this comment

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

LGTM


WHILE @RowsAffected > 0
BEGIN
UPDATE TOP (@BatchSize) [dbo].[Organization]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note: I tested this against a backup and it took about a minute to run.

@sonarqubecloud
Copy link
Copy Markdown

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

Labels

ai-review-vnext Request a Claude code review using the vNext workflow needs-qa

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants