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.

Sign up for GitHub

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

Jump to bottom

Feature: pass-by-value API #549

Open
kycutler opened this issue Oct 30, 2020 · 1 comment · May be fixed by #568
Open

Feature: pass-by-value API #549

kycutler opened this issue Oct 30, 2020 · 1 comment · May be fixed by #568

Comments

@kycutler
Copy link

I want to host nbdime as a multi-tenant, stateless REST service. However the diff & merge APIs currently only support local paths or URLs as arguments. This requires either uploading the files to the server or hosting them publicly, both of which I want to avoid.

Proposed change:

Allow the diff & merge apis to take notebook json as arguments; i.e. for /api/diff the request JSON would now be:

{
    "base":   "filename.ipynb" | "http://your-domain/url/path" | json_notebook,
    "remote": "filename.ipynb" | "http://your-domain/url/path" | json_notebook
}

If the endpoint receives a dictionary for any of the arguments, it would interpret it as a notebook and use that directly.

Some kind of --disallow-notebook-paths server flag to disable passing a path or URL to the API would be good too, for security purposes.

I'm willing to make the changes for this, just looking for any feedback first.
@vidartf
Copy link
Collaborator

vidartf commented Nov 10, 2020

Hi! I think this sounds like a good idea, but it would maybe be cleaner to make a separate handler and entry point instead of adding a flag, i.e.:

  • Make a new module in the webapp folder.
  • Add the new request handlers in that module (i.e. diff and merge api handler that only accept JSON).
  • Copy code from nbdimeserver submodule (or refactor relevant parts for reuse) to make a new server with the new handlers (and probably without the tool handlers).
  • Make the new server a separate entrypoint.

This way, the security can be more cleanly controlled (making sure we do not expose any files), and a different set of configurations can be applied to this server (what makes sense for a single user's one-off server probably doesn't make sense for a multi-tenant service).

That is just me thinking out loud, so if you have any thoughts or concerns, I'm happy to discuss (with or without a PR). 👍

@kycutler kycutler linked a pull request Mar 3, 2021 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add pass-by-value API server kycutler/nbdime
2 participants
@vidartf @kycutler