-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Check python version and enable https where it's possible #2307
Conversation
@sampsyo Do we have a minimum supported Python 3 version? (SNI wasn't included in Python 3.0.0 either, so the check might need to check for early versions of Py3 as well… Depending on minimum supported Py3 version.) |
|
||
VARIOUS_ARTISTS_ID = '89ad4ac3-39f7-470e-963a-56509c546377' | ||
BASE_URL = 'http://musicbrainz.org/' | ||
|
||
if sys.version_info > (2, 7, 9): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comparison should be "greater than or equal to", since 2.7.9 also has SNI support.
My best guess from this thread is that 3.2+ supports SNI. Fortunately, 3.2 is below our minimum required Py3 version. Would you mind defining a |
Current coverage is 76.85% (diff: 75.00%)@@ master #2307 diff @@
==========================================
Files 65 65
Lines 13220 13223 +3
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 10159 10163 +4
+ Misses 3061 3060 -1
Partials 0 0
|
OMG I wish @codecov-io would stop commenting. 😠 This is looking great so far! The last thing we'll need once all the endpoint changes are in place is to update the tests. Some tests use mocking to return fake responses to URL requests; these typically use hard-coded URLs. We'll need to change the tests to return the mocked response for both HTTP and HTTPS versions of each URL. |
I meant to suggest that if ui.SNI_SUPPORTED: instead of this: if ui.SNI_SUPPORTED >= (2, 7, 9): This variable should also probably reside in the |
|
Yes, please. |
This reverts commit f60c911.
Add SNI_SUPPORTED
Add SNI_SUPPORTED Add SNI_SUPPORTED
Okay, so, what shall we do with tests? |
OK, let's take a look! The rough prescription is:
It would also be nice to ensure that all the endpoints actually do support HTTPS. That just involves running each piece of functionality to make sure it still works when contacting the server over HTTPS. |
Hi, @sampsyo. Can you give me some comments on my work? I have never written unit tests before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The progress looks great; thanks for looking at the tests! I've added a couple of comments here.
To keep the conversation going, the next step (once you have a set of changes ready) should be to look at the test failures. If you see anything obvious, you can fix that stuff right away. If you see failures you can't explain, please feel free to ask about them—point me toward the test you don't understand and I'll take a closer look.
Finally, we should remember to check that these endpoints actually support SSL. It would also be good to verify which of them use SNI—for ones that don't, we can use SSL without a version check.
self.source.fetch_image(self.candidate, self.extra) | ||
self.source.fetch_image(self.candidate_https, self.extra) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this test controls the URL on "both ends," i.e. the request and the response, I'm not sure it needs updating. Was it failing before?
AAO_URL_HTTPS = 'http://www.albumart.org/index_detail.php?asin={0}' \ | ||
.format(ASIN) | ||
CAA_URL_HTTPS = 'http://coverartarchive.org/release/{0}/front' \ | ||
.format(MBID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can avoid copying and pasting here? For example, we could put most of the URLs (starting with ://
) into a variable and then concatenate on the https
or http
part separately.
album = _common.Bag(asin=self.ASIN) | ||
self.plugin.art_for_album(album, [self.dpath]) | ||
self.assertEqual(len(responses.calls), 1) | ||
self.assertEqual(responses.calls[0].request.url, self.AMAZON_URL) | ||
self.assertEqual(responses.calls[0].request.url, self.AMAZON_URL_HTTPS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to assert that the requested URL is equal to two different URLs, which probably isn't what you meant—instead, you probably want to write an assertion that says that the requested URL is either the HTTP one or the HTTPS one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The next couple of tests seem to have the same issue.)
Hi, @sampsyo. Sorry for delay. I found that amazon and albumart.org don't support https, so I removed https urls for them. |
No worries, and thank you! It looks like Travis still has a few style/error reports: https://travis-ci.org/beetbox/beets/jobs/182870011#L848-L853 It also looks like there might be a typo in some URL, where there's "htts" instead of https? https://travis-ci.org/beetbox/beets/jobs/182870006#L913 |
It's strange, because I fixed typo in one of my previous commits b65a7da |
Got can behave in mysterious ways. 👻 Those flake8 errors shouldn't be hard to address either. |
Okay, my numerous commits look ridiculous. I should do something with it. |
It's cool; I don't have anything against long series of commits. But if you like cleaner histories, the terms to google are [git squash]. |
Yeah, that's cool too. |
It would be great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of duplicating hardcoded urls in if/else branches, why not write :
PROXY_URL = '%s://example.com' % 'https' if util.SNI_SUPPORTED else 'http'
if util.SNI_SUPPORTED: | ||
API_URL = 'https://ws.audioscrobbler.com/2.0/' | ||
else: | ||
API_URL = 'https://ws.audioscrobbler.com/2.0/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https => http ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's less readable.
I did a little audit using the `openssl` command-line tool to find the servers that don't require SNI. Here's what I found: icbrainz.org: SNI images.weserv.nl: inconclusive, but docs say yes SNI coverartarchive.org: SNI webservice.fanart.tv: *no* SNI dbpedia.org: *no* SNI en.wikipedia.org: *no* SNI ws.audioscrobbler.com: *no* SNI api.microsofttranslator.com: *no* SNI In summary, *only* MusicBrainz and CoverArtArchive were found to require SNI. So I'm using SSL unconditionally on all the other sites.
We don't need quite so many checks now that SSL isn't conditional most of the time.
Merged with a few changes. Thank you! ✨ |
No description provided.