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

JCL-299: verify method #403

Merged
merged 7 commits into from
Apr 14, 2023
Merged

JCL-299: verify method #403

merged 7 commits into from
Apr 14, 2023

Conversation

timea-solid
Copy link
Contributor

@timea-solid timea-solid commented Apr 6, 2023

@jholleran Could you please take a look at the verify(...) method I added. I understand there are options in the call. What can they be? is it different from access grant to access request?
Any help is welcome.
Conclusion: we took out options from call.

@timea-solid timea-solid requested a review from a team as a code owner April 6, 2023 09:16
@timea-solid timea-solid temporarily deployed to ESS PodSpaces April 6, 2023 09:16 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS PodSpaces April 6, 2023 09:16 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS DevNext April 6, 2023 09:16 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS PodSpaces April 6, 2023 09:16 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS PodSpaces April 6, 2023 09:16 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS DevNext April 6, 2023 09:16 — with GitHub Actions Inactive
@timea-solid timea-solid marked this pull request as draft April 6, 2023 09:16
@timea-solid timea-solid changed the title JCL-299: verify call JCL-299: verify method Apr 6, 2023
@timea-solid timea-solid temporarily deployed to ESS PodSpaces April 6, 2023 12:58 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS PodSpaces April 6, 2023 12:58 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS DevNext April 6, 2023 12:58 — with GitHub Actions Inactive
return v1Metadata().thenCompose(metadata -> {

final Map<String, Object> presentation = new HashMap<>();
presentation.put(VERIFIABLE_CREDENTIAL, accessGrant);

Choose a reason for hiding this comment

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

The /verify endpoint can also take a VerifiablePresentation. Do you need this in this client?

if (isSuccess(status)) {
return processVerificationResult(input);
}
throw new AccessGrantException("Unable to perform Access Grant verify: HTTP error " + status,

Choose a reason for hiding this comment

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

In the case of an error the response will also contain an error, errorDescription and an optional reason. These could be useful for a downstream client to know but they are getting dropped here and we are only giving them the status.

Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the VC-API spec, the verify endpoint will return a 200 in cases where the credential is invalid, but there is no defined response for, say, 4xx or 5xx category errors.

I think it would be great for the client to try to parse 4xx/5xx error bodies as if they contain those response, but that can be part of an overall improvement in how errors are processed.

@timea-solid timea-solid temporarily deployed to ESS PodSpaces April 14, 2023 10:12 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS PodSpaces April 14, 2023 10:12 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS DevNext April 14, 2023 10:12 — with GitHub Actions Inactive
@timea-solid timea-solid marked this pull request as ready for review April 14, 2023 10:12
@timea-solid timea-solid temporarily deployed to ESS PodSpaces April 14, 2023 12:17 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS PodSpaces April 14, 2023 12:17 — with GitHub Actions Inactive
@timea-solid timea-solid temporarily deployed to ESS DevNext April 14, 2023 12:17 — with GitHub Actions Inactive
Copy link
Collaborator

@acoburn acoburn left a comment

Choose a reason for hiding this comment

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

LGTM

if (isSuccess(status)) {
return processVerificationResult(input);
}
throw new AccessGrantException("Unable to perform Access Grant verify: HTTP error " + status,
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the VC-API spec, the verify endpoint will return a 200 in cases where the credential is invalid, but there is no defined response for, say, 4xx or 5xx category errors.

I think it would be great for the client to try to parse 4xx/5xx error bodies as if they contain those response, but that can be part of an overall improvement in how errors are processed.

@timea-solid timea-solid merged commit dfbdc56 into main Apr 14, 2023
@timea-solid timea-solid deleted the JCL-299-verify branch April 14, 2023 15:28
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