Skip to content

Conversation

@paulo-ocean
Copy link
Contributor

@paulo-ocean paulo-ocean commented Feb 4, 2025

Fixes #816

Changes proposed in this PR:

  • On validateDDOCommand only getValidationSignature for authenticated requests
  • this requires changes at sdk level (and cli, if we publish with CLI) as we need to build/send a signature on the request
  • CLI and SDK related changes are addressed in separate PRs (system tests will fail until those are up to date)...
  • we need to merge: SDK -> CLI -> Nodes (in this order)
  • SDK PR add validate function signature changes, missing update docs and others ocean.js#1914

@paulo-ocean paulo-ocean self-assigned this Feb 4, 2025
@paulo-ocean
Copy link
Contributor Author

paulo-ocean commented Feb 5, 2025

this is blocked by #826
needs support for AUTHORIZED_PUBLISHERS & AUTHORIZED_PUBLISHERS_LIST

@paulo-ocean
Copy link
Contributor Author

this is blocked by #826 needs support for AUTHORIZED_PUBLISHERS & AUTHORIZED_PUBLISHERS_LIST

unblocked now

@paulo-ocean
Copy link
Contributor Author

blocked again by oceanprotocol/ocean.js#1913

@paulo-ocean paulo-ocean changed the title wip: getValidationSignature only for authentificated requests validateDDO: getValidationSignature only for authentificated requests Feb 24, 2025
@paulo-ocean paulo-ocean marked this pull request as ready for review March 25, 2025 10:18
@alexcos20 alexcos20 closed this Jan 6, 2026
@alexcos20 alexcos20 deleted the issue-816-validateddo-signature branch January 6, 2026 07:53
Copy link
Member

@alexcos20 alexcos20 left a 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:

  1. Valid DDO with valid publisherAddress, nonce, and signature.
  2. Valid DDO with invalid signature.
  3. Valid DDO with an expired or reused nonce.
  4. Valid DDO where publisherAddress lacks updateMetadata role.
  5. Valid DDO where publisherAddress is not in AUTHORIZED_PUBLISHERS or AUTHORIZED_PUBLISHERS_LIST.
  6. Valid DDO for an unsupported chain.
  7. Request missing publisherAddress, nonce, or signature (and confirm the 400 response is desired).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ValidateDDOCommand - getValidationSignature only for authentificated requests

3 participants