feat: add team membership check to GitHub authentication method #2891
Conversation
|
I've tested it locally and it worked! |
There was a problem hiding this comment.
Change looks awesome. Thanks for taking the time to contribute it and the attention to tests is great. Nice improvements in the server tests there 💅.
I put one clarifying question for my own sake and one take it or leave it style suggestion. But it primarily looks great to me and im happy to merge as-is if you want to vito the style suggestion and we're happy with assuming folks have less than 100 memberships.
| teamOk := true | ||
| if userTeamsByOrg != nil { | ||
| allowedTeams := s.config.Methods.Github.Method.AllowedTeams[org] | ||
| userTeams := userTeamsByOrg[org] | ||
|
|
||
| teamOk = slices.ContainsFunc(allowedTeams, func(team string) bool { | ||
| return userTeams[team] | ||
| }) | ||
| } | ||
|
|
||
| return teamOk |
There was a problem hiding this comment.
Take it or leave it: You could save a little on the right hand side justification here by returning early.
| teamOk := true | |
| if userTeamsByOrg != nil { | |
| allowedTeams := s.config.Methods.Github.Method.AllowedTeams[org] | |
| userTeams := userTeamsByOrg[org] | |
| teamOk = slices.ContainsFunc(allowedTeams, func(team string) bool { | |
| return userTeams[team] | |
| }) | |
| } | |
| return teamOk | |
| if userTeamsByOrg == nil { | |
| return true | |
| } | |
| allowedTeams := s.config.Methods.Github.Method.AllowedTeams[org] | |
| userTeams := userTeamsByOrg[org] | |
| return slices.ContainsFunc(allowedTeams, func(team string) bool { | |
| return userTeams[team] | |
| }) |
There was a problem hiding this comment.
Sure! I'll change it and rebase with main!
| @@ -28,6 +28,7 @@ const ( | |||
| githubAPI = "https://api.github.com" | |||
| githubUser endpoint = "/user" | |||
| githubUserOrganizations endpoint = "/user/orgs" | |||
| githubUserTeams endpoint = "/user/teams?per_page=100" | |||
There was a problem hiding this comment.
I take it we're just going to assume likelihood is 99.9% of folks have less that 100 team memberships and cross that bridge when we come to it for the outliers?
Seems reasonable to me, just clarifying.
There was a problem hiding this comment.
Exactly, I can't image someone having more than 100 team memberships (even they may exists 😆)!
Also, this is the same number used by Grafana as @markphelps pointed out: SocialGithub.fetchTeamMemberships
There was a problem hiding this comment.
That's awesome. Makes total sense.
thepabloaguilar
force-pushed
the
issue-2849
branch
from
d11da42
to
c926603
Compare
|
Thanks a lot for you feedback @GeorgeMac, I really appreciate it! |
There was a problem hiding this comment.
thank you @thepabloaguilar !! amazing work and thanks for all the tests too!
|
@all-contributors please add @thepabloaguilar for code |
|
I've put up a pull request to add @thepabloaguilar! 🎉 |
As we've discussed in the related issue I'm opening this PR to introduce a new filter for GitHub OAuth method allowing people to define
allowed_teamsplus toallowed_organizations!Closes #2849