-
Notifications
You must be signed in to change notification settings - Fork 397
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.
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
feat: p/demo/accesscontrol & p/demo/timelock #2307
base: master
Are you sure you want to change the base?
Changes from 84 commits
9ca3745
0748c45
1569339
1a04741
b439489
e7e2a39
d723750
5c2e768
ac70b14
0f4968d
5a30a47
cc86b90
2f7dbcc
5999935
0528c63
374b4f3
b1bf45d
4414bc9
6746863
eebd5c8
d1930b0
5dca52f
1e2a159
a4ae651
7df946a
91a21ea
84a6e30
36102fc
5b04454
dadcc5d
4e1b98e
5364f67
9a23760
7f6e861
863d1c8
59432e0
d08fc5c
13d7a1c
fca1601
bee7811
35279e0
8a7d710
0035a82
8da6e06
dc30291
2b70215
3c3bb84
2ff3f48
d72594a
2dee17b
30866f5
5a87e5b
c79bf43
d6ee9b5
312730a
fc9128e
923168f
6b4175f
1eee065
8b1e104
f57e5a5
d0f317b
e94a0b4
0de057d
f5a716f
d233452
56ba1f1
ae3a3db
973cbdc
af534a0
ca375b8
7bee06e
f905404
86af787
7998a78
e973d18
877536b
1cca09a
2f78427
555c084
26505de
1593f79
cb919ec
97ba66c
fd49b32
000c615
3b4c63a
0ced75a
c5f5604
7dd9256
ce643c4
8fe3820
13f8de4
de86c45
3f99de7
a667cd9
f3b01fd
982b1c7
88a3500
3811e19
b142277
b74e3da
26622eb
d72de48
78a99ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see we're using However, I think an equally possible flow is that of having a realm which has an access control list. In this case, actually, we shouldn't do any checks on PrevRealm(); the realm can just use it unexported. But I suggest you have an option for the ACL to not have a "owner"; in which case the PrevRealm checks are simply not performed. Allows someone else to compose other rules on top as well. Btw if Roles is meant to be exposed in a realm, then its fields should be unexported. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,185 @@ | ||
package accesscontrol | ||
|
||
import ( | ||
"std" | ||
|
||
"gno.land/p/demo/avl" | ||
"gno.land/p/demo/ownable" | ||
) | ||
|
||
const ( | ||
RoleName = "roleName" | ||
Sender = "sender" | ||
Account = "account" | ||
NewAdminRole = "newAdminRole" | ||
) | ||
kazai777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Role struct to store role information | ||
type Role struct { | ||
Name string | ||
Holders *avl.Tree // std.Address -> struct{} | ||
Ownable *ownable.Ownable | ||
} | ||
|
||
// Roles struct to store all Roles information | ||
type Roles struct { | ||
AllRoles []*Role | ||
kazai777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Ownable *ownable.Ownable | ||
} | ||
|
||
func validRoleName(name string) error { | ||
if len(name) > 30 || name == "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The limit on 30 seems kind of arbitrary for a package; more like a kind of validation that should be done, if anything, on the side of a realm (as an end-user application). But I don't expect many realms to publicly allow adding roles, anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the limit of 30 is intended to prevent someone from too overloading the chaine, for example by setting it to 1M characters. Do you think it's better to set a higher maximum or to let the user do it himself, even if this leaves open the possibility of defining a large number of characters? |
||
return ErrNameRole | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// NewRole creates a new instance of Role | ||
func NewRole(name string, admin std.Address) (*Role, error) { | ||
if err := validRoleName(name); err != nil { | ||
return nil, err | ||
} | ||
|
||
return &Role{ | ||
Name: name, | ||
Holders: avl.NewTree(), | ||
Ownable: ownable.NewWithAddress(admin), | ||
}, nil | ||
} | ||
|
||
// CreateRole create a new role within the realm | ||
func (rs *Roles) CreateRole(name string) (*Role, error) { | ||
if err := validRoleName(name); err != nil { | ||
return nil, ErrNameRole | ||
} | ||
|
||
if err := rs.Ownable.CallerIsOwner(); err != nil { | ||
return nil, err | ||
} | ||
|
||
for _, role := range rs.AllRoles { | ||
if role.Name == name { | ||
return nil, ErrRoleSameName | ||
} | ||
} | ||
|
||
role, err := NewRole(name, rs.Ownable.Owner()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
rs.AllRoles = append(rs.AllRoles, role) | ||
|
||
std.Emit( | ||
"RoleCreated", | ||
RoleName, name, | ||
Sender, rs.Ownable.Owner().String(), | ||
) | ||
|
||
return role, nil | ||
} | ||
|
||
// HasRole check if an account has a specific role | ||
func (r *Role) HasRole(account std.Address) bool { | ||
kazai777 marked this conversation as resolved.
Show resolved
Hide resolved
kazai777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return r.Holders.Has(account.String()) | ||
} | ||
|
||
// FindRole searches for a role by its name | ||
func (rs *Roles) FindRole(name string) (*Role, error) { | ||
for _, role := range rs.AllRoles { | ||
if role.Name == name { | ||
return role, nil | ||
} | ||
} | ||
|
||
return nil, ErrRoleNotFound | ||
} | ||
|
||
// GrantRole grants a role to an account | ||
func (rs *Roles) GrantRole(name string, account std.Address) error { | ||
r, err := rs.FindRole(name) | ||
if err != nil { | ||
return ErrRoleNotFound | ||
} | ||
|
||
err = r.Ownable.CallerIsOwner() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
r.Holders.Set(account.String(), struct{}{}) | ||
|
||
std.Emit( | ||
"RoleGranted", | ||
RoleName, r.Name, | ||
Account, account.String(), | ||
Sender, std.PrevRealm().Addr().String(), | ||
) | ||
|
||
return nil | ||
} | ||
|
||
// RevokeRole revokes a role from an account | ||
func (rs *Roles) RevokeRole(name string, account std.Address) error { | ||
r, err := rs.FindRole(name) | ||
if err != nil { | ||
return ErrRoleNotFound | ||
} | ||
|
||
err = r.Ownable.CallerIsOwner() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
r.Holders.Remove(account.String()) | ||
|
||
std.Emit( | ||
"RoleRevoked", | ||
RoleName, r.Name, | ||
Account, account.String(), | ||
Sender, std.PrevRealm().Addr().String(), | ||
) | ||
|
||
return nil | ||
} | ||
|
||
// RenounceRole allows an account to renounce a role it holds | ||
func (rs *Roles) RenounceRole(name string) error { | ||
Comment on lines
+166
to
+167
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say we remove this.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know what you mean, it's true that I didn't take the case of the |
||
r, err := rs.FindRole(name) | ||
if err != nil { | ||
return ErrRoleNotFound | ||
} | ||
|
||
caller := std.PrevRealm().Addr() | ||
|
||
if !r.HasRole(caller) { | ||
return ErrAccountNotRole | ||
} | ||
|
||
r.Holders.Remove(caller.String()) | ||
|
||
std.Emit( | ||
"RoleRenounced", | ||
RoleName, r.Name, | ||
Account, caller.String(), | ||
Sender, caller.String(), | ||
) | ||
|
||
return nil | ||
} | ||
|
||
// SetRoleAdmin transfers the ownership of the role to a new administrator | ||
func (r *Role) SetRoleAdmin(admin std.Address) error { | ||
if err := r.Ownable.TransferOwnership(admin); err != nil { | ||
return err | ||
} | ||
|
||
std.Emit( | ||
"RoleSet", | ||
RoleName, r.Name, | ||
NewAdminRole, r.Ownable.Owner().String(), | ||
) | ||
|
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,192 @@ | ||
package accesscontrol | ||
|
||
import ( | ||
"std" | ||
"testing" | ||
|
||
"gno.land/p/demo/ownable" | ||
"gno.land/p/demo/testutils" | ||
"gno.land/p/demo/uassert" | ||
) | ||
|
||
var ( | ||
admin = testutils.TestAddress("admin1") | ||
newAdmin = testutils.TestAddress("admin2") | ||
user1 = testutils.TestAddress("user1") | ||
user2 = testutils.TestAddress("user2") | ||
|
||
roleName = "TestRole" | ||
) | ||
|
||
func initSetup(admin std.Address) *Roles { | ||
return &Roles{ | ||
AllRoles: []*Role{}, | ||
Ownable: ownable.NewWithAddress(admin), | ||
} | ||
} | ||
|
||
func TestCreateRole(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
std.TestSetOrigCaller(admin) | ||
|
||
role, err := roles.CreateRole(roleName) | ||
uassert.NoError(t, err) | ||
uassert.True(t, role != nil, "role should not be nil") | ||
uassert.Equal(t, role.Name, roleName) | ||
|
||
_, err = roles.CreateRole(roleName) | ||
uassert.Error(t, err, "should fail on duplicate role creation") | ||
} | ||
|
||
func TestGrantRole(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
_, err := roles.CreateRole(roleName) | ||
uassert.NoError(t, err) | ||
|
||
err = roles.GrantRole(roleName, user1) | ||
uassert.NoError(t, err) | ||
|
||
role, err := roles.FindRole(roleName) | ||
uassert.NoError(t, err) | ||
uassert.True(t, role.HasRole(user1), "user1 should have the TestRole") | ||
} | ||
|
||
func TestGrantRoleByNonOwner(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
_, err := roles.CreateRole(roleName) | ||
uassert.NoError(t, err) | ||
|
||
std.TestSetOrigCaller(user2) | ||
roles.Ownable.TransferOwnership(user2) | ||
err = roles.GrantRole(roleName, user1) | ||
uassert.Error(t, err, "non-owner should not be able to grant roles") | ||
|
||
roles.Ownable.TransferOwnership(admin) | ||
} | ||
|
||
func TestRevokeRole(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
std.TestSetOrigCaller(admin) | ||
|
||
_, err := roles.CreateRole(roleName) | ||
uassert.NoError(t, err) | ||
err = roles.GrantRole(roleName, user1) | ||
uassert.NoError(t, err) | ||
|
||
err = roles.RevokeRole(roleName, user1) | ||
uassert.NoError(t, err) | ||
|
||
role, err := roles.FindRole(roleName) | ||
uassert.NoError(t, err) | ||
uassert.False(t, role.HasRole(user1), "user1 should no longer have the TestRole") | ||
} | ||
|
||
func TestRenounceRole(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
std.TestSetOrigCaller(admin) | ||
|
||
_, err := roles.CreateRole(roleName) | ||
uassert.NoError(t, err) | ||
err = roles.GrantRole(roleName, user1) | ||
uassert.NoError(t, err) | ||
|
||
roles.Ownable.TransferOwnership(user1) | ||
std.TestSetOrigCaller(user1) | ||
err = roles.RenounceRole(roleName) | ||
uassert.NoError(t, err) | ||
|
||
role, err := roles.FindRole(roleName) | ||
uassert.NoError(t, err) | ||
uassert.False(t, role.HasRole(user1), "user1 should have renounced the TestRole") | ||
} | ||
|
||
func TestSetRoleAdmin(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
std.TestSetOrigCaller(admin) | ||
|
||
role, err := roles.CreateRole(roleName) | ||
uassert.NoError(t, err) | ||
|
||
err = role.SetRoleAdmin(newAdmin) | ||
uassert.NoError(t, err, "admin change should succeed") | ||
|
||
std.TestSetOrigCaller(newAdmin) | ||
uassert.Equal(t, role.Ownable.Owner(), newAdmin, "the new admin should be newAdmin") | ||
|
||
std.TestSetOrigCaller(admin) | ||
uassert.NotEqual(t, role.Ownable.Owner(), admin, "the old admin should no longer be the owner") | ||
} | ||
|
||
func TestCreateRoleInvalidName(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
std.TestSetOrigCaller(admin) | ||
|
||
_, err := roles.CreateRole("") | ||
uassert.Error(t, err, "should fail on empty role name") | ||
|
||
longRoleName := "thisisaverylongrolenamethatexceedsthenormallimitfortestingpurposes" | ||
_, err = roles.CreateRole(longRoleName) | ||
uassert.Error(t, err, "should fail on very long role name") | ||
} | ||
|
||
func TestRevokeRoleByNonOwner(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
std.TestSetOrigCaller(admin) | ||
|
||
_, err := roles.CreateRole(roleName) | ||
uassert.NoError(t, err) | ||
err = roles.GrantRole(roleName, user1) | ||
uassert.NoError(t, err) | ||
|
||
std.TestSetOrigCaller(user2) | ||
err = roles.RevokeRole(roleName, user1) | ||
uassert.Error(t, err, "non-owner should not be able to revoke roles") | ||
} | ||
|
||
func TestGrantRoleToNonExistentRole(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
std.TestSetOrigCaller(admin) | ||
|
||
err := roles.GrantRole("NonExistentRole", user1) | ||
uassert.Error(t, err, "should fail when granting non-existent role") | ||
} | ||
|
||
func TestRevokeRoleFromNonExistentRole(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
std.TestSetOrigCaller(admin) | ||
|
||
err := roles.RevokeRole("NonExistentRole", user1) | ||
uassert.Error(t, err, "should fail when revoking non-existent role") | ||
} | ||
|
||
func TestRenounceNonExistentRole(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
std.TestSetOrigCaller(user1) | ||
|
||
err := roles.RenounceRole("NonExistentRole") | ||
uassert.Error(t, err, "should fail when renouncing non-existent role") | ||
} | ||
|
||
func TestDeleteRole(t *testing.T) { | ||
roles := initSetup(admin) | ||
|
||
std.TestSetOrigCaller(admin) | ||
|
||
role, err := roles.CreateRole(roleName) | ||
uassert.NoError(t, err) | ||
|
||
roles.AllRoles = []*Role{} // Clear roles for testing purpose | ||
_, err = roles.FindRole(roleName) | ||
uassert.Error(t, err, "should fail when trying to find deleted role") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make a case for how this is different / better than
p/demo/acl
?I'm not saying it's perfect, just that
demo/
should probably contain one preferred ACL implementation. We can decide to move this one top/<name>/accesscontrol
, or that one top/nt/acl
. (cc'ing also @moul for an opinion on what to do here)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although
acl
andaccesscontrol
may seem similar at first glance,accesscontrol
stands out due to its ability to implement role hierarchies as well as dynamic permission optionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give an example? Namely, of where this distinction is useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine a user can belong to several groups with different permissions. With an
ACL
system, each permission has to be checked individually for the user, which can become complex depending on the number of users in the group. InAccesscontrol
lets you manage access via hierarchical roles(e.g. Admin, Manager, Employee)
: each role has specific permissions automatically applied to all its members. This simplifies authorization management and makes the system more flexible, especially for large groups