Skip to content

Conversation

@audreyality
Copy link
Member

@audreyality audreyality commented May 28, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-21918
https://bitwarden.atlassian.net/browse/PM-29783
https://bitwarden.atlassian.net/browse/PM-29789

https://bitwarden.atlassian.net/wiki/spaces/EN/pages/2164654083/Email-authenticated+Sends+-+Updated

Implements #5895

📔 Objective

Original

Update send API models to support email/OTP administration.

Important

This PR includes documentation for the related models. I mostly inferred what each of these are. Please focus on them during review to confirm that the comments are correct.

Updated

In a recent tech breakdown it was decided we would also add an explicit property AuthType that will allow the client to determine the authentication strategy to use (email OTP, password, or none) without relying on the presence or absence of Emails or Password properties in order to infer the strategy. The latest iteration of this PR accomplishes the following:

  • authentication strategy inference is still preserved, and is used as a fallback when the nullable AuthType property is not set
  • send-related models that were previously updated to include Emails were also updated to include the AuthType property
  • most FIXMEs were addressed: One overload of ToSend was renamed to UpdateSend and GlobalSettings were eliminated from one of the updated files
  • extensive test coverage was added to cover AuthType inference, explicit assignment, and other behavior not formerly covered in SendsController

⏰ 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

@audreyality audreyality requested a review from a team as a code owner May 28, 2025 18:34
@audreyality audreyality added the hold Hold this PR or item until later; DO NOT MERGE label May 28, 2025
@audreyality audreyality marked this pull request as draft May 28, 2025 18:35
@audreyality

This comment was marked as outdated.

@github-actions

This comment was marked as resolved.

@codecov
Copy link

codecov bot commented May 28, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.72%. Comparing base (96622d7) to head (6bfacec).
⚠️ Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
src/Api/Tools/Controllers/SendsController.cs 51.51% 10 Missing and 6 partials ⚠️
.../Core/Tools/SendFeatures/Queries/SendOwnerQuery.cs 88.88% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5895      +/-   ##
==========================================
+ Coverage   58.58%   58.72%   +0.14%     
==========================================
  Files        1920     1922       +2     
  Lines       85264    85328      +64     
  Branches     7632     7649      +17     
==========================================
+ Hits        49953    50112     +159     
+ Misses      33465    33356     -109     
- Partials     1846     1860      +14     

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

@audreyality audreyality added needs-qa and removed hold Hold this PR or item until later; DO NOT MERGE labels Jun 26, 2025
@audreyality audreyality marked this pull request as ready for review June 26, 2025 20:57
@djsmith85 djsmith85 self-requested a review June 30, 2025 16:13
djsmith85

This comment was marked as outdated.

@audreyality audreyality requested a review from djsmith85 July 1, 2025 12:42
@sonarqubecloud

This comment was marked as outdated.

djsmith85
djsmith85 previously approved these changes Jul 1, 2025
@audreyality audreyality marked this pull request as draft July 28, 2025 18:37
@audreyality

This comment was marked as outdated.

@audreyality audreyality marked this pull request as ready for review August 15, 2025 20:29
@audreyality audreyality requested a review from a team as a code owner August 15, 2025 20:29
Copy link
Collaborator

@jaasen-livefront jaasen-livefront left a comment

Choose a reason for hiding this comment

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

Approve vault change

@harr1424 harr1424 dismissed stale reviews from quexten and mkincaid-bw via f9b700e December 23, 2025 17:57
itsadrago
itsadrago previously approved these changes Dec 23, 2025
itsadrago
itsadrago previously approved these changes Dec 23, 2025
-- in 2 server releases
@Emails NVARCHAR(1024) = NULL
@Emails NVARCHAR(4000) = NULL,
@AuthType TINYINT = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Default value should be NULL

Suggested change
@AuthType TINYINT = 2
@AuthType TINYINT = NULL

@CipherId UNIQUEIDENTIFIER = NULL,
@Emails NVARCHAR(1024) = NULL
@Emails NVARCHAR(4000) = NULL,
@AuthType TINYINT = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Default value should be NULL

Suggested change
@AuthType TINYINT = 2
@AuthType TINYINT = NULL

Copy link
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.

I see the updates now, LGTM

@harr1424 harr1424 merged commit 484a8e4 into main Dec 31, 2025
70 of 71 checks passed
@harr1424 harr1424 deleted the tools/pm-21918/send-authentication-commands branch December 31, 2025 20:37
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.