[PM-35791] Remove legacy Send access endpoints

[mcamirault]

🎟️ Tracking

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

📔 Objective

This PR removes two now-unused endpoints that were used for Send access and all associated tests. It also adds one test using SendTextModel since the V1 tests were the only ones that used it. This PR should be merged before this server PR goes but shouldn't have to wait for this clients PR since that's pinned to a specific SDK version.

🚨 Breaking Changes

We're removing two functions that are called in clients, but those callers are themselves uncalled and being removed in this PR

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

Reviewed the removal of the legacy V1 Send access endpoints (access_send_v1, get_file_download_data_v1) from crates/bitwarden-send/src/access.rs along with their tests, and the replacement text-send test for the V2 access_using_auth path. Verified no remaining references to the removed functions anywhere in the workspace, and that imports (chrono::{DateTime, Utc}) are still required by SendAccessResponse. One test assertion in the newly added test contradicts its own mock data and will fail.

Code Review Details

• ❌ : test_access_send_text mocks a Text send but asserts SendType::File, so the assertion panics
  • crates/bitwarden-send/src/access.rs:237

[github-actions]

🔍 SDK Breaking Change Detection

SDK Version: tools/pm-35791/email-verification-cleanup (91bccb3)

⚠️ If breaking changes are detected, a corresponding pull request addressing them must be ready for merge in the affected client repository.

Client	Status	Details
typescript	⚠️ Workflow execution issues	Check workflow logs for details
android	✅ No breaking changes detected	Compilation passed with new SDK version - View Details

---

Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm.
Results update as workflows complete.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.81%. Comparing base (2926ef0) to head (912b14d).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1300      +/-   ##
==========================================
+ Coverage   85.79%   85.81%   +0.01%     
==========================================
  Files         487      490       +3     
  Lines       69999    70481     +482     
==========================================
+ Hits        60059    60482     +423     
- Misses       9940     9999      +59     

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

[mcamirault]

@claude Please re-review

[claude]

Claude finished @mcamirault's task in 1m 25s —— View job

---

Re-review — PR #1300

Overall Assessment: ✅ APPROVE (with two minor, non-blocking suggestions)

• Gather context (diff vs base, full file, repo-wide reference search)
• Verify the previously-flagged CRITICAL is fixed
• Check for orphaned imports / consts / dead references
• Assess test coverage of the retained conversion paths

---

Previous finding: resolved ✅

test_access_send_text (crates/bitwarden-send/src/access.rs:210) now mocks SendType::Text and asserts crate::SendType::Text (line 237). The text payload assertions were also restored (lines 240-242), so the SendTextModel → SendAccessTextResponse mapping is genuinely exercised — which was the stated reason for adding the test. No remaining type mismatch.

Verification performed

• Repo-wide search (--exclude-dir=target) finds zero remaining references to access_send_v1 / get_file_download_data_v1 or their camelCase binding names. The only surviving hits for get_send_file_download_data are in the generated bitwarden-api-api crate (crates/bitwarden-api-api/src/apis/sends_api.rs:55,232) — correct, since that's generated from the server spec and shouldn't be hand-edited.
• No orphaned imports: chrono::{DateTime, Utc} (line 4) is still needed by SendAccessResponse::expiration_date (line 35); models is used by both TryFrom/From impls; in the test module SendTextModel, SendFileModel, SendFileDownloadDataResponseModel, and all three consts (SEND_ID, FILE_ID, ACCESS_TOKEN) remain in use.
• The SendType shadowing in mod tests is correct — the explicit bitwarden_api_api::models::SendType import shadows the use super::* glob, and the asserts disambiguate with crate::SendType.
• ⚠️ I was not able to run cargo test -p bitwarden-send --all-features — the command needs approval in this environment. The review below is static analysis only; CI/codecov results are consistent with it.

---

💭 SUGGESTED — coverage regression on expiration_date / creator_identifier

crates/bitwarden-send/src/access.rs:227

The deleted test_access_send_v1_text was the only test that supplied non-None values for these two fields. All three surviving access_send tests now pass expiration_date: None, creator_identifier: None, so these lines in TryFrom<models::SendAccessResponseModel> are no longer exercised:

expiration_date: r.expiration_date.map(|s| s.parse()).transpose()?,   // line 149
creator_identifier: r.creator_identifier,                              // line 150

That means the date-parsing path — and with it the only route to AccessSendError::Parse / SendParseError — is now untested. This matches the codecov report on this PR (Misses +1). Since you're already replacing the V1 text test, populating these two fields costs nothing:

expiration_date: Some("2025-01-10T00:00:00Z".to_string()),
creator_identifier: Some("user@example.com".to_string()),

assert_eq!(
    result.expiration_date,
    Some("2025-01-10T00:00:00Z".parse::<DateTime<Utc>>().unwrap())
);
assert_eq!(result.creator_identifier, Some("user@example.com".to_string()));

Same reasoning applies, more weakly, to hidden: t.hidden.unwrap_or(false) (line 141) — the Some(false) mock and a None mock are indistinguishable in the assertion, so the unwrap_or default is untested. Setting hidden: Some(true) would make the mapping meaningful. Fix this →

💭 SUGGESTED — awkward doc comment wrap

crates/bitwarden-send/src/access.rs:179

/// Gets file download data for a file send, authenticated
/// with a send access token.

Dropping "V2 API endpoint" left the line short; rustfmt won't reflow doc comments. Worth joining into one line for consistency with the access_send doc directly above it. Purely cosmetic.

---

Notes on the breaking change

The breaking-change label and the TypeScript detection failure are expected and documented in the PR description (callers removed in bitwarden/clients#22080; that PR must be ready to merge). The Android check passes, consistent with these methods never having been exposed through UniFFI. Merge ordering relative to the server PR is as you described — the SDK no longer calls the V1 endpoints, so removing them server-side afterwards is safe.

Nothing blocking. Both suggestions are optional; the coverage one is worth taking since the test is already being rewritten in this PR.
· Branch: tools/pm-35791/email-verification-cleanup