[PM-24618] Integrate CLI sends with server branch #18106
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #18106 +/- ##
==========================================
+ Coverage 42.24% 42.41% +0.17%
==========================================
Files 3598 3599 +1
Lines 104480 104540 +60
Branches 15765 15786 +21
==========================================
+ Hits 44133 44344 +211
+ Misses 58467 58289 -178
- Partials 1880 1907 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
harr1424
changed the title
harr1424
changed the title
|
Claude finished @harr1424's task in 4m 48s —— View job Overall Assessment: APPROVE This PR successfully integrates email-based OTP authentication for CLI sends, adding the AuthType enum following ADR-0025 patterns, mutual exclusivity validation between emails and password, and comprehensive test coverage (765 lines of new tests). The implementation correctly handles authType inference and data flow through all model layers. Code Review Details
Additional ObservationsPositive aspects:
Future improvements (not blocking):
Code coverage gaps (per Codecov):
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| req.maxAccessCount = maxAccessCount; | ||
| req.emails = emails; | ||
|
|
||
| if (emails != null && emails.length > 0) { |
There was a problem hiding this comment.
Details and fix
Line 82 extracts emails with ?? undefined, but line 93 checks emails.length > 0 without first verifying emails is not undefined. This will throw TypeError: Cannot read property 'length' of undefined if emails is undefined.
Fix:
if (emails != null && emails.length > 0) {
req.authType = AuthType.Email;
} else if (password != null && password.trim().length > 0) {
req.authType = AuthType.Password;
} else {
req.authType = AuthType.None;
}Why: JavaScript/TypeScript will throw when accessing .length on undefined. The nullish coalescing on line 82 explicitly returns undefined, making this a runtime error waiting to happen.
There was a problem hiding this comment.
Loose equality operator != will handle undefined as intended here, which is likely why the suggested "fix" is identical to the existing code.
| this.deletionDate = o.deletionDate; | ||
| this.expirationDate = o.expirationDate; | ||
| this.passwordSet = o.password != null; | ||
| this.emails = o.emails ?? []; |
There was a problem hiding this comment.
❌ CRITICAL: Missing authType assignment in constructor causes data loss
Details and fix
The constructor (lines 98-132) creates a SendResponse from a SendView but doesn't copy the authType field. This is added at line 124, but the corresponding field in the toView static method (line 36-58) is also missing authType assignment.
Current state:
- Line 124:
this.authType = o.authType;✓ (constructor copies it) - Line 55:
view.emails = send.emails ?? []; - Line 58: Missing
view.authType = send.authType;
Fix needed in toView method around line 57:
view.emails = send.emails ?? [];
view.disabled = send.disabled;
view.hideEmail = send.hideEmail;
view.authType = send.authType; // Add this line
return view;Impact: The toView method is used in create.command.ts line 147. Without copying authType, sends lose their authentication type during the create flow.
|
|
||
| req.key = null; | ||
| req.maxAccessCount = maxAccessCount; | ||
| req.emails = emails; |
There was a problem hiding this comment.
authType calculated but never assigned to encrypted send before API call
Details and fix
Lines 93-99 calculate req.authType based on whether emails or password is provided, but this value is never transferred to encSend before the API save at line 154.
Looking at line 152: encSend.emails = emails && emails.join(","); - the emails are assigned, but authType is not.
Fix needed around line 152:
encSend.deletionDate = sendView.deletionDate;
encSend.expirationDate = sendView.expirationDate;
encSend.emails = emails && emails.join(",");
encSend.authType = req.authType; // Add thisImpact: The server receives sends without the authType field, potentially breaking email-based OTP authentication or causing it to default incorrectly.
| import { SendFile } from "./send-file"; | ||
| import { SendText } from "./send-text"; | ||
|
|
||
| export const AuthType = Object.freeze({ |
There was a problem hiding this comment.
❓ QUESTION: Should AuthType come from SDK instead of being defined here?
Context
@itsadrago previously asked whether this enum should be sourced from the sdk-internal repo's api-api crate rather than being defined in the clients repository.
Current pattern: This follows the same pattern as SendType (also defined in clients repo using Object.freeze), which aligns with ADR-0025 (no TypeScript enums).
Consideration: If the server defines this enum and it's available via SDK, importing it would ensure the values stay synchronized. However, I don't see evidence of this enum existing in the current SDK.
Question for team: Is this enum part of the API contract that should come from SDK, or is it correct to define it client-side like SendType?
| const hasEmails = req.emails != null && req.emails.length > 0; | ||
| const hasPassword = req.password != null && req.password !== ""; | ||
|
|
||
| if (hasEmails && hasPassword) { | ||
| return Response.badRequest("--password and --emails are mutually exclusive."); | ||
| } |
There was a problem hiding this comment.
🎨 SUGGESTED: Apply this same validation pattern to create.command for consistency
Details
This validation correctly checks for meaningful presence (length > 0 for emails, non-empty string for password) rather than just checking if values are defined.
However, create.command.ts line 85 uses a different pattern:
if (emails !== undefined && password !== undefined) {
return Response.badRequest("--password and --emails are mutually exclusive.");
}This creates inconsistent behavior:
- edit: Allows
{"emails": [], "password": "foo"}(correct per tests) - create: Rejects
{"emails": [], "password": "foo"}(incorrect per create.command.spec.ts line 287-306)
Recommendation: Update create.command line 85-86 to use the same validation pattern you've implemented here:
const hasEmails = emails != null && emails.length > 0;
const hasPassword = password != null && password.trim().length > 0;
if (hasEmails && hasPassword) {
return Response.badRequest("--password and --emails are mutually exclusive.");
}This ensures both commands treat empty arrays and whitespace-only passwords consistently as "no auth specified".
| None: 2, | ||
| } as const); | ||
|
|
||
| export type AuthType = (typeof AuthType)[keyof typeof AuthType]; |
There was a problem hiding this comment.
❓ QUESTION: Should AuthType enum be defined in SDK instead?
Context
The AuthType enum is defined here in the clients repository, but this feature requires coordination with the server (per PR description linking to bitwarden/server#5895).
Typically, shared enums/types used by both client and server are defined in the SDK to ensure consistency.
Questions:
- Is there a corresponding AuthType in the server codebase?
- Should this be moved to the SDK for cross-repository consistency?
- Are the enum values (0=Email, 1=Password, 2=None) aligned with server expectations?
This doesn't block the PR but should be clarified for future maintenance.
mcamirault
left a comment
mcamirault
left a comment
There was a problem hiding this comment.
Overall looks good, though I believe the Create command tests aren't checking what they should
| if (emails != null && emails.length > 0) { | ||
| req.authType = AuthType.Email; | ||
| } else if (password != null && password.trim().length > 0) { |
There was a problem hiding this comment.
⛏️ Use the variables from 85-86 so this block is the same as in edit.command.ts
| encSend.deletionDate = sendView.deletionDate; | ||
| encSend.expirationDate = sendView.expirationDate; | ||
| encSend.emails = emails && emails.join(","); | ||
| encSend.authType = req.authType; |
There was a problem hiding this comment.
❓ The sendService.encrypt method already gets the other three fields from the SendView model when it creates the Send model. Is there a reason we're setting fields again here? Can't we move line 156 into the service then delete 152-156?
| encSend.deletionDate = sendView.deletionDate; | ||
| encSend.expirationDate = sendView.expirationDate; | ||
| encSend.emails = req.emails && req.emails.join(","); | ||
| encSend.authType = req.authType; |
There was a problem hiding this comment.
Same comment as on the create command
| }), | ||
| null, | ||
| undefined, | ||
| ); |
There was a problem hiding this comment.
authType was set correctly. There should be another check here of what was passed to the API the same way the Edit command tests do (lines 96-98 of edit.command.spec.ts). Same for all the tests in this file
| ? s.emails | ||
| .split(",") | ||
| .map((e) => e.trim()) | ||
| .filter((e) => e) |
There was a problem hiding this comment.
This filter call doesn't seem necessary
There was a problem hiding this comment.
The server shouldn't send back any emails consisting of empty strings or strings consisting only of whitespace, but in this situation the filter can be used to discard such "falsy" values.
None of the other fields in this class are validated, so I agree it may make sense to remove it if adhering to existing conventions in this file is more important that programming defensively.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-24618
https://bitwarden.atlassian.net/browse/PM-23109
bitwarden/server#5895
📔 Objective
PM-24618: reveal the functionality related to email based OTP in the CLI's send help output
bw send --helpPM-23109: update models to add an
AuthTypeenum and ensure the process of creating and receiving a send supports storing emails associated with the send to be used for email based OTP authentication and theAuthTypeenum value💻 Command Output
⏰ Reminders before review
🦮 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