From 602c458ab6773e56e512c032c16fe4c7cddc1c44 Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 22 Jun 2023 14:52:25 +0200 Subject: [PATCH] Add finer permission requirements for managing webhooks (#25463) --- app/controllers/admin/webhooks_controller.rb | 3 +++ app/models/webhook.rb | 22 +++++++++++++++++++ app/policies/webhook_policy.rb | 4 ++-- app/views/admin/webhooks/_form.html.haml | 2 +- config/locales/activerecord.en.yml | 4 ++++ .../admin/webhooks_controller_spec.rb | 2 +- spec/policies/webhook_policy_spec.rb | 22 ++++++++++++++++--- 7 files changed, 52 insertions(+), 7 deletions(-) diff --git a/app/controllers/admin/webhooks_controller.rb b/app/controllers/admin/webhooks_controller.rb index e087476658..f1aad7c4b5 100644 --- a/app/controllers/admin/webhooks_controller.rb +++ b/app/controllers/admin/webhooks_controller.rb @@ -28,6 +28,7 @@ module Admin authorize :webhook, :create? @webhook = Webhook.new(resource_params) + @webhook.current_account = current_account if @webhook.save redirect_to admin_webhook_path(@webhook) @@ -39,6 +40,8 @@ module Admin def update authorize @webhook, :update? + @webhook.current_account = current_account + if @webhook.update(resource_params) redirect_to admin_webhook_path(@webhook) else diff --git a/app/models/webhook.rb b/app/models/webhook.rb index c46fce743e..14f33c5fc4 100644 --- a/app/models/webhook.rb +++ b/app/models/webhook.rb @@ -24,6 +24,8 @@ class Webhook < ApplicationRecord status.updated ).freeze + attr_writer :current_account + scope :enabled, -> { where(enabled: true) } validates :url, presence: true, url: true @@ -31,6 +33,7 @@ class Webhook < ApplicationRecord validates :events, presence: true validate :validate_events + validate :validate_permissions validate :validate_template before_validation :strip_events @@ -48,12 +51,31 @@ class Webhook < ApplicationRecord update!(enabled: false) end + def required_permissions + events.map { |event| Webhook.permission_for_event(event) } + end + + def self.permission_for_event(event) + case event + when 'account.approved', 'account.created', 'account.updated' + :manage_users + when 'report.created' + :manage_reports + when 'status.created', 'status.updated' + :view_devops + end + end + private def validate_events errors.add(:events, :invalid) if events.any? { |e| EVENTS.exclude?(e) } end + def validate_permissions + errors.add(:events, :invalid_permissions) if defined?(@current_account) && required_permissions.any? { |permission| !@current_account.user_role.can?(permission) } + end + def validate_template return if template.blank? diff --git a/app/policies/webhook_policy.rb b/app/policies/webhook_policy.rb index a2199a333f..577e891b66 100644 --- a/app/policies/webhook_policy.rb +++ b/app/policies/webhook_policy.rb @@ -14,7 +14,7 @@ class WebhookPolicy < ApplicationPolicy end def update? - role.can?(:manage_webhooks) + role.can?(:manage_webhooks) && record.required_permissions.all? { |permission| role.can?(permission) } end def enable? @@ -30,6 +30,6 @@ class WebhookPolicy < ApplicationPolicy end def destroy? - role.can?(:manage_webhooks) + role.can?(:manage_webhooks) && record.required_permissions.all? { |permission| role.can?(permission) } end end diff --git a/app/views/admin/webhooks/_form.html.haml b/app/views/admin/webhooks/_form.html.haml index 8d019ff43b..c870e943f4 100644 --- a/app/views/admin/webhooks/_form.html.haml +++ b/app/views/admin/webhooks/_form.html.haml @@ -5,7 +5,7 @@ = f.input :url, wrapper: :with_block_label, input_html: { placeholder: 'https://' } .fields-group - = f.input :events, collection: Webhook::EVENTS, wrapper: :with_block_label, include_blank: false, as: :check_boxes, collection_wrapper_tag: 'ul', item_wrapper_tag: 'li' + = f.input :events, collection: Webhook::EVENTS, wrapper: :with_block_label, include_blank: false, as: :check_boxes, collection_wrapper_tag: 'ul', item_wrapper_tag: 'li', disabled: Webhook::EVENTS.filter { |event| !current_user.role.can?(Webhook.permission_for_event(event)) } .fields-group = f.input :template, wrapper: :with_block_label, input_html: { placeholder: '{ "content": "Hello {{object.username}}" }' } diff --git a/config/locales/activerecord.en.yml b/config/locales/activerecord.en.yml index 8aee15659f..a53c7c6e9e 100644 --- a/config/locales/activerecord.en.yml +++ b/config/locales/activerecord.en.yml @@ -53,3 +53,7 @@ en: position: elevated: cannot be higher than your current role own_role: cannot be changed with your current role + webhook: + attributes: + events: + invalid_permissions: cannot include events you don't have the rights to diff --git a/spec/controllers/admin/webhooks_controller_spec.rb b/spec/controllers/admin/webhooks_controller_spec.rb index 074956c555..0ccfbbcc6e 100644 --- a/spec/controllers/admin/webhooks_controller_spec.rb +++ b/spec/controllers/admin/webhooks_controller_spec.rb @@ -48,7 +48,7 @@ describe Admin::WebhooksController do end context 'with an existing record' do - let!(:webhook) { Fabricate :webhook } + let!(:webhook) { Fabricate(:webhook, events: ['account.created', 'report.created']) } describe 'GET #show' do it 'returns http success and renders view' do diff --git a/spec/policies/webhook_policy_spec.rb b/spec/policies/webhook_policy_spec.rb index 1eac8932d4..909311461a 100644 --- a/spec/policies/webhook_policy_spec.rb +++ b/spec/policies/webhook_policy_spec.rb @@ -8,16 +8,32 @@ describe WebhookPolicy do let(:admin) { Fabricate(:user, role: UserRole.find_by(name: 'Admin')).account } let(:john) { Fabricate(:account) } - permissions :index?, :create?, :show?, :update?, :enable?, :disable?, :rotate_secret?, :destroy? do + permissions :index?, :create? do context 'with an admin' do it 'permits' do - expect(policy).to permit(admin, Tag) + expect(policy).to permit(admin, Webhook) end end context 'with a non-admin' do it 'denies' do - expect(policy).to_not permit(john, Tag) + expect(policy).to_not permit(john, Webhook) + end + end + end + + permissions :show?, :update?, :enable?, :disable?, :rotate_secret?, :destroy? do + let(:webhook) { Fabricate(:webhook, events: ['account.created', 'report.created']) } + + context 'with an admin' do + it 'permits' do + expect(policy).to permit(admin, webhook) + end + end + + context 'with a non-admin' do + it 'denies' do + expect(policy).to_not permit(john, webhook) end end end