Commit d1c94f03 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'explicit-requesters-scope' into 'master'

Exclude requesters from Project#members, Group#members and User#members

## What does this MR do?

It excludes requesters from the `Project#members`, `Group#members` and `User#members` associations, and adds new `Project#requesters` and `Group#requesters` associations.

## Are there points in the code the reviewer needs to double check?

No.

## Why was this MR needed?

Without this, if you call `project.members`, requesters are included in the results! This is at best misleading, and at worst can lead to security issues. By excluding requesters from the `#members` associations, we avoid introducing security inadvertently since you have to call the `#requesters` association explicitly to get requesters.

## What are the relevant issue numbers?

This is something I realized while fixing the security issue #19102.

## Does this MR meet the acceptance criteria?

- [x] I don't think this needs a CHANGELOG since this is an internal change
- Tests
  - [x] Added for this feature/bug
  - [ ] 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 !4946
parents 0ccdc631 bd78f573
...@@ -10,6 +10,7 @@ class Admin::GroupsController < Admin::ApplicationController ...@@ -10,6 +10,7 @@ class Admin::GroupsController < Admin::ApplicationController
def show def show
@members = @group.members.order("access_level DESC").page(params[:members_page]) @members = @group.members.order("access_level DESC").page(params[:members_page])
@requesters = @group.requesters
@projects = @group.projects.page(params[:projects_page]) @projects = @group.projects.page(params[:projects_page])
end end
......
...@@ -20,7 +20,8 @@ class Admin::ProjectsController < Admin::ApplicationController ...@@ -20,7 +20,8 @@ class Admin::ProjectsController < Admin::ApplicationController
@group_members = @group.members.order("access_level DESC").page(params[:group_members_page]) @group_members = @group.members.order("access_level DESC").page(params[:group_members_page])
end end
@project_members = @project.project_members.page(params[:project_members_page]) @project_members = @project.members.page(params[:project_members_page])
@requesters = @project.requesters
end end
def transfer def transfer
......
...@@ -10,7 +10,7 @@ module MembershipActions ...@@ -10,7 +10,7 @@ module MembershipActions
end end
def approve_access_request def approve_access_request
@member = membershipable.members.request.find(params[:id]) @member = membershipable.requesters.find(params[:id])
return render_403 unless can?(current_user, action_member_permission(:update, @member), @member) return render_403 unless can?(current_user, action_member_permission(:update, @member), @member)
...@@ -20,7 +20,8 @@ module MembershipActions ...@@ -20,7 +20,8 @@ module MembershipActions
end end
def leave def leave
@member = membershipable.members.find_by(user_id: current_user) @member = membershipable.members.find_by(user_id: current_user) ||
membershipable.requesters.find_by(user_id: current_user)
Members::DestroyService.new(@member, current_user).execute Members::DestroyService.new(@member, current_user).execute
source_type = @member.real_source_type.humanize(capitalize: false) source_type = @member.real_source_type.humanize(capitalize: false)
......
...@@ -7,7 +7,7 @@ class Groups::GroupMembersController < Groups::ApplicationController ...@@ -7,7 +7,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
def index def index
@project = @group.projects.find(params[:project_id]) if params[:project_id] @project = @group.projects.find(params[:project_id]) if params[:project_id]
@members = @group.group_members @members = @group.group_members
@members = @members.non_pending unless can?(current_user, :admin_group, @group) @members = @members.non_invite unless can?(current_user, :admin_group, @group)
if params[:search].present? if params[:search].present?
users = @group.users.search(params[:search]).to_a users = @group.users.search(params[:search]).to_a
...@@ -15,6 +15,7 @@ class Groups::GroupMembersController < Groups::ApplicationController ...@@ -15,6 +15,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
end end
@members = @members.order('access_level DESC').page(params[:page]).per(50) @members = @members.order('access_level DESC').page(params[:page]).per(50)
@requesters = @group.requesters if can?(current_user, :admin_group, @group)
@group_member = @group.group_members.new @group_member = @group.group_members.new
end end
...@@ -34,7 +35,8 @@ class Groups::GroupMembersController < Groups::ApplicationController ...@@ -34,7 +35,8 @@ class Groups::GroupMembersController < Groups::ApplicationController
end end
def destroy def destroy
@group_member = @group.group_members.find(params[:id]) @group_member = @group.members.find_by(id: params[:id]) ||
@group.requesters.find_by(id: params[:id])
Members::DestroyService.new(@group_member, current_user).execute Members::DestroyService.new(@group_member, current_user).execute
......
...@@ -6,7 +6,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController ...@@ -6,7 +6,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
def index def index
@project_members = @project.project_members @project_members = @project.project_members
@project_members = @project_members.non_pending unless can?(current_user, :admin_project, @project) @project_members = @project_members.non_invite unless can?(current_user, :admin_project, @project)
if params[:search].present? if params[:search].present?
users = @project.users.search(params[:search]).to_a users = @project.users.search(params[:search]).to_a
...@@ -19,7 +19,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController ...@@ -19,7 +19,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
if @group if @group
@group_members = @group.group_members @group_members = @group.group_members
@group_members = @group_members.non_pending unless can?(current_user, :admin_group, @group) @group_members = @group_members.non_invite unless can?(current_user, :admin_group, @group)
if params[:search].present? if params[:search].present?
users = @group.users.search(params[:search]).to_a users = @group.users.search(params[:search]).to_a
...@@ -29,6 +29,8 @@ class Projects::ProjectMembersController < Projects::ApplicationController ...@@ -29,6 +29,8 @@ class Projects::ProjectMembersController < Projects::ApplicationController
@group_members = @group_members.order('access_level DESC') @group_members = @group_members.order('access_level DESC')
end end
@requesters = @project.requesters if can?(current_user, :admin_project, @project)
@project_member = @project.project_members.new @project_member = @project.project_members.new
@project_group_links = @project.project_group_links @project_group_links = @project.project_group_links
end end
...@@ -48,7 +50,8 @@ class Projects::ProjectMembersController < Projects::ApplicationController ...@@ -48,7 +50,8 @@ class Projects::ProjectMembersController < Projects::ApplicationController
end end
def destroy def destroy
@project_member = @project.project_members.find(params[:id]) @project_member = @project.members.find_by(id: params[:id]) ||
@project.requesters.find_by(id: params[:id])
Members::DestroyService.new(@project_member, current_user).execute Members::DestroyService.new(@project_member, current_user).execute
......
...@@ -12,6 +12,17 @@ module MembersHelper ...@@ -12,6 +12,17 @@ module MembersHelper
can?(current_user, action_member_permission(:admin, member), member.source) can?(current_user, action_member_permission(:admin, member), member.source)
end end
def can_see_request_access_button?(source)
source_parent = source.respond_to?(:group) && source.group
return false if source_parent && source.group.members.exists?(user_id: current_user.id)
return false if source_parent && source.group.requesters.exists?(user_id: current_user.id)
return false if source.members.exists?(user_id: current_user.id)
return true if source.requesters.exists?(user_id: current_user.id)
true
end
def remove_member_message(member, user: nil) def remove_member_message(member, user: nil)
user = current_user if defined?(current_user) user = current_user if defined?(current_user)
......
...@@ -6,15 +6,16 @@ class Group < Namespace ...@@ -6,15 +6,16 @@ class Group < Namespace
include AccessRequestable include AccessRequestable
include Referable include Referable
has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember' has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember'
alias_method :members, :group_members alias_method :members, :group_members
has_many :users, -> { where(members: { requested_at: nil }) }, through: :group_members has_many :users, through: :group_members
has_many :owners, has_many :owners,
-> { where(members: { requested_at: nil, access_level: Gitlab::Access::OWNER }) }, -> { where(members: { access_level: Gitlab::Access::OWNER }) },
through: :group_members, through: :group_members,
source: :user source: :user
has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember'
has_many :project_group_links, dependent: :destroy has_many :project_group_links, dependent: :destroy
has_many :shared_projects, through: :project_group_links, source: :project has_many :shared_projects, through: :project_group_links, source: :project
has_many :notification_settings, dependent: :destroy, as: :source has_many :notification_settings, dependent: :destroy, as: :source
......
...@@ -30,8 +30,6 @@ class Member < ActiveRecord::Base ...@@ -30,8 +30,6 @@ class Member < ActiveRecord::Base
scope :invite, -> { where.not(invite_token: nil) } scope :invite, -> { where.not(invite_token: nil) }
scope :non_invite, -> { where(invite_token: nil) } scope :non_invite, -> { where(invite_token: nil) }
scope :request, -> { where.not(requested_at: nil) } scope :request, -> { where.not(requested_at: nil) }
scope :non_request, -> { where(requested_at: nil) }
scope :non_pending, -> { non_request.non_invite }
scope :has_access, -> { where('access_level > 0') } scope :has_access, -> { where('access_level > 0') }
scope :guests, -> { where(access_level: GUEST) } scope :guests, -> { where(access_level: GUEST) }
......
...@@ -108,9 +108,13 @@ class Project < ActiveRecord::Base ...@@ -108,9 +108,13 @@ class Project < ActiveRecord::Base
has_many :snippets, dependent: :destroy, class_name: 'ProjectSnippet' has_many :snippets, dependent: :destroy, class_name: 'ProjectSnippet'
has_many :hooks, dependent: :destroy, class_name: 'ProjectHook' has_many :hooks, dependent: :destroy, class_name: 'ProjectHook'
has_many :protected_branches, dependent: :destroy has_many :protected_branches, dependent: :destroy
has_many :project_members, dependent: :destroy, as: :source, class_name: 'ProjectMember'
has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'ProjectMember'
alias_method :members, :project_members alias_method :members, :project_members
has_many :users, -> { where(members: { requested_at: nil }) }, through: :project_members has_many :users, through: :project_members
has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'ProjectMember'
has_many :deploy_keys_projects, dependent: :destroy has_many :deploy_keys_projects, dependent: :destroy
has_many :deploy_keys, through: :deploy_keys_projects has_many :deploy_keys, through: :deploy_keys_projects
has_many :users_star_projects, dependent: :destroy has_many :users_star_projects, dependent: :destroy
......
...@@ -22,12 +22,12 @@ class ProjectTeam ...@@ -22,12 +22,12 @@ class ProjectTeam
end end
def find_member(user_id) def find_member(user_id)
member = project.members.non_request.find_by(user_id: user_id) member = project.members.find_by(user_id: user_id)
# If user is not in project members # If user is not in project members
# we should check for group membership # we should check for group membership
if group && !member if group && !member
member = group.members.non_request.find_by(user_id: user_id) member = group.members.find_by(user_id: user_id)
end end
member member
...@@ -137,10 +137,10 @@ class ProjectTeam ...@@ -137,10 +137,10 @@ class ProjectTeam
def max_member_access(user_id) def max_member_access(user_id)
access = [] access = []
access += project.members.non_request.where(user_id: user_id).has_access.pluck(:access_level) access += project.members.where(user_id: user_id).has_access.pluck(:access_level)
if group if group
access += group.members.non_request.where(user_id: user_id).has_access.pluck(:access_level) access += group.members.where(user_id: user_id).has_access.pluck(:access_level)
end end
if project.invited_groups.any? && project.allowed_to_share_with_group? if project.invited_groups.any? && project.allowed_to_share_with_group?
...@@ -168,14 +168,14 @@ class ProjectTeam ...@@ -168,14 +168,14 @@ class ProjectTeam
end end
def fetch_members(level = nil) def fetch_members(level = nil)
project_members = project.members.non_request project_members = project.members
group_members = group ? group.members.non_request : [] group_members = group ? group.members : []
invited_members = [] invited_members = []
if project.invited_groups.any? && project.allowed_to_share_with_group? if project.invited_groups.any? && project.allowed_to_share_with_group?
project.project_group_links.each do |group_link| project.project_group_links.each do |group_link|
invited_group = group_link.group invited_group = group_link.group
im = invited_group.members.non_request im = invited_group.members
if level if level
int_level = GroupMember.access_level_roles[level.to_s.singularize.titleize] int_level = GroupMember.access_level_roles[level.to_s.singularize.titleize]
......
...@@ -89,16 +89,16 @@ ...@@ -89,16 +89,16 @@
%hr %hr
= button_tag 'Add users to group', class: "btn btn-create" = button_tag 'Add users to group', class: "btn btn-create"
= render 'shared/members/requests', membership_source: @group, members: @members.request = render 'shared/members/requests', membership_source: @group, requesters: @requesters
.panel.panel-default .panel.panel-default
.panel-heading .panel-heading
%strong= @group.name %strong= @group.name
group members group members
%span.badge= @group.members.non_request.size %span.badge= @group.members.size
.pull-right .pull-right
= link_to icon('pencil-square-o', text: 'Manage Access'), polymorphic_url([@group, :members]), class: "btn btn-xs" = link_to icon('pencil-square-o', text: 'Manage Access'), polymorphic_url([@group, :members]), class: "btn btn-xs"
%ul.well-list.group-users-list.content-list %ul.well-list.group-users-list.content-list
= render partial: 'shared/members/member', collection: @members.non_request, as: :member, locals: { show_controls: false } = render partial: 'shared/members/member', collection: @members, as: :member, locals: { show_controls: false }
.panel-footer .panel-footer
= paginate @members.non_request, param_name: 'members_page', theme: 'gitlab' = paginate @members, param_name: 'members_page', theme: 'gitlab'
...@@ -137,16 +137,16 @@ ...@@ -137,16 +137,16 @@
.panel-heading .panel-heading
%strong= @group.name %strong= @group.name
group members group members
%span.badge= @group_members.non_request.size %span.badge= @group_members.size
.pull-right .pull-right
= link_to admin_group_path(@group), class: 'btn btn-xs' do = link_to admin_group_path(@group), class: 'btn btn-xs' do
= icon('pencil-square-o', text: 'Manage Access') = icon('pencil-square-o', text: 'Manage Access')
%ul.well-list.content-list %ul.well-list.content-list
= render partial: 'shared/members/member', collection: @group_members.non_request, as: :member, locals: { show_controls: false } = render partial: 'shared/members/member', collection: @group_members, as: :member, locals: { show_controls: false }
.panel-footer .panel-footer
= paginate @group_members.non_request, param_name: 'group_members_page', theme: 'gitlab' = paginate @group_members, param_name: 'group_members_page', theme: 'gitlab'
= render 'shared/members/requests', membership_source: @project, members: @project_members.request = render 'shared/members/requests', membership_source: @project, requesters: @requesters
.panel.panel-default .panel.panel-default
.panel-heading .panel-heading
...@@ -156,6 +156,6 @@ ...@@ -156,6 +156,6 @@
.pull-right .pull-right
= link_to icon('pencil-square-o', text: 'Manage Access'), polymorphic_url([@project, :members]), class: "btn btn-xs" = link_to icon('pencil-square-o', text: 'Manage Access'), polymorphic_url([@project, :members]), class: "btn btn-xs"
%ul.well-list.project_members.content-list %ul.well-list.project_members.content-list
= render partial: 'shared/members/member', collection: @project_members.non_request, as: :member, locals: { show_controls: false } = render partial: 'shared/members/member', collection: @project_members, as: :member, locals: { show_controls: false }
.panel-footer .panel-footer
= paginate @project_members.non_request, param_name: 'project_members_page', theme: 'gitlab' = paginate @project_members, param_name: 'project_members_page', theme: 'gitlab'
- page_title "Members" - page_title "Members"
.group-members-page.prepend-top-default .group-members-page.prepend-top-default
- if current_user && current_user.can?(:admin_group_member, @group) - if can?(current_user, :admin_group_member, @group)
.panel.panel-default .panel.panel-default
.panel-heading .panel-heading
Add new user to group Add new user to group
...@@ -11,13 +11,13 @@ ...@@ -11,13 +11,13 @@
.new-group-member-holder .new-group-member-holder
= render "new_group_member" = render "new_group_member"
= render 'shared/members/requests', membership_source: @group, members: @members.request = render 'shared/members/requests', membership_source: @group, requesters: @requesters
.panel.panel-default .panel.panel-default
.panel-heading .panel-heading
%strong #{@group.name} %strong #{@group.name}
group members group members
%span.badge= @members.non_request.size %span.badge= @members.size
.controls .controls
= form_tag group_group_members_path(@group), method: :get, class: 'form-inline member-search-form' do = form_tag group_group_members_path(@group), method: :get, class: 'form-inline member-search-form' do
.form-group .form-group
...@@ -25,8 +25,8 @@ ...@@ -25,8 +25,8 @@
= button_tag class: 'btn', title: 'Search' do = button_tag class: 'btn', title: 'Search' do
= icon("search") = icon("search")
%ul.content-list %ul.content-list
= render partial: 'shared/members/member', collection: @members.non_request, as: :member = render partial: 'shared/members/member', collection: @members, as: :member
= paginate @members.non_request, theme: 'gitlab' = paginate @members, theme: 'gitlab'
:javascript :javascript
$('form.member-search-form').on('submit', function(event) { $('form.member-search-form').on('submit', function(event) {
......
- if current_user - if current_user
- can_edit = can?(current_user, :admin_group, @group) - can_edit = can?(current_user, :admin_group, @group)
- member = @group.members.non_request.find_by(user_id: current_user.id) - member = @group.members.find_by(user_id: current_user.id)
- can_leave = member && can?(current_user, :destroy_group_member, member) - can_leave = member && can?(current_user, :destroy_group_member, member)
.controls .controls
......
...@@ -7,7 +7,7 @@ ...@@ -7,7 +7,7 @@
%ul.dropdown-menu.dropdown-menu-align-right %ul.dropdown-menu.dropdown-menu-align-right
- can_edit = can?(current_user, :admin_project, @project) - can_edit = can?(current_user, :admin_project, @project)
-# We don't use @project.team.find_member because it searches for group members too... -# 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) - member = @project.members.find_by(user_id: current_user.id)
- can_leave = member && can?(current_user, :destroy_project_member, member) - can_leave = member && can?(current_user, :destroy_project_member, member)
= render 'layouts/nav/project_settings', can_edit: can_edit = render 'layouts/nav/project_settings', can_edit: can_edit
......
- @project_group_links.each do |group_links| - @project_group_links.each do |group_links|
- shared_group = group_links.group - shared_group = group_links.group
- shared_group_members = shared_group.members.non_request - shared_group_members = shared_group.members
- shared_group_users_count = shared_group_members.size - shared_group_users_count = shared_group_members.size
.panel.panel-default .panel.panel-default
.panel-heading .panel-heading
......
...@@ -13,12 +13,12 @@ ...@@ -13,12 +13,12 @@
Users with access to this project are listed below. Users with access to this project are listed below.
= render "new_project_member" = render "new_project_member"
= render 'shared/members/requests', membership_source: @project, members: @project_members.request = render 'shared/members/requests', membership_source: @project, requesters: @requesters
= render 'team', members: @project_members.non_request = render 'team', members: @project_members
- if @group - if @group
= render "group_members", members: @group_members.non_request = render "group_members", members: @group_members
- if @project_group_links.any? && @project.allowed_to_share_with_group? - if @project_group_links.any? && @project.allowed_to_share_with_group?
= render "shared_group_members" = render "shared_group_members"
- member = source.members.find_by(user_id: current_user.id) - if can_see_request_access_button?(source)
- group_member = source.group.members.find_by(user_id: current_user.id) if source.respond_to?(:group) && source.group - if requester = source.requesters.find_by(user_id: current_user.id)
- unless group_member
- if member
- if member.request?
= link_to 'Withdraw Access Request', polymorphic_path([:leave, source, :members]), = link_to 'Withdraw Access Request', polymorphic_path([:leave, source, :members]),
method: :delete, method: :delete,
data: { confirm: remove_member_message(member) }, data: { confirm: remove_member_message(requester) },
class: 'btn' class: 'btn'
- else - else
= link_to 'Request Access', polymorphic_path([:request_access, source, :members]), = link_to 'Request Access', polymorphic_path([:request_access, source, :members]),
......
- if members.any? - if requesters.any?
.panel.panel-default .panel.panel-default
.panel-heading .panel-heading
%strong= membership_source.name %strong= membership_source.name
access requests access requests
%span.badge= members.size %span.badge= requesters.size
%ul.content-list %ul.content-list
= render partial: 'shared/members/member', collection: members, as: :member = render partial: 'shared/members/member', collection: requesters, as: :member
...@@ -133,7 +133,7 @@ describe Groups::GroupMembersController do ...@@ -133,7 +133,7 @@ describe Groups::GroupMembersController do
expect(response).to set_flash.to 'Your access request to the group has been withdrawn.' expect(response).to set_flash.to 'Your access request to the group has been withdrawn.'
expect(response).to redirect_to(group_path(group)) expect(response).to redirect_to(group_path(group))
expect(group.members.request).to be_empty expect(group.requesters).to be_empty
expect(group.users).not_to include user expect(group.users).not_to include user
end end
end end
...@@ -153,7 +153,7 @@ describe Groups::GroupMembersController do ...@@ -153,7 +153,7 @@ describe Groups::GroupMembersController do
expect(response).to set_flash.to 'Your request for access has been queued for review.' expect(response).to set_flash.to 'Your request for access has been queued for review.'
expect(response).to redirect_to(group_path(group)) expect(response).to redirect_to(group_path(group))
expect(group.members.request.exists?(user_id: user)).to be_truthy expect(group.requesters.exists?(user_id: user)).to be_truthy
expect(group.users).not_to include user expect(group.users).not_to include user
end end
end end
...@@ -175,7 +175,7 @@ describe Groups::GroupMembersController do ...@@ -175,7 +175,7 @@ describe Groups::GroupMembersController do
let(:group_requester) { create(:user) } let(:group_requester) { create(:user) }
let(:member) do let(:member) do
group.request_access(group_requester) group.request_access(group_requester)
group.members.request.find_by(user_id: group_requester) group.requesters.find_by(user_id: group_requester)
end end
context 'when user does not have enough rights' do context 'when user does not have enough rights' do
......
...@@ -187,7 +187,7 @@ describe Projects::ProjectMembersController do ...@@ -187,7 +187,7 @@ describe Projects::ProjectMembersController do
expect(response).to set_flash.to 'Your access request to the project has been withdrawn.' expect(response).to set_flash.to 'Your access request to the project has been withdrawn.'
expect(response).to redirect_to(namespace_project_path(project.namespace, project)) expect(response).to redirect_to(namespace_project_path(project.namespace, project))
expect(project.members.request).to be_empty expect(project.requesters).to be_empty
expect(project.users).not_to include user expect(project.users).not_to include user
end end
end end
...@@ -210,7 +210,7 @@ describe Projects::ProjectMembersController do ...@@ -210,7 +210,7 @@ describe Projects::ProjectMembersController do
expect(response).to redirect_to( expect(response).to redirect_to(
namespace_project_path(project.namespace, project) namespace_project_path(project.namespace, project)
) )
expect(project.members.request.exists?(user_id: user)).to be_truthy expect(project.requesters.exists?(user_id: user)).to be_truthy
expect(project.users).not_to include user expect(project.users).not_to include user
end end
end end
...@@ -233,7 +233,7 @@ describe Projects::ProjectMembersController do ...@@ -233,7 +233,7 @@ describe Projects::ProjectMembersController do
let(:team_requester) { create(:user) } let(:team_requester) { create(:user) }
let(:member) do let(:member) do
project.request_access(team_requester) project.request_access(team_requester)
project.members.request.find_by(user_id: team_requester.id) project.requesters.find_by(user_id: team_requester.id)
end end
context 'when user does not have enough rights' do context 'when user does not have enough rights' do
......
...@@ -40,7 +40,7 @@ feature 'Groups > Members > Owner manages access requests', feature: true do ...@@ -40,7 +40,7 @@ feature 'Groups > Members > Owner manages access requests', feature: true do
end end
def expect_visible_access_request(group, user) def expect_visible_access_request(group, user)
expect(group.members.request.exists?(user_id: user)).to be_truthy expect(group.requesters.exists?(user_id: user)).to be_truthy
expect(page).to have_content "#{group.name} access requests 1" expect(page).to have_content "#{group.name} access requests 1"
expect(page).to have_content user.name expect(page).to have_content user.name
end end
......
...@@ -18,7 +18,7 @@ feature 'Groups > Members > User requests access', feature: true do ...@@ -18,7 +18,7 @@ feature 'Groups > Members > User requests access', feature: true do
expect(ActionMailer::Base.deliveries.last.to).to eq [owner.notification_email] expect(ActionMailer::Base.deliveries.last.to).to eq [owner.notification_email]
expect(ActionMailer::Base.deliveries.last.subject).to match "Request to join the #{group.name} group" expect(ActionMailer::Base.deliveries.last.subject).to match "Request to join the #{group.name} group"
expect(group.members.request.exists?(user_id: user)).to be_truthy expect(group.requesters.exists?(user_id: user)).to be_truthy
expect(page).to have_content 'Your request for access has been queued for review.' expect(page).to have_content 'Your request for access has been queued for review.'
expect(page).to have_content 'Withdraw Access Request' expect(page).to have_content 'Withdraw Access Request'
...@@ -42,7 +42,7 @@ feature 'Groups > Members > User requests access', feature: true do ...@@ -42,7 +42,7 @@ feature 'Groups > Members > User requests access', feature: true do
scenario 'user is not listed in the group members page' do scenario 'user is not listed in the group members page' do
click_link 'Request Access' click_link 'Request Access'
expect(group.members.request.exists?(user_id: user)).to be_truthy expect(group.requesters.exists?(user_id: user)).to be_truthy
click_link 'Members' click_link 'Members'
...@@ -54,11 +54,11 @@ feature 'Groups > Members > User requests access', feature: true do ...@@ -54,11 +54,11 @@ feature 'Groups > Members > User requests access', feature: true do
scenario 'user can withdraw its request for access' do scenario 'user can withdraw its request for access' do
click_link 'Request Access' click_link 'Request Access'
expect(group.members.request.exists?(user_id: user)).to be_truthy expect(group.requesters.exists?(user_id: user)).to be_truthy
click_link 'Withdraw Access Request' click_link 'Withdraw Access Request'
expect(group.members.request.exists?(user_id: user)).to be_falsey expect(group.requesters.exists?(user_id: user)).to be_falsey
expect(page).to have_content 'Your access request to the group has been withdrawn.' expect(page).to have_content 'Your access request to the group has been withdrawn.'
end end
end end
...@@ -40,7 +40,7 @@ feature 'Projects > Members > Master manages access requests', feature: true do ...@@ -40,7 +40,7 @@ feature 'Projects > Members > Master manages access requests', feature: true do
end end
def expect_visible_access_request(project, user) def expect_visible_access_request(project, user)
expect(project.members.request.exists?(user_id: user)).to be_truthy expect(project.requesters.exists?(user_id: user)).to be_truthy
expect(page).to have_content "#{project.name} access requests 1" expect(page).to have_content "#{project.name} access requests 1"
expect(page).to have_content user.name expect(page).to have_content user.name
end end
......
...@@ -17,7 +17,7 @@ feature 'Projects > Members > User requests access', feature: true do ...@@ -17,7 +17,7 @@ feature 'Projects > Members > User requests access', feature: true do
expect(ActionMailer::Base.deliveries.last.to).to eq [master.notification_email] expect(ActionMailer::Base.deliveries.last.to).to eq [master.notification_email]
expect(ActionMailer::Base.deliveries.last.subject).to eq "Request to join the #{project.name_with_namespace} project" expect(ActionMailer::Base.deliveries.last.subject).to eq "Request to join the #{project.name_with_namespace} project"
expect(project.members.request.exists?(user_id: user)).to be_truthy expect(project.requesters.exists?(user_id: user)).to be_truthy
expect(page).to have_content 'Your request for access has been queued for review.' expect(page).to have_content 'Your request for access has been queued for review.'
expect(page).to have_content 'Withdraw Access Request' expect(page).to have_content 'Withdraw Access Request'
...@@ -27,7 +27,7 @@ feature 'Projects > Members > User requests access', feature: true do ...@@ -27,7 +27,7 @@ feature 'Projects > Members > User requests access', feature: true do
scenario 'user is not listed in the project members page' do scenario 'user is not listed in the project members page' do
click_link 'Request Access' click_link 'Request Access'
expect(project.members.request.exists?(user_id: user)).to be_truthy expect(project.requesters.exists?(user_id: user)).to be_truthy
open_project_settings_menu open_project_settings_menu
click_link 'Members' click_link 'Members'
...@@ -41,11 +41,11 @@ feature 'Projects > Members > User requests access', feature: true do ...@@ -41,11 +41,11 @@ feature 'Projects > Members > User requests access', feature: true do
scenario 'user can withdraw its request for access' do scenario 'user can withdraw its request for access' do
click_link 'Request Access' click_link 'Request Access'
expect(project.members.request.exists?(user_id: user)).to be_truthy expect(project.requesters.exists?(user_id: user)).to be_truthy
click_link 'Withdraw Access Request' click_link 'Withdraw Access Request'
expect(project.members.request.exists?(user_id: user)).to be_falsey expect(project.requesters.exists?(user_id: user)).to be_falsey
expect(page).to have_content 'Your access request to the project has been withdrawn.' expect(page).to have_content 'Your access request to the project has been withdrawn.'
end end
......
...@@ -57,6 +57,72 @@ describe MembersHelper do ...@@ -57,6 +57,72 @@ describe MembersHelper do
end end
end end
describe '#can_see_request_access_button?' do
let(:user) { create(:user) }
let(:group) { create(:group, :public) }
let(:project) { create(:project, :public, group: group) }
before do
allow(helper).to receive(:current_user).and_return(user)
end
context 'source is a group' do
context 'current_user is not a member' do
it 'returns true' do
expect(helper.can_see_request_access_button?(group)).to be_truthy
end
end
context 'current_user is a member' do
it 'returns false' do
group.add_owner(user)
expect(helper.can_see_request_access_button?(group)).to be_falsy
end
end
context 'current_user is a requester' do
it 'returns true' do
group.request_access(user)
expect(helper.can_see_request_access_button?(group)).to be_truthy
end
end
end
context 'source is a project' do
context 'current_user is not a member' do
it 'returns true' do
expect(helper.can_see_request_access_button?(project)).to be_truthy
end
end
context 'current_user is a group member' do
it 'returns false' do
group.add_owner(user)
expect(helper.can_see_request_access_button?(project)).to be_falsy
end
end
context 'current_user is a group requester' do
it 'returns false' do
group.request_access(user)
expect(helper.can_see_request_access_button?(project)).to be_falsy
end
end
context 'current_user is a member' do
it 'returns false' do
project.team << [user, :master]
expect(helper.can_see_request_access_button?(project)).to be_falsy
end
end
end
end
describe '#remove_member_message' do describe '#remove_member_message' do
let(:requester) { build(:user) } let(:requester) { build(:user) }
let(:project) { create(:project) } let(:project) { create(:project) }
......
...@@ -406,7 +406,7 @@ describe Notify do ...@@ -406,7 +406,7 @@ describe Notify do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project_member) do let(:project_member) do
project.request_access(user) project.request_access(user)
project.members.request.find_by(user_id: user.id) project.requesters.find_by(user_id: user.id)
end end
subject { Notify.member_access_requested_email('project', project_member.id) } subject { Notify.member_access_requested_email('project', project_member.id) }
...@@ -433,7 +433,7 @@ describe Notify do ...@@ -433,7 +433,7 @@ describe Notify do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project_member) do let(:project_member) do
project.request_access(user) project.request_access(user)
project.members.request.find_by(user_id: user.id) project.requesters.find_by(user_id: user.id)
end end
subject { Notify.member_access_requested_email('project', project_member.id) } subject { Notify.member_access_requested_email('project', project_member.id) }
...@@ -459,7 +459,7 @@ describe Notify do ...@@ -459,7 +459,7 @@ describe Notify do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project_member) do let(:project_member) do
project.request_access(user) project.request_access(user)
project.members.request.find_by(user_id: user.id) project.requesters.find_by(user_id: user.id)
end end
subject { Notify.member_access_denied_email('project', project.id, user.id) } subject { Notify.member_access_denied_email('project', project.id, user.id) }
...@@ -684,7 +684,7 @@ describe Notify do ...@@ -684,7 +684,7 @@ describe Notify do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:group_member) do let(:group_member) do
group.request_access(user) group.request_access(user)
group.members.request.find_by(user_id: user.id) group.requesters.find_by(user_id: user.id)
end end
subject { Notify.member_access_requested_email('group', group_member.id) } subject { Notify.member_access_requested_email('group', group_member.id) }
...@@ -705,7 +705,7 @@ describe Notify do ...@@ -705,7 +705,7 @@ describe Notify do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:group_member) do let(:group_member) do
group.request_access(user) group.request_access(user)
group.members.request.find_by(user_id: user.id) group.requesters.find_by(user_id: user.id)
end end
subject { Notify.member_access_denied_email('group', group.id, user.id) } subject { Notify.member_access_denied_email('group', group.id, user.id) }
......
...@@ -16,7 +16,7 @@ describe AccessRequestable do ...@@ -16,7 +16,7 @@ describe AccessRequestable do
before { group.request_access(user) } before { group.request_access(user) }
it { expect(group.members.request.exists?(user_id: user)).to be_truthy } it { expect(group.requesters.exists?(user_id: user)).to be_truthy }
end end
end end
...@@ -34,7 +34,7 @@ describe AccessRequestable do ...@@ -34,7 +34,7 @@ describe AccessRequestable do
before { project.request_access(user) } before { project.request_access(user) }
it { expect(project.members.request.exists?(user_id: user)).to be_truthy } it { expect(project.requesters.exists?(user_id: user)).to be_truthy }
end end
end end
end end
...@@ -7,9 +7,38 @@ describe Group, models: true do ...@@ -7,9 +7,38 @@ describe Group, models: true do
it { is_expected.to have_many :projects } it { is_expected.to have_many :projects }
it { is_expected.to have_many(:group_members).dependent(:destroy) } it { is_expected.to have_many(:group_members).dependent(:destroy) }
it { is_expected.to have_many(:users).through(:group_members) } it { is_expected.to have_many(:users).through(:group_members) }
it { is_expected.to have_many(:owners).through(:group_members) }
it { is_expected.to have_many(:requesters).dependent(:destroy) }
it { is_expected.to have_many(:project_group_links).dependent(:destroy) } it { is_expected.to have_many(:project_group_links).dependent(:destroy) }
it { is_expected.to have_many(:shared_projects).through(:project_group_links) } it { is_expected.to have_many(:shared_projects).through(:project_group_links) }
it { is_expected.to have_many(:notification_settings).dependent(:destroy) } it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
describe '#members & #requesters' do
let(:requester) { create(:user) }
let(:developer) { create(:user) }
before do
group.request_access(requester)
group.add_developer(developer)
end
describe '#members' do
it 'includes members and exclude requesters' do
member_user_ids = group.members.pluck(:user_id)
expect(member_user_ids).to include(developer.id)
expect(member_user_ids).not_to include(requester.id)
end
end
describe '#requesters' do
it 'does not include requesters' do
requester_user_ids = group.requesters.pluck(:user_id)
expect(requester_user_ids).to include(requester.id)
expect(requester_user_ids).not_to include(developer.id)
end
end
end
end end
describe 'modules' do describe 'modules' do
......
...@@ -73,10 +73,10 @@ describe Member, models: true do ...@@ -73,10 +73,10 @@ describe Member, models: true do
@accepted_invite_member = project.members.invite.find_by_invite_email('toto2@example.com').tap { |u| u.accept_invite!(accepted_invite_user) } @accepted_invite_member = project.members.invite.find_by_invite_email('toto2@example.com').tap { |u| u.accept_invite!(accepted_invite_user) }
requested_user = create(:user).tap { |u| project.request_access(u) } requested_user = create(:user).tap { |u| project.request_access(u) }
@requested_member = project.members.request.find_by(user_id: requested_user.id) @requested_member = project.requesters.find_by(user_id: requested_user.id)
accepted_request_user = create(:user).tap { |u| project.request_access(u) } accepted_request_user = create(:user).tap { |u| project.request_access(u) }
@accepted_request_member = project.members.request.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request } @accepted_request_member = project.requesters.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request }
end end
describe '.invite' do describe '.invite' do
...@@ -103,22 +103,6 @@ describe Member, models: true do ...@@ -103,22 +103,6 @@ describe Member, models: true do
it { expect(described_class.request).not_to include @accepted_request_member } it { expect(described_class.request).not_to include @accepted_request_member }
end end
describe '.non_request' do
it { expect(described_class.non_request).to include @master }
it { expect(described_class.non_request).to include @invited_member }
it { expect(described_class.non_request).to include @accepted_invite_member }
it { expect(described_class.non_request).not_to include @requested_member }
it { expect(described_class.non_request).to include @accepted_request_member }
end
describe '.non_pending' do
it { expect(described_class.non_pending).to include @master }
it { expect(described_class.non_pending).not_to include @invited_member }
it { expect(described_class.non_pending).to include @accepted_invite_member }
it { expect(described_class.non_pending).not_to include @requested_member }
it { expect(described_class.non_pending).to include @accepted_request_member }
end
describe '.owners_and_masters' do describe '.owners_and_masters' do
it { expect(described_class.owners_and_masters).to include @owner } it { expect(described_class.owners_and_masters).to include @owner }
it { expect(described_class.owners_and_masters).to include @master } it { expect(described_class.owners_and_masters).to include @master }
......
...@@ -11,6 +11,8 @@ describe Project, models: true do ...@@ -11,6 +11,8 @@ describe Project, models: true do
it { is_expected.to have_many(:issues).dependent(:destroy) } it { is_expected.to have_many(:issues).dependent(:destroy) }
it { is_expected.to have_many(:milestones).dependent(:destroy) } it { is_expected.to have_many(:milestones).dependent(:destroy) }
it { is_expected.to have_many(:project_members).dependent(:destroy) } it { is_expected.to have_many(:project_members).dependent(:destroy) }
it { is_expected.to have_many(:users).through(:project_members) }
it { is_expected.to have_many(:requesters).dependent(:destroy) }
it { is_expected.to have_many(:notes).dependent(:destroy) } it { is_expected.to have_many(:notes).dependent(:destroy) }
it { is_expected.to have_many(:snippets).class_name('ProjectSnippet').dependent(:destroy) } it { is_expected.to have_many(:snippets).class_name('ProjectSnippet').dependent(:destroy) }
it { is_expected.to have_many(:deploy_keys_projects).dependent(:destroy) } it { is_expected.to have_many(:deploy_keys_projects).dependent(:destroy) }
...@@ -31,6 +33,34 @@ describe Project, models: true do ...@@ -31,6 +33,34 @@ describe Project, models: true do
it { is_expected.to have_many(:environments).dependent(:destroy) } it { is_expected.to have_many(:environments).dependent(:destroy) }
it { is_expected.to have_many(:deployments).dependent(:destroy) } it { is_expected.to have_many(:deployments).dependent(:destroy) }
it { is_expected.to have_many(:todos).dependent(:destroy) } it { is_expected.to have_many(:todos).dependent(:destroy) }
describe '#members & #requesters' do
let(:project) { create(:project) }
let(:requester) { create(:user) }
let(:developer) { create(:user) }
before do
project.request_access(requester)
project.team << [developer, :developer]
end
describe '#members' do
it 'includes members and exclude requesters' do
member_user_ids = project.members.pluck(:user_id)
expect(member_user_ids).to include(developer.id)
expect(member_user_ids).not_to include(requester.id)
end
end
describe '#requesters' do
it 'does not include requesters' do
requester_user_ids = project.requesters.pluck(:user_id)
expect(requester_user_ids).to include(requester.id)
expect(requester_user_ids).not_to include(developer.id)
end
end
end
end end
describe 'modules' do describe 'modules' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment