Skip to content

Prevent service instance sharers from attempting to share with themselves - #992

Merged
elenasharma merged 14 commits into
cloudfoundry:masterfrom
cloudfoundry-incubator:pr-service-instance-sharing-no-sharing-to-own-space
Nov 16, 2017
Merged

Prevent service instance sharers from attempting to share with themselves#992
elenasharma merged 14 commits into
cloudfoundry:masterfrom
cloudfoundry-incubator:pr-service-instance-sharing-no-sharing-to-own-space

Conversation

@deniseyu

Copy link
Copy Markdown
Contributor

As an app dev (sharer), I cannot share service instances with myself. #152109683

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

What

This PR closes a loophole in the validation logic when a service instance share is created. If the service instance's own space GUID is in the body of the request to the sharing service instances endpoint, a 422 will be returned and no service instance shares will be recorded (or audited). This approach is consistent with the pattern of failing a request if any GUIDs in the body of the request do not correspond to valid spaces that the user can read.

Changes:

  • Raise 422 when source space is included in list of target spaces

PR

  • I have viewed signed and have 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 on bosh lite

Thanks, sapi

@cfdreddbot

Copy link
Copy Markdown

Hey deniseyu!

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

Copy link
Copy Markdown

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

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

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

@ericpromislow

Copy link
Copy Markdown
Contributor

Maybe moving all the checks in ServiceInstanceShare#create to a helper method will remove the code complexity level. It's not a complex method per se, just long.

Derik Evangelista and others added 14 commits November 16, 2017 16:38
* The broker can return a shareable field as part of the service metadata
response to /v2/catalog.

[#152540454]

Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>
… instance

* This behaviour already existed, this commit just adds additional tests

[#150973376]

Signed-off-by: Denise Yu <dyu@pivotal.io>
* Only developers who have write access to the service instance
can perform an update.

[#150973390]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
or unshare

[#151441010]

Signed-off-by: Denise Yu <dyu@pivotal.io>
* The intended behavior is that users who have
access to a shared service instance, but not developer access to the
originating space of the instance may not create service keys from the
instance.
* This behavior was already correct. This commit simply adds tests to
lock down the behavior.

[#152592943]

Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>
* The intended behavior is that users who have access to a shared service instance, but not developer access to the
originating space of the instance may not list service keys for the instance.
* This behavior was already correct. This commit simply adds tests to lock down the behavior.

[#151950132]

Signed-off-by: Denise Yu <dyu@pivotal.io>
* The intended behavior is that users who have access to a shared service instance, but not developer access to the originating space of the instance may not delete service key associated with the instance.
* This behavior was already correct. This commit simply adds tests to lock down the behavior.

[#152721273]

Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>
* The intended behavior is that users who have access to a shared service instance, but not developer access to the originating space of the instance may not GET specific service keys for the instance.
* This behavior was already correct. This commit simply adds tests to lock down the behavior.

[#152721273]

Signed-off-by: Denise Yu <dyu@pivotal.io>
* Add explicit check in ServiceInstanceShare.create
* Add new API error type

[#151997784]

Signed-off-by: Denise Yu <dyu@pivotal.io>
* Add explicit check in ServiceInstanceShare.create
* Add new API error type

[#152036417]

Signed-off-by: Denise Yu <dyu@pivotal.io>
* Raise 422 when source space is included in list of target spaces

[#152109683]

Signed-off-by: Denise Yu <dyu@pivotal.io>
@elenasharma

Copy link
Copy Markdown
Contributor

Hi @deniseyu - we accidentally merged this PR. Could you please open a new PR with the same changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants