Remove protected_atrributes gem and start moving to strong params

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

10 files changed, 39 insertions, 40 deletions

diff --git a/Gemfile b/Gemfile
index 39ffd95b2e2..233271e0aa3 100644
--- a/Gemfile
+++ b/Gemfile
@@ -10,8 +10,6 @@ end
 
 gem "rails", "~> 4.1.0"
 
-gem "protected_attributes"
-
 # Make links from text
 gem 'rails_autolink', '~> 1.1'
 
diff --git a/Gemfile.lock b/Gemfile.lock
index 382633c2246..987959d6805 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -331,8 +331,6 @@ GEM
       websocket-driver (>= 0.2.0)
     polyglot (0.3.4)
     posix-spawn (0.3.8)
-    protected_attributes (1.0.5)
-      activemodel (>= 4.0.1, < 5.0)
     pry (0.9.12.4)
       coderay (~> 1.0)
       method_source (~> 0.8)
@@ -635,7 +633,6 @@ DEPENDENCIES
   org-ruby
   pg
   poltergeist (~> 1.5.1)
-  protected_attributes
   pry
   quiet_assets (~> 1.0.1)
   rack-attack
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index ffe65cb41c5..bf05845effe 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -42,7 +42,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def new
-    @issue = @project.issues.new(params[:issue])
+    @issue = @project.issues.new(issue_params)
     respond_with(@issue)
   end
 
@@ -59,7 +59,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def create
-    @issue = Issues::CreateService.new(project, current_user, params[:issue]).execute
+    @issue = Issues::CreateService.new(project, current_user, issue_params).execute
 
     respond_to do |format|
       format.html do
@@ -76,7 +76,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def update
-    @issue = Issues::UpdateService.new(project, current_user, params[:issue]).execute(issue)
+    @issue = Issues::UpdateService.new(project, current_user, issue_params).execute(issue)
 
     respond_to do |format|
       format.js
@@ -144,4 +144,11 @@ class Projects::IssuesController < Projects::ApplicationController
       raise ActiveRecord::RecordNotFound.new
     end
   end
+
+  def issue_params
+    params.require(:issue).permit(
+      :title, :assignee_id, :position, :description,
+      :milestone_id, :label_list, :state_event
+    )
+  end
 end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 89f4ab01a3f..6e7dfd7c8af 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -60,7 +60,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def new
-    @merge_request = MergeRequest.new(params[:merge_request])
+    @merge_request = MergeRequest.new(merge_request_params)
     @merge_request.source_project = @project unless @merge_request.source_project
     @merge_request.target_project ||= (@project.forked_from_project || @project)
     @target_branches = @merge_request.target_project.nil? ? [] : @merge_request.target_project.repository.branch_names
@@ -110,7 +110,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def create
     @target_branches ||= []
-    @merge_request = MergeRequests::CreateService.new(project, current_user, params[:merge_request]).execute
+    @merge_request = MergeRequests::CreateService.new(project, current_user, merge_request_params).execute
 
     if @merge_request.valid?
       redirect_to project_merge_request_path(@merge_request.target_project, @merge_request), notice: 'Merge request was successfully created.'
@@ -122,7 +122,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def update
-    @merge_request = MergeRequests::UpdateService.new(project, current_user, params[:merge_request]).execute(@merge_request)
+    @merge_request = MergeRequests::UpdateService.new(project, current_user, merge_request_params).execute(@merge_request)
 
     if @merge_request.valid?
       respond_to do |format|
@@ -263,4 +263,12 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     can?(current_user, action, project)
   end
+
+  def merge_request_params
+    params.require(:merge_request).permit(
+      :title, :assignee_id, :source_project_id, :source_branch,
+      :target_project_id, :target_branch, :milestone_id,
+      :state_event, :description, :label_list
+    )
+  end
 end
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 0d15b458b70..597efa40ded 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -20,7 +20,7 @@ class ProjectsController < ApplicationController
   end
 
   def create
-    @project = ::Projects::CreateService.new(current_user, params[:project]).execute
+    @project = ::Projects::CreateService.new(current_user, project_params).execute
     flash[:notice] = 'Project was successfully created.' if @project.saved?
 
     respond_to do |format|
@@ -29,7 +29,7 @@ class ProjectsController < ApplicationController
   end
 
   def update
-    status = ::Projects::UpdateService.new(@project, current_user, params).execute
+    status = ::Projects::UpdateService.new(@project, current_user, project_params).execute
 
     respond_to do |format|
       if status
@@ -44,7 +44,7 @@ class ProjectsController < ApplicationController
   end
 
   def transfer
-    ::Projects::TransferService.new(project, current_user, params[:project]).execute
+    ::Projects::TransferService.new(project, current_user, project_params).execute
   end
 
   def show
@@ -85,7 +85,7 @@ class ProjectsController < ApplicationController
       redirect_to import_project_path(@project)
     end
 
-    @project.import_url = params[:project][:import_url]
+    @project.import_url = project_params[:import_url]
 
     if @project.save
       @project.reload
@@ -185,4 +185,12 @@ class ProjectsController < ApplicationController
   def user_layout
     current_user ? "projects" : "public_projects"
   end
+
+  def project_params
+    params.require(:project).permit(
+      :name, :path, :description, :issues_tracker, :label_list,
+      :issues_enabled, :merge_requests_enabled, :snippets_enabled, :issues_tracker_id,
+      :wiki_enabled, :visibility_level, :import_url, :last_activity_at, :namespace_id
+    )
+  end
 end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index f0c2e552273..a116a9354cb 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -33,9 +33,6 @@ class Issue < ActiveRecord::Base
   scope :of_group, ->(group) { where(project_id: group.project_ids) }
   scope :of_user_team, ->(team) { where(project_id: team.project_ids, assignee_id: team.member_ids) }
 
-  attr_accessible :title, :assignee_id, :position, :description,
-                  :milestone_id, :label_list, :state_event
-
   acts_as_taggable_on :labels
 
   scope :cared, ->(user) { where(assignee_id: user) }
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index bfea209bf6d..367f8281430 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -36,10 +36,6 @@ class MergeRequest < ActiveRecord::Base
 
   delegate :commits, :diffs, :last_commit, :last_commit_short_sha, to: :merge_request_diff, prefix: nil
 
-  attr_accessible :title, :assignee_id, :source_project_id, :source_branch,
-                  :target_project_id, :target_branch, :milestone_id,
-                  :state_event, :description, :label_list
-
   attr_accessor :should_remove_source_branch
 
   # When this attribute is true some MR validation is ignored
diff --git a/app/models/project.rb b/app/models/project.rb
index 762b540b7a3..fde4a31ff7c 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -38,12 +38,6 @@ class Project < ActiveRecord::Base
 
   ActsAsTaggableOn.strict_case_match = true
 
-  attr_accessible :name, :path, :description, :issues_tracker, :label_list,
-    :issues_enabled, :merge_requests_enabled, :snippets_enabled, :issues_tracker_id,
-    :wiki_enabled, :visibility_level, :import_url, :last_activity_at, as: [:default, :admin]
-
-  attr_accessible :namespace_id, :creator_id, as: :admin
-
   acts_as_taggable_on :labels, :issues_default_labels
 
   attr_accessor :new_default_branch
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index 551a3653cad..d21bba69b51 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -1,19 +1,19 @@
 module Projects
   class UpdateService < BaseService
-    def execute(role = :default)
-      params[:project].delete(:namespace_id)
+    def execute
+      params.delete(:namespace_id)
       # check that user is allowed to set specified visibility_level
-      unless can?(current_user, :change_visibility_level, project) && Gitlab::VisibilityLevel.allowed_for?(current_user, params[:project][:visibility_level])
-        params[:project].delete(:visibility_level)
+      unless can?(current_user, :change_visibility_level, project) && Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
+        params.delete(:visibility_level)
       end
 
-      new_branch = params[:project].delete(:default_branch)
+      new_branch = params.delete(:default_branch)
 
       if project.repository.exists? && new_branch && new_branch != project.default_branch
         project.change_head(new_branch)
       end
 
-      if project.update_attributes(params[:project], as: role)
+      if project.update_attributes(params)
         if project.previous_changes.include?('namespace_id')
           project.send_move_instructions
         end
diff --git a/config/application.rb b/config/application.rb
index 0a77f58f6d1..58a5949c653 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -41,12 +41,6 @@ module Gitlab
     # 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
-
     # Enable the asset pipeline
     config.assets.enabled = true
     config.assets.paths << Emoji.images_path