diff options
| -rw-r--r-- | app/controllers/groups/application_controller.rb | 2 | ||||
| -rw-r--r-- | app/controllers/groups_controller.rb | 11 | ||||
| -rw-r--r-- | app/helpers/visibility_level_helper.rb | 3 | ||||
| -rw-r--r-- | app/models/ability.rb | 2 | ||||
| -rw-r--r-- | app/models/group.rb | 22 | ||||
| -rw-r--r-- | app/models/project.rb | 20 | ||||
| -rw-r--r-- | app/services/groups/base_service.rb | 15 | ||||
| -rw-r--r-- | app/services/groups/create_service.rb | 8 | ||||
| -rw-r--r-- | app/services/groups/update_service.rb | 17 | ||||
| -rw-r--r-- | app/services/projects/create_service.rb | 13 | ||||
| -rw-r--r-- | app/views/groups/show.html.haml | 3 | ||||
| -rw-r--r-- | lib/api/entities.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/visibility_level.rb | 4 | ||||
| -rw-r--r-- | spec/factories/groups.rb | 12 | ||||
| -rw-r--r-- | spec/finders/groups_finder_spec.rb | 13 | ||||
| -rw-r--r-- | spec/finders/personal_projects_finder_spec.rb | 19 | ||||
| -rw-r--r-- | spec/helpers/visibility_level_helper_spec.rb | 9 | ||||
| -rw-r--r-- | spec/models/group_spec.rb | 10 | ||||
| -rw-r--r-- | spec/models/project_spec.rb | 8 | ||||
| -rw-r--r-- | spec/services/groups/create_service_spec.rb | 4 | ||||
| -rw-r--r-- | spec/services/groups/update_service_spec.rb | 36 |
21 files changed, 118 insertions, 115 deletions
diff --git a/app/controllers/groups/application_controller.rb b/app/controllers/groups/application_controller.rb index be801858eaf..795ce50fe92 100644 --- a/app/controllers/groups/application_controller.rb +++ b/app/controllers/groups/application_controller.rb @@ -9,7 +9,7 @@ class Groups::ApplicationController < ApplicationController end def authorize_read_group! - unless @group and can?(current_user, :read_group, @group) + unless @group && can?(current_user, :read_group, @group) if current_user.nil? return authenticate_user! else diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index ba2057eb2c8..b635fb150ae 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -105,17 +105,6 @@ class GroupsController < Groups::ApplicationController @projects ||= ProjectsFinder.new.execute(current_user, group: group).sorted_by_activity end - # Dont allow unauthorized access to group - def authorize_read_group! - unless can?(current_user, :read_group, @group) - if current_user.nil? - return authenticate_user! - else - return render_404 - end - end - end - def authorize_create_group! unless can?(current_user, :create_group, nil) return render_404 diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index 930cc883634..7fa18ba9079 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -85,7 +85,6 @@ module VisibilityLevelHelper end def skip_level?(form_model, level) - form_model.is_a?(Project) && - !form_model.visibility_level_allowed?(level) + form_model.is_a?(Project) && !form_model.visibility_level_allowed?(level) end end diff --git a/app/models/ability.rb b/app/models/ability.rb index ffcf05dcd33..61d5e7dc859 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -295,7 +295,7 @@ class Ability end def can_read_group?(user, group) - user.admin? || group.public? || (group.internal? && !user.external?) || group.users.include?(user) || + user.admin? || group.public? || (group.internal? && !user.external?) || group.users.include?(user) || ProjectsFinder.new.execute(user, group: group).any? end diff --git a/app/models/group.rb b/app/models/group.rb index b094a65e3d6..17c69af4d1b 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -29,6 +29,8 @@ class Group < Namespace has_many :shared_projects, through: :project_group_links, source: :project validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } + validate :visibility_level_allowed_by_projects + validates :avatar, file_size: { maximum: 200.kilobytes.to_i } mount_uploader :avatar, AvatarUploader @@ -80,6 +82,26 @@ class Group < Namespace visibility_level end + def visibility_level_allowed_by_projects + unless visibility_level_allowed? + 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.") + end + end + + def visibility_level_allowed? + projects_visibility = self.projects.pluck(:visibility_level) + + allowed_by_projects = projects_visibility.none? { |project_visibility| self.visibility_level < project_visibility } + + unless allowed_by_projects + 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.") + end + + allowed_by_projects + end + def avatar_url(size = nil) if avatar.present? [gitlab_config.url, avatar.url].join diff --git a/app/models/project.rb b/app/models/project.rb index 2828385a5f6..7c10ab35431 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -73,7 +73,7 @@ class Project < ActiveRecord::Base update_column(:last_activity_at, self.created_at) end - # update visibility_levet of forks + # update visibility_level of forks after_update :update_forks_visibility_level def update_forks_visibility_level return unless visibility_level < visibility_level_was @@ -197,6 +197,7 @@ class Project < ActiveRecord::Base validate :avatar_type, if: ->(project) { project.avatar.present? && project.avatar_changed? } validates :avatar, file_size: { maximum: 200.kilobytes.to_i } + validate :visibility_level_allowed_in_group add_authentication_token_field :runners_token before_save :ensure_runners_token @@ -446,6 +447,12 @@ class Project < ActiveRecord::Base errors[:base] << ("Can't check your ability to create project") end + def visibility_level_allowed_in_group + unless visibility_level_allowed? + self.errors.add(:visibility_level, "#{self.visibility_level} is not allowed in a #{self.group.visibility_level} group.") + end + end + def to_param path end @@ -961,9 +968,14 @@ class Project < ActiveRecord::Base issues.opened.count end - def visibility_level_allowed?(level) - allowed_by_forks = forked? ? Gitlab::VisibilityLevel.allowed_fork_levels(forked_from_project.visibility_level).include?(level.to_i) : true - allowed_by_groups = group.present? ? level.to_i <= group.visibility_level : true + def visibility_level_allowed?(level = self.visibility_level) + allowed_by_forks = if forked? + Gitlab::VisibilityLevel.allowed_fork_levels(forked_from_project.visibility_level).include?(level) + else + true + end + + allowed_by_groups = group.present? ? level <= group.visibility_level : true allowed_by_forks && allowed_by_groups end diff --git a/app/services/groups/base_service.rb b/app/services/groups/base_service.rb index 053b6a05281..1db81216084 100644 --- a/app/services/groups/base_service.rb +++ b/app/services/groups/base_service.rb @@ -8,18 +8,13 @@ module Groups private - def visibility_allowed_for_user?(level) + def visibility_allowed_for_user? + level = group.visibility_level allowed_by_user = Gitlab::VisibilityLevel.allowed_for?(current_user, level) - @group.errors.add(:visibility_level, "You are not authorized to set this permission level.") unless allowed_by_user - allowed_by_user - end - def visibility_allowed_for_project?(level) - projects_visibility = group.projects.pluck(:visibility_level) - - allowed_by_projects = !projects_visibility.any? { |project_visibility| level.to_i < project_visibility } - @group.errors.add(:visibility_level, "Cannot be changed. There are projects with higher visibility permissions.") unless allowed_by_projects - allowed_by_projects + group.errors.add(:visibility_level, "#{level} has been restricted by your GitLab administrator.") unless allowed_by_user + + allowed_by_user end end end diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index 38742369d82..f605ccca81b 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -2,14 +2,16 @@ module Groups class CreateService < Groups::BaseService def initialize(user, params = {}) @current_user, @params = user, params.dup - @group = Group.new(@params) end def execute - return @group unless visibility_allowed_for_user?(@params[:visibility_level]) + @group = Group.new(params) + + return @group unless visibility_allowed_for_user? + @group.name = @group.path.dup unless @group.name @group.save - @group.add_owner(@current_user) + @group.add_owner(current_user) @group end end diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index b910e0fde98..0b0c5a35d37 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -1,20 +1,15 @@ -#Checks visibility level permission check before updating a group -#Do not allow to put Group visibility level smaller than its projects -#Do not allow unauthorized permission levels +# Checks visibility level permission check before updating a group +# Do not allow to put Group visibility level smaller than its projects +# Do not allow unauthorized permission levels module Groups class UpdateService < Groups::BaseService def execute - return false unless visibility_level_allowed?(params[:visibility_level]) - group.update_attributes(params) - end - - private + group.assign_attributes(params) - def visibility_level_allowed?(level) - return true unless level.present? + return false unless visibility_allowed_for_user? - visibility_allowed_for_project?(level) && visibility_allowed_for_user?(level) + group.save end end end diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 4c121106bda..cebfc432002 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -9,13 +9,8 @@ module Projects @project = Project.new(params) - # Make sure that the user is allowed to use the specified visibility - # level - - unless visibility_level_allowed? - deny_visibility_level(@project) - return @project - end + # Make sure that the user is allowed to use the specified visibility level + return @project unless visibility_level_allowed? # Set project name from path if @project.name.present? && @project.path.present? @@ -55,9 +50,7 @@ module Projects @project.save if @project.persisted? && !@project.import? - unless @project.create_repository - raise 'Failed to create repository' - end + raise 'Failed to create repository' unless @project.create_repository end end diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 4be117667db..222c3e4a40e 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -17,8 +17,7 @@ .cover-title %h1 = @group.name - - %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: "#{group_visibility_description(@group)}"} + %span.visibility-icon.has_tooltip{ data: { container: 'body', placement: 'left' }, title: group_visibility_description(@group) } = visibility_level_icon(@group.visibility_level, fw: false) .cover-desc.username diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 20565e368dd..197e826e5bc 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -85,7 +85,7 @@ module API end class Group < Grape::Entity - expose :id, :name, :path, :description + expose :id, :name, :path, :description, :visibility_level expose :avatar_url expose :web_url do |group, options| diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb index f193fb282ab..58cd8df5219 100644 --- a/lib/gitlab/visibility_level.rb +++ b/lib/gitlab/visibility_level.rb @@ -9,8 +9,8 @@ module Gitlab extend ActiveSupport::Concern included do - scope :public_only, -> { where(visibility_level: PUBLIC) } - scope :public_and_internal_only, -> { where(visibility_level: [PUBLIC, INTERNAL] ) } + scope :public_only, -> { where(visibility_level: PUBLIC) } + scope :public_and_internal_only, -> { where(visibility_level: [PUBLIC, INTERNAL] ) } end PRIVATE = 0 unless const_defined?(:PRIVATE) diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb index 4a3a155d7ff..2d47a6f6c4c 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -3,5 +3,17 @@ FactoryGirl.define do sequence(:name) { |n| "group#{n}" } path { name.downcase.gsub(/\s/, '_') } type 'Group' + + trait :public do + visibility_level Gitlab::VisibilityLevel::PUBLIC + end + + trait :internal do + visibility_level Gitlab::VisibilityLevel::INTERNAL + end + + trait :private do + visibility_level Gitlab::VisibilityLevel::PRIVATE + end end end diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb index d0fd7af8cc3..d5d111e8d15 100644 --- a/spec/finders/groups_finder_spec.rb +++ b/spec/finders/groups_finder_spec.rb @@ -2,11 +2,11 @@ require 'spec_helper' describe GroupsFinder do describe '#execute' do - let(:user) { create(:user) } - let!(:private_group) { create(:group, visibility_level: 0) } - let!(:internal_group) { create(:group, visibility_level: 10) } - let!(:public_group) { create(:group, visibility_level: 20) } - let(:finder) { described_class.new } + let(:user) { create(:user) } + let!(:private_group) { create(:group, :private) } + let!(:internal_group) { create(:group, :internal) } + let!(:public_group) { create(:group, :public) } + let(:finder) { described_class.new } describe 'execute' do describe 'without a user' do @@ -23,7 +23,8 @@ describe GroupsFinder do end context 'external user' do - before { user.update_attribute(external: true) } + let(:user) { create(:user, external: true) } + it { is_expected.to eq([public_group]) } end end diff --git a/spec/finders/personal_projects_finder_spec.rb b/spec/finders/personal_projects_finder_spec.rb index 8758f61903c..a4681fe59d8 100644 --- a/spec/finders/personal_projects_finder_spec.rb +++ b/spec/finders/personal_projects_finder_spec.rb @@ -1,24 +1,17 @@ require 'spec_helper' describe PersonalProjectsFinder do - let(:source_user) { create(:user) } - let(:current_user) { create(:user) } - - let(:finder) { described_class.new(source_user) } - - let!(:public_project) do - create(:project, :public, namespace: source_user.namespace, name: 'A', - path: 'A') - end + let(:source_user) { create(:user) } + let(:current_user) { create(:user) } + let(:finder) { described_class.new(source_user) } + let!(:public_project) { create(:project, :public, namespace: source_user.namespace) } let!(:private_project) do - create(:project, :private, namespace: source_user.namespace, name: 'B', - path: 'B') + create(:project, :private, namespace: source_user.namespace, path: 'mepmep') end let!(:internal_project) do - create(:project, :internal, namespace: source_user.namespace, name: 'c', - path: 'C') + create(:project, :internal, namespace: source_user.namespace, path: 'C') end before do diff --git a/spec/helpers/visibility_level_helper_spec.rb b/spec/helpers/visibility_level_helper_spec.rb index ef60ef75fbe..ff98249570d 100644 --- a/spec/helpers/visibility_level_helper_spec.rb +++ b/spec/helpers/visibility_level_helper_spec.rb @@ -66,13 +66,8 @@ describe VisibilityLevelHelper do describe "skip_level?" do describe "forks" do - let(:project) { create(:project, :internal) } - let(:fork_project) { create(:forked_project_with_submodules) } - - before do - fork_project.build_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id) - fork_project.save - end + let(:project) { create(:project, :internal) } + let(:fork_project) { create(:project, forked_from_project: project) } it "skips levels" do expect(skip_level?(fork_project, Gitlab::VisibilityLevel::PUBLIC)).to be_truthy diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 135d298e10f..0591aa089d8 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -57,18 +57,18 @@ describe Group, models: true do end describe 'scopes' do - let!(:private_group) { create(:group, visibility_level: 0) } - let!(:internal_group) { create(:group, visibility_level: 10) } - let!(:public_group) { create(:group, visibility_level: 20) } + let!(:private_group) { create(:group, :private) } + let!(:internal_group) { create(:group, :internal) } + let!(:public_group) { create(:group, :public) } describe 'public_only' do - subject { described_class.public_only } + subject { described_class.public_only.to_a } it{ is_expected.to eq([public_group]) } end describe 'public_and_internal_only' do - subject { described_class.public_and_internal_only } + subject { described_class.public_and_internal_only.to_a } it{ is_expected.to eq([public_group, internal_group]) } end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 7fd3726c6ad..74383204250 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -571,12 +571,8 @@ describe Project, models: true do end context 'when checking on forked project' do - let(:forked_project) { create :forked_project_with_submodules } - - before do - forked_project.build_forked_project_link(forked_to_project_id: forked_project.id, forked_from_project_id: project.id) - forked_project.save - end + let(:project) { create(:project, :internal) } + let(:forked_project) { create(:project, forked_from_project: project) } it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::PRIVATE)).to be_truthy } it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_truthy } diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index b938a2f0c05..6aefb48a4e8 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' describe Groups::CreateService, services: true do - let!(:user) { create(:user) } - let!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } } + let!(:user) { create(:user) } + let!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } } describe "execute" do let!(:service) { described_class.new(user, group_params ) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index c759e32342d..9d427ff2d90 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -1,10 +1,10 @@ require 'spec_helper' describe Groups::UpdateService, services: true do - let!(:user) { create(:user) } - let!(:private_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } - let!(:internal_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } - let!(:public_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) } + let!(:user) { create(:user) } + let!(:private_group) { create(:group, :private) } + let!(:internal_group) { create(:group, :internal) } + let!(:public_group) { create(:group, :public) } describe "execute" do context "project visibility_level validation" do @@ -14,28 +14,28 @@ describe Groups::UpdateService, services: true do before do public_group.add_user(user, Gitlab::Access::MASTER) - create(:project, :public, group: public_group, name: 'B', path: 'B') + create(:project, :public, group: public_group) end it "cant downgrade permission level" do expect(service.execute).to be_falsy - expect(public_group.errors.count).to eq(1) + expect(public_group.errors.count).to eq(2) end end - context "internal group with internal project" do - let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE ) } - - before do - internal_group.add_user(user, Gitlab::Access::MASTER) - create(:project, :internal, group: internal_group, name: 'B', path: 'B') + context "internal group with internal project" do + let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE ) } + + before do + internal_group.add_user(user, Gitlab::Access::MASTER) + create(:project, :internal, group: internal_group) + end + + it "cant downgrade permission level" do + expect(service.execute).to be_falsy + expect(internal_group.errors.count).to eq(2) + end end - - it "cant downgrade permission level" do - expect(service.execute).to be_falsy - expect(internal_group.errors.count).to eq(1) - end - end end end |