search

Sphereon-Opensource / SIOP-OID4VP

Self Issued OpenID Provider v2 (SIOP) with optional OpenID for Verifiable Presentations (OpenID4VP)
77 stars 25 forks source link

WIP: first batch of suggestions #15

Closed sksadjad closed 2 years ago

sksadjad commented 2 years ago

this can't be merged in it's current form. it's created just to start the conversation about our next set of changes

sksadjad commented 2 years ago

@nklomp just for later reference: looked for these fields in these docs: https://openid.net/specs/openid-connect-4-verifiable-presentations-1_0-00.html (20 May 2021) https://openid.net/specs/openid-connect-4-verifiable-presentations-1_0-04.html (9 September 2021) https://openid.net/specs/openid-connect-4-verifiable-presentations-1_0-06.html (10 December 2021) https://openid.net/specs/openid-connect-4-verifiable-presentations-1_0-07.html (17 December 2021) https://openid.net/specs/openid-connect-4-verifiable-presentations-1_0-08.html (28 January 2022) https://openid.net/specs/openid-connect-4-verifiable-presentations-1_0-10.html (22 April 2022) https://openid.net/specs/openid-4-verifiable-presentations-1_0.html (1_0-12 - 21 June 2022) and couldn't find the values that we were talking about. as you can see from the version 1_0-00 to 1_0-12 there are some revisions missing, I couldn't find them anywhere to compare the mattrglobal version is https://mattrglobal.github.io/oidc-client-bound-assertions-spec/ (20 April 2021) which is older than the first version that I have access to right now

codecov-commenter commented 2 years ago

Codecov Report

Merging #15 (3016c72) into develop (ee38791) will increase coverage by 0.52%. The diff coverage is 92.61%.

@@             Coverage Diff             @@
##           develop      #15      +/-   ##
===========================================
+ Coverage    84.95%   85.47%   +0.52%     
===========================================
  Files           25       25              
  Lines         1050     1150     +100     
  Branches       257      284      +27     
===========================================
+ Hits           892      983      +91     
- Misses         158      163       +5     
- Partials         0        4       +4
Flag Coverage Δ
unittest 85.47% <92.61%> (+0.52%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/functions/DidJWT.ts 76.52% <ø> (ø)
...c/main/schemas/AuthenticationRequestOpts.schema.ts 100.00% <ø> (ø)
.../main/schemas/AuthenticationResponseOpts.schema.ts 100.00% <ø> (ø)
src/main/OP.ts 86.30% <50.00%> (-2.28%) :arrow_down:
src/main/RP.ts 88.46% <50.00%> (-3.38%) :arrow_down:
src/main/functions/DidSiopMetadata.ts 87.50% <86.48%> (-1.98%) :arrow_down:
src/main/RPBuilder.ts 90.47% <96.61%> (+4.76%) :arrow_up:
src/main/AuthenticationRequest.ts 85.22% <100.00%> (ø)
src/main/AuthenticationRequestRegistration.ts 85.00% <100.00%> (-1.37%) :arrow_down:
src/main/AuthenticationResponse.ts 81.51% <100.00%> (ø)
... and 9 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

sksadjad commented 2 years ago

Hey @nklomp if you agree with the content, I can start to make it a nicer pr, right now, I'm just trying to make it work, and I'm passing all the previous tests

nklomp commented 2 years ago

Some small remarks left

nklomp commented 2 years ago

One comment lef, which is simply removing the did:ethr from that exact location as it was demo code