search

github-education-resources / classroom

GitHub Classroom automates repository creation and access control, making it easy for teachers to distribute starter code and collect assignments on GitHub.
https://classroom.github.com
1.34k stars 564 forks source link

Remove user from all classrooms tied to a GH organization that has revoked their access #2480

Open JessRudder opened 4 years ago

JessRudder commented 4 years ago

Addresses to https://github.com/github/education/issues/988

Our previous code assumed a 1-to-1 mapping of GitHub organizations to classrooms. However, many of our teachers are using a single organization on GitHub as a source for multiple classrooms.

This update ensures that when access is revoked for a GitHub organization for a user, the user is removed from all of the associated classrooms as well.

Previously we would not remove classroom access if a user was the only user with admin access to the classroom. With this update, we will be removing access regardless. In a small number of cases, this may result in admin-less classrooms.

spinecone commented 4 years ago

Should we run a migration/transition to clean up existing user/classroom associations that should have been removed already?

JessRudder commented 4 years ago

@spinecone probably...since it's triggered by a webhook when someone's removed from the GitHub organization, I don't think there's a simple way of doing it. We'll likely need to iterate through every user/github_organization combo and make sure the user still has access (though I'm definitely open to a cleaner way of doing it).

Should that be part of this PR so we know when/why it was added? Or should that come in a follow on PR?

spinecone commented 4 years ago

I don't know of a cleaner way than iterating through every user (or organization, whichever has fewer records), I would just make sure to use as few github.com API requests as possible so we don't hit any rate limits. It seems cleaner to me to have everything in this one PR, but I defer to what makes the most sense to you.

JessRudder commented 4 years ago

@spinecone I prefer having things together, but I end up getting a lot of feedback that my PRs are too big, so, I'm probably overly cautious now. I'll get it added in and re-ping for review.

JessRudder commented 4 years ago

@spinecone this should be ready to review again. I've added a rake task that will go through and check for any records that may need to be cleaned up. It relies on the client for the user that is being checked, so, we shouldn't end up hitting any API limits. I went with a rake task instead of a migration as this may be something we need to run again in the future.

Also, no need to rush the check on this. I won't be deploying it this late on a holiday weekend and I won't be back until Tuesday, so there's plenty of time.