From 16024fb5665cb1cacf24205decdc248f6e35111c Mon Sep 17 00:00:00 2001 From: Syphax bouazzouni Date: Wed, 3 Apr 2024 15:00:12 +0200 Subject: [PATCH] Fix: user creation security (#60) * extract slice tests helper to the parent class for reusability * add a test for the creation of an admin user * enforce the security of admin user creation * update slices controller to enforce admin security --- bin/ontoportal | 1 + controllers/slices_controller.rb | 5 +- controllers/users_controller.rb | 2 + test/controllers/test_slices_controller.rb | 67 +++++++++++++++++++--- test/controllers/test_users_controller.rb | 41 ++++++++++++- test/test_case.rb | 22 +++++++ 6 files changed, 127 insertions(+), 11 deletions(-) diff --git a/bin/ontoportal b/bin/ontoportal index ea93cebd..66f1a654 100755 --- a/bin/ontoportal +++ b/bin/ontoportal @@ -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" diff --git a/controllers/slices_controller.rb b/controllers/slices_controller.rb index a31f799e..9033222c 100644 --- a/controllers/slices_controller.rb +++ b/controllers/slices_controller.rb @@ -41,17 +41,20 @@ class SlicesController < ApplicationController ## # 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? 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 @@ class SlicesController < ApplicationController end halt 204 end - + private def create_slice diff --git a/controllers/users_controller.rb b/controllers/users_controller.rb index 09a1835b..d647c9dc 100644 --- a/controllers/users_controller.rb +++ b/controllers/users_controller.rb @@ -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) diff --git a/test/controllers/test_slices_controller.rb b/test/controllers/test_slices_controller.rb index 92ce6b1d..601b15a7 100644 --- a/test/controllers/test_slices_controller.rb +++ b/test/controllers/test_slices_controller.rb @@ -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 \ No newline at end of file diff --git a/test/controllers/test_users_controller.rb b/test/controllers/test_users_controller.rb index 3710b503..a165a5d7 100644 --- a/test/controllers/test_users_controller.rb +++ b/test/controllers/test_users_controller.rb @@ -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 diff --git a/test/test_case.rb b/test/test_case.rb index 43a736b3..d58bcace 100644 --- a/test/test_case.rb +++ b/test/test_case.rb @@ -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