Relax access requests limitations and refactor AccessRequestsService state
#41717
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…an access request This was an artificial limitation, we only need to pass the correct cluster name to each requested resource.
gzdunek
added
teleport-connect
gzdunek
commented
May 17, 2024
web/packages/teleterm/src/ui/services/workspacesService/accessRequestsService.ts
Show resolved
Hide resolved
The aim of this refactor is to disallow mixing resource and role access requests at the type level. Another thing is that for resource access requests we need to store cluster name. Because it's not needed for roles, we have to separate these two resource kinds.
…nstead of when switching resource tab
…ctions with resource access requests
gzdunek
force-pushed
the
gzdunek/relax-access-requests-limitations
branch
from
fd0574a
to
fdc25d8
Compare
ravicious
reviewed
May 20, 2024
web/packages/teleterm/src/ui/services/workspacesService/accessRequestsService.ts
Outdated
Show resolved
Hide resolved
web/packages/teleterm/src/ui/services/workspacesService/accessRequestsService.ts
Outdated
Show resolved
Hide resolved
ravicious
reviewed
May 21, 2024
web/packages/teleterm/src/ui/AccessRequestCheckout/useAccessRequestCheckout.ts
Outdated
Show resolved
Hide resolved
web/packages/teleterm/src/ui/DocumentAccessRequests/NewRequest/useNewRequest.ts
Outdated
Show resolved
Hide resolved
web/packages/teleterm/src/ui/services/workspacesService/accessRequestsService.ts
Show resolved
Hide resolved
web/packages/teleterm/src/ui/services/workspacesService/accessRequestsService.ts
Show resolved
Hide resolved
web/packages/teleterm/src/ui/DocumentAccessRequests/NewRequest/useNewRequest.ts
Outdated
Show resolved
Hide resolved
avatus
approved these changes
May 21, 2024
The original type `ResourceKind` was too wide, we don't support `user_group` and `windows_desktop` anyway.
ravicious
approved these changes
May 22, 2024
.../teleterm/src/ui/ModalsHost/modals/ChangeAccessRequestKind/ChangeAccessRequestKind.story.tsx
Outdated
Show resolved
Hide resolved
...ckages/teleterm/src/ui/ModalsHost/modals/ChangeAccessRequestKind/ChangeAccessRequestKind.tsx
Outdated
Show resolved
Hide resolved
web/packages/teleterm/src/ui/DocumentAccessRequests/NewRequest/useNewRequest.ts
Outdated
Show resolved
Hide resolved
web/packages/teleterm/src/ui/services/workspacesService/accessRequestsService.ts
Show resolved
Hide resolved
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As of now, we had two limitations when creating access requests:
As it turned out, 1. was an artificial limitation. It's perfectly fine for the backend to have requests from many clusters in a single access request.
But why did I want to remove it? It's because in the search bar you have access to all resources, so we can't easily block adding resources from a different cluster (and tbh the previous check that was kept in the cluster switcher was really easy to bypass by simply opening multiple access requests tabs).
However, after I removed this limitation, another problem occurred: we don't store cluster URI for the resources.
To fix this, I had to refactor the access requests service (and its state in particular).
The
{app: {}, node: {}, ...}was replaced by aMapkey by resource URI. This ensures that resources are unique among clusters.The map value is a request object, which keeps a resource kind, uri, and for servers their hostname.
Previously, the hostname was stored as
kindIds[resourceId] = resourceName || resourceId. I must say that I didn't like it, mainly because it was up to the caller to set the name (and it was really easy to miss it). Now it is required by the types for the particular resource type.As of 2: I moved the check to the access requests service level, because as above, we can't control what the user adds to the request: it may add a role and the go the search bar and add a resource.
I also changed the state shape, now it's
roles | resources, not roles & resources` to better reflect the business logic.TODO: show cluster name in the request checkout of there is more than one cluster in the request.