Skip to content

Do not send "assume role" usage event when dropping access request#42533

Merged
gzdunek merged 5 commits intomasterfrom
gzdunek/assume-role-event
Jun 10, 2024
Merged

Do not send "assume role" usage event when dropping access request#42533
gzdunek merged 5 commits intomasterfrom
gzdunek/assume-role-event

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Jun 6, 2024

Closes #42484

To fix the issue, I separated methods for assuming and dropping requests. We never assume and drop a request at the same time, so I think this change will make this API easier to use. Let me know what you think about it.

While I was there, I also removed cluster.connect checks. Each of these calls 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.

After that I removed some methods completely, as they only passed calls to tshd.

gzdunek added 3 commits June 6, 2024 11:26
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.
@gzdunek gzdunek added teleport-connect Issues related to Teleport Connect. backport/branch/v14 no-changelog Indicates that a PR does not require a changelog entry labels Jun 6, 2024
@gzdunek gzdunek requested a review from ravicious June 6, 2024 10:03
@github-actions github-actions Bot requested a review from ibeckermayer June 6, 2024 10:03
@gzdunek gzdunek requested review from avatus and removed request for ibeckermayer June 6, 2024 10:04
Comment thread web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts Outdated
rootClusterUri,
accessRequestId: requestId,
});
})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  const [deleteRequestAttempt, runDeleteRequest] = useAsync(() =>
    retry(() =>
      ctx.tshd.deleteAccessRequest({
        rootClusterUri,
        accessRequestId: requestId,
      })
    )
  );
web/packages/teleterm/src/ui/DocumentAccessRequests/ReviewAccessRequest/useReviewAccessRequest.ts:74:7 - error TS2739: Type 'CloneableUnaryCall<DeleteAccessRequestRequest, EmptyResponse>' is missing the following properties from type 'Promise<FinishedUnaryCall<DeleteAccessRequestRequest, EmptyResponse>>': catch, finally, [Symbol.toStringTag]

74       ctx.tshd.deleteAccessRequest({
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
75         rootClusterUri,
   ~~~~~~~~~~~~~~~~~~~~~~~
76         accessRequestId: requestId,
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
77       })
   ~~~~~~~~

  web/packages/teleterm/src/ui/DocumentAccessRequests/ReviewAccessRequest/useReviewAccessRequest.ts:55:17
    55     <T>(action: () => Promise<T>) => retryWithRelogin(ctx, clusterUri, action),
                       ~~~~~~~~~~~~~~~~
    The expected type comes from the return type of this signature.

I guess we forgot to implement catch and finally on the cloneable client? 😩 Or just maybe it's a matter of adding relevant types. This whole time I've assumed they were implemented in terms of then. 🤷

Anyway, it's something we should tackle down the road.

Copy link
Copy Markdown
Contributor Author

@gzdunek gzdunek Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we didn't not forget.

protobuf-ts type exposes only .then - this concept is called thenable https://masteringjs.io/tutorials/fundamentals/thenable.

To simplify this call we could remove await:

  const [deleteRequestAttempt, runDeleteRequest] = useAsync(() =>
    retry(async () =>
      ctx.tshd.deleteAccessRequest({
        rootClusterUri,
        accessRequestId: requestId,
      })
    )
  );

but I wanted the attempt to be of type Attempt<void>, since we are not interested in the response anyway.

@gzdunek gzdunek added this pull request to the merge queue Jun 10, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 10, 2024
@gzdunek gzdunek enabled auto-merge June 10, 2024 14:47
@gzdunek gzdunek added this pull request to the merge queue Jun 10, 2024
Merged via the queue into master with commit 50139a6 Jun 10, 2024
@gzdunek gzdunek deleted the gzdunek/assume-role-event branch June 10, 2024 15:06
@public-teleport-github-review-bot
Copy link
Copy Markdown

@gzdunek See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Create PR
branch/v16 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm teleport-connect Issues related to Teleport Connect. ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Connect sends "assume role" usage event when dropping access request

3 participants