Merge pull request #131 from ontoportal-lirmm/feature/fix-user-creati…

…on-security

Feature: enforce user creation security

Gemfile.lock

@@ -26,7 +26,7 @@ GIT
 
 GIT
   remote: https://github.com/ncbo/ncbo_cron.git
-  revision: bb93561f78522cf6b289afc81b3bf86cdbbb8cfc
+  revision: 6317dc4976d2ab8e17104887bab0abf5f412b2ef
   branch: develop
   specs:
     ncbo_cron (0.0.1)
@@ -110,7 +110,7 @@ GEM
     ast (2.4.2)
     backports (3.24.1)
     base64 (0.2.0)
-    bcrypt (3.1.19)
+    bcrypt (3.1.20)
     bcrypt_pbkdf (1.1.0)
     bigdecimal (1.4.2)
     builder (3.2.4)
@@ -133,10 +133,9 @@ GEM
     dante (0.2.0)
     date (3.3.4)
     docile (1.4.0)
-    domain_name (0.5.20190701)
-      unf (>= 0.0.5, < 1.0.0)
+    domain_name (0.6.20231109)
     ed25519 (1.3.0)
-    faraday (2.7.11)
+    faraday (2.7.12)
       base64
       faraday-net_http (>= 2.0, < 3.1)
       ruby2_keywords (>= 0.0.4)
@@ -166,10 +165,10 @@ GEM
     google-cloud-env (1.6.0)
       faraday (>= 0.17.3, < 3.0)
     google-cloud-errors (1.3.1)
-    google-protobuf (3.25.0-aarch64-linux)
-    google-protobuf (3.25.0-arm64-darwin)
-    google-protobuf (3.25.0-x86_64-darwin)
-    google-protobuf (3.25.0-x86_64-linux)
+    google-protobuf (3.25.1-aarch64-linux)
+    google-protobuf (3.25.1-arm64-darwin)
+    google-protobuf (3.25.1-x86_64-darwin)
+    google-protobuf (3.25.1-x86_64-linux)
     googleapis-common-protos (1.4.0)
       google-protobuf (~> 3.14)
       googleapis-common-protos-types (~> 1.2)
@@ -226,7 +225,7 @@ GEM
       redis
     multi_json (1.15.0)
     net-http-persistent (2.9.4)
-    net-imap (0.4.4)
+    net-imap (0.4.6)
       date
       net-protocol
     net-pop (0.1.2)
@@ -255,7 +254,7 @@ GEM
     pry (0.14.2)
       coderay (~> 1.1)
       method_source (~> 1.0)
-    public_suffix (5.0.3)
+    public_suffix (5.0.4)
     racc (1.7.3)
     rack (1.6.13)
     rack-accept (0.4.5)
@@ -344,7 +343,7 @@ GEM
       rack-test
       sinatra (~> 1.4.0)
       tilt (>= 1.3, < 3)
-    sshkit (1.21.5)
+    sshkit (1.21.6)
       net-scp (>= 1.1.2)
       net-ssh (>= 2.8.0)
     systemu (2.6.5)
@@ -353,9 +352,6 @@ GEM
     timeout (0.4.1)
     tzinfo (2.0.6)
       concurrent-ruby (~> 1.0)
-    unf (0.1.4)
-      unf_ext
-    unf_ext (0.0.8.2)
     unicode-display_width (2.5.0)
     unicorn (6.1.0)
       kgio (~> 2.6)
@@ -371,6 +367,7 @@ PLATFORMS
   arm64-darwin-22
   x86_64-darwin-18
   x86_64-darwin-21
+  x86_64-darwin-23
   x86_64-linux
 
 DEPENDENCIES
@@ -423,4 +420,4 @@ DEPENDENCIES
   unicorn-worker-killer
 
 BUNDLED WITH
-   2.3.15
+   2.4.21

controllers/users_controller.rb

@@ -81,6 +81,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
@@ -92,6 +93,7 @@ class UsersController < ApplicationController
 
     # Delete a user
     delete '/:username' do
+      error 403, "Access denied" unless current_user.admin?
       User.find(params[:username]).first.delete
       halt 204
     end
@@ -109,6 +111,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

test/controllers/test_slices_controller.rb

@@ -15,7 +15,7 @@ def self.before_suite
                         password: "12345"
                       }).save
     @@new_slice_data = { acronym: 'tst-c', name: "Test Slice C", ontologies: ont_acronyms}
-    @@old_security_setting = LinkedData.settings.enable_security
+    enable_security
   end
 
   def self.after_suite
@@ -26,7 +26,7 @@ def self.after_suite
 
   def setup
     self.class.reset_security
-    self.class.rest_to_not_admin(@@user)
+    self.class.reset_to_not_admin(@@user)
     LinkedData::Models::Slice.find(@@new_slice_data[:acronym]).first&.delete
   end
 
@@ -38,28 +38,28 @@ def test_all_slices
   end
 
   def test_create_slices
-    enable_security
+    self.class.enable_security
 
     post "/slices?apikey=#{@@user.apikey}", MultiJson.dump(@@new_slice_data), "CONTENT_TYPE" => "application/json"
     assert_equal 403, last_response.status
 
-    make_admin(@@user)
+    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
-    enable_security
+    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
 
-    make_admin(@@user)
+    self.class.make_admin(@@user)
 
     delete "/slices/#{@@new_slice_data[:acronym]}?apikey=#{@@user.apikey}"
     assert 201, last_response.status
@@ -76,23 +76,4 @@ def self._create_slice(acronym, name, ontologies)
     slice.save
   end
 
-  def enable_security
-    LinkedData.settings.enable_security = true
-  end
-
-  def self.reset_security
-    LinkedData.settings.enable_security = @@old_security_setting
-  end
-
-  def make_admin(user)
-    user.bring_remaining
-    user.role = [LinkedData::Models::Users::Role.find(LinkedData::Models::Users::Role::ADMIN).first]
-    user.save
-  end
-
-  def self.rest_to_not_admin(user)
-    user.bring_remaining
-    user.role = [LinkedData::Models::Users::Role.find(LinkedData::Models::Users::Role::DEFAULT).first]
-    user.save
-  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_user(@@username)
+  end
+
   def test_all_users
     get '/users'
     assert last_response.ok?
@@ -48,7 +59,8 @@ def test_create_new_user
     assert last_response.ok?
     assert MultiJson.load(last_response.body)["username"].eql?(@@username)
 
-    delete created_user["@id"]
+    _delete_user(created_user["username"])
+
     post "/users", MultiJson.dump(user.merge(username: @@username)), "CONTENT_TYPE" => "application/json"
     assert last_response.status == 201
     assert MultiJson.load(last_response.body)["username"].eql?(@@username)
@@ -79,13 +91,21 @@ def test_update_patch_user
   end
 
   def test_delete_user
-    delete "/users/ben"
-    assert last_response.status == 204
+    self.class.enable_security
+
+    delete "/users/ben?apikey=#{@@users.first.apikey}"
+    assert_equal 403,  last_response.status
+
+    self.class.make_admin(@@users.first)
+    delete "/users/ben?apikey=#{@@users.first.apikey}"
+    assert_equal 204,  last_response.status
 
     @@usernames.delete("ben")
+    self.class.reset_security
+    self.class.reset_to_not_admin(@@users.first)
 
     get "/users/ben"
-    assert last_response.status == 404
+    assert_equal 404, last_response.status
   end
 
   def test_user_not_found
@@ -100,4 +120,36 @@ def test_authentication
     assert user["username"].eql?(@@usernames.first)
   end
 
+
+  private
+
+  def _delete_user(username)
+    LinkedData::Models::User.find(@@username).first&.delete
+  end
+  def _create_admin_user(apikey: nil)
+    user = {email: "#{@@username}@example.org", password: "pass_the_word", role: ['ADMINISTRATOR']}
+    _delete_user(@@username)
+
+    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

@@ -192,4 +192,26 @@ def get_errors(response)
     return errors.strip
   end
 
+  def self.enable_security
+    @@old_security_setting = LinkedData.settings.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
+
 end