Skip to content

SG-38306 Python2 Removal - Part 7 - various #404

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

Open
wants to merge 6 commits into
base: ticket/SG-38306-python2-removal-httplib2
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Integration and unit tests are provided.
- (Note: Running `pip install -r tests/ci_requirements.txt` will install this package)
- A `tests/config` file (you can copy an example from `tests/example_config`).
- Tests can be run individually like this: `nosetests --config="nose.cfg" tests/test_client.py`
- Make sure to not forget the `--config="nose.cfg"` option. This option tells nose to use our config file. This will exclude python 2- and 3-specific files in the `/lib` directory, preventing a failure from being reported by nose for compilation due to incompatible syntax in those files.
- Make sure to not forget the `--config="nose.cfg"` option. This option tells nose to use our config file.
- `test_client` and `tests_unit` use mock server interaction and do not require a Flow Production Tracking instance to be available (no modifications to `tests/config` are necessary).
- `test_api` and `test_api_long` *do* require a Flow Production Tracking instance, with a script key available for the tests. The server and script user values must be supplied in the `tests/config` file. The tests will add test data to your server based on information in your config. This data will be manipulated by the tests, and should not be used for other purposes.
- To run all of the tests, use the shell script `run-tests`.
Expand Down
10 changes: 5 additions & 5 deletions azure-pipelines-templates/run-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ parameters:

jobs:
# The job will be named after the OS and Azure will suffix the strategy to make it unique
# so we'll have a job name "Windows Python 2.7" for example. What's a strategy? Strategies are the
# name of the keys under the strategy.matrix scope. So for each OS we'll have "<OS> Python 2.7" and
# "<OS> Python 3.7".
# so we'll have a job name "Windows Python 3.9" for example. What's a strategy? Strategies are the
# name of the keys under the strategy.matrix scope. So for each OS we'll have "<OS> Python 3.9" and
# "<OS> Python 3.10".
- job: ${{ parameters.name }}
pool:
vmImage: ${{ parameters.vm_image }}
Expand Down Expand Up @@ -68,8 +68,8 @@ jobs:
versionSpec: '$(python.version)'
addToPath: True

# Install all dependencies needed for running the tests. This command is good for
# Python 2 and 3, but also for all OSes
# Install all dependencies needed for running the tests. This command is good
# for all OSes
- script: |
python -m pip install --upgrade pip setuptools wheel
python -m pip install -r tests/ci_requirements.txt
Expand Down
59 changes: 16 additions & 43 deletions shotgun_api3/shotgun.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import json
import http.client # Used for secure file upload
import http.cookiejar # used for attachment upload
import io # used for attachment upload
import io
import logging
import mimetypes
import os
Expand All @@ -49,14 +49,14 @@
import urllib.parse
import urllib.request
import uuid # used for attachment upload
import xml.etree.ElementTree

Check notice on line 52 in shotgun_api3/shotgun.py

View check run for this annotation

ShotGrid Chorus / security/bandit

B405: blacklist

Using xml.etree.ElementTree to parse untrusted XML data is known to be vulnerable to XML attacks. Replace xml.etree.ElementTree with the equivalent defusedxml package, or make sure defusedxml.defuse_stdlib() is called. secure coding id: PYTH-INJC-10.

Check failure on line 52 in shotgun_api3/shotgun.py

View check run for this annotation

ShotGrid Chorus / security/semgrep

app.chorus.semgrep.rules.python.lang.security.use-defused-xml.use-defused-xml

The Python documentation recommends using `defusedxml` instead of `xml` because the native Python `xml` library is vulnerable to XML External Entity (XXE) attacks. These attacks can leak confidential data and "XML bombs" can cause denial of service.

# Import Error and ResponseError (even though they're unused in this file) since they need
# to be exposed as part of the API.
from xmlrpc.client import Error, ProtocolError, ResponseError # noqa

# Python 2/3 compatibility
from .lib import six
from .lib import sgsix
from .lib import sgutils
from .lib.httplib2 import Http, ProxyInfo, socks
from .lib.sgtimezone import SgTimezone
Expand Down Expand Up @@ -329,7 +329,7 @@
``windows``, or ``None`` (if the current platform couldn't be determined).
:ivar str local_path_field: The PTR field used for local file paths. This is calculated using
the value of ``platform``. Ex. ``local_path_mac``.
:ivar str py_version: Simple version of Python executable as a string. Eg. ``2.7``.
:ivar str py_version: Simple version of Python executable as a string. Eg. ``3.9``.
:ivar str ssl_version: Version of OpenSSL installed. Eg. ``OpenSSL 1.0.2g 1 Mar 2016``. This
info is only available in Python 2.7+ if the ssl module was imported successfully.
Defaults to ``unknown``
Expand Down Expand Up @@ -567,18 +567,6 @@
:class:`~shotgun_api3.MissingTwoFactorAuthenticationFault` will be raised if the
``auth_token`` is invalid.
.. todo: Add this info to the Authentication section of the docs

.. note:: A note about proxy connections: If you are using Python <= v2.6.2, HTTPS
connections through a proxy server will not work due to a bug in the :mod:`urllib2`
library (see http://bugs.python.org/issue1424152). This will affect upload and
download-related methods in the Shotgun API (eg. :meth:`~shotgun_api3.Shotgun.upload`,
:meth:`~shotgun_api3.Shotgun.upload_thumbnail`,
:meth:`~shotgun_api3.Shotgun.upload_filmstrip_thumbnail`,
:meth:`~shotgun_api3.Shotgun.download_attachment`. Normal CRUD methods for passing JSON
data should still work fine. If you cannot upgrade your Python installation, you can see
the patch merged into Python v2.6.3 (http://hg.python.org/cpython/rev/0f57b30a152f/) and
try and hack it into your installation but YMMV. For older versions of Python there
are other patches that were proposed in the bug report that may help you as well.
"""

# verify authentication arguments
Expand Down Expand Up @@ -617,13 +605,7 @@
if script_name is not None or api_key is not None:
raise ValueError("cannot provide an auth_code with script_name/api_key")

# Can't use 'all' with python 2.4
if (
len(
[x for x in [session_token, script_name, api_key, login, password] if x]
)
== 0
):
if not any([session_token, script_name, api_key, login, password]):
if connect:
raise ValueError(
"must provide login/password, session_token or script_name/api_key"
Expand Down Expand Up @@ -2879,8 +2861,7 @@
This parameter exists only for backwards compatibility for scripts specifying
the parameter with keywords.
:returns: If ``file_path`` is provided, returns the path to the file on disk. If
``file_path`` is ``None``, returns the actual data of the file, as str in Python 2 or
bytes in Python 3.
``file_path`` is ``None``, returns the actual data of the file, as bytes.
:rtype: str | bytes
"""
# backwards compatibility when passed via keyword argument
Expand Down Expand Up @@ -2941,12 +2922,13 @@
]

if body:
xml = "".join(body)
# Once python 2.4 support is not needed we can think about using
# elementtree. The doc is pretty small so this shouldn't be an issue.
match = re.search("<Message>(.*)</Message>", xml)
if match:
err += " - %s" % (match.group(1))
try:
root = xml.etree.ElementTree.fromstring("".join(body))

Check warning on line 2926 in shotgun_api3/shotgun.py

View check run for this annotation

ShotGrid Chorus / security/bandit

B314: blacklist

Using xml.etree.ElementTree.fromstring to parse untrusted XML data is known to be vulnerable to XML attacks. Replace xml.etree.ElementTree.fromstring with its defusedxml equivalent function or make sure defusedxml.defuse_stdlib() is called secure coding id: PYTH-INJC-10.
message_elem = root.find(".//Message")
if message_elem is not None and message_elem.text:
err += f" - {message_elem.text}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err += f" - {message_elem.text}"
err = f"{err} - {message_elem.text}"

except xml.etree.ElementTree.ParseError:
err += "\n%s\n" % "".join(body)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err += "\n%s\n" % "".join(body)
err = "{err}\n%s\n" % "".join(body)

elif e.code == 409 or e.code == 410:
# we may be dealing with a file that is pending/failed a malware scan, e.g:
# 409: This file is undergoing a malware scan, please try again in a few minutes
Expand Down Expand Up @@ -4694,17 +4676,12 @@
handler_order = urllib.request.HTTPHandler.handler_order - 10 # needs to run first

def http_request(self, request):
# get_data was removed in 3.4. since we're testing against 3.6 and
# 3.7, this should be sufficient.
if six.PY3:
data = request.data
else:
data = request.get_data()
data = request.data
if data is not None and not isinstance(data, str):
files = []
params = []
for key, value in data.items():
if isinstance(value, sgsix.file_types):
if isinstance(value, io.IOBase):
files.append((key, value))
else:
params.append((key, value))
Expand All @@ -4715,12 +4692,8 @@
boundary, data = self.encode(params, files)
content_type = "multipart/form-data; boundary=%s" % boundary
request.add_unredirected_header("Content-Type", content_type)
# add_data was removed in 3.4. since we're testing against 3.6 and
# 3.7, this should be sufficient.
if six.PY3:
request.data = data
else:
request.add_data(data)
request.data = data

return request

def encode(self, params, files, boundary=None, buffer=None):
Expand Down
11 changes: 4 additions & 7 deletions tests/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,13 +176,10 @@ def _mock_http(self, data, headers=None, status=None):
return

if not isinstance(data, str):
if six.PY2:
data = json.dumps(data, ensure_ascii=False, encoding="utf-8")
else:
data = json.dumps(
data,
ensure_ascii=False,
)
data = json.dumps(
data,
ensure_ascii=False,
)

resp_headers = {
"cache-control": "no-cache",
Expand Down
51 changes: 4 additions & 47 deletions tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,6 @@
from shotgun_api3.lib import six
from shotgun_api3.lib.httplib2 import Http

# To mock the correct exception when testion on Python 2 and 3, use the
# ShotgunSSLError variable from sgsix that contains the appropriate exception
# class for the current Python version.
from shotgun_api3.lib.sgsix import ShotgunSSLError

import shotgun_api3

from . import base
Expand Down Expand Up @@ -272,44 +267,6 @@ def test_upload_download(self):
"sg_uploaded_movie",
tag_list="monkeys, everywhere, send, help",
)
if six.PY2:
# In Python2, make sure that non-utf-8 encoded paths raise when they
# can't be converted to utf-8. For Python3, we'll skip these tests
# since string encoding is handled differently.

# We need to touch the file we're going to test with first. We can't
# bundle a file with this filename in the repo due to some pip install
# problems on Windows. Note that the path below is utf-8 encoding of
# what we'll eventually encode as shift-jis.
file_path_s = os.path.join(this_dir, "./\xe3\x81\x94.shift-jis")
file_path_u = file_path_s.decode("utf-8")

with open(
file_path_u if sys.platform.startswith("win") else file_path_s, "w"
) as fh:
fh.write("This is just a test file with some random data in it.")

self.assertRaises(
shotgun_api3.ShotgunError,
self.sg.upload,
"Version",
self.version["id"],
file_path_u.encode("shift-jis"),
"sg_uploaded_movie",
tag_list="monkeys, everywhere, send, help",
)

# But it should work in all cases if a unicode string is used.
self.sg.upload(
"Version",
self.version["id"],
file_path_u,
"sg_uploaded_movie",
tag_list="monkeys, everywhere, send, help",
)

# cleanup
os.remove(file_path_u)

# cleanup
os.remove(file_path)
Expand Down Expand Up @@ -2299,7 +2256,7 @@ def my_side_effect2(*args, **kwargs):
@unittest.mock.patch("shotgun_api3.shotgun.Http.request")
def test_sha2_error(self, mock_request):
# Simulate the exception raised with SHA-2 errors
mock_request.side_effect = ShotgunSSLError(
mock_request.side_effect = ssl.SSLError(
"[Errno 1] _ssl.c:480: error:0D0C50A1:asn1 "
"encoding routines:ASN1_item_verify: unknown message digest "
"algorithm"
Expand All @@ -2326,7 +2283,7 @@ def test_sha2_error(self, mock_request):

try:
self.sg.info()
except ShotgunSSLError:
except ssl.SSLError:
# ensure the api has reset the values in the correct fallback behavior
self.assertTrue(self.sg.config.no_ssl_validation)
self.assertTrue(shotgun_api3.shotgun.NO_SSL_VALIDATION)
Expand All @@ -2339,7 +2296,7 @@ def test_sha2_error(self, mock_request):
@unittest.mock.patch("shotgun_api3.shotgun.Http.request")
def test_sha2_error_with_strict(self, mock_request):
# Simulate the exception raised with SHA-2 errors
mock_request.side_effect = ShotgunSSLError(
mock_request.side_effect = ssl.SSLError(
"[Errno 1] _ssl.c:480: error:0D0C50A1:asn1 "
"encoding routines:ASN1_item_verify: unknown message digest "
"algorithm"
Expand All @@ -2356,7 +2313,7 @@ def test_sha2_error_with_strict(self, mock_request):

try:
self.sg.info()
except ShotgunSSLError:
except ssl.SSLError:
# ensure the api has NOT reset the values in the fallback behavior because we have
# set the env variable to force validation
self.assertFalse(self.sg.config.no_ssl_validation)
Expand Down
5 changes: 1 addition & 4 deletions tests/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -660,10 +660,7 @@ def _assert_decode_resonse(self, ensure_ascii, data):
connect=False,
)

if six.PY3:
j = json.dumps(d, ensure_ascii=ensure_ascii)
else:
j = json.dumps(d, ensure_ascii=ensure_ascii, encoding="utf-8")
j = json.dumps(d, ensure_ascii=ensure_ascii)
self.assertEqual(d, sg._decode_response(headers, j))

headers["content-type"] = "text/javascript"
Expand Down