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

feature/SPRIND-137 #295

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from
Open

feature/SPRIND-137 #295

wants to merge 24 commits into from

Conversation

zoemaas
Copy link
Contributor

@zoemaas zoemaas commented Dec 6, 2024

No description provided.

@zoemaas zoemaas self-assigned this Dec 6, 2024
# Conflicts:
#	packages/oid4vci-holder/package.json
#	packages/oid4vci-issuer-store/package.json
#	packages/oid4vci-issuer/package.json
#	pnpm-lock.yaml
@zoemaas zoemaas marked this pull request as ready for review December 11, 2024 15:49
 into feature/SPRIND-137

# Conflicts:
#	packages/ebsi-support/package.json
#	packages/mdl-mdoc/package.json
#	packages/oid4vci-holder/package.json
#	packages/oid4vci-issuer-rest-api/package.json
#	packages/oid4vci-issuer-rest-client/package.json
#	packages/oid4vci-issuer-store/package.json
#	packages/oid4vci-issuer/package.json
#	packages/siopv2-oid4vp-common/package.json
#	packages/siopv2-oid4vp-op-auth/package.json
#	packages/siopv2-oid4vp-rp-auth/package.json
#	packages/siopv2-oid4vp-rp-rest-api/package.json
#	packages/w3c-vc-api/package.json
#	pnpm-lock.yaml
 into feature/SPRIND-137

# Conflicts:
#	packages/ebsi-support/package.json
#	packages/mdl-mdoc/package.json
#	packages/oid4vci-holder/package.json
#	packages/oid4vci-issuer-rest-api/package.json
#	packages/oid4vci-issuer-rest-client/package.json
#	packages/oid4vci-issuer-store/package.json
#	packages/oid4vci-issuer/package.json
#	packages/siopv2-oid4vp-common/package.json
#	packages/siopv2-oid4vp-op-auth/package.json
#	packages/siopv2-oid4vp-rp-auth/package.json
#	packages/siopv2-oid4vp-rp-rest-api/package.json
#	packages/w3c-vc-api/package.json
#	pnpm-lock.yaml

if (areRequiredCredentialsPresent !== Status.ERROR && verifiableCredentials) {
const uniqueDigitalCredentials: UniqueDigitalCredential[] = verifiableCredentials.map((vc) => {
// @ts-ignore FIXME Funke
Copy link

Choose a reason for hiding this comment

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

Suggested change
// @ts-ignore FIXME Funke
// @ts-expect-error: Funke needs to be fixed in XYZ-1234

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have a ticket for that? Because it's not my implementation and I have no idea what is it about.

Copy link
Contributor

@nklomp nklomp left a comment

Choose a reason for hiding this comment

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

siop response should never become undefined. Also several unneeded casts to any.

@@ -344,32 +357,62 @@ export class DidAuthSiopOpAuthenticator implements IAgentPlugin {

const pex = new PEX()
const verifiableCredentialsWithDefinition: Array<VerifiableCredentialsWithDefinition> = []
const dcqlCredentialsWithCredentials: Map<DcqlCredentialRepresentation, UniqueDigitalCredential> = new Map()

if (authorizationRequestData.presentationDefinitions !== undefined && authorizationRequestData.presentationDefinitions !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if Array.isArray()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added if Array.isArray() check

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't mean add. Just replace. Those "if undefined and null" are not the way we check elsewhere anyway. Just do an if on the object. Array.isArray will never be true for null or undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using only Array.isArray() now

const udc = selectedCredentials.find((udc) => udc.hash == hash)

if (!udc) {
throw Error('UniqueDigitalCredential could not be found')
Copy link
Contributor

Choose a reason for hiding this comment

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

throw error vs Promise.reject(Error) below (which is prefered)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to use Promise.reject(Error)

return Promise.reject(Error('None of the selected credentials match any of the presentation definitions.'))
}

} else if (authorizationRequestData.dcqlQuery !== undefined && authorizationRequestData.dcqlQuery !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

else if (authorizationRequestData.dclQuery)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using else if (authorizationRequestData.dclQuery) now

} else if (authorizationRequestData.dcqlQuery !== undefined && authorizationRequestData.dcqlQuery !== null) {
//TODO Only SD-JWT and MSO MDOC are supported at the moment
if (this.hasMDocCredentials(selectedCredentials) || this.hasSdJwtCredentials(selectedCredentials)) {
selectedCredentials.forEach((vc: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added type assertion

// todo: Change issuer value in case we do not use identifier. Use key.meta.jwkThumbprint then
responseSignerOpts: idOpts!,
})
return undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

A SIOP response cannot be undefined that is an error case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throwing an error

};

private retrieveEncodedCredential = (credential: UniqueDigitalCredential) => {
// FIXME Remove any
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change to any to begin with. Check of original is present or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed "any"

@@ -382,21 +425,40 @@ export class DidAuthSiopOpAuthenticator implements IAgentPlugin {
context,
)

const contentType = response.headers.get('content-type') || ''
const contentType = response?.headers.get('content-type') || ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not allow response to be undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed '?'

url: response.url,
queryParams: decodeUriAsJson(response.url),
url: response?.url,
queryParams: response && decodeUriAsJson(response?.url),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not allow response to be undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already throwing an error from the siopSendAuthorizationResponse(...) method if there are nor PDs neither DCQL query, thus the response is not undefined anymore. Removed the 'response &&' at line 439

let responseBody: any = null

const text = await response.text()
const text = await response?.text()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not allow response to be undefined

Copy link
Contributor Author

@zoemaas zoemaas Jan 9, 2025

Choose a reason for hiding this comment

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

Removed '?'

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.

3 participants