tl-its-umich-edu / my-learning-analytics

My Learning Analytics (MyLA)
Apache License 2.0
36 stars 39 forks source link

Provide a way to give staff users access to other courses #1583

Closed jonespm closed 2 weeks ago

jonespm commented 3 months ago

In Django, the staff role allows users access to admin pages and superuser gives the user all permissions.

However there's currently no way to give staff access to view courses that they aren't explicitly in. This is because rules is checking for superuser.

https://github.com/tl-its-umich-edu/my-learning-analytics/blob/d400cce524986a9127ed037ac144009869174527/dashboard/rules.py#L14

There's also some inconsistency about the naming here like in utils it says "is_superuser" is given if the user is staff. This should just be consistent to be "is_staff". Staff and superuser have different meanings.

dashboard/common/utils.py
87:    is_superuser = current_user.is_staff

I think we'd have to do a fix here to either have it so the staff user is the one with full access to every course and make the naming in the code consistent. Or we'd have to add a new explicit permission that could be checked that would give this user access to all the courses. I feel like doing staff is enough but we might have to talk about it. We could probably include this in the next release.


Test Plan:

Test as instructor, Student all feature should work as expected Test with Myla staff role: Can create Myla courses and access then as well Test as Myla full Admin ( is_superuser and is_staff): Full access to whole app

zqian commented 1 month ago

According to the prod database, there is no user who is a staff but not a superuser, or vice versa.

select *  
from auth_user au 
where au.is_staff != au.is_superuser 

So I think this is an easy problem to solve: just remove the check of is_staff, and always check for is_superuser value.

WDYT?

clalande commented 1 month ago

We would like to have users who are staff, who can see & support classes we are running MyLA in, but not have full super user access. We don’t want to enroll them in the classes as instructors.

— Chris


From: Zhen Qian @.> Sent: Tuesday, August 27, 2024 8:39:31 PM To: tl-its-umich-edu/my-learning-analytics @.> Cc: Chris Lalande @.>; Manual @.> Subject: Re: [tl-its-umich-edu/my-learning-analytics] Provide a way to give staff users access to other courses (Issue #1583)

According to the prod database, there is no user who is a staff but not a superuser, or vice versa.

select * from auth_user au where au.is_staff != au.is_superuser

So I think this is an easy problem to solve: just remove the check of is_staff, and always check for is_superuser value.

WDYT?

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/tl-its-umich-edu/my-learning-analytics/issues/1583*issuecomment-2313859114__;Iw!!Mak6IKo!Kv8uyPyyrcotx3qFpdIueuZKo9qWkRYySzpBaPt53zvtpMN1jRm58lEXv4OPq1-Kzqo-93hCVWSPWstkvMHxuHxlv64E$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AAMTRCMNBEXL3C74YWM3Y5LZTUL4HAVCNFSM6AAAAABI65QSCWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJTHA2TSMJRGQ__;!!Mak6IKo!Kv8uyPyyrcotx3qFpdIueuZKo9qWkRYySzpBaPt53zvtpMN1jRm58lEXv4OPq1-Kzqo-93hCVWSPWstkvMHxuCJm9Oii$. You are receiving this because you are subscribed to this thread.Message ID: @.***>

zqian commented 1 month ago

Thanks for the input, Chris!

clalande commented 1 month ago

You bet. I wrote something up originally, I’ll see it I can find it and add it to the issue.

— Chris


From: Zhen Qian @.> Sent: Wednesday, August 28, 2024 9:32:41 AM To: tl-its-umich-edu/my-learning-analytics @.> Cc: Chris Lalande @.>; Manual @.> Subject: Re: [tl-its-umich-edu/my-learning-analytics] Provide a way to give staff users access to other courses (Issue #1583)

Thanks for the input, Chris!

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/tl-its-umich-edu/my-learning-analytics/issues/1583*issuecomment-2315330988__;Iw!!Mak6IKo!MENDHZT-Jb4b0WUEFKPCkmytyWtilP0hVZ46lLvA6KWBqkmaJHpTpz_ERE_btG2d8td213rXBOxDP4sqcXT9bWSval0b$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AAMTRCNFLEYQBCMKW5JVGPLZTXGPTAVCNFSM6AAAAABI65QSCWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJVGMZTAOJYHA__;!!Mak6IKo!MENDHZT-Jb4b0WUEFKPCkmytyWtilP0hVZ46lLvA6KWBqkmaJHpTpz_ERE_btG2d8td213rXBOxDP4sqcXT9bf8B3A02$. You are receiving this because you are subscribed to this thread.Message ID: @.***>

jonespm commented 1 month ago

This is a new feature request,

Generally we've just checked both options and haven't implemented them separately. This issue is to clean that up and make these permissions clear and distinct.

is_superuser would allow all course access and all admin access, including adding users and changing permissions is_staff is more limited and would allow all course access and some admin access

So this would be giving support if is_staff is checked without is_superuser.

clalande commented 1 month ago

Re_MyLA_Error_for_some_Roles.pdf

I found the email thread where we discussed this last spring, and have attached this as a PDF for reference.

jennlove-um commented 1 week ago

Testing passes in beta for student, teacher, myla-admin-staff, and myla-admin-superuser.