feat: WIP Github Oauth integration #159
Conversation
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def get_gh_username(user: User) -> str | None: |
There was a problem hiding this comment.
How the caching is supposed to work? Or is it that everytime we hit those tags, we run a query to GitHub API?
There was a problem hiding this comment.
Right now there's no caching, and that's a problem of course. The tags are going away, they just allowed me to quickly work with the Github calls.
My plan is to move the logic to map Github memberships to Django's group membership and do authorization from there. These mappings would act as a cache and then the question will be how often do we want to refresh them.
There was a problem hiding this comment.
In that case, it must be very explicit what is the upperbound on the refresh for the authorizations and I suggest to have an systemd timer to refresh them every X and let user figure out what they want.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def get_team_member_ids(orgname: str, teamname: str) -> set[int]: |
There was a problem hiding this comment.
Any reason not to use @functools.lru_cache with an appropriate size?
There was a problem hiding this comment.
When I was mentioning caching I was thinking about database caching and getting the info from there, but I see that you meant memoization.
I'll implement it with functools decorators. I think to make it work the memoized function will have to be triggered by a signal so it's always executed by the running webserver process, otherwise the command will not profit from the memoized cache.
Then the systemd timer can call this command by invalidating first the cache.
As an alternative to the systemd timer, I saw that cachedtools has timed versions of functools caches with TTLCache. I haven't read the implementation yet to see the tradeoffs, and it would also mean configuring the TTL at settings level, which seems less flexible that changing the refresh timer.
src/website/shared/auth.py
Outdated
| social_user.socialaccount_set.filter(provider="github").first() # type: ignore | ||
| ) | ||
| if social_account: | ||
| return social_account.extra_data.get("login") # type: ignore |
There was a problem hiding this comment.
Sounds like what you care about is .values_list('extra_data__login') or something like that, no? You could get rid of the many attributes that could get passed via the SQL query.
There was a problem hiding this comment.
Doing
def get_gh_username(user: User) -> str | None:
"""
Return the Github username of a given Auth.User.
"""
social_user = User.objects.get(id=user.id) # type: ignore
try:
extra_login: str = (
social_user.socialaccount_set.filter(provider="github")
.values_list('extra_data__login', flat=True)[0]
)
return extra_login
except Exception:
logger.exception("Failed to get GitHub username for user %s.", user)
return None
Gives me
django.core.exceptions.FieldError: Unsupported lookup 'login' for JSONField or join on the field not permitted.
For now I'll keep the extra_data.get("login") and give this another spin later.
The permissions given to maintainers are limited to objects from models related to the derivations containing them in their maintainers metadata field. This means that the maintainers group and fetching for nixpkgs-maintainers team membership is not needed to assign their relevant per-object permissions.
Make intention more clear by removing the function and callling get_team_member_ids inside the update_group command.
This way it's possible to point them at testing values.
This in case a request comes bypassing the UI form, which can be replicated in the test case. When users are using the UI form, it already gets correctly populated with only valid options.
The caches need to be invalidated when the state of users change (added/removed from groups, from metadata.maintainers, etc). lru_cache does not allow for fine-grained invalidation so the correct caching mechanism needs to be considered before going for this optimization.
| ) | ||
| self.assertContains(redirect_response, "1 nixpkgs issue", status_code=200) | ||
|
|
||
| def test_committer_cannot_add_non_related_issue_from_admin_site(self) -> None: |
There was a problem hiding this comment.
| def test_committer_cannot_add_non_related_issue_from_admin_site(self) -> None: | |
| def test_maintainer_cannot_add_non_related_issue_from_admin_site(self) -> None: |
This only tests what maintainers can do
There was a problem hiding this comment.
Also there seems to be no test for actual committers.
| @lru_cache(maxsize=1) | ||
| def get_gh_organization(orgname: str) -> Organization | None: | ||
| """ | ||
| Return the Github Organization instance given an organization name. | ||
| """ | ||
| try: | ||
| return github.get_organization(login=orgname) | ||
| except Exception: | ||
| logger.exception("Failed to get organization %s", orgname) | ||
| return None |
There was a problem hiding this comment.
This is not needed as we're only caching two teams, so it's at most two calls to get_organization for an application lifetime.
| """ | ||
| Return whether a given username is a member of a Github team | ||
| """ | ||
| gh_named_user: NamedUser = cast(NamedUser, github.get_user(login=username)) |
There was a problem hiding this comment.
We can use get_user_by_id here, which returns a NamedUser
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class Command(BaseCommand): |
There was a problem hiding this comment.
We probably rather want to set up a webhook on team membership events
No description provided.