From 48373628a1721e1a484e1c6ef905b66786ef0b43 Mon Sep 17 00:00:00 2001 From: Tejaswini Chile Date: Tue, 8 Nov 2022 07:16:00 +0530 Subject: [PATCH] fix: Macros authorizations (#5779) Macros policy update. ref: #5730 --- .../api/v1/accounts/macros_controller.rb | 8 +- app/models/macro.rb | 19 ++-- app/models/user.rb | 22 ++-- app/policies/macro_policy.rb | 20 +++- ...082737_change_macros_created_by_to_null.rb | 9 ++ db/schema.rb | 8 +- .../api/v1/accounts/macros_controller_spec.rb | 101 +++++++++++++++++- spec/models/macro_spec.rb | 28 ++++- 8 files changed, 177 insertions(+), 38 deletions(-) create mode 100644 db/migrate/20221102082737_change_macros_created_by_to_null.rb diff --git a/app/controllers/api/v1/accounts/macros_controller.rb b/app/controllers/api/v1/accounts/macros_controller.rb index 3e812ed63..a13d74995 100644 --- a/app/controllers/api/v1/accounts/macros_controller.rb +++ b/app/controllers/api/v1/accounts/macros_controller.rb @@ -1,6 +1,6 @@ class Api::V1::Accounts::MacrosController < Api::V1::Accounts::BaseController - before_action :check_authorization before_action :fetch_macro, only: [:show, :update, :destroy, :execute] + before_action :check_authorization, only: [:show, :update, :destroy, :execute] def index @macros = Macro.with_visibility(current_user, params) @@ -55,6 +55,8 @@ class Api::V1::Accounts::MacrosController < Api::V1::Accounts::BaseController head :ok end + private + def process_attachments actions = @macro.actions.filter_map { |k, _v| k if k['action_name'] == 'send_attachment' } return if actions.blank? @@ -80,4 +82,8 @@ class Api::V1::Accounts::MacrosController < Api::V1::Accounts::BaseController def fetch_macro @macro = Current.account.macros.find_by(id: params[:id]) end + + def check_authorization + authorize(@macro) if @macro.present? + end end diff --git a/app/models/macro.rb b/app/models/macro.rb index 16affee09..0eeb5b458 100644 --- a/app/models/macro.rb +++ b/app/models/macro.rb @@ -9,23 +9,21 @@ # created_at :datetime not null # updated_at :datetime not null # account_id :bigint not null -# created_by_id :bigint not null -# updated_by_id :bigint not null +# created_by_id :bigint +# updated_by_id :bigint # # Indexes # -# index_macros_on_account_id (account_id) -# index_macros_on_created_by_id (created_by_id) -# index_macros_on_updated_by_id (updated_by_id) +# index_macros_on_account_id (account_id) # class Macro < ApplicationRecord include Rails.application.routes.url_helpers belongs_to :account belongs_to :created_by, - class_name: :User + class_name: :User, optional: true belongs_to :updated_by, - class_name: :User + class_name: :User, optional: true has_many_attached :files enum visibility: { personal: 0, global: 1 } @@ -41,10 +39,9 @@ class Macro < ApplicationRecord end def self.with_visibility(user, params) - records = user.administrator? ? Current.account.macros : Current.account.macros.global - records = records.or(personal.where(created_by_id: user.id)) if user.agent? - records.page(current_page(params)) - records + records = Current.account.macros.global + records = records.or(personal.where(created_by_id: user.id)) + records.order(:id).page(current_page(params)) end def self.current_page(params) diff --git a/app/models/user.rb b/app/models/user.rb index 743471049..3094990d5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -92,17 +92,13 @@ class User < ApplicationRecord has_many :team_members, dependent: :destroy_async has_many :teams, through: :team_members has_many :articles, foreign_key: 'author_id', dependent: :nullify - has_many :portal_members, - class_name: :PortalMember, - dependent: :destroy_async - has_many :portals, - through: :portal_members, - class_name: :Portal, - dependent: :nullify, - source: :portal - has_many :macros, foreign_key: 'created_by_id', dependent: :destroy_async - + has_many :portal_members, class_name: :PortalMember, dependent: :destroy_async + has_many :portals, through: :portal_members, source: :portal, + class_name: :Portal, + dependent: :nullify + has_many :macros, foreign_key: 'created_by_id' before_validation :set_password_and_uid, on: :create + after_destroy :remove_macros scope :order_by_full_name, -> { order('lower(name) ASC') } @@ -205,4 +201,10 @@ class User < ApplicationRecord count: notifications.where(account_id: account_id).count } end + + private + + def remove_macros + macros.personal.destroy_all + end end diff --git a/app/policies/macro_policy.rb b/app/policies/macro_policy.rb index 3c67424a5..4febc0ff7 100644 --- a/app/policies/macro_policy.rb +++ b/app/policies/macro_policy.rb @@ -8,22 +8,34 @@ class MacroPolicy < ApplicationPolicy end def show? - true + @record.global? || author? end def update? - true + author? || (@account_user.administrator? && @record.global?) end def destroy? - true + author? || orphan_record? end def execute? - true + @record.global? || author? end def attach_file? true end + + private + + def author? + @record.created_by == @account_user.user + end + + def orphan_record? + return @account_user.administrator? if @record.created_by.nil? && @record.global? + + false + end end diff --git a/db/migrate/20221102082737_change_macros_created_by_to_null.rb b/db/migrate/20221102082737_change_macros_created_by_to_null.rb new file mode 100644 index 000000000..f3f318028 --- /dev/null +++ b/db/migrate/20221102082737_change_macros_created_by_to_null.rb @@ -0,0 +1,9 @@ +class ChangeMacrosCreatedByToNull < ActiveRecord::Migration[6.1] + def change + change_column_null :macros, :created_by_id, true + change_column_null :macros, :updated_by_id, true + + remove_index :macros, :created_by_id, if_exists: true + remove_index :macros, :updated_by_id, if_exists: true + end +end diff --git a/db/schema.rb b/db/schema.rb index d989b754b..b086ef781 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_10_17_201914) do +ActiveRecord::Schema.define(version: 2022_11_02_082737) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" @@ -577,14 +577,12 @@ ActiveRecord::Schema.define(version: 2022_10_17_201914) do t.bigint "account_id", null: false t.string "name", null: false t.integer "visibility", default: 0 - t.bigint "created_by_id", null: false - t.bigint "updated_by_id", null: false + t.bigint "created_by_id" + t.bigint "updated_by_id" t.jsonb "actions", default: {}, null: false t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false t.index ["account_id"], name: "index_macros_on_account_id" - t.index ["created_by_id"], name: "index_macros_on_created_by_id" - t.index ["updated_by_id"], name: "index_macros_on_updated_by_id" end create_table "mentions", force: :cascade do |t| diff --git a/spec/controllers/api/v1/accounts/macros_controller_spec.rb b/spec/controllers/api/v1/accounts/macros_controller_spec.rb index c34cf3a86..26bff7b60 100644 --- a/spec/controllers/api/v1/accounts/macros_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/macros_controller_spec.rb @@ -23,14 +23,15 @@ RSpec.describe 'Api::V1::Accounts::MacrosController', type: :request do get "/api/v1/accounts/#{account.id}/macros", headers: administrator.create_new_auth_token - visible_macros = account.macros + visible_macros = account.macros.global.or(account.macros.personal.where(created_by_id: administrator.id)).order(:id) expect(response).to have_http_status(:success) body = JSON.parse(response.body) expect(body['payload'].length).to eq(visible_macros.count) - expect(body['payload'].first['id']).to eq(Macro.first.id) - expect(body['payload'].last['id']).to eq(Macro.last.id) + + expect(body['payload'].first['id']).to eq(visible_macros.first.id) + expect(body['payload'].last['id']).to eq(visible_macros.last.id) end end @@ -42,7 +43,7 @@ RSpec.describe 'Api::V1::Accounts::MacrosController', type: :request do expect(response).to have_http_status(:success) body = JSON.parse(response.body) - visible_macros = account.macros.global.or(account.macros.personal.where(created_by_id: agent.id)) + visible_macros = account.macros.global.or(account.macros.personal.where(created_by_id: agent.id)).order(:id) expect(body['payload'].length).to eq(visible_macros.count) expect(body['payload'].first['id']).to eq(visible_macros.first.id) @@ -181,6 +182,19 @@ RSpec.describe 'Api::V1::Accounts::MacrosController', type: :request do json_response = JSON.parse(response.body) expect(json_response['name']).to eql(params['name']) end + + it 'Unauthorize to update the macro' do + macro = create(:macro, account: account, created_by: agent, updated_by: agent) + + put "/api/v1/accounts/#{account.id}/macros/#{macro.id}", + params: params, + headers: agent_1.create_new_auth_token + + json_response = JSON.parse(response.body) + + expect(response).to have_http_status(:unauthorized) + expect(json_response['error']).to eq('You are not authorized to do this action') + end end end @@ -214,6 +228,27 @@ RSpec.describe 'Api::V1::Accounts::MacrosController', type: :request do expect(response).to have_http_status(:not_found) end + + it 'Unauthorize to fetch other agents private macro' do + macro = create(:macro, account: account, created_by: agent, updated_by: agent, visibility: :personal) + + get "/api/v1/accounts/#{account.id}/macros/#{macro.id}", + headers: agent_1.create_new_auth_token + + json_response = JSON.parse(response.body) + + expect(response).to have_http_status(:unauthorized) + expect(json_response['error']).to eq('You are not authorized to do this action') + end + + it 'authorize to fetch other agents public macro' do + macro = create(:macro, account: account, created_by: agent, updated_by: agent, visibility: :global) + + get "/api/v1/accounts/#{account.id}/macros/#{macro.id}", + headers: agent_1.create_new_auth_token + + expect(response).to have_http_status(:success) + end end end @@ -314,4 +349,62 @@ RSpec.describe 'Api::V1::Accounts::MacrosController', type: :request do end end end + + describe 'DELETE /api/v1/accounts/{account.id}/macros/{macro.id}' do + let!(:macro) { create(:macro, account: account, created_by: administrator, updated_by: administrator) } + + context 'when it is an authenticated user' do + it 'Deletes the macro' do + delete "/api/v1/accounts/#{account.id}/macros/#{macro.id}", + headers: administrator.create_new_auth_token + + expect(response).to have_http_status(:success) + end + + it 'deletes the orphan public record with admin credentials' do + macro = create(:macro, account: account, created_by: agent, updated_by: agent, visibility: :global) + + expect(macro.created_by).to eq(agent) + + agent.destroy! + + expect(macro.reload.created_by).to be_nil + + delete "/api/v1/accounts/#{account.id}/macros/#{macro.id}", + headers: administrator.create_new_auth_token + + expect(response).to have_http_status(:success) + end + + it 'can not delete orphan public record with agent credentials' do + macro = create(:macro, account: account, created_by: agent, updated_by: agent, visibility: :global) + + expect(macro.created_by).to eq(agent) + + agent.destroy! + + expect(macro.reload.created_by).to be_nil + + delete "/api/v1/accounts/#{account.id}/macros/#{macro.id}", + headers: agent_1.create_new_auth_token + + json_response = JSON.parse(response.body) + + expect(response).to have_http_status(:unauthorized) + expect(json_response['error']).to eq('You are not authorized to do this action') + end + + it 'Unauthorize to delete the macro' do + macro = create(:macro, account: account, created_by: agent, updated_by: agent) + + delete "/api/v1/accounts/#{account.id}/macros/#{macro.id}", + headers: agent_1.create_new_auth_token + + json_response = JSON.parse(response.body) + + expect(response).to have_http_status(:unauthorized) + expect(json_response['error']).to eq('You are not authorized to do this action') + end + end + end end diff --git a/spec/models/macro_spec.rb b/spec/models/macro_spec.rb index 2879e38ae..ee91b2b93 100644 --- a/spec/models/macro_spec.rb +++ b/spec/models/macro_spec.rb @@ -6,8 +6,6 @@ RSpec.describe Macro, type: :model do describe 'associations' do it { is_expected.to belong_to(:account) } - it { is_expected.to belong_to(:created_by) } - it { is_expected.to belong_to(:updated_by) } end describe 'validations' do @@ -71,7 +69,9 @@ RSpec.describe Macro, type: :model do Current.user = admin Current.account = account - expect(described_class.with_visibility(admin, {}).count).to eq(account.macros.count) + macros = account.macros.global.or(account.macros.personal.where(created_by_id: admin.id)) + + expect(described_class.with_visibility(admin, {}).count).to eq(macros.count) end end @@ -90,4 +90,26 @@ RSpec.describe Macro, type: :model do end end end + + describe '#associations' do + let(:agent) { create(:user, account: account, role: :agent) } + let!(:global_macro) { FactoryBot.create(:macro, account: account, created_by: agent, updated_by: agent, visibility: :global, actions: []) } + let!(:personal_macro) { FactoryBot.create(:macro, account: account, created_by: agent, updated_by: agent, visibility: :personal, actions: []) } + + context 'when you delete the author' do + it 'nullify the created_by column' do + expect(global_macro.created_by).to eq(agent) + expect(global_macro.updated_by).to eq(agent) + expect(personal_macro.created_by).to eq(agent) + expect(personal_macro.updated_by).to eq(agent) + + personal_macro_id = personal_macro.id + agent.destroy! + + expect(global_macro.reload.created_by).to be_nil + expect(global_macro.reload.updated_by).to be_nil + expect(described_class.find_by(id: personal_macro_id)).to be_nil + end + end + end end