feat: Support RBAC in webhook

e2e_tests/tests/cluster/test_webhooks.py

@@ -2,10 +2,12 @@
 import random
 import time
 import uuid
+from typing import Optional
 
 import pytest
 
-from determined.common.api import bindings
+from determined.common import api
+from determined.common.api import bindings, errors
 from tests import api_utils
 from tests import config as conf
 from tests import experiment as exp
@@ -270,3 +272,135 @@ def test_specific_webhook() -> None:
     assert len(responses) == 0
     responses = server2.close_and_return_responses()
     assert len(responses) == 1
+
+
+def create_default_webhook(sess: api.Session, workspaceId: Optional[int] = None) -> int:
+    webhook_trigger = bindings.v1Trigger(
+        triggerType=bindings.v1TriggerType.EXPERIMENT_STATE_CHANGE,
+        condition={"state": "COMPLETED"},
+    )
+    webhook_url = "http://localhost"
+    res = bindings.post_PostWebhook(
+        sess,
+        body=bindings.v1Webhook(
+            url=webhook_url,
+            webhookType=bindings.v1WebhookType.DEFAULT,
+            triggers=[webhook_trigger],
+            mode=bindings.v1WebhookMode.WORKSPACE,
+            name="",
+            workspaceId=workspaceId,
+        ),
+    )
+    assert res.webhook.id is not None
+    return res.webhook.id or 0
+
+
+@pytest.mark.e2e_cpu
+def test_webhook_permission() -> None:
+    # non-admin should not be able to create global webhook.
+    user1_sess = api_utils.user_session()
+    with pytest.raises(errors.APIException):
+        create_default_webhook(user1_sess)
+
+    # admin should be able to create global webhook.
+    admin_sess = api_utils.admin_session()
+    global_webhook_id = create_default_webhook(admin_sess)
+
+    # non-admin should be able to view global webhook.
+    res = bindings.get_GetWebhooks(user1_sess)
+    assert any(w.id == global_webhook_id for w in res.webhooks)
+
+    # user should be able to add webhook to their own workspace
+    username = api_utils.get_random_string()
+    (user2_sess, _) = api_utils.create_test_user(
+        user=bindings.v1User(username=username, active=True, admin=False),
+    )
+    workspace = bindings.post_PostWorkspace(
+        user2_sess,
+        body=bindings.v1PostWorkspaceRequest(
+            name=f"workspace_aug_{uuid.uuid4().hex[:8]}",
+        ),
+    ).workspace
+
+    workspace_webhook_id = create_default_webhook(user2_sess, workspace.id)
+    # user should not add workspace to other users' workspace
+    with pytest.raises(errors.APIException):
+        create_default_webhook(user1_sess, workspace.id)
+    # user should be able to get webhook from their own workspace
+    res = bindings.get_GetWebhooks(user2_sess)
+    assert any(w.id == workspace_webhook_id for w in res.webhooks)
+    # user should not be able to get webhook from other users' workspace
+    res = bindings.get_GetWebhooks(user1_sess)
+    assert not any(w.id == workspace_webhook_id for w in res.webhooks)
+    # admin should be able to get all webhooks
+    res = bindings.get_GetWebhooks(admin_sess)
+    assert any(w.id == workspace_webhook_id for w in res.webhooks)
+    assert any(w.id == global_webhook_id for w in res.webhooks)
+    # user should not delete webhook from other users' workspace
+    with pytest.raises(errors.APIException):
+        bindings.delete_DeleteWebhook(user1_sess, id=workspace_webhook_id)
+    # non admin should not delete global webhooks
+    with pytest.raises(errors.APIException):
+        bindings.delete_DeleteWebhook(user1_sess, id=global_webhook_id)
+    with pytest.raises(errors.APIException):
+        bindings.delete_DeleteWebhook(user2_sess, id=global_webhook_id)
+    # admin should be able to delete global webhook
+    bindings.delete_DeleteWebhook(admin_sess, id=global_webhook_id)
+    # user should be able to delete webhook from their own workspace
+    bindings.delete_DeleteWebhook(user2_sess, id=workspace_webhook_id)
+
+
+@pytest.mark.e2e_cpu_rbac
+@api_utils.skipif_rbac_not_enabled()
+def test_webhook_rbac() -> None:
+    # non-admin should not be able to create global webhook.
+    user1_sess = api_utils.user_session()
+    with pytest.raises(errors.ForbiddenException):
+        create_default_webhook(user1_sess)
+
+    # admin should be able to create global webhook.
+    admin_sess = api_utils.admin_session()
+    global_webhook_id = create_default_webhook(admin_sess)
+
+    # non-admin should be able to view global webhook.
+    res = bindings.get_GetWebhooks(user1_sess)
+    assert any(w.id == global_webhook_id for w in res.webhooks)
+
+    # user should be able to add webhook to workspace they have Editor access.
+    username = api_utils.get_random_string()
+    (user2_sess, _) = api_utils.create_test_user(
+        user=bindings.v1User(username=username, active=True, admin=False),
+    )
+    workspace = bindings.post_PostWorkspace(
+        admin_sess,
+        body=bindings.v1PostWorkspaceRequest(
+            name=f"workspace_aug_{uuid.uuid4().hex[:8]}",
+        ),
+    ).workspace
+    api_utils.assign_user_role(admin_sess, username, role="Editor", workspace=workspace.name)
+    workspace_webhook_id = create_default_webhook(user2_sess, workspace.id)
+    # user without Editor access should not manage webhook
+    with pytest.raises(errors.ForbiddenException):
+        create_default_webhook(user1_sess, workspace.id)
+    # user should be able to get webhook from workspace they have access to
+    res = bindings.get_GetWebhooks(user2_sess)
+    assert any(w.id == workspace_webhook_id for w in res.webhooks)
+    # user should not be able to get webhook from other users' workspace
+    res = bindings.get_GetWebhooks(user1_sess)
+    assert not any(w.id == workspace_webhook_id for w in res.webhooks)
+    # admin should be able to get all webhooks
+    res = bindings.get_GetWebhooks(admin_sess)
+    assert any(w.id == workspace_webhook_id for w in res.webhooks)
+    assert any(w.id == global_webhook_id for w in res.webhooks)
+    # user should not delete webhook from workspace they have no access
+    with pytest.raises(errors.ForbiddenException):
+        bindings.delete_DeleteWebhook(user1_sess, id=workspace_webhook_id)
+    # user should not delete global webhooks
+    with pytest.raises(errors.ForbiddenException):
+        bindings.delete_DeleteWebhook(user1_sess, id=global_webhook_id)
+    with pytest.raises(errors.ForbiddenException):
+        bindings.delete_DeleteWebhook(user2_sess, id=global_webhook_id)
+    # admin should be able to delete global webhook
+    bindings.delete_DeleteWebhook(admin_sess, id=global_webhook_id)
+    # user with editor access to workspace should be able to delete it's webhook
+    bindings.delete_DeleteWebhook(user2_sess, id=workspace_webhook_id)

master/internal/webhooks/api_webhook.go

@@ -15,6 +15,7 @@ import (
 	"google.golang.org/grpc/status"
 
 	"github.com/determined-ai/determined/master/internal/grpcutil"
+	"github.com/determined-ai/determined/master/pkg/model"
 	"github.com/determined-ai/determined/master/pkg/ptrs"
 	"github.com/determined-ai/determined/proto/pkg/apiv1"
 	"github.com/determined-ai/determined/proto/pkg/webhookv1"
@@ -23,18 +24,25 @@ import (
 // WebhooksAPIServer is an embedded api server struct.
 type WebhooksAPIServer struct{}
 
-// AuthorizeRequest checks if the user has CanEditWebhooks permissions.
+// authorizeEditRequest checks if the user has CanEditWebhooks permissions.
 // TODO remove this eventually since authz replaces this
 // We can't yet since we use it else where.
-func AuthorizeRequest(ctx context.Context) error {
+func authorizeEditRequest(ctx context.Context, workspaceID int32) error {
 	curUser, _, err := grpcutil.GetUser(ctx)
 	if err != nil {
-		return status.Errorf(codes.Internal, "failed to get the user: %s", err)
+		return status.Errorf(codes.Internal, "failed to get the user: %v", err)
 	}
-	authErr := AuthZProvider.Get().
-		CanEditWebhooks(ctx, curUser)
-	if authErr != nil {
-		return status.Error(codes.PermissionDenied, authErr.Error())
+	var workspace *model.Workspace
+	if workspaceID > 0 {
+		workspace, err = getWorkspace(ctx, workspaceID)
+		if err != nil {
+			return err
+		}
+	}
+
+	err = AuthZProvider.Get().CanEditWebhooks(ctx, curUser, workspace)
+	if err != nil {
+		return err
 	}
 	return nil
 }
@@ -43,10 +51,15 @@ func AuthorizeRequest(ctx context.Context) error {
 func (a *WebhooksAPIServer) GetWebhooks(
 	ctx context.Context, req *apiv1.GetWebhooksRequest,
 ) (*apiv1.GetWebhooksResponse, error) {
-	if err := AuthorizeRequest(ctx); err != nil {
+	curUser, _, err := grpcutil.GetUser(ctx)
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "failed to get the user: %v", err)
+	}
+	workspaceIDs, err := AuthZProvider.Get().WebhookAvailableWorkspaces(ctx, curUser)
+	if err != nil {
 		return nil, err
 	}
-	webhooks, err := GetWebhooks(ctx)
+	webhooks, err := getWebhooks(ctx, &workspaceIDs)
 	if err != nil {
 		return nil, err
 	}
@@ -57,9 +70,10 @@ func (a *WebhooksAPIServer) GetWebhooks(
 func (a *WebhooksAPIServer) PostWebhook(
 	ctx context.Context, req *apiv1.PostWebhookRequest,
 ) (*apiv1.PostWebhookResponse, error) {
-	if err := AuthorizeRequest(ctx); err != nil {
+	if err := authorizeEditRequest(ctx, req.Webhook.WorkspaceId); err != nil {
 		return nil, err
 	}
+
 	if len(req.Webhook.Triggers) == 0 {
 		return nil, status.Errorf(
 			codes.InvalidArgument,
@@ -106,7 +120,11 @@ func (a *WebhooksAPIServer) PostWebhook(
 func (a *WebhooksAPIServer) DeleteWebhook(
 	ctx context.Context, req *apiv1.DeleteWebhookRequest,
 ) (*apiv1.DeleteWebhookResponse, error) {
-	if err := AuthorizeRequest(ctx); err != nil {
+	webhook, err := GetWebhook(ctx, int(req.Id))
+	if err != nil {
+		return nil, err
+	}
+	if err := authorizeEditRequest(ctx, webhook.Proto().WorkspaceId); err != nil {
 		return nil, err
 	}
 	if err := DeleteWebhook(ctx, WebhookID(req.Id)); err != nil {
@@ -119,14 +137,13 @@ func (a *WebhooksAPIServer) DeleteWebhook(
 func (a *WebhooksAPIServer) TestWebhook(
 	ctx context.Context, req *apiv1.TestWebhookRequest,
 ) (*apiv1.TestWebhookResponse, error) {
-	if err := AuthorizeRequest(ctx); err != nil {
-		return nil, err
-	}
-
 	webhook, err := GetWebhook(ctx, int(req.Id))
 	if err != nil {
 		return nil, err
 	}
+	if err := authorizeEditRequest(ctx, webhook.Proto().WorkspaceId); err != nil {
+		return nil, err
+	}
 
 	eventID := uuid.New()
 	log.Infof("creating webhook payload for event %v", eventID)

master/internal/webhooks/authz_basic_impl.go

@@ -4,22 +4,40 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/determined-ai/determined/master/internal/db"
 	"github.com/determined-ai/determined/master/pkg/model"
 )
 
 // WebhookAuthZBasic is basic OSS controls.
 type WebhookAuthZBasic struct{}
 
 // CanEditWebhooks always returns true and a nil error.
+// workspace being nil means the webhook is globally scoped.
 func (a *WebhookAuthZBasic) CanEditWebhooks(
-	ctx context.Context, curUser *model.User,
+	ctx context.Context, curUser *model.User, workspace *model.Workspace,
 ) (serverError error) {
+	if workspace != nil && curUser.ID == workspace.UserID {
+		return nil
+	}
 	if !curUser.Admin {
-		return fmt.Errorf("non admin users can't edit webhooks")
+		return fmt.Errorf("non admin users can't edit global webhooks")
 	}
 	return nil
 }
 
+// WebhookAvailableWorkspaces returns a list of workspaces that user can get webhooks from.
+func (a *WebhookAuthZBasic) WebhookAvailableWorkspaces(
+	ctx context.Context, curUser *model.User,
+) (workspaceIDsWithPermsFilter []int32, serverError error) {
+	var workspaceIDs []int32
+	q := db.Bun().NewSelect().Table("workspaces").Column("id")
+	if !curUser.Admin {
+		q.Where("user_id = ?", curUser.ID)
+	}
+	err := q.Scan(ctx, &workspaceIDs)
+	return workspaceIDs, err
+}
+
 func init() {
 	AuthZProvider.Register("basic", &WebhookAuthZBasic{})
 }

master/internal/webhooks/authz_iface.go

@@ -10,10 +10,12 @@ import (
 // WebhookAuthZ describes authz methods for experiments.
 type WebhookAuthZ interface {
 	// GET /api/v1/webhooks
+	WebhookAvailableWorkspaces(
+		ctx context.Context, curUser *model.User) (workspaceIDsWithPermsFilter []int32, serverError error)
 	// POST /api/v1/webhooks
 	// DELETE /api/v1/webhooks/:webhook_id
 	// POST /api/v1/webhooks/test/:webhook_id
-	CanEditWebhooks(ctx context.Context, curUser *model.User) (serverError error)
+	CanEditWebhooks(ctx context.Context, curUser *model.User, workspace *model.Workspace) (serverError error)
 }
 
 // AuthZProvider is the authz registry for experiments.

master/internal/webhooks/authz_permissive.go

@@ -11,10 +11,18 @@ type WebhookAuthZPermissive struct{}
 
 // CanEditWebhooks calls RBAC authz but enforces basic authz.
 func (p *WebhookAuthZPermissive) CanEditWebhooks(
-	ctx context.Context, curUser *model.User,
+	ctx context.Context, curUser *model.User, workspace *model.Workspace,
 ) error {
-	_ = (&WebhookAuthZRBAC{}).CanEditWebhooks(ctx, curUser)
-	return (&WebhookAuthZBasic{}).CanEditWebhooks(ctx, curUser)
+	_ = (&WebhookAuthZRBAC{}).CanEditWebhooks(ctx, curUser, workspace)
+	return (&WebhookAuthZBasic{}).CanEditWebhooks(ctx, curUser, workspace)
+}
+
+// WebhookAvailableWorkspaces calls RBAC authz but enforces basic authz.
+func (p *WebhookAuthZPermissive) WebhookAvailableWorkspaces(
+	ctx context.Context, curUser *model.User,
+) (workspaceIDsWithPermsFilter []int32, serverError error) {
+	_, _ = (&WebhookAuthZRBAC{}).WebhookAvailableWorkspaces(ctx, curUser)
+	return (&WebhookAuthZBasic{}).WebhookAvailableWorkspaces(ctx, curUser)
 }
 
 func init() {