Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

register contents_manager.files_handler_class directly #2917

Merged
merged 1 commit into from Oct 11, 2017

Conversation

Projects
None yet
4 participants
@minrk
Copy link
Member

minrk commented Oct 10, 2017

rather than trying to call one handler from another, which is unreliable and can cause misbehavior.

closes jupyter/jupyter#292

Normally used when ContentsManager is not a FileContentsManager.
FileContentsManager subclasses use AuthenticatedFilesManager by default,
a subclass of StaticFileManager.

This comment has been minimized.

@takluyver

takluyver Oct 10, 2017

Member

"AuthenticatedFilesManager" and "StaticFileManager" should be "...Handler", I think?

This comment has been minimized.

@minrk

minrk Oct 11, 2017

Author Member

I got Managers on the mind. Fixed.

@@ -299,7 +299,8 @@ def init_handlers(self, settings):
handlers.extend(load_handlers('services.kernelspecs.handlers'))
handlers.extend(load_handlers('services.security.handlers'))
handlers.extend(load_handlers('services.shutdown'))

handlers.extend(settings['contents_manager'].get_extra_handlers())

This comment has been minimized.

@takluyver

takluyver Oct 10, 2017

Member

This is being added after the default files handler (the one which uses the contents manager). Doesn't that mean that this one (using static files) will never actually be used?

This comment has been minimized.

@minrk

minrk Oct 11, 2017

Author Member

I removed the global registration of the default files handler here, so this is where it gets registered, whichever class it is. On the base ContentsManager, it will be the generic FilesHandler, but on FileContentsManager subclasses it will be AuthenticatedFileHandler.

This comment has been minimized.

@minrk

minrk Oct 11, 2017

Author Member

The relevant change below is that the base ContentsManager has files_handler_class = FilesHandler by default, so get_extra_handlers always returns something unless a ContentsManager explicitly overrides files_handler_class = None, in which case the /files/ endpoint is disabled.

This comment has been minimized.

@takluyver

takluyver Oct 11, 2017

Member

Got it, thanks.

@takluyver takluyver added this to the 5.2 milestone Oct 10, 2017

@ebraminio

This comment has been minimized.

Copy link
Contributor

ebraminio commented Oct 10, 2017

Thank you @minrk, just for the sake of curiosity, do you know why this could cause more issue on Safari? Thanks

register contents_manager.files_handler_class directly
rather than trying to call one handler from another,
which is unreliable and can cause misbehavior.

@minrk minrk force-pushed the minrk:fix-auth-file-handler branch from 618b99e to a69ddb6 Oct 11, 2017

@minrk

This comment has been minimized.

Copy link
Member Author

minrk commented Oct 11, 2017

@ebraminio I have no idea why I can only reproduce this in Safari. My guess is that it's a coincidence of how it implements fetching and/or caching of requests that makes it more likely to trigger.

@@ -299,7 +299,8 @@ def init_handlers(self, settings):
handlers.extend(load_handlers('services.kernelspecs.handlers'))
handlers.extend(load_handlers('services.security.handlers'))
handlers.extend(load_handlers('services.shutdown'))

handlers.extend(settings['contents_manager'].get_extra_handlers())

This comment has been minimized.

@takluyver

takluyver Oct 11, 2017

Member

Got it, thanks.

@takluyver takluyver merged commit dd8b695 into jupyter:master Oct 11, 2017

4 checks passed

codecov/patch 100% of diff hit (target 0%)
Details
codecov/project 79.13% (+<.01%) compared to a8c6b8b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ebraminio

This comment has been minimized.

Copy link
Contributor

ebraminio commented Oct 11, 2017

Confirmed the fix, thanks!

@minrk minrk deleted the minrk:fix-auth-file-handler branch Oct 13, 2017

@gnestor gnestor modified the milestone: 5.2 Oct 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.