Commit 31266c5b authored by Douwe Maan's avatar Douwe Maan

Address feedback

parent ae7b2ef6
class Projects::ApplicationController < ApplicationController class Projects::ApplicationController < ApplicationController
skip_before_action :authenticate_user! skip_before_action :authenticate_user!
before_action :project, :repository before_action :project
before_action :repository
layout 'project' layout 'project'
helper_method :repository, :can_collaborate_with_project? helper_method :repository, :can_collaborate_with_project?
......
...@@ -2,7 +2,7 @@ class Projects::UploadsController < Projects::ApplicationController ...@@ -2,7 +2,7 @@ class Projects::UploadsController < Projects::ApplicationController
skip_before_action :reject_blocked!, :project, skip_before_action :reject_blocked!, :project,
:repository, if: -> { action_name == 'show' && image? } :repository, if: -> { action_name == 'show' && image? }
before_action :authenticate_user!, only: [:create] before_action :authorize_upload_file!, only: [:create]
def create def create
link_to_file = ::Projects::UploadService.new(project, params[:file]). link_to_file = ::Projects::UploadService.new(project, params[:file]).
......
...@@ -171,15 +171,13 @@ class IssuableFinder ...@@ -171,15 +171,13 @@ class IssuableFinder
end end
def by_scope(items) def by_scope(items)
case params[:scope] || 'all' case params[:scope]
when 'created-by-me', 'authored' then when 'created-by-me', 'authored'
items.where(author_id: current_user.id) items.where(author_id: current_user.id)
when 'all' then when 'assigned-to-me'
items
when 'assigned-to-me' then
items.where(assignee_id: current_user.id) items.where(assignee_id: current_user.id)
else else
raise 'You must specify default scope' items
end end
end end
......
...@@ -5,11 +5,6 @@ class JoinedGroupsFinder < UnionFinder ...@@ -5,11 +5,6 @@ class JoinedGroupsFinder < UnionFinder
# Finds the groups of the source user, optionally limited to those visible to # Finds the groups of the source user, optionally limited to those visible to
# the current user. # the current user.
#
# current_user - If given the groups of "@user" will only include the groups
# "current_user" can also see.
#
# Returns an ActiveRecord::Relation.
def execute(current_user = nil) def execute(current_user = nil)
segments = all_groups(current_user) segments = all_groups(current_user)
......
...@@ -40,11 +40,11 @@ module VisibilityLevelHelper ...@@ -40,11 +40,11 @@ module VisibilityLevelHelper
def group_visibility_level_description(level) def group_visibility_level_description(level)
case level case level
when Gitlab::VisibilityLevel::PRIVATE when Gitlab::VisibilityLevel::PRIVATE
"The group can be accessed only by members." "The group and its projects can only be viewed by members."
when Gitlab::VisibilityLevel::INTERNAL when Gitlab::VisibilityLevel::INTERNAL
"The group can be accessed by any logged user." "The group and any internal projects can be viewed by any logged in user."
when Gitlab::VisibilityLevel::PUBLIC when Gitlab::VisibilityLevel::PUBLIC
"The group can be accessed without any authentication." "The group and any public projects can be viewed without any authentication."
end end
end end
...@@ -63,12 +63,21 @@ module VisibilityLevelHelper ...@@ -63,12 +63,21 @@ module VisibilityLevelHelper
end end
end end
def group_visibility_icon_description(group) def visibility_icon_description(form_model)
"#{visibility_level_label(group.visibility_level)} - #{group_visibility_level_description(group.visibility_level)}" case form_model
when Project
project_visibility_icon_description(form_model.visibility_level)
when Group
group_visibility_icon_description(form_model.visibility_level)
end
end
def group_visibility_icon_description(level)
"#{visibility_level_label(level)} - #{group_visibility_level_description(level)}"
end end
def project_visibility_icon_description(project) def project_visibility_icon_description(level)
"#{visibility_level_label(project.visibility_level)} - #{project_visibility_level_description(project.visibility_level)}" "#{visibility_level_label(level)} - #{project_visibility_level_description(level)}"
end end
def visibility_level_label(level) def visibility_level_label(level)
......
...@@ -170,7 +170,8 @@ class Ability ...@@ -170,7 +170,8 @@ class Ability
:read_note, :read_note,
:create_project, :create_project,
:create_issue, :create_issue,
:create_note :create_note,
:upload_file
] ]
end end
...@@ -298,7 +299,11 @@ class Ability ...@@ -298,7 +299,11 @@ class Ability
end end
def can_read_group?(user, group) def can_read_group?(user, group)
user.admin? || group.public? || (group.internal? && !user.external?) || group.users.include?(user) || return true if user.admin?
return true if group.public?
return true if group.internal? && !user.external?
return true if group.users.include?(user)
GroupProjectsFinder.new(group).execute(user).any? GroupProjectsFinder.new(group).execute(user).any?
end end
......
...@@ -7,7 +7,7 @@ ...@@ -7,7 +7,7 @@
# path :string(255) not null # path :string(255) not null
# owner_id :integer # owner_id :integer
# visibility_level :integer default(20), not null # visibility_level :integer default(20), not null
# created_at :key => "value", datetime # created_at :datetime
# updated_at :datetime # updated_at :datetime
# type :string(255) # type :string(255)
# description :string(255) default(""), not null # description :string(255) default(""), not null
...@@ -83,9 +83,7 @@ class Group < Namespace ...@@ -83,9 +83,7 @@ class Group < Namespace
end end
def visibility_level_allowed_by_projects def visibility_level_allowed_by_projects
projects_visibility = self.projects.pluck(:visibility_level) allowed_by_projects = self.projects.where('visibility_level > ?', self.visibility_level).none?
allowed_by_projects = projects_visibility.all? { |project_visibility| self.visibility_level >= project_visibility }
unless allowed_by_projects unless allowed_by_projects
level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase
......
...@@ -12,7 +12,7 @@ module Groups ...@@ -12,7 +12,7 @@ module Groups
return @group return @group
end end
@group.name = @group.path.dup unless @group.name @group.name ||= @group.path.dup
@group.save @group.save
@group.add_owner(current_user) @group.add_owner(current_user)
@group @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 unauthorized permission levels
module Groups module Groups
class UpdateService < Groups::BaseService class UpdateService < Groups::BaseService
def execute def execute
......
...@@ -17,7 +17,7 @@ ...@@ -17,7 +17,7 @@
.cover-title .cover-title
%h1 %h1
= @group.name = @group.name
%span.visibility-icon.has_tooltip{ data: { container: 'body' }, title: group_visibility_icon_description(@group) } %span.visibility-icon.has_tooltip{ data: { container: 'body' }, title: visibility_icon_description(@group) }
= visibility_level_icon(@group.visibility_level, fw: false) = visibility_level_icon(@group.visibility_level, fw: false)
.cover-desc.username .cover-desc.username
......
...@@ -5,7 +5,7 @@ ...@@ -5,7 +5,7 @@
.cover-title.project-home-desc .cover-title.project-home-desc
%h1 %h1
= @project.name = @project.name
%span.visibility-icon.has_tooltip{data: { container: 'body' }, title: project_visibility_icon_description(@project)} %span.visibility-icon.has_tooltip{data: { container: 'body' }, title: visibility_icon_description(@project)}
= visibility_level_icon(@project.visibility_level, fw: false) = visibility_level_icon(@project.visibility_level, fw: false)
- if @project.description.present? - if @project.description.present?
......
...@@ -21,7 +21,7 @@ ...@@ -21,7 +21,7 @@
= icon('users') = icon('users')
= number_with_delimiter(group.users.count) = number_with_delimiter(group.users.count)
%span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: group_visibility_icon_description(group)} %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: visibility_icon_description(group)}
= visibility_level_icon(group.visibility_level, fw: false) = visibility_level_icon(group.visibility_level, fw: false)
= image_tag group_icon(group), class: "avatar s40 hidden-xs" = image_tag group_icon(group), class: "avatar s40 hidden-xs"
......
...@@ -27,7 +27,7 @@ ...@@ -27,7 +27,7 @@
%span %span
= icon('star') = icon('star')
= project.star_count = project.star_count
%span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: project_visibility_icon_description(project)} %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: visibility_icon_description(project)}
= visibility_level_icon(project.visibility_level, fw: false) = visibility_level_icon(project.visibility_level, fw: false)
.title .title
......
class AddVisibilityLevelToGroups < ActiveRecord::Migration class AddVisibilityLevelToGroups < ActiveRecord::Migration
def change def up
#All groups public by default add_column :namespaces, :visibility_level, :integer, null: false, default: Gitlab::VisibilityLevel::PUBLIC
add_column :namespaces, :visibility_level, :integer, null: false, default: allowed_visibility_level add_index :namespaces, :visibility_level
# Unfortunately, this is needed on top of the `default`, since we don't want the configuration specific
# `allowed_visibility_level` to end up in schema.rb
if allowed_visibility_level < Gitlab::VisibilityLevel::PUBLIC
execute("UPDATE namespaces SET visibility_level = #{allowed_visibility_level}")
end
end
def down
remove_column :namespaces, :visibility_level
end end
private
def allowed_visibility_level def allowed_visibility_level
# TODO: Don't use `current_application_settings` return 20
allowed_levels = Gitlab::VisibilityLevel.values - current_application_settings.restricted_visibility_levels application_settings = select_one("SELECT restricted_visibility_levels FROM application_settings ORDER BY id DESC LIMIT 1")
if application_settings
restricted_visibility_levels = YAML.safe_load(application_settings["restricted_visibility_levels"]) rescue nil
end
restricted_visibility_levels ||= []
allowed_levels = Gitlab::VisibilityLevel.values - restricted_visibility_levels
allowed_levels.max allowed_levels.max
end end
end end
# Create visibility level field on DB
# Sets default_visibility_level to value on settings if not restricted
# If value is restricted takes higher visibility level allowed
class AddDefaultGroupVisibilityToApplicationSettings < ActiveRecord::Migration
def up
add_column :application_settings, :default_group_visibility, :integer
execute("UPDATE application_settings SET default_group_visibility = #{allowed_visibility_level}")
end
def down
remove_column :application_settings, :default_group_visibility
end
private
def allowed_visibility_level
application_settings = select_one("SELECT restricted_visibility_levels FROM application_settings ORDER BY id DESC LIMIT 1")
if application_settings
restricted_visibility_levels = YAML.safe_load(application_settings["restricted_visibility_levels"]) rescue nil
end
restricted_visibility_levels ||= []
allowed_levels = Gitlab::VisibilityLevel.values - restricted_visibility_levels
allowed_levels.max
end
end
...@@ -77,6 +77,7 @@ ActiveRecord::Schema.define(version: 20160320204112) do ...@@ -77,6 +77,7 @@ ActiveRecord::Schema.define(version: 20160320204112) do
t.boolean "akismet_enabled", default: false t.boolean "akismet_enabled", default: false
t.string "akismet_api_key" t.string "akismet_api_key"
t.boolean "email_author_in_body", default: false t.boolean "email_author_in_body", default: false
t.integer "default_group_visibility"
end end
create_table "audit_events", force: :cascade do |t| create_table "audit_events", force: :cascade do |t|
......
...@@ -15,11 +15,11 @@ describe 'Internal Group access', feature: true do ...@@ -15,11 +15,11 @@ describe 'Internal Group access', feature: true do
let(:project_guest) { create(:user) } let(:project_guest) { create(:user) }
before do before do
group.add_user(owner, Gitlab::Access::OWNER) group.add_owner(owner)
group.add_user(master, Gitlab::Access::MASTER) group.add_master(master)
group.add_user(developer, Gitlab::Access::DEVELOPER) group.add_developer(developer)
group.add_user(reporter, Gitlab::Access::REPORTER) group.add_reporter(reporter)
group.add_user(guest, Gitlab::Access::GUEST) group.add_guest(guest)
project.team << [project_guest, :guest] project.team << [project_guest, :guest]
end end
......
...@@ -15,11 +15,11 @@ describe 'Private Group access', feature: true do ...@@ -15,11 +15,11 @@ describe 'Private Group access', feature: true do
let(:project_guest) { create(:user) } let(:project_guest) { create(:user) }
before do before do
group.add_user(owner, Gitlab::Access::OWNER) group.add_owner(owner)
group.add_user(master, Gitlab::Access::MASTER) group.add_master(master)
group.add_user(developer, Gitlab::Access::DEVELOPER) group.add_developer(developer)
group.add_user(reporter, Gitlab::Access::REPORTER) group.add_reporter(reporter)
group.add_user(guest, Gitlab::Access::GUEST) group.add_guest(guest)
project.team << [project_guest, :guest] project.team << [project_guest, :guest]
end end
......
...@@ -15,11 +15,11 @@ describe 'Public Group access', feature: true do ...@@ -15,11 +15,11 @@ describe 'Public Group access', feature: true do
let(:project_guest) { create(:user) } let(:project_guest) { create(:user) }
before do before do
group.add_user(owner, Gitlab::Access::OWNER) group.add_owner(owner)
group.add_user(master, Gitlab::Access::MASTER) group.add_master(master)
group.add_user(developer, Gitlab::Access::DEVELOPER) group.add_developer(developer)
group.add_user(reporter, Gitlab::Access::REPORTER) group.add_reporter(reporter)
group.add_user(guest, Gitlab::Access::GUEST) group.add_guest(guest)
project.team << [project_guest, :guest] project.team << [project_guest, :guest]
end end
......
...@@ -5,64 +5,70 @@ describe JoinedGroupsFinder do ...@@ -5,64 +5,70 @@ describe JoinedGroupsFinder do
let!(:profile_owner) { create(:user) } let!(:profile_owner) { create(:user) }
let!(:profile_visitor) { create(:user) } let!(:profile_visitor) { create(:user) }
let!(:private_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } let!(:private_group) { create(:group, :private) }
let!(:private_group_2) { create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } let!(:private_group_2) { create(:group, :private) }
let!(:internal_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } let!(:internal_group) { create(:group, :internal) }
let!(:internal_group_2) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } let!(:internal_group_2) { create(:group, :internal) }
let!(:public_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) } let!(:public_group) { create(:group, :public) }
let!(:public_group_2) { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) } let!(:public_group_2) { create(:group, :public) }
let!(:finder) { described_class.new(profile_owner) } let!(:finder) { described_class.new(profile_owner) }
describe 'execute' do context 'without a user' do
context 'without a user only shows public groups from profile owner' do before do
before { public_group.add_user(profile_owner, Gitlab::Access::MASTER)} public_group.add_master(profile_owner)
subject { finder.execute }
it { is_expected.to eq([public_group]) }
end end
context 'only shows groups where both users are authorized to see' do it 'only shows public groups from profile owner' do
subject { finder.execute(profile_visitor) } expect(finder.execute).to eq([public_group])
end
end
context "with a user" do
before do before do
private_group.add_user(profile_owner, Gitlab::Access::MASTER) private_group.add_master(profile_owner)
private_group.add_user(profile_visitor, Gitlab::Access::DEVELOPER) private_group.add_developer(profile_visitor)
internal_group.add_user(profile_owner, Gitlab::Access::MASTER) internal_group.add_master(profile_owner)
public_group.add_user(profile_owner, Gitlab::Access::MASTER) public_group.add_master(profile_owner)
end end
it { is_expected.to eq([public_group, internal_group, private_group]) } it 'only shows groups where both users are authorized to see' do
expect(finder.execute(profile_visitor)).to eq([public_group, internal_group, private_group])
end end
context 'shows group if profile visitor is in one of its projects' do context 'if profile visitor is in one of its projects' do
before do before do
public_group.add_user(profile_owner, Gitlab::Access::MASTER) public_group.add_master(profile_owner)
private_group.add_user(profile_owner, Gitlab::Access::MASTER) private_group.add_master(profile_owner)
project = create(:project, :private, group: private_group, name: 'B', path: 'B') project = create(:project, :private, group: private_group, name: 'B', path: 'B')
project.team.add_user(profile_visitor, Gitlab::Access::DEVELOPER) project.team.add_developer(profile_visitor)
end end
subject { finder.execute(profile_visitor) } it 'shows group' do
expect(finder.execute(profile_visitor)).to eq([public_group, private_group])
it { is_expected.to eq([public_group, private_group]) } end
end end
context 'external users' do context 'external users' do
before do before do
profile_visitor.update_attributes(external: true) profile_visitor.update_attributes(external: true)
public_group.add_user(profile_owner, Gitlab::Access::MASTER) public_group.add_master(profile_owner)
internal_group.add_user(profile_owner, Gitlab::Access::MASTER) internal_group.add_master(profile_owner)
end end
subject { finder.execute(profile_visitor) } context 'if not a member' do
it "does not show internal groups" do
expect(finder.execute(profile_visitor)).to eq([public_group])
end
end
it "doest not show internal groups if not member" do context "if authorized" do
expect(subject).to eq([public_group]) before do
internal_group.add_master(profile_visitor)
end end
it "shows internal groups if authorized" do it "shows internal groups if authorized" do
internal_group.add_user(profile_visitor, Gitlab::Access::MASTER) expect(finder.execute(profile_visitor)).to eq([public_group, internal_group])
expect(subject).to eq([public_group, internal_group]) end
end end
end end
end end
......
...@@ -3,7 +3,7 @@ require 'spec_helper' ...@@ -3,7 +3,7 @@ require 'spec_helper'
describe ProjectsFinder do describe ProjectsFinder do
describe '#execute' do describe '#execute' do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) } let(:group) { create(:group, :public) }
let!(:private_project) do let!(:private_project) do
create(:project, :private, name: 'A', path: 'A') create(:project, :private, name: 'A', path: 'A')
......
...@@ -3,7 +3,7 @@ require 'spec_helper' ...@@ -3,7 +3,7 @@ require 'spec_helper'
describe SnippetsFinder do describe SnippetsFinder do
let(:user) { create :user } let(:user) { create :user }
let(:user1) { create :user } let(:user1) { create :user }
let(:group) { create :group, visibility_level: Gitlab::VisibilityLevel::PUBLIC } let(:group) { create :group, :public }
let(:project1) { create(:empty_project, :public, group: group) } let(:project1) { create(:empty_project, :public, group: group) }
let(:project2) { create(:empty_project, :private, group: group) } let(:project2) { create(:empty_project, :private, group: group) }
......
...@@ -67,9 +67,9 @@ describe Group, models: true do ...@@ -67,9 +67,9 @@ describe Group, models: true do
end end
describe 'public_and_internal_only' do describe 'public_and_internal_only' do
subject { described_class.public_and_internal_only.to_a.sort } subject { described_class.public_and_internal_only.to_a }
it{ is_expected.to eq([group, internal_group].sort) } it{ is_expected.to match_array([group, internal_group]) }
end end
end end
......
...@@ -721,8 +721,8 @@ describe Project, models: true do ...@@ -721,8 +721,8 @@ describe Project, models: true do
let(:private_group) { create(:group, visibility_level: 0) } let(:private_group) { create(:group, visibility_level: 0) }
let(:internal_group) { create(:group, visibility_level: 10) } let(:internal_group) { create(:group, visibility_level: 10) }
let(:private_project) { create :project, group: private_group, visibility_level: Gitlab::VisibilityLevel::PRIVATE } let(:private_project) { create :project, :private, group: private_group }
let(:internal_project) { create :project, group: internal_group, visibility_level: Gitlab::VisibilityLevel::INTERNAL } let(:internal_project) { create :project, :internal, group: internal_group }
context 'when group is private project can not be internal' do context 'when group is private project can not be internal' do
it { expect(private_project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_falsey } it { expect(private_project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_falsey }
......
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