search

cloudfoundry / cloud_controller_ng

Cloud Foundry Cloud Controller
Apache License 2.0
194 stars 360 forks source link

Allow space application supporter to access specific service broker endpoints. #2239

Closed monamohebbi closed 3 years ago

monamohebbi commented 3 years ago

Issue

Allow space application supporter to access specific service broker endpoints.

Context

We are introducing a new role and we want to make sure it has the right access.

Expected result

A space application support should be able access the following endpoints:

GET /v3/service_brokers GET /v3/service_brokers/:guid

A space application should get a 403 unauthorized error when trying to curl the following endpoints:

DELETE /v3/service_brokers/:guid PATCH /v3/service_brokers/:guid POST /v3/service_brokers

Acceptance

A space application supporter would see the same info as a space developer assigned to the same space for these and only these service broker endpoints.

Documentation

When I browse to any of these endpoints on v3 docs I can see the Space Application Supporter role in the list of permitted roles with an indication that this role is not fully implemented and the permissions will be changing.

will-gant commented 3 years ago

Starting this now.

will-gant commented 3 years ago

@monamohebbi Before I continue, could I please just confirm that it's intended that space auditors, space managers, and org managers should not be able to access the /v3/service_brokers endpoint? This is what the docs show, and this is indeed how the endpoint currently works. It's a little strange to me, and out of step with the way we're making a number of other endpoints work (most of which we're switching to use the untrusted_can_read_from_space? method in /lib/cloud_controller/permissions.rb, which grants the three aforementioned roles access in addition to space developers and space application supporters). Why shouldn't space auditors, etc be allowed to see service brokers?

It's also a little strange to me - I've noticed the same thing in user stories for other endpoints, but haven't queried it until now - that the space application supporter will not be allowed to get individual resources, but is allowed to list all of them (which includes all the information they could have obtained by getting one individually). Is this really desired, or should I take it as implied that any user story asking for the list endpoint to be implemented also expects the get one to be done as well?

will-gant commented 3 years ago

Just chasing up on this, as I've left it on ice pending a response.

monamohebbi commented 3 years ago

Hi @will-gant,

Sorry for the delayed response.

Cloud controller permissions can be a little strange and opinionated. I can try and do a little git archaeology to find out why space auditors would not be able to see this endpoint. As for space managers, in many cases their view/act permissions are much more restricted than a space developer, even though they can assign themselves that role. However, changing the set of permissions a role has could cause some unexpected but dangerous security holes, depending on how a user might be expecting the product to act in certain edge cases. So, unless we have a compelling security reason to change the historic permission access for an endpoint I think it's best to just leave it be.

As for the list/get endpoints, I think many times this just comes down to poor or confusing documentation. This comment might provide some clarity, and let me know if it doesn't address your question fully. I strongly encourage you to PR in any changes to the documentation that you think would help clarify what results a user should expect from a given endpoint.

Best, Mona

monamohebbi commented 3 years ago

Actually after a second pass at the docs, I think you are right and we did intend to allow the space supporter access to the show endpoint as well. Updating the issue now. Additionally I made some updates on how we want the error messages to behave.

will-gant commented 3 years ago

Many thanks for the replies, which I think clear up the confusion for me.

I'm actually on vacation for about ten days as of three hours from now, so I'm setting this back to 'to do' for now so that it's possible for someone else to pick this up in the meantime in case I end up blocking things by keeping it assigned to me in the interim.