feat: Introduce Permission Sets #2002
Conversation
|
Manuals preview: Link to Manuals |
| @@ -25,6 +25,7 @@ pub async fn user_id_from_authentication_id( | |||
| }; | |||
|
|
|||
| match app | |||
| .users() | |||
There was a problem hiding this comment.
this looks weird - probably should be core_users()
There was a problem hiding this comment.
Yeah, I also find it unpleasant. How would you find renaming CoreUser into CoreAccess as a module for access control, i. e. users, roles, permission sets etc.
core_users() would work, too.
jirijakes
changed the title
jirijakes
force-pushed
the
access-group
branch
3 times, most recently
from
82b0787 to
881fc48
Compare
| let permission_sets = self.permission_sets().find_all(permission_set_ids).await?; | ||
|
|
||
| for (permission_set_id, _) in permission_sets { | ||
| let _ = role.add_permission_set(permission_set_id, audit_info.clone()); |
| .await?; | ||
|
|
||
| self.users() | ||
| .bootstrap_superuser(email, superuser_role.name, &mut db) |
There was a problem hiding this comment.
should probably pass superuser_role.id?
There was a problem hiding this comment.
Also I think we have standardized on having db always be the first arg.
| /// Generates Permission Sets based on provided hierarchy of modules and | ||
| /// returns all existing Permission Sets. For use during application bootstrap. | ||
| // | ||
| // Warning: think thrice if you need to make the method more visible. |
There was a problem hiding this comment.
Why this warning here? We should always use the minimal visibility on every fn in the entire code base.
core/user/src/role/mod.rs
Outdated
| self.repo.find_by_id(&role_id).await | ||
| } | ||
|
|
||
| pub async fn list(&self, sub: &<Audit as AuditSvc>::Subject) -> Result<Vec<Role>, RoleError> { |
There was a problem hiding this comment.
We always want to expose the cursor.
| .entities) | ||
| } | ||
|
|
||
| pub async fn update(&self, role: &mut Role) -> Result<(), RoleError> { |
There was a problem hiding this comment.
Why would update be exposed? role mutation use cases should be in this file.
| .await | ||
| } | ||
|
|
||
| pub async fn create_role_with_permissions_sets( |
There was a problem hiding this comment.
isn't create_role the entry point? Does this need to be pub?
| /// Creates a role with name “superuser” that will have all given permission sets. | ||
| /// Used for bootstrapping the application. | ||
| // | ||
| // Warning: think thrice if you need to make the method more visible. |
lib/authz/src/permission_set.rs
Outdated
| /// | ||
| /// To obtain qualified name of the action and its related object, two | ||
| /// segments of the path need to be known (i. e. `P = (String, String)`). | ||
| pub struct Action<P = (String, String)> { |
There was a problem hiding this comment.
ActionDescription<> with aliases NoPath | EntityPath | ModuleAndEntityPath
There was a problem hiding this comment.
Can we try to use &static str?
There was a problem hiding this comment.
Aliases done but &'static str have not tried yet.
jirijakes
force-pushed
the
access-group
branch
4 times, most recently
from
646ff84 to
eab51ca
Compare
This has an unfortunate consequence in modules CoreUser and Users both being accessed via `users()`, for example `app.users().users().create_user()`. Better naming would be desirable. For example CoreAccess and `app.access().users()`?
thevaibhav-dixit
force-pushed
the
access-group
branch
from
8087009 to
aee47fb
Compare
|
Squashed Vaibhav's and Justin's changes into one commit for easier review. Otherwise no change. One question – with this module-read/write permission sets, some granularity and nuances that existed before are now lost. For example, previously 'assign role to user' and 'revoke role from user' could be done only by a dedicated role (superuser); a role that could create users could not assign roles to them. Now it is possible because they all share the same 'user write' permission set. The question is, how precise and intentional was the previous role-based system? Do we have to preserve all these nuances? Does it have to be precise from the beginning or will we do it gradually, as needed? |
Notes and hightlights for reviewers:
async fn bootstrap_access_control)LanaAction::action_descriptions()Permission Sets are predefined named sets of permissions that can be assigned to Roles by administrators. Their purpose is to shield administrators from the underworld of granular and dynamic permissions.
Tasks:
UserunderCoreUser(f3ea97e)CoreUserwill now containRole,UserandAccessGroupentitiesapp.users().users().create_user()app.access().users(),app.access().roles(),app.access().permission_sets()?app.core_users().users()accounting()vs.core_accounting()app.users()but that would imply a hierarchy of the entities that does otherwise existname alternatives: Permission Set, Permission Group, Permission Bundle or other permutations of the words(renamed from originally proposed Access Group)core/user/src/permission_set/mod.rsPermissionandPermissionstypes are placeholders for something else, unknown yetbullet-proofway to collect all permissions (20770d7)in const context generates hierarchy of modules, objects and actionsusesstrum::EnumCountand Rust arrays to check on compile time that we provide correct number of itemsat first only a few meaningful permission sets + one catch-allat first permission sets mirror currently used roles: superuser, admin, bank manager, accountantCoreUsertoCoreAccessTasks will be updated on the go. The order does not have to be followed, some tasks may be done in parallel and some tasks may be transferred to a distinct PR. Feel free to comment anytime.