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

Identity Hub: Fix VerifiableCredentialsJwtServiceImpl's "NotSuchMethodError" (#43) #20

Merged

Conversation

marcgs
Copy link
Contributor

@marcgs marcgs commented Aug 8, 2022

What this PR changes/adds

Introduce a fix for the "NoSuchMethodError" observed in the CI pipeline for the cloud system tests: https://github.com/agera-edc/MinimumViableDataspace/runs/7693917471?check_suite_focus=true

The cause of the issue is still unclear. It seems that the toJSONObject method is not available due to a version mismatch of the nimbus library. This does not happen when running tests locally, nor when running the local system tests in CI. Why this only manifests when running the cloud system tests in CI is still a mystery.

MicrosoftTeams-image

This PR introduces a workaround by adapting the VerifiableCredentialsJwtServiceImpl not to use this method.

Logging was improved in order to debug the issue and this PR contains additional logging statements that proved to be useful.

Why it does that

Fix Identity Hub integration in MVD.

Linked Issue(s)

Relates to #21

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • added relevant details to the changelog? (skip with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

Fix NoSuchMethodError in VerifiableCredentialsJwtServiceImpl. Add more logging.

var claims = successfulResults.stream()
if (!failedResults.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

This is dangerous: what if a dataspace requires that all claims must be verified? Maybe you could introduce a VerificationResult as a subclass of AbstractResult, that allows for transporting both the successful claims, and the failed ones? That way, the caller of the verifyCredentials method could decide.

Copy link
Member

Choose a reason for hiding this comment

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

--> will be done in another issue/PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, this will be tackled in a separate story: agera-edc#47. This behaviour was not changed with the current PR.

@paullatzelsperger paullatzelsperger merged commit 8602590 into eclipse-edc:main Aug 8, 2022
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.

2 participants