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

Vendor: Upgrade to jwt-go 3.0.0 #229

Closed
leetal opened this issue Aug 22, 2016 · 10 comments
Closed

Vendor: Upgrade to jwt-go 3.0.0 #229

leetal opened this issue Aug 22, 2016 · 10 comments

Comments

@leetal
Copy link
Contributor

leetal commented Aug 22, 2016

This is essentially the same that have been done at fosite issue 76.

Basically the new version of jwt-go have better support for claims, and right now hydra won't build when using the head of fosite due to versioning clashes.

I will start fixing this tomorrow and supply a PR tomorrow evening.

@leetal
Copy link
Contributor Author

leetal commented Aug 23, 2016

@arekkas I think i have successfully updated jwt-go in hydra now as well to 3.0.0. All tests in the oauth2 package pass. BUT, the following commit 7faee6bb @ fosite really messes up the tests in hydra/warden. Thus, i cannot verify the changes i've made and i really think you should take a look at what that commit does to hydra.

@aeneasr
Copy link
Member

aeneasr commented Aug 23, 2016

ok, will do, thanks for the effort so far

@leetal
Copy link
Contributor Author

leetal commented Aug 23, 2016 via email

@aeneasr
Copy link
Member

aeneasr commented Aug 23, 2016

no i think it works fine, the test cases seem ok:

    scopes = []string{"hydra", "openid", "offline"}
    assert.False(t, strategy(scopes, "foo.bar"))
    assert.False(t, strategy(scopes, "foo"))
    assert.True(t, strategy(scopes, "hydra"))
    assert.True(t, strategy(scopes, "hydra.bar"))
    assert.True(t, strategy(scopes, "openid"))
    assert.True(t, strategy(scopes, "openid.baz.bar"))
    assert.True(t, strategy(scopes, "offline"))
    assert.True(t, strategy(scopes, "offline.baz.bar.baz"))

@aeneasr
Copy link
Member

aeneasr commented Aug 23, 2016

could you open a PR with your changes so i know which branch to look at?

@leetal
Copy link
Contributor Author

leetal commented Aug 23, 2016

On its way! Keep in mind that tests might/will fail. I cannot guarantee anything ;)

@aeneasr
Copy link
Member

aeneasr commented Aug 23, 2016

that's ok

@aeneasr
Copy link
Member

aeneasr commented Aug 24, 2016

resolved

@aeneasr aeneasr closed this as completed Aug 24, 2016
@mfzl
Copy link
Contributor

mfzl commented Aug 24, 2016

@arekkas @leetal I was in a hurry with this commit: ory/fosite@7faee6b

I realized after I pushed that it can be simplified to this:

if current != needle {
    break
}

@aeneasr
Copy link
Member

aeneasr commented Aug 24, 2016

Ah yes, if you want, create a PR

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

No branches or pull requests

3 participants