SharedDirectoryAnnounce

[ibeckermayer]

UX

Allows the user to select "Share Directory" from the dropdown

And choose a directory to share

If necessary, the user will be prompted to allow access to the directory

At this point, the session will terminate because we will receive an unimplemented TDP message.

Notes

Couldn't find a permission name

Unfortunately, unlike the clipboard feature, I wasn't able to find a permission name to query for this permission preemptively

webapps/packages/teleport/src/DesktopSession/useClipboard.ts

Line 61 in bc6586b

navigator.permissions.query({ name: permissionName }).then(perm => {

The list of permission names for Chrome is linked by MDN's documentation to here, but none of these are what we are looking for.

Mysterious DOMException

We will also occasionally get a Uncaught (in promise) DOMException: Document is not focused. error when this permission modal pops up, or maybe more accurately, once sharedDirHandle is initialized:

webapps/packages/teleport/src/DesktopSession/DesktopSession.tsx

Lines 162 to 171 in bc6586b

	window
	.showDirectoryPicker()
	.then(sharedDirHandle => {
	setIsSharingDirectory(true);
	tdpClient.sharedDirectory = sharedDirHandle;
	tdpClient.sendSharedDirectoryAnnounce();
	})
	.catch(() => {
	setIsSharingDirectory(false);
	});

It's not entirely clear to me why this happens -- I've struggled to find a way to reproduce it consistently, though denying the permission in the modal and then asking for it again tends to work most of the time. There is limited information on Google about it. Most of the information relates to the clipboard API, which we dealt with here by checking for focus:

webapps/packages/teleport/src/DesktopSession/useTdpClientCanvas.tsx

Lines 150 to 159 in bc6586b

	// We must check that the DOM is focused or navigator.clipboard.readText throws an error.
	if (enableClipboardSharing && document.hasFocus()) {
	navigator.clipboard.readText().then(text => {
	if (text) {
	cli.sendClipboardData({
	data: text,
	});
	}
	});
	}

However even if I try adding such a check like so

        onShareDirectory={() => {
          window
            .showDirectoryPicker()
            .then(sharedDirHandle => {
              console.log('in then');
              if (document.hasFocus()) {
                setIsSharingDirectory(true);
                tdpClient.sharedDirectory = sharedDirHandle;
                tdpClient.sendSharedDirectoryAnnounce();
              } else {
                throw new Error(
                  'Cannot handle directory while document does not have focus'
                );
              }
            })
            .catch(err => {
              console.log('in catch');
              console.log('err = ' + err);
              setIsSharingDirectory(false);
            });
        }}

I still encounter the error. The odd thing is, without the check, everything still seems to run according to plan, even if the error occurs. With the check, the error occurs but then tdpClient.sendSharedDirectoryAnnounce(); never gets called.

As far as I can tell, the worst thing that will happen by letting this error slide is that the console will have some uncaught exceptions. It could just be a function of this being a bleeding edge feature.

Other known issues

• This feature will only be supported in Chromium based browsers. For now I am ignoring other browsers: Handle directory sharing in non-chrome browsers #959
• Handle directory sharing initialization before login completes teleport#21072

Opportunistically

Also removes the recording symbol, closes gravitational/teleport#13973

How to test manually

teleport

Build and run the windows-desktop-directory-sharing branch, editing this line in the Makefile to have the directory_sharing build flag:

$(BUILDDIR)/teleport: ensure-webassets bpf-bytecode rdpclient
	GOOS=$(OS) GOARCH=$(ARCH) $(CGOFLAG) go build -tags "$(PAM_TAG) $(FIPS_TAG) $(BPF_TAG) $(WEBASSETS_TAG) $(RDPCLIENT_TAG) directory_sharing" -o $(BUILDDIR)/teleport $(BUILDFLAGS) ./tool/teleport

webapps

Change the value below to true and run the dev server

webapps/packages/teleport/src/config.ts

Line 32 in becfad3

enableDirectorySharing: false, // note to reviewers: should be false in any PRs.

Requires backport to v9/v10

[ravicious]

Is there a cluster I could connect to to try this out?

[ravicious]

A preemptive approval with two caveats:

1. I didn't have a chance to actually test this.
2. I don't know what our requirements are when it comes to browser support for desktop access. If we run some checks before the execution gets to window.showDirectoryPicker() then it's fine. But if not then we should add some feature checks.

[ravicious]

Do we need to handle browsers which don't support this API?

[ibeckermayer]

Yeah, I left that as a "known issue" for now: #959

[ravicious]

This feels like one of those logic puzzles… 🤔

But in all seriousness, is it supposed to be true or false in the end? 😅

[ibeckermayer]

Haha

You got it correct, its supposed to be false in the end. Fixed.

[hatched]

Should probably add a check for the existence of window.showDirectoryPicker so that this doesn't throw on unsupported browsers.

And as a little nitpick, I don't like defining functions within the render portion of a component as it bloats the markup and makes it harder to follow.

[ibeckermayer]

Should probably add a check for the existence of window.showDirectoryPicker so that this doesn't throw on unsupported browsers.

I left that as a "known issue" for now: #959, since we will want to add some error handling and UI message, but I am just focusing on getting the core functionality working.

[hatched]

I read #959 as actually implementing the handling functionality, not protecting against the application throwing in browsers that don't support it.

[ibeckermayer]

Basically I'm trying to charge forward with the core implementation in browsers that actually support it, and then go back at the end and write the code that makes sure the UX isn't totally confusing in browsers that don't.

[ravicious]

FWIW, I tried it with Firefox and I wasn't able to get to the point where the UI would allow me to trigger this code. The Web UI blocked me from using remote desktops and showed this message:

Your user role supports clipboard sharing over desktop access, however this feature is only available on chromium based browsers like Brave or Google Chrome. Please switch to a supported browser.

It didn't start the session so there was no risk of this code crashing.

[kimlisa]

We will also occasionally get a Uncaught (in promise) DOMException: Document is not focused. error when this permission modal pops up, or maybe more accurately, once sharedDirHandle is initialized

not sure what the error is without manually testing it but did you try focusing before calling the func?

[ibeckermayer]

Is there a cluster I could connect to to try this out?

Not that I know of. @zmb3 is there something we can spin up or that you can hand off?

[ibeckermayer]

onShareDirectory

No I hadn't tried that. I just tried

const onShareDirectory = () => {
  window.focus();
  ...
};

and

const onShareDirectory = () => {
  document.getElementById('app').focus();
  ...
};

to no avail

[ravicious]

Re: "Document is not focused" error, it's interesting that it's reported as

Uncaught (in promise) DOMException: Document is not focused.

This code doesn't seem to be what's triggering it as the event handler function itself isn't executed within a promise. The promise returned by showDirectoryPicker has a catch so it can't be the source of it as well.

webapps/packages/teleport/src/DesktopSession/DesktopSession.tsx

Lines 161 to 172 in bc6586b

	onShareDirectory={() => {
	window
	.showDirectoryPicker()
	.then(sharedDirHandle => {
	setIsSharingDirectory(true);
	tdpClient.sharedDirectory = sharedDirHandle;
	tdpClient.sendSharedDirectoryAnnounce();
	})
	.catch(() => {
	setIsSharingDirectory(false);
	});
	}}

Matter of fact, when I changed the event handler to this:

        onShareDirectory={() =>
          Promise.resolve()
            .then(() => window.showDirectoryPicker())
            .then(sharedDirHandle => {
              setIsSharingDirectory(true);
              tdpClient.sharedDirectory = sharedDirHandle;
              tdpClient.sendSharedDirectoryAnnounce();
            })
            .catch(e => {
              console.log('onShareDirectory error', { e });
              setIsSharingDirectory(false);
            })
        }

the error was still reported as "Uncaught (in promise)". 🤔

[ibeckermayer]

@ravicious

Yeah I was messing around and noticed the same thing (I tried wrapping that entire section in a try catch). The behavior is also inconsistent, for example I tried just now and wasn't able to induce the error. Seems like a bug in the implementation somewhere.