Merge pull request #363 from loki-project/shared-sender-keys

Fix SSK Group Leaving Race Condition
This commit is contained in:
Niels Andriesse 2020-10-08 15:05:21 +11:00 committed by GitHub
commit ba1033bfe5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 169 additions and 121 deletions

View File

@ -57,7 +57,7 @@
<Button
style="@style/Widget.Session.Button.Common.ProminentOutline"
android:id="@+id/btnCreateNewPrivateChat"
android:id="@+id/createNewPrivateChatButton"
android:layout_width="196dp"
android:layout_height="@dimen/medium_button_height"
android:layout_marginTop="@dimen/medium_spacing"

View File

@ -1,9 +1,9 @@
<?xml version="1.0" encoding="utf-8"?>
<RelativeLayout
xmlns:android="http://schemas.android.com/apk/res/android"
<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
android:layout_width="match_parent"
android:layout_height="match_parent"
xmlns:tools="http://schemas.android.com/tools"
xmlns:app="http://schemas.android.com/apk/res-auto"
tools:context="org.thoughtcrime.securesms.loki.activities.EditClosedGroupActivity">
<LinearLayout
@ -146,4 +146,22 @@
</LinearLayout>
<RelativeLayout
android:id="@+id/loader"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:background="#A4000000"
android:visibility="gone"
android:alpha="0">
<com.github.ybq.android.spinkit.SpinKitView
style="@style/SpinKitView.Large.ThreeBounce"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginTop="8dp"
android:layout_centerInParent="true"
app:SpinKit_Color="@android:color/white" />
</RelativeLayout>
</RelativeLayout>

View File

@ -123,7 +123,7 @@
<Button
style="@style/Widget.Session.Button.Common.ProminentOutline"
android:id="@+id/btnCreateNewPrivateChat"
android:id="@+id/createNewPrivateChatButton"
android:layout_width="196dp"
android:layout_height="@dimen/medium_button_height"
android:layout_marginTop="@dimen/medium_spacing"

View File

@ -32,11 +32,10 @@ import org.whispersystems.libsignal.util.guava.Optional
import java.lang.ref.WeakReference
class CreateClosedGroupActivity : PassphraseRequiredActionBarActivity(), LoaderManager.LoaderCallbacks<List<String>> {
private var isLoading = false
set(newValue) { field = newValue; invalidateOptionsMenu() }
private var members = listOf<String>()
set(value) {
field = value
selectContactsAdapter.members = value
}
set(value) { field = value; selectContactsAdapter.members = value }
private val selectContactsAdapter by lazy {
SelectContactsAdapter(this, GlideApp.with(this))
@ -49,21 +48,17 @@ class CreateClosedGroupActivity : PassphraseRequiredActionBarActivity(), LoaderM
// region Lifecycle
override fun onCreate(savedInstanceState: Bundle?, isReady: Boolean) {
super.onCreate(savedInstanceState, isReady)
setContentView(R.layout.activity_create_closed_group)
supportActionBar!!.title = resources.getString(R.string.activity_create_closed_group_title)
recyclerView.adapter = this.selectContactsAdapter
recyclerView.layoutManager = LinearLayoutManager(this)
btnCreateNewPrivateChat.setOnClickListener { createNewPrivateChat() }
createNewPrivateChatButton.setOnClickListener { createNewPrivateChat() }
LoaderManager.getInstance(this).initLoader(0, null, this)
}
override fun onCreateOptionsMenu(menu: Menu?): Boolean {
menuInflater.inflate(R.menu.menu_done, menu)
return members.isNotEmpty()
return members.isNotEmpty() && !isLoading
}
// endregion
@ -91,7 +86,7 @@ class CreateClosedGroupActivity : PassphraseRequiredActionBarActivity(), LoaderM
// region Interaction
override fun onOptionsItemSelected(item: MenuItem): Boolean {
when(item.itemId) {
R.id.doneButton -> createClosedGroup()
R.id.doneButton -> if (!isLoading) { createClosedGroup() }
}
return super.onOptionsItemSelected(item)
}
@ -125,9 +120,11 @@ class CreateClosedGroupActivity : PassphraseRequiredActionBarActivity(), LoaderM
return Toast.makeText(this, R.string.activity_create_closed_group_too_many_group_members_error, Toast.LENGTH_LONG).show()
}
val userPublicKey = TextSecurePreferences.getLocalNumber(this)
isLoading = true
loader.fadeIn()
ClosedGroupsProtocol.createClosedGroup(this, name.toString(), selectedMembers + setOf( userPublicKey )).successUi { groupID ->
loader.fadeOut()
isLoading = false
val threadID = DatabaseFactory.getThreadDatabase(this).getThreadIdFor(Recipient.from(this, Address.fromSerialized(groupID), false))
if (!isFinishing) {
openConversationActivity(this, threadID, Recipient.from(this, Address.fromSerialized(groupID), false))

View File

@ -13,17 +13,23 @@ import android.view.inputmethod.EditorInfo
import android.view.inputmethod.InputMethodManager
import android.widget.Toast
import androidx.appcompat.content.res.AppCompatResources
import kotlinx.android.synthetic.main.activity_create_closed_group.*
import kotlinx.android.synthetic.main.activity_create_closed_group.emptyStateContainer
import kotlinx.android.synthetic.main.activity_create_closed_group.mainContentContainer
import kotlinx.android.synthetic.main.activity_edit_closed_group.*
import kotlinx.android.synthetic.main.activity_edit_closed_group.loader
import kotlinx.android.synthetic.main.activity_linked_devices.recyclerView
import network.loki.messenger.R
import nl.komponents.kovenant.ui.failUi
import nl.komponents.kovenant.ui.successUi
import org.thoughtcrime.securesms.PassphraseRequiredActionBarActivity
import org.thoughtcrime.securesms.database.Address
import org.thoughtcrime.securesms.database.DatabaseFactory
import org.thoughtcrime.securesms.groups.GroupManager
import org.thoughtcrime.securesms.loki.dialogs.ClosedGroupEditingOptionsBottomSheet
import org.thoughtcrime.securesms.loki.protocol.ClosedGroupsProtocol
import org.thoughtcrime.securesms.loki.utilities.fadeIn
import org.thoughtcrime.securesms.loki.utilities.fadeOut
import org.thoughtcrime.securesms.mms.GlideApp
import org.thoughtcrime.securesms.recipients.Recipient
import org.thoughtcrime.securesms.util.GroupUtil
@ -36,6 +42,8 @@ class EditClosedGroupActivity : PassphraseRequiredActionBarActivity() {
private val originalMembers = HashSet<String>()
private val members = HashSet<String>()
private var hasNameChanged = false
private var isLoading = false
set(newValue) { field = newValue; invalidateOptionsMenu() }
private lateinit var groupID: String
private lateinit var originalName: String
@ -115,7 +123,7 @@ class EditClosedGroupActivity : PassphraseRequiredActionBarActivity() {
override fun onCreateOptionsMenu(menu: Menu): Boolean {
menuInflater.inflate(R.menu.menu_edit_closed_group, menu)
return members.isNotEmpty()
return members.isNotEmpty() && !isLoading
}
// endregion
@ -166,7 +174,7 @@ class EditClosedGroupActivity : PassphraseRequiredActionBarActivity() {
// region Interaction
override fun onOptionsItemSelected(item: MenuItem): Boolean {
when (item.itemId) {
R.id.action_apply -> commitChanges()
R.id.action_apply -> if (!isLoading) { commitChanges() }
}
return super.onOptionsItemSelected(item)
}
@ -238,10 +246,19 @@ class EditClosedGroupActivity : PassphraseRequiredActionBarActivity() {
}
if (isSSKBasedClosedGroup) {
ClosedGroupsProtocol.update(this, groupPublicKey!!, members.map { it.address.serialize() }, name)
isLoading = true
loader.fadeIn()
ClosedGroupsProtocol.update(this, groupPublicKey!!, members.map { it.address.serialize() }, name).successUi {
loader.fadeOut()
isLoading = false
finish()
}.failUi { exception ->
val message = if (exception is ClosedGroupsProtocol.Error) exception.description else "An error occurred"
Toast.makeText(this@EditClosedGroupActivity, message, Toast.LENGTH_LONG).show()
isLoading = false
}
} else {
GroupManager.updateGroup(this, groupID, members, null, name, admins)
}
finish()
}
}

View File

@ -125,7 +125,7 @@ class HomeActivity : PassphraseRequiredActionBarActivity, ConversationClickListe
recyclerView.adapter = homeAdapter
recyclerView.layoutManager = LinearLayoutManager(this)
// Set up empty state view
btnCreateNewPrivateChat.setOnClickListener { createNewPrivateChat() }
createNewPrivateChatButton.setOnClickListener { createNewPrivateChat() }
// This is a workaround for the fact that CursorRecyclerViewAdapter doesn't actually auto-update (even though it says it will)
LoaderManager.getInstance(this).restartLoader(0, null, object : LoaderManager.LoaderCallbacks<Cursor> {

View File

@ -25,6 +25,7 @@ import org.whispersystems.signalservice.api.messages.SignalServiceGroup
import org.whispersystems.signalservice.api.messages.SignalServiceGroup.GroupType
import org.whispersystems.signalservice.internal.push.SignalServiceProtos
import org.whispersystems.signalservice.internal.push.SignalServiceProtos.GroupContext
import org.whispersystems.signalservice.loki.api.SnodeAPI
import org.whispersystems.signalservice.loki.protocol.closedgroups.ClosedGroupRatchet
import org.whispersystems.signalservice.loki.protocol.closedgroups.ClosedGroupSenderKey
import org.whispersystems.signalservice.loki.protocol.closedgroups.SharedSenderKeysImplementation
@ -39,6 +40,12 @@ object ClosedGroupsProtocol {
val isSharedSenderKeysEnabled = true
val groupSizeLimit = 20
sealed class Error(val description: String) : Exception() {
object NoThread : Error("Couldn't find a thread associated with the given group public key")
object NoPrivateKey : Error("Couldn't find a private key associated with the given group public key.")
object InvalidUpdate : Error("Invalid group update.")
}
public fun createClosedGroup(context: Context, name: String, members: Collection<String>): Promise<String, Exception> {
val deferred = deferred<String, Exception>()
Thread {
@ -98,10 +105,12 @@ object ClosedGroupsProtocol {
val name = group.title
val oldMembers = group.members.map { it.serialize() }.toSet()
val newMembers = oldMembers.minus(userPublicKey)
update(context, groupPublicKey, newMembers, name)
return update(context, groupPublicKey, newMembers, name).get()
}
public fun update(context: Context, groupPublicKey: String, members: Collection<String>, name: String) {
public fun update(context: Context, groupPublicKey: String, members: Collection<String>, name: String): Promise<Unit, Exception> {
val deferred = deferred<Unit, Exception>()
Thread {
val userPublicKey = TextSecurePreferences.getLocalNumber(context)
val sskDatabase = DatabaseFactory.getSSKDatabase(context)
val groupDB = DatabaseFactory.getGroupDatabase(context)
@ -109,7 +118,7 @@ object ClosedGroupsProtocol {
val group = groupDB.getGroup(groupID).orNull()
if (group == null) {
Log.d("Loki", "Can't update nonexistent closed group.")
return
return@Thread deferred.reject(Error.NoThread)
}
val oldMembers = group.members.map { it.serialize() }.toSet()
val newMembers = members.minus(oldMembers)
@ -119,7 +128,7 @@ object ClosedGroupsProtocol {
val groupPrivateKey = DatabaseFactory.getSSKDatabase(context).getClosedGroupPrivateKey(groupPublicKey)
if (groupPrivateKey == null) {
Log.d("Loki", "Couldn't get private key for closed group.")
return
return@Thread deferred.reject(Error.NoPrivateKey)
}
val wasAnyUserRemoved = members.toSet().intersect(oldMembers) != oldMembers.toSet()
val removedMembers = oldMembers.minus(members)
@ -128,14 +137,20 @@ object ClosedGroupsProtocol {
if (wasAnyUserRemoved) {
if (isUserLeaving && removedMembers.count() != 1) {
Log.d("Loki", "Can't remove self and others simultaneously.")
return
return@Thread deferred.reject(Error.InvalidUpdate)
}
// Send the update to the group (don't include new ratchets as everyone should regenerate new ratchets individually)
// Establish sessions if needed
establishSessionsWithMembersIfNeeded(context, members)
// Send the update to the existing members using established channels (don't include new ratchets as everyone should regenerate new ratchets individually)
for (member in oldMembers) {
@Suppress("NAME_SHADOWING")
val closedGroupUpdateKind = ClosedGroupUpdateMessageSendJob.Kind.Info(Hex.fromStringCondensed(groupPublicKey),
name, setOf(), membersAsData, adminsAsData)
val job = ClosedGroupUpdateMessageSendJob(groupPublicKey, closedGroupUpdateKind)
@Suppress("NAME_SHADOWING")
val job = ClosedGroupUpdateMessageSendJob(member, closedGroupUpdateKind)
job.setContext(context)
job.onRun() // Run the job immediately
}
// Delete all ratchets (it's important that this happens * after * sending out the update)
sskDatabase.removeAllClosedGroupRatchets(groupPublicKey)
// Remove the group from the user's set of public keys to poll for if the user is leaving. Otherwise generate a new ratchet and
@ -147,8 +162,6 @@ object ClosedGroupsProtocol {
// Notify the PN server
LokiPushNotificationManager.performOperation(context, ClosedGroupOperation.Unsubscribe, groupPublicKey, userPublicKey)
} else {
// Establish sessions if needed
establishSessionsWithMembersIfNeeded(context, members)
// Send closed group update messages to any new members using established channels
for (member in newMembers) {
@Suppress("NAME_SHADOWING")
@ -211,6 +224,9 @@ object ClosedGroupsProtocol {
val infoType = if (isUserLeaving) GroupContext.Type.QUIT else GroupContext.Type.UPDATE
val threadID = DatabaseFactory.getThreadDatabase(context).getThreadIdFor(Recipient.from(context, Address.fromSerialized(groupID), false))
insertOutgoingInfoMessage(context, groupID, infoType, name, members, admins, threadID)
deferred.resolve(Unit)
}.start()
return deferred.promise
}
@JvmStatic