Merge branch 'refactor/global-permissions-for-internal-users' into 'master'

Refactor/global permissions for internal users

See merge request !9598

21 files changed, 105 insertions, 65 deletions

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 7ffde71c3b1..24504685e48 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -29,11 +29,7 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def impersonate
-    if user.blocked?
-      flash[:alert] = "You cannot impersonate a blocked user"
-
-      redirect_to admin_user_path(user)
-    else
+    if can?(user, :log_in)
       session[:impersonator_id] = current_user.id
 
       warden.set_user(user, scope: :user)
@@ -43,6 +39,17 @@ class Admin::UsersController < Admin::ApplicationController
       flash[:alert] = "You are now impersonating #{user.username}"
 
       redirect_to root_path
+    else
+      flash[:alert] =
+        if user.blocked?
+          "You cannot impersonate a blocked user"
+        elsif user.internal?
+          "You cannot impersonate an internal user"
+        else
+          "You cannot impersonate a user who cannot log in"
+        end
+
+      redirect_to admin_user_path(user)
     end
   end
 
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 1c66c530cd2..b7ce081a5cd 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -67,7 +67,7 @@ class ApplicationController < ActionController::Base
     token_string = params[:private_token].presence || request.headers['PRIVATE-TOKEN'].presence
     user = User.find_by_authentication_token(token_string) || User.find_by_personal_access_token(token_string)
 
-    if user
+    if user && can?(user, :log_in)
       # Notice we are passing store false, so the user is not
       # actually stored in the session and a token is needed
       # for every request. If you want the token to work as a
@@ -90,7 +90,7 @@ class ApplicationController < ActionController::Base
     current_application_settings.after_sign_out_path.presence || new_user_session_path
   end
 
-  def can?(object, action, subject)
+  def can?(object, action, subject = :global)
     Ability.allowed?(object, action, subject)
   end
 
diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb
index 4c497711fc0..ea441b1736b 100644
--- a/app/controllers/concerns/authenticates_with_two_factor.rb
+++ b/app/controllers/concerns/authenticates_with_two_factor.rb
@@ -23,7 +23,7 @@ module AuthenticatesWithTwoFactor
   #
   # Returns nil
   def prompt_for_two_factor(user)
-    return locked_user_redirect(user) if user.access_locked?
+    return locked_user_redirect(user) unless user.can?(:log_in)
 
     session[:otp_user_id] = user.id
     setup_u2f_authentication(user)
@@ -37,10 +37,9 @@ module AuthenticatesWithTwoFactor
 
   def authenticate_with_two_factor
     user = self.resource = find_user
+    return locked_user_redirect(user) unless user.can?(:log_in)
 
-    if user.access_locked?
-      locked_user_redirect(user)
-    elsif user_params[:otp_attempt].present? && session[:otp_user_id]
+    if user_params[:otp_attempt].present? && session[:otp_user_id]
       authenticate_with_two_factor_via_otp(user)
     elsif user_params[:device_response].present? && session[:otp_user_id]
       authenticate_with_two_factor_via_u2f(user)
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 4663b6e7fc6..05f9ee1ee90 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -118,7 +118,7 @@ class GroupsController < Groups::ApplicationController
   end
 
   def authorize_create_group!
-    unless can?(current_user, :create_group, nil)
+    unless can?(current_user, :create_group)
       return render_404
     end
   end
diff --git a/app/models/ability.rb b/app/models/ability.rb
index ad6c588202e..f3692a5a067 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -56,15 +56,16 @@ class Ability
       end
     end
 
-    def allowed?(user, action, subject)
+    def allowed?(user, action, subject = :global)
       allowed(user, subject).include?(action)
     end
 
-    def allowed(user, subject)
+    def allowed(user, subject = :global)
+      return BasePolicy::RuleSet.none if subject.nil?
       return uncached_allowed(user, subject) unless RequestStore.active?
 
       user_key = user ? user.id : 'anonymous'
-      subject_key = subject ? "#{subject.class.name}/#{subject.id}" : 'global'
+      subject_key = subject == :global ? 'global' : "#{subject.class.name}/#{subject.id}"
       key = "/ability/#{user_key}/#{subject_key}"
       RequestStore[key] ||= uncached_allowed(user, subject).freeze
     end
diff --git a/app/models/guest.rb b/app/models/guest.rb
index 01285ca1264..df287c277a7 100644
--- a/app/models/guest.rb
+++ b/app/models/guest.rb
@@ -1,6 +1,6 @@
 class Guest
   class << self
-    def can?(action, subject)
+    def can?(action, subject = :global)
       Ability.allowed?(nil, action, subject)
     end
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index 76fb4cd470e..39c1281179b 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -126,7 +126,6 @@ class User < ActiveRecord::Base
   validate :unique_email, if: ->(user) { user.email_changed? }
   validate :owns_notification_email, if: ->(user) { user.notification_email_changed? }
   validate :owns_public_email, if: ->(user) { user.public_email_changed? }
-  validate :ghost_users_must_be_blocked
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
   before_validation :generate_password, on: :create
@@ -350,12 +349,27 @@ class User < ActiveRecord::Base
     def ghost
       unique_internal(where(ghost: true), 'ghost', 'ghost%s@example.com') do |u|
         u.bio = 'This is a "Ghost User", created to hold all issues authored by users that have since been deleted. This user cannot be removed.'
-        u.state = :blocked
         u.name = 'Ghost User'
       end
     end
   end
 
+  def self.internal_attributes
+    [:ghost]
+  end
+
+  def internal?
+    self.class.internal_attributes.any? { |a| self[a] }
+  end
+
+  def self.internal
+    where(Hash[internal_attributes.zip([true] * internal_attributes.size)])
+  end
+
+  def self.non_internal
+    where(Hash[internal_attributes.zip([[false, nil]] * internal_attributes.size)])
+  end
+
   #
   # Instance methods
   #
@@ -452,12 +466,6 @@ class User < ActiveRecord::Base
     errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
   end
 
-  def ghost_users_must_be_blocked
-    if ghost? && !blocked?
-      errors.add(:ghost, 'cannot be enabled for a user who is not blocked.')
-    end
-  end
-
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record
@@ -563,14 +571,14 @@ class User < ActiveRecord::Base
   end
 
   def can_create_group?
-    can?(:create_group, nil)
+    can?(:create_group)
   end
 
   def can_select_namespace?
     several_namespaces? || admin
   end
 
-  def can?(action, subject)
+  def can?(action, subject = :global)
     Ability.allowed?(self, action, subject)
   end
 
@@ -955,6 +963,14 @@ class User < ActiveRecord::Base
     self.admin = (new_level == 'admin')
   end
 
+  protected
+
+  # override, from Devise::Validatable
+  def password_required?
+    return false if internal?
+    super
+  end
+
   private
 
   def ci_projects_union
@@ -1055,7 +1071,6 @@ class User < ActiveRecord::Base
 
     scope.create(
       username: username,
-      password: Devise.friendly_token,
       email: email,
       &creation_block
     )
diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb
index e07b144355a..8890409d056 100644
--- a/app/policies/base_policy.rb
+++ b/app/policies/base_policy.rb
@@ -12,6 +12,10 @@ class BasePolicy
       new(Set.new, Set.new)
     end
 
+    def self.none
+      empty.freeze
+    end
+
     def can?(ability)
       @can_set.include?(ability) && !@cannot_set.include?(ability)
     end
@@ -49,7 +53,8 @@ class BasePolicy
   end
 
   def self.class_for(subject)
-    return GlobalPolicy if subject.nil?
+    return GlobalPolicy if subject == :global
+    raise ArgumentError, 'no policy for nil' if subject.nil?
 
     if subject.class.try(:presenter?)
       subject = subject.subject
@@ -79,7 +84,7 @@ class BasePolicy
   end
 
   def abilities
-    return RuleSet.empty if @user && @user.blocked?
+    return RuleSet.none if @user && @user.blocked?
     return anonymous_abilities if @user.nil?
     collect_rules { rules }
   end
diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb
index 3c2fbe6b56b..cb72c2b4590 100644
--- a/app/policies/global_policy.rb
+++ b/app/policies/global_policy.rb
@@ -4,5 +4,12 @@ class GlobalPolicy < BasePolicy
 
     can! :create_group if @user.can_create_group
     can! :read_users_list
+
+    unless @user.blocked? || @user.internal?
+      can! :log_in unless @user.access_locked?
+      can! :access_api
+      can! :access_git
+      can! :receive_notifications
+    end
   end
 end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index fbad85d310e..d12692ecc90 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -465,7 +465,7 @@ class NotificationService
     end
 
     users = users.to_a.compact.uniq
-    users = users.reject(&:blocked?)
+    users = users.select { |u| u.can?(:receive_notifications) }
 
     users.reject do |user|
       global_notification_setting = user.global_notification_setting
diff --git a/app/views/admin/users/_head.html.haml b/app/views/admin/users/_head.html.haml
index d20be373564..be41c33b853 100644
--- a/app/views/admin/users/_head.html.haml
+++ b/app/views/admin/users/_head.html.haml
@@ -2,11 +2,13 @@
   = @user.name
   - if @user.blocked?
     %span.cred (Blocked)
+  - if @user.internal?
+    %span.cred (Internal)
   - if @user.admin
     %span.cred (Admin)
 
   .pull-right
-    - unless @user == current_user || @user.blocked?
+    - if @user != current_user && @user.can?(:log_in)
       = link_to 'Impersonate', impersonate_admin_user_path(@user), method: :post, class: "btn btn-nr btn-grouped btn-info"
     = link_to edit_admin_user_path(@user), class: "btn btn-nr btn-grouped" do
       %i.fa.fa-pencil-square-o
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index a9b364da9e1..bd22b82476b 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -97,7 +97,7 @@ module API
     end
 
     def authenticate!
-      unauthorized! unless current_user
+      unauthorized! unless current_user && can?(current_user, :access_api)
     end
 
     def authenticate_non_get!
@@ -116,7 +116,7 @@ module API
       forbidden! unless current_user.is_admin?
     end
 
-    def authorize!(action, subject = nil)
+    def authorize!(action, subject = :global)
       forbidden! unless can?(current_user, action, subject)
     end
 
@@ -134,7 +134,7 @@ module API
       end
     end
 
-    def can?(object, action, subject)
+    def can?(object, action, subject = :global)
       Ability.allowed?(object, action, subject)
     end
 
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 549003f576a..2d4d5a25221 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -45,7 +45,7 @@ module API
         use :pagination
       end
       get do
-        unless can?(current_user, :read_users_list, nil)
+        unless can?(current_user, :read_users_list)
           render_api_error!("Not authorized.", 403)
         end
 
diff --git a/lib/banzai/reference_parser/base_parser.rb b/lib/banzai/reference_parser/base_parser.rb
index 2058a58d0ae..b121c37c5d0 100644
--- a/lib/banzai/reference_parser/base_parser.rb
+++ b/lib/banzai/reference_parser/base_parser.rb
@@ -210,7 +210,7 @@ module Banzai
           grouped_objects_for_nodes(nodes, Project, 'data-project')
       end
 
-      def can?(user, permission, subject)
+      def can?(user, permission, subject = :global)
         Ability.allowed?(user, permission, subject)
       end
 
diff --git a/lib/gitlab/allowable.rb b/lib/gitlab/allowable.rb
index f48abcc86d5..e4f7cad2b79 100644
--- a/lib/gitlab/allowable.rb
+++ b/lib/gitlab/allowable.rb
@@ -1,6 +1,6 @@
 module Gitlab
   module Allowable
-    def can?(user, action, subject)
+    def can?(user, action, subject = :global)
       Ability.allowed?(user, action, subject)
     end
   end
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index 6ce9b229294..f260c0c535f 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -8,7 +8,7 @@ module Gitlab
     end
 
     def can_do_action?(action)
-      return false if no_user_or_blocked?
+      return false unless can_access_git?
 
       @permission_cache ||= {}
       @permission_cache[action] ||= user.can?(action, project)
@@ -19,7 +19,7 @@ module Gitlab
     end
 
     def allowed?
-      return false if no_user_or_blocked?
+      return false unless can_access_git?
 
       if user.requires_ldap_check? && user.try_obtain_ldap_lease
         return false unless Gitlab::LDAP::Access.allowed?(user)
@@ -29,7 +29,7 @@ module Gitlab
     end
 
     def can_push_to_branch?(ref)
-      return false if no_user_or_blocked?
+      return false unless can_access_git?
 
       if project.protected_branch?(ref)
         return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user)
@@ -44,7 +44,7 @@ module Gitlab
     end
 
     def can_merge_to_branch?(ref)
-      return false if no_user_or_blocked?
+      return false unless can_access_git?
 
       if project.protected_branch?(ref)
         access_levels = project.protected_branches.matching(ref).map(&:merge_access_levels).flatten
@@ -55,15 +55,15 @@ module Gitlab
     end
 
     def can_read_project?
-      return false if no_user_or_blocked?
+      return false unless can_access_git?
 
       user.can?(:read_project, project)
     end
 
     private
 
-    def no_user_or_blocked?
-      user.nil? || user.blocked?
+    def can_access_git?
+      user && user.can?(:access_git)
     end
   end
 end
diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb
index ae609160e18..f32d1f78b40 100644
--- a/spec/features/login_spec.rb
+++ b/spec/features/login_spec.rb
@@ -48,6 +48,18 @@ feature 'Login', feature: true do
     end
   end
 
+  describe 'with the ghost user' do
+    it 'disallows login' do
+      login_with(User.ghost)
+
+      expect(page).to have_content('Invalid Login or password.')
+    end
+
+    it 'does not update Devise trackable attributes' do
+      expect { login_with(User.ghost) }.not_to change { User.ghost.reload.sign_in_count }
+    end
+  end
+
   describe 'with two-factor authentication' do
     def enter_code(code)
       fill_in 'user_otp_attempt', with: code
diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb
index 30f8fdf91b2..92d70cfc64c 100644
--- a/spec/models/ability_spec.rb
+++ b/spec/models/ability_spec.rb
@@ -1,6 +1,12 @@
 require 'spec_helper'
 
 describe Ability, lib: true do
+  context 'using a nil subject' do
+    it 'is always empty' do
+      expect(Ability.allowed(nil, nil).to_set).to be_empty
+    end
+  end
+
   describe '.can_edit_note?' do
     let(:project) { create(:empty_project) }
     let(:note) { create(:note_on_issue, project: project) }
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 3166b951a42..9da4140f3ce 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -210,22 +210,6 @@ describe User, models: true do
         end
       end
     end
-
-    describe 'ghost users' do
-      it 'does not allow a non-blocked ghost user' do
-        user = build(:user, :ghost)
-        user.state = 'active'
-
-        expect(user).to be_invalid
-      end
-
-      it 'allows a blocked ghost user' do
-        user = build(:user, :ghost)
-        user.state = 'blocked'
-
-        expect(user).to be_valid
-      end
-    end
   end
 
   describe "scopes" do
diff --git a/spec/policies/base_policy_spec.rb b/spec/policies/base_policy_spec.rb
index 63acc0b68cd..02acdcb36df 100644
--- a/spec/policies/base_policy_spec.rb
+++ b/spec/policies/base_policy_spec.rb
@@ -1,17 +1,19 @@
 require 'spec_helper'
 
 describe BasePolicy, models: true do
-  let(:build) { Ci::Build.new }
-
   describe '.class_for' do
     it 'detects policy class based on the subject ancestors' do
-      expect(described_class.class_for(build)).to eq(Ci::BuildPolicy)
+      expect(described_class.class_for(GenericCommitStatus.new)).to eq(CommitStatusPolicy)
     end
 
     it 'detects policy class for a presented subject' do
-      presentee = Ci::BuildPresenter.new(build)
+      presentee = Ci::BuildPresenter.new(Ci::Build.new)
 
       expect(described_class.class_for(presentee)).to eq(Ci::BuildPolicy)
     end
+
+    it 'uses GlobalPolicy when :global is given' do
+      expect(described_class.class_for(:global)).to eq(GlobalPolicy)
+    end
   end
 end
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index a89676fec93..988a57a80ea 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -436,7 +436,7 @@ describe API::Helpers, api: true do
 
     context 'current_user is present' do
       before do
-        expect_any_instance_of(self.class).to receive(:current_user).and_return(true)
+        expect_any_instance_of(self.class).to receive(:current_user).at_least(:once).and_return(User.new)
       end
 
       it 'does not raise an error' do