-
Notifications
You must be signed in to change notification settings - Fork 705
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
Package repository permissions endpoint #5604
Conversation
Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
✅ Deploy Preview for kubeapps-dev ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
// Each access review is requested in a go routine | ||
for _, v := range AccessVerbs { | ||
wg.Add(1) | ||
go func(verb string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each SelfSubjectAccessReview
is created on a go routine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat! It's great we can finally free the UI up from performing the low-level canI
for the repo auth.
@@ -720,6 +720,48 @@ | |||
] | |||
} | |||
}, | |||
"/core/packages/v1alpha1/repositories/c/{context.cluster}/permissions": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FTR, we should also update the openapi.yaml
file when upgrading the API.
See #5610
kubeappsapis.core.plugins.v1alpha1.Plugin plugin = 1; | ||
|
||
map<string, bool> global = 2; | ||
|
||
map<string, bool> namespace = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add a short sentence above each param for the autogenerated docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great job @castelblanque! 👏🏻
From the code, it seems that "global" is considered equivalent to not providing a namespace when doing the checks, which i assume is doing a check cluster wide (similar to the --all-namespace option?) This is technically not accurate, and aside from cluster admins, this may not always return the correct results. In the case of Carvel and Helm plugins, being able to create/update/delete global repositories is equivalent to having the permission on the corresponding global namespace - which is different than having permissions at the cluster level. For a user scoped to a namespace, "list" and "get" will return false for the global checks, even though the user can list and view global repositories. Flux does not really have a notion of global repositories, I think the checks should always be false for the global map (i.e. we can skip the checks). |
Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
Merge Signed-off-by: Rafa Castelblanque <[email protected]>
Signed-off-by: Rafa Castelblanque <[email protected]>
Thanks all for the review! @dlaloue-vmware I agree, switched now to the global namespace (in Helm and Kapp). For Flux, no global info is returned. |
) ### Description of the change This PR sits on top of PR #5604 and adapts UI to use the API endpoint introduced to return allowed permissions for package repositories. It also fixes #5542, opened for Flux. Areas affected are: - `Add package repository` button is enabled only if the user has `create` permission at current namespace or cluster level. - `Edit/Delete` buttons on each repo are enabled if user has `update` permission in repository type. - `Show repositories in all namespaces` switch is enabled if user has `list` permissions cluster-wide. ### Benefits Real permissions granted to the current user are checked to adapt the UI when managing package repositories. ### Possible drawbacks Some more fine-tuning of the UI is needed to take advantage of the new permissions API endpoint. ### Applicable issues - fixes #5542 Signed-off-by: Rafa Castelblanque <[email protected]> Co-authored-by: Dimitri Laloue <[email protected]>
Description of the change
Adds a new procedure
GetPackageRepositoryPermissions
in the repositories API that allows to get the permissions for the current user with regards to package repositories.Returned structure of data is (per plugin):
get
,list
, etc.)Benefits
This endpoint will allow to:
canI
requests from frontend.Possible drawbacks
N/A
Applicable issues