feat: add support for OIDC VC by hperl · Pull Request #3575 · ory/hydra

hperl · 2023-07-17T12:14:02Z

Related issue(s)

Requires ory/fosite#758

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

codecov · 2023-07-20T08:46:23Z

Codecov Report

Merging #3575 (88c7c93) into master (eb89af7) will decrease coverage by 0.07%.
The diff coverage is 74.00%.

❗ Current head 88c7c93 differs from pull request most recent head 949008f. Consider uploading reports for the commit 949008f to get more accurate results

@@            Coverage Diff             @@
##           master    #3575      +/-   ##
==========================================
- Coverage   76.39%   76.32%   -0.07%     
==========================================
  Files         130      132       +2     
  Lines        9746     9879     +133     
==========================================
+ Hits         7445     7540      +95     
- Misses       1799     1824      +25     
- Partials      502      515      +13

Files Changed	Coverage Δ
aead/xchacha20.go	`66.66% <0.00%> (-3.93%)`	⬇️
client/manager_test_helpers.go	`98.52% <ø> (ø)`
oauth2/handler.go	`67.56% <72.56%> (+0.42%)`	⬆️
x/int_to_bytes.go	`75.00% <75.00%> (ø)`
persistence/sql/persister_nonce.go	`80.95% <80.95%> (ø)`
driver/config/provider.go	`82.88% <100.00%> (+0.13%)`	⬆️
driver/config/provider_fosite.go	`82.60% <100.00%> (+0.79%)`	⬆️
driver/registry_base.go	`83.69% <100.00%> (ø)`
fositex/config.go	`86.11% <100.00%> (ø)`

alnr

Very nicely done!

aeneasr

Thank you! This looks pretty good. I think there's a bit of clean-up we need to do.

Also, should we add a proper end-to-end test in playwright/cypress for this?

alnr · 2023-07-31T14:13:04Z

Please re-run make sdk

hperl · 2023-08-01T12:30:27Z

Thanks for the review @aeneasr and @alnr! I'll clean up this PR and we should be ready for the final round.

I don't think we need Cypress/PW E2E tests, because the credentials endpoint is already fully tested and we would just repeat building the JWT proof in JavaScript.

hperl · 2023-08-02T08:06:08Z

I addressed all review comments now. Mainly I:

added swagger annotations and regenerated the SDK
added proper hints to all errors
refactored the nonce manager based on @alnr's suggestion to just encrypt the timestamp, which results in a more compact representation of the nonce

alnr · 2023-08-02T08:56:09Z

LGTM. Needs ory/fosite#758 to be merged first, right?

alnr · 2023-08-02T10:20:32Z

Just a tiny lint issue

hperl · 2023-08-02T11:37:26Z

LGTM. Needs ory/fosite#758 to be merged first, right?

There is a circular dependency for pushing Fosite and Hydra. Fosite uses Hydra for the OIDC conformancy tests (which fail on the Fosite PR), and will only pass with master Hydra when this PR here is merged.

So I suggest merging this first, then the Fosite PR, then remove the replace directive in Hydra.

aeneasr

Very nice, just one minor swagger comment ...

This adds initial support for issuing verifiable credentials as specified in https://openid.net/specs/openid-connect-userinfo-vc-1_0.html. Because the spec is still in draft, public identifiers are suffixed with `draft_00`.

hperl self-assigned this Jul 17, 2023

hperl force-pushed the hperl/oidc-vc branch 2 times, most recently from a895998 to 2046994 Compare July 19, 2023 12:21

hperl marked this pull request as ready for review July 19, 2023 12:22

hperl requested a review from aeneasr as a code owner July 19, 2023 12:22

hperl force-pushed the hperl/oidc-vc branch 3 times, most recently from b9e59dd to 9f112ef Compare July 20, 2023 06:51

hperl commented Jul 20, 2023

View reviewed changes

Comment thread oauth2/oauth2_auth_code_test.go Outdated

hperl commented Jul 20, 2023

View reviewed changes

Comment thread oauth2/handler.go

hperl commented Jul 20, 2023

View reviewed changes

Comment thread persistence/sql/persister_nonce_test.go Outdated

hperl force-pushed the hperl/oidc-vc branch 2 times, most recently from e5f2c1a to 515ffb4 Compare July 20, 2023 08:26

hperl force-pushed the hperl/oidc-vc branch 4 times, most recently from 6c7748c to c1a8d58 Compare July 20, 2023 11:23

aeneasr reviewed Jul 24, 2023

View reviewed changes

Comment thread oauth2/handler.go Outdated

hperl force-pushed the hperl/oidc-vc branch 4 times, most recently from aa05e95 to 1270c1a Compare July 25, 2023 10:56

hperl requested review from aeneasr and alnr July 26, 2023 07:52

hperl commented Jul 31, 2023

View reviewed changes

Comment thread oauth2/handler.go Outdated

hperl force-pushed the hperl/oidc-vc branch from 1270c1a to 6404b55 Compare July 31, 2023 11:28

alnr reviewed Jul 31, 2023

View reviewed changes

Comment thread oauth2/handler.go Outdated

alnr reviewed Jul 31, 2023

View reviewed changes

Comment thread oauth2/handler.go Outdated

alnr reviewed Jul 31, 2023

View reviewed changes

Comment thread oauth2/handler.go Outdated

alnr reviewed Jul 31, 2023

View reviewed changes

Comment thread persistence/sql/persister_nonce.go

alnr reviewed Jul 31, 2023

View reviewed changes

Comment thread oauth2/oauth2_auth_code_test.go Outdated

alnr requested changes Jul 31, 2023

View reviewed changes

aeneasr reviewed Jul 31, 2023

View reviewed changes

hperl force-pushed the hperl/oidc-vc branch from 6404b55 to 39a6eba Compare August 2, 2023 08:06

hperl requested review from aeneasr and alnr August 2, 2023 08:06

hperl force-pushed the hperl/oidc-vc branch from 39a6eba to 523eb2e Compare August 2, 2023 11:23

hperl force-pushed the hperl/oidc-vc branch from 523eb2e to 676b788 Compare August 2, 2023 12:13

alnr previously approved these changes Aug 2, 2023

View reviewed changes

aeneasr reviewed Aug 3, 2023

View reviewed changes

Comment thread oauth2/handler.go Outdated

hperl dismissed alnr’s stale review via b76f06d August 4, 2023 07:53

hperl force-pushed the hperl/oidc-vc branch from 676b788 to b76f06d Compare August 4, 2023 07:53

feat: add support for OIDC VC

949008f

This adds initial support for issuing verifiable credentials as specified in https://openid.net/specs/openid-connect-userinfo-vc-1_0.html. Because the spec is still in draft, public identifiers are suffixed with `draft_00`.

hperl force-pushed the hperl/oidc-vc branch from b76f06d to 949008f Compare August 4, 2023 07:54

aeneasr approved these changes Aug 4, 2023

View reviewed changes

aeneasr merged commit 219a7c0 into master Aug 4, 2023

aeneasr deleted the hperl/oidc-vc branch August 4, 2023 08:57

Conversation

hperl commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issue(s)

Checklist

Further Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alnr left a comment

Choose a reason for hiding this comment

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alnr commented Jul 31, 2023

Uh oh!

hperl commented Aug 1, 2023

Uh oh!

hperl commented Aug 2, 2023

Uh oh!

alnr commented Aug 2, 2023

Uh oh!

alnr commented Aug 2, 2023

Uh oh!

hperl commented Aug 2, 2023

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hperl commented Jul 17, 2023 •

edited

Loading

codecov Bot commented Jul 20, 2023 •

edited

Loading