Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: user creation security #60

Merged
merged 7 commits into from
Apr 3, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
@@ -467,4 +467,4 @@ DEPENDENCIES
webmock

BUNDLED WITH
2.4.21
2.3.23
1 change: 1 addition & 0 deletions bin/ontoportal
Original file line number Diff line number Diff line change
@@ -100,6 +100,7 @@ build_docker_run_cmd() {
eval "$docker_run_cmd"
}


# Function to handle the "dev" and "test" options
run_command() {
local custom_command="$1"
5 changes: 4 additions & 1 deletion controllers/slices_controller.rb
Original file line number Diff line number Diff line change
@@ -41,17 +41,20 @@
##
# Create a new slice
post do
error 403, "Access denied" unless current_user && current_user.admin?
create_slice
end

# Delete a slice
delete '/:slice' do
error 403, "Access denied" unless current_user && current_user.admin?
LinkedData::Models::Slice.find(params[:slice]).first.delete
halt 204
end

# Update an existing slice
patch '/:slice' do
error 403, "Access denied" unless current_user && current_user.admin?

Check warning on line 57 in controllers/slices_controller.rb

Codecov / codecov/patch

controllers/slices_controller.rb#L57

Added line #L57 was not covered by tests
slice = LinkedData::Models::Slice.find(params[:slice]).include(LinkedData::Models::Slice.attributes(:all)).first
populate_from_params(slice, params)
if slice.valid?
@@ -61,7 +64,7 @@
end
halt 204
end

private

def create_slice
2 changes: 2 additions & 0 deletions controllers/users_controller.rb
Original file line number Diff line number Diff line change
@@ -80,6 +80,7 @@ class UsersController < ApplicationController
# Update an existing submission of an user
patch '/:username' do
user = User.find(params[:username]).include(User.attributes).first
params.delete("role") unless current_user.admin?
populate_from_params(user, params)
if user.valid?
user.save
@@ -102,6 +103,7 @@ def create_user
params ||= @params
user = User.find(params["username"]).first
error 409, "User with username `#{params["username"]}` already exists" unless user.nil?
params.delete("role") unless current_user.admin?
user = instance_from_params(User, params)
if user.valid?
user.save(send_notifications: false)
67 changes: 58 additions & 9 deletions test/controllers/test_slices_controller.rb
Original file line number Diff line number Diff line change
@@ -3,28 +3,77 @@
class TestSlicesController < TestCase

def self.before_suite
onts = LinkedData::SampleData::Ontology.create_ontologies_and_submissions(ont_count: 1, submission_count: 0)[2]
ont_count, ont_acronyms, @@onts = LinkedData::SampleData::Ontology.create_ontologies_and_submissions(ont_count: 1, submission_count: 0)

@@slice_acronyms = ["tst-a", "tst-b"].sort
_create_slice(@@slice_acronyms[0], "Test Slice A", onts)
_create_slice(@@slice_acronyms[1], "Test Slice B", onts)
_create_slice(@@slice_acronyms[0], "Test Slice A", @@onts)
_create_slice(@@slice_acronyms[1], "Test Slice B", @@onts)

@@user = User.new({
username: "test-slice",
email: "test-slice@example.org",
password: "12345"
}).save
@@new_slice_data = { acronym: 'tst-c', name: "Test Slice C", ontologies: ont_acronyms}
@@old_security_setting = LinkedData.settings.enable_security
end

def self.after_suite
LinkedData::Models::Slice.all.each(&:delete)
@@user.delete
reset_security(@@old_security_setting)
end

def setup
self.class.reset_security(@@old_security_setting)
self.class.reset_to_not_admin(@@user)
LinkedData::Models::Slice.find(@@new_slice_data[:acronym]).first&.delete
end

def test_all_slices
get "/slices"
assert last_response.ok?
slices = MultiJson.load(last_response.body)
assert_equal @@slice_acronyms, slices.map {|s| s["acronym"]}.sort
assert_equal @@slice_acronyms, slices.map { |s| s["acronym"] }.sort
end

def test_create_slices
self.class.enable_security

post "/slices?apikey=#{@@user.apikey}", MultiJson.dump(@@new_slice_data), "CONTENT_TYPE" => "application/json"
assert_equal 403, last_response.status

self.class.make_admin(@@user)

post "/slices?apikey=#{@@user.apikey}", MultiJson.dump(@@new_slice_data), "CONTENT_TYPE" => "application/json"

assert 201, last_response.status
end

def test_delete_slices
self.class.enable_security
LinkedData.settings.enable_security = @@old_security_setting
self.class._create_slice(@@new_slice_data[:acronym], @@new_slice_data[:name], @@onts)


delete "/slices/#{@@new_slice_data[:acronym]}?apikey=#{@@user.apikey}"
assert_equal 403, last_response.status

self.class.make_admin(@@user)

delete "/slices/#{@@new_slice_data[:acronym]}?apikey=#{@@user.apikey}"
assert 201, last_response.status
end

private

def self._create_slice(acronym, name, ontologies)
slice = LinkedData::Models::Slice.new({
acronym: acronym,
name: "Test #{name}",
ontologies: ontologies
})
acronym: acronym,
name: "Test #{name}",
ontologies: ontologies
})
slice.save
end
end

end
41 changes: 40 additions & 1 deletion test/controllers/test_users_controller.rb
Original file line number Diff line number Diff line change
@@ -6,7 +6,7 @@ def self.before_suite
@@usernames = %w(fred goerge henry ben mark matt charlie)

# Create them again
@@usernames.each do |username|
@@users = @@usernames.map do |username|
User.new(username: username, email: "#{username}@example.org", password: "pass_word").save
end

@@ -21,6 +21,17 @@ def self._delete_users
end
end

def test_admin_creation
existent_user = @@users.first #no admin

refute _create_admin_user(apikey: existent_user.apikey), "A no admin user can't create an admin user or update it to an admin"

existent_user = self.class.make_admin(existent_user)
assert _create_admin_user(apikey: existent_user.apikey), "Admin can create an admin user or update it to be an admin"
self.class.reset_to_not_admin(existent_user)
delete "/users/#{@@username}"
end

def test_all_users
get '/users'
assert last_response.ok?
@@ -136,4 +147,32 @@ def test_oauth_authentication
assert data[:email], user["email"]
end
end

private
def _create_admin_user(apikey: nil)
user = {email: "#{@@username}@example.org", password: "pass_the_word", role: ['ADMINISTRATOR']}
LinkedData::Models::User.find(@@username).first&.delete

put "/users/#{@@username}", MultiJson.dump(user), "CONTENT_TYPE" => "application/json", "Authorization" => "apikey token=#{apikey}"
assert last_response.status == 201
created_user = MultiJson.load(last_response.body)
assert created_user["username"].eql?(@@username)

get "/users/#{@@username}?apikey=#{apikey}"
assert last_response.ok?
user = MultiJson.load(last_response.body)
assert user["username"].eql?(@@username)

return true if user["role"].eql?(['ADMINISTRATOR'])

patch "/users/#{@@username}", MultiJson.dump(role: ['ADMINISTRATOR']), "CONTENT_TYPE" => "application/json", "Authorization" => "apikey token=#{apikey}"
assert last_response.status == 204

get "/users/#{@@username}?apikey=#{apikey}"
assert last_response.ok?
user = MultiJson.load(last_response.body)
assert user["username"].eql?(@@username)

true if user["role"].eql?(['ADMINISTRATOR'])
end
end
22 changes: 22 additions & 0 deletions test/test_case.rb
Original file line number Diff line number Diff line change
@@ -200,6 +200,27 @@ def get_errors(response)
return errors.strip
end

def self.enable_security
LinkedData.settings.enable_security = true
end

def self.reset_security(old_security = @@old_security_setting)
LinkedData.settings.enable_security = old_security
end


def self.make_admin(user)
user.bring_remaining
user.role = [LinkedData::Models::Users::Role.find(LinkedData::Models::Users::Role::ADMIN).first]
user.save
end

def self.reset_to_not_admin(user)
user.bring_remaining
user.role = [LinkedData::Models::Users::Role.find(LinkedData::Models::Users::Role::DEFAULT).first]
user.save
end

def unused_port
max_retries = 5
retries = 0
@@ -219,4 +240,5 @@ def port_in_use?(port)
rescue Errno::EADDRINUSE
true
end

end