From 619267c518e4e8b54aef9f87290753d218c41f2f 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 afb543beb07b2..6dece63ee8981 100644 --- a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts +++ b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts @@ -392,23 +392,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 f163d537a2d856743999ba900476d71b2786a78d 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 6dece63ee8981..daaa1b5cec49e 100644 --- a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts +++ b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts @@ -338,13 +338,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; @@ -352,25 +345,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, }); @@ -381,11 +359,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, @@ -423,12 +396,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, @@ -438,12 +405,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; @@ -456,12 +417,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 140d1cb64aae923a9d254c887e2d196cc9ce1917 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 b43648876daf4..7aeeb4fb83427 100644 --- a/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts +++ b/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts @@ -107,8 +107,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') @@ -121,8 +121,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 daaa1b5cec49e..cadec712cfc5e 100644 --- a/web/packages/teleterm/src/ui/services/clusters/clustersService.ts +++ b/web/packages/teleterm/src/ui/services/clusters/clustersService.ts @@ -32,7 +32,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, @@ -337,34 +336,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 a0329109d4baaa2e669de1f9d8cdcce8ea0b5ca7 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 7aeeb4fb83427..fca1a0247c075 100644 --- a/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts +++ b/web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts @@ -122,8 +122,6 @@ export default function useAccessRequestCheckout() { subResourceName: '', })), }); - return response; - }).then(response => { setResourceRequestRoles(response.applicableRoles); setSelectedResourceRequestRoles(response.applicableRoles); })