From 5865d94339ec84457bd899f86dafd78aa75ca0e5 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Mon, 25 Jun 2018 12:35:57 +0200 Subject: [PATCH] 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. --- .../concerns/has_group_relation_definition.rb | 64 +++++++++++++++++++ app/models/concerns/has_groups.rb | 8 ++- app/models/group.rb | 8 +-- app/models/role_group.rb | 13 ++-- app/models/user_group.rb | 38 +---------- .../has_group_relation_definition_examples.rb | 62 ++++++++++++++++++ spec/models/concerns/has_groups_examples.rb | 18 ++++++ spec/models/role_group_spec.rb | 21 ++++++ spec/models/user_group_spec.rb | 9 +++ test/unit/user_group_test.rb | 44 ------------- 10 files changed, 191 insertions(+), 94 deletions(-) create mode 100644 app/models/concerns/has_group_relation_definition.rb create mode 100644 spec/models/concerns/has_group_relation_definition_examples.rb create mode 100644 spec/models/role_group_spec.rb create mode 100644 spec/models/user_group_spec.rb delete mode 100644 test/unit/user_group_test.rb diff --git a/app/models/concerns/has_group_relation_definition.rb b/app/models/concerns/has_group_relation_definition.rb new file mode 100644 index 000000000..1323c167a --- /dev/null +++ b/app/models/concerns/has_group_relation_definition.rb @@ -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 diff --git a/app/models/concerns/has_groups.rb b/app/models/concerns/has_groups.rb index b04930570..d65572c2e 100644 --- a/app/models/concerns/has_groups.rb +++ b/app/models/concerns/has_groups.rb @@ -7,8 +7,12 @@ module HasGroups attr_accessor :group_access_buffer - after_create :process_group_access_buffer - after_update :process_group_access_buffer + after_save :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 diff --git a/app/models/group.rb b/app/models/group.rb index 96a563e56..67ec0041f 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -6,10 +6,10 @@ class Group < ApplicationModel include ChecksLatestChangeObserved include HasHistory - has_and_belongs_to_many :users, after_add: :cache_update, after_remove: :cache_update - belongs_to :email_address - belongs_to :signature - validates :name, presence: true + belongs_to :email_address + belongs_to :signature + + validates :name, presence: true activity_stream_permission 'admin.group' end diff --git a/app/models/role_group.rb b/app/models/role_group.rb index c684af14a..67052f4b9 100644 --- a/app/models/role_group.rb +++ b/app/models/role_group.rb @@ -1,13 +1,10 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class RoleGroup < ApplicationModel - self.table_name = 'roles_groups' - self.primary_keys = :role_id, :group_id, :access - belongs_to :role - belongs_to :group - validates :access, presence: true + include HasGroupRelationDefinition - def self.ref_key - :role_id - end + self.table_name = 'roles_groups' + + # don't list roles in Group association result + Group.association_attributes_ignored :roles end diff --git a/app/models/user_group.rb b/app/models/user_group.rb index b1d473764..ce6995e2c 100644 --- a/app/models/user_group.rb +++ b/app/models/user_group.rb @@ -1,41 +1,7 @@ # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/ class UserGroup < ApplicationModel - self.table_name = 'groups_users' - self.primary_keys = :user_id, :group_id, :access - belongs_to :user - belongs_to :group - validates :access, presence: true + include HasGroupRelationDefinition - def self.ref_key - :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 + self.table_name = 'groups_users' end diff --git a/spec/models/concerns/has_group_relation_definition_examples.rb b/spec/models/concerns/has_group_relation_definition_examples.rb new file mode 100644 index 000000000..86d1e49ab --- /dev/null +++ b/spec/models/concerns/has_group_relation_definition_examples.rb @@ -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 diff --git a/spec/models/concerns/has_groups_examples.rb b/spec/models/concerns/has_groups_examples.rb index 3b2ddaee2..c79f5eaea 100644 --- a/spec/models/concerns/has_groups_examples.rb +++ b/spec/models/concerns/has_groups_examples.rb @@ -242,6 +242,24 @@ RSpec.shared_examples 'HasGroups' do described_class.group_through.klass.count }.by(-3) 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 context 'new instance' do diff --git a/spec/models/role_group_spec.rb b/spec/models/role_group_spec.rb new file mode 100644 index 000000000..cc924c7f2 --- /dev/null +++ b/spec/models/role_group_spec.rb @@ -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 diff --git a/spec/models/user_group_spec.rb b/spec/models/user_group_spec.rb new file mode 100644 index 000000000..7dd7d2499 --- /dev/null +++ b/spec/models/user_group_spec.rb @@ -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 diff --git a/test/unit/user_group_test.rb b/test/unit/user_group_test.rb deleted file mode 100644 index d7a98fc92..000000000 --- a/test/unit/user_group_test.rb +++ /dev/null @@ -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