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

fix: ape plugins list bad auth and unnecessary requests #2365

Merged
merged 3 commits into from
Nov 1, 2024
Merged
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
32 changes: 29 additions & 3 deletions src/ape/utils/_github.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from pathlib import Path
from typing import Any, Optional, Union

from requests import Session
from requests import HTTPError, Session
from requests.adapters import HTTPAdapter
from urllib3.util.retry import Retry

Expand Down Expand Up @@ -224,8 +224,34 @@ def _get(self, url: str, params: Optional[dict] = None) -> Any:
def _request(self, method: str, url: str, **kwargs) -> Any:
url = f"{self.API_URL_PREFIX}/{url}"
response = self.__session.request(method, url, **kwargs)
response.raise_for_status()
return response.json()

try:
response.raise_for_status()
except HTTPError as err:
if err.response.status_code == 401 and self.__session.headers.get("Authorization"):
token = self.__session.headers["Authorization"]
del self.__session.headers["Authorization"]
response = self.__session.request(method, url, **kwargs)
try:
response.raise_for_status() # Raise exception if the retry also fails
except HTTPError:
# Even without the Authorization token, the request still failed.
# Raise the original error in this case. Also, put back token just in case.
self.__session.headers["Authorization"] = token
raise err
else:
# The request failed with Authorization but succeeded without.
# Let the user know their token is likely expired.
logger.warning(
"Requests are not authorized! GITHUB_ACCESS_TOKEN is likely expired; "
"received 401 when attempted to use it. If you need GitHub authorization, "
"try resetting your token."
)
return response.json()

else:
# Successful response status code!
return response.json()


github_client = _GithubClient()
3 changes: 2 additions & 1 deletion src/ape_plugins/_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ def _display_all_callback(ctx, param, value):
help="Display all plugins installed and available (including Core)",
)
def _list(cli_ctx, to_display):
metadata = PluginMetadataList.load(cli_ctx.plugin_manager)
include_available = PluginType.AVAILABLE in to_display
metadata = PluginMetadataList.load(cli_ctx.plugin_manager, include_available=include_available)
if output := metadata.to_str(include=to_display):
click.echo(output)
if not metadata.installed and not metadata.third_party:
Expand Down
59 changes: 58 additions & 1 deletion tests/functional/utils/test_github.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from pathlib import Path

import pytest
from requests.exceptions import ConnectTimeout
from requests.exceptions import ConnectTimeout, HTTPError

from ape.utils._github import _GithubClient
from ape.utils.os import create_tempdir
Expand Down Expand Up @@ -98,3 +98,60 @@ def test_get_org_repos(self, github_client, mock_session):
params = call.kwargs["params"]
# Show we are fetching more than the default 30 per page.
assert params == {"per_page": 100, "page": 1}

def test_available_plugins(self, mocker, github_client, mock_session):
response1 = mocker.MagicMock()
response1.json.return_value = [{"name": "ape-myplugin"}]
response2 = mocker.MagicMock()
response2.json.return_value = []

def get_org_repos(method, url, **kwargs):
if kwargs["params"]["page"] == 1:
return response1
else:
# End.
return response2

mock_session.request.side_effect = get_org_repos
actual = github_client.available_plugins
assert actual == {"ape_myplugin"}

def test_available_plugins_handles_401(self, mocker, github_client, mock_session, ape_caplog):
"""
When you get a 401 from using a token, Ape's GitHub client should not
only warn the user but retry the request w/o authorization, as it likely
will still work.
"""
mock_session.headers = {"Authorization": "token mytoken"}

response1 = mocker.MagicMock()
response1.json.return_value = [{"name": "ape-myplugin"}]
response2 = mocker.MagicMock()
response2.json.return_value = []

bad_auth_response = mocker.MagicMock()
bad_auth_response.status_code = 401
bad_auth_response.raise_for_status.side_effect = HTTPError(response=bad_auth_response)

def get_org_repos(method, url, **kwargs):
if mock_session.headers.get("Authorization") == "token mytoken":
return bad_auth_response
elif kwargs["params"]["page"] == 1:
return response1
else:
# End.
return response2

mock_session.request.side_effect = get_org_repos
actual = github_client.available_plugins

# Still works, even with bad auth.
assert actual == {"ape_myplugin"}

# Show we got our log message.
expected = (
"Requests are not authorized! GITHUB_ACCESS_TOKEN is likely "
"expired; received 401 when attempted to use it. If you need "
"GitHub authorization, try resetting your token."
)
assert ape_caplog.head == expected
Loading