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

Allow space application supporter to access specific app feature endpoints and app PATCH endpoints #2310

Conversation

philippthun
Copy link
Member

  • Space Application Supporter can assign current droplet

    • Addition of permission checks for changed endpoint.
      • Move droplet creation to parent context (describe) for reuse.
      • Move event emission test into own context (grouped with similar test).
    • Group sidecar tests in context.
  • Space Application Supporter can update the revisions feature

    • Ensure that space application supporter cannot update the ssh feature.
  • Space Application Supporter can access specific app feature endpoints

    • Addition of permission checks for all /apps/:guid/features endpoints.
    • Addition of tests for the revisions app feature.

Closes #2272
Partly implements #2280

  • 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

philippthun and others added 2 commits June 2, 2021 16:10
- Addition of permission checks for all /apps/:guid/features endpoints.
- Addition of tests for the revisions app feature.

Co-authored-by: Marc Misoch <marc.misoch@sap.com>
- Ensure that space application supporter cannot update the ssh feature.

Co-authored-by: Marc Misoch <marc.misoch@sap.com>
Co-authored-by: Andy Paine <andy.paine@engineerbetter.com>
@philippthun philippthun force-pushed the space-application-supporter-features-and-apps-patch branch from 9f01d89 to d6a44c4 Compare June 2, 2021 14:11
- Addition of permission checks for changed endpoint.
-- Move droplet creation to parent context (describe) for reuse.
-- Move event emission test into own context (grouped with similar
   test).
- Group sidecar tests in context.

Co-authored-by: Marc Misoch <marc.misoch@sap.com>
@philippthun philippthun force-pushed the space-application-supporter-features-and-apps-patch branch from d6a44c4 to 4ed0b74 Compare June 2, 2021 14:29
@philippthun philippthun changed the title Allow space application supporter to access specific app feature endpoints and app POST endpoints Allow space application supporter to access specific app feature endpoints and app PATCH endpoints Jun 2, 2021
@sweinstein22 sweinstein22 self-requested a review June 2, 2021 17:23
@sweinstein22 sweinstein22 linked an issue Jun 2, 2021 that may be closed by this pull request
5 tasks
@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"
          }
        },
      ...
    }
  ]
}

✅ GET requests

  • Ran cf curl against /v3/apps/:guid/features, /v3/apps/:guid/features/revisions, /v3/apps/:guid/features/ssh as user and as a space developer, piped results to files
  • Confirmed that output of calls was the same:
    $ diff space_app_supporter_get_features space_dev_get_features
    
    $ diff space_app_supporter_get_feature_revisions space_dev_get_feature_revisions
    
    $ diff space_app_supporter_get_feature_ssh space_dev_get_feature_ssh
    
    
  • Manually confirmed output looks correct

✅ PATCH requests

PATCH /v3/apps/:guid/features/revisions

  • Ran cf curl -X PATCH /v3/apps/86bb6af6-6e14-425a-97b7-d9cc4f3b4ea3/features/revisions -d '{ "enabled": false }' as user and as a space developer, piped results to files
    • Confirmed that changes actually took effect between calls:
      $ cf curl /v3/apps/86bb6af6-6e14-425a-97b7-d9cc4f3b4ea3/features/revisions
      {
        "name": "revisions",
        "description": "Enable versioning of an application",
        "enabled": false
      }
      
    • Switched state back between runs so I could use the exact same curl
  • Confirmed that output of calls was the same:
    $ diff space_app_supporter_patch_feature_revisions space_dev_patch_feature_revisions
    
    
  • Manually confirmed output looks correct

PATCH /v3/apps/:guid/relationships/current_droplet

  • Created second droplet for application by pushing the app again
  • Ran cf curl -X PATCH /v3/apps/86bb6af6-6e14-425a-97b7-d9cc4f3b4ea3/relationships/current_droplet -d '{ "data": { "guid": "c5cf083c-a70d-4c38-ad4e-44df117d4368" } }' as user and as a space developer, piped results to files
    • Switched current droplet back between runs so I could use the exact same curl
  • Confirmed that output of calls was the same:
    $ diff space_app_supporter_current_droplet space_dev_current_droplet
    
    
  • Manually confirmed output looks correct

PATCH /v3/apps/:guid/features/ssh

Confirmed that this endpoint has not been accounted for, as it is not part of the list of endpoints mentioned in either Github issue but is adjacent to the PATCH /features/revisions requests

$ cf curl -X PATCH /v3/apps/86bb6af6-6e14-425a-97b7-d9cc4f3b4ea3/features/ssh -d '{ "enabled": false }'
{
  "errors": [
    {
      "detail": "You are not authorized to perform the requested action",
      "title": "CF-NotAuthorized",
      "code": 10003
    }
  ]
}

It also does not appear to have been accounted for in the existing Github issues, so I've added it to an issue for overlooked endpoints: #2311


Code Review

Overall looks good, things noted:

  • Looked over refactors, doesn't look like we've dropped code coverage in any way
  • Added permission checks for endpoints touched in this PR
  • Once the ssh PATCH endpoint is implemented some of the logic in app_features_controller.rb should be able to be simplified, but it looks good given current constraints
  • In user_helpers there's another correction to an error made previously that improperly mocked out disallow_user_write_access

Copy link
Contributor

@sweinstein22 sweinstein22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes Requested:

It looks like there aren't any docs changes included in this PR. As PATCH /v3/apps/:guid/features/:name in a half-completed state, it makes sense to leave the new role off of the docs for now, indication should be given on the other endpoints that the space application supporter role has access to them.

@philippthun
Copy link
Member Author

Oh, we forgot the docs...

With regards to the /v3/apps/:guid/features/:name endpoint: it was our understanding that the space application supporter is not allowed to enable/disable ssh. This would mean that this endpoint is completed and should also be documented.

@sweinstein22
Copy link
Contributor

I'm just going based on the spreadsheet that I was handed to do acceptance against, but I'm going to make sure we have someone following up on that particular endpoint.

To that end, if we can include documentation that the v3/apps/:guid/features/revision endpoint is available to Space Application Supporters that would be great, thanks @philippthun

@philippthun
Copy link
Member Author

@sweinstein22 I've added this missing docs.

Copy link
Contributor

@sweinstein22 sweinstein22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @philippthun !

@sweinstein22 sweinstein22 merged commit a75bdee into cloudfoundry:main Jun 3, 2021
@sweinstein22
Copy link
Contributor

I've confirmed that PATCH /v3/apps/:guid/features/ssh should be excluded for this role, thanks for raising the question!

@mkocher mkocher added the space-application-supporter https://github.com/cloudfoundry/cfar-proposals/issues/22 label Aug 23, 2021
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
4 participants