Allow notebooks to be trusted without triggering a save #2718
Conversation
| # We need this check in case the notebook is | ||
| # trusted before save | ||
| if not self.notary.check_signature(nb): | ||
| self.check_and_sign(nb, path) |
There was a problem hiding this comment.
check_signature shouldn't be called before check_and_sign, which consumes trust metadata. I think .save probably doesn't need to change at all.
| @json_errors | ||
| @web.authenticated | ||
| @gen.coroutine | ||
| def put(self,path=''): |
There was a problem hiding this comment.
POST is probably more appropriate for a request with no content.
| @@ -443,7 +443,7 @@ def check_and_sign(self, nb, path=''): | |||
| if self.notary.check_cells(nb): | |||
| self.notary.sign(nb) | |||
| else: | |||
| self.log.warning("Saving untrusted notebook %s", path) | |||
| self.log.warning("Signing notebook %s", path) | |||
There was a problem hiding this comment.
This message is backwards. In this event, the notebook is not signed. It is still untrusted.
| window.location.reload(); | ||
| nb.contents.trust(nb.notebook_path) | ||
| .then(function(res) { | ||
| nb.events.trigger("trust_changed.Notebook", true) |
There was a problem hiding this comment.
it should still trigger reload to get the newly trusted notebook without sanitized output.
And .save should only be avoided when the notebook is read-only. We should still save & reload for non-read-only notebooks to avoid losing unsaved changes (which is unavoidable for read-only).
|
@minrk Thanks for the feedback! Took care of those comments. |
| @@ -443,7 +443,9 @@ def check_and_sign(self, nb, path=''): | |||
| if self.notary.check_cells(nb): | |||
| self.notary.sign(nb) | |||
| else: | |||
| self.log.warning("Saving untrusted notebook %s", path) | |||
There was a problem hiding this comment.
Maybe leave this at "Notebook %s is not trusted."
| }); | ||
| nb.save_notebook(); |
There was a problem hiding this comment.
Can you still trigger save here if the notebook is not read-only? That way we can avoid losing unsaved changes when the notebook is editable.
var p;
if (nb.writable and nb.dirty) {
p = nb.save_notebook();
} else {
p = Promise.resolve();
}
p.then(function () {
return nb.contents.trust...There was a problem hiding this comment.
So if its read-only, we save it and then trust..Thanks, never noticed the dirty attribute.
|
@minrk Do you think this ready to merge? |
| self.notary.sign(nb) | ||
| # We're checking the signature *after* the | ||
| # trusted keys have been removed | ||
| if not self.notary.check_signature(nb): |
There was a problem hiding this comment.
I think this entire method should be left exactly as it was. Adding this check here is not correct because it prevents updating the signatures of trusted notebooks.
|
@gnestor almost. There's one addition in check_and_sign that should be reverted, but the rest should be set. |
|
I think this is a bug even on master. I noticed that without the check, the same signature was re-inserted each time the notebook was trusted. @minrk, I probably shouldn't check this here - the issue should be fixed in nbformat. So, I'll revert that change, squash my commits and then we're good to go? |
|
@Madhu94 exactly right. This shouldn't be changed here, but fixed in nbformat itself. Thanks for digging in! |
Madhu94
force-pushed
the
trust_without_save
branch
from
fb47c20
to
8abd7b9
Compare
Madhu94
force-pushed
the
trust_without_save
branch
from
8abd7b9
to
2208917
Compare
|
@minrk I rebased and squashed my commits and reverted that check in |
|
@minrk this is, I think, the last remaining PR for 5.1. Is it ready to go? |
|
Yes, indeed. |
|
Thanks, @Madhu94! |
|
Can I add this to the 5.1 changelog ? |
|
Yes, please do! |
Closes #195.
Trust should now add the notebook's signature without needing to trigger a save. Save will still trust the notebook, after checking and if it has not already been trusted.
I have not added an automatic refresh after trust, let me know if you think this would be a good idea.