From f2a9ee258e0ee3a6fe0cb614e4b73c56dcd7339d Mon Sep 17 00:00:00 2001
From: Felipe Artur <felipefac@gmail.com>
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