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.

Sign up for GitHub

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

Jump to bottom

Adapted policy to the claims format of the IdentityHubCredentialsVerifier. #227

Merged
merged 72 commits into from
Aug 2, 2022
Merged

Adapted policy to the claims format of the IdentityHubCredentialsVerifier. #227

merged 72 commits into from
Aug 2, 2022

Conversation

ouphi
Copy link

@ouphi ouphi commented Jul 28, 2022 •
edited
Loading

What this PR changes/adds

Adapted policy to the claims format returned by the IdentityHubCredentialsVerifier.

Why it does that

MVD will be using the IdentityHubCredentialsVerifier. Adapted the policy to the claim format that the IdentityHubCredentialsVerifier returns.

Further notes

Changed the MockCredentialVerifier to return the same format as the IdentityHubCredentialsVerifier.

Linked Issue(s)

agera-edc/IdentityHub#32

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?

@ouphi ouphi changed the title [Do not review] Adapted policy to VC. [Do not review] Adapted policy to the claims format of the IdentityHubCredentialsVerifier. Jul 28, 2022
@ouphi ouphi force-pushed the feature/32/32-adapt-mvd-policy branch from 2f3bdc8 to 348f871 Compare July 28, 2022 13:47
@cpeeyush cpeeyush self-requested a review July 29, 2022 12:44
@ouphi ouphi changed the base branch from feature/32/adapt-mvd-policy to feature/224/224-edc-update July 29, 2022 13:58
Base automatically changed from feature/224/224-edc-update to feature/224-edc-update July 29, 2022 16:06
@ouphi ouphi changed the base branch from feature/224-edc-update to feature/32/adapt-mvd-policy August 2, 2022 07:35
cpeeyush
cpeeyush reviewed Aug 2, 2022
View reviewed changes
...ions/policies/src/main/java/org/eclipse/dataspaceconnector/mvd/RegionConstraintFunction.java Outdated
try {
var region = verifiableCredential.getCredentialSubject().get(REGION_KEY);
return Optional.ofNullable((String) region);
} catch (Exception e) {
Copy link

@cpeeyush cpeeyush Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit: is there a particular reason why are catching all of the exception here and then returning empty optional, instead not letting exception be thrown or bubble up?

same as in above method getVerifiableCredential because this method may also throw IllegalArgumentException and in case of other exceptions current implementation will give the impression that evaluate failed with false response but it is actually something else.

Copy link
Author

@ouphi ouphi Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I changed the exception to ClassCastException to make it better.
It is in case a region can't be casted to string. To avoid breaking the entire flow checking the policy.

Copy link

@cpeeyush cpeeyush Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

or maybe I was thinking too much your trick with optional may also work if you catch all exception with logging error about it.

And just a java doc on the method that we continue to process to avoid any Verifiable credential which do not have region as per the correct format.

cpeeyush
cpeeyush approved these changes Aug 2, 2022
View reviewed changes
Copy link

@cpeeyush cpeeyush left a comment

Choose a reason for hiding this comment

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

Approved with suggestions.

@paullatzelsperger paullatzelsperger merged commit a3ca8c6 into feature/32/adapt-mvd-policy Aug 2, 2022
marcgs added a commit that referenced this pull request Aug 4, 2022
@marcgs
Squashed commit of the following:
451fcdf 
commit 7c5069d
Author: Ophélie Le Mentec <17216799+ouphi@users.noreply.github.com>
Date:   Tue Aug 2 14:37:15 2022 +0200

    Cleanup to make the catching of exceptions related to the Verifiable credential casting more clear. (#231)

    * Cleanup.

    * Use only one method to get the region.

commit 618f40f
Merge: a3ca8c6 508dc29
Author: Ophelie Le Mentec <17216799+ouphi@users.noreply.github.com>
Date:   Tue Aug 2 14:17:38 2022 +0200

    Merged main.

commit a3ca8c6
Author: Ophélie Le Mentec <17216799+ouphi@users.noreply.github.com>
Date:   Tue Aug 2 11:38:06 2022 +0200

    feat: adapt policy to the claims format of the IdentityHubCredentialsVerifier. (#227)

    * Update action.yml

    * Update deploy.yaml

    * Adapted policy to VC.

    * .

    * .

    * Update action.yml

    * Update IdentityHubIntegrationTest.java

    * Update IdentityHubIntegrationTest.java

    * Update IdentityHubIntegrationTest.java

    * Fixed checkstyle.

    * Changed identityhub commit.

    * Adapt ide hub client contract in tests.

    * Renamed constraint function.

    * Renamed constraint function.

    * Updated credentials verifier.

    * checkstyle.

    * Fixed tests.

    * Fixed checkstyle.

    * Updated javadoc.

    * Used Duty instead of Permission.

    * Improved issuer comment.

    * Improved tests.

    * Revert "Used Duty instead of Permission."

    This reverts commit 04569cd.

    * Revert "Revert "Used Duty instead of Permission.""

    This reverts commit 348f871.

    * Revert "Revert "Revert "Used Duty instead of Permission."""

    This reverts commit 654d038.

    * cleanup.

    * Simplified toMappedVC.

    * Added tests for RegionConstraintFunction.

    * Used constants.

    * Fix.

    * Added a test.

    * Fixed checkstyle.

    * checkstyle.

    * Improved test coverage.

    * Added region key.

    * Used constant for iss.

    * Fixed checktyls.

    * use private.

    * applied comment.

    * Used ofNullable.

    * Used identityHubGroup and identityHubVersion.

    * Applied comment.

    * Removed unused method.

    * Applied comment.

    * .

    * checkstyle.

    * Fixed import.

    * Used last idHub commit.

    * Added NEQ operator.

    * Update MockCredentialsVerifier.java

    * Update SeedPoliciesExtension.java

    * Update MockCredentialsVerifier.java

    * Update SeedPoliciesExtension.java

    * Revert using static vckey var.

    * Revert "Used last idHub commit."

    This reverts commit 54c6439.

    * Update MockCredentialsVerifierTest.java

    * Update deploy.yaml

    * Used provider.

    * Made provider method public.

    * Merged PR updating EDC version.

    * Revert "Merged PR updating EDC version."

    This reverts commit 75e628b.

    * Used constant verifiable credentials key.

    * Changed identityHub commit.

    * Fix.

    * Use failure message.

    * Added IN operator.

    * use ClassCastException.

    * Added test.

    * Applied comment.

    * Do not use Optional as a returned type.

    Co-authored-by: Alexandre Gattiker <algattik@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paullatzelsperger paullatzelsperger paullatzelsperger approved these changes

@marcgs marcgs marcgs approved these changes

@cpeeyush cpeeyush cpeeyush approved these changes

@algattik algattik algattik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ouphi @cpeeyush @marcgs @algattik @paullatzelsperger