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 }