Skip to content
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

Add response error message output to HTTP Status 401 Errors in FileDownloader #15983

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

shoeffner
Copy link
Contributor

@shoeffner shoeffner commented Mar 28, 2024

Changelog: Fix: Add response error message output to HTTP Status 401 Errors in FileDownloader.
Docs: Omit

(was #15981, but I committed with the wrong mail address so the GPG signatures were broken)

The source download step attempts to load source_credentials.json when it is present. When following the recommended steps of passing credentials to the source_credentials.json via os.getenv('BACKUP_USER') etc., this can result in the json string "None" when no authentication is provided.
This effectively gets translated to the curl call when being read:

curl -i -u None:None https://artifactory.example.org/artifactory/source-backup/

This causes an error with status code 401, which currently does not propagate the error message and is slightly misleading, as a source_credentials.json is provided but the content is wrong:

ERROR: conanfile.py (example/1.0): Error in source() method, line 17
        get(self, self.url, sha256=sha256, strip_root=True)
        ConanException: The source backup server 'https://artifactory.example.org/artifactory/source-backup/' needs authentication: . Please provide 'source_credentials.json'

With this fix, the error message is propagated in the same way others in the same block, resulting in a more useful message, although the "please provide" is still misleading.

ERROR: conanfile.py (example/1.0): Error in source() method, line 17
        get(self, self.url, sha256=sha256, strip_root=True)
        ConanException: The source backup server 'https://artifactory.example.org/artifactory/source-backup/' needs authentication: {
  "errors" : [ {
    "status" : 401,
    "message" : "Bad credentials"
  } ]
}. Please provide 'source_credentials.json'

(after creating the PR... sorry for not following the exact procedure of waiting for feedback etc.)

  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@CLAassistant
Copy link

CLAassistant commented Mar 28, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Would be great to capture this message in one test. I can contribute this next week if you cannot figure out how/where to do it.

Thanks for your contribution!

@memsharded memsharded added this to the 2.3.0 milestone Mar 28, 2024
@memsharded memsharded self-assigned this Mar 28, 2024
@shoeffner shoeffner force-pushed the develop2 branch 3 times, most recently from f0a4843 to 50dd44b Compare March 29, 2024 10:58
Also change the TestFileServer to distinguish between the two cases
so that we can test them by replacing the bad credentials error
message "Not authorized" with "Bad credentials".
The source download step attempts to load source_credentials.json when it is present.
When following the recommended steps of passing credentials to the source_credentials.json via
os.getenv('BACKUP_USER') etc., this can result in the json string "None" when
no authentication is provided.
This effectively gets translated to the curl call when being read:

    curl -i -u None:None https://artifactory.example.org/artifactory/source-backup/

This causes an error with status code 401, which currently does not propagate
the error message and is slightly misleading, as a source_credentials.json is
provided but the content is wrong:

    ERROR: conanfile.py (example/1.0): Error in source() method, line 17
            get(self, self.url, sha256=sha256, strip_root=True)
            ConanException: The source backup server 'https://artifactory.example.org/artifactory/source-backup/' needs authentication: . Please provide 'source_credentials.json'

With this fix, the error message is propagated in the same way others in the
same block, resulting in a more useful message, although the "please provide"
is still misleading.

    ERROR: conanfile.py (example/1.0): Error in source() method, line 17
            get(self, self.url, sha256=sha256, strip_root=True)
            ConanException: The source backup server 'https://artifactory.example.org/artifactory/source-backup/' needs authentication: {
  "errors" : [ {
    "status" : 401,
    "message" : "Bad credentials"
  } ]
}. Please provide 'source_credentials.json'
@shoeffner
Copy link
Contributor Author

I sorted the commits in the following order:

  • I added two additional requirements missing from the dev_requirements which I needed to run the tests (PyJWT and pluginbase).
  • I added (failing) tests and changed the response of the TestFileServer to contain "Bad credentials" if there are bad credentials to distinguish between missing and bad credentials (there was no distinction before)
  • I re-added my changes to make the tests pass

I am also fighting with my git settings, hence the "unverified" commits in between... Now they should all be signed properly.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Many thanks for this contribution!

@@ -4,3 +4,5 @@ parameterized>=0.6.3
mock>=1.3.0, <1.4.0
WebTest>=2.0.18, <2.1.0
bottle
PyJWT
Copy link
Member

Choose a reason for hiding this comment

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

Good, we used to install requirements_server.txt for this, but I agree that this shouldn't be necessary and installing the dev requirements should be enough to run the tests too.

@@ -77,6 +77,27 @@ def test_download_forbidden(self, _manual):
download(conanfile, file_server.fake_url + "/forbidden", dest)
assert "403 Forbidden" in str(exc.value)

def test_download_unauthorized_no_credentials(self, _manual):
Copy link
Member

Choose a reason for hiding this comment

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

Excellent. Adding a new test instead of modifying existing ones is generally better, nice one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

@memsharded memsharded merged commit c412990 into conan-io:develop2 Mar 29, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] response error message in file_downloader is missing for 401 responses
3 participants