Skip to content

Extend onlyAuthorized to support extra functions in AccessManager #5014

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.

Sign up for GitHub

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

Merged
Amxx merged 8 commits into from
May 27, 2024
Merged

Extend onlyAuthorized to support extra functions in AccessManager #5014

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
37f874f
Extend `onlyAuthorized` to support extra functions
ernestognw Apr 10, 2024
d5be4dd
Update .changeset/light-news-listen.md
ernestognw Apr 10, 2024
f2d58aa
simplification
Amxx Apr 11, 2024
d0fd2ab
Address test comments
ernestognw Apr 11, 2024
3bda544
fix partch
Amxx May 23, 2024
8306341
fix tests
Amxx May 23, 2024
33a21f8
Allow closing the AccessManager
ernestognw May 24, 2024
5f81c0f
Codespell
ernestognw May 24, 2024
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
Prev Previous commit
Next Next commit
Address test comments
  • Loading branch information
@ernestognw
ernestognw committed Apr 11, 2024
commit d0fd2ab9b3d89b7bf42e0b1d4a490d5c283aceb8
2 changes: 1 addition & 1 deletion test/access/manager/AccessManager.behavior.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ function shouldBehaveLikeAManagedRestrictedOperation() {
}

/**
* @requires this.{manager,roles,calldata,role}
* @requires this.{target,manager,roles,calldata,role}
*/
function shouldBehaveLikeASelfRestrictedOperation() {
const getAccessPath = LIKE_COMMON_GET_ACCESS;
Expand Down
17 changes: 9 additions & 8 deletions test/access/manager/AccessManager.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1674,15 +1674,16 @@ describe('AccessManager', function () {
describe('access managed self operations', function () {
describe('when calling a restricted target function', function () {
beforeEach('set required role', function () {
this.method = this.target.fnRestricted.getFragment();
this.method = this.manager.fnRestricted.getFragment();
this.role = { id: 785913n };
this.manager.$_setTargetFunctionRole(this.target, this.method.selector, this.role.id);
Copy link
Collaborator

@Amxx Amxx Apr 11, 2024

Choose a reason for hiding this comment

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

Should we set the function role for this.manager, this.method.selector ? It feels like we are testing restricted operation on the target when we should test the restricted operation on the manager.

});

describe('restrictions', function () {
beforeEach('set method and args', function () {
this.calldata = this.target.interface.encodeFunctionData(this.method, []);
this.calldata = this.manager.interface.encodeFunctionData(this.method, []);
this.caller = this.user;
this.target = this.manager; // For shouldBehaveLikeASelfRestrictedOperation
});

shouldBehaveLikeASelfRestrictedOperation();
Expand All @@ -1692,11 +1693,11 @@ describe('AccessManager', function () {
await this.manager.$_grantRole(this.role.id, this.user, 0, 0);

await expect(
this.target.connect(this.user)[this.method.selector]({
this.manager.connect(this.user)[this.method.selector]({
data: this.calldata,
}),
)
.to.emit(this.target, 'CalledRestricted')
.to.emit(this.manager, 'CalledRestricted')
.withArgs(this.user);
});
});
Expand All @@ -1707,19 +1708,19 @@ describe('AccessManager', function () {
beforeEach('set required role', async function () {
this.role = { id: 879435n };
await this.manager.$_setTargetFunctionRole(
this.target,
this.target[method].getFragment().selector,
this.manager,
this.manager[method].getFragment().selector,
this.role.id,
);
});

it('succeeds called by anyone', async function () {
await expect(
this.target.connect(this.user)[method]({
this.manager.connect(this.user)[method]({
data: this.calldata,
}),
)
.to.emit(this.target, 'CalledUnrestricted')
.to.emit(this.manager, 'CalledUnrestricted')
.withArgs(this.user);
});
});
Expand Down