From 479b24ea2781159c43423eec90f07ea00e46f9e8 Mon Sep 17 00:00:00 2001
From: alex-sig <143193681+alex-sig@users.noreply.github.com>
Date: Thu, 4 Apr 2024 13:12:50 +0530
Subject: [PATCH] OTWO-7200 Prevent unauthorized Org actions (#1782)

---
 app/controllers/concerns/org_filters.rb       |  9 ++++-
 config/locales/organizations.en.yml           |  1 +
 .../organizations_controller_test.rb          | 35 ++++++++++++++++---
 3 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/app/controllers/concerns/org_filters.rb b/app/controllers/concerns/org_filters.rb
index 5ebfbc7c9..bef0eb599 100644
--- a/app/controllers/concerns/org_filters.rb
+++ b/app/controllers/concerns/org_filters.rb
@@ -11,7 +11,7 @@ module OrgFilters
     before_action :session_required, only: %i[new create]
     before_action :admin_session_required, only: %i[new create]
     before_action :handle_default_view, only: :show
-    before_action :set_editor, only: %i[list_managers new_manager claim_projects_list claim_project]
+    before_action :set_editor, only: %i[list_managers new_manager claim_projects_list claim_project remove_project]
     before_action :show_permissions_alert, only: %i[manage_projects new_manager edit
                                                     claim_projects_list list_managers settings]
     before_action :redirect_ro_explores_pages, only: :index
@@ -19,6 +19,7 @@ module OrgFilters
     before_action :can_claim_project, only: :claim_project
     after_action :schedule_analysis, only: %i[claim_project remove_project]
     before_action :avoid_global_search, only: %i[manage_projects claim_projects_list]
+    before_action :org_must_be_edit_authorized, only: %i[new_manager remove_project]
   end
 
   def schedule_analysis
@@ -76,6 +77,12 @@ def can_claim_project
     render plain: t('.unauthorized') unless request.xhr? && @organization.edit_authorized?
   end
 
+  def org_must_be_edit_authorized
+    return if logged_in? && @organization.edit_authorized?
+
+    redirect_back fallback_location: @organization, notice: t('.unauthorized')
+  end
+
   def set_project
     @project = Project.from_param(params[:project_id]).take
     raise ParamRecordNotFound unless @project
diff --git a/config/locales/organizations.en.yml b/config/locales/organizations.en.yml
index 4e2213f92..3391ffe7d 100644
--- a/config/locales/organizations.en.yml
+++ b/config/locales/organizations.en.yml
@@ -1,5 +1,6 @@
 en:
   organizations:
+    unauthorized: 'You are not authorized to perform this action.'
     active_remove_project_button:
       remove_from: 'Remove %{project} from %{org}?'
       remove: 'Remove'
diff --git a/test/controllers/organizations_controller_test.rb b/test/controllers/organizations_controller_test.rb
index 4dd195312..1f1c5a89b 100644
--- a/test/controllers/organizations_controller_test.rb
+++ b/test/controllers/organizations_controller_test.rb
@@ -323,11 +323,12 @@ class OrganizationsControllerTest < ActionController::TestCase
   end
 
   describe 'remove_project' do
+    before { login_as account }
+
     it 'should remove project from org' do
-      login_as account
       pro1 = create(:project, name: 'test name1', organization_id: organization.id)
 
-      get :remove_project, params: { id: organization.to_param, project_id: pro1.id, source: 'manage_projects' }
+      put :remove_project, params: { id: organization.to_param, project_id: pro1.id, source: 'manage_projects' }
 
       assert_redirected_to manage_projects_organization_path(organization)
       _(flash[:success]).must_equal I18n.t('organizations.remove_project.success', name: pro1.name)
@@ -335,29 +336,53 @@ class OrganizationsControllerTest < ActionController::TestCase
     end
 
     it 'should remove project from org and redirect to claim_projects_list' do
-      login_as account
       pro1 = create(:project, name: 'test name1', organization_id: organization.id)
 
-      get :remove_project, params: { id: organization.to_param, project_id: pro1.id, source: 'claim_projects_list' }
+      put :remove_project, params: { id: organization.to_param, project_id: pro1.id, source: 'claim_projects_list' }
 
       assert_redirected_to claim_projects_list_organization_path(organization)
       _(flash[:success]).must_equal I18n.t('organizations.remove_project.success', name: pro1.name)
       _(pro1.reload.organization_id).must_be_nil
     end
+
+    it 'must prevent unauthorized project removal' do
+      restrict_edits_to_managers(organization)
+
+      project = create(:project, name: 'test name1', organization_id: organization.id)
+
+      put :remove_project, params: { id: organization.to_param, project_id: project.id, source: 'claim_projects_list' }
+
+      assert_redirected_to organization_path(organization)
+      _(flash[:notice]).must_equal I18n.t('organizations.unauthorized')
+      _(project.reload.organization_id).must_equal organization.id
+    end
   end
 
   describe 'new_manager' do
     it 'should show new manager form for get request' do
+      login_as account
       get :new_manager, params: { id: organization, account_id: account.id }
       assert_response :ok
     end
 
-    it 'should show new manager form for get request' do
+    it 'should show new manager form for post request' do
+      login_as account
+
       post :new_manager, params: { id: organization, account_id: account.id }
 
       assert_redirected_to list_managers_organization_path(organization)
       _(assigns(:manage).target).must_equal organization
     end
+
+    it 'must prevent unauthorized manager creation' do
+      restrict_edits_to_managers(organization)
+
+      post :new_manager, params: { id: organization, account_id: account.id }
+
+      assert_redirected_to organization_path(organization)
+      _(flash[:notice]).must_equal I18n.t('organizations.unauthorized')
+      _(organization.managers.pluck(:id)).must_be :empty?
+    end
   end
 
   describe 'create' do