SECURITY: Avoid mass assignment on user create

This commit is contained in:
Robin Ward 2016-08-05 11:57:13 -04:00
parent cda108da56
commit 429f27ec96
4 changed files with 101 additions and 10 deletions

View File

@ -343,7 +343,7 @@ class UsersController < ApplicationController
), ),
errors: user.errors.to_hash, errors: user.errors.to_hash,
values: user.attributes.slice('name', 'username', 'email'), values: user.attributes.slice('name', 'username', 'email'),
is_developer: UsernameCheckerService.new.is_developer?(user.email) is_developer: UsernameCheckerService.is_developer?(user.email)
} }
end end
rescue ActiveRecord::StatementInvalid rescue ActiveRecord::StatementInvalid
@ -675,10 +675,21 @@ class UsersController < ApplicationController
end end
def user_params def user_params
params.permit(:name, :email, :password, :username, :active, :staged) result = params.permit(:name, :email, :password, :username)
.merge(ip_address: request.remote_ip, .merge(ip_address: request.remote_ip,
registration_ip_address: request.remote_ip, registration_ip_address: request.remote_ip,
locale: user_locale) 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 end
def user_locale def user_locale

View File

@ -1,5 +1,4 @@
class SearchPostSerializer < PostSerializer class SearchPostSerializer < PostSerializer
has_one :topic, serializer: TopicListItemSerializer has_one :topic, serializer: TopicListItemSerializer
attributes :like_count attributes :like_count

View File

@ -23,4 +23,9 @@ class UsernameCheckerService
Rails.configuration.respond_to?(:developer_emails) && Rails.configuration.developer_emails.include?(value) Rails.configuration.respond_to?(:developer_emails) && Rails.configuration.developer_emails.include?(value)
end end
def self.is_developer?(email)
UsernameCheckerService.new.is_developer?(email)
end
end end

View File

@ -382,12 +382,15 @@ describe UsersController do
@user.password = "strongpassword" @user.password = "strongpassword"
end end
def post_user let(:post_user_params) do
xhr :post, :create, { name: @user.name,
name: @user.name,
username: @user.username, username: @user.username,
password: "strongpassword", password: "strongpassword",
email: @user.email email: @user.email }
end
def post_user
xhr :post, :create, post_user_params
end end
context 'when creating a user' do context 'when creating a user' do
@ -453,6 +456,79 @@ describe UsersController do
end end
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 context 'when creating an active user (confirmed email)' do
before { User.any_instance.stubs(:active?).returns(true) } before { User.any_instance.stubs(:active?).returns(true) }