Skip to content
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

refactor: move module access controller #56

Merged
merged 4 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion solidity/contracts/Oracle.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {IRequestModule} from '../interfaces/modules/request/IRequestModule.sol';
import {IResolutionModule} from '../interfaces/modules/resolution/IResolutionModule.sol';
import {IResponseModule} from '../interfaces/modules/response/IResponseModule.sol';
import {ValidatorLib} from '../libraries/ValidatorLib.sol';
import {OracleAccessController} from './OracleAccessController.sol';
import {OracleAccessController} from './access/OracleAccessController.sol';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I didn't like the files thrown in contracts/

import {OracleTypehash} from './utils/OracleTypehash.sol';

contract Oracle is IOracle, OracleAccessController {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {IAccessController} from '../interfaces/IAccessController.sol';
import {IAccessModule} from '../interfaces/modules/access/IAccessModule.sol';
import {IAccessController} from '../../interfaces/access/IAccessController.sol';
import {IAccessModule} from '../../interfaces/modules/access/IAccessModule.sol';

abstract contract CommonAccessController is IAccessController {
/**
Expand Down
50 changes: 50 additions & 0 deletions solidity/contracts/access/ModuleAccessController.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {IOracle} from '../../interfaces/IOracle.sol';
import {IModuleAccessController} from '../../interfaces/access/IModuleAccessController.sol';
import {Module} from '../Module.sol';
import {CommonAccessController} from './CommonAccessController.sol';

abstract contract ModuleAccessController is IModuleAccessController, CommonAccessController, Module {
constructor(IOracle _oracle) Module(_oracle) {}
Comment on lines +9 to +10
Copy link
Member

@0xJabberwock 0xJabberwock Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does ModuleAccessController inherit from Module, if it is not a module itself? In my opinion, it would make more sense to make the latter inherit from the former, such that:

  • Oracle is OracleAccessController and
  • Module is ModuleAccessController, while
  • MyAccessModule is Module.


/**
* @notice Returns an access control object using the contract address as user and the given data
* @dev should only be used by modules as the self-access-control object
* @param _data Arbitrary data
* @return _accessControl The self access control for this contract.
*/
function _defaultAccessControl(bytes memory _data) internal view returns (AccessControl memory _accessControl) {
_accessControl = AccessControl({user: address(this), data: _data});
}

/**
* @notice Returns an access control object using the contract address as user, and empty data
*
* @dev should only be used by modules as the self-access-control object
* @return _accessControl The self access control for this contract.
*/
function _defaultAccessControl() internal view returns (AccessControl memory _accessControl) {
_accessControl = _defaultAccessControl(bytes(''));
}

modifier hasAccess(
address _accessModule,
bytes32 _typehash,
bytes memory _params,
AccessControl memory _accessControl
) {
if (_accessControl.user != msg.sender) {
if (_accessModule == address(0)) {
revert AccessController_NoAccess();
} else {
if (!ORACLE.isAccessModuleApproved(_accessControl.user, _accessModule)) {
revert ModuleAccessController_AccessModuleNotApproved();
}
_hasAccess(_accessModule, _typehash, _params, _accessControl);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#55 will impact here.

Similarly in OracleAccessController.

}
}
_;
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {IOracleAccessController} from '../interfaces/IOracleAccessController.sol';
import {IOracleAccessController} from '../../interfaces/access/IOracleAccessController.sol';
import {CommonAccessController} from './CommonAccessController.sol';

abstract contract OracleAccessController is IOracleAccessController, CommonAccessController {
Expand Down
2 changes: 1 addition & 1 deletion solidity/interfaces/IOracle.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {IAccessController, IOracleAccessController} from './IOracleAccessController.sol';
import {IAccessController, IOracleAccessController} from './access/IOracleAccessController.sol';

/**
* @title Oracle
Expand Down
15 changes: 15 additions & 0 deletions solidity/interfaces/access/IModuleAccessController.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {IAccessController} from './IAccessController.sol';

/**
* @title Module Access Controller Interface
* @notice Interface for the module access controller
*/
interface IModuleAccessController is IAccessController {
/**
* @notice Thrown when user has not approved the access module
*/
error ModuleAccessController_AccessModuleNotApproved();
}
2 changes: 1 addition & 1 deletion solidity/interfaces/modules/access/IAccessModule.sol
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {IAccessController} from '../../IAccessController.sol';
import {IModule} from '../../IModule.sol';
import {IAccessController} from '../../access/IAccessController.sol';

/**
* @title AccessModule
Expand Down
2 changes: 1 addition & 1 deletion solidity/test/integration/IntegrationBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {IResolutionModule} from '../../interfaces/modules/resolution/IResolution
import {IResponseModule} from '../../interfaces/modules/response/IResponseModule.sol';

import {IOracle, Oracle} from '../../contracts/Oracle.sol';
import {IAccessController, IOracleAccessController} from '../../interfaces/IOracleAccessController.sol';
import {IAccessController, IOracleAccessController} from '../../interfaces/access/IOracleAccessController.sol';

import {IMockAccounting, MockAccounting} from '../mocks/contracts/MockAccounting.sol';
import {MockCallback} from '../mocks/contracts/MockCallback.sol';
Expand Down
2 changes: 1 addition & 1 deletion solidity/test/unit/CommonAccessController.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ pragma solidity ^0.8.19;

import 'forge-std/Test.sol';

import {CommonAccessController, IAccessController} from '../../contracts/CommonAccessController.sol';
import {CommonAccessController, IAccessController} from '../../contracts/access/CommonAccessController.sol';
import {IAccessController, IAccessModule} from '../../interfaces/modules/access/IAccessModule.sol';
import {Helpers} from '../utils/Helpers.sol';

Expand Down
184 changes: 184 additions & 0 deletions solidity/test/unit/ModuleAccessController.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {IModuleAccessController, ModuleAccessController} from '../../contracts/access/ModuleAccessController.sol';
import {IOracle} from '../../interfaces/IOracle.sol';

import {IOracleAccessController} from '../../interfaces/access/IOracleAccessController.sol';
import {IAccessController, IAccessModule} from '../../interfaces/modules/access/IAccessModule.sol';
import {Helpers} from '../utils/Helpers.sol';
import 'forge-std/Test.sol';

contract MockModuleAccessController is ModuleAccessController {
constructor(IOracle _oracle) ModuleAccessController(_oracle) {}

function hasAccessForTest(
address _accessModule,
bytes32 _typehash,
bytes memory _params,
AccessControl memory _accessControl
) public hasAccess(_accessModule, _typehash, _params, _accessControl) {}

function moduleName() external pure returns (string memory _moduleName) {
_moduleName = 'MockModuleAccessController';
}
}

/**
* @title Module Abstract Unit tests
*/
contract BaseTest is Test, Helpers {
// A mock access controller
MockModuleAccessController public moduleAccessController;
// The oracle
IOracle public oracle;
// Mock caller
address public caller;

// Events
event AccessModuleSet(address indexed user, address indexed accessModule, bool approved);

Check warning on line 39 in solidity/test/unit/ModuleAccessController.t.sol

View workflow job for this annotation

GitHub Actions / Run Linters (20.x)

'user' should start with _

Check warning on line 39 in solidity/test/unit/ModuleAccessController.t.sol

View workflow job for this annotation

GitHub Actions / Run Linters (20.x)

'accessModule' should start with _

Check warning on line 39 in solidity/test/unit/ModuleAccessController.t.sol

View workflow job for this annotation

GitHub Actions / Run Linters (20.x)

'approved' should start with _

/**
* @notice Deploy the access controller
*/
function setUp() public {
oracle = IOracle(makeAddr('Oracle'));
moduleAccessController = new MockModuleAccessController(oracle);
caller = makeAddr('Caller');
}

function _setAccessModuleForTest(address _user, address _accessModule, bool _approved) public {

Check warning on line 50 in solidity/test/unit/ModuleAccessController.t.sol

View workflow job for this annotation

GitHub Actions / Run Linters (20.x)

'_setAccessModuleForTest' should not start with _
vm.mockCall(
address(oracle),
abi.encodeWithSelector(IOracleAccessController.isAccessModuleApproved.selector, _user, _accessModule),
abi.encode(_approved)
);
}
}

contract ModuleAccessController_Unit_HasAccess is BaseTest {
modifier happyPath(
address _accessModule,
bytes32 _typehash,
bytes memory _params,
IAccessController.AccessControl memory _accessControl
) {
vm.assume(_accessControl.user != caller);
vm.assume(_accessModule != address(0) && _accessModule != caller);
vm.assume(_params.length < 32);

_setAccessModuleForTest(_accessControl.user, _accessModule, true);

vm.startPrank(caller);
_;
}

function test_revertIfNoAccessModuleAndSenderIsNotUser(
address _accessModule,
bytes32 _typehash,
bytes memory _params,
IAccessController.AccessControl memory _accessControl
) public happyPath(_accessModule, _typehash, _params, _accessControl) {
// Expect the revert
vm.expectRevert(abi.encodeWithSelector(IAccessController.AccessController_NoAccess.selector));

// Call the hasAccess function
moduleAccessController.hasAccessForTest(address(0), _typehash, _params, _accessControl);
}

function test_revertIfAccessModuleNotApproved(
address _accessModule,
bytes32 _typehash,
bytes memory _params,
IAccessController.AccessControl memory _accessControl
) public happyPath(_accessModule, _typehash, _params, _accessControl) {
// Ensure the access module is not approved
_setAccessModuleForTest(_accessControl.user, _accessModule, false);

// Expect the revert
vm.expectRevert(
abi.encodeWithSelector(IModuleAccessController.ModuleAccessController_AccessModuleNotApproved.selector)
);

// Call the hasAccess function
moduleAccessController.hasAccessForTest(_accessModule, _typehash, _params, _accessControl);
}

function test_revertIfNoHasAccess(
address _accessModule,
bytes32 _typehash,
bytes memory _params,
IAccessController.AccessControl memory _accessControl
) public happyPath(_accessModule, _typehash, _params, _accessControl) {
IAccessModule.AccessControlParameters memory _expectedParams = IAccessModule.AccessControlParameters({
sender: caller,
accessControl: _accessControl,
typehash: _typehash,
typehashParams: _params
});

// Expect the hasAccess function to not be called
_mockAndExpect(
_accessModule,
abi.encodeWithSelector(IAccessModule.hasAccess.selector, abi.encode(_expectedParams)),
abi.encode(false)
);

// Expect the revert
vm.expectRevert(abi.encodeWithSelector(IAccessController.AccessController_NoAccess.selector));

// Call the hasAccess function
moduleAccessController.hasAccessForTest(_accessModule, _typehash, _params, _accessControl);
}

function test_hasAccessSenderIsNotUser(
address _accessModule,
bytes32 _typehash,
bytes memory _params,
IAccessController.AccessControl memory _accessControl
) public happyPath(_accessModule, _typehash, _params, _accessControl) {
IAccessModule.AccessControlParameters memory _expectedParams = IAccessModule.AccessControlParameters({
sender: caller,
accessControl: _accessControl,
typehash: _typehash,
typehashParams: _params
});

// Expect the hasAccess function to be called
_mockAndExpect(
_accessModule,
abi.encodeWithSelector(IAccessModule.hasAccess.selector, abi.encode(_expectedParams)),
abi.encode(true)
);

// Call the hasAccess function
moduleAccessController.hasAccessForTest(_accessModule, _typehash, _params, _accessControl);
}

function test_hasAccessSenderIsUser(
address _accessModule,
bytes32 _typehash,
bytes memory _params,
IAccessController.AccessControl memory _accessControl
) public happyPath(_accessModule, _typehash, _params, _accessControl) {
_setAccessModuleForTest(caller, _accessModule, true);
_accessControl.user = caller;
// We expect the hasAccess function to not be called
vm.expectCall(
_accessModule,
abi.encodeWithSelector(
IAccessModule.hasAccess.selector,
abi.encode(
IAccessModule.AccessControlParameters({
sender: caller,
accessControl: _accessControl,
typehash: _typehash,
typehashParams: _params
})
)
),
0
);
moduleAccessController.hasAccessForTest(_accessModule, _typehash, _params, _accessControl);
}
}
2 changes: 1 addition & 1 deletion solidity/test/unit/Oracle.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {IOracle} from '../../interfaces/IOracle.sol';

import {IDisputeModule} from '../../interfaces/modules/dispute/IDisputeModule.sol';

import {IAccessController, IOracleAccessController} from '../../interfaces/IOracleAccessController.sol';
import {IAccessController, IOracleAccessController} from '../../interfaces/access/IOracleAccessController.sol';
import {IAccessModule} from '../../interfaces/modules/access/IAccessModule.sol';
import {IFinalityModule} from '../../interfaces/modules/finality/IFinalityModule.sol';
import {IRequestModule} from '../../interfaces/modules/request/IRequestModule.sol';
Expand Down
2 changes: 1 addition & 1 deletion solidity/test/unit/OracleAccessController.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import 'forge-std/Test.sol';

import {IOracleAccessController, OracleAccessController} from '../../contracts/OracleAccessController.sol';
import {IOracleAccessController, OracleAccessController} from '../../contracts/access/OracleAccessController.sol';
import {IAccessController, IAccessModule} from '../../interfaces/modules/access/IAccessModule.sol';
import {Helpers} from '../utils/Helpers.sol';

Expand Down Expand Up @@ -32,7 +32,7 @@
address public caller;

// Events
event AccessModuleSet(address indexed user, address indexed accessModule, bool approved);

Check warning on line 35 in solidity/test/unit/OracleAccessController.t.sol

View workflow job for this annotation

GitHub Actions / Run Linters (20.x)

'user' should start with _

Check warning on line 35 in solidity/test/unit/OracleAccessController.t.sol

View workflow job for this annotation

GitHub Actions / Run Linters (20.x)

'accessModule' should start with _

Check warning on line 35 in solidity/test/unit/OracleAccessController.t.sol

View workflow job for this annotation

GitHub Actions / Run Linters (20.x)

'approved' should start with _

/**
* @notice Deploy the access controller
Expand Down
2 changes: 1 addition & 1 deletion solidity/test/utils/Helpers.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
pragma solidity ^0.8.19;

import {IOracle} from '../../contracts/Oracle.sol';
import {IAccessController} from '../../interfaces/IAccessController.sol';
import {IAccessController} from '../../interfaces/access/IAccessController.sol';
import {Test} from 'forge-std/Test.sol';

contract Helpers is Test {
Expand Down
Loading