[web] Moderated Session file transfers#24583
[web] Moderated Session file transfers#24583avatus merged 38 commits intomichaelmyers/web_terminal_file_request_handlersfrom
Conversation
This PR separates the absolute positioned container from the FileTransfer component. We will be adding FileTransferRequests in the same spot, and this will allow them to be visible together without css shenanigans trying to absolutely place them together. FileTransferRequests are separate from FileTransfers in that they interact with the tty.
| setSession(prev => ({ | ||
| ...prev, | ||
| moderated: data.session.moderated, | ||
| })); |
There was a problem hiding this comment.
We weren't setting any returned session data at all here previously so no server fetched data was being stored. I couldn't find a reason for this, but just in case, i only added the moderated property. However, i'd like to just set all returned data here with setSession(data.session) . In the future, we can have a lot more things held on the session, such as already active fileTransferRequests . Thoughts?
There was a problem hiding this comment.
It looks the code on master never uses session from this hook at all. setSession(data.session) sounds good to me if what you need to do is use this piece of state somewhere else.
I couldn't figure out quickly what shape is under data.session, but if that's something that has all the field that are on the Session interface, then that's even better. Right now it seems that the concept of a session in useSshSession is used only as a container for some ID data for the session ({login, serverId, clusterId, sid }), while not really caring about all the other fields that Session defines. If we were to continue doing this, I'd probably recommend using a separate type here. In Connect we'd probably use a URI of some kind.
| @@ -0,0 +1,162 @@ | |||
| import { useState } from 'react'; | |||
There was a problem hiding this comment.
I decided to put this logic in it's own hook because it got a bit complicated to throw all into the useSshSession. I know we had a conversation around avoid Container + hook paradigm, but I don't think this is that. Happy to throw it into useSshSession or even in DocumentSsh if we really wanna do it but, it seems to fit nicely here.
| }; | ||
| }; | ||
|
|
||
| export type FilesStore = ReturnType<typeof useFilesStore>; |
There was a problem hiding this comment.
Will update this away from ReturnType
avatus
force-pushed
the
michaelmyers/web_terminal_file_request_handlers
branch
from
959073c to
8521b0d
Compare
…ichaelmyers/web/moderated_file_transfers
|
I get an error when I try to join the session as a moderator. I'm not sure if it's a problem with my config or not, could you share yours? ErrorsThe error is: followed by and I assume one of these last two goes to the peer and one to the moderator. My role configIncluded in the New role that my user has together with the default |
|
I am able to join as both moderator and observer. My user has the My role config
spec:
allow:
kubernetes_resources:
- kind: pod
name: '*'
namespace: '*'
require_session_join:
- count: 1
filter: contains(user.spec.roles, "auditor")
kinds:
- k8s
- ssh
modes:
- moderator
name: Auditor oversight
on_leave: ""
spec:
allow:
join_sessions:
- kinds:
- k8s
- ssh
modes:
- moderator
- observer
name: Auditor oversight
roles:
- prod-access
- auditor
kubernetes_resources:
- kind: pod
name: '*'
namespace: '*'
rules:
- resources:
- session_tracker
verbs:
- list
- read |
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
I just skimmed through the recent changes. The hooks look much better now, thanks for refactoring this.
I'll review the rest of the code on Monday.
| onApprove: (string, boolean) => void; | ||
| onDeny: (string, boolean) => void; |
|
|
||
| // only set if both are params are present | ||
| if (moderatedSessonId && fileTransferRequestId) { | ||
| path = `${path}&file_transfer_request_id=${fileTransferRequestId}&moderated_session_id=${moderatedSessonId}`; |
There was a problem hiding this comment.
Could file_transfer_request_id and moderated_session_id be added to cfg.api.scp as optional params, so we don't have to craft the URL by hand?
There was a problem hiding this comment.
as far as I'm aware, the generatePath function that we use to template these URLs doesn't like having an optional parameter. If I have moderatedSessionId=:moderatedSessionId and it ISN'T passed, it doesn't skip, it actually throws error. So we have to hand craft the url to include these optionally.
I believe generatePath is supposed to be used when creating routes (from react router) and not just as some string replacement. At least, thats how I read it.
Similar issue with the webauthn below in the same function.
| pendingFile = this._getPendingFile(locationAndName); | ||
| // cleanup if file exists. It's ok if it doesn't exist, we check thaat in the handler | ||
| if (pendingFile) { | ||
| delete this._pendingUploads[locationAndName]; |
There was a problem hiding this comment.
I don't know if this is even a realistic case, but what if I send two requests in a row for the same file and then I get two approvals? It looks like this code will remove the file after the first approval comes and the second upload will fail? The comment suggests that this is an expected scenario.
There was a problem hiding this comment.
Yeah I guess that would happen with this code but it is a somewhat unrealistic case that you wouldn't want to upload the same file to the same place at the same time right? A way to prevent this would be an error earlier up but because its such an edge case, an error at any spot would probably suffice. Let me know your thoughts if you have any more!
Could always do something like, set the file initially the way we are now with locationAndName and then when we receive the file transfer request, we can update the key with the ID? idk if all that would be worth it or make sense, just spitballin.
There was a problem hiding this comment.
It seems to me that the only reliable option would be to have the request ID immediately after creating the request (but this would be possible if we used something like fetch API, not websockets). So as we can't do that, I'd leave it with no changes.
| abortController: AbortController, | ||
| moderatedSessionParams?: ModeratedSessionParams | ||
| ) { | ||
| if (session.moderated) { |
There was a problem hiding this comment.
I don't understand how the moderated session works I think :(
- User tries to download a file so
downloadis invoked. - We are in a moderated session so a request is sent and we abort early.
- The approval comes in and we run
downloadagain. session.moderatedis false now, correct? When does the value change?
There was a problem hiding this comment.
Just from my shallow understanding of this I think session.moderated comes from #24238 and is set per-session. That is, you create a moderated session and it stays "moderated" throughout its lifespan, it cannot become "unmoderated".
There was a problem hiding this comment.
That is, you create a moderated session and it stays "moderated" throughout its lifespan, it cannot become "unmoderated".
This makes sense, but I don't understand how the code here
runs the actual download ifsession.moderated is true 🤔
There was a problem hiding this comment.
If session.moderated is true, we only send the tty.sendFileDownloadRequest. The actual file transfer is handled when we receive an approved message from the server. However, you are correct that this is WRONG! It took me awhile to figure out what happened because it actually works (when you read it, it shouldnt!).
Originally, when useFileTransfer was inside of useSshSession, I had a handleUpload, and handleDownload function exported and it did that check. I accidentally conflated those two when doing the refactor and removed it thinking "sweet, I can just have one function now". and it worked during testing but it really shouldn't now that you mention it
The reason why this 'works' now is the function download that we are passing to handleFileTransferApproval is passed before session is defined and we are inside a memoized document. So session.moderated isn't even set in that version of download, where as the one the user uses from the file transfer dialog checks session that that invocation time.
So even tho it works, it really shouldn't and is no way readable in it's current state. I'm going to add back the download/handleDownload paradigm and it'll work as expected and be much more readable. Thanks for calling this out
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
I got through almost everything other than those changes in lib/term, I'll look at that tomorrow!
|
I think I tackled all the feedback. Will do another quick cleanup pass in the morning as well. The main bits are
|
| setSession(prev => ({ | ||
| ...prev, | ||
| moderated: data.session.moderated, | ||
| })); |
There was a problem hiding this comment.
It looks the code on master never uses session from this hook at all. setSession(data.session) sounds good to me if what you need to do is use this piece of state somewhere else.
I couldn't figure out quickly what shape is under data.session, but if that's something that has all the field that are on the Session interface, then that's even better. Right now it seems that the concept of a session in useSshSession is used only as a container for some ID data for the session ({login, serverId, clusterId, sid }), while not really caring about all the other fields that Session defines. If we were to continue doing this, I'd probably recommend using a separate type here. In Connect we'd probably use a URI of some kind.
| useEffect(() => { | ||
| // the tty will be init outside of this hook, so we wait until | ||
| // it exists and then attach file transfer handlers to it | ||
| if (tty) { |
There was a problem hiding this comment.
FWIW, you can return early if there's no tty and get rid and remove one level of nesting both from the effect body and the cleanup function.
| file?: File | ||
| ) { | ||
| removeFileTransferRequest(request.requestID); | ||
| if (request.requester !== user.username) { |
There was a problem hiding this comment.
You cannot use isOwnRequest here because request in this context comes over the wire and isOwnRequest is something applicable only to requests stored in the state of this hook, right?
If that's the case, I'm not sure if putting isOwnRequest on FileTransferRequest is a good idea. We end up with two different types of requests and you have to remember which one you're dealing with in the given scenario.
What do you think about exporting a standalone isOwnRequest function from this file which accepts a request and a user and returns a boolean? You could use it here and in FileTransferRequests.
Here specifically it'd make the code easier to read rather than having to recall what request.requester !== user.username means.
There was a problem hiding this comment.
I went with exporting the function but there is another pattern throughout the codebase (for web mostly) that is like makeUiFileTransferRequest where other things are added, and basically morphs a response from an API into something usable by the api. I seen it a lot in our tables.
Not sure if that would be better here right now since we're only adding this one field, but if it starts to get more complicated, I'll abstract it to that in the future.
Again, for now, exporting a function is just fine.
| handleUpload, | ||
| handleDownload, |
There was a problem hiding this comment.
What do you think about renaming these two to getUploader and getDownloader and returning them from the hook as transferHandlers? They seem to be used as such anyway.
Right now the hook defines four functions called handle*, but half of them does something entirely different than the other half.
There was a problem hiding this comment.
Love the name change, will do. Naming is hard!
| }); | ||
| } | ||
|
|
||
| function updateFileTransferRequests(data: FileTransferRequest) { |
There was a problem hiding this comment.
Since the names of two other listeners we put on tty start with handle, I think we might be better off using this convention for this function as well.
I'd even consider putting two comment blocks to clearly delineate the two sections of the file, like
/*
* Transfer handlers
*/
// …
/*
* TTY event listeners
*/Looking at the transfer handlers and TTY event listeners, they don't seem to depend on each other, so they could be as well defined in two separate hooks, let's say something like useTtyFileTransferRequestListeners and useSshSessionFileTransferHandlers.
If the event listeners and transfer handlers are unlikely to change together, then splitting them like this would be much better than putting comment blocks to explain that these two sections of the file are related to two different responsibilities.
But I'm not sure if they're actually going to change together and I'm not sure if there are any other changes we'll make to the file transfer code, so feel free to ignore this.
Though I have to say, it'd make it much easier to under what all those functions are doing! But I know that this is like a round three of code review so I understand if making further changes is just tiresome.
There was a problem hiding this comment.
I won't move them into separate hooks but i moved things around a bit. I added your comment blocks and also moved the tty event listeners up to right under the useEffect hook where they are bound. I also added a comment to each one to describe what it's handling. It may include a bit more backend info than needed but hey, not a huge problem to have a bit more info.
Although, I did run into using functions before declarations after moving a bunch of stuff to useCallback so its kinda wild again anyway. With those changes, I don't think its more clear in terms of method order, but definitely more clear with comments. Idk, you tell me.
Though I have to say, it'd make it much easier to under what all those functions are doing! But I know that this is like a round three of code review so I understand if making further changes is just tiresome.
Code review is never tiresome for this reviewee!
| ); | ||
| } | ||
|
|
||
| export function DocumentSsh({ doc, visible }: PropTypes) { |
There was a problem hiding this comment.
If nothing is going to use this then we can drop the export.
There was a problem hiding this comment.
I wanted to make one last round of QA, but the upload appears to no longer work. The moment I approve an upload request, I get "disconnected: continuation after FIN, bad MASK" on the moderator side, the peer gets disconnected. The cluster process panics: (similar to #24728)
panic: repeated read on failed websocket connection
goroutine 2169 [running]:
github.com/gorilla/websocket.(*Conn).NextReader(0x140026cde40)
github.com/gorilla/websocket@v1.5.0/conn.go:1030 +0x270
github.com/gorilla/websocket.(*Conn).ReadMessage(0x10b4235c0?)
github.com/gorilla/websocket@v1.5.0/conn.go:1093 +0x1c
github.com/gravitational/teleport/lib/web.(*TerminalStream).Read(0x140012a1140, {0x14001d73cdc, 0x1, 0x10a4075e0?})
github.com/gravitational/teleport/lib/web/terminal.go:1151 +0x50
github.com/gravitational/teleport/lib/client.handleNonPeerControls({0x14001824330, 0x9}, 0x1400167e300, 0x14002f4bfa0)
github.com/gravitational/teleport/lib/client/session.go:645 +0x64
github.com/gravitational/teleport/lib/client.(*NodeSession).pipeInOut.func2()
github.com/gravitational/teleport/lib/client/session.go:717 +0x90
created by github.com/gravitational/teleport/lib/client.(*NodeSession).pipeInOut
github.com/gravitational/teleport/lib/client/session.go:714 +0x24c
There was a problem hiding this comment.
Hmmm... was this was MFA enabled? I couldn't get this to reproduce and then I could with MFA. However, I also got this to panic on master without my changes. I couldn't figure out the answer yet but will dig into it. Might not be related?
For example, if I joined with moderated on the first user, then joined as the "moderator" on a second user, ANYTHING would give me that error you listed above. even if i pressed some random key like "u"
| ); | ||
| } | ||
| }; | ||
| }, [tty]); |
There was a problem hiding this comment.
Here we skipped updateFileTransferRequests, handleFileTransferApproval and handleFileTransferDenied from the deps and I'm afraid it can result in some weird bugs (and you already experienced problems with an incorrect memoization).
I know that fixing this means adding quite a lot of useCallback :/
There was a problem hiding this comment.
I tried adding the update in this dep array and also using useCallback and all them still resulted in empty state arrays. The only reliable way I found to get this update to work is how I have it now, which is using prevstate in the setState function which doesn't need any useCallback anyway.
There is all kind of messy memoization going on around these components. It's been quite difficult to work with.
There was a problem hiding this comment.
which is using prevstate in the setState function which doesn't need any useCallback anyway.
Right, using prevState is the correct way to handle such case https://react.dev/reference/react/useEffect#updating-state-based-on-previous-state-from-an-effect.
But the outer function (for example updateFileTransferRequests) should still be memoized, although setState inside is stable. If you tried to access some other state in this function, you would get incorrect results because it would always operate on the initial state value. Currently, you only update the state, so it works, but if we want to be 100% correct we should add memoize them.
There was a problem hiding this comment.
Sure sounds good. Will update today
…ichaelmyers/web/moderated_file_transfers
|
@avatus sorry for a delay on this 😓 I think the PR looks good, I only want to test it tomorrow. |
|
@avatus is the actual file transfer location verified against the requested location? It looks like it is not. I made a request to upload a file to I don't think it is desired behavior? |
It definitely used to be, let me inspect |
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
I'll do a final round of QA in a sec with a role-based config (since the cluster-wide setting seems to be broken atm). For now I'm submitting some of the minor issues I noticed when looking at the recent changes.
| ); | ||
| }; | ||
| }, [tty]); | ||
| }, [tty, handleFileTransferDenied, handleFileTransferApproval]); |
There was a problem hiding this comment.
This will end up running the effect on every render. Despite handleFileTransferDenied and handleFileTransferApproval utilizing useCallback, some of their deeper deps outside of this file do not use it and are recreated from scratch on each render.
The patch below limits the number of times the listeners are reattached when joining the session (opening the page and the peer passing the MFA challenge) from 6 to 4. I'm not sure if it guarantees that the listeners are reattached only when necessary though because I didn't have time to perform further inspection on this.
Patch
diff --git a/web/packages/shared/hooks/useAttemptNext.ts b/web/packages/shared/hooks/useAttemptNext.ts
index 5e3ec01093..e93bce6f2c 100644
--- a/web/packages/shared/hooks/useAttemptNext.ts
+++ b/web/packages/shared/hooks/useAttemptNext.ts
@@ -26,10 +26,10 @@ export default function useAttemptNext(status = '' as Attempt['status']) {
statusText: '',
}));
- function handleError(err: Error) {
+ const handleError = useCallback((err: Error) => {
logger.error('attempt', err);
setAttempt({ status: 'failed', statusText: err.message });
- }
+ }, []);
const run = useCallback((fn: Callback) => {
try {
diff --git a/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts b/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts
index 9d329e2d87..d75b3986a7 100644
--- a/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts
+++ b/web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts
@@ -14,6 +14,7 @@
* limitations under the License.
*/
+import { useCallback } from 'react';
import useAttempt from 'shared/hooks/useAttemptNext';
import cfg, { UrlScpParams } from 'teleport/config';
@@ -22,28 +23,31 @@ import auth from 'teleport/services/auth/auth';
export default function useGetScpUrl(addMfaToScpUrls: boolean) {
const { setAttempt, attempt, handleError } = useAttempt('');
- async function getScpUrl(params: UrlScpParams) {
- setAttempt({
- status: 'processing',
- statusText: '',
- });
- if (!addMfaToScpUrls) {
- return cfg.getScpUrl(params);
- }
- try {
- let webauthn = await auth.getWebauthnResponse();
+ const getScpUrl = useCallback(
+ async (params: UrlScpParams) => {
setAttempt({
- status: 'success',
+ status: 'processing',
statusText: '',
});
- return cfg.getScpUrl({
- webauthn,
- ...params,
- });
- } catch (error) {
- handleError(error);
- }
- }
+ if (!addMfaToScpUrls) {
+ return cfg.getScpUrl(params);
+ }
+ try {
+ let webauthn = await auth.getWebauthnResponse();
+ setAttempt({
+ status: 'success',
+ statusText: '',
+ });
+ return cfg.getScpUrl({
+ webauthn,
+ ...params,
+ });
+ } catch (error) {
+ handleError(error);
+ }
+ },
+ [addMfaToScpUrls, handleError, setAttempt]
+ );
return {
getScpUrl,Also, TIL that react-hooks/exhausive-deps is smart enough where it sees that handleFileTransferUpdate calls only the state setter and thus doesn't complain about it being missing from the dep array.
There was a problem hiding this comment.
After you applied that patch, I noticed that the listeners are getting re-attached after every file transfer. I managed to nail it down to the start function from useFilesStore.
This patch completely eliminates the issue:
Patch 2
diff --git a/web/packages/shared/components/FileTransfer/useFilesStore.ts b/web/packages/shared/components/FileTransfer/useFilesStore.ts
index 0f0970868a..5378c2fb2f 100644
--- a/web/packages/shared/components/FileTransfer/useFilesStore.ts
+++ b/web/packages/shared/components/FileTransfer/useFilesStore.ts
@@ -98,44 +98,6 @@ export const useFilesStore = () => {
const [state, dispatch] = useReducer(reducer, initialState);
const abortControllers = useRef(new Map<string, AbortController>());
- const start = async (options: {
- name: string;
- runFileTransfer(
- abortController: AbortController
- ): Promise<FileTransferListeners>;
- }) => {
- const abortController = new AbortController();
- const fileTransfer = await options.runFileTransfer(abortController);
-
- if (!fileTransfer) {
- return;
- }
-
- const id = new Date().getTime() + options.name;
-
- dispatch({ type: 'add', payload: { id, name: options.name } });
- abortControllers.current.set(id, abortController);
-
- fileTransfer.onProgress(progress => {
- updateTransferState(id, {
- type: 'processing',
- progress,
- });
- });
- fileTransfer.onError(error => {
- updateTransferState(id, {
- type: 'error',
- progress: undefined,
- error,
- });
- });
- fileTransfer.onComplete(() => {
- updateTransferState(id, {
- type: 'completed',
- });
- });
- };
-
const updateTransferState = useCallback(
(id: string, transferState: TransferState) => {
dispatch({ type: 'updateTransferState', payload: { id, transferState } });
@@ -143,6 +105,47 @@ export const useFilesStore = () => {
[]
);
+ const start = useCallback(
+ async (options: {
+ name: string;
+ runFileTransfer(
+ abortController: AbortController
+ ): Promise<FileTransferListeners>;
+ }) => {
+ const abortController = new AbortController();
+ const fileTransfer = await options.runFileTransfer(abortController);
+
+ if (!fileTransfer) {
+ return;
+ }
+
+ const id = new Date().getTime() + options.name;
+
+ dispatch({ type: 'add', payload: { id, name: options.name } });
+ abortControllers.current.set(id, abortController);
+
+ fileTransfer.onProgress(progress => {
+ updateTransferState(id, {
+ type: 'processing',
+ progress,
+ });
+ });
+ fileTransfer.onError(error => {
+ updateTransferState(id, {
+ type: 'error',
+ progress: undefined,
+ error,
+ });
+ });
+ fileTransfer.onComplete(() => {
+ updateTransferState(id, {
+ type: 'completed',
+ });
+ });
+ },
+ [updateTransferState]
+ );
+
const cancel = useCallback((id: string) => {
abortControllers.current?.get(id).abort();
}, []);
diff --git a/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts b/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts
index d1e4fa504b..ec5f8f844e 100644
--- a/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts
+++ b/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts
@@ -51,6 +51,7 @@ export const useFileTransfer = (
addMfaToScpUrls: boolean
) => {
const { filesStore } = useFileTransferContext();
+ const { start: startTransfer } = filesStore;
const ctx = useConsoleContext();
const currentUser = ctx.getStoreUser();
const [fileTransferRequests, setFileTransferRequests] = useState<
@@ -58,6 +59,7 @@ export const useFileTransfer = (
>([]);
const { getScpUrl, attempt: getMfaResponseAttempt } =
useGetScpUrl(addMfaToScpUrls);
+ const { clusterId, serverId, login } = currentDoc;
const download = useCallback(
async (
@@ -65,12 +67,11 @@ export const useFileTransfer = (
abortController: AbortController,
moderatedSessionParams?: ModeratedSessionParams
) => {
- const { clusterId, serverId, login } = currentDoc;
const url = await getScpUrl({
location,
- clusterId: clusterId,
- serverId: serverId,
- login: login,
+ clusterId,
+ serverId,
+ login,
filename: location,
moderatedSessonId: moderatedSessionParams?.moderatedSessionId,
fileTransferRequestId: moderatedSessionParams?.fileRequestId,
@@ -84,7 +85,7 @@ export const useFileTransfer = (
}
return getHttpFileTransferHandlers().download(url, abortController);
},
- [currentDoc, getScpUrl]
+ [clusterId, serverId, login, getScpUrl]
);
const upload = useCallback(
@@ -94,12 +95,11 @@ export const useFileTransfer = (
abortController: AbortController,
moderatedSessionParams?: ModeratedSessionParams
) => {
- const { clusterId, serverId, login } = currentDoc;
const url = await getScpUrl({
location,
- clusterId: clusterId,
- serverId: serverId,
- login: login,
+ clusterId,
+ serverId,
+ login,
filename: file.name,
moderatedSessonId: moderatedSessionParams?.moderatedSessionId,
fileTransferRequestId: moderatedSessionParams?.fileRequestId,
@@ -113,7 +113,7 @@ export const useFileTransfer = (
}
return getHttpFileTransferHandlers().upload(url, file, abortController);
},
- [currentDoc, getScpUrl]
+ [clusterId, serverId, login, getScpUrl]
);
/*
@@ -141,7 +141,7 @@ export const useFileTransfer = (
}
if (request.download) {
- return filesStore.start({
+ return startTransfer({
name: request.location,
runFileTransfer: abortController =>
download(request.location, abortController, {
@@ -155,7 +155,7 @@ export const useFileTransfer = (
if (!file) {
throw new Error('Approved file not found for upload.');
}
- return filesStore.start({
+ return startTransfer({
name: request.filename,
runFileTransfer: abortController =>
upload(request.location, file, abortController, {
@@ -164,7 +164,7 @@ export const useFileTransfer = (
}),
});
},
- [currentUser.username, download, filesStore, upload]
+ [currentUser.username, download, startTransfer, upload]
);
// handleFileTransferUpdate is called when a FILE_TRANSFER_REQUEST event is received. This is used when
@@ -197,9 +197,11 @@ export const useFileTransfer = (
if (!tty) {
return;
}
+
tty.on(EventType.FILE_TRANSFER_REQUEST, handleFileTransferUpdate);
tty.on(EventType.FILE_TRANSFER_REQUEST_APPROVE, handleFileTransferApproval);
tty.on(EventType.FILE_TRANSFER_REQUEST_DENY, handleFileTransferDenied);
+
return () => {
tty.removeListener(
EventType.FILE_TRANSFER_REQUEST,I moved that destructuring of currentDoc outside of the effect, so that the effect deps have only what's absolutely necessary. Though this might not be needed to fix the problem of reattaching, I don't think the doc itself gets updated that often. Still, the more precise the deps are the better.
filesStore should not be a dep since it's a new object on every render.
| ); | ||
| }; | ||
| }, [tty]); | ||
| }, [tty, handleFileTransferDenied, handleFileTransferApproval]); |
There was a problem hiding this comment.
Because of how useSshSession was originally written, tty comes from a ref. React will not re-render the component when tty changes so this effect works only "by accident", most probably because the same effect in useSshSession which sets ttyRef.current also sets session which is stored in the state and passed to useFileTransfer.
I'm not sure if it's a good practice to store thing in a ref but then pass it around without the ref. Alas, that's how useSshSession works on master at the moment.
If you don't feel like refactoring all of this right now (I wouldn't), I'd at least leave a TODO comment about refactoring useSshSession to pass ttyRef around instead and mentioning that this hook here works only incidentally.
| /* | ||
| * Transfer handlers | ||
| */ | ||
|
|
||
| function removeFileTransferRequest(requestId: string) { | ||
| setFileTransferRequests(prevstate => | ||
| prevstate.filter(ft => ft.requestID !== requestId) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This should be in the section above, shouldn't it? It's not used in transfer handlers.
| if (!tty) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This conditional in the cleanup function doesn't seem to be necessary. If tty is falsy, then the effect will return early with no cleanup function. AFAIK tty cannot suddenly become falsy within this scope as it's value is captured within the scope. This wouldn't be true had we used ttyRef where in theory ttyRef.current could become undefined at any point if someone updated ttyRef.current = undefined.
avatus
force-pushed
the
michaelmyers/web/moderated_file_transfers
branch
from
6276491 to
bf8b2e0
Compare
…ichaelmyers/web/moderated_file_transfers
…ichaelmyers/web/moderated_file_transfers
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
Transfers in moderated sessions seem to work fine, minus some potential issues that I mentioned in my comment.
Transfers in regular sessions seem to still work fine, both through the Web UI and Connect.
I tested them both for root cluster transfers and leaf cluster transfers, with the exception of testing transfers in moderated sessions on a leaf cluster – it'd be nice if someone could test it since my enterprise setup doesn't have a leaf cluster.
There was a problem hiding this comment.
Upload & download work fine on Chrome, but on Firefox I see "CredentialContainer request is not allowed." for both and on Safari there's "The document is not focused.".
This made me realize that probably on both of those browsers the auth APIs are available only when the window is active. When testing this locally on a single device, the window with the peer session would be inactive while I switched to the moderator session to accept the request.
I booted up my personal laptop and sure enough, the transfer works on both browsers.
Although when I was accepting the requests as the moderator, I was not being asked to complete the MFA check, is this the intended behavior? I don't remember how it worked with the cluster-wide setting. Maybe that's related to the general issues with moderated sessions that you're investigating.
Another potential issue I ran into is that immediately after I transfer something (I think I was uploading a file), I see this in the logs:
2023-04-28T18:18:22+02:00 ERRO [TRANSPORT] ssh stream terminated unexpectedly: rpc error: code = Canceled desc = context canceled pid:89950.1 transportv1/transport.go:236
There was a problem hiding this comment.
I wasn't able to reproduce the "document is not focused" bug. Similar happened here I believe.
I wonder if the CredentialContainer issue for firefox is due to wonky https. I can't log in on firefox either with per session mfa because it has a bit different way of interacting with the certs (since the vite change).
However, outside of the per-session-mfa stuff, the moderated file transfers were tested and worked on all the browsers listed above. Might be out of scope for this specific PR but I can add some todos
The error you received after the transfer also happens on master FWIW, I think it might have showed after the switch from scp -> sftp. I can inspect and will put up a PR after this if I find a fix
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
I'm leaving a preemptive approval since it seems that there are some issues with per-session MFA that are unrelated to your changes with file transfer (like the taps to confirm presence or I assume to approve requests too?). I don't want to block you while you figure this out.
The PR seems to be in a good shape overall now.
gzdunek
left a comment
gzdunek
left a comment
There was a problem hiding this comment.
I'm leaving an approval too, as Michael said in DM, the the bug from here #24583 (comment) will be fixed in other PR.
public-teleport-github-review-bot
Bot
removed the request for review
from ibeckermayer
Yup, fix was added here (to base branch) 2ae127e. Thanks! |
…ichaelmyers/web/moderated_file_transfers
4 participants
Part of: #23546
Relies on #24581
This PR adds the ability to request file transfers and respond to them in a moderated session.
Demo here:
Screen.Recording.2023-04-11.at.7.14.10.PM.mov
If you'd like to test, you can follow setup for moderated sessions here and then go and request some files!
Notes:
_pendingUploadstottybecause the state was being mega weird. Lots of memoization and what not going on so getting this from state was not working at all. Plus, storing in the tty is closer to the handler that is getting that pendingUpload so I think it's the best spot for it rather than passing it around everywhere.TODO
[x] Add filesStore to the Connect callsite of FileTransfers
[x] Fix FileTransfers tests with a mock filesStore
[ ] Add storybook tests for the new FileTransferRequests component
[ ] Add some tests for
useFileTransferand other related changes.