-
Notifications
You must be signed in to change notification settings - Fork 20
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
feature/SDK-39_well-known-oidf #164
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #164 +/- ##
===========================================
- Coverage 49.11% 48.49% -0.63%
===========================================
Files 74 75 +1
Lines 4976 5114 +138
Branches 1731 1773 +42
===========================================
+ Hits 2444 2480 +36
- Misses 2529 2631 +102
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 Please be more careful with your imports. Mulltiple of them are off
2 OIDF has nothing todo with OID4VC, except for the fact it can be used as a registry for trust establishment. So it for sure does not belong in the VCI REST API or anything
@@ -24,6 +24,9 @@ import { DIDDocument } from 'did-resolver' | |||
import * as jose from 'jose' | |||
|
|||
import { generateDid, getIssuerCallbackV1_0_11, getIssuerCallbackV1_0_13, verifyCredential } from '../IssuerCallback' | |||
import { | |||
AuthorizationServerMetadataBuilder | |||
} from '@sphereon/oid4vci-issuer/dist/builder/AuthorizationServerMetadataBuilder' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} from '@sphereon/oid4vci-common' | ||
import { VcIssuer } from '@sphereon/oid4vci-issuer/dist/VcIssuer' | ||
import { CredentialSupportedBuilderV1_13, VcIssuerBuilder } from '@sphereon/oid4vci-issuer/dist/builder' | ||
import { | ||
AuthorizationServerMetadataBuilder | ||
} from '@sphereon/oid4vci-issuer/dist/builder/AuthorizationServerMetadataBuilder' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -12,6 +12,9 @@ import { | |||
URIState, | |||
} from '@sphereon/oid4vci-common' | |||
import { VcIssuer } from '@sphereon/oid4vci-issuer' | |||
import { | |||
AuthorizationServerMetadataBuilder | |||
} from '@sphereon/oid4vci-issuer/dist/builder/AuthorizationServerMetadataBuilder' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} | ||
router.get(WellKnownEndpoints.OAUTH_AS, authorizationServerHandler) | ||
|
||
const openidFederationHandler = (request: Request, response: Response) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a REST API implementation of a OID4VC protocol which has little relation to OIDF all of a sudden handling OIDF metadata and wellknowns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nklomp
The requested task was to host https://agent.wallet.bd.demo.sphereon.com/oid4vci/.well-known/openid-federation. Since the path was /oid4vci/* I assumed it was part of the issuer logic.
Plus it's already hosting /.well-known/openid-credential-issuer and /.well-known/oauth-authorization-server there, so it was the easiest place to do it.
Shall I create a separate oidf-metadata or something module for this in ssi-sdk?
packages/issuer/lib/VcIssuer.ts
Outdated
} from '@sphereon/oid4vci-common' | ||
import { CredentialEventNames, CredentialOfferEventNames, EVENTS } from '@sphereon/oid4vci-common' | ||
import { CredentialIssuerMetadataOptsV1_0_13 } from '@sphereon/oid4vci-common' | ||
import { OpenidFederationMetadata } from '@sphereon/oid4vci-common/dist/types/OpenidFederationMetadata' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be more careful with your imports. There are multiple occasions I have to point out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, but going to be removed
packages/issuer/lib/VcIssuer.ts
Outdated
@@ -45,6 +46,8 @@ import { CredentialDataSupplier, CredentialDataSupplierArgs, CredentialIssuanceI | |||
|
|||
export class VcIssuer<DIDDoc extends object> { | |||
private readonly _issuerMetadata: CredentialIssuerMetadataOptsV1_0_13 | |||
private readonly _authorizationServerMetadata: AuthorizationServerMetadata | |||
private readonly _openidFederationMetadata?: OpenidFederationMetadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be here
} from '@sphereon/oid4vci-common' | ||
import { CredentialIssuerMetadataOptsV1_0_13 } from '@sphereon/oid4vci-common' | ||
import { OpenidFederationMetadata } from '@sphereon/oid4vci-common/dist/types/OpenidFederationMetadata' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imports again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, but going to be removed
@@ -22,6 +24,8 @@ import { IssuerMetadataBuilderV1_13 } from './IssuerMetadataBuilderV1_13' | |||
export class VcIssuerBuilder<DIDDoc extends object> { | |||
issuerMetadataBuilder?: IssuerMetadataBuilderV1_13 | |||
issuerMetadata: Partial<CredentialIssuerMetadataOptsV1_0_13> = {} | |||
authorizationServerMetadata: Partial<AuthorizationServerMetadata> = {} | |||
openidFederationMetadata: Partial<OpenidFederationMetadata> = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be here
I wasn't done yet and also I did not review myself yet. I created a draft PR in IJ, but apparently it created a regular one. |
…wn-oidf # Conflicts: # packages/siop-oid4vp/lib/authorization-response/AuthorizationResponse.ts # packages/siop-oid4vp/lib/authorization-response/OpenID4VP.ts
No description provided.