Space application supporter can access deployments endpoints

[will-gant]

• Enable the Space Application Supporter to create, get, list, update and cancel deployment objects.
• Add unit tests to prove each of the above.

Closes #2218

• I have reviewed the contributing guide

• I have viewed, signed, and submitted the Contributor License Agreement

• I have made this pull request to the main branch

• I have run all the unit tests using bundle exec rake

• [] I have run CF Acceptance Tests

[will-gant]

I've rebased this off of main.

[monamohebbi]

Looks good!

One callout: we made a decision midway through the planning to not allow the space application supporter access to any of the PATCH endpoints that only allow access to metadata and labels, so it looks like there is a mistake in this issue. Could you remove the commit a50aef296fad0fdf91591f842a133f3f18472e37 from this PR?

[will-gant]

@monamohebbi Thanks for this. I've made that change (and also an extra commit updating the docs accordingly).

I've just asked around at the SAP Control Plane team, which is who I'm working with at the moment, and we believe nobody in the team has permissions to update the user stories in the GitHub backlog for creating this role.

Would it be possible to update these stories to remove "PATCH" from the description for endpoints that we know only let you update metadata/labels? This will help prevent colleagues from accidentally implementing this for other endpoints when we do that work. Thanks!

[sweinstein22]

Acceptance

Behavior Check

GET Requests

Ran cf curl against GET /v3/deployments/:guid, GET /v3/deployments as both a space developer and a space application supporter. Piped output into files and compared them, as well as checked the output manually:

$ diff space-app-supporter-deployment-guid space-dev-deployment-guid

$ diff space-app-supporter-deployments space-dev-deployments

POST /v3/deployments

Ran cf curl -X POST /v3/deployments -d '{"droplet":{"guid":"d68e1299-a531-44ba-b981-f7892946d814"}, "strategy":"rolling", "relationships":{"app":{"data": {"guid":"f462839b-d351-415b-b2fd-d5f4403a2a67"}}}}' as both roles, diffed output. Confirmed that diff was only related to guids and timestamps:

$ diff space-app-supporter-post-deployment space-dev-post-deployment
2,4c2,4
<   "guid": "8582c10d-94d5-4296-a22e-4a19213204f0",
<   "created_at": "2021-06-22T23:30:49Z",
<   "updated_at": "2021-06-22T23:30:49Z",
---
>   "guid": "5f8c579e-efac-4878-900e-871dc5f2b22b",
>   "created_at": "2021-06-22T23:08:45Z",
>   "updated_at": "2021-06-22T23:08:45Z",
9c9
<       "last_successful_healthcheck": "2021-06-22T23:30:49Z"
---
>       "last_successful_healthcheck": "2021-06-22T23:08:45Z"
21c21
<       "guid": "12ff2e1e-474d-4792-abc0-ce92ca7a4db3",
---
>       "guid": "de45040a-58f7-4d1e-aa9b-05bad3108e8f",
46c46
<       "href": "https://api.clover-horse.capi.land/v3/deployments/8582c10d-94d5-4296-a22e-4a19213204f0"
---
>       "href": "https://api.clover-horse.capi.land/v3/deployments/5f8c579e-efac-4878-900e-871dc5f2b22b"
52c52
<       "href": "https://api.clover-horse.capi.land/v3/deployments/8582c10d-94d5-4296-a22e-4a19213204f0/actions/cancel",
---
>       "href": "https://api.clover-horse.capi.land/v3/deployments/5f8c579e-efac-4878-900e-871dc5f2b22b/actions/cancel",

PATCH /v3/deployments/:guid

Looks like this endpoint got overlooked

$ cf curl -X PATCH /v3/deployments/5f8c579e-efac-4878-900e-871dc5f2b22b -d '{ "metadata": { "labels": { "key": "value2" }, "annotations": {"note": "less detailed information"}}}'
{
  "errors": [
    {
      "detail": "Deployment not found",
      "title": "CF-ResourceNotFound",
      "code": 10010
    }
  ]
}

Confirmed that running cf curl /v3/deployments/5f8c579e-efac-4878-900e-871dc5f2b22b returned a deployment as the space app supporter, and that running the above PATCH call returned the correct response for space developers.

POST /v3/deployments/:guid/actions/cancel

There's no output to this api request, but I confirmed that curling the associated deployment shows a cancelled state as the space app supporter:

$ cf curl -X POST /v3/deployments/969bf3b9-1f62-4098-8eb5-7779eaa217c0/actions/cancel
$ cf curl /v3/deployments/969bf3b9-1f62-4098-8eb5-7779eaa217c0
{
  "guid": "969bf3b9-1f62-4098-8eb5-7779eaa217c0",
  "status": {
    "value": "FINALIZED",
    "reason": "CANCELED",
    ...
  },
  "strategy": "rolling",
  ...
}

[sweinstein22]

Changes Requested:

PATCH /v3/deployments/:guid

Looks like this endpoint got overlooked, but it is part of the issue associated with this PR.

Rereading the comments in this PR, looks like this was intentional. I'm going to edit the original issue to remove the PATCH endpoint.

Code Check

Similar to a comment I made on a previous PR from y'all, if we could shift the request spec to utilize the shared examples that would be great! It'll help us reduce the code duplication. Let me know if you need further clarification on what that might look like.

Docs Check

Minor detail, but we've been trying to alphabetize the roles. If we could reorder the placement of the 'Space Application Supporter' in the list accordingly that would be great!

[svkrieger]

Hi @sweinstein22,

this endpoint is a bit confusing to me because of the following points:

• The documentation here says that the Org Auditor can access GET /v3/deployments but cannot access GET /v3/deployments/:guid, which does not make sense to me as the list endpoint shows more info than the get endpoint.
• Furthermore here is tested that all roles (including no role) can access GET /v3/deployments. So the documentation should state that all roles can access it.
• I don't quite understand why the roles Org Billing Manager, Org Auditor and No Role will get a 200 there with an empty resources list. Other endpoints would just return a 404 then. Being permitted to access an endpoint, but not being permitted to see any resources does also make not much sense to me. In this case it would be better to return a 404 or 403 and remove them from the list of permitted roles in the documentation and adjust the endpoint accordingly. Maybe you know what the comment here means and then all makes sense. I can't relate this comment to what is being tested there. As the same comment is present in the domains_spec.rb file (where it seems to belong to) I guess it's just a copy paste mistake.
• In general the structure of the tests in the spec file seem wrong. The test I linked you above, which test the filtering for all users is inside context 'with an admin who can see all deployments' do, which again does not make sense.

Maybe I'm missing something to get this picture right. Anyways I'd be glad if you could shed some light on this. I think it makes sense to clarify how this endpoint is supposed to work from a permissions point of view, before we continue adding another role in.

[galenhammond]

Hi @svkrieger,

Returning an empty list for non-permitted users is in accordance with out style guide. We believe the reasoning for this is, from an implementation standpoint, we filter on user provided queries and the user's given permissions together in the fetcher classes and return the results to the controller. In the controller, where we would throw a 403 or 404, we can't be sure if the list is empty be cause of permissions restrictions or the filtering/an empty database.

Given this, if the org auditor is technically permitted to use the list endpoint but not the view endpoint, this would mean the org auditor would receive an empty set when using the list endpoint.

No roles not being included in the link you provided is a good thing to call out. I think this is an error that shows up in many places in our documentation. I think updating that documentation would ideally be a part of this PR.

The shared org comment you linked is probably a copy paste error as you mentioned. Shared orgs are a concept linked to shared domains, and given this exact comment can be found in the domains_spec.rb, I think your assumption is correct. Again, it would be a big help if removing that comment could also be a part of this PR.

We haven't had the chance to look too much at the format of this spec. Reformatting our specs has been an ongoing chore that we've been working on along with these space supporter issues. We hope to make sure that the specs are formatted in a reasonable way before merging in any new code.

Hope this answers your questions!

@monamohebbi & Galen

[sweinstein22]

Hi @will-gant , just wanted to check in on this as it's been a bit. Let us know if you have further questions, thanks!

[will-gant]

@sweinstein22 Apologies for the delay - I've been on leave for the past week. My colleague @svkrieger, who has also been working on this, has also has several days off over the same period. I'm looking at this today, and will hopefully get it sorted!

[sweinstein22]

No worries @will-gant ! Just wanted to make sure you weren't blocked on something on our end. Hope you enjoyed your time off!

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Will Gant (3bf5b24, 2519eb2, a50aef2, 60bf32a, 978e404, a92c2a4, 9b87911, 20f13b2, 46925a5, 1c2cc28, 110d3ed, 348743f, adbd783, 95582b7, ae2e861, 6298b3a, 600e9e5, cb06089)
• ✅ Sven Krieger (4d499cd, 22aeda3, 0a9bcc2, 4c0174a)

[will-gant]

@sweinstein22 Aw, thanks! I've just pushed my best attempt at doing this with the shared examples. Let me know what you think :)

[sweinstein22]

Looks great! Thanks so much for making the adjustments, and thanks for the PR!