Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Enable Ruby Proc for require_confirm_worker_params #32

Merged

Conversation

khoaanguyenn
Copy link
Contributor

@khoaanguyenn khoaanguyenn commented Jan 17, 2023

Background

  • We need a feature to enable require_confirm_worker_params to also accept Ruby callable object in the configuration as follows
SidekiqAdhocJob.configure do |config|
  config.module_names = [:Module]

  config.require_confirm_worker_names = %w[TestWorker TestJob] # current implementation
  config.require_confirm_worker_names = ->(_worker_name) { true } # apply to all workers
  config.require_confirm_worker_names = RubyCallable.new # Ruby callable object 
  ...
end
SidekiqAdhocJob.init
  • From the PR Add prompted confirmation #30 , confirmation feature was added. However, when input incorrect challenge keyword, nothing happens but closing the prompt. This is not a good user experience, because they might not know that the inputed keyword is incorrect.

  • The Sidekiq upgrade 6.1.3 -> 6.2.0, causes Sidekiq::Web needs a valid Rack session for CSRF protection. to every requests to server, thus requests test suite eventually fail and print out the following error message

6) GET /adhoc_jobs/:name job not found redirects to index page
     Failure/Error: get '/adhoc-jobs/invalid_worker'

     RuntimeError:
       Sidekiq::Web needs a valid Rack session for CSRF protection. If this is a Rails app,
       make sure you mount Sidekiq::Web *inside* your application routes:


       Rails.application.routes.draw do
         mount Sidekiq::Web => "/sidekiq"
         ....
       end


       If this is a Rails app in API mode, you need to enable sessions.

         https://guides.rubyonrails.org/api_app.html#using-session-middlewares

       If this is a bare Rack app, use a session middleware before Sidekiq::Web:

         # first, use IRB to create a shared secret key for sessions and commit it
         require 'securerandom'; File.open(".session.key", "w") {|f| f.write(SecureRandom.hex(32)) }

         # now use the secret with a session cookie middleware
         use Rack::Session::Cookie, secret: File.read(".session.key"), same_site: true, max_age: 86400
         run Sidekiq::Web
     # ./spec/sidekiq_adhoc_job/requests/jobs/show_spec.rb:130:in `block (3 levels) in <top (required)>'

Design

  • Adjust javascript confirmation.
  • Allows #require_confirm_worker_params to accept Ruby callable object.
  • Add Gemfile.lock to this repository.
  • Add more helpers to shared contexts.
  • Disable Sidekiq CSRF in the test suite. Reference
    def disable_csrf
      allow_any_instance_of(Sidekiq::Web::CsrfProtection).to receive(:accept?).and_return(true)
    end
  • Fix all failed specs.
  • Refactor specs

Impact

  • Show alert when the inputed challenge keyword is incorrect.
  • Enable #require_confirm_worker_params to accept Ruby callable object such as Proc, lambda or any Ruby class that implements #call method.
  • All tests are passed ✅

Caveats

None.

Testing

Install this branch to test the new behaviors.

  gem 'sidekiq_adhoc_job', git: 'https://github.com/gohkhoonhiang/sidekiq_adhoc_job.git', branch: 'feature/require_confirm_proc'
  1. Ensure the test suite all passed
      bundle exec rspec
  2. When inputting incorrect challenge keyword the alert Your confirmation input is incorrect, try again. should be displayed.
  3. Can config Ruby callable object to #require_confirm_worker_params.

Docs

@teohm
Copy link
Contributor

teohm commented Jan 17, 2023

The require_confirm_worker_params changes look good to me.

However, it looks like the CI build failed?

@khoaanguyenn
Copy link
Contributor Author

Yes, the bundler gem now only supports x86_64-darwin-21 platform, but the current platform is x86_64-linux one. Travis CI build. Thus, we need to change the OS build config

@gohkhoonhiang
Copy link
Owner

@khoaanguyenn Can we not commit the lock file and try, because you specified x86_64-darwin-21 in the lock file? Also, what was the reason to commit the lock file?

Copy link
Owner

@gohkhoonhiang gohkhoonhiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, and fixing some styling issues! There're just some texts to fix in the specs. Could you also help to update the README https://github.com/gohkhoonhiang/sidekiq_adhoc_job/blob/master/README.md#usage section to include the various ways to configure the require_confirm_worker_params?

spec/sidekiq_adhoc_job_spec.rb Outdated Show resolved Hide resolved
spec/sidekiq_adhoc_job_spec.rb Outdated Show resolved Hide resolved
spec/sidekiq_adhoc_job_spec.rb Outdated Show resolved Hide resolved
spec/sidekiq_adhoc_job_spec.rb Outdated Show resolved Hide resolved
@gohkhoonhiang gohkhoonhiang added enhancement New feature or request release-2.2.2 labels Jan 18, 2023
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
spec/sidekiq_adhoc_job_spec.rb Outdated Show resolved Hide resolved
spec/sidekiq_adhoc_job_spec.rb Outdated Show resolved Hide resolved
spec/sidekiq_adhoc_job_spec.rb Outdated Show resolved Hide resolved
spec/sidekiq_adhoc_job_spec.rb Outdated Show resolved Hide resolved
@gohkhoonhiang gohkhoonhiang merged commit 2d8198a into gohkhoonhiang:master Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request release-2.2.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants