Commit b959ae55 authored by Zeger-Jan van de Weg's avatar Zeger-Jan van de Weg Committed by Felipe Artur

Improve group visibility level feature

parent 0a7f7161
...@@ -9,7 +9,7 @@ class Groups::ApplicationController < ApplicationController ...@@ -9,7 +9,7 @@ class Groups::ApplicationController < ApplicationController
end end
def authorize_read_group! 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? if current_user.nil?
return authenticate_user! return authenticate_user!
else else
......
...@@ -105,17 +105,6 @@ class GroupsController < Groups::ApplicationController ...@@ -105,17 +105,6 @@ class GroupsController < Groups::ApplicationController
@projects ||= ProjectsFinder.new.execute(current_user, group: group).sorted_by_activity @projects ||= ProjectsFinder.new.execute(current_user, group: group).sorted_by_activity
end 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! def authorize_create_group!
unless can?(current_user, :create_group, nil) unless can?(current_user, :create_group, nil)
return render_404 return render_404
......
...@@ -85,7 +85,6 @@ module VisibilityLevelHelper ...@@ -85,7 +85,6 @@ module VisibilityLevelHelper
end end
def skip_level?(form_model, level) def skip_level?(form_model, level)
form_model.is_a?(Project) && form_model.is_a?(Project) && !form_model.visibility_level_allowed?(level)
!form_model.visibility_level_allowed?(level)
end end
end end
...@@ -29,6 +29,8 @@ class Group < Namespace ...@@ -29,6 +29,8 @@ class Group < Namespace
has_many :shared_projects, through: :project_group_links, source: :project has_many :shared_projects, through: :project_group_links, source: :project
validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? } 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 } validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
mount_uploader :avatar, AvatarUploader mount_uploader :avatar, AvatarUploader
...@@ -80,6 +82,26 @@ class Group < Namespace ...@@ -80,6 +82,26 @@ class Group < Namespace
visibility_level visibility_level
end 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) def avatar_url(size = nil)
if avatar.present? if avatar.present?
[gitlab_config.url, avatar.url].join [gitlab_config.url, avatar.url].join
......
...@@ -73,7 +73,7 @@ class Project < ActiveRecord::Base ...@@ -73,7 +73,7 @@ class Project < ActiveRecord::Base
update_column(:last_activity_at, self.created_at) update_column(:last_activity_at, self.created_at)
end end
# update visibility_levet of forks # update visibility_level of forks
after_update :update_forks_visibility_level after_update :update_forks_visibility_level
def update_forks_visibility_level def update_forks_visibility_level
return unless visibility_level < visibility_level_was return unless visibility_level < visibility_level_was
...@@ -197,6 +197,7 @@ class Project < ActiveRecord::Base ...@@ -197,6 +197,7 @@ class Project < ActiveRecord::Base
validate :avatar_type, validate :avatar_type,
if: ->(project) { project.avatar.present? && project.avatar_changed? } if: ->(project) { project.avatar.present? && project.avatar_changed? }
validates :avatar, file_size: { maximum: 200.kilobytes.to_i } validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
validate :visibility_level_allowed_in_group
add_authentication_token_field :runners_token add_authentication_token_field :runners_token
before_save :ensure_runners_token before_save :ensure_runners_token
...@@ -446,6 +447,12 @@ class Project < ActiveRecord::Base ...@@ -446,6 +447,12 @@ class Project < ActiveRecord::Base
errors[:base] << ("Can't check your ability to create project") errors[:base] << ("Can't check your ability to create project")
end 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 def to_param
path path
end end
...@@ -961,9 +968,14 @@ class Project < ActiveRecord::Base ...@@ -961,9 +968,14 @@ class Project < ActiveRecord::Base
issues.opened.count issues.opened.count
end end
def visibility_level_allowed?(level) def visibility_level_allowed?(level = self.visibility_level)
allowed_by_forks = forked? ? Gitlab::VisibilityLevel.allowed_fork_levels(forked_from_project.visibility_level).include?(level.to_i) : true allowed_by_forks = if forked?
allowed_by_groups = group.present? ? level.to_i <= group.visibility_level : true 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 allowed_by_forks && allowed_by_groups
end end
......
...@@ -8,18 +8,13 @@ module Groups ...@@ -8,18 +8,13 @@ module Groups
private 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) 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) group.errors.add(:visibility_level, "#{level} has been restricted by your GitLab administrator.") unless allowed_by_user
projects_visibility = group.projects.pluck(:visibility_level)
allowed_by_projects = !projects_visibility.any? { |project_visibility| level.to_i < project_visibility } allowed_by_user
@group.errors.add(:visibility_level, "Cannot be changed. There are projects with higher visibility permissions.") unless allowed_by_projects
allowed_by_projects
end end
end end
end end
...@@ -2,14 +2,16 @@ module Groups ...@@ -2,14 +2,16 @@ module Groups
class CreateService < Groups::BaseService class CreateService < Groups::BaseService
def initialize(user, params = {}) def initialize(user, params = {})
@current_user, @params = user, params.dup @current_user, @params = user, params.dup
@group = Group.new(@params)
end end
def execute 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.name = @group.path.dup unless @group.name
@group.save @group.save
@group.add_owner(@current_user) @group.add_owner(current_user)
@group @group
end end
end end
......
#Checks visibility level permission check before updating a group # Checks visibility level permission check before updating a group
#Do not allow to put Group visibility level smaller than its projects # Do not allow to put Group visibility level smaller than its projects
#Do not allow unauthorized permission levels # Do not allow unauthorized permission levels
module Groups module Groups
class UpdateService < Groups::BaseService class UpdateService < Groups::BaseService
def execute def execute
return false unless visibility_level_allowed?(params[:visibility_level]) group.assign_attributes(params)
group.update_attributes(params)
end
private
def visibility_level_allowed?(level) return false unless visibility_allowed_for_user?
return true unless level.present?
visibility_allowed_for_project?(level) && visibility_allowed_for_user?(level) group.save
end end
end end
end end
...@@ -9,13 +9,8 @@ module Projects ...@@ -9,13 +9,8 @@ module Projects
@project = Project.new(params) @project = Project.new(params)
# Make sure that the user is allowed to use the specified visibility # Make sure that the user is allowed to use the specified visibility level
# level return @project unless visibility_level_allowed?
unless visibility_level_allowed?
deny_visibility_level(@project)
return @project
end
# Set project name from path # Set project name from path
if @project.name.present? && @project.path.present? if @project.name.present? && @project.path.present?
...@@ -55,9 +50,7 @@ module Projects ...@@ -55,9 +50,7 @@ module Projects
@project.save @project.save
if @project.persisted? && !@project.import? if @project.persisted? && !@project.import?
unless @project.create_repository raise 'Failed to create repository' unless @project.create_repository
raise 'Failed to create repository'
end
end end
end end
......
...@@ -17,8 +17,7 @@ ...@@ -17,8 +17,7 @@
.cover-title .cover-title
%h1 %h1
= @group.name = @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) = visibility_level_icon(@group.visibility_level, fw: false)
.cover-desc.username .cover-desc.username
......
...@@ -85,7 +85,7 @@ module API ...@@ -85,7 +85,7 @@ module API
end end
class Group < Grape::Entity class Group < Grape::Entity
expose :id, :name, :path, :description expose :id, :name, :path, :description, :visibility_level
expose :avatar_url expose :avatar_url
expose :web_url do |group, options| expose :web_url do |group, options|
......
...@@ -3,5 +3,17 @@ FactoryGirl.define do ...@@ -3,5 +3,17 @@ FactoryGirl.define do
sequence(:name) { |n| "group#{n}" } sequence(:name) { |n| "group#{n}" }
path { name.downcase.gsub(/\s/, '_') } path { name.downcase.gsub(/\s/, '_') }
type 'Group' 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
end end
...@@ -3,9 +3,9 @@ require 'spec_helper' ...@@ -3,9 +3,9 @@ require 'spec_helper'
describe GroupsFinder do describe GroupsFinder do
describe '#execute' do describe '#execute' do
let(:user) { create(:user) } let(:user) { create(:user) }
let!(:private_group) { create(:group, visibility_level: 0) } let!(:private_group) { create(:group, :private) }
let!(:internal_group) { create(:group, visibility_level: 10) } let!(:internal_group) { create(:group, :internal) }
let!(:public_group) { create(:group, visibility_level: 20) } let!(:public_group) { create(:group, :public) }
let(:finder) { described_class.new } let(:finder) { described_class.new }
describe 'execute' do describe 'execute' do
...@@ -23,7 +23,8 @@ describe GroupsFinder do ...@@ -23,7 +23,8 @@ describe GroupsFinder do
end end
context 'external user' do context 'external user' do
before { user.update_attribute(external: true) } let(:user) { create(:user, external: true) }
it { is_expected.to eq([public_group]) } it { is_expected.to eq([public_group]) }
end end
end end
......
...@@ -3,22 +3,15 @@ require 'spec_helper' ...@@ -3,22 +3,15 @@ require 'spec_helper'
describe PersonalProjectsFinder do describe PersonalProjectsFinder do
let(:source_user) { create(:user) } let(:source_user) { create(:user) }
let(:current_user) { create(:user) } let(:current_user) { create(:user) }
let(:finder) { described_class.new(source_user) } let(:finder) { described_class.new(source_user) }
let!(:public_project) { create(:project, :public, namespace: source_user.namespace) }
let!(:public_project) do
create(:project, :public, namespace: source_user.namespace, name: 'A',
path: 'A')
end
let!(:private_project) do let!(:private_project) do
create(:project, :private, namespace: source_user.namespace, name: 'B', create(:project, :private, namespace: source_user.namespace, path: 'mepmep')
path: 'B')
end end
let!(:internal_project) do let!(:internal_project) do
create(:project, :internal, namespace: source_user.namespace, name: 'c', create(:project, :internal, namespace: source_user.namespace, path: 'C')
path: 'C')
end end
before do before do
......
...@@ -67,12 +67,7 @@ describe VisibilityLevelHelper do ...@@ -67,12 +67,7 @@ describe VisibilityLevelHelper do
describe "skip_level?" do describe "skip_level?" do
describe "forks" do describe "forks" do
let(:project) { create(:project, :internal) } let(:project) { create(:project, :internal) }
let(:fork_project) { create(:forked_project_with_submodules) } let(:fork_project) { create(:project, forked_from_project: project) }
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
it "skips levels" do it "skips levels" do
expect(skip_level?(fork_project, Gitlab::VisibilityLevel::PUBLIC)).to be_truthy expect(skip_level?(fork_project, Gitlab::VisibilityLevel::PUBLIC)).to be_truthy
......
...@@ -57,18 +57,18 @@ describe Group, models: true do ...@@ -57,18 +57,18 @@ describe Group, models: true do
end end
describe 'scopes' do describe 'scopes' do
let!(:private_group) { create(:group, visibility_level: 0) } let!(:private_group) { create(:group, :private) }
let!(:internal_group) { create(:group, visibility_level: 10) } let!(:internal_group) { create(:group, :internal) }
let!(:public_group) { create(:group, visibility_level: 20) } let!(:public_group) { create(:group, :public) }
describe 'public_only' do describe 'public_only' do
subject { described_class.public_only } subject { described_class.public_only.to_a }
it{ is_expected.to eq([public_group]) } it{ is_expected.to eq([public_group]) }
end end
describe 'public_and_internal_only' do 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]) } it{ is_expected.to eq([public_group, internal_group]) }
end end
......
...@@ -571,12 +571,8 @@ describe Project, models: true do ...@@ -571,12 +571,8 @@ describe Project, models: true do
end end
context 'when checking on forked project' do context 'when checking on forked project' do
let(:forked_project) { create :forked_project_with_submodules } let(:project) { create(:project, :internal) }
let(:forked_project) { create(:project, forked_from_project: project) }
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
it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::PRIVATE)).to be_truthy } 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 } it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_truthy }
......
...@@ -2,9 +2,9 @@ require 'spec_helper' ...@@ -2,9 +2,9 @@ require 'spec_helper'
describe Groups::UpdateService, services: true do describe Groups::UpdateService, services: true do
let!(:user) { create(:user) } let!(:user) { create(:user) }
let!(:private_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } let!(:private_group) { create(:group, :private) }
let!(:internal_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } let!(:internal_group) { create(:group, :internal) }
let!(:public_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) } let!(:public_group) { create(:group, :public) }
describe "execute" do describe "execute" do
context "project visibility_level validation" do context "project visibility_level validation" do
...@@ -14,12 +14,12 @@ describe Groups::UpdateService, services: true do ...@@ -14,12 +14,12 @@ describe Groups::UpdateService, services: true do
before do before do
public_group.add_user(user, Gitlab::Access::MASTER) 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 end
it "cant downgrade permission level" do it "cant downgrade permission level" do
expect(service.execute).to be_falsy expect(service.execute).to be_falsy
expect(public_group.errors.count).to eq(1) expect(public_group.errors.count).to eq(2)
end end
end end
...@@ -28,12 +28,12 @@ describe Groups::UpdateService, services: true do ...@@ -28,12 +28,12 @@ describe Groups::UpdateService, services: true do
before do before do
internal_group.add_user(user, Gitlab::Access::MASTER) internal_group.add_user(user, Gitlab::Access::MASTER)
create(:project, :internal, group: internal_group, name: 'B', path: 'B') create(:project, :internal, group: internal_group)
end end
it "cant downgrade permission level" do it "cant downgrade permission level" do
expect(service.execute).to be_falsy expect(service.execute).to be_falsy
expect(internal_group.errors.count).to eq(1) expect(internal_group.errors.count).to eq(2)
end end
end end
end end
......
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