Fixed cache invalidation bug race condition: Changes to group relation participant are not reflected to the other side and FE updates will be incomplete/outdated.
This commit is contained in:
parent
e5d05349db
commit
5865d94339
10 changed files with 191 additions and 94 deletions
64
app/models/concerns/has_group_relation_definition.rb
Normal file
64
app/models/concerns/has_group_relation_definition.rb
Normal file
|
@ -0,0 +1,64 @@
|
||||||
|
# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
|
||||||
|
module HasGroupRelationDefinition
|
||||||
|
extend ActiveSupport::Concern
|
||||||
|
|
||||||
|
included do
|
||||||
|
|
||||||
|
self.table_name = "groups_#{group_relation_model_identifier}s"
|
||||||
|
self.primary_keys = ref_key, :group_id, :access
|
||||||
|
|
||||||
|
belongs_to group_relation_model_identifier
|
||||||
|
belongs_to :group
|
||||||
|
|
||||||
|
validates :access, presence: true
|
||||||
|
validate :validate_access
|
||||||
|
|
||||||
|
after_save :touch_related
|
||||||
|
after_destroy :touch_related
|
||||||
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
|
||||||
|
def group_relation_instance
|
||||||
|
@group_relation_instance ||= send(group_relation_model_identifier)
|
||||||
|
end
|
||||||
|
|
||||||
|
def group_relation_model_identifier
|
||||||
|
@group_relation_model_identifier ||= self.class.group_relation_model_identifier
|
||||||
|
end
|
||||||
|
|
||||||
|
def touch_related
|
||||||
|
# rubocop:disable Rails/SkipsModelValidations
|
||||||
|
group.touch if group&.persisted?
|
||||||
|
group_relation_instance.touch if group_relation_instance&.persisted?
|
||||||
|
# rubocop:enable Rails/SkipsModelValidations
|
||||||
|
end
|
||||||
|
|
||||||
|
def validate_access
|
||||||
|
query = self.class.where(
|
||||||
|
group_relation_model_identifier => group_relation_instance,
|
||||||
|
group: group
|
||||||
|
)
|
||||||
|
|
||||||
|
query = if access == 'full'
|
||||||
|
query.where.not(access: 'full')
|
||||||
|
else
|
||||||
|
query.where(access: 'full')
|
||||||
|
end
|
||||||
|
|
||||||
|
return if !query.exists?
|
||||||
|
errors.add(:access, "#{group_relation_model_identifier.to_s.capitalize} can have full or granular access to group")
|
||||||
|
end
|
||||||
|
|
||||||
|
# methods defined here are going to extend the class, not the instance of it
|
||||||
|
class_methods do
|
||||||
|
|
||||||
|
def group_relation_model_identifier
|
||||||
|
@group_relation_model_identifier ||= model_name.singular.split('_').first.to_sym
|
||||||
|
end
|
||||||
|
|
||||||
|
def ref_key
|
||||||
|
@ref_key ||= "#{group_relation_model_identifier}_id".to_sym
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
|
@ -7,8 +7,12 @@ module HasGroups
|
||||||
|
|
||||||
attr_accessor :group_access_buffer
|
attr_accessor :group_access_buffer
|
||||||
|
|
||||||
after_create :process_group_access_buffer
|
after_save :process_group_access_buffer
|
||||||
after_update :process_group_access_buffer
|
|
||||||
|
# add association to Group, too but ignore it in asset output
|
||||||
|
Group.has_many group_through_identifier
|
||||||
|
Group.has_many model_name.collection.to_sym, through: group_through_identifier, after_add: :cache_update, after_remove: :cache_update, dependent: :destroy
|
||||||
|
Group.association_attributes_ignored group_through_identifier
|
||||||
|
|
||||||
association_attributes_ignored :groups, group_through_identifier
|
association_attributes_ignored :groups, group_through_identifier
|
||||||
|
|
||||||
|
|
|
@ -6,10 +6,10 @@ class Group < ApplicationModel
|
||||||
include ChecksLatestChangeObserved
|
include ChecksLatestChangeObserved
|
||||||
include HasHistory
|
include HasHistory
|
||||||
|
|
||||||
has_and_belongs_to_many :users, after_add: :cache_update, after_remove: :cache_update
|
belongs_to :email_address
|
||||||
belongs_to :email_address
|
belongs_to :signature
|
||||||
belongs_to :signature
|
|
||||||
validates :name, presence: true
|
validates :name, presence: true
|
||||||
|
|
||||||
activity_stream_permission 'admin.group'
|
activity_stream_permission 'admin.group'
|
||||||
end
|
end
|
||||||
|
|
|
@ -1,13 +1,10 @@
|
||||||
# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
|
# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
|
||||||
|
|
||||||
class RoleGroup < ApplicationModel
|
class RoleGroup < ApplicationModel
|
||||||
self.table_name = 'roles_groups'
|
include HasGroupRelationDefinition
|
||||||
self.primary_keys = :role_id, :group_id, :access
|
|
||||||
belongs_to :role
|
|
||||||
belongs_to :group
|
|
||||||
validates :access, presence: true
|
|
||||||
|
|
||||||
def self.ref_key
|
self.table_name = 'roles_groups'
|
||||||
:role_id
|
|
||||||
end
|
# don't list roles in Group association result
|
||||||
|
Group.association_attributes_ignored :roles
|
||||||
end
|
end
|
||||||
|
|
|
@ -1,41 +1,7 @@
|
||||||
# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
|
# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
|
||||||
|
|
||||||
class UserGroup < ApplicationModel
|
class UserGroup < ApplicationModel
|
||||||
self.table_name = 'groups_users'
|
include HasGroupRelationDefinition
|
||||||
self.primary_keys = :user_id, :group_id, :access
|
|
||||||
belongs_to :user
|
|
||||||
belongs_to :group
|
|
||||||
validates :access, presence: true
|
|
||||||
|
|
||||||
def self.ref_key
|
self.table_name = 'groups_users'
|
||||||
:user_id
|
|
||||||
end
|
|
||||||
|
|
||||||
def cache_update
|
|
||||||
group.cache_update(nil)
|
|
||||||
user.cache_update(nil)
|
|
||||||
super
|
|
||||||
end
|
|
||||||
|
|
||||||
def cache_delete
|
|
||||||
group.cache_update(nil)
|
|
||||||
user.cache_update(nil) if user.present?
|
|
||||||
super
|
|
||||||
end
|
|
||||||
|
|
||||||
private
|
|
||||||
|
|
||||||
def validate_access
|
|
||||||
query = self.class.where(group: group, user: user)
|
|
||||||
|
|
||||||
query = if access == 'full'
|
|
||||||
query.where.not(access: 'full')
|
|
||||||
else
|
|
||||||
query.where(access: 'full')
|
|
||||||
end
|
|
||||||
|
|
||||||
errors.add(:access, 'User can have full or granular access to group') if query.exists?
|
|
||||||
end
|
|
||||||
|
|
||||||
validate :validate_access
|
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,62 @@
|
||||||
|
# Requires: let!(:group_relation_instance) { ... }
|
||||||
|
RSpec.shared_examples 'HasGroupRelationDefinition' do
|
||||||
|
|
||||||
|
let(:group_relation_model_key) { group_relation_instance.model_name.element }
|
||||||
|
|
||||||
|
context 'relation creation' do
|
||||||
|
|
||||||
|
it 'refreshes updated_at of related instances' do
|
||||||
|
group = create(:group)
|
||||||
|
|
||||||
|
travel 1.minute
|
||||||
|
|
||||||
|
expect do
|
||||||
|
described_class.create!(
|
||||||
|
group_relation_model_key => group_relation_instance,
|
||||||
|
group: group
|
||||||
|
)
|
||||||
|
end.to change {
|
||||||
|
group.reload.updated_at
|
||||||
|
}.and change {
|
||||||
|
group_relation_instance.reload.updated_at
|
||||||
|
}
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'related instance deletion' do
|
||||||
|
|
||||||
|
it 'refreshes updated_at of group instance' do
|
||||||
|
group = create(:group)
|
||||||
|
|
||||||
|
described_class.create!(
|
||||||
|
group_relation_model_key => group_relation_instance,
|
||||||
|
group: group
|
||||||
|
)
|
||||||
|
|
||||||
|
travel 1.minute
|
||||||
|
|
||||||
|
expect do
|
||||||
|
group.destroy
|
||||||
|
end.to change {
|
||||||
|
group_relation_instance.reload.updated_at
|
||||||
|
}
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'refreshes updated_at of relation instance' do
|
||||||
|
group = create(:group)
|
||||||
|
|
||||||
|
described_class.create!(
|
||||||
|
group_relation_model_key => group_relation_instance,
|
||||||
|
group: group
|
||||||
|
)
|
||||||
|
|
||||||
|
travel 1.minute
|
||||||
|
|
||||||
|
expect do
|
||||||
|
group_relation_instance.destroy
|
||||||
|
end.to change {
|
||||||
|
group.reload.updated_at
|
||||||
|
}
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
|
@ -242,6 +242,24 @@ RSpec.shared_examples 'HasGroups' do
|
||||||
described_class.group_through.klass.count
|
described_class.group_through.klass.count
|
||||||
}.by(-3)
|
}.by(-3)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'prevents having full and other privilege at the same time' do
|
||||||
|
|
||||||
|
invalid_combination = %w[full read change]
|
||||||
|
exception = ActiveRecord::RecordInvalid
|
||||||
|
|
||||||
|
expect do
|
||||||
|
group_access_instance.group_names_access_map = {
|
||||||
|
group_full.name => invalid_combination,
|
||||||
|
}
|
||||||
|
end.to raise_error(exception)
|
||||||
|
|
||||||
|
expect do
|
||||||
|
group_access_instance.group_names_access_map = {
|
||||||
|
group_full.name => invalid_combination.reverse,
|
||||||
|
}
|
||||||
|
end.to raise_error(exception)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'new instance' do
|
context 'new instance' do
|
||||||
|
|
21
spec/models/role_group_spec.rb
Normal file
21
spec/models/role_group_spec.rb
Normal file
|
@ -0,0 +1,21 @@
|
||||||
|
require 'rails_helper'
|
||||||
|
require 'models/concerns/has_group_relation_definition_examples'
|
||||||
|
|
||||||
|
RSpec.describe RoleGroup do
|
||||||
|
|
||||||
|
let!(:group_relation_instance) { create(:role) }
|
||||||
|
|
||||||
|
include_examples 'HasGroupRelationDefinition'
|
||||||
|
|
||||||
|
it 'prevents roles from beeing in Group assets' do
|
||||||
|
|
||||||
|
group = create(:group)
|
||||||
|
|
||||||
|
described_class.create!(
|
||||||
|
group: group,
|
||||||
|
role: create(:role)
|
||||||
|
)
|
||||||
|
expect(group.assets({})[:Group][group.id]).not_to include('role_ids')
|
||||||
|
end
|
||||||
|
|
||||||
|
end
|
9
spec/models/user_group_spec.rb
Normal file
9
spec/models/user_group_spec.rb
Normal file
|
@ -0,0 +1,9 @@
|
||||||
|
require 'rails_helper'
|
||||||
|
require 'models/concerns/has_group_relation_definition_examples'
|
||||||
|
|
||||||
|
RSpec.describe UserGroup do
|
||||||
|
|
||||||
|
let!(:group_relation_instance) { create(:agent_user) }
|
||||||
|
|
||||||
|
include_examples 'HasGroupRelationDefinition'
|
||||||
|
end
|
|
@ -1,44 +0,0 @@
|
||||||
require 'test_helper'
|
|
||||||
|
|
||||||
class UserGroupTest < ActiveSupport::TestCase
|
|
||||||
test 'user group permissions' do
|
|
||||||
rand = rand(9_999_999_999)
|
|
||||||
agent1 = User.create!(
|
|
||||||
login: "agent-permission-check#{rand}@example.com",
|
|
||||||
firstname: 'vaild_agent_group_permission-1',
|
|
||||||
lastname: 'Agent',
|
|
||||||
email: "agent-permission-check#{rand}@example.com",
|
|
||||||
password: 'agentpw',
|
|
||||||
active: true,
|
|
||||||
roles: Role.where(name: 'Agent'),
|
|
||||||
groups: Group.all,
|
|
||||||
updated_by_id: 1,
|
|
||||||
created_by_id: 1,
|
|
||||||
)
|
|
||||||
|
|
||||||
group1 = Group.create!(
|
|
||||||
name: "GroupPermissionsTest-#{rand(9_999_999_999)}",
|
|
||||||
active: true,
|
|
||||||
updated_by_id: 1,
|
|
||||||
created_by_id: 1,
|
|
||||||
)
|
|
||||||
|
|
||||||
assert_nothing_raised do
|
|
||||||
UserGroup.create!(user: agent1, group: group1, access: 'full')
|
|
||||||
end
|
|
||||||
|
|
||||||
assert_raises do
|
|
||||||
UserGroup.create!(user: agent1, group: group1, access: 'read')
|
|
||||||
end
|
|
||||||
|
|
||||||
UserGroup.where(user: agent1, group: group1).destroy_all
|
|
||||||
|
|
||||||
assert_nothing_raised do
|
|
||||||
UserGroup.create!(user: agent1, group: group1, access: 'read')
|
|
||||||
end
|
|
||||||
|
|
||||||
assert_raises do
|
|
||||||
UserGroup.create!(user: agent1, group: group1, access: 'full')
|
|
||||||
end
|
|
||||||
end
|
|
||||||
end
|
|
Loading…
Reference in a new issue