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

feat: Support RBAC in webhook #9859

Merged
merged 8 commits into from
Aug 29, 2024
Merged

feat: Support RBAC in webhook #9859

merged 8 commits into from
Aug 29, 2024

Conversation

gt2345
Copy link
Contributor

@gt2345 gt2345 commented Aug 22, 2024

Ticket

MD-478

Description

Part of workload alerting project.
ERD link: https://hpe-aiatscale.atlassian.net/wiki/spaces/ENGINEERIN/pages/1666809868/Workload+Alerting+ERD

Add RBAC support to webhook.

Other improvements:

  • Add copy name action for webhook that has name
  • Add toast when creating webhook

Test Plan

Backend: e2e tests

WebUI:

With rbac disabled, user can only view/edit webhooks from their own workspace.

Create a non-admin user with no workspace,
with extend webhook flag on, navigate to the webhook page, the user should only be able to view webhooks not associated with any workspace.
The user should not see the “Create webhook” button, since they should not be able to create global level nor workspace level webhook.
Create a workspace.
At the webhook page, user should see the “Create webhook” button, and at the create webhook modal, user can only select the workspace they own.

With rbac enabled, non-admin user can view/edit webhooks from workspace they have Editor/EditorRestricted/EditorProjectRestricted/WorkspaceAdmin role.

Create a non-admin user with no workspace,
with extend webhook flag on, navigate to the webhook page, the user should only be able to view webhooks not associated with any workspace.
Grant the user view access for a workspace, the user should be able to view webhooks under that workspace, but should not delete/test those webhooks. The user also should not be able to create webhook for that workspace.
Grant the user editor/admin access for that workspace, the user now can click on the action button and test/delete webhooks under that workspace. The user should be able to create webhook for that workspace.
Grant the user editor/workspace admin/cluster admin role at settings, the user should be able to create webhooks under any workspace, and should be able to test/delete any webhook.

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

Copy link

netlify bot commented Aug 22, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 06daac3
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66d0899b38e3e00008ebf7cd

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 7.93651% with 174 lines in your changes missing coverage. Please review.

Project coverage is 54.71%. Comparing base (54b6165) to head (06daac3).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
webui/react/src/pages/WebhookList.tsx 0.00% 53 Missing ⚠️
webui/react/src/hooks/usePermissions.ts 22.72% 34 Missing ⚠️
master/internal/webhooks/api_webhook.go 0.00% 22 Missing ⚠️
master/internal/webhooks/authz_rbac.go 0.00% 20 Missing ⚠️
webui/react/src/components/WebhookCreateModal.tsx 0.00% 16 Missing ⚠️
webui/react/src/components/NavigationSideBar.tsx 0.00% 11 Missing ⚠️
master/internal/webhooks/authz_basic_impl.go 0.00% 10 Missing ⚠️
master/internal/webhooks/authz_permissive.go 0.00% 5 Missing ⚠️
master/internal/webhooks/postgres_webhook.go 62.50% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9859      +/-   ##
==========================================
- Coverage   54.75%   54.71%   -0.05%     
==========================================
  Files        1261     1261              
  Lines      156425   156566     +141     
  Branches     3598     3594       -4     
==========================================
+ Hits        85655    85660       +5     
- Misses      70639    70775     +136     
  Partials      131      131              
Flag Coverage Δ
backend 45.21% <7.69%> (-0.03%) ⬇️
harness 72.62% <ø> (ø)
web 54.46% <8.06%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
master/internal/webhooks/postgres_webhook.go 60.41% <62.50%> (-0.15%) ⬇️
master/internal/webhooks/authz_permissive.go 14.28% <0.00%> (-10.72%) ⬇️
master/internal/webhooks/authz_basic_impl.go 7.14% <0.00%> (-12.86%) ⬇️
webui/react/src/components/NavigationSideBar.tsx 0.00% <0.00%> (ø)
webui/react/src/components/WebhookCreateModal.tsx 0.00% <0.00%> (ø)
master/internal/webhooks/authz_rbac.go 4.54% <0.00%> (-28.79%) ⬇️
master/internal/webhooks/api_webhook.go 0.00% <0.00%> (ø)
webui/react/src/hooks/usePermissions.ts 73.58% <22.72%> (-3.60%) ⬇️
webui/react/src/pages/WebhookList.tsx 0.00% <0.00%> (ø)

... and 3 files with indirect coverage changes

Copy link
Contributor

@amandavialva01 amandavialva01 left a comment

Choose a reason for hiding this comment

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

This LGTM modulo a few comments, awesome job with this!

Copy link
Contributor

@ashtonG ashtonG left a comment

Choose a reason for hiding this comment

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

web changes ok

@@ -286,19 +286,30 @@ func GetWebhook(ctx context.Context, webhookID int) (*Webhook, error) {
return &webhook, nil
}

// GetWebhooks returns all Webhooks from the DB.
func GetWebhooks(ctx context.Context) (Webhooks, error) {
// getWebhooks returns Webhooks from the DB whose scopes are in workspaceIDs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I know I commented on this comment yesterday, but I just wanted to add a little bit more to it for clarity!
Can we maybe say
// getWebhooks returns all global webhooks from the DB and all webhooks whose scopes are in workspaceIDs.
I was confused before as to why users with no Viewer (or any) permissions were able to see global webhooks in the e2e tests, and this I think helps to clarify that!

Copy link
Contributor

@amandavialva01 amandavialva01 left a comment

Choose a reason for hiding this comment

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

Looks amazing! Awesome work

@gt2345 gt2345 merged commit db92bad into main Aug 29, 2024
82 of 96 checks passed
@gt2345 gt2345 deleted the gt/478-webhook-rbac-1 branch August 29, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants