[WIP] Add extra check for dashboards behind auth #229

viniciusdc · 2022-04-29T17:52:53Z

closes #225

This does not solve #190 completely but presents a second validation step using only a single fetch which enables the request to be performed in the same browser session.

There are some points here that I would like to point out:

I am adding an extra condition here to check for gateway in the URL, but that's not ideal. I would like to have another opinion on which mechanic should be used here. I originally thought of passing an extra setting to the lab extension config to enable/disable this optional validation...
My knowledge of TS/JS is very premature, so I just added this simple branch here. But I think the best approach should have been something like an Interceptor to verify the response and then move accordingly to that...

Well, this issue is fixed in our case with these changes, but I would love to know what I can do to improve this contribution and have such a feature added to main 😄

ian-r-rose

Thanks for this @viniciusdc, and sorry for the slow review! The logic looks sound to me (though I haven't been able to test this on a QHub-like deployment). My main request is that we expose this as a setting rather than a "gateway" URL check.

But I think the best approach should have been something like an Interceptor

I'm not familiar with this, can you elaborate?

src/dashboard.tsx

viniciusdc · 2022-05-09T19:12:51Z

Hi @ian-r-rose thanks for the comments above, I tried to add what you suggested, but I am not sure how to incorporate the changes made to DaskDashboardLauncher.IOptions into the testDaskDashboard new parameter.

ian-r-rose

Thanks @viniciusdc! Left a few style comments, but I think those could mostly be fixed by getting the pre-commit hooks working.

Regarding passing the option down to the dashboard check: perhaps my earlier suggestion of using an additional option in an IOption object was overkill, we could also just have a setter on the URLInput class (since that is the class that actually checks the URL). It's still an unfortunate number of steps, but one fewer than what I suggested before :). So it would look something like

in onSettingsChanged read the new setting value
Update that value using a new setter on URLInput
The URLInput object then passes the setting value into the dashboard check utility function.

schema/plugin.json

src/dashboard.tsx

schema/plugin.json

src/index.ts

viniciusdc · 2022-05-11T19:18:08Z

Thanks @viniciusdc! Left a few style comments, but I think those could mostly be fixed by getting the pre-commit hooks working.

Regarding passing the option down to the dashboard check: perhaps my earlier suggestion of using an additional option in an IOption object was overkill, we could also just have a setter on the URLInput class (since that is the class that actually checks the URL). It's still an unfortunate number of steps, but one fewer than what I suggested before :). So it would look something like

in onSettingsChanged read the new setting value

Update that value using a new setter on URLInput

The URLInput object then passes the setting value into the dashboard check utility function.

Hi @ian-r-rose, many thanks for the explanation. I tried to implement these changes (I think I understand a little bit more how this works 🙂 ). Let me know your thoughts about this. Thanks in advance

ian-r-rose

This is looking pretty good @viniciusdc, I just have one request to add get accessor, and I think this will be good to go!

src/dashboard.tsx

ian-r-rose · 2022-05-16T15:55:02Z

src/dashboard.tsx

@@ -301,9 +301,11 @@ export class URLInput extends Widget {
  /**
   * The in browser dashboard check for authenticated dashboards.
   */
+  get browserDashboardCheck(): boolean {
+    return this.browserDashboardCheck;


These should be underscored attributes to not shadow the method names

uhm I see, I was not sure about how to use _ in there. Thanks @ian-r-rose. So, just to be sure, I only need to change the methods inside the setter/getter, do I also need to change inside the class as well?

Sorry to be terse! Typically what we do is add a private attribute (see the bottom of this class) which is prefixed with an underscore. It's a bit Java-ey, but hopefully not too bad.

So this line would become

return this._browserDashboardCheck;

and further down with the other private attributes you would have

private _browserDashboardCheck = false;

Cool, just added that. Let me know if its correct 🙂

ian-r-rose

This looks great, thanks for all your work on this @viniciusdc! Looks like pre-commit is unhappy, but for a reason unrelated to this PR, so I'll go ahead and fix that myself.

@viniciusdc, can I ask you to confirm that the PR in this state still works for your use-case, since I'm not set up to test it myself?

Also, cc @jsignell, who has requested a similar option in the past.

viniciusdc · 2022-05-16T18:06:52Z

This looks great, thanks for all your work on this @viniciusdc! Looks like pre-commit is unhappy, but for a reason unrelated to this PR, so I'll go ahead and fix that myself.

@viniciusdc, can I ask you to confirm that the PR in this state still works for your use-case, since I'm not set up to test it myself?

Also, cc @jsignell, who has requested a similar option in the past.

Thanks @ian-r-rose for the review, I am currently testing it now will let you know once I finish. Just I question, where should I modify in the config the new attribute to propagate it?

ian-r-rose · 2022-05-16T18:19:20Z

where should I modify in the config the new attribute to propagate it?

As a user, you should be able to go to the JupyterLab settings editor (Settings -> Advanced Settings Editor) and find the "Dask" tab. That will open a JSON editor in which you can change the settings. These settings are stored in JSON files on disk (you can find the path to those files if you want to manually edit them by running jupyter lab paths. That's probably enough for our purposes here.

As a system admin, you'll want to distribute a settings file to your users that have the appropriate values for these settings. That way they won't need to change the settings themselves. This functionality is documented here.

ian-r-rose · 2022-05-16T18:43:18Z

Whoops, found one more place where we need to pass this._browserDashboardCheck to testDaskDashboard, mind if I push here @viniciusdc?

viniciusdc · 2022-05-16T18:44:16Z

Hi @ian-r-rose, so I was able to toggle the options and I can see the table

the only problem is that it loses connection right after that

The connection to dask dashboard https://qhubstages2.qhub.dev/gateway/clusters/dev.6f863607b3684dd1b8191cf207877cd2/ has been lost

ian-r-rose · 2022-05-16T18:44:54Z

Okay, this looks good to me if you are happy with it @viniciusdc

viniciusdc · 2022-05-16T18:45:27Z

I will check again with this addition, thanks for updating it 🙂

ian-r-rose · 2022-05-16T18:45:36Z

the only problem is that it loses connection right after that

Hmm, weird. Do you see anything in the Network tab in your browser dev tools?

viniciusdc · 2022-05-16T18:48:45Z

the only problem is that it loses connection right after that

Hmm, weird. Do you see anything in the Network tab in your browser dev tools?

I can see two responses, one from individual-plots.json which contains the json:

Request URL: https://qhubstages2.qhub.dev/gateway/clusters/dev.6f863607b3684dd1b8191cf207877cd2/individual-plots.json
Request Method: GET
Status Code: 304 
Remote Address: 35.225.180.207:443
Referrer Policy: strict-origin-when-cross-origin

the second one though

https://qhubstages2.qhub.dev/user/test-user/dask/dashboard-check/https%3A%2F%2Fqhubstages2.qhub.dev%2Fgateway%2Fclusters%2Fdev.6f863607b3684dd1b8191cf207877cd2%2F?1652726806120
Request Method: GET
Status Code: 200 
Remote Address: 35.225.180.207:443
Referrer Policy: strict-origin-when-cross-origin

with the following body

isActive: false
plots: {}
url: "https://qhubstages2.qhub.dev/gateway/clusters/dev.6f863607b3684dd1b8191cf207877cd2/"

ian-r-rose · 2022-05-16T18:53:09Z

Getting two different responses suggests to me that the issue is with having two different checks to testDaskDashboard, one with true, one with false. Maybe you need to rebuild the extension with my last commit?

viniciusdc · 2022-05-16T18:54:38Z

Getting two different responses suggests to me that the issue is with having two different checks to testDaskDashboard, one with true, one with false. Maybe you need to rebuild the extension with my last commit?

Sure, I think that might be the case as well, rebuilding now.

viniciusdc · 2022-05-16T19:11:43Z

Hi @ian-r-rose, that did the trick. Thank you, its working flawlessly 😄

ian-r-rose · 2022-05-16T19:13:30Z

Hi @ian-r-rose, that did the trick. Thank you, its working flawlessly smile

Great, in it goes, thanks @viniciusdc!

viniciusdc · 2022-05-16T19:15:46Z

Thanks for all the help with this @ian-r-rose!! Hope to contribute more in the future with you all!

ian-r-rose · 2022-06-21T23:32:08Z

Thanks again for this @viniciusdc! I've just published version 5.3.0 of dask-labextension which includes this feature. Please let me know if any follow-up work is needed!

viniciusdc added 7 commits March 23, 2022 10:59

Extends dashboard eval

93b879f

Extends dashboard eval

257cb95

New attempt

e33e9d4

Add debug option

a126c85

Add elif condition for gateway

d9e6835

Remove duplicate json read

13d489f

clean

b1d6788

ian-r-rose reviewed May 4, 2022

View reviewed changes

src/dashboard.tsx Outdated Show resolved Hide resolved

src/dashboard.tsx Outdated Show resolved Hide resolved

viniciusdc added 2 commits May 9, 2022 16:10

Add new option to schema

d116dc8

Add testing option to fetch dashboard url

acd957b

viniciusdc requested a review from ian-r-rose May 10, 2022 20:40

ian-r-rose reviewed May 10, 2022

View reviewed changes

schema/plugin.json Outdated Show resolved Hide resolved

schema/plugin.json Outdated Show resolved Hide resolved

src/dashboard.tsx Outdated Show resolved Hide resolved

schema/plugin.json Show resolved Hide resolved

src/index.ts Outdated Show resolved Hide resolved

viniciusdc added 3 commits May 11, 2022 16:02

Add browserDashboardCheck to URLInput

c163859

Update descriptions to trigger pre-commit

9b61ddc

Update key description to target usecase

ee910dd

viniciusdc requested a review from ian-r-rose May 11, 2022 19:18

ian-r-rose reviewed May 12, 2022

View reviewed changes

src/dashboard.tsx Show resolved Hide resolved

src/dashboard.tsx Outdated Show resolved Hide resolved

Add getter for new option and rm this.update

0efcc94

ian-r-rose reviewed May 16, 2022

View reviewed changes

viniciusdc added 2 commits May 16, 2022 13:40

underscored attributes

b7c059b

rm console debug msg

fad23ea

ian-r-rose approved these changes May 16, 2022

View reviewed changes

Ian Rose added 2 commits May 16, 2022 11:21

Bump node version in CI

fb507ce

Bump black

8fde45b

Missed a spot where we need to pass `_browserDashboardCheck through.

2e6c33e

ian-r-rose merged commit fc8de03 into dask:main May 16, 2022

viniciusdc mentioned this pull request May 30, 2022

[bug] Unable to connect to dashboard using Dask-Gateway and Labextension nebari-dev/nebari#1012

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add extra check for dashboards behind auth #229

[WIP] Add extra check for dashboards behind auth #229

viniciusdc commented Apr 29, 2022 •

edited

Loading

ian-r-rose left a comment

viniciusdc commented May 9, 2022 •

edited

Loading

ian-r-rose left a comment

viniciusdc commented May 11, 2022

ian-r-rose left a comment

ian-r-rose May 16, 2022

viniciusdc May 16, 2022 •

edited

Loading

ian-r-rose May 16, 2022

viniciusdc May 16, 2022

ian-r-rose left a comment

viniciusdc commented May 16, 2022 •

edited

Loading

ian-r-rose commented May 16, 2022

ian-r-rose commented May 16, 2022

viniciusdc commented May 16, 2022

ian-r-rose commented May 16, 2022

viniciusdc commented May 16, 2022

ian-r-rose commented May 16, 2022

viniciusdc commented May 16, 2022 •

edited

Loading

ian-r-rose commented May 16, 2022

viniciusdc commented May 16, 2022

viniciusdc commented May 16, 2022

ian-r-rose commented May 16, 2022

viniciusdc commented May 16, 2022

ian-r-rose commented Jun 21, 2022

[WIP] Add extra check for dashboards behind auth #229

[WIP] Add extra check for dashboards behind auth #229

Conversation

viniciusdc commented Apr 29, 2022 • edited Loading

ian-r-rose left a comment

Choose a reason for hiding this comment

viniciusdc commented May 9, 2022 • edited Loading

ian-r-rose left a comment

Choose a reason for hiding this comment

viniciusdc commented May 11, 2022

ian-r-rose left a comment

Choose a reason for hiding this comment

ian-r-rose May 16, 2022

Choose a reason for hiding this comment

viniciusdc May 16, 2022 • edited Loading

Choose a reason for hiding this comment

ian-r-rose May 16, 2022

Choose a reason for hiding this comment

viniciusdc May 16, 2022

Choose a reason for hiding this comment

ian-r-rose left a comment

Choose a reason for hiding this comment

viniciusdc commented May 16, 2022 • edited Loading

ian-r-rose commented May 16, 2022

ian-r-rose commented May 16, 2022

viniciusdc commented May 16, 2022

ian-r-rose commented May 16, 2022

viniciusdc commented May 16, 2022

ian-r-rose commented May 16, 2022

viniciusdc commented May 16, 2022 • edited Loading

ian-r-rose commented May 16, 2022

viniciusdc commented May 16, 2022

viniciusdc commented May 16, 2022

ian-r-rose commented May 16, 2022

viniciusdc commented May 16, 2022

ian-r-rose commented Jun 21, 2022

viniciusdc commented Apr 29, 2022 •

edited

Loading

viniciusdc commented May 9, 2022 •

edited

Loading

viniciusdc May 16, 2022 •

edited

Loading

viniciusdc commented May 16, 2022 •

edited

Loading

viniciusdc commented May 16, 2022 •

edited

Loading