From c509de08004bf6042ef5cb0bf0c7abd7484e118f Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Thu, 6 Jun 2024 11:26:18 +0200 Subject: [PATCH 1/4] Separate assuming and dropping roles, send a usage event only when assuming --- .../useAssumedRolesBar.ts | 6 +--- .../DocumentAccessRequests/useAssumeAccess.ts | 2 +- .../ui/services/clusters/clustersService.ts | 30 ++++++++++++------- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/web/packages/teleterm/src/ui/AccessRequestCheckout/useAssumedRolesBar.ts b/web/packages/teleterm/src/ui/AccessRequestCheckout/useAssumedRolesBar.ts index dd466c411eaba..3bce0793dc8ea 100644 --- a/web/packages/teleterm/src/ui/AccessRequestCheckout/useAssumedRolesBar.ts +++ b/web/packages/teleterm/src/ui/AccessRequestCheckout/useAssumedRolesBar.ts @@ -50,11 +50,7 @@ export function useAssumedRolesBar(assumedRequest: AssumedRequest) { return retryWithRelogin( ctx, rootClusterUri, - () => - // only passing the 'unassumed' role id as the backend will - // persist any other access requests currently available that - // are not present in the dropIds array - ctx.clustersService.assumeRole(rootClusterUri, [], [assumedRequest.id]) + () => ctx.clustersService.dropRoles(rootClusterUri, [assumedRequest.id]) // TODO(gzdunek): We should refresh the resources, // the same as after assuming a role in `useAssumeAccess`. // Unfortunately, we can't do this because we don't have access to `ResourcesContext`. diff --git a/web/packages/teleterm/src/ui/DocumentAccessRequests/useAssumeAccess.ts b/web/packages/teleterm/src/ui/DocumentAccessRequests/useAssumeAccess.ts index 25f84499fbbc1..741d5337e3405 100644 --- a/web/packages/teleterm/src/ui/DocumentAccessRequests/useAssumeAccess.ts +++ b/web/packages/teleterm/src/ui/DocumentAccessRequests/useAssumeAccess.ts @@ -34,7 +34,7 @@ export function useAssumeAccess() { const [assumeRoleAttempt, runAssumeRole] = useAsync((requestId: string) => retryWithRelogin(ctx, clusterUri, async () => { - await ctx.clustersService.assumeRole(rootClusterUri, [requestId], []); + await ctx.clustersService.assumeRoles(rootClusterUri, [requestId]); // refresh the current resource tabs requestResourcesRefresh(); }) diff --git a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts index 3dcc94a33aab4..6c4683d1561f2 100644 --- a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts +++ b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts @@ -420,23 +420,31 @@ export class ClustersService extends ImmutableStore }); } - async assumeRole( + /** Assumes roles for the given requests. */ + async assumeRoles( rootClusterUri: uri.RootClusterUri, - requestIds: string[], - dropIds: string[] - ) { - const cluster = this.state.clusters.get(rootClusterUri); - // TODO(ravicious): Remove check for cluster.connected. See the comment in getRequestableRoles. - if (!cluster.connected) { - return; - } + requestIds: string[] + ): Promise { await this.client.assumeRole({ rootClusterUri, accessRequestIds: requestIds, - dropRequestIds: dropIds, + dropRequestIds: [], }); this.usageService.captureAccessRequestAssumeRole(rootClusterUri); - return this.syncRootCluster(rootClusterUri); + await this.syncRootCluster(rootClusterUri); + } + + /** Drops roles for the given requests. */ + async dropRoles( + rootClusterUri: uri.RootClusterUri, + requestIds: string[] + ): Promise { + await this.client.assumeRole({ + rootClusterUri, + accessRequestIds: [], + dropRequestIds: requestIds, + }); + await this.syncRootCluster(rootClusterUri); } async getAccessRequest( From 2608cd353ba859cfb50cfe7615c8fde564bce8e8 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Thu, 6 Jun 2024 11:38:10 +0200 Subject: [PATCH 2/4] Remove `cluster.connected` checks Each call is wrapped in `retryWithRelogin` in UI and `clusters.AddMetadataToRetryableError` in tshd. If the cluster is not connected, we should ask the user to log in, rather than fail silently. --- .../ui/services/clusters/clustersService.ts | 47 +------------------ 1 file changed, 1 insertion(+), 46 deletions(-) diff --git a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts index 6c4683d1561f2..90a57ab61bb4d 100644 --- a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts +++ b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts @@ -366,13 +366,6 @@ export class ClustersService extends ImmutableStore } async getRequestableRoles(params: GetRequestableRolesRequest) { - const cluster = this.state.clusters.get(params.clusterUri); - // TODO(ravicious): Remove check for cluster.connected. This check should be done earlier in the - // UI rather than be repeated in each ClustersService method. - if (!cluster.connected) { - return; - } - const { response } = await this.client.getRequestableRoles(params); return response; @@ -380,25 +373,10 @@ export class ClustersService extends ImmutableStore getAssumedRequests(rootClusterUri: uri.RootClusterUri) { const cluster = this.state.clusters.get(rootClusterUri); - // TODO(ravicious): Remove check for cluster.connected. See the comment in getRequestableRoles. - if (!cluster?.connected) { - return {}; - } - - return cluster.loggedInUser?.assumedRequests || {}; - } - - getAssumedRequest(rootClusterUri: uri.RootClusterUri, requestId: string) { - return this.getAssumedRequests(rootClusterUri)[requestId]; + return cluster?.loggedInUser?.assumedRequests || {}; } async getAccessRequests(rootClusterUri: uri.RootClusterUri) { - const cluster = this.state.clusters.get(rootClusterUri); - // TODO(ravicious): Remove check for cluster.connected. See the comment in getRequestableRoles. - if (!cluster.connected) { - return; - } - const { response } = await this.client.getAccessRequests({ clusterUri: rootClusterUri, }); @@ -409,11 +387,6 @@ export class ClustersService extends ImmutableStore rootClusterUri: uri.RootClusterUri, requestId: string ) { - const cluster = this.state.clusters.get(rootClusterUri); - // TODO(ravicious): Remove check for cluster.connected. See the comment in getRequestableRoles. - if (!cluster.connected) { - return; - } await this.client.deleteAccessRequest({ rootClusterUri, accessRequestId: requestId, @@ -451,12 +424,6 @@ export class ClustersService extends ImmutableStore rootClusterUri: uri.RootClusterUri, requestId: string ) { - const cluster = this.state.clusters.get(rootClusterUri); - // TODO(ravicious): Remove check for cluster.connected. See the comment in getRequestableRoles. - if (!cluster.connected) { - return; - } - const { response } = await this.client.getAccessRequest({ clusterUri: rootClusterUri, accessRequestId: requestId, @@ -466,12 +433,6 @@ export class ClustersService extends ImmutableStore } async reviewAccessRequest(params: ReviewAccessRequestRequest) { - const cluster = this.state.clusters.get(params.rootClusterUri); - // TODO(ravicious): Remove check for cluster.connected. See the comment in getRequestableRoles. - if (!cluster.connected) { - return; - } - const { response } = await this.client.reviewAccessRequest(params); this.usageService.captureAccessRequestReview(params.rootClusterUri); return response.request; @@ -484,12 +445,6 @@ export class ClustersService extends ImmutableStore } async createAccessRequest(params: CreateAccessRequestRequest) { - const cluster = this.state.clusters.get(params.rootClusterUri); - // TODO(ravicious): Remove check for cluster.connected. See the comment in getRequestableRoles. - if (!cluster.connected) { - return; - } - const response = await this.client.createAccessRequest(params); if (!params.dryRun) { this.usageService.captureAccessRequestCreate( From cd4acf29ef83ceb9f7b473be1a786c4ca2ef17a4 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Thu, 6 Jun 2024 11:51:15 +0200 Subject: [PATCH 3/4] Remove methods from `ClustersService` than only forward the call `TshdClient` --- .../useAccessRequestCheckout.ts | 9 +++---- .../useReviewAccessRequest.ts | 9 ++++--- .../useAccessRequests.tsx | 7 +++--- .../ui/services/clusters/clustersService.ts | 24 ------------------- 4 files changed, 15 insertions(+), 34 deletions(-) diff --git a/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts b/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts index dd5c082c7d0f1..a2477c71b07d1 100644 --- a/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts +++ b/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts @@ -114,8 +114,8 @@ export default function useAccessRequestCheckout() { const data = getPendingAccessRequestsPerResource(pendingAccessRequest); runFetchResourceRoles(() => - retryWithRelogin(ctx, clusterUri, () => - ctx.clustersService.getRequestableRoles({ + retryWithRelogin(ctx, clusterUri, async () => { + const { response } = await ctx.tshd.getRequestableRoles({ clusterUri: rootClusterUri, resourceIds: data .filter(d => d.kind !== 'role') @@ -128,8 +128,9 @@ export default function useAccessRequestCheckout() { clusterName: d.clusterName, subResourceName: '', })), - }) - ).then(response => { + }); + return response; + }).then(response => { setResourceRequestRoles(response.applicableRoles); setSelectedResourceRequestRoles(response.applicableRoles); }) diff --git a/web/packages/teleterm/src/ui/DocumentAccessRequests/ReviewAccessRequest/useReviewAccessRequest.ts b/web/packages/teleterm/src/ui/DocumentAccessRequests/ReviewAccessRequest/useReviewAccessRequest.ts index 48f270ed787c4..65b75086c0f18 100644 --- a/web/packages/teleterm/src/ui/DocumentAccessRequests/ReviewAccessRequest/useReviewAccessRequest.ts +++ b/web/packages/teleterm/src/ui/DocumentAccessRequests/ReviewAccessRequest/useReviewAccessRequest.ts @@ -70,9 +70,12 @@ export function useReviewAccessRequest({ ) ); const [deleteRequestAttempt, runDeleteRequest] = useAsync(() => - retry(() => - ctx.clustersService.deleteAccessRequest(rootClusterUri, requestId) - ) + retry(async () => { + await ctx.tshd.deleteAccessRequest({ + rootClusterUri, + accessRequestId: requestId, + }); + }) ); const [submitReviewAttempt, runSubmitReview] = useAsync( (review: SubmitReview) => diff --git a/web/packages/teleterm/src/ui/DocumentAccessRequests/useAccessRequests.tsx b/web/packages/teleterm/src/ui/DocumentAccessRequests/useAccessRequests.tsx index 497296f9373ad..aa895d2b044d0 100644 --- a/web/packages/teleterm/src/ui/DocumentAccessRequests/useAccessRequests.tsx +++ b/web/packages/teleterm/src/ui/DocumentAccessRequests/useAccessRequests.tsx @@ -74,9 +74,10 @@ export default function useAccessRequests(doc: types.DocumentAccessRequests) { const getRequests = async () => { try { - const response = await retryWithRelogin(ctx, clusterUri, () => - ctx.clustersService.getAccessRequests(rootClusterUri) - ); + const response = await retryWithRelogin(ctx, clusterUri, async () => { + const { response } = await ctx.tshd.getAccessRequests({ clusterUri }); + return response.requests; + }); setAttempt({ status: 'success' }); // transform tshd access request to the webui access request and add flags const requests = response.map(r => makeUiAccessRequest(r)); diff --git a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts index 90a57ab61bb4d..2bd3a1576a71c 100644 --- a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts +++ b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts @@ -35,7 +35,6 @@ import { Server } from 'gen-proto-ts/teleport/lib/teleterm/v1/server_pb'; import { Database } from 'gen-proto-ts/teleport/lib/teleterm/v1/database_pb'; import { CreateAccessRequestRequest, - GetRequestableRolesRequest, ReviewAccessRequestRequest, PromoteAccessRequestRequest, PasswordlessPrompt, @@ -365,34 +364,11 @@ export class ClustersService extends ImmutableStore return response.clusters; } - async getRequestableRoles(params: GetRequestableRolesRequest) { - const { response } = await this.client.getRequestableRoles(params); - - return response; - } - getAssumedRequests(rootClusterUri: uri.RootClusterUri) { const cluster = this.state.clusters.get(rootClusterUri); return cluster?.loggedInUser?.assumedRequests || {}; } - async getAccessRequests(rootClusterUri: uri.RootClusterUri) { - const { response } = await this.client.getAccessRequests({ - clusterUri: rootClusterUri, - }); - return response.requests; - } - - async deleteAccessRequest( - rootClusterUri: uri.RootClusterUri, - requestId: string - ) { - await this.client.deleteAccessRequest({ - rootClusterUri, - accessRequestId: requestId, - }); - } - /** Assumes roles for the given requests. */ async assumeRoles( rootClusterUri: uri.RootClusterUri, From 95ec50e5f17e225076cf724f81e3fdf7091e86d2 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Mon, 10 Jun 2024 16:03:05 +0200 Subject: [PATCH 4/4] Do not mix async/await with then --- .../src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts b/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts index a2477c71b07d1..fd638cf528bd1 100644 --- a/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts +++ b/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts @@ -129,8 +129,6 @@ export default function useAccessRequestCheckout() { subResourceName: '', })), }); - return response; - }).then(response => { setResourceRequestRoles(response.applicableRoles); setSelectedResourceRequestRoles(response.applicableRoles); })