diff options
author | Douwe Maan <douwe@selenight.nl> | 2016-03-22 02:09:20 +0300 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2016-03-22 02:09:20 +0300 |
commit | 31266c5be4748f57a7d56bbcc6f06d570cbf5356 (patch) | |
tree | 1ee744a7303335cf1a270d92ec6b9e955a52cace | |
parent | ae7b2ef62cdf61c990f914d776a6fdfc2bc49fa2 (diff) |
Address feedback
24 files changed, 154 insertions, 100 deletions
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 9c8433c260b..657ee94cfd7 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -1,6 +1,7 @@ class Projects::ApplicationController < ApplicationController skip_before_action :authenticate_user! - before_action :project, :repository + before_action :project + before_action :repository layout 'project' helper_method :repository, :can_collaborate_with_project? diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index 94c51eeb94d..caed064dfbc 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -2,7 +2,7 @@ class Projects::UploadsController < Projects::ApplicationController skip_before_action :reject_blocked!, :project, :repository, if: -> { action_name == 'show' && image? } - before_action :authenticate_user!, only: [:create] + before_action :authorize_upload_file!, only: [:create] def create link_to_file = ::Projects::UploadService.new(project, params[:file]). diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index dd4208880b6..046286dd9e1 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -171,15 +171,13 @@ class IssuableFinder end def by_scope(items) - case params[:scope] || 'all' - when 'created-by-me', 'authored' then + case params[:scope] + when 'created-by-me', 'authored' items.where(author_id: current_user.id) - when 'all' then - items - when 'assigned-to-me' then + when 'assigned-to-me' items.where(assignee_id: current_user.id) else - raise 'You must specify default scope' + items end end diff --git a/app/finders/joined_groups_finder.rb b/app/finders/joined_groups_finder.rb index 2a3f0296d37..47174980258 100644 --- a/app/finders/joined_groups_finder.rb +++ b/app/finders/joined_groups_finder.rb @@ -5,11 +5,6 @@ class JoinedGroupsFinder < UnionFinder # Finds the groups of the source user, optionally limited to those visible to # 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) segments = all_groups(current_user) diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index 5b1bfb261a5..3a83ae15dd8 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -40,11 +40,11 @@ module VisibilityLevelHelper def group_visibility_level_description(level) case level 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 - "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 - "The group can be accessed without any authentication." + "The group and any public projects can be viewed without any authentication." end end @@ -63,12 +63,21 @@ module VisibilityLevelHelper end end - def group_visibility_icon_description(group) - "#{visibility_level_label(group.visibility_level)} - #{group_visibility_level_description(group.visibility_level)}" + def visibility_icon_description(form_model) + 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 - def project_visibility_icon_description(project) - "#{visibility_level_label(project.visibility_level)} - #{project_visibility_level_description(project.visibility_level)}" + def project_visibility_icon_description(level) + "#{visibility_level_label(level)} - #{project_visibility_level_description(level)}" end def visibility_level_label(level) diff --git a/app/models/ability.rb b/app/models/ability.rb index 42b978e04d5..fa2345f6faa 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -170,7 +170,8 @@ class Ability :read_note, :create_project, :create_issue, - :create_note + :create_note, + :upload_file ] end @@ -298,8 +299,12 @@ class Ability end def can_read_group?(user, group) - user.admin? || group.public? || (group.internal? && !user.external?) || group.users.include?(user) || - GroupProjectsFinder.new(group).execute(user).any? + 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? end def namespace_abilities(user, namespace) diff --git a/app/models/group.rb b/app/models/group.rb index 900fcd71ff3..b332601c59b 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -7,7 +7,7 @@ # path :string(255) not null # owner_id :integer # visibility_level :integer default(20), not null -# created_at :key => "value", datetime +# created_at :datetime # updated_at :datetime # type :string(255) # description :string(255) default(""), not null @@ -83,9 +83,7 @@ class Group < Namespace end def visibility_level_allowed_by_projects - projects_visibility = self.projects.pluck(:visibility_level) - - allowed_by_projects = projects_visibility.all? { |project_visibility| self.visibility_level >= project_visibility } + allowed_by_projects = self.projects.where('visibility_level > ?', self.visibility_level).none? unless allowed_by_projects level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index 46c2a53e1f6..2bccd584dde 100644 --- a/app/services/groups/create_service.rb +++ b/app/services/groups/create_service.rb @@ -12,7 +12,7 @@ module Groups return @group end - @group.name = @group.path.dup unless @group.name + @group.name ||= @group.path.dup @group.save @group.add_owner(current_user) @group diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index b70e2e4aaa9..99ad12b1003 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -1,7 +1,3 @@ -# 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 diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 5a9fa5d9a4d..820743dc8dd 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -17,7 +17,7 @@ .cover-title %h1 = @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) .cover-desc.username diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index d4bbafbd40f..514cbfa339d 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -5,7 +5,7 @@ .cover-title.project-home-desc %h1 = @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) - if @project.description.present? diff --git a/app/views/shared/groups/_group.html.haml b/app/views/shared/groups/_group.html.haml index db416b9d91a..66b7ef99650 100644 --- a/app/views/shared/groups/_group.html.haml +++ b/app/views/shared/groups/_group.html.haml @@ -21,7 +21,7 @@ = icon('users') = 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) = image_tag group_icon(group), class: "avatar s40 hidden-xs" diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index 3b987987676..803dd95bc65 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/app/views/shared/projects/_project.html.haml @@ -27,7 +27,7 @@ %span = icon('star') = 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) .title diff --git a/db/migrate/20160301124843_add_visibility_level_to_groups.rb b/db/migrate/20160301124843_add_visibility_level_to_groups.rb index 89b5ac19983..8121a0c6f90 100644 --- a/db/migrate/20160301124843_add_visibility_level_to_groups.rb +++ b/db/migrate/20160301124843_add_visibility_level_to_groups.rb @@ -1,12 +1,30 @@ class AddVisibilityLevelToGroups < ActiveRecord::Migration - def change - #All groups public by default - add_column :namespaces, :visibility_level, :integer, null: false, default: allowed_visibility_level + def up + add_column :namespaces, :visibility_level, :integer, null: false, default: Gitlab::VisibilityLevel::PUBLIC + 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 + private + def allowed_visibility_level - # TODO: Don't use `current_application_settings` - allowed_levels = Gitlab::VisibilityLevel.values - current_application_settings.restricted_visibility_levels + return 20 + 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 diff --git a/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb b/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb new file mode 100644 index 00000000000..37179926d42 --- /dev/null +++ b/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb @@ -0,0 +1,27 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index bb3f0497539..dce2bfe62ca 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -77,6 +77,7 @@ ActiveRecord::Schema.define(version: 20160320204112) do t.boolean "akismet_enabled", default: false t.string "akismet_api_key" t.boolean "email_author_in_body", default: false + t.integer "default_group_visibility" end create_table "audit_events", force: :cascade do |t| diff --git a/spec/features/security/group/internal_access_spec.rb b/spec/features/security/group/internal_access_spec.rb index d76eb454fe5..71b783b7276 100644 --- a/spec/features/security/group/internal_access_spec.rb +++ b/spec/features/security/group/internal_access_spec.rb @@ -15,11 +15,11 @@ describe 'Internal Group access', feature: true do let(:project_guest) { create(:user) } before do - group.add_user(owner, Gitlab::Access::OWNER) - group.add_user(master, Gitlab::Access::MASTER) - group.add_user(developer, Gitlab::Access::DEVELOPER) - group.add_user(reporter, Gitlab::Access::REPORTER) - group.add_user(guest, Gitlab::Access::GUEST) + group.add_owner(owner) + group.add_master(master) + group.add_developer(developer) + group.add_reporter(reporter) + group.add_guest(guest) project.team << [project_guest, :guest] end diff --git a/spec/features/security/group/private_access_spec.rb b/spec/features/security/group/private_access_spec.rb index 8ca4a0ac83b..cc9aee802f9 100644 --- a/spec/features/security/group/private_access_spec.rb +++ b/spec/features/security/group/private_access_spec.rb @@ -15,11 +15,11 @@ describe 'Private Group access', feature: true do let(:project_guest) { create(:user) } before do - group.add_user(owner, Gitlab::Access::OWNER) - group.add_user(master, Gitlab::Access::MASTER) - group.add_user(developer, Gitlab::Access::DEVELOPER) - group.add_user(reporter, Gitlab::Access::REPORTER) - group.add_user(guest, Gitlab::Access::GUEST) + group.add_owner(owner) + group.add_master(master) + group.add_developer(developer) + group.add_reporter(reporter) + group.add_guest(guest) project.team << [project_guest, :guest] end diff --git a/spec/features/security/group/public_access_spec.rb b/spec/features/security/group/public_access_spec.rb index f556fabb51e..db986683dbe 100644 --- a/spec/features/security/group/public_access_spec.rb +++ b/spec/features/security/group/public_access_spec.rb @@ -15,12 +15,12 @@ describe 'Public Group access', feature: true do let(:project_guest) { create(:user) } before do - group.add_user(owner, Gitlab::Access::OWNER) - group.add_user(master, Gitlab::Access::MASTER) - group.add_user(developer, Gitlab::Access::DEVELOPER) - group.add_user(reporter, Gitlab::Access::REPORTER) - group.add_user(guest, Gitlab::Access::GUEST) - + group.add_owner(owner) + group.add_master(master) + group.add_developer(developer) + group.add_reporter(reporter) + group.add_guest(guest) + project.team << [project_guest, :guest] end diff --git a/spec/finders/joined_groups_finder_spec.rb b/spec/finders/joined_groups_finder_spec.rb index 7b6fc837e5f..66a250f9dd1 100644 --- a/spec/finders/joined_groups_finder_spec.rb +++ b/spec/finders/joined_groups_finder_spec.rb @@ -5,64 +5,70 @@ describe JoinedGroupsFinder do let!(:profile_owner) { create(:user) } let!(:profile_visitor) { create(:user) } - let!(:private_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } - let!(:private_group_2) { create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } - let!(:internal_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } - let!(:internal_group_2) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } - let!(:public_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) } - let!(:public_group_2) { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) } + let!(:private_group) { create(:group, :private) } + let!(:private_group_2) { create(:group, :private) } + let!(:internal_group) { create(:group, :internal) } + let!(:internal_group_2) { create(:group, :internal) } + let!(:public_group) { create(:group, :public) } + let!(:public_group_2) { create(:group, :public) } let!(:finder) { described_class.new(profile_owner) } - describe 'execute' do - context 'without a user only shows public groups from profile owner' do - before { public_group.add_user(profile_owner, Gitlab::Access::MASTER)} - subject { finder.execute } - - it { is_expected.to eq([public_group]) } + context 'without a user' do + before do + public_group.add_master(profile_owner) end - context 'only shows groups where both users are authorized to see' do - subject { finder.execute(profile_visitor) } + it 'only shows public groups from profile owner' do + expect(finder.execute).to eq([public_group]) + end + end - before do - private_group.add_user(profile_owner, Gitlab::Access::MASTER) - private_group.add_user(profile_visitor, Gitlab::Access::DEVELOPER) - internal_group.add_user(profile_owner, Gitlab::Access::MASTER) - public_group.add_user(profile_owner, Gitlab::Access::MASTER) - end + context "with a user" do + before do + private_group.add_master(profile_owner) + private_group.add_developer(profile_visitor) + internal_group.add_master(profile_owner) + public_group.add_master(profile_owner) + 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 - 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 - public_group.add_user(profile_owner, Gitlab::Access::MASTER) - private_group.add_user(profile_owner, Gitlab::Access::MASTER) + public_group.add_master(profile_owner) + private_group.add_master(profile_owner) 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 - subject { finder.execute(profile_visitor) } - - it { is_expected.to eq([public_group, private_group]) } + it 'shows group' do + expect(finder.execute(profile_visitor)).to eq([public_group, private_group]) + end 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) + public_group.add_master(profile_owner) + internal_group.add_master(profile_owner) end - subject { finder.execute(profile_visitor) } - - it "doest not show internal groups if not member" do - expect(subject).to eq([public_group]) + 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 "shows internal groups if authorized" do - internal_group.add_user(profile_visitor, Gitlab::Access::MASTER) - expect(subject).to eq([public_group, internal_group]) + context "if authorized" do + before do + internal_group.add_master(profile_visitor) + end + + it "shows internal groups if authorized" do + expect(finder.execute(profile_visitor)).to eq([public_group, internal_group]) + end end end end diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index 194c9543772..0a1cc3b3df7 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe ProjectsFinder do describe '#execute' do let(:user) { create(:user) } - let(:group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) } + let(:group) { create(:group, :public) } let!(:private_project) do create(:project, :private, name: 'A', path: 'A') diff --git a/spec/finders/snippets_finder_spec.rb b/spec/finders/snippets_finder_spec.rb index b8940483dfb..810016c9658 100644 --- a/spec/finders/snippets_finder_spec.rb +++ b/spec/finders/snippets_finder_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe SnippetsFinder do let(:user) { 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(:project2) { create(:empty_project, :private, group: group) } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 68e213f4816..7bfca1e72c3 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -67,9 +67,9 @@ describe Group, models: true do end 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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 757324184bd..20f06f4b7e1 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -721,8 +721,8 @@ describe Project, models: true do let(:private_group) { create(:group, visibility_level: 0) } let(:internal_group) { create(:group, visibility_level: 10) } - let(:private_project) { create :project, group: private_group, visibility_level: Gitlab::VisibilityLevel::PRIVATE } - let(:internal_project) { create :project, group: internal_group, visibility_level: Gitlab::VisibilityLevel::INTERNAL } + let(:private_project) { create :project, :private, group: private_group } + let(:internal_project) { create :project, :internal, group: internal_group } context 'when group is private project can not be internal' do it { expect(private_project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_falsey } |