From 563f1bbc648c21d0d81d8d37c8e338d24beee482 Mon Sep 17 00:00:00 2001 From: eutopian Date: Mon, 9 Dec 2024 17:59:51 -0500 Subject: [PATCH] ensure that workflow id is unique --- .../v0.8/workflow/dev/WorkflowRegistry.sol | 47 +++++++++++++++---- .../WorkflowRegistry.registerWorkflow.t.sol | 29 ++++++++++++ .../WorkflowRegistry.registerWorkflow.tree | 2 + .../WorkflowRegistry.updateWorkflow.t.sol | 25 ++++++++++ .../WorkflowRegistry.updateWorkflow.tree | 2 + 5 files changed, 97 insertions(+), 8 deletions(-) diff --git a/contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol b/contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol index 0e6ae3450ac..b6ddeb9ae8c 100644 --- a/contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol +++ b/contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol @@ -43,6 +43,8 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { /// @dev Mapping to track workflows by secretsURL hash (owner + secretsURL). /// This is used to find all workflows that have the same secretsURL when a force secrets update event is requested. mapping(bytes32 secretsURLHash => EnumerableSet.Bytes32Set workflowKeys) private s_secretsHashToWorkflows; + /// @dev Keep track of all workflowIDs to ensure uniqueness. + mapping(bytes32 workflowID => bool inUse) private s_usedWorkflowIDs; /// @dev List of all authorized EOAs/contracts allowed to access this contract's state functions. All view functions are open access. EnumerableSet.AddressSet private s_authorizedAddresses; @@ -203,13 +205,15 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { ) external registryNotLocked { _validatePermissions(donID, msg.sender); _validateWorkflowName(bytes(workflowName).length); - _validateWorkflowMetadata(workflowID, bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length); + _validateWorkflowURLs(bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length); bytes32 workflowKey = computeHashKey(msg.sender, workflowName); if (s_workflows[workflowKey].owner != address(0)) { revert WorkflowAlreadyRegistered(); } + _requireUniqueWorkflowID(workflowID); + // Create new workflow entry s_workflows[workflowKey] = WorkflowMetadata({ workflowID: workflowID, @@ -272,7 +276,7 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { string calldata configURL, string calldata secretsURL ) external registryNotLocked { - _validateWorkflowMetadata(newWorkflowID, bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length); + _validateWorkflowURLs(bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length); WorkflowMetadata storage workflow = _getWorkflowFromStorage(msg.sender, workflowKey); @@ -295,6 +299,12 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { revert WorkflowContentNotUpdated(); } + // Ensure the new workflowID is unique + _requireUniqueWorkflowID(newWorkflowID); + + // Free the old workflowID + _releaseWorkflowID(currentWorkflowID); + // Update all fields that have changed and the relevant sets workflow.workflowID = newWorkflowID; if (!sameBinaryURL) { @@ -387,6 +397,9 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { revert AddressNotAuthorized(msg.sender); } + // Release the workflowID for reuse + _releaseWorkflowID(workflow.workflowID); + // Remove the workflow from the owner and DON mappings s_ownerWorkflowKeys[msg.sender].remove(workflowKey); s_donWorkflowKeys[workflow.donID].remove(workflowKey); @@ -508,6 +521,28 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { return workflow; } + /// @notice Ensures the given workflowID is unique and marks it as used. + /// @param workflowID The workflowID to validate and consume. + function _requireUniqueWorkflowID( + bytes32 workflowID + ) internal { + if (workflowID == bytes32(0)) revert InvalidWorkflowID(); + + if (s_usedWorkflowIDs[workflowID]) { + revert WorkflowIDAlreadyExists(); + } + + s_usedWorkflowIDs[workflowID] = true; + } + + /// @notice Frees up a previously used workflowID, allowing it to be reused. + /// @param workflowID The workflowID to release. + function _releaseWorkflowID( + bytes32 workflowID + ) internal { + s_usedWorkflowIDs[workflowID] = false; + } + // ================================================================ // | Workflow Queries | // ================================================================ @@ -636,16 +671,12 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { // | Validation | // ================================================================ - /// @dev Internal function to validate the metadata for a workflow. - /// @param workflowID The unique identifier for the workflow. - function _validateWorkflowMetadata( - bytes32 workflowID, + /// @dev Internal function to validate the urls for a workflow. + function _validateWorkflowURLs( uint256 binaryURLLength, uint256 configURLLength, uint256 secretsURLLength ) internal pure { - if (workflowID == bytes32(0)) revert InvalidWorkflowID(); - if (binaryURLLength > MAX_URL_LENGTH) { revert URLTooLong(binaryURLLength, MAX_URL_LENGTH); } diff --git a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.t.sol b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.t.sol index 426fbfcc502..859437196cd 100644 --- a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.t.sol +++ b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.t.sol @@ -138,6 +138,35 @@ contract WorkflowRegistry_registerWorkflow is WorkflowRegistrySetup { ); } + // whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed + function test_RevertWhen_TheWorkflowIDIsAlreadyInUsedByAnotherWorkflow() external { + vm.startPrank(s_authorizedAddress); + + // Register a valid workflow first + s_registry.registerWorkflow( + s_validWorkflowName, + s_validWorkflowID, + s_allowedDonID, + WorkflowRegistry.WorkflowStatus.ACTIVE, + s_validBinaryURL, + s_validConfigURL, + s_validSecretsURL + ); + + vm.expectRevert(WorkflowRegistry.WorkflowIDAlreadyExists.selector); + s_registry.registerWorkflow( + "ValidWorkflow2", + s_validWorkflowID, + s_allowedDonID, + WorkflowRegistry.WorkflowStatus.ACTIVE, + s_validBinaryURL, + s_validConfigURL, + s_validSecretsURL + ); + + vm.stopPrank(); + } + // whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed function test_RevertWhen_TheWorkflowNameIsAlreadyUsedByTheOwner() external { vm.startPrank(s_authorizedAddress); diff --git a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.tree b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.tree index 75cdf940575..eabbf58d464 100644 --- a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.tree +++ b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.tree @@ -18,6 +18,8 @@ WorkflowRegistry.registerWorkflow │ └── it should revert ├── when the workflowID is invalid │ └── it should revert + ├── when the workflowID is already in used by another workflow + │ └── it should revert ├── when the workflow name is already used by the owner │ └── it should revert └── when the workflow inputs are all valid diff --git a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.t.sol b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.t.sol index 5058512ba7b..93ef12a9f19 100644 --- a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.t.sol +++ b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.t.sol @@ -158,6 +158,31 @@ contract WorkflowRegistry_updateWorkflow is WorkflowRegistrySetup { s_registry.updateWorkflow(s_validWorkflowKey, bytes32(0), s_validBinaryURL, s_validConfigURL, s_newValidSecretsURL); } + // whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed whenTheCallerIsTheWorkflowOwner + function test_RevertWhen_TheWorkflowIDIsAlreadyInUsedByAnotherWorkflow() external { + // Register a workflow first + _registerValidWorkflow(); + + // Register another workflow with another workflow ID + vm.prank(s_authorizedAddress); + s_registry.registerWorkflow( + "ValidWorkflow2", + s_newValidWorkflowID, + s_allowedDonID, + WorkflowRegistry.WorkflowStatus.ACTIVE, + s_validBinaryURL, + s_validConfigURL, + s_validSecretsURL + ); + + // Update the workflow with a workflow ID that is already in use by another workflow. + vm.prank(s_authorizedAddress); + vm.expectRevert(WorkflowRegistry.WorkflowIDAlreadyExists.selector); + s_registry.updateWorkflow( + s_validWorkflowKey, s_newValidWorkflowID, s_validBinaryURL, s_validConfigURL, s_newValidSecretsURL + ); + } + // whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed whenTheCallerIsTheWorkflowOwner function test_WhenTheWorkflowInputsAreAllValid() external { // Register a workflow first. diff --git a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.tree b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.tree index 0d4da7cb32e..9b8243a8672 100644 --- a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.tree +++ b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.tree @@ -25,6 +25,8 @@ WorkflowRegistry.updateWorkflow │ └── it should revert ├── when the workflowID is invalid │ └── it should revert + ├── when the workflowID is already in used by another workflow + │ └── it should revert └── when the workflow inputs are all valid ├── it should update the existing workflow in s_workflows with the new values ├── it should emit {WorkflowUpdatedV1}