[GitHub] Add syntax to allow specific teams in a GitHub organization

[j0nnyr0berts]

• Currently it is only possible to specify 'allowed_organisations' and not teams within those organisations.
• This PR enables specifying allowed teams in the format: allowed_organisations = <organisation>:<team>
• Existing organisation check functionality is unchanged

---

Working notes by Erik

• Closes [GitHub] We can authorize organizations, but what about teams within them? #265
• Action point: write tests
• Action point: decide on API (allowed_organizations to accept <org>:<team> syntax)
• Action point: add relevant documentation

[manics]

@sgibson91 since you just commented on #265 (comment) would you mind testing this PR, and then merging if it works?

[sgibson91]

@manics on my to-do list for tomorrow! Is there a recommended testing workflow?

Existing organisation check functionality is unchanged

@j0nnyr0berts just so I understand correctly. The behaviour I'm particularly looking for is member of org OR member of team in org. Is this what you're implying with the above statement? Update: I may need to double-check I've understood the requirements here (I'm translating someone else's workflow :D )

[consideRatio]

Thank you @j0nnyr0berts for this work!! I love to see that you have already provided a lot of test code! ❤️ 🎉

I'd like a think through about the API a bit. This makes allowed_organizations accept a org:team syntax and I wonder if we should instead have allowed_teams or similar or not. I just want us to think about it a bit and motivate the choice a bit.

Questions in my mind that may influence my opinion on the API choice
Q: do organizations members include all team members?
Q: is there a more tightly scoped permission than read:org such as read:team available?

This PR would also need an update of the documentation to make sure users become aware of the new feature. It should also be marked as "closes #265" to automatically close that issue.

[sgibson91]

I'd like a think through about the API a bit. This makes allowed_organizations accept a org:team syntax and I wonder if we should instead have allowed_teams or similar or not. I just want us to think about it a bit and motivate the choice a bit.

This would have been how I approached it. As in my above comment, I'm not sure if I'd need an or conditional (allow members from a given org or a given team).

[j0nnyr0berts]

@manics on my to-do list for tomorrow! Is there a recommended testing workflow?

Existing organisation check functionality is unchanged

@j0nnyr0berts just so I understand correctly. The behaviour I'm particularly looking for is member of org OR member of team in org. Is this what you're implying with the above statement?

Hey! Functional testing is a little bit of a work up (or at least the way I approached it!). I created an organisation and team using my personal Github and generated a test oauth token to use with in the full example Dockerfile. Let me know if you could use a hand.

[j0nnyr0berts]

I'd like a think through about the API a bit. This makes allowed_organizations accept a org:team syntax and I wonder if we should instead have allowed_teams or similar or not. I just want us to think about it a bit and motivate the choice a bit.

This would have been how I approached it. As in my above comment, I'm not sure if I'd need an or conditional (allow members from a given org or a given team).

Purely given ambiguity in interpretation here, I'd be all for having a separate allowed_teams input.

[j0nnyr0berts]

Thank you @j0nnyr0berts for this work!! I love to see that you have already provided a lot of test code! ❤️ 🎉

I'd like a think through about the API a bit. This makes allowed_organizations accept a org:team syntax and I wonder if we should instead have allowed_teams or similar or not. I just want us to think about it a bit and motivate the choice a bit.

Questions in my mind that may influence my opinion on the API choice
Q: do organizations members include all team members?
Q: is there a more tightly scoped permission than read:org such as read:team available?

This PR would also need an update of the documentation to make sure users become aware of the new feature. It should also be marked as "closes #265" to automatically close that issue.

Unfortunately, read:org is required for the teams endpoint

[manics]

I tried to add an external collaborator to a team a few months ago and it wasn't possible, so I presume team members have to be a subset of organisation members.

@sgibson91 for testing i think setting up a JupyterHub and checking only members in the specified team can login is sufficient. The unit tests check the logic but unfortunately they can't check an external API response for real. If they could reviewing PRs in this repo would be a lot easier!

[consideRatio]

@sgibson91 @manics @j0nnyr0berts what do you think makes for a good configuration API exposed to users?

1. Adding support for allowed_organizations to accept strings with the syntax org_name:team_name.
2. Adding configuration allowed_teams accepting strings with the syntax org_name:team_name.
3. Something else?

I think having allowed_teams will make our code a bit more easy to read, and a bit more obvious to the end user through its name, so I'm currently inclined for 2.

[sgibson91]

I think I would expect to be able to use allowed_organizations to, by default, allow everyone in my org and allowed_teams to specify a team. That might be too complicated to resolve in the code though?

[sgibson91]

Can we see any more edge cases?

• a single org is defined with multiple teams, but not all teams belong to that org
• ...

[consideRatio]

I think I would expect to be able to use allowed_organizations to, by default, allow everyone in my org and allowed_teams to specify a team. That might be too complicated to resolve in the code though?

Is it correct that you mean to say you assume by using both, you can get the union of org_a, org_b, and org_c:team_c1 for example? I agree, that is what I'd expect and assume we would implement as well if we have allowed_teams.

# Example that can make sense
c.GitHubOAuthenticator.allowed_organizations = ["org_a", "org_b"]
c.GitHubOAuthenticator.allowed_teams = ["org_c:team_c1", "org_c:team_c2"]

# Example that doesn't make sense and perhaps should cause
# a warning that org_d:team_d1 won't make sense to specify if
# org_d is allowed already.
c.GitHubOAuthenticator.allowed_organizations = ["org_d"]
c.GitHubOAuthenticator.allowed_teams = ["org_d:team_d1"]

[consideRatio]

a single org is defined with multiple teams, but not all teams belong to that org

Hmm... But not all teams belong to that org? I think GitHub Teams are always associated with an org, and based on @manics answer above, I assume also that all team members of an org are members of the org as well.

[sgibson91]

Is it correct that you mean to say you assume by using both, you can get the union of org_a, org_b, and org_c:team_c1 for example? I agree, that is what I'd expect and assume we would implement as well if we have allowed_teams.

Exactly!

Hmm... But not all teams belong to that org? I think GitHub Teams are always associated with an org, and based on @manics answer above, I assume also that all team members of an org are members of the org as well.

I think I meant something like the below example. You have defined an org with allowed_organizations and used allowed_teams to define a team that belongs to a different org (not that it is org-less). But judging from your code example, this wouldn't be an issue?

# Example that can make sense
c.GitHubOAuthenticator.allowed_organizations = ["org_a", "org_b"]
c.GitHubOAuthenticator.allowed_teams = ["org_c:team_c1", "org_c:team_c2"]

[consideRatio]

But judging from your code example, this wouldn't be an issue?

I think the current code implementation is to lookup one org OR team at the time in a sequence via a REST APIcalls. An org or team membership will just influence what REST API endpoint is accessed to make the lookup, so X orgs + Y teams is no more problematic than X orgs or Y teams in isolation.

[manics]

Either seems fine to me. If you asked me to choose I'd go for a single property allowed_organizations instead of two, because

allowed_organizations = ['jupyterhub', 'other-org:team']

is equivalent to

allowed_teams = ['jupyterhub:<all-members>', 'other-org:team']

[consideRatio]

Okay 👍 for going with this api if @sgibson91 agree then!

[sgibson91]

Okay 👍 for going with this api if @sgibson91 agree then!

Sure, so long as we have some docs! :)

[consideRatio]

To summarize then, @j0nnyr0berts update of documentation is what remains I think!

[j0nnyr0berts]

I will also look to update the zero-to-jupyterhub-k8s auth docs if/when the k8s-hub image is updated to include these changes.

[sgibson91]

I'm happy for this to be merged if others are :) Thank you @j0nnyr0berts!

[consideRatio]

I will also look to update the zero-to-jupyterhub-k8s auth docs if/when the k8s-hub image is updated to include these changes.

Aaaaah thank you, you are welcome to do so right away in z2jh if you want @j0nnyr0berts, I'll cut a release quickly with this.

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[consideRatio]

Thank you @j0nnyr0berts, @sgibson91, and @manics !

[consideRatio]

This will be released as part of version 14.2.0