From 1c8b44f326360fa5c948ac385e3a3c595cbd2d66 Mon Sep 17 00:00:00 2001 From: rhymes Date: Sun, 20 Dec 2020 15:15:40 +0100 Subject: [PATCH] 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 | 10 ++++- app/controllers/users_controller.rb | 5 +++ ..._integrations_github_repositories.html.erb | 5 ++- ...454_cleanup_orphan_git_hub_repositories.rb | 22 +++++++++ ...leanup_orphan_git_hub_repositories_spec.rb | 28 ++++++++++++ spec/requests/admin/users_spec.rb | 45 +++++++++++++++++-- spec/requests/user/user_settings_spec.rb | 32 ++++++++++--- 7 files changed, 134 insertions(+), 13 deletions(-) create mode 100644 lib/data_update_scripts/20201217162454_cleanup_orphan_git_hub_repositories.rb create mode 100644 spec/lib/data_update_scripts/cleanup_orphan_git_hub_repositories_spec.rb diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index bb059ff0e..f3dcc07cd 100644 --- a/app/controllers/admin/users_controller.rb +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 698d0aa0f..37450cda4 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/app/views/users/_integrations_github_repositories.html.erb b/app/views/users/_integrations_github_repositories.html.erb index b8bbd0af0..59a9917a8 100644 --- a/app/views/users/_integrations_github_repositories.html.erb +++ b/app/views/users/_integrations_github_repositories.html.erb @@ -5,7 +5,10 @@ GitHub

- Pin your GitHub repositories to your profile + Pin your GitHub repositories to your profile. +

+

+ Repositories will be disappear from your profile if you remove the OAuth association with GitHub.

diff --git a/lib/data_update_scripts/20201217162454_cleanup_orphan_git_hub_repositories.rb b/lib/data_update_scripts/20201217162454_cleanup_orphan_git_hub_repositories.rb new file mode 100644 index 000000000..f2fff3837 --- /dev/null +++ b/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 diff --git a/spec/lib/data_update_scripts/cleanup_orphan_git_hub_repositories_spec.rb b/spec/lib/data_update_scripts/cleanup_orphan_git_hub_repositories_spec.rb new file mode 100644 index 000000000..5c58b6aca --- /dev/null +++ b/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 diff --git a/spec/requests/admin/users_spec.rb b/spec/requests/admin/users_spec.rb index 56be77f66..38adac0a7 100644 --- a/spec/requests/admin/users_spec.rb +++ b/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 diff --git a/spec/requests/user/user_settings_spec.rb b/spec/requests/user/user_settings_spec.rb index 1f488e635..a66d8ea21 100644 --- a/spec/requests/user/user_settings_spec.rb +++ b/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...