Added SSL Validation Override and CA Selection #15
Conversation
- Added fields to login page to a) disable ssl validation or b) select a custom Certificate Authority certificate to use with the server. - Changed visibility of widgets on login page from INVISIBLE to GONE so they don't take up space while hidden (since this was causing weird spacing issues with the new fields). - Added state to settings to store ssl validation choice or certificate data. - Added fields to various HTTP methods to disable ssl validation or set valid certificate authority if either setting is enabled.
There was a problem hiding this comment.
Thanks for the contribution!
The "Disable ssl verification" option works well. The other option for selecting an own certificate I'll test tomorrow (having trouble creating a self-signed certificate with SAN on Windows).
The build (which is currently broken because of #17) would complain about the formatting, could you run gradlew spotlessApply to format all files?
- Moved certificate-related utilities to separate class - Added settings method to return an entire SSLSettings object; refactored methods using separate parameters to take single SSLSettings parameter - Advanced Settings section on login page now hides / shows along with other buttons to prevent it from showing up in front of the loading spinner - Fixed star imports - Refactored applySslSettings as per code from merge request - Fixed formatting
|
I think I covered all the changes request, also formatted using the spotlessApply. Let me know if you find anything else you want changed! |
There was a problem hiding this comment.
Selecting an certificate works well!
Images from applications currently doesn't load because Picasso doesn't use the certificates see https://stackoverflow.com/a/31860131/4244993
app/src/main/java/com/github/gotify/service/WebSocketConnection.java
Outdated
Show resolved
Hide resolved
- Removed comment - Moved SSLSettings to its own top-level class - Fixed picasso not setting SSL settings and failing to load images over self-signed connection
- Switched raw strings to string resources - Removed unused fields / views from LoginActivity - Reset 'Check Version' button text when changing SSL settings - Fixed formatting
|
I modified the MessagesActivity class to attempt to set the Picasso singleton instance onCreate with an instance that uses the app's SSL settings and it looks to be working for me. The alternative would be creating a custom holder for a Picasso instance and using that instead of the built-in instance, but that seems a bit messier. I don't know the lifetime of the Picasso instance (whether it's tied to the lifetime of the activity, or to the app), and since you can't change the singleton once it's been set, there might be an edge-case where someone logs out and logs back in, but uses the same SSL settings from the previous session. Not sure if this is likely enough to matter, though. |
There was a problem hiding this comment.
I don't know the lifetime of the Picasso instance (whether it's tied to the lifetime of the activity, or to the app), and since you can't change the singleton once it's been set, there might be an edge-case where someone logs out and logs back in, but uses the same SSL settings from the previous session. Not sure if this is likely enough to matter, though.
Good point, yeah this should definitly be fixed. How about creating a single non static instance in MessageActivity#onCreate and passing that to the ListMessageAdapter? (The Context constructor parameter can then be removed from the ListMessageAdapter). Like this a new instance will be created for every MessageActivity and after re-login it should use the new SSLSettings.
The rest looks great! After this fix I'll test it again on my phone and if all is alright then I'll merge this PR (:
|
Good idea, I'll go ahead and try that right now. I think the context still needs to be passed though, in order to inflate the view? I can just add another parameter for the picasso instance. EDIT: Added local instance to MessagesActivity which is passed ListMessageAdapter, seems to work fine for me. |
- MessagesActivity now constructs a new Picasso instance each onCreate and passes it to ListMessageAdapter
|
Thanks for your time! |
Added fields to login page to a) disable ssl validation or b) select
a custom Certificate Authority certificate to use with the server.
Changed visibility of widgets on login page from INVISIBLE to GONE so
they don't take up space while hidden (since this was causing weird
spacing issues with the new fields).
Added state to settings to store ssl validation choice or certificate
data.
Added fields to various HTTP methods to disable ssl validation or set
valid certificate authority if either setting is enabled.