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.

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

[FEATURE REQUEST] Prevent http traffic #4110

Merged
merged 7 commits into from
Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Summary
* Enhancement - Improve grid mode: [#4027](https://github.com/owncloud/android/issues/4027)
* Enhancement - Improve UX of creation dialog: [#4031](https://github.com/owncloud/android/issues/4031)
* Enhancement - File name conflict starting by (1): [#4040](https://github.com/owncloud/android/pull/4040)
* Enhancement - Prevent http traffic with branding options: [#4066](https://github.com/owncloud/android/issues/4066)
* Enhancement - Support "per app" language change on Android 13+: [#4082](https://github.com/owncloud/android/issues/4082)

Details
Expand Down Expand Up @@ -138,6 +139,13 @@ Details
https://github.com/owncloud/android/issues/3946
https://github.com/owncloud/android/pull/4040

* Enhancement - Prevent http traffic with branding options: [#4066](https://github.com/owncloud/android/issues/4066)

Adding branding option for prevent http traffic.

https://github.com/owncloud/android/issues/4066
https://github.com/owncloud/android/pull/4110

* Enhancement - Support "per app" language change on Android 13+: [#4082](https://github.com/owncloud/android/issues/4082)

The locales_config.xml file has been created for the application to detect the language that
Expand Down
6 changes: 6 additions & 0 deletions changelog/unreleased/4110
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Enhancement: Prevent http traffic with branding options

Adding branding option for prevent http traffic.

https://github.com/owncloud/android/issues/4066
https://github.com/owncloud/android/pull/4110
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ val viewModelModule = module {
PassCodeViewModel(get(), get(), action)
}

viewModel { AuthenticationViewModel(get(), get(), get(), get(), get(), get(), get(), get(), get(), get(), get(), get(), get(), get()) }
viewModel { AuthenticationViewModel(get(), get(), get(), get(), get(), get(), get(), get(), get(), get(), get(), get(), get(), get(), get()) }
viewModel { OAuthViewModel(get(), get(), get(), get()) }
viewModel { SettingsViewModel(get()) }
viewModel { SettingsSecurityViewModel(get(), get()) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import androidx.lifecycle.MediatorLiveData
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.owncloud.android.MainApp
import com.owncloud.android.R
import com.owncloud.android.domain.authentication.oauth.RegisterClientUseCase
import com.owncloud.android.domain.authentication.oauth.RequestTokenUseCase
import com.owncloud.android.domain.authentication.oauth.model.ClientRegistrationInfo
Expand All @@ -45,6 +46,7 @@ import com.owncloud.android.domain.webfinger.usecases.GetOwnCloudInstancesFromAu
import com.owncloud.android.extensions.ViewModelExt.runUseCaseWithResult
import com.owncloud.android.presentation.authentication.oauth.OAuthUtils
import com.owncloud.android.presentation.common.UIResult
import com.owncloud.android.providers.ContextProvider
import com.owncloud.android.providers.CoroutinesDispatcherProvider
import com.owncloud.android.providers.WorkManagerProvider
import kotlinx.coroutines.launch
Expand All @@ -65,6 +67,7 @@ class AuthenticationViewModel(
private val requestTokenUseCase: RequestTokenUseCase,
private val registerClientUseCase: RegisterClientUseCase,
private val coroutinesDispatcherProvider: CoroutinesDispatcherProvider,
private val contextProvider: ContextProvider,
) : ViewModel() {

val codeVerifier: String = OAuthUtils().generateRandomCodeVerifier()
Expand Down Expand Up @@ -99,7 +102,11 @@ class AuthenticationViewModel(
showLoading = true,
liveData = _serverInfo,
useCase = getServerInfoAsyncUseCase,
useCaseParams = GetServerInfoAsyncUseCase.Params(serverPath = serverUrl, creatingAccount = creatingAccount)
useCaseParams = GetServerInfoAsyncUseCase.Params(
serverPath = serverUrl,
creatingAccount = creatingAccount,
secureConnectionEnforced = contextProvider.getBoolean(R.bool.enforce_secure_connection),
)
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import com.owncloud.android.domain.authentication.oauth.model.ResponseType
import com.owncloud.android.domain.authentication.oauth.model.TokenRequest
import com.owncloud.android.domain.exceptions.NoNetworkConnectionException
import com.owncloud.android.domain.exceptions.OwncloudVersionNotSupportedException
import com.owncloud.android.domain.exceptions.SSLErrorException
import com.owncloud.android.domain.exceptions.ServerNotReachableException
import com.owncloud.android.domain.exceptions.StateMismatchException
import com.owncloud.android.domain.exceptions.UnauthorizedException
Expand Down Expand Up @@ -272,6 +273,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted
text = getString(R.string.error_no_network_connection)
setCompoundDrawablesWithIntrinsicBounds(R.drawable.no_network, 0, 0, 0)
}

else -> binding.webfingerStatusText.run {
text = uiResult.getThrowableOrNull()?.parseError("", resources, true)
setCompoundDrawablesWithIntrinsicBounds(R.drawable.common_error, 0, 0, 0)
Expand Down Expand Up @@ -355,6 +357,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted
performGetAuthorizationCodeRequest(oidcServerConfiguration.authorizationEndpoint.toUri())
}
}

else -> {
binding.serverStatusText.run {
text = getString(R.string.auth_unsupported_auth_method)
Expand All @@ -379,14 +382,22 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted
when (uiResult.error) {
is CertificateCombinedException ->
showUntrustedCertDialog(uiResult.error)

is OwncloudVersionNotSupportedException -> binding.serverStatusText.run {
text = getString(R.string.server_not_supported)
setCompoundDrawablesWithIntrinsicBounds(R.drawable.common_error, 0, 0, 0)
}

is NoNetworkConnectionException -> binding.serverStatusText.run {
text = getString(R.string.error_no_network_connection)
setCompoundDrawablesWithIntrinsicBounds(R.drawable.no_network, 0, 0, 0)
}

is SSLErrorException -> binding.serverStatusText.run {
text = getString(R.string.ssl_connection_not_secure)
setCompoundDrawablesWithIntrinsicBounds(R.drawable.common_error, 0, 0, 0)
}

else -> binding.serverStatusText.run {
text = uiResult.error?.parseError("", resources, true)
setCompoundDrawablesWithIntrinsicBounds(R.drawable.common_error, 0, 0, 0)
Expand Down Expand Up @@ -430,6 +441,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted
}
showOrHideBasicAuthFields(shouldBeVisible = false)
}

else -> {
binding.serverStatusText.isVisible = false
binding.authStatusText.run {
Expand Down Expand Up @@ -461,6 +473,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted
)
}
}

is UIResult.Error -> {
Timber.e(uiResult.error, "Client registration failed.")
performGetAuthorizationCodeRequest(authorizationEndpoint)
Expand Down Expand Up @@ -585,6 +598,7 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted
clientRegistrationInfo = clientRegistrationInfo
)
}

is UIResult.Error -> {
Timber.e(uiResult.error, "OAuth request to exchange authorization code for tokens failed")
updateOAuthStatusIconAndText(uiResult.error)
Expand Down
4 changes: 4 additions & 0 deletions owncloudApp/src/main/res/values/setup.xml
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,8 @@

<!-- Webfinger -->
<string name="webfinger_lookup_server"></string>

<!-- Secure connection -->
<bool name="enforce_secure_connection">false</bool>

</resources>
1 change: 1 addition & 0 deletions owncloudApp/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@
<string name="oauth_check_onoff">Login with oAuth2</string>
<string name="oauth_login_connection">Connecting to OAuth2 server…</string>

<string name="ssl_connection_not_secure">Connection is not secure, http traffic is not allowed.</string>
<string name="ssl_validator_header">The identity of the server could not be verified</string>
<string name="ssl_validator_reason_cert_not_trusted">- The server certificate is not trusted</string>
<string name="ssl_validator_reason_cert_expired">- The server certificate expired</string>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

package com.owncloud.android.presentation.viewmodels.authentication

import com.owncloud.android.R
import com.owncloud.android.domain.UseCaseResult
import com.owncloud.android.domain.authentication.oauth.RegisterClientUseCase
import com.owncloud.android.domain.authentication.oauth.RequestTokenUseCase
Expand Down Expand Up @@ -125,6 +126,7 @@ class AuthenticationViewModelTest : ViewModelTest() {
every { anyConstructed<OAuthUtils>().generateRandomCodeVerifier() } returns "CODE VERIFIER"
every { anyConstructed<OAuthUtils>().generateCodeChallenge(any()) } returns "CODE CHALLENGE"
every { anyConstructed<OAuthUtils>().generateRandomState() } returns "STATE"
every { contextProvider.getBoolean(R.bool.enforce_secure_connection) } returns false

testCoroutineDispatcher.pauseDispatcher()

Expand All @@ -142,7 +144,8 @@ class AuthenticationViewModelTest : ViewModelTest() {
requestTokenUseCase = requestTokenUseCase,
registerClientUseCase = registerClientUseCase,
workManagerProvider = workManagerProvider,
coroutinesDispatcherProvider = coroutineDispatcherProvider
coroutinesDispatcherProvider = coroutineDispatcherProvider,
contextProvider = contextProvider,
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ package com.owncloud.android.domain.exceptions

import java.lang.Exception

class SSLErrorException : Exception()
class SSLErrorException(override val message: String? = null) : Exception(message)
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,29 @@
package com.owncloud.android.domain.server.usecases

import com.owncloud.android.domain.BaseUseCaseWithResult
import com.owncloud.android.domain.exceptions.SSLErrorException
import com.owncloud.android.domain.server.ServerInfoRepository
import com.owncloud.android.domain.server.model.ServerInfo
import com.owncloud.android.domain.server.model.ServerInfo.Companion.HTTPS_PREFIX
import com.owncloud.android.domain.server.model.ServerInfo.Companion.HTTP_PREFIX
import java.util.Locale

class GetServerInfoAsyncUseCase(
private val serverInfoRepository: ServerInfoRepository
private val serverInfoRepository: ServerInfoRepository,
) : BaseUseCaseWithResult<ServerInfo, GetServerInfoAsyncUseCase.Params>() {
override fun run(params: Params): ServerInfo {
val normalizedServerUrl = normalizeProtocolPrefix(params.serverPath).trimEnd(TRAILING_SLASH)
return serverInfoRepository.getServerInfo(normalizedServerUrl, params.creatingAccount)
val serverInfo = serverInfoRepository.getServerInfo(normalizedServerUrl, params.creatingAccount)
if (!serverInfo.isSecureConnection && params.secureConnectionEnforced) {
throw SSLErrorException("Connection is not secure, http traffic is not allowed.")
}
return serverInfo
}

data class Params(
val serverPath: String,
val creatingAccount: Boolean,
val secureConnectionEnforced: Boolean,
)

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
*/
package com.owncloud.android.domain.server.usecases

import com.owncloud.android.domain.exceptions.SSLErrorException
import com.owncloud.android.domain.server.ServerInfoRepository
import com.owncloud.android.domain.server.usecases.GetServerInfoAsyncUseCase.Companion.TRAILING_SLASH
import com.owncloud.android.testutil.OC_INSECURE_SERVER_INFO_BASIC_AUTH
import com.owncloud.android.testutil.OC_SECURE_SERVER_INFO_BASIC_AUTH
import io.mockk.every
import io.mockk.spyk
Expand All @@ -32,7 +34,11 @@ class GetServerInfoAsyncUseCaseTest {

private val repository: ServerInfoRepository = spyk()
private val useCase = GetServerInfoAsyncUseCase((repository))
private val useCaseParams = GetServerInfoAsyncUseCase.Params(serverPath = "http://demo.owncloud.com", false)
private val useCaseParams = GetServerInfoAsyncUseCase.Params(
serverPath = "http://demo.owncloud.com",
creatingAccount = false,
secureConnectionEnforced = false,
)
private val useCaseParamsWithSlash = useCaseParams.copy(serverPath = useCaseParams.serverPath.plus(TRAILING_SLASH))

@Test
Expand Down Expand Up @@ -70,4 +76,29 @@ class GetServerInfoAsyncUseCaseTest {

verify(exactly = 1) { repository.getServerInfo(useCaseParams.serverPath, false) }
}

@Test
fun `Should throw SSLErrorException when secureConnectionEnforced is true and ServerInfoRepository returns ServerInfo with isSecureConnection returning false`() {
every { repository.getServerInfo(useCaseParams.serverPath, false) } returns OC_INSECURE_SERVER_INFO_BASIC_AUTH

val useCaseResult = useCase.execute(useCaseParams.copy(secureConnectionEnforced = true))

assertTrue(useCaseResult.isError)
assertTrue(useCaseResult.getThrowableOrNull() is SSLErrorException)

verify(exactly = 1) { repository.getServerInfo(useCaseParams.serverPath, false) }
}

@Test
fun `Should work correctly when secureConnectionEnforced is true and ServerInfoRepository returns ServerInfo with isSecureConnection returning true`() {
every { repository.getServerInfo(useCaseParams.serverPath, false) } returns OC_SECURE_SERVER_INFO_BASIC_AUTH

val useCaseResult = useCase.execute(useCaseParams.copy(secureConnectionEnforced = true))

assertTrue(useCaseResult.isSuccess)
assertEquals(OC_SECURE_SERVER_INFO_BASIC_AUTH, useCaseResult.getDataOrNull())

verify(exactly = 1) { repository.getServerInfo(useCaseParams.serverPath, false) }
}

}