Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Space application supporter /v3/spaces endpoint permissions #2314

Merged
merged 7 commits into from
Jun 4, 2021

Conversation

mkocher
Copy link
Member

@mkocher mkocher commented Jun 3, 2021

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change: let space application supporters access some /v3/spaces endpoints.

  • An explanation of the use cases your change solves: space application supporters can get and list spaces as well as get the isolation segment

  • 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

closes #2226

mkocher and others added 4 commits June 1, 2021 22:22
Co-authored-by: Matthew Kocher <mkocher@pivotal.io>
Co-authored-by: Merric de Launey <mdelauney@pivotal.io>
Co-authored-by: Matthew Kocher <mkocher@pivotal.io>
Co-authored-by: Merric de Launey <mdelauney@pivotal.io>
…tion_segment`

[#2226]

Co-authored-by: Carson Long <lcarson@vmware.com>
Co-authored-by: Merric de Launey <mdelauney@pivotal.io>
role.

[#2226]

Co-authored-by: Matthew Kocher <mkocher@pivotal.io>
Co-authored-by: Carson Long <lcarson@vmware.com>
@mkocher mkocher added the space-application-supporter https://github.com/cloudfoundry/cfar-proposals/issues/22 label Jun 3, 2021
@mkocher mkocher added this to the space-application-supporter milestone Jun 3, 2021
@mkocher
Copy link
Member Author

mkocher commented Jun 3, 2021

We updated package.json in the docs as it got updated when we ran npm install. Let us know if this wasn't the right thing to do.

@mkocher mkocher requested a review from sweinstein22 June 3, 2021 17:36
@sweinstein22
Copy link
Contributor

Acceptance

Confirmed user only has space_application_supporter and organization_user roles:

cf curl /v3/roles?user_guids="d8685663-a98a-4c4d-86b0-4523dd45a8b1"
{
  ...
  "resources": [
    {
      "guid": "29ab067c-7205-4fe4-be13-bd63009fcc2a",
      "type": "space_application_supporter",
      "relationships": {
        "user": {
          "data": {
            "guid": "d8685663-a98a-4c4d-86b0-4523dd45a8b1"
          }
        },
        ...
    },
    {
      "guid": "f079f79a-81d9-4ec1-8229-051ddafc3c72",
      "type": "organization_user",
      "relationships": {
        "user": {
          "data": {
            "guid": "d8685663-a98a-4c4d-86b0-4523dd45a8b1"
          }
        },
      ...
  ]
}

Setup work:

Registered and assigned an isolation segment to the space:

$ cf create-isolation-segment test-iso-segment
Creating isolation segment test-iso-segment as admin...
OK
$ cf enable-org-isolation org test-iso-segment
Enabling isolation segment test-iso-segment for org org as admin...
OK
$ cf set-space-isolation-segment space test-iso-segment
Updating isolation segment of space space in org org as admin...
OK

TIP: Restart applications in this space to relocate them to this isolation segment.
$ cf space space
Getting info for space space in org org as admin...

name:                      space
org:                       org
apps:                      test
services:
isolation segment:         test-iso-segment
...

Testing Endpoints

Ran cf curl against /v3/spaces/:guid/relationships/isolation_segment, /v3/spaces, /v3/spaces/:guid, /v3/spaces/:guid/features/ssh as both user and a space developer, piped output to files

Checked that there was no difference in output and manually confirmed that output looked correct:

$ diff space_app_supporter_space_iso_segment space_dev_space_iso_segment

$ diff space_app_supporter_spaces space_dev_spaces

$ diff space_app_supporter_space_guid space_dev_space_guid

$ diff space_dev_space_ssh space_app_supporter_space_ssh
2,4c2,8
<   "name": "ssh",
<   "description": "Enable SSHing into apps in the space.",
<   "enabled": true
---
>   "errors": [
>     {
>       "detail": "Space not found",
>       "title": "CF-ResourceNotFound",
>       "code": 10010
>     }
>   ]

I'm noticing that in the issue linked in this PR it's called out that the ssh endpoint should be handled in issue #2272, but I believe that one only addresses app level features. It also looks like GET /v3/spaces/:guid/features may not be accounted for, and is not listed on the spreadsheet.

Docs

Similar to the note from the section above, the GET /v3/spaces/:guid/features/:name endpoint documentation hasn't been touched, but the rest of the endpoints look good.

Bumping the package.json file should be fine, it's locking down versions of packages that were used when running npm install which weren't previously included in the lockfile. Since we have dependabot bumping docs dependencies as well it shouldn't prevent those packages from being upgraded as necessary, and I didn't have any issues starting up the docs locally.

One minor thing that might be notable is this PR has put the Space Application Supporter role in alphabetical order, while the other work with the new role has put it at the end of the list. I don't think it's a blocker to merging this in, and personally I like the alphabetized pattern better, just something to be aware of. Whenever we get to the point where we're pulling off the 'Experimental' labels it would be nice to make the ordering uniform as well.

@sweinstein22
Copy link
Contributor

@mkocher @ctlong Let me know whether you want to include GET /v3/spaces/:guid/features/:name in this PR or if you'd like to account for that in a separate issue. I think it also makes sense for them to have access to GET /v3/spaces/:guid/features (since they can get the individual features already) so in either case it would be great to do those two endpoints together.

Other than that these changes look good, thanks!

@ctlong
Copy link
Member

ctlong commented Jun 3, 2021

@sweinstein22 you're right, the linked issue does not cover GET /v3/spaces/:guid/features/:name. We'll go ahead and enable that endpoint for this role now (as well as add documentation).

Bumping the package.json file should be fine

Great! We'll leave it in.

One minor thing that might be notable is this PR has put the Space Application Supporter role in alphabetical order, while the other work with the new role has put it at the end of the list

We agree with you that alphabetization seems to be the way to go. We'll aim to alphabetize the existing documentation in this PR.

@ctlong disagrees with vim about sorting

Authored-by: Matthew Kocher <mkocher@pivotal.io>
@mkocher
Copy link
Member Author

mkocher commented Jun 4, 2021

@sweinstein22 Added the space features endpoints and alphabetized the roles in the docs. Please take a look.

@sweinstein22
Copy link
Contributor

✅ Second Round Acceptance

Testing Endpoints

Ran cf curl against /v3/spaces/:guid/features and /v3/spaces/:guid/features/ssh as both user and a space developer, piped output to files

Checked that there was no difference in output and manually confirmed that output looked correct:

$ diff space_dev_space_features space_app_supporter_space_features

$ diff space_dev_space_ssh space_app_supporter_space_ssh

Docs

Looks good, thanks for alphabetizing them all! It makes my hyper organized brain very happy 🎉

@sweinstein22 sweinstein22 merged commit 4b770eb into main Jun 4, 2021
@sweinstein22 sweinstein22 deleted the space_application_supporter_spaces_permissions branch June 4, 2021 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
space-application-supporter https://github.com/cloudfoundry/cfar-proposals/issues/22
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow space application supporter to access specific spaces endpoints.
4 participants