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

fix: e2ei endpoint is called even when the feature is disabled #2508

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ interface MLSFailure : CoreFailure {
}

interface E2EIFailure : CoreFailure {

data object Disabled : E2EIFailure
data object MissingDiscoveryUrl : E2EIFailure
data class FailedInitialization(val step: E2EIEnrollmentResult.E2EIStep) : E2EIFailure
data class FailedOAuth(val reason: String) : E2EIFailure
data class FailedFinalization(val step: E2EIEnrollmentResult.E2EIStep) : E2EIFailure
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import kotlinx.datetime.Instant

data class E2EISettings(
val isRequired: Boolean,
val discoverUrl: String,
val discoverUrl: String?,
Copy link
Member Author

Choose a reason for hiding this comment

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

the url can be null even when the feature is enabled 😢

val gracePeriodEnd: Instant?
) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
package com.wire.kalium.logic.data.e2ei

import com.wire.kalium.logic.CoreFailure
import com.wire.kalium.logic.configuration.UserConfigRepository
import com.wire.kalium.logic.functional.Either
import com.wire.kalium.logic.functional.map
import com.wire.kalium.logic.wrapApiRequest
Expand All @@ -45,7 +44,7 @@ interface CertificateRevocationListRepository {
internal class CertificateRevocationListRepositoryDataSource(
private val acmeApi: ACMEApi,
private val metadataDAO: MetadataDAO,
private val userConfigRepository: UserConfigRepository
private val e2eiRepository: E2EIRepository
) : CertificateRevocationListRepository {
override suspend fun getCRLs(): CRLUrlExpirationList? =
metadataDAO.getSerializable(CRL_LIST_KEY, CRLUrlExpirationList.serializer())
Expand Down Expand Up @@ -83,8 +82,8 @@ internal class CertificateRevocationListRepositoryDataSource(
}

override suspend fun getCurrentClientCrlUrl(): Either<CoreFailure, String> =
userConfigRepository.getE2EISettings().map {
(Url(it.discoverUrl).protocolWithAuthority)
e2eiRepository.discoveryUrl().map {
Url(it).protocolWithAuthority
}

override suspend fun getClientDomainCRL(url: String): Either<CoreFailure, ByteArray> =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import com.wire.kalium.cryptography.AcmeDirectory
import com.wire.kalium.cryptography.NewAcmeAuthz
import com.wire.kalium.cryptography.NewAcmeOrder
import com.wire.kalium.logic.CoreFailure
import com.wire.kalium.logic.E2EIFailure
import com.wire.kalium.logic.configuration.UserConfigRepository
import com.wire.kalium.logic.data.client.E2EIClientProvider
import com.wire.kalium.logic.data.client.MLSClientProvider
Expand All @@ -35,6 +36,7 @@ import com.wire.kalium.logic.functional.Either
import com.wire.kalium.logic.functional.flatMap
import com.wire.kalium.logic.functional.fold
import com.wire.kalium.logic.functional.getOrFail
import com.wire.kalium.logic.functional.left
import com.wire.kalium.logic.functional.map
import com.wire.kalium.logic.kaliumLogger
import com.wire.kalium.logic.wrapApiRequest
Expand Down Expand Up @@ -91,6 +93,7 @@ interface E2EIRepository {
suspend fun fetchFederationCertificates(): Either<CoreFailure, Unit>
suspend fun getCurrentClientCrlUrl(): Either<CoreFailure, String>
suspend fun getClientDomainCRL(url: String): Either<CoreFailure, ByteArray>
fun discoveryUrl(): Either<CoreFailure, String>
}

@Suppress("LongParameterList")
Expand All @@ -116,9 +119,9 @@ class E2EIRepositoryImpl(
})
}

override suspend fun fetchAndSetTrustAnchors(): Either<CoreFailure, Unit> = userConfigRepository.getE2EISettings().flatMap {
override suspend fun fetchAndSetTrustAnchors(): Either<CoreFailure, Unit> = discoveryUrl().flatMap {
wrapApiRequest {
acmeApi.getTrustAnchors(Url(it.discoverUrl).protocolWithAuthority)
acmeApi.getTrustAnchors(Url(it).protocolWithAuthority)
}.flatMap { trustAnchors ->
mlsClientProvider.getMLSClient().flatMap { mlsClient ->
wrapE2EIRequest {
Expand All @@ -128,9 +131,9 @@ class E2EIRepositoryImpl(
}
}

override suspend fun loadACMEDirectories(): Either<CoreFailure, AcmeDirectory> = userConfigRepository.getE2EISettings().flatMap {
override suspend fun loadACMEDirectories(): Either<CoreFailure, AcmeDirectory> = discoveryUrl().flatMap {
wrapApiRequest {
acmeApi.getACMEDirectories(it.discoverUrl)
acmeApi.getACMEDirectories(it)
}.flatMap { directories ->
e2EIClientProvider.getE2EIClient().flatMap { e2eiClient ->
wrapE2EIRequest {
Expand Down Expand Up @@ -301,29 +304,37 @@ class E2EIRepositoryImpl(
Either.Right(e2EIClient.getOAuthRefreshToken())
}

override suspend fun fetchFederationCertificates(): Either<CoreFailure, Unit> = userConfigRepository.getE2EISettings().flatMap {
wrapApiRequest {
acmeApi.getACMEFederation(Url(it.discoverUrl).protocolWithAuthority)
}.flatMap { data ->
mlsClientProvider.getMLSClient().flatMap { mlsClient ->
wrapMLSRequest {
mlsClient.registerIntermediateCa(data)
override suspend fun fetchFederationCertificates(): Either<CoreFailure, Unit> = discoveryUrl()
.flatMap {
wrapApiRequest {
acmeApi.getACMEFederation(Url(it).protocolWithAuthority)
}.flatMap { data ->
mlsClientProvider.getMLSClient().flatMap { mlsClient ->
wrapMLSRequest {
mlsClient.registerIntermediateCa(data)
}
}
}
}
}

override fun discoveryUrl(): Either<CoreFailure, String> =
Copy link
Member Author

Choose a reason for hiding this comment

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

the idea of this function is to have a central place where the errors with e2ei url are caught

userConfigRepository.getE2EISettings().flatMap { settings ->
when {
!settings.isRequired -> E2EIFailure.Disabled.left()
settings.discoverUrl == null -> E2EIFailure.MissingDiscoveryUrl.left()
else -> Either.Right(settings.discoverUrl)
}
}

override suspend fun nukeE2EIClient() {
e2EIClientProvider.nuke()
}

override suspend fun getCurrentClientCrlUrl(): Either<CoreFailure, String> =
userConfigRepository.getE2EISettings().map {
(Url(it.discoverUrl).protocolWithAuthority)
}
discoveryUrl().map { Url(it).protocolWithAuthority }

override suspend fun getClientDomainCRL(url: String): Either<CoreFailure, ByteArray> =
wrapApiRequest {
acmeApi.getClientDomainCRL(url)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -131,13 +131,22 @@ class FeatureConfigMapperImpl : FeatureConfigMapper {
)

override fun fromDTO(data: FeatureConfigData.E2EI?): E2EIModel =
E2EIModel(
data?.let {
E2EIModel(
E2EIConfigModel(
data.config.url,
data.config.verificationExpirationSeconds
),
fromDTO(data.status)
)
} ?: E2EIModel(
E2EIConfigModel(
data?.config?.url ?: "",
data?.config?.verificationExpirationSeconds ?: 0L
null,
0
),
fromDTO(data?.status ?: FeatureFlagStatusDTO.DISABLED)
Status.DISABLED
)

override fun fromModel(status: Status): FeatureFlagStatusDTO =
when (status) {
Status.ENABLED -> FeatureFlagStatusDTO.ENABLED
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,6 @@ data class E2EIModel(
)

data class E2EIConfigModel(
val discoverUrl: String,
val discoverUrl: String?,
val verificationExpirationSeconds: Long
)
Original file line number Diff line number Diff line change
Expand Up @@ -1555,7 +1555,7 @@ class UserSessionScope internal constructor(
get() = CertificateRevocationListRepositoryDataSource(
acmeApi = globalScope.unboundNetworkContainer.acmeApi,
metadataDAO = userStorage.database.metadataDAO,
userConfigRepository = userConfigRepository
e2eiRepository = e2eiRepository
)

private val proteusPreKeyRefiller: ProteusPreKeyRefiller
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ internal class SyncFeatureConfigsUseCaseImpl(
private val appLockConfigHandler: AppLockConfigHandler
) : SyncFeatureConfigsUseCase {
override suspend operator fun invoke(): Either<CoreFailure, Unit> =
featureConfigRepository.getFeatureConfigs().flatMap {
featureConfigRepository.getFeatureConfigs().flatMap { it ->
// TODO handle other feature flags and after it bump version in [SlowSyncManager.CURRENT_VERSION]
guestRoomConfigHandler.handle(it.guestRoomLinkModel)
fileSharingConfigHandler.handle(it.fileSharingModel)
Expand All @@ -71,7 +71,7 @@ internal class SyncFeatureConfigsUseCaseImpl(
conferenceCallingConfigHandler.handle(it.conferenceCallingModel)
passwordChallengeConfigHandler.handle(it.secondFactorPasswordChallengeModel)
selfDeletingMessagesConfigHandler.handle(it.selfDeletingMessagesModel)
e2EIConfigHandler.handle(it.e2EIModel)
it.e2EIModel?.let { e2EIModel -> e2EIConfigHandler.handle(e2EIModel) }
appLockConfigHandler.handle(it.appLockModel)
Either.Right(Unit)
}.onFailure { networkFailure ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,12 @@ class E2EIConfigHandler(
private val userConfigRepository: UserConfigRepository,
) {
fun handle(e2eiConfig: E2EIModel): Either<CoreFailure, Unit> {
val gracePeriodEnd = DateTimeUtil.currentInstant()
.plus(e2eiConfig.config.verificationExpirationSeconds.toDuration(DurationUnit.SECONDS))
val gracePeriodEnd = e2eiConfig.config.verificationExpirationSeconds
.toDuration(DurationUnit.SECONDS)
.let {
DateTimeUtil.currentInstant().plus(it)
}

userConfigRepository.setE2EISettings(
E2EISettings(
isRequired = e2eiConfig.status == Status.ENABLED,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,9 @@ class CertificateRevocationListRepositoryTest {
val metadataDAO = mock(classOf<MetadataDAO>())

@Mock
val userConfigRepository = mock(classOf<UserConfigRepository>())
val e2eiRepository = mock(classOf<E2EIRepository>())

fun arrange() = this to CertificateRevocationListRepositoryDataSource(acmeApi, metadataDAO, userConfigRepository)
fun arrange() = this to CertificateRevocationListRepositoryDataSource(acmeApi, metadataDAO, e2eiRepository)

suspend fun withEmptyList() = apply {
given(metadataDAO).coroutine {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import com.wire.kalium.cryptography.MLSClient
import com.wire.kalium.cryptography.NewAcmeAuthz
import com.wire.kalium.cryptography.NewAcmeOrder
import com.wire.kalium.logic.CoreFailure
import com.wire.kalium.logic.E2EIFailure
import com.wire.kalium.logic.StorageFailure
import com.wire.kalium.logic.configuration.E2EISettings
import com.wire.kalium.logic.configuration.UserConfigRepository
Expand Down Expand Up @@ -68,6 +69,7 @@ import io.mockative.thenDoNothing
import io.mockative.time
import io.mockative.verify
import kotlinx.coroutines.test.runTest
import kotlinx.datetime.Instant
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertIs
Expand Down Expand Up @@ -309,7 +311,7 @@ class E2EIRepositoryTest {
// Given
val (arrangement, e2eiRepository) = Arrangement()
.withGetNewAuthzRequestSuccessful()
.withSendAuthorizationRequestSucceed(url = RANDOM_URL ,DtoAuthorizationChallengeType.DPoP)
.withSendAuthorizationRequestSucceed(url = RANDOM_URL, DtoAuthorizationChallengeType.DPoP)
.withSendAcmeRequestApiSucceed()
.withSetAuthzResponseSuccessful()
.withSendAcmeRequestApiSucceed()
Expand Down Expand Up @@ -414,7 +416,7 @@ class E2EIRepositoryTest {
val expected = Arrangement.AUTHORIZATIONS_RESULT
// Given
val (arrangement, e2eiRepository) = Arrangement()
.withSendAuthorizationRequestSucceed(url = authorizationsUrls[0],DtoAuthorizationChallengeType.DPoP)
.withSendAuthorizationRequestSucceed(url = authorizationsUrls[0], DtoAuthorizationChallengeType.DPoP)
.withSendAuthorizationRequestSucceed(url = authorizationsUrls[1], DtoAuthorizationChallengeType.OIDC)
.withGetE2EIClientSuccessful()
.withGetMLSClientSuccessful()
Expand Down Expand Up @@ -926,7 +928,7 @@ class E2EIRepositoryTest {
// Given

val (arrangement, e2eiRepository) = Arrangement()
.withGettingE2EISettingsReturns(Either.Right(E2EI_TEAM_SETTINGS.copy(discoverUrl = RANDOM_URL+"/random/path")))
.withGettingE2EISettingsReturns(Either.Right(E2EI_TEAM_SETTINGS.copy(discoverUrl = RANDOM_URL + "/random/path")))
.withFetchAcmeTrustAnchorsApiSucceed()
.withGetMLSClientSuccessful()
.withRegisterTrustAnchors()
Expand All @@ -953,6 +955,52 @@ class E2EIRepositoryTest {
.with(eq(Arrangement.RANDOM_BYTE_ARRAY.decodeToString()))
.wasInvoked(once)
}

@Test
fun givenE2EIIsDisabled_whenCallingDiscoveryUrl_thenItFailWithDisabled() {
val (arrangement, e2eiRepository) = Arrangement()
.withGettingE2EISettingsReturns(Either.Right(E2EISettings(false, null, Instant.DISTANT_FUTURE)))
.arrange()

e2eiRepository.discoveryUrl().shouldFail {
assertIs<E2EIFailure.Disabled>(it)
}

verify(arrangement.userConfigRepository)
.function(arrangement.userConfigRepository::getE2EISettings)
.wasInvoked(once)
}

@Test
fun givenE2EIIsEnabledAndDiscoveryUrlIsNull_whenCallingDiscoveryUrl_thenItFailWithMissingDiscoveryUrl() {
val (arrangement, e2eiRepository) = Arrangement()
.withGettingE2EISettingsReturns(Either.Right(E2EISettings(true, null, Instant.DISTANT_FUTURE)))
.arrange()

e2eiRepository.discoveryUrl().shouldFail {
assertIs<E2EIFailure.MissingDiscoveryUrl>(it)
}

verify(arrangement.userConfigRepository)
.function(arrangement.userConfigRepository::getE2EISettings)
.wasInvoked(once)
}

@Test
fun givenE2EIIsEnabledAndDiscoveryUrlIsNotNull_whenCallingDiscoveryUrl_thenItSucceed() {
val (arrangement, e2eiRepository) = Arrangement()
.withGettingE2EISettingsReturns(Either.Right(E2EISettings(true, RANDOM_URL, Instant.DISTANT_FUTURE)))
.arrange()

e2eiRepository.discoveryUrl().shouldSucceed {
assertEquals(RANDOM_URL, it)
}

verify(arrangement.userConfigRepository)
.function(arrangement.userConfigRepository::getE2EISettings)
.wasInvoked(once)
}

private class Arrangement {

fun withGetE2EIClientSuccessful() = apply {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ data class E2EIConfigDTO(
@SerialName("acmeDiscoveryUrl")
val url: String?,
@SerialName("verificationExpiration")
val verificationExpirationSeconds: Long = 0L
val verificationExpirationSeconds: Long
)

@OptIn(ExperimentalSerializationApi::class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ data class TeamSettingsSelfDeletionStatusEntity(
@Serializable
data class E2EISettingsEntity(
@SerialName("status") val status: Boolean,
@SerialName("discoverUrl") val discoverUrl: String,
@SerialName("discoverUrl") val discoverUrl: String?,
Copy link
Member Author

Choose a reason for hiding this comment

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

according to swagger this URL can be null

@SerialName("gracePeriodEndMs") val gracePeriodEndMs: Long?,
)

Expand Down
Loading