Merge branch 'ee-1439-read-only-user' into 'master'

Backport changes from gitlab-org/gitlab-ee!998

See merge request !8984

15 files changed, 265 insertions, 80 deletions

diff --git a/.flayignore b/.flayignore
index 44df2ba2371..fc64b0b5892 100644
--- a/.flayignore
+++ b/.flayignore
@@ -1,3 +1,4 @@
 *.erb
 lib/gitlab/sanitizers/svg/whitelist.rb
 lib/gitlab/diff/position_tracer.rb
+app/policies/project_policy.rb
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index aa0f8d434dc..1cd50852e89 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -175,7 +175,7 @@ class Admin::UsersController < Admin::ApplicationController
 
   def user_params_ce
     [
-      :admin,
+      :access_level,
       :avatar,
       :bio,
       :can_create_group,
diff --git a/app/finders/group_projects_finder.rb b/app/finders/group_projects_finder.rb
index aa8f4c1d0e4..3b9a421b118 100644
--- a/app/finders/group_projects_finder.rb
+++ b/app/finders/group_projects_finder.rb
@@ -18,7 +18,7 @@ class GroupProjectsFinder < UnionFinder
     projects = []
 
     if current_user
-      if @group.users.include?(current_user) || current_user.admin?
+      if @group.users.include?(current_user)
         projects << @group.projects unless only_shared
         projects << @group.shared_projects unless only_owned
       else
diff --git a/app/models/user.rb b/app/models/user.rb
index 6c98224de35..f64d0c17a45 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -904,6 +904,21 @@ class User < ActiveRecord::Base
     end
   end
 
+  def access_level
+    if admin?
+      :admin
+    else
+      :regular
+    end
+  end
+
+  def access_level=(new_level)
+    new_level = new_level.to_s
+    return unless %w(admin regular).include?(new_level)
+
+    self.admin = (new_level == 'admin')
+  end
+
   private
 
   def ci_projects_union
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index f5fd50745aa..f8594e29547 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -218,25 +218,7 @@ class ProjectPolicy < BasePolicy
   def anonymous_rules
     return unless project.public?
 
-    can! :read_project
-    can! :read_board
-    can! :read_list
-    can! :read_wiki
-    can! :read_label
-    can! :read_milestone
-    can! :read_project_snippet
-    can! :read_project_member
-    can! :read_merge_request
-    can! :read_note
-    can! :read_pipeline
-    can! :read_commit_status
-    can! :read_container_image
-    can! :download_code
-    can! :download_wiki_code
-    can! :read_cycle_analytics
-
-    # NOTE: may be overridden by IssuePolicy
-    can! :read_issue
+    base_readonly_access!
 
     # Allow to read builds by anonymous user if guests are allowed
     can! :read_build if project.public_builds?
@@ -269,4 +251,31 @@ class ProjectPolicy < BasePolicy
       :"admin_#{name}"
     ]
   end
+
+  private
+
+  # A base set of abilities for read-only users, which
+  # is then augmented as necessary for anonymous and other
+  # read-only users.
+  def base_readonly_access!
+    can! :read_project
+    can! :read_board
+    can! :read_list
+    can! :read_wiki
+    can! :read_label
+    can! :read_milestone
+    can! :read_project_snippet
+    can! :read_project_member
+    can! :read_merge_request
+    can! :read_note
+    can! :read_pipeline
+    can! :read_commit_status
+    can! :read_container_image
+    can! :download_code
+    can! :download_wiki_code
+    can! :read_cycle_analytics
+
+    # NOTE: may be overridden by IssuePolicy
+    can! :read_issue
+  end
 end
diff --git a/app/policies/project_snippet_policy.rb b/app/policies/project_snippet_policy.rb
index 57acccfafd9..3a96836917e 100644
--- a/app/policies/project_snippet_policy.rb
+++ b/app/policies/project_snippet_policy.rb
@@ -3,7 +3,7 @@ class ProjectSnippetPolicy < BasePolicy
     can! :read_project_snippet if @subject.public?
     return unless @user
 
-    if @user && @subject.author == @user || @user.admin?
+    if @user && (@subject.author == @user || @user.admin?)
       can! :read_project_snippet
       can! :update_project_snippet
       can! :admin_project_snippet
diff --git a/app/views/admin/users/_access_levels.html.haml b/app/views/admin/users/_access_levels.html.haml
new file mode 100644
index 00000000000..7855239dfe5
--- /dev/null
+++ b/app/views/admin/users/_access_levels.html.haml
@@ -0,0 +1,37 @@
+%fieldset
+  %legend Access
+  .form-group
+    = f.label :projects_limit, class: 'control-label'
+    .col-sm-10= f.number_field :projects_limit, min: 0, class: 'form-control'
+
+  .form-group
+    = f.label :can_create_group, class: 'control-label'
+    .col-sm-10= f.check_box :can_create_group
+
+  .form-group
+    = f.label :access_level, class: 'control-label'
+    .col-sm-10
+      - editing_current_user = (current_user == @user)
+
+      = f.radio_button :access_level, :regular, disabled: editing_current_user
+      = label_tag :regular do
+        Regular
+      %p.light
+        Regular users have access to their groups and projects
+
+      = f.radio_button :access_level, :admin, disabled: editing_current_user
+      = label_tag :admin do
+        Admin
+      %p.light
+        Administrators have access to all groups, projects and users and can manage all features in this installation
+      - if editing_current_user
+        %p.light
+          You cannot remove your own admin rights.
+
+  .form-group
+    = f.label :external, class: 'control-label'
+    .col-sm-10
+      = f.check_box :external do
+        External
+      %p.light
+        External users cannot see internal or private projects unless access is explicitly granted. Also, external users cannot create projects or groups.
diff --git a/app/views/admin/users/_form.html.haml b/app/views/admin/users/_form.html.haml
index 3145212728f..e911af3f6f9 100644
--- a/app/views/admin/users/_form.html.haml
+++ b/app/views/admin/users/_form.html.haml
@@ -40,28 +40,7 @@
           = f.label :password_confirmation, class: 'control-label'
           .col-sm-10= f.password_field :password_confirmation, disabled: f.object.force_random_password, class: 'form-control'
 
-    %fieldset
-      %legend Access
-      .form-group
-        = f.label :projects_limit, class: 'control-label'
-        .col-sm-10= f.number_field :projects_limit, min: 0, class: 'form-control'
-
-      .form-group
-        = f.label :can_create_group, class: 'control-label'
-        .col-sm-10= f.check_box :can_create_group
-
-      .form-group
-        = f.label :admin, class: 'control-label'
-        - if current_user == @user
-          .col-sm-10= f.check_box :admin, disabled: true
-          .col-sm-10 You cannot remove your own admin rights.
-        - else
-          .col-sm-10= f.check_box :admin
-
-      .form-group
-        = f.label :external, class: 'control-label'
-        .col-sm-10= f.check_box :external
-        .col-sm-10 External users cannot see internal or private projects unless access is explicitly granted. Also, external users cannot create projects or groups.
+    = render partial: 'access_levels', locals: { f: f }
 
     %fieldset
       %legend Profile
diff --git a/app/views/projects/notes/_notes_with_form.html.haml b/app/views/projects/notes/_notes_with_form.html.haml
index fbd2bff5bbb..08c73d94a09 100644
--- a/app/views/projects/notes/_notes_with_form.html.haml
+++ b/app/views/projects/notes/_notes_with_form.html.haml
@@ -13,7 +13,7 @@
           = image_tag avatar_icon(current_user), alt: current_user.to_reference, class: 'avatar s40'
       .timeline-content.timeline-content-form
         = render "projects/notes/form", view: diff_view
-    - else
+    - elsif !current_user
       .disabled-comment.text-center
         .disabled-comment-text.inline
           Please
diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb
index c7953af29dd..a4e966e4016 100644
--- a/lib/gitlab/visibility_level.rb
+++ b/lib/gitlab/visibility_level.rb
@@ -13,7 +13,19 @@ module Gitlab
       scope :public_and_internal_only,  -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
       scope :non_public_only,           -> { where.not(visibility_level: PUBLIC) }
 
-      scope :public_to_user, -> (user) { user && !user.external ? public_and_internal_only : public_only }
+      scope :public_to_user, -> (user) do
+        if user
+          if user.admin?
+            all
+          elsif !user.external?
+            public_and_internal_only
+          else
+            public_only
+          end
+        else
+          public_only
+        end
+      end
     end
 
     PRIVATE  = 0 unless const_defined?(:PRIVATE)
diff --git a/spec/features/admin/admin_users_spec.rb b/spec/features/admin/admin_users_spec.rb
index a586f8d3184..c0807b8c507 100644
--- a/spec/features/admin/admin_users_spec.rb
+++ b/spec/features/admin/admin_users_spec.rb
@@ -211,7 +211,7 @@ describe "Admin::Users", feature: true do
         fill_in "user_email", with: "bigbang@mail.com"
         fill_in "user_password", with: "AValidPassword1"
         fill_in "user_password_confirmation", with: "AValidPassword1"
-        check "user_admin"
+        choose "user_access_level_admin"
         click_button "Save changes"
       end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 2d09d7c7fed..fe88ec63af6 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1422,4 +1422,37 @@ describe User, models: true do
       expect(user.project_authorizations.where(access_level: Gitlab::Access::REPORTER).exists?).to eq(true)
     end
   end
+
+  describe '#access_level=' do
+    let(:user) { build(:user) }
+
+    it 'does nothing for an invalid access level' do
+      user.access_level = :invalid_access_level
+
+      expect(user.access_level).to eq(:regular)
+      expect(user.admin).to be false
+    end
+
+    it "assigns the 'admin' access level" do
+      user.access_level = :admin
+
+      expect(user.access_level).to eq(:admin)
+      expect(user.admin).to be true
+    end
+
+    it "doesn't clear existing access levels when an invalid access level is passed in" do
+      user.access_level = :admin
+      user.access_level = :invalid_access_level
+
+      expect(user.access_level).to eq(:admin)
+      expect(user.admin).to be true
+    end
+
+    it "accepts string values in addition to symbols" do
+      user.access_level = 'admin'
+
+      expect(user.access_level).to eq(:admin)
+      expect(user.admin).to be true
+    end
+  end
 end
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index eeab9827d99..0a5edf35f59 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -10,61 +10,59 @@ describe ProjectPolicy, models: true do
   let(:project) { create(:empty_project, :public, namespace: owner.namespace) }
 
   let(:guest_permissions) do
-    [
-      :read_project, :read_board, :read_list, :read_wiki, :read_issue, :read_label,
-      :read_milestone, :read_project_snippet, :read_project_member,
-      :read_note, :create_project, :create_issue, :create_note,
-      :upload_file
+    %i[
+      read_project read_board read_list read_wiki read_issue read_label
+      read_milestone read_project_snippet read_project_member
+      read_note create_project create_issue create_note
+      upload_file
     ]
   end
 
   let(:reporter_permissions) do
-    [
-      :download_code, :fork_project, :create_project_snippet, :update_issue,
-      :admin_issue, :admin_label, :admin_list, :read_commit_status, :read_build,
-      :read_container_image, :read_pipeline, :read_environment, :read_deployment,
-      :read_merge_request, :download_wiki_code
+    %i[
+      download_code fork_project create_project_snippet update_issue
+      admin_issue admin_label admin_list read_commit_status read_build
+      read_container_image read_pipeline read_environment read_deployment
+      read_merge_request download_wiki_code
     ]
   end
 
   let(:team_member_reporter_permissions) do
-    [
-      :build_download_code, :build_read_container_image
-    ]
+    %i[build_download_code build_read_container_image]
   end
 
   let(:developer_permissions) do
-    [
-      :admin_merge_request, :update_merge_request, :create_commit_status,
-      :update_commit_status, :create_build, :update_build, :create_pipeline,
-      :update_pipeline, :create_merge_request, :create_wiki, :push_code,
-      :resolve_note, :create_container_image, :update_container_image,
-      :create_environment, :create_deployment
+    %i[
+      admin_merge_request update_merge_request create_commit_status
+      update_commit_status create_build update_build create_pipeline
+      update_pipeline create_merge_request create_wiki push_code
+      resolve_note create_container_image update_container_image
+      create_environment create_deployment
     ]
   end
 
   let(:master_permissions) do
-    [
-      :push_code_to_protected_branches, :update_project_snippet, :update_environment,
-      :update_deployment, :admin_milestone, :admin_project_snippet,
-      :admin_project_member, :admin_note, :admin_wiki, :admin_project,
-      :admin_commit_status, :admin_build, :admin_container_image,
-      :admin_pipeline, :admin_environment, :admin_deployment
+    %i[
+      push_code_to_protected_branches update_project_snippet update_environment
+      update_deployment admin_milestone admin_project_snippet
+      admin_project_member admin_note admin_wiki admin_project
+      admin_commit_status admin_build admin_container_image
+      admin_pipeline admin_environment admin_deployment
     ]
   end
 
   let(:public_permissions) do
-    [
-      :download_code, :fork_project, :read_commit_status, :read_pipeline,
-      :read_container_image, :build_download_code, :build_read_container_image,
-      :download_wiki_code
+    %i[
+      download_code fork_project read_commit_status read_pipeline
+      read_container_image build_download_code build_read_container_image
+      download_wiki_code
     ]
   end
 
   let(:owner_permissions) do
-    [
-      :change_namespace, :change_visibility_level, :rename_project, :remove_project,
-      :archive_project, :remove_fork_project, :destroy_merge_request, :destroy_issue
+    %i[
+      change_namespace change_visibility_level rename_project remove_project
+      archive_project remove_fork_project destroy_merge_request destroy_issue
     ]
   end
 
diff --git a/spec/policies/project_snippet_policy_spec.rb b/spec/policies/project_snippet_policy_spec.rb
new file mode 100644
index 00000000000..d0758af57dd
--- /dev/null
+++ b/spec/policies/project_snippet_policy_spec.rb
@@ -0,0 +1,101 @@
+require 'spec_helper'
+
+describe ProjectSnippetPolicy, models: true do
+  let(:current_user) { create(:user) }
+
+  let(:author_permissions) do
+    [
+      :update_project_snippet,
+      :admin_project_snippet
+    ]
+  end
+
+  subject { described_class.abilities(current_user, project_snippet).to_set }
+
+  context 'public snippet' do
+    let(:project_snippet) { create(:project_snippet, :public) }
+
+    context 'no user' do
+      let(:current_user) { nil }
+
+      it do
+        is_expected.to include(:read_project_snippet)
+        is_expected.not_to include(*author_permissions)
+      end
+    end
+
+    context 'regular user' do
+      it do
+        is_expected.to include(:read_project_snippet)
+        is_expected.not_to include(*author_permissions)
+      end
+    end
+  end
+
+  context 'internal snippet' do
+    let(:project_snippet) { create(:project_snippet, :internal) }
+
+    context 'no user' do
+      let(:current_user) { nil }
+
+      it do
+        is_expected.not_to include(:read_project_snippet)
+        is_expected.not_to include(*author_permissions)
+      end
+    end
+
+    context 'regular user' do
+      it do
+        is_expected.to include(:read_project_snippet)
+        is_expected.not_to include(*author_permissions)
+      end
+    end
+  end
+
+  context 'private snippet' do
+    let(:project_snippet) { create(:project_snippet, :private) }
+
+    context 'no user' do
+      let(:current_user) { nil }
+
+      it do
+        is_expected.not_to include(:read_project_snippet)
+        is_expected.not_to include(*author_permissions)
+      end
+    end
+
+    context 'regular user' do
+      it do
+        is_expected.not_to include(:read_project_snippet)
+        is_expected.not_to include(*author_permissions)
+      end
+    end
+
+    context 'snippet author' do
+      let(:project_snippet) { create(:project_snippet, :private, author: current_user) }
+
+      it do
+        is_expected.to include(:read_project_snippet)
+        is_expected.to include(*author_permissions)
+      end
+    end
+
+    context 'project team member' do
+      before { project_snippet.project.team << [current_user, :developer] }
+
+      it do
+        is_expected.to include(:read_project_snippet)
+        is_expected.not_to include(*author_permissions)
+      end
+    end
+
+    context 'admin user' do
+      let(:current_user) { create(:admin) }
+
+      it do
+        is_expected.to include(:read_project_snippet)
+        is_expected.to include(*author_permissions)
+      end
+    end
+  end
+end
diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb
index 531180e48a1..7c0fccb9d41 100644
--- a/spec/services/groups/update_service_spec.rb
+++ b/spec/services/groups/update_service_spec.rb
@@ -51,7 +51,7 @@ describe Groups::UpdateService, services: true do
   end
 
   context 'rename group' do
-    let!(:service) { described_class.new(internal_group, user, path: 'new_path') }
+    let!(:service) { described_class.new(internal_group, user, path: SecureRandom.hex) }
 
     before do
       internal_group.add_user(user, Gitlab::Access::MASTER)