Skip to content
This repository was archived by the owner on Jul 14, 2022. It is now read-only.

Add a 'report only' feature where activerecord models will not blow up when firewalled but will just log - #3

Open
JackMc wants to merge 5 commits into
masterfrom
add-report-only
Open

Add a 'report only' feature where activerecord models will not blow up when firewalled but will just log#3
JackMc wants to merge 5 commits into
masterfrom
add-report-only

Conversation

@JackMc

@JackMc JackMc commented Mar 12, 2018

Copy link
Copy Markdown
Contributor

Currently, the gem will always blow up when a firewalled constraint is violated. This isn’t great for when you’re trying to implement this new safeguard in a production application.

This adds an option to firewalled_belongs_to called report_only that instead of raising the exception will print out the error message to the log at the INFO level.

I also added an “image” model & association to each user which uses the report only type of belongs to.

Comment thread test/test_helper.rb
fixtures :all

# Add more helper methods to be used by all tests here...
def assert_logged(message)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@JackMc
JackMc force-pushed the add-report-only branch 2 times, most recently from a9781e4 to 4efbb7f Compare March 13, 2018 04:15
@JackMc
JackMc requested review from clayton-shopify and sgrif March 13, 2018 04:17
@JackMc JackMc changed the title [WIP] Add a 'report only' feature where activerecord models will not blow up when firewalled but will just log Add a 'report only' feature where activerecord models will not blow up when firewalled but will just log Mar 13, 2018
@JackMc

JackMc commented Mar 13, 2018

Copy link
Copy Markdown
Contributor Author

This is good for review now (no longer WIP)

Comment thread activerecord-firewall.gemspec Outdated
s.files = Dir["{app,config,db,lib}/**/*", "MIT-LICENSE", "Rakefile", "README.md"]

s.add_dependency "rails", "~> 5.1.5"
s.add_dependency "rails", "~> 5.0.0"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need to depend on rails. This gem seems to depend only on activerecord and activesupport.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey, I tried to switch to just these as dependencies and I get this error from running bin/test:

bin/test:5:in `require': cannot load such file -- rails/plugin/test (LoadError)
	from bin/test:5:in `<main>'

I figure this is from not having railties, which if I add that then gives me this error:

/Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/railtie/configuration.rb:95:in `method_missing': undefined method `action_mailer' for #<Rails::Application::Configuration:0x00007f91c6190258> (NoMethodError)
	from /Users/jackmc/src/github.com/Shopify/activerecord-firewall/test/dummy/config/environments/test.rb:30:in `block in <top (required)>'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/railtie.rb:211:in `instance_eval'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/railtie.rb:211:in `configure'
	from /Users/jackmc/src/github.com/Shopify/activerecord-firewall/test/dummy/config/environments/test.rb:1:in `<top (required)>'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:292:in `require'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:292:in `block in require'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:258:in `load_dependency'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:292:in `require'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/engine.rb:600:in `block (2 levels) in <class:Engine>'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/engine.rb:599:in `each'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/engine.rb:599:in `block in <class:Engine>'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/initializable.rb:30:in `instance_exec'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/initializable.rb:30:in `run'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/initializable.rb:59:in `block in run_initializers'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:228:in `block in tsort_each'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:350:in `block (2 levels) in each_strongly_connected_component'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:422:in `block (2 levels) in each_strongly_connected_component_from'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:431:in `each_strongly_connected_component_from'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:421:in `block in each_strongly_connected_component_from'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/initializable.rb:48:in `each'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/initializable.rb:48:in `tsort_each_child'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:415:in `call'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:415:in `each_strongly_connected_component_from'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:349:in `block in each_strongly_connected_component'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:347:in `each'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:347:in `call'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:347:in `each_strongly_connected_component'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:226:in `tsort_each'
	from /opt/rubies/2.4.2/lib/ruby/2.4.0/tsort.rb:205:in `tsort_each'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/initializable.rb:58:in `run_initializers'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/application.rb:353:in `initialize!'
	from /Users/jackmc/src/github.com/Shopify/activerecord-firewall/test/dummy/config/environment.rb:5:in `<top (required)>'
	from /Users/jackmc/src/github.com/Shopify/activerecord-firewall/test/test_helper.rb:3:in `require'
	from /Users/jackmc/src/github.com/Shopify/activerecord-firewall/test/test_helper.rb:3:in `<top (required)>'
	from /Users/jackmc/src/github.com/Shopify/activerecord-firewall/test/activerecord/firewall_test.rb:1:in `require'
	from /Users/jackmc/src/github.com/Shopify/activerecord-firewall/test/activerecord/firewall_test.rb:1:in `<top (required)>'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/test_unit/runner.rb:50:in `require'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/test_unit/runner.rb:50:in `block in load_tests'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/test_unit/runner.rb:50:in `each'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/test_unit/runner.rb:50:in `load_tests'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/test_unit/runner.rb:39:in `run'
	from /Users/jackmc/.gem/ruby/2.4.2/gems/railties-5.1.7/lib/rails/plugin/test.rb:7:in `<top (required)>'
	from bin/test:5:in `require'
	from bin/test:5:in `<main>'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That second error is because your dummy app is using action mailer in the configs. You can remove it .

@JackMc
JackMc force-pushed the add-report-only branch 2 times, most recently from 1d44746 to 2e9ff10 Compare April 7, 2019 21:19
@JackMc
JackMc force-pushed the add-report-only branch from 8541d86 to 84942e9 Compare April 7, 2019 21:36
@JackMc

JackMc commented Apr 7, 2019

Copy link
Copy Markdown
Contributor Author

I pulled the commit that had rubocop & style changes. I'll add this in a separate PR to keep the diff clear

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants