Skip to content

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

Closed
deniseyu wants to merge 32 commits into
cloudfoundry:masterfrom
cloudfoundry-incubator:pr-service-instance-sharing-no-sharing-to-self
Closed

Prevent service instance sharers from attempting to share with themselves#988
deniseyu wants to merge 32 commits into
cloudfoundry:masterfrom
cloudfoundry-incubator:pr-service-instance-sharing-no-sharing-to-self

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 #987, which should be merged first. The actual changes on top of #987 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

Alex Blease and others added 30 commits November 7, 2017 13:58
[#152035378]
Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>
[#152035378]

Signed-off-by: Alex Blease <ablease@pivotal.io>
/v2/spaces/:guid/services endpoint now includes service instances that
have been shared into the given space.

[#152035378]

Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>
* The only visible change is that the global auditor role now receives
"true" for read when calling /v2/service_instances/:guid/permissions.
This tells the service broker that the user is allowed to view the
service instance dashboard. They already have read access to the service
instance, so we don't anticipate that this is a problem.

[#152035378]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
… spaces

* Also cleaned up some of the null checking in service_instance_acess
and added tests for access permissions on shared service instances.

[#152035378]

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

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

Signed-off-by: Denise Yu <dyu@pivotal.io>
* List all service instances that a user has permission to see

[#152073155]

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

Signed-off-by: Alex Blease <ablease@pivotal.io>
This change allows developers who have been granted access to a
service instance through service instance sharing, to query that service
instance via /v3/service_instances.

[#152344116]

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

Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>
* All service_instances have shared_from_url
* shared_from endpoint returns space name and org name of service
instance, if that service instance has been shared
* If not shared, this endpoint returns 204 No Content

[#150973038]

Signed-off-by: Denise Yu <dyu@pivotal.io>
* All service_instance responses now return a shared_to_url
* /v2/service_instances/:guid/shared_to endpoint returns a list of space and org name pairs indicating where the instance has been shared to

[#151432798]

Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>
* The /v2/service_instance/:guid/shared_to response body now includes
the 'bound_app_count' key, which shows the number of apps bound to the
service instance in each of the spaces that the instance has been shared
to.

[#151432730]

Signed-off-by: Jen Spinney <jennifer.spinney@suse.com>
instances

[#152314627]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
* This fixes an issue that any share between two spaces results in a
shared name pool between spaces. Name collisions should only occur if
the service instance has been shared into the space.

[#152314627]
[#152470931]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
[#151710830]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
* Change error code
* Grammar

[#151710830]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
[Finishes #151710830]

Signed-off-by: Derik Evangelista <devangelista@pivotal.io>
* 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>
jenspinney and others added 2 commits November 9, 2017 16:24
* 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>
* Raise 422 when source space is included in list of target spaces
* Minor refactor to check_spaces_are_writable

[#152109683]

Signed-off-by: Denise Yu <dyu@pivotal.io>
@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/152759320

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

@deniseyu deniseyu changed the title As an app dev (sharer), I cannot share service instances with myself Prevent service instance sharers from attempting to share with themselves Nov 10, 2017
@deniseyu deniseyu closed this Nov 14, 2017
@deniseyu
deniseyu deleted the pr-service-instance-sharing-no-sharing-to-self branch November 28, 2017 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants