Support for self-signed certificates #318
No reviewers
Labels
No Label
Priority-high
Accepting-merge-requests
API
API-dependency
Backport
Blocked
Brainstorming
Breaking
Bug
Changelog
CI
Cleanup
Confirmed
Discussion
Documentation
Duplicate
Enhancement
External-dependecy
F-droid
Feature
Google-play
Improvement
Invalid
Investigate
LGTM-done
LGTM-need
Long-term
Major-release
Minor-release
Needs-cleanup
Needs-feedback
Needs-help
Priority-critical
Priority-low
Priority-medium
Question
Ready
Refactor
Regression
Release
Repository
Security
Suggestion
Support
Testing
Translation
UI/UX
Upstream
Website
WIP
No Milestone
No Assignees
3 Participants
Due Date
No due date set.
Blocks
#345 Adding caching to picasso service
gitnex/GitNex
Reference: gitnex/GitNex#318
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch ":self-signed"
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?
This code is based on https://github.com/ge0rg/MemorizingTrustManager, but i had to change a lot because it is not well maintained.
@ -483,7 +481,6 @@ public class LoginActivity extends BaseActivity implements View.OnClickListener
if (response.isSuccessful()) {
if (response.code() == 200) {
Reason to remove this blank line?
@ -461,2 +458,3 @@
public void onFailure(@NonNull Call<GiteaVersion> callVersion, @NotNull Throwable throwable) {
Log.e("onFailure-version", throwable.toString());
}
Reason to remove this blank line?
@ -21,6 +21,7 @@ import android.widget.TextView;
import androidx.annotation.NonNull;
import androidx.appcompat.app.AlertDialog;
import com.tooltip.Tooltip;
import org.jetbrains.annotations.NotNull;
Should only import androidx NonNull
import androidx.annotation.NonNull;
instead@ -458,3 +458,1 @@
Log.e("onFailure-version", t.toString());
public void onFailure(@NonNull Call<GiteaVersion> callVersion, @NotNull Throwable throwable) {
Use NonNull
Changing
t
to throwable is not necessary@ -21,3 +26,3 @@
public class IssuesService {
public static <S> S createService(Class<S> serviceClass, String instanceURL, Context ctx) {
public static <S> S createService(Class<S> serviceClass, String instanceURL, Context context) {
ctx
can be kept as is.@ -61,0 +62,4 @@
Retrofit retrofit = builder.build();
return retrofit.create(serviceClass);
} catch(Exception e) {
catch should be in new line
@ -61,0 +63,4 @@
Retrofit retrofit = builder.build();
return retrofit.create(serviceClass);
} catch(Exception e) {
e.printStackTrace();
Please use Logger
Log.e();
@ -21,3 +26,3 @@
public class PullRequestsService {
public static <S> S createService(Class<S> serviceClass, String instanceURL, Context ctx) {
public static <S> S createService(Class<S> serviceClass, String instanceURL, Context context) {
Same comments apply as IssuesService
@ -54,2 +42,3 @@
sslContext.init(null, new X509TrustManager[]{memorizingTrustManager}, new SecureRandom());
Retrofit retrofit = builder.build();
OkHttpClient okHttpClient = new OkHttpClient.Builder().cache(cache).sslSocketFactory(sslContext.getSocketFactory(), memorizingTrustManager).hostnameVerifier(memorizingTrustManager.wrapHostnameVerifier(HttpsURLConnection.getDefaultHostnameVerifier())).addInterceptor(new Interceptor() {
Please keep the new lines as is and keep
//.addInterceptor(logging)
also for debugging purpose.@ -46,3 +49,1 @@
} else {
request = request.newBuilder().header("Cache-Control", "public, only-if-cached, max-stale=" + 60 * 60 * 24 * 30).build();
}
Please use if statement instead if possible.
@ -0,0 +2,4 @@
/**
* Author Georg Lukas, modified by anonTree1417
*/
New line after this
@ -0,0 +13,4 @@
import android.util.SparseArray;
import android.os.Build;
import android.os.Handler;
Please remove all blank lines between imports
@ -569,0 +572,4 @@
<string name="mtm_cert_expired">The server certificate is expired.</string>
<string name="mtm_accept_servername">Accept Mismatching Server Name?</string>
<string name="mtm_hostname_mismatch">Server could not authenticate as \"%s\". The certificate is only valid for:</string>
No blank line required here
@ -569,0 +575,4 @@
<string name="mtm_connect_anyway">Do you want to connect anyway?</string>
<string name="mtm_cert_details">Certificate details:</string>
Here too
@ -569,0 +579,4 @@
<string name="mtm_decision_always">Always</string>
<string name="mtm_decision_once">Once</string>
<string name="mtm_decision_abort">Abort</string>
And here too
Thanks for your contribution.
I have just commented only for the formatting and things which are redundant and can be removed like change ctx to context OR t to throwable etc.
You can read this wiki page https://gitea.com/gitnex/GitNex/wiki/Code-Standards to have better idea.
We also have code style to format the code(not all) for you, under Code -> Reformat code in AS.
Few other things to take note of are:
I haven't tested the functionality yet. Will do that soon.
pleace resolve confilcts
@mmarif Should be done with
d2190ad189
ande1ccb4e404
Resolved with
5ee872dc6e
Support for self-signed certificatesto WIP: Support for self-signed certificatesI have to work on some improvements.
Ok, please let us know once you are done with all the improvements, fixes, formatting etc.
WIP: Support for self-signed certificatesto Support for self-signed certificatesEverything i wanted to fix (and improve) is now done.
Error message when wrong port is given.
Both dialogs, if certificate is self-signed or hostname doesn't match.
Support for self-signed certificatesto WIP: Support for self-signed certificatesWIP: Support for self-signed certificatesto Support for self-signed certificatesAdded support for Picasso
0dda996444
Hi @anonTree1417
We will be removing extra services from clients. Saw that you added a new one. I would suggest to use RetrofitClient instead. #309
Also please test this PR against all cases like having ssl, http etc.
Out of curiosity, what scenario you have to use self signed certificate?
@mmarif
I dont think this is possible with Picasso, because RetrofitClient seems to be used to access plain(-text) data only, not images.
I have tested a lot already as you can already see in my screenshots.
I am using it for my personal server. I cant sign any certificates with Let'sEncrypt for example, because i dont have any domain (access via dynamic ip).
@ -0,0 +36,4 @@
.sslSocketFactory(sslContext.getSocketFactory(), memorizingTrustManager)
.hostnameVerifier(memorizingTrustManager.wrapHostnameVerifier(HttpsURLConnection.getDefaultHostnameVerifier()));
builder.downloader(new OkHttp3Downloader(okHttpClient.build()));
Picasso needs a downloader (in this case OkHttp3Downloader in conjunction with a OkHttpClient).
All these changes do also only apply if a self-signed certificate is used.
If there were any major bugs, they would most likely not occur when using servers with officially signed certificates.
But as already mentioned, i have tested everything so far.
Ok, cool then.
I will test later if you are done with it.
@mmarif I'm already done.
I have just tested this PR. Before I comment on things, I have some changes to push to your fork. Can you add me and @6543 to your forked repo.
But please make a new branch of self-signed. Then we can merge those two.
It should not be done that way. Normally it should be pushed to current branch, which then will automatically update upstream with new commit.
But anyway I have granted your wish. New branch is self-signed2.
Thanks.
Now comes to the suggestions/ideas etc.
What could be the problem here and what does it mean?
What is this Log.e(// important!!) for?
I have commented out this line and the app does work. What does this library do?
Please check the app without and see if it works. If so remove this library afterwards.
If I am disconnected, the cache does not work and app does not ask for the cert(when choose ONCE).
This could be just me, but I find
ONCE
annoying and unncessary. Almost every screen few API calls, the dialog keep poping up. To me Abort and Trust buttons are enough.What you think @6543
This is important one, does the cert saved inside app local space? In that case how to remove the cert if I don't need it anymore?
When an exception is thrown the object of Retrofit could be not fully initialized.
This line is very important for debugging. It prints on which URL the error occurred. This can be useful if the server was badly configured and the configured host is different from your current host.
#316 (comment)
This behaviour is related to how the system works. But i can remove that. I shouldnt matter for the average user.
eg.:
Thats fine then.
If it's only for debugging purpose, then for actual code it should be commented out like how logging intercepter is commented.
Does disabling this library gives you any error? Becuase it does not for me.
Yes, Abort and Trust are enough.
Please see my changes and let me know if you have any question.
Your changes are fine. I will merge them.
Ok.
I will test that again.
@mmarf Ok, i have implemented your wishes. The only thing i am struggling with at the moment is the library. Everything works for me, even if the library is missing, BUT if its missing, exceptions get thrown nevertheless. Could you check if your stacktrace shows similar signs in that case?
It does not throw any error. Aside that it does add 5MB to the package size of the APK though.
I would suggest to remove it.
Also comment out the 2nd Log.e.
Log.e("PicassoService", exception.toString());
Once done, I will test again.
Ok, done.
@ -178,9 +178,9 @@ public class IssueCommentsAdapter extends RecyclerView.Adapter<IssueCommentsAdap
}
if (currentItem.getUser().getAvatar_url() != null) {
@mmarif this need a refactor! if () do x else do same ?!?
This issue is not related to this pull request.
looked throu the code ...
@ -182,9 +182,9 @@ public class IssuesAdapter extends RecyclerView.Adapter<RecyclerView.ViewHolder>
}
if (issuesModel.getUser().getAvatar_url() != null) {
same useless if statement here
This issue is not related to this pull request.
I know butit would fit in ...
You can send an extra PR if you like to divide stuff
I dont know whether mixing these different things up would really fit.
Maybe a thing for another pull reuqest?
@anonTree1417 you could add a refacotr to
IssueCommentsAdapter.java
andIssuesAdapter.java
both have a if function witch executes the same codeif ( true) { A(); } else { A(); }
==A()
@6543 This issue is not related to this pull request. (again)
It would result in a merge confilct if I create a pr for it ...
Why? I can resolve those...
Just create another pull request and i will resolve any related conflicts
I found nothing related to your code so this pr should be fine - but didn't test it against local-noSSL; local-SSL, global-validSSL
and one function I miss: clear manualy-acepted certs in app-settings
EDIT: can you add a smal config menue to clear the cache or eafen show whats stored i int to the app-congis?
@6543 Hope, youre happy now.
abdfe8d9d1
b9eee16294
@anonTree1417 great work.
I will test later with the new changes you added. If anything I will push to self-sign2 branch.
Im resolving all conflicts now.. Please pull all changes to self-signed2 branch.
@anonTree1417 tested this is comming to an end :) and yes the option for the config menue is great!!
only one bug I found: "you have to hit login 3 times to pass"
video: https://cloud.obermui.de/s/ajLaBXPRkzMacLZ
@anonTree1417 I have just pushed few changes. Please merge for upstream. The rest is all good.
Cache is still not working but maybe that will be for another day.
Just noticed the same thing as @6543 mentioned. If this can be fixed, we are good for merging this.
Should be fixed with
59e8f6f94a
What kind of cache? (Picasso, Retrofit)
Is this related to my changes?
In fact, caching of picasso does not work. Everything else should do.
Retrofit.
If you off internet whatever have you browsed while on internet you are able to browse them.
This from my testing does not work here.
Why is the cache not working then?
It does work. Didnt think of picasso's own, indipendent cache.
I have uploaded a sample video to discord develop chat room.
It does not work only with self signed certs logins.
@mmarif
Errors thrown with self-signed cert and when offline.
Additional errors
HTTP 504 sounds like a gateway timeout
https://stackoverflow.com/questions/42927216/retrofitokhttp-http-504-unsatisfiable-request-only-if-cached
:) Thank you @6543 for reviewing.
@mmarif cache works fine (on vallid ssl or http mode), only avatar pictures are not cached <- witch is also on master the case (#344)
And if you are using ssl with self generated cert it wont cache ... cant find the reason jet but this should not block this pull
I will fix that.
Thank you @anonTree1417 for the work on this.