Make app works with strong params

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

17 files changed, 84 insertions, 90 deletions

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 85dce0db8f2..44c93471df4 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -13,7 +13,7 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def new
-    @user = User.build_user
+    @user = User.new
   end
 
   def edit
@@ -37,15 +37,12 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def create
-    admin = user_params.delete("admin")
-
     opts = {
       force_random_password: true,
       password_expires_at: Time.now
     }
 
-    @user = User.build_user(user_params.merge(opts), as: :admin)
-    @user.admin = (admin && admin.to_i > 0)
+    @user = User.new(user_params.merge(opts))
     @user.created_by_id = current_user.id
     @user.generate_password
     @user.skip_confirmation!
@@ -62,19 +59,15 @@ class Admin::UsersController < Admin::ApplicationController
   end
 
   def update
-    admin = user_params.delete("admin")
-
-    if user_params[:password].blank?
-      user_params.delete(:password)
-      user_params.delete(:password_confirmation)
-    end
-
-    if admin.present?
-      user.admin = !admin.to_i.zero?
+    if params[:user][:password].present?
+      user_params.merge(
+        password: params[:user][:password],
+        password_confirmation: params[:user][:password_confirmation],
+      )
     end
 
     respond_to do |format|
-      if user.update_attributes(user_params, as: :admin)
+      if user.update_attributes(user_params)
         user.confirm!
         format.html { redirect_to [:admin, user], notice: 'User was successfully updated.' }
         format.json { head :ok }
@@ -118,10 +111,10 @@ class Admin::UsersController < Admin::ApplicationController
 
   def user_params
     params.require(:user).permit(
-      :email, :password, :password_confirmation, :remember_me, :bio, :name, :username,
+      :email, :remember_me, :bio, :name, :username,
       :skype, :linkedin, :twitter, :website_url, :color_scheme_id, :theme_id, :force_random_password,
       :extern_uid, :provider, :password_expires_at, :avatar, :hide_no_ssh_key,
-      :projects_limit, :can_create_group,
+      :projects_limit, :can_create_group, :admin
     )
   end
 end
diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index 40c352dab0c..f3f0e69b83a 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -7,7 +7,7 @@ class Profiles::EmailsController < ApplicationController
   end
 
   def create
-    @email = current_user.emails.new(params[:email])
+    @email = current_user.emails.new(email_params)
 
     flash[:alert] = @email.errors.full_messages.first unless @email.save
 
@@ -23,4 +23,10 @@ class Profiles::EmailsController < ApplicationController
       format.js { render nothing: true }
     end
   end
+
+  private
+
+  def email_params
+    params.require(:email).permit(:email)
+  end
 end
diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index f7c9651d050..e877f9b9049 100644
--- a/app/controllers/profiles_controller.rb
+++ b/app/controllers/profiles_controller.rb
@@ -14,7 +14,7 @@ class ProfilesController < ApplicationController
   end
 
   def update
-    user_params.delete(:email) if @user.ldap_user?
+    user_params.except!(:email) if @user.ldap_user?
 
     if @user.update_attributes(user_params)
       flash[:notice] = "Profile was successfully updated"
diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb
index 6e1a76ff417..d20937ea8ea 100644
--- a/app/controllers/projects/deploy_keys_controller.rb
+++ b/app/controllers/projects/deploy_keys_controller.rb
@@ -22,7 +22,7 @@ class Projects::DeployKeysController < Projects::ApplicationController
   end
 
   def create
-    @key = DeployKey.new(params[:deploy_key])
+    @key = DeployKey.new(deploy_key_params)
 
     if @key.valid? && @project.deploy_keys << @key
       redirect_to project_deploy_keys_path(@project)
@@ -58,4 +58,8 @@ class Projects::DeployKeysController < Projects::ApplicationController
   def available_keys
     @available_keys ||= current_user.accessible_deploy_keys
   end
+
+  def deploy_key_params
+    params.require(:deploy_key).permit(:key, :title)
+  end
 end
diff --git a/app/models/note.rb b/app/models/note.rb
index 436b75adc5e..ed4829b2b39 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -61,13 +61,13 @@ class Note < ActiveRecord::Base
     def create_status_change_note(noteable, project, author, status, source)
       body = "_Status changed to #{status}#{' by ' + source.gfm_reference if source}_"
 
-      create({
+      create(
         noteable: noteable,
         project: project,
         author: author,
         note: body,
         system: true
-      }, without_protection: true)
+      )
     end
 
     # +noteable+ was referenced from +mentioner+, by including GFM in either +mentioner+'s description or an associated Note.
@@ -86,7 +86,7 @@ class Note < ActiveRecord::Base
         note_options.merge!(noteable: noteable)
       end
 
-      create(note_options, without_protection: true)
+      create(note_options)
     end
 
     def create_milestone_change_note(noteable, project, author, milestone)
@@ -96,13 +96,13 @@ class Note < ActiveRecord::Base
                "_Milestone changed to #{milestone.title}_"
              end
 
-      create({
+      create(
         noteable: noteable,
         project: project,
         author: author,
         note: body,
         system: true
-      }, without_protection: true)
+      )
     end
 
     def create_assignee_change_note(noteable, project, author, assignee)
@@ -114,7 +114,7 @@ class Note < ActiveRecord::Base
         author: author,
         note: body,
         system: true
-      }, without_protection: true)
+      })
     end
 
     def discussions_from_notes(notes)
diff --git a/app/models/project.rb b/app/models/project.rb
index fde4a31ff7c..c242fb0fc5f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -27,14 +27,17 @@
 class Project < ActiveRecord::Base
   include Gitlab::ShellAdapter
   include Gitlab::VisibilityLevel
+  include Gitlab::ConfigHelper
+  extend Gitlab::ConfigHelper
   extend Enumerize
 
   default_value_for :archived, false
-  default_value_for :issues_enabled, true
-  default_value_for :merge_requests_enabled, true
-  default_value_for :wiki_enabled, true
+  default_value_for :visibility_level, gitlab_config_features.visibility_level
+  default_value_for :issues_enabled, gitlab_config_features.issues
+  default_value_for :merge_requests_enabled, gitlab_config_features.merge_requests
+  default_value_for :wiki_enabled, gitlab_config_features.wiki
   default_value_for :wall_enabled, false
-  default_value_for :snippets_enabled, true
+  default_value_for :snippets_enabled, gitlab_config_features.snippets
 
   ActsAsTaggableOn.strict_case_match = true
 
@@ -249,7 +252,7 @@ class Project < ActiveRecord::Base
   end
 
   def web_url
-    [Gitlab.config.gitlab.url, path_with_namespace].join("/")
+    [gitlab_config.url, path_with_namespace].join("/")
   end
 
   def web_url_without_protocol
@@ -470,7 +473,7 @@ class Project < ActiveRecord::Base
   end
 
   def http_url_to_repo
-    [Gitlab.config.gitlab.url, "/", path_with_namespace, ".git"].join('')
+    [gitlab_config.url, "/", path_with_namespace, ".git"].join('')
   end
 
   # Check if current branch name is marked as protected in the system
diff --git a/app/models/user.rb b/app/models/user.rb
index 6ce57f086bc..5fca392d350 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -50,10 +50,15 @@ require 'carrierwave/orm/activerecord'
 require 'file_size_validator'
 
 class User < ActiveRecord::Base
+  include Gitlab::ConfigHelper
+  extend Gitlab::ConfigHelper
+
   default_value_for :admin, false
-  default_value_for :can_create_group, true
+  default_value_for :can_create_group, gitlab_config.default_can_create_group
   default_value_for :can_create_team, false
   default_value_for :hide_no_ssh_key, false
+  default_value_for :projects_limit, gitlab_config.default_projects_limit
+  default_value_for :theme_id, gitlab_config.default_theme
 
   devise :database_authenticatable, :token_authenticatable, :lockable, :async,
          :recoverable, :rememberable, :trackable, :validatable, :omniauthable, :confirmable, :registerable
@@ -211,20 +216,8 @@ class User < ActiveRecord::Base
       where('users.username = ? OR users.id = ?', name_or_id.to_s, name_or_id.to_i).first
     end
 
-    def build_user(attrs = {}, options= {})
-      if options[:as] == :admin
-        User.new(defaults.merge(attrs.symbolize_keys), options)
-      else
-        User.new(attrs, options).with_defaults
-      end
-    end
-
-    def defaults
-      {
-        projects_limit: Gitlab.config.gitlab.default_projects_limit,
-        can_create_group: Gitlab.config.gitlab.default_can_create_group,
-        theme_id: Gitlab.config.gitlab.default_theme
-      }
+    def build_user(attrs = {})
+      User.new(attrs)
     end
   end
 
@@ -302,7 +295,7 @@ class User < ActiveRecord::Base
   end
 
   def can_change_username?
-    Gitlab.config.gitlab.username_changing_enabled
+    gitlab_config.username_changing_enabled
   end
 
   def can_create_project?
@@ -477,7 +470,7 @@ class User < ActiveRecord::Base
 
   def avatar_url(size = nil)
     if avatar.present?
-      URI::join(Gitlab.config.gitlab.url, avatar.url).to_s
+      URI::join(gitlab_config.url, avatar.url).to_s
     else
       GravatarService.new.execute(email, size)
     end
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 169e1e95b4b..a0e57144435 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -1,7 +1,7 @@
 module Issues
   class UpdateService < Issues::BaseService
     def execute(issue)
-      state = params.delete('state_event') || params.delete(:state_event)
+      state = params[:state_event]
 
       case state
       when 'reopen'
@@ -10,7 +10,7 @@ module Issues
         Issues::CloseService.new(project, current_user, {}).execute(issue)
       end
 
-      if params.present? && issue.update_attributes(params)
+      if params.present? && issue.update_attributes(params.except(:state_event))
         issue.reset_events_cache
 
         if issue.previous_changes.include?('milestone_id')
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index f1aa8b73930..6e416a0080c 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -7,10 +7,10 @@ module MergeRequests
     def execute(merge_request)
       # We dont allow change of source/target projects
       # after merge request was created
-      params.delete(:source_project_id)
-      params.delete(:target_project_id)
+      params.except!(:source_project_id)
+      params.except!(:target_project_id)
 
-      state = params.delete('state_event') || params.delete(:state_event)
+      state = params[:state_event]
 
       case state
       when 'reopen'
@@ -19,7 +19,7 @@ module MergeRequests
         MergeRequests::CloseService.new(project, current_user, {}).execute(merge_request)
       end
 
-      if params.present? && merge_request.update_attributes(params)
+      if params.present? && merge_request.update_attributes(params.except(:state_event))
         merge_request.reset_events_cache
 
         if merge_request.previous_changes.include?('milestone_id')
diff --git a/app/services/milestones/update_service.rb b/app/services/milestones/update_service.rb
index 307e96a2b36..ed64847f429 100644
--- a/app/services/milestones/update_service.rb
+++ b/app/services/milestones/update_service.rb
@@ -1,7 +1,7 @@
 module Milestones
   class UpdateService < Milestones::BaseService
     def execute(milestone)
-      state = params.delete('state_event') || params.delete(:state_event)
+      state = params[:state_event]
 
       case state
       when 'activate'
@@ -11,7 +11,7 @@ module Milestones
       end
 
       if params.present?
-        milestone.update_attributes(params)
+        milestone.update_attributes(params.except(:state_event))
       end
 
       milestone
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index dfadcfd296a..3565e4e4f70 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -5,27 +5,13 @@ module Projects
     end
 
     def execute
-      # get namespace id
-      namespace_id = params.delete(:namespace_id)
+      @project = Project.new(params)
 
-      # check that user is allowed to set specified visibility_level
+      # Reset visibility levet if is not allowed to set it
       unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
-        params.delete(:visibility_level)
+        @project.visibility_level = default_features.visibility_level
       end
 
-      # Load default feature settings
-      default_features = Gitlab.config.gitlab.default_projects_features
-
-      default_opts = {
-        issues_enabled: default_features.issues,
-        wiki_enabled: default_features.wiki,
-        snippets_enabled: default_features.snippets,
-        merge_requests_enabled: default_features.merge_requests,
-        visibility_level: default_features.visibility_level
-      }.stringify_keys
-
-      @project = Project.new(default_opts.merge(params))
-
       # Parametrize path for project
       #
       # Ex.
@@ -33,13 +19,14 @@ module Projects
       #
       @project.path = @project.name.dup.parameterize unless @project.path.present?
 
+      # get namespace id
+      namespace_id = params[:namespace_id]
 
       if namespace_id
         # Find matching namespace and check if it allowed
         # for current user if namespace_id passed.
-        if allowed_namespace?(current_user, namespace_id)
-          @project.namespace_id = namespace_id
-        else
+        unless allowed_namespace?(current_user, namespace_id)
+          @project.namespace_id = nil
           deny_namespace
           return @project
         end
diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb
index d115e92a105..e39fe882cb1 100644
--- a/app/services/projects/transfer_service.rb
+++ b/app/services/projects/transfer_service.rb
@@ -12,7 +12,7 @@ module Projects
     class TransferError < StandardError; end
 
     def execute
-      namespace_id = params.delete(:namespace_id)
+      namespace_id = params[:namespace_id]
       namespace = Namespace.find_by(id: namespace_id)
 
       if allowed_transfer?(current_user, project, namespace)
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index d21bba69b51..36877a61679 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -1,23 +1,18 @@
 module Projects
   class UpdateService < BaseService
     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[:visibility_level])
-        params.delete(:visibility_level)
+        params[:visibility_level] = project.visibility_level
       end
 
-      new_branch = params.delete(:default_branch)
+      new_branch = params[:default_branch]
 
       if project.repository.exists? && new_branch && new_branch != project.default_branch
         project.change_head(new_branch)
       end
 
-      if project.update_attributes(params)
-        if project.previous_changes.include?('namespace_id')
-          project.send_move_instructions
-        end
-
+      if project.update_attributes(params.except(:default_branch))
         if project.previous_changes.include?('path')
           project.rename_repo
         end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index b6a5806d646..d7d209e16f7 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -98,10 +98,14 @@ module API
 
     def attributes_for_keys(keys)
       attrs = {}
+
       keys.each do |key|
-        attrs[key] = params[key] if params[key].present? or (params.has_key?(key) and params[key] == false)
+        if params[key].present? or (params.has_key?(key) and params[key] == false)
+          attrs[key] = params[key]
+        end
       end
-      attrs
+
+      ActionController::Parameters.new(attrs).permit!
     end
 
     # error helpers
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 92dbe97f0a4..6085edab599 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -59,7 +59,7 @@ module API
         authenticated_as_admin!
         required_attributes! [:email, :password, :name, :username]
         attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio, :can_create_group, :admin]
-        user = User.build_user(attrs, as: :admin)
+        user = User.build_user(attrs)
         admin = attrs.delete(:admin)
         user.admin = admin unless admin.nil?
         if user.save
diff --git a/lib/gitlab/config_helper.rb b/lib/gitlab/config_helper.rb
new file mode 100644
index 00000000000..41880069e4c
--- /dev/null
+++ b/lib/gitlab/config_helper.rb
@@ -0,0 +1,9 @@
+module Gitlab::ConfigHelper
+  def gitlab_config_features
+    Gitlab.config.gitlab.default_projects_features
+  end
+
+  def gitlab_config
+    Gitlab.config.gitlab
+  end
+end
diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb
index 38e33c0eee5..94d59180e15 100644
--- a/lib/gitlab/oauth/user.rb
+++ b/lib/gitlab/oauth/user.rb
@@ -27,7 +27,7 @@ module Gitlab
             password_confirmation: password,
           }
 
-          user = model.build_user(opts, as: :admin)
+          user = model.build_user(opts)
           user.skip_confirmation!
 
           # Services like twitter and github does not return email via oauth