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

fix: issue where attributes and predicates match #640

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion demo/src/Faber.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ export class Faber extends BaseAgent {

public async acceptConnection(invitation_url: string) {
const connectionRecord = await this.receiveConnectionRequest(invitation_url)

this.connectionRecordAliceId = await this.waitForConnection(connectionRecord)
}

Expand Down
13 changes: 7 additions & 6 deletions packages/core/src/modules/proofs/services/ProofService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { EventEmitter } from '../../../agent/EventEmitter'
import { InjectionSymbols } from '../../../constants'
import { Attachment, AttachmentData } from '../../../decorators/attachment/Attachment'
import { AriesFrameworkError } from '../../../error'
import { checkProofRequestForDuplicates } from '../../../utils'
import { JsonEncoder } from '../../../utils/JsonEncoder'
import { JsonTransformer } from '../../../utils/JsonTransformer'
import { uuid } from '../../../utils/uuid'
Expand Down Expand Up @@ -257,8 +258,8 @@ export class ProofService {
comment?: string
}
): Promise<ProofProtocolMsgReturnType<RequestPresentationMessage>> {
// Assert attribute and predicate group names do not match
this.assertAttributePredicateGroupNamesDoNotMatch(proofRequest)
// Assert attribute and predicate (group) names do not match
checkProofRequestForDuplicates(proofRequest)

// Assert
proofRecord.assertState(ProofState.ProposalReceived)
Expand Down Expand Up @@ -305,8 +306,8 @@ export class ProofService {
): Promise<ProofProtocolMsgReturnType<RequestPresentationMessage>> {
this.logger.debug(`Creating proof request`)

// Assert attribute and predicate group names do not match
this.assertAttributePredicateGroupNamesDoNotMatch(proofRequest)
// Assert attribute and predicate (group) names do not match
checkProofRequestForDuplicates(proofRequest)

// Assert
connectionRecord?.assertReady()
Expand Down Expand Up @@ -369,8 +370,8 @@ export class ProofService {
}
await validateOrReject(proofRequest)

// Assert attribute and predicate group names do not match
this.assertAttributePredicateGroupNamesDoNotMatch(proofRequest)
// Assert attribute and predicate (group) names do not match
checkProofRequestForDuplicates(proofRequest)

this.logger.debug('received proof request', proofRequest)

Expand Down
240 changes: 240 additions & 0 deletions packages/core/src/utils/__tests__/indyProofRequest.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
import type { IndyCredentialMetadata } from '../../modules/credentials/models/CredentialInfo'
import type { CustomCredentialTags, CredentialPreviewAttribute, RequestCredentialMessage } from '@aries-framework/core'

import { checkProofRequestForDuplicates } from '..'
import { Attachment, AttachmentData } from '../../decorators/attachment/Attachment'

import {
CredentialMetadataKeys,
AriesFrameworkError,
AttributeFilter,
PredicateType,
ProofAttributeInfo,
ProofPredicateInfo,
ProofRequest,
CredentialState,
OfferCredentialMessage,
CredentialRecord,
CredentialPreview,
} from '@aries-framework/core'

export const INDY_CREDENTIAL_OFFER_ATTACHMENT_ID = 'libindy-cred-offer-0'

const credentialPreview = CredentialPreview.fromRecord({
name: 'John',
age: '99',
})

const offerAttachment = new Attachment({
id: INDY_CREDENTIAL_OFFER_ATTACHMENT_ID,
mimeType: 'application/json',
data: new AttachmentData({
base64:
'eyJzY2hlbWFfaWQiOiJhYWEiLCJjcmVkX2RlZl9pZCI6IlRoN01wVGFSWlZSWW5QaWFiZHM4MVk6MzpDTDoxNzpUQUciLCJub25jZSI6Im5vbmNlIiwia2V5X2NvcnJlY3RuZXNzX3Byb29mIjp7fX0',
}),
})

const mockCredentialRecord = ({
state,
requestMessage,
metadata,
threadId,
connectionId,
tags,
id,
credentialAttributes,
}: {
state?: CredentialState
requestMessage?: RequestCredentialMessage
metadata?: IndyCredentialMetadata & { indyRequest: Record<string, unknown> }
tags?: CustomCredentialTags
threadId?: string
connectionId?: string
id?: string
credentialAttributes?: CredentialPreviewAttribute[]
} = {}) => {
const offerMessage = new OfferCredentialMessage({
comment: 'some comment',
credentialPreview: credentialPreview,
offerAttachments: [offerAttachment],
})

const credentialRecord = new CredentialRecord({
offerMessage,
id,
credentialAttributes: credentialAttributes || credentialPreview.attributes,
requestMessage,
state: state || CredentialState.OfferSent,
threadId: threadId ?? offerMessage.id,
connectionId: connectionId ?? '123',
tags,
})

if (metadata?.indyRequest) {
credentialRecord.metadata.set(CredentialMetadataKeys.IndyRequest, { ...metadata.indyRequest })
}

if (metadata?.schemaId) {
credentialRecord.metadata.add(CredentialMetadataKeys.IndyCredential, {
schemaId: metadata.schemaId,
})
}

if (metadata?.credentialDefinitionId) {
credentialRecord.metadata.add(CredentialMetadataKeys.IndyCredential, {
credentialDefinitionId: metadata.credentialDefinitionId,
})
}

return credentialRecord
}

describe('Present Proof', () => {
let secCredDef: CredentialRecord
let credDef: CredentialRecord

beforeAll(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not have to be async

credDef = mockCredentialRecord({ state: CredentialState.OfferSent })
secCredDef = mockCredentialRecord({ state: CredentialState.OfferSent })
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we're doing unit tests we can just create a random credential definition id (took this one from https://indyscan.io).

Also because the checkProofRequestForDuplicates method doesn't use the credential definition at all, we don't have to provide a second credential definition id. This should suffice (we also don't need the mockCredentialRecord etc... as the credential record is not used, we only need an id):

Suggested change
credDef = mockCredentialRecord({ state: CredentialState.OfferSent })
secCredDef = mockCredentialRecord({ state: CredentialState.OfferSent })
const credentialDefinitionId = '9vPXgSpQJPkJEALbLXueBp:3:CL:57753:tag1'

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 still want to test if it passes with an attribute and a predicate with different credential id's? or should I remove those tests then as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that could be useful, but this method doesn't test that. It never touches the credentialDefinitionId (note credentialDefinitionId is different from the credentialId. )

We should do an actual proof exchange to be able to test that (and should be done in proofs.test.ts)

})

test('attribute names match, same cred def filter', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not have to be async

const attributes = {
name: new ProofAttributeInfo({
name: 'age',
restrictions: [
new AttributeFilter({
credentialDefinitionId: credDef.id,
}),
],
}),
age: new ProofAttributeInfo({
name: 'age',
restrictions: [
new AttributeFilter({
credentialDefinitionId: credDef.id,
}),
],
}),
}

const nonce = 'testtesttest12345'
Copy link
Contributor

Choose a reason for hiding this comment

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

@TimoGlastra I created an issue for this, #646. Is this solution fine or do we need to expose the generateNonce function in a more accessible way?


const proofRequest = new ProofRequest({
name: 'proof-request',
version: '1.0',
nonce,
requestedAttributes: attributes,
})

expect(() => checkProofRequestForDuplicates(proofRequest)).toThrowError(AriesFrameworkError)
})

test('attribute names match with predicates name, same cred def filter', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not have to be async

const attributes = {
name: new ProofAttributeInfo({
name: 'age',
restrictions: [
new AttributeFilter({
credentialDefinitionId: credDef.id,
}),
],
}),
}

const predicates = {
age: new ProofPredicateInfo({
name: 'age',
predicateType: PredicateType.GreaterThanOrEqualTo,
predicateValue: 50,
restrictions: [
new AttributeFilter({
credentialDefinitionId: credDef.id,
}),
],
}),
}

const nonce = 'testtesttest12345'

const proofRequest = new ProofRequest({
name: 'proof-request',
version: '1.0',
nonce,
requestedAttributes: attributes,
requestedPredicates: predicates,
})

expect(() => checkProofRequestForDuplicates(proofRequest)).toThrowError(AriesFrameworkError)
})

test('attribute names match, different cred def filter', async () => {
const attributes = {
name: new ProofAttributeInfo({
name: 'age',
restrictions: [
new AttributeFilter({
credentialDefinitionId: credDef.id,
}),
],
}),
age: new ProofAttributeInfo({
name: 'age',
restrictions: [
new AttributeFilter({
credentialDefinitionId: secCredDef.id,
}),
],
}),
}

const nonce = 'testtesttest12345'

const proofRequest = new ProofRequest({
name: 'proof-request',
version: '1.0',
nonce,
requestedAttributes: attributes,
})

expect(() => checkProofRequestForDuplicates(proofRequest)).toThrowError(AriesFrameworkError)
})

test('attribute name matches with predicate name, different cred def filter', async () => {
const attributes = {
name: new ProofAttributeInfo({
name: 'age',
restrictions: [
new AttributeFilter({
credentialDefinitionId: credDef.id,
}),
],
}),
}

const predicates = {
age: new ProofPredicateInfo({
name: 'age',
predicateType: PredicateType.GreaterThanOrEqualTo,
predicateValue: 50,
restrictions: [
new AttributeFilter({
credentialDefinitionId: secCredDef.id,
}),
],
}),
}

const nonce = 'testtesttest12345'

const proofRequest = new ProofRequest({
name: 'proof-request',
version: '1.0',
nonce,
requestedAttributes: attributes,
requestedPredicates: predicates,
})

expect(() => checkProofRequestForDuplicates(proofRequest)).toThrowError(AriesFrameworkError)
})
})
11 changes: 11 additions & 0 deletions packages/core/src/utils/assertNoDuplicates.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { AriesFrameworkError } from '../error/AriesFrameworkError'

export function assertNoDuplicatesInArray(arr: string[]) {
const arrayLength = arr.length
const uniqueArrayLength = new Set(arr).size

if (arrayLength === uniqueArrayLength) return

const duplicates = arr.filter((item, index) => arr.indexOf(item) != index)
throw new AriesFrameworkError(`The proof request contains duplicate items: ${duplicates.toString()}`)
}
1 change: 1 addition & 0 deletions packages/core/src/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export * from './MultiBaseEncoder'
export * from './buffer'
export * from './MultiHashEncoder'
export * from './JWE'
export * from './indyProofRequest'
22 changes: 22 additions & 0 deletions packages/core/src/utils/indyProofRequest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import type { ProofRequest } from '../modules/proofs/models/ProofRequest'

import { assertNoDuplicatesInArray } from './assertNoDuplicates'

export function attributesToArray(proofRequest: ProofRequest) {
// Attributes can contain either a `name` string value or an `names` string array. We reduce it to a single array
// containing all attribute names from the requested attributes.
return Array.from(proofRequest.requestedAttributes.values()).reduce<string[]>(
(names, a) => [...names, ...(a.name ? [a.name] : a.names ? a.names : [])],
[]
)
}

export function predicatesToArray(proofRequest: ProofRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think predicateNamesToArray, and same for attributes, Is a better name.

return Array.from(proofRequest.requestedPredicates.values()).map((a) => a.name)
}

export function checkProofRequestForDuplicates(proofRequest: ProofRequest) {
const attributes = attributesToArray(proofRequest)
const predicates = predicatesToArray(proofRequest)
assertNoDuplicatesInArray(attributes.concat(predicates))
}
26 changes: 26 additions & 0 deletions packages/core/tests/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -664,3 +664,29 @@ export async function setupProofsTest(faberName: string, aliceName: string, auto
aliceReplay,
}
}

export async function setupSecondCredential(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still being used somewhere?

faberAgent: Agent,
aliceAgent: Agent,
faberConnection: ConnectionRecord
): Promise<string> {
const { definition } = await prepareForIssuance(faberAgent, ['name', 'age'])

const credentialPreview = CredentialPreview.fromRecord({
name: 'John',
age: '99',
})

await issueCredential({
issuerAgent: faberAgent,
issuerConnectionId: faberConnection.id,
holderAgent: aliceAgent,
credentialTemplate: {
credentialDefinitionId: definition.id,
comment: 'some comment about credential',
preview: credentialPreview,
},
})

return definition.id
}
5 changes: 2 additions & 3 deletions packages/core/tests/proofs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ describe('Present Proof', () => {
testLogger.test('Initializing the agents')
;({ faberAgent, aliceAgent, credDefId, faberConnection, aliceConnection, presentationPreview } =
await setupProofsTest('Faber agent', 'Alice agent'))
testLogger.test('Issuing second credential')
})

afterAll(async () => {
Expand Down Expand Up @@ -365,8 +366,6 @@ describe('Present Proof', () => {
requestedAttributes: attributes,
requestedPredicates: predicates,
})
).rejects.toThrowError(
`The proof request contains an attribute group name that matches a predicate group name: age`
)
).rejects.toThrowError(`The proof request contains duplicate items: age`)
})
})