Skip to content
This repository has been archived by the owner on May 26, 2020. It is now read-only.

Allow using a cookie as a transport for the token #275

Merged

Conversation

moises-silva
Copy link
Contributor

A new configuration 'JWT_AUTH_COOKIE' has been added. If it set
to None (the default), everything works the same.

It can be set to a string to use as the name of the cookie that
can be used to carry the token. Note the 'Authorization' header is
still accepted as the primary method, but it falls back to the
designated cookie if present.

When a token is requested and 'JWT_AUTH_COOKIE' is set, the response
will set the given cookie with the token string.

Fix #120

A new configuration 'JWT_AUTH_COOKIE' has been added. If it set
to None (the default), everything works the same.

It can be set to a string to use as the name of the cookie that
can be used to carry the token. Note the 'Authorization' header is
still accepted as the primary method, but it falls back to the
designated cookie if present.

When a token is requested and 'JWT_AUTH_COOKIE' is set, the response
will set the given cookie with the token string.

Fix jpadilla#120
@moises-silva moises-silva force-pushed the feature/jwt-auth-cookie-issue-120 branch from b9125a6 to 947dde2 Compare October 24, 2016 21:58
@genomics-geek
Copy link

Any plans for this to be included in a release?

@angvp
Copy link
Contributor

angvp commented Feb 28, 2017

@jpadilla ^ ?

@orf
Copy link
Contributor

orf commented Feb 28, 2017

I'm personally against including this, as storing a JWT in a cookie kind of defeats the point of using a JWT. It's usually an either-or situation, JWT's get around some of the limitations of cookies.

@moises-silva
Copy link
Contributor Author

@orf As mentioned by someone else on issue #120, the JWT spec doesn't mandate a specific transport for the token, and it's left open so you can use whatever transport makes sense for your application. Yes, the most common way to transmit the token is in the Authorization header, but JWT's are way more about signing and validating the claims than the transport used. The transport is largely irrelevant (hence, barely mentioned in the spec as a secondary matter), and supporting cookies as a transport makes sense for applications that have to interface with other backends that only support cookies but support some sort of auth gateway plugin (such as nginx http auth). I've described my use case in detail on issue #120 that I won't repeat the specifics here.

@moises-silva
Copy link
Contributor Author

@orf Heh, re-reading issue #120 I see on April last year you seemed to have a clear use case for this (similar-ish to mine). If this were a largely invasive PR I think it'd make more sense to have it as a separate package, but I don't think we want to end up as the nodejs community with 3 line packages, do we? ;)

@orf
Copy link
Contributor

orf commented Feb 28, 2017

That's a good point, I forgot about that! We worked around that issue, but I guess the use case is valid in some cases. @jpadilla what do you think?

@jpadilla
Copy link
Owner

Thanks for bumping this and the reminder. After reading through #120 I think we can just merge this in.

@moises-silva thanks for putting this together. The only thing we're probably missing here are a few tests to avoid breaking support in the future.

As a quick note, anyone that wants to step up and help out with maintenance will be greatly appreciated.

@moises-silva
Copy link
Contributor Author

@jpadilla Thanks, I'll see if I can put some tests together next week and submit a new PR for those.

@jpadilla
Copy link
Owner

jpadilla commented Mar 1, 2017

@moises-silva sounds good, will merge this now, thanks again!

@jpadilla jpadilla merged commit 9b9735c into jpadilla:master Mar 1, 2017
@genomics-geek
Copy link

genomics-geek commented Mar 1, 2017

Awesome! thanks @jpadilla and @moises-silva

@jpadilla jpadilla added this to the 1.10 milestone Mar 22, 2017
@jpadilla
Copy link
Owner

Just released this under v1.10. Thank you all!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants