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: allow users with role Viewer and above to view resource quotas #9822

Merged
merged 8 commits into from
Aug 15, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions harness/determined/common/api/bindings.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions master/internal/api_workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -1558,8 +1558,8 @@ func (a *apiServer) GetKubernetesResourceQuotas(
if err != nil {
return nil, err
}
err = workspace.AuthZProvider.Get().CanGetWorkspaceID(
ctx, *curUser, req.Id,
err = workspace.AuthZProvider.Get().CanViewResourceQuotas(
ctx, *curUser,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of removing the CanGetWorkspaceID check, should we maybe do
a.getWorkspaceAndCheckCanDoActions(ctx, req.Id, false, workspace.AuthZProvider.Get().CanGetWorkspace, workspace.AuthZProvider.Get().CanViewResourceQuotas) so that we perform both of these checks? Or do we not need the CanGetWorkspaceID check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we can check 2 permissions at a time, so I added both checks in a different way

)
if err != nil {
return nil, err
Expand Down
6 changes: 6 additions & 0 deletions master/internal/workspace/authz_basic_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,12 @@ func (a *WorkspaceAuthZBasic) CanSetResourceQuotas(ctx context.Context, curUser
return nil
}

// CanViewResourceQuotas returns a nil error.
func (a *WorkspaceAuthZBasic) CanViewResourceQuotas(ctx context.Context, curUser model.User,
) error {
return nil
}

// CanSetWorkspacesDefaultPools returns a nil error.
func (a *WorkspaceAuthZBasic) CanSetWorkspacesDefaultPools(
ctx context.Context, curUser model.User, workspace *workspacev1.Workspace,
Expand Down
2 changes: 1 addition & 1 deletion master/internal/workspace/authz_iface.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ type WorkspaceAuthZ interface {
CanCreateWorkspaceWithCheckpointStorageConfig(ctx context.Context, curUser model.User) error
CanSetWorkspaceNamespaceBindings(ctx context.Context, curUser model.User) error
CanSetResourceQuotas(ctx context.Context, curUser model.User) error

CanViewResourceQuotas(ctx context.Context, curUser model.User) error
// PATCH /api/v1/workspaces/:workspace_id
CanSetWorkspacesName(
ctx context.Context, curUser model.User, workspace *workspacev1.Workspace,
Expand Down
8 changes: 8 additions & 0 deletions master/internal/workspace/authz_permissive.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,14 @@ func (p *WorkspaceAuthZPermissive) CanSetResourceQuotas(
return (&WorkspaceAuthZBasic{}).CanSetResourceQuotas(ctx, curUser)
}

// CanViewResourceQuotas calls RBAC authz but enforces basic authz.
func (p *WorkspaceAuthZPermissive) CanViewResourceQuotas(
ctx context.Context, curUser model.User,
) error {
_ = (&WorkspaceAuthZRBAC{}).CanViewResourceQuotas(ctx, curUser)
return (&WorkspaceAuthZBasic{}).CanViewResourceQuotas(ctx, curUser)
}

func init() {
AuthZProvider.Register("permissive", &WorkspaceAuthZPermissive{})
}
15 changes: 15 additions & 0 deletions master/internal/workspace/authz_rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,21 @@ func (r *WorkspaceAuthZRBAC) CanSetResourceQuotas(ctx context.Context,
rbacv1.PermissionType_PERMISSION_TYPE_SET_RESOURCE_QUOTAS)
}

// CanViewResourceQuotas determines whether a user can view resource quotas on a workspace.
func (r *WorkspaceAuthZRBAC) CanViewResourceQuotas(ctx context.Context,
curUser model.User,
) (err error) {
fields := audit.ExtractLogFields(ctx)
addInfoWithoutWorkspace(curUser, fields,
rbacv1.PermissionType_PERMISSION_TYPE_VIEW_RESOURCE_QUOTAS)
defer func() {
audit.LogFromErr(fields, err)
}()

return db.DoesPermissionMatch(ctx, curUser.ID, nil,
rbacv1.PermissionType_PERMISSION_TYPE_VIEW_RESOURCE_QUOTAS)
}

func hasPermissionOnWorkspace(ctx context.Context, uid model.UserID,
workspace *workspacev1.Workspace, permID rbacv1.PermissionType,
) error {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/* Add RBAC permissions for creating/updating/deleting namespace-workspace bindings and
resource quotas. */
INSERT into permissions(id, name, global_only) VALUES
(11003, 'view resource quotas', false);

INSERT INTO permission_assignments(permission_id, role_id) VALUES
(11003, 1),
(11003, 2),
(11003, 4),
salonig23 marked this conversation as resolved.
Show resolved Hide resolved
(11003, 5),
(11003, 7),
(11003, 8),
(11003, 9);
18 changes: 12 additions & 6 deletions proto/pkg/rbacv1/rbac.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions proto/src/determined/rbac/v1/rbac.proto
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ enum PermissionType {

// Ability to set resource quotas on workspaces.
PERMISSION_TYPE_SET_RESOURCE_QUOTAS = 11002;

// Ability to view resource quotas on workspaces.
PERMISSION_TYPE_VIEW_RESOURCE_QUOTAS = 11003;
}

// RoleAssignmentSummary is used to describe permissions a user has.
Expand Down
119 changes: 91 additions & 28 deletions webui/react/src/components/WorkspaceCreateModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ const WorkspaceCreateModalComponent: React.FC<Props> = ({ onClose, workspaceId }
canModifyWorkspaceCheckpointStorage,
canSetWorkspaceNamespaceBindings,
canSetResourceQuotas,
canViewResourceQuotas,
canModifyWorkspace,
} = usePermissions();
const info = useObservable(determinedStore.info);
const [form] = Form.useForm<FormInputs>();
Expand Down Expand Up @@ -119,6 +121,8 @@ const WorkspaceCreateModalComponent: React.FC<Props> = ({ onClose, workspaceId }
[resourceManagers, workspaceId],
);

const loadedResourceQuotaList = Loadable.getOrElse({}, resourceQuotasList);
johnkim-det marked this conversation as resolved.
Show resolved Hide resolved

const initFields = useCallback(
(ws?: Workspace) => {
if (ws) {
Expand All @@ -143,12 +147,12 @@ const WorkspaceCreateModalComponent: React.FC<Props> = ({ onClose, workspaceId }
namespaceBindingsList.forEach((bindingsList) => {
form.setFieldValue('bindings', { ...bindingsList });
});
resourceQuotasList.forEach((quotasList) => {
form.setFieldValue('resourceQuotas', { ...quotasList });
});
for (const key in loadedResourceQuotaList) {
form.setFieldValue(['resourceQuotas', key], loadedResourceQuotaList[key]);
}
}
},
[form, namespaceBindingsList, resourceQuotasList],
[form, loadedResourceQuotaList, namespaceBindingsList],
);

const loadableWorkspace = useObservable(workspaceStore.getWorkspace(workspaceId || 0));
Expand Down Expand Up @@ -185,7 +189,10 @@ const WorkspaceCreateModalComponent: React.FC<Props> = ({ onClose, workspaceId }
required: true,
},
]}>
<Input maxLength={80} />
<Input
disabled={(workspace && !canModifyWorkspace({ workspace })) ?? false}
salonig23 marked this conversation as resolved.
Show resolved Hide resolved
maxLength={80}
/>
</Form.Item>
{canSetWorkspaceNamespaceBindings && resourceManagers.length > 0 && (
<>
Expand All @@ -211,7 +218,12 @@ const WorkspaceCreateModalComponent: React.FC<Props> = ({ onClose, workspaceId }
name={['bindings', name, 'autoCreateNamespace']}
valuePropName="checked">
<Toggle
onChange={() => form.setFieldValue(['resourceQuotas', name], undefined)}
onChange={() => {
form.setFieldValue(['resourceQuotas', name], undefined);
if (loadedResourceQuotaList[name]) {
delete loadedResourceQuotaList[name];
}
ashtonG marked this conversation as resolved.
Show resolved Hide resolved
}}
/>
</Form.Item>
{canSetResourceQuotas && (
Expand All @@ -238,6 +250,25 @@ const WorkspaceCreateModalComponent: React.FC<Props> = ({ onClose, workspaceId }
</>
</>
)}
{!canSetWorkspaceNamespaceBindings &&
canViewResourceQuotas &&
workspaceId &&
info.branding === BrandingType.HPE &&
resourceManagers.length > 0 && (
<>
<Divider />
Resource Quotas
<>
{resourceManagers.map((name) => (
<Fragment key={name}>
<Form.Item label={`Cluster Name: ${name}`} name={['resourceQuotas', name]}>
<Input disabled={true} />
salonig23 marked this conversation as resolved.
Show resolved Hide resolved
</Form.Item>
</Fragment>
))}
</>
</>
)}
{canModifyAUG && (
<>
<Divider />
Expand Down Expand Up @@ -342,16 +373,20 @@ const WorkspaceCreateModalComponent: React.FC<Props> = ({ onClose, workspaceId }
loadableWorkspace,
form,
idPrefix,
workspace,
canModifyWorkspace,
canSetWorkspaceNamespaceBindings,
resourceManagers,
canViewResourceQuotas,
info.branding,
canModifyAUG,
useAgentUser,
useAgentGroup,
canModifyCPS,
useCheckpointStorage,
watchBindings,
info.branding,
canSetResourceQuotas,
loadedResourceQuotaList,
]);

const handleSubmit = useCallback(async () => {
Expand Down Expand Up @@ -430,8 +465,22 @@ const WorkspaceCreateModalComponent: React.FC<Props> = ({ onClose, workspaceId }
resourceQuotaBody['id'] = response.id;
}

if (values.resourceQuotas) {
resourceQuotaBody['clusterQuotaPairs'] = pick(values.resourceQuotas, resourceManagers);
let submittedRQ = values.resourceQuotas;
if (loadedResourceQuotaList) {
submittedRQ = Object.keys(values.resourceQuotas ?? {}).reduce(
(memo: Record<string, number>, name) => {
if (values.resourceQuotas) {
if (values.resourceQuotas[name] !== loadedResourceQuotaList[name]) {
memo[name] = values.resourceQuotas[name];
}
}
return memo;
},
{},
);
}
if (submittedRQ && Object.keys(submittedRQ).length !== 0) {
resourceQuotaBody['clusterQuotaPairs'] = pick(submittedRQ, resourceManagers);
await setResourceQuotas(resourceQuotaBody);
}
form.resetFields();
Expand All @@ -455,26 +504,40 @@ const WorkspaceCreateModalComponent: React.FC<Props> = ({ onClose, workspaceId }
});
}
}
}, [form, workspaceId, canModifyAUG, canModifyCPS, resourceManagers]);
}, [form, canModifyAUG, canModifyCPS, workspaceId, loadedResourceQuotaList, resourceManagers]);

return (
<Modal
cancel
size="medium"
submit={{
form: idPrefix + FORM_ID,
handleError,
handler: handleSubmit,
text: 'Save Workspace',
}}
title={`${workspaceId ? 'Edit' : 'New'} Workspace`}
onClose={() => {
initFields(undefined);
onClose?.();
}}>
{modalContent}
</Modal>
);
if ((workspace && !canModifyWorkspace({ workspace })) ?? false) {
salonig23 marked this conversation as resolved.
Show resolved Hide resolved
return (
<Modal
size="medium"
title="Workspace Config"
salonig23 marked this conversation as resolved.
Show resolved Hide resolved
onClose={() => {
initFields(undefined);
onClose?.();
}}>
{modalContent}
</Modal>
);
} else {
return (
<Modal
cancel
size="medium"
submit={{
form: idPrefix + FORM_ID,
handleError,
handler: handleSubmit,
text: 'Save Workspace',
}}
title={`${workspaceId ? 'Edit' : 'New'} Workspace`}
onClose={() => {
initFields(undefined);
onClose?.();
}}>
{modalContent}
</Modal>
);
}
};

export default WorkspaceCreateModalComponent;
12 changes: 12 additions & 0 deletions webui/react/src/hooks/usePermissions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export interface PermissionsHook {
canViewModelRegistry: (arg0: WorkspacePermissionsArgs) => boolean;
canViewWorkspace: (arg0: WorkspacePermissionsArgs) => boolean;
canViewWorkspaces: boolean;
canViewResourceQuotas: boolean;
loading: boolean;
}

Expand Down Expand Up @@ -208,6 +209,7 @@ const usePermissions = (): PermissionsHook => {
canViewGroups: canViewGroups(rbacOpts),
canViewModelRegistry: (args: WorkspacePermissionsArgs) =>
canViewModelRegistry(rbacOpts, args.workspace),
canViewResourceQuotas: canViewResourceQuotas(rbacOpts),
canViewWorkspace: (args: WorkspacePermissionsArgs) =>
canViewWorkspace(rbacOpts, args.workspace),
canViewWorkspaces: canViewWorkspaces(rbacOpts),
Expand Down Expand Up @@ -626,6 +628,16 @@ const canSetResourceQuotas = ({
);
};

const canViewResourceQuotas = ({
currentUser,
rbacEnabled,
userAssignments,
userRoles,
}: RbacOptsProps): boolean => {
const permitted = relevantPermissions(userAssignments, userRoles);
return !!currentUser && (!rbacEnabled || permitted.has(V1PermissionType.VIEWRESOURCEQUOTAS));
};

const canViewWorkspace = (
{ rbacEnabled, userAssignments, userRoles }: RbacOptsProps,
workspace?: PermissionWorkspace,
Expand Down
Loading
Loading