Commit cdc87fed authored by Timothy Andrew's avatar Timothy Andrew

Implement backend review comments from @DouweM.

Mainly related to increasing compatibility with CE, and trying to avoid merge
conflicts.

1. Create an `EE::AuditorUser` module with auditor-specific methods. Mixed into
   the `User` model.

2. Create an `EE::User` module with EE-specific user methods. Mixed into the
   `User` model.

3. Don't block creation of regular users when the auditor addon is disabled (bug
   in original implementation).
parent 4f022d9a
module AuditorUserHelper
def license_allows_auditor_user?
@license_allows_auditor_user ||= (::License.current && ::License.current.add_on?('GitLab_Auditor_User'))
end
end
......@@ -7,10 +7,6 @@ module PathLocksHelper
@license_allows_file_locks ||= (::License.current && ::License.current.add_on?('GitLab_FileLocks'))
end
def license_allows_auditor_user?
@license_allows_auditor_user ||= (::License.current && ::License.current.add_on?('GitLab_Auditor_User'))
end
def text_label_for_lock(file_lock, path)
if file_lock.path == path
"Locked by #{file_lock.user.name}"
......
module EE
# AuditorUser EE mixin
#
# This module is intended to encapsulate EE-specific model logic
# related to auditor (readonly access to all projects) users. It
# is prepended to the `User` model.
module AuditorUser
extend ActiveSupport::Concern
include AuditorUserHelper
included do
# We aren't using the `auditor?` method for the `if` condition here
# because `auditor?` returns `false` when the `auditor` column is `true`
# and the auditor add-on absent. We want to run this validation
# regardless of the add-on's presence, so we need to check the `auditor`
# column directly.
validate :auditor_requires_license_add_on, if: :auditor
validate :cannot_be_admin_and_auditor
end
def cannot_be_admin_and_auditor
if admin? && auditor?
errors.add(:admin, "user cannot also be an Auditor.")
end
end
def auditor_requires_license_add_on
unless license_allows_auditor_user?
errors.add(:auditor, 'user cannot be created without the "GitLab_Auditor_User" addon')
end
end
def auditor?
license_allows_auditor_user? && self.auditor
end
def admin_or_auditor?
admin? || auditor?
end
end
end
module EE
# User EE mixin
#
# This module is intended to encapsulate EE-specific model logic
# and be prepended in the `User` model
module User
extend ActiveSupport::Concern
def access_level
if admin?
:admin
elsif auditor?
:auditor
else
:regular
end
end
def access_level=(new_level)
# new_level can be a symbol or a string
new_level = new_level.to_s
self.admin = (new_level == :admin)
self.auditor = (new_level == :auditor)
end
end
end
......@@ -10,6 +10,8 @@ class User < ActiveRecord::Base
include CaseSensitivity
include TokenAuthenticatable
prepend EE::GeoAwareAvatar
prepend EE::User
prepend EE::AuditorUser
DEFAULT_NOTIFICATION_LEVEL = :participating
......@@ -123,8 +125,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 :cannot_be_admin_and_auditor
validate :auditor_requires_license_add_on
validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
before_validation :generate_password, on: :create
......@@ -455,18 +455,6 @@ class User < ActiveRecord::Base
end
end
def cannot_be_admin_and_auditor
if admin? && auditor?
errors.add(:admin, "user cannot also be an Auditor.")
end
end
def auditor_requires_license_add_on
unless ::License.current && ::License.current.add_on?('GitLab_Auditor_User')
errors.add(:auditor, 'user cannot be created without the "GitLab_Auditor_User" addon')
end
end
# Returns the groups a user has access to
def authorized_groups
union = Gitlab::SQL::Union.
......@@ -545,16 +533,6 @@ class User < ActiveRecord::Base
admin
end
def auditor?
@license_allows_auditors ||= (::License.current && ::License.current.add_on?('GitLab_Auditor_User'))
@license_allows_auditors && self.auditor
end
def admin_or_auditor?
admin? || auditor?
end
def require_ssh_key?
keys.count == 0 && Gitlab::ProtocolAccess.allowed?('ssh')
end
......@@ -957,23 +935,6 @@ class User < ActiveRecord::Base
Gitlab::UserActivities::ActivitySet.record(self)
end
def access_level
if admin?
:admin
elsif auditor?
:auditor
else
:regular
end
end
def access_level=(new_level)
# new_level can be a symbol or a string
new_level = new_level.to_s
self.admin = (new_level == :admin)
self.auditor = (new_level == :auditor)
end
private
def ci_projects_union
......
......@@ -276,8 +276,8 @@ class ProjectPolicy < BasePolicy
private
# A base set of abilities for read-only users, which
# is then augmented as necessary for anonymous and auditor
# users.
# is then augmented as necessary for anonymous and other
# read-only users.
def base_readonly_access!
can! :read_project
can! :read_board
......
......@@ -1512,6 +1512,12 @@ describe User, models: true do
expect(build(:user, :auditor)).to be_valid
end
it "allows creating a regular user if the addon isn't enabled" do
allow_any_instance_of(License).to receive(:add_ons).and_return({})
expect(build(:user)).to be_valid
end
end
context '#auditor?' 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