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

Support for HTTPS TLS connections on older devices #332

Merged
merged 4 commits into from
Feb 12, 2017
Merged

Support for HTTPS TLS connections on older devices #332

merged 4 commits into from
Feb 12, 2017

Conversation

marcopar
Copy link
Contributor

@marcopar marcopar commented Feb 11, 2017

Devices with SDK version between 16 and 20 (and even some on 21) support TLS but TLS is not enabled by default.
Many websites are switching to HTTPS requiring the use of TLS (i.e. NASA sites)

This patch adds a factory for creating OkHttpClient with TLS enabled on those SDK versions.
It then modifies DownloadArtTask so it uses that factory.

Without this patch Muzei fails to download images from those websites on those devices because with TLS disabled the SSL handshake fails.

Background:
For example it fails to download images using my muzei source that fetches images from the NASA / SDO project website that switched to TLS at the end of 2016.
https://github.com/marcopar/SDOViewer
It is possible to reproduce the problem running Muzei and my source on a API19 emulator instance.

This patch has been built following the indications on this OkHttp tracker:
square/okhttp#2372
As you can see the issue is referred by several other trackers recognizing the problem.

@marcopar
Copy link
Contributor Author

I adapted the implementation to somewhat match the one included in Facebook React, that is based on the same source but i think it's more polished and enables support also for TLS 1.1.
See:
facebook/react-native#9840

Bonus: If they find some bug it will be easier to merge the fixes.

Copy link
Collaborator

@ianhanniballake ianhanniballake left a comment

Choose a reason for hiding this comment

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

Could you move the OkHttpClientFactory and TLSSocketFactory classes to the sync package? Thanks for the pull request!

package com.google.android.apps.muzei;

/**
* Created by marcopar on 11/02/17.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add the copyright/licensing block please

package com.google.android.apps.muzei;

/**
* Created by marcopar on 11/02/17.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this comment block

@@ -0,0 +1,105 @@
package com.google.android.apps.muzei;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add the copyright/licensing block please

/**
* Factory for OkHttpClient, supports the creation of clients enabling TLS on devices where it's not enabled by default (mainly pre lollipop)
*/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this extra line


return client;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove extra lines

* True if enabling TLS is needed on current device (SDK version >= 16 and < 22)
*/
public static boolean isTLSEnableNeeded() {
if (Build.VERSION.SDK_INT >= 16 && Build.VERSION.SDK_INT < 22) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the Build.VERSION_CODES constants

@marcopar
Copy link
Contributor Author

Request changes pushed.

@ianhanniballake ianhanniballake merged commit 44ba5c1 into muzei:master Feb 12, 2017
@ianhanniballake
Copy link
Collaborator

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants