-
Notifications
You must be signed in to change notification settings - Fork 19
start refactoring creds behaviour for address #849
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
base: main
Are you sure you want to change the base?
Conversation
src/utils/credentials.ts
Outdated
| // if no address-based credentials are defined (both allow and deny lists are empty), access to the asset is restricted to everybody; | ||
| // to allow access to everybody, the symbol * will be used in the allow list; | ||
| // if a web3 address is present on both deny and allow lists, the deny list takes precedence | ||
| // and access to the asset is denied for the respective address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of surprised that we agreed to implement this change. So the default behaviour is that if an asset published, no one can access it? Seems contrary to our mission statement 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's copied from the specs yes #804 #810 OceanProtocolEnterprise#25
makes perfectly sense to me for security reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to enterprise users... In the past Ocean has always taken the approach that everything is open by default and Enterprises must take extra steps if they don't like the default. So this is a fundamental shift in that approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not gonna discuss that... please check with @alexcos20 . I just followed the specs and they make sense to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything changing if we have a discussion, you may as well just go ahead. Nothing wrong with the implementation
jamiehewitt15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
…-accessList add support for accessList on download checkCredentials
# Conflicts: # src/@types/DDO/Credentials.ts # src/test/unit/credentials.test.ts # src/utils/credentials.ts
mariacarmina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
# Conflicts: # package-lock.json # package.json
alexcos20
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This pull request refactors and enhances the credential checking mechanism, primarily driven by an update to the @oceanprotocol/ddo-js library (from 0.0.6 to 0.0.8). The core checkCredentials function has been made asynchronous, now supports accessList type credentials by interacting with blockchain contracts, and introduces match_allow and match_deny rules (supporting 'any' and 'all' logic). Crucially, the default behavior for undefined or empty credentials has shifted from implicitly allowing access to explicitly denying it, which is a significant security improvement. Unit tests have been updated and extended to cover these new behaviors and asynchronous calls.
Comments:
• [WARNING][security] The current logic accessGrantedDDOLevel = areKnownCredentialTypes(ddo.credentials) ? await checkCredentials(...) : true implies that if the DDO contains unknown credential types (areKnownCredentialTypes returns false), access is automatically granted. This could be a security vulnerability if an attacker uses an unknown credential type to bypass checks. It might be safer to default to false (deny access) if unknown credential types are encountered, or to log a severe warning.
Consider changing true to false here, or handling unknown types explicitly with a security policy.
• [WARNING][security] Similar to the DDO-level credentials, the logic accessGrantedServiceLevel = areKnownCredentialTypes(service.credentials) ? await checkCredentials(...) : true could lead to a security bypass if service-level credentials contain unknown types. If areKnownCredentialTypes returns false, access is currently granted.
Consider changing true to false here to deny access by default for unknown credential types at the service level.
• [INFO][style] The findCredential function (which was removed) used to map values to lowercase. The new isAddressCredentialMatch function also maps to lowercase. Ensure this case-insensitivity is consistently applied where needed for address comparisons to prevent unexpected mismatches.
• [INFO][other] The checkAccessListCredential function handles chainId not being provided or supportedNetwork not found by returning false. This is good fail-safe behavior. However, it's worth noting that if a DDO specifies an accessList credential but no chainId is provided in the context of the check, this credential type will effectively be ignored (denied).
This behavior is acceptable, but it should be clearly documented for users creating DDOs with accessList credentials.
• [INFO][test] The unit tests cover the new match_allow: any and match_allow: all logic, as well as the default deny behavior. This is excellent. However, there aren't specific tests that mock and verify the behavior of checkAccessListCredential interacting with a simulated AccessList contract. This would involve mocking the getBlockchainHandler, getSigner, and getNFTContract calls, and then simulating findAccountFromAccessList responses.
While the try-catch in findAccessListCredentials provides safety, explicit tests for both successful and failed access list lookups would increase confidence in this critical path.
• [INFO][dependency] The @oceanprotocol/ddo-js package has been updated from 0.0.6 to 0.0.8. This update involves changes in its direct dependencies (e.g., removal of crypto, jose, @rdfjs/dataset and addition of rdf-data-factory, rdf-literal). Given the impact on credential logic, it's a significant change. Ensure that the new ddo-js version is fully compatible and any breaking changes or updated assumptions are handled.
alexcos20
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This pull request significantly refactors the credential checking mechanism within Ocean Node. It upgrades the @oceanprotocol/ddo-js dependency from 0.0.6 to 0.0.8, centralizing credential type definitions. The checkCredentials function is made asynchronous to support on-chain checks for accessList credentials and its internal logic has been rewritten to explicitly handle allow and deny lists with match_allow and match_deny rules. Notably, the default behavior for credential validation has changed: if no explicit allow or deny rules match, access is now denied by default, contrasting with the previous implicit allowance. New helper functions (isValidCredentialsList, isAddressCredentialMatch, checkAddressCredential, checkAccessListCredential, checkDenyList, checkAllowList, isAddressMatchAll) have been introduced to modularize the logic, and a utility function getBlockchainHandler has been added for blockchain interactions.
Comments:
• [ERROR][bug] The default behavior of checkCredentials has changed from implicitly allowing access (returning true for undefined or empty credentials/lists) to explicitly denying access (returning false). This is a breaking change that impacts the security posture and could unintentionally restrict access for existing assets or configurations that relied on the previous lenient default. Please confirm this change is intentional and consider explicit migration paths or clear documentation for users.
• [ERROR][security] The checkDenyList function currently only evaluates CREDENTIALS_TYPES.ADDRESS rules. It does not include logic to check CREDENTIALS_TYPES.ACCESS_LIST rules for denial. If an accessList is specified in the deny array, users belonging to that access list would still be granted access, which is a potential security vulnerability. Please extend the checkDenyList to correctly evaluate accessList credentials.
• [WARNING][bug] In checkAddressCredential, if an address matches an ADDRESS type rule or a wildcard * rule, it immediately returns true if match_allow is 'any' (or undefined). This logic effectively short-circuits the match_allow: 'all' condition for the ADDRESS credential type, potentially allowing access even if other required credential types in the allow list (e.g., ACCESS_LIST) are not met when match_allow: 'all' is specified. Please clarify if match_allow: 'all' applies across all credential types in the allow list, or only within a specific credential type. If it applies across all types, the short-circuit for ADDRESS needs to be reconsidered.
• [INFO][other] The @oceanprotocol/ddo-js dependency has been updated, and package-lock.json shows significant changes in its transitive dependencies, including the removal of crypto, jose, lodash, and axios. While this might be an internal optimization within ddo-js, it's worth noting these changes as they might have implications for bundle size, security audits, or potential conflicts if other parts of ocean-node also directly used older versions of these libraries for different purposes. Please ensure that these dependency changes do not negatively impact other functionalities or introduce new vulnerabilities.
• [INFO][style] The new getBlockchainHandler utility function is a good addition for reducing boilerplate code. Consider adding a JSDoc comment for its purpose, parameters, and return value.
Fixes #810 #804
Changes proposed in this PR: