From 4b0eedac61712328686bc4c47607b5cffe9ee07a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Ce=C4=BEuch?= Date: Fri, 22 Sep 2023 12:15:36 +0200 Subject: [PATCH 01/13] add brakeman do gh test workflow --- .github/workflows/workflow.yml | 1 + Gemfile | 1 + Gemfile.lock | 2 ++ 3 files changed, 4 insertions(+) diff --git a/.github/workflows/workflow.yml b/.github/workflows/workflow.yml index 17c75ca54..590a15e47 100644 --- a/.github/workflows/workflow.yml +++ b/.github/workflows/workflow.yml @@ -30,6 +30,7 @@ jobs: with: bundler-cache: true + - run: bundle exec brakeman - run: bundle exec rails db:setup --trace - run: bundle exec rails test diff --git a/Gemfile b/Gemfile index f6e8912b5..826347ca1 100644 --- a/Gemfile +++ b/Gemfile @@ -47,6 +47,7 @@ gem 'pg_search' gem 'bootsnap', '>= 1.4.4', require: false group :development, :test do + gem "brakeman" gem 'dotenv-rails' gem 'pry-rails' gem 'pry-byebug' diff --git a/Gemfile.lock b/Gemfile.lock index 2c862765b..cd93bc104 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -77,6 +77,7 @@ GEM bindex (0.8.1) bootsnap (1.16.0) msgpack (~> 1.2) + brakeman (6.0.1) builder (3.2.4) byebug (11.1.3) capybara (3.39.1) @@ -385,6 +386,7 @@ PLATFORMS DEPENDENCIES annotate bootsnap (>= 1.4.4) + brakeman capybara capybara-screenshot clockwork From 5a1e8105e1d273858aeb357bab1acc0a07033885 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Ce=C4=BEuch?= Date: Fri, 22 Sep 2023 12:41:41 +0200 Subject: [PATCH 02/13] run brakeman in separate workflow and add brakeman.ignore --- .github/workflows/workflow.yml | 16 ++++- config/brakeman.ignore | 108 +++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 config/brakeman.ignore diff --git a/.github/workflows/workflow.yml b/.github/workflows/workflow.yml index 590a15e47..1a9b0fb7e 100644 --- a/.github/workflows/workflow.yml +++ b/.github/workflows/workflow.yml @@ -5,6 +5,21 @@ on: branches: '**' jobs: + brakeman: + name: Brakeman + + if: ${{github.repository == 'solver-it-sro/govbox-pro'}} + + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v2 + - uses: ruby/setup-ruby@v1 + with: + bundler-cache: true + + - run: bundle exec brakeman + test: if: ${{github.repository == 'solver-it-sro/govbox-pro'}} @@ -30,7 +45,6 @@ jobs: with: bundler-cache: true - - run: bundle exec brakeman - run: bundle exec rails db:setup --trace - run: bundle exec rails test diff --git a/config/brakeman.ignore b/config/brakeman.ignore new file mode 100644 index 000000000..b75f63055 --- /dev/null +++ b/config/brakeman.ignore @@ -0,0 +1,108 @@ +{ + "ignored_warnings": [ + { + "warning_type": "Dynamic Render Path", + "warning_code": 15, + "fingerprint": "0d7a447e47382893b06895c67cb02fc7084b4904e795f2488049b4083d5ef829", + "check_name": "Render", + "message": "Render path contains parameter value", + "file": "app/views/admin/tags/show.html.erb", + "line": 6, + "link": "https://brakemanscanner.org/docs/warning_types/dynamic_render_path/", + "code": "render(action => policy_scope([:admin, Tag]).find(params[:id]), {})", + "render_path": [ + { + "type": "controller", + "class": "Admin::TagsController", + "method": "show", + "line": 12, + "file": "app/controllers/admin/tags_controller.rb", + "rendered": { + "name": "admin/tags/show", + "file": "app/views/admin/tags/show.html.erb" + } + } + ], + "location": { + "type": "template", + "template": "admin/tags/show" + }, + "user_input": "params[:id]", + "confidence": "Weak", + "cwe_id": [ + 22 + ], + "note": "" + }, + { + "warning_type": "Dynamic Render Path", + "warning_code": 15, + "fingerprint": "206fab310dd6225cc18046a3fa3d8d2e15898077ebf7140753f6104ac8952297", + "check_name": "Render", + "message": "Render path contains parameter value", + "file": "app/views/admin/boxes/show.html.erb", + "line": 6, + "link": "https://brakemanscanner.org/docs/warning_types/dynamic_render_path/", + "code": "render(action => policy_scope([:admin, Box]).find(params[:id]), {})", + "render_path": [ + { + "type": "controller", + "class": "Admin::BoxesController", + "method": "show", + "line": 12, + "file": "app/controllers/admin/boxes_controller.rb", + "rendered": { + "name": "admin/boxes/show", + "file": "app/views/admin/boxes/show.html.erb" + } + } + ], + "location": { + "type": "template", + "template": "admin/boxes/show" + }, + "user_input": "params[:id]", + "confidence": "Weak", + "cwe_id": [ + 22 + ], + "note": "" + }, + { + "warning_type": "Dynamic Render Path", + "warning_code": 15, + "fingerprint": "b8a2fb69d5ae58b1a2ef3054ed2a602436392f1db28b5ef31c0ec249e0fec16a", + "check_name": "Render", + "message": "Render path contains parameter value", + "file": "app/views/admin/tenants/show.html.erb", + "line": 14, + "link": "https://brakemanscanner.org/docs/warning_types/dynamic_render_path/", + "code": "render(action => policy_scope([:admin, Tenant]).find(params[:id]), {})", + "render_path": [ + { + "type": "controller", + "class": "Admin::TenantsController", + "method": "show", + "line": 14, + "file": "app/controllers/admin/tenants_controller.rb", + "rendered": { + "name": "admin/tenants/show", + "file": "app/views/admin/tenants/show.html.erb" + } + } + ], + "location": { + "type": "template", + "template": "admin/tenants/show" + }, + "user_input": "params[:id]", + "confidence": "Weak", + "cwe_id": [ + 22 + ], + "note": "" + } + ], + "updated": "2023-09-22 12:38:37 +0200", + "brakeman_version": "6.0.1" +} From 26340ca8abdbd215fc1aa5dd86378eca37f7c5f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Ce=C4=BEuch?= Date: Fri, 22 Sep 2023 13:37:13 +0200 Subject: [PATCH 03/13] rm repo lock to run tests in workflow --- .github/workflows/workflow.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.github/workflows/workflow.yml b/.github/workflows/workflow.yml index 1a9b0fb7e..7e186ac26 100644 --- a/.github/workflows/workflow.yml +++ b/.github/workflows/workflow.yml @@ -8,8 +8,6 @@ jobs: brakeman: name: Brakeman - if: ${{github.repository == 'solver-it-sro/govbox-pro'}} - runs-on: ubuntu-latest steps: @@ -21,8 +19,6 @@ jobs: - run: bundle exec brakeman test: - if: ${{github.repository == 'solver-it-sro/govbox-pro'}} - runs-on: ubuntu-latest env: From fef0f062d17d54e2ad3529296a18c0fd6716f8da Mon Sep 17 00:00:00 2001 From: Miroslav Hettes Date: Fri, 22 Sep 2023 17:05:48 +0200 Subject: [PATCH 04/13] Introduce a with_advisory_lock! method --- app/jobs/govbox/process_message_job.rb | 2 +- app/models/application_record.rb | 12 ++++++++++++ app/models/govbox/message.rb | 5 ----- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/jobs/govbox/process_message_job.rb b/app/jobs/govbox/process_message_job.rb index 24a8ee777..16be44f0d 100644 --- a/app/jobs/govbox/process_message_job.rb +++ b/app/jobs/govbox/process_message_job.rb @@ -4,7 +4,7 @@ module Govbox class ProcessMessageJob < ApplicationJob queue_as :default - retry_on ::Govbox::Message::FailedToAcquireLockError, wait: :exponentially_longer, attempts: Float::INFINITY + retry_on ::ApplicationRecord::FailedToAcquireLockError, wait: :exponentially_longer, attempts: Float::INFINITY def perform(govbox_message) ActiveRecord::Base.transaction do diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 10a4cba84..9e44c581f 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -1,3 +1,15 @@ class ApplicationRecord < ActiveRecord::Base self.abstract_class = true + + class FailedToAcquireLockError < StandardError + end + + def self.with_advisory_lock!(lock_name, options = {}, &block) + result = with_advisory_lock_result(lock_name, options, &block) + if result.lock_was_acquired? + result.result + else + raise FailedToAcquireLockError + end + end end diff --git a/app/models/govbox/message.rb b/app/models/govbox/message.rb index 266a63fac..e1829c1e6 100644 --- a/app/models/govbox/message.rb +++ b/app/models/govbox/message.rb @@ -54,14 +54,9 @@ def self.create_message_with_thread!(govbox_message) message end - raise FailedToAcquireLockError unless message - self.create_message_objects(message, govbox_message.payload) end - class FailedToAcquireLockError < StandardError - end - private def self.create_message(govbox_message) From 17893d1ea8349f1002795de6b92dcec8c66efacd Mon Sep 17 00:00:00 2001 From: Miroslav Hettes Date: Fri, 22 Sep 2023 17:07:38 +0200 Subject: [PATCH 05/13] Index message thread with advisory_lock --- app/jobs/searchable/reindex_message_thread_job.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/jobs/searchable/reindex_message_thread_job.rb b/app/jobs/searchable/reindex_message_thread_job.rb index 53f985c9f..32c952a2e 100644 --- a/app/jobs/searchable/reindex_message_thread_job.rb +++ b/app/jobs/searchable/reindex_message_thread_job.rb @@ -1,7 +1,13 @@ class Searchable::ReindexMessageThreadJob < ApplicationJob queue_as :default + retry_on ::ApplicationRecord::FailedToAcquireLockError, wait: :exponentially_longer, attempts: Float::INFINITY + def perform(message_thread) - ::Searchable::MessageThread.index_record(message_thread) + ::Searchable::MessageThread.transaction do + ::Searchable::MessageThread.with_advisory_lock!("mt_#{message_thread.id}", transaction: true, timeout_seconds: 10) do + ::Searchable::MessageThread.index_record(message_thread) + end + end end end From 2bf7ac658ef74ee8bfe2b359695d4864e092f2e6 Mon Sep 17 00:00:00 2001 From: Miroslav Hettes Date: Fri, 22 Sep 2023 17:29:56 +0200 Subject: [PATCH 06/13] Limit total number of unfinished jobs for reindexing --- app/jobs/searchable/reindex_message_thread_job.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/app/jobs/searchable/reindex_message_thread_job.rb b/app/jobs/searchable/reindex_message_thread_job.rb index 32c952a2e..c925f9b34 100644 --- a/app/jobs/searchable/reindex_message_thread_job.rb +++ b/app/jobs/searchable/reindex_message_thread_job.rb @@ -1,6 +1,16 @@ class Searchable::ReindexMessageThreadJob < ApplicationJob queue_as :default + include GoodJob::ActiveJobExtensions::Concurrency + + good_job_control_concurrency_with( + # Maximum number of unfinished jobs to allow with the concurrency key + # Can be an Integer or Lambda/Proc that is invoked in the context of the job + total_limit: 1, + + key: -> { "Searchable::ReindexMessageThreadJob-#{arguments.first.id}" } + ) + retry_on ::ApplicationRecord::FailedToAcquireLockError, wait: :exponentially_longer, attempts: Float::INFINITY def perform(message_thread) From 4f5dde982a93493c33108376d1503e9938557c59 Mon Sep 17 00:00:00 2001 From: Miroslav Hettes Date: Fri, 22 Sep 2023 17:44:27 +0200 Subject: [PATCH 07/13] Discard on deserialization error --- app/jobs/searchable/reindex_message_thread_job.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/jobs/searchable/reindex_message_thread_job.rb b/app/jobs/searchable/reindex_message_thread_job.rb index c925f9b34..8d3fb73ee 100644 --- a/app/jobs/searchable/reindex_message_thread_job.rb +++ b/app/jobs/searchable/reindex_message_thread_job.rb @@ -8,10 +8,11 @@ class Searchable::ReindexMessageThreadJob < ApplicationJob # Can be an Integer or Lambda/Proc that is invoked in the context of the job total_limit: 1, - key: -> { "Searchable::ReindexMessageThreadJob-#{arguments.first.id}" } + key: -> { "Searchable::ReindexMessageThreadJob-#{arguments.first.try(:id)}" } ) retry_on ::ApplicationRecord::FailedToAcquireLockError, wait: :exponentially_longer, attempts: Float::INFINITY + discard_on ActiveJob::DeserializationError def perform(message_thread) ::Searchable::MessageThread.transaction do From e0020f68fe5e1f35bb36d4a80be2edad7a3a6b73 Mon Sep 17 00:00:00 2001 From: Miroslav Hettes Date: Mon, 25 Sep 2023 10:41:54 +0200 Subject: [PATCH 08/13] Skip reindexing if message thread is nil --- app/jobs/searchable/reindex_message_thread_job.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/jobs/searchable/reindex_message_thread_job.rb b/app/jobs/searchable/reindex_message_thread_job.rb index 8d3fb73ee..5ff72caee 100644 --- a/app/jobs/searchable/reindex_message_thread_job.rb +++ b/app/jobs/searchable/reindex_message_thread_job.rb @@ -15,6 +15,8 @@ class Searchable::ReindexMessageThreadJob < ApplicationJob discard_on ActiveJob::DeserializationError def perform(message_thread) + return if message_thread.nil? + ::Searchable::MessageThread.transaction do ::Searchable::MessageThread.with_advisory_lock!("mt_#{message_thread.id}", transaction: true, timeout_seconds: 10) do ::Searchable::MessageThread.index_record(message_thread) From eff4b95bb9b440a21fb90501d26aebc8a126ec51 Mon Sep 17 00:00:00 2001 From: Miroslav Hettes Date: Mon, 25 Sep 2023 11:03:58 +0200 Subject: [PATCH 09/13] Remove advisory lock from job with concurrency control --- app/jobs/searchable/reindex_message_thread_job.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/jobs/searchable/reindex_message_thread_job.rb b/app/jobs/searchable/reindex_message_thread_job.rb index 5ff72caee..41f7732f1 100644 --- a/app/jobs/searchable/reindex_message_thread_job.rb +++ b/app/jobs/searchable/reindex_message_thread_job.rb @@ -11,16 +11,11 @@ class Searchable::ReindexMessageThreadJob < ApplicationJob key: -> { "Searchable::ReindexMessageThreadJob-#{arguments.first.try(:id)}" } ) - retry_on ::ApplicationRecord::FailedToAcquireLockError, wait: :exponentially_longer, attempts: Float::INFINITY discard_on ActiveJob::DeserializationError def perform(message_thread) return if message_thread.nil? - ::Searchable::MessageThread.transaction do - ::Searchable::MessageThread.with_advisory_lock!("mt_#{message_thread.id}", transaction: true, timeout_seconds: 10) do - ::Searchable::MessageThread.index_record(message_thread) - end - end + ::Searchable::MessageThread.index_record(message_thread) end end From 829ef9b80c22e27094f1fa038c1357fb5f2dbfb9 Mon Sep 17 00:00:00 2001 From: Miroslav Hettes Date: Mon, 25 Sep 2023 11:17:00 +0200 Subject: [PATCH 10/13] Use correct method --- app/models/govbox/message.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/govbox/message.rb b/app/models/govbox/message.rb index e1829c1e6..2833a2f67 100644 --- a/app/models/govbox/message.rb +++ b/app/models/govbox/message.rb @@ -27,7 +27,7 @@ def replyable? end def self.create_message_with_thread!(govbox_message) - message = MessageThread.with_advisory_lock(govbox_message.correlation_id, transaction: true, timeout_seconds: 10) do + message = MessageThread.with_advisory_lock!(govbox_message.correlation_id, transaction: true, timeout_seconds: 10) do folder = Folder.find_or_create_by!( name: "Inbox", box: govbox_message.box From 2a8d995490749e9c85dbc3d18809494c96d6c317 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Ce=C4=BEuch?= Date: Tue, 26 Sep 2023 09:36:36 +0200 Subject: [PATCH 11/13] set max body size to 256m --- .gitlab/auto-deploy-values.yaml | 3 +++ .gitlab/prod-auto-deploy-values.yaml | 3 +++ 2 files changed, 6 insertions(+) diff --git a/.gitlab/auto-deploy-values.yaml b/.gitlab/auto-deploy-values.yaml index 55dd112f7..27cc19e33 100644 --- a/.gitlab/auto-deploy-values.yaml +++ b/.gitlab/auto-deploy-values.yaml @@ -6,6 +6,9 @@ image: secrets: - name: skdigital-bonet-registry application.migrateCommand: ["bundle", "exec", "rails", "db:migrate"] +ingress: + annotations: + nginx.ingress.kubernetes.io/proxy-body-size: 256m livenessProbe: initialDelaySeconds: 5 timeoutSeconds: 5 diff --git a/.gitlab/prod-auto-deploy-values.yaml b/.gitlab/prod-auto-deploy-values.yaml index 55dd112f7..27cc19e33 100644 --- a/.gitlab/prod-auto-deploy-values.yaml +++ b/.gitlab/prod-auto-deploy-values.yaml @@ -6,6 +6,9 @@ image: secrets: - name: skdigital-bonet-registry application.migrateCommand: ["bundle", "exec", "rails", "db:migrate"] +ingress: + annotations: + nginx.ingress.kubernetes.io/proxy-body-size: 256m livenessProbe: initialDelaySeconds: 5 timeoutSeconds: 5 From ffa32a0f8c438157952eab1b2e2de64cf4f9028d Mon Sep 17 00:00:00 2001 From: stage-rl Date: Wed, 27 Sep 2023 08:41:16 +0200 Subject: [PATCH 12/13] Split index action to index and scroll on threads --- .../message_threads_table_component.html.erb | 2 +- app/controllers/message_threads_controller.rb | 28 +++++++++++-------- app/policies/message_thread_policy.rb | 4 +++ app/views/message_threads/index.html.erb | 2 +- ...rbo_stream.erb => scroll.turbo_stream.erb} | 2 +- config/routes.rb | 3 +- 6 files changed, 25 insertions(+), 16 deletions(-) rename app/views/message_threads/{index.turbo_stream.erb => scroll.turbo_stream.erb} (89%) diff --git a/app/components/message_threads_table_component.html.erb b/app/components/message_threads_table_component.html.erb index a4ceaa663..47943b7cb 100644 --- a/app/components/message_threads_table_component.html.erb +++ b/app/components/message_threads_table_component.html.erb @@ -1,6 +1,6 @@
- <%= form_with(url: merge_message_threads_path, local: true) do |form|%> + <%= form_with(url: merge_message_threads_path) do |form|%>
diff --git a/app/controllers/message_threads_controller.rb b/app/controllers/message_threads_controller.rb index b358dd723..a46012c11 100644 --- a/app/controllers/message_threads_controller.rb +++ b/app/controllers/message_threads_controller.rb @@ -18,24 +18,28 @@ def update def index authorize MessageThread + index_common + end + + def scroll + authorize MessageThread + index_common + end + def index_common cursor = MessageThreadCollection.init_cursor(search_params[:cursor]) - @message_threads, @next_cursor = MessageThreadCollection.all( - scope: message_thread_policy_scope.includes(:tags, :box), - search_permissions: search_permissions, - query: search_params[:q], - no_visible_tags: search_params[:no_visible_tags] == '1' && Current.user.admin?, - cursor: cursor - ) + @message_threads, @next_cursor = + MessageThreadCollection.all( + scope: message_thread_policy_scope.includes(:tags, :box), + search_permissions: search_permissions, + query: search_params[:q], + no_visible_tags: search_params[:no_visible_tags] == "1" && Current.user.admin?, + cursor: cursor + ) @next_cursor = MessageThreadCollection.serialize_cursor(@next_cursor) @next_page_params = search_params.to_h.merge(cursor: @next_cursor).merge(format: :turbo_stream) - - respond_to do |format| - format.html # GET - format.turbo_stream # POST - end end def merge diff --git a/app/policies/message_thread_policy.rb b/app/policies/message_thread_policy.rb index 6ba588a6a..f62e29c65 100644 --- a/app/policies/message_thread_policy.rb +++ b/app/policies/message_thread_policy.rb @@ -31,6 +31,10 @@ def index? true end + def scroll? + true + end + def update? true end diff --git a/app/views/message_threads/index.html.erb b/app/views/message_threads/index.html.erb index 7d3b4a33b..b63624c8e 100644 --- a/app/views/message_threads/index.html.erb +++ b/app/views/message_threads/index.html.erb @@ -8,7 +8,7 @@ <% component.with_next_page_area do %> <%= render Turbo::NextPageAreaComponent.new( id: @next_cursor, - url: message_threads_url(@next_page_params) + url: scroll_message_threads_url(@next_page_params) ) %> <% end %> diff --git a/app/views/message_threads/index.turbo_stream.erb b/app/views/message_threads/scroll.turbo_stream.erb similarity index 89% rename from app/views/message_threads/index.turbo_stream.erb rename to app/views/message_threads/scroll.turbo_stream.erb index 46fe76d81..b6aa7ab5b 100644 --- a/app/views/message_threads/index.turbo_stream.erb +++ b/app/views/message_threads/scroll.turbo_stream.erb @@ -7,7 +7,7 @@ <%= turbo_stream.append "next_page_area" do %> <%= render Turbo::NextPageComponent.new( id:@next_cursor, - url: message_threads_url(@next_page_params) + url: scroll_message_threads_url(@next_page_params) ) %> <% end %> <% else %> diff --git a/config/routes.rb b/config/routes.rb index e0a106de0..a5d92f322 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -50,7 +50,8 @@ resources :message_threads do collection do - post 'merge' + post :merge + get :scroll end resources :messages end From b9ff504eff9165b8278fedf79d8fd19e8d667e74 Mon Sep 17 00:00:00 2001 From: stage-rl Date: Wed, 27 Sep 2023 08:56:26 +0200 Subject: [PATCH 13/13] Refactor threads controller index and scroll actions --- app/controllers/message_threads_controller.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/controllers/message_threads_controller.rb b/app/controllers/message_threads_controller.rb index a46012c11..dc670ca08 100644 --- a/app/controllers/message_threads_controller.rb +++ b/app/controllers/message_threads_controller.rb @@ -1,5 +1,6 @@ class MessageThreadsController < ApplicationController before_action :set_message_thread, only: %i[show update] + before_action :load_threads, only: %i[index scroll] def show authorize @message_thread @@ -18,15 +19,13 @@ def update def index authorize MessageThread - index_common end def scroll authorize MessageThread - index_common end - def index_common + def load_threads cursor = MessageThreadCollection.init_cursor(search_params[:cursor]) @message_threads, @next_cursor =