[WIP] permissions API #2246
[WIP] permissions API #2246
Conversation
This patch renames some more types that were missed in PR fabric8-services#1868
The /apps API has been replaced by the JSON-API conformant version /deployments. Fixes fabric8-services#1889
Codecov Report
@@ Coverage Diff @@
## master #2246 +/- ##
==========================================
- Coverage 70.17% 69.53% -0.64%
==========================================
Files 171 172 +1
Lines 16625 16676 +51
==========================================
- Hits 11666 11596 -70
- Misses 3829 3946 +117
- Partials 1130 1134 +4
Continue to review full report at Codecov.
|
| @@ -64,6 +65,9 @@ func NewSpaceController( | |||
| resourceManager: resourceManager, | |||
| DeploymentsClient: http.DefaultClient, | |||
| CodebaseClient: http.DefaultClient, | |||
| ClientGetter: &defaultClientGetter{ | |||
There was a problem hiding this comment.
The DeleteSpace method uses the above DeploymentsClient to call the deployments API via the Goa-generated client. Should we do the same here instead of using the ClientGetter?
There was a problem hiding this comment.
I did it this way because the new CanGet*() API's are internal, and it seemed more efficient to call directly. However, it's messy in several ways: I had to pass in config.Registry instead of SpaceConfiguration, and it uses the internal CanGetSpace() call, which is ... internal. So I felt slightly dirty coding it.
If I understand it, your proposal is to call //deployments/Space/{spaceID}?qp=true under the hood and use the output of that?
There was a problem hiding this comment.
Oh right, I forgot that in order to get the permissions data we have to get all the space data as well. I don't think this would be a good idea, because of the substantial overhead for data we won't use. Perhaps we should stick with your current approach.
There was a problem hiding this comment.
We could add a parameter to only get the root space object, but maybe I should just leave that in a comment?
There was a problem hiding this comment.
Or even just skip getting any actual data for the space and only return the permissions. This may be over-complicating one API endpoint though. I'm fine with leaving as a comment for now. I imagine this would need to be addressed when moving deployments out of WIT though.
controller/deployments.go
Outdated
| } | ||
| if canGetSpace { | ||
| methods = append(methods, "GET") | ||
| } |
There was a problem hiding this comment.
CanGetSpace returns whether we can GET /api/deployments/spaces/:spaceID, but this link is for GET /api/spaces/:spaceID.
There was a problem hiding this comment.
I figured it was moot since they have to to the GET to get there in the first place, and GET is all we allow. But I didn't realize at the time the permissions are different. I have removed this link.
| Meta: &app.EndpointAccess{ | ||
| Methods: methods, | ||
| }, | ||
| } |
There was a problem hiding this comment.
We could also include links to the stats and statseries endpoints here.
| a.Required("name", "permissions") | ||
| }) | ||
|
|
||
| var internalAccess = a.Type("InternalAccess", func() { |
|
@stooke are you still working on this? |
This PR is put out for discussion on what the permissions API should be.
Currently permissions checking is only done when a new query parameter "qp=true" is added to the queries below.
This PR implements a new optional section in the JSON returned from /api/deployments/spaces/{spaceId}; a "links" section under the "data" object (this list of permissions is not inclusive as our k8s API doesn't check all of them).
Also, under each deployment object returned by the same query, in the Links object, added:
And lastly, from /api/deployments/spaces/{spaceId}, the link to the deployments API is added into the links section returned:
The intent is that the list of available HTTP methods would change based on accessibility of the given spaceId by the current user.
At this time, new tests have not been added to check this behaviour.