From 80b330d73f2f58d4e72a1f72450e350b0b68539d Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Tue, 7 Jul 2020 13:14:58 +0200 Subject: [PATCH] Refactoring: Clarify complex Permission queries This commit was prepared to facilitate a larger refactoring assignment as part of the Pundit migration. As a bonus, it includes some SQL query optimizations! === Why was this refactoring necessary? It's not, strictly speaking. But the Pundit migration involves taking complex querying logic and moving it into Scope classes as appropriate, and deciding where things belong is really difficult when you can't see what they're doing. === So how does this refactoring fix the problem? * It replaces raw SQL queries with Ruby-esque ActiveRecord queries. * It replaces complex, procedural code that's full of loops and obscure local variable assignment with compact, cleanly-formatted code that follows Ruby idioms and uses meaningful variable names. In my opinion, it's much faster and easier to understand what the code does this way. === What kinds of SQL query optimizations are included? * n+1 query: user_access_token#index instantiated all active permissions and then called current_user.permissions? on _every single one._ A fresh installation of Zammad contains 57 permissions, so this was a lot of unnecessary queries. The method has been rewritten to make only one query instead. * User#permissions? used to query the DB once for each argument it was given. Now, it only queries the DB once, even when given many arguments. * We had a couple SQL queries that used both #select and #pluck. (When using #pluck, #select is redundant.) Removing #select from these calls did not improve performance, but it did clean up unnecessary code. --- .../user_access_token_controller.rb | 48 +++++++------------ app/models/user.rb | 44 ++++++++--------- spec/models/user_spec.rb | 46 ++++++++++++++++++ 3 files changed, 85 insertions(+), 53 deletions(-) diff --git a/app/controllers/user_access_token_controller.rb b/app/controllers/user_access_token_controller.rb index 224716014..56d0d4077 100644 --- a/app/controllers/user_access_token_controller.rb +++ b/app/controllers/user_access_token_controller.rb @@ -27,42 +27,28 @@ curl http://localhost/api/v1/user_access_token -v -u #{login}:#{password} =end def index - tokens = Token.where(action: 'api', persistent: true, user_id: current_user.id).order(updated_at: :desc, label: :asc) - token_list = [] - tokens.each do |token| - attributes = token.attributes - attributes.delete('persistent') - attributes.delete('name') - token_list.push attributes - end - local_permissions = current_user.permissions - local_permissions_new = {} - local_permissions.each_key do |key| - keys = ::Permission.with_parents(key) - keys.each do |local_key| - next if local_permissions_new.key?([local_key]) + tokens = Token.select(Token.column_names - %w[persistent name]) + .where(action: 'api', persistent: true, user_id: current_user.id) + .order(updated_at: :desc, label: :asc) - if local_permissions[local_key] == true - local_permissions_new[local_key] = true - next - end - local_permissions_new[local_key] = false - end - end - permissions = [] - Permission.all.where(active: true).order(:name).each do |permission| - next if !local_permissions_new.key?(permission.name) && !current_user.permissions?(permission.name) + base_query = Permission.order(:name).where(active: true) + permission_names = current_user.permissions.keys + ancestor_names = permission_names.flat_map { |name| Permission.with_parents(name) }.uniq - + permission_names + descendant_names = permission_names.map { |name| "#{name}.%" } - permission_attributes = permission.attributes - if local_permissions_new[permission.name] == false - permission_attributes['preferences']['disabled'] = true - end - permissions.push permission_attributes + permissions = base_query.where(name: [*ancestor_names, *permission_names]) + + descendant_names.each do |name| + permissions = permissions.or(base_query.where('permissions.name LIKE ?', name)) end + permissions.select { |permission| permission.name.in?(ancestor_names) } + .each { |permission| permission.preferences['disabled'] = true } + render json: { - tokens: token_list, - permissions: permissions, + tokens: tokens.map(&:attributes), + permissions: permissions.map(&:attributes), }, status: :ok end diff --git a/app/models/user.rb b/app/models/user.rb index d4076a9ed..b60157594 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -392,13 +392,12 @@ returns =end def permissions - list = {} - ::Permission.select('permissions.name, permissions.preferences').joins(:roles).where('roles.id IN (?) AND permissions.active = ?', role_ids, true).pluck(:name, :preferences).each do |permission| - next if permission[1]['selectable'] == false - - list[permission[0]] = true - end - list + ::Permission.joins(roles: :users) + .where(users: { id: id }, roles: { active: true }, active: true) + .pluck(:name, :preferences) + .each_with_object({}) do |(name, preferences), hash| + hash[name] = true if preferences['selectable'] != false + end end =begin @@ -419,23 +418,24 @@ returns =end - def permissions?(key) - Array(key).each do |local_key| - list = [] - if local_key.end_with?('.*') - local_key = local_key.sub('.*', '.%') - permissions = ::Permission.with_parents(local_key) - list = ::Permission.select('preferences').joins(:roles).where('roles.id IN (?) AND roles.active = ? AND (permissions.name IN (?) OR permissions.name LIKE ?) AND permissions.active = ?', role_ids, true, permissions, local_key, true).pluck(:preferences) - else - permission = ::Permission.lookup(name: local_key) - break if permission&.active == false + def permissions?(names) + base_query = ::Permission.joins(roles: :users) + .where(users: { id: id }) + .where(roles: { active: true }) + .where(active: true) - permissions = ::Permission.with_parents(local_key) - list = ::Permission.select('preferences').joins(:roles).where('roles.id IN (?) AND roles.active = ? AND permissions.name IN (?) AND permissions.active = ?', role_ids, true, permissions, true).pluck(:preferences) - end - return true if list.present? + permission_names = Array(names).reject { |name| ::Permission.lookup(name: name)&.active == false } + verbatim_names = permission_names.flat_map { |name| ::Permission.with_parents(name) }.uniq + wildcard_names = permission_names.select { |name| name.end_with?('.*') } + .map { |name| name.sub('.*', '.%') } + + permissions = base_query.where(name: verbatim_names) + + wildcard_names.each do |name| + permissions = permissions.or(base_query.where('permissions.name LIKE ?', name)) end - false + + permissions.exists? end =begin diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 784d3653a..6a84c691c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -369,6 +369,52 @@ RSpec.describe User, type: :model do end end + describe '#permissions' do + let(:user) { create(:agent).tap { |u| u.roles = [u.roles.first] } } + let(:role) { user.roles.first } + let(:permissions) { role.permissions } + + it 'returns a hash of => true' do + expect(user.permissions).to eq(permissions.each.with_object({}) { |p, hash| hash[p.name] = true }) + end + + shared_examples 'for omissions' do + it 'omits them from the returned hash' do + expect(user.permissions.keys).not_to include(*omitted_permissions.map(&:name)) + end + end + + context 'for permissions that do not belong to this user' do + let(:omitted_permissions) { Permission.all - permissions } + + include_examples 'for omissions' + end + + context 'for inactive permissions' do + before { omitted_permissions.each { |p| p.update(active: false) } } + + let!(:omitted_permissions) { permissions.first(1) } + + include_examples 'for omissions' + end + + context 'for permissions on inactive roles' do + before { role.update(active: false) } + + let(:omitted_permissions) { permissions } + + include_examples 'for omissions' + end + + context 'for permissions with !preferences["selectable"]' do + before { omitted_permissions.each { |p| p.update(preferences: { selectable: false }) } } + + let(:omitted_permissions) { permissions.first(1) } + + include_examples 'for omissions' + end + end + describe '#permissions?' do subject(:user) { create(:user, roles: [role]) }