From 65cb1993ae3ee173369b0a7fed2e00eb76a3e01b Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Wed, 5 Jun 2024 19:14:43 +0600 Subject: [PATCH 1/6] fix: oidc authService.dispose() can throw exception, which can crash app --- .../kotlin/at/bitfire/davdroid/network/HttpClient.kt | 8 ++++++-- .../syncadapter/DefaultAccountAuthenticatorService.kt | 6 +++++- .../at/bitfire/davdroid/syncadapter/SyncManager.kt | 6 +++++- .../at/bitfire/davdroid/ui/setup/GoogleLoginFragment.kt | 6 +++++- .../davdroid/ui/setup/OpenIdAuthenticationViewModel.kt | 7 ++++++- .../davdroid/ui/signout/OpenIdEndSessionActivity.kt | 9 ++++++++- 6 files changed, 35 insertions(+), 7 deletions(-) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt b/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt index b5fff9d5f..e540e17f9 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt @@ -89,8 +89,12 @@ class HttpClient private constructor( } override fun close() { - authService?.dispose() - okHttpClient.cache?.close() + try { + okHttpClient.cache?.close() + authService?.dispose() + } catch (e: Exception) { + Logger.log.log(Level.SEVERE, "failed to clear resource on close httpClient", e) + } } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/DefaultAccountAuthenticatorService.kt b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/DefaultAccountAuthenticatorService.kt index da635b072..16adca035 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/DefaultAccountAuthenticatorService.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/DefaultAccountAuthenticatorService.kt @@ -232,7 +232,11 @@ abstract class DefaultAccountAuthenticatorService : Service(), OnAccountsUpdateL result.putString(AccountManager.KEY_AUTHTOKEN, authState.accessToken) response?.onResult(result) - authorizationService.dispose() + try { + authorizationService.dispose() + } catch (e: Exception) { + Logger.log.log(Level.INFO, "failed to dispose oidc authorizationService", e) + } } val result = Bundle() diff --git a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncManager.kt b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncManager.kt index 2a6688353..dd6b8418a 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncManager.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/syncadapter/SyncManager.kt @@ -221,7 +221,11 @@ abstract class SyncManager, out CollectionType: L performSync(DEFAULT_RETRY_AFTER, DEFAULT_SECOND_RETRY_AFTER, DEFAULT_MAX_RETRY_TIME) } - authorizationService.dispose() + try { + authorizationService.dispose() + } catch (e: Exception) { + Logger.log.log(Level.INFO, "failed to dispose oidc authorizationService", e) + } } } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/GoogleLoginFragment.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/GoogleLoginFragment.kt index 6ef6e0971..21c91f488 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/GoogleLoginFragment.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/GoogleLoginFragment.kt @@ -209,7 +209,11 @@ class GoogleLoginFragment(private val defaultEmail: String? = null): Fragment() } override fun onCleared() { - authService.dispose() + try { + authService.dispose() + } catch (e: Exception) { + Logger.log.log(Level.INFO, "failed to dispose oidc authorizationService", e) + } } } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/OpenIdAuthenticationViewModel.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/OpenIdAuthenticationViewModel.kt index 175872dc5..f777a8d80 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/OpenIdAuthenticationViewModel.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/OpenIdAuthenticationViewModel.kt @@ -62,8 +62,13 @@ class OpenIdAuthenticationViewModel(application: Application) : AndroidViewModel } override fun onCleared() { - authorizationService.dispose() super.onCleared() + try { + authorizationService.dispose() + } catch (e: Exception) { + Logger.log.log(Level.INFO, "failed to dispose oidc authorizationService", e) + } + } fun getAuthState(): AuthState { diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/signout/OpenIdEndSessionActivity.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/signout/OpenIdEndSessionActivity.kt index 631e4b8f7..5001315b4 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/ui/signout/OpenIdEndSessionActivity.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/ui/signout/OpenIdEndSessionActivity.kt @@ -20,11 +20,13 @@ import android.accounts.AccountManager import android.app.Activity import android.os.Bundle import at.bitfire.davdroid.authorization.IdentityProvider +import at.bitfire.davdroid.log.Logger import at.bitfire.davdroid.settings.AccountSettings import net.openid.appauth.AuthState import net.openid.appauth.AuthorizationService import net.openid.appauth.AuthorizationServiceConfiguration import net.openid.appauth.EndSessionRequest +import java.util.logging.Level class OpenIdEndSessionActivity : Activity() { @@ -79,7 +81,12 @@ class OpenIdEndSessionActivity : Activity() { } override fun onDestroy() { - authorizationService?.dispose() super.onDestroy() + + try { + authorizationService?.dispose() + } catch (e: Exception) { + Logger.log.log(Level.INFO, "failed to dispose oidc authorizationService", e) + } } } -- GitLab From 8cf3c1cfc057a9e637b54578afaf61bb0beafae7 Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Wed, 5 Jun 2024 19:16:13 +0600 Subject: [PATCH 2/6] chore: remove clientSecret for murena OIDC auth flow As recomended by the industry, we will use only PKCE key for secrecy & remove clientSecret for murena OIDC flow --- .gitlab-ci.yml | 1 - app/build.gradle | 1 - .../at/bitfire/davdroid/authorization/IdentityProvider.kt | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 8af528d99..012978291 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -7,7 +7,6 @@ stages: before_script: - echo email.key=$PEPPER >> local.properties - echo MURENA_CLIENT_ID=$MURENA_CLIENT_ID >> local.properties - - echo MURENA_CLIENT_SECRET=$MURENA_CLIENT_SECRET >> local.properties - echo MURENA_REDIRECT_URI=$MURENA_REDIRECT_URI >> local.properties - echo MURENA_LOGOUT_REDIRECT_URI=$MURENA_LOGOUT_REDIRECT_URI >> local.properties - echo MURENA_BASE_URL=$MURENA_BASE_URL >> local.properties diff --git a/app/build.gradle b/app/build.gradle index b51c845a0..ad6359db0 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -109,7 +109,6 @@ android { defaultConfig { buildConfigField "String", "MURENA_CLIENT_ID", "\"${retrieveKey("MURENA_CLIENT_ID")}\"" - buildConfigField "String", "MURENA_CLIENT_SECRET", "\"${retrieveKey("MURENA_CLIENT_SECRET")}\"" buildConfigField "String", "MURENA_REDIRECT_URI", "\"${retrieveKey("MURENA_REDIRECT_URI")}\"" buildConfigField "String", "MURENA_LOGOUT_REDIRECT_URI", "\"${retrieveKey("MURENA_LOGOUT_REDIRECT_URI")}\"" buildConfigField "String", "MURENA_BASE_URL", "\"${retrieveKey("MURENA_BASE_URL")}\"" diff --git a/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt b/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt index f7fce1c16..c7d8741aa 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt @@ -42,7 +42,7 @@ enum class IdentityProvider( authEndpoint = null, tokenEndpoint = null, clientId = BuildConfig.MURENA_CLIENT_ID, - clientSecret = BuildConfig.MURENA_CLIENT_SECRET, + clientSecret = null, redirectUri = BuildConfig.MURENA_REDIRECT_URI + ":/redirect", logoutRedirectUri = BuildConfig.MURENA_LOGOUT_REDIRECT_URI + ":/redirect", scope = "openid address profile email phone roles offline_access web-origins microprofile-jwt", -- GitLab From 5d366c55cdc455f80773ffdc4dd5954f164e6433 Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Wed, 5 Jun 2024 19:16:27 +0600 Subject: [PATCH 3/6] chore: enable murena OIDC support --- .../at/bitfire/davdroid/ui/setup/EeloAuthenticatorModel.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorModel.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorModel.kt index c65296df1..53691500e 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorModel.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorModel.kt @@ -27,7 +27,7 @@ class EeloAuthenticatorModel(application: Application) : AndroidViewModel(applic companion object { // as https://gitlab.e.foundation/e/backlog/-/issues/6287 is blocked, the openId implementation is not ready yet. // But we want to push the changes so later we won't face any conflict. So we are disabling the openId feature for now. - const val enableOpenIdSupport = false + const val enableOpenIdSupport = true } private var initialized = false -- GitLab From e6594260f8f9f18f68b789129b1e336e09312b34 Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Mon, 30 Sep 2024 17:52:29 +0600 Subject: [PATCH 4/6] fix: remove extra scopes for murena oidc for OIDC login, the accessToken length is dependent on the requested scopes. Which cause /e/OS mail is not working as expected for long accessToken (mailServer has a barier on perline length of request). Removing not required scopes resolve this issue. issue: https://gitlab.e.foundation/e/os/backlog/-/issues/2497 --- .../at/bitfire/davdroid/authorization/IdentityProvider.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt b/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt index f7fce1c16..868529719 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt @@ -45,7 +45,7 @@ enum class IdentityProvider( clientSecret = BuildConfig.MURENA_CLIENT_SECRET, redirectUri = BuildConfig.MURENA_REDIRECT_URI + ":/redirect", logoutRedirectUri = BuildConfig.MURENA_LOGOUT_REDIRECT_URI + ":/redirect", - scope = "openid address profile email phone roles offline_access web-origins microprofile-jwt", + scope = "openid profile email", userInfoEndpoint = null, baseUrl = BuildConfig.MURENA_BASE_URL, ), -- GitLab From 9f7c3a7121a3a67cdf3f222fdf601ff12577ece0 Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Mon, 30 Sep 2024 19:37:09 +0600 Subject: [PATCH 5/6] fixup --- .../at/bitfire/davdroid/authorization/IdentityProvider.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt b/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt index 868529719..c7cdbf441 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/authorization/IdentityProvider.kt @@ -45,7 +45,7 @@ enum class IdentityProvider( clientSecret = BuildConfig.MURENA_CLIENT_SECRET, redirectUri = BuildConfig.MURENA_REDIRECT_URI + ":/redirect", logoutRedirectUri = BuildConfig.MURENA_LOGOUT_REDIRECT_URI + ":/redirect", - scope = "openid profile email", + scope = "openid profile email offline_access", userInfoEndpoint = null, baseUrl = BuildConfig.MURENA_BASE_URL, ), -- GitLab From 5792b76a92e4d07d7c8860e4fb66ec1fae78f429 Mon Sep 17 00:00:00 2001 From: Fahim Salam Chowdhury Date: Thu, 17 Oct 2024 19:29:21 +0600 Subject: [PATCH 6/6] chore: improve according to code review - update constants variable name - fix log level --- .../main/kotlin/at/bitfire/davdroid/network/HttpClient.kt | 2 +- .../davdroid/ui/setup/DetectConfigurationFragment.kt | 6 ++---- .../davdroid/ui/setup/EeloAuthenticatorFragment.kt | 8 ++++---- .../bitfire/davdroid/ui/setup/EeloAuthenticatorModel.kt | 2 +- 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt b/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt index e540e17f9..d61ba172a 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/network/HttpClient.kt @@ -93,7 +93,7 @@ class HttpClient private constructor( okHttpClient.cache?.close() authService?.dispose() } catch (e: Exception) { - Logger.log.log(Level.SEVERE, "failed to clear resource on close httpClient", e) + Logger.log.log(Level.INFO, "failed to clear resource on close httpClient", e) } } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/DetectConfigurationFragment.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/DetectConfigurationFragment.kt index 92114462c..be7d2750e 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/DetectConfigurationFragment.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/DetectConfigurationFragment.kt @@ -10,7 +10,6 @@ import android.os.Bundle import android.view.LayoutInflater import android.view.View import android.view.ViewGroup -import androidx.core.content.ContextCompat import androidx.fragment.app.DialogFragment import androidx.fragment.app.Fragment import androidx.fragment.app.activityViewModels @@ -18,14 +17,13 @@ import androidx.fragment.app.viewModels import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData -import at.bitfire.davdroid.Constants +import at.bitfire.davdroid.ECloudAccountHelper import at.bitfire.davdroid.R import at.bitfire.davdroid.db.Credentials import at.bitfire.davdroid.log.Logger import at.bitfire.davdroid.servicedetection.DavResourceFinder import at.bitfire.davdroid.ui.DebugInfoActivity import com.google.android.material.dialog.MaterialAlertDialogBuilder -import at.bitfire.davdroid.ECloudAccountHelper import java.lang.ref.WeakReference import java.net.URI import java.util.logging.Level @@ -47,7 +45,7 @@ class DetectConfigurationFragment: Fragment() { return } - val blockOnUnauthorizedException = (accountType == getString(R.string.eelo_account_type)) && !EeloAuthenticatorModel.enableOpenIdSupport + val blockOnUnauthorizedException = (accountType == getString(R.string.eelo_account_type)) && !EeloAuthenticatorModel.ENABLE_OIDC_SUPPORT val baseURI = loginModel.baseURI ?: return diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorFragment.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorFragment.kt index 77b0252c7..47bbafcf0 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorFragment.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorFragment.kt @@ -79,7 +79,7 @@ class EeloAuthenticatorFragment : Fragment() { passwordEditText = v.root.findViewById(R.id.urlpwd_password) passwordHolder = v.root.findViewById(R.id.password_holder) - passwordHolder.isVisible = !EeloAuthenticatorModel.enableOpenIdSupport + passwordHolder.isVisible = !EeloAuthenticatorModel.ENABLE_OIDC_SUPPORT serverToggleButton.setOnClickListener { expandCollapse() } @@ -87,7 +87,7 @@ class EeloAuthenticatorFragment : Fragment() { val tfaButton = v.root.findViewById(R.id.twofa_info_button) tfaButton.setOnClickListener { show2FAInfoDialog() } - tfaButton.isVisible = !EeloAuthenticatorModel.enableOpenIdSupport + tfaButton.isVisible = !EeloAuthenticatorModel.ENABLE_OIDC_SUPPORT userIdEditText.doOnTextChanged { text, _, _, _ -> val domain = computeDomain(text) @@ -205,7 +205,7 @@ class EeloAuthenticatorFragment : Fragment() { private fun login() { handleNoNetworkAvailable() - val handleOpenIdAuth = EeloAuthenticatorModel.enableOpenIdSupport && !toggleButtonState + val handleOpenIdAuth = EeloAuthenticatorModel.ENABLE_OIDC_SUPPORT && !toggleButtonState val userId = userIdEditText.text.toString() val password = passwordEditText.text.toString() @@ -306,7 +306,7 @@ class EeloAuthenticatorFragment : Fragment() { serverUrlEditText.isEnabled = false toggleButtonState = false - if(!EeloAuthenticatorModel.enableOpenIdSupport) { + if(!EeloAuthenticatorModel.ENABLE_OIDC_SUPPORT) { return } diff --git a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorModel.kt b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorModel.kt index 53691500e..9b05b2e60 100644 --- a/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorModel.kt +++ b/app/src/main/kotlin/at/bitfire/davdroid/ui/setup/EeloAuthenticatorModel.kt @@ -27,7 +27,7 @@ class EeloAuthenticatorModel(application: Application) : AndroidViewModel(applic companion object { // as https://gitlab.e.foundation/e/backlog/-/issues/6287 is blocked, the openId implementation is not ready yet. // But we want to push the changes so later we won't face any conflict. So we are disabling the openId feature for now. - const val enableOpenIdSupport = true + const val ENABLE_OIDC_SUPPORT = true } private var initialized = false -- GitLab