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

Add ability to inject extra claims and xsrfToken #248

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

Add ability to inject extra claims and xsrfToken #248

wants to merge 6 commits into from

Conversation

lwcolton
Copy link

@lwcolton lwcolton commented Apr 2, 2016

JWT generation functions were moved into the JWTFactory class.
The JWTRequestAuthenticator class was added,and was made the
parent class of ApiRequestAuthenticator, OAuthRequestAuthenticator,
and OAuthClientCredentialsRequestAuthenticator. These three
authenticator classes now allow specifying arbitrary key-value
pairs to add in the JWT claims. The also now have support for
auto-generating a uuid4 to be used as a CSRF token in the JWT,
as described in the "Cookies" section of
https://stormpath.com/blog/where-to-store-your-jwts-cookies-vs-html5-web-storage/

Colton Leekley-Winslow added 6 commits April 2, 2016 17:56
JWT generation functions were moved into the JWTFactory class.
The JWTRequestAuthenticator class was added,and was made the
parent class of ApiRequestAuthenticator, OAuthRequestAuthenticator,
and OAuthClientCredentialsRequestAuthenticator.  These three
authenticator classes now allow specifying arbitrary key-value
pairs to add in the JWT claims.  The also now have support for
auto-generating a uuid4 to be used as a CSRF token in the JWT,
as described in the "Cookies" section of
https://stormpath.com/blog/where-to-store-your-jwts-cookies-vs-html5-web-storage/
@lwcolton
Copy link
Author

lwcolton commented Apr 7, 2016

Codacy is mad because I duplicated code in my test functions. I'm more interested to here what @rdegges or someome from SP thinks of the changes

@rdegges
Copy link
Contributor

rdegges commented Apr 7, 2016

Thanks for the PR! Going to review this as soon as I have a chance. I've been out on vacation, and am heading to a conference in NYC for a few days. Sorry for the delay :(

@rdegges
Copy link
Contributor

rdegges commented Jun 13, 2016

Hey @lwcolton, this looks really cool! The refactoring in here is much appreciated.

I realize it's taken me way longer than I would have liked to review this, but I wanted to leave some comments.

  • I don't think we want to have the xsrf stuff implemented in the Python SDK. All web frameworks tend to implement this differently, and I don't want this logic embedded in the underlying Python SDK as it should remain independent to a web framework's concerns.

Other than that, everything looks great.

I'm happy to selectively include things from the PR if you want, or you can update it to remove the XSRF stuff yourself and I'll merge directly.

Just let me know! If I don't hear back, I'll just go ahead and merge in the bits we'll use (minus the XSRF stuff).

Thanks! <3333

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.

2 participants