From a139e469a7bd178aa987432534d87934c08cdd8f Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Fri, 5 Aug 2016 11:57:13 -0400 Subject: [PATCH] SECURITY: Avoid mass assignment on user create --- app/controllers/users_controller.rb | 19 +++-- app/serializers/search_post_serializer.rb | 1 - app/services/username_checker_service.rb | 5 ++ spec/controllers/users_controller_spec.rb | 84 +++++++++++++++++++++-- 4 files changed, 100 insertions(+), 9 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index d20d090c4fb..f85adeb16e1 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -343,7 +343,7 @@ class UsersController < ApplicationController ), errors: user.errors.to_hash, values: user.attributes.slice('name', 'username', 'email'), - is_developer: UsernameCheckerService.new.is_developer?(user.email) + is_developer: UsernameCheckerService.is_developer?(user.email) } end rescue ActiveRecord::StatementInvalid @@ -674,9 +674,20 @@ class UsersController < ApplicationController end def user_params - params.permit(:name, :email, :password, :username, :active) - .merge(ip_address: request.remote_ip, registration_ip_address: request.remote_ip, - locale: user_locale) + result = params.permit(:name, :email, :password, :username) + .merge(ip_address: request.remote_ip, + registration_ip_address: request.remote_ip, + locale: user_locale) + + if !UsernameCheckerService.is_developer?(result['email']) && + is_api? && + current_user.present? && + current_user.admin? + + result.merge!(params.permit(:active, :staged)) + end + + result end def user_locale diff --git a/app/serializers/search_post_serializer.rb b/app/serializers/search_post_serializer.rb index 614ba8b7901..1e92d044bec 100644 --- a/app/serializers/search_post_serializer.rb +++ b/app/serializers/search_post_serializer.rb @@ -1,5 +1,4 @@ class SearchPostSerializer < PostSerializer - has_one :topic, serializer: TopicListItemSerializer attributes :like_count diff --git a/app/services/username_checker_service.rb b/app/services/username_checker_service.rb index 64beba09b60..91bfe42de93 100644 --- a/app/services/username_checker_service.rb +++ b/app/services/username_checker_service.rb @@ -23,4 +23,9 @@ class UsernameCheckerService Rails.configuration.respond_to?(:developer_emails) && Rails.configuration.developer_emails.include?(value) end + + def self.is_developer?(email) + UsernameCheckerService.new.is_developer?(email) + end + end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index ff0e0d58c4e..92843c1c6fa 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -382,12 +382,15 @@ describe UsersController do @user.password = "strongpassword" end - def post_user - xhr :post, :create, - name: @user.name, + let(:post_user_params) do + { name: @user.name, username: @user.username, password: "strongpassword", - email: @user.email + email: @user.email } + end + + def post_user + xhr :post, :create, post_user_params end context 'when creating a user' do @@ -453,6 +456,79 @@ describe UsersController do end end + context "creating as active" do + it "won't create the user as active" do + xhr :post, :create, post_user_params.merge(active: true) + expect(JSON.parse(response.body)['active']).to be_falsey + end + + context "with a regular api key" do + let(:user) { Fabricate(:user) } + let(:api_key) { Fabricate(:api_key, user: user) } + + it "won't create the user as active with a regular key" do + xhr :post, :create, post_user_params.merge(active: true, api_key: api_key.key) + expect(JSON.parse(response.body)['active']).to be_falsey + end + end + + context "with an admin api key" do + let(:user) { Fabricate(:admin) } + let(:api_key) { Fabricate(:api_key, user: user) } + + it "creates the user as active with a regular key" do + xhr :post, :create, post_user_params.merge(active: true, api_key: api_key.key) + expect(JSON.parse(response.body)['active']).to be_truthy + end + + it "won't create the developer as active" do + UsernameCheckerService.expects(:is_developer?).returns(true) + + xhr :post, :create, post_user_params.merge(active: true, api_key: api_key.key) + expect(JSON.parse(response.body)['active']).to be_falsy + end + end + end + + context "creating as staged" do + it "won't create the user as staged" do + xhr :post, :create, post_user_params.merge(staged: true) + new_user = User.where(username: post_user_params[:username]).first + expect(new_user.staged?).to eq(false) + end + + context "with a regular api key" do + let(:user) { Fabricate(:user) } + let(:api_key) { Fabricate(:api_key, user: user) } + + it "won't create the user as staged with a regular key" do + xhr :post, :create, post_user_params.merge(staged: true, api_key: api_key.key) + new_user = User.where(username: post_user_params[:username]).first + expect(new_user.staged?).to eq(false) + end + end + + context "with an admin api key" do + let(:user) { Fabricate(:admin) } + let(:api_key) { Fabricate(:api_key, user: user) } + + it "creates the user as staged with a regular key" do + xhr :post, :create, post_user_params.merge(staged: true, api_key: api_key.key) + + new_user = User.where(username: post_user_params[:username]).first + expect(new_user.staged?).to eq(true) + end + + it "won't create the developer as staged" do + UsernameCheckerService.expects(:is_developer?).returns(true) + xhr :post, :create, post_user_params.merge(staged: true, api_key: api_key.key) + + new_user = User.where(username: post_user_params[:username]).first + expect(new_user.staged?).to eq(false) + end + end + end + context 'when creating an active user (confirmed email)' do before { User.any_instance.stubs(:active?).returns(true) }