-
Notifications
You must be signed in to change notification settings - Fork 19
validateDDO: getValidationSignature only for authentificated requests #835
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
Conversation
|
this is blocked by #826 |
unblocked now |
|
blocked again by oceanprotocol/ocean.js#1913 |
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 enhances DDO validation by integrating cryptographic signature verification, nonce checks, and on-chain NFT permission verification. It also includes substantial refactoring for code clarity and consistency, particularly in handling API responses and Elasticsearch queries. A new type definition NftRoles and several utility functions for blockchain interaction and credential checks have been added. The changes impact the /assets/ddo/validate endpoint, making it more robust but also introducing stricter requirements for DDO updates.
Comments:
• [WARNING][style] These console.log statements should be removed before merging. They are typically used for debugging and should not be present in production code.
• [WARNING][style] This console.log statement should be removed before merging. Debugging output should not remain in production code.
• [WARNING][bug] The behavior of the validateDDO endpoint has changed significantly. Previously, it would return a (potentially empty) signature even if no publisherAddress, nonce, or signature were provided, given the DDO was structurally valid. Now, if these parameters are missing, it returns a 400 error. This is a breaking change for consumers who might have relied on the previous behavior for 'syntactic-only' validation.
Consider if a structurally valid DDO without signature info should still return a 200 status with an informative message (e.g., 'DDO is valid, but no signature provided for cryptographic verification') instead of a 400 error, to distinguish between malformed DDOs and DDOs that are simply not cryptographically verified in the request.
• [WARNING][bug] The getNftPermissions function directly awaits nftContract.getPermissions(addressToCheck). If nftAddress is not a valid ERC721Template contract, or if the contract call fails for other reasons (e.g., network issues, invalid ABI), this could lead to an unhandled exception crashing the process. Consider adding a try-catch block around the contract call to handle potential errors gracefully, returning default or error-specific NftRoles.
• [INFO][performance] The isAddress(task.ddo.nftAddress) check seems redundant here. The validateObject call on line 858 already ensures the DDO structure is valid, which would include nftAddress being present and formatted correctly if the schema requires it. If the schema validation for nftAddress is robust, this isAddress check might not be strictly necessary and could be removed to reduce redundancy.
• [WARNING][style] These console.log statements should be removed before merging. They are typically used for debugging and should not be present in production code.
• [INFO][clarity] The error message 'Invalid DDO data or missing DDO version' is a good improvement. It clearly communicates the issue to the client.
• [INFO][testing] Given the critical nature of these new validation steps (nonce, publisher permissions, authorized publishers list), comprehensive unit and integration tests are essential. Please ensure there are tests covering:
- Valid DDO with valid publisherAddress, nonce, and signature.
- Valid DDO with invalid signature.
- Valid DDO with an expired or reused nonce.
- Valid DDO where publisherAddress lacks
updateMetadatarole. - Valid DDO where publisherAddress is not in
AUTHORIZED_PUBLISHERSorAUTHORIZED_PUBLISHERS_LIST. - Valid DDO for an unsupported chain.
- Request missing
publisherAddress,nonce, orsignature(and confirm the 400 response is desired).
Fixes #816
Changes proposed in this PR:
validateDDOCommandonlygetValidationSignaturefor authenticated requests