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

Initial webservice #1

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Initial webservice #1

wants to merge 8 commits into from

Conversation

bradtaylor
Copy link
Owner

This is a large PR representing the bulk of the work setting up the service, including the routing, authentication, and notebook conversion. The readme has info on the choice of web framework.

There is still some housekeeping to be done- adding docstrings to the utility methods and creating API documentation.

I also need to create the app configuration for deploying the service on Google App Engine.

bradtaylor added 8 commits June 2, 2019 21:26
Set up a small sanic app with two routes:
1. A status route that returns 200 and the text representation ('OK')
2. A stub for the notebook convert endpoint. Right now it returns static HTML. Later it will return the result of a notebook->html conversion
Import the nbconvert library and programmatically perform the request json -> html conversion

I tested this manually using a few notebooks, including tables and images.
output was visually equivalent in the browser to nbconvert on command line, and to Calhoun
Update route decorators to shorthand versions
Update handler names to match Calhoun, as these get picked up as default route names
Added some information regarding the project description, choice of framework, and background. Will add configuration and deployment once I've worked on those features.
In Calhoun, the service this is based on, the /api/convert endpoint is authorized via a call to the SAM authorization service that powers Terra. I'm not entirely sure this is necessary- the user is only getting access to a reformated version of data they are sending in. Perhaps checking that the user is registered is a way of protecting from automated DOS attacks. Perhaps it was done to avoid changes to the security boundary for compliance. I will follow up here. For now, I am calling the same authorization service endpoint.

Added basic call to the SAM authorization service.
I don't love the elifs for handling the response out of SAM. Better error handling TBD

Also TBD is parameterizing the SAM root from config.

Updated readme
More effectively handling scenarios where the user is unauthorized, not enabled in Terra, otherwise Forbidden, or there is a problem contacting the SAM authorization service.
Passing in SAM_ROOT from a config file, to facilitate different environments.

Simplified error handling in authorization functions to make use of Sanic's built in exception handling
Authorization methods are only meant to be called by the 'authorized' decorator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant