Add database and Windows access sections to the role editor #47992
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,8 @@ import { | |
| kubernetesVerbOptions, | ||
| KubernetesResourceModel, | ||
| AppAccessSpec, | ||
| DatabaseAccessSpec, | ||
| WindowsDesktopAccessSpec, | ||
| } from './standardmodel'; | ||
| import { EditorSaveCancelButton } from './Shared'; | ||
| import { RequiresResetToStandard } from './RequiresResetToStandard'; | ||
|
|
@@ -359,7 +361,13 @@ const Section = ({ | |
| /** | ||
| * All access spec kinds, in order of appearance in the resource kind dropdown. | ||
| */ | ||
| const allAccessSpecKinds: AccessSpecKind[] = [ | ||
| 'kube_cluster', | ||
| 'node', | ||
| 'app', | ||
| 'db', | ||
| 'windows_desktop', | ||
| ]; | ||
|
|
||
| /** Maps access specification kind to UI component configuration. */ | ||
| const specSections: Record< | ||
|
|
@@ -385,6 +393,16 @@ const specSections: Record< | |
| tooltip: 'Configures access to applications', | ||
| component: AppAccessSpecSection, | ||
| }, | ||
| db: { | ||
| title: 'Databases', | ||
| tooltip: 'Configures access to databases', | ||
| component: DatabaseAccessSpecSection, | ||
| }, | ||
| windows_desktop: { | ||
| title: 'Windows Desktops', | ||
| tooltip: 'Configures access to Windows desktops', | ||
| component: WindowsDesktopAccessSpecSection, | ||
| }, | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -636,6 +654,97 @@ export function AppAccessSpecSection({ | |
| ); | ||
| } | ||
|
|
||
| export function DatabaseAccessSpecSection({ | ||
| value, | ||
| isProcessing, | ||
| onChange, | ||
| }: SectionProps<DatabaseAccessSpec>) { | ||
| return ( | ||
| <> | ||
| <Box mb={3}> | ||
| <Text typography="body3" mb={1}> | ||
| Labels | ||
| </Text> | ||
| <LabelsInput | ||
| disableBtns={isProcessing} | ||
| labels={value.labels} | ||
| setLabels={labels => onChange?.({ ...value, labels })} | ||
| /> | ||
| </Box> | ||
| <FieldSelectCreatable | ||
|
There was a problem hiding this comment. Is any of these three fields ever supposed to show suggestions? When I click on the input and it says "No options", it feels like something is broken. If we never intend to show suggestions here, perhaps we could not show that "No options" message? 
There was a problem hiding this comment. That's a good point. I'll disable these, just as we disable them in the user invitation dialog. There was a problem hiding this comment. @ravicious Well, I thought it's an one-liner, but no. The existing UI that does it better way actually reimplements a part of the internal React Select logic; basically takes over the responsibility of creating items. Since this is not a reusable component, I'd rather first discuss what we would like the interaction to look with the UX team and then apply it consistently to other places (yes, we do the same thing in other places). There was a problem hiding this comment. Plot twist! I found out about |
||
| isMulti | ||
| label="Names" | ||
|
There was a problem hiding this comment. I'm not sure if I should direct this feedback to you or the design team, but I'd prepend "Database" to the labels of all three fields. Teleport and its configuration and roles involved a lot of different names, roles and users. If it's not explicitly spelled out, it's hard to know what kind of names I'm supposed to put here without hovering over the info icon. Or perhaps alternatively we could not prepend "Database" but just spell out the text of the info icon within the form, without requiring the user to hover over it. There was a problem hiding this comment. I'll change to "database names", "database users', and "database roles"./ |
||
| toolTipContent="List of database names that this role is allowed to connect to" | ||
| isDisabled={isProcessing} | ||
| formatCreateLabel={label => `Name: ${label}`} | ||
| components={{ | ||
| DropdownIndicator: null, | ||
| }} | ||
| value={value.names} | ||
| onChange={names => onChange?.({ ...value, names })} | ||
| /> | ||
| <FieldSelectCreatable | ||
| isMulti | ||
| label="Roles" | ||
| toolTipContent="List of database roles for automatic user creation" | ||
|
There was a problem hiding this comment. I'm unsure what this means. Could you explain the automatic user creation to me? There was a problem hiding this comment. This help message is a bit vague. I found another one in the docs that is more clear IMHO:
If we spell it out as "Database Roles" instead of just "Roles", I think it'll be more even clear that we're talking about roles in the database sense of that word. There was a problem hiding this comment. Oh, but even then the description I provided is not that great because it fails to mention that this is relevant only for automatic user provisioning. I think this field should be clearly marked as optional and also have a link to the docs somewhere. There was a problem hiding this comment. I'll explain as much as I can, but we'll need to rethink linking to docs here, as these tooltips are sticky, and therefore not clickable. Also we still don't have clear conclusion about unified presentation of links in our app. @rishibarbhaya-design, that's some food for thought. |
||
| isDisabled={isProcessing} | ||
| formatCreateLabel={label => `Role: ${label}`} | ||
| components={{ | ||
| DropdownIndicator: null, | ||
| }} | ||
| value={value.roles} | ||
| onChange={roles => onChange?.({ ...value, roles })} | ||
| /> | ||
| <FieldSelectCreatable | ||
| isMulti | ||
| label="Users" | ||
|
There was a problem hiding this comment. I don't know if this is something we deliberately decided or not, but from the time I spent playing with our db access, it seems to me that the order of the fields should be database names, database users and then database roles. You might want to confirm it with the db team or someone who has actually set up db access in a production cluster. DB names and users are something you have to set up to use DB access. Roles are optional and are only relevant for auto user provisioning. DB users also directly impacts what you later see in the dropdown in Connect when you're about to connect to a database. I think it's a good reason for those fields to be above db roles. There was a problem hiding this comment. You're right, switching to names -> users -> roles. |
||
| toolTipContent="List of database users that this role is allowed to connect as" | ||
|
There was a problem hiding this comment. Would "Database Users" make more sense here, or make it less ambiguous at least? If UX explicitly said use "Users" instead of "Database Users", ignore this comment. |
||
| isDisabled={isProcessing} | ||
| formatCreateLabel={label => `User: ${label}`} | ||
| components={{ | ||
| DropdownIndicator: null, | ||
| }} | ||
|
Comment on lines
+718
to
+720
|
||
| value={value.users} | ||
| onChange={users => onChange?.({ ...value, users })} | ||
| mb={0} | ||
| /> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| export function WindowsDesktopAccessSpecSection({ | ||
| value, | ||
| isProcessing, | ||
| onChange, | ||
| }: SectionProps<WindowsDesktopAccessSpec>) { | ||
| return ( | ||
| <> | ||
| <Box mb={3}> | ||
| <Text typography="body3" mb={1}> | ||
| Labels | ||
| </Text> | ||
| <LabelsInput | ||
| disableBtns={isProcessing} | ||
| labels={value.labels} | ||
| setLabels={labels => onChange?.({ ...value, labels })} | ||
| /> | ||
| </Box> | ||
| <FieldSelectCreatable | ||
| isMulti | ||
| label="Logins" | ||
| toolTipContent="List of desktop logins that this role is allowed to use" | ||
| isDisabled={isProcessing} | ||
| formatCreateLabel={label => `Login: ${label}`} | ||
| components={{ | ||
| DropdownIndicator: null, | ||
| }} | ||
| value={value.logins} | ||
| onChange={logins => onChange?.({ ...value, logins })} | ||
| /> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| export const EditorWrapper = styled(Box)<{ mute?: boolean }>` | ||
| opacity: ${p => (p.mute ? 0.4 : 1)}; | ||
| pointer-events: ${p => (p.mute ? 'none' : '')}; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a blocker for the initial release, but long-term I think the role editor should guide you wrt the wildcard value and somehow tell you which fields accept it as a value. It's important part of label matching and the current UI doesn't focus on that.
I know there are also some fields where you can use wildcard as a pattern-matching tool (e.g.,
dev-*), but idk off the top of my head which fields support that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely. The UX team didn't give us any directions here, but I'll have to discuss it with them. In one of my previous PRs, I added an explanation of the wildcard to the help test in the Kubernetes resource view; I'll look into whether these fields also support wildcards and document it if I find anything.