Use strong_parameters and drop protected_attributes

* Ensure admin attribute can not be set at sign up and the agents user_id is not changeable
* Remove ACCESSIBLE_ATTRIBUTES from the User model
* Remove side effect on agent_params from AgentsController#build_agent

Gemfile

@@ -80,7 +80,6 @@ unless Gem::Version.new(Bundler::VERSION) >= Gem::Version.new('1.5.0')
   exit 1
 end
 
-gem 'protected_attributes', '~>1.0.8' # This must be loaded before some other gems, like delayed_job.
 gem 'ace-rails-ap', '~> 2.0.1'
 gem 'bootstrap-kaminari-views', '~> 0.0.3'
 gem 'bundler', '>= 1.5.0'

Gemfile.lock

@@ -387,8 +387,6 @@ GEM
       multi_json (~> 1.0)
       websocket-driver (>= 0.2.0)
     polyglot (0.3.5)
-    protected_attributes (1.0.8)
-      activemodel (>= 4.0.1, < 5.0)
     pry (0.10.3)
       coderay (~> 1.1.0)
       method_source (~> 0.8.1)
@@ -656,7 +654,6 @@ DEPENDENCIES
   omniauth-wunderlist!
   pg (~> 0.18.3)
   poltergeist
-  protected_attributes (~> 1.0.8)
   pry-byebug
   pry-rails
   quiet_assets

app/concerns/oauthable.rb

@@ -3,7 +3,6 @@ module Oauthable
 
   included do |base|
     @valid_oauth_providers = :all
-    attr_accessible :service_id
     validates_presence_of :service_id
   end
 

app/controllers/admin/users_controller.rb

@@ -19,10 +19,8 @@ class Admin::UsersController < ApplicationController
   end
 
   def create
-    admin = params[:user].delete(:admin)
-    @user = User.new(params[:user])
+    @user = User.new(user_params)
     @user.requires_no_invitation_code!
-    @user.admin = admin
 
     respond_to do |format|
       if @user.save
@@ -40,10 +38,8 @@ class Admin::UsersController < ApplicationController
   end
 
   def update
-    admin = params[:user].delete(:admin)
     params[:user].except!(:password, :password_confirmation) if params[:user][:password].blank?
-    @user.assign_attributes(params[:user])
-    @user.admin = admin
+    @user.assign_attributes(user_params)
 
     respond_to do |format|
       if @user.save
@@ -106,6 +102,10 @@ class Admin::UsersController < ApplicationController
 
   private
 
+  def user_params
+    params.require(:user).permit(:email, :username, :password, :password_confirmation, :admin)
+  end
+
   def find_user
     @user = User.find(params[:id])
   end

app/controllers/agents/dry_runs_controller.rb

@@ -14,7 +14,7 @@ module Agents
     end
 
     def create
-      attrs = params[:agent] || {}
+      attrs = agent_params
       if agent = current_user.agents.find_by(id: params[:agent_id])
         # POST /agents/:id/dry_run
         if attrs.present?

app/controllers/agents_controller.rb

@@ -160,7 +160,7 @@ class AgentsController < ApplicationController
     @agent = current_user.agents.find(params[:id])
 
     respond_to do |format|
-      if @agent.update_attributes(params[:agent])
+      if @agent.update_attributes(agent_params)
         format.html { redirect_back "'#{@agent.name}' was successfully updated.", return: agents_path }
         format.json { render json: @agent, status: :ok, location: agent_path(@agent) }
       else
@@ -220,9 +220,9 @@ class AgentsController < ApplicationController
   end
 
   def build_agent
-    @agent = Agent.build_for_type(params[:agent].delete(:type),
+    @agent = Agent.build_for_type(agent_params[:type],
                                   current_user,
-                                  params[:agent])
+                                  agent_params.except(:type))
   end
 
   def initialize_presenter

app/controllers/application_controller.rb

@@ -60,4 +60,15 @@ class ApplicationController < ActionController::Base
       @basecamp_agent = current_user.agents.where(type: 'Agents::BasecampAgent').first
     end
   end
+
+  def agent_params
+    return {} unless params[:agent]
+    @agent_params ||= begin
+      options = params[:agent].delete(:options) if params[:agent][:options].present?
+      params[:agent].permit(:memory, :name, :type, :schedule, :disabled, :keep_events_for, :propagate_immediately, :drop_pending_events,
+                            source_ids: [], receiver_ids: [], scenario_ids: [], controller_ids: [], control_target_ids: []).tap do |agent_params|
+        agent_params[:options] = options if options
+      end
+    end
+  end
 end

app/controllers/scenarios_controller.rb

@@ -69,7 +69,7 @@ class ScenariosController < ApplicationController
   end
 
   def create
-    @scenario = current_user.scenarios.build(params[:scenario])
+    @scenario = current_user.scenarios.build(scenario_params)
 
     respond_to do |format|
       if @scenario.save
@@ -86,7 +86,7 @@ class ScenariosController < ApplicationController
     @scenario = current_user.scenarios.find(params[:id])
 
     respond_to do |format|
-      if @scenario.update_attributes(params[:scenario])
+      if @scenario.update_attributes(scenario_params)
         format.html { redirect_to @scenario, notice: 'This Scenario was successfully updated.' }
         format.json { head :no_content }
       else
@@ -115,4 +115,11 @@ class ScenariosController < ApplicationController
       format.json { head :no_content }
     end
   end
+
+  private
+
+  def scenario_params
+    params.require(:scenario).permit(:name, :description, :public, :source_url,
+                                     :tag_fg_color, :tag_bg_color, :icon, agent_ids: [])
+  end
 end

app/controllers/user_credentials_controller.rb

@@ -48,7 +48,7 @@ class UserCredentialsController < ApplicationController
   end
 
   def create
-    @user_credential = current_user.user_credentials.build(params[:user_credential])
+    @user_credential = current_user.user_credentials.build(user_credential_params)
 
     respond_to do |format|
       if @user_credential.save
@@ -65,7 +65,7 @@ class UserCredentialsController < ApplicationController
     @user_credential = current_user.user_credentials.find(params[:id])
 
     respond_to do |format|
-      if @user_credential.update_attributes(params[:user_credential])
+      if @user_credential.update_attributes(user_credential_params)
         format.html { redirect_to user_credentials_path, notice: 'Your credential was successfully updated.' }
         format.json { head :no_content }
       else
@@ -84,4 +84,10 @@ class UserCredentialsController < ApplicationController
       format.json { head :no_content }
     end
   end
+
+  private
+
+  def user_credential_params
+    params.require(:user_credential).permit(:credential_name, :credential_value, :mode)
+  end
 end

app/models/agent.rb

@@ -24,8 +24,6 @@ class Agent < ActiveRecord::Base
 
   EVENT_RETENTION_SCHEDULES = [["Forever", 0], ['1 hour', 1.hour], ['6 hours', 6.hours], ["1 day", 1.day], *([2, 3, 4, 5, 7, 14, 21, 30, 45, 90, 180, 365].map {|n| ["#{n} days", n.days] })]
 
-  attr_accessible :options, :memory, :name, :type, :schedule, :controller_ids, :control_target_ids, :disabled, :source_ids, :receiver_ids, :scenario_ids, :keep_events_for, :propagate_immediately, :drop_pending_events
-
   json_serialize :options, :memory
 
   validates_presence_of :name, :user

app/models/agent_log.rb

@@ -2,8 +2,6 @@
 # in Agents' detail pages.  AgentLogs with a `level` of 4 or greater are considered "errors" and automatically update
 # Agents' `last_error_log_at` column.  These are often used to determine if an Agent is `working?`.
 class AgentLog < ActiveRecord::Base
-  attr_accessible :agent, :inbound_event, :level, :message, :outbound_event
-
   belongs_to :agent
   belongs_to :inbound_event, :class_name => "Event"
   belongs_to :outbound_event, :class_name => "Event"

app/models/control_link.rb

@@ -1,7 +1,5 @@
 # A ControlLink connects Agents in a control flow from the `controller` to the `control_target`.
 class ControlLink < ActiveRecord::Base
-  attr_accessible :controller_id, :target_id
-
   belongs_to :controller, class_name: 'Agent', inverse_of: :control_links_as_controller
   belongs_to :control_target, class_name: 'Agent', inverse_of: :control_links_as_control_target
 end

app/models/event.rb

@@ -7,8 +7,6 @@ class Event < ActiveRecord::Base
   include JSONSerializedField
   include LiquidDroppable
 
-  attr_accessible :lat, :lng, :location, :payload, :user_id, :user, :expires_at
-
   acts_as_mappable
 
   json_serialize :payload

app/models/link.rb

@@ -1,7 +1,5 @@
 # A Link connects Agents in a directed Event flow from the `source` to the `receiver`.
 class Link < ActiveRecord::Base
-  attr_accessible :source_id, :receiver_id
-
   belongs_to :source, :class_name => "Agent", :inverse_of => :links_as_source
   belongs_to :receiver, :class_name => "Agent", :inverse_of => :links_as_receiver
 

app/models/scenario.rb

@@ -1,9 +1,6 @@
 class Scenario < ActiveRecord::Base
   include HasGuid
 
-  attr_accessible :name, :agent_ids, :description, :public, :source_url,
-                  :tag_fg_color, :tag_bg_color, :icon
-
   belongs_to :user, :counter_cache => :scenario_count, :inverse_of => :scenarios
   has_many :scenario_memberships, :dependent => :destroy, :inverse_of => :scenario
   has_many :agents, :through => :scenario_memberships, :inverse_of => :scenarios

app/models/service.rb

@@ -1,6 +1,4 @@
 class Service < ActiveRecord::Base
-  attr_accessible :provider, :name, :token, :secret, :refresh_token, :expires_at, :global, :options, :uid
-
   serialize :options, Hash
 
   belongs_to :user, :inverse_of => :services

app/models/user.rb

@@ -12,11 +12,6 @@ class User < ActiveRecord::Base
   # This is in addition to a real persisted field like 'username'
   attr_accessor :login
 
-  ACCESSIBLE_ATTRIBUTES = [ :email, :username, :login, :password, :password_confirmation, :remember_me, :invitation_code ]
-
-  attr_accessible *ACCESSIBLE_ATTRIBUTES
-  attr_accessible *(ACCESSIBLE_ATTRIBUTES + [:admin]), :as => :admin
-
   validates_presence_of :username
   validates :username, uniqueness: { case_sensitive: false }
   validates_format_of :username, :with => /\A[a-zA-Z0-9_-]{3,15}\Z/, :message => "can only contain letters, numbers, underscores, and dashes, and must be between 3 and 15 characters in length."

app/models/user_credential.rb

@@ -1,8 +1,6 @@
 class UserCredential < ActiveRecord::Base
   MODES = %w[text java_script]
 
-  attr_accessible :credential_name, :credential_value, :mode
-
   belongs_to :user
 
   validates_presence_of :credential_name

config/application.rb

@@ -39,12 +39,6 @@ module Huginn
     # like if you have constraints or database-specific column types
     # config.active_record.schema_format = :sql
 
-    # Enforce whitelist mode for mass assignment.
-    # This will create an empty whitelist of attributes available for mass-assignment for all models
-    # in your app. As such, your models will need to explicitly whitelist or blacklist accessible
-    # parameters by using an attr_accessible or attr_protected declaration.
-    config.active_record.whitelist_attributes = true
-
     # Do not swallow errors in after_commit/after_rollback callbacks.
     config.active_record.raise_in_transactional_callbacks = true
 

config/environments/development.rb

@@ -26,8 +26,8 @@ Huginn::Application.configure do
   # Only use best-standards-support built into browsers
   config.action_dispatch.best_standards_support = :builtin
 
-  # Raise exception on mass assignment protection for Active Record models
-  config.active_record.mass_assignment_sanitizer = :strict
+  # Raise exception for unpermitted parameters
+  config.action_controller.action_on_unpermitted_parameters = :raise
 
   # Raise an error on page load if there are pending migrations.
   config.active_record.migration_error = :page_load

config/environments/test.rb

@@ -33,8 +33,8 @@ Huginn::Application.configure do
 
   config.action_mailer.raise_delivery_errors = true
 
-  # Raise exception on mass assignment protection for Active Record models
-  config.active_record.mass_assignment_sanitizer = :strict
+  # Raise exception for unpermitted parameters
+  config.action_controller.action_on_unpermitted_parameters = :raise
 
   # Randomize the order test cases are executed.
   config.active_support.test_order = :random

spec/controllers/admin/users_controller_spec.rb

@@ -15,7 +15,7 @@ describe Admin::UsersController do
       it 'does not import the default scenario' do
         stub(DefaultScenarioImporter).import(is_a(User)) { fail "Should not attempt import" }
         sign_in users(:jane)
-        post :create, :user => {}
+        post :create, :user => {username: 'user'}
       end
     end
   end

spec/controllers/agents_controller_spec.rb

@@ -280,6 +280,13 @@ describe AgentsController do
       expect(response).to render_template("edit")
     end
 
+    it 'does not allow to modify the agents user_id' do
+      sign_in users(:bob)
+      expect {
+        post :update, :id => agents(:bob_website_agent).to_param, :agent => valid_attributes(:user_id => users(:jane).id)
+      }.to raise_error(ActionController::UnpermittedParameters)
+    end
+
     describe "redirecting back" do
       before do
         sign_in users(:bob)

spec/controllers/scenarios_controller_spec.rb

@@ -116,7 +116,7 @@ describe ScenariosController do
     it "will not create Scenarios for other users" do
       expect {
         post :create, :scenario => valid_attributes(:user_id => users(:jane).id)
-      }.to raise_error(ActiveModel::MassAssignmentSecurity::Error)
+      }.to raise_error(ActionController::UnpermittedParameters)
     end
   end
 
@@ -138,6 +138,12 @@ describe ScenariosController do
       expect(assigns(:scenario)).to have(1).errors_on(:name)
       expect(response).to render_template("edit")
     end
+
+    it 'adds an agent to the scenario' do
+      expect {
+        post :update, :id => scenarios(:bob_weather).to_param, :scenario => { :name => "new_name", :public => "1", agent_ids: scenarios(:bob_weather).agent_ids + [agents(:bob_website_agent).id] }
+      }.to change { scenarios(:bob_weather).agent_ids.length }.by(1)
+    end
   end
 
   describe 'PUT enable_or_disable_all_agents' do

spec/controllers/user_credentials_controller_spec.rb

@@ -69,7 +69,7 @@ describe UserCredentialsController do
     it "will not create UserCredentials for other users" do
       expect {
         post :create, :user_credential => valid_attributes(:user_id => users(:jane).id)
-      }.to raise_error(ActiveModel::MassAssignmentSecurity::Error)
+      }.to raise_error(ActionController::UnpermittedParameters)
     end
   end
 

spec/controllers/users/registrations_controller_spec.rb

@@ -5,13 +5,16 @@ module Users
     include Devise::TestHelpers
 
     describe "POST create" do
+      before do
+        @request.env["devise.mapping"] = Devise.mappings[:user]
+      end
+
       context 'with valid params' do
         it "imports the default scenario for the new user" do
           mock(DefaultScenarioImporter).import(is_a(User))
 
-          @request.env["devise.mapping"] = Devise.mappings[:user]
           post :create, :user => {username: 'jdoe', email: 'jdoe@example.com',
-            password: 's3cr3t55', password_confirmation: 's3cr3t55', admin: false, invitation_code: 'try-huginn'}
+            password: 's3cr3t55', password_confirmation: 's3cr3t55', invitation_code: 'try-huginn'}
         end
       end
 
@@ -19,10 +22,13 @@ module Users
         it "does not import the default scenario" do
           stub(DefaultScenarioImporter).import(is_a(User)) { fail "Should not attempt import" }
 
-          @request.env["devise.mapping"] = Devise.mappings[:user]
           setup_controller_for_warden
           post :create, :user => {}
         end
+
+        it 'does not allow to set the admin flag' do
+          expect { post :create, :user => {admin: 'true'} }.to raise_error(ActionController::UnpermittedParameters)
+        end
       end
     end
   end

spec/models/user_credential_spec.rb

@@ -8,14 +8,6 @@ describe UserCredential do
     it { should validate_presence_of(:user_id) }
   end
 
-  describe "mass assignment" do
-    it { should allow_mass_assignment_of :credential_name }
-
-    it { should allow_mass_assignment_of :credential_value }
-
-    it { should_not allow_mass_assignment_of :user_id }
-  end
-
   describe "cleaning fields" do
     it "should trim whitespace" do
       user_credential = user_credentials(:bob_aws_key)