Skip to content
This repository has been archived by the owner on Jun 24, 2024. It is now read-only.

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4fb2bb2
Fix existing test and create new failing test for the multi-classroom…
JessRudder Nov 26, 2019
32f8152
Ensure user is removed from all classrooms tied to the given github_o…
JessRudder Nov 27, 2019
01cd5f1
Disable the rubocop linter for methodlength on the removal job
JessRudder Nov 27, 2019
ae71fcb
In progress method starting wtih orgs
JessRudder Nov 27, 2019
2a34790
Rake task that checks that users are still org members and removes th…
JessRudder Nov 27, 2019
3d00951
Changes to make the linters happy.
JessRudder Nov 27, 2019
36bae66
Simplify and add some output
JessRudder Nov 27, 2019
6cf3c03
Merge branch 'master' into revoke-all-classroom-access
JessRudder Dec 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions app/jobs/organization_event_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

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 for empty? will always be false. Unless that needs to be an any? or .count == 0?

Copy link
Contributor Author

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.

Copy link
Contributor

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 😭)


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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

organizations = Organization.where(github_id: github_organization_id).includes(:users)

However, I end up with an error from the bullet gem:

     Bullet::Notification::UnoptimizedQueryError:
       user: jessrudder
        
       AVOID eager loading detected
         Organization => [:users]
         Remove from your finder: :includes => [:users]
       Call stack
         /Users/jessrudder/github/classroom/spec/support/bullet.rb:16:in `block (2 levels) in <top (required)>'
         bin/rspec:16:in `load'
         bin/rspec:16:in `<main>'

If an includes would be the right thing to do here to avoid the N+1, I can whitelist this for the bullet gem so it doesn't unfairly fail the test. If not, is there another way to avoid the N+1?

Copy link
Contributor

@jeffrafter jeffrafter Dec 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The 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
27 changes: 27 additions & 0 deletions lib/tasks/remove_classroom_users_without_github_org_access.rake
View file
Open in desktop
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 github_org.admin?(user.github_login) returns false. I'm 99% sure I've written it right, but, you can never be too sure!

Copy link
Contributor

Choose a reason for hiding this comment

The 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
16 changes: 15 additions & 1 deletion spec/jobs/organization_event_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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