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

auth/jwt: prevent concurrent reads and writes on MapClaims #564

Merged
merged 5 commits into from
Jul 17, 2017

Conversation

peterbourgon
Copy link
Member

Closes #562.

@peterbourgon peterbourgon changed the title auth/jwt: MapClaims: failing test auth/jwt: prevent concurrent reads and writes on MapClaims Jul 1, 2017
@litriv
Copy link
Contributor

litriv commented Jul 10, 2017

@peterbourgon looks like the test is in place and working (failing). What do you think of passing a claim factory to NewParser, that can be be used to create a new jwt.Claims, as in

func NewParser(keyFunc jwt.Keyfunc, method jwt.SigningMethod, newClaims func() claims jwt.Claims) endpoint.Middleware {
    return func(next endpoint.Endpoint) endpoint.Endpoint {
        return func(ctx context.Context, request interface{}) (response interface{}, err error) {
    ...
    token, err := jwt.ParseWithClaims(tokenString, newClaims(), func(token *jwt.Token) 
    (interface{}, error) {
        ...
    })
    ...
}

If agreed, then I will make the change, test my side and commit to this pull request if you are too busy?

@peterbourgon
Copy link
Member Author

Yes, sorry, I got the failing test but then failed to find the time to fix it ;)

That's exactly what's needed. Probably best to define a new type ClaimFactory func(...) jwt.Claim, take one in the constructor, and probably provide a default EmptyClaimFactory implementation — or whatever would be most appropriate, I'm not a JWT expert.

@litriv
Copy link
Contributor

litriv commented Jul 10, 2017

@peterbourgon sounds about right. I'm on it 👍

@litriv
Copy link
Contributor

litriv commented Jul 11, 2017

@peterbourgon this is now fixed. how shall i go about getting the changes to the repo? probably easiest if you can give me temporary write access so i can just push to this branch?

@basvanbeek
Copy link
Member

why not just fork it to your account and submit a new PR referencing this one?

add claimsFactory type
make NewParser take a claimsFactory instead of an instance of jwt.Claims
use claimsFactory to create a jwt.Claims to pass in to jwt.ParseWithClaims
update NewParser calls in tests to take a claimsFactory instead of a jwt.Claims instance
@litriv
Copy link
Contributor

litriv commented Jul 11, 2017

@basvanbeek i created a pull request to merge with this branch, but if it's better to create one for merging with master, then let me know and i'll do it. thanks

@B3rs
Copy link

B3rs commented Jul 17, 2017

Hi @peterbourgon, I just encountered the same problem, any clue on when this PR will be merged into master? thanks

@peterbourgon
Copy link
Member Author

How about now?

@peterbourgon peterbourgon merged commit 0d70b13 into master Jul 17, 2017
@peterbourgon peterbourgon deleted the issue-562 branch July 17, 2017 16:23
jamesgist pushed a commit to jamesgist/kit that referenced this pull request Nov 1, 2024
auth/jwt: prevent concurrent reads and writes on MapClaims
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.

4 participants