Fixes multiple assignment bug

Fixes #679

`<<` instantly saves the added association. We were adding the project from the route parameter onto the site object temporarily, assuming the site would not be saved.

Instead whenever the route was accessed with any project id, that project id was added to the list of available projects - creating an security bypass.

I also took this opportunity to disallow the creation of new sites that belong to multiple projects. It is a feature we plan to remove anyway.

We still have to allow updating multi-project sites - at least until we can ensure there are no examples of them in the database.

app/controllers/sites_controller.rb

@@ -42,7 +42,7 @@ def show
   def new
     do_new_resource
     get_project_if_exists
-    do_set_attributes
+    do_set_attributes(site_params(create: true))
     do_authorize_instance
 
     # initialize lat/lng to Brisbane-ish
@@ -65,7 +65,7 @@ def edit
   # POST /projects/:project_id/sites
   def create
     do_new_resource
-    do_set_attributes(site_params)
+    do_set_attributes(site_params(for_create: true))
     get_project_if_exists
     do_authorize_instance
 
@@ -96,7 +96,7 @@ def update
     @original_site_name = @site.name
 
     respond_to do |format|
-      if @site.update(site_params)
+      if @site.update(site_params(for_create: false))
         format.html { redirect_to [@project, @site], notice: 'Site was successfully updated.' }
         format.json { respond_show }
       else
@@ -225,16 +225,23 @@ def nav_menu
 
   def get_project
     @project = Project.find(params[:project_id])
-    # avoid the same project assigned more than once to a site
-    @site.projects << @project if defined?(@site) && !@site.projects.include?(@project)
   end
 
   def get_project_if_exists
+    # none of this matters for the shallow routes
     return unless params.key?(:project_id)
 
-    @project = Project.find(params[:project_id])
-    # avoid the same project assigned more than once to a site
-    @site.projects << @project if defined?(@site) && defined?(@project) && !@site.projects.include?(@project)
+    project_id = params[:project_id].to_i
+
+    # for show/edit/update, check that the site belongs to the project
+    if @site.present? && !@site.new_record? && !@site.project_ids.include?(project_id)
+
+      # this site does not belong to the project in the route parameter
+      raise ActiveRecord::RecordNotFound
+    end
+
+    # for index just load it to use for the permissions check
+    @project = Project.find(project_id)
   end
 
   def list_permissions
@@ -245,13 +252,27 @@ def list_permissions
     end
   end
 
-  def site_params
-    params.require(:site).permit(
+  def site_params(for_create:)
+    result = params.require(:site).permit(
       :name, :latitude, :longitude, :description, :image, :notes, :tzinfo_tz, :region_id, project_ids: []
     )
-  end
 
-  def site_show_params
-    params.permit(:id, :project_id, site: {})
+    # normalize the project_ids between the route parameter and the body
+    # route param does not exist for shallow routes
+    if params.key?(:project_id)
+      # is it also in the body?
+      if result.key?(:project_ids)
+        # if so it should be a subset of the supplied project_ids
+        unless result[:project_ids].include?(params[:project_id].to_i)
+          raise CustomErrors::BadRequestError, '`project_ids` must include the project id in the route parameter'
+        end
+      elsif for_create
+        # otherwise fill the body with the route parameter
+        # but not for updates - we don't want to change the project unless explicitly requested
+        result[:project_ids] = [params[:project_id].to_i]
+      end
+    end
+
+    result
   end
 end

app/models/site.rb

@@ -100,6 +100,11 @@ class Site < ApplicationRecord
     content_type: %r{^image/(jpg|jpeg|pjpeg|png|x-png|gif)$},
     message: 'file type %<value>s is not allowed (only jpeg/png/gif images)'
 
+  # AT 2024: soft deprecating sites existing in more than one project
+  # Causes many issues and is officially replaced by the project-region-site relationship
+  # Later work will remove the projects_sites join table
+  validate :only_one_site_per_project, on: :create
+
   # commonly used queries
   #scope :specified_sites, lambda { |site_ids| where('id in (:ids)', { :ids => site_ids } ) }
   #scope :sites_in_project, lambda { |project_ids| where(Project.specified_projects, { :ids => project_ids } ) }
@@ -222,6 +227,12 @@ def self.add_location_jitter(value, min, max, seed)
     modified_value
   end
 
+  def only_one_site_per_project
+    return if project_ids.count == 1
+
+    errors.add(:project_ids, 'Site must belong to exactly one project')
+  end
+
   # Define filter api settings
   def self.filter_settings
     {

spec/factories/site_factory.rb

@@ -44,6 +44,7 @@
     sequence(:description) { |n| "site description #{n}" }
 
     creator
+    projects { [create(:project)] }
 
     trait :with_lat_long do
       # Random.rand returns "a random integer greater than or equal to zero and less than the argument"
@@ -65,7 +66,7 @@
         raise 'Creator was blank' if evaluator.creator.blank?
 
         create_list(:audio_recording_with_audio_events_and_bookmarks, evaluator.audio_recording_count,
-                    site: site, creator: evaluator.creator, uploader: evaluator.creator)
+          site:, creator: evaluator.creator, uploader: evaluator.creator)
       end
     end
 

spec/models/site_spec.rb

@@ -193,8 +193,8 @@
   it { is_expected.to belong_to(:deleter).with_foreign_key(:deleter_id).optional }
 
   it 'errors on checking orphaned site if site is orphaned' do
-    # depends on factory not automatically associating a site with any projects
     site = create(:site)
+    site.projects = []
     expect {
       Access::Core.check_orphan_site!(site)
     }.to raise_error(CustomErrors::OrphanedSiteError)

spec/requests/sites_spec.rb

@@ -12,7 +12,7 @@
         }
       }
       post "/projects/#{project.id}/sites", params: body, headers: api_request_headers(owner_token, send_body: true),
-as: :json
+        as: :json
       expect(response).to have_http_status(:success)
       expect(api_result).to include(data: hash_including({
         timezone_information: hash_including({
@@ -35,7 +35,8 @@
       expect_error(400, 'Site testy test site () is not in any projects.')
     end
 
-    example 'can create an a site that belongs to multiple projects' do
+    # AT 2024: soft-deprecating the many to many project-site relationship
+    example 'canNOT create an a site that belongs to multiple projects' do
       second_project = create(:project)
       body = {
         site: {
@@ -46,14 +47,9 @@
 
       post '/sites', params: body, headers: api_request_headers(owner_token, send_body: true), as: :json
 
-      aggregate_failures do
-        expect(response).to have_http_status(:success)
-        expect(api_result).to include({
-          data: hash_including({
-            project_ids: match_array([project.id, second_project.id])
-          })
-        })
-      end
+      expect_error(:unprocessable_entity, 'Record could not be saved', {
+        project_ids: ['Site must belong to exactly one project']
+      })
     end
 
     example 'update a site so that it belongs to multiple projects' do
@@ -118,4 +114,167 @@
       )])
     end
   end
+
+  describe 'accidental assignment bug' do
+    # https://github.com/QutEcoacoustics/baw-server/issues/679
+    create_entire_hierarchy
+
+    let(:site) { create(:site) }
+    let(:another_site) { create(:site) }
+
+    def assert_isolation
+      site.reload
+      another_site.reload
+
+      aggregate_failures do
+        expect(another_site.project_ids).to match([another_site.projects.first[:id]])
+        expect(another_site.project_ids).not_to include(*site.projects.map(&:id))
+
+        expect(site.projects.map(&:site_ids)).not_to include(another_site.id)
+        expect(another_site.projects.map(&:site_ids)).not_to include(site.id)
+
+        expect(site.project_ids).to match(site.projects.map(&:id))
+        expect(site.project_ids).not_to include(*another_site.projects.map(&:id))
+      end
+    end
+
+    before do
+      Permission.new(
+        user: owner_user, project: site.projects.first, level: :owner,
+        creator: admin_user
+      ).save!
+      Permission.new(
+        user: owner_user, project: another_site.projects.first, level: :owner, creator: admin_user
+      ).save!
+
+      assert_isolation
+    end
+
+    after do
+      assert_isolation
+    end
+
+    it 'does not assign a site to the wrong project' do
+      get "/projects/#{site.projects.first.id}/sites/#{another_site.id}", **api_headers(owner_token)
+
+      expect_error(:not_found, 'Could not find the requested item.')
+    end
+
+    it 'does not assign a site to the wrong project (anonymous)' do
+      get "/projects/#{site.projects.first.id}/sites/#{another_site.id}", **api_headers(anonymous_token)
+
+      expect_error(:not_found, 'Could not find the requested item.')
+    end
+
+    it 'does not assign a site to the wrong project (filter)' do
+      get "/projects/#{site.projects.first.id}/sites/#{another_site.id}/filter", **api_headers(owner_token)
+
+      expect_error(:not_found, 'Could not find the requested page.')
+    end
+
+    it 'does not assign a site to the wrong project (update)' do
+      body = {
+        site: {
+          name: 'new name'
+        }
+      }
+
+      patch "/projects/#{site.projects.first.id}/sites/#{another_site.id}", params: body,
+        **api_with_body_headers(owner_token)
+
+      expect_error(:not_found, 'Could not find the requested item.')
+    end
+
+    it 'we can still create sites' do
+      body = {
+        site: {
+          name: 'new name',
+          project_ids: [site.projects.first.id]
+        }
+      }
+
+      post "/projects/#{site.projects.first.id}/sites", params: body, **api_with_body_headers(owner_token)
+
+      expect_success
+    end
+
+    it 'we can still create sites (but we cant mix ids)' do
+      body = {
+        site: {
+          name: 'new name',
+          project_ids: [another_site.projects.first.id]
+        }
+      }
+
+      post "/projects/#{site.projects.first.id}/sites", params: body, **api_with_body_headers(owner_token)
+
+      expect_error(:bad_request,
+        'The request was not valid: `project_ids` must include the project id in the route parameter')
+    end
+
+    it 'we can still create sites (with just a route parameter)' do
+      body = {
+        site: {
+          name: 'new name'
+        }
+      }
+
+      post "/projects/#{site.projects.first.id}/sites", params: body, **api_with_body_headers(owner_token)
+
+      expect_success
+    end
+
+    it 'we can still create sites (with just a body parameter)' do
+      body = {
+        site: {
+          name: 'new name',
+          project_ids: [site.projects.first.id]
+        }
+      }
+
+      post '/sites', params: body, **api_with_body_headers(owner_token)
+
+      expect_success
+    end
+
+    it 'rejects multiple project ids' do
+      body = {
+        site: {
+          name: 'new name',
+          project_ids: [site.projects.first.id, another_site.project_ids.first]
+        }
+      }
+
+      post "/projects/#{site.projects.first.id}/sites", params: body, **api_with_body_headers(owner_token)
+
+      expect_error(:unprocessable_entity, 'Record could not be saved', {
+        project_ids: ['Site must belong to exactly one project']
+      })
+    end
+
+    it 'fails if project id is invalid' do
+      get "/projects/999999/sites/#{another_site.id}", **api_headers(owner_token)
+      expect_error(:not_found, 'Could not find the requested item.')
+    end
+
+    # lastly some sanity checks
+
+    it 'can list sites' do
+      get "/projects/#{site.projects.first.id}/sites", **api_headers(owner_token)
+
+      expect_success
+    end
+
+    it 'can show a site' do
+      get "/projects/#{site.projects.first.id}/sites/#{site.id}", **api_headers(owner_token)
+
+      expect_success
+    end
+
+    it 'can filter sites' do
+      get "/projects/#{site.projects.first.id}/sites/filter", **api_headers(owner_token)
+
+      expect_success
+    end
+  end
 end

spec/requests/stats_spec.rb

@@ -23,7 +23,7 @@
         users_online: an_instance_of(Integer),
         users_total: User.count,
         online_window_start: an_instance_of(String),
-        projects_total: 2,
+        projects_total: 3,
         regions_total: 1,
         sites_total: 3,
         annotations_total: 2,

spec/support/creation_helper.rb

@@ -381,8 +381,8 @@ def create_region(creator, project)
       end
 
       def create_site(creator, project, region: nil, name: nil)
-        site = FactoryBot.create(:site, :with_lat_long, creator:)
-        site.projects << project
+        site = FactoryBot.build(:site, :with_lat_long, creator:, projects: [project])
+
         site.region = region unless region.nil?
         site.name = name unless name.nil?
         site.save!