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

@@ -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"

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

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)

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: "[email protected]",
+                        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

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

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