Allow unsharing from a space without org visibilty by ablease · Pull Request #1024 · cloudfoundry/cloud_controller_ng

ablease · 2017-12-05T16:27:59Z

As an app dev (sharer), I can unshare a service instance from a space where I do not have access.
#153244755

NOTE: This PR builds on top of #1023, which should be merged first. The actual changes on top of #1023 can be viewed in this diff.

This PR address a bug that was discovered after this PR was made to the CLI. When attempting to unshare a service instance from a space where the current user did not have access to that space, the unshare failed.

I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the master branch
I have run all the unit tests using bundle exec rake
I have run CF Acceptance Tests

Thanks,

Sapi team (alex, @deniseyu, @jenspinney)

cfdreddbot · 2017-12-05T16:28:01Z

Hey ablease!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

cf-gitbot · 2017-12-05T16:28:01Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/153410785

The labels on this github issue will be updated when the story is started.

Gerg · 2017-12-13T01:36:17Z


    private

+    class ServiceInstanceSharedToEagerLoader


This class will not be private unless you do

private_constant :ServiceInstanceSharedToEagerLoader

Thanks, we've made this change.

Changes for this included in #1033 and can be viewed here: cloudfoundry-incubator@ff8276e#diff-9b86a08059c9bdc2b6875a70abf9ee29R366

Gerg · 2017-12-13T01:42:40Z

    def create_paginated_collection_renderer(service_instance)
      VCAP::CloudController::RestController::PaginatedCollectionRenderer.new(
-        VCAP::CloudController::RestController::SecureEagerLoader.new,
+        ServiceInstanceSharedToEagerLoader.new,


This feels like a pretty fundamental change to service instances controller. Subverting the framework and how it expects to load associations could lead to confusing behavior later. I'm surprised this didn't break any existing tests.

Is there another way we could configure the framework to eager load organizations in addition to it's usual machinations?

The create_paginated_collection_renderer method, the ServiceInstanceSharedToEagerLoader and the ServiceInstanceSharedToSerializer are only used by the '/v2/service_instances/:guid/shared_to' endpoint. These are used to support paginated rendering using the custom ServiceInstanceSharedToPresenter presenter. We could not use the existing serialiser class as SharedTo does not have an associated model object. The eager loader is overridden in order to load the organisation model that is required by the presenter to display the org name.

These changes are confined to the shared_to endpoint, so it hasn't changed behaviour elsewhere in the controller.

* cloudfoundry#1006 * cloudfoundry#1007 * cloudfoundry#1024 Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>

* cloudfoundry#1006 * cloudfoundry#1007 * cloudfoundry#1024 Signed-off-by: Denise Yu <dyu@pivotal.io>

[#152922808] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

[#152922808] Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>

[#152922808] Signed-off-by: Denise Yu <dyu@pivotal.io>

* Add additional tests for user provided services and service instance sharing Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>

422 Unprocessable is a more appropriate error message as the syntax of the request is correct, however the semantics of the request are wrong. [#153176525] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

* This allows developers who does not have cross space access to discover the space_guid when using service instance sharing. [#153244755] Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>

* This could introduce a name clash in spaces receiving a shared instance [#152109574]

[#152881797]

[#153244755] Signed-off-by: Alex Blease <ablease@pivotal.io>

* cloudfoundry#1006 * cloudfoundry#1007 * cloudfoundry#1024 Signed-off-by: Denise Yu <dyu@pivotal.io>

elenasharma · 2017-12-15T20:18:41Z


      describe 'permissions' do
        let(:user) { User.make }
+        let(:other_org) { Organization.make }


Per the conversation that led to PR #1023, please prefer target_org over other_org; same for space.

Thanks @elenasharma, it will be easier for us to fix this with a new commit on top rather than rebase everything, would that be OK?

elenasharma · 2017-12-15T20:19:56Z

              it "has a #{expected_status} http status code" do
                get "/v2/service_instances/#{instance.guid}/shared_to"
-                expect(last_response.status).to eq(expected_status), "Expected #{expected_status}, got: #{last_response.status}, role: #{role}"
+                expect(last_response.status).to eq(expected_status), "Expected #{expected_status}, got: #{last_response.status}, role: #{role}, response: #{last_response.body}"


Is the last_response.body part intentional? We often use this during development for extra information, but typically don't commit uses of it.

This was probably unintentional! We will remove it.

elenasharma · 2017-12-15T20:32:14Z

          end
        end

        context 'when the user is a member of the org/space where the service instance was shared to' do


We'd expect to see a test case for the encountered bug, e.g. given two developers: a developer in space-1 and space-2 and a developer in space-1:

the first developer shares service instance SI into space-2

the second developer can unshare SI out of space-2

@elenasharma we're not clear what you mean here. This endpoint is not for unsharing, it's for a developer in a service instance's source space to view the space GUIDs where the service instance has been shared to. Developers in recipient spaces shouldn't have access to this, because then they'd learn about all the other places that the service instance has been shared to, and this is a workflow we don't want to currently permit.

We do test the workflow where a source-space-only developer unshares a service instance in the v3/controllers unit test for unsharing: https://github.com/cloudfoundry/cloud_controller_ng/blob/master/spec/unit/controllers/v3/service_instances_controller_spec.rb#L480-L505

If we've misunderstood what you meant, please let us know!

cf-gitbot added the unscheduled label Dec 5, 2017

deniseyu mentioned this pull request Dec 7, 2017

Allow SpaceDevelopers to unshare service instances when feature flag is disabled #1027

Merged

4 tasks

Gerg reviewed Dec 13, 2017

View reviewed changes

Samze pushed a commit to cloudfoundry-incubator/cloud_controller_ng_sapi that referenced this pull request Dec 13, 2017

Incorporate PR feedback for service instance sharing

797f6a2

* cloudfoundry#1006 * cloudfoundry#1007 * cloudfoundry#1024 Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>

Samze added a commit to cloudfoundry-incubator/cloud_controller_ng_sapi that referenced this pull request Dec 13, 2017

Incorporate PR feedback for service instance sharing

ff8276e

* cloudfoundry#1006 * cloudfoundry#1007 * cloudfoundry#1024 Signed-off-by: Denise Yu <dyu@pivotal.io>

Samze mentioned this pull request Dec 13, 2017

Incorporate Service Instance Sharing Feedback #1033

Merged

4 tasks

Denise Yu and others added 10 commits December 15, 2017 13:49

Add space_guids query parameter to GET /v3/service_instances

34362de

[#152922808] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

Add space relationship to service instance presentation

28d141f

[#152922808] Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>

Add docs for service instance space relationship

afa2108

[#152922808] Signed-off-by: Denise Yu <dyu@pivotal.io>

Additional test for combining query params on v3 service instances

87dbe9f

[#152922808] Signed-off-by: Denise Yu <dyu@pivotal.io>

/v3/service_instances does not return user provided services

3eb6414

* Add additional tests for user provided services and service instance sharing Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>

Well-formed but invalid share requests should return 422

f219ad5

422 Unprocessable is a more appropriate error message as the syntax of the request is correct, however the semantics of the request are wrong. [#153176525] Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>

Shared service instance information includes space_guid

b4010d6

* This allows developers who does not have cross space access to discover the space_guid when using service instance sharing. [#153244755] Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>

Disable renaming a shared service instance

dd18177

* This could introduce a name clash in spaces receiving a shared instance [#152109574]

Reword some test contexts and descriptions for clarity

3d447ba

[#152881797]

Allow unsharing from a space without org visibility

e98c34c

[#153244755] Signed-off-by: Alex Blease <ablease@pivotal.io>

deniseyu pushed a commit to cloudfoundry-incubator/cloud_controller_ng_sapi that referenced this pull request Dec 15, 2017

Incorporate PR feedback for service instance sharing

652f5e8

* cloudfoundry#1006 * cloudfoundry#1007 * cloudfoundry#1024 Signed-off-by: Denise Yu <dyu@pivotal.io>

deniseyu force-pushed the pr-service-instance-sharing-shared-to-without-org-visibilty branch from b92253d to e98c34c Compare December 15, 2017 14:48

elenasharma reviewed Dec 15, 2017

View reviewed changes

maryamklabib merged commit e98c34c into cloudfoundry:master Dec 18, 2017

cf-gitbot added delivered accepted and removed unscheduled delivered labels Dec 18, 2017

jenspinney mentioned this pull request Dec 27, 2017

Cosmetic clean up in test file #1048

Merged

3 tasks

deniseyu deleted the pr-service-instance-sharing-shared-to-without-org-visibilty branch January 23, 2018 11:43

Uh oh!

Conversation

ablease commented Dec 5, 2017

Uh oh!

cfdreddbot commented Dec 5, 2017

Uh oh!

cf-gitbot commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deniseyu Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deniseyu Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

deniseyu Dec 18, 2017 •

edited

Loading

deniseyu Dec 18, 2017 •

edited

Loading