Allow space application supporter to access specific service plan endpoints #2349

svkrieger · 2021-06-23T08:44:31Z

A space application support can access the following endpoints:

GET /v3/service_plans
GET /v3/service_plans/:guid/visibility
GET /v3/service_plans/:guid

Implements #2237

Remark: I'd be glad for feedback on whether I put the role into the right places and whether I added appropriate methods in the correct places. The changes should grant the new role the right permissions and the changes do not interfere with other endpoints which might use methods which were changed, because all unit tests are green. But I don't know whether the coding I added or adjusted fits into the overall picture.

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

mkocher · 2021-06-24T23:22:54Z

chatted with @sweinstein22. Let's 1) add a guid check to the shared behavior and 2) rename the "as a space developer" context to something like "as a logged in user" or "as a permitted user".

monamohebbi

Code review looks good to me except for one thing to call out - I noticed you have created a new COMPLETE_PERMISSIONS in the shared_example spec instead of following the + space_application_supporter. Our plan is to do a larger refactor at the completion of the entire track where we remove all the + and add space_application_supporter to the offical ALL_PERMISSIONS so that when run the entire test suite we can be sure all examples work with the set of all permissions. I think changing the pattern halfway through might make us a little error prone later on, and I wouldn't want to end up accidentally keeping ALL_PERMISSIONS and COMPLETE_PERMISSIONS since that could be a confusing pattern .Could you refactor it to match the existing pattern?

svkrieger · 2021-06-29T12:29:03Z

Code review looks good to me except for one thing to call out - I noticed you have created a new COMPLETE_PERMISSIONS in the shared_example spec instead of following the + space_application_supporter. Our plan is to do a larger refactor at the completion of the entire track where we remove all the + and add space_application_supporter to the offical ALL_PERMISSIONS so that when run the entire test suite we can be sure all examples work with the set of all permissions. I think changing the pattern halfway through might make us a little error prone later on, and I wouldn't want to end up accidentally keeping ALL_PERMISSIONS and COMPLETE_PERMISSIONS since that could be a confusing pattern .Could you refactor it to match the existing pattern?

Oh I see. Yeah that makes sense. Just thought I reduce code duplication a bit. I refactored that to match the other PRs.

svkrieger · 2021-06-29T13:14:13Z

chatted with @sweinstein22. Let's 1) add a guid check to the shared behavior and 2) rename the "as a space developer" context to something like "as a logged in user" or "as a permitted user".

I don't really know which code pieces you are referring to. Could it be that you are talking about another PR? I found the context "as a space developer" in this PRs changed files where I also commented on: #2323
If I'm wrong and you are talking about this one here, could you point me to the code which should be changed? Thanks!

sweinstein22 · 2021-06-30T17:05:00Z

Hi @svkrieger , you're right that comment was in relation to a different PR, hopefully it didn't cause you too much confusion. Thanks for checking in!

sweinstein22

Acceptance

Setup

Pushed overview-broker and enabled plans globally

Behavior Check

Mostly leaving this to Mona, just ran through a quick curl against the endpoints, looks reasonable to me.

Docs Check

All three endpoints already indicate that they can be accessed by All Roles, no changes required

sweinstein22

Code Check

Implementation looks good to me , in request spec confirmed that shared examples are being used, that response codes and bodies are being checked.

Looks like we're still using COMPLETE_PERMISSIONS in spec/request/service_plans_spec.rb, if we could swap that one back to ALL_PERMISSIONS just to make it easier for us to refactor everything once the role has been completely implemented that would be great. Other than that, this looks good to go, thanks for the PR!

linux-foundation-easycla · 2021-07-01T15:09:38Z

The committers are authorized under a signed CLA.

✅ Sven Krieger (99a90cc, a393ab4)
✅ Aftab Alam (9dcceec)

sweinstein22

Looks great, thanks for the PR!

One minor detail, @iaftab-alam could you sign the EasyCLA cert? Thanks!

iaftab-alam · 2021-07-02T08:01:59Z

@sweinstein22 already requested for the EasyCLA cert.

iaftab-alam · 2021-07-05T09:18:33Z

@sweinstein22 got approval for EasyCLA cert. Can we re-run the failing validation job again?

iaftab-alam · 2021-07-08T11:07:43Z

Hey @monamohebbi, can you please approve the changes.

monamohebbi · 2021-07-14T22:42:31Z

Acceptance

Behavior Check

Setup: created 4 service brokers each assigned a plan with a different visibility type from set admin, org, space, and public.

`GET` Endpoints

Ran cf curl against /v3/service_plans/:guid, /v3/service_plans as a space supporter in the targeted org and space. Manually confirmed output looked correct, and checked for diffs:

$ diff dev supporter

$ diff show_dev show_supporter

`GET` Endpoints when assigned a role in a separate space in the same org

Confirmed that output looked correct manually. Checked that diff only included timestamps, guids, and user details under created_by section:

$ diff otherspace_dev otherspace_supporter

$ diff otherspace_show_dev otherspace_show_supporter

`GET` Endpoints when assigned a role in a separate org

Confirmed that output looked correct manually. Checked that diff only included timestamps, guids, and user details under created_by section:

$ diff otherorg_dev otherorg_supporter

$ diff otherorg_show_dev otherorg_show_supporter

Code Check

Unfortunately, due to a series of vacations and other requests I've had to deprioritize acceptance for a while. Since I'm just getting around to this now, the rebase didn't fix the naming conflicts in the newly added functions in the queryer/permissions/controller. If you could just take the word application out of the new functions and the respective calls I think you should be good to go; that is how I tested things off this branch locally.

…points

Signed-off-by: Aftab Alam <[email protected]>

svkrieger · 2021-07-15T14:12:52Z

Hi @monamohebbi,

I did the changes. It was kinda complex though, because of the perm deletion. I'd suggest repeating the acceptance tests with the current state.

monamohebbi · 2021-07-15T19:31:00Z

Sorry about that! Thanks for the quick updates. I'll run through them and do unit testing again.

monamohebbi

Double checked the endpoints using the same work flow and got the same results. This looks ready to be merged. Thanks!

…points (#2349) * Allow space application supporter to access specific service plan endpoints Signed-off-by: Aftab Alam <[email protected]>

cf-gitbot added the unscheduled label Jun 23, 2021

sweinstein22 added the space-application-supporter https://github.com/cloudfoundry/cfar-proposals/issues/22 label Jun 23, 2021

sweinstein22 requested a review from monamohebbi June 23, 2021 16:54

sweinstein22 linked an issue Jun 23, 2021 that may be closed by this pull request

Allow space application supporter to access specific service plan endpoints. #2237

Closed

monamohebbi suggested changes Jun 25, 2021

View reviewed changes

sweinstein22 reviewed Jun 30, 2021

View reviewed changes

sweinstein22 suggested changes Jun 30, 2021

View reviewed changes

sweinstein22 approved these changes Jul 1, 2021

View reviewed changes

will-gant mentioned this pull request Jul 5, 2021

Space application supporter can list service offerings #2364

Merged

5 tasks

svkrieger closed this Jul 6, 2021

cf-gitbot removed the unscheduled label Jul 6, 2021

svkrieger reopened this Jul 6, 2021

cf-gitbot added the unscheduled label Jul 6, 2021

svkrieger closed this Jul 7, 2021

cf-gitbot removed the unscheduled label Jul 7, 2021

svkrieger reopened this Jul 7, 2021

cf-gitbot added the unscheduled label Jul 7, 2021

svkrieger force-pushed the space-application-supporter-service-plans branch from bf878fe to 787cb02 Compare July 15, 2021 09:25

svkrieger and others added 3 commits July 15, 2021 12:49

Allow space application supporter to access specific service plan end…

e8e420f

…points

Refactor permissions

fc57d06

Signed-off-by: Aftab Alam <[email protected]>

swap COMPLETE_PERMISSIONS with ALL_PERMISSIONS

4d2cccc

svkrieger added 3 commits July 15, 2021 12:49

Rename space application supporter to space supporter

b44f894

Fix method name

cf23456

Rename leftovers after rebase to main

21aecc9

svkrieger force-pushed the space-application-supporter-service-plans branch from f5d57cb to 21aecc9 Compare July 15, 2021 11:07

monamohebbi self-requested a review July 15, 2021 21:25

monamohebbi approved these changes Jul 15, 2021

View reviewed changes

sweinstein22 merged commit fddf001 into cloudfoundry:main Jul 15, 2021

cf-gitbot removed the unscheduled label Jul 15, 2021

bepotts pushed a commit that referenced this pull request Jul 19, 2021

Allow space application supporter to access specific service plan end…

4032d41

…points (#2349) * Allow space application supporter to access specific service plan endpoints Signed-off-by: Aftab Alam <[email protected]>

svkrieger mentioned this pull request Sep 6, 2022

[ARI] Add svkrieger as capi approver cloudfoundry/community#401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow space application supporter to access specific service plan endpoints #2349

Allow space application supporter to access specific service plan endpoints #2349

svkrieger commented Jun 23, 2021

mkocher commented Jun 24, 2021

monamohebbi left a comment

svkrieger commented Jun 29, 2021

svkrieger commented Jun 29, 2021

sweinstein22 commented Jun 30, 2021

sweinstein22 left a comment •

edited

Loading

sweinstein22 left a comment

linux-foundation-easycla bot commented Jul 1, 2021 •

edited

Loading

sweinstein22 left a comment

iaftab-alam commented Jul 2, 2021

iaftab-alam commented Jul 5, 2021

iaftab-alam commented Jul 8, 2021

monamohebbi commented Jul 14, 2021

svkrieger commented Jul 15, 2021

monamohebbi commented Jul 15, 2021

monamohebbi left a comment

Allow space application supporter to access specific service plan endpoints #2349

Allow space application supporter to access specific service plan endpoints #2349

Conversation

svkrieger commented Jun 23, 2021

mkocher commented Jun 24, 2021

monamohebbi left a comment

Choose a reason for hiding this comment

svkrieger commented Jun 29, 2021

svkrieger commented Jun 29, 2021

sweinstein22 commented Jun 30, 2021

sweinstein22 left a comment • edited Loading

Choose a reason for hiding this comment

Acceptance

Setup

Behavior Check

Docs Check

sweinstein22 left a comment

Choose a reason for hiding this comment

Code Check

linux-foundation-easycla bot commented Jul 1, 2021 • edited Loading

sweinstein22 left a comment

Choose a reason for hiding this comment

iaftab-alam commented Jul 2, 2021

iaftab-alam commented Jul 5, 2021

iaftab-alam commented Jul 8, 2021

monamohebbi commented Jul 14, 2021

Acceptance

Behavior Check

GET Endpoints

GET Endpoints when assigned a role in a separate space in the same org

GET Endpoints when assigned a role in a separate org

Code Check

svkrieger commented Jul 15, 2021

monamohebbi commented Jul 15, 2021

monamohebbi left a comment

Choose a reason for hiding this comment

sweinstein22 left a comment •

edited

Loading

linux-foundation-easycla bot commented Jul 1, 2021 •

edited

Loading

`GET` Endpoints

`GET` Endpoints when assigned a role in a separate space in the same org

`GET` Endpoints when assigned a role in a separate org