fix: Macros authorizations (#5779)

Macros policy update.

ref: #5730

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

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)

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

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

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

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|

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

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