Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: give superusers all studio permissions [BB-7481] #32569

Conversation

0x29a
Copy link
Contributor

@0x29a 0x29a commented Jun 26, 2023

Description

Users that have is_superuser == True, but is_staff == False can't access Studio. This PR fixes this.

Testing instructions

  1. Switch your devstack to this branch.
  2. Create a superuser, set it's is_staff field to False.
  3. Try clicking View in Studio button here:
    image
  4. You should be able to access Studio.

private-ref

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 26, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Jun 26, 2023

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@navinkarkera
Copy link
Contributor

@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 GlobalStaff().has_user. Should we update this method instead? Something like

    def has_user(self, user):
        return bool(user and (user.is_superuser or user.is_staff))

@0x29a
Copy link
Contributor Author

0x29a commented Jun 26, 2023

@navinkarkera, ah, you right, thanks for catching this. I wanted to avoid changing the GlobalStaff behavior in any way, but it looks like we have to. I'll update this shortly.

@0x29a 0x29a force-pushed the 0x29a/bb7481/give-superusers-all-studio-permissions branch from 3611a39 to 8ef5575 Compare June 26, 2023 11:05
@navinkarkera
Copy link
Contributor

@0x29a 👍

  • I tested this: (Tested in local devstack by creating a superuser with no staff access)
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

wanted to avoid changing the GlobalStaff behavior in any way, but it looks like we have to

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.

@mphilbrick211
Copy link

Hi @0x29a - just checking in on this!

@mphilbrick211
Copy link

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

@0x29a 0x29a force-pushed the 0x29a/bb7481/give-superusers-all-studio-permissions branch 2 times, most recently from 469c9e2 to 26dca2c Compare September 13, 2023 11:10
@0x29a
Copy link
Contributor Author

0x29a commented Sep 13, 2023

@mphilbrick211, apologies for the delay here. I rebased the branch, looks like it passed all checks.

@mphilbrick211 mphilbrick211 requested a review from a team September 14, 2023 13:36
@mphilbrick211
Copy link

Hi @openedx/teaching-and-learning! This is ready for review.

@gabor-boros gabor-boros force-pushed the 0x29a/bb7481/give-superusers-all-studio-permissions branch from 26dca2c to 243d5ea Compare October 1, 2023 07:34
@mphilbrick211
Copy link

Hi @gabor-boros! Just flagging that there are a couple failing checks.

@0x29a 0x29a force-pushed the 0x29a/bb7481/give-superusers-all-studio-permissions branch from 243d5ea to 408ee9f Compare October 16, 2023 09:40
@0x29a
Copy link
Contributor Author

0x29a commented Oct 16, 2023

@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.

@mphilbrick211
Copy link

Hi @openedx/teaching-and-learning - putting this one back on the radar!

@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Oct 25, 2023
@0x29a
Copy link
Contributor Author

0x29a commented Nov 8, 2023

Hi @mphilbrick211, any news regarding this pull request?

@mphilbrick211
Copy link

Hi @mphilbrick211, any news regarding this pull request?

My apologies @0x29a - I will check on this for you.

@@ -136,7 +136,7 @@ class GlobalStaff(AccessRole):
The global staff role
"""
def has_user(self, user):
return bool(user and user.is_staff)
return bool(user and (user.is_superuser or user.is_staff))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The django is_staff flag 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 new is_global_staff flag should be created to separate this role from the "has django admin access" role. (This is very similar to how the is_active flag 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.)
  2. 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 True for 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

@open-craft-grove
Copy link

Sandbox destroy request received.

@openedx-webhooks
Copy link

@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.

@0x29a 0x29a deleted the 0x29a/bb7481/give-superusers-all-studio-permissions branch February 21, 2024 10:07
@itsjeyd itsjeyd removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

7 participants