Skip to content

Add building blocks for Acl plugin#5

Merged
mfornet merged 27 commits into
near:aclfrom
mooori:acl
Oct 4, 2022
Merged

Add building blocks for Acl plugin#5
mfornet merged 27 commits into
near:aclfrom
mooori:acl

Conversation

@mooori
Copy link
Copy Markdown
Contributor

@mooori mooori commented Sep 16, 2022

Acl: Access control lists

This PR adds building blocks for an Acl plugin, inspired by OpenZeppelin Access Control.

There is a contract in near-plugins/tests/contracts/access_controllable which shows how to use the plugin. In integration test near-plugins/tests/access_controllable.rs the contract is compiled, deployed and transactions are sent to it (all via workspaces-rs).

# Command to execute the tests:
cargo test --test access_controllable

Note: If this doesn't compile, #6 should fix it.

Overview

  • The contract developer specifies an enum whose variants represent roles.
  • The developer derives AccessControlRole for that enum. It defines a bitflag type that encodes permissions and implements helpers for the conversion between roles (i.e. variants of the enum) and bitflags.
  • The contract itself gets the attribute #[access_control] which injects the structure that manages permissions into the contract.
  • The developer can then use functions and attributes to interact with Acl, e.g.
    • self.acl_has_role(role, account_id))
    • #[access_control_any(roles(Role::LevelA, Role::LevelB)])

Roadmap

This PR adds the basics for Acl and the contract and integration test mentioned above ensure that everything is wired together correctly. Upcoming PRs will add functionality to make this plugin feature equivalent to OpenZeppelin Access Control.

The target branch is acl (instead of master) since there will be more PRs on top of this.

Comment thread near-plugins/src/access_control_role.rs Outdated
Comment thread near-plugins/tests/contracts/access_controllable/src/lib.rs Outdated
Comment thread near-plugins/tests/contracts/access_controllable/src/lib.rs Outdated
Copy link
Copy Markdown
Member

@mfornet mfornet left a comment

Choose a reason for hiding this comment

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

The code looks good. Please address the comments below and let's merge it. Then we need to add admin primitives and public functions.

Comment thread near-plugins/tests/access_controllable.rs Outdated
Comment thread near-plugins/tests/contracts/access_controllable/src/lib.rs
Comment thread near-plugins-derive/src/access_control_role.rs Outdated
Comment thread near-plugins-derive/src/access_control_role.rs Outdated
Comment thread near-plugins-derive/src/utils.rs Outdated
Comment thread near-plugins-derive/src/access_controllable.rs Outdated
Comment thread near-plugins-derive/src/access_controllable.rs
Comment thread near-plugins-derive/src/access_controllable.rs
#[private]
fn acl_grant_role_unchecked(&mut self, role: String, account_id: ::near_sdk::AccountId) -> bool {
let role = <#role_type>::try_from(role.as_str()).expect(#ERR_PARSE_ROLE);
self.#acl_field.grant_role_unchecked(role, &account_id)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you remind me what was the reason to have acl as an injected field versus having a function that builds this object dynamically? For example:

Suggested change
self.#acl_field.grant_role_unchecked(role, &account_id)
self.__acl_object().grant_role_unchecked(role, &account_id)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The part I don't like about injecting the code without letting developers know is that they might struggle to upgrade their contracts and serialize/deserialize the state later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If the field is not injected we (or the developers) have to manually (de)serialize and read/write the object from/to storage. Probably using storage_read and storage_write. We discussed offline to inject the field, to avoid the steps mentioned above.


I have not yet checked if it's feasible, but another approach might be:

  • Ask the developer to have a field __acl: (). The type this field should ultimately have is defined only inside the attribute macro. So I think developers can't write __acl: AclType in their contract.
  • Inside the macro, we change the type of this field (instead of injecting a new field).

Probably it wouldn't be a huge win, but makes it more explicit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

near-floating-state fits nicely here! Let's stay with injection for now to finalize this PR soon? Then once near-floating-state is on crates.io we can introduce it for Acl in a follow-up PR.

Comment thread near-plugins-derive/src/access_controllable.rs Outdated
@mooori mooori marked this pull request as ready for review September 28, 2022 16:55
@mooori mooori requested a review from mfornet September 28, 2022 16:55
Copy link
Copy Markdown
Member

@mfornet mfornet left a comment

Choose a reason for hiding this comment

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

LGTM. Merging now, and admin methods will be added in a follow-up PR.

This was referenced Mar 27, 2026
This was referenced Apr 7, 2026
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.

3 participants