Web: Redo locking feature by kimlisa · Pull Request #25305 · gravitational/teleport

kimlisa · 2023-04-28T02:41:42Z

part of #25168
enterprise PR: https://github.com/gravitational/teleport.e/pull/1237

recommend reviewing by commit (i plan to delete the old locks code after review)

it was hard to work with one size fit all data structure for the different types of resources especially when there are three types of pagination being used, so decided to re-do and have everything have its own table

design and ux is copied from access requests

in addition

i didn't agree with supplying a manual add input box for every resource (especially when you can now use the search bar) so it only shows up for logins
i copied the table display/formatting from other places, figured we can decide later what to customize if we want to
add access checking, will not render lock tab if user doesn't have lock.list perm and does not render new lock screen if they don't have lock.create and lock.update perm
add confirmation dialogue when user is deleting locks
** since we create a lock per resource select (this was how it worked before and figured it was okay), there's no guarantee that all will succeed in a row, so we allow user to re-attempt the failed ones
supports all pagination types until we come up with another one ;;

Improvements for later:

writing test and stories
deduplicate code (shares similarities with access request)
some cells are not aligned perfectly to center (you can tell on smaller screen with access requests)
** it looks like you can make batch lock request (as long the target keys are unique), like for example, locking one user and one device can be batched into one lock request. Locking two user has to be separate requests. Not sure if we wanted to support batching for less trip to back, but left it alone for another time.

Demo (using a smaller page size to demonstrate paginating):

Screen.Recording.2023-04-27.at.7.04.24.PM.mov

public-teleport-github-review-bot · 2023-04-28T02:42:09Z

@kimlisa - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

kimlisa · 2023-04-28T02:45:17Z

@rudream @avatus i would really appreciate it if ya'll can help me try to break this feature 🙏

zmb3 · 2023-04-28T17:48:56Z

add access checking, will not render lock tab if user doesn't have lock.list perm and does not render new lock screen if they don't have lock.create and lock.update perm

We want to get away from this pattern of completely hiding parts of Teleport, because it prevents people from learning that these features exist.

For now it's fine, but we should start to prefer to always show the tabs and just indicate that the user doesn't have access.

avatus

Looks and feels really good to use now. I left some UI comments but more of a discussion rather than suggested changes. Feel free to push back/ignore them all.

avatus · 2023-04-28T17:20:59Z

I don't think request is good language here but I'm not 100% sold on my suggestion. Best I could come up with.

Suggested change

+ Add to request

+ Add Target

If we use the language from the above comments like "Lock Request" then we can just keep this as is.

avatus · 2023-04-28T17:22:22Z

Proceed to Lock? Proceed to Lock Request? Feel free to skip

avatus · 2023-04-28T17:22:46Z

Suggested change

<Text bold>Resources Added ({numAddedResources})</Text>

<Text bold>Targets Added ({numAddedResources})</Text>

avatus · 2023-04-28T17:23:36Z

Suggested change

Submit Request

Create Locks

I think it makes sense here to say Create Locks even if used in the button to open this dialog

avatus · 2023-04-28T17:26:51Z

Suggested change

{locks.length} {pluralize(locks.length, 'Resource')} Selected

{locks.length} {pluralize(locks.length, 'Target')} Added

Again, not 100% sold here as you are selecting resources TO lock so you can skip this one.

avatus · 2023-04-28T17:27:55Z

Back to Locks?

avatus · 2023-04-28T17:30:01Z

Suggested change

Listing logins are not supported. Use the input box below to manually

define a login to lock. <br />

Double check the spelling before adding.

Listing logins is not supported. Use the input box below to manually

define a login to lock. <br />

Double check the spelling before adding.

avatus · 2023-04-28T17:49:55Z

Suggested change

<Box>New Request</Box>

<Box>New Lock Request</Box>

zmb3 · 2023-04-28T17:51:29Z

Shouldn't this just call MakeLock in a loop rather than repeating the same code?

zmb3 · 2023-04-28T17:51:56Z

Suggested change

Lock access `json:"lock"`

Locks access `json:"lock"`

Most of these fields are plural.

zmb3 · 2023-04-28T17:55:21Z

Missing license on a lot of these new files.

zmb3 · 2023-04-28T18:00:56Z

Suggested change

// to define the type of lock to be locked.

// to define the type of resource to be locked.

zmb3 · 2023-04-28T18:01:44Z

This bit us in the past, right? It's not always a name.

Also, I'm fine with this as-is for simplicity, but note that the data model for the frontend doesn't match the backend now. On the backend, a lock target can have multiple different resources (so long as they are different types).

This bit us in the past, right? It's not always a name.

i think this only applies to mapping backend with frontend fields, which isn't the case here. this type is only for frontend use. in the api response we get a mapping of targets, it wasn't very useful in its map form so I converted it into a list of LockTarget and added it as an extra field to the frontend model. i did it in this layer so we don't have to always process it when we use it in components.

we have a lot of frontend models that don't match the backend b/c we customize it further to suit our needs. i thought it was okay to do but maybe it shouldn't be done in the service layer. @ryanclark thoughts?

avatus · 2023-04-28T19:21:33Z

Suggested change

return json.map(lock => makeLock(lock));

return json.map(makeLock);

kimlisa · 2023-04-28T20:12:49Z

add access checking, will not render lock tab if user doesn't have lock.list perm and does not render new lock screen if they don't have lock.create and lock.update perm

We want to get away from this pattern of completely hiding parts of Teleport, because it prevents people from learning that these features exist.

For now it's fine, but we should start to prefer to always show the tabs and just indicate that the user doesn't have access.

is this something recently decided? i'll double check with xin in our upcoming monday meeting, but i thought we wanted it to be possible to turn off features? There are cases where we don't want to hide things, and thought those only applied when the user at least met the minimum criteria eg. most features look for at least a list access

(i'll leave it as is for now though)

avatus

Tested about everything I could think of.
We can update some of the test plan points later but I plan on making a few large changes to that soon anyway.

I'd love to have @rudream comb over this as well

avatus · 2023-04-28T21:31:44Z

Can we update locks?

you can

teleport/lib/auth/auth_with_roles.go

Line 4876 in 860d910

// UpsertLock upserts a lock.

(but no exposed api for the web to use atm), it's an upsert action that requires user to have both the create and update rule

zmb3

I was not able to break it. Thanks so much for this.

Also reverts body `overflow: hidden` Fixes #24885

public-teleport-github-review-bot · 2023-05-01T19:04:34Z

@kimlisa See the table below for backport results.

Branch	Result
branch/v13	Failed

Also reverts body `overflow: hidden` Fixes #24885

github-actions Bot requested review from ibeckermayer and ravicious April 28, 2023 02:42

github-actions Bot added size/xl ui labels Apr 28, 2023

kimlisa requested review from avatus and rudream and removed request for ibeckermayer and ravicious April 28, 2023 02:44

kimlisa added backport/branch/v13 labels Apr 28, 2023

avatus reviewed Apr 28, 2023

View reviewed changes

zmb3 reviewed Apr 28, 2023

View reviewed changes

avatus reviewed Apr 28, 2023

View reviewed changes

kimlisa requested review from avatus and zmb3 April 28, 2023 20:14

kimlisa force-pushed the lisa/redo-lock-feature branch 2 times, most recently from 5105601 to 43eade2 Compare April 28, 2023 20:21

avatus approved these changes Apr 28, 2023

View reviewed changes

zmb3 approved these changes Apr 28, 2023

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from rudream April 28, 2023 22:37

Redo web lock feature

adcbf98

Also reverts body `overflow: hidden` Fixes #24885

kimlisa force-pushed the lisa/redo-lock-feature branch from 2981d85 to adcbf98 Compare May 1, 2023 18:29

Merge branch 'master' into lisa/redo-lock-feature

2d78849

kimlisa enabled auto-merge May 1, 2023 18:32

kimlisa added this pull request to the merge queue May 1, 2023

Merged via the queue into master with commit a0773f9 May 1, 2023

kimlisa deleted the lisa/redo-lock-feature branch May 1, 2023 19:03

kimlisa added a commit that referenced this pull request May 1, 2023

Redo web lock feature (#25305)

a6666db

Also reverts body `overflow: hidden` Fixes #24885

kimlisa mentioned this pull request May 1, 2023

[v13] Redo web lock feature (#25305) #25439

Merged

kimlisa added a commit that referenced this pull request May 1, 2023

Redo web lock feature (#25305) (#25439)

1cc4c43

Also reverts body `overflow: hidden` Fixes #24885

r0mant mentioned this pull request Jul 14, 2023

Release 13.2.2 #29158

Closed

	<Text bold>Resources Added ({numAddedResources})</Text>
	<Text bold>Targets Added ({numAddedResources})</Text>

	{locks.length} {pluralize(locks.length, 'Resource')} Selected
	{locks.length} {pluralize(locks.length, 'Target')} Added

-        Listing logins are not supported. Use the input box below to manually
-        define a login to lock. <br />
-        Double check the spelling before adding.
+        Listing logins is not supported. Use the input box below to manually
+        define a login to lock. <br />
+        Double check the spelling before adding.

	// to define the type of lock to be locked.
	// to define the type of resource to be locked.

	return json.map(lock => makeLock(lock));
	return json.map(makeLock);

Conversation

kimlisa commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Demo (using a smaller page size to demonstrate paginating):

Uh oh!

public-teleport-github-review-bot Bot commented Apr 28, 2023

Uh oh!

kimlisa commented Apr 28, 2023

Uh oh!

zmb3 commented Apr 28, 2023

Uh oh!

avatus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimlisa commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avatus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

public-teleport-github-review-bot Bot commented May 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kimlisa commented Apr 28, 2023 •

edited

Loading

kimlisa commented Apr 28, 2023 •

edited

Loading