From 0617d048fdca57ff4adb5a30a4dd6c72d6d3b827 Mon Sep 17 00:00:00 2001 From: Anthony Truskinger Date: Fri, 9 Aug 2024 09:57:37 +1000 Subject: [PATCH] Fixes multiple assignment bug (#680) 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 | 47 ++++++-- app/models/site.rb | 13 ++ spec/factories/site_factory.rb | 3 +- spec/models/site_spec.rb | 2 +- spec/requests/sites_spec.rb | 177 ++++++++++++++++++++++++++-- spec/requests/stats_spec.rb | 2 +- spec/support/creation_helper.rb | 4 +- 7 files changed, 220 insertions(+), 28 deletions(-) diff --git a/app/controllers/sites_controller.rb b/app/controllers/sites_controller.rb index 41679462..e8204071 100644 --- a/app/controllers/sites_controller.rb +++ b/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(for_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 diff --git a/app/models/site.rb b/app/models/site.rb index 7d00cac5..42df0274 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -100,6 +100,13 @@ class Site < ApplicationRecord content_type: %r{^image/(jpg|jpeg|pjpeg|png|x-png|gif)$}, message: 'file type %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. + # Can't enforce this for updates because it would prevent any update for + # sites that are currently in more than one project. + 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 +229,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 { diff --git a/spec/factories/site_factory.rb b/spec/factories/site_factory.rb index 33bd4d6e..bfcc6988 100644 --- a/spec/factories/site_factory.rb +++ b/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 diff --git a/spec/models/site_spec.rb b/spec/models/site_spec.rb index 4b4171ce..13dff9c7 100644 --- a/spec/models/site_spec.rb +++ b/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) diff --git a/spec/requests/sites_spec.rb b/spec/requests/sites_spec.rb index 7717888e..5c639a9c 100644 --- a/spec/requests/sites_spec.rb +++ b/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 'can *NOT* 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,165 @@ )]) end end + + describe 'accidental assignment bug' do + # https://github.com/QutEcoacoustics/baw-server/issues/679 + 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 diff --git a/spec/requests/stats_spec.rb b/spec/requests/stats_spec.rb index fed1ee74..2e5652b1 100644 --- a/spec/requests/stats_spec.rb +++ b/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, diff --git a/spec/support/creation_helper.rb b/spec/support/creation_helper.rb index e3e1a27a..2ee71efd 100644 --- a/spec/support/creation_helper.rb +++ b/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!