From d413f8e4e426e2cb2dc61d5a72d84a7dc67a28c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 15 Aug 2017 20:25:47 -0500 Subject: Add validation for visibility level of sub groups Sub groups should not have a visibility level higher than its parent. --- app/models/group.rb | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'app/models/group.rb') diff --git a/app/models/group.rb b/app/models/group.rb index 2816a68257c..15355418d05 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -26,6 +26,7 @@ class Group < Namespace validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } validate :visibility_level_allowed_by_projects + validate :visibility_level_allowed_by_parent validates :avatar, file_size: { maximum: 200.kilobytes.to_i } @@ -102,6 +103,14 @@ class Group < Namespace full_name end + def visibility_level_allowed_by_parent + return if parent_id.blank? + + if parent && (visibility_level > parent.visibility_level) + errors.add(:visibility_level, "#{visibility} is not allowed since the parent group has a #{parent.visibility} visibility.") + end + end + def visibility_level_allowed_by_projects allowed_by_projects = self.projects.where('visibility_level > ?', self.visibility_level).none? -- cgit v1.2.1 From b2b9d63f9b7301cbef9d1e1b8d4ad3cefeacf35d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 23 Aug 2017 11:51:11 -0500 Subject: Add validation to check visibility level of sub groups. --- app/models/group.rb | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) (limited to 'app/models/group.rb') diff --git a/app/models/group.rb b/app/models/group.rb index 15355418d05..fdd175341b3 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -26,6 +26,7 @@ class Group < Namespace validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } validate :visibility_level_allowed_by_projects + validate :visibility_level_allowed_by_sub_groups, if: :visibility_level_changed? validate :visibility_level_allowed_by_parent validates :avatar, file_size: { maximum: 200.kilobytes.to_i } @@ -112,14 +113,25 @@ class Group < Namespace end def visibility_level_allowed_by_projects - allowed_by_projects = self.projects.where('visibility_level > ?', self.visibility_level).none? + check_visibility_level_for(:projects) + end + + def visibility_level_allowed_by_sub_groups + check_visibility_level_for(:children) + end - unless allowed_by_projects + def check_visibility_level_for(children_type) + base_query = public_send(children_type) + children_have_higher_visibility = base_query.where('visibility_level > ?', visibility_level).exists? + + if children_have_higher_visibility + children_label = children_type == :projects ? 'projects' : 'sub groups' level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase - self.errors.add(:visibility_level, "#{level_name} is not allowed since there are projects with higher visibility.") + + self.errors.add(:visibility_level, "#{level_name} is not allowed since there are #{children_label} with higher visibility.") end - allowed_by_projects + children_have_higher_visibility end def avatar_url(**args) -- cgit v1.2.1 From af6968a15859a309cbb93a0327fc1d4be36041bc Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Thu, 24 Aug 2017 16:21:10 -0500 Subject: separate visibility_level_allowed logic from model validators --- app/models/group.rb | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) (limited to 'app/models/group.rb') diff --git a/app/models/group.rb b/app/models/group.rb index fdd175341b3..b093e0b200c 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -105,33 +105,35 @@ class Group < Namespace end def visibility_level_allowed_by_parent - return if parent_id.blank? + return if visibility_level_allowed_by_parent? - if parent && (visibility_level > parent.visibility_level) - errors.add(:visibility_level, "#{visibility} is not allowed since the parent group has a #{parent.visibility} visibility.") - end + errors.add(:visibility_level, "#{visibility} is not allowed since the parent group has a #{parent.visibility} visibility.") end def visibility_level_allowed_by_projects - check_visibility_level_for(:projects) + return if visibility_level_allowed_by_projects? + + errors.add(:visibility_level, "#{visibility} is not allowed since this group contains projects with higher visibility.") end def visibility_level_allowed_by_sub_groups - check_visibility_level_for(:children) + return if visibility_level_allowed_by_sub_groups? + + errors.add(:visibility_level, "#{visibility} is not allowed since there are sub-groups with higher visibility.") end - def check_visibility_level_for(children_type) - base_query = public_send(children_type) - children_have_higher_visibility = base_query.where('visibility_level > ?', visibility_level).exists? + def visibility_level_allowed_by_parent?(level = self.visibility_level) + return true unless parent_id.present? || parent - if children_have_higher_visibility - children_label = children_type == :projects ? 'projects' : 'sub groups' - level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase + level <= parent.visibility_level + end - self.errors.add(:visibility_level, "#{level_name} is not allowed since there are #{children_label} with higher visibility.") - end + def visibility_level_allowed_by_projects?(level = self.visibility_level) + projects.where('visibility_level > ?', level).none? + end - children_have_higher_visibility + def visibility_level_allowed_by_sub_groups?(level = self.visibility_level) + children.where('visibility_level > ?', level).none? end def avatar_url(**args) -- cgit v1.2.1 From 37edce19dd2f006ef2117ca8a9f05398e704a11c Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Thu, 24 Aug 2017 17:19:49 -0500 Subject: recognize instances where group visibility levels are unavailable --- app/models/group.rb | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'app/models/group.rb') diff --git a/app/models/group.rb b/app/models/group.rb index b093e0b200c..257a5075d2c 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -136,6 +136,12 @@ class Group < Namespace children.where('visibility_level > ?', level).none? end + def visibility_level_allowed?(level = self.visibility_level) + visibility_level_allowed_by_parent?(level) && + visibility_level_allowed_by_projects?(level) && + visibility_level_allowed_by_sub_groups?(level) + end + def avatar_url(**args) # We use avatar_path instead of overriding avatar_url because of carrierwave. # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864 -- cgit v1.2.1 From aff72ece729ab33afa8c3a4a83e97c5383ce5ed8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Fri, 25 Aug 2017 15:03:16 -0500 Subject: Fix broken spec. parent_id is being set to 0 by RSpec. --- app/models/group.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models/group.rb') diff --git a/app/models/group.rb b/app/models/group.rb index 257a5075d2c..78084f76869 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -123,7 +123,7 @@ class Group < Namespace end def visibility_level_allowed_by_parent?(level = self.visibility_level) - return true unless parent_id.present? || parent + return true unless parent_id && parent_id.nonzero? level <= parent.visibility_level end -- cgit v1.2.1 From b7b49c4a15e592904a41df72aa5ab9b791abc0ef Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Fri, 25 Aug 2017 15:54:05 -0500 Subject: prefer !x.exists? instead of x.none? to prevent unnecessary instantiation of records --- app/models/group.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app/models/group.rb') diff --git a/app/models/group.rb b/app/models/group.rb index 78084f76869..d4d201f05b7 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -129,11 +129,11 @@ class Group < Namespace end def visibility_level_allowed_by_projects?(level = self.visibility_level) - projects.where('visibility_level > ?', level).none? + !projects.where('visibility_level > ?', level).exists? end def visibility_level_allowed_by_sub_groups?(level = self.visibility_level) - children.where('visibility_level > ?', level).none? + !children.where('visibility_level > ?', level).exists? end def visibility_level_allowed?(level = self.visibility_level) -- cgit v1.2.1 From 6f03ddcdc3af1fbb840498a0e4765458079f0b0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Tue, 29 Aug 2017 00:49:01 -0500 Subject: Address some suggestions from first code review --- app/models/group.rb | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) (limited to 'app/models/group.rb') diff --git a/app/models/group.rb b/app/models/group.rb index d4d201f05b7..0e6d88f6a91 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -26,7 +26,7 @@ class Group < Namespace validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } validate :visibility_level_allowed_by_projects - validate :visibility_level_allowed_by_sub_groups, if: :visibility_level_changed? + validate :visibility_level_allowed_by_sub_groups validate :visibility_level_allowed_by_parent validates :avatar, file_size: { maximum: 200.kilobytes.to_i } @@ -104,24 +104,6 @@ class Group < Namespace full_name end - def visibility_level_allowed_by_parent - return if visibility_level_allowed_by_parent? - - errors.add(:visibility_level, "#{visibility} is not allowed since the parent group has a #{parent.visibility} visibility.") - end - - def visibility_level_allowed_by_projects - return if visibility_level_allowed_by_projects? - - errors.add(:visibility_level, "#{visibility} is not allowed since this group contains projects with higher visibility.") - end - - def visibility_level_allowed_by_sub_groups - return if visibility_level_allowed_by_sub_groups? - - errors.add(:visibility_level, "#{visibility} is not allowed since there are sub-groups with higher visibility.") - end - def visibility_level_allowed_by_parent?(level = self.visibility_level) return true unless parent_id && parent_id.nonzero? @@ -304,11 +286,29 @@ class Group < Namespace list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten end - protected + private def update_two_factor_requirement return unless require_two_factor_authentication_changed? || two_factor_grace_period_changed? users.find_each(&:update_two_factor_requirement) end + + def visibility_level_allowed_by_parent + return if visibility_level_allowed_by_parent? + + errors.add(:visibility_level, visibility_error_for(:parent, level: visibility, parent_level: parent.visibility)) + end + + def visibility_level_allowed_by_projects + return if visibility_level_allowed_by_projects? + + errors.add(:visibility_level, visibility_error_for(:projects, level: visibility)) + end + + def visibility_level_allowed_by_sub_groups + return if visibility_level_allowed_by_sub_groups? + + errors.add(:visibility_level, visibility_error_for(:sub_groups, level: visibility)) + end end -- cgit v1.2.1 From 6cad21efbe25ffe1c0a3a153a25ed9601b50c427 Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Wed, 30 Aug 2017 14:37:08 -0500 Subject: revert changes to visibility level helpers from 6f03ddc --- app/models/group.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'app/models/group.rb') diff --git a/app/models/group.rb b/app/models/group.rb index 0e6d88f6a91..3778b832a47 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -297,18 +297,18 @@ class Group < Namespace def visibility_level_allowed_by_parent return if visibility_level_allowed_by_parent? - errors.add(:visibility_level, visibility_error_for(:parent, level: visibility, parent_level: parent.visibility)) + errors.add(:visibility_level, "#{visibility} is not allowed since the parent group has a #{parent.visibility} visibility.") end def visibility_level_allowed_by_projects return if visibility_level_allowed_by_projects? - errors.add(:visibility_level, visibility_error_for(:projects, level: visibility)) + errors.add(:visibility_level, "#{visibility} is not allowed since this group contains projects with higher visibility.") end def visibility_level_allowed_by_sub_groups return if visibility_level_allowed_by_sub_groups? - errors.add(:visibility_level, visibility_error_for(:sub_groups, level: visibility)) + errors.add(:visibility_level, "#{visibility} is not allowed since there are sub-groups with higher visibility.") end end -- cgit v1.2.1