-
Notifications
You must be signed in to change notification settings - Fork 0
Wip/verify unime core compat #4
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
base: wip/verify
Are you sure you want to change the base?
Conversation
We have at least one issuer that doesn't have the locale set. The spec makes it optional: https://openid.net/specs/openid-4-verifiable-credential-issuance-1_0.html#section-11.2.3-2.10.2.2 > locale: OPTIONAL. String value that identifies the language of this > object represented as a language tag taken from values defined in BCP47 > [RFC5646]. There MUST be only one object for each language identifier.
The url should be URL-encoded as per the spec. Usually, this does not matter, as url-encoding most URLS is involute - won't change anything But it may, if the url in the offer contains e.g. arguments, anchors, a port etc.
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.
Pull Request Overview
This PR adjusts schema validation and credential-offer handling to align with the spec and improve unime-core interop.
- Made
locale
optional in issuer metadata schema - Standardized quote style and added URI decoding in
credentialOfferFromUrl
- Added handling for
AcademicEnrollmentCredential
display name and issuer fallback in UI components
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/lib/schemas/OpenidCredentialIssuerMetadataSchema.ts | Marked locale as optional in the display object |
src/lib/services/credential-offer.service.ts | Switched to double quotes and decode credential_offer_uri before fetching |
src/context/ContainerContext.tsx | Added branch for AcademicEnrollmentCredential display name |
src/components/Credentials/CredentialInfo.js | Introduced issuerName state and fallback in the issuer row |
Comments suppressed due to low confidence (2)
src/components/Credentials/CredentialInfo.js:217
- The issuer row falls back to
issuerName
(frombeautifiedForm.issuer.name
) but otherwise usescredentialSubject.issuer
. Verify whether the fallback should replacecredentialSubject.issuer
entirely or if the two fields are intended to differ.
{!credentialSubjectRows.some(row => row.name === 'issuer') && renderRow('issuer', 'Issuer', parsedCredential?.vc?.credentialSubject?.issuer || issuerName, screenType)}
src/lib/services/credential-offer.service.ts:7
- Consider adding unit tests for
credentialOfferFromUrl
to cover scenarios wherecredential_offer
is present, wherecredential_offer_uri
is decoded successfully, and where fetch fails or returns non-OK responses.
export const credentialOfferFromUrl = async (url: string) => {
} | ||
try { | ||
const response = await fetch(parsedUrl.searchParams.get(PARAM_CREDENTIAL_OFFER_URI), {}); | ||
const offerURI = | ||
parsedUrl.searchParams.get(PARAM_CREDENTIAL_OFFER_URI) || ""; |
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.
Using || ''
for credential_offer_uri
means offerURI
can be empty and lead to fetch('')
. Add a guard to return an error or throw if the parameter is missing before attempting to fetch.
parsedUrl.searchParams.get(PARAM_CREDENTIAL_OFFER_URI) || ""; | |
parsedUrl.searchParams.get(PARAM_CREDENTIAL_OFFER_URI); | |
if (!offerURI) { | |
throw new Error(`Missing or empty parameter: ${PARAM_CREDENTIAL_OFFER_URI}`); | |
} |
Copilot uses AI. Check for mistakes.
const offerURI = | ||
parsedUrl.searchParams.get(PARAM_CREDENTIAL_OFFER_URI) || ""; | ||
const decodedOfferURI = decodeURIComponent(offerURI); | ||
const response = await fetch(decodedOfferURI, {}); |
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.
After fetching the credential offer, check response.ok
and handle non-2xx statuses (e.g., throw or return a structured error) before calling response.json()
to improve error clarity.
const response = await fetch(decodedOfferURI, {}); | |
const response = await fetch(decodedOfferURI, {}); | |
if (!response.ok) { | |
throw new Error(`Failed to fetch credential offer: ${response.status} ${response.statusText}`); | |
} |
Copilot uses AI. Check for mistakes.
if (isOpenBadgeCredential) return beautifiedForm.credentialSubject.achievement.name; | ||
if (t.includes('SupportCredential') || t.includes('ExamEnrollmentCredential')) return beautifiedForm.credentialSubject.title; | ||
if (t.includes('AcademicEnrollmentCredential')) return beautifiedForm.credentialSubject.name; | ||
if (storedCredentialConfigurationId === 'EduID') return 'eduID' | ||
|
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.
[nitpick] This growing chain of if
statements for credential types may become hard to extend. Consider refactoring into a mapping or switch statement to improve readability and make future additions simpler.
if (isOpenBadgeCredential) return beautifiedForm.credentialSubject.achievement.name; | |
if (t.includes('SupportCredential') || t.includes('ExamEnrollmentCredential')) return beautifiedForm.credentialSubject.title; | |
if (t.includes('AcademicEnrollmentCredential')) return beautifiedForm.credentialSubject.name; | |
if (storedCredentialConfigurationId === 'EduID') return 'eduID' | |
const credentialTypeMapping: Record<string, () => string> = { | |
'OpenBadgeCredential': () => beautifiedForm.credentialSubject.achievement.name, | |
'SupportCredential': () => beautifiedForm.credentialSubject.title, | |
'ExamEnrollmentCredential': () => beautifiedForm.credentialSubject.title, | |
'AcademicEnrollmentCredential': () => beautifiedForm.credentialSubject.name, | |
}; | |
if (storedCredentialConfigurationId === 'EduID') return 'eduID'; | |
for (const [key, getName] of Object.entries(credentialTypeMapping)) { | |
if (t.includes(key)) { | |
return getName(); | |
} | |
} |
Copilot uses AI. Check for mistakes.
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.
This is a temporary solution pending a discussion pertaining to credential card title display logic. There is no need to optimize it right now.
@@ -1,16 +1,21 @@ | |||
import { CredentialOfferSchema } from '../schemas/CredentialOfferSchema'; | |||
import { CredentialOfferSchema } from "../schemas/CredentialOfferSchema"; |
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.
I would just keep single quotes unless double quotes is the preferred formatting. In the latter case, please also update the formatting in other touched files.
This pull request introduces some minor changes to comply to the spec and interop with unime-core running at poc9
Schema and Validation Updates:
src/lib/schemas/OpenidCredentialIssuerMetadataSchema.ts
: Made thelocale
field optional in thedisplay
object within the schema.src/lib/services/credential-offer.service.ts
: Standardized quotation marks to double quotes and improved the handling ofcredential_offer_uri
by decoding the URI before making a fetch request.