[Connect] Add Document Access Requests by avatus · Pull Request #1203 · gravitational/webapps

avatus · 2022-09-23T20:41:01Z

teleport PR
Original description can be found here.

My thought process to give you some idea of the decisions I made

Most of the work was in clustersService. It seemed like any actual 'api calls' were happening there with the rest of the services being tied to frontend state.

accessRequestsService was based on how documentsService was created since it is workspace specific.

although you can only create a single access request at a time, we wanted to 'state' to persist across tabs to allow for multiple searches and to support the 'Proceed to Checkout' button across the entire workspace. It added quite a bit of overhead but I think it works nicely.

I tried to reuse components from webapps.e Workflow however some would break due to a Link being present. I would generally recreate the 'page'-ish level and import components and then add my own logic with a useComponentName. Similar pattern to what I saw on web but without the 'Container' (basically created my own "Container").

One thing I'm not quite happy with is the RequestCheckout decision. Its a large component and I didnt want to copy all of it to avoid using a Link in the successful checkout flow so I decided to add an prop to pass that component in. It's a bit ugly solution but allows us to reuse 90% of the code and just take out the Buttons with Links. Would love feedback and maybe its a solution if we're happy.

adding more tests but wanted to get the review started, those will come shortly.

This PR has all the proto file changes so the files changed number is a bit bloated but still quite large.

and a neat little video to show off the basic flow/features

Untitled.mov

Fixes gravitational/teleport#15169

avatus · 2022-09-23T23:44:07Z

Sorry for the messy commit history. Had some weird submodule issues and merge conflicts so ended up nuking my local and bringing changes over.
I realize now I didn't properly bring over my leaf cluster logic so I will get that in on Monday

gzdunek · 2022-09-27T08:13:57Z

  });
 }
+
+export function makeResourceID({


I'd inline this function :)

gzdunek · 2022-09-27T08:15:54Z

+export type ReviewAccessRequestParams = {
+  state: string;
+  reason: string;
+  roles: [string];


I think you meant string[]

gzdunek · 2022-09-27T08:34:00Z

+        width: '100%',
+      })}
+    >
+      <DialogHeader justifyContent="space-between" mb={0}>


We should have a simpler DialogConfirmation component... it is a lot of code to just show confirmation.
But this is out of scope for this PR.

gzdunek · 2022-09-27T08:40:14Z

+    const rootClusterUri = workspacesService.getRootClusterUri();
+    const accessRequestsService =
+      workspacesService.getWorkspaceAccessRequestsService(rootClusterUri);


Can be replaced with workspacesService.getActiveWorkspaceAccessRequestsService() (also in lines 42-44)

gzdunek · 2022-09-27T09:25:06Z

+      draftState: (draft: {
+        isAccessRequestsBarCollapsed: boolean;
+        pendingAccessRequest: PendingAccessRequest;
+        assumed: Record<string, AccessRequest>;


Hmm is there any benefit of saving access requests in an object instead of an array? Because of that we have to use Object.keys() in a lot of places.

I thought about this same thing tbh. I believe its because when adding a resource, we must check the type + name since the name isn't guaranteed to be unique (imagine a node named teleport and a db named teleport), however, I am storing by the ID anyway and I'm not sure it has the same issue? Sort of a relic of the past but, either either Object.keys in places, or array.filter/map in others to get counts+types in other places.

So the key has structure type-resourceName?

gzdunek · 2022-09-27T10:19:31Z

+const changeSelectedClusterWarning =
+  'Resources from different clusters cannot be combined in an access request. Current items selected will be cleared. Are you sure you want to continue?';
+
+export default function ChangeResourceDialog({


This component is used with a name <ConfirmChangeDialog> and sits inside ConfirmClusterChangeDialog.tsx, that's a bit confusing :p
Using one name will improve readability.

gzdunek · 2022-09-27T10:23:34Z

+      Object.keys(pendingAccessRequest.node).length +
+      Object.keys(pendingAccessRequest.db).length +
+      Object.keys(pendingAccessRequest.app).length +
+      Object.keys(pendingAccessRequest.kube_cluster).length +
+      Object.keys(pendingAccessRequest.windows_desktop).length;


The same code we have in useAccessRequestsButton. I'd see it in the accessRequestsService.

gzdunek · 2022-09-27T10:28:03Z

+
+import { NavigationItem } from './NavigationItem';
+
+export function NavigationMenuContainer() {


Is this component needed? I'd use early return in NavigationMenu if activeRootCluster is empty.

gzdunek · 2022-09-27T10:31:03Z

+    if (!docService) {
+      return;
+    }
+    const doc = docService.createAccessRequestDocument({ clusterUri, state });
+    docService.add(doc);
+    docService.open(doc.uri);


This logic should live in navigationItems:

{ title: 'New Access Request', Icon: <Add fontSize={2} /> onNavigate: () => {...} },

NavigationItem doesn't need to know about docService or anything like that.

I agree, because we may need to do something special per item eventually. How is navigationItems supposed to be aware of the doc service? I'm a bit lost there, can you point to an existing pattern similar to this?

I'm thinking about something like this:

function getNavigationItems(documentsService: DocumentsService): { title: string; Icon: JSX.Element; onNavigate(): void; }[] { return [ { title: 'New Access Request', Icon: <Add fontSize={2} />, onNavigate: () => documentsService.open(....), }, { title: 'Review Access Requests', Icon: <OpenBox fontSize={2} />, onNavigate: () => documentsService.open(....) }, ]};

and then:

<Flex flexDirection="column"> {getNavigationItems(documentsService).map((item, index) => (

This is awesome thank you. yes, absolutely the best path to go with.

This came out great! Wonderful suggestion

gzdunek · 2022-09-27T10:51:59Z

+};
+
+export type ReviewAccessRequestParams = {
+  state: string;


What are the possible states?

ravicious · 2022-09-28T12:43:50Z

+  {
+    title: 'New Access Request',
+    Icon: <Add fontSize={2} />,
+    state: 'creating',
+  },
+  {
+    title: 'Review Access Requests',
+    Icon: <OpenBox fontSize={2} />,
+    state: 'reviewing',
+  },


Re: feature detection in Connect, do we plan to do that before the release of access requests?

I believe that is to be a fast-follow

ravicious · 2022-09-29T15:16:02Z

    }
  }

+  async fetchServers(params: ServerSideParams) {


The callsites that call these new methods from clustersService need to wrap the calls to those methods in retryWithRelogin.

When I came back to the running dev version of Electron, I saw this:

I think this couldn't happen by itself because AFAIK Connect just doesn't decide to make a request by itself, I must've went from the view request "page" to the list of all requests.

In any case, the general rule is that the backend handlers that use RetryWithRelogin need to be wrapped with retryWithRelogin on the frontend for the whole system to take effect.

Look up how other places in the frontend app do this. retryWithRelogin needs to tightly wrap the call to ClustersService.

You might have a problem with passing a meaningful value as the resourceUri argument to retryWithRelogin. In case of, for example, listing all resources, I don't think you'll have a meaningful resource URI to pass there. In that case, that argument is used only to get the root cluster URI, so you can just pass the cluster URI (if it's a leaf cluster URI, retryWithRelogin will extract the root cluster URI from it automatically).

Ok, i wrapped the requests. Funnily enough, that fetchServers seems to have been vestigial from before the `getAllServers' and 'getServers' distinction as it isn't used anywhere in the codebase atm.

gzdunek · 2022-10-11T13:43:55Z

I think we are ready to the merge. The only thing that still puzzles me is the state we store on the disk (surprise 🙂). I didn’t realize that tshd returns activeRequestsList, with that knowledge it seems to me that we shouldn’t store anything.
The main problem with the current solution is that accessRequestsService mirrors tsh behavior (for example when we click Switch back, we need to send a request and then remove the role from the accessRequestsService state manually). It increases complexity, every operation needs to be done twice so the state is in sync with tsh.
Instead I would suggest:

Remove assumed field from accessRequestsService.
Enhance syncClusterInfo method to fetch access requests. This doesn’t have to significantly degrade performance, we can fetch accessRequests only when we know that there is something to fetch (also in the future we can fetch them by ID).

const assumedAccessRequests = (
      cluster.loggedInUser.activeRequestsList.length
        ? await this.client.getAccessRequests(clusterUri)
        : []

Keep the data from above in a cluster state. I wasn't sure about this at first, but what is the difference between the assumed roles and activeRequestsList that we have there currently? By doing this we won't have to worry about syncing them. Also we don’t need to keep the whole response, it can be only id, roles and expires fields.

The only downside of this is that AssumedRolesBar won’t show immediately, but with a slight delay (depending on how quick we fetch the data). IMO we can live with this, the same as we live with loading the resource tables or leaf clusters. Duplicating the state, just to overcome this seems unnecessary and we should solve it in another way (like block the workspace until the data is loaded; btw it would solve another problem that we have with async data loading).

My main motivation is to store as little state as possible in app_state.json as its structure can’t be easily changed between versions. Let me know what you think about it, I don’t want to force you to do it (and maybe I'm wrong in some parts) 😄
I prepared the changes locally, but we can merge the PR now and maybe I will create another PR?

* init webapps.e ref * update web.e * added imported components * udpated to correct imported types * webapps.e update * readme update and lint fix * removed extra console logs * sub head fix for webe * reattached webape head * fixed tests * added access request change confirm dialog * null check * webappe * count to accessRequestSerivce, import/export names, small fixes * fixed imports and createrequest params * updated imports from removed containers in teleterm-e * optional params for reuse outside of ClusterLogout * webapps.e ref update * update webapps.e ref * webapps.e ref update * webapps.e ref update and navigation item update * webapps.e update * align sync button center to match access request size * added accessRequestsSerice tests * updating webapps.e * created accessRequests object on workspace type and cleanup * added requestId to documentaccessrequest type and webapps.e update * protobuf updates changing created/expires from string to timestamppb * webappse update and review feedback * updated protos with comments * changing delete/review access request responses to void * updated getaccessrequests to not include ID * protobuf updates * webappse update * update to resource tables styles * update to standard access_request_id field in RPCs * webappse upddate * updated tests and added types * updated webappse * clusterService method for getrequestableroles, retryWithRelogin for fetch, webappse update * webappse update * story fixes * added kubes support to tsh * proto files * update webappse * Provide `WorkspaceContext` for documents in each workspace * updating webappse * webappse update * accessrequestservice fix for resoruces * updated webapps.e after master merge * removed extra data from persisted state for assumed access requests * update webappse * protobuf files * lint fixes * adding getWorkspaces stub to fix tests * webappse update * webappse snapshots update * protobuf updates Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com>

* [Connect] Add Document Access Requests (#1203) * init webapps.e ref * update web.e * added imported components * udpated to correct imported types * webapps.e update * readme update and lint fix * removed extra console logs * sub head fix for webe * reattached webape head * fixed tests * added access request change confirm dialog * null check * webappe * count to accessRequestSerivce, import/export names, small fixes * fixed imports and createrequest params * updated imports from removed containers in teleterm-e * optional params for reuse outside of ClusterLogout * webapps.e ref update * update webapps.e ref * webapps.e ref update * webapps.e ref update and navigation item update * webapps.e update * align sync button center to match access request size * added accessRequestsSerice tests * updating webapps.e * created accessRequests object on workspace type and cleanup * added requestId to documentaccessrequest type and webapps.e update * protobuf updates changing created/expires from string to timestamppb * webappse update and review feedback * updated protos with comments * changing delete/review access request responses to void * updated getaccessrequests to not include ID * protobuf updates * webappse update * update to resource tables styles * update to standard access_request_id field in RPCs * webappse upddate * updated tests and added types * updated webappse * clusterService method for getrequestableroles, retryWithRelogin for fetch, webappse update * webappse update * story fixes * added kubes support to tsh * proto files * update webappse * Provide `WorkspaceContext` for documents in each workspace * updating webappse * webappse update * accessrequestservice fix for resoruces * updated webapps.e after master merge * removed extra data from persisted state for assumed access requests * update webappse * protobuf files * lint fixes * adding getWorkspaces stub to fix tests * webappse update * webappse snapshots update * protobuf updates Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com> * webapps.e update Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com>

This method wasn't used anywhere since #1203 got merged.

* Remove ClustersService methods related to apps I had problems with the new types and apps because I didn't create a separate type for app URI. So I decided to remove it all because it isn't used anyway. * Remove WorkspacesService.getWorkspacesDocumentsServices This method wasn't used anywhere since #1203 got merged.

avatus added 10 commits September 23, 2022 00:26

init webapps.e ref

081f731

update web.e

03b628b

added imported components

41bba3c

udpated to correct imported types

ab3fa41

webapps.e update

dd37615

readme update and lint fix

b8c6611

removed extra console logs

bbfbe20

sub head fix for webe

d1bfb7e

reattached webape head

77d6521

fixed tests

a0b96e1

avatus requested review from gzdunek and ravicious September 23, 2022 20:41

avatus mentioned this pull request Sep 23, 2022

[connect] access requests for Teleport Connect gravitational/teleport#16694

Merged

avatus added 3 commits September 24, 2022 13:50

added access request change confirm dialog

6791552

null check

9f536c6

webappe

f63fc5e

gzdunek reviewed Sep 27, 2022

View reviewed changes

count to accessRequestSerivce, import/export names, small fixes

5081a11

ravicious reviewed Sep 28, 2022

View reviewed changes

Comment thread packages/teleterm/src/ui/services/clusters/clustersService.ts

avatus added 7 commits September 28, 2022 08:31

fixed imports and createrequest params

9e2ff12

updated imports from removed containers in teleterm-e

21e6652

optional params for reuse outside of ClusterLogout

1e72490

webapps.e ref update

dc955e6

update webapps.e ref

d3c5c22

webapps.e ref update

89acc2b

webapps.e ref update and navigation item update

b05f50f

ravicious reviewed Sep 29, 2022

View reviewed changes

webapps.e update

4719078

gzdunek and others added 12 commits October 10, 2022 17:17

Provide WorkspaceContext for documents in each workspace

392327e

updating webappse

f1c7c42

webappse update

72a9c9a

accessrequestservice fix for resoruces

e0e62d3

merged master

db437fd

updated webapps.e after master merge

c1b17bb

removed extra data from persisted state for assumed access requests

acb697a

update webappse

f80824e

protobuf files

3566adf

merged master

13ff1a0

lint fixes

6ee8462

adding getWorkspaces stub to fix tests

d33472f

gzdunek approved these changes Oct 11, 2022

View reviewed changes

avatus added 5 commits October 11, 2022 11:31

webappse update

ca927a6

Merge branch 'master' into michaelmyers/connect/access_requests

60cc37b

webappse snapshots update

5dd9930

Merge branch 'master' into michaelmyers/connect/access_requests

fed8f70

protobuf updates

89629ff

avatus merged commit 2309cc0 into master Oct 11, 2022

This was referenced Oct 12, 2022

[v11] [Connect] Add Document Access Requests (#1203) #1252

Merged

Add enterprise module support to Connect builds gravitational/teleport#17315

Merged

ravicious mentioned this pull request Oct 24, 2022

Connect: Remove proprietary imports from OSS build gravitational/teleport#17706

Closed

3 tasks

ravicious added a commit that referenced this pull request Dec 2, 2022

Remove WorkspacesService.getWorkspacesDocumentsServices

6710755

This method wasn't used anywhere since #1203 got merged.

ravicious added a commit that referenced this pull request Dec 14, 2022

Remove WorkspacesService.getWorkspacesDocumentsServices

338f3dc

This method wasn't used anywhere since #1203 got merged.


		import { NavigationItem } from './NavigationItem';

		export function NavigationMenuContainer() {

Conversation

avatus commented Sep 23, 2022 • edited by zmb3 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avatus commented Sep 23, 2022

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzdunek commented Oct 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

avatus commented Sep 23, 2022 •

edited by zmb3

Loading