search

nelc / futurex-openedx-extensions

Open edX APIs and reporting tools
Other
0 stars 0 forks source link

bug: get roles can report a user with no roles #69

Closed shadinaif closed 2 weeks ago

shadinaif commented 2 months ago

Get roles API will report a user with empty roles if that user has no roles in the tenants, but has a global role.

This is not wrong data, but rather a wrong representation of data. The user indeed has access to the tenant because of the global role course_creator_role. But the serializer is not designed to represent global roles

{
    "user_id": 20095,
    "email": "shadinaif+2@gmail.com",
    "username": "shadinaif2",
    "full_name": "Shadi Naif",
    "alternative_full_name": "",
    "tenants": {}
}

to resolve this, I think of two solutions:

option1: do not show the record at all is that right? we're not showing superusers and system-staff which is fine. But if a non system-staff has explicit access to create roles in my tenant, then I need to know!

option2: add global roles to the result's root

{
    "user_id": 20095,
    "email": "shadinaif+2@gmail.com",
    "username": "shadinaif2",
    "full_name": "Shadi Naif",
    "alternative_full_name": "",
    "global_roles": ["course_creator_group"],
    "tenants": {}
}
shadinaif commented 1 month ago

@OmarIthawi please take a look here and let me know what you think. We have two global roles in the system: course_creator_group and support

OmarIthawi commented 1 month ago

option1: do not show the record at all is that right? we're not showing superusers and system-staff which is fine. But if a non system-staff has explicit access to create roles in my tenant, then I need to know!

In my opinion, this is the right role. Given the complexity we have in the system at the moment, providing less information should provide a better UX.

These users are similar to superuser and is_staff accounts, those are mostly only NeLC accounts that have system-wide permissions.

Worst-case, we should remove those roles from the database and replace them with more standard roles such as user.is_staff so it's not a concern.

shadinaif commented 2 weeks ago

this is resolved by adding the global_roles in the response and removing course_creator_group in the data cleaning process. UX is important as mentioned by Omar, but having an odd user displayed in all tenants because of an unneeded role is a good thing in my opinion. It'll avoid having global access for a user by mistake