Issue 4549 - Implement the Windows Store API to upload development builds

sitescripts/extensions/bin/createNightlies.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2016 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 """
 
 Nightly builds generation script
 ================================
 
   This script generates nightly builds of extensions, together
   with changelogs and documentation.
 
 """
 
 import ConfigParser
 import base64
-from datetime import datetime
 import hashlib
 import hmac
 import json
 import logging
 import os
 import pipes
 import random
 import shutil
 import struct
 import subprocess
 import sys
 import tempfile
 import time
 from urllib import urlencode
 import urllib2
 import urlparse
-import httplib
 import zipfile
 import contextlib
 
 from xml.dom.minidom import parse as parseXml
 
 from sitescripts.extensions.utils import (
     compareVersions, Configuration,
     writeAndroidUpdateManifest
 )
 from sitescripts.utils import get_config, get_template
 
 MAX_BUILDS = 50
+
+
+# Google and Microsoft APIs use HTTP error codes with error message in
+# body. So we add the response body to the HTTPError to get more
+# meaningful error messages.
+class HTTPErrorBodyHandler(urllib2.HTTPDefaultErrorHandler):
+    def http_error_default(self, req, fp, code, msg, hdrs):
+        raise urllib2.HTTPError(req.get_full_url(), code,
+                                '{}\n{}'.format(msg, fp.read()), hdrs, fp)
 
 
 class NightlyBuild(object):
     """
       Performs the build process for an extension,
       generating changelogs and documentation.
     """
 
     def __init__(self, config):
         """
@@ skipping 400 matching lines @@
         try:
             urllib2.urlopen(request).close()
         except urllib2.HTTPError as e:
             try:
                 logging.error(e.read())
             finally:
                 e.close()
             raise
 
     def uploadToChromeWebStore(self):
-        # Google APIs use HTTP error codes with error message in body. So we add
-        # the response body to the HTTPError to get more meaningful error messages.
-
-        class HTTPErrorBodyHandler(urllib2.HTTPDefaultErrorHandler):
-            def http_error_default(self, req, fp, code, msg, hdrs):
-                raise urllib2.HTTPError(req.get_full_url(), code, '%s\n%s' % (msg, fp.read()), hdrs, fp)
 
         opener = urllib2.build_opener(HTTPErrorBodyHandler)
 
         # use refresh token to obtain a valid access token
         # https://developers.google.com/accounts/docs/OAuth2WebServer#refresh
 
         response = json.load(opener.open(
             'https://accounts.google.com/o/oauth2/token',
 
             urlencode([
@@ skipping 36 matching lines @@
         request.get_method = lambda: 'POST'
         request.add_header('Authorization', auth_token)
         request.add_header('x-goog-api-version', '2')
         request.add_header('Content-Length', '0')
 
         response = json.load(opener.open(request))
 
         if any(status not in ('OK', 'ITEM_PENDING_REVIEW') for status in response['status']):
             raise Exception({'status': response['status'], 'statusDetail': response['statusDetail']})
 
-    def get_response(self, connection,
-                     method, url, body=None, headers={}):

[Sebastian Noack, 2017/02/09 12:51:57]

Using mutable types as default arguments is rather dangerous and confusing,
since they are initialized on function definition, not on function call.

  def f(k, d={})
      d[k] = True
      return d

  f('foo')             => {'foo': True}
  f('bar')             => {'foo': True, 'bar': True}
  f('foo') is f('bar') => True

I cannot even tell whether that is a problem in practice here, without looking
at the code of HTTPSConnection.request(). But it's generally wise to just avoid
mutable types in default arguments. Here, it would even reduce the code:

  def get_response(self, connection, *args):
      connection.request(*args)
      ...

[Oleksandr, 2017/02/13 02:57:36]

Done.

-        connection.request(method, url, body, headers)
-        response = connection.getresponse()
-        if (response.status >= 300):

[Sebastian Noack, 2017/02/09 12:51:57]

The parentheses here are redundant, flake8-abp would have complained about that.
So by now I'm pretty sure that our flake8 configuration isn't in effect in your
environment. Note that tox.ini lists some flake8 extensions (including ours
flake8-abp) as dependency. If you run tox, it will setup a virtualenv where it
installs all dependencies, and then runs flake8 (and our tests). So even if VS
Code has flake8 integration, outside of tox, you'd have to install flake8
extensions manually/globally.

[Sebastian Noack, 2017/02/09 12:51:57]

Shouldn't we rather check for non-2xx? In Python this can be done pretty easily:

  if 200 <= response.status < 300:
      ...

[Vasily Kuznetsov, 2017/02/09 16:37:58]

Your condition seems to check for 2xx instead of non-2xx :)

Another thing is: Httplib doesn't seem to handle redirects and things like this.
Could this create a problem for us, if some API calls would try redirecting us
or is this not a real possibility?

[Sebastian Noack, 2017/02/09 20:50:23]

Would it be a problem if a new connection is used for each request? If not,
perhaps we should consider just using urllib2, it resolves redirects
automatically, and already raises an exception for non-2xx responses.

[Oleksandr, 2017/02/13 02:57:36]

Original thinking is that we do not handle HTTP 3xx responses (redirects) so
raise exception as if it was and error. For HTTP 1xx responses aren't really
defined by HTTP standard so we just assume go along if they do happen.
Currently it doesn't create any problems. I, in fact considered not handling
redirects as a good thing. If there's ever an API change I think we should know
about it, rather than blindly redirecting. 
There are no problems to create a new connection for each call AFAIK. Using one
connection just feels more robust. I don't feel strong about this, but wouldn't
it be better to move to Requests (or urllib3) not urllib2 then? Those support
Keep-Alive at least. 

[Vasily Kuznetsov, 2017/02/13 10:44:13]

I think I would prefer to just use urllib2 too because it's in standard library
and the code would be simpler. It might be a bit slower because of no keepalive,
but if it works, I don't think the loss of performance would be material for
this script.

[Sebastian Noack, 2017/02/14 12:36:00]

Well, the HTTP status codes 100, 101 and 102 are defined in the standard, and
none of them is expected here. So raising an exception in that case (like
urllib2 would do) makes sense.
Well, a redirect doesn't necessarily indicate a permanent relocation of the API.
In fact there is a dedicated status code (307) specifically for temporary
redirects. One example is where you get redirected to a random server so that
the load is distributed between several servers. If they don't already do
something like that, it might be unlikely that they will do at some point. But
if you don't handle redirects you are implementing an incomplete HTTP client.
Well robust, seems to be the wrong word, given that status codes and redirects
aren't handled automatically. Not to mention, what happens if the server closes
the connection, before we are done?

The problem with Requests is a largely leaky abstraction. Most notably
connections are leaked in an LRU cache, until either the process terminates or
the capacity of the connection pool is exceeded. urllib3 gives you control over
the connection pooling at least, but it has other issues that Requests, which
uses urllib3 under the hood, attempts to work around. That said, these libraries
are kind of a mess, and generally I'd like to avoid third-party libraries.

While reusing the connection would be nice, it doesn't seem to justify extra
complexity or even third-party libraries. Also note that the Chrome Webstore
upload, which is quite similar, is using urllib2 as well, and I'm quite happy
with that code. So consistency might be another argument for urllib2 here.

-            raise Exception({'status': response.status,
-                             'statusDetail': response.reason})
-        return response.read().decode()

[Sebastian Noack, 2017/02/09 12:51:57]

I looked at the code of httplib, and indeed responses need to be closed. That
might seem counter-intuitive, as the connection is reused, and the response
merely represents a chunk of data retrieved over that connection. But each
response has its own file descriptor, created with socket.makefile(), which
reads data from the underlying connection.

You might have figured that it also works if you don't close the response. That
is because file descriptors are automatically closed when the script terminates,
and this script opens less than the maximum number of file descriptors on your
operating system. Even before the script terminates, some file descriptors may
be closed while the garbage collector calls the __del__ method of unreferenced
file objects. However, this can quickly become a problem if this script will
have to deal with more file descriptors in the future, or if we should ever use
this code in the context of a long-running service which usually does not
terminate, or if we use a different Python implementation that does not support
__del__, or if we run it in an environment with a lower maximum number of file
descriptors. So always make sure, to close any resources as soon as possible.

[Sebastian Noack, 2017/02/09 12:51:57]

It seems wherever get_repsonse() is called, and the return-value/response isn't
ignored, JSON is expected. So how about parsing the JSON here, rather than
duplicating json.loads() 4 times?

[Vasily Kuznetsov, 2017/02/09 16:37:58]

You're right, it does have a close() method indeed (that wasn't very obvious
from the docs:
https://docs.python.org/2/library/httplib.html#httpresponse-objects). However,
looking at the source code as well, it seems that `response.read()` would close
the response by itself so there's no need to close it.

[Sebastian Noack, 2017/02/09 20:50:23]

The code is rather complex with a lot of special cases. But it seems the
underlying file descriptor is at least closed automatically when the complete
response has been read. So in the current patch, we still leak file descriptors,
because we not always read from the response. This should no longer happen when
we parse the returned json no matter what, as I suggested in another comment.
But I still feel that it's cleaner and more robust if we close it explicitly.

[Vasily Kuznetsov, 2017/02/09 21:01:55]

Yeah, I was assuming always parsing, I also think it's a good idea. Also I think
I will agree with you about closing the response after reading just to be on the
safe side and not have to understand all that code in httplib.

What bothered me before about closing it is that `HTTPResponse.close` is not
even mentioned in the documentation so technically it could be removed. Having
looked at how things are in Python 3, I don't think this is a real issue. There
the responses inherit some generic stream class from `io` module and therefore
would have a `close` method.

[Oleksandr, 2017/02/13 02:57:36]

There is no json response for deleting a submission and for updating it. That is
why I opted for not including json.loads here.

[Oleksandr, 2017/02/13 02:57:36]

It is not possible to reuse the connection object until the response is read. In
the worst case scenario we may forget to read exactly one response, which
implies it is not closed by a read operation. But even then it will be closed
when the connection is closed. So I still don't see a reason for manual response
closes.

[Sebastian Noack, 2017/02/14 12:36:00]

I see. So if json.load() would fail for delete/update actions, I guess, it makes
sense to just return the plain response text, rather than magically handling
that special case, here.

[Sebastian Noack, 2017/02/14 12:36:00]

Good point. So it seems, in fact, there isn't any scenario where, the response's
file descriptor isn't automatically closed (if the connection is closed). Also
considering that the response's close() method is undocumented in Python 2, not
explicitly closing it seems reasonable, if we'd stick with httplib.

-
     def get_windows_store_access_token(self):
-
-        auth_token = ''
         # use refresh token to obtain a valid access token
         # https://docs.microsoft.com/en-us/azure/active-directory/active-directory-protocols-oauth-code#refreshing-the-access-tokens
-        token_server = 'login.microsoftonline.com'
-        with contextlib.closing(
-                httplib.HTTPSConnection(token_server)) as tokenConnection:
-
-            # Get access token
-            token_path = '/{}/oauth2/token'.format(self.config.tenantID)
-            token_response = json.loads(self.get_response(
-                tokenConnection, 'POST', token_path,
-                urlencode([
-                    ('refresh_token', self.config.refreshToken),
-                    ('client_id', self.config.clientID),
-                    ('client_secret', self.config.clientSecret),
-                    ('grant_type', 'refresh_token'),
-                    ('resource', 'https://graph.windows.net')
-                ])))
-            auth_token = '{0[token_type]} {0[access_token]}'.format(
-                token_response)

[Sebastian Noack, 2017/02/09 12:51:57]

Nit: If you just call the variable "response", you don't have to wrap this line.
And since there is no other response in this context, there shouldn't be any
ambiguity.

[Oleksandr, 2017/02/13 02:57:36]

Done.

+        server = 'https://login.microsoftonline.com'
+        token_path = '{}/{}/oauth2/token'.format(server, self.config.tenantID)
+
+        opener = urllib2.build_opener(HTTPErrorBodyHandler)
+        post_data = urlencode([
+            ('refresh_token', self.config.refreshToken),
+            ('client_id', self.config.clientID),
+            ('client_secret', self.config.clientSecret),
+            ('grant_type', 'refresh_token'),
+            ('resource', 'https://graph.windows.net')
+        ])
+        request = urllib2.Request(token_path, post_data)
+        with contextlib.closing(opener.open(request)) as response:
+            data = json.load(response)
+            auth_token = '{0[token_type]} {0[access_token]}'.format(data)
 
         return auth_token
 
     def upload_appx_file_to_windows_store(self, file_upload_url):
-
         # Add .appx file to a .zip file
         zip_path = os.path.splitext(self.path)[0] + '.zip'
         with zipfile.ZipFile(zip_path, 'w', zipfile.ZIP_DEFLATED) as zf:
             zf.write(self.path, os.path.basename(self.path))
 
         # Upload that .zip file
         file_upload_url = file_upload_url.replace('+', '%2B')
-        parts = httplib.urlsplit(file_upload_url)
-        with contextlib.closing(
-                httplib.HTTPSConnection(parts.netloc)) as file_upload_con:
-            file_headers = {'x-ms-blob-type': 'BlockBlob'}
-            file_upload_con.request('PUT', '{}?{}{}'.format(

[Vasily Kuznetsov, 2017/02/09 16:37:58]

Maybe combining the url fragments on a separate line would make this
construction a bit more compact and readable?

[Oleksandr, 2017/02/13 02:57:36]

Done.

-                                        parts.path,
-                                        parts.query,
-                                        parts.fragment),
-                                    open(zip_path, 'rb'), file_headers)
-            file_upload_con.getresponse().read()
-
+        request = urllib2.Request(file_upload_url)
+        request.get_method = lambda: 'PUT'
+        request.add_header('x-ms-blob-type', 'BlockBlob')
+
+        opener = urllib2.build_opener(HTTPErrorBodyHandler)
+
+        with open(zip_path, 'rb') as file:
+            request.add_header('Content-Length',
+                               os.fstat(file.fileno()).st_size - file.tell())
+            request.add_data(file)
+            opener.open(request).close()
+
+    # Clone the previous submission for the new one. Largely based on code
+    # from https://msdn.microsoft.com/en-us/windows/uwp/monetize/python-code-examples-for-the-windows-store-submission-api#create-an-app-submission
     def upload_to_windows_store(self):
-
-        auth_token = self.get_windows_store_access_token()
-
-        # Clone the previous submission for the new one. Largely based on code
-        # from https://msdn.microsoft.com/en-us/windows/uwp/monetize/python-code-examples-for-the-windows-store-submission-api#create-an-app-submission
-        headers = {'Authorization': auth_token,
+        opener = urllib2.build_opener(HTTPErrorBodyHandler)
+
+        headers = {'Authorization': self.get_windows_store_access_token(),
                    'Content-type': 'application/json'}
 
-        api_server = 'manage.devcenter.microsoft.com'
-        with contextlib.closing(
-                httplib.HTTPSConnection(api_server)) as connection:
-
-            # Get application
-            # https://docs.microsoft.com/en-us/windows/uwp/monetize/get-an-app
-            api_path = '/v1.0/my/applications/{}'.format(
-                self.config.devbuildGalleryID)
-            app_obj = json.loads(self.get_response(connection, 'GET',
-                                                   api_path, '', headers))
-
-            # Delete existing in-progress submission
-            # https://docs.microsoft.com/en-us/windows/uwp/monetize/delete-an-app-submission
-            submissions_path = api_path + '/submissions'
-            if 'pendingApplicationSubmission' in app_obj:
-                remove_id = app_obj['pendingApplicationSubmission']['id']
-                self.get_response(connection, 'DELETE',
-                                  '%s/%s' % (submissions_path, remove_id),
-                                  '', headers)
-
-            # Create submission
-            # https://msdn.microsoft.com/en-us/windows/uwp/monetize/create-an-app-submission
-            submission = json.loads(self.get_response(
-                                        connection, 'POST',
-                                        submissions_path, '', headers))
-
-            submission_id = submission['id']
-            file_upload_url = submission['fileUploadUrl']
-
-            # Update submission
-            old_submission = submission['applicationPackages'][0]
-            old_submission['fileStatus'] = 'PendingDelete'
-            submission['applicationPackages'].append(
-                {'fileStatus': 'PendingUpload'})
-            added_submission = submission['applicationPackages'][1]
-            added_submission['fileName'] = os.path.basename(self.path)
-
-            old_min_sys_ram = old_submission['minimumSystemRam']
-            added_submission['minimumSystemRam'] = old_min_sys_ram
-
-            old_directx_version = old_submission['minimumDirectXVersion']
-            added_submission['minimumDirectXVersion'] = old_directx_version
-
-            new_submission_path = '{}/{}'.format(
-                submissions_path, submission_id)
-                
-            self.get_response(connection, 'PUT', new_submission_path,
-                              json.dumps(submission), headers)
-
-            self.upload_appx_file_to_windows_store(file_upload_url)
-
-            # Commit submission
-            # https://msdn.microsoft.com/en-us/windows/uwp/monetize/commit-an-app-submission
-            submission = json.loads(self.get_response(connection, 'POST',
-                                    new_submission_path + '/commit',
-                                    '', headers))
-
-            if submission['status'] != 'CommitStarted':
-                raise Exception({'status': submission['status'],
-                                 'statusDetails': submission['statusDetails']})
+        # Get application
+        # https://docs.microsoft.com/en-us/windows/uwp/monetize/get-an-app
+        api_path = '{}/v1.0/my/applications/{}'.format(
+            'https://manage.devcenter.microsoft.com',
+            self.config.devbuildGalleryID
+        )
+
+        request = urllib2.Request(api_path, None, headers)
+        with contextlib.closing(opener.open(request)) as response:
+            app_obj = json.load(response)
+
+        # Delete existing in-progress submission
+        # https://docs.microsoft.com/en-us/windows/uwp/monetize/delete-an-app-submission
+        submissions_path = api_path + '/submissions'
+        if 'pendingApplicationSubmission' in app_obj:
+            remove_id = app_obj['pendingApplicationSubmission']['id']
+            remove_path = '{}/{}'.format(submissions_path, remove_id)
+            request = urllib2.Request(remove_path, '', headers)
+            request.get_method = lambda: 'DELETE'
+            opener.open(request).close()
+
+        # Create submission
+        # https://msdn.microsoft.com/en-us/windows/uwp/monetize/create-an-app-submission
+        request = urllib2.Request(submissions_path, '', headers)
+        request.get_method = lambda: 'POST'
+        with contextlib.closing(opener.open(request)) as response:
+            submission = json.load(response)
+
+        submission_id = submission['id']
+        file_upload_url = submission['fileUploadUrl']
+
+        new_submission_path = '{}/{}'.format(submissions_path,
+                                             submission_id)
+
+        request = urllib2.Request(new_submission_path, None, headers)
+        opener.open(request).close()
+
+        self.upload_appx_file_to_windows_store(file_upload_url)
+
+        # Commit submission
+        # https://msdn.microsoft.com/en-us/windows/uwp/monetize/commit-an-app-submission
+        commit_path = '{}/commit'.format(new_submission_path)
+        request = urllib2.Request(commit_path, '', headers)
+        request.get_method = lambda: 'POST'
+        with contextlib.closing(opener.open(request)) as response:
+            submission = json.load(response)
+
+        if submission['status'] != 'CommitStarted':
+            raise Exception({'status': submission['status'],
+                             'statusDetails': submission['statusDetails']})
 
     def run(self):
         """
           Run the nightly build process for one extension
         """
         try:
             if self.config.type == 'ie':
                 # We cannot build IE builds, simply list the builds already in
                 # the directory. Basename has to be deduced from the repository name.
                 self.basename = os.path.basename(self.config.repository)
@@ skipping 74 matching lines @@
         except Exception as ex:
             logging.error('The build for %s failed:', repo)
             logging.exception(ex)
 
     file = open(nightlyConfigFile, 'wb')
     nightlyConfig.write(file)
 
 
 if __name__ == '__main__':
     main()