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

wip: pre-authorized_code flow #375

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jessevanmuijden
Copy link
Contributor

@jessevanmuijden jessevanmuijden commented Oct 22, 2024

The intention of this draft PR is to provide a basis for collecting feedback on how to fit the pre-authorized_code flow implementation into the existing architecture of wwwallet. I have commented on the challenges I faced on the files of the PR and summarized my challenges below the video. Some refactoring of the wwwallet architecture may be needed to support pre-authorized_code flow. It is probably best to split the problem up into smaller problems that are easy to solve separately. I may have broken the existing flows in this draft, which needs to be addressed.

Feature

As Martin Jørgensen from MBO Beek
I want to save an Academic Base Credential into my wwWallet
In order to prove my affiliation with MBO Beek to other organizations

Given I am logged in as Martin Jørgensen to my Eduwallet account on https://mbob.idp.dev.eduwallet.nl/simplesaml/module.php/profile on my desktop
And I am logged in to my wwWallet account on my smartphone
When I scan the QR code on my Eduwallet profile page with the wwWallet
Then my Academic Base Credential issued by MBO Beek appears in my wwWallet

Demo of feature in action

pre-authorized_code.mov

Challenges

  • useCheckUrl.ts acts like a router and contains quite a bit of logic. The pre-authorized_code flow may be easier to integrate if some of the logic is moved to a functional component.
  • The application now errors if the credential does not contain a credentialImageSvgTemplateURL or a credentialImageURL, or has no credentialImage at all. Also, the credentialImage can reside under credential.vc.credentialImage if the VC has a different format. More flexibility in retrieving the credentialImage as well as a fallback image would be helpful.
  • The credential fields to render in the credential info is hardcoded. While it is fine to have hard-baked templates for some credentials, it would be nice to have a default rendering method for any other credentials based on the credential definition that can be found here: https://agent.dev.eduwallet.nl/mbob/.well-known/openid-credential-issuer, e.g. parsedCredential?.vc?.credentialSubject
  • The credentialParserRegistry now only has a parser for the vc+sd-jwt format. It should intelligently parse the jwt_vc_json format if applicable.
  • Somehow, credentialConfigurationId and credentialIssuerIdentifier fields are not stored in the IndexedDB, while they are stored on demo.wwwallet.org. Having the credentialIssuerIdentifier would be very helpful for displaying the credential based on credential configuration fetched from the issuer's configuration endpoint.
  • With pre-authorized-code flow, the credential can be issued to wwwallet in a simple procedure, with zero configuration, as is required for the other example issuers. This allows us to experiment with any issuer, even when their configuration is not stored in the database of the wallet-backend-server. I don't know how to marry this with the current application architecture for authorization code flow that calls handleCredentialOffer, which is strongly designed toward Authorization, on openID4VCIClients from the container that are constructed based on configured issuers fetched from the database, while I would like to use some of the dependencies that these clients are composed with.
  • The subsequent fetches in the pre-authorized code flow implementation can be abstracted out to an OpenId4VC client, but we already have one, so adding more methods to that interface may get confusing. We could refactor to separate clients for pre-authorized code flow and authorization code flow.
  • credential_configurations_supported is an array of one or more supported formats. A chain pattern may be appropriate for deciding which supported format to use, since this affects subsequent processing of the credential.
  • The cryptographic_binding_methods_supported of the issuer https://agent.dev.eduwallet.nl/mbob/.well-known/openid-credential-issuer are ["did:jwk", "did:key"], so the credential only considered the jwt verified with a kid header with the did as a value. This means that we should have different ways to construct jwts depending on the cryptographic_binding_methods_supported of the issuer.

Additional notes

  • After scanning the QR code the wwWallet React app flashes 5 times. This UX can be improved by refactoring the components such that fewer re-renders are triggered
  • When the URL contains a credential offer, the issuer configuration of all configured issuers are fetched. It could be more efficient to fetch only the issuer configuration from the /well-known/ endpoint of the issuer in the credential offer.
  • Indenting with spaces instead of tabs would make the PRs on GitHub easier to read. Github displays tabs as 8 spaces.

) : parsedCredential && parsedCredential.credentialImage.credentialImageURL && (
<img src={parsedCredential.credentialImage.credentialImageURL} alt={"Credential"} className={className} onClick={onClick} />
) : (
<img src={parsedCredential?.credentialImage?.credentialImageURL || 'https://picsum.photos/300/200' } alt={"Credential"} className={className} onClick={onClick} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The application now errors if the credential does not contain a credentialImageSvgTemplateURL or a credentialImageURL, or has no credentialImage at all. Also, the credentialImage can reside under credential.vc.credentialImage if the VC has a different format. More flexibility in retrieving the credentialImage as well as a fallback image would be helpful.

@@ -86,6 +86,11 @@ const CredentialInfo = ({ credential, mainClassName = "text-sm lg:text-base w-fu
{renderRow('grade', 'Grade', parsedCredential?.grade, screenType)}
{renderRow('id', 'Social Security Number', parsedCredential?.ssn, screenType)}
{renderRow('id', 'Document Number', parsedCredential?.document_number, screenType)}

Copy link
Contributor Author

@jessevanmuijden jessevanmuijden Oct 22, 2024

Choose a reason for hiding this comment

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

The credential fields to render in the credential info is hardcoded. While it is fine to have hard-baked templates for some credentials, it would be nice to have a default rendering method for any other credentials based on the credential definition that can be found here: https://agent.dev.eduwallet.nl/mbob/.well-known/openid-credential-issuer. Also, the credential format needs to be taken into account, e.g. parsedCredential?.vc?.credentialSubject

@@ -10,7 +10,7 @@ const CredentialJson = ({ credential, textAreaRows='10' }) => {

useEffect(() => {
if (container) {
container.credentialParserRegistry.parse(credential).then((c) => {
container.credentialParserRegistry.parse(credential.vc || credential).then((c) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The credentialParserRegistry now only has a parser for the vc+sd-jwt format. It should intelligently parse the jwt_vc_json format if applicable.

@@ -32,6 +42,10 @@ export const parseSdJwtCredential = async (credential: string | object): Promise
}

}

return {
beautifiedForm: parseJwt(credential),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably be moved to a separate parser for the jwt_vc_json format, please advise.

const credential = await credentialResponse.json();

await storeCredential({
credentialConfigurationId: did.id,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow, credentialConfigurationId and credentialIssuerIdentifier fields are not stored in the IndexedDB, while they are stored on demo.wwwallet.org. Having the credentialIssuerIdentifier would be very helpful for displaying the credential based on credential configuration fetched from the issuer's configuration endpoint.

@@ -41,7 +44,88 @@ function useCheckURL(urlToCheck: string): {
throw new Error("User handle could not be extracted from keystore");
}
const u = new URL(urlToCheck);
if (u.protocol === 'openid-credential-offer' || u.searchParams.get('credential_offer') || u.searchParams.get('credential_offer_uri') ) {
if (u.searchParams.get('credential_offer_uri') ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This if-statement breaks the existing implementation, but I used it to finalize the implementation in its purest form following the path of least resistance. This helps to demonstrate that with pre-authorized-code flow, the credential can be issued to wwwallet in a simple procedure, with zero configuration, as is required for the other example issuers.

},
});
const openIdCredentialIssuer = await openIdCredentialIssuerResponse.json();
// const metadata = OpenidCredentialIssuerMetadataSchema.parse(openIdCredentialIssuer);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The schema parser cannot parse the jwt_vc_json format. We could make the parser more flexible or use different schema parsers depending on the format.

Comment on lines +48 to +49
const credentailOfferResponse = await fetch(u.searchParams.get('credential_offer_uri'));
const credentialOffer = await credentailOfferResponse.json();
Copy link
Contributor Author

@jessevanmuijden jessevanmuijden Oct 22, 2024

Choose a reason for hiding this comment

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

This and subsequent fetches can be abstracted out to an OpenId4VC client, but we already have one, so adding more methods to that interface may get confusing. We could refactor to separate clients for pre-authorized code flow and authorization code flow.

// const metadata = OpenidCredentialIssuerMetadataSchema.parse(openIdCredentialIssuer);
const metadata = openIdCredentialIssuer; // @todo: investigate schema errors

const generateNonceProof = async (cNonce: string, audience: string, clientId: string): Promise<{ jws: string }> => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is copy-pasted from an injected dependency. It would be better to inject the same dependency into some sort of PreAuthorizedCodeFlowClient.

return { jws: proof_jwt };
};

const storeCredential = async (c: StorableCredential) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also copy-pasted from an injected dependency. It would be better to inject the same dependency into some sort of PreAuthorizedCodeFlowClient.

'proof_type': 'jwt',
'jwt': jws,
},
format: VerifiableCredentialFormat.JWT_VC_JSON, // @todo: retrieve from credential_configurations_supported
Copy link
Contributor Author

Choose a reason for hiding this comment

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

credential_configurations_supported is an array of one or more supported formats. A chain pattern may be appropriate for deciding which supported format to use, since this affects subsequent processing of the credential.

'jwt': jws,
},
format: VerifiableCredentialFormat.JWT_VC_JSON, // @todo: retrieve from credential_configurations_supported
...(metadata.credential_configurations_supported.AcademicBaseCredential), // @todo: retrieve from credential_configurations_supported
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fetched dynamically, but is now hardcoded for the demo.


if (!credentialImageURL) { // prrovide fallback method through the OpenID credential issuer metadata
if (!credentialImageURL) { // provide fallback method through the OpenID credential issuer metadata
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fallback breaks if the issuer is not pre-configured in the database of the wallet-backend-server.

@@ -4,4 +4,6 @@ export type StorableCredential = {
credentialIdentifier: string;
format: VerifiableCredentialFormat;
credential: string;
credentialIssuerIdentifier?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this not needed to store these fields on demo.wwwallet.org?

@@ -37,7 +37,7 @@ export class OpenID4VCIHelper implements IOpenID4VCIHelper {
}
catch(err) {
console.error(err);
throw new Error("Couldn't get Credential Issuer Metadata");
// throw new Error("Couldn't get Credential Issuer Metadata");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, the application breaks when the issuer is not pre-configured in the database of the wallet-backend-server.

})
.setProtectedHeader({
alg: keypair.alg,
typ: "openid4vci-proof+jwt",
jwk: { ...keypair.publicKey, key_ops: ['verify'] } as JWK,
// jwk: { ...keypair.publicKey, key_ops: ['verify'] } as JWK,
kid: did,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cryptographic_binding_methods_supported of the issuer https://agent.dev.eduwallet.nl/mbob/.well-known/openid-credential-issuer are ["did:jwk", "did:key"], so the credential only considered the jwt verified with a kid header with the did as a value. This means that we should have different ways to construct jwts depending on the cryptographic_binding_methods_supported of the issuer.

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

Successfully merging this pull request may close these issues.

1 participant