Commit 7d3860c3 authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot

Merge branch 'security-subgroup-member-list-parent-group-member-14-10' into '14-10-stable-ee'

Subgroup member can list members of parent group

See merge request gitlab-org/security/gitlab!2480
parents 0c542486 b59c49fa
...@@ -67,6 +67,12 @@ class Groups::ApplicationController < ApplicationController ...@@ -67,6 +67,12 @@ class Groups::ApplicationController < ApplicationController
end end
end end
def authorize_read_group_member!
unless can?(current_user, :read_group_member, group)
render_403
end
end
def build_canonical_path(group) def build_canonical_path(group)
params[:group_id] = group.to_param params[:group_id] = group.to_param
......
...@@ -14,6 +14,7 @@ class Groups::GroupMembersController < Groups::ApplicationController ...@@ -14,6 +14,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
# Authorize # Authorize
before_action :authorize_admin_group_member!, except: admin_not_required_endpoints before_action :authorize_admin_group_member!, except: admin_not_required_endpoints
before_action :authorize_read_group_member!, only: :index
skip_before_action :check_two_factor_requirement, only: :leave skip_before_action :check_two_factor_requirement, only: :leave
skip_cross_project_access_check :index, :update, :destroy, :request_access, skip_cross_project_access_check :index, :update, :destroy, :request_access,
......
...@@ -22,6 +22,7 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy ...@@ -22,6 +22,7 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
condition(:share_with_group_locked, scope: :subject) { @subject.share_with_group_lock? } condition(:share_with_group_locked, scope: :subject) { @subject.share_with_group_lock? }
condition(:parent_share_with_group_locked, scope: :subject) { @subject.parent&.share_with_group_lock? } condition(:parent_share_with_group_locked, scope: :subject) { @subject.parent&.share_with_group_lock? }
condition(:can_change_parent_share_with_group_lock) { can?(:change_share_with_group_lock, @subject.parent) } condition(:can_change_parent_share_with_group_lock) { can?(:change_share_with_group_lock, @subject.parent) }
condition(:can_read_group_member) { can_read_group_member? }
desc "User is a project bot" desc "User is a project bot"
condition(:project_bot) { user.project_bot? && access_level >= GroupMember::GUEST } condition(:project_bot) { user.project_bot? && access_level >= GroupMember::GUEST }
...@@ -127,6 +128,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy ...@@ -127,6 +128,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
rule { ~public_group & ~has_access }.prevent :read_counts rule { ~public_group & ~has_access }.prevent :read_counts
rule { ~can_read_group_member }.policy do
prevent :read_group_member
end
rule { ~can?(:read_group) }.policy do rule { ~can?(:read_group) }.policy do
prevent :read_design_activity prevent :read_design_activity
end end
...@@ -308,6 +313,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy ...@@ -308,6 +313,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
true true
end end
def can_read_group_member?
!(@subject.private? && access_level == GroupMember::NO_ACCESS)
end
def resource_access_token_creation_allowed? def resource_access_token_creation_allowed?
resource_access_token_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed? resource_access_token_feature_available? && group.root_ancestor.namespace_settings.resource_access_token_creation_allowed?
end end
......
...@@ -93,6 +93,9 @@ For more information, view the [permissions table](../../permissions.md#group-me ...@@ -93,6 +93,9 @@ For more information, view the [permissions table](../../permissions.md#group-me
## Subgroup membership ## Subgroup membership
NOTE:
There is a bug that causes some pages in the parent group to be accessible by subgroup members. For more details, see [this issue](https://gitlab.com/gitlab-org/gitlab/-/issues/340421).
When you add a member to a group, that member is also added to all subgroups. The user's permissions are inherited from When you add a member to a group, that member is also added to all subgroups. The user's permissions are inherited from
the group's parent. the group's parent.
......
...@@ -433,6 +433,13 @@ module EE ...@@ -433,6 +433,13 @@ module EE
super super
end end
override :can_read_group_member?
def can_read_group_member?
return true if user&.can_read_all_resources?
super
end
def ldap_lock_bypassable? def ldap_lock_bypassable?
return false unless ::Feature.enabled?(:ldap_settings_unlock_groups_by_owners) return false unless ::Feature.enabled?(:ldap_settings_unlock_groups_by_owners)
return false unless ::Gitlab::CurrentSettings.allow_group_owners_to_manage_ldap? return false unless ::Gitlab::CurrentSettings.allow_group_owners_to_manage_ldap?
......
...@@ -15,6 +15,10 @@ module API ...@@ -15,6 +15,10 @@ module API
public_send("find_#{source_type}!", id) # rubocop:disable GitlabSecurity/PublicSend public_send("find_#{source_type}!", id) # rubocop:disable GitlabSecurity/PublicSend
end end
def authorize_read_source_member!(source_type, source)
authorize! :"read_#{source_type}_member", source
end
def authorize_admin_source!(source_type, source) def authorize_admin_source!(source_type, source)
authorize! :"admin_#{source_type}", source authorize! :"admin_#{source_type}", source
end end
......
...@@ -30,6 +30,8 @@ module API ...@@ -30,6 +30,8 @@ module API
get ":id/members" do get ":id/members" do
source = find_source(source_type, params[:id]) source = find_source(source_type, params[:id])
authorize_read_source_member!(source_type, source)
members = paginate(retrieve_members(source, params: params)) members = paginate(retrieve_members(source, params: params))
present_members members present_members members
...@@ -49,6 +51,8 @@ module API ...@@ -49,6 +51,8 @@ module API
get ":id/members/all" do get ":id/members/all" do
source = find_source(source_type, params[:id]) source = find_source(source_type, params[:id])
authorize_read_source_member!(source_type, source)
members = paginate(retrieve_members(source, params: params, deep: true)) members = paginate(retrieve_members(source, params: params, deep: true))
present_members members present_members members
...@@ -64,6 +68,8 @@ module API ...@@ -64,6 +68,8 @@ module API
get ":id/members/:user_id" do get ":id/members/:user_id" do
source = find_source(source_type, params[:id]) source = find_source(source_type, params[:id])
authorize_read_source_member!(source_type, source)
members = source_members(source) members = source_members(source)
member = members.find_by!(user_id: params[:user_id]) member = members.find_by!(user_id: params[:user_id])
...@@ -81,6 +87,8 @@ module API ...@@ -81,6 +87,8 @@ module API
get ":id/members/all/:user_id" do get ":id/members/all/:user_id" do
source = find_source(source_type, params[:id]) source = find_source(source_type, params[:id])
authorize_read_source_member!(source_type, source)
members = find_all_members(source) members = find_all_members(source)
member = members.find_by!(user_id: params[:user_id]) member = members.find_by!(user_id: params[:user_id])
......
...@@ -97,7 +97,7 @@ RSpec.describe 'Private Group access' do ...@@ -97,7 +97,7 @@ RSpec.describe 'Private Group access' do
it { is_expected.to be_allowed_for(:developer).of(group) } it { is_expected.to be_allowed_for(:developer).of(group) }
it { is_expected.to be_allowed_for(:reporter).of(group) } it { is_expected.to be_allowed_for(:reporter).of(group) }
it { is_expected.to be_allowed_for(:guest).of(group) } it { is_expected.to be_allowed_for(:guest).of(group) }
it { is_expected.to be_allowed_for(project_guest) } it { is_expected.to be_denied_for(project_guest) }
it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:user) }
it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:external) }
it { is_expected.to be_denied_for(:visitor) } it { is_expected.to be_denied_for(:visitor) }
......
...@@ -184,6 +184,21 @@ RSpec.describe API::Members do ...@@ -184,6 +184,21 @@ RSpec.describe API::Members do
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.map { |u| u['id'] }).to match_array [maintainer.id, developer.id, nested_user.id] expect(json_response.map { |u| u['id'] }).to match_array [maintainer.id, developer.id, nested_user.id]
end end
context 'with a subgroup' do
let(:group) { create(:group, :private)}
let(:subgroup) { create(:group, :private, parent: group)}
let(:project) { create(:project, group: subgroup) }
before do
subgroup.add_developer(developer)
end
it 'subgroup member cannot get parent group members list' do
get api("/groups/#{group.id}/members/all", developer)
expect(response).to have_gitlab_http_status(:forbidden)
end
end
end end
shared_examples 'GET /:source_type/:id/members/(all/):user_id' do |source_type, all| shared_examples 'GET /:source_type/:id/members/(all/):user_id' do |source_type, all|
......
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