chore: refactor and test RBAC config policies work [CM-530] (#9943)

master/.mockery.yaml

@@ -62,6 +62,12 @@ packages:
         config:
           filename: rm_authz_iface.go
           mockname: ResourceManagerAuthZ
+  github.com/determined-ai/determined/master/internal/configpolicy:
+    interfaces:
+      ConfigPolicyAuthZ:
+        config:
+          filename: config_policy_authz_iface.go
+          mockname: ConfigPolicyAuthZ
   github.com/determined-ai/determined/master/internal/task:
     interfaces:
       AllocationService:

master/internal/api_config_policies.go

@@ -9,11 +9,9 @@ import (
 	"google.golang.org/protobuf/types/known/structpb"
 	"gopkg.in/yaml.v3"
 
-	"github.com/determined-ai/determined/master/internal/cluster"
 	"github.com/determined-ai/determined/master/internal/configpolicy"
 	"github.com/determined-ai/determined/master/internal/grpcutil"
 	"github.com/determined-ai/determined/master/internal/license"
-	"github.com/determined-ai/determined/master/internal/workspace"
 	"github.com/determined-ai/determined/master/pkg/ptrs"
 	"github.com/determined-ai/determined/proto/pkg/apiv1"
 )
@@ -78,7 +76,7 @@ func (a *apiServer) GetWorkspaceConfigPolicies(
 		return nil, err
 	}
 
-	err = workspace.AuthZProvider.Get().CanViewWorkspaceConfigPolicies(ctx, *curUser, w)
+	err = configpolicy.AuthZProvider.Get().CanViewWorkspaceConfigPolicies(ctx, *curUser, w)
 	if err != nil {
 		return nil, err
 	}
@@ -102,12 +100,11 @@ func (a *apiServer) GetGlobalConfigPolicies(
 		return nil, err
 	}
 
-	permErr, err := cluster.AuthZProvider.Get().CanViewGlobalConfigPolicies(ctx, curUser)
+	err = configpolicy.AuthZProvider.Get().CanViewGlobalConfigPolicies(ctx, curUser)
 	if err != nil {
 		return nil, err
-	} else if permErr != nil {
-		return nil, permErr
 	}
+
 	resp, err := a.getConfigPolicies(ctx, nil, req.WorkloadType)
 	if err != nil {
 		return nil, err
@@ -166,7 +163,7 @@ func (a *apiServer) DeleteWorkspaceConfigPolicies(
 		return nil, err
 	}
 
-	err = workspace.AuthZProvider.Get().CanModifyWorkspaceConfigPolicies(ctx, *curUser, w)
+	err = configpolicy.AuthZProvider.Get().CanModifyWorkspaceConfigPolicies(ctx, *curUser, w)
 	if err != nil {
 		return nil, err
 	}
@@ -198,11 +195,9 @@ func (a *apiServer) DeleteGlobalConfigPolicies(
 		return nil, err
 	}
 
-	permErr, err := cluster.AuthZProvider.Get().CanModifyGlobalConfigPolicies(ctx, curUser)
+	err = configpolicy.AuthZProvider.Get().CanModifyGlobalConfigPolicies(ctx, curUser)
 	if err != nil {
 		return nil, err
-	} else if permErr != nil {
-		return nil, permErr
 	}
 
 	if !configpolicy.ValidWorkloadType(req.WorkloadType) {

master/internal/api_config_policies_intg_test.go

@@ -6,10 +6,12 @@ import (
 	"testing"
 
 	"github.com/google/uuid"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
 	"github.com/determined-ai/determined/master/internal/configpolicy"
 	"github.com/determined-ai/determined/master/internal/db"
+	"github.com/determined-ai/determined/master/internal/mocks"
 	"github.com/determined-ai/determined/master/internal/user"
 	"github.com/determined-ai/determined/master/internal/workspace"
 	"github.com/determined-ai/determined/master/pkg/model"
@@ -350,3 +352,65 @@ func setUpTaskConfigPolicies(ctx context.Context, t *testing.T,
 	err = configpolicy.SetTaskConfigPolicies(ctx, taskConfigPolicies)
 	require.NoError(t, err)
 }
+
+func TestAuthZCanModifyConfigPolicies(t *testing.T) {
+	api, workspaceAuthZ, _, ctx := setupWorkspaceAuthZTest(t, nil)
+	testutils.MustLoadLicenseAndKeyFromFilesystem("../../")
+	configPolicyAuthZ := setupConfigPolicyAuthZ()
+
+	workspaceAuthZ.On("CanCreateWorkspace", mock.Anything, mock.Anything).Return(nil).Once()
+	workspaceAuthZ.On("CanGetWorkspace", mock.Anything, mock.Anything, mock.Anything).
+		Return(nil).Twice()
+
+	wkspResp, err := api.PostWorkspace(ctx, &apiv1.PostWorkspaceRequest{Name: uuid.New().String()})
+	require.NoError(t, err)
+	workspaceID := wkspResp.Workspace.Id
+
+	// (Workspace-level) Deny with permission access error.
+	expectedErr := fmt.Errorf("canModifyConfigPoliciesError")
+	configPolicyAuthZ.On("CanModifyWorkspaceConfigPolicies", mock.Anything, mock.Anything,
+		mock.Anything).Return(expectedErr).Once()
+
+	_, err = api.DeleteWorkspaceConfigPolicies(ctx,
+		&apiv1.DeleteWorkspaceConfigPoliciesRequest{
+			WorkspaceId:  workspaceID,
+			WorkloadType: model.NTSCType,
+		})
+	require.Equal(t, expectedErr, err)
+
+	// Nil error returns whatever the delete request returned.
+	configPolicyAuthZ.On("CanModifyWorkspaceConfigPolicies", mock.Anything, mock.Anything,
+		mock.Anything).Return(nil).Once()
+	_, err = api.DeleteWorkspaceConfigPolicies(ctx,
+		&apiv1.DeleteWorkspaceConfigPoliciesRequest{
+			WorkspaceId:  workspaceID,
+			WorkloadType: model.NTSCType,
+		})
+	require.NoError(t, err)
+
+	// (Global) Deny with permission access error.
+	expectedErr = fmt.Errorf("canModifyGlobalConfigPoliciesError")
+	configPolicyAuthZ.On("CanModifyGlobalConfigPolicies", mock.Anything, mock.Anything).
+		Return(expectedErr, nil).Once()
+
+	_, err = api.DeleteGlobalConfigPolicies(ctx,
+		&apiv1.DeleteGlobalConfigPoliciesRequest{WorkloadType: model.NTSCType})
+	require.Equal(t, expectedErr, err)
+
+	// Nil error returns whatever the delete request returned.
+	configPolicyAuthZ.On("CanModifyGlobalConfigPolicies", mock.Anything, mock.Anything).
+		Return(nil, nil).Once()
+	_, err = api.DeleteGlobalConfigPolicies(ctx,
+		&apiv1.DeleteGlobalConfigPoliciesRequest{WorkloadType: model.NTSCType})
+	require.NoError(t, err)
+}
+
+var cpAuthZ *mocks.ConfigPolicyAuthZ
+
+func setupConfigPolicyAuthZ() *mocks.ConfigPolicyAuthZ {
+	if cpAuthZ == nil {
+		cpAuthZ = &mocks.ConfigPolicyAuthZ{}
+		configpolicy.AuthZProvider.Register("mock", cpAuthZ)
+	}
+	return cpAuthZ
+}

master/internal/cluster/authz_basic_impl.go

@@ -68,21 +68,6 @@ func (a *MiscAuthZBasic) CanViewExternalJobs(
 	return nil, nil
 }
 
-// CanModifyGlobalConfigPolicies checks if user has access to modify global task config policies.
-func (a *MiscAuthZBasic) CanModifyGlobalConfigPolicies(ctx context.Context, curUser *model.User,
-) (permErr error, err error) {
-	if !curUser.Admin {
-		return grpcutil.ErrPermissionDenied, nil
-	}
-	return nil, nil
-}
-
-// CanViewGlobalConfigPolicies returns a nil error.
-func (a *MiscAuthZBasic) CanViewGlobalConfigPolicies(ctx context.Context, curUser *model.User,
-) (permErr error, err error) {
-	return nil, nil
-}
-
 func init() {
 	AuthZProvider.Register("basic", &MiscAuthZBasic{})
 }

master/internal/cluster/authz_iface.go

@@ -57,15 +57,6 @@ type MiscAuthZ interface {
 	CanViewExternalJobs(
 		ctx context.Context, curUser *model.User,
 	) (permErr error, err error)
-
-	// CanModifyGlobalConfigPolicies returns an error if the user is not authorized to
-	// modify task config policies.
-	CanModifyGlobalConfigPolicies(ctx context.Context, curUser *model.User,
-	) (permErr error, err error)
-
-	// CanViewGlobalConfigPolicies returns a nil error.
-	CanViewGlobalConfigPolicies(ctx context.Context, curUser *model.User,
-	) (permErr error, err error)
 }
 
 // AuthZProvider is the authz registry for Notebooks, Shells, and Commands.

master/internal/cluster/authz_permissive.go

@@ -65,23 +65,6 @@ func (a *MiscAuthZPermissive) CanViewExternalJobs(
 	return (&MiscAuthZBasic{}).CanViewExternalJobs(ctx, curUser)
 }
 
-// CanModifyGlobalConfigPolicies calls the RBAC implementation and returns if
-// the user has access to modfy global task config policies.
-func (a *MiscAuthZPermissive) CanModifyGlobalConfigPolicies(
-	ctx context.Context, curUser *model.User,
-) (permErr error, err error) {
-	_, _ = (&MiscAuthZRBAC{}).CanModifyGlobalConfigPolicies(ctx, curUser)
-	return (&MiscAuthZBasic{}).CanModifyGlobalConfigPolicies(ctx, curUser)
-}
-
-// CanViewGlobalConfigPolicies calls the RBAC implementation but always allows access.
-func (a *MiscAuthZPermissive) CanViewGlobalConfigPolicies(
-	ctx context.Context, curUser *model.User,
-) (permErr error, err error) {
-	_, _ = (&MiscAuthZRBAC{}).CanViewGlobalConfigPolicies(ctx, curUser)
-	return (&MiscAuthZBasic{}).CanViewGlobalConfigPolicies(ctx, curUser)
-}
-
 func init() {
 	AuthZProvider.Register("permissive", &MiscAuthZPermissive{})
 }

master/internal/configpolicy/authz_basic_impl.go

@@ -0,0 +1,49 @@
+package configpolicy
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/determined-ai/determined/master/internal/grpcutil"
+	"github.com/determined-ai/determined/master/pkg/model"
+	"github.com/determined-ai/determined/proto/pkg/workspacev1"
+)
+
+// ConfigPolicyAuthZBasic is classic OSS controls.
+type ConfigPolicyAuthZBasic struct{}
+
+// CanModifyWorkspaceConfigPolicies requires curUser to be an admin or workspace owner.
+func (a *ConfigPolicyAuthZBasic) CanModifyWorkspaceConfigPolicies(
+	ctx context.Context, curUser model.User, workspace *workspacev1.Workspace,
+) error {
+	if !curUser.Admin && curUser.ID != model.UserID(workspace.UserId) {
+		return fmt.Errorf("only admins may set config policies for workspaces")
+	}
+	return nil
+}
+
+// CanViewWorkspaceConfigPolicies returns a nil error.
+func (a *ConfigPolicyAuthZBasic) CanViewWorkspaceConfigPolicies(
+	ctx context.Context, curUser model.User, workspace *workspacev1.Workspace,
+) error {
+	return nil
+}
+
+// CanModifyGlobalConfigPolicies requires curUser to be an admin.
+func (a *ConfigPolicyAuthZBasic) CanModifyGlobalConfigPolicies(ctx context.Context, curUser *model.User,
+) error {
+	if !curUser.Admin {
+		return grpcutil.ErrPermissionDenied
+	}
+	return nil
+}
+
+// CanViewGlobalConfigPolicies returns a nil error.
+func (a *ConfigPolicyAuthZBasic) CanViewGlobalConfigPolicies(ctx context.Context, curUser *model.User,
+) error {
+	return nil
+}
+
+func init() {
+	AuthZProvider.Register("basic", &ConfigPolicyAuthZBasic{})
+}

master/internal/configpolicy/authz_iface.go

@@ -0,0 +1,33 @@
+package configpolicy
+
+import (
+	"context"
+
+	"github.com/determined-ai/determined/master/internal/authz"
+	"github.com/determined-ai/determined/master/pkg/model"
+	"github.com/determined-ai/determined/proto/pkg/workspacev1"
+)
+
+// ConfigPolicyAuthZ describes authz methods for config policies.
+type ConfigPolicyAuthZ interface {
+	// PUT /api/v1/config-policies/workspaces/:workspace-id/:type
+	CanModifyWorkspaceConfigPolicies(ctx context.Context, curUser model.User,
+		workspace *workspacev1.Workspace,
+	) error
+	// GET /api/v1/config-policies/workspaces/:workspace-id/:type
+	CanViewWorkspaceConfigPolicies(ctx context.Context, curUser model.User,
+		workspace *workspacev1.Workspace,
+	) error
+
+	// CanModifyGlobalConfigPolicies returns an error if the user is not authorized to
+	// modify task config policies.
+	CanModifyGlobalConfigPolicies(ctx context.Context, curUser *model.User,
+	) error
+
+	// CanViewGlobalConfigPolicies returns a nil error.
+	CanViewGlobalConfigPolicies(ctx context.Context, curUser *model.User,
+	) error
+}
+
+// AuthZProvider providers WorkspaceAuthZ implementations.
+var AuthZProvider authz.AuthZProviderType[ConfigPolicyAuthZ]

master/internal/configpolicy/authz_permissive.go

@@ -0,0 +1,44 @@
+package configpolicy
+
+import (
+	"context"
+
+	"github.com/determined-ai/determined/master/pkg/model"
+	"github.com/determined-ai/determined/proto/pkg/workspacev1"
+)
+
+// ConfigPolicyAuthZPermissive is the permission implementation.
+type ConfigPolicyAuthZPermissive struct{}
+
+// CanModifyWorkspaceConfigPolicies calls RBAC authz but enforces basic authz.
+func (p *ConfigPolicyAuthZPermissive) CanModifyWorkspaceConfigPolicies(
+	ctx context.Context, curUser model.User, workspace *workspacev1.Workspace,
+) error {
+	_ = (&ConfigPolicyAuthZRBAC{}).CanModifyWorkspaceConfigPolicies(ctx, curUser, workspace)
+	return (&ConfigPolicyAuthZBasic{}).CanModifyWorkspaceConfigPolicies(ctx, curUser, workspace)
+}
+
+// CanViewWorkspaceConfigPolicies calls RBAC authz but enforces basic authz.
+func (p *ConfigPolicyAuthZPermissive) CanViewWorkspaceConfigPolicies(
+	ctx context.Context, curUser model.User, workspace *workspacev1.Workspace,
+) error {
+	_ = (&ConfigPolicyAuthZRBAC{}).CanViewWorkspaceConfigPolicies(ctx, curUser, workspace)
+	return (&ConfigPolicyAuthZBasic{}).CanViewWorkspaceConfigPolicies(ctx, curUser, workspace)
+}
+
+// CanModifyGlobalConfigPolicies calls the RBAC implementation and returns if
+// the user has access to modfy global task config policies.
+func (p *ConfigPolicyAuthZPermissive) CanModifyGlobalConfigPolicies(
+	ctx context.Context, curUser *model.User,
+) error {
+	_ = (&ConfigPolicyAuthZRBAC{}).CanModifyGlobalConfigPolicies(ctx, curUser)
+	return (&ConfigPolicyAuthZBasic{}).CanModifyGlobalConfigPolicies(ctx, curUser)
+}
+
+// CanViewGlobalConfigPolicies calls the RBAC implementation but always allows access.
+func (p *ConfigPolicyAuthZPermissive) CanViewGlobalConfigPolicies(
+	ctx context.Context, curUser *model.User,
+) error {
+	_ = (&ConfigPolicyAuthZRBAC{}).CanViewGlobalConfigPolicies(ctx, curUser)
+	return (&ConfigPolicyAuthZBasic{}).CanViewGlobalConfigPolicies(ctx, curUser)
+}