diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-06-22 01:15:29 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2016-06-22 01:15:29 +0000 |
commit | c11006ac6c395556f7d326a9630d4d32a07005cc (patch) | |
tree | d1acfababad5521b49ae28a04625331aa05e44ca /app | |
parent | 0909535ef755e1e1cbd3a5c4c8b8f5d00012e538 (diff) | |
parent | 909a0ff3ace1eb82a4296764777a552779c39839 (diff) | |
download | gitlab-ce-c11006ac6c395556f7d326a9630d4d32a07005cc.tar.gz |
Merge branch '18755-fix-destroy-project-causes-post_decline_request-to-be-executed' into 'master'
Resolve "Destroying a project causes post_decline_request to be executed"
## What does this MR do?
Ensure we don't send "access request declined" to access requesters when a project is deleted.
## Are there points in the code the reviewer needs to double check?
I've created a service to decouple the notification sending from the AR model.
## Why was this MR needed?
Because there was an issue.
## What are the relevant issue numbers?
Fixes #18755, #18750.
## Does this MR meet the acceptance criteria?
- [x] No CHANGELOG needed.
- [x] Tests
- [x] Added for this feature/bug
- [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
See merge request !4744
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/application_controller.rb | 4 | ||||
-rw-r--r-- | app/controllers/concerns/membership_actions.rb | 31 | ||||
-rw-r--r-- | app/controllers/groups/group_members_controller.rb | 8 | ||||
-rw-r--r-- | app/controllers/projects/project_members_controller.rb | 8 | ||||
-rw-r--r-- | app/models/member.rb | 7 | ||||
-rw-r--r-- | app/models/members/group_member.rb | 12 | ||||
-rw-r--r-- | app/models/members/project_member.rb | 12 | ||||
-rw-r--r-- | app/services/members/destroy_service.rb | 21 | ||||
-rw-r--r-- | app/services/notification_service.rb | 21 | ||||
-rw-r--r-- | app/views/layouts/nav/_group_settings.html.haml | 36 | ||||
-rw-r--r-- | app/views/layouts/nav/_project.html.haml | 11 | ||||
-rw-r--r-- | app/views/layouts/nav/_project_settings.html.haml | 2 |
12 files changed, 71 insertions, 102 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index dd1bc6f5d52..9cc31620d9f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -36,6 +36,10 @@ class ApplicationController < ActionController::Base render_404 end + rescue_from Gitlab::Access::AccessDeniedError do |exception| + render_403 + end + def redirect_back_or_default(default: root_path, options: {}) redirect_to request.referer.present? ? :back : default, options end diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index a24273fad0b..52dc396af6a 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -21,29 +21,18 @@ module MembershipActions def leave @member = membershipable.members.find_by(user_id: current_user) - return render_403 unless @member + Members::DestroyService.new(@member, current_user).execute source_type = @member.real_source_type.humanize(capitalize: false) - - if can?(current_user, action_member_permission(:destroy, @member), @member) - notice = - if @member.request? - "Your access request to the #{source_type} has been withdrawn." - else - "You left the \"#{@member.source.human_name}\" #{source_type}." - end - @member.destroy - - redirect_to [:dashboard, @member.real_source_type.tableize], notice: notice - else - if cannot_leave? - alert = "You can not leave the \"#{@member.source.human_name}\" #{source_type}." - alert << " Transfer or delete the #{source_type}." - redirect_to polymorphic_url(membershipable), alert: alert + notice = + if @member.request? + "Your access request to the #{source_type} has been withdrawn." else - render_403 + "You left the \"#{@member.source.human_name}\" #{source_type}." end - end + redirect_path = @member.request? ? @member.source : [:dashboard, @member.real_source_type.tableize] + + redirect_to redirect_path, notice: notice end protected @@ -51,8 +40,4 @@ module MembershipActions def membershipable raise NotImplementedError end - - def cannot_leave? - raise NotImplementedError - end end diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index d0f2e2949f0..2c49fe3833e 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -36,9 +36,7 @@ class Groups::GroupMembersController < Groups::ApplicationController def destroy @group_member = @group.group_members.find(params[:id]) - return render_403 unless can?(current_user, :destroy_group_member, @group_member) - - @group_member.destroy + Members::DestroyService.new(@group_member, current_user).execute respond_to do |format| format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' } @@ -68,8 +66,4 @@ class Groups::GroupMembersController < Groups::ApplicationController # MembershipActions concern alias_method :membershipable, :group - - def cannot_leave? - @group.last_owner?(current_user) - end end diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index 35d067cd029..6ba32d33403 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -50,9 +50,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController def destroy @project_member = @project.project_members.find(params[:id]) - return render_403 unless can?(current_user, :destroy_project_member, @project_member) - - @project_member.destroy + Members::DestroyService.new(@project_member, current_user).execute respond_to do |format| format.html do @@ -98,8 +96,4 @@ class Projects::ProjectMembersController < Projects::ApplicationController # MembershipActions concern alias_method :membershipable, :project - - def cannot_leave? - current_user == @project.owner - end end diff --git a/app/models/member.rb b/app/models/member.rb index 4ee3f1bb5c2..c74a16367db 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -48,7 +48,6 @@ class Member < ActiveRecord::Base after_create :post_create_hook, unless: [:pending?, :importing?] after_update :post_update_hook, unless: [:pending?, :importing?] after_destroy :post_destroy_hook, unless: :pending? - after_destroy :post_decline_request, if: :request? delegate :name, :username, :email, to: :user, prefix: true @@ -188,7 +187,7 @@ class Member < ActiveRecord::Base end def send_request - # override in subclass + notification_service.new_access_request(self) end def post_create_hook @@ -215,10 +214,6 @@ class Member < ActiveRecord::Base post_create_hook end - def post_decline_request - # override in subclass - end - def system_hook_service SystemHooksService.new end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 363db877968..2f13d339c89 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -33,12 +33,6 @@ class GroupMember < Member super end - def send_request - notification_service.new_group_access_request(self) - - super - end - def post_create_hook notification_service.new_group_member(self) @@ -64,10 +58,4 @@ class GroupMember < Member super end - - def post_decline_request - notification_service.decline_group_access_request(self) - - super - end end diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb index 250ee04fd1d..e9d3a82ba15 100644 --- a/app/models/members/project_member.rb +++ b/app/models/members/project_member.rb @@ -111,12 +111,6 @@ class ProjectMember < Member super end - def send_request - notification_service.new_project_access_request(self) - - super - end - def post_create_hook unless owner? event_service.join_project(self.project, self.user) @@ -152,12 +146,6 @@ class ProjectMember < Member super end - def post_decline_request - notification_service.decline_project_access_request(self) - - super - end - def event_service EventCreateService.new end diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb new file mode 100644 index 00000000000..15358f80208 --- /dev/null +++ b/app/services/members/destroy_service.rb @@ -0,0 +1,21 @@ +module Members + class DestroyService < BaseService + attr_accessor :member, :current_user + + def initialize(member, user) + @member, @current_user = member, user + end + + def execute + unless member && can?(current_user, "destroy_#{member.type.underscore}".to_sym, member) + raise Gitlab::Access::AccessDeniedError + end + + member.destroy + + if member.request? && member.user != current_user + notification_service.decline_access_request(member) + end + end + end +end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 19832a19b2b..590350a11e5 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -181,15 +181,16 @@ class NotificationService end end - # Project access request - def new_project_access_request(project_member) - mailer.member_access_requested_email(project_member.real_source_type, project_member.id).deliver_later + # Members + def new_access_request(member) + mailer.member_access_requested_email(member.real_source_type, member.id).deliver_later end - def decline_project_access_request(project_member) - mailer.member_access_denied_email(project_member.real_source_type, project_member.project.id, project_member.user.id).deliver_later + def decline_access_request(member) + mailer.member_access_denied_email(member.real_source_type, member.source_id, member.user_id).deliver_later end + # Project invite def invite_project_member(project_member, token) mailer.member_invited_email(project_member.real_source_type, project_member.id, token).deliver_later end @@ -216,15 +217,7 @@ class NotificationService mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later end - # Group access request - def new_group_access_request(group_member) - mailer.member_access_requested_email(group_member.real_source_type, group_member.id).deliver_later - end - - def decline_group_access_request(group_member) - mailer.member_access_denied_email(group_member.real_source_type, group_member.group.id, group_member.user.id).deliver_later - end - + # Group invite def invite_group_member(group_member, token) mailer.member_invited_email(group_member.real_source_type, group_member.id, token).deliver_later end diff --git a/app/views/layouts/nav/_group_settings.html.haml b/app/views/layouts/nav/_group_settings.html.haml index dac46648b9f..3a24b09ab7e 100644 --- a/app/views/layouts/nav/_group_settings.html.haml +++ b/app/views/layouts/nav/_group_settings.html.haml @@ -1,16 +1,22 @@ - if current_user - - if access = @group.users.find_by(id: current_user.id) - .controls - .dropdown.group-settings-dropdown - %a.dropdown-new.btn.btn-default#group-settings-button{href: '#', 'data-toggle' => 'dropdown'} - = icon('cog') - = icon('caret-down') - %ul.dropdown-menu.dropdown-menu-align-right - - if can?(current_user, :admin_group, @group) - = nav_link(path: 'groups#projects') do - = link_to projects_group_path(@group), title: 'Projects' do - Projects - %li.divider - %li - = link_to edit_group_path(@group) do - Edit Group + - can_edit = can?(current_user, :admin_group, @group) + - member = @group.members.non_request.find_by(user_id: current_user.id) + - can_leave = member && can?(current_user, :destroy_group_member, member) + + .controls + .dropdown.group-settings-dropdown + %a.dropdown-new.btn.btn-default#group-settings-button{href: '#', 'data-toggle' => 'dropdown'} + = icon('cog') + = icon('caret-down') + %ul.dropdown-menu.dropdown-menu-align-right + = nav_link(path: 'groups#projects') do + = link_to 'Projects', projects_group_path(@group), title: 'Projects' + %li.divider + - if can_edit + %li + = link_to 'Edit Group', edit_group_path(@group) + - if can_leave + %li + = link_to polymorphic_path([:leave, @group, :members]), + data: { confirm: leave_confirmation_message(@group) }, method: :delete, title: 'Leave group' do + Leave Group diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml index 7762746f848..27e840df503 100644 --- a/app/views/layouts/nav/_project.html.haml +++ b/app/views/layouts/nav/_project.html.haml @@ -5,19 +5,20 @@ = icon('cog') = icon('caret-down') %ul.dropdown-menu.dropdown-menu-align-right - - is_project_member = @project.users.exists?(current_user.id) - - access = @project.team.max_member_access(current_user.id) - can_edit = can?(current_user, :admin_project, @project) + -# We don't use @project.team.find_member because it searches for group members too... + - member = @project.members.non_request.find_by(user_id: current_user.id) + - can_leave = member && can?(current_user, :destroy_project_member, member) - = render 'layouts/nav/project_settings', access: access, can_edit: can_edit + = render 'layouts/nav/project_settings', can_edit: can_edit - - if can_edit || is_project_member + - if can_edit || can_leave %li.divider - if can_edit %li = link_to edit_project_path(@project) do Edit Project - - if is_project_member + - if can_leave %li = link_to polymorphic_path([:leave, @project, :members]), data: { confirm: leave_confirmation_message(@project) }, method: :delete, title: 'Leave project' do diff --git a/app/views/layouts/nav/_project_settings.html.haml b/app/views/layouts/nav/_project_settings.html.haml index 13d32bd1354..51a54b4f262 100644 --- a/app/views/layouts/nav/_project_settings.html.haml +++ b/app/views/layouts/nav/_project_settings.html.haml @@ -3,7 +3,7 @@ = link_to namespace_project_project_members_path(@project.namespace, @project), title: 'Members', class: 'team-tab tab' do %span Members -- if access && can_edit +- if can_edit - if @project.allowed_to_share_with_group? = nav_link(controller: :group_links) do = link_to namespace_project_group_links_path(@project.namespace, @project), title: "Groups" do |