Fixes for persisting trust by joyceerhl · Pull Request #12950 · microsoft/vscode-python · GitHub

For #12933

Don't unnecessarily reorder the notebook json in model.getContent, it interferes with hashing (and also adds unnecessary noise to diffs). This will address this scenario
Use model.getContent for checking isNotebookTrusted to make sure we're loading and trusting the same thing always. This will address this scenario

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
Title summarizes what is changing.
Has a news entry file (remember to thank yourself!).
Appropriate comments and documentation strings in the code.
Has sufficient logging.
Has telemetry for enhancements.
Unit tests & system/integration tests are added/updated.
Test plan is updated as appropriate.
package-lock.json has been regenerated by running npm install (if dependencies have changed).
The wiki is updated with any design decisions/details.

Codecov Report

❗ No coverage uploaded for pull request base (master@9d2d4d2). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master   #12950   +/-   ##
=========================================
  Coverage          ?   60.01%           
=========================================
  Files             ?      672           
  Lines             ?    36590           
  Branches          ?     5135           
=========================================
  Hits              ?    21958           
  Misses            ?    13534           
  Partials          ?     1098

Impacted Files	Coverage Δ
...rc/client/datascience/notebookStorage/baseModel.ts	55.43% <100.00%> (ø)
...nt/testing/common/services/discoveredTestParser.ts	15.62% <0.00%> (ø)
...c/client/testing/common/services/contextService.ts	92.30% <0.00%> (ø)
.../interpreterSecurity/interpreterSecurityStorage.ts	97.50% <0.00%> (ø)
...application/diagnostics/commands/execVSCCommand.ts	100.00% <0.00%> (ø)
src/client/common/utils/text.ts	69.38% <0.00%> (ø)
src/client/testing/unittest/main.ts	70.58% <0.00%> (ø)
...ent/interpreter/display/shebangCodeLensProvider.ts	94.33% <0.00%> (ø)
src/client/common/experiments/manager.ts	95.94% <0.00%> (ø)
src/client/activation/jedi.ts	21.35% <0.00%> (ø)
... and 663 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d2d4d2...35abf6c. Read the comment docs.

In order to enforce setting and checking trust with the same contents, Rich Chiodo (@rchiodo) and I think it would be good to refactor trustNotebook and isNotebookTrusted to take INotebookModel as a sole parameter, and then grab the file URI and contents from the model. (Don Jayamanne (@DonJayamanne) I know you suggested doing this at the very beginning, so hopefully this is good news.)

This PR doesn't do that because we're releasing tomorrow and a major refactor is not strictly necessary to submit this bug fix; we just needed to construct the model, check trust with model.getContents, and update trust accordingly. I'll be submitting a separate PR for the refactor.

Why do we need to perform an update if the notebook is not trusted?

It starts out as trusted I believe.

In reply to: 454653784 [](ancestors = 454653784)

But we could skip this update if the notebook comes back as already trusted.

In reply to: 454655061 [](ancestors = 454655061,454653784)

Because the notebook trust is defaulted to true: https://github.com/microsoft/vscode-python/pull/12950/files#diff-9023bf28fe1ab2fbf6208e4cad576866R352

That's because it's going to be trusted anyway without calling isNotebookTrusted if contents === undefined || isUntitledFile(file) (which is what we did before this PR). And then if we actually need to check if the notebook is trusted, we update the model with the result of the actual check.

Yes but if the result comes back as trusted, you don't need to update the model.

Yes but the model is already trusted at that point. So updating it is redundant.

I'm suggesting this:

if (!(await this.trustService.isNotebookTrusted(file, contentsToCheck))) {
     model.update({
                source: 'user',
                kind: 'updateTrust',
                oldDirty: model.isDirty,
                newDirty: model.isDirty,
                isNotebookTrusted: false

}

Yeah those changes are already in there

🕐

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

* Only allow Enter / NumpadEnter w/o ctrl/shift/alt (#12939) * Send telemetry for notebook trust prompt selections (#12964) * Fixes for persisting trust (#12950)

* Update model.isTrusted on trust change (#12820) (#12823) * Reduce visual complexity of trust prompt (#12839) (#12847) * Port python 2.7 fix to release (#12877) * port color fix on collapse all (#12895) (#12897) * fix a color on collapse all (#12895) * update changelog * Merge fixes into July release (#12889) Co-authored-by: Timothy Ruscica <35348871+techwithtim@users.noreply.github.com> * Merge more fixes into july release (#12918) Co-authored-by: Joyce Er <joyceerhuiling@gmail.com> * Port trust fixes (#12929) * Fix regressions in trusted notebooks (#12902) * Handle trustAllNotebooks selection * Fix bug where after trusting, UI didn't update * Recover from ENOENT due to missing parent directory when trusting notebook (#12913) * Disable keydown on native cells in untrusted notebooks (#12914) * Hide editor icons when editor is not a notebook (#12934) (#12935) * Check for hideFromUser before activating current terminal (#12942) (#12956) * Check for hideFromUser before activating current terminal * Add tests * Tweak logic * Port final trust fixes for release (#12965) * Only allow Enter / NumpadEnter w/o ctrl/shift/alt (#12939) * Send telemetry for notebook trust prompt selections (#12964) * Fixes for persisting trust (#12950) * Display survey for native notebooks on/after 1st August (#12961) (#12975) Co-authored-by: Joyce Er <joyceerhuiling@gmail.com> Co-authored-by: Joyce Er <joyceerhuiling@gmail.com> * Contains cherry picks, version updates, change log updates (#12983) * Update version and change log * Improve detection when LS is fully loaded for IntelliCode (#12853) * Fix path * Actually fix settings * Add news * Add test * Format * Suppress 'jediEnabled' removal * Drop survey first launch threshold * Wait for client ready * Handle async dispose * Fix the date Co-authored-by: Mikhail Arkhipov <mikhaila@microsoft.com> * hide the gather button while a cell is executing (#12984) * Update date (#13002) * remove release notes from the start page (#13032) * Cherry pick, version change and change log update (#13079) * Ensure languageServer value is valid, send event during activate (#13064) * Update change log and version * Activate banner prompt for Pylance (#12817) * Fix path * Actually fix settings * Add news * Add test * Format * Suppress 'jediEnabled' removal * Drop survey first launch threshold * Remove LS experiments * Frequency + tests * Fix test * Update message to match spec * Open workspace for extension rather than changing setting * Fix localization string * Show banners asynchronously * Add experiments * Formatting * Typo * Put back verifyAll * Remove obsolete experiments, add Pylance * Suppress experiment if Pylance is installed * PR feedback Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com> * Update change log as per comments Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Co-authored-by: Mikhail Arkhipov <mikhaila@microsoft.com> * Port fix the gather survey (#13086) (#13105) * Fix the gather survey (#13086) * fix the gather survey added 'gather stats' telemetry mention the gather comments to update the python ext * oops * fix tests and address comments * update gather stats when resetting the kernel * set globalstate vars to 0 when we open vs code * fix gather stats telemetry * fix tests * fix tests for real Co-authored-by: Joyce Er <joyceerhuiling@gmail.com> Co-authored-by: Ian Huff <ianhu@microsoft.com> Co-authored-by: David Kutugata <dakutuga@microsoft.com> Co-authored-by: Don Jayamanne <don.jayamanne@yahoo.com> Co-authored-by: Timothy Ruscica <35348871+techwithtim@users.noreply.github.com> Co-authored-by: Mikhail Arkhipov <mikhaila@microsoft.com> Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>

Preserve json layout in getContent

35abf6c

Joyce Er (joyceerhl) added the no-changelog No news entry required label Jul 14, 2020

Joyce Er (joyceerhl) requested review from Don Jayamanne (DonJayamanne) and Rich Chiodo (rchiodo) July 14, 2020 18:11

Use getContent for checking isNotebookTrusted

83b4d49

Joyce Er (joyceerhl) requested review from David Kutugata (DavidKutu) and Ian Huff (IanMatthewHuff) July 14, 2020 18:20

Joyce Er (joyceerhl) marked this pull request as ready for review July 14, 2020 18:21

Rich Chiodo (rchiodo) reviewed Jul 14, 2020

View reviewed changes

Comment thread src/client/datascience/interactive-ipynb/nativeEditorStorage.ts Outdated

Ian Huff (IanMatthewHuff) approved these changes Jul 14, 2020

View reviewed changes

Don Jayamanne (DonJayamanne) requested changes Jul 14, 2020

View reviewed changes

Rich Chiodo (rchiodo) requested changes Jul 14, 2020

View reviewed changes

Don Jayamanne (DonJayamanne) reviewed Jul 14, 2020

View reviewed changes

Comment thread src/client/datascience/interactive-ipynb/nativeEditorStorage.ts Outdated

Joyce Er (joyceerhl) added 2 commits July 14, 2020 14:41

Fix comments

3ac4b9f

PR feedback

f6afa36

Joyce Er (joyceerhl) requested review from Don Jayamanne (DonJayamanne), Ian Huff (IanMatthewHuff) and Rich Chiodo (rchiodo) July 14, 2020 21:48

Rich Chiodo (rchiodo) approved these changes Jul 14, 2020

View reviewed changes

Don Jayamanne (DonJayamanne) approved these changes Jul 14, 2020

View reviewed changes

Joyce Er (joyceerhl) merged commit 9c31e46 into microsoft:master Jul 14, 2020

Joyce Er (joyceerhl) added a commit that referenced this pull request Jul 14, 2020

Fixes for persisting trust (#12950 )

5b78126

Joyce Er (joyceerhl) added a commit that referenced this pull request Jul 15, 2020

Port final trust fixes for release (#12965 )

cc412b1

* Only allow Enter / NumpadEnter w/o ctrl/shift/alt (#12939) * Send telemetry for notebook trust prompt selections (#12964) * Fixes for persisting trust (#12950)

Fixes for persisting trust by joyceerhl · Pull Request #12950 · microsoft/vscode-python · GitHub

Uh oh!

Fixes for persisting trust - #12950

Fixes for persisting trust#12950Joyce Er (joyceerhl) merged 4 commits intomicrosoft:mastermicrosoft/vscode-python:masterfrom joyceerhl:getcontentCopy head branch name to clipboard

Conversation

Joyce Er (joyceerhl) commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Codecov Comments Bot (codecov-commenter) commented Jul 14, 2020

Codecov Report

Uh oh!

Joyce Er (joyceerhl) commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Don Jayamanne (DonJayamanne) Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

Joyce Er (joyceerhl) Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

Joyce Er (joyceerhl) Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud Bot commented Jul 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fixes for persisting trust#12950
Joyce Er (joyceerhl) merged 4 commits into
microsoft:masterfrom
joyceerhl:getcontent

Joyce Er (joyceerhl) commented Jul 14, 2020 •
edited
Loading

Joyce Er (joyceerhl) commented Jul 14, 2020 •
edited
Loading

Joyce Er (joyceerhl) Jul 14, 2020 •
edited
Loading