From cedfcfe01f14c6ea029eb3d34e63e228b1683b4a Mon Sep 17 00:00:00 2001 From: Amanda Vialva <144278621+amandavialva01@users.noreply.github.com> Date: Thu, 19 Sep 2024 13:50:29 -0400 Subject: [PATCH] chore: refactor and test RBAC config policies work [CM-530] (#9943) --- master/.mockery.yaml | 6 ++ master/internal/api_config_policies.go | 15 +-- .../internal/api_config_policies_intg_test.go | 64 +++++++++++++ master/internal/cluster/authz_basic_impl.go | 15 --- master/internal/cluster/authz_iface.go | 9 -- master/internal/cluster/authz_permissive.go | 17 ---- .../internal/configpolicy/authz_basic_impl.go | 49 ++++++++++ master/internal/configpolicy/authz_iface.go | 33 +++++++ .../internal/configpolicy/authz_permissive.go | 44 +++++++++ master/internal/configpolicy/authz_rbac.go | 96 +++++++++++++++++++ master/internal/db/postgres_rbac_intg_test.go | 12 ++- master/internal/workspace/authz_basic_impl.go | 18 ---- master/internal/workspace/authz_iface.go | 6 -- master/internal/workspace/authz_permissive.go | 16 ---- master/internal/workspace/authz_rbac.go | 32 ------- ...0917113855_modify-tcp-rbac-perms.tx.up.sql | 5 + 16 files changed, 310 insertions(+), 127 deletions(-) create mode 100644 master/internal/configpolicy/authz_basic_impl.go create mode 100644 master/internal/configpolicy/authz_iface.go create mode 100644 master/internal/configpolicy/authz_permissive.go create mode 100644 master/internal/configpolicy/authz_rbac.go create mode 100644 master/static/migrations/20240917113855_modify-tcp-rbac-perms.tx.up.sql diff --git a/master/.mockery.yaml b/master/.mockery.yaml index 5782c892cae..b4a35614184 100644 --- a/master/.mockery.yaml +++ b/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: diff --git a/master/internal/api_config_policies.go b/master/internal/api_config_policies.go index 193a49af74a..297c023031b 100644 --- a/master/internal/api_config_policies.go +++ b/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) { diff --git a/master/internal/api_config_policies_intg_test.go b/master/internal/api_config_policies_intg_test.go index 626dd146d96..c1527e9132a 100644 --- a/master/internal/api_config_policies_intg_test.go +++ b/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 +} diff --git a/master/internal/cluster/authz_basic_impl.go b/master/internal/cluster/authz_basic_impl.go index bccbac2033e..13c8a1fc17d 100644 --- a/master/internal/cluster/authz_basic_impl.go +++ b/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{}) } diff --git a/master/internal/cluster/authz_iface.go b/master/internal/cluster/authz_iface.go index 3f1da55f3b6..b505a8db8aa 100644 --- a/master/internal/cluster/authz_iface.go +++ b/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. diff --git a/master/internal/cluster/authz_permissive.go b/master/internal/cluster/authz_permissive.go index e97ef917097..108bea73fb7 100644 --- a/master/internal/cluster/authz_permissive.go +++ b/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{}) } diff --git a/master/internal/configpolicy/authz_basic_impl.go b/master/internal/configpolicy/authz_basic_impl.go new file mode 100644 index 00000000000..e9dfe6ac0e0 --- /dev/null +++ b/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{}) +} diff --git a/master/internal/configpolicy/authz_iface.go b/master/internal/configpolicy/authz_iface.go new file mode 100644 index 00000000000..15f420a63e6 --- /dev/null +++ b/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] diff --git a/master/internal/configpolicy/authz_permissive.go b/master/internal/configpolicy/authz_permissive.go new file mode 100644 index 00000000000..b8ae61d36bb --- /dev/null +++ b/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) +} diff --git a/master/internal/configpolicy/authz_rbac.go b/master/internal/configpolicy/authz_rbac.go new file mode 100644 index 00000000000..263eb2da4e6 --- /dev/null +++ b/master/internal/configpolicy/authz_rbac.go @@ -0,0 +1,96 @@ +package configpolicy + +import ( + "context" + "strconv" + + log "github.com/sirupsen/logrus" + + "github.com/determined-ai/determined/master/internal/db" + "github.com/determined-ai/determined/master/internal/rbac/audit" + "github.com/determined-ai/determined/master/pkg/model" + "github.com/determined-ai/determined/master/pkg/ptrs" + "github.com/determined-ai/determined/proto/pkg/rbacv1" + "github.com/determined-ai/determined/proto/pkg/workspacev1" +) + +func init() { + AuthZProvider.Register("rbac", &ConfigPolicyAuthZRBAC{}) +} + +// ConfigPolicyAuthZRBAC is RBAC authorization for config policies. +type ConfigPolicyAuthZRBAC struct{} + +// CanModifyWorkspaceConfigPolicies determines whether a user can modify +// workspace task config policies. +func (r *ConfigPolicyAuthZRBAC) CanModifyWorkspaceConfigPolicies( + ctx context.Context, curUser model.User, workspace *workspacev1.Workspace, +) (err error) { + fields := audit.ExtractLogFields(ctx) + addConfigPolicyInfo(curUser, workspace, fields, []rbacv1.PermissionType{ + rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_WORKSPACE_CONFIG_POLICIES, + }) + defer func() { + audit.LogFromErr(fields, err) + }() + + return db.DoesPermissionMatch(ctx, curUser.ID, ptrs.Ptr(workspace.Id), + rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_WORKSPACE_CONFIG_POLICIES) +} + +// CanViewWorkspaceConfigPolicies determines whether a user can view +// workspace task config policies. +func (r *ConfigPolicyAuthZRBAC) CanViewWorkspaceConfigPolicies( + ctx context.Context, curUser model.User, workspace *workspacev1.Workspace, +) (err error) { + fields := audit.ExtractLogFields(ctx) + addConfigPolicyInfo(curUser, workspace, fields, []rbacv1.PermissionType{ + rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_WORKSPACE_CONFIG_POLICIES, + }) + defer func() { + audit.LogFromErr(fields, err) + }() + + return db.DoesPermissionMatch(ctx, curUser.ID, ptrs.Ptr(workspace.Id), + rbacv1.PermissionType_PERMISSION_TYPE_VIEW_WORKSPACE_CONFIG_POLICIES) +} + +// CanModifyGlobalConfigPolicies checks if the user can modify global +// task config policies. +func (r *ConfigPolicyAuthZRBAC) CanModifyGlobalConfigPolicies( + ctx context.Context, curUser *model.User, +) error { + return db.DoesPermissionMatch( + ctx, + curUser.ID, + nil, + rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_GLOBAL_CONFIG_POLICIES, + ) +} + +// CanViewGlobalConfigPolicies checks if the user can view global task config policies. +func (r *ConfigPolicyAuthZRBAC) CanViewGlobalConfigPolicies( + ctx context.Context, curUser *model.User, +) error { + return db.DoesPermissionMatch( + ctx, + curUser.ID, + nil, + rbacv1.PermissionType_PERMISSION_TYPE_VIEW_GLOBAL_CONFIG_POLICIES, + ) +} + +func addConfigPolicyInfo(curUser model.User, + workspace *workspacev1.Workspace, + logFields log.Fields, + permissions []rbacv1.PermissionType, +) { + logFields["userID"] = curUser.ID + logFields["permissionsRequired"] = []audit.PermissionWithSubject{ + { + PermissionTypes: permissions, + SubjectType: "config policy", + SubjectIDs: []string{strconv.Itoa(int(workspace.Id))}, + }, + } +} diff --git a/master/internal/db/postgres_rbac_intg_test.go b/master/internal/db/postgres_rbac_intg_test.go index 447355c8e02..bb5918beb34 100644 --- a/master/internal/db/postgres_rbac_intg_test.go +++ b/master/internal/db/postgres_rbac_intg_test.go @@ -427,7 +427,8 @@ func TestPermissionMatch(t *testing.T) { err = DoesPermissionMatch(ctx, userIDWorkspaceAdmin, &workspaceID, rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_GLOBAL_CONFIG_POLICIES) - require.NoError(t, err) + require.IsType(t, authz.PermissionDeniedError{}, err, + "user should not have permission to edit global config policies") err = DoesPermissionMatch(ctx, userIDWorkspaceAdmin, &workspaceID, rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_WORKSPACE_CONFIG_POLICIES) @@ -530,7 +531,8 @@ func TestPermissionMatch(t *testing.T) { err = DoesPermissionMatchAll(ctx, userIDWorkspaceAdmin, rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_GLOBAL_CONFIG_POLICIES, workspaceID) - require.NoError(t, err) + require.IsType(t, authz.PermissionDeniedError{}, err, + "user should not have permission to edit global config policies") err = DoesPermissionMatchAll(ctx, userIDWorkspaceAdmin, rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_WORKSPACE_CONFIG_POLICIES, workspaceID) @@ -653,7 +655,8 @@ func TestPermissionMatch(t *testing.T) { err = DoesPermissionMatchAll(ctx, userIDWorkspaceAdmin, rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_GLOBAL_CONFIG_POLICIES, workspaceIDs...) - require.NoError(t, err) + require.IsType(t, authz.PermissionDeniedError{}, err, + "user should not have permission to edit global config policies") err = DoesPermissionMatchAll(ctx, userIDWorkspaceAdmin, rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_WORKSPACE_CONFIG_POLICIES, workspaceIDs...) @@ -832,7 +835,8 @@ func TestPermissionMatch(t *testing.T) { err = DoPermissionsExist(ctx, userIDWorkspaceAdmin, rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_GLOBAL_CONFIG_POLICIES) - require.NoError(t, err) + require.IsType(t, authz.PermissionDeniedError{}, err, + "user should not have permission to edit global config policies") err = DoPermissionsExist(ctx, userIDWorkspaceAdmin, rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_WORKSPACE_CONFIG_POLICIES) diff --git a/master/internal/workspace/authz_basic_impl.go b/master/internal/workspace/authz_basic_impl.go index 718b4a845c1..fea3692da41 100644 --- a/master/internal/workspace/authz_basic_impl.go +++ b/master/internal/workspace/authz_basic_impl.go @@ -190,24 +190,6 @@ func (a *WorkspaceAuthZBasic) CanSetWorkspacesDefaultPools( return nil } -// CanModifyWorkspaceConfigPolicies returns a nil error or the user is not an admin -// or owner of the workspace. -func (a *WorkspaceAuthZBasic) 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 *WorkspaceAuthZBasic) CanViewWorkspaceConfigPolicies( - ctx context.Context, curUser model.User, workspace *workspacev1.Workspace, -) error { - return nil -} - func init() { AuthZProvider.Register("basic", &WorkspaceAuthZBasic{}) } diff --git a/master/internal/workspace/authz_iface.go b/master/internal/workspace/authz_iface.go index 0c20cd21869..cb4b77aa401 100644 --- a/master/internal/workspace/authz_iface.go +++ b/master/internal/workspace/authz_iface.go @@ -84,12 +84,6 @@ type WorkspaceAuthZ interface { CanUnpinWorkspace( ctx context.Context, curUser model.User, workspace *workspacev1.Workspace, ) error - // 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 } // AuthZProvider providers WorkspaceAuthZ implementations. diff --git a/master/internal/workspace/authz_permissive.go b/master/internal/workspace/authz_permissive.go index af6031cd204..df7bfffe93a 100644 --- a/master/internal/workspace/authz_permissive.go +++ b/master/internal/workspace/authz_permissive.go @@ -179,22 +179,6 @@ func (p *WorkspaceAuthZPermissive) CanViewResourceQuotas( return (&WorkspaceAuthZBasic{}).CanViewResourceQuotas(ctx, curUser) } -// CanModifyWorkspaceConfigPolicies RBAC authz but enforces basic authz. -func (p *WorkspaceAuthZPermissive) CanModifyWorkspaceConfigPolicies( - ctx context.Context, curUser model.User, workspace *workspacev1.Workspace, -) error { - _ = (&WorkspaceAuthZRBAC{}).CanModifyWorkspaceConfigPolicies(ctx, curUser, workspace) - return (&WorkspaceAuthZBasic{}).CanModifyWorkspaceConfigPolicies(ctx, curUser, workspace) -} - -// CanViewWorkspaceConfigPolicies RBAC authz but enforces basic authz. -func (p *WorkspaceAuthZPermissive) CanViewWorkspaceConfigPolicies( - ctx context.Context, curUser model.User, workspace *workspacev1.Workspace, -) error { - _ = (&WorkspaceAuthZRBAC{}).CanViewWorkspaceConfigPolicies(ctx, curUser, workspace) - return (&WorkspaceAuthZBasic{}).CanViewWorkspaceConfigPolicies(ctx, curUser, workspace) -} - func init() { AuthZProvider.Register("permissive", &WorkspaceAuthZPermissive{}) } diff --git a/master/internal/workspace/authz_rbac.go b/master/internal/workspace/authz_rbac.go index b2adcf89a8b..36c530ec598 100644 --- a/master/internal/workspace/authz_rbac.go +++ b/master/internal/workspace/authz_rbac.go @@ -412,38 +412,6 @@ func (r *WorkspaceAuthZRBAC) CanViewResourceQuotas(ctx context.Context, rbacv1.PermissionType_PERMISSION_TYPE_VIEW_RESOURCE_QUOTAS) } -// CanModifyWorkspaceConfigPolicies determines whether a user can modify -// workspace task config policies. -func (r *WorkspaceAuthZRBAC) CanModifyWorkspaceConfigPolicies( - ctx context.Context, curUser model.User, workspace *workspacev1.Workspace, -) (err error) { - fields := audit.ExtractLogFields(ctx) - addInfoWithoutWorkspace(curUser, fields, - rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_WORKSPACE_CONFIG_POLICIES) - defer func() { - audit.LogFromErr(fields, err) - }() - - return db.DoesPermissionMatch(ctx, curUser.ID, nil, - rbacv1.PermissionType_PERMISSION_TYPE_MODIFY_WORKSPACE_CONFIG_POLICIES) -} - -// CanViewWorkspaceConfigPolicies determines whether a user can view -// workspace task config policies. -func (r *WorkspaceAuthZRBAC) CanViewWorkspaceConfigPolicies( - ctx context.Context, curUser model.User, workspace *workspacev1.Workspace, -) (err error) { - fields := audit.ExtractLogFields(ctx) - addInfoWithoutWorkspace(curUser, fields, - rbacv1.PermissionType_PERMISSION_TYPE_VIEW_WORKSPACE_CONFIG_POLICIES) - defer func() { - audit.LogFromErr(fields, err) - }() - - return db.DoesPermissionMatch(ctx, curUser.ID, nil, - rbacv1.PermissionType_PERMISSION_TYPE_VIEW_WORKSPACE_CONFIG_POLICIES) -} - func hasPermissionOnWorkspace(ctx context.Context, uid model.UserID, workspace *workspacev1.Workspace, permID rbacv1.PermissionType, ) error { diff --git a/master/static/migrations/20240917113855_modify-tcp-rbac-perms.tx.up.sql b/master/static/migrations/20240917113855_modify-tcp-rbac-perms.tx.up.sql new file mode 100644 index 00000000000..33ed59445fd --- /dev/null +++ b/master/static/migrations/20240917113855_modify-tcp-rbac-perms.tx.up.sql @@ -0,0 +1,5 @@ +/* Remove modify global config policies RBAC permission from WorkspaceAdmin. */ +DELETE FROM permission_assignments WHERE permission_id = 11004 and role_id = 2; + +/* Assign global_only to global RBAC permission. */ +UPDATE permissions SET global_only = true WHERE id = 11004;