Add database and Windows access sections to the role editor #47992

bl-nero · 2024-10-28T12:44:03Z

Also, rearrange some of the tests to get rid of large swaths of duplicate data.

To turn on the new UI, go to developer tools -> Application -> Local storage and add a grv_teleport_use_new_role_editor key set to true. This will be lifted once UI is good to be released.

Figma:

aws-amplify-us-west-2 · 2024-10-28T12:45:44Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-47992.d3pp5qlev8mo18.amplifyapp.com

avatus

Awesome!

avatus · 2024-11-04T17:53:53Z

web/packages/teleport/src/Roles/RoleEditor/StandardEditor.tsx

+      <FieldSelectCreatable
+        isMulti
+        label="Roles"
+        toolTipContent="List of database roles for automatic user creation"


I'm unsure what this means. Could you explain the automatic user creation to me?

This help message is a bit vague. I found another one in the docs that is more clear IMHO:

Roles the user can access on a database when they authenticate to a database server.

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.

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.

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.

avatus · 2024-11-04T17:54:52Z

web/packages/teleport/src/Roles/RoleEditor/StandardEditor.tsx

+        label="Users"
+        toolTipContent="List of database users that this role is allowed to connect as"


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.

avatus · 2024-11-04T17:55:17Z

web/packages/teleport/src/Roles/RoleEditor/StandardEditor.tsx

+        components={{
+          DropdownIndicator: null,
+        }}


TIL! Love it

ravicious · 2024-11-14T14:56:26Z

I'll take a look tomorrow, I'm a bit swamped with PR reviews.

ravicious

Looks very good! I added a bunch of UX issues/suggestions that can be addressed outside of this PR.

ravicious · 2024-11-15T14:23:46Z

web/packages/teleport/src/Roles/RoleEditor/StandardEditor.tsx

+          setLabels={labels => onChange?.({ ...value, labels })}
+        />
+      </Box>
+      <FieldSelectCreatable


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?

That's a good point. I'll disable these, just as we disable them in the user invitation dialog.

@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).

Plot twist! I found out about openMenuOnClick, I'll use that. It was a one-liner, after all. Well, almost.

ravicious · 2024-11-15T14:28:47Z

web/packages/teleport/src/Roles/RoleEditor/StandardEditor.tsx

+      </Box>
+      <FieldSelectCreatable
+        isMulti
+        label="Names"


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.

I'll change to "database names", "database users', and "database roles"./

ravicious · 2024-11-15T14:32:00Z

web/packages/teleport/src/Roles/RoleEditor/StandardEditor.tsx

+      <FieldSelectCreatable
+        isMulti
+        label="Roles"
+        toolTipContent="List of database roles for automatic user creation"


This help message is a bit vague. I found another one in the docs that is more clear IMHO:

Roles the user can access on a database when they authenticate to a database server.

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.

ravicious · 2024-11-15T14:35:30Z

web/packages/teleport/src/Roles/RoleEditor/StandardEditor.tsx

+      <FieldSelectCreatable
+        isMulti
+        label="Roles"
+        toolTipContent="List of database roles for automatic user creation"


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.

ravicious · 2024-11-15T14:37:14Z

web/packages/teleport/src/Roles/RoleEditor/StandardEditor.tsx

+      />
+      <FieldSelectCreatable
+        isMulti
+        label="Users"


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.

You're right, switching to names -> users -> roles.

ravicious · 2024-11-15T14:41:23Z

web/packages/teleport/src/Roles/RoleEditor/StandardEditor.tsx

@@ -636,6 +654,97 @@ export function AppAccessSpecSection({
  );
 }

+export function DatabaseAccessSpecSection({


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.

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.

bl-nero · 2024-11-18T15:32:31Z

@ravicious @avatus PTAL at the last commit; if no comments, I'll merge it tomorrow.

ravicious

Looking good, I found one minor issue.

ravicious · 2024-11-18T16:27:33Z

web/packages/teleport/src/Roles/RoleEditor/StandardEditor.tsx

+        label="Database Names"
+        toolTipContent={
+          <>
+            List of database names that this role is allowed to connect to.


For some reason the dot at the end of this line is not visible in the tooltip for me.

Oops, that's a copy&paste blunder. Thanks for spotting. :)

* Add database and Windows access sections to the role editor * review * Fix ta typo

…49211) * Add database and Windows access sections to the role editor * review * Fix ta typo

Add database and Windows access sections to the role editor

5e1ff4e

bl-nero added the no-changelog Indicates that a PR does not require a changelog entry label Oct 28, 2024

github-actions bot added the size/md label Oct 28, 2024

github-actions bot requested a review from avatus October 28, 2024 12:44

github-actions bot added the ui label Oct 28, 2024

github-actions bot requested a review from kiosion October 28, 2024 12:44

bl-nero mentioned this pull request Oct 30, 2024

Create a WebInterface for roles creation #46612

Closed

avatus approved these changes Nov 4, 2024

View reviewed changes

bl-nero requested a review from ravicious November 13, 2024 16:45

ravicious approved these changes Nov 15, 2024

View reviewed changes

public-teleport-github-review-bot bot removed request for kiosion November 15, 2024 14:57

bl-nero added 2 commits November 18, 2024 13:07

Merge branch 'master' into bl-nero/role-editor-6

238a06f

review

0277b0c

avatus approved these changes Nov 18, 2024

View reviewed changes

ravicious approved these changes Nov 18, 2024

View reviewed changes

bl-nero added 2 commits November 19, 2024 14:29

Fix ta typo

4488980

Merge branch 'master' into bl-nero/role-editor-6

8769d12

bl-nero enabled auto-merge November 19, 2024 13:30

bl-nero added this pull request to the merge queue Nov 19, 2024

Merged via the queue into master with commit c1c7ce3 Nov 19, 2024
40 checks passed

bl-nero deleted the bl-nero/role-editor-6 branch November 19, 2024 13:47

bl-nero added a commit that referenced this pull request Nov 19, 2024

Add database and Windows access sections to the role editor (#47992)

0e122bf

* Add database and Windows access sections to the role editor * review * Fix ta typo

bl-nero mentioned this pull request Nov 19, 2024

[v17] Add database and Windows access sections to the role editor #49211

Merged

github-merge-queue bot pushed a commit that referenced this pull request Nov 20, 2024

Add database and Windows access sections to the role editor (#47992) (#…

eaadaec

…49211) * Add database and Windows access sections to the role editor * review * Fix ta typo

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add database and Windows access sections to the role editor #47992

Add database and Windows access sections to the role editor #47992

bl-nero commented Oct 28, 2024

aws-amplify-us-west-2 bot commented Oct 28, 2024

avatus left a comment

avatus Nov 4, 2024

ravicious Nov 15, 2024

ravicious Nov 15, 2024

bl-nero Nov 18, 2024 •

edited

Loading

avatus Nov 4, 2024

avatus Nov 4, 2024

ravicious commented Nov 14, 2024

ravicious left a comment

ravicious Nov 15, 2024

bl-nero Nov 18, 2024

bl-nero Nov 18, 2024

bl-nero Nov 18, 2024

ravicious Nov 15, 2024

bl-nero Nov 18, 2024

ravicious Nov 15, 2024

ravicious Nov 15, 2024

ravicious Nov 15, 2024

bl-nero Nov 18, 2024

ravicious Nov 15, 2024

bl-nero Nov 18, 2024

bl-nero commented Nov 18, 2024

ravicious left a comment

ravicious Nov 18, 2024

bl-nero Nov 19, 2024

		label="Users"
		toolTipContent="List of database users that this role is allowed to connect as"

Add database and Windows access sections to the role editor #47992

Add database and Windows access sections to the role editor #47992

Conversation

bl-nero commented Oct 28, 2024

aws-amplify-us-west-2 bot commented Oct 28, 2024

avatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bl-nero Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious commented Nov 14, 2024

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bl-nero commented Nov 18, 2024

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bl-nero Nov 18, 2024 •

edited

Loading