diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2018-07-31 13:09:19 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2018-07-31 13:09:19 +0000 |
commit | 5a3948a5733d7d4dc09bfa2daeaad5cc15ba2a00 (patch) | |
tree | 6b25b01e29b3d33f35a409ad1d6b5be7b0f636ae /app/services | |
parent | e9d04585f872121d4b1f96e019946cfa48d2f915 (diff) | |
parent | bdc8396e25e6eba6edcf2896daa49bb49695ef8c (diff) | |
download | gitlab-ce-5a3948a5733d7d4dc09bfa2daeaad5cc15ba2a00.tar.gz |
Merge branch 'todos-visibility-change' into 'master'
Delete todos when users loses target read permissions
See merge request gitlab-org/gitlab-ce!20665
Diffstat (limited to 'app/services')
-rw-r--r-- | app/services/issues/update_service.rb | 2 | ||||
-rw-r--r-- | app/services/members/destroy_service.rb | 8 | ||||
-rw-r--r-- | app/services/projects/update_service.rb | 32 | ||||
-rw-r--r-- | app/services/todos/destroy/base_service.rb | 33 | ||||
-rw-r--r-- | app/services/todos/destroy/confidential_issue_service.rb | 39 | ||||
-rw-r--r-- | app/services/todos/destroy/entity_leave_service.rb | 59 | ||||
-rw-r--r-- | app/services/todos/destroy/private_features_service.rb | 40 | ||||
-rw-r--r-- | app/services/todos/destroy/project_private_service.rb | 30 |
8 files changed, 236 insertions, 7 deletions
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index c02dddf67b2..faa4c8a5a4f 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -37,6 +37,8 @@ module Issues end if issue.previous_changes.include?('confidential') + # don't enqueue immediately to prevent todos removal in case of a mistake + TodosDestroyer::ConfidentialIssueWorker.perform_in(1.hour, issue.id) if issue.confidential? create_confidentiality_note(issue) end diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index aca0ba66646..c186a5971dc 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -15,6 +15,8 @@ module Members notification_service.decline_access_request(member) end + enqeue_delete_todos(member) + after_execute(member: member) member @@ -22,6 +24,12 @@ module Members private + def enqeue_delete_todos(member) + type = member.is_a?(GroupMember) ? 'Group' : 'Project' + # don't enqueue immediately to prevent todos removal in case of a mistake + TodosDestroyer::EntityLeaveWorker.perform_in(1.hour, member.user_id, member.source_id, type) + end + def can_destroy_member?(member) can?(current_user, destroy_member_permission(member), member) end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index d3dc11435fe..31ab4fbe49e 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -25,13 +25,7 @@ module Projects return validation_failed! if project.errors.any? if project.update(params.except(:default_branch)) - if project.previous_changes.include?('path') - project.rename_repo - else - system_hook_service.execute_hooks_for(project, :update) - end - - update_pages_config if changing_pages_https_only? + after_update success else @@ -47,6 +41,30 @@ module Projects private + def after_update + todos_features_changes = %w( + issues_access_level + merge_requests_access_level + repository_access_level + ) + project_changed_feature_keys = project.project_feature.previous_changes.keys + + if project.previous_changes.include?(:visibility_level) && project.private? + # don't enqueue immediately to prevent todos removal in case of a mistake + TodosDestroyer::ProjectPrivateWorker.perform_in(1.hour, project.id) + elsif (project_changed_feature_keys & todos_features_changes).present? + TodosDestroyer::PrivateFeaturesWorker.perform_in(1.hour, project.id) + end + + if project.previous_changes.include?('path') + project.rename_repo + else + system_hook_service.execute_hooks_for(project, :update) + end + + update_pages_config if changing_pages_https_only? + end + def validation_failed! model_errors = project.errors.full_messages.to_sentence error_message = model_errors.presence || 'Project could not be updated!' diff --git a/app/services/todos/destroy/base_service.rb b/app/services/todos/destroy/base_service.rb new file mode 100644 index 00000000000..dff5e1f30e5 --- /dev/null +++ b/app/services/todos/destroy/base_service.rb @@ -0,0 +1,33 @@ +module Todos + module Destroy + class BaseService + def execute + return unless todos_to_remove? + + without_authorized(todos).delete_all + end + + private + + def without_authorized(items) + items.where('user_id NOT IN (?)', authorized_users) + end + + def authorized_users + ProjectAuthorization.select(:user_id).where(project_id: project_ids) + end + + def todos + raise NotImplementedError + end + + def project_ids + raise NotImplementedError + end + + def todos_to_remove? + raise NotImplementedError + end + end + end +end diff --git a/app/services/todos/destroy/confidential_issue_service.rb b/app/services/todos/destroy/confidential_issue_service.rb new file mode 100644 index 00000000000..c5b66df057a --- /dev/null +++ b/app/services/todos/destroy/confidential_issue_service.rb @@ -0,0 +1,39 @@ +module Todos + module Destroy + class ConfidentialIssueService < ::Todos::Destroy::BaseService + extend ::Gitlab::Utils::Override + + attr_reader :issue + + def initialize(issue_id) + @issue = Issue.find_by(id: issue_id) + end + + private + + override :todos + def todos + Todo.where(target: issue) + .where('user_id != ?', issue.author_id) + .where('user_id NOT IN (?)', issue.assignees.select(:id)) + end + + override :todos_to_remove? + def todos_to_remove? + issue&.confidential? + end + + override :project_ids + def project_ids + issue.project_id + end + + override :authorized_users + def authorized_users + ProjectAuthorization.select(:user_id) + .where(project_id: project_ids) + .where('access_level >= ?', Gitlab::Access::REPORTER) + end + end + end +end diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb new file mode 100644 index 00000000000..2ff9f94b718 --- /dev/null +++ b/app/services/todos/destroy/entity_leave_service.rb @@ -0,0 +1,59 @@ +module Todos + module Destroy + class EntityLeaveService < ::Todos::Destroy::BaseService + extend ::Gitlab::Utils::Override + + attr_reader :user_id, :entity + + def initialize(user_id, entity_id, entity_type) + unless %w(Group Project).include?(entity_type) + raise ArgumentError.new("#{entity_type} is not an entity user can leave") + end + + @user_id = user_id + @entity = entity_type.constantize.find_by(id: entity_id) + end + + private + + override :todos + def todos + if entity.private? + Todo.where(project_id: project_ids, user_id: user_id) + else + project_ids.each do |project_id| + TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user_id) + end + + Todo.where( + target_id: confidential_issues.select(:id), target_type: Issue, user_id: user_id + ) + end + end + + override :project_ids + def project_ids + case entity + when Project + [entity.id] + when Namespace + Project.select(:id).where(namespace_id: entity.self_and_descendants.select(:id)) + end + end + + override :todos_to_remove? + def todos_to_remove? + # if an entity is provided we want to check always at least private features + !!entity + end + + def confidential_issues + assigned_ids = IssueAssignee.select(:issue_id).where(user_id: user_id) + + Issue.where(project_id: project_ids, confidential: true) + .where('author_id != ?', user_id) + .where('id NOT IN (?)', assigned_ids) + end + end + end +end diff --git a/app/services/todos/destroy/private_features_service.rb b/app/services/todos/destroy/private_features_service.rb new file mode 100644 index 00000000000..4d8e2877bfb --- /dev/null +++ b/app/services/todos/destroy/private_features_service.rb @@ -0,0 +1,40 @@ +module Todos + module Destroy + class PrivateFeaturesService < ::Todos::Destroy::BaseService + attr_reader :project_ids, :user_id + + def initialize(project_ids, user_id = nil) + @project_ids = project_ids + @user_id = user_id + end + + def execute + ProjectFeature.where(project_id: project_ids).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? + + remove_todos(project_features.project_id, target_types) + end + end + + private + + def private?(feature_level) + feature_level == ProjectFeature::PRIVATE + end + + def remove_todos(project_id, target_types) + items = Todo.where(project_id: project_id) + items = items.where(user_id: user_id) if user_id + + items.where('user_id NOT IN (?)', authorized_users) + .where(target_type: target_types) + .delete_all + end + end + end +end diff --git a/app/services/todos/destroy/project_private_service.rb b/app/services/todos/destroy/project_private_service.rb new file mode 100644 index 00000000000..171933e7cbc --- /dev/null +++ b/app/services/todos/destroy/project_private_service.rb @@ -0,0 +1,30 @@ +module Todos + module Destroy + class ProjectPrivateService < ::Todos::Destroy::BaseService + extend ::Gitlab::Utils::Override + + attr_reader :project + + def initialize(project_id) + @project = Project.find_by(id: project_id) + end + + private + + override :todos + def todos + Todo.where(project_id: project_ids) + end + + override :project_ids + def project_ids + project.id + end + + override :todos_to_remove? + def todos_to_remove? + project&.private? + end + end + end +end |