Commit d1df36e3 authored by Sean McGivern's avatar Sean McGivern

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

Refactor/global permissions for internal users

See merge request !9598
parents f49868ad 90e11fb2
...@@ -29,11 +29,7 @@ class Admin::UsersController < Admin::ApplicationController ...@@ -29,11 +29,7 @@ class Admin::UsersController < Admin::ApplicationController
end end
def impersonate def impersonate
if user.blocked? if can?(user, :log_in)
flash[:alert] = "You cannot impersonate a blocked user"
redirect_to admin_user_path(user)
else
session[:impersonator_id] = current_user.id session[:impersonator_id] = current_user.id
warden.set_user(user, scope: :user) warden.set_user(user, scope: :user)
...@@ -43,6 +39,17 @@ class Admin::UsersController < Admin::ApplicationController ...@@ -43,6 +39,17 @@ class Admin::UsersController < Admin::ApplicationController
flash[:alert] = "You are now impersonating #{user.username}" flash[:alert] = "You are now impersonating #{user.username}"
redirect_to root_path 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
end end
......
...@@ -67,7 +67,7 @@ class ApplicationController < ActionController::Base ...@@ -67,7 +67,7 @@ class ApplicationController < ActionController::Base
token_string = params[:private_token].presence || request.headers['PRIVATE-TOKEN'].presence 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) 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 # Notice we are passing store false, so the user is not
# actually stored in the session and a token is needed # actually stored in the session and a token is needed
# for every request. If you want the token to work as a # for every request. If you want the token to work as a
...@@ -90,7 +90,7 @@ class ApplicationController < ActionController::Base ...@@ -90,7 +90,7 @@ class ApplicationController < ActionController::Base
current_application_settings.after_sign_out_path.presence || new_user_session_path current_application_settings.after_sign_out_path.presence || new_user_session_path
end end
def can?(object, action, subject) def can?(object, action, subject = :global)
Ability.allowed?(object, action, subject) Ability.allowed?(object, action, subject)
end end
......
...@@ -23,7 +23,7 @@ module AuthenticatesWithTwoFactor ...@@ -23,7 +23,7 @@ module AuthenticatesWithTwoFactor
# #
# Returns nil # Returns nil
def prompt_for_two_factor(user) 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 session[:otp_user_id] = user.id
setup_u2f_authentication(user) setup_u2f_authentication(user)
...@@ -37,10 +37,9 @@ module AuthenticatesWithTwoFactor ...@@ -37,10 +37,9 @@ module AuthenticatesWithTwoFactor
def authenticate_with_two_factor def authenticate_with_two_factor
user = self.resource = find_user user = self.resource = find_user
return locked_user_redirect(user) unless user.can?(:log_in)
if user.access_locked? if user_params[:otp_attempt].present? && session[:otp_user_id]
locked_user_redirect(user)
elsif user_params[:otp_attempt].present? && session[:otp_user_id]
authenticate_with_two_factor_via_otp(user) authenticate_with_two_factor_via_otp(user)
elsif user_params[:device_response].present? && session[:otp_user_id] elsif user_params[:device_response].present? && session[:otp_user_id]
authenticate_with_two_factor_via_u2f(user) authenticate_with_two_factor_via_u2f(user)
......
...@@ -118,7 +118,7 @@ class GroupsController < Groups::ApplicationController ...@@ -118,7 +118,7 @@ class GroupsController < Groups::ApplicationController
end end
def authorize_create_group! def authorize_create_group!
unless can?(current_user, :create_group, nil) unless can?(current_user, :create_group)
return render_404 return render_404
end end
end end
......
...@@ -56,15 +56,16 @@ class Ability ...@@ -56,15 +56,16 @@ class Ability
end end
end end
def allowed?(user, action, subject) def allowed?(user, action, subject = :global)
allowed(user, subject).include?(action) allowed(user, subject).include?(action)
end 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? return uncached_allowed(user, subject) unless RequestStore.active?
user_key = user ? user.id : 'anonymous' 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}" key = "/ability/#{user_key}/#{subject_key}"
RequestStore[key] ||= uncached_allowed(user, subject).freeze RequestStore[key] ||= uncached_allowed(user, subject).freeze
end end
......
class Guest class Guest
class << self class << self
def can?(action, subject) def can?(action, subject = :global)
Ability.allowed?(nil, action, subject) Ability.allowed?(nil, action, subject)
end end
end end
......
...@@ -126,7 +126,6 @@ class User < ActiveRecord::Base ...@@ -126,7 +126,6 @@ class User < ActiveRecord::Base
validate :unique_email, if: ->(user) { user.email_changed? } validate :unique_email, if: ->(user) { user.email_changed? }
validate :owns_notification_email, if: ->(user) { user.notification_email_changed? } validate :owns_notification_email, if: ->(user) { user.notification_email_changed? }
validate :owns_public_email, if: ->(user) { user.public_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 } validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
before_validation :generate_password, on: :create before_validation :generate_password, on: :create
...@@ -350,12 +349,27 @@ class User < ActiveRecord::Base ...@@ -350,12 +349,27 @@ class User < ActiveRecord::Base
def ghost def ghost
unique_internal(where(ghost: true), 'ghost', 'ghost%s@example.com') do |u| 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.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' u.name = 'Ghost User'
end end
end 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 # Instance methods
# #
...@@ -452,12 +466,6 @@ class User < ActiveRecord::Base ...@@ -452,12 +466,6 @@ class User < ActiveRecord::Base
errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email) errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
end 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 def update_emails_with_primary_email
primary_email_record = emails.find_by(email: email) primary_email_record = emails.find_by(email: email)
if primary_email_record if primary_email_record
...@@ -563,14 +571,14 @@ class User < ActiveRecord::Base ...@@ -563,14 +571,14 @@ class User < ActiveRecord::Base
end end
def can_create_group? def can_create_group?
can?(:create_group, nil) can?(:create_group)
end end
def can_select_namespace? def can_select_namespace?
several_namespaces? || admin several_namespaces? || admin
end end
def can?(action, subject) def can?(action, subject = :global)
Ability.allowed?(self, action, subject) Ability.allowed?(self, action, subject)
end end
...@@ -955,6 +963,14 @@ class User < ActiveRecord::Base ...@@ -955,6 +963,14 @@ class User < ActiveRecord::Base
self.admin = (new_level == 'admin') self.admin = (new_level == 'admin')
end end
protected
# override, from Devise::Validatable
def password_required?
return false if internal?
super
end
private private
def ci_projects_union def ci_projects_union
...@@ -1055,7 +1071,6 @@ class User < ActiveRecord::Base ...@@ -1055,7 +1071,6 @@ class User < ActiveRecord::Base
scope.create( scope.create(
username: username, username: username,
password: Devise.friendly_token,
email: email, email: email,
&creation_block &creation_block
) )
......
...@@ -12,6 +12,10 @@ class BasePolicy ...@@ -12,6 +12,10 @@ class BasePolicy
new(Set.new, Set.new) new(Set.new, Set.new)
end end
def self.none
empty.freeze
end
def can?(ability) def can?(ability)
@can_set.include?(ability) && !@cannot_set.include?(ability) @can_set.include?(ability) && !@cannot_set.include?(ability)
end end
...@@ -49,7 +53,8 @@ class BasePolicy ...@@ -49,7 +53,8 @@ class BasePolicy
end end
def self.class_for(subject) 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?) if subject.class.try(:presenter?)
subject = subject.subject subject = subject.subject
...@@ -79,7 +84,7 @@ class BasePolicy ...@@ -79,7 +84,7 @@ class BasePolicy
end end
def abilities def abilities
return RuleSet.empty if @user && @user.blocked? return RuleSet.none if @user && @user.blocked?
return anonymous_abilities if @user.nil? return anonymous_abilities if @user.nil?
collect_rules { rules } collect_rules { rules }
end end
......
...@@ -4,5 +4,12 @@ class GlobalPolicy < BasePolicy ...@@ -4,5 +4,12 @@ class GlobalPolicy < BasePolicy
can! :create_group if @user.can_create_group can! :create_group if @user.can_create_group
can! :read_users_list 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
end end
...@@ -465,7 +465,7 @@ class NotificationService ...@@ -465,7 +465,7 @@ class NotificationService
end end
users = users.to_a.compact.uniq users = users.to_a.compact.uniq
users = users.reject(&:blocked?) users = users.select { |u| u.can?(:receive_notifications) }
users.reject do |user| users.reject do |user|
global_notification_setting = user.global_notification_setting global_notification_setting = user.global_notification_setting
......
...@@ -2,11 +2,13 @@ ...@@ -2,11 +2,13 @@
= @user.name = @user.name
- if @user.blocked? - if @user.blocked?
%span.cred (Blocked) %span.cred (Blocked)
- if @user.internal?
%span.cred (Internal)
- if @user.admin - if @user.admin
%span.cred (Admin) %span.cred (Admin)
.pull-right .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 '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 = link_to edit_admin_user_path(@user), class: "btn btn-nr btn-grouped" do
%i.fa.fa-pencil-square-o %i.fa.fa-pencil-square-o
......
...@@ -97,7 +97,7 @@ module API ...@@ -97,7 +97,7 @@ module API
end end
def authenticate! def authenticate!
unauthorized! unless current_user unauthorized! unless current_user && can?(current_user, :access_api)
end end
def authenticate_non_get! def authenticate_non_get!
...@@ -116,7 +116,7 @@ module API ...@@ -116,7 +116,7 @@ module API
forbidden! unless current_user.is_admin? forbidden! unless current_user.is_admin?
end end
def authorize!(action, subject = nil) def authorize!(action, subject = :global)
forbidden! unless can?(current_user, action, subject) forbidden! unless can?(current_user, action, subject)
end end
...@@ -134,7 +134,7 @@ module API ...@@ -134,7 +134,7 @@ module API
end end
end end
def can?(object, action, subject) def can?(object, action, subject = :global)
Ability.allowed?(object, action, subject) Ability.allowed?(object, action, subject)
end end
......
...@@ -45,7 +45,7 @@ module API ...@@ -45,7 +45,7 @@ module API
use :pagination use :pagination
end end
get do get do
unless can?(current_user, :read_users_list, nil) unless can?(current_user, :read_users_list)
render_api_error!("Not authorized.", 403) render_api_error!("Not authorized.", 403)
end end
......
...@@ -210,7 +210,7 @@ module Banzai ...@@ -210,7 +210,7 @@ module Banzai
grouped_objects_for_nodes(nodes, Project, 'data-project') grouped_objects_for_nodes(nodes, Project, 'data-project')
end end
def can?(user, permission, subject) def can?(user, permission, subject = :global)
Ability.allowed?(user, permission, subject) Ability.allowed?(user, permission, subject)
end end
......
module Gitlab module Gitlab
module Allowable module Allowable
def can?(user, action, subject) def can?(user, action, subject = :global)
Ability.allowed?(user, action, subject) Ability.allowed?(user, action, subject)
end end
end end
......
...@@ -8,7 +8,7 @@ module Gitlab ...@@ -8,7 +8,7 @@ module Gitlab
end end
def can_do_action?(action) def can_do_action?(action)
return false if no_user_or_blocked? return false unless can_access_git?
@permission_cache ||= {} @permission_cache ||= {}
@permission_cache[action] ||= user.can?(action, project) @permission_cache[action] ||= user.can?(action, project)
...@@ -19,7 +19,7 @@ module Gitlab ...@@ -19,7 +19,7 @@ module Gitlab
end end
def allowed? 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 if user.requires_ldap_check? && user.try_obtain_ldap_lease
return false unless Gitlab::LDAP::Access.allowed?(user) return false unless Gitlab::LDAP::Access.allowed?(user)
...@@ -29,7 +29,7 @@ module Gitlab ...@@ -29,7 +29,7 @@ module Gitlab
end end
def can_push_to_branch?(ref) def can_push_to_branch?(ref)
return false if no_user_or_blocked? return false unless can_access_git?
if project.protected_branch?(ref) if project.protected_branch?(ref)
return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user)
...@@ -44,7 +44,7 @@ module Gitlab ...@@ -44,7 +44,7 @@ module Gitlab
end end
def can_merge_to_branch?(ref) def can_merge_to_branch?(ref)
return false if no_user_or_blocked? return false unless can_access_git?
if project.protected_branch?(ref) if project.protected_branch?(ref)
access_levels = project.protected_branches.matching(ref).map(&:merge_access_levels).flatten access_levels = project.protected_branches.matching(ref).map(&:merge_access_levels).flatten
...@@ -55,15 +55,15 @@ module Gitlab ...@@ -55,15 +55,15 @@ module Gitlab
end end
def can_read_project? def can_read_project?
return false if no_user_or_blocked? return false unless can_access_git?
user.can?(:read_project, project) user.can?(:read_project, project)
end end
private private
def no_user_or_blocked? def can_access_git?
user.nil? || user.blocked? user && user.can?(:access_git)
end end
end end
end end
...@@ -48,6 +48,18 @@ feature 'Login', feature: true do ...@@ -48,6 +48,18 @@ feature 'Login', feature: true do
end end
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 describe 'with two-factor authentication' do
def enter_code(code) def enter_code(code)
fill_in 'user_otp_attempt', with: code fill_in 'user_otp_attempt', with: code
......
require 'spec_helper' require 'spec_helper'
describe Ability, lib: true do 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 describe '.can_edit_note?' do
let(:project) { create(:empty_project) } let(:project) { create(:empty_project) }
let(:note) { create(:note_on_issue, project: project) } let(:note) { create(:note_on_issue, project: project) }
......
...@@ -210,22 +210,6 @@ describe User, models: true do ...@@ -210,22 +210,6 @@ describe User, models: true do
end end
end 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 end
describe "scopes" do describe "scopes" do
......
require 'spec_helper' require 'spec_helper'
describe BasePolicy, models: true do describe BasePolicy, models: true do
let(:build) { Ci::Build.new }
describe '.class_for' do describe '.class_for' do
it 'detects policy class based on the subject ancestors' 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 end
it 'detects policy class for a presented subject' do 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) expect(described_class.class_for(presentee)).to eq(Ci::BuildPolicy)
end end
it 'uses GlobalPolicy when :global is given' do
expect(described_class.class_for(:global)).to eq(GlobalPolicy)
end
end end
end end
...@@ -436,7 +436,7 @@ describe API::Helpers, api: true do ...@@ -436,7 +436,7 @@ describe API::Helpers, api: true do
context 'current_user is present' do context 'current_user is present' do
before 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 end
it 'does not raise an error' do it 'does not raise an error' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment