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.

Madhu94 closed this

Aug 2, 2017

Madhu94 reopened this

Aug 2, 2017

minrk reviewed

Aug 2, 2017

View reviewed changes

Copy link

Member

minrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks! I left a few comments inline.

notebook/services/contents/filemanager.py Outdated

		# We need this check in case the notebook is
		# trusted before save
		ifnotself.notary.check_signature(nb):
		self.check_and_sign(nb,path)

Copy link

Member

minrkAug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

check_signature shouldn't be called beforecheck_and_sign, which consumes trust metadata. I think.save probably doesn't need to change at all.

notebook/services/contents/handlers.py Outdated

		@json_errors
		@web.authenticated
		@gen.coroutine
		defput(self,path=''):

Copy link

Member

minrkAug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

POST is probably more appropriate for a request with no content.

notebook/services/contents/manager.py Outdated

		self.notary.sign(nb)
		else:
		self.log.warning("Saving untrusted notebook %s",path)
		self.log.warning("Signing notebook %s",path)

Copy link

Member

minrkAug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This message is backwards. In this event, the notebook isnot signed. It is still untrusted.

notebook/static/notebook/js/notebook.js Outdated

		window.location.reload();
		nb.contents.trust(nb.notebook_path)
		.then(function(res){
		nb.events.trigger("trust_changed.Notebook",true)

Copy link

Member

minrkAug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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).

Copy link

ContributorAuthor

Madhu94 commentedAug 2, 2017•
edited
Loading

@minrk Thanks for the feedback! Took care of those comments.

minrk reviewed

Aug 2, 2017

View reviewed changes

Copy link

Member

minrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for the quick turnaround!

notebook/services/contents/manager.py

		ifself.notary.check_cells(nb):
		self.notary.sign(nb)
		else:
		self.log.warning("Saving untrusted notebook %s",path)

Copy link

Member

minrkAug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Maybe leave this at "Notebook %s is not trusted."

notebook/static/notebook/js/notebook.js

		},function(err){
		console.log(err);
		});
		nb.save_notebook();

Copy link

Member

minrkAug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

varp;if (nb.writableandnb.dirty) {p=nb.save_notebook();}else {p=Promise.resolve();}p.then(function () {returnnb.contents.trust...

Copy link

ContributorAuthor

Madhu94Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

So if its read-only, we save it and then trust..Thanks, never noticed thedirty attribute.

Madhu94 closed this

Aug 2, 2017

Madhu94 reopened this

Aug 2, 2017

Copy link

ContributorAuthor

Madhu94 commentedAug 2, 2017•
edited
Loading

@minrk In addition to the changes you requested, I addedthis, I think the check makes sense here.

Copy link

Contributor

gnestor commentedAug 4, 2017

@minrk Do you think this ready to merge?

gnestor added component:Notebook enhancement labels

Aug 4, 2017

gnestor added this to the5.1 milestone

Aug 4, 2017

minrk reviewed

Aug 4, 2017

View reviewed changes

notebook/services/contents/manager.py Outdated

		self.notary.sign(nb)
		# We're checking the signature after the
		# trusted keys have been removed
		ifnotself.notary.check_signature(nb):

Copy link

Member

minrkAug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

minrk commentedAug 4, 2017

@gnestor almost. There's one addition in check_and_sign that should be reverted, but the rest should be set.

Copy link

ContributorAuthor

Madhu94 commentedAug 4, 2017•
edited
Loading

@minrk The waystore_signature for the signature store is implemented, the signaturesdon't get updated.
I logged an issue with nbformat, please let me know if I'm wronghere

Copy link

ContributorAuthor

Madhu94 commentedAug 4, 2017

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?