From f2a9ee258e0ee3a6fe0cb614e4b73c56dcd7339d Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Tue, 1 Mar 2016 12:22:29 -0300 Subject: [PATCH] Add permission level to groups --- app/controllers/explore/groups_controller.rb | 2 +- app/controllers/groups_controller.rb | 2 +- app/finders/groups_finder.rb | 31 +++++++++++++++++++ app/helpers/groups_helper.rb | 4 +++ app/helpers/visibility_level_helper.rb | 13 ++++++++ app/models/ability.rb | 5 +-- app/models/group.rb | 25 +++++++++------ app/views/groups/edit.html.haml | 2 ++ app/views/groups/new.html.haml | 2 ++ app/views/shared/_group_tips.html.haml | 1 - ...01124843_add_visibility_level_to_groups.rb | 9 ++++++ db/schema.rb | 10 +++--- spec/finders/groups_finder_spec.rb | 25 +++++++++++++++ spec/helpers/groups_helper.rb | 15 +++++++++ spec/helpers/visibility_level_helper_spec.rb | 8 +++++ spec/models/group_spec.rb | 18 +++++++++++ 16 files changed, 153 insertions(+), 19 deletions(-) create mode 100644 app/finders/groups_finder.rb create mode 100644 db/migrate/20160301124843_add_visibility_level_to_groups.rb create mode 100644 spec/finders/groups_finder_spec.rb diff --git a/app/controllers/explore/groups_controller.rb b/app/controllers/explore/groups_controller.rb index a9bf4321f73..9575a87ee41 100644 --- a/app/controllers/explore/groups_controller.rb +++ b/app/controllers/explore/groups_controller.rb @@ -1,6 +1,6 @@ class Explore::GroupsController < Explore::ApplicationController def index - @groups = Group.order_id_desc + @groups = GroupsFinder.new.execute(current_user) @groups = @groups.search(params[:search]) if params[:search].present? @groups = @groups.sort(@sort = params[:sort]) @groups = @groups.page(params[:page]).per(PER_PAGE) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index f05c29e9974..13de19bc141 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -131,7 +131,7 @@ class GroupsController < Groups::ApplicationController end def group_params - params.require(:group).permit(:name, :description, :path, :avatar, :public) + params.require(:group).permit(:name, :description, :path, :avatar, :public, :visibility_level) end def load_events diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb new file mode 100644 index 00000000000..a3a8cd541de --- /dev/null +++ b/app/finders/groups_finder.rb @@ -0,0 +1,31 @@ +class GroupsFinder + def execute(current_user = nil) + + segments = all_groups(current_user) + + if segments.length > 1 + union = Gitlab::SQL::Union.new(segments.map { |s| s.select(:id) }) + Group.where("namespaces.id IN (#{union.to_sql})").order_id_desc + else + segments.first + end + end + + private + + def all_groups(current_user) + if current_user + [current_user.authorized_groups, public_and_internal_groups] + else + [Group.public_only] + end + end + + def public_groups + Group.unscoped.public_only + end + + def public_and_internal_groups + Group.unscoped.public_and_internal_only + end +end diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index 1d36969cd62..b1f0a765bb9 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -19,6 +19,10 @@ module GroupsHelper end end + def can_change_group_visibility_level?(group) + can?(current_user, :change_visibility_level, group) + end + def group_icon(group) if group.is_a?(String) group = Group.find_by(path: group) diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index 71d33b445c2..c47342534a8 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -19,6 +19,8 @@ module VisibilityLevelHelper case form_model when Project project_visibility_level_description(level) + when Group + group_visibility_level_description(level) when Snippet snippet_visibility_level_description(level, form_model) end @@ -35,6 +37,17 @@ module VisibilityLevelHelper end end + def group_visibility_level_description(level) + case level + when Gitlab::VisibilityLevel::PRIVATE + "The group can be accessed only by members." + when Gitlab::VisibilityLevel::INTERNAL + "The group can be accessed by any logged user." + when Gitlab::VisibilityLevel::PUBLIC + "The group can be accessed without any authentication." + end + end + def snippet_visibility_level_description(level, snippet = nil) case level when Gitlab::VisibilityLevel::PRIVATE diff --git a/app/models/ability.rb b/app/models/ability.rb index fe9e0aab717..c84ded61606 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -275,11 +275,12 @@ class Ability rules << :read_group end - # Only group masters and group owners can create new projects in group + # Only group masters and group owners can create new projects and change permission level if group.has_master?(user) || group.has_owner?(user) || user.admin? rules += [ :create_projects, - :admin_milestones + :admin_milestones, + :change_visibility_level ] end diff --git a/app/models/group.rb b/app/models/group.rb index 76042b3e3fd..26914f55541 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -2,15 +2,16 @@ # # Table name: namespaces # -# id :integer not null, primary key -# name :string(255) not null -# path :string(255) not null -# owner_id :integer -# created_at :datetime -# updated_at :datetime -# type :string(255) -# description :string(255) default(""), not null -# avatar :string(255) +# id :integer not null, primary key +# name :string(255) not null +# path :string(255) not null +# owner_id :integer +# visibility_level :integer default(20), not null +# created_at :key => "value", datetime +# updated_at :datetime +# type :string(255) +# description :string(255) default(""), not null +# avatar :string(255) # require 'carrierwave/orm/activerecord' @@ -18,8 +19,10 @@ require 'file_size_validator' class Group < Namespace include Gitlab::ConfigHelper + include Gitlab::VisibilityLevel include Referable + has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember' alias_method :members, :group_members has_many :users, through: :group_members @@ -32,6 +35,10 @@ class Group < Namespace after_create :post_create_hook after_destroy :post_destroy_hook + scope :public_only, -> { where(visibility_level: Group::PUBLIC) } + scope :public_and_internal_only, -> { where(visibility_level: [Group::PUBLIC, Group::INTERNAL] ) } + + class << self def search(query) where("LOWER(namespaces.name) LIKE :query or LOWER(namespaces.path) LIKE :query", query: "%#{query.downcase}%") diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml index 3430f56a9c9..ea223d2209f 100644 --- a/app/views/groups/edit.html.haml +++ b/app/views/groups/edit.html.haml @@ -23,6 +23,8 @@ %hr = link_to 'Remove avatar', group_avatar_path(@group.to_param), data: { confirm: "Group avatar will be removed. Are you sure?"}, method: :delete, class: "btn btn-remove btn-sm remove-avatar" + = render 'shared/visibility_level', f: f, visibility_level: @group.visibility_level, can_change_visibility_level: can_change_group_visibility_level?(@group), form_model: @group + .form-actions = f.submit 'Save group', class: "btn btn-save" diff --git a/app/views/groups/new.html.haml b/app/views/groups/new.html.haml index 4bc31cabea6..1526ca42634 100644 --- a/app/views/groups/new.html.haml +++ b/app/views/groups/new.html.haml @@ -17,6 +17,8 @@ .col-sm-10 = render 'shared/choose_group_avatar_button', f: f + = render 'shared/visibility_level', f: f, visibility_level: @group.visibility_level, can_change_visibility_level: true, form_model: @group + .form-group .col-sm-offset-2.col-sm-10 = render 'shared/group_tips' diff --git a/app/views/shared/_group_tips.html.haml b/app/views/shared/_group_tips.html.haml index e5cf783beb7..46e4340511a 100644 --- a/app/views/shared/_group_tips.html.haml +++ b/app/views/shared/_group_tips.html.haml @@ -1,6 +1,5 @@ %ul %li A group is a collection of several projects - %li Groups are private by default %li Members of a group may only view projects they have permission to access %li Group project URLs are prefixed with the group namespace %li Existing projects may be moved into a group diff --git a/db/migrate/20160301124843_add_visibility_level_to_groups.rb b/db/migrate/20160301124843_add_visibility_level_to_groups.rb new file mode 100644 index 00000000000..b0afe795f42 --- /dev/null +++ b/db/migrate/20160301124843_add_visibility_level_to_groups.rb @@ -0,0 +1,9 @@ +class AddVisibilityLevelToGroups < ActiveRecord::Migration + def change + #All groups will be private when created + add_column :namespaces, :visibility_level, :integer, null: false, default: 0 + + #Set all existing groups to public + Group.update_all(visibility_level: 20) + end +end diff --git a/db/schema.rb b/db/schema.rb index a74b86d8e2f..d0e3bb769d2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,8 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160309140734) do - +ActiveRecord::Schema.define(version: 20160305220806) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -568,14 +567,15 @@ ActiveRecord::Schema.define(version: 20160309140734) do add_index "milestones", ["title"], name: "index_milestones_on_title", using: :btree create_table "namespaces", force: :cascade do |t| - t.string "name", null: false - t.string "path", null: false + t.string "name", null: false + t.string "path", null: false t.integer "owner_id" t.datetime "created_at" t.datetime "updated_at" t.string "type" - t.string "description", default: "", null: false + t.string "description", default: "", null: false t.string "avatar" + t.integer "visibility_level", default: 0, null: false end add_index "namespaces", ["created_at", "id"], name: "index_namespaces_on_created_at_and_id", using: :btree diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb new file mode 100644 index 00000000000..ed24954af7a --- /dev/null +++ b/spec/finders/groups_finder_spec.rb @@ -0,0 +1,25 @@ +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 } + + describe 'execute' do + describe 'without a user' do + subject { finder.execute } + + it { is_expected.to eq([public_group]) } + end + + describe 'with a user' do + subject { finder.execute(user) } + + it { is_expected.to eq([public_group, internal_group]) } + end + end + end +end diff --git a/spec/helpers/groups_helper.rb b/spec/helpers/groups_helper.rb index 4ea90a80a92..01ec9e5a07f 100644 --- a/spec/helpers/groups_helper.rb +++ b/spec/helpers/groups_helper.rb @@ -18,4 +18,19 @@ describe GroupsHelper do expect(group_icon(group.path)).to match('group_avatar.png') end end + + describe 'permissions' do + let(:group) { create(:group) } + let!(:user) { create(:user) } + + before do + allow(self).to receive(:current_user).and_return(user) + allow(self).to receive(:can?) { true } + end + + it 'checks user ability to change permissions' do + expect(self).to receive(:can?).with(user, :change_visibility_level, group) + can_change_group_visibility_level?(group) + end + end end diff --git a/spec/helpers/visibility_level_helper_spec.rb b/spec/helpers/visibility_level_helper_spec.rb index cd7596a763d..ef60ef75fbe 100644 --- a/spec/helpers/visibility_level_helper_spec.rb +++ b/spec/helpers/visibility_level_helper_spec.rb @@ -8,6 +8,7 @@ describe VisibilityLevelHelper do end let(:project) { build(:project) } + let(:group) { build(:group) } let(:personal_snippet) { build(:personal_snippet) } let(:project_snippet) { build(:project_snippet) } @@ -19,6 +20,13 @@ describe VisibilityLevelHelper do end end + context 'used with a Group' do + it 'delegates groups to #group_visibility_level_description' do + expect(visibility_level_description(Gitlab::VisibilityLevel::PRIVATE, group)) + .to match /group/i + end + end + context 'called with a Snippet' do it 'delegates snippets to #snippet_visibility_level_description' do expect(visibility_level_description(Gitlab::VisibilityLevel::INTERNAL, project_snippet)) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 3c995053eec..25aa77dc4e8 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -56,6 +56,24 @@ describe Group, models: true do end 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) } + + describe 'public_only' do + subject { described_class.public_only } + + it{ is_expected.to eq([public_group]) } + end + + describe 'public_and_internal_only' do + subject { described_class.public_and_internal_only } + + it{ is_expected.to eq([public_group, internal_group]) } + end + end + describe '#to_reference' do it 'returns a String reference to the object' do expect(group.to_reference).to eq "@#{group.name}" -- GitLab