Commit 248887b3 authored by Jan Provaznik's avatar Jan Provaznik

Merge branch '12873-support-restricting-group-access-by-multiple-ip-subnets' into 'master'

Resolve "Support restricting group access by multiple IP subnets"

See merge request gitlab-org/gitlab-ee!15142
parents b0a99370 7e4f50c1
......@@ -342,7 +342,7 @@ underlying projects, issues, etc, by IP address. This can help ensure that
particular content doesn't leave the premises, while not blocking off access to
the entire instance.
Add whitelisted IP subnet using CIDR notation to the group settings and anyone
Add one or more whitelisted IP subnets using CIDR notation in comma separated format to the group settings and anyone
coming from a different IP address won't be able to access the restricted
content.
......
......@@ -6,7 +6,6 @@ module EE
extend ::Gitlab::Utils::Override
prepended do
before_action :set_ip_restriction, only: [:edit]
before_action :set_allowed_domain, only: [:edit]
end
......@@ -33,7 +32,7 @@ module EE
params_ee << { insight_attributes: [:id, :project_id, :_destroy] } if current_group&.insights_available?
params_ee << :file_template_project_id if current_group&.feature_available?(:custom_file_templates_for_namespace)
params_ee << :custom_project_templates_group_id if current_group&.group_project_template_available?
params_ee << { ip_restriction_attributes: [:id, :range] } if current_group&.feature_available?(:group_ip_restriction)
params_ee << :ip_restriction_ranges if current_group&.feature_available?(:group_ip_restriction)
params_ee << { allowed_email_domain_attributes: [:id, :domain] } if current_group&.feature_available?(:group_allowed_email_domains)
end
end
......@@ -61,12 +60,6 @@ module EE
EE::User::DEFAULT_GROUP_VIEW
end
def set_ip_restriction
return if group.ip_restriction.present?
group.build_ip_restriction
end
def set_allowed_domain
return if group.allowed_email_domain.present?
......
......@@ -19,8 +19,7 @@ module EE
has_many :epics
has_one :saml_provider
has_one :ip_restriction
accepts_nested_attributes_for :ip_restriction, allow_destroy: true, reject_if: :all_blank
has_many :ip_restrictions, autosave: true
has_one :insight, foreign_key: :namespace_id
accepts_nested_attributes_for :insight, allow_destroy: true
has_one :scim_oauth_access_token
......@@ -113,6 +112,12 @@ module EE
end
end
def ip_restriction_ranges
return unless ip_restrictions.present?
ip_restrictions.map(&:range).join(",")
end
def human_ldap_access
::Gitlab::Access.options_with_owner.key(ldap_access)
end
......@@ -177,10 +182,10 @@ module EE
all_projects.where('EXISTS (?)', ::Vulnerabilities::Occurrence.select(1).where('vulnerability_occurrences.project_id = projects.id')).pluck_primary_key
end
def root_ancestor_ip_restriction
return ip_restriction if parent_id.nil?
def root_ancestor_ip_restrictions
return ip_restrictions if parent_id.nil?
root_ancestor.ip_restriction
root_ancestor.ip_restrictions
end
def root_ancestor_allowed_email_domain
......
......@@ -12,7 +12,7 @@ module EE
return false if group.errors.present?
end
handle_deletion
handle_changes
remove_insight_if_insight_project_absent
......@@ -79,23 +79,17 @@ module EE
end
end
def handle_deletion
handle_ip_restriction_deletion
def handle_changes
handle_allowed_domain_deletion
handle_ip_restriction_update
end
def handle_ip_restriction_deletion
return unless associations_editable?
return unless group.ip_restriction.present?
def handle_ip_restriction_update
comma_separated_ranges = params.delete(:ip_restriction_ranges)
ip_restriction_params = params[:ip_restriction_attributes]
return if comma_separated_ranges.nil?
return unless ip_restriction_params
if ip_restriction_params[:range]&.blank?
ip_restriction_params[:_destroy] = 1
end
IpRestrictions::UpdateService.new(group, comma_separated_ranges).execute
end
def associations_editable?
......
# frozen_string_literal: true
module EE
module IpRestrictions
class UpdateService
include ::Gitlab::Utils::StrongMemoize
# This class is responsible for updating the ip_restrictions of a specific group.
# It takes in comma separated subnets as input, eg: '192.168.1.0/8,10.0.0.0/8'
# For a group with existing ip_restriction records, this service:
# marks the records that exist for the group right now, but are not in `comma_separated_ranges` for deletion.
# builds new ip_restriction records that do not exist for the group right now, but exists in `comma_separated_ranges`
def initialize(group, comma_separated_ranges)
@group = group
@comma_separated_ranges = comma_separated_ranges
end
def execute
mark_deleted_ranges_for_destruction
build_new_ip_restriction_records
end
private
attr_reader :group, :comma_separated_ranges
def mark_deleted_ranges_for_destruction
return unless ranges_to_be_deleted.present?
group.ip_restrictions.each do |ip_restriction|
if ranges_to_be_deleted.include? ip_restriction.range
ip_restriction.mark_for_destruction
end
end
end
def build_new_ip_restriction_records
return unless ranges_to_be_created.present?
ranges_to_be_created.each do |range|
group.ip_restrictions.build(range: range)
end
end
def ranges_to_be_deleted
strong_memoize(:ranges_to_be_deleted) do
existing_ranges - current_ranges
end
end
def ranges_to_be_created
strong_memoize(:ranges_to_be_created) do
current_ranges - existing_ranges
end
end
def existing_ranges
group.ip_restrictions.map(&:range)
end
def current_ranges
comma_separated_ranges.split(",").map(&:strip).uniq
end
end
end
end
......@@ -4,14 +4,13 @@
%h5= _('Restrict access by IP address')
= f.fields_for :ip_restriction do |ip_restriction_form|
.form-group
- if read_only
= ip_restriction_form.text_field :range, value: group.root_ancestor_ip_restriction&.range, class: 'form-control', disabled: true, placeholder: _('No value set by top-level parent group.')
.form-text.text-muted
= _('IP address restriction is not editable in subgroups. Value inherited from top-level parent group.')
- else
= ip_restriction_form.text_field :range, class: 'form-control', placeholder: _('Enter IP address range')
.form-group
- if read_only
= f.text_field :ip_restriction_ranges, value: group.root_ancestor&.ip_restriction_ranges, class: 'form-control', disabled: true, placeholder: _('No value set by top-level parent group.')
.form-text.text-muted
- read_more_link = link_to(_('Read more'), help_page_path('user/group/index', anchor: 'ip-access-restriction-ultimate'))
= _('This group, including all subgroups, projects and git repositories, will only be reachable from the specified IP address range. Example: <code>192.168.0.0/24</code>. %{read_more_link}.').html_safe % { read_more_link: read_more_link }
= _('IP address restriction is not editable in subgroups. Value inherited from top-level parent group.')
- else
= f.text_field :ip_restriction_ranges, class: 'form-control', placeholder: _('Enter IP address range')
.form-text.text-muted
- read_more_link = link_to(_('Read more'), help_page_path('user/group/index', anchor: 'ip-access-restriction-ultimate'))
= _('This group, including all subgroups, projects and git repositories, will only be reachable from the specified IP address range. Multiple addresses are supported with comma delimiters. Example: <code>192.168.0.0/24,192.168.1.0/24</code>. %{read_more_link}.').html_safe % { read_more_link: read_more_link }
---
title: Support restricting group access by multiple IP subnets
merge_request: 15142
author:
type: added
......@@ -22,9 +22,11 @@ module Gitlab
attr_reader :group
def allows_address?(address)
return true unless group.root_ancestor_ip_restriction
root_ancestor_ip_restrictions = group.root_ancestor_ip_restrictions
group.root_ancestor_ip_restriction.allows_address?(address)
return true unless root_ancestor_ip_restrictions.present?
root_ancestor_ip_restrictions.any? { |ip_restriction| ip_restriction.allows_address?(address) }
end
end
end
......
......@@ -20,17 +20,19 @@ describe Gitlab::IpRestriction::Enforcer do
context 'with restriction' do
before do
create(:ip_restriction, group: group, range: range)
ranges.each do |range|
create(:ip_restriction, group: group, range: range)
end
end
context 'address is within the range' do
let(:range) { '192.168.0.0/24' }
context 'address is within one of the ranges' do
let(:ranges) { ['192.168.0.0/24', '255.255.255.224/27'] }
it { is_expected.to be_truthy }
end
context 'address is outside the range' do
let(:range) { '10.0.0.0/8' }
context 'address is outside all of the ranges' do
let(:ranges) { ['10.0.0.0/8', '255.255.255.224/27'] }
it { is_expected.to be_falsey }
end
......
......@@ -17,6 +17,7 @@ describe Group do
it { is_expected.to belong_to(:file_template_project).class_name('Project').without_validating_presence }
it { is_expected.to have_many(:dependency_proxy_blobs) }
it { is_expected.to have_many(:cycle_analytics_stages) }
it { is_expected.to have_many(:ip_restrictions) }
it { is_expected.to have_one(:dependency_proxy_setting) }
end
......@@ -223,6 +224,28 @@ describe Group do
end
end
describe '#ip_restriction_ranges' do
context 'group with no associated ip_restriction records' do
it 'returns nil' do
expect(group.ip_restriction_ranges).to eq(nil)
end
end
context 'group with associated ip_restriction records' do
let(:ranges) { ['192.168.0.0/24', '10.0.0.0/8'] }
before do
ranges.each do |range|
create(:ip_restriction, group: group, range: range)
end
end
it 'returns a comma separated string of ranges of its ip_restriction records' do
expect(group.ip_restriction_ranges).to eq('192.168.0.0/24,10.0.0.0/8')
end
end
end
describe '#checked_file_template_project' do
let(:valid_project) { create(:project, namespace: group) }
......
......@@ -20,7 +20,7 @@ describe GroupsController, type: :request do
context 'setting ip_restriction' do
let(:group) { create(:group) }
let(:params) { { group: { ip_restriction_attributes: { range: range } } } }
let(:params) { { group: { ip_restriction_ranges: range } } }
let(:range) { '192.168.0.0/24' }
before do
......@@ -32,11 +32,25 @@ describe GroupsController, type: :request do
context 'top-level group' do
context 'when ip_restriction does not exist' do
context 'valid param' do
it 'creates ip restriction' do
expect { subject }
.to(change { group.reload.ip_restriction&.range }
.from(nil).to('192.168.0.0/24'))
expect(response).to have_gitlab_http_status(302)
shared_examples 'creates ip restrictions' do
it 'creates ip restrictions' do
expect { subject }
.to(change { group.reload.ip_restrictions.map(&:range) }
.from([]).to(range.split(',')))
expect(response).to have_gitlab_http_status(302)
end
end
context 'single IP subnet' do
let(:range) { '192.168.0.0/24' }
it_behaves_like 'creates ip restrictions'
end
context 'multiple IP subnets' do
let(:range) { '192.168.0.0/24,192.168.0.1/8' }
it_behaves_like 'creates ip restrictions'
end
end
......@@ -45,36 +59,77 @@ describe GroupsController, type: :request do
it 'adds error message' do
expect { subject }
.not_to(change { group.reload.ip_restriction }.from(nil))
.not_to(change { group.reload.ip_restrictions.count }.from(0))
expect(response).to have_gitlab_http_status(200)
expect(response.body).to include('Ip restriction range is an invalid IP address range')
expect(response.body).to include('Ip restrictions range is an invalid IP address range')
end
end
end
context 'when ip_restriction already exists' do
let!(:ip_restriction) { IpRestriction.create!(group: group, range: '10.0.0.0/8') }
let(:params) { { group: { ip_restriction_attributes: { id: ip_restriction.id, range: range } } } }
let(:params) { { group: { ip_restriction_ranges: range } } }
context 'ip restriction param set' do
context 'valid param' do
it 'updates ip restriction' do
expect { subject }
.to(change { group.reload.ip_restriction.range }
.from('10.0.0.0/8').to('192.168.0.0/24'))
expect(response).to have_gitlab_http_status(302)
shared_examples 'updates ip restrictions' do
it 'updates ip restrictions' do
expect { subject }
.to(change { group.reload.ip_restrictions.map(&:range) }
.from(['10.0.0.0/8']).to(range.split(',')))
expect(response).to have_gitlab_http_status(302)
end
end
context 'single subnet' do
let(:range) { '192.168.0.0/24' }
it_behaves_like 'updates ip restrictions'
end
context 'multiple subnets' do
context 'a new subnet along with the existing one' do
let(:range) { '10.0.0.0/8,192.168.1.0/8' }
it_behaves_like 'updates ip restrictions'
end
context 'completely new range of subnets' do
let(:range) { '192.168.0.0/24,192.168.1.0/8' }
it_behaves_like 'updates ip restrictions'
end
end
end
context 'invalid param' do
let(:range) { 'boom!' }
shared_examples 'does not update existing ip restrictions' do
it 'does not change ip restriction records' do
expect { subject }
.not_to(change { group.reload.ip_restrictions.map(&:range) }
.from(['10.0.0.0/8']))
end
it 'adds error message' do
subject
expect(response).to have_gitlab_http_status(200)
expect(response.body).to include('Ip restrictions range is an invalid IP address range')
end
end
it 'adds error message' do
expect { subject }
.not_to(change { group.reload.ip_restriction.range }
.from('10.0.0.0/8'))
expect(response).to have_gitlab_http_status(200)
expect(response.body).to include('Ip restriction range is an invalid IP address range')
context 'not a valid subnet' do
let(:range) { 'boom!' }
it_behaves_like 'does not update existing ip restrictions'
end
context 'multiple IP subnets' do
context 'any one of them being not a valid' do
let(:range) { '192.168.0.0/24,boom!' }
it_behaves_like 'does not update existing ip restrictions'
end
end
end
end
......@@ -84,7 +139,7 @@ describe GroupsController, type: :request do
it 'deletes ip restriction' do
expect { subject }
.to(change { group.reload.ip_restriction }.to(nil))
.to(change { group.reload.ip_restrictions.count }.to(0))
expect(response).to have_gitlab_http_status(302)
end
end
......@@ -96,16 +151,16 @@ describe GroupsController, type: :request do
it 'does not create ip restriction' do
expect { subject }
.not_to change { group.reload.ip_restriction }.from(nil)
.not_to change { group.reload.ip_restrictions.count }.from(0)
expect(response).to have_gitlab_http_status(200)
expect(response.body).to include('Ip restriction base IP subnet restriction only allowed for top-level groups')
expect(response.body).to include('Ip restrictions base IP subnet restriction only allowed for top-level groups')
end
end
context 'with empty ip restriction param' do
let(:params) do
{ group: { two_factor_grace_period: 42,
ip_restriction_attributes: { range: "" } } }
ip_restriction_ranges: "" } }
end
it 'updates group setting' do
......@@ -126,7 +181,7 @@ describe GroupsController, type: :request do
it 'does not create ip restriction' do
expect { subject }
.not_to change { group.reload.ip_restriction }.from(nil)
.not_to change { group.reload.ip_restrictions.count }.from(0)
expect(response).to have_gitlab_http_status(302)
end
end
......
# frozen_string_literal: true
require 'spec_helper'
describe EE::IpRestrictions::UpdateService do
let(:group) { create(:group) }
subject { described_class.new(group, comma_separated_ranges).execute }
describe '#execute' do
context 'for a group that has no ip restriction' do
context 'with valid IP subnets' do
let(:comma_separated_ranges) { '192.168.0.0/24,10.0.0.0/8' }
it 'builds new ip_restriction records' do
subject
expect { group.save! }
.to(change { group.ip_restrictions.count }.from(0).to(2))
end
it 'builds new ip_restriction records with the provided ranges' do
expect { subject }
.to(change { group.ip_restrictions.map(&:range) }.from([]).to(comma_separated_ranges.split(",")))
end
end
end
context 'for a group that already has ip restriction' do
let(:ranges) { ['192.168.0.0/24', '10.0.0.0/8'] }
before do
ranges.each do |range|
create(:ip_restriction, group: group, range: range)
end
end
context 'with empty range' do
let(:comma_separated_ranges) { '' }
it 'marks all existing ip_restriction records for destruction' do
expect { subject }
.to(change { group.ip_restrictions.select(&:marked_for_destruction?).size }.from(0).to(2))
end
end
context 'with valid IP subnets' do
before do
subject
end
context 'with an entirely new set of ranges' do
shared_examples 'removes all existing ip_restriction records' do
it 'marks all the existing ip_restriction records for destruction' do
records_marked_for_destruction = group.ip_restrictions.select(&:marked_for_destruction?)
expect(records_marked_for_destruction.map(&:range)).to eq(ranges)
end
end
context 'each range in the list is unique' do
let(:comma_separated_ranges) { '255.255.0.0/16,255.255.128.0/17' }
it_behaves_like 'removes all existing ip_restriction records'
it 'builds new ip_restriction records with all of the specified ranges' do
newly_built_ip_restriction_records = group.ip_restrictions.select { |ip_restriction| ip_restriction.id.nil? }
expect(newly_built_ip_restriction_records.map(&:range)).to eq(comma_separated_ranges.split(","))
end
end
context 'ranges in the list repeats' do
let(:comma_separated_ranges) { '255.255.0.0/16,255.255.0.0/16,255.255.128.0/17' }
it_behaves_like 'removes all existing ip_restriction records'
it 'builds new ip_restriction records with only the unique ranges in the specified ranges' do
newly_built_ip_restriction_records = group.ip_restrictions.select { |ip_restriction| ip_restriction.id.nil? }
expect(newly_built_ip_restriction_records.map(&:range)).to eq(comma_separated_ranges.split(",").uniq)
end
end
end
context 'replacing one of the existing range with another' do
# replacing '10.0.0.0/8' with '255.255.128.0/17' and retaining '192.168.0.0/24'
let(:comma_separated_ranges) { '192.168.0.0/24,255.255.128.0/17' }
it 'marks the ip_restriction record of the replaced range for destruction' do
ip_restriction_record_of_replaced_range = group.ip_restrictions.find { |ip_restriction| ip_restriction.range == '10.0.0.0/8' }
expect(ip_restriction_record_of_replaced_range.marked_for_destruction?).to be_truthy
end
it 'retains the ip_restriction record of the other existing range' do
ip_restriction_record_of_other_existing_range = group.ip_restrictions.find { |ip_restriction| ip_restriction.range == '192.168.0.0/24' }
expect(ip_restriction_record_of_other_existing_range.marked_for_destruction?).to be_falsey
end
it 'builds a new ip_restriction record with the newly specified range' do
newly_built_ip_restriction_records = group.ip_restrictions.select { |ip_restriction| ip_restriction.id.nil? }
expect(newly_built_ip_restriction_records.size).to eq(1)
expect(newly_built_ip_restriction_records.map(&:range)).to include('255.255.128.0/17')
end
end
end
end
end
end
......@@ -167,14 +167,14 @@ describe Groups::UpdateService, '#execute' do
let!(:ip_restriction) { IpRestriction.create!(group: group, range: '10.0.0.0/8') }
context 'empty ip restriction param' do
let(:params) { { ip_restriction_attributes: { id: ip_restriction.id, range: '' } } }
let(:params) { { ip_restriction_ranges: '' } }
it 'deletes ip restriction' do
expect(group.ip_restriction.range).to eql('10.0.0.0/8')
expect(group.ip_restrictions.first.range).to eql('10.0.0.0/8')
subject
expect(group.reload.ip_restriction).to be_nil
expect(group.reload.ip_restrictions.count).to eq(0)
end
end
end
......
......@@ -19,35 +19,66 @@ describe 'groups/edit.html.haml' do
end
context 'top-level group' do
shared_examples 'renders ip_restriction setting' do
it 'renders ranges in comma separated format' do
render
expect(rendered).to render_template('groups/settings/_ip_restriction')
expect(rendered).to(have_field('group_ip_restriction_ranges',
{ disabled: false,
with: ranges.join(",") }))
end
end
before do
create(:ip_restriction, group: group, range: '192.168.0.0/24')
ranges.each do |range|
create(:ip_restriction, group: group, range: range)
end
end
it 'renders ip_restriction setting' do
render
context 'with single subnet' do
let(:ranges) { ['192.168.0.0/24'] }
expect(rendered).to render_template('groups/settings/_ip_restriction')
expect(rendered).to(have_field('group_ip_restriction_attributes_range',
{ disabled: false,
with: '192.168.0.0/24' }))
it_behaves_like 'renders ip_restriction setting'
end
context 'with multiple subnets' do
let(:ranges) { ['192.168.0.0/24', '192.168.1.0/8'] }
it_behaves_like 'renders ip_restriction setting'
end
end
context 'subgroup' do
shared_examples 'renders read-only ip_restriction setting of root ancestor' do
it 'renders disabled ranges of root ancestor in comma separated format' do
render
expect(rendered).to render_template('groups/settings/_ip_restriction')
expect(rendered).to(have_field('group_ip_restriction_ranges',
{ disabled: true,
with: ranges.join(",") }))
end
end
let(:group) { create(:group, :nested) }
before do
create(:ip_restriction, group: group.parent, range: '192.168.0.0/24')
group.build_ip_restriction
ranges.each do |range|
create(:ip_restriction, group: group.parent, range: range)
end
end
it 'show read-only ip_restriction setting of root ancestor' do
render
context 'with single subnet' do
let(:ranges) { ['192.168.0.0/24'] }
expect(rendered).to render_template('groups/settings/_ip_restriction')
expect(rendered).to(have_field('group_ip_restriction_attributes_range',
{ disabled: true,
with: '192.168.0.0/24' }))
it_behaves_like 'renders read-only ip_restriction setting of root ancestor'
end
context 'with multiple subnets' do
let(:ranges) { ['192.168.0.0/24', '192.168.1.0/8'] }
it_behaves_like 'renders read-only ip_restriction setting of root ancestor'
end
end
......
......@@ -15232,7 +15232,7 @@ msgstr ""
msgid "This group does not provide any group Runners yet."
msgstr ""
msgid "This group, including all subgroups, projects and git repositories, will only be reachable from the specified IP address range. Example: <code>192.168.0.0/24</code>. %{read_more_link}."
msgid "This group, including all subgroups, projects and git repositories, will only be reachable from the specified IP address range. Multiple addresses are supported with comma delimiters. Example: <code>192.168.0.0/24,192.168.1.0/24</code>. %{read_more_link}."
msgstr ""
msgid "This is a \"Ghost User\", created to hold all issues authored by users that have since been deleted. This user cannot be removed."
......
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