Refactor Git operations and auth (#1066)

Co-authored-by: Harsh Shandilya <me@msfjarvis.dev>

7 files changed, 71 insertions, 170 deletions

diff --git a/app/src/main/java/com/zeapo/pwdstore/git/operation/BreakOutOfDetached.kt b/app/src/main/java/com/zeapo/pwdstore/git/operation/BreakOutOfDetached.kt
index b46b66cf..6c7fee3a 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/operation/BreakOutOfDetached.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/operation/BreakOutOfDetached.kt
@@ -7,46 +7,30 @@ package com.zeapo.pwdstore.git.operation
 import androidx.appcompat.app.AppCompatActivity
 import com.google.android.material.dialog.MaterialAlertDialogBuilder
 import com.zeapo.pwdstore.R
-import com.zeapo.pwdstore.git.GitCommandExecutor
-import java.io.File
 import org.eclipse.jgit.api.RebaseCommand
 
-class BreakOutOfDetached(fileDir: File, callingActivity: AppCompatActivity) : GitOperation(fileDir, callingActivity) {
-
-    private val branchName = "conflicting-$remoteBranch-${System.currentTimeMillis()}"
+class BreakOutOfDetached(callingActivity: AppCompatActivity) : GitOperation(callingActivity) {
 
     override val commands = arrayOf(
         // abort the rebase
         git.rebase().setOperation(RebaseCommand.Operation.ABORT),
         // git checkout -b conflict-branch
-        git.checkout().setCreateBranch(true).setName(branchName),
+        git.checkout().setCreateBranch(true).setName("conflicting-$remoteBranch-${System.currentTimeMillis()}"),
         // push the changes
         git.push().setRemote("origin"),
         // switch back to ${gitBranch}
         git.checkout().setName(remoteBranch),
     )
 
-    override suspend fun execute() {
-        if (!git.repository.repositoryState.isRebasing) {
-            MaterialAlertDialogBuilder(callingActivity)
-                .setTitle(callingActivity.resources.getString(R.string.git_abort_and_push_title))
-                .setMessage("The repository is not rebasing, no need to push to another branch")
-                .setPositiveButton(callingActivity.resources.getString(R.string.dialog_ok)) { _, _ ->
-                    callingActivity.finish()
-                }.show()
-            return
-        }
-        GitCommandExecutor(callingActivity, this).execute()
-    }
-
-    override fun onSuccess() {
+    override fun preExecute() = if (!git.repository.repositoryState.isRebasing) {
         MaterialAlertDialogBuilder(callingActivity)
             .setTitle(callingActivity.resources.getString(R.string.git_abort_and_push_title))
-            .setMessage("There was a conflict when trying to rebase. " +
-                "Your local $remoteBranch branch was pushed to another branch named conflicting-$remoteBranch-....\n" +
-                "Use this branch to resolve conflict on your computer")
+            .setMessage(callingActivity.resources.getString(R.string.git_break_out_of_detached_unneeded))
             .setPositiveButton(callingActivity.resources.getString(R.string.dialog_ok)) { _, _ ->
                 callingActivity.finish()
             }.show()
+        false
+    } else {
+        true
     }
 }
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/operation/CloneOperation.kt b/app/src/main/java/com/zeapo/pwdstore/git/operation/CloneOperation.kt
index a032a669..256437a8 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/operation/CloneOperation.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/operation/CloneOperation.kt
@@ -5,34 +5,18 @@
 package com.zeapo.pwdstore.git.operation
 
 import androidx.appcompat.app.AppCompatActivity
-import com.zeapo.pwdstore.git.GitCommandExecutor
-import java.io.File
 import org.eclipse.jgit.api.Git
 import org.eclipse.jgit.api.GitCommand
 
 /**
  * Creates a new clone operation
  *
- * @param fileDir the git working tree directory
  * @param uri URL to clone the repository from
  * @param callingActivity the calling activity
  */
-class CloneOperation(fileDir: File, uri: String, callingActivity: AppCompatActivity) : GitOperation(fileDir, callingActivity) {
+class CloneOperation(callingActivity: AppCompatActivity, uri: String) : GitOperation(callingActivity) {
 
     override val commands: Array<GitCommand<out Any>> = arrayOf(
-        Git.cloneRepository().setBranch(remoteBranch).setDirectory(repository?.workTree).setURI(uri),
+        Git.cloneRepository().setBranch(remoteBranch).setDirectory(repository.workTree).setURI(uri),
     )
-
-    override suspend fun execute() {
-        GitCommandExecutor(callingActivity, this, finishActivityOnEnd = false).execute()
-    }
-
-    override fun onSuccess() {
-        callingActivity.finish()
-    }
-
-    override fun onError(err: Exception) {
-        finishFromErrorDialog = false
-        super.onError(err)
-    }
 }
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/operation/GitOperation.kt b/app/src/main/java/com/zeapo/pwdstore/git/operation/GitOperation.kt
index 1b47d79b..2a72005e 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/operation/GitOperation.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/operation/GitOperation.kt
@@ -6,30 +6,27 @@ package com.zeapo.pwdstore.git.operation
 
 import android.content.Intent
 import android.widget.Toast
-import androidx.annotation.CallSuper
-import androidx.core.content.edit
 import androidx.fragment.app.FragmentActivity
-import com.github.ajalt.timberkt.Timber.d
+import com.github.michaelbull.result.Err
+import com.github.michaelbull.result.Ok
+import com.github.michaelbull.result.Result
 import com.google.android.material.dialog.MaterialAlertDialogBuilder
 import com.zeapo.pwdstore.R
 import com.zeapo.pwdstore.UserPreference
-import com.zeapo.pwdstore.git.ErrorMessages
+import com.zeapo.pwdstore.git.GitCommandExecutor
 import com.zeapo.pwdstore.git.config.AuthMode
 import com.zeapo.pwdstore.git.config.GitSettings
-import com.zeapo.pwdstore.git.sshj.InteractivePasswordFinder
 import com.zeapo.pwdstore.git.sshj.SshAuthData
 import com.zeapo.pwdstore.git.sshj.SshKey
 import com.zeapo.pwdstore.git.sshj.SshjSessionFactory
 import com.zeapo.pwdstore.utils.BiometricAuthenticator
 import com.zeapo.pwdstore.utils.PasswordRepository
-import com.zeapo.pwdstore.utils.PreferenceKeys
-import com.zeapo.pwdstore.utils.getEncryptedPrefs
-import com.zeapo.pwdstore.utils.sharedPrefs
-import java.io.File
 import kotlin.coroutines.resume
 import kotlin.coroutines.suspendCoroutine
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
+import net.schmizz.sshj.common.DisconnectReason
+import net.schmizz.sshj.common.SSHException
 import net.schmizz.sshj.userauth.password.PasswordFinder
 import org.eclipse.jgit.api.Git
 import org.eclipse.jgit.api.GitCommand
@@ -37,24 +34,22 @@ import org.eclipse.jgit.api.TransportCommand
 import org.eclipse.jgit.errors.UnsupportedCredentialItem
 import org.eclipse.jgit.transport.CredentialItem
 import org.eclipse.jgit.transport.CredentialsProvider
-import org.eclipse.jgit.transport.SshSessionFactory
+import org.eclipse.jgit.transport.SshTransport
+import org.eclipse.jgit.transport.Transport
 import org.eclipse.jgit.transport.URIish
 
-const val ANDROID_KEYSTORE_ALIAS_SSH_KEY = "ssh_key"
-
 /**
  * Creates a new git operation
  *
- * @param gitDir the git working tree directory
  * @param callingActivity the calling activity
  */
-abstract class GitOperation(gitDir: File, internal val callingActivity: FragmentActivity) {
+abstract class GitOperation(protected val callingActivity: FragmentActivity) {
 
     abstract val commands: Array<GitCommand<out Any>>
-    private var provider: CredentialsProvider? = null
     private val hostKeyFile = callingActivity.filesDir.resolve(".host_key")
-    protected var finishFromErrorDialog = true
-    protected val repository = PasswordRepository.getRepository(gitDir)
+    private var sshSessionFactory: SshjSessionFactory? = null
+
+    protected val repository = PasswordRepository.getRepository(null)!!
     protected val git = Git(repository)
     protected val remoteBranch = GitSettings.branch
 
@@ -90,27 +85,6 @@ abstract class GitOperation(gitDir: File, internal val callingActivity: Fragment
         }
     }
 
-    private fun withPasswordAuthentication(passwordFinder: InteractivePasswordFinder): GitOperation {
-        val sessionFactory = SshjSessionFactory(SshAuthData.Password(passwordFinder), hostKeyFile)
-        SshSessionFactory.setInstance(sessionFactory)
-        this.provider = HttpsCredentialsProvider(passwordFinder)
-        return this
-    }
-
-    private fun withSshKeyAuthentication(passphraseFinder: InteractivePasswordFinder): GitOperation {
-        val sessionFactory = SshjSessionFactory(SshAuthData.SshKey(passphraseFinder), hostKeyFile)
-        SshSessionFactory.setInstance(sessionFactory)
-        this.provider = null
-        return this
-    }
-
-    private fun withOpenKeychainAuthentication(activity: FragmentActivity): GitOperation {
-        val sessionFactory = SshjSessionFactory(SshAuthData.OpenKeychain(activity), hostKeyFile)
-        SshSessionFactory.setInstance(sessionFactory)
-        this.provider = null
-        return this
-    }
-
     private fun getSshKey(make: Boolean) {
         try {
             // Ask the UserPreference to provide us with the ssh-key
@@ -124,16 +98,30 @@ abstract class GitOperation(gitDir: File, internal val callingActivity: Fragment
         }
     }
 
-    fun setCredentialProvider() {
-        provider?.let { credentialsProvider ->
-            commands.filterIsInstance<TransportCommand<*, *>>().forEach { it.setCredentialsProvider(credentialsProvider) }
+    private fun registerAuthProviders(authData: SshAuthData, credentialsProvider: CredentialsProvider? = null) {
+        sshSessionFactory = SshjSessionFactory(authData, hostKeyFile)
+        commands.filterIsInstance<TransportCommand<*, *>>().forEach { command ->
+            command.setTransportConfigCallback { transport: Transport ->
+                (transport as? SshTransport)?.sshSessionFactory = sshSessionFactory
+                credentialsProvider?.let { transport.credentialsProvider = it }
+            }
         }
     }
 
     /**
-     * Executes the GitCommand in an async task
+     * Executes the GitCommand in an async task.
      */
-    abstract suspend fun execute()
+    suspend fun execute(): Result<Unit, Throwable> {
+        if (!preExecute()) {
+            return Ok(Unit)
+        }
+        val operationResult = GitCommandExecutor(
+            callingActivity,
+            this,
+        ).execute()
+        postExecute()
+        return operationResult
+    }
 
     private fun onMissingSshKeyFile() {
         MaterialAlertDialogBuilder(callingActivity)
@@ -151,9 +139,7 @@ abstract class GitOperation(gitDir: File, internal val callingActivity: Fragment
             }.show()
     }
 
-    suspend fun executeAfterAuthentication(
-        authMode: AuthMode,
-    ) {
+    suspend fun executeAfterAuthentication(authMode: AuthMode): Result<Unit, Throwable> {
         when (authMode) {
             AuthMode.SshKey -> if (SshKey.exists) {
                 if (SshKey.mustAuthenticate) {
@@ -167,9 +153,12 @@ abstract class GitOperation(gitDir: File, internal val callingActivity: Fragment
                     }
                     when (result) {
                         is BiometricAuthenticator.Result.Success -> {
-                            withSshKeyAuthentication(CredentialFinder(callingActivity, authMode)).execute()
+                            registerAuthProviders(
+                                SshAuthData.SshKey(CredentialFinder(callingActivity, AuthMode.SshKey)))
+                        }
+                        is BiometricAuthenticator.Result.Cancelled -> {
+                            return Err(SSHException(DisconnectReason.AUTH_CANCELLED_BY_USER))
                         }
-                        is BiometricAuthenticator.Result.Cancelled -> callingActivity.finish()
                         is BiometricAuthenticator.Result.Failure -> {
                             throw IllegalStateException("Biometric authentication failures should be ignored")
                         }
@@ -183,42 +172,37 @@ abstract class GitOperation(gitDir: File, internal val callingActivity: Fragment
                         }
                     }
                 } else {
-                    withSshKeyAuthentication(CredentialFinder(callingActivity, authMode)).execute()
+                    registerAuthProviders(SshAuthData.SshKey(CredentialFinder(callingActivity, AuthMode.SshKey)))
                 }
             } else {
                 onMissingSshKeyFile()
             }
-            AuthMode.OpenKeychain -> withOpenKeychainAuthentication(callingActivity).execute()
-            AuthMode.Password -> withPasswordAuthentication(
-                CredentialFinder(callingActivity, authMode)).execute()
-            AuthMode.None -> execute()
+            AuthMode.OpenKeychain -> registerAuthProviders(SshAuthData.OpenKeychain(callingActivity))
+            AuthMode.Password -> {
+                val credentialFinder = CredentialFinder(callingActivity, AuthMode.Password)
+                val httpsCredentialProvider = HttpsCredentialsProvider(credentialFinder)
+                registerAuthProviders(
+                    SshAuthData.Password(CredentialFinder(callingActivity, AuthMode.Password)),
+                    httpsCredentialProvider)
+            }
+            AuthMode.None -> {
+            }
         }
+        return execute()
     }
 
     /**
-     * Action to execute on error
+     * Called before execution of the Git operation.
+     * Return false to cancel.
      */
-    @CallSuper
-    open fun onError(err: Exception) {
-        // Clear various auth related fields on failure
-        callingActivity.getEncryptedPrefs("git_operation").edit {
-            remove(PreferenceKeys.HTTPS_PASSWORD)
+    open fun preExecute() = true
+
+    private suspend fun postExecute() {
+        withContext(Dispatchers.IO) {
+            sshSessionFactory?.close()
         }
-        callingActivity.sharedPrefs.edit { remove(PreferenceKeys.SSH_OPENKEYSTORE_KEYID) }
-        d(err)
-        MaterialAlertDialogBuilder(callingActivity)
-            .setTitle(callingActivity.resources.getString(R.string.jgit_error_dialog_title))
-            .setMessage(ErrorMessages[err])
-            .setPositiveButton(callingActivity.resources.getString(R.string.dialog_ok)) { _, _ ->
-                if (finishFromErrorDialog) callingActivity.finish()
-            }.show()
     }
 
-    /**
-     * Action to execute on success
-     */
-    open fun onSuccess() {}
-
     companion object {
 
         const val GET_SSH_KEY_FROM_CLONE = 201
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/operation/PullOperation.kt b/app/src/main/java/com/zeapo/pwdstore/git/operation/PullOperation.kt
index d31b2aa4..d4ce6d01 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/operation/PullOperation.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/operation/PullOperation.kt
@@ -5,24 +5,11 @@
 package com.zeapo.pwdstore.git.operation
 
 import androidx.appcompat.app.AppCompatActivity
-import com.zeapo.pwdstore.git.GitCommandExecutor
-import java.io.File
-import org.eclipse.jgit.api.Git
 import org.eclipse.jgit.api.GitCommand
 
-/**
- * Creates a new git operation
- *
- * @param fileDir the git working tree directory
- * @param callingActivity the calling activity
- */
-class PullOperation(fileDir: File, callingActivity: AppCompatActivity) : GitOperation(fileDir, callingActivity) {
+class PullOperation(callingActivity: AppCompatActivity) : GitOperation(callingActivity) {
 
     override val commands: Array<GitCommand<out Any>> = arrayOf(
-        Git(repository).pull().setRebase(true).setRemote("origin"),
+        git.pull().setRebase(true).setRemote("origin"),
     )
-
-    override suspend fun execute() {
-        GitCommandExecutor(callingActivity, this).execute()
-    }
 }
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/operation/PushOperation.kt b/app/src/main/java/com/zeapo/pwdstore/git/operation/PushOperation.kt
index 33b20a06..d3bee209 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/operation/PushOperation.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/operation/PushOperation.kt
@@ -5,25 +5,11 @@
 package com.zeapo.pwdstore.git.operation
 
 import androidx.appcompat.app.AppCompatActivity
-import com.zeapo.pwdstore.git.GitCommandExecutor
-import java.io.File
-import org.eclipse.jgit.api.Git
 import org.eclipse.jgit.api.GitCommand
 
-/**
- * Creates a new git operation
- *
- * @param fileDir the git working tree directory
- * @param callingActivity the calling activity
- */
-class PushOperation(fileDir: File, callingActivity: AppCompatActivity) : GitOperation(fileDir, callingActivity) {
+class PushOperation(callingActivity: AppCompatActivity) : GitOperation(callingActivity) {
 
     override val commands: Array<GitCommand<out Any>> = arrayOf(
-        Git(repository).push().setPushAll().setRemote("origin"),
+        git.push().setPushAll().setRemote("origin"),
     )
-
-    override suspend fun execute() {
-        setCredentialProvider()
-        GitCommandExecutor(callingActivity, this).execute()
-    }
 }
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/operation/ResetToRemoteOperation.kt b/app/src/main/java/com/zeapo/pwdstore/git/operation/ResetToRemoteOperation.kt
index 0d0dc019..2df7a2b4 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/operation/ResetToRemoteOperation.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/operation/ResetToRemoteOperation.kt
@@ -5,17 +5,9 @@
 package com.zeapo.pwdstore.git.operation
 
 import androidx.appcompat.app.AppCompatActivity
-import com.zeapo.pwdstore.git.GitCommandExecutor
-import java.io.File
 import org.eclipse.jgit.api.ResetCommand
 
-/**
- * Creates a new git operation
- *
- * @param fileDir the git working tree directory
- * @param callingActivity the calling activity
- */
-class ResetToRemoteOperation(fileDir: File, callingActivity: AppCompatActivity) : GitOperation(fileDir, callingActivity) {
+class ResetToRemoteOperation(callingActivity: AppCompatActivity) : GitOperation(callingActivity) {
 
     override val commands = arrayOf(
         // Stage all files
@@ -28,8 +20,4 @@ class ResetToRemoteOperation(fileDir: File, callingActivity: AppCompatActivity)
         // branches from 'master' to anything else.
         git.branchCreate().setName(remoteBranch).setForce(true),
     )
-
-    override suspend fun execute() {
-        GitCommandExecutor(callingActivity, this).execute()
-    }
 }
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/operation/SyncOperation.kt b/app/src/main/java/com/zeapo/pwdstore/git/operation/SyncOperation.kt
index 35b12810..a0a80fe3 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/operation/SyncOperation.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/operation/SyncOperation.kt
@@ -5,16 +5,8 @@
 package com.zeapo.pwdstore.git.operation
 
 import androidx.appcompat.app.AppCompatActivity
-import com.zeapo.pwdstore.git.GitCommandExecutor
-import java.io.File
 
-/**
- * Creates a new git operation
- *
- * @param fileDir the git working tree directory
- * @param callingActivity the calling activity
- */
-class SyncOperation(fileDir: File, callingActivity: AppCompatActivity) : GitOperation(fileDir, callingActivity) {
+class SyncOperation(callingActivity: AppCompatActivity) : GitOperation(callingActivity) {
 
     override val commands = arrayOf(
         // Stage all files
@@ -28,8 +20,4 @@ class SyncOperation(fileDir: File, callingActivity: AppCompatActivity) : GitOper
         // Push it all back
         git.push().setPushAll().setRemote("origin"),
     )
-
-    override suspend fun execute() {
-        GitCommandExecutor(callingActivity, this).execute()
-    }
 }