Remove GitHub repositories when GitHub identity is removed (#11951)

* Remove GitHub repositories when user deletes their own GitHub identity

* Remove GitHub repos also from the admin remove identity

* Add a notice in Settings/Extension

* Add data update script to cleanup orphan GitHub repos

app/controllers/admin/users_controller.rb

@@ -102,9 +102,17 @@ module Admin
     def remove_identity
       identity = Identity.find(user_params[:identity_id])
       @user = identity.user
+
       begin
-        identity.delete
+        identity.destroy
+
         @user.update("#{identity.provider}_username" => nil)
+
+        # GitHub repositories are tied with the existence of the GitHub identity
+        # as we use the user's GitHub token to fetch them from the API.
+        # We should delete them when a user unlinks their GitHub account.
+        @user.github_repos.destroy_all if identity.provider.to_sym == :github
+
         flash[:success] = "The #{identity.provider.capitalize} identity was successfully deleted and backed up."
       rescue StandardError => e
         flash[:danger] = e.message

app/controllers/users_controller.rb

@@ -133,6 +133,11 @@ class UsersController < ApplicationController
         :profile_updated_at => Time.current,
       )
 
+      # GitHub repositories are tied with the existence of the GitHub identity
+      # as we use the user's GitHub token to fetch them from the API.
+      # We should delete them when a user unlinks their GitHub account.
+      @user.github_repos.destroy_all if provider.provider_name == :github
+
       flash[:settings_notice] = "Your #{provider.official_name} account was successfully removed."
     else
       flash[:error] = error_message

app/views/users/_integrations_github_repositories.html.erb

@@ -5,7 +5,10 @@
         GitHub
       </h2>
       <p class="color-base-70">
-        Pin your GitHub repositories to your profile
+        Pin your GitHub repositories to your profile.
+      </p>
+      <p class="color-base-70">
+        <em>Repositories will be disappear from your profile if you remove the OAuth association with GitHub.</em>
       </p>
     </header>
 

lib/data_update_scripts/20201217162454_cleanup_orphan_git_hub_repositories.rb

@@ -0,0 +1,22 @@
+module DataUpdateScripts
+  class CleanupOrphanGitHubRepositories
+    def run
+      # load orphan GithubRepo ids
+      rows = ActiveRecord::Base.connection.execute(
+        <<-SQL,
+          SELECT id FROM github_repos
+          WHERE user_id NOT IN (
+            SELECT users.id FROM users
+            JOIN identities ON users.id = identities.user_id
+            WHERE provider = 'github'
+          )
+        SQL
+      )
+
+      github_repos_ids = rows.map { |row| row["id"] }
+
+      # load them in batches and delete them from AR so that we can trigger the after_destroy callbacks
+      GithubRepo.where(id: github_repos_ids).find_each(&:destroy)
+    end
+  end
+end

spec/lib/data_update_scripts/cleanup_orphan_git_hub_repositories_spec.rb

@@ -0,0 +1,28 @@
+require "rails_helper"
+require Rails.root.join(
+  "lib/data_update_scripts/20201217162454_cleanup_orphan_git_hub_repositories.rb",
+)
+
+describe DataUpdateScripts::CleanupOrphanGitHubRepositories do
+  let(:user) { create(:user, :with_identity, identities: [:github]) }
+
+  before do
+    omniauth_mock_providers_payload
+    create(:github_repo, user: user)
+  end
+
+  it "does not delete a repository if the user has a GitHub identity" do
+    expect do
+      described_class.new.run
+    end.not_to change(user.github_repos, :count)
+  end
+
+  it "deletes a repository if the user does not have a GitHub identity" do
+    github_identity = user.identities.github.first
+    github_identity.destroy
+
+    expect do
+      described_class.new.run
+    end.to change(user.github_repos, :count).by(-user.github_repos.size)
+  end
+end

spec/requests/admin/users_spec.rb

@@ -138,16 +138,53 @@ RSpec.describe "admin/users", type: :request do
   end
 
   describe "DELETE /admin/users/:id/remove_identity" do
+    let(:provider) { Authentication::Providers.available.first }
+    let(:user) do
+      omniauth_mock_providers_payload
+      create(:user, :with_identity)
+    end
+
+    before do
+      omniauth_mock_providers_payload
+      allow(SiteConfig).to receive(:authentication_providers).and_return(Authentication::Providers.available)
+    end
+
     it "removes the given identity" do
       identity = user.identities.first
-      delete "/admin/users/#{user.id}/remove_identity", params: { user: { identity_id: identity.id } }
-      expect { identity.reload }.to raise_error ActiveRecord::RecordNotFound
+
+      delete remove_identity_admin_user_path(user.id), params: { user: { identity_id: identity.id } }
+
+      expect { identity.reload }.to raise_error(ActiveRecord::RecordNotFound)
     end
 
     it "updates their social account's username to nil" do
       identity = user.identities.first
-      delete "/admin/users/#{user.id}/remove_identity", params: { user: { identity_id: identity.id } }
-      expect(user.reload.github_username).to eq nil
+
+      delete remove_identity_admin_user_path(user.id), params: { user: { identity_id: identity.id } }
+
+      expect(user.public_send("#{identity.provider}_username")).to be(nil)
+    end
+
+    it "does not remove GitHub repositories if the removed identity is not GitHub" do
+      create(:github_repo, user: user)
+
+      identity = user.identities.twitter.first
+
+      expect do
+        delete remove_identity_admin_user_path(user.id), params: { user: { identity_id: identity.id } }
+      end.not_to change(user.github_repos, :count)
+    end
+
+    it "removes GitHub repositories if the removed identity is GitHub" do
+      repo = create(:github_repo, user: user)
+
+      identity = user.identities.github.first
+
+      expect do
+        delete remove_identity_admin_user_path(user.id), params: { user: { identity_id: identity.id } }
+      end.to change(user.github_repos, :count).by(-1)
+
+      expect(GithubRepo.exists?(id: repo.id)).to be(false)
     end
   end
 

spec/requests/user/user_settings_spec.rb

@@ -375,31 +375,31 @@ RSpec.describe "UserSettings", type: :request do
 
       it "removes the correct identity" do
         expect do
-          delete "/users/remove_identity", params: { provider: provider }
+          delete users_remove_identity_path, params: { provider: provider }
         end.to change(user.identities, :count).by(-1)
 
         expect(user.identities.map(&:provider)).not_to include(provider)
       end
 
       it "empties their associated username" do
-        delete "/users/remove_identity", params: { provider: provider }
+        delete users_remove_identity_path, params: { provider: provider }
 
         expect(user.public_send("#{provider}_username")).to be(nil)
       end
 
       it "updates the profile_updated_at timestamp" do
         original_profile_updated_at = user.profile_updated_at
-        delete "/users/remove_identity", params: { provider: provider }
+        delete users_remove_identity_path, params: { provider: provider }
         expect(user.profile_updated_at.to_i).to be > original_profile_updated_at.to_i
       end
 
       it "redirects successfully to /settings/account" do
-        delete "/users/remove_identity", params: { provider: provider }
+        delete users_remove_identity_path, params: { provider: provider }
         expect(response).to redirect_to("/settings/account")
       end
 
       it "renders a successful response message" do
-        delete "/users/remove_identity", params: { provider: provider }
+        delete users_remove_identity_path, params: { provider: provider }
         auth_provider = Authentication::Providers.get!(provider)
 
         expected_notice = "Your #{auth_provider.official_name} account was successfully removed."
@@ -409,7 +409,7 @@ RSpec.describe "UserSettings", type: :request do
       it "redirects the user with an error if the corresponding provider has been since disabled" do
         providers = Authentication::Providers.available - [provider]
         allow(Authentication::Providers).to receive(:enabled).and_return(providers)
-        delete "/users/remove_identity", params: { provider: provider }
+        delete users_remove_identity_path, params: { provider: provider }
         expect(response).to redirect_to("/settings/account")
 
         error = "An error occurred. Please try again or send an email to: #{SiteConfig.email_addresses[:contact]}"
@@ -420,9 +420,27 @@ RSpec.describe "UserSettings", type: :request do
         providers = Authentication::Providers.available.first(2)
         allow(user).to receive(:identities).and_return(user.identities.where(provider: providers))
 
-        delete "/users/remove_identity", params: { provider: providers.first }
+        delete users_remove_identity_path, params: { provider: providers.first }
         expect(response.body).not_to include("Remove OAuth Associations")
       end
+
+      it "does not remove GitHub repositories if the removed identity is not GitHub" do
+        create(:github_repo, user: user)
+
+        expect do
+          delete users_remove_identity_path, params: { provider: :twitter }
+        end.not_to change(user.github_repos, :count)
+      end
+
+      it "removes GitHub repositories if the removed identity is GitHub" do
+        repo = create(:github_repo, user: user)
+
+        expect do
+          delete users_remove_identity_path, params: { provider: :github }
+        end.to change(user.github_repos, :count).by(-1)
+
+        expect(GithubRepo.exists?(id: repo.id)).to be(false)
+      end
     end
 
     # Users won't be able to do this via the view, but in case they hit the route somehow...