Allow list with cohort and auth admin roles #341

vzotova · 2025-01-20T00:29:25Z

Type of PR:

Bugfix
Feature
Documentation
Other

Required reviews:

1
2
3

What this does:

High-level idea of the changes introduced in this PR.
List relevant API changes (if any), as well as related PRs and issues.

Issues fixed/closed:

Fixes #...

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

What should reviewers focus on?
Is there a particular commit/function/section of your PR that requires more attention from reviewers?

derekpierre

So based on @cygnusv 's comment in Discord:

Maybe we can just add a flag to disallow setting auth admins, so with the flag off the contract simply behaves as a normal global allows list. The point would be to have a single type of contract

The "flag" at the moment, is really just either using the GlobalAllowList or the ManagedAllowList as the accessController for a Ritual. Correct?

tests/test_managed_allow_list.py

derekpierre · 2025-01-21T14:12:11Z

contracts/contracts/coordination/ManagedAllowList.sol

-        }
+    function grantAuthAdminRole(uint32 ritualId, address account) external {
+        initializeCohortAdminRole(ritualId);
+        grantRole(ritualRole(ritualId, AUTH_ADMIN_BASE), account);


So just to confirm, if the ritual authority calls grantRole() directly without having first called initializeCohortAdminRole(), the call will fail.

But, if the ritual authority either:

makes the two separate calls in here themselves directly
OR

calls grantAuthAdminRole()

then granting the role will work correctly.

derekpierre

Approving (assuming my clarifications were correct) - nice work 🎸 !

cygnusv

Still digesting

contracts/contracts/coordination/ManagedAllowList.sol

cygnusv

More comments. There's still a design issue we need to address before moving forward.

cygnusv · 2025-01-28T11:47:59Z

contracts/contracts/coordination/ManagedAllowList.sol

-        require(address(_subscription) != address(0), "Subscription cannot be the zero address");
-        subscription = _subscription;
+    constructor(Coordinator _coordinator) GlobalAllowList(_coordinator) {
+        _disableInitializers();


GlobalAllowList constructor already _disableInitializers(), but I guess we do it just in case this dependence is removed, right? In any case, calling it twice doesn't do any harm. Just double-checking on this.

just a habit, I'll remove it

cygnusv · 2025-01-28T11:54:19Z

contracts/contracts/coordination/ManagedAllowList.sol

+        _grantRole(cohortAdminRole, authority);
+    }
+
+    function grantAuthAdminRole(uint32 ritualId, address account) external {


We need some docstrings here saying that this can only be called by the cohort admin

cygnusv · 2025-01-28T11:59:01Z

contracts/contracts/coordination/subscription/Subscription.sol

Just to be sure: this contract is removed because it was only used in the old ManagedAllowList and we already have AbstractSubscription.sol, correct?

Yep, Old Subscription contract was there just as reference (and mostly to compile old ManagedAllowList)

cygnusv · 2025-01-28T12:02:49Z

contracts/contracts/coordination/ManagedAllowList.sol

 import "./GlobalAllowList.sol";
-import "./Coordinator.sol";
-import {UpfrontSubscriptionWithEncryptorsCap} from "./subscription/Subscription.sol";

 /**
 * @title ManagedAllowList
 * @notice Manages a list of addresses that are authorized to decrypt ciphertexts, with additional management features.


Suggested change

* @notice Manages a list of addresses that are authorized to decrypt ciphertexts, with additional management features.

* @notice Manages a list of addresses that are authorized to encrypt ciphertexts, with additional management features.

cygnusv · 2025-01-28T12:27:49Z

contracts/contracts/coordination/ManagedAllowList.sol

-                    subscription.authorizationActionsCap(ritualId, addresses[i]),
-                "Authorization cap exceeded"
-            );
+    function initializeCohortAdminRole(uint32 ritualId) public {


When it is expected this function to be called? The way I see it, we could do it in the coordinator when the ritual is initiated. This shouldn't matter in the case of ritual failing, but if ritual succeeds, then this is already done from day 1.

I'd avoid any changes in Coordinator to call it
This function included in grantAuthAdminRole function, so it will be called anyway during first granting

cygnusv · 2025-01-28T12:31:53Z

tests/test_managed_allow_list.py

-        managed_allow_list.addAdministrators(RITUAL_ID, [admin.address], ADMIN_CAP, sender=admin)
+    managed_allow_list.grantRole(auth_admin_role, initiator, sender=initiator)
+    with ape.reverts("Encryptor must be authorized by the sender first"):
+        managed_allow_list.deauthorize(ritual_id, [deployer.address], sender=initiator)


Here's some food for thought for a new test:

Cohort admin authorizes auth admin A

Cohort admin authorizes auth admin B

A authorizes encryptor E

B authorizes encryptor E

A deauthorizes encryptor E

What should happen with E? Is E still authorized or not?

cc @arjunhassard @derekpierre @KPrasch

Interesting scenario. Given that E is used by both A and B, what scenario could yield that? 🤔 . An encryption service of some sort? But it would be using the same signer for both A and B? Maybe the user encrypting for themselves on two different apps (A and B) using the same ritual...?

At first blush, it seems that E should still be authorized for B. It's unclear the relationship between A and B, and feels like without that knowledge the safest philosophy is that A's actions should not affect B's.

Did you have existing thoughts here?

'B authorizes encryptor E' - that will fail with the current code, encryptor can be authorized only by one auth admin

Right, if B can't authorize E becasue it's already authorized by A, then the problem disappears. We need to think carefully if this is acceptable, but if so, then I don't have further comments.

Would the following scenario be a possibility? Two apps, A and B, each being authAdmins for the same ritual (use as part of using the same broader service) and wanting to control their own set of encryptors. The encryptor could be the user and the user utilizes the same wallet across the two apps?

could you elaborate a bit more
so we have two auth admins? and same ritual? each of them can have own set of encryptors within paid subscription

Yes. Effectively, two auth admins for the same ritual. e.g. Ceramic pays for a ritual, r, for their developers, and then assigns two separate developers/application owners, A and B, as auth admins as part of their respective applications.

Both applications allow their individual users to be encryptors. User, U, uses both apps and wants to use their EOA wallet as an encryptor...?

I see, now it's not allowed, but point that we don't have separation for two apps using same ritual, so for contract it will be that any admin authorizes User and this User can authorize for the ritual which effectively means both apps

Co-authored-by: derekpierre <[email protected]> Co-authored-by: David Núñez <[email protected]>

vzotova self-assigned this Jan 20, 2025

vzotova force-pushed the new-allow-list branch from 2c3f202 to 8cb5b5b Compare January 20, 2025 20:49

vzotova changed the title ~~[WIP] Allow list with cohort and auth admin roles~~ Allow list with cohort and auth admin roles Jan 20, 2025

vzotova marked this pull request as ready for review January 20, 2025 21:03

vzotova requested review from KPrasch, arjunhassard, cygnusv, theref, derekpierre and manumonti January 20, 2025 21:03

derekpierre reviewed Jan 21, 2025

View reviewed changes

derekpierre approved these changes Jan 22, 2025

View reviewed changes

cygnusv reviewed Jan 27, 2025

View reviewed changes

contracts/contracts/coordination/ManagedAllowList.sol Show resolved Hide resolved

contracts/contracts/coordination/ManagedAllowList.sol Outdated Show resolved Hide resolved

vzotova added 3 commits January 27, 2025 09:50

Draft of allow list with additional roles

c9cc8b7

Tests for ManagedAllowList

b38fa48

Adds wrapper method to simplify granting auth admin role

3fce81e

vzotova force-pushed the new-allow-list branch 2 times, most recently from a2bf89c to 86c48c3 Compare January 27, 2025 14:56

cygnusv reviewed Jan 28, 2025

View reviewed changes

vzotova force-pushed the new-allow-list branch from 86c48c3 to b077c1b Compare January 28, 2025 15:10

Apply suggestions from code review

813227e

Co-authored-by: derekpierre <[email protected]> Co-authored-by: David Núñez <[email protected]>

vzotova force-pushed the new-allow-list branch from b077c1b to 813227e Compare January 28, 2025 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow list with cohort and auth admin roles #341

Allow list with cohort and auth admin roles #341

vzotova commented Jan 20, 2025

derekpierre left a comment

derekpierre Jan 21, 2025

derekpierre left a comment

cygnusv left a comment

cygnusv left a comment

cygnusv Jan 28, 2025

vzotova Jan 28, 2025

vzotova Jan 28, 2025

cygnusv Jan 28, 2025

vzotova Jan 28, 2025

cygnusv Jan 28, 2025

vzotova Jan 28, 2025

cygnusv Jan 28, 2025

vzotova Jan 28, 2025

cygnusv Jan 28, 2025

vzotova Jan 28, 2025

cygnusv Jan 28, 2025

derekpierre Jan 28, 2025

vzotova Jan 28, 2025

cygnusv Jan 28, 2025

derekpierre Jan 28, 2025

vzotova Jan 28, 2025

derekpierre Jan 28, 2025

vzotova Jan 28, 2025

	* @notice Manages a list of addresses that are authorized to decrypt ciphertexts, with additional management features.
	* @notice Manages a list of addresses that are authorized to encrypt ciphertexts, with additional management features.

Allow list with cohort and auth admin roles #341

Are you sure you want to change the base?

Allow list with cohort and auth admin roles #341

Conversation

vzotova commented Jan 20, 2025

derekpierre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre left a comment

Choose a reason for hiding this comment

cygnusv left a comment

Choose a reason for hiding this comment

cygnusv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment