Code fixes

This commit is contained in:
Felipe Artur 2016-03-17 19:42:46 -03:00
parent a18ac62756
commit 0a7f716119
24 changed files with 178 additions and 125 deletions

View file

@ -115,6 +115,24 @@
color: #4c4e54;
font-size: 23px;
line-height: 1.1;
h1 {
color: #313236;
margin-bottom: 6px;
font-size: 23px;
}
.visibility-icon {
display: inline-block;
margin-left: 5px;
font-size: 18px;
color: $gray;
}
p {
padding: 0 $gl-padding;
color: #5c5d5e;
}
}
.cover-desc {

View file

@ -385,25 +385,3 @@ table {
margin-right: -$gl-padding;
border-top: 1px solid $border-color;
}
.cover-title{
h1 {
color: #313236;
margin: 0;
margin-bottom: 6px;
font-size: 23px;
font-weight: normal;
}
.visibility-icon {
display: inline-block;
margin-left: 5px;
font-size: 18px;
color: $gray;
}
p {
padding: 0 $gl-padding;
color: #5c5d5e;
}
}

View file

@ -28,9 +28,9 @@ class GroupsController < Groups::ApplicationController
end
def create
@group = Group.new(group_params)
@group = Groups::CreateService.new(current_user, group_params).execute
if Groups::CreateService.new(@group, current_user, group_params).execute
if @group.persisted?
redirect_to @group, notice: "Group '#{@group.name}' was successfully created."
else
render action: "new"

View file

@ -10,8 +10,9 @@ class ContributedProjectsFinder
# visible by this user.
#
# Returns an ActiveRecord::Relation.
def execute(current_user = nil)
if current_user && !current_user.external?
if current_user
relation = projects_visible_to_user(current_user)
else
relation = public_projects
@ -24,9 +25,7 @@ class ContributedProjectsFinder
def projects_visible_to_user(current_user)
authorized = @user.contributed_projects.visible_to_user(current_user)
union = Gitlab::SQL::Union.
new([authorized.select(:id), public_projects.select(:id)])
union = Gitlab::SQL::Union.new([authorized.select(:id), public_projects.select(:id)])
Project.where("projects.id IN (#{union.to_sql})")
end

View file

@ -14,9 +14,17 @@ class GroupsFinder
def all_groups(current_user)
if current_user
[current_user.authorized_groups, Group.unscoped.public_and_internal_only]
user_groups(current_user)
else
[Group.unscoped.public_only]
end
end
def user_groups(current_user)
if current_user.external?
[current_user.authorized_groups, Group.unscoped.public_only]
else
[current_user.authorized_groups, Group.unscoped.public_and_internal_only]
end
end
end

View file

@ -12,7 +12,7 @@ class JoinedGroupsFinder
#
# Returns an ActiveRecord::Relation.
def execute(current_user = nil)
if current_user && !current_user.external?
if current_user
relation = groups_visible_to_user(current_user)
else
relation = public_groups
@ -29,7 +29,7 @@ class JoinedGroupsFinder
# "@user" that "current_user" also has access to.
def groups_visible_to_user(current_user)
base = @user.authorized_groups.visible_to_user(current_user)
extra = public_and_internal_groups
extra = current_user.external? ? public_groups : public_and_internal_groups
union = Gitlab::SQL::Union.new([base.select(:id), extra.select(:id)])
Group.where("namespaces.id IN (#{union.to_sql})")

View file

@ -11,7 +11,7 @@ class PersonalProjectsFinder
#
# Returns an ActiveRecord::Relation.
def execute(current_user = nil)
if current_user && !current_user.external?
if current_user
relation = projects_visible_to_user(current_user)
else
relation = public_projects
@ -23,10 +23,7 @@ class PersonalProjectsFinder
private
def projects_visible_to_user(current_user)
authorized = @user.personal_projects.visible_to_user(current_user)
union = Gitlab::SQL::Union.
new([authorized.select(:id), public_and_internal_projects.select(:id)])
union = Gitlab::SQL::Union.new(projects_for_user_ids(current_user))
Project.where("projects.id IN (#{union.to_sql})")
end
@ -38,4 +35,14 @@ class PersonalProjectsFinder
def public_and_internal_projects
@user.personal_projects.public_and_internal_only
end
def projects_for_user_ids(current_user)
authorized = @user.personal_projects.visible_to_user(current_user)
if current_user.external?
[authorized.select(:id), public_projects.select(:id)]
else
[authorized.select(:id), public_and_internal_projects.select(:id)]
end
end
end

View file

@ -295,11 +295,8 @@ class Ability
end
def can_read_group?(user, group)
if user.external?
group.public? || ProjectsFinder.new.execute(user, group: group).any?
else
user.admin? || group.public? || group.internal? || group.users.include?(user) || ProjectsFinder.new.execute(user, group: group).any?
end
user.admin? || group.public? || (group.internal? && !user.external?) || group.users.include?(user) ||
ProjectsFinder.new.execute(user, group: group).any?
end
def namespace_abilities(user, namespace)

View file

@ -6,8 +6,20 @@ module Groups
@group, @current_user, @params = group, user, params.dup
end
def add_error_message(message)
group.errors.add(:visibility_level, message)
private
def visibility_allowed_for_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)
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
end
end
end

View file

@ -1,17 +1,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 false unless visibility_level_allowed?(params[:visibility_level])
return @group unless visibility_allowed_for_user?(@params[:visibility_level])
@group.name = @group.path.dup unless @group.name
@group.save(params) && @group.add_owner(current_user)
end
private
def visibility_level_allowed?(level)
allowed = Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
add_error_message("Visibility level restricted by admin.") unless allowed
allowed
@group.save
@group.add_owner(@current_user)
@group
end
end
end

View file

@ -14,24 +14,7 @@ module Groups
def visibility_level_allowed?(level)
return true unless level.present?
allowed_by_projects = visibility_by_project(level)
allowed_by_user = visibility_by_user(level)
allowed_by_projects && allowed_by_user
end
def visibility_by_project(level)
projects_visibility = group.projects.pluck(:visibility_level)
allowed_by_projects = !projects_visibility.any?{ |project_visibility| level.to_i < project_visibility }
add_error_message("Cannot be changed. There are projects with higher visibility permissions.") unless allowed_by_projects
allowed_by_projects
end
def visibility_by_user(level)
allowed_by_user = Gitlab::VisibilityLevel.allowed_for?(current_user, level)
add_error_message("You are not authorized to set this permission level.") unless allowed_by_user
allowed_by_user
visibility_allowed_for_project?(level) && visibility_allowed_for_user?(level)
end
end
end

View file

@ -18,7 +18,7 @@
%h1
= @group.name
%span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: "#{visibility_level_label(@group.visibility_level)} - #{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
@ -28,14 +28,6 @@
.cover-desc.description
= markdown(@group.description, pipeline: :description)
%ul.nav-links
%li.active
= link_to "#activity", 'data-toggle' => 'tab' do
Activity
%li
= link_to "#projects", 'data-toggle' => 'tab' do
Projects
- if can?(current_user, :read_group, @group)
%div{ class: container_class }
.top-area

View file

@ -90,10 +90,6 @@ production: &base
snippets: false
builds: true
## Default group features settings
default_groups_features:
visibility_level: 20
## Webhook settings
# Number of seconds to wait for HTTP response after sending webhook HTTP POST request (default: 10)
# webhook_timeout: 10

View file

@ -13,15 +13,9 @@ class AddDefaultGroupVisibilityToApplicationSettings < ActiveRecord::Migration
end
private
def allowed_visibility_level
default_visibility = Settings.gitlab.default_groups_features['visibility_level']
restricted_levels = current_application_settings.restricted_visibility_levels
return default_visibility unless restricted_levels.present?
if restricted_levels.include?(default_visibility)
Gitlab::VisibilityLevel.values.select{ |vis_level| vis_level unless restricted_levels.include?(vis_level) }.last
else
default_visibility
end
def allowed_visibility_level
allowed_levels = Gitlab::VisibilityLevel.values - current_application_settings.restricted_visibility_levels
allowed_levels.max
end
end

View file

@ -29,7 +29,6 @@ module Gitlab
session_expire_delay: Settings.gitlab['session_expire_delay'],
default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'],
default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'],
default_group_visibility: Settings.gitlab.default_groups_features['visibility_level'],
restricted_signup_domains: Settings.gitlab['restricted_signup_domains'],
import_sources: ['github','bitbucket','gitlab','gitorious','google_code','fogbugz','git'],
shared_runners_enabled: Settings.gitlab_ci['shared_runners_enabled'],

View file

@ -6,19 +6,18 @@
module Gitlab
module VisibilityLevel
extend CurrentSettings
extend ActiveSupport::Concern
included do
scope :public_only, -> { where(visibility_level: PUBLIC) }
scope :public_and_internal_only, -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
end
PRIVATE = 0 unless const_defined?(:PRIVATE)
INTERNAL = 10 unless const_defined?(:INTERNAL)
PUBLIC = 20 unless const_defined?(:PUBLIC)
class << self
def included(base)
base.class_eval do
scope :public_only, -> { where(visibility_level: PUBLIC) }
scope :public_and_internal_only, -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
end
end
def values
options.values
end

View file

@ -12,9 +12,12 @@ describe 'Internal group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to be_allowed_for :user }
it { is_expected.to_not be_allowed_for :visitor }
it { is_expected.to be_denied_for :visitor }
it { is_expected.to be_denied_for :external }
end
context "when user in group project" do
@ -31,9 +34,11 @@ describe 'Internal group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to be_allowed_for :user }
it { is_expected.to_not be_allowed_for :visitor }
it { is_expected.to be_denied_for :visitor }
it { is_expected.to be_denied_for :external }
end
context "when user in group project" do
@ -50,9 +55,11 @@ describe 'Internal group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to be_allowed_for :user }
it { is_expected.to_not be_allowed_for :visitor }
it { is_expected.to be_denied_for :visitor }
it { is_expected.to be_denied_for :external }
end
context "when user in group project" do
@ -70,9 +77,11 @@ describe 'Internal group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to be_allowed_for :user }
it { is_expected.to_not be_allowed_for :visitor }
it { is_expected.to be_denied_for :visitor }
it { is_expected.to be_denied_for :external }
end
context "when user in group project" do
@ -89,9 +98,11 @@ describe 'Internal group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to be_allowed_for :user }
it { is_expected.to_not be_allowed_for :visitor }
it { is_expected.to be_denied_for :visitor }
it { is_expected.to be_denied_for :external }
end
context "when user in group project" do

View file

@ -14,9 +14,11 @@ describe 'Private group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to_not be_allowed_for :user }
it { is_expected.to_not be_allowed_for :visitor }
it { is_expected.to be_denied_for :user }
it { is_expected.to be_denied_for :visitor }
it { is_expected.to be_denied_for :external }
end
context "when user in group project" do
@ -33,9 +35,11 @@ describe 'Private group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to_not be_allowed_for :user }
it { is_expected.to_not be_allowed_for :visitor }
it { is_expected.to be_denied_for :user }
it { is_expected.to be_denied_for :visitor }
it { is_expected.to be_denied_for :external }
end
context "when user in group project" do
@ -52,9 +56,11 @@ describe 'Private group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to_not be_allowed_for :user }
it { is_expected.to_not be_allowed_for :visitor }
it { is_expected.to be_denied_for :user }
it { is_expected.to be_denied_for :visitor }
it { is_expected.to be_denied_for :external }
end
context "when user in group project" do
@ -72,9 +78,11 @@ describe 'Private group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to_not be_allowed_for :user }
it { is_expected.to_not be_allowed_for :visitor }
it { is_expected.to be_denied_for :user }
it { is_expected.to be_denied_for :visitor }
it { is_expected.to be_denied_for :external }
end
context "when user in group project" do
@ -91,9 +99,11 @@ describe 'Private group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to_not be_allowed_for :user }
it { is_expected.to_not be_allowed_for :visitor }
it { is_expected.to be_denied_for :user }
it { is_expected.to be_denied_for :visitor }
it { is_expected.to be_denied_for :external }
end
context "when user in group project" do

View file

@ -14,9 +14,11 @@ describe 'Public group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to be_allowed_for :user }
it { is_expected.to be_allowed_for :visitor }
it { is_expected.to be_allowed_for :external }
end
context "when user in group project" do
@ -33,9 +35,11 @@ describe 'Public group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to be_allowed_for :user }
it { is_expected.to be_allowed_for :visitor }
it { is_expected.to be_allowed_for :external }
end
context "when user in group project" do
@ -52,9 +56,11 @@ describe 'Public group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to be_allowed_for :user }
it { is_expected.to be_allowed_for :visitor }
it { is_expected.to be_allowed_for :external }
end
context "when user in group project" do
@ -72,9 +78,11 @@ describe 'Public group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to be_allowed_for :user }
it { is_expected.to be_allowed_for :visitor }
it { is_expected.to be_allowed_for :external }
end
context "when user in group project" do
@ -91,9 +99,11 @@ describe 'Public group access', feature: true do
it { is_expected.to be_allowed_for group_member(:master) }
it { is_expected.to be_allowed_for group_member(:reporter) }
it { is_expected.to be_allowed_for group_member(:guest) }
it { is_expected.to be_allowed_for external_guest }
it { is_expected.to be_allowed_for :admin }
it { is_expected.to be_allowed_for :user }
it { is_expected.to be_allowed_for :visitor }
it { is_expected.to be_allowed_for :external }
end
context "when user in group project" do

View file

@ -18,8 +18,15 @@ describe GroupsFinder do
describe 'with a user' do
subject { finder.execute(user) }
context 'normal user' do
it { is_expected.to eq([public_group, internal_group]) }
end
context 'external user' do
before { user.update_attribute(external: true) }
it { is_expected.to eq([public_group]) }
end
end
end
end
end

View file

@ -46,6 +46,25 @@ describe JoinedGroupsFinder do
it { is_expected.to eq([public_group, private_group]) }
end
context 'external users' do
before do
profile_visitor.update_attributes(external: true)
public_group.add_user(profile_owner, Gitlab::Access::MASTER)
internal_group.add_user(profile_owner, Gitlab::Access::MASTER)
end
subject { finder.execute(profile_visitor) }
it "doest not show internal groups if not member" do
expect(subject).to eq([public_group])
end
it "shows internal groups if authorized" do
internal_group.add_user(profile_visitor, Gitlab::Access::MASTER)
expect(subject).to eq([public_group, internal_group])
end
end
end
end
end

View file

@ -16,6 +16,11 @@ describe PersonalProjectsFinder do
path: 'B')
end
let!(:internal_project) do
create(:project, :internal, namespace: source_user.namespace, name: 'c',
path: 'C')
end
before do
private_project.team << [current_user, Gitlab::Access::DEVELOPER]
end
@ -29,6 +34,14 @@ describe PersonalProjectsFinder do
describe 'with a current user' do
subject { finder.execute(current_user) }
context 'normal user' do
it { is_expected.to eq([internal_project, private_project, public_project]) }
end
context 'external' do
before { current_user.update_attributes(external: true) }
it { is_expected.to eq([private_project, public_project]) }
end
end
end

View file

@ -2,21 +2,19 @@ require 'spec_helper'
describe Groups::CreateService, 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!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } }
describe "execute" do
let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::PUBLIC ) }
let!(:service) { described_class.new(user, group_params ) }
subject { service.execute }
context "create groups without restricted visibility level" do
it { is_expected.to be_truthy }
it { is_expected.to be_persisted }
end
context "cannot create group with restricted visibility level" do
before { allow(current_application_settings).to receive(:restricted_visibility_levels).and_return([Gitlab::VisibilityLevel::PUBLIC]) }
it { is_expected.to be_falsy }
it { is_expected.to_not be_persisted }
end
end
end

View file

@ -14,4 +14,8 @@ module GroupAccessHelper
create(:user).tap { |user| grp.add_user(user, level) }
end
def external_guest(grp=group())
create(:user, external: true).tap { |user| grp.add_user(user, Gitlab::Access::GUEST) }
end
end