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.

Sign up for GitHub

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

Jump to bottom

chore: integrate with fosite v0.40 (go-jose migration) #2526

Merged
merged 4 commits into from
May 31, 2021
Merged

chore: integrate with fosite v0.40 (go-jose migration) #2526

merged 4 commits into from
May 31, 2021

Conversation

narg95
Copy link
Contributor

@narg95 narg95 commented May 10, 2021

Related issue

Proposed changes

This PR shows the required changes to integrate Hydra with Fosite's PR #593

Checklist

Further comments

narg95
narg95 commented May 10, 2021
View reviewed changes
x/jwt.go Show resolved Hide resolved
@narg95 narg95 mentioned this pull request May 10, 2021
Merged
5 tasks
@narg95
Copy link
Contributor Author

narg95 commented May 10, 2021

If you are fine with those changes and once fosite's PR is merged, I can update this PR so that hydra can immediately integrate with latest fosite

aeneasr
aeneasr previously approved these changes May 11, 2021
View reviewed changes
@aeneasr
Copy link
Member

aeneasr commented May 13, 2021

Sorry, I always have to manually trigger the github actions CI pipeline to see if OIDC passes.

@narg95 narg95 changed the title chore: fosite go jose migration chore: integrate with fosite v0.40.0 (go-jose migration) May 22, 2021
@narg95 narg95 changed the title chore: integrate with fosite v0.40.0 (go-jose migration) chore: integrate with fosite v0.40 (go-jose migration) May 22, 2021
@narg95 narg95 mentioned this pull request May 22, 2021
Closed
@narg95 narg95 mentioned this pull request May 23, 2021
Merged
5 tasks
@narg95 narg95 marked this pull request as ready for review May 23, 2021 17:48
@narg95
Copy link
Contributor Author

narg95 commented May 23, 2021

@aeneasr once all checks pass this PR can be merge.
I will work in bringing consistency to timestamp claims in id_tokens next week.
Enjoy your holidays :)

@narg95 narg95 requested a review from aeneasr May 27, 2021 21:15
@aeneasr
Copy link
Member

aeneasr commented May 28, 2021

Thank you! I've merged the fosite PR and released it as v0.40.2

@aeneasr aeneasr mentioned this pull request May 28, 2021
Merged
3 tasks
@narg95
Copy link
Contributor Author

narg95 commented May 28, 2021

great!
I have updated fosite's dependency to v0.40.2 and rebased on master.
If checks pass this can be merged 🤞

@aeneasr aeneasr merged commit 5bdc4bc into ory:master May 31, 2021
mitar
mitar reviewed Jun 2, 2021
View reviewed changes
consent/strategy_default_test.go
@@ -27,7 +27,7 @@ import (
"net/http/httptest"
"testing"

jwtgo "github.com/dgrijalva/jwt-go"
jwtgo "github.com/ory/fosite/token/jwt"
"github.com/stretchr/testify/require"

"github.com/ory/fosite/token/jwt"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nicer to have just single import here, "github.com/ory/fosite/token/jwt", no need for both "github.com/ory/fosite/token/jwt" and jwtgo "github.com/ory/fosite/token/jwt".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, grouping of imports is now wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mitar mitar mitar left review comments

@aeneasr aeneasr aeneasr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@narg95 @aeneasr @mitar