Skip to content

Commit

Permalink
chore(github): address Adam Thornton's review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
vit-zikmund committed Dec 10, 2024
1 parent 2c0d562 commit f7370eb
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 7 deletions.
4 changes: 2 additions & 2 deletions docs/source/auth-providers.md
Original file line number Diff line number Diff line change
Expand Up @@ -205,13 +205,13 @@ The GitHub repository permissions are mapped to [Giftless permissions](#permissi
To minimize the traffic to GitHub for each LFS action, most of the auth data is being temporarily cached in memory, which improves performance, but naturally also ignores immediate changes for identities with changed permissions.

### GitHub Auth Flow
Here's a description of the authentication & authorization flow. If any of these steps fails, the request gets rejected. As the supported token flavors have a very different way of authentication, they're described separately:
Here's a description of the authentication & authorization flow. If any of these steps fails, the request gets rejected. As the supported token flavors have very different ways of authentication, they're described separately:

#### Personal Access Tokens (`ghp_`, `_github_pat_` and likely other [token flavors](https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/about-authentication-to-github#githubs-token-formats) `gho_`, `ghu_`)
These tokens eventually represent a real user. For the authenticator to work properly, the token must have these permissions:
- `read:org` for "Classic" or
- `metadata:read` for the fine-grained kind.
- The user has to be a collaborator of the target repository with an adequate role for reading or writing.
- The user has to be a collaborator on the target repository with an adequate role for reading or writing.

1. The URI of the primary git LFS (HTTP) [`batch` request](https://github.com/git-lfs/git-lfs/blob/main/docs/api/batch.md) is used to determine what GitHub organization and repository is being targeted (e.g. `https://<server>/<org>/<repo>.git/info/lfs/...`). The request's `Authentication` header is searched for the required token in the `password` part of the `Basic` HTTP auth.
2. The token is then used in a [`/user`](https://docs.github.com/en/rest/users/users?apiVersion=2022-11-28#get-the-authenticated-user) GitHub API call to get its identity data.
Expand Down
17 changes: 13 additions & 4 deletions giftless/auth/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,8 @@ def _extract_auth(self, request: flask.Request) -> tuple[str | None, str]:
def api_get(self, uri: str) -> dict[str, Any]:
if self._session is None:
raise RuntimeError(
"Calling CallContext.api_get() outside of a with block."
"CallContext is a context manager maintaining a requests "
"session. Call api_get() only within its entered context."
)
response = self._session.get(
f"{self._api_url}{uri}",
Expand All @@ -365,8 +366,9 @@ def api_get_paginated(
) -> Generator[dict[str, Any], None, None]:
if self._session is None:
raise RuntimeError(
"Calling CallContext.api_get_paginated() "
"outside of a with block."
"CallContext is a context manager maintaining a requests "
"session. Call api_get_paginated() only within its entered "
"context."
)

per_page = min(max(per_page, 1), 100)
Expand All @@ -384,7 +386,13 @@ def api_get_paginated(

yield from (item for item in response_json.get(list_name, []))

# check the 'link' header for the 'next' page URL
if next_url := response.links.get("next", {}).get("url"):
# extract the page number from the URL that looks like
# https://api.github.com/some/collections?page=4
# urlparse(next_url).query returns "page=4"
# parse_qs() parses that into {'page': ['4']}
# when 'page' is missing, we supply a fake ['0'] to stop
next_page = int(
parse_qs(urlparse(next_url).query).get("page", ["0"])[0]
)
Expand Down Expand Up @@ -424,7 +432,8 @@ def _perm_ttl(perms: set[Permission]) -> float:
else:
return cc.auth_other_ttl

# expiration factory providing a 'ttu' function respecting least_ttl
# expiration factory providing a 'ttu' function for 'TLRUCache'
# respecting specified least_ttl
def expiration(
least_ttl: float | None = None,
) -> Callable[[Any, set[Permission], float], float]:
Expand Down
2 changes: 1 addition & 1 deletion tests/auth/test_github.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ def test_config_schema_api_timeout() -> None:
DEFAULT_CONFIG = gh.Config.from_dict({})
DEFAULT_USER_DICT = {
"login": "kingofthebritons",
"id": "12345678",
"id": "125678",
"name": "arthur",
"email": "[email protected]",
}
Expand Down

0 comments on commit f7370eb

Please sign in to comment.