Tentative self signed ssl support #141
No reviewers
Labels
No Label
Difficulty - Beginner friendly
Difficulty = Easy
Difficulty = Hard
Difficulty = Medium
Priority = CRITICAL
Priority = High
Priority = Low
Priority = Normal
Priority = Some day
Status - Fixed somewhere else
Status - No details provided
Status = Can't fix
Status = Duplicate
Status = Help wanted
Status = Invalid
Status = Need more details
Status = Taken
Status = Wontfix
Type - Selfoss works like this
Type = Bug
Type = Chore
Type = Enhancement
Type = Feature
Type = Question
Type = SELFOSS API ISSUE
Type = Tools
Type = UX/Design
Up For Grabs
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Louvorg/ReaderForSelfoss-multiplatform#141
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "davidoskky/ReaderForSelfoss-multiplatform:self_ssl"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Types of changes
Idea on implementing the support for self signed certificates. This has to be done separately on Android and IOS due to the different http clients. I'm unsure if this approach currently trusts any certificate for any domain and thus even certificates from domains different from the one we are accessing or just trusts any authority but does check that the domain is the correct one.
@AmineL, let me know if this is an appropriate approach and I shall introduce a switch at login time to choose between the normal behavior and this.
3ac8adfa5c
to7a5e2f1155
@davidoskky we should definitely have a switch at login for this.
The switch should also be pre-checked for api 24 users with a text below explaining why this is needed, or let the user handle things server side.
7a5e2f1155
to4e0117877d
Eventually got around completing this...
I tested it on API 21 and it works, however the SSL problem is also present in Glide and thus images are not currently displayed with this fix for people who had this problem before.
I'm sorry for mixing in the gradle upgrade in this PR.
4e0117877d
tob793301706
WIP: Tentative self signed ssl supportto Tentative self signed ssl support@davidoskky there is a build issue. Could you check this, please ?
I cannot reproduce the build error, it compiles correctly on my system. Did you try rebuilding the project? It definitely is something related with the gradle upgrade.
I relaunched the build on the CI and the error is still here.
You can see that here
f900d456a1
to042089e78a
042089e78a
to3646658cb1
Fixed the build problem
@ -60,3 +59,2 @@
// Flag to enable support for the new language APIs
sourceCompatibility = JavaVersion.VERSION_11
targetCompatibility = JavaVersion.VERSION_11
sourceCompatibility = JavaVersion.VERSION_17
Is the java version upgrade necessary for something ?
I will check, I did upgrade for a reason related to the gradle upgrade but didn't note down the exact motivation.
I have checked, it is not necessary. I can revert it if it is preferable.
Yes, please.
@ -83,3 +83,3 @@
val sourceGroup = binding.sourcesGroup
repository.getSourcesDetailsOrStats().forEach { source ->
repository.getSourcesDetailsOrStats().forEachIndexed { _, source ->
Why use the
forEachIndexed
, but don't use the index ? (same for the one just after)This had to do with forEach not being supported on older Android versions.
@ -54,0 +55,4 @@
android:id="@+id/selfSigned"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:text="@string/disableSSL"
disableSSL
should bedisable_ssl
@ -1,5 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<resources xmlns:tools="http://schemas.android.com/tools">
Please add back all the deleted
xmlns:tools="http://schemas.android.com/tools"
.This will cause issues with the translation tool.
@ -130,4 +130,5 @@
<string name="update_source">Update source</string>
<string name="confirm_disconnect_title">Disconnect ?</string>
<string name="confirm_disconnect_description">You will be disconnected from your selfoss instance.</string>
<string name="disableSSL">Disabilita SSL</string>
Translations should be done on the translation tool. This will be overwritten when merged.
@ -0,0 +52,4 @@
.build()
}
}
install(ContentNegotiation) {
From this point on, everything should work the same for ios. Is there a way to handle this better ?
I believe I can refactor it so that most things stay in common code.
These change break ios client handling. Please keep in mind to not break ios handling with the changed you make.
Indeed, I'll try to see if there is any way to use a shared http client among Android and iOS; as far as I understood okHTTP can only be used in Android; I can check if any of the other ones supported by ktor is multiplatform.
I have reverted to Java 11. I have substituted forEachIndexed with forEach; it builds and works on emulated android 21 but the CI build fails.
The CI build was already failing before. Your changes were not the cause of this.
I'm now using the correct android gradle plugin version; that was the reason of the previously failed builds.
The CI build failed but didn't provide any reason for the failure.
@davidoskky I restarted the build, it's now working.
I'll re review it pretty soon
@ -142,3 +144,4 @@
repository.refreshLoginInformation(url, login, password)
CoroutineScope(Dispatchers.Main).launch {
repository.updateApiInformation()
repository.updateApiInformation()
is called ingoToMain
which is called at line 152. This should not be needed.Updating the api information is required for the following login call. This can probably be fixed by restructuring the code of the login page.
@ -54,0 +55,4 @@
android:id="@+id/selfSigned"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:text="@string/disable_ssl"
@string/disable_ssl
should be named@string/use_self_signed_cert
As I understand it, this would be misleading because we are not just accepting self signed certificates but rather disregarding ssl validation all together. The SSL certificate may very well be for another domain or missing altogether.
@ -130,4 +130,5 @@
<string name="update_source">Update source</string>
<string name="confirm_disconnect_title">Disconnect ?</string>
<string name="confirm_disconnect_description">You will be disconnected from your selfoss instance.</string>
<string name="disable_ssl">Disable SSL</string>
<string name="disable_ssl">Disable SSL</string>
=><string name="use_self_signed_cert">Use a self signed certificate</string>
in all the files.@ -1,5 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<resources xmlns:tools="http://schemas.android.com/tools">
Please revert this change.
@ -1,5 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>
<resources xmlns:tools="http://schemas.android.com/tools">
No idea why I added this file. Can you please delete it ?
@ -27,0 +37,4 @@
fun createHttpClient(
appSettingsService: AppSettingsService,
api: SelfossApi
Why is this needed ? Can't it be replaced by
this
?Why was gradle and all the dependencies updated in this PR ?
Updating gradle forces to use java 17, so I'm unable to test this.
Can you revert all the version updates ? They should be done in a separate PR.
0aac2cba0b
to4482234e1a
I was for some reason convinced that the gradle upgrade was necessary due to some dependency. It was not. Since I've written all the upgrade related stuff already I'll submit the PR soon.
@ -60,2 +64,3 @@
dependencies {
implementation("io.ktor:ktor-client-okhttp:2.1.1")
implementation("com.squareup.okhttp3:okhttp:4.10.0")
implementation("io.ktor:ktor-client-okhttp:2.2.4")
There are still version changes. Can you please revert them ?
@ -36,4 +51,3 @@
prettyPrint = true
isLenient = true
ignoreUnknownKeys = true
explicitNulls = false
Why was this removed ?
@ -91,2 +101,2 @@
private fun shouldHavePostLogin() = appSettingsService.getApiVersion() != -1
private fun hasLoginInfo() =
fun shouldHavePostLogin() = appSettingsService.getApiVersion() != -1
fun hasLoginInfo() =
These two should be reverted to private.
@ -1,5 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<resources xmlns:tools="http://schemas.android.com/tools">
Please revert this.
@ -29,13 +29,17 @@ kotlin {
sourceSets {
val commonMain by getting {
dependencies {
implementation("io.ktor:ktor-client-core:2.1.1")
Please revert this
Do you mean the whole version upgrade?
Yes please