Feat/gov exec #24
Feat/gov exec #24
Conversation
WalkthroughThis pull request introduces several enhancements to the codebase, including the addition of a new linter ( Changes
Sequence Diagram(s)sequenceDiagram
participant AuthProxy
participant GovernanceSystem
participant Client
AuthProxy->>GovernanceSystem: AskGovPermittedActions()
GovernanceSystem-->>AuthProxy: Return permitted actions
AuthProxy->>Client: Authenticate()
sequenceDiagram
participant Proxy
participant DataverseClient
participant LawStoneClient
Proxy->>DataverseClient: AskGovTellAction()
DataverseClient-->>Proxy: Return action permission
Proxy->>LawStoneClient: Store()
LawStoneClient-->>Proxy: Confirm storage
Tip Announcements
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review due to trivial changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
amimart
force-pushed
the
feat/gov-exec
branch
from
e11374a to
65d8de2
Compare
Codecov ReportAttention: Patch coverage is
|
amimart
force-pushed
the
feat/gov-exec
branch
from
d828fc3 to
75b1df8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range, codebase verification and nitpick comments (1)
provider/storage/proxy.go (1)
Line range hint
38-58: LGTM!The changes to the
NewProxyconstructor are approved.The modifications to accept
baseURLas a parameter and initialize a new field in theProxystruct are necessary to support the new functionality of constructing URLs dynamically.The modification to ensure a trailing slash is a good practice to maintain consistency.
The static analysis tool has reported that lines 49-50, 55, 57-58 are not covered by tests.
Do you want me to generate the missing test cases to improve the code coverage?
Tools
GitHub Check: codecov/patch
[warning] 49-50: provider/storage/proxy.go#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 55-55: provider/storage/proxy.go#L55
Added line #L55 was not covered by tests
[warning] 57-58: provider/storage/proxy.go#L57-L58
Added lines #L57 - L58 were not covered by tests
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (12)
- .golangci.yml (1 hunks)
- Makefile (1 hunks)
- auth/proxy.go (1 hunks)
- auth/proxy_test.go (1 hunks)
- dataverse/client.go (3 hunks)
- dataverse/export_test.go (1 hunks)
- dataverse/governance.go (2 hunks)
- dataverse/governance_test.go (3 hunks)
- provider/storage/http.go (2 hunks)
- provider/storage/proxy.go (6 hunks)
- testutil/dataverse_mocks.go (1 hunks)
- testutil/law_stone_client_mocks.go (1 hunks)
Additional context used
GitHub Check: codecov/patch
provider/storage/http.go
[warning] 30-31: provider/storage/http.go#L30-L31
Added lines #L30 - L31 were not covered by tests
[warning] 36-36: provider/storage/http.go#L36
Added line #L36 was not covered by tests
[warning] 39-39: provider/storage/http.go#L39
Added line #L39 was not covered by tests
[warning] 47-48: provider/storage/http.go#L47-L48
Added lines #L47 - L48 were not covered by tests
[warning] 53-53: provider/storage/http.go#L53
Added line #L53 was not covered by tests
[warning] 56-56: provider/storage/http.go#L56
Added line #L56 was not covered by testsauth/proxy.go
[warning] 54-54: auth/proxy.go#L54
Added line #L54 was not covered by testsdataverse/client.go
[warning] 65-67: dataverse/client.go#L65-L67
Added lines #L65 - L67 were not covered by testsprovider/storage/proxy.go
[warning] 49-50: provider/storage/proxy.go#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 55-55: provider/storage/proxy.go#L55
Added line #L55 was not covered by tests
[warning] 57-58: provider/storage/proxy.go#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 78-78: provider/storage/proxy.go#L78
Added line #L78 was not covered by tests
[warning] 83-84: provider/storage/proxy.go#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 99-105: provider/storage/proxy.go#L99-L105
Added lines #L99 - L105 were not covered by tests
[warning] 108-110: provider/storage/proxy.go#L108-L110
Added lines #L108 - L110 were not covered by tests
[warning] 113-113: provider/storage/proxy.go#L113
Added line #L113 was not covered by teststestutil/dataverse_mocks.go
[warning] 43-43: testutil/dataverse_mocks.go#L43
Added line #L43 was not covered by tests
[warning] 45-46: testutil/dataverse_mocks.go#L45-L46
Added lines #L45 - L46 were not covered by tests
[warning] 52-52: testutil/dataverse_mocks.go#L52
Added line #L52 was not covered by tests
[warning] 54-54: testutil/dataverse_mocks.go#L54
Added line #L54 was not covered by tests
[warning] 58-63: testutil/dataverse_mocks.go#L58-L63
Added lines #L58 - L63 were not covered by tests
[warning] 67-69: testutil/dataverse_mocks.go#L67-L69
Added lines #L67 - L69 were not covered by teststestutil/law_stone_client_mocks.go
[warning] 33-36: testutil/law_stone_client_mocks.go#L33-L36
Added lines #L33 - L36 were not covered by tests
[warning] 40-41: testutil/law_stone_client_mocks.go#L40-L41
Added lines #L40 - L41 were not covered by tests
[warning] 45-49: testutil/law_stone_client_mocks.go#L45-L49
Added lines #L45 - L49 were not covered by tests
[warning] 51-54: testutil/law_stone_client_mocks.go#L51-L54
Added lines #L51 - L54 were not covered by tests
[warning] 58-61: testutil/law_stone_client_mocks.go#L58-L61
Added lines #L58 - L61 were not covered by tests
[warning] 65-69: testutil/law_stone_client_mocks.go#L65-L69
Added lines #L65 - L69 were not covered by tests
[warning] 71-74: testutil/law_stone_client_mocks.go#L71-L74
Added lines #L71 - L74 were not covered by tests
[warning] 78-81: testutil/law_stone_client_mocks.go#L78-L81
Added lines #L78 - L81 were not covered by tests
Gitleaks
dataverse/governance_test.go
173-173: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
182-182: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
191-191: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
204-204: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
242-242: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
263-263: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
341-341: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
351-351: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
361-361: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
375-375: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
393-393: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
415-415: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (10)
dataverse/export_test.go (1)
11-11: LGTM!The changes to the
NewDataverseClientfunction are approved:
- The new
lawStoneFactoryparameter allows the function to accept an additional dependency.- The
clientstruct returned by the function now includes thelawStoneFactoryfield, enabling interactions with theLawStoneFactorywithin the constructed client.The changes are consistent with the provided summary and expand the functionality without introducing any issues.
Also applies to: 16-16
.golangci.yml (1)
Line range hint
22-22: LGTM!The changes to the
.golangci.ymlconfiguration file are approved:
- The addition of the
goconstlinter enhances the static analysis capabilities of the project by detecting repeated string and numeric literals in the code.- Excluding the
goconstlinter for test files using theexclude-rulessection is a reasonable decision to avoid false positives and allow for more flexibility in test code.The changes are consistent with the provided summary and improve the code quality without introducing any issues.
Also applies to: 96-96
dataverse/governance.go (2)
34-60: LGTM!The code changes are approved. The new
AskGovPermittedActionsmethod enhances the functionality of the governance module by allowing for dynamic querying of permitted actions. The error handling ensures robustness in case of failures.
62-80: LGTM!The code changes are approved. The new
AskGovTellActionmethod further enhances the functionality of the governance module by allowing for checking specific action permissions. The error handling ensures robustness in case of failures.auth/proxy_test.go (1)
66-66: Verify the impact of the change on the test case.The mocked method call has been changed from
ExecGovtoAskGovPermittedActions, which signifies a shift in the focus of the test from executing government actions to querying permitted actions.Please ensure that this change aligns with the intended behavior of the
AuthProxyand does not adversely impact the test coverage or the functionality being validated.Makefile (1)
84-84: LGTM!The addition of the command to generate mocks for
LawStoneQueryClientenhances the testing capabilities. The changes are approved.testutil/law_stone_client_mocks.go (1)
1-102: Skipping review of generated mock code.This file contains generated mock code for
LawStoneQueryClient. It's not meant to be modified manually.The lack of test coverage reported by
codecov/patchfor the generated mock code is expected and can be ignored.Tools
GitHub Check: codecov/patch
[warning] 33-36: testutil/law_stone_client_mocks.go#L33-L36
Added lines #L33 - L36 were not covered by tests
[warning] 40-41: testutil/law_stone_client_mocks.go#L40-L41
Added lines #L40 - L41 were not covered by tests
[warning] 45-49: testutil/law_stone_client_mocks.go#L45-L49
Added lines #L45 - L49 were not covered by tests
[warning] 51-54: testutil/law_stone_client_mocks.go#L51-L54
Added lines #L51 - L54 were not covered by tests
[warning] 58-61: testutil/law_stone_client_mocks.go#L58-L61
Added lines #L58 - L61 were not covered by tests
[warning] 65-69: testutil/law_stone_client_mocks.go#L65-L69
Added lines #L65 - L69 were not covered by tests
[warning] 71-74: testutil/law_stone_client_mocks.go#L71-L74
Added lines #L71 - L74 were not covered by tests
[warning] 78-81: testutil/law_stone_client_mocks.go#L78-L81
Added lines #L78 - L81 were not covered by testsdataverse/governance_test.go (3)
173-173: Ignore Gitleaks warnings.The strings flagged by Gitleaks as potential API key leaks are actually test DIDs, not real API keys. These warnings can be ignored as false positives.
Also applies to: 182-182, 191-191, 204-204, 221-221, 242-242, 263-263, 341-341, 351-351, 361-361, 375-375, 393-393, 415-415
Tools
Gitleaks
173-173: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
160-325: LGTM!The
TestClient_AskGovPermittedActionsfunction thoroughly tests theAskGovPermittedActionsmethod of thedataverseclient. It covers various scenarios, including error handling and expected outcomes based on different responses from the law-stone client. The test cases are well-structured and comprehensive. The changes are approved.Tools
Gitleaks
173-173: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
182-182: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
191-191: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
204-204: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
242-242: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
263-263: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
327-477: LGTM!The
TestClient_AskGovTellActionfunction thoroughly tests theAskGovTellActionmethod of thedataverseclient. It covers scenarios for both successful and erroneous responses, validating that the client correctly interprets the results. The test cases are well-structured and comprehensive. The changes are approved.Tools
Gitleaks
341-341: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
351-351: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
361-361: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
375-375: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
393-393: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
415-415: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Wire the governance/consents checks by asking the resource's
law-stonesmart contract instance.Details
The
AskGovPermittedActions&AskGovTellActionmethods of thedataverse.Clientallows respectively to ask the allowed actions for a DID and tell if an action is permitted for a DID.Those are now used when authenticating an identity and when requesting to read or store a resource on a
storage.Proxy.It is using under the hood the
Askquery message of thelaw-stonesmart contract with thetell_permitted_actions(Who, Actions).&tell(Who, Action, Result, Evidence).prolog queries.I've taken the opportunity to issue the publication credential when storing a new resource.