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

refactor: Change local_acp implementation to use acp_core #2691

Merged
merged 46 commits into from
Jun 19, 2024

Conversation

Lodek
Copy link
Member

@Lodek Lodek commented Jun 6, 2024

Relevant issue(s)

Resolve #2694

Description

This PR changes the underlying implementation of the ACPLocal Reference Monitor from SourceHub to ACP Core.
The changes aim to allow the WASM build which was broken since the inclusion of SourceHub as a dependency.

Notable changes from this refactor include:

  • Replace SourceHub as a dependency for acp_core for ACP Local
  • The field name in a Policy is now correctly marked as required. This was a consequence of an issue on SourceHub side which allowed unnamed policies to be included. Therefore, in order to correct the tests, a name was given to all policies in the codebase.
  • The previous implementation of ACP Local made use of a special notion of account which had no sequence numbers. As such, re-submitting a Policy generated the same ID. This behavior isn't compatible with how SourceHub deals with IDs and the new implementation using ACP Core reflects that. That is, every created Policy, independent of its payload, will have unique IDs.
  • Updated the Identity struct to generate a DID instead of a SourceHub address

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • [] I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Test suite ie make test

Specify the platform(s) on which this was tested:

  • Arch Linux

acp/source_hub_client.go Outdated Show resolved Hide resolved
acp/acp_local.go Outdated Show resolved Hide resolved
replacement.py Outdated Show resolved Hide resolved
tests/integration/acp/add_policy/with_no_resources_test.go Outdated Show resolved Hide resolved
tests/integration/acp/index/fixture.go Outdated Show resolved Hide resolved
@shahzadlone shahzadlone added the acp Related to the acp (access control) system label Jun 7, 2024
acp/acp_local.go Outdated Show resolved Hide resolved
acp/acp_local.go Outdated Show resolved Hide resolved
acp/acp_local.go Outdated Show resolved Hide resolved
@AndrewSisley
Copy link
Contributor

todo: Please sort out the PR description :)

Adding info regarding why this is being done would be/have-been particularly useful. And please make sure it is linked to a Defra repo github issue.

go.mod Outdated Show resolved Hide resolved
@Lodek
Copy link
Member Author

Lodek commented Jun 7, 2024

Is there any action required on my part regarding the Check Vulnerabilities Workflow?

@AndrewSisley
Copy link
Contributor

Is there any action required on my part regarding the Check Vulnerabilities Workflow?

It looks like the vulnerability only exists in the runtime/toolchain version you tried to update to - so if you sort out the go.mod that job should pass anyway

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

Sorry, I wasn't sure If I should review the PR or not as I wasn't pinged again for review since last review, but it seemed like there was some activity and you did some suggestions from the previous review, so leaving some small todos for ya, it looks really good now :)

acp/identity/identity.go Outdated Show resolved Hide resolved
acp/identity/identity_test.go Outdated Show resolved Hide resolved
acp/source_hub_client.go Outdated Show resolved Hide resolved
@Lodek Lodek requested a review from shahzadlone June 14, 2024 17:04
acp/acp_local.go Outdated
Comment on lines 119 to 122
marshalType := types.PolicyMarshalingType_SHORT_YAML
if isJSON := fastjson.Validate(policy) == nil; isJSON { // Detect JSON format.
marshalType = types.PolicyMarshalingType_SHORT_JSON
}
Copy link
Member

Choose a reason for hiding this comment

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

question: Why was the detection moved from source_hub_client into the local acp logic? Wouldn't this be nicer for @AndrewSisley's remote acp implementation as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, fair. I'll add a custom Defra type for it, as I did for the Registration Result

Copy link
Member

@shahzadlone shahzadlone Jun 14, 2024

Choose a reason for hiding this comment

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

I think just leaving as it was before should be fine? why do we need a custom type?

EDIT: I see what you mean, just reviewed the last commit

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

Just left one question, please resolve before merging. Giving an LGTM.

Thanks @Lodek for doing all the requested changes :)

WOOT WOOT your 1st DefraDB PR 🥳

@shahzadlone
Copy link
Member

Also the related issue won't close upon merge, nor is it picked up by github at the moment, because you just have #2694 in the PR description.

So please don't forget to add the resolving key word (close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved) with it.

So you can put for example:

Resolve #2694

@shahzadlone shahzadlone merged commit 6573d8c into sourcenetwork:develop Jun 19, 2024
39 checks passed
@Lodek Lodek deleted the refactor/local-acp branch June 19, 2024 17:50
@fredcarle fredcarle added this to the DefraDB v0.12 milestone Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acp Related to the acp (access control) system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Defra's WASM compilation with the ACP Local feature
4 participants