Skip to content

Conversation

@andrebispo5
Copy link
Contributor

🎟️ Tracking

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

📔 Objective

This pull request updates the bulk cipher sharing functionality to ensure cipher IDs are included in the request payload, which is necessary for proper identification during bulk operations. The changes primarily affect the request model and its initialization, as well as related tests and the service layer.

Bulk Share Model and Request Model Updates:

  • The BulkShareCiphersRequestModel now uses CipherRequestModel instead of CipherCreateRequestModel for its ciphers property, allowing for inclusion of cipher IDs in bulk share requests.
  • The initializer for BulkShareCiphersRequestModel passes includeId: true to CipherRequestModel, ensuring each cipher's ID is included in the request.
  • The CipherRequestModel struct adds an optional id field, with documentation noting its use for bulk share operations.
  • The CipherRequestModel initializer gains a new includeId parameter, defaulting to false, and sets the id field accordingly. [1] [2]

Testing and Service Layer Adjustments:

  • The bulk share ciphers request tests are updated to expect the cipher IDs in the JSON payload, verifying the new request format.
  • The cipher service code is refactored to move the dictionary creation for cipher ID lookup after the API call, ensuring consistency with the updated request flow.

⏰ 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

@andrebispo5 andrebispo5 requested review from a team and matt-livefront as code owners January 20, 2026 12:00
Copilot AI review requested due to automatic review settings January 20, 2026 12:00
@github-actions github-actions bot added app:password-manager Bitwarden Password Manager app context t:misc Change Type - ¯\_(ツ)_/¯ labels Jan 20, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the bulk cipher sharing functionality by ensuring cipher IDs are included in the request payload. The changes modify the request model structure to flatten the bulk share request format, making it compatible with the server's expected format for vault migration operations.

Changes:

  • Updated BulkShareCiphersRequestModel to use CipherRequestModel directly instead of CipherCreateRequestModel, flattening the request structure
  • Added optional id field to CipherRequestModel with a new includeId parameter for controlled inclusion
  • Refactored CipherService to optimize dictionary creation timing

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
BitwardenShared/Core/Vault/Models/Request/CipherRequestModel.swift Adds optional id field and includeId parameter to support bulk share operations
BitwardenShared/Core/Vault/Models/Request/BulkShareCiphersRequestModel.swift Changes from CipherCreateRequestModel to CipherRequestModel with includeId: true to flatten request structure
BitwardenShared/Core/Vault/Services/API/Cipher/Requests/BulkShareCiphersRequestTests.swift Updates test expectations to match new flattened request structure with cipher IDs
BitwardenShared/Core/Vault/Services/CipherService.swift Moves dictionary creation after API call for logical flow optimization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Details44070894-4a73-464a-aa45-d2fc334f3dc3

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Jan 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.98%. Comparing base (73467b6) to head (c325ef6).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2265      +/-   ##
==========================================
+ Coverage   85.88%   85.98%   +0.10%     
==========================================
  Files        1760     1768       +8     
  Lines      150938   152280    +1342     
==========================================
+ Hits       129628   130942    +1314     
- Misses      21310    21338      +28     

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

Comment on lines +232 to +235
// Create a dictionary for quick lookup of original ciphers by ID.
let ciphersById = Dictionary(uniqueKeysWithValues: ciphers.compactMap { cipher in
cipher.id.map { ($0, cipher) }
})
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Why is it necessary to move this after the server call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved it because if the server call fails we won't be making unnecessary computations.

favorite: cipher.favorite,
fields: cipher.fields?.map(CipherFieldModel.init),
folderId: cipher.folderId,
id: includeId ? cipher.id : nil,
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Which requests need the id to be nil when the cipher.id is not nil? I'm asking to see if we can just always set it as the cipher.id instead of checking the boolean.

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

Labels

app:password-manager Bitwarden Password Manager app context t:misc Change Type - ¯\_(ツ)_/¯

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants