From fe5b1f174b3cc6eae1b432daa754473b06d40f14 Mon Sep 17 00:00:00 2001 From: Isobel Redelmeier Date: Fri, 29 Sep 2017 15:01:48 -0700 Subject: [PATCH 1/2] Delete roles in Perm when deleting by username [Finishes #150978621, #150978620, #150978619] --- Gemfile.lock | 15 +-- .../runtime/organizations_controller.rb | 3 + lib/cloud_controller/perm/client.rb | 14 +++ spec/integration/perm_spec.rb | 102 +++++++++++++++++- spec/unit/lib/perm/client_spec.rb | 37 +++++++ 5 files changed, 160 insertions(+), 11 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 34d0a12db96..b408ff46793 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,6 +1,6 @@ GIT remote: https://github.com/cloudfoundry-incubator/perm-rb.git - revision: f1893eba5aab28bbb8c095234ece170171a3b71d + revision: 21553f380ccebbd670fc264e0edfca45341405be branch: master specs: cf-perm (0.0.0) @@ -187,9 +187,9 @@ GEM multi_json (~> 1.10) retriable (~> 1.4) signet (~> 0.6) - google-protobuf (3.4.1.1) google-protobuf (3.4.1.1-universal-darwin) - google-protobuf (3.4.1.1-x86_64-linux) + googleapis-common-protos-types (1.0.0) + google-protobuf (~> 3.0) googleauth (0.5.3) faraday (~> 0.12) jwt (~> 1.4) @@ -198,14 +198,9 @@ GEM multi_json (~> 1.11) os (~> 0.9) signet (~> 0.7) - grpc (1.6.0) - google-protobuf (~> 3.1) - googleauth (~> 0.5.1) - grpc (1.6.0-universal-darwin) - google-protobuf (~> 3.1) - googleauth (~> 0.5.1) - grpc (1.6.0-x86_64-linux) + grpc (1.6.2-universal-darwin) google-protobuf (~> 3.1) + googleapis-common-protos-types (~> 1.0.0) googleauth (~> 0.5.1) hashdiff (0.3.4) http-cookie (1.0.3) diff --git a/app/controllers/runtime/organizations_controller.rb b/app/controllers/runtime/organizations_controller.rb index 7b73f23f679..f10fb172342 100644 --- a/app/controllers/runtime/organizations_controller.rb +++ b/app/controllers/runtime/organizations_controller.rb @@ -225,8 +225,11 @@ def get_memory_usage(guid) if recursive_delete? && role == :user org.send("remove_#{role}_recursive", user) + space_ids = org.spaces.map(&:guid) + @perm_client.unassign_roles(org_ids: [guid], space_ids: space_ids, user_id: user_id, issuer: SecurityContext.token['iss']) else org.send("remove_#{role}", user) + @perm_client.unassign_org_role(role: role, org_id: guid, user_id: user_id, issuer: SecurityContext.token['iss']) end @user_event_repository.record_organization_role_remove( diff --git a/lib/cloud_controller/perm/client.rb b/lib/cloud_controller/perm/client.rb index 1325be7c0df..59f0f4dc5e1 100644 --- a/lib/cloud_controller/perm/client.rb +++ b/lib/cloud_controller/perm/client.rb @@ -40,6 +40,20 @@ def unassign_space_role(role:, space_id:, user_id:, issuer:) unassign_role(role: space_role(role, space_id), user_id: user_id, issuer: issuer) end + def unassign_roles(org_ids: [], space_ids: [], user_id:, issuer:) + space_ids.each do |space_id| + VCAP::CloudController::SpacesController::ROLE_NAMES.each do |role| + unassign_space_role(role: role, space_id: space_id, user_id: user_id, issuer: issuer) + end + end + + org_ids.each do |org_id| + VCAP::CloudController::OrganizationsController::ROLE_NAMES.each do |role| + unassign_org_role(role: role, org_id: org_id, user_id: user_id, issuer: issuer) + end + end + end + private attr_reader :client, :enabled diff --git a/spec/integration/perm_spec.rb b/spec/integration/perm_spec.rb index b02e759f0fb..01a2805bd0a 100644 --- a/spec/integration/perm_spec.rb +++ b/spec/integration/perm_spec.rb @@ -8,7 +8,7 @@ include ControllerHelpers let(:assigner) { VCAP::CloudController::IsolationSegmentAssign.new } - let(:assignee) { VCAP::CloudController::User.make } + let(:assignee) { VCAP::CloudController::User.make(username: 'not-really-a-person') } let(:uaa_target) { 'test.example.com' } let(:perm_host) { ENV.fetch('PERM_RPC_HOST') { 'localhost:6283' } } @@ -22,6 +22,7 @@ } allow_any_instance_of(VCAP::CloudController::UaaClient).to receive(:usernames_for_ids).with([assignee.guid]).and_return({ assignee.guid => assignee.username }) + allow_any_instance_of(VCAP::CloudController::UaaClient).to receive(:id_for_username).with(assignee.username).and_return(assignee.guid) allow_any_instance_of(VCAP::CloudController::UaaTokenDecoder).to receive(:uaa_issuer).and_return(issuer) set_current_user_as_admin(iss: issuer) @@ -214,6 +215,75 @@ end end + describe 'DELETE /v2/organizations/:guid/:role' do + let(:org) { VCAP::CloudController::Organization.make } + + ORG_ROLES.each do |role| + describe "DELETE /v2/organizations/:guid/#{role}s" do + let(:role_name) { "org-#{role}-#{org.guid}" } + + before do + client.create_role role_name + end + + it "removes the user from the org #{role} role" do + client.assign_role(role_name: role_name, actor_id: assignee.guid, issuer: issuer) + + delete "/v2/organizations/#{org.guid}/#{role}s", { 'username' => assignee.username }.to_json + expect(last_response.status).to eq(204) + + expect(client.has_role?(role_name: role_name, actor_id: assignee.guid, issuer: issuer)).to be(false) + roles = client.list_actor_roles(actor_id: assignee.guid, issuer: issuer) + expect(roles).to be_empty + end + + it "does nothing if the user does not have the org #{role} role" do + delete "/v2/organizations/#{org.guid}/#{role}s/#{assignee.guid}" + expect(last_response.status).to eq(204) + end + end + end + + describe 'DELETE /v2/organizations/:guid/users?recursive=true' do + let!(:org1) { VCAP::CloudController::Organization.make(user_guids: [assignee.guid]) } + let!(:org2) { VCAP::CloudController::Organization.make(user_guids: [assignee.guid]) } + let!(:org1_space) { VCAP::CloudController::Space.make(organization: org1) } + let!(:org2_space) { VCAP::CloudController::Space.make(organization: org2) } + + before do + client.create_role("org-user-#{org1.guid}") + client.assign_role(role_name: "org-user-#{org1.guid}", actor_id: assignee.guid, issuer: issuer) + client.create_role("org-user-#{org2.guid}") + client.assign_role(role_name: "org-user-#{org2.guid}", actor_id: assignee.guid, issuer: issuer) + + SPACE_ROLES.each do |role| + client.create_role("space-#{role}-#{org1_space.guid}") + put "/v2/spaces/#{org1_space.guid}/#{role}s/#{assignee.guid}" + expect(last_response.status).to eq(201) + client.create_role("space-#{role}-#{org2_space.guid}") + put "/v2/spaces/#{org2_space.guid}/#{role}s/#{assignee.guid}" + expect(last_response.status).to eq(201) + end + end + + it 'removes the user from all org and space roles for that org and no other' do + delete "/v2/organizations/#{org1.guid}/users?recursive=true", { 'username' => assignee.username }.to_json + expect(last_response.status).to eq(204) + + ORG_ROLES.each do |role| + expect(client.has_role?(role_name: "org-#{role}-#{org1.guid}", actor_id: assignee.guid, issuer: issuer)).to be(false) + end + + expect(client.has_role?(role_name: "org-user-#{org2.guid}", actor_id: assignee.guid, issuer: issuer)).to be(true) + + SPACE_ROLES.each do |role| + expect(client.has_role?(role_name: "space-#{role}-#{org1_space.guid}", actor_id: assignee.guid, issuer: issuer)).to be(false) + expect(client.has_role?(role_name: "space-#{role}-#{org2_space.guid}", actor_id: assignee.guid, issuer: issuer)).to be(true) + end + end + end + end + describe 'DELETE /v2/organizations/:guid/:role/:user_guid' do let(:org) { VCAP::CloudController::Organization.make } @@ -379,6 +449,36 @@ end end + describe 'DELETE /v2/spaces/:guid/:role' do + let(:org) { VCAP::CloudController::Organization.make } + let(:space) { + VCAP::CloudController::Space.make( + organization: org, + ) + } + + SPACE_ROLES.each do |role| + describe "DELETE /v2/spaces/:guid/#{role}s" do + let(:role_name) { "space-#{role}-#{space.guid}" } + + before do + client.create_role role_name + end + + it "removes the user from the space #{role} role" do + client.assign_role(actor_id: assignee.guid, issuer: issuer, role_name: role_name) + + delete "/v2/spaces/#{space.guid}/#{role}s", { 'username' => assignee.username }.to_json + expect(last_response.status).to eq(200) + + expect(client.has_role?(actor_id: assignee.guid, issuer: issuer, role_name: role_name)).to be(false) + roles = client.list_actor_roles(actor_id: assignee.guid, issuer: issuer) + expect(roles).to be_empty + end + end + end + end + describe 'DELETE /v2/spaces/:guid/:role/:user_guid' do let(:org) { VCAP::CloudController::Organization.make } let(:space) { diff --git a/spec/unit/lib/perm/client_spec.rb b/spec/unit/lib/perm/client_spec.rb index c90c872a840..d42d514a10a 100644 --- a/spec/unit/lib/perm/client_spec.rb +++ b/spec/unit/lib/perm/client_spec.rb @@ -111,6 +111,43 @@ module VCAP::CloudController::Perm end end + describe '#unassign_roles' do + let(:org_id2) { SecureRandom.uuid } + let(:space_id2) { SecureRandom.uuid } + + it 'unassigns the user from all roles for the org and spaces' do + subject.unassign_roles(org_ids: [org_id, org_id2], space_ids: [space_id, space_id2], user_id: user_id, issuer: issuer) + + [:user, :manager, :billing_manager, :auditor].each do |role| + expect(client).to have_received(:unassign_role). + with(role_name: "org-#{role}-#{org_id}", actor_id: user_id, issuer: issuer) + expect(client).to have_received(:unassign_role). + with(role_name: "org-#{role}-#{org_id2}", actor_id: user_id, issuer: issuer) + end + + [:developer, :manager, :auditor].each do |role| + expect(client).to have_received(:unassign_role). + with(role_name: "space-#{role}-#{space_id}", actor_id: user_id, issuer: issuer) + expect(client).to have_received(:unassign_role). + with(role_name: "space-#{role}-#{space_id2}", actor_id: user_id, issuer: issuer) + end + end + + it 'does not fail if something does not exist' do + allow(client).to receive(:unassign_role).and_raise(GRPC::NotFound) + + expect { + subject.unassign_roles(org_ids: [org_id], space_ids: [space_id], user_id: user_id, issuer: issuer) + }.not_to raise_error + end + + it 'does nothing when disabled' do + disabled_subject.unassign_org_role(role: 'developer', org_id: org_id, user_id: user_id, issuer: issuer) + + expect(client).not_to have_received(:unassign_role) + end + end + describe '#create_space_role' do it 'creates the correct role' do subject.create_space_role(role: 'developer', space_id: space_id) From 510cde0edf7e446c567f8584b552c7ae37db017f Mon Sep 17 00:00:00 2001 From: Isobel Redelmeier Date: Fri, 29 Sep 2017 16:09:43 -0700 Subject: [PATCH 2/2] Fix perm version --- Gemfile.lock | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index b408ff46793..34d0a12db96 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,6 +1,6 @@ GIT remote: https://github.com/cloudfoundry-incubator/perm-rb.git - revision: 21553f380ccebbd670fc264e0edfca45341405be + revision: f1893eba5aab28bbb8c095234ece170171a3b71d branch: master specs: cf-perm (0.0.0) @@ -187,9 +187,9 @@ GEM multi_json (~> 1.10) retriable (~> 1.4) signet (~> 0.6) + google-protobuf (3.4.1.1) google-protobuf (3.4.1.1-universal-darwin) - googleapis-common-protos-types (1.0.0) - google-protobuf (~> 3.0) + google-protobuf (3.4.1.1-x86_64-linux) googleauth (0.5.3) faraday (~> 0.12) jwt (~> 1.4) @@ -198,9 +198,14 @@ GEM multi_json (~> 1.11) os (~> 0.9) signet (~> 0.7) - grpc (1.6.2-universal-darwin) + grpc (1.6.0) + google-protobuf (~> 3.1) + googleauth (~> 0.5.1) + grpc (1.6.0-universal-darwin) + google-protobuf (~> 3.1) + googleauth (~> 0.5.1) + grpc (1.6.0-x86_64-linux) google-protobuf (~> 3.1) - googleapis-common-protos-types (~> 1.0.0) googleauth (~> 0.5.1) hashdiff (0.3.4) http-cookie (1.0.3)