feat(debug): migrate one conversation to MLS - WPB-23052 - #5049
feat(debug): migrate one conversation to MLS - WPB-23052#5049MohamadJaara wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a developer-mode-only action in the conversation details UI to migrate a single eligible team group conversation from Proteus/mixed to MLS, backed by a focused data-model use case and tests. This supports targeted MLS migration testing without triggering the broader rollout coordinator.
Changes:
- Add a new conversation action (“Migrate to MLS”) gated behind developer mode + eligibility checks, with confirmation and success/failure feedback.
- Introduce
MigrateConversationToMLSUseCaseto handle Proteus → mixed → MLS and mixed → MLS, including MLS group establish/join steps. - Add UI locator + data-model unit tests covering Proteus/mixed/MLS/missing/unsupported scenarios.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| WireUI/Sources/WireLocators/Locators.swift | Adds a new locator for the MLS migration action. |
| wire-ios/Wire-iOS/Sources/UserInterface/GroupDetails/ConversationActions/ConversationActionController+MLSMigration.swift | Implements the UI confirmation flow and triggers the migration use case. |
| wire-ios/Wire-iOS/Sources/UserInterface/GroupDetails/ConversationActions/ConversationActionController.swift | Wires the new action into the controller action dispatcher. |
| wire-ios/Wire-iOS/Sources/UserInterface/GroupDetails/ConversationActions/ConversationAction.swift | Adds the new action, gating logic, title, and accessibility identifier mapping. |
| wire-ios-data-model/Tests/MLS/MigrateConversationToMLSUseCaseTests.swift | Adds unit tests for the focused migration use case. |
| wire-ios-data-model/Source/MLS/Migration/MigrateConversationToMLSUseCase.swift | Adds the focused migration use case implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case .migrateToMLS: | ||
| return "Migrate to MLS" |
| let controller = UIAlertController( | ||
| title: "Migrate conversation to MLS?", | ||
| message: "This internal action bypasses the normal MLS rollout conditions and immediately migrates this conversation.", | ||
| preferredStyle: .alert | ||
| ) | ||
| controller.addAction(.cancel()) | ||
| controller.addAction( | ||
| UIAlertAction( | ||
| title: "Migrate", | ||
| style: .destructive, | ||
| accessibilityIdentifier: Locators.ConversationDetailsActions.migrateToMLS.rawValue |
Test Results4 514 tests 4 486 ✅ 7m 58s ⏱️ Results for commit e586a96. ♻️ This comment has been updated with latest results. Summary: workflow run #31750536714 |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
johnxnguyen
left a comment
There was a problem hiding this comment.
Looks good, thanks for the contribution! I left a couple comments.
| "meta.menu.migrate_to_mls" = "Migrate to MLS"; | ||
| "meta.menu.mls_migration.confirmation.title" = "Migrate conversation to MLS?"; | ||
| "meta.menu.mls_migration.confirmation.message" = "This internal action bypasses the normal MLS rollout conditions and immediately migrates this conversation."; | ||
| "meta.menu.mls_migration.confirmation.button" = "Migrate"; | ||
| "meta.menu.mls_migration.success.title" = "MLS migration completed"; | ||
| "meta.menu.mls_migration.success.message" = "The conversation now uses MLS."; | ||
| "meta.menu.mls_migration.failure.title" = "MLS migration failed"; | ||
| "meta.menu.mls_migration.failure.conversation_not_found" = "The conversation could not be found."; | ||
| "meta.menu.mls_migration.failure.unsupported_conversation" = "Only team group conversations can be migrated."; | ||
| "meta.menu.mls_migration.failure.missing_mls_service" = "The MLS service is unavailable."; | ||
| "meta.menu.mls_migration.failure.missing_mls_group_id" = "The conversation does not have an MLS group ID."; |
There was a problem hiding this comment.
question: do we need to localize these strings if this is only an internal menu? If not then I would not include them here, otherwise they will be pushed to Crowdin and require translation.
| case markUnread | ||
| case remove | ||
| case favorite(isFavorite: Bool) | ||
| case migrateToMLS |
There was a problem hiding this comment.
suggestion: rather than adding the option here, it may be nicer to place this developer tool with the other developer tools. If you open a group conversation and then shake the device, the developer tools with bee presented with a section at the top specific to the current conversation. Check out ConversationDeveloperActionsProvider, where you can see where to add such a button to invoke a use case. Since it's SwiftUI, the alerts will also be easier to display.
There was a problem hiding this comment.
there is a new idea to have this show not only internal but for normal users aswell behind some action like click the protocol info 5 times i will adjust the PR once design for it is done
samwyndham
left a comment
There was a problem hiding this comment.
Nice work. I added a few suggestions and will wait for the design to be finalized (regarding how the user starts the migration) before approving.
| present(controller) | ||
| } | ||
|
|
||
| private func migrateConversationToMLS(_ conversation: ZMConversation) { |
There was a problem hiding this comment.
suggestion: Make async and move Task to the caller.
| } | ||
| } | ||
|
|
||
| private func presentMLSMigrationSuccess() { |
There was a problem hiding this comment.
suggestion: Mark as @mainactor so the caller doesn't need to do MainActor.run
| present(controller) | ||
| } | ||
|
|
||
| private func presentMLSMigrationFailure(_ error: Error) { |
There was a problem hiding this comment.
suggestion: Mark as @mainactor so the caller doesn't need to do MainActor.run
|
|
||
| var listActions: [Action] { | ||
| actions.filter { $0 != .delete } | ||
| addingMLSMigrationActionIfNeeded(to: actions.filter { $0 != .delete }) |
There was a problem hiding this comment.
suggestion: Remove changes to listActions & deleteActions and instead add it to availableGroupActions() which these will use internally.
| private func createConversation( | ||
| messageProtocol: MessageProtocol, | ||
| conversationType: ZMConversationType = .group, | ||
| groupID: MLSGroupID = .random(), |
There was a problem hiding this comment.
Semgrep identified an issue, but thinks it may be safe to ignore.
The App uses an insecure Random Number Generator.
Why this might be safe to ignore:
This match is in a unit test helper, not production code, and the random value is only used to populate a test conversation group ID. There is no security-sensitive use like key generation, token creation, or session handling here, so fixing it would not meaningfully improve application security.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by ios_insecure_random_no_generator.
You can view more details about this finding in the Semgrep AppSec Platform.
| } | ||
|
|
||
| func testInvoke_ProteusConversation_MigratesAndFinalisesToMLS() async throws { | ||
| let groupID = MLSGroupID.random() |
There was a problem hiding this comment.
Semgrep identified an issue, but thinks it may be safe to ignore.
The App uses an insecure Random Number Generator.
Why this might be safe to ignore:
This match is in a unit test file, where a random group ID is being generated as test data rather than for a security-sensitive production use. The rule appears to have matched a custom
.random()helper by name, not evidence that an insecure RNG API is actually being used here.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by ios_insecure_random_no_generator.
You can view more details about this finding in the Semgrep AppSec Platform.
| } | ||
|
|
||
| func testInvoke_MixedConversation_JoinsMissingGroupBeforeFinalising() async throws { | ||
| let groupID = MLSGroupID.random() |
There was a problem hiding this comment.
Semgrep identified an issue, but thinks it may be safe to ignore.
The App uses an insecure Random Number Generator.
Why this might be safe to ignore:
This match is in unit test code (
testInvoke_...) whereMLSGroupID.random()is only creating test data, not generating security-sensitive randomness in production. The rule appears to have matched a generic.random()call, but in this context it does not meaningfully affect application security.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by ios_insecure_random_no_generator.
You can view more details about this finding in the Semgrep AppSec Platform.
| } | ||
|
|
||
| func testInvoke_MixedConversation_FinalisesToMLS() async throws { | ||
| let groupID = MLSGroupID.random() |
There was a problem hiding this comment.
Semgrep identified an issue, but thinks it may be safe to ignore.
The App uses an insecure Random Number Generator.
Why this might be safe to ignore:
This match is in a unit test file, where the random value is only used to create test group IDs and not for production security behavior. The rule likely overmatched a custom
.random()helper name without showing that it uses an insecure RNG or that any attacker-controlled input or security-sensitive randomness is involved.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by ios_insecure_random_no_generator.
You can view more details about this finding in the Semgrep AppSec Platform.
What changed
Why
Developers need a targeted way to exercise MLS migration for one conversation without triggering the normal bulk rollout flow.
Impact
The option is only visible when developer mode is enabled, and only for non-MLS group conversations in the self user's team. Production users do not see the action.
Validation
The focused migration use-case tests pass, and the full Wire-iOS app target builds successfully from a clean derived-data directory.