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 161 matching lines @@
         metadata = packager.readMetadata(self.tempdir, self.config.type)
         certs = xarfile.read_certificates_and_key(self.config.keyFile)[0]
 
         self.certificateID = packager.get_developer_identifier(certs)
         self.version = packager.getBuildVersion(self.tempdir, metadata, False,
                                                 self.buildNum)
         self.shortVersion = metadata.get('general', 'version')
         self.basename = metadata.get('general', 'basename')
         self.updatedFromGallery = False
 
-    def readEdgeMetadata(self):
+    def read_edge_metadata(self):
         """
           Read Edge-specific metadata from metadata file.
         """
-        import buildtools.packagerEdge as packagerEdge

[Vasily Kuznetsov, 2017/02/07 12:38:15]

You don't really need the `import ... as ...` syntax if you're not renaming
`packagerEdge` to something. Could be just `from buildtools import
packagerEdge`, which avoids the repetition.

[Oleksandr, 2017/02/09 00:57:14]

Done.

+        from buildtools import packager
         # Now read metadata file
-        metadata = packagerEdge.packager.readMetadata(self.tempdir,

[Vasily Kuznetsov, 2017/02/07 12:38:15]

This `packagerEdge.packager` is actually `buildtools.packager` module (it's
imported into `packagerEdge` on line 12
https://hg.adblockplus.org/buildtools/file/tip/packagerEdge.py#l12). Also it
seems like `packagerEdge` itself is not used in this function. Is there some
specific reason you're importing `packagerEdge` instead of just directly
importing `buildtools.packager`?

[Oleksandr, 2017/02/09 00:57:15]

Done.

-                                                      self.config.type)
-        self.version = packagerEdge.packager.getBuildVersion(self.tempdir,
-                                                             metadata, False,
-                                                             self.buildNum)
+        metadata = packager.readMetadata(self.tempdir, self.config.type)
+        self.version = packager.getBuildVersion(self.tempdir, metadata, False,
+                                                self.buildNum)
         self.basename = metadata.get('general', 'basename')
 
         self.compat = []
 
     def writeUpdateManifest(self):
         """
           Writes update manifest for the current build
         """
         baseDir = os.path.join(self.config.nightliesDirectory, self.basename)
         if self.config.type == 'safari':
@@ skipping 210 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 uploadToWindowsStore(self):

[Vasily Kuznetsov, 2017/02/07 12:38:16]

This method is quite long. It seems like it basically does three things: getting
the access token, app submission API manipulations and zip file uploading.
Perhaps we could factor the access token part and the uploading part into
separate methods. They seem easy to separate from the main API code that
revolves around `ingestionConnection`. What do you think?

[Sebastian Noack, 2017/02/07 13:54:43]

Also new methods (and variables) should use underscores for compliance with our
coding practices and PEP-8, even if surrounding legacy code is still using
camelcase. Otherwise, it will just become more difficult to get rid of
inappropriately camelcased names in the long-term.

[Oleksandr, 2017/02/09 00:57:15]

Done.

-

[Vasily Kuznetsov, 2017/02/07 12:38:16]

Also the ingestionConnection-related code seems to be doing a lot of the same
thing: sending a request, getting a json response and decoding it (sometimes
ignoring it). Perhaps we could reduce code duplication and make it more compact
by creating a method (or a local function) that takes the parameters of
`ingestConnection.request()` and returns a decoded response?

[Oleksandr, 2017/02/09 00:57:15]

Done.

-        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)

[Vasily Kuznetsov, 2017/02/07 12:38:16]

You're reading the content of `fp` and at the same time returning the file
descriptor in the exception. Would not that produce confusion if whatever is
catching this error would try to read from the file descriptor?

[Vasily Kuznetsov, 2017/02/07 12:38:16]

Our style guide now recommends using `'{} bla {}'.format(foo, bar)` instead of
`'% bla %' % (foo, bar). Even though there's existing code using % around here,
it would be better to use `.format` for all new code.

[Sebastian Noack, 2017/02/07 13:54:42]

The thing is HttpError objects are both exceptions and file-like objects. So
there has to be an underlying file-like object (I suppose) that is closed when
HTTPError.close() is called. But why do we need custom error handling here in
the first place?

[Oleksandr, 2017/02/09 00:57:15]

I have moved all the networking code to just httplib, since there was no reason
to use urllib2.

-
-        opener = urllib2.build_opener(HTTPErrorBodyHandler)
-
+    def get_windows_store_access_token(self):
         # 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
-
-        response = json.load(opener.open(

[Sebastian Noack, 2017/02/07 13:54:42]

The response object isn't closed here. The canonical way to make sure file-like
objects are closed properly, in modern Python code, is using the with-statement.
However, in Python 2, urllib2.Response doesn't implement __enter__/__exit__ (the
magic methods that are called by the with statement) yet. So you'd have to wrap
the call with contextlib.closing() (or use try/finally).

  with contextlib.closing(opener.open(...)) as response:
    data = json.load(response)

-            'https://login.microsoftonline.com/{0}/oauth2/token'.format(

[Sebastian Noack, 2017/02/07 13:54:43]

Thanks for using the format() method here, but the explicit index is redundant
here, '..{}..'.format(...) would be sufficient, unless you want to reuse a
particular argument more than once.

-                self.config.tenantID),
-            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 = response['token_type'] + ' ' + response['access_token']

[Sebastian Noack, 2017/02/07 13:54:42]

As agreed on in our coding practices, we use the format() method, rather than
the + operator, when concatenating more than two strings, which in particular
comes handy here where you use values form a dictionary:
  
  auth_token = '{[token_type]} {[access_token]}'.format(response)

[Oleksandr, 2017/02/09 00:57:16]

Done.

-
-        # 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

[Vasily Kuznetsov, 2017/02/07 12:38:15]

This is a very long line. I wonder if we could use a url shortener on this url?
Also has disadvantages since the url becomes kind of opaque, but we'd have a
nice neat line like.

# Based on code from https://goo.gl/9eymX7

What do you think?

[Sebastian Noack, 2017/02/07 13:54:42]

I'd rather not obfuscate URLs in the source code (comments). Not to mention,
relying on third-party tools, so that if whatever URL shortener we'd use, if it
should be suspended at some point, we end up with broken links. On the other
hand, this will become a problem, once we enforce the max line length with
flake8, here. I guess, unless somebody has a better idea, let's leave this long
line alone for now.

[Vasily Kuznetsov, 2017/02/07 15:05:26]

Yeah, good point. I also don't see an ideal solution here, and this is not the
only place where this arises. Perhaps we should just relax the line length rule
for these cases of long urls in comments somehow.

[Oleksandr, 2017/02/09 00:57:15]

Acknowledged.

-        headers = {'Authorization': auth_token,
-                   'Content-type': 'application/json',
-                   'User-Agent': 'Python'}

[Sebastian Noack, 2017/02/07 13:54:42]

Is it even necessary to set a User-Agent here?

[Oleksandr, 2017/02/09 00:57:15]

It is not. Removed.

-
-        apiServer = 'manage.devcenter.microsoft.com'

[Vasily Kuznetsov, 2017/02/07 12:38:15]

PEP8 (and therefore our style guide) recommends lowercase variable names with
words separated by underscores and we started following that for all new code.
The names of methods of `NightlyBuild` can probably stay as they are since
that's consistent with all existing methods but for the names of the variables
inside of the method it would be better to follow the style guide.

[Sebastian Noack, 2017/02/07 13:54:43]

As I said above, I think we should also avoid camelcase for new methods. The
consistency argument isn't very consistent while we still insist on underscores
for variable names. But in the end I don't care much about consistency here, but
rather prefer more code being compliant with PEP-8. In particular, if we are not
going to implement elaborate tests for this, changing it later will be a hassle.

[Oleksandr, 2017/02/09 00:57:15]

Done.

-        ingestionConnection = httplib.HTTPSConnection(apiServer)

[Sebastian Noack, 2017/02/07 13:54:42]

"ingestion" seems like a weird term to use here, is this from Microsoft's
terminology? Otherwise, I might prefer going with just "connection".

[Oleksandr, 2017/02/09 00:57:15]

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')
+        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):
+        opener = urllib2.build_opener(HTTPErrorBodyHandler)
+
+        headers = {'Authorization': self.get_windows_store_access_token(),
+                   'Content-type': 'application/json'}
 
         # Get application
-        appPath = '/v1.0/my/applications/%s' % self.config.devbuildGalleryID
-
-        # https://msdn.microsoft.com/en-us/windows/uwp/monetize/get-an-add-on

[Vasily Kuznetsov, 2017/02/07 12:38:15]

Is this URL relevant to what we're doing here? The code around seems to be from
the link above and I couldn't see how this page is relevant. Or am I just
missing something?

[Oleksandr, 2017/02/09 00:57:16]

Fixed, and also same for URL below.

-        ingestionConnection.request('GET', appPath, '', headers)
-        appResponse = ingestionConnection.getresponse()
+        # 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://msdn.microsoft.com/en-us/windows/uwp/monetize/get-an-add-on
-        appObj = json.loads(appResponse.read().decode())
-
-        submissionsPath = appPath + '/submissions'
-        if 'pendingApplicationSubmission' in appObj:
-            removeId = appObj['pendingApplicationSubmission']['id']
-            ingestionConnection.request('DELETE',
-                                        '%s/%s' % (submissionsPath, removeId),
-                                        '', headers)
-            deleteSubmissionResponse = ingestionConnection.getresponse()
-            deleteSubmissionResponse.read()

[Vasily Kuznetsov, 2017/02/07 12:38:15]

Should we do some error checking here? Or will we just get a non-2xx error code
(and hence an exception) in case of error?

[Sebastian Noack, 2017/02/07 13:54:42]

Also what about closing the response?

[Oleksandr, 2017/02/09 00:57:14]

If successful, the response would have an empty body. If not, there will be
non-2xx error code, which means we would raise an exception.

[Oleksandr, 2017/02/09 00:57:15]

I don't think we have to close the response. We will close the connection now,
however.

+        # 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
-        ingestionConnection.request('POST', submissionsPath, '', headers)
-        createSubmissionResponse = ingestionConnection.getresponse()
-
-        submission = json.loads(
-                createSubmissionResponse.read().decode()
-            )
-
-        submissionId = submission['id']
-        fileUploadUrl = submission['fileUploadUrl']
-
-        # Update submission
-        oldSubmission = submission['applicationPackages'][0]
-        oldSubmission['fileStatus'] = 'PendingDelete'
-        submission['applicationPackages'].append(
-            {'fileStatus': 'PendingUpload'})
-        addedSubmission = submission['applicationPackages'][1]
-        addedSubmission['fileName'] = os.path.basename(self.path)
-        addedSubmission['minimumSystemRam'] = oldSubmission['minimumSystemRam']
-
-        oldDirectXVersion = oldSubmission['minimumDirectXVersion']
-        addedSubmission['minimumDirectXVersion'] = oldDirectXVersion
-
-        newSubmissionPath = '%s/%s' % (submissionsPath, submissionId)
-        ingestionConnection.request('PUT', newSubmissionPath,
-                                    json.dumps(submission), headers)
-        ingestionConnection.getresponse().read()
-
-        # Add .appx file to a .zip file
-        zipPath = os.path.splitext(self.path)[0] + '.zip'
-        with zipfile.ZipFile(zipPath, 'w', zipfile.ZIP_DEFLATED) as zf:
-            zf.write(self.path, os.path.basename(self.path))
-
-        # Upload that .zip file
-        request = urllib2.Request(fileUploadUrl.replace('+', '%2B'))
-        request.get_method = lambda: 'PUT'
-        request.add_header('x-ms-blob-type', 'BlockBlob')
-
-        with open(zipPath, 'rb') as file:
-            fileSize = os.fstat(file.fileno()).st_size - file.tell()
-            request.add_header('Content-Length', fileSize)
-            request.add_data(file)
-            opener.open(request)
+        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
-        ingestionConnection.request('POST',
-                                    newSubmissionPath + '/commit',
-                                    '', headers)
-        submission = json.loads(

[Sebastian Noack, 2017/02/07 13:54:42]

Couldn't you simply use json.load(ingestionConnection.getresponse()) here?

-                ingestionConnection.getresponse().read().decode()
-            )
+        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']})

[Sebastian Noack, 2017/02/07 13:54:43]

The indentation here seems off. The keys should be aligned.

[Oleksandr, 2017/02/09 00:57:14]

Done.

-        ingestionConnection.close()

[Sebastian Noack, 2017/02/07 13:54:42]

It would be better to use the with-statement (or try/finally) to make sure the
connection is closed (see the related comment above).

[Oleksandr, 2017/02/09 00:57:15]

Done.

+                             '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)
             else:
                 # copy the repository into a temporary directory
                 self.copyRepository()
                 self.buildNum = self.getCurrentBuild()
 
                 # get meta data from the repository
                 if self.config.type == 'android':
                     self.readAndroidMetadata()
                 elif self.config.type == 'chrome':
                     self.readChromeMetadata()
                 elif self.config.type == 'safari':
                     self.readSafariMetadata()
                 elif self.config.type in {'gecko', 'gecko-webext'}:
                     self.readGeckoMetadata()
                 elif self.config.type == 'edge':
-                    self.readEdgeMetadata()
+                    self.read_edge_metadata()
                 else:
                     raise Exception('Unknown build type {}' % self.config.type)
 
                 # create development build
                 self.build()
 
                 # write out changelog
                 self.writeChangelog(self.getChanges())
 
                 # write update manifest
@@ skipping 11 matching lines @@
 
             # update nightlies config
             self.config.latestRevision = self.revision
 
             if (self.config.type in {'gecko', 'gecko-webext'} and
                     self.config.galleryID and
                     get_config().has_option('extensions', 'amo_key')):
                 self.uploadToMozillaAddons()
             elif self.config.type == 'chrome' and self.config.clientID and self.config.clientSecret and self.config.refreshToken:
                 self.uploadToChromeWebStore()
             elif self.config.type == 'edge' and self.config.clientID and self.config.clientSecret and self.config.refreshToken and self.config.tenantID:

[Vasily Kuznetsov, 2017/02/07 12:38:15]

What happens if we don't have `config.clientID` or `config.clientSecret` or
others? Is it right to just silently do nothing?

[Sebastian Noack, 2017/02/07 13:54:42]

At least this seems consistent with the equivalanet logic for Mozilla Add-Ons
and the Chrome Webstore, above. So unless we plan to change the behavior there,
I think the logic for the Windows Store is correct and consistent.

[Vasily Kuznetsov, 2017/02/07 15:05:26]

Acknowledged.

-                self.uploadToWindowsStore()
+                self.upload_to_windows_store()
 
         finally:
             # clean up
             if self.tempdir:
                 shutil.rmtree(self.tempdir, ignore_errors=True)
 
 
 def main():
     """
       main function for createNightlies.py
@@ skipping 15 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()