-
Notifications
You must be signed in to change notification settings - Fork 46
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
docs(adr): ADR-014: Improve the permission system #869
Conversation
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
|
||
// RegisterPermission allows to register the permission with the given name and returns its value | ||
func RegisterPermission(permissionName string) Permission { | ||
permission := Permission(strings.ToUpper(strings.ReplaceAll(permissionName, " ", "_"))) |
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.
Since users will be one day able to create their own, we should take under consideration the fact that permissionName
s could arrive here in different formats, e.g. : Permission.Manage.Subspace
or Permission12!_Manage_Subspace
.
To avoid this, I think we can rely on two possible regular expressions to make sure inserted permissions:
- Only contains letters and spaces:
/^[a-zA-Z\s]*$/
no number or special symbols). - Are in
camelCase
format:(?<=[a-z])(?=[A-Z])|(?<=[A-Z])(?=[A-Z][a-z])
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.
I don't think we should have anything like this. Ideally, in the future, the only users that will be able to specify custom permissions will be subspaces owners or admins. It's going to be their responsibilities to create permissions with a readable name. Otherwise it's going to be something that makes their lives harder. It won't anyway impact any other user or subspace owner as permissions will be stored within each subspace anyway. If they create a super complex permission, they are the ones that will need to remember and use them later when setting them, so I think it's going to be something that auto-regulates itself
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.
Ok fine. Then I guess we're fine like this!
Co-authored-by: Leonardo Bragagnolo <leo.braga95@gmail.com>
|
||
// RegisterPermission allows to register the permission with the given name and returns its value | ||
func RegisterPermission(permissionName string) Permission { | ||
permission := Permission(strings.ToUpper(strings.ReplaceAll(permissionName, " ", "_"))) |
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.
Ok fine. Then I guess we're fine like this!
|
||
## Further Discussions | ||
|
||
In the future, we might even allow developers to register custom permissions within their own subspaces, and then request those permissions during the execution of different messages. This could be done by allowing them to specify a `MsgTypeURL -> []Permission` entry for each kind of message type, so that when a message with `MsgTypeURL` gets executed, the user needs to have the specified permission in order to successfully perform the request. |
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.
Does it assume the subspace is controlled by contract, and the feature is about registering the permission for contract?
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.
No, the idea is to allow any subspace owner to set a subspace-specific set of permissions to be checked. This might be useful if they want some kind of custom permission to be set on a user before they can do something. An example might be allowing only specific users to interact with smart contracts inside their subspace (e.g. paying users) or anything else.
Anyway, this is there only as a reminder for future iterations that we might want to implement.
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.
Looks good to me.
Description
This ADR contains the details of how the current permission system can be improved, based on #855.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change