diff options
4 files changed, 265 insertions, 0 deletions
diff --git a/changelogs/unreleased/todos-visibility-migration.yml b/changelogs/unreleased/todos-visibility-migration.yml new file mode 100644 index 00000000000..651facc4ec8 --- /dev/null +++ b/changelogs/unreleased/todos-visibility-migration.yml @@ -0,0 +1,5 @@ +--- +title: Remove todos of users without access to targets migration +merge_request: 20927 +author: +type: other diff --git a/db/migrate/20180717125853_remove_restricted_todos.rb b/db/migrate/20180717125853_remove_restricted_todos.rb new file mode 100644 index 00000000000..fdf43921a73 --- /dev/null +++ b/db/migrate/20180717125853_remove_restricted_todos.rb @@ -0,0 +1,31 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. +# frozen_string_literal: true + +class RemoveRestrictedTodos < ActiveRecord::Migration + DOWNTIME = false + disable_ddl_transaction! + + MIGRATION = 'RemoveRestrictedTodos'.freeze + BATCH_SIZE = 1000 + DELAY_INTERVAL = 5.minutes.to_i + + class Project < ActiveRecord::Base + include EachBatch + + self.table_name = 'projects' + end + + def up + Project.where('EXISTS (SELECT 1 FROM todos WHERE todos.project_id = projects.id)') + .each_batch(of: BATCH_SIZE) do |batch, index| + range = batch.pluck('MIN(id)', 'MAX(id)').first + + BackgroundMigrationWorker.perform_in(index * DELAY_INTERVAL, MIGRATION, range) + end + end + + def down + # nothing to do + end +end diff --git a/lib/gitlab/background_migration/remove_restricted_todos.rb b/lib/gitlab/background_migration/remove_restricted_todos.rb new file mode 100644 index 00000000000..68f3fa62170 --- /dev/null +++ b/lib/gitlab/background_migration/remove_restricted_todos.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true +# rubocop:disable Style/Documentation + +module Gitlab + module BackgroundMigration + class RemoveRestrictedTodos + PRIVATE_FEATURE = 10 + PRIVATE_PROJECT = 0 + + class Project < ActiveRecord::Base + self.table_name = 'projects' + end + + class ProjectAuthorization < ActiveRecord::Base + self.table_name = 'project_authorizations' + end + + class ProjectFeature < ActiveRecord::Base + self.table_name = 'project_features' + end + + class Todo < ActiveRecord::Base + include EachBatch + + self.table_name = 'todos' + end + + class Issue < ActiveRecord::Base + include EachBatch + + self.table_name = 'issues' + end + + def perform(start_id, stop_id) + projects = Project.where('EXISTS (SELECT 1 FROM todos WHERE todos.project_id = projects.id)') + .where(id: start_id..stop_id) + + projects.each do |project| + remove_confidential_issue_todos(project.id) + + if project.visibility_level == PRIVATE_PROJECT + remove_non_members_todos(project.id) + else + remove_restricted_features_todos(project.id) + end + end + end + + private + + def remove_non_members_todos(project_id) + Todo.where(project_id: project_id) + .where('user_id NOT IN (?)', authorized_users(project_id)) + .each_batch(of: 5000) do |batch| + batch.delete_all + end + end + + def remove_confidential_issue_todos(project_id) + # min access level to access a confidential issue is reporter + min_reporters = authorized_users(project_id) + .select(:user_id) + .where('access_level >= ?', 20) + + confidential_issues = Issue.select(:id, :author_id).where(confidential: true, project_id: project_id) + confidential_issues.each_batch(of: 100) do |batch| + batch.each do |issue| + assigned_users = IssueAssignee.select(:user_id).where(issue_id: issue.id) + + todos = Todo.where(target_type: 'Issue', target_id: issue.id) + .where('user_id NOT IN (?)', min_reporters) + .where('user_id NOT IN (?)', assigned_users) + todos = todos.where('user_id != ?', issue.author_id) if issue.author_id + + todos.delete_all + end + end + end + + def remove_restricted_features_todos(project_id) + ProjectFeature.where(project_id: project_id).each do |project_features| + target_types = [] + target_types << 'Issue' if private?(project_features.issues_access_level) + target_types << 'MergeRequest' if private?(project_features.merge_requests_access_level) + target_types << 'Commit' if private?(project_features.repository_access_level) + + next if target_types.empty? + + Todo.where(project_id: project_id) + .where('user_id NOT IN (?)', authorized_users(project_id)) + .where(target_type: target_types) + .delete_all + end + end + + def private?(feature_level) + feature_level == PRIVATE_FEATURE + end + + def authorized_users(project_id) + ProjectAuthorization.select(:user_id).where(project_id: project_id) + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/remove_restricted_todos_spec.rb b/spec/lib/gitlab/background_migration/remove_restricted_todos_spec.rb new file mode 100644 index 00000000000..dae754112dc --- /dev/null +++ b/spec/lib/gitlab/background_migration/remove_restricted_todos_spec.rb @@ -0,0 +1,124 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::RemoveRestrictedTodos, :migration, schema: 20180704204006 do + let(:projects) { table(:projects) } + let(:users) { table(:users) } + let(:todos) { table(:todos) } + let(:issues) { table(:issues) } + let(:assignees) { table(:issue_assignees) } + let(:project_authorizations) { table(:project_authorizations) } + let(:project_features) { table(:project_features) } + + let(:todo_params) { { author_id: 1, target_type: 'Issue', action: 1, state: :pending } } + + before do + users.create(id: 1, email: 'user@example.com', projects_limit: 10) + users.create(id: 2, email: 'reporter@example.com', projects_limit: 10) + users.create(id: 3, email: 'guest@example.com', projects_limit: 10) + + projects.create!(id: 1, name: 'project-1', path: 'project-1', visibility_level: 0, namespace_id: 1) + projects.create!(id: 2, name: 'project-2', path: 'project-2', visibility_level: 0, namespace_id: 1) + + issues.create(id: 1, project_id: 1) + issues.create(id: 2, project_id: 2) + + project_authorizations.create(user_id: 2, project_id: 2, access_level: 20) # reporter + project_authorizations.create(user_id: 3, project_id: 2, access_level: 10) # guest + + todos.create(todo_params.merge(user_id: 1, project_id: 1, target_id: 1)) # out of project ids range + todos.create(todo_params.merge(user_id: 1, project_id: 2, target_id: 2)) # non member + todos.create(todo_params.merge(user_id: 2, project_id: 2, target_id: 2)) # reporter + todos.create(todo_params.merge(user_id: 3, project_id: 2, target_id: 2)) # guest + end + + subject { described_class.new.perform(2, 5) } + + context 'when a project is private' do + it 'removes todos of users without project access' do + expect { subject }.to change { Todo.count }.from(4).to(3) + end + + context 'with a confidential issue' do + it 'removes todos of users without project access and guests for confidential issues' do + issues.create(id: 3, project_id: 2, confidential: true) + issues.create(id: 4, project_id: 1, confidential: true) # not in the batch + todos.create(todo_params.merge(user_id: 3, project_id: 2, target_id: 3)) + todos.create(todo_params.merge(user_id: 2, project_id: 2, target_id: 3)) + todos.create(todo_params.merge(user_id: 1, project_id: 1, target_id: 4)) + + expect { subject }.to change { Todo.count }.from(7).to(5) + end + end + end + + context 'when a project is public' do + before do + projects.find(2).update_attribute(:visibility_level, 20) + end + + context 'when all features have the same visibility as the project, no confidential issues' do + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end + end + + context 'with confidential issues' do + before do + users.create(id: 4, email: 'author@example.com', projects_limit: 10) + users.create(id: 5, email: 'assignee@example.com', projects_limit: 10) + issues.create(id: 3, project_id: 2, confidential: true, author_id: 4) + assignees.create(user_id: 5, issue_id: 3) + + todos.create(todo_params.merge(user_id: 1, project_id: 2, target_id: 3)) # to be deleted + todos.create(todo_params.merge(user_id: 2, project_id: 2, target_id: 3)) # authorized user + todos.create(todo_params.merge(user_id: 3, project_id: 2, target_id: 3)) # to be deleted guest + todos.create(todo_params.merge(user_id: 4, project_id: 2, target_id: 3)) # conf issue author + todos.create(todo_params.merge(user_id: 5, project_id: 2, target_id: 3)) # conf issue assignee + end + + it 'removes confidential issue todos for non authorized users' do + expect { subject }.to change { Todo.count }.from(9).to(7) + end + end + + context 'features visibility restrictions' do + before do + todo_params.merge!(project_id: 2, user_id: 1, target_id: 3) + todos.create(todo_params.merge(user_id: 1, target_id: 3, target_type: 'MergeRequest')) + todos.create(todo_params.merge(user_id: 1, target_id: 3, target_type: 'Commit')) + end + + context 'when issues are restricted to project members' do + before do + project_features.create(issues_access_level: 10, project_id: 2) + end + + it 'removes non members issue todos' do + expect { subject }.to change { Todo.count }.from(6).to(5) + end + end + + context 'when merge requests are restricted to project members' do + before do + project_features.create(merge_requests_access_level: 10, project_id: 2) + end + + it 'removes non members issue todos' do + expect { subject }.to change { Todo.count }.from(6).to(5) + end + end + + context 'when repository and merge requests are restricted to project members' do + before do + project_features.create(repository_access_level: 10, merge_requests_access_level: 10, project_id: 2) + end + + it 'removes non members commit and merge requests todos' do + expect { subject }.to change { Todo.count }.from(6).to(4) + end + end + end + end +end |