-
Notifications
You must be signed in to change notification settings - Fork 566
Remove user from all classrooms tied to a GH organization that has revoked their access #2480
base: master
Are you sure you want to change the base?
Changes from 7 commits
4fb2bb2
32f8152
01cd5f1
ae71fcb
2a34790
3d00951
36bae66
6cf3c03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,21 +5,31 @@ class OrganizationEventJob < ApplicationJob | |
queue_as :github_event | ||
|
||
# rubocop:disable Metrics/AbcSize | ||
# rubocop:disable Metrics/MethodLength | ||
def perform(payload_body) | ||
return true unless payload_body.dig("action") == "member_removed" | ||
|
||
github_user_id = payload_body.dig("membership", "user", "id") | ||
github_organization_id = payload_body.dig("organization", "id") | ||
@organization = Organization.find_by(github_id: github_organization_id) | ||
organizations = Organization.where(github_id: github_organization_id) | ||
|
||
return false if @organization.blank? | ||
return false if @organization.users.count == 1 | ||
return false if organizations.empty? | ||
|
||
@user = @organization.users.find_by(uid: github_user_id) | ||
failed_removals = organizations.reject do |org| | ||
user = org.users.find_by(uid: github_user_id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure how many orgs we will generally have or if there is a way to avoid this N+1. It seems like it will be negligible and in a job... so... not a big deal? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried fixing this by using an includes where I grab the organizations above:
However, I end up with an error from the bullet gem:
If an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to confirm I think this is probably okay as an N+1, I don't think we will have too many of these. And I realized it is just run by the rake task. |
||
|
||
return true unless @user | ||
TransferAssignmentsService.new(@organization, @user).transfer | ||
@organization.users.delete(@user) | ||
if user | ||
TransferAssignmentsService.new(org, user).transfer | ||
org.users.delete(user) | ||
else | ||
false | ||
end | ||
end | ||
|
||
# This method should only report success (by returning true) if we were able to remove | ||
# the user from all of the organizations tied to the given github_organization_id | ||
failed_removals.empty? | ||
end | ||
# rubocop:enable Metrics/AbcSize | ||
# rubocop:enable Metrics/MethodLength | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# frozen_string_literal: true | ||
|
||
# Checks if classroom admins have access to the associated GitHub organization. If not | ||
# the admin is removed from the classroom | ||
task :remove_classroom_users_without_github_org_access do | ||
User.find_in_batches(batch_size: 250).with_index do |group, batch| | ||
puts "Processing batch ##{batch}" | ||
group.each do |user| | ||
github_org_ids = user.organizations.pluck(:github_id).uniq | ||
next unless github_org_ids.any? | ||
|
||
github_org_ids.each do |gh_id| | ||
github_org = GitHubOrganization.new(user.github_client, gh_id) | ||
next if github_org.admin?(user.github_login) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would love a double check on this line. If this returns true, I want to move on to the next iteration. I only want to move forward to the payload line if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it true that admins cannot have their access to an organization revoked? I would think we would want to respect the right of an organization to restrict access to their repositories from anyone they want. |
||
|
||
payload = { | ||
"action": "member_removed", | ||
"membership": { "user": { "id": user.github_user.id } }, | ||
"organization": { "id": gh_id } | ||
} | ||
|
||
puts "Removing #{user.github_login} from org with id ##{gh_id}" | ||
OrganizationEventJob.perform_later(payload) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,9 +42,23 @@ | |
|
||
OrganizationEventJob.perform_now(payload) | ||
|
||
expect { @organization.users.find(uid: @user.uid) }.to raise_error(ActiveRecord::RecordNotFound) | ||
expect(@organization.users).not_to include(@user) | ||
expect(assignment.reload.creator_id).not_to eq(@user.id) | ||
end | ||
|
||
it "deletes user from multiple organizations (classrooms) mapped to the same GitHub organization" do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oof this is another one of those times when I wish we didn't have 2 different concepts of "organization" in the backend :C There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! I can't decide if it's going to get better or worse if we move into the monolith. |
||
org2 = create(:organization, github_id: payload.dig("organization", "id")) | ||
@user.organizations << org2 | ||
@user.save | ||
|
||
expect(@organization.users).to include(@user) | ||
expect(org2.users).to include(@user) | ||
|
||
OrganizationEventJob.perform_now(payload) | ||
|
||
expect(@organization.users).not_to include(@user) | ||
expect(org2.users).not_to include(@user) | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need an
.all?
It feels like this will return a scope and the subsequent check forempty?
will always be false. Unless that needs to be anany?
or.count == 0
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely want the collection here, so, we shouldn't tack the
.all?
on, otherwise, we'll be iterating over a boolean on line 18.I think
empty?
does work here though. That query returns an ActiveRecord::Relation. If you run.empty?
on an AR relation, it behaves as expected (if there are no records in it, it returns true, else false).But, it seems like
.empty?
might be a non-standard thing to use here. I ended up using it because I had a vague recollection that one of the checks was less effecient than the others. Based on this post it seemed.empty?
was the way to go. That said, it's unlikely we'll be in the situation where this type of optimization will truly matter, so if.any?
would be the more common thing to use here, I'm happy to update the check on line 16.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, if it works then it's good! Either way. I realized I had a formatting error above:
.all?
was supposed to be.all
? (i.e., the question mark wasn't meant to be part of the method, but was definitely formatted that way 😭)