From 9798ba5309f2d8e6bf3bc94d1978a946579ce9b1 Mon Sep 17 00:00:00 2001 From: Jonatan Sundeen Date: Wed, 29 Nov 2023 09:48:35 +0100 Subject: [PATCH] Artifactory module basic auth fix This fixes issue with basic auth causing 401 Bad Request. Using HTTPBasicAuthHandler instead of header. --- salt/modules/artifactory.py | 96 ++++++------------- .../pytests/unit/modules/test_artifactory.py | 21 ++-- 2 files changed, 37 insertions(+), 80 deletions(-) diff --git a/salt/modules/artifactory.py b/salt/modules/artifactory.py index 0f01d89e82f..f48325a4aae 100644 --- a/salt/modules/artifactory.py +++ b/salt/modules/artifactory.py @@ -18,7 +18,6 @@ log = logging.getLogger(__name__) __virtualname__ = "artifactory" - def __virtual__(): """ Only load if elementtree xml library is available. @@ -26,6 +25,14 @@ def __virtual__(): return True +def set_basic_auth(url, username, password): + pw_mgr = urllib.request.HTTPPasswordMgrWithDefaultRealm() + pw_mgr.add_password(None, url, username, password) + basic_auth_handler = urllib.request.HTTPBasicAuthHandler(pw_mgr) + opener = urllib.request.build_opener(basic_auth_handler) + urllib.request.install_opener(opener) + + def get_latest_snapshot( artifactory_url, repository, @@ -75,21 +82,14 @@ def get_latest_snapshot( target_dir, classifier, ) - - headers = {} if username and password: - headers["Authorization"] = "Basic {}".format( - salt.utils.hashutils.base64_encodestring( - "{}:{}".format(username.replace("\n", ""), password.replace("\n", "")) - ) - ) + set_basic_auth(artifactory_url, username, password) artifact_metadata = _get_artifact_metadata( artifactory_url=artifactory_url, repository=repository, group_id=group_id, artifact_id=artifact_id, - headers=headers, - use_literal_group_id=use_literal_group_id, + use_literal_group_id=use_literal_group_id ) version = artifact_metadata["latest_version"] snapshot_url, file_name = _get_snapshot_url( @@ -100,12 +100,11 @@ def get_latest_snapshot( version=version, packaging=packaging, classifier=classifier, - headers=headers, use_literal_group_id=use_literal_group_id, ) target_file = __resolve_target_file(file_name, target_dir, target_file) - return __save_artifact(snapshot_url, target_file, headers) + return __save_artifact(snapshot_url, target_file) def get_snapshot( @@ -162,13 +161,8 @@ def get_snapshot( target_dir, classifier, ) - headers = {} if username and password: - headers["Authorization"] = "Basic {}".format( - salt.utils.hashutils.base64_encodestring( - "{}:{}".format(username.replace("\n", ""), password.replace("\n", "")) - ) - ) + set_basic_auth(artifactory_url, username, password) snapshot_url, file_name = _get_snapshot_url( artifactory_url=artifactory_url, repository=repository, @@ -178,12 +172,11 @@ def get_snapshot( packaging=packaging, snapshot_version=snapshot_version, classifier=classifier, - headers=headers, use_literal_group_id=use_literal_group_id, ) target_file = __resolve_target_file(file_name, target_dir, target_file) - return __save_artifact(snapshot_url, target_file, headers) + return __save_artifact(snapshot_url, target_file) def get_latest_release( @@ -235,19 +228,13 @@ def get_latest_release( target_dir, classifier, ) - headers = {} if username and password: - headers["Authorization"] = "Basic {}".format( - salt.utils.hashutils.base64_encodestring( - "{}:{}".format(username.replace("\n", ""), password.replace("\n", "")) - ) - ) + set_basic_auth(artifactory_url, username, password) version = __find_latest_version( artifactory_url=artifactory_url, repository=repository, group_id=group_id, artifact_id=artifact_id, - headers=headers, ) release_url, file_name = _get_release_url( repository, @@ -261,7 +248,7 @@ def get_latest_release( ) target_file = __resolve_target_file(file_name, target_dir, target_file) - return __save_artifact(release_url, target_file, headers) + return __save_artifact(release_url, target_file) def get_release( @@ -317,13 +304,8 @@ def get_release( target_dir, classifier, ) - headers = {} if username and password: - headers["Authorization"] = "Basic {}".format( - salt.utils.hashutils.base64_encodestring( - "{}:{}".format(username.replace("\n", ""), password.replace("\n", "")) - ) - ) + set_basic_auth(artifactory_url, username, password) release_url, file_name = _get_release_url( repository, group_id, @@ -336,7 +318,7 @@ def get_release( ) target_file = __resolve_target_file(file_name, target_dir, target_file) - return __save_artifact(release_url, target_file, headers) + return __save_artifact(release_url, target_file) def __resolve_target_file(file_name, target_dir, target_file=None): @@ -354,11 +336,8 @@ def _get_snapshot_url( packaging, snapshot_version=None, classifier=None, - headers=None, use_literal_group_id=False, ): - if headers is None: - headers = {} has_classifier = classifier is not None and classifier != "" if snapshot_version is None: @@ -369,7 +348,6 @@ def _get_snapshot_url( group_id=group_id, artifact_id=artifact_id, version=version, - headers=headers, ) if ( not has_classifier @@ -503,7 +481,6 @@ def _get_artifact_metadata_xml( repository, group_id, artifact_id, - headers, use_literal_group_id=False, ): @@ -516,8 +493,8 @@ def _get_artifact_metadata_xml( ) try: - request = urllib.request.Request(artifact_metadata_url, None, headers) - artifact_metadata_xml = urllib.request.urlopen(request).read() + log.debug("Metadata url %s", artifact_metadata_url) + artifact_metadata_xml = urllib.request.urlopen(artifact_metadata_url).read() except (HTTPError, URLError) as err: message = "Could not fetch data from url: {}. ERROR: {}".format( artifact_metadata_url, err @@ -533,16 +510,14 @@ def _get_artifact_metadata( repository, group_id, artifact_id, - headers, - use_literal_group_id=False, + use_literal_group_id=False ): metadata_xml = _get_artifact_metadata_xml( artifactory_url=artifactory_url, repository=repository, group_id=group_id, artifact_id=artifact_id, - headers=headers, - use_literal_group_id=use_literal_group_id, + use_literal_group_id=use_literal_group_id ) root = ET.fromstring(metadata_xml) @@ -580,7 +555,6 @@ def _get_snapshot_version_metadata_xml( group_id, artifact_id, version, - headers, use_literal_group_id=False, ): @@ -594,8 +568,7 @@ def _get_snapshot_version_metadata_xml( ) try: - request = urllib.request.Request(snapshot_version_metadata_url, None, headers) - snapshot_version_metadata_xml = urllib.request.urlopen(request).read() + snapshot_version_metadata_xml = urllib.request.urlopen(snapshot_version_metadata_url).read() except (HTTPError, URLError) as err: message = "Could not fetch data from url: {}. ERROR: {}".format( snapshot_version_metadata_url, err @@ -607,15 +580,14 @@ def _get_snapshot_version_metadata_xml( def _get_snapshot_version_metadata( - artifactory_url, repository, group_id, artifact_id, version, headers + artifactory_url, repository, group_id, artifact_id, version ): metadata_xml = _get_snapshot_version_metadata_xml( artifactory_url=artifactory_url, repository=repository, group_id=group_id, artifact_id=artifact_id, - version=version, - headers=headers, + version=version ) metadata = ET.fromstring(metadata_xml) @@ -657,7 +629,6 @@ def __find_latest_version( repository, group_id, artifact_id, - headers, use_literal_group_id=False, ): @@ -670,8 +641,7 @@ def __find_latest_version( ) try: - request = urllib.request.Request(latest_version_url, None, headers) - version = urllib.request.urlopen(request).read() + version = urllib.request.urlopen(latest_version_url).read() except (HTTPError, URLError) as err: message = "Could not fetch data from url: {}. ERROR: {}".format( latest_version_url, err @@ -686,7 +656,7 @@ def __find_latest_version( return version -def __save_artifact(artifact_url, target_file, headers): +def __save_artifact(artifact_url, target_file): log.debug("__save_artifact(%s, %s)", artifact_url, target_file) result = {"status": False, "changes": {}, "comment": ""} @@ -695,7 +665,7 @@ def __save_artifact(artifact_url, target_file, headers): checksum_url = artifact_url + ".sha1" checksum_success, artifact_sum, checksum_comment = __download( - checksum_url, headers + checksum_url ) if checksum_success: artifact_sum = salt.utils.stringutils.to_unicode(artifact_sum) @@ -725,8 +695,7 @@ def __save_artifact(artifact_url, target_file, headers): log.debug("Downloading: %s -> %s", artifact_url, target_file) try: - request = urllib.request.Request(artifact_url, None, headers) - f = urllib.request.urlopen(request) + f = urllib.request.urlopen(artifact_url) with salt.utils.files.fopen(target_file, "wb") as local_file: local_file.write(salt.utils.stringutils.to_bytes(f.read())) result["status"] = True @@ -755,15 +724,13 @@ def __get_classifier_url(classifier): return "-" + classifier if has_classifier else "" -def __download(request_url, headers): +def __download(request_url): log.debug("Downloading content from %s", request_url) - success = False content = None comment = None try: - request = urllib.request.Request(request_url, None, headers) - url = urllib.request.urlopen(request) + url = urllib.request.urlopen(request_url) content = url.read() success = True except HTTPError as e: @@ -793,11 +760,10 @@ def __get_error_comment(http_error, request_url): def __append_comment(new_comment, current_comment=""): return current_comment + "\n" + new_comment - class ArtifactoryError(Exception): def __init__(self, value): super().__init__() self.value = value def __str__(self): - return repr(self.value) + return repr(self.value) \ No newline at end of file diff --git a/tests/pytests/unit/modules/test_artifactory.py b/tests/pytests/unit/modules/test_artifactory.py index 220cdf9ad88..dfc70f12627 100644 --- a/tests/pytests/unit/modules/test_artifactory.py +++ b/tests/pytests/unit/modules/test_artifactory.py @@ -35,7 +35,6 @@ def test_artifact_get_metadata(): repository="libs-releases", group_id="com.company.sampleapp.web-module", artifact_id="web", - headers={}, ) assert metadata["latest_version"] == "1.1_RC11" @@ -78,7 +77,6 @@ def test_snapshot_version_get_metadata(): group_id="com.company.sampleapp.web-module", artifact_id="web", version="1.1_RC8-SNAPSHOT", - headers={}, ) assert metadata["snapshot_versions"]["war"] == "1.1_RC8-20140418.150212-1" @@ -144,7 +142,6 @@ def test_construct_url_for_snapshot_version(): artifact_id="web", version="1.0_RC10-SNAPSHOT", packaging="war", - headers={}, ) assert ( @@ -195,7 +192,6 @@ def test_get_snapshot_url_with_classifier(): version="1.1_RC8-SNAPSHOT", packaging="war", classifier="test", - headers={}, ) assert ( @@ -249,7 +245,6 @@ def test_get_snapshot_url_without_classifier(): artifact_id="web", version="1.1_RC8-SNAPSHOT", packaging="war", - headers={}, ) @@ -277,8 +272,7 @@ def test_get_latest_snapshot_username_password(): ) save_artifact_mock.assert_called_with( "http://artifactory.example.com/artifactory/snapshot", - "/path/to/file", - {"Authorization": "Basic dXNlcjpwYXNzd29yZA==\n"}, + "/path/to/file" ) @@ -304,8 +298,7 @@ def test_get_snapshot_username_password(): ) save_artifact_mock.assert_called_with( "http://artifactory.example.com/artifactory/snapshot", - "/path/to/file", - {"Authorization": "Basic dXNlcjpwYXNzd29yZA==\n"}, + "/path/to/file" ) @@ -333,8 +326,7 @@ def test_get_latest_release_username_password(): ) save_artifact_mock.assert_called_with( "http://artifactory.example.com/artifactory/release", - "/path/to/file", - {"Authorization": "Basic dXNlcjpwYXNzd29yZA==\n"}, + "/path/to/file" ) @@ -360,8 +352,7 @@ def test_get_release_username_password(): ) save_artifact_mock.assert_called_with( "http://artifactory.example.com/artifactory/release", - "/path/to/file", - {"Authorization": "Basic dXNlcjpwYXNzd29yZA==\n"}, + "/path/to/file" ) @@ -378,7 +369,7 @@ def test_save_artifact_file_exists_checksum_equal(): return_value=(True, sum_bin, None), ): result = getattr(artifactory, "__save_artifact")( - artifact_url=artifact_url, target_file=target_file, headers={} + artifact_url=artifact_url, target_file=target_file ) assert result == { "status": True, @@ -394,7 +385,7 @@ def test_save_artifact_file_exists_checksum_equal(): return_value=(True, sum_str, None), ): result = getattr(artifactory, "__save_artifact")( - artifact_url=artifact_url, target_file=target_file, headers={} + artifact_url=artifact_url, target_file=target_file ) assert result == { "status": True,