jupyter / nb2kg

Other
73 stars 31 forks source link

KG_HEADERS Should allow for 'Authorization': <token_value> and not hard code 'token' in the <token_value> #28

Closed chuyqa closed 5 years ago

chuyqa commented 5 years ago

Regarding the KG_HEADERS:

KG_HEADERS = json.loads(os.getenv('KG_HEADERS', '{}'))
KG_HEADERS.update({
    'Authorization': 'token {}'.format(os.getenv('KG_AUTH_TOKEN', ''))
})

For common JWT Header based authentication, the format for the token will be

{'Authorization':'Bearer <actual_token>'}
{'Authorization':'Bearer eyJ1....oWSQ'}

The problem with nb2kg's setup for these headers is the 'token string will always be prepended.

kevin-bates commented 5 years ago

@chuyqa - thanks for the issue. I know nothing about websocket authorization other than the google I just performed. :smile:

Since there will be others possibly using the current form, would it make sense to introduce a new variable - KG_BEARER_TOKEN that is then inserted into 'Bearer <bearer_token>' just as KG_AUTH_TOKEN is inserted into 'token <auth_token>'?

If so, we'd look for `KG_BEARER_TOKEN first, then fallback to the current functionality if not present.

I'm not in a position to test this kind of thing out. Would you be able to make this change?

Once the change is made here, we'll want to update the embedded version in notebook. The env names are different and it supports command-line arguments.

chuyqa commented 5 years ago

One interesting part here is that if KG_BEARER_TOKEN is not set, we'll still set the headers to 'Authorization': 'token '. This would need a bit of refactoring If we go the route of

As we'd still need to adjust KG_AUTH_TOKEN to only update if set.

Given that only 1 Authorization: <> can be present in the Headers, what are your thoughts on an approach where we make KG_HEADERS slightly more flexible and allow it to pass in Authorization without it being over-written, while keeping the existing functionality if KG_HEADERS does not include Authorization?

PR with this approach https://github.com/jupyter/nb2kg/pull/30

kevin-bates commented 5 years ago

Closing via #30