feat: add support for repo team/collaborator permissions#65
Conversation
tstollin
left a comment
There was a problem hiding this comment.
Thank you @ruizink for your contribution!
I like the design on referencing the team from the repository spec in contrast to the way the GitHub API handles this (repos are added to teams), as this approach keeps the relevant information in one spec.
Unfortunately, I found some issues that would need to be addressed before being able to merge this PR. Please refer to the attached comments.
Of course, feel free to ask if something is unclear or you want to discuss some points. :)
| // +kubebuilder:validation:MaxLength=100 | ||
| // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,99}$` | ||
| // +kubebuilder:validation:Required | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
Please use a ref to a Team. This keeps the design in line with the rest of the operator and prevents some other issues furthe down.
There was a problem hiding this comment.
Updated to use TeamRef instead of a string.
| log := logPkg.FromContext(ctx) | ||
| log.V(1).Info("Reconciling repository teams on GitHub") | ||
|
|
||
| orgTeams, err := r.GitHub.Client.GetAllTeamsForOrg(ctx, r.GitHub.Resource.Owner) |
There was a problem hiding this comment.
Getting all teams on every reconciliation is very rate-limit cost intensive. This would be fixed by using a TeamRef in the spec as the team can then be fetched by a simple kube api call. A reconciled team also carries the slug in its status.
There was a problem hiding this comment.
Removed this call when implementing TeamRef.
| } | ||
|
|
||
| // Support both team names and slugs in the repository spec. | ||
| teamIdentifierToSlug := make(map[string]string) |
There was a problem hiding this comment.
this mapping would be obsolete by using a team ref.
There was a problem hiding this comment.
Yep, removed when implementing TeamRef.
| slug := team.GetSlug() | ||
| teamIdentifierToSlug[slug] = slug | ||
| if team.Name != nil { | ||
| teamIdentifierToSlug[team.GetName()] = slug |
There was a problem hiding this comment.
possible collision if the name of one team coincides with the slug of another. Not sure if this can happen.
There was a problem hiding this comment.
Removed when implementing TeamRef.
| // RepositoryCollaboratorPermission defines repository-level access for a GitHub user. | ||
| type RepositoryCollaboratorPermission struct { | ||
| // Username is the GitHub username of the member. | ||
| // Supports standard GitHub usernames and Enterprise Managed Users-style |
There was a problem hiding this comment.
i think this will not be possible as collaborators can be either from inside the organization or external collaborators. the later will not carry the organization suffix. To support both kinds of collaborators you can either require full explicit usernames (ignoring the memberSuffixes completely), i.e. myusername_mymembersuffix or distinguish between inside and outside collaborators. But I'm not sure whether the GitHub API supports distinguishing collaborators in this way.
There was a problem hiding this comment.
As far as I am aware, suffixes are only a thing if the Organization belongs to an Enterprise with EMU. Otherwise, all usernames are "global" (without suffix).
Once inside an Enterprise with EMU, you cannot reference "global" accounts, even external collaborators need new accounts to be provisioned via IdP, which will result in those accounts having a suffix.
If my interpretation is right, then, there's no way you can have a mix of collaborators with or without suffix inside the same Organization.
| - name: security-team | ||
| permission: triage | ||
|
|
||
| # --- Team Permissions --- |
There was a problem hiding this comment.
| # --- Team Permissions --- | |
| # --- Collaborators Permissions --- |
| // Both team name and team slug are supported. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=100 | ||
| // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,99}$` |
There was a problem hiding this comment.
this regex does not support team names with spaces. But would be solved by switching to a Team ref.
There was a problem hiding this comment.
Removed when implementing TeamRef.
| - name: permission | ||
| type: | ||
| scalar: string | ||
| default: push |
There was a problem hiding this comment.
does not match https://github.com/Interhyp/git-hubby/pull/65/changes#diff-af197a37ff5fcea424a8806b2080d8123817f4a713881afaabd7d8bc8754c36fR275
I've broken the auto-generation for the applyconfigurations in one of my recent changes. I've fixed it on a branch I'm currently working on. sry for that
There was a problem hiding this comment.
this includes the fix: https://github.com/Interhyp/git-hubby/pull/67/changes#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52
EDIT: i've merged my pr, you should receive the fix upon rebasing
| if collaborator == nil || collaborator.Login == nil { | ||
| continue | ||
| } | ||
| existingCollaboratorsByName[*collaborator.Login] = *collaborator.RoleName |
There was a problem hiding this comment.
possible nil-pointer deref in *collaborator.RoleName as role name is not a required field as per the api response schema. Please use GetRoleName() or a nil check.
There was a problem hiding this comment.
Nice catch. Fixed using collaborator.GetRoleName().
There was a problem hiding this comment.
i see an important design gap in this reconciler: collaborators are invited upon being added to a repository. Until they have accepted the invite they will not be returned as collaborators and the reconcile loop would try to send out further invitations by adding the user to the repository as collaborator (if that is even possible).
This reconciler needs to be redesigned to handle open invitations and take them into consideration with regards to the desired state stored in github. additionally, it needs to update or even recall any open invitations if the permissions change.
This should map out the possible states:
| Desired | GitHub state | Action |
|---|---|---|
| ✅ Present | Not collaborator, no pending invite | AddCollaborator (send invite) |
| ✅ Present | Pending invite, same permission | No-op |
| ✅ Present | Pending invite, different permission | Cancel invite → AddCollaborator |
| ✅ Present | Accepted collaborator, same permission | No-op |
| ✅ Present | Accepted collaborator, different permission | AddCollaborator (updates permission) |
| ❌ Not present | Pending invite | Cancel invitation |
| ❌ Not present | Accepted collaborator | RemoveCollaborator |
There was a problem hiding this comment.
also we need to consider the case where users decline invitations. I'm not sure how to handle this, but I'm sure that it would be bad design to just reinvite them until they accept the invitation.
There was a problem hiding this comment.
This completely flew under the radar for me, because my Orgs belong to an Enterprise with EMU, in which case, there's no invitation workflow. Definitely something super important!
There was a problem hiding this comment.
As far as I understand you, you only need both features for an enterprise environment. I think it would be way easier for now if you just add the support for enterprise orgs (using func (o *Organization) HasEnterpriseFeatures() bool and skipping reconciliation if it returns false). This way you need not care about the invitation flow and the distiction between external or org collaborators.
The implementation for non-enterprise orgs could then be added later.
There was a problem hiding this comment.
The thing is, an Organization with Enterprise plan can still follow the invitation workflow, as long as the Enterprise doesn't use EMU (external IdP).
Team permissions on repository are not affected by this intitation workflow.
Another option for collaborators would be to check if the user is a member of the organization. Skip if it's not a member, so that no invite is ever sent. (Pretty much what's done when managing team members here). This will limit the functionality to org members, though. External collaborators, which require an invite would be skipped.
WDYT?
5fa98b8 to
2093cc9
Compare
Signed-off-by: Mário Santos <[email protected]>
2093cc9 to
e257107
Compare
Description
Adds the ability to configure permissions for both Teams and Collaborators inside a Repository
Type of Change
Checklist
Related Issues