apply review feedback

Author: johha

app/controllers/v3/organizations_controller.rb

@@ -51,7 +51,7 @@ def create
     message = VCAP::CloudController::OrganizationCreateMessage.new(hashed_params[:body])
     unprocessable!(message.errors.full_messages) unless message.valid?
 
-    unauthorized! if message.requested?(:suspended) && message.suspended && !permission_queryer.can_write_globally?
+    unauthorized! if suspended_by_non_admin?(message)
 
     org = nil
 
@@ -81,7 +81,7 @@ def update
     message = VCAP::CloudController::OrganizationUpdateMessage.new(hashed_params[:body])
     unprocessable!(message.errors.full_messages) unless message.valid?
 
-    unauthorized! if message.requested?(:suspended) && message.suspended && !permission_queryer.can_write_globally?
+    unauthorized! if suspended_by_non_admin?(message)
 
     org = OrganizationUpdate.new(user_audit_info).update(org, message)
 
@@ -227,6 +227,11 @@ def isolation_segment_not_found!
     resource_not_found!(:isolation_segment)
   end
 
+  def suspended_by_non_admin?(message)
+    message.requested?(:suspended) && message.suspended && !permission_queryer.can_write_globally?
+  end
+
+
   def fetch_org(guid)
     Organization.where(guid:).first
   end

spec/request/organizations_spec.rb

@@ -171,32 +171,35 @@ module VCAP::CloudController
       end
 
       context 'when a non-admin sets the suspended field' do
+        let(:org) { Organization.make }
+        let(:space) { Space.make(organization: org) }
+
         before do
           VCAP::CloudController::FeatureFlag.make(name: 'user_org_creation', enabled: true)
         end
 
-        it 'returns 403 NotAuthorized for suspended: true' do
-          suspended_request_body = { name: 'sneaky-org', suspended: true }.to_json
-
-          expect do
-            post '/v3/organizations', suspended_request_body, user_header
-          end.not_to change(Organization, :count)
+        context 'with suspended: true' do
+          let(:api_call) do
+            ->(user_headers) { post '/v3/organizations', { name: "org-#{SecureRandom.uuid}", suspended: true }.to_json, user_headers }
+          end
+          let(:expected_codes_and_responses) do
+            h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze)
+            h['admin'] = { code: 201 }
+            h
+          end
 
-          expect(last_response.status).to eq(403)
-          expect(last_response).to have_error_message('You are not authorized to perform the requested action')
+          it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
         end
 
-        it 'allows suspended: false (the default) as a no-op' do
-          request_body = { name: 'noop-suspended-org', suspended: false }.to_json
-
-          expect do
-            post '/v3/organizations', request_body, user_header
-          end.to change(Organization, :count).by 1
+        context 'with suspended: false (no-op)' do
+          let(:api_call) do
+            ->(user_headers) { post '/v3/organizations', { name: "org-#{SecureRandom.uuid}", suspended: false }.to_json, user_headers }
+          end
+          let(:expected_codes_and_responses) do
+            Hash.new({ code: 201 }.freeze)
+          end
 
-          expect(last_response.status).to eq(201)
-          created_org = Organization.last
-          expect(created_org.name).to eq('noop-suspended-org')
-          expect(created_org).not_to be_suspended
+          it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
         end
       end
     end
@@ -1338,54 +1341,56 @@ module VCAP::CloudController
         it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
       end
 
-      context 'when a OrgManager mutates the suspended field' do
-        let(:org_manager) { VCAP::CloudController::User.make }
-        let(:org_manager_header) { headers_for(org_manager) }
-
-        before do
-          organization1.add_user(org_manager)
-          organization1.add_manager(org_manager)
-        end
+      context 'when a non-admin mutates the suspended field' do
+        let(:org) { Organization.make }
+        let(:space) { Space.make(organization: org) }
 
-        it 'returns 403 NotAuthorized when attempting to suspend an active org' do
-          patch "/v3/organizations/#{organization1.guid}",
-                { suspended: true }.to_json,
-                org_manager_header.merge('CONTENT_TYPE' => 'application/json')
+        context 'on an active org with suspended: true' do
+          let(:api_call) do
+            ->(user_headers) { patch "/v3/organizations/#{org.guid}", { suspended: true }.to_json, user_headers.merge('CONTENT_TYPE' => 'application/json') }
+          end
+          let(:expected_codes_and_responses) do
+            h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze)
+            h['admin'] = { code: 200 }
+            h['no_role'] = { code: 404 }
+            h
+          end
 
-          expect(last_response.status).to eq(403)
-          expect(last_response).to have_error_message('You are not authorized to perform the requested action')
-          expect(organization1.reload).not_to be_suspended
+          it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
         end
 
-        it 'returns CF-OrgSuspended when attempting to un-suspend a suspended org' do
-          organization1.update(status: VCAP::CloudController::Organization::SUSPENDED)
-
-          patch "/v3/organizations/#{organization1.guid}",
-                { suspended: false }.to_json,
-                org_manager_header.merge('CONTENT_TYPE' => 'application/json')
-
-          expect(last_response.status).to eq(403)
-          expect(last_response).to have_error_message('The organization is suspended')
-          expect(organization1.reload).to be_suspended
-        end
+        context 'on a suspended org with suspended: false (un-suspend)' do
+          let(:api_call) do
+            ->(user_headers) { patch "/v3/organizations/#{org.guid}", { suspended: false }.to_json, user_headers.merge('CONTENT_TYPE' => 'application/json') }
+          end
+          let(:expected_codes_and_responses) do
+            h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze)
+            h['admin'] = { code: 200 }
+            h['org_manager'] = { code: 403, errors: CF_ORG_SUSPENDED }
+            h['no_role'] = { code: 404 }
+            h
+          end
 
-        it 'allows benign updates that do not touch the suspended field' do
-          patch "/v3/organizations/#{organization1.guid}",
-                { name: 'renamed-by-manager' }.to_json,
-                org_manager_header.merge('CONTENT_TYPE' => 'application/json')
+          before do
+            org.update(status: VCAP::CloudController::Organization::SUSPENDED)
+          end
 
-          expect(last_response.status).to eq(200)
-          expect(organization1.reload.name).to eq('renamed-by-manager')
+          it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
         end
 
-        it 'allows a no-op suspended update (suspended matches current state)' do
-          patch "/v3/organizations/#{organization1.guid}",
-                { name: 'still-active', suspended: false }.to_json,
-                org_manager_header.merge('CONTENT_TYPE' => 'application/json')
+        context 'on an active org with suspended: false (no-op)' do
+          let(:api_call) do
+            ->(user_headers) { patch "/v3/organizations/#{org.guid}", { suspended: false }.to_json, user_headers.merge('CONTENT_TYPE' => 'application/json') }
+          end
+          let(:expected_codes_and_responses) do
+            h = Hash.new({ code: 403, errors: CF_NOT_AUTHORIZED }.freeze)
+            h['admin'] = { code: 200 }
+            h['org_manager'] = { code: 200 }
+            h['no_role'] = { code: 404 }
+            h
+          end
 
-          expect(last_response.status).to eq(200)
-          expect(organization1.reload.name).to eq('still-active')
-          expect(organization1.reload).not_to be_suspended
+          it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
         end
       end
     end