Add admin rule tab to the role editor #49764

bl-nero · 2024-12-04T14:31:30Z

Also tightens some constraints about standard role editor conformance.

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

Requires #49546
Contributes to #46612

bl-nero · 2024-12-06T09:29:47Z

@gzdunek @ravicious I have just realized that the design didn't include a remove button, and I followed it too literally. I added a capability to remove admin rules, will update the screenshot in a sec.

Also tightens some constraints about standard role editor conformance.

ravicious

I included a bunch of suggestions, but the admin rule tab seems to work fine.

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

ravicious · 2024-12-06T13:47:22Z

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


 describe('ServerAccessSpecSection', () => {
  const setup = () => {
    const onChange = jest.fn();
    let validator: Validator;
    render(
-      <StatefulSection<ServerAccessSpec>
+      <StatefulSection<ServerAccessSpec, AccessSpecValidationResult>
        component={ServerAccessSpecSection}
        defaultValue={newAccessSpec('node')}


Nit for the future, as it's not directly related to what's in this PR and changing this at this point could require a lot of not-strictly-necessary work: I noticed that those overloads on newAccessSpec don't seem to provide a lot of type safety. For example, this doesn't raise a type error about the implementation of newAccessSpec:

-export function newAccessSpec(kind: 'db'): DatabaseAccessSpec; +export function newAccessSpec(kind: 'db'): AppAccessSpec;

https://www.typescriptlang.org/docs/handbook/2/functions.html#writing-good-overloads

Always prefer parameters with union types instead of overloads when possible.

There's a couple of other solutions we could consider. If it's important for us to be able to call a function and get a specific variant of AccessSpec back, we could have a separate function for each variant (e.g. newDatabaseAccessSpec) that then get called from newAccessSpec. If that's not important, then we could have just a single export function newAccessSpec(kind: AccessSpecKind): AccessSpec definition without the overloads.

As a side note on overloads, TypeScript has a ton of features for expressing things that are legal in JavaScript, as TypeScript has to account for the fact that JavaScript is a dynamic language and that there are libraries which weren't written with JS in mind. But when authoring code, I found that usually I'm better off pretending that TypeScript is a static language with a type system akin to that found in Rust, F#, Haskell, or Elm. That is, I let go of the constructs like overloads which are supposed to help with typing JS code and make use mostly of product and sum types.

Thanks, I'll keep it in mind.

ravicious · 2024-12-06T13:50:08Z

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

+  test('validation', async () => {
+    const { user, validator } = setup();
+    await user.click(screen.getByRole('button', { name: 'Add New' }));
+    act(() => validator.validate());


Is there a way to trigger validation by interacting with the UI rather than depending on validator being available?

There would, of course, be a way if I wrapped the component with a button as a part of our test case. I just thought it's unnecessary, since the component under test in this case doesn't have that capability built-in, so it doesn't matter whether we add UI around it just for the testing purposes or not. Why do you think it would be beneficial?

We often depend on some internal deps within our tests which then leads to brittle or flaky tests, see #49870. But I suppose using validator here makes sense, as you're not testing a complete UI that's surfaced to the user, but rather a utility for building such UIs.

ravicious · 2024-12-06T14:09:16Z

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

+      title="Admin Rule"
+      tooltip="A rule that gives users administrative access to certain kinds of resources"


Feedback for the design choice, I don't think it's blocking for this PR: as a Teleport user, I'm kind of surprised this is called "admin rules". I don't think our documentation uses such concept and I wouldn't really consider them to be exclusively about administrative access: certain verbs such as read or list are needed just to use resources, not administer them.

@ravicious @zmb3 Any better ideas, then?

My 2c: here we use a term "access rules" (and "access controls") https://goteleport.com/docs/reference/access-controls/roles

@gzdunek Yes, but this is an umbrella term for all of the data in this form.

(Sorry, misread your comment. Yes, you're right.)

Since Zac and Grzegorz agree, I preemptively changed it to "access rules" to be consistent with existing terminology.

ravicious · 2024-12-06T14:13:54Z

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

+      tooltip="A rule that gives users administrative access to certain kinds of resources"
+      removable
+      isProcessing={isProcessing}
+      validation={validation}


It's a minor issue, but when I click "Create role" while there's an invalid admin rule in the form, any other new role that I add gets automatically validated and highlighted as invalid, since I'm yet to fill out any fields.

I don't think it's a blocker, personally I just don't like forms that tell me that a field is invalid before I had a chance to even touch it. ;)

validating-new-rules.mov

@ravicious Unfortunately, our validation library doesn't support a notion of a touched input. It only supports a global "validating" state. I may try to extend it in future, but to be honest, I think that we'd better off just migrating to a third-party library instead.

web/packages/teleport/src/Roles/RoleEditor/standardmodel.ts

ravicious · 2024-12-06T14:25:52Z

web/packages/teleport/src/Roles/RoleEditor/standardmodel.ts

-    requiresReset: !isEmpty(rest),
+    rules: rulesModel,
+    requiresReset:
+      kubernetesResourcesRequireReset || rulesRequireReset || !isEmpty(rest),


It might be a good idea to explain here why !isEmpty(rest) means that a reset is required and perhaps rename rest to conditionsUnsupportedByEditor or something.

Changed to unsupportedConditions. Also changed in other places where this pattern was used.

ravicious · 2024-12-06T14:29:57Z

api/types/constants.go

+	// The `Kind*` constants in this const block identify resource kinds used for
+	// storage an/or and access control. Please keep these in sync with the
+	// `ResourceKind` enum in
+	// `web/packages/teleport/src/services/resources/types.ts`.


I feel like this comment block is almost guaranteed to be ignored, but I don't really have a better idea how to solve this. If I was ever adding a new kind, I'd do "Go to definition" on an existing, similar kind and never see this comment.

I feel the same, but that's the least I could do. The ultimatlely correct solution for this one would probably be consuming a protobuf enum, but that would be a much bigger undertaking.

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

gzdunek · 2024-12-09T10:41:55Z

web/packages/teleport/src/Roles/RoleEditor/standardmodel.ts

+  resources: readonly ResourceKindOption[];
+  verbs: readonly VerbOption[];


Just curious: do we need to store Option<ResourceKind, string> in the model? Isn't the label needed only for visual purposes and could be created dynamically?

It would most probably work, as long as ReactSelect uses values, instead of option objects, for telling options apart. It just seemed arbitrary which way should be chosen: we either have to map names to options on the model conversion level, or on the data editing level, so I picked the first way, especially that from my quick look at the codebase, this seemed like the dominant approach. Let me know if you thing there's any significant drawback of one versus another.

I think the model should be kept as lean as possible; values that can be derived shouldn't be part of state https://react.dev/learn/choosing-the-state-structure#avoid-redundant-state.

Are there any significant drawbacks of the current approach? I’d say one potential downside is the inconvenience of accessing values like verb.value instead of just verb at higher levels in the component tree, that don't care about the labels.

But I think we can move forward with the current implementation. I’m aware that ReactSelect can be inconvenient to work with when using plain values, as it only accepts objects. Also I'm not sure if this is worth spending time on it at this point.

gzdunek · 2024-12-09T10:49:13Z

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

+      title="Admin Rule"
+      tooltip="A rule that gives users administrative access to certain kinds of resources"


My 2c: here we use a term "access rules" (and "access controls") https://goteleport.com/docs/reference/access-controls/roles

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

public-teleport-github-review-bot · 2024-12-09T18:08:46Z

@bl-nero See the table below for backport results.

Branch	Result
branch/v17	Failed

* Add admin rule tab to the role editor Also tightens some constraints about standard role editor conformance. * Add a way to delete an admin rule * Review

bl-nero added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Dec 4, 2024

github-actions bot requested review from gzdunek and ravicious December 4, 2024 14:31

github-actions bot added size/lg ui labels Dec 4, 2024

bl-nero force-pushed the bl-nero/role-editor-8 branch from 5e10c55 to 17771a2 Compare December 4, 2024 14:59

bl-nero mentioned this pull request Dec 4, 2024

Add the options panel to role editor #49801

Merged

Base automatically changed from bl-nero/role-editor-7 to master December 5, 2024 13:22

bl-nero added 2 commits December 6, 2024 13:22

Add admin rule tab to the role editor

ac995c9

Also tightens some constraints about standard role editor conformance.

Add a way to delete an admin rule

0bacc2f

bl-nero force-pushed the bl-nero/role-editor-8 branch from 4d4eaa6 to 0bacc2f Compare December 6, 2024 12:22

ravicious approved these changes Dec 6, 2024

View reviewed changes

gzdunek reviewed Dec 9, 2024

View reviewed changes

Review

ea7ee56

bl-nero requested a review from gzdunek December 9, 2024 13:16

gzdunek approved these changes Dec 9, 2024

View reviewed changes

bl-nero added this pull request to the merge queue Dec 9, 2024

Merged via the queue into master with commit 02141ca Dec 9, 2024
45 checks passed

bl-nero deleted the bl-nero/role-editor-8 branch December 9, 2024 18:07

bl-nero added a commit that referenced this pull request Dec 9, 2024

Add admin rule tab to the role editor (#49764)

63c6675

* Add admin rule tab to the role editor Also tightens some constraints about standard role editor conformance. * Add a way to delete an admin rule * Review

bl-nero mentioned this pull request Dec 9, 2024

[v17] Add admin rule tab to the role editor #49953

Merged

bl-nero added a commit that referenced this pull request Dec 9, 2024

Add admin rule tab to the role editor (#49764)

442bda2

* Add admin rule tab to the role editor Also tightens some constraints about standard role editor conformance. * Add a way to delete an admin rule * Review

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add admin rule tab to the role editor #49764

Add admin rule tab to the role editor #49764

bl-nero commented Dec 4, 2024 •

edited

Loading

bl-nero commented Dec 6, 2024

ravicious left a comment

ravicious Dec 6, 2024

bl-nero Dec 9, 2024

ravicious Dec 6, 2024

bl-nero Dec 9, 2024

ravicious Dec 9, 2024

ravicious Dec 6, 2024

bl-nero Dec 9, 2024

gzdunek Dec 9, 2024

bl-nero Dec 9, 2024

bl-nero Dec 9, 2024

bl-nero Dec 9, 2024

ravicious Dec 6, 2024

bl-nero Dec 9, 2024

ravicious Dec 6, 2024

bl-nero Dec 9, 2024

ravicious Dec 6, 2024

bl-nero Dec 9, 2024

gzdunek Dec 9, 2024

bl-nero Dec 9, 2024

gzdunek Dec 9, 2024

gzdunek Dec 9, 2024

public-teleport-github-review-bot bot commented Dec 9, 2024

		title="Admin Rule"
		tooltip="A rule that gives users administrative access to certain kinds of resources"

		resources: readonly ResourceKindOption[];
		verbs: readonly VerbOption[];

Add admin rule tab to the role editor #49764

Add admin rule tab to the role editor #49764

Conversation

bl-nero commented Dec 4, 2024 • edited Loading

bl-nero commented Dec 6, 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

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

public-teleport-github-review-bot bot commented Dec 9, 2024

bl-nero commented Dec 4, 2024 •

edited

Loading