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

adding vc validation to IsValidCredentialApplicationForManifest function #230

Merged
merged 8 commits into from
Oct 18, 2022

Conversation

nitro-neal
Copy link
Contributor

adding vc validation to IsValidCredentialApplicationForManifest function

@@ -144,8 +145,9 @@ func (cf *CredentialResponse) IsValid() error {
}

// IsValidCredentialApplicationForManifest validates the rules on how a credential manifest [cm] and credential application [ca] relate to each other https://identity.foundation/credential-manifest/#credential-application
func IsValidCredentialApplicationForManifest(cm CredentialManifest, ca CredentialApplication) error {
func IsValidCredentialApplicationForManifest(cm CredentialManifest, ca CredentialApplication, vcs []credential.VerifiableCredential) error {
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider making this a variadic argument so it can be an optional parameter

IsValidCredentialApplicationForManifest(cm CredentialManifest, ca CredentialApplication, vcs ...credential.VerifiableCredential)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, so if there is no vcs then would it be expected that the application's presentation_submission's descriptor_map be empty?
image

Copy link
Member

Choose a reason for hiding this comment

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

yes, exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

	t.Run("only ca cm validation, no vcs", func(tt *testing.T) {
		cm, ca, _ := getValidTestCmCaVc(tt)
		
		cm.PresentationDefinition.InputDescriptors = make([]exchange.InputDescriptor, 0)
		ca.PresentationSubmission.DescriptorMap = make([]exchange.SubmissionDescriptor, 0)

		err := IsValidCredentialApplicationForManifest(cm, ca)

		assert.NoError(tt, err)
	})

Trying to get the above to work , but getting error - presentation definition must have at least one input descriptor

what would the input descriptor look like if there are no vcs? Can this case exist and be a valid ca,cm?

Copy link
Member

Choose a reason for hiding this comment

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

if there are input descriptors there must be VCs, so if there's a presentation definition at all - there must be VCs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok good stuff, I was trying to make a presentation definition with 0 input descriptors,
but yup this works now with just an empty presentation definition

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2022

Codecov Report

Merging #230 (c704f5b) into main (57151b8) will increase coverage by 0.07%.
The diff coverage is 63.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #230      +/-   ##
==========================================
+ Coverage   58.03%   58.10%   +0.07%     
==========================================
  Files          35       35              
  Lines        4060     4115      +55     
==========================================
+ Hits         2356     2391      +35     
- Misses       1317     1330      +13     
- Partials      387      394       +7     
Impacted Files Coverage Δ
credential/manifest/model.go 58.71% <63.16%> (+2.71%) ⬆️

nitro-neal and others added 2 commits October 18, 2022 11:24
Co-authored-by: Gabe <gcohen@squareup.com>
@nitro-neal nitro-neal merged commit ab2c5dd into main Oct 18, 2022
@nitro-neal nitro-neal deleted the valid-cm-for-ca-vcs branch October 18, 2022 17:10
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.

None yet

3 participants