Render widgets on all clients in collaborative mode

[trungleduc]

References

Fixes #11435

Code changes

• Add a new signal displayModelRequested in CodeCellModel, this signal is emitted when a display_data message is processed.
• Attach a trust button to the cell output area when a display_data message from other clients is processed.
• Make the trust icon in the status bar clickable, which will open a dialog asking users for the trust status of the current notebook.

User-facing changes

• When a widget display command is executed in one client, all other clients will display the "plain/text" version of the output with a button that allows switching to the rich mime types.

widget.mp4

• The trust shield status bar indicator is now a button, users can open a dialog to set the trust status of the notebook:
  • Selecting "No" will untrust all contents of the notebook.
  • Selecting "Yes" will trust all contents of the notebook.
  • Selecting "Always" will trust all current contents and new contents coming from other users.

shield-button.mp4

Backwards-incompatible changes

N/A

[jupyterlab-probot]

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

[hbcarlos]

Thanks, @trungleduc! LGTM!

[github-actions]

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

The mean relative comparison is computed with 95% confidence.

Results table

Test file	large_code_notebook	large_md_notebook
open
chromium
actual	4218 <- [4402 - 4547 - 5462] -> 12527	2754 <- [2831 - 2888 - 3106] -> 3832
expected	4238 <- [4371 - 4463 - 5413] -> 7385	2735 <- [2808 - 2904 - 3044] -> 3543
Mean relative change	3.5% ± 5.1%	1.1% ± 1.7%
switch-from
chromium
actual	681 <- [745 - 774 - 888] -> 1175	444 <- [500 - 568 - 591] -> 949
expected	655 <- [738 - 770 - 837] -> 1218	443 <- [483 - 562 - 588] -> 698
Mean relative change	1.4% ± 4.2%	2.3% ± 3.4%
switch-to
chromium
actual	726 <- [760 - 776 - 817] -> 993	543 <- [621 - 632 - 643] -> 1219
expected	723 <- [753 - 767 - 794] -> 1011	536 <- [615 - 624 - 632] -> 1613
Mean relative change	1.3% ± 1.9%	0.7% ± 3.9%
close
chromium
actual	728 <- [806 - 833 - 1133] -> 1262	524 <- [546 - 561 - 608] -> 700
expected	737 <- [791 - 836 - 1152] -> 1261	518 <- [534 - 550 - 608] -> 998
Mean relative change	-0.7% ± 5.2%	1.2% ± 2.5%

Changes are computed with expected as reference.

[hbcarlos]

@jtpio Can you take a look and merge?

[fcollonval]

I find the per output prompt far too invasive. As a user I would expect that if I trust or untrust a document, but I do see the point of trusting only some outputs.

So I would suggest getting a status only for the all notebook. This brings me to the fact that we already have a trust status on the notebook. So should we not revisit it to deal with real-time collaboration.

I will bring this up at the next meeting.

[trungleduc]

Hi @fcollonval, I initially started with only one prompt for trusting the notebook but then I realized in the context of RTC, we can trust the contents of the notebook at a moment but can not be sure about the contents coming after from other users.

Btw I'm happy to update the PR after the conclusion of the next meeting.

[jtpio]

I will bring this up at the next meeting.

We can point folks to try it out with the Binder link on this PR since it has RTC enabled.

[fcollonval]

People have nice idea in the yesterday meeting for the UI:

• [Pete] origin of Colab's iframe outputs. rather than a dialog could it render the non-interactive outputs (text/plain) for remote outputs, with a button to allow switching to the rich mime types?
• [Tony] accessibility?
• [Sylvain] a shield thing by untrusted output
• [Mike] There is already a similar pattern with trust button for the html viewer.

The idea would be to display the "plain/text" version of the output with a button (?) that allow to load the more complex view if the user trust the output. It would be good to have a way to set it up for the full notebook though - maybe extending the trust shield status bar indicator.

[fcollonval]

And read the trust feature description in notebook documentation. I'm convinced that the RTC should not be considered as a special case and should follow that feature behavior.

[krassowski]

And read the trust feature description in notebook documentation. I'm convinced that the RTC should not be considered as a special case and should follow that feature behavior.

I politely disagree. We should treat it as a special case because in a sessions run by teachers/instructors for students we should not expose everyone in the class to a potential jokingly intended attack from one of the participants. Maybe this is something for which there should be roles, like only certain users have the rights to render widgets as trusted by default, and all others result in the plaintext + "I trust this output" shield button (the rationale is that the output presented interactively by the instructor/host should be trusted but the output from other participants should not; once the host trusts that output it would be trusted for everyone). Is this technically feasible to identify the host of the meeting right now, or is this something which will be only possible once the user package is in?

Also, the preview could be an isolated iframe instead of the plaintext.

[trungleduc]

I agree with @krassowski on this point, but since we haven't got the RBAC yet I think it will be difficult to identify the host of a session. @hbcarlos may have more insight about it?

I like the idea about the per-output trust button but we should have also some way to trust every output. I'm thinking about this scenario:

 When I connect to a session with other clients already connected, I get a prompt for trusting all outputs.
 If I say no, I will get the "plain/text" version with a button to render it later.

[fcollonval]

True I forget the sync is two ways...

For me the problem of the host is not related to the trust but as mentioned @trungleduc to sharing rights (that could already be implemented as they don't need to know about the user - they should be part of the shared session when created).

If I compare with VS Code Live Share feature (in which you can share a terminal), from the host point of view you specify the right for the sharing resources (read-only, read and write). In the case of the notebook we certainly need more rights levels (adding the execution right for instance). And we should avoid having too complex states. But it definitely seems we should reboot the RTC meeting to discuss that kind of scenarii and how to address them (for instance at first being able to support a case like once the host trusts that output it would be trusted for everyone sounds to me going against what is proposed here as this is the owner of the client that says if he trusts the output or not).

[hbcarlos]

Is this technically feasible to identify the host of the meeting right now, or is this something which will be only possible once the user package is in?

I'm not 100% sure, but I don't think we can identify the host, not even with the user package in. The only way would be to have RBAC and only users with ¿ADMIN or RUN? roles can render the outputs without asking for trust.

[fcollonval]

I have some questions but mostly the code looks good.

[trungleduc]

@fcollonval Thanks for the review! Suggestions were applied in the last commit.

[trungleduc]

Switched to draft since I'm having some issues with ipywidgets 8

[trungleduc]

With the changes in ipywidgets 8, cell output always has the cell number on the left, so I need to move the trust button to the far right of the output area.

widget.mp4

[trungleduc]

@fcollonval can you re-run the failed tests? I can not reproduce the failed tests on my PC.

[SylvainCorlay]

This looks good to me !

[davidbrochart]

Should this approach be generalized to all outputs (not only widgets), and become the new trust mechanism?

[Carreau]

Should this approach be generalized to all outputs (not only widgets), and become the new trust mechanism?

Definitely no for me, this is a completely different trust mechanism with completely different goals than the current one. This "new" trust mechanism does not consider sharing in different ways than with RT collaborations, and running headless. The current trust mechanism, is made for exchange of notebooks that will potentially be ran for example via nbconvert, or on a batch script.

A local trust to each cell/output does not work.

For example looking at local trust:

 - cd repo
 - cd cache
 - rm -rf *
 - cd .. 
 - cd .. 

An d assume I trust this notebook, and each cell is "trusted" independently.
An attacker get this notebook, and reorder it, not changing the new "trust" status:

 - cd .. 
 - cd .. 
 - rm -rf *
 - cd repo
 - cd cache

And now my $HOME is gone if I run "ipython my_notebook.ipynb". This is just an example, and when we designed the trust mechanism circa 2014, we found others examples where a set of multiple trusted and harmless items independently can become harmful together.

This is why the trust is global, and attached to the serialised version of the notebook via signature.

[echarles]

Should this approach be generalized to all outputs (not only widgets), and become the jupyter-server/jupyter_server#891 (comment)?

I don't fully yet get sense on that "new trust mecanism" looking at this PR. However, I am sure that any such change in the jupyter security paragdim should be further more discussed, brainstormed and approved from many jupyter subproject, and for sure by the jupyter security team.

My first feedback is that if any new approach lowers down the security/trust guards, it should be a no-go.

[davidbrochart]

From @krassowski's comment:
"I think that you misunderstood what the trust feature does. It is not there to protect from say malicious Python code. It is there to protect from consequences of automatic execution of JavaScript and SVG."

Which is in contradiction with your example @Carreau.

[Carreau]

1. It depends how you look at it.

2. I gave a simple Python example, bout you could craft similar one in JS output, they are just more complicated to read. Or as I said in other places, we have seen attacks that rely on injecting things in prompt numbers (that actually can be strings as * is valid), notebook metadata, title .... and many other things.

For 1) Really it is there to protect against crafted notebook and unintended execution. In current notebook this happend to be JS/SVG. And it also happens that JS/SVG is sometime hidden.

The current notebook decides to treat a valid and properly signed notebook to a notebook were each cell is marked as "trusted", and load it. But by default the notebook don't run Python code when loaded – therefore there is no need to use trust for this. But this is also reflected by the fact that a notebook is trusted if and only if all python cells, even if they have no output have been executed by current user.

Though you can't design the jupyter trust mechanism to take into account only one frontend that happen to use Yjs and RTC, you should consider all other frontend (vs code, nbviewer, nbconvert, whatever), that may in the end take other decisions.

And it is not necessary that there is a single "trust" mechanism.

From what I can tell, there are two cases:

A) load from storage (you want to make sure this is globally trusted/not tampered with).

B) trust across peers in with YJS. I see both as complementary.

And really A) is closer to any other application trusting/signing that you see on windows/mac/linux. Either you "ask the user" (which to click on "trust"), or you want some form of certificate/singing, which currently we only do locally with current user key.

[ellisonbg]

A few questions and thoughts:

• I reviewed Fix notebook trust in RTC #12756, which would make cell-trust part of the shared model. I think the translation of notebook trust in the RTC setting is "we all trust this code when any of us trust this code." If a collaborator is not trusted at that level, I don't believe they should have execute access to that shared JupyterLab to begin with.
• I wouldn't use the notebook trust abstraction that we have to indicate if a specific human collaborator should be trusted. Anytime you allow for RTC, participants can misbehave, even without code execution. RBAC is the right tool to solve that, by giving a user the appropriate level of access based on their role.
• I am not sure if this current PR makes sense if Fix notebook trust in RTC #12756 is merged. Would that other PR ensure that all outputs would show for all users once one of them runs (and trusts) the cell?
• I am concerned that this PR introduces a secondary trust abstraction, but doesn't tie that back to who ran the cell. If we were going to ask uses to do this type of approval ("someone ran this cell, do you want to view the output") I think we should show the identity of the person that ran the cell.

[trungleduc]

@ellisonbg I'm completely in line with the direction of #12756. Closing this PR in favor of the other solution.