Skip to content

[PM-39557] Replace invite link key columns with an opaque Invite blob#7896

Merged
r-tome merged 12 commits into
mainfrom
ac/pm-39557/update-schema-to-use-invite-blob
Jul 6, 2026
Merged

[PM-39557] Replace invite link key columns with an opaque Invite blob#7896
r-tome merged 12 commits into
mainfrom
ac/pm-39557/update-schema-to-use-invite-blob

Conversation

@r-tome

@r-tome r-tome commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

Store the invite link's cryptographic material as a single opaque Invite blob plus a SupportsConfirmation flag instead of the separate EncryptedInviteKey/EncryptedOrgKey columns, so the server stays a pure storage/transport layer and the blob's format can evolve without AC changes.

r-tome added 6 commits June 30, 2026 16:28
…d EncryptedOrgKey with Invite blob and SupportsConfirmation flag. Update stored procedures and migration script accordingly.
…ptedInviteKey with Invite and add SupportsConfirmation flag. Update model snapshot for MySQL, PostgreSQL, and SQLite.
…and EncryptedOrgKey with Invite property and add SupportsConfirmation flag. Update related command and request classes accordingly.
…pportsConfirmation flag, reflecting recent schema changes.
…and EncryptedOrgKey with Invite property in Create and Refresh request models, and update OrganizationInviteLinkResponseModel to include SupportsConfirmation flag. Adjust related command request methods accordingly.
…s various controllers and models, ensuring consistency with recent schema changes. Adjust assertions and request models accordingly to reflect the new structure.
@r-tome r-tome added the ai-review Request a Claude code review label Jun 30, 2026
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the replacement of the invite link's EncryptedInviteKey/EncryptedOrgKey columns with a single opaque Invite blob plus a SupportsConfirmation flag across the API models, entity, commands, Dapper repository/stored procedures, SSDT schema, EF migrations for all three providers, and the accompanying unit/integration tests. The change is internally consistent: the SSDT table's resulting column order matches what the dated migration produces (drop EncryptedOrgKey, rename EncryptedInviteKeyInvite, add SupportsConfirmation at the end), and the migration follows the idempotent COL_LENGTH guard pattern. Dropping the [EncryptedString] format check on Invite is intentional and documented — the server treats the value as an opaque blob it only stores and transports. Test coverage is thorough across request-model validation, command behavior, and repository round-trips.

No blocking findings. The open reviewer threads (column ordering, SupportsConfirmation sourcing) already capture the substantive discussion, and the code addresses the column-ordering concern.

name: "EncryptedOrgKey",
table: "OrganizationInviteLink");

migrationBuilder.RenameColumn(

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.

EF decided to rename this column instead of dropping and creating a new one. I guess there is no harm because this feature hasn't been rolled out yet.

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.

Like the SQL migration, this will cause the table to end up with columns out of order from the definition defined in src/Sql/dbo/Tables/OrganizationInviteLink.sql, so I think these should be written so that the existing columns are dropped and then the new ones added at the end.

If you don't want to do that, you could have the SQL Server migration follow the EF pattern and drop EncryptedOrgKey, rename EncryptedInviteKey to Invite, then add SupportsConfirmation. It doesn't really matter except that the end-result of the tables should be consistent across environments.

@r-tome r-tome marked this pull request as ready for review June 30, 2026 15:46
@r-tome r-tome requested review from a team as code owners June 30, 2026 15:46
@r-tome r-tome requested a review from jrmccannon June 30, 2026 15:46
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ D)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.87%. Comparing base (e899a9c) to head (29dab05).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7896      +/-   ##
==========================================
+ Coverage   61.35%   65.87%   +4.51%     
==========================================
  Files        2236     2236              
  Lines       98547    98547              
  Branches     8911     8911              
==========================================
+ Hits        60468    64917    +4449     
+ Misses      35943    31397    -4546     
- Partials     2136     2233      +97     

☔ View full report in Codecov by Harness.
📢 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.

@mkincaid-bw mkincaid-bw left a comment

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.

Some minor changes requested.

Comment on lines 1 to 13
CREATE TABLE [dbo].[OrganizationInviteLink]
(
[Id] UNIQUEIDENTIFIER NOT NULL,
[Code] UNIQUEIDENTIFIER NOT NULL,
[OrganizationId] UNIQUEIDENTIFIER NOT NULL,
[AllowedDomains] NVARCHAR(MAX) NOT NULL,
[EncryptedInviteKey] NVARCHAR(MAX) NOT NULL,
[EncryptedOrgKey] NVARCHAR(MAX) NULL,
[CreationDate] DATETIME2(7) NOT NULL,
[RevisionDate] DATETIME2(7) NOT NULL,
[Id] UNIQUEIDENTIFIER NOT NULL,
[Code] UNIQUEIDENTIFIER NOT NULL,
[OrganizationId] UNIQUEIDENTIFIER NOT NULL,
[AllowedDomains] NVARCHAR(MAX) NOT NULL,
[Invite] NVARCHAR(MAX) NOT NULL,
[SupportsConfirmation] BIT NOT NULL,
[CreationDate] DATETIME2(7) NOT NULL,
[RevisionDate] DATETIME2(7) NOT NULL,
CONSTRAINT [PK_OrganizationInviteLink] PRIMARY KEY CLUSTERED ([Id] ASC),
CONSTRAINT [FK_OrganizationInviteLink_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id]) ON DELETE CASCADE
);

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.

The net result of the migration script will add these columns at the end, so the table definition will look like this:

CREATE TABLE [dbo].[OrganizationInviteLink]
(
    [Id]                   UNIQUEIDENTIFIER NOT NULL,
    [Code]                 UNIQUEIDENTIFIER NOT NULL,
    [OrganizationId]       UNIQUEIDENTIFIER NOT NULL,
    [AllowedDomains]       NVARCHAR(MAX)    NOT NULL,
    [CreationDate]         DATETIME2(7)     NOT NULL,
    [RevisionDate]         DATETIME2(7)     NOT NULL,
    [Invite]               NVARCHAR(MAX)    NOT NULL,
	[SupportsConfirmation] BIT              NOT NULL,
    CONSTRAINT [PK_OrganizationInviteLink] PRIMARY KEY CLUSTERED ([Id] ASC),
    CONSTRAINT [FK_OrganizationInviteLink_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id]) ON DELETE CASCADE
);

Comment thread util/Migrator/DbScripts/2026-06-30_00_OrganizationInviteLinkInviteBlob.sql Outdated
Comment thread util/Migrator/DbScripts/2026-06-30_00_OrganizationInviteLinkInviteBlob.sql Outdated
name: "EncryptedOrgKey",
table: "OrganizationInviteLink");

migrationBuilder.RenameColumn(

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.

Like the SQL migration, this will cause the table to end up with columns out of order from the definition defined in src/Sql/dbo/Tables/OrganizationInviteLink.sql, so I think these should be written so that the existing columns are dropped and then the new ones added at the end.

If you don't want to do that, you could have the SQL Server migration follow the EF pattern and drop EncryptedOrgKey, rename EncryptedInviteKey to Invite, then add SupportsConfirmation. It doesn't really matter except that the end-result of the tables should be consistent across environments.

r-tome added 4 commits July 2, 2026 11:54
Rewrite the OrganizationInviteLink migration to drop EncryptedOrgKey, rename
EncryptedInviteKey to Invite, and add SupportsConfirmation at the end, matching
the EF migrations. Reorder the SSDT table definition so SupportsConfirmation
sits after RevisionDate, keeping src/Sql/dbo in sync with what a fresh run of
all migrations produces across the four supported databases.
Renamed from 2026-06-30 so environments that already applied the previous
version pick up the corrected script as a new, distinct migration.
…ngth

- Add a required SupportsConfirmation field to the create/refresh API
  request models and command request records for forward-compatibility.
  The commands still hardcode the persisted value to false because auto
  confirmation isn't supported yet; when Milestone 3 ships the flow, the
  hardcode is removed without changing the request shape.
- Cap Invite to 3000 characters via [EncryptedStringLength] on both
  request models to bound the opaque blob. Intentionally not adding
  [EncryptedString] — the server treats the blob as opaque so the format
  can evolve without server changes.
- Update unit and integration tests to populate the now-required
  SupportsConfirmation field and assert the server-side override.
@r-tome r-tome requested review from JimmyVo16 and mkincaid-bw July 2, 2026 13:11
@r-tome

r-tome commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@mkincaid-bw thanks for the review and suggestions! I have followed your suggestion to update sql to match EF and rename the existing column and add the new one at the end.

@jrmccannon jrmccannon added the t:feature Change Type - Feature Development label Jul 2, 2026
mkincaid-bw
mkincaid-bw previously approved these changes Jul 2, 2026

@mkincaid-bw mkincaid-bw left a comment

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.

DB Changes LGTM but I just wanted to leave a note since this could be flagged by others:

This PR technically violates EDD with the column drop/rename. However, the OrganizationInviteLink table in prod is currently empty and from what I understand, the code has never been enabled in prod, so we should be fine. I also checked QA and the table is empty there as well.

r-tome added 2 commits July 3, 2026 16:40
… SupportsConfirmation from requests

- Modify CreateOrganizationInviteLinkCommand and RefreshOrganizationInviteLinkCommand to use the SupportsConfirmation value from the request instead of hardcoding it to false.
- Update related tests to assert the correct SupportsConfirmation value based on the request input, ensuring consistency in behavior across the application.
@r-tome

r-tome commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

DB Changes LGTM but I just wanted to leave a note since this could be flagged by others:

This PR technically violates EDD with the column drop/rename. However, the OrganizationInviteLink table in prod is currently empty and from what I understand, the code has never been enabled in prod, so we should be fine. I also checked QA and the table is empty there as well.

@mkincaid-bw I agree this violates EDD. Thanks for checking prod and QA and since those tables are empty and this feature hasn't been released no data is at risk.

@r-tome r-tome requested a review from mkincaid-bw July 3, 2026 16:12
@r-tome r-tome merged commit 7b36f20 into main Jul 6, 2026
56 checks passed
@r-tome r-tome deleted the ac/pm-39557/update-schema-to-use-invite-blob branch July 6, 2026 17:01
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 t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants