Authorizations #358

nicolaslara · 2022-06-14T15:01:27Z

This PR should replace #305 .

The approach here is much simpler with the authorizations integrated into the proposals. We can discuss if this is the right approach, but the middleware approach added so much complexity that I don't think it's worth going that route.

The most interesting part of this PR should be the message-filter contract implementation and its tests. Additionally the proposal tests should give an idea of how this would be used in a more realistic scenario.

The PR is not production-ready, but I'd like to get some feedback before moving forward with the implementation.

Co-authored-by: zeke <30676292+ezekiiel@users.noreply.github.com>

…and more generic. Need to consider safety concerns

…s not supported by wasmvm due to floats

authorizations/message-filter/src/contract.rs

blue-note · 2022-07-18T23:49:26Z

contracts/cw-proposal-single/src/contract.rs

+    //  * Write proper tests for the auth, and more realistic tests for this proposal where we don't fake the sender
+    //
+    let response = Response::default();
+    let response = if let Some(auths) = AUTHORIZATION_MODULE.may_load(deps.storage)? {


are the performance implications of adding this query to every message negligible?

The performance implications here are:

Loading the addr to the authorization module

Calling the querier

The query loads any sub-authorizations and queries those

In the best case scenario, this has very low overhead. If you have many authorizations configured, though, it could be considerable if the authorization contracts are complex. However, I'd argue that if you've added authorizations here, then that's exactly the behaviour you want: the authorizations need to be checked to know if the message is allowed or not.

That said, I'm not sure how useful it is to be able to add auths to proposal-single. I added that cause it was the easiest way to test initially, but then figured out that making the auth manager behave like a proposal allows for more flexibility.

Right now, the authorizations contract can serve as a proposal, so you can add that contract to the dao as a proposal manager and any proposals that require auths can go through it while you can still have proposal-single for voting proposals. I still have to clean some of this up and add tests, but would love your opinion on whether it makes sense to have auths as part of proposal-single or not.

… Proposals now use queries.

nicolaslara · 2022-08-01T12:08:11Z

This is now "feature complete". It currently supports:

Ability to restrict the proposals that can be executed via proposal-single (based on the sender and the messages)
Adding a cw-auth-middleware contract to cw-core as a proposal. This means we can call Execute on cw-auth-middleware and, if the auth passes, have the dao execute the messages in the same block (without the need for voting)
Adding cw-4 groups to cw-auth-middleware so that any member of the group shares the same permissions
Three authorization contracts:
- message-filter: The most powerful auth, that accepts a custom message matching pattern and allows or rejects any message that matches the pattern
- and: Authorizes a message if all its children authorize it
- whitelist: Authorizes any message by a whitelisted sender

There are a lot of changes and improvements that I would like to improve here, but I think I should focus on the minimal cleanup needed to be able to ship this and keep adding the improvements in future versions.

For reference, these are some of the things I would like to improve (in no particular order):

Rename cw-auth-middlware to cw-auth-manager. It's not a middleware!
Document and define the Authorization spec
Simplify code reuse. This could be done by:
- Implementing an Authorization trait that contains default implementations
- Either reorganize the code so the contract one object (like ng-contracts or Sylvia (https://github.com/CosmWasm/sylvia))
- Or by adding generic functions that can be imported by the different auth implementation
- Messages in the auth spec may need to be derived in sub auth contracts
Split group management into its own contract. This would simplify the implementation of the auth manager.
Add logical auth contracts for composition (any, all, not). This simplifies the construction of complex auths and makes the auth manager cleaner
Make it easy to create complex auths via nested instantiate messages (similar to cw-core)
Allow the auth manager to specify the default Allow/Reject behaviour (similar to message filter)
Tooling that makes it easy to use auths. Can be done via:
- Proxy-accounts for keplr
- JS code that wraps a transaction in the necessary auth boilerplate so that apps can implement the calls themselves
Document everything

JakeHartnell · 2022-08-04T05:15:39Z

authorizations/and/src/contract.rs

+#[cfg_attr(not(feature = "library"), entry_point)]
+pub fn query(deps: Deps, env: Env, msg: QueryMsg) -> StdResult<Binary> {
+    match msg {
+        QueryMsg::Authorize { msgs, sender } => authorize_messages(deps, env, msgs, sender),


Rename to QueryMsg::IsAuthorized?

authorize_messages sounds like an execute method when it's actually a query handler.

ethanfrey

This needs more design discussion on a few levels

ethanfrey · 2022-08-04T05:20:46Z

authorizations/message-filter/src/contract.rs

+    Number(u64), // Why doesn't u128 work here?
+    String(String),
+    Array(Vec<Value>),
+    Object(Map<String, Value>),


I am quite worried about using Map in any serde serialisable struct.
Also, very odd to see this come from schemars.

This may be safe, but needs some very careful checking.
Minimally, check the json encoding "manually" in test cases.
Also ensure that when this compiles to wasm, it passes check_contract
(see this example)

Ran the checks and they seem to pass. I can run them on the optimized contract later.

I've tested this manually and it works on the chain, and schemars uses https://docs.rs/indexmap/latest/indexmap/ internally to preserve ordering. I can add some more tests to the serialization/deserialization specifically focusing on ordering (is non-determinism your main concern?)

ethanfrey · 2022-08-04T05:25:39Z

authorizations/message-filter/src/contract.rs

+    })
+}
+
+#[cfg_attr(not(feature = "library"), entry_point)]


I don't think this need to be a contract but a library imported directly by thing using it.

It makes sense to refactor the message parsing and matching into a library. We would still need a message filter contract if we want DAOs to add auths in a generic way (for example: authorize an account to mint NFTs from the DAO account to give to members)

JakeHartnell · 2022-08-05T05:29:08Z

authorizations/and/Cargo.toml

@@ -0,0 +1,54 @@
+[package]
+name = "and"


Can we think of a better name for this contract? 😂

logical-and? Thought maybe it should be something auth specific, like must-pass-all or authorization-joiner-and 🤷 open to suggestions

JakeHartnell · 2022-08-07T07:57:57Z

contracts/cw-proposal-single/src/contract.rs

+    // Check that the proposal is authorized if authorizations are configured
+    let response = Response::default();
+    let response = if let Some(auths) = AUTHORIZATION_MODULE.may_load(deps.storage)? {


That said, I'm not sure how useful it is to be able to add auths to proposal-single. I added that cause it was the easiest way to test initially, but then figured out that making the auth manager behave like a proposal allows for more flexibility.

Ok, getting caught up on this... I think allowing the auth manager to behave like a proposal would be best.

Here's what I envision:

SubDAO -------> AuthorizationsContract --------> DAO

So the parent DAO would add the Authorizations module as a proposal module. The authorizations contract would then be able to call ExecuteProposalHook on the main DAO. The SubDAO would simply vote to call the authorizations contract with the message they intend to execute.

Yeah, I like this approach too.

Just to be clear, this is currently possible (see the tests on cw-auth-middleware), the questions is if this should be the only way of doing it or we should keep the changes to proposal single as well.

JakeHartnell · 2022-08-07T07:59:41Z

contracts/cw-auth-middleware/src/contract.rs

+#[cfg_attr(not(feature = "library"), entry_point)]
+pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> StdResult<Binary> {
+    match msg {
+        QueryMsg::Authorize { msgs, sender } => query_authorizations(deps, msgs, sender),


Maybe rename to IsAuthorized?

JakeHartnell · 2022-08-07T11:44:31Z

This is now "feature complete". It currently supports:

Ability to restrict the proposals that can be executed via proposal-single (based on the sender and the messages)

Adding a cw-auth-middleware contract to cw-core as a proposal. This means we can call Execute on cw-auth-middleware and, if the auth passes, have the dao execute the messages in the same block (without the need for voting)

Adding cw-4 groups to cw-auth-middleware so that any member of the group shares the same permissions

Three authorization contracts:

message-filter: The most powerful auth, that accepts a custom message matching pattern and allows or rejects any message that matches the pattern

and: Authorizes a message if all its children authorize it

whitelist: Authorizes any message by a whitelisted sender

Wow, this is a lot! Very powerful, I like the general direction.

Less might be more here as we will need to get these contracts documented and audited.

Would be nice to simplify things a bit. Think MVP. What things can we remove? IMO the main contracts are the cw-auth-middleware and message-filter.

Split group management into its own contract. This would simplify the implementation of the auth manager.

I would argue groups should not be part of cw-auth-middleware because we can achieve the same functionality using cw1-whitelist! So it's already done! cw1-whitelist can be managed by a group or a DAO.

Implementing an Authorization trait that contains default implementations

This is a great idea!

Messages in the auth spec may need to be derived in sub auth contracts

"Sub Auth contracts seem like something we could put off for a V2.

Add logical auth contracts for composition (any, all, not). This simplifies the construction of complex auths and makes the auth manager cleaner

Make it easy to create complex auths via nested instantiate messages (similar to cw-core)

Allow the auth manager to specify the default Allow/Reject behaviour (similar to message filter)

More things that I would suggest should be out of scope until a V2. Let's build something simple first and do the hard stuff like message filtering well and then think about about how such things can be composed.

Tooling that makes it easy to use auths.

Integrating proposal templates and UI for this would be a great start... but again MVP!

This is a really great start though. Happy to setup a call to talk through this stuff. I was initially skeptical about implementing message filter as a separate contract, but I think I'm convinced now.

nicolaslara · 2022-08-09T09:55:32Z

Thanks for the feedback @JakeHartnell! I'm a 100% onboard with simplifying this.

I would argue groups should not be part of cw-auth-middleware because we can achieve the same functionality using cw1-whitelist! So it's already done! cw1-whitelist can be managed by a group or a DAO.

I agree they should not be part of cw-auth-middleware. It's also one of the main sources of complexity, so removing it would help.

The only issue I see with using cw1-whitelist for group auth, is that you have to interact the dao in a different way if you're the one authorized vs if the cw1-whitelist contract is the one authorized, but that may be a tradeoff that is worth it for the simplicity gains.

Let's discuss this on a call and make a plan for shipping it!

JakeHartnell · 2022-08-09T21:50:20Z

The only issue I see with using cw1-whitelist for group auth, is that you have to interact the dao in a different way if you're the one authorized vs if the cw1-whitelist contract is the one authorized, but that may be a tradeoff that is worth it for the simplicity gains.

Worth the tradeoff! Getting people to use it will inform next steps. Simple is good.

Let's discuss this on a call and make a plan for shipping it!
Let's do it!

nicolaslara · 2022-08-19T09:48:25Z

Started some of the work of removing bloat and generalizing auths as an interface here: https://github.com/nicolaslara/dao-contracts/tree/authorizations_as_interface. Still a WIP, but it should already be closer to an MVP. I want to cleanup the interface and document how to use it. Maybe the easiest way is to just create a template for a minimal auth that can be created via cargo generate to avoid adding more complexity to the Trait

nicolaslara · 2022-09-06T09:48:34Z

another short update here: I've been trying to move everything that is not specific to daodao to its own repo: https://github.com/nicolaslara/authorizations. Most of it is there and the interface is relatively clean/stable. Now I just need to modify the daodao parts to use that interface. I'll try to have this ready by cosmoverse

nicolaslara · 2022-09-24T14:10:59Z

a much simple implementation of this is now at https://github.com/DA0-DA0/dao-contracts/compare/main...nicolaslara:dao-contracts:nico/authorizations?expand=1

This is now only an Authorizations based proposal for dao dao, all the implementations of auths have been moved to a different repo.

@JakeHartnell @ezekiiel we should discuss this at cosmoverse

0xekez · 2022-11-17T21:16:27Z

i'm going to close this for now as i feel our focus is elsewhere. the work here is very good. lets reopen this when we return to authorizations and have some more coherency about what our design requirements are.

nicolaslara and others added 26 commits May 24, 2022 13:54

added storage for an authorizations contract

bbf6814

added initial implementation and updated initialization everywhere

a6fc247

added authorization checking

e0e062e

initial authorization tests

f87390c

Added initial tests and simple authorization

2fe5d00

removed unnecessary stuff from the template

6bb31e2

removed circleci

b907f8a

removed unnecessary schema info

11cfc5c

Update contracts/cw-auth-manager/src/contract.rs

35d54c7

Co-authored-by: zeke <30676292+ezekiiel@users.noreply.github.com>

whitelist contract cleanup

85c8159

cleaner error

20467b0

added validation to the whitelist contract

5ccda74

adding authorizations to groups.

4bdaf72

actually using the group in the manager

4af1bc2

new group auth experiment. Storing auths as a string. This is easier …

1e118c9

…and more generic. Need to consider safety concerns

initial refactor to do auth as a middlware

1e304bd

working simplified authorizations

24e45d5

initial message matching work

145403d

importing auth as a library

2cc58c5

cleanup and tests

3bdddd8

using serde_json_wasm and deserializing manually because serde_json i…

5f47c3f

…s not supported by wasmvm due to floats

testing initial message filtering

813bd50

fixed deep matching, fixed authorizations api

7978f63

remove auths

8656603

using execute instead of query

d6528e8

matching improvements and more tests

9f74681

nicolaslara mentioned this pull request Jun 14, 2022

WIP: Authorization experiments #305

Closed

nicolaslara added 2 commits June 15, 2022 08:50

set cw-core back to how it was

931c5bb

Merge branch 'main' into authorizations_simple

4b371e2

JakeHartnell requested a review from 0xekez June 16, 2022 09:46

JakeHartnell mentioned this pull request Jun 25, 2022

Adding "titles" or "permissions" #159

Closed

blue-note reviewed Jul 18, 2022

View reviewed changes

authorizations/message-filter/src/contract.rs Outdated Show resolved Hide resolved

blue-note reviewed Jul 18, 2022

View reviewed changes

nicolaslara added 10 commits July 25, 2022 11:25

added tests of using the authorization as a proposal

553a392

style change

c3baeb9

fixed copy/paste typo

a79bfe6

empty auths never authorize

6912f49

added groups

1460ff0

fixed replies for update and added remaining group tests

1071263

update proposal todo

00e5919

fixed fmt with +nightly

d378b4d

added groups to the update message

b257a0e

Allowing for *and* and *or* auths. Fixed tests to reflect the change.…

3f3f424

… Proposals now use queries.

nicolaslara added 2 commits August 1, 2022 14:36

Merge branch 'main' into authorizations_simple

cc5d231

added diagrams from walkthrough

93b6cb7

JakeHartnell reviewed Aug 4, 2022

View reviewed changes

ethanfrey reviewed Aug 4, 2022

View reviewed changes

JakeHartnell reviewed Aug 5, 2022

View reviewed changes

JakeHartnell reviewed Aug 7, 2022

View reviewed changes

nicolaslara mentioned this pull request Oct 11, 2022

966 grants spike CosmWasm/wasmd#978

Closed

0xekez closed this Nov 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authorizations #358

Authorizations #358

nicolaslara commented Jun 14, 2022

blue-note Jul 18, 2022

nicolaslara Jul 19, 2022 •

edited

nicolaslara commented Aug 1, 2022 •

edited

JakeHartnell Aug 4, 2022

ethanfrey left a comment

ethanfrey Aug 4, 2022

nicolaslara Aug 4, 2022

ethanfrey Aug 4, 2022

nicolaslara Aug 6, 2022

JakeHartnell Aug 5, 2022

nicolaslara Aug 6, 2022

JakeHartnell Aug 7, 2022

nicolaslara Aug 7, 2022 •

edited

JakeHartnell Aug 7, 2022

JakeHartnell commented Aug 7, 2022

nicolaslara commented Aug 9, 2022

JakeHartnell commented Aug 9, 2022

nicolaslara commented Aug 19, 2022

nicolaslara commented Sep 6, 2022

nicolaslara commented Sep 24, 2022

0xekez commented Nov 17, 2022

Authorizations #358

Authorizations #358

Conversation

nicolaslara commented Jun 14, 2022

Choose a reason for hiding this comment

nicolaslara Jul 19, 2022 • edited

Choose a reason for hiding this comment

nicolaslara commented Aug 1, 2022 • edited

Choose a reason for hiding this comment

ethanfrey 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

nicolaslara Aug 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeHartnell commented Aug 7, 2022

nicolaslara commented Aug 9, 2022

JakeHartnell commented Aug 9, 2022

nicolaslara commented Aug 19, 2022

nicolaslara commented Sep 6, 2022

nicolaslara commented Sep 24, 2022

0xekez commented Nov 17, 2022

nicolaslara Jul 19, 2022 •

edited

nicolaslara commented Aug 1, 2022 •

edited

nicolaslara Aug 7, 2022 •

edited