Allow admins (and only admins) to change owner (#2169)

* Allow admins (and only admins) to change owner

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

* Admin changing owner: Add explanation and more tests

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

---------

Signed-off-by: David A. Wheeler <dwheeler@dwheeler.com>

app/controllers/projects_controller.rb

@@ -274,11 +274,15 @@ def create
   # rubocop:disable Metrics/AbcSize, Metrics/MethodLength
   # rubocop:disable Metrics/PerceivedComplexity
   def update
-    if repo_url_change_allowed?
+    # Only accept updates if there's no repo_url change OR if change is ok
+    if repo_url_unchanged_or_change_allowed?
       # Send CDN purge early, to give it time to distribute purge request
       @project.purge_cdn_project
       old_badge_level = @project.badge_level
-      project_params.each do |key, user_value| # mass assign
+      final_project_params = project_params
+      # Only admins can *directly* change the project owner (user_id)
+      final_project_params = project_params.except('user_id') unless current_user.admin?
+      final_project_params.each do |key, user_value| # mass assign
         @project[key] = user_value
       end
       Chief.new(@project, client_factory).autofill
@@ -611,7 +615,7 @@ def repo_url_delay_expired?
   # But otherwise normal users can't change the repo_urls in less than
   # REPO_URL_CHANGE_DELAY days.  Allowing users to change repo_urls, but only
   # with large delays, reduces the administration effort required.
-  def repo_url_change_allowed?
+  def repo_url_unchanged_or_change_allowed?
     return true unless @project.repo_url?
     return true if project_params[:repo_url].nil?
     return true if current_user.admin?

app/models/project.rb

@@ -202,7 +202,11 @@ class Project < ApplicationRecord
               message: :begin_with_cpe
             }
 
-  validates :user_id, presence: true
+  validates :user_id,
+            numericality: {
+              only_integer: true,
+              greater_than_or_equal_to: 1
+            }
 
   Criteria.each_value do |criteria|
     criteria.each_value do |criterion|

app/views/projects/_form_0.html.erb

@@ -310,7 +310,20 @@
                 '0', 'Basics', 'Other', f, project, is_disabled
               ) %>
         </ul>
-
+      <% if current_user && current_user.admin? %>
+        <div class="row">
+        <div class="col-xs-12">
+          <br>
+            New owner id (a positive integer):
+            <%= f.text_field :user_id,
+                             hide_label: true, class:"form-control",
+                             placeholder: nil,
+                             spellcheck: false,
+                             disabled: is_disabled %><br>
+            If changed, consider adding the old owner to additional rights list.
+        </div>
+        </div>
+      <% end %>
       <div class="row">
       <div class="col-xs-12">
         <br>

test/controllers/projects_controller_test.rb

@@ -589,6 +589,44 @@ def only_correct_criteria_selectable(level)
     assert_equal new_name, @project.name
   end
 
+  test 'admin can change owner of other users project' do
+    log_in_as(@admin)
+    old_user = @project.user
+    assert_not_equal @admin.id, old_user.id
+    # We SHOULD see the option to change the owner id
+    get "/en/projects/#{@project.id}/edit"
+    assert_response :success
+    assert_includes @response.body, 'New owner id'
+    # Let's ensure we CAN change it.
+    # Admin will own this project after this instruction.
+    patch "/en/projects/#{@project.id}", params: {
+      project: { user_id: @admin.id }
+    }
+    assert_redirected_to project_path(assigns(:project))
+    @project.reload
+    assert_equal @admin.id, @project.user_id
+  end
+
+  # We don't currently allow normal users to change the owner to
+  # anyone else, in case the recipient doesn't want it.
+  test 'Normal user cannot change owner of their own project' do
+    # Verify test setup - @project is owned by @user
+    assert_equal @project.user_id, @user.id
+    log_in_as(@user)
+    # We should NOT see the option to change the owner id
+    get "/en/projects/#{@project.id}/edit"
+    assert_response :success
+    assert_not_includes @response.body, 'New owner id'
+    # Let's ensure we can't change it.
+    patch "/en/projects/#{@project.id}", params: {
+      project: { user_id: @admin.id }
+    }
+    assert_redirected_to project_path(assigns(:project))
+    @project.reload
+    # Notice that nothing has changed.
+    assert_equal @project.user_id, @user.id
+  end
+
   test 'Cannot evade /badge match with /badge/..' do
     get "/projects/#{@perfect_passing_project.id}/badge/..",
         params: { format: 'svg' }