[users] Implementing user_group

[frgfm]

Following up on #45, I think it is sound to have a discussion about the multiple access levels in the API. With the current design of scopes, we cannot efficiently separate accesses of large user groups.

Requirements

• We need to restrict "superadmin" from common users
• Users need to be segmented into groups, where each group only has access to a restricted scope (i.e. a local firefighter, while having a "user" scope, should not have access to the same data as someone from another region, while that person is also a "user")
• Groups need to be easily editable
• security on routes will have to combine those to provide proper and secure access to each route

Design suggestion

Simply put, I believe two fields are required:

• access_type: admin, user, device, me
• access_group: ID that would reference a group (we could add a groups table with group_id, and group_name)

[frgfm]

Hi @florianriche :wave:

I had the occasion to think about this over the weekend and here is my suggestion !

Feature constraints

• groups need to be easily editable (adding / removing users)
• groups need to condition the access of a user on multiple (perhaps all) routes
• in theory, the user itself (apart from superadmins, and say "managers"/"supervisors") cannot change its group allocation
• the group allocation need to spread further onto other objects (listing all devices from a specific region, or all events ?)

Design suggestion

I think we would benefit from splitting the work into the following PRs:

• [x] Adding a "groups" table with the following fields: id, name (make a first PR with this + unittests, I suggest adapting #137 for this)
• [x] Modifying the "access" table to have id, login, hashed_password, type (formerly "scopes"), group_id (so that we could dynamically link a user/device to a group) #144
• [x] Tracing each table to a group:
• users have their group_id through their access (superadmins would have no group),
• devices too (by default set to the same group id as the user that register it)
• installation, alert, media through their device
• event indirectly by checking their starting alert (unsure how to handle if multiple alerts from different groups point towards it)
• sites will need a new group_id field (#147)
• [ ] Reviewing route access: in the same fashion as for current Security, if we want to protect a GET route to the group, we would require to have either the admin access type (fetching everything) or by checking the group_id, we limit results to the one accessible to this group

Any thoughts @pyronear/back-end ?

[florianriche]

Seems like a good design to me. However I think there is an issue for the last point. In the "Security" schema, you don't have have access to the requested id. E.g for this route below:

@router.get("/{user_id}/", response_model=UserRead, summary="Get information about a specific user")
async def get_user(user_id: int = Path(..., gt=0), _=Security(get_current_user, scopes=["admin"])):

We can't pass user_id to get_current_user (or any other function for that matters). I guess that could be solved through a refactor (e.g: using a decorator somehow but my point) but my point is that it won't be as trivial as expected.

[frgfm]

@florianriche sorry I didn't get your point. Since get_current_user can be changed with get_current_access we will have access to the scopes and group_id for instance ?

Apart from that, let's turn your #137 into the standalone group PR if that's alright with you :)

[florianriche]

What I mean is that you can't pass arguments to get_current_user/get_current_access. So you can't make sure that the resources that is being requested belong to the current user group. You will have to handle that loic directly into the route code. That is doable, just not as clean as altering get_current_user. But maybe I am overlooking somehing.

Agree to alter #137, will do that.

[florianriche]

@pyronear/back-end / @frgfm I am ready to iterate further on that topic but do we agree that we can't use exactly the same mechanism as the "Security" one (see previous message) ? I just want to make sure I understand all the mechanisms you have in mind.

[frgfm]

Yes I agree!

[frgfm]

@pyronear/back-end a few relevant questions:

1. for tables that do not have an access, should we add group_id field ? (installation, alert, media, event)
2. How do process group information?

On the second part, here is a suggestion

Route type	scope	group processing
GET	admin	None
GET	other	denied unless the requested object & the access share the same group_id**
POST	admin	can specify group_id upon creation
POST	other	group_id automatically set to the same as the access
UPDATE	admin	can update group_id
UPDATE	other	denied unless the requested object & the access share the same group_id
DELETE	admin	None
DELETE	other	denied unless the requested object & the access share the same group_id and that the route doesn't require admin rights

** For general fetching (not a specific id), it would filter the results

This is just a general suggestion but each route might deserve its own decision.

What do you think?

[florianriche]

Overall I agree with your suggestion. Although I above all agree with the fact we will need to think about it on a route by route basis.

A few comments/questions:

• I would keep the schema as is because I hope that at some point we will be more integrated with SqlAlchemy. Using sqlALchemy syntax, one can easily access fields through other tables.
• A few questions and comments:
  • Can you specify group_id upon creation => This is valid only for site/user/device right?
  • And group_id automatically set to the same as the access ==> One also need to check the entities also belong to the same group (e.g device_id specified in installation matches the access)
  • can update group_id; you mean it is the only entity that can update the group_id field ?

NB: In this implementation, all groups are identical, there is no hierarchy notion. In the future we will need to have another system to manage complex relationships

[frgfm]

Regarding your questions:

• correct
• installations routes being admin only, I didn't even consider this possibility to be honest
• I meant: only an admin can update the group_id of an entity

Regarding the horizontally of groups: yes, I kept this for another issue but I would suggest having in access: type (user, admin, device), group_id (SDIS 07 vs SDIS 40), level (group manager vs. member)