Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: clean up inactive users from the org, teams and repos #12

Merged
merged 20 commits into from
Jun 10, 2022
Merged

Conversation

galargh
Copy link
Contributor

@galargh galargh commented May 18, 2022

PR Description from the draft stage

Dear reviewers

This is a first run at cleaning up libp2p org GitHub configuration. I want to put it out there to start a conversation and refine the approach. I'll leave it in a draft state until we're happy with the results. Feedback highly appreciated 🙇

Description

Approach

TLDR Remove a user from org/team/repo if they didn't perform any actions related to that identity in the last 12 months. See the script that I implements the approach.

Audit Log

Use the audit log as the source of information for the activity within libp2p organisation. It can be downloaded as a JSON from organisation settings page (that's what I did).

The audit log is a list of actions that happened inside the organisation.

There is no information on what kind of permissions a specific action required but I think we could prepare it if we wanted to start answering questions like:

  • When did user A last perform an action that required organisation admin permissions?
  • Did action B require write access to repository C?

The audit log can be accessed through API only in GitHub Enterprise which we don't have.

Organisation Members

If a user didn't perform any action in the organisation in the past 12 months, remove them from the organisation.

Repository Collaborators

For each repository in the organisation, if a repository collaborator didn't perform any action in the repository in the past 12 months, remove them from the repository.

Team Members

For each team in the organisation, if a team member didn't perform any action in any of the repositories that are connected to that team in the past 12 months, remove them from the team.

If a team has no members, delete the team and team repositories.

Result

Using this approach we'd remove 515 resources altogether.

Open Questions/Ideas

  • If none of the team members were active in a repository that is assigned to the team, should we remove that repository from the team? I think not (because, for example, even when there is no activity we'd like all go repos to be assigned to the Go repos team) but it's something I considered and that's what I implemented first.
  • Audit log is missing read actions. I suspect that's how protocollabsit-readonly user is being removed from the org. Are there any other actions it might be missing?
  • We should be careful about recently added users. For example, I've seen ajnavarro being removed from w3dt-stewards team but they have only been added recently so I assume they didn't have time to perform any actions just yet.
  • Leave only PL leadership as org admins.
  • Give w3dt-stewards write access to this repository.
  • Remove collaborators with only pull permissions to public repos (it is redundant).
  • Remove collaborators which also have access to a repository through some team.
  • Remove collaborators who are also org members. Add them to relevant teams so that they don't loose access.
  • Cap max permission for teams/collaborators at push ⚠️ . This will need more consideration. In particular because only Admins receive Dependabot alerts fro vulnerable dependencies in a repository.

Description

This PR removes org/team/repo members if they have been inactive in that context for the past 12 months. Some changes have been adjusted following the discussions that happened while the PR was in a draft stage.

This PR is going to get merged at the EOD Friday, June 10.

FAQ

How do I check why I was mentioned?

The best way to check it is to search for your GitHub username in the terraform plan output that is part of this workflow run output: https://github.com/libp2p/github-mgmt/runs/6808695393?check_suite_focus=true

Remember to expand the Show terraform plans section.

What do I do if my permissions are being removed but I do need them?

Please comment on the appropriate line and I (@galargh) will revert that particular change.

What do I do if I notice this after the PR was merged and my permissions that I need were removed?

All you have to do is to make a PR to this repo that adds your permissions back. You can have a look at this doc for guidance.

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2022

Before merge, verify that all the following plans are correct. They will be applied as-is after the merge.

Terraform plans

Terraform plans are too long to post as a comment. Please inspect Plan > Comment > Show terraform plans instead.

@mxinden

This comment was marked as resolved.

@mxinden

This comment was marked as resolved.

@mxinden

This comment was marked as resolved.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a first look. Thanks @galargh for pushing this forward.

How about we tag everyone in a comment that is about to lose some kind of permissions. Best to communicate this explicitly, instead of silently taking away permissions. Can include something along the lines of: "Happy to revert the change in case you ever require the permissions again".

github/libp2p/membership.json Outdated Show resolved Hide resolved
"permission": "push"
}
},
"developer-meetings": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hasn't been touched since 2018. Can we just archive the hole repository?

@mxinden
Copy link
Member

mxinden commented May 22, 2022

Also tagging the other libp2p team members: @marten-seemann @MarcoPolo @elenaf9 @achingbrain.

@galargh

This comment was marked as resolved.

@galargh
Copy link
Contributor Author

galargh commented May 23, 2022

Took a first look. Thanks @galargh for pushing this forward.

How about we tag everyone in a comment that is about to lose some kind of permissions. Best to communicate this explicitly, instead of silently taking away permissions. Can include something along the lines of: "Happy to revert the change in case you ever require the permissions again".

Thank you for the review! I really appreciate it :)

Yes! I think that's exactly what we should do. Tag everyone, ask their opinion of the change, give instructions on how to revert it if they need to do so in the future and set a specific date when this PR will get merged. I just wanted to know your opinion on the direction first before tagging everyone.

@mxinden

This comment was marked as resolved.

github/libp2p/membership.json Outdated Show resolved Hide resolved
github/libp2p/membership.json Outdated Show resolved Hide resolved
github/libp2p/membership.json Outdated Show resolved Hide resolved
github/libp2p/membership.json Outdated Show resolved Hide resolved
github/libp2p/membership.json Outdated Show resolved Hide resolved
github/libp2p/membership.json Outdated Show resolved Hide resolved
github/libp2p/membership.json Outdated Show resolved Hide resolved
@galargh
Copy link
Contributor Author

galargh commented Jun 8, 2022

I'm tagging all affected users for visibility:

Affected users (part 3)

@galargh galargh marked this pull request as ready for review June 8, 2022 07:12
@willscott
Copy link
Contributor

willscott commented Jun 8, 2022

What does 'inactive in the last 12 months' mean? I have been making PRs to repos in this org (example)

edit: it looks like for many affected people it's removing them from specific repos they've been inactive on, not full org membership

@vyzo
Copy link

vyzo commented Jun 8, 2022

you must be doing something wrong, i am very much active.

@galargh
Copy link
Contributor Author

galargh commented Jun 8, 2022

In general, I believe we'd want to keep the number members to a minimum - especially that being added back just got a whole lot easier. @mxinden @BigLep What do you think?

Why?
For more context: I believe I was the one who actually set up this org in the first place. It's nice to be a legacy member. :)

One thing is that, even without extra assignments, being an org member comes with some permissions - like read access to private repos, ability to create repos, etc. I don't think it's a deal breaker but certainly something to consider.

From looking here and here, it looks like the permissions someone gains by being an organization member are:

  1. Create repositories
  2. Create teams
  3. Create project boards
  4. See all organization members and teams
  5. @mention any visible team
  6. View and post public team discussions to all teams
  7. Hide comments on writable commits, pull requests, and issues

I don't believe org gives them read access to private repositories (I can anecdotally confirm this as I had to be manually added libp2p-vulnerabilities some time back). As a result, none of those actions seem critical from an OpSec regard with folks that have previously been active contributors to the libp2p community.

Unless anyone spots other concerns, I'm on board with creating a "inactive-alumni" team that non-active members are part of (but that doesn't have permissions for any specific repos). This gives a nod to their historical role and allows an alumni to show the org badge in their GitHub profile.

I double checked and our current base permission for org members is Read which gives all the org members read access to private repos too. I think it might make sense to go back to No permission by default so that we're more explicit about who has access to what. Let's do it at a later date though since this change is quite massive already.

Tomorrow, I'll prepare a PR (targeting this branch) with the alumni team setup.

@vyzo
Copy link

vyzo commented Jun 8, 2022 via email

Comment on lines 1593 to 1625
"py-libp2p": {
"alexh": {
"permission": "push"
},
"carver": {
"permission": "admin"
},
"ralexstokes": {
"permission": "admin"
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that I'm not active. I don't have any special desire to be a maintainer, though there have been times in the past when there was no one available/capable to take care of maintenance and I was happy to do some work. Sometimes that work has required admin access (like adding a readthedocs url hook to fix the doc builds).

My main concern here is that it seems like everyone is being removed from the repo. Presumably I'm missing something about how permissions are working here. I would just worry about getting things working quickly in some sort of emergency.

If you feel like leaving me on as a backup maintainer, I'm cool with that. Otherwise, I'd just ask that it's really clear who we can communicate with, in the case of an urgent need.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a valid point - I added you back as an admin to that repo - eec0f22

"role": "member"
},
"MarcoPolo": {
"role": "member"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this section for? Should I be readded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is team_membership in contributors team.

Copy link

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the fact that someone created a repo should give them admin permissions. We start repos as individuals, and move them into the libp2p org because they should be a part of libp2p. I see that I'm still the admin for some of the repos that I've created, and I think that those special permissions should be revoked.

@RichardLitt
Copy link
Member

@marten-seemann if you're referring to my comments - I don't want admin permissions. I'd just like to stay in the org, please. It sucked to be removed from @ipfs, and I'd rather be seen as someone who was essential to this community with the badge. Agreed on the lack of admin permissions.

@galargh
Copy link
Contributor Author

galargh commented Jun 10, 2022

I'm not sure why the fact that someone created a repo should give them admin permissions. We start repos as individuals, and move them into the libp2p org because they should be a part of libp2p. I see that I'm still the admin for some of the repos that I've created, and I think that those special permissions should be revoked.

It's certainly not the last step of auditing permissions in our orgs - including libp2p - and I do hope we eventually get to a point where we don't have admin powers where not needed. The most immediate next steps are:

@galargh
Copy link
Contributor Author

galargh commented Jun 10, 2022

As per the description, I'm merging the PR today. Thank you all for your participation, I appreciate it a lot ❤️ ❤️ ❤️

If you find yourself missing some permissions, remember that all you have to do is to make a PR to this repo that adds your permissions back. You can have a look at this doc for guidance. Feel free to ping me as well 😄

@dhuseby
Copy link
Collaborator

dhuseby commented Feb 9, 2024

@galargh and @BigLep one thing I noticed in the permissions is that we have maintainer "teams defined" for some repos:

github-mgmt/github/libp2p.yml

Lines 8182 to 8191 in e1c4dfa

go-libp2p Maintainers:
description: Trusted reviewers for merging into go-libp2p repositories.
members:
maintainer:
- BigLep
- MarcoPolo
- marten-seemann
member:
- sukunrt
- vyzo

and

github-mgmt/github/libp2p.yml

Lines 8294 to 8303 in e1c4dfa

rust-libp2p Maintainers:
description: Trusted reviewers for merging into rust-libp2p repositories and
publishing to crates.io.
members:
maintainer:
- mxinden
member:
- jxs
- MarcoPolo
- thomaseizinger

and

github-mgmt/github/libp2p.yml

Lines 8199 to 8203 in e1c4dfa

js-libp2p ChainSafe Maintainers:
members:
member:
- mpetrunic
- wemeetagain

this isn't consistent across all of the repos. is that the "preferred" way to do this? if so, can we just have a team per implementation and group them all close together at the top so it's easier to understand this file?

@dhuseby
Copy link
Collaborator

dhuseby commented Feb 9, 2024

Also, what is the significance of the list of "members"?

member:
- 2color
- aamnv
- aarshkshah1992

@dhuseby
Copy link
Collaborator

dhuseby commented Feb 9, 2024

Also, IMO, this list should be extremely small (3-5 people).

admin:
- andyschwab-admin
- aschmahmann
- daviddias
- galargh
- jacobheun
- jbenet
- marten-seemann
- momack2
- raulk
- Stebalien
- whyrusleeping

Probably just the libp2p Foundation board members.

@BigLep
Copy link
Contributor

BigLep commented Feb 13, 2024

Hi @dhuseby. A 2024 round of cleanup is underway as part of ipfs/ipfs#511.

I'll put a couple of comments here, but we should discuss more as new PRs are opened this week:

one thing I noticed in the permissions is that we have maintainer "teams defined" for some repos. this isn't consistent across all of the repos. is that the "preferred" way to do this?

I think it makes sense to have a maintainer team per implementation. That way all repos related to that implementation can declare the maintainer team as the admins. It also makes it easy to @mention the team in PRs

if so, can we just have a team per implementation and group them all close together at the top so it's easier to understand this file?

Currently github-mgmt does a lexigraphical sort. There is a backlog item to start changing the sort function: ipdxco/github-as-code#114

Also, what is the significance of the list of "members"?

That list of members are those who are part of the github Organization.

Also, IMO, this list should be extremely small (3-5 people).

Agreed. That is phase 1 in ipfs/ipfs#511. It is getting handled in #202

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet