Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Teleterm: Define access request fields necessary for selecting time options #39050

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Mar 7, 2024

part of #35436

I also sneaked in an export RadioGroup.tsx required in an enterprise work

Copy link

github-actions bot commented Mar 7, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@kimlisa kimlisa removed the request for review from ryanclark March 7, 2024 06:39
@kimlisa kimlisa added backport/branch/v15 no-changelog Indicates that a PR does not require a changelog entry labels Mar 7, 2024
@kimlisa
Copy link
Contributor Author

kimlisa commented Mar 7, 2024

@gzdunek @ravicious , just FYI, this PR adds more fields than just assume_start_time, the other fields are required for calculating/rendering time options for users (like in the web UI)

Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

Submitted some nits, looks good overall.

proto/teleport/lib/teleterm/v1/access_request.proto Outdated Show resolved Hide resolved
proto/teleport/lib/teleterm/v1/service.proto Show resolved Hide resolved
web/packages/teleterm/src/services/tshd/types.ts Outdated Show resolved Hide resolved
proto/teleport/lib/teleterm/v1/service.proto Outdated Show resolved Hide resolved
web/packages/teleterm/src/services/tshd/types.ts Outdated Show resolved Hide resolved
@kimlisa
Copy link
Contributor Author

kimlisa commented Mar 8, 2024

@ravicious @gzdunek can i get a sanity check for this commit and this follow up enterprise PR

maxDuration: params.maxDuration,
requestTtl: params.requestTtl,
assumeStartTime: params.assumeStartTime,
resourceIds: params.resourceIds,
Copy link
Member

Choose a reason for hiding this comment

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

req can be dropped completely and we can pass params directly:

diff --git a/web/packages/teleterm/src/services/tshd/createClient.ts b/web/packages/teleterm/src/services/tshd/createClient.ts
index 6aa17d551b..c99a1b67b6 100644
--- a/web/packages/teleterm/src/services/tshd/createClient.ts
+++ b/web/packages/teleterm/src/services/tshd/createClient.ts
@@ -295,19 +295,8 @@ export function createTshdClient(
     },
 
     async createAccessRequest(params: types.CreateAccessRequestParams) {
-      const req = api.CreateAccessRequestRequest.create({
-        rootClusterUri: params.rootClusterUri,
-        suggestedReviewers: params.suggestedReviewers,
-        roles: params.roles,
-        reason: params.reason,
-        dryRun: params.dryRun,
-        maxDuration: params.maxDuration,
-        requestTtl: params.requestTtl,
-        assumeStartTime: params.assumeStartTime,
-        resourceIds: params.resourceIds,
-      });
       return new Promise<AccessRequest>((resolve, reject) => {
-        tshd.createAccessRequest(req, (err, response) => {
+        tshd.createAccessRequest(params, (err, response) => {
           if (err) {
             reject(err);
           } else {

AssumeStartTime: getProtoTimestamp(req.GetAssumeStartTime()),
MaxDuration: timestamppb.New(req.GetMaxDuration()),
RequestTtl: timestamppb.New(req.Expiry()),
SessionTtl: timestamppb.New(req.GetSessionTLL()),
Copy link
Member

Choose a reason for hiding this comment

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

The changes from this PR seem okay and they typecheck. I'm able to successfully submit an access request, but when I then go to list all requests in Connect, I see "Invalid time value".

I added a console.error to see the exact error:

diff --git a/web/teleterm/src/ui/DocumentAccessRequests/useAccessRequests.tsx b/web/teleterm/src/ui/DocumentAccessRequests/useAccessRequests.tsx
index 216dae57..d03f6e36 100644
--- a/web/teleterm/src/ui/DocumentAccessRequests/useAccessRequests.tsx
+++ b/web/teleterm/src/ui/DocumentAccessRequests/useAccessRequests.tsx
@@ -57,6 +57,7 @@ export default function useAccessRequests(doc: types.DocumentAccessRequests) {
       );
       setAccessRequests(requests);
     } catch (err) {
+      console.log(err);
       setAttempt({
         status: 'failed',
         statusText: err.message,

And the error is:

RangeError: Invalid time value
    at formatDistanceStrict (date-fns.js?v=1abd8bfc:2759:11)
    at getDurationText (makeAccessRequest.ts:87:20)
    at makeAccessRequest (makeAccessRequest.ts:24:22)
    at makeUiAccessRequest (useAccessRequests.tsx:98:10)
    at useAccessRequests.tsx:56:17
    at Array.map (<anonymous>)
    at getRequests (useAccessRequests.tsx:55:33)

It looks like makeAccessRequest from e/web/teleport/src/services/workflow/makeAccessRequest.ts expects dates to be Date instances. The argument to it doesn't have a type, so we didn't get type errors for that.

Perhaps makeUiAccessRequest from e/web/teleterm/src/ui/DocumentAccessRequests/useAccessRequests.tsx needs to be updated?

Copy link
Contributor Author

@kimlisa kimlisa Mar 12, 2024

Choose a reason for hiding this comment

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

thanks so much for catching these, i updated the types here and added a test to hopefully catch them easier in future. probably the best way to prevent is to type the response value right? but i think that's going to create more refactoring

i also ran teleterm to test these changes out

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, in teleterm we shouldn't use makeAccessRequest from web as it converts the fully typed response to any :(
But this is out of scope for this PR.

@kimlisa kimlisa force-pushed the lisa/teleterm/new-access-request-fields branch from ff803d9 to 96bc3b6 Compare March 13, 2024 03:19
@kimlisa kimlisa enabled auto-merge March 13, 2024 03:20
@kimlisa kimlisa force-pushed the lisa/teleterm/new-access-request-fields branch from 61527b8 to b91e349 Compare March 13, 2024 18:56
@kimlisa kimlisa added this pull request to the merge queue Mar 14, 2024
Merged via the queue into master with commit f33bbbf Mar 14, 2024
40 checks passed
@kimlisa kimlisa deleted the lisa/teleterm/new-access-request-fields branch March 14, 2024 01:04
@public-teleport-github-review-bot

@kimlisa See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v14 backport/branch/v15 no-changelog Indicates that a PR does not require a changelog entry size/md ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants