feat: allow http calls when checking Certificate Revocation List (WPB-6493) #2491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ohassine
requested review from
a team,
typfel,
alexandreferris,
vitorhugods,
Garzas and
saleniuk
and removed request for
a team
8 tasks
Test Results2 271 tests - 636 2 221 ✔️ - 564 14s ⏱️ - 2m 20s Results for commit a634d73. ± Comparison against base commit fff203c. This pull request removes 2907 and adds 2271 tests. Note that renamed tests count towards both.This pull request removes 122 skipped tests and adds 50 skipped tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
MohamadJaara
requested changes
Feb 13, 2024
| @@ -35,8 +39,15 @@ private object OkHttpSingleton { | |||
| .writeTimeout(WEBSOCKET_TIMEOUT, TimeUnit.MILLISECONDS) | |||
| }.connectionSpecs(supportedConnectionSpecs()).build() | |||
| } | |||
| private val clearTextTrafficClient by lazy { | |||
| OkHttpClient.Builder().apply { | |||
There was a problem hiding this comment.
please do not create multiple instances of okHttp you can reuse the same one that is already in the singleton
There was a problem hiding this comment.
something like
fun buildClearTextTrafficOkhttpClient(): OkHttpClient = OkHttpSingleton.createNew {
connectionSpecs(listOf(ConnectionSpec.CLEARTEXT))
}and the private val clearTextTrafficClient by lazy { can be deleted
There was a problem hiding this comment.
The current okHttp client has RESTRICTED_TLS spec, I created another one that have CLEARTEXT spec to allow HTTP connection
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
|
| @@ -103,5 +103,9 @@ private fun OkHttpClient.Builder.ignoreAllSSLErrors() { | |||
|
|
|||
| fun supportedConnectionSpecs(): List<ConnectionSpec> { | |||
| val wireSpec = ConnectionSpec.Builder(ConnectionSpec.RESTRICTED_TLS).build() | |||
| return listOf(wireSpec, ConnectionSpec.CLEARTEXT) | |||
There was a problem hiding this comment.
The ConnectionSpec.CLEARTEXT was already added, so not sure why the new Http client is needed?
There was a problem hiding this comment.
because wireSpec has RESTRICTED_TLS
There was a problem hiding this comment.
Which will only allow connections through HTTPS
…l' into allow_http_call_when_checking_crl
MohamadJaara
approved these changes
Feb 14, 2024
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2491 +/- ##
=============================================
- Coverage 58.41% 58.38% -0.03%
Complexity 21 21
=============================================
Files 1172 1172
Lines 45313 45332 +19
Branches 4286 4287 +1
=============================================
- Hits 26470 26468 -2
- Misses 16920 16940 +20
- Partials 1923 1924 +1
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
ohassine
added a commit
that referenced
this pull request
Feb 14, 2024
…-6493) (#2491) * feat: allow http calls when checking Certificate Revocation List * chore: detekt * chore: unit test * fix: use of okhttp sharedClient instead of creating a new one * chore: unit test * chore: unit test * chore: add missing function for ios target --------- Co-authored-by: Mojtaba Chenani <chenani@outlook.com>
Merged
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Issues
We need to verify certificates by fetching Certificate Revocation List. This CRL can only be called on HTTP, it's already signed by the issuer so there is no security issue.
BUT by default, Android OS does not allow HTTP calls and throw an exception for that.
Solutions
In AndroidManifest, I allowed
usesCleartextTrafficand created in a new okhttp client that could be use on HTTP.Others calls are still called on HTTPS as we
okhttpClientwithConnectionSpec.RESTRICTED_TLSNeeds releases with:
Testing
Test Coverage (Optional)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.