Add functions to manage Acl (super-)admins by mooori · Pull Request #8 · near/near-plugins

mooori · 2022-10-05T08:24:34Z

This PR builds on top of #5 and adds Acl functions related to (super-)admin permissions. A super-admin may act as admin for every role, these tests provide an overview of super-admin permissions.

Overview

Trait methods are added to AccessControllable.
The attribute #[access_controllable] adds implementations of these methods.
The new methods are tested in near-plugins/tests/access_controllable.rs

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

# This makes `workspaces` write more than 1G to /tmp (on my machine).
# Depending on the size of your /tmp, a cleanup might be required afterwards.
du --summarize --total --human-readable /tmp/sandbox-*
rm -r /tmp/sandbox-*

Notes on super-admin

Currently only acl_init_super_admin is added to the public interface, i.e. only the contract itself may add a super-admin. Potential further additions could be:

Allow super-admin to add/revoke other accounts as super-admin.
- acl_add_super_admin(account_id)
- acl_revoke_super_admin(account_id)

Allow a super-admin to renounce their super-admin permission.
- acl_renounce_super_admin()

All of the above are basically just wrappers for
acl_{add, revoke}_super_admin_unchecked which verify the permission of the
caller. Similar to how `acl_add_admin` is a simple wrapper around
`acl_add_admin_unchecked`.

Next up

A follow-up PR to add some missing functions related to roles:

acl_grant_role
acl_revoke_role[_unchecked]
acl_renounce_role

Add functions acl_{is, add}_admin* Implement {revoke, renounce} admin Add tests for acl_{revoke, renounce}_admin*

Add acl_is_super_admin Test acl_{add, revoke}_super_admin_unchecked

mfornet

Looks good in general. I left a comment about removing unchecked methods from the public contract interface.

mfornet · 2022-10-06T13:01:53Z

FYI: project doesn't compile for me when doing cargo test

mooori · 2022-10-06T14:58:05Z

FYI: project doesn't compile for me when doing cargo test

Hm on my machine it compiles and all tests are passing. I'm having near-sdk version 4.0.0 in Cargo.lock, is it the same for you? There might be issues when locally 4.0.0.-pre* is installed.

If it's not related to the version of near-sdk, could you please post more infos about the compilation error?

mooori · 2022-10-06T18:40:42Z

I'm in the process of removing the *_unchecked methods and noticed that:

Tests rely on calling `*_unchecked` methods

They have been since the first Acl PR. An easy fix is manually exporting *_unchecked methods in the test contract.

// near-plugins/tests/contracts/access_controllable/src/lib.rs

// Helpers to let integration tests call *_unchecked methods.
#[near_bindgen]
impl StatusMessage {
    #[private]
    pub fn acl_add_admin_unchecked(&mut self, role: Role, account_id: AccountId) -> bool {
        self.__acl.add_admin_unchecked(role, &account_id)
    }

    // same for the other *_unchecked methods
}

Developers can no longer use `*_unchecked` for initialization

Hence they would have to call internal methods, for instance:

#[near_bindgen]
impl Contract {
    #[init]
    pub fn new() -> Self {
        self.__acl.add_super_admin_unchecked(&env::currenct_account_id());
        // ...
    }
}

To avoid that, a workaround could be having some method like this in the trait:

/// Makes the contract a super-admin. It is #[private] in the implementation
/// provided by this trait, i.e. only the contract itself may call this method.
fn acl_make_self_super_admin();

Once the contract is super-admin, it can call all checked methods like acl_add_admin and acl_grant_role.

Side note: OpenZeppelin Acl has unchecked methods, e.g. _grant_role().

mfornet · 2022-10-07T19:11:35Z

IMO, unchecked methods are an internal detail about our default implementation and not mandatory for every other implementation out there. It is conceivable that other implementations use different mechanisms internally. Arguably unchecked-like methods will be present one way or another, but we should try to keep the main interface as simple as possible.

Side note: OpenZeppelin Acl has unchecked methods, e.g. _grant_role().

This is a great example of what I try to convey. _grantRole is only part of the default implementation provided by OpenZeppelin, but it is not callable or present in the global Access Control Interface (IAccessControl).

Tests rely on calling *_unchecked methods

We can keep this method behind a testing feature flag.

Developers can no longer use *_unchecked for initialization
Proposal: fn acl_make_self_super_admin();

I like the new proposal, even we can take it further and have the method:

fn acl_init_super_admin(accountId: AccountId);

This method can only be called once and will add the account passed as an argument as super admin. In particular, it is mandatory (or strongly recommended) to developers that this is called during initialization.

mfornet · 2022-10-07T19:32:24Z

Hm on my machine it compiles and all tests are passing. I'm having near-sdk version 4.0.0 in Cargo.lock, is it the same for you? There might be issues when locally 4.0.0.-pre* is installed.

If it's not related to the version of near-sdk, could you please post more infos about the compilation error?

Check compilation-error-logs and Cargo.lock.

mfornet · 2022-10-07T19:33:28Z

+/// Exposing internal methods to facilitate integration testing.
+#[near_bindgen]
+impl StatusMessage {
+    #[private]
+    pub fn acl_add_super_admin_unchecked(&mut self, account_id: AccountId) -> bool {
+        self.__acl.add_super_admin_unchecked(&account_id)
+    }
+
+    #[private]
+    pub fn acl_revoke_super_admin_unchecked(&mut self, account_id: AccountId) -> bool {
+        self.__acl.revoke_super_admin_unchecked(&account_id)
+    }
+
+    #[private]
+    pub fn acl_add_admin_unchecked(&mut self, role: Role, account_id: AccountId) -> bool {
+        self.__acl.add_admin_unchecked(role, &account_id)
+    }
+
+    #[private]
+    pub fn acl_revoke_admin_unchecked(&mut self, role: Role, account_id: AccountId) -> bool {
+        self.__acl.revoke_admin_unchecked(role, &account_id)
+    }
+
+    #[private]
+    pub fn acl_grant_role_unchecked(&mut self, role: Role, account_id: AccountId) -> bool {
+        self.__acl.grant_role_unchecked(role, &account_id)
+    }
+}


Let's add this behind [integration-test] feature flag, similar as it is used in the engine today.

https://github.com/aurora-is-near/near-plugins/blob/b2e1d8369059bee4b0e31cb92b965a59a8a2717f/near-plugins/tests/access_controllable.rs#L25

The contract containing these methods is compiled and deployed via workspaces::compile_project which has only one parameter: project_path: &str. AFAIK this prevents us from working with features in this contract, since we'd have to enable required features by default (see below).

The following works, though if the integration-test feature needs to be enabled by default, I'm not sure if it makes sense to have it?

# near-plugins/tests/contracts/access_controllable/Cargo.toml [features] default = ["integration-test"] integration-test = []

It might be worth a feature request for compile_project to have an additional parameter &[&str] that specifies the features to be enabled.

mooori · 2022-10-10T06:10:52Z

fn acl_init_super_admin(accountId: AccountId);
This method can only be called once and will add the account passed as an argument as super admin.

Do we want it to be callable strictly once? I think that would require a new field like initiated: bool on the Acl struct.

Alternatively we might relax it a bit and say acl_init_super_admin succeeds only if there are no super-admins. This condition is met during initialization and additionally when a contract has (by mistake) removed all super-admins. So it's kind of a recovery mechanism.

mooori · 2022-10-10T08:45:07Z

Regarding the compilation errors: After removing Cargo.lock in addition to cargo clean I could reproduce the errors. Skimming over them, looks like they're all in dependencies. I'm looking into it...

mooori · 2022-10-10T11:54:14Z

Regarding the compilation errors: After removing Cargo.lock in addition to cargo clean I could reproduce the errors. Skimming over them, looks like they're all in dependencies. I'm looking into it...

Running cargo test in a fresh directory pulled in different, inconsistent versions of near* dependencies. Updating the workspaces dev-dependency to the latest version should fix it (done in the latest commit).

mfornet

LGTM. I'll revisit later about having init_super_admin as a public method, hidden under a private modifier, I think this is no needed and can be internal. But let's move forward with this PR for now.

mooori added 5 commits October 5, 2022 08:54

Implement and test admin functionality

4090e6f

Add functions acl_{is, add}_admin* Implement {revoke, renounce} admin Add tests for acl_{revoke, renounce}_admin*

Implement and test super-admin functionality

36ec43f

Add acl_is_super_admin Test acl_{add, revoke}_super_admin_unchecked

test: verify super-admin privileges

a236e59

test: extend test_attribute_access_control_any

a8b6cac

test: verify previously ignored return values

9bbc751

mooori marked this pull request as ready for review October 5, 2022 08:29

mooori requested a review from mfornet October 5, 2022 08:32

mooori mentioned this pull request Oct 6, 2022

Add functions to manage Acl roles #9

Merged

mfornet requested changes Oct 6, 2022

View reviewed changes

Comment thread near-plugins/src/access_controllable.rs Outdated

Comment thread near-plugins-derive/src/access_controllable.rs Outdated

Remove acl_*_unchecked methods from trait

c5e0320

mooori commented Oct 7, 2022

View reviewed changes

Comment thread near-plugins/tests/contracts/access_controllable/src/lib.rs

mooori requested a review from mfornet October 7, 2022 09:32

mooori mentioned this pull request Oct 7, 2022

Add functions acl_get_{admins, grantees} #12

Merged

mfornet reviewed Oct 7, 2022

View reviewed changes

Update dev-dependency workspaces

62c9f58

Add acl_init_super_admin

a07afa8

mooori requested a review from mfornet October 11, 2022 09:42

mfornet approved these changes Oct 11, 2022

View reviewed changes

mfornet merged commit 3510396 into near:acl Oct 11, 2022

mrLSD approved these changes Nov 15, 2023

View reviewed changes

Conversation

mooori commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Notes on super-admin

Next up

Uh oh!

mfornet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mfornet commented Oct 6, 2022

Uh oh!

mooori commented Oct 6, 2022

Uh oh!

mooori commented Oct 6, 2022

Tests rely on calling *_unchecked methods

Developers can no longer use *_unchecked for initialization

Uh oh!

Uh oh!

mfornet commented Oct 7, 2022

Uh oh!

mfornet commented Oct 7, 2022

Uh oh!

mfornet Oct 7, 2022

Choose a reason for hiding this comment

Uh oh!

mooori Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mooori commented Oct 10, 2022

Uh oh!

mooori commented Oct 10, 2022

Uh oh!

mooori commented Oct 10, 2022

Uh oh!

mfornet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mooori commented Oct 5, 2022 •

edited

Loading

Tests rely on calling `*_unchecked` methods

Developers can no longer use `*_unchecked` for initialization

mooori Oct 10, 2022 •

edited

Loading