Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable reset all and skip options #7721

Merged
merged 4 commits into from
Dec 7, 2022
Merged

Enable reset all and skip options #7721

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
329a997
Dismiss bottomsheet on skipping verification
amitkma Dec 5, 2022
976cc48
Enable reset all and skip options
amitkma Dec 6, 2022
2f30718
Change ResetAll bottomsheet event to no-op for user verification
amitkma Dec 6, 2022
d2966fc
Fix strings and improve state step logic in SharedSecureStorageViewModel
amitkma Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions library/ui-strings/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@
<string name="action_disconnect">Disconnect</string>
<string name="action_play">Play</string>
<string name="action_dismiss">Dismiss</string>
<string name="action_reset">Reset</string>
<string name="action_reset">Proceed to Reset</string>
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather create a new string, this one might be used somewhere else?

Copy link
Contributor Author

@amitkma amitkma Dec 7, 2022 •
edited

Choose a reason for hiding this comment

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

I already checked that it is not used anywhere other than reset fragment. I will add a new string as per contribution guidelines.

<string name="action_learn_more">Learn more</string>
<string name="action_next">Next</string>
<string name="action_got_it">Got it</string>
Expand Down Expand Up @@ -2634,9 +2634,10 @@
<string name="failed_to_access_secure_storage">Failed to access secure storage</string>
<string name="bad_passphrase_key_reset_all_action">Forgot or lost all recovery options? Reset everything</string>
<string name="secure_backup_reset_all">Reset everything</string>
<string name="secure_backup_reset_all_no_other_devices">Only do this if you have no other device you can verify this device with.</string>
<string name="secure_backup_reset_all_no_other_devices">Resetting your verification keys cannot be undone. After resetting, you won't have access to old encrypted messages, and any friends who have previously verified you will see security warnings until you re-verify with them.</string>
Copy link
Member

Choose a reason for hiding this comment

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

create a new string here? and mark this one as eventually? Check the Contributing.md for strings.

I think we can keep a short version and a longer one. Use the short in the bottomsheet and the long in the reset fragment?

Copy link
Member

Choose a reason for hiding this comment

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

Also won'tneeds an escape

<string name="secure_backup_reset_if_you_reset_all">If you reset everything</string>
<string name="secure_backup_reset_no_history">You will restart with no history, no messages, trusted devices or trusted users</string>
<string name="secure_backup_reset_danger_warning">Please only proceed if you\'re sure you\'ve lost all of your other devices and your security key.</string>
<plurals name="secure_backup_reset_devices_you_can_verify">
<item quantity="one">Show the device you can verify with now</item>
<item quantity="other">Show %d devices you can verify with now</item>
Expand Down
13 changes: 9 additions & 4 deletions vector/src/main/java/im/vector/app/features/crypto/quads/SharedSecureStorageActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class SharedSecureStorageActivity :
val requestedSecrets: List<String> = emptyList(),
val resultKeyStoreAlias: String,
val writeSecrets: List<Pair<String, String>> = emptyList(),
val currentStep: SharedSecureStorageViewState.Step = SharedSecureStorageViewState.Step.EnterPassphrase,
) : Parcelable

private val viewModel: SharedSecureStorageViewModel by viewModel()
Expand Down Expand Up @@ -150,7 +151,8 @@ class SharedSecureStorageActivity :
context: Context,
keyId: String? = null,
requestedSecrets: List<String>,
resultKeyStoreAlias: String = DEFAULT_RESULT_KEYSTORE_ALIAS
resultKeyStoreAlias: String = DEFAULT_RESULT_KEYSTORE_ALIAS,
initialStep: SharedSecureStorageViewState.Step = SharedSecureStorageViewState.Step.EnterPassphrase
): Intent {
require(requestedSecrets.isNotEmpty())
return Intent(context, SharedSecureStorageActivity::class.java).also {
Expand All @@ -159,7 +161,8 @@ class SharedSecureStorageActivity :
Args(
keyId = keyId,
requestedSecrets = requestedSecrets,
resultKeyStoreAlias = resultKeyStoreAlias
resultKeyStoreAlias = resultKeyStoreAlias,
currentStep = initialStep
)
)
}
Expand All @@ -169,7 +172,8 @@ class SharedSecureStorageActivity :
context: Context,
keyId: String? = null,
writeSecrets: List<Pair<String, String>>,
resultKeyStoreAlias: String = DEFAULT_RESULT_KEYSTORE_ALIAS
resultKeyStoreAlias: String = DEFAULT_RESULT_KEYSTORE_ALIAS,
initialStep: SharedSecureStorageViewState.Step = SharedSecureStorageViewState.Step.EnterPassphrase
): Intent {
require(writeSecrets.isNotEmpty())
return Intent(context, SharedSecureStorageActivity::class.java).also {
Expand All @@ -178,7 +182,8 @@ class SharedSecureStorageActivity :
Args(
keyId = keyId,
writeSecrets = writeSecrets,
resultKeyStoreAlias = resultKeyStoreAlias
resultKeyStoreAlias = resultKeyStoreAlias,
currentStep = initialStep,
)
)
}
Expand Down
13 changes: 9 additions & 4 deletions vector/src/main/java/im/vector/app/features/crypto/quads/SharedSecureStorageViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ data class SharedSecureStorageViewState(
val ready: Boolean = false,
val hasPassphrase: Boolean = true,
val checkingSSSSAction: Async<Unit> = Uninitialized,
val step: Step = Step.EnterPassphrase,
val step: Step = Step.ResetAll,
val activeDeviceCount: Int = 0,
val showResetAllAction: Boolean = false,
val userId: String = "",
Expand All @@ -74,7 +74,8 @@ data class SharedSecureStorageViewState(
} else {
RequestType.ReadSecrets(args.requestedSecrets)
},
resultKeyStoreAlias = args.resultKeyStoreAlias
resultKeyStoreAlias = args.resultKeyStoreAlias,
step = args.currentStep,
)

enum class Step {
Expand Down Expand Up @@ -125,15 +126,17 @@ class SharedSecureStorageViewModel @AssistedInject constructor(
copy(
hasPassphrase = true,
ready = true,
step = SharedSecureStorageViewState.Step.EnterPassphrase
step = if (initialState.step == SharedSecureStorageViewState.Step.ResetAll) SharedSecureStorageViewState.Step.ResetAll
else SharedSecureStorageViewState.Step.EnterPassphrase
Copy link
Member

Choose a reason for hiding this comment

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

I find this a bit strange.
Wouldn't it be better to test if reset mode before trying to get the keyResult?
If state is not reset try to get keyResult and set the hasPassphrase/ready?

)
}
} else {
setState {
copy(
hasPassphrase = false,
ready = true,
step = SharedSecureStorageViewState.Step.EnterKey
step = if (initialState.step == SharedSecureStorageViewState.Step.ResetAll) SharedSecureStorageViewState.Step.ResetAll
else SharedSecureStorageViewState.Step.EnterKey
)
}
}
Expand Down Expand Up @@ -203,6 +206,7 @@ class SharedSecureStorageViewModel @AssistedInject constructor(
_viewEvents.post(SharedSecureStorageViewEvent.Dismiss)
}
}
/*
SharedSecureStorageViewState.Step.ResetAll -> {
setState {
copy(
Expand All @@ -211,6 +215,7 @@ class SharedSecureStorageViewModel @AssistedInject constructor(
)
}
}
*/
amitkma marked this conversation as resolved.
Show resolved Hide resolved
else -> {
_viewEvents.post(SharedSecureStorageViewEvent.Dismiss)
}
Expand Down
1 change: 1 addition & 0 deletions vector/src/main/java/im/vector/app/features/crypto/verification/VerificationAction.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ sealed class VerificationAction : VectorViewModelAction {
data class GotItConclusion(val verified: Boolean) : VerificationAction()
object FailedToGetKeysFrom4S : VerificationAction()
object SkipVerification : VerificationAction()
object ForgotResetAll: VerificationAction()
amitkma marked this conversation as resolved.
Show resolved Hide resolved
object VerifyFromPassphrase : VerificationAction()
object ReadyPendingVerification : VerificationAction()
object CancelPendingVerification : VerificationAction()
Expand Down
1 change: 1 addition & 0 deletions ...main/java/im/vector/app/features/crypto/verification/VerificationBottomSheetViewEvents.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import im.vector.app.core.platform.VectorViewEvents
sealed class VerificationBottomSheetViewEvents : VectorViewEvents {
object Dismiss : VerificationBottomSheetViewEvents()
object AccessSecretStore : VerificationBottomSheetViewEvents()
object ResetAll: VerificationBottomSheetViewEvents()
amitkma marked this conversation as resolved.
Show resolved Hide resolved
object GoToSettings : VerificationBottomSheetViewEvents()
data class ModalError(val errorMessage: CharSequence) : VerificationBottomSheetViewEvents()
}
14 changes: 13 additions & 1 deletion .../main/java/im/vector/app/features/crypto/verification/self/SelfVerificationBottomSheet.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import im.vector.app.core.extensions.toMvRxBundle
import im.vector.app.core.platform.VectorBaseBottomSheetDialogFragment
import im.vector.app.databinding.BottomSheetVerificationBinding
import im.vector.app.features.crypto.quads.SharedSecureStorageActivity
import im.vector.app.features.crypto.quads.SharedSecureStorageViewState
import im.vector.app.features.crypto.verification.VerificationAction
import im.vector.app.features.crypto.verification.VerificationBottomSheetViewEvents
import kotlinx.parcelize.Parcelize
Expand Down Expand Up @@ -92,7 +93,18 @@ class SelfVerificationBottomSheet : VectorBaseBottomSheetDialogFragment<BottomSh
requireContext(),
null, // use default key
listOf(MASTER_KEY_SSSS_NAME, USER_SIGNING_KEY_SSSS_NAME, SELF_SIGNING_KEY_SSSS_NAME, KEYBACKUP_SECRET_SSSS_NAME),
SharedSecureStorageActivity.DEFAULT_RESULT_KEYSTORE_ALIAS
SharedSecureStorageActivity.DEFAULT_RESULT_KEYSTORE_ALIAS,
)
)
}
VerificationBottomSheetViewEvents.ResetAll -> {
secretStartForActivityResult.launch(
SharedSecureStorageActivity.newReadIntent(
requireContext(),
null, // use default key
listOf(MASTER_KEY_SSSS_NAME, USER_SIGNING_KEY_SSSS_NAME, SELF_SIGNING_KEY_SSSS_NAME, KEYBACKUP_SECRET_SSSS_NAME),
SharedSecureStorageActivity.DEFAULT_RESULT_KEYSTORE_ALIAS,
SharedSecureStorageViewState.Step.ResetAll
)
)
}
Expand Down
4 changes: 2 additions & 2 deletions ...c/main/java/im/vector/app/features/crypto/verification/self/SelfVerificationController.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,8 @@ class SelfVerificationController @Inject constructor(
}
}
is Success -> {
val invoke = action.invoke()
if (invoke) {
val value = action.invoke()
if (value) {
verifiedSuccessTile()
bottomDone { (host.listener as? InteractionListener)?.onDoneFrom4S() }
} else {
Expand Down
4 changes: 2 additions & 2 deletions ...src/main/java/im/vector/app/features/crypto/verification/self/SelfVerificationFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,11 @@ class SelfVerificationFragment : VectorBaseFragment<BottomSheetVerificationChil
}

override fun onClickSkip() {
TODO("Not yet implemented")
viewModel.handle(VerificationAction.SkipVerification)
}

override fun onClickResetSecurity() {
TODO("Not yet implemented")
viewModel.handle(VerificationAction.ForgotResetAll)
}

override fun onDoneFrom4S() {
Expand Down
14 changes: 12 additions & 2 deletions ...rc/main/java/im/vector/app/features/crypto/verification/self/SelfVerificationViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package im.vector.app.features.crypto.verification.self

import android.util.Log
amitkma marked this conversation as resolved.
Show resolved Hide resolved
import com.airbnb.mvrx.Async
import com.airbnb.mvrx.Fail
import com.airbnb.mvrx.Loading
Expand Down Expand Up @@ -286,8 +287,17 @@ class SelfVerificationViewModel @AssistedInject constructor(
}
}
}
VerificationAction.SecuredStorageHasBeenReset -> TODO()
VerificationAction.SkipVerification -> TODO()
VerificationAction.SecuredStorageHasBeenReset -> {
if (session.cryptoService().crossSigningService().allPrivateKeysKnown()) {
_viewEvents.post(VerificationBottomSheetViewEvents.Dismiss)
}
}
VerificationAction.SkipVerification -> {
_viewEvents.post(VerificationBottomSheetViewEvents.Dismiss)
}
VerificationAction.ForgotResetAll -> {
_viewEvents.post(VerificationBottomSheetViewEvents.ResetAll)
}
VerificationAction.StartSASVerification -> {
withState { state ->
val request = state.pendingRequest.invoke() ?: return@withState
Expand Down
3 changes: 3 additions & 0 deletions .../main/java/im/vector/app/features/crypto/verification/user/UserVerificationBottomSheet.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ class UserVerificationBottomSheet : VectorBaseBottomSheetDialogFragment<BottomSh
.setPositiveButton(R.string.ok, null)
.show()
}
VerificationBottomSheetViewEvents.ResetAll -> {
// no-op for user verification
}
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions ...rc/main/java/im/vector/app/features/crypto/verification/user/UserVerificationViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,9 @@ class UserVerificationViewModel @AssistedInject constructor(
// Not applicable for user verification
}
VerificationAction.RequestSelfVerification -> TODO()
VerificationAction.ForgotResetAll -> {
// Not applicable for user verification
}
}
}

Expand Down
20 changes: 4 additions & 16 deletions vector/src/main/res/layout/fragment_ssss_reset_all.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,19 +60,6 @@
tools:text="Show 2 devices you can verify with now"
tools:visibility="visible" />

<TextView
android:id="@+id/ssss_reset_text3"
style="@style/Widget.Vector.TextView.Subtitle"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_marginStart="16dp"
android:layout_marginTop="16dp"
android:text="@string/secure_backup_reset_if_you_reset_all"
android:textColor="?colorError"
android:textStyle="bold"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@id/ssss_reset_other_devices" />

<TextView
android:id="@+id/ssss_reset_text4"
style="@style/Widget.Vector.TextView.Body"
Expand All @@ -82,9 +69,10 @@
android:layout_marginTop="16dp"
android:layout_marginEnd="16dp"
android:layout_marginBottom="16dp"
android:text="@string/secure_backup_reset_no_history"
android:text="@string/secure_backup_reset_danger_warning"
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's for consistency with web?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

android:textColor="?vctr_content_primary"
app:layout_constraintTop_toBottomOf="@id/ssss_reset_text3" />
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@id/ssss_reset_other_devices" />

<Button
android:id="@+id/ssss_reset_button_cancel"
Expand Down Expand Up @@ -118,4 +106,4 @@

</androidx.constraintlayout.widget.ConstraintLayout>

</ScrollView>
</ScrollView>