fix: give superusers all studio permissions [BB-7481]#32569
fix: give superusers all studio permissions [BB-7481]#325690x29a wants to merge 1 commit intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @0x29a! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
|
@0x29a This works but the course listing page in studio shows nothing for such a user. This is because the course listing code also makes use of def has_user(self, user):
return bool(user and (user.is_superuser or user.is_staff)) |
|
@navinkarkera, ah, you right, thanks for catching this. I wanted to avoid changing the |
3611a39 to
8ef5575
Compare
|
@0x29a 👍
Yep, even I am now a bit unsure about our solution here. The user cannot login to django admin even if they are a superuser which feels weird. Listing one more option for discussion: set staff as True if superuser is True while saving a user. |
|
Hi @0x29a - just checking in on this! |
|
Hi @0x29a! Checking back in on this, and flagging the new tests that have popped up per the recent update: https://discuss.openedx.org/t/minor-change-to-edx-platform-check-statuses/11131 |
469c9e2 to
26dca2c
Compare
|
@mphilbrick211, apologies for the delay here. I rebased the branch, looks like it passed all checks. |
|
Hi @openedx/teaching-and-learning! This is ready for review. |
26dca2c to
243d5ea
Compare
|
Hi @gabor-boros! Just flagging that there are a couple failing checks. |
243d5ea to
408ee9f
Compare
|
@mphilbrick211, I think those checks failed due to some issues unrelated to these changes. Now they're passing, so this is ready for upstream review. |
|
Hi @openedx/teaching-and-learning - putting this one back on the radar! |
|
Hi @mphilbrick211, any news regarding this pull request? |
My apologies @0x29a - I will check on this for you. |
| """ | ||
| def has_user(self, user): | ||
| return bool(user and user.is_staff) | ||
| return bool(user and (user.is_superuser or user.is_staff)) |
There was a problem hiding this comment.
Conceptually, "global staff" and "super user" are two different roles. I don't think that this PR is helpful, because it says that "superusers have the global staff role", which I don't think is true.
There are two root problems and IMHO we should try to fix one of the root problems if we're going to be fixing something here:
- The django
is_staffflag is meant to indicate whether the user has access to the django admin. It is not meant to be used for other things like indicating some openedx-specific "global staff" role that grants all kinds of other permissions. A newis_global_staffflag should be created to separate this role from the "has django admin access" role. (This is very similar to how theis_activeflag is supposed to enabled/disable accounts, but is instead used to track email activation, which causes a number of bugs and requires workarounds throughout the system.) - Somewhere, the code to check "can this user access Studio" is implemented wrong. It should only be checking for a permission, and any permissions check will always return
Truefor superusers. But instead, it's hard-coded to check for specific roles like "global staff", which is why the superuser role is seemingly not granting the permission.
CC @hsinkoff
|
Sandbox destroy request received. |
|
Sandbox deployment successful. Sandbox LMS is available at pr-32569-139931.staging.do.opencraft.hosting Instance config : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5652418326-config.yml |
|
@0x29a Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Users that have
is_superuser == True, butis_staff == Falsecan't access Studio. This PR fixes this.Testing instructions
is_stafffield toFalse.View in Studiobutton here:private-ref