Add Studio Token Auth Flow #4931

julieg18 · 2023-10-30T17:37:30Z

Demo

~~https://github.com/iterative/vscode-dvc/assets/43496356/c0b597c5-0a76-4481-94c3-fd356a124bd2~~

Screen.Recording.2023-11-10.at.11.10.59.AM.mov

Part of #3864

extension/src/setup/studio.ts

julieg18 · 2023-11-09T15:50:22Z

extension/src/vscode/external.test.ts

+  jest.resetAllMocks()
+})
+
+describe('openUrl', () => {


Went ahead and added a small test for openUrl since we didn't seem to have one.

'should handle a message from the webview to open Studio' tested it indirectly (anything that used mockOpenExternal tested the call to the same level as this). It seems like you've deleted all of those tests which means mockOpenExternal can be removed from buildSetup.

extension/src/vscode/external.test.ts

julieg18 · 2023-11-09T15:59:41Z

extension/src/vscode/external.ts

+export const waitForUriResponse = (path: string, onResponse: () => unknown) => {
+  window.registerUriHandler({
+    handleUri(uri: Uri): ProviderResult<void> {
+      if (uri.path.startsWith(path)) {


Originally, I was going to check for uri.path === path, but there's an issue with Studio redirecting an encoded version of our callback URL.

Instead of getting redirected to vscode://iterative.dvc/studio-complete-auth?windowId=2, Studio will redirect the user to vscode://iterative.dvc/studio-complete-auth%253FwindowId=2, which makes the path studio-complete-auth%253F, failing an equals check.

I'm asking Thomas if Studio can redirect the user to an decoded version... Though now that I think about it, it should be a small change, I can probably open a pr myself 🤔

For now, I'm using uri.path.startsWith, but we can change it to an === check once Studio is updated.

decodeURI() does not work here?

Yes, technically we could use decodeURI here to check the path, but we need the user to be sent to the vscode URL with the windowId query since I believe it helps VSCode to route the user correctly depending on certain circumstances.

We could use decodeURI as a temporary solution instead of .startsWith() though!

shcheklein · 2023-11-09T19:59:19Z

Looks good to me. @yathomasi @amritghimire we still need to review the messaging on the Studio side (e.g. Device Activation - etc - do we really need those low level terms?). Why do we need to click Continue, then Authorize - we don't need to buttons to my mind there ... Let's please one more time review it.

@julieg18 we need to find a way to get rid of the pause at the end (feels like nothing is happening) or show some spinner (with some timeout). Otherwise it was a bit confusing.

Good stuff.

sroy3 · 2023-11-09T19:48:39Z

extension/src/setup/studio.test.ts

+import { Toast } from '../vscode/toast'
+
+const mockedFetch = jest.mocked(Fetch)
+const mockedFetchDefault = jest.fn()


Why not import fetch from 'node-fetch' and

const mockedFetch = jest.mocked(fetch) const mockedFetchDefault = jest.fn() mockedFetch.mockIplementation(() => mockedFetchDefault)

Hmmmm, this code gives me an error: mockedFetch.mockImplementation is not a function. Maybe it's because because fetch is a function, not a source object?

extension/src/setup/studio.ts

sroy3 · 2023-11-09T20:02:43Z

extension/src/test/suite/setup/index.test.ts

      const webview = await setup.showWebview()
      await webview.isReady()
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      stub(Setup.prototype as any, 'getCliCompatible').returns(true)


Why is this private in the first place?

private getCliCompatible() { return this.cliCompatible }

That does not seem to have any use since it's accessing a local variable. Is this code for tests only? Is there no other way to change the value? Why not set it public then? cc. @mattseddon (I think you're the one that added it)

Is there no other way to change the value?

Yes, that is the reason that I remember... https://github.com/iterative/vscode-dvc/pull/3768/files#r1178629676

sroy3 · 2023-11-09T20:07:15Z

extension/src/vscode/external.ts

+export const waitForUriResponse = (path: string, onResponse: () => unknown) => {
+  window.registerUriHandler({
+    handleUri(uri: Uri): ProviderResult<void> {
+      if (uri.path.startsWith(path)) {


decodeURI() does not work here?

sroy3 · 2023-11-09T20:12:15Z

webview/src/setup/components/studio/Connect.tsx

@@ -21,26 +18,13 @@ export const Connect: React.FC = () => {
          <a href="https://dvc.org/doc/studio/user-guide/projects-and-experiments/live-metrics-and-plots#set-up-an-access-token">
            access token
          </a>{' '}
-          generated from your Studio profile page.
+          generated from your Studio profile page. Request a token below or{' '}
+          <button className={styles.buttonAsLink} onClick={saveStudioToken}>


The style seems different than the previous link (buttonAsLink might need an update, underline vs none). Also, it feels weird to have this link here (before the "Get Token" button that is mentioned before), I feel like it should be a button on the same level as the "Get Token" one.

[Q] Why do we need this escape hatch? Will the endpoint always return a new token or does it fail if the user has one setup already?

Also, it feels weird to have this link here (before the "Get Token" button that is mentioned before), I feel like it should be a button on the same level as the "Get Token" one.

Makes sense, I can move it to a secondary button.

[Q] Why do we need this escape hatch? Will the endpoint always return a new token or does it fail if the user has one setup already?

Yes, the endpoint always creates a new one (your studio profile can hold multiple now) which I why I wanted to leave the option to add an already created one. If preferred, I can remove the option though if we want to keep onboarding as simplified as possible.

extension/src/setup/studio.test.ts

mattseddon · 2023-11-10T04:46:16Z

extension/src/setup/studio.ts

+      return fetchStudioToken(deviceCode, tokenUri, false)
+    }
+    return Toast.showError(
+      'Unable to get token. Please try again later or add an already created token.'


[C] IMO, this should be a modal. The user will have no chance of missing it.

[Q] Is there a reason to retry this? Have you seen the process work on these second try? If we only try to contact the server once, we can (easily) provide details of the failure based on the response we get. If the call is unstable then we should be working with the Studio team, not trying to patch it up.

[C] IMO, this should be a modal. The user will have no chance of missing it.

Good point!

[Q] Is there a reason to retry this? Have you seen the process work on these second try? If we only try to contact the server once, we can (easily) provide details of the failure based on the response we get. If the call is unstable then we should be working with the Studio team, not trying to patch it up.

Oh, you're right. When I first reviewed the Studio code I thought there was a chance of the token not being ready by the time the user is redirected, but I now see that this should work on the first try. I'll remove the retry attempt.

extension/src/setup/studio.ts

mattseddon · 2023-11-10T04:51:55Z

extension/src/test/suite/setup/index.test.ts

      const webview = await setup.showWebview()
      await webview.isReady()
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      stub(Setup.prototype as any, 'getCliCompatible').returns(true)


Is there no other way to change the value?

Yes, that is the reason that I remember... https://github.com/iterative/vscode-dvc/pull/3768/files#r1178629676

extension/src/vscode/external.test.ts

mattseddon · 2023-11-10T04:58:58Z

extension/src/vscode/external.test.ts

+  jest.resetAllMocks()
+})
+
+describe('openUrl', () => {


'should handle a message from the webview to open Studio' tested it indirectly (anything that used mockOpenExternal tested the call to the same level as this). It seems like you've deleted all of those tests which means mockOpenExternal can be removed from buildSetup.

mattseddon · 2023-11-10T05:09:09Z

webview/src/setup/components/studio/Connect.tsx

@@ -21,26 +18,13 @@ export const Connect: React.FC = () => {
          <a href="https://dvc.org/doc/studio/user-guide/projects-and-experiments/live-metrics-and-plots#set-up-an-access-token">
            access token
          </a>{' '}
-          generated from your Studio profile page.
+          generated from your Studio profile page. Request a token below or{' '}
+          <button className={styles.buttonAsLink} onClick={saveStudioToken}>


[Q] Why do we need this escape hatch? Will the endpoint always return a new token or does it fail if the user has one setup already?

julieg18 · 2023-11-10T21:57:18Z

Alright, resolved/responded to all comments. What's left:

Add some kind of sign (probably just a loading screen) to the Studio section when were fetching the token after the user has authorized (Add Studio Token Auth Flow #4931 (comment))

I'd like to do this one in a followup and merge this a first iteration on Monday :)

codeclimate · 2023-11-13T13:34:46Z

Code Climate has analyzed commit a4e1baa and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.2% (0.0% change).

View more on Code Climate.

wip demo

4097ca7

julieg18 added the product PR that affects product label Oct 30, 2023

julieg18 self-assigned this Oct 30, 2023

Update text

1c447a4

julieg18 mentioned this pull request Oct 30, 2023

Simplify Studio onboarding screen and workflow #3864

Closed

3 tasks

julieg18 added 4 commits October 31, 2023 09:02

Improve frontend components

1d257fb

Add frontend tests

7fd57d7

Remove unused code

40690ca

Clarify backend naming

e356aac

julieg18 added 2 commits November 2, 2023 15:38

Merge branch 'main' into add-studio-token-auth-flow

0c76c72

Move studio related code into its own module

1932709

codeclimate bot reviewed Nov 2, 2023

View reviewed changes

extension/src/setup/studio.ts Outdated Show resolved Hide resolved

julieg18 mentioned this pull request Nov 2, 2023

Add Setup Studio module #4943

Merged

julieg18 added 4 commits November 7, 2023 09:25

Start updating code for studio change

0eb294b

Merge branch 'main' into add-studio-token-auth-flow

f107be9

Remove need for user code

a8519ee

Finish broken test

b21bc81

julieg18 added 6 commits November 8, 2023 08:14

Remove need to verify page

8e43752

rename functions

ff0c070

Update tests

9a11485

Bit of cleanup

61f0882

Add temporary solution to windowId bug

4a34e10

Improve token fetch request

a53059f

julieg18 commented Nov 9, 2023

View reviewed changes

julieg18 marked this pull request as ready for review November 9, 2023 16:06

julieg18 requested review from mattseddon and sroy3 as code owners November 9, 2023 16:06

julieg18 changed the title ~~Add Studio Token Auth Flow (wip)~~ Add Studio Token Auth Flow Nov 9, 2023

julieg18 requested a review from shcheklein November 9, 2023 16:07

sroy3 approved these changes Nov 9, 2023

View reviewed changes

mattseddon approved these changes Nov 10, 2023

View reviewed changes

julieg18 added 3 commits November 10, 2023 11:29

Resolve most comments

203d364

Use modal instead of toast plus fix typo on test

465d52f

Remove unneeded styles imports

7286714

Merge branch 'main' into add-studio-token-auth-flow

a4e1baa

julieg18 enabled auto-merge (squash) November 13, 2023 13:34

julieg18 merged commit b1decdb into main Nov 13, 2023
3 checks passed

julieg18 deleted the add-studio-token-auth-flow branch November 13, 2023 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Studio Token Auth Flow #4931

Add Studio Token Auth Flow #4931

julieg18 commented Oct 30, 2023 •

edited

Loading

julieg18 Nov 9, 2023

mattseddon Nov 10, 2023

julieg18 Nov 9, 2023

julieg18 Nov 9, 2023

sroy3 Nov 9, 2023

julieg18 Nov 10, 2023 •

edited

Loading

shcheklein commented Nov 9, 2023

sroy3 Nov 9, 2023

julieg18 Nov 10, 2023

sroy3 Nov 9, 2023

mattseddon Nov 10, 2023

sroy3 Nov 9, 2023

sroy3 Nov 9, 2023

mattseddon Nov 10, 2023

julieg18 Nov 10, 2023 •

edited

Loading

mattseddon Nov 10, 2023

julieg18 Nov 10, 2023

mattseddon Nov 10, 2023

mattseddon Nov 10, 2023

mattseddon Nov 10, 2023

julieg18 commented Nov 10, 2023

codeclimate bot commented Nov 13, 2023

Add Studio Token Auth Flow #4931

Add Studio Token Auth Flow #4931

Conversation

julieg18 commented Oct 30, 2023 • edited Loading

Demo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

shcheklein commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 commented Nov 10, 2023

codeclimate bot commented Nov 13, 2023

julieg18 commented Oct 30, 2023 •

edited

Loading

julieg18 Nov 10, 2023 •

edited

Loading

julieg18 Nov 10, 2023 •

edited

Loading