Skip to content
This repository has been archived by the owner on Oct 15, 2021. It is now read-only.

Apply version checks to add-ons and plugins in the blocklist #74

Merged
merged 3 commits into from
Feb 6, 2018
Merged

Apply version checks to add-ons and plugins in the blocklist #74

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04245cf
Apply version checks to add-ons and plugins in the blocklist
gijsk Feb 5, 2018
870cc39
Pin pytest
gijsk Feb 5, 2018
ddfa110
Review nits
gijsk Feb 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 35 additions & 36 deletions amo2kinto/exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,39 @@ def build_version_range(root, item, app_id, app_ver=None):
maxVersion=tA['maxVersion'])


def is_related_to(item, app_id):
"""Return True if the item relates to the given app_id."""
def is_related_to(item, app_id, app_ver=None):
"""Return True if the item relates to the given app_id (and app_ver, if passed)."""
if not item.get('versionRange'):
return True

for vR in item['versionRange']:
if not vR.get('targetApplication'):
return True
if get_related_targetApplication(vR, app_id, app_ver) is not None:
return True
return False

for tA in vR['targetApplication']:
if tA['guid'] == app_id:
return True

return False
def get_related_targetApplication(vR, app_id, app_ver):
"""Return the first matching target application in this version range.
Returns None if there are no target applications or no matching ones."""
if not vR.get('targetApplication'):
return None

for tA in vR['targetApplication']:
Copy link
Contributor

Choose a reason for hiding this comment

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

targetApplication = vR.get('targetApplication')
if not targetApplication:
    return None
for tA in targetApplication:
    ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point, thanks. :-)

if tA['guid'] == app_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

There used to be a or 'guid' not in targetApplication in the previous code. Maybe you want to do tA.get('guid') instead (to avoid KeyError and obtain None)

if not app_ver:
return tA
# We purposefully use maxVersion only, so that the blocklist contains items
# whose minimum version is ahead of the version we get passed. This means
# the blocklist we serve is "future-proof" for app upgrades.
if between(version_int(app_ver), '0', tA.get('maxVersion', '*')):
return tA

return None

def write_addons_items(xml_tree, records, app_id, api_ver=3):

def write_addons_items(xml_tree, records, app_id, api_ver=3, app_ver=None):
"""Generate the addons blocklists.

<emItem blockID="i372" id="[email protected]">
Expand All @@ -80,7 +96,7 @@ def write_addons_items(xml_tree, records, app_id, api_ver=3):
emItems = etree.SubElement(xml_tree, 'emItems')
groupby = {}
for item in records:
if is_related_to(item, app_id):
if is_related_to(item, app_id, app_ver):
if item['guid'] in groupby:
emItem = groupby[item['guid']]
# When creating new records from the Kinto Admin we don't have proper blockID.
Expand Down Expand Up @@ -137,35 +153,17 @@ def write_plugin_items(xml_tree, records, app_id, api_ver=3, app_ver=None):

pluginItems = etree.SubElement(xml_tree, 'pluginItems')
for item in records:
if is_related_to(item, app_id):
for versionRange in item.get('versionRange', []):
if not versionRange.get('targetApplication'):
add_plugin_item(pluginItems, item, versionRange,
for versionRange in item.get('versionRange', []):
if not versionRange.get('targetApplication'):
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we use is_related_to() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this first, but it didn't work, which is why I factored out part of is_related_to(). The problem is you can invoke add_plugin_item both with and without a targetApplication. Iff you pass one, it includes information from that targetApplication into the bit it writes out into blocklist.xml. In other words, when I did that, the tests failed. :-)

add_plugin_item(pluginItems, item, versionRange,
app_id=app_id, api_ver=api_ver,
app_ver=app_ver)
else:
targetApplication = get_related_targetApplication(versionRange, app_id, app_ver)
if targetApplication is not None:
add_plugin_item(pluginItems, item, versionRange, targetApplication,
app_id=app_id, api_ver=api_ver,
app_ver=app_ver)
else:
for targetApplication in versionRange['targetApplication']:
is_targetApplication_related = (
'guid' not in targetApplication or
targetApplication['guid'] == app_id
)
if is_targetApplication_related:
if api_ver < 3 and app_ver is not None:
app_version = version_int(app_ver)
is_version_related = between(
app_version,
targetApplication.get('minVersion', 0),
targetApplication.get('maxVersion', '*'))
if is_version_related:
add_plugin_item(
pluginItems, item, versionRange,
targetApplication, app_id=app_id,
api_ver=api_ver, app_ver=app_ver)
else:
add_plugin_item(
pluginItems, item, versionRange,
targetApplication, app_id=app_id,
api_ver=api_ver, app_ver=app_ver)


def add_plugin_item(pluginItems, item, version, tA=None, app_id=None,
Expand Down Expand Up @@ -485,7 +483,8 @@ def main(args=None):

write_addons_items(xml_tree, addons_records,
api_ver=args.api_version,
app_id=args.app)
app_id=args.app,
app_ver=args.app_version)
write_plugin_items(xml_tree, plugin_records,
api_ver=args.api_version,
app_id=args.app,
Expand Down
76 changes: 76 additions & 0 deletions amo2kinto/tests/test_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,78 @@ def test_addon_record_with_no_targetApplication_matching():
""".decode('utf-8')


def test_addon_record_with_no_targetApplication_version_matching():
xml_tree = etree.Element(
'blocklist',
xmlns="http://www.mozilla.org/2006/addons-blocklist",
lastupdate='1459262434336'
)

data = ADDONS_DATA.copy()
data['versionRange'] = [{
"targetApplication": [
{"guid": constants.FIREFOX_APPID,
"minVersion": "3.6",
"maxVersion": "3.6.*"}
],
"minVersion": "0",
"maxVersion": "*",
"severity": 3
}]

exporter.write_addons_items(xml_tree, [data],
constants.FIREFOX_APPID, app_ver="3.7")

result = etree.tostring(
etree.ElementTree(xml_tree),
pretty_print=True,
xml_declaration=True,
encoding='UTF-8').decode('utf-8')

assert result == b"""<?xml version='1.0' encoding='UTF-8'?>
<blocklist lastupdate="1459262434336" \
xmlns="http://www.mozilla.org/2006/addons-blocklist">
<emItems/>
</blocklist>
""".decode('utf-8')


def test_addon_record_with_future_targetApplication_version_matching():
xml_tree = etree.Element(
'blocklist',
xmlns="http://www.mozilla.org/2006/addons-blocklist",
lastupdate='1459262434336'
)

exporter.write_addons_items(xml_tree, [ADDONS_DATA],
constants.FIREFOX_APPID, app_ver="3.1")

result = etree.tostring(
etree.ElementTree(xml_tree),
pretty_print=True,
xml_declaration=True,
encoding='UTF-8').decode('utf-8')

assert result == b"""<?xml version='1.0' encoding='UTF-8'?>
<blocklist lastupdate="1459262434336" \
xmlns="http://www.mozilla.org/2006/addons-blocklist">
<emItems>
<emItem blockID="i454" id="[email protected]">
<prefs>
<pref>test.blocklist</pref>
</prefs>
<versionRange minVersion="0" maxVersion="*" severity="3">
<targetApplication id="{ec8030f7-c20a-464f-9b0e-13a3a9e97384}">
<versionRange maxVersion="3.6.*" minVersion="3.6"/>
</targetApplication>
</versionRange>
<versionRange minVersion="0" maxVersion="*"/>
</emItem>
</emItems>
</blocklist>
""".decode('utf-8')


def test_addon_record_group_by_blockID():
xml_tree = etree.Element(
'blocklist',
Expand Down Expand Up @@ -335,6 +407,10 @@ def test_between_does_not_fails_with_none():
assert exporter.between(compare.version_int("46.2"), None, None)


def test_get_related_targetApplication_accepts_versionRange_without_targetApplication():
assert exporter.get_related_targetApplication({}, None, None) is None


def test_plugin_record():
xml_tree = etree.Element(
'blocklist',
Expand Down