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: allow_to_create_new_org checks org autocreate [FC-0076] #36094

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cms/djangoapps/contentstore/rest_api/v1/serializers/home.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ class StudioHomeSerializer(serializers.Serializer):
child=serializers.CharField(),
allow_empty=True
)
allowed_organizations_for_libraries = serializers.ListSerializer(
child=serializers.CharField(),
allow_empty=True
)
archived_courses = CourseCommonSerializer(required=False, many=True)
can_access_advanced_settings = serializers.BooleanField()
can_create_organizations = serializers.BooleanField()
Expand Down
9 changes: 8 additions & 1 deletion cms/djangoapps/contentstore/rest_api/v1/views/home.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from rest_framework.request import Request
from rest_framework.response import Response
from rest_framework.views import APIView
from organizations import api as org_api
from openedx.core.lib.api.view_utils import view_auth_classes

from ....utils import get_home_context, get_course_context, get_library_context
Expand Down Expand Up @@ -51,6 +52,7 @@ def get(self, request: Request):
"allow_to_create_new_org": true,
"allow_unicode_course_id": false,
"allowed_organizations": [],
"allowed_organizations_for_libraries": [],
"archived_courses": [],
"can_access_advanced_settings": true,
"can_create_organizations": true,
Expand Down Expand Up @@ -79,7 +81,12 @@ def get(self, request: Request):

home_context = get_home_context(request, True)
home_context.update({
'allow_to_create_new_org': settings.FEATURES.get('ENABLE_CREATOR_GROUP', True) and request.user.is_staff,
# 'allow_to_create_new_org' is actually about auto-creating organizations
# (e.g. when creating a course or library), so we add an additional test.
'allow_to_create_new_org': (
home_context['can_create_organizations'] and
org_api.is_autocreate_enabled()
),
Comment on lines -82 to +89
Copy link
Contributor Author

@pomegranited pomegranited Jan 10, 2025

Choose a reason for hiding this comment

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

For reference, the can_create_organizations value is set here:

def user_can_create_organizations(user):
"""
Returns True if the user can create organizations.
"""
return user.is_staff or not settings.FEATURES.get('ENABLE_CREATOR_GROUP', False)

which is pretty different from the original allow_to_create_new_org. But it didn't make sense to me that we'd use a different default value for settings.FEATURES['ENABLE_CREATOR_GROUP'] AND require the user to be staff here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pomegranited This seems reasonable but the logic while creating courses seems different. If the setting to create new org is set to False, the API only allows user/staff to create courses in organizations they are allowed in:

We also have get_allowed_organizations_for_libraries for libraries, so probably we need to do the same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch @navinkarkera ! see f1f40e0

'studio_name': settings.STUDIO_NAME,
'studio_short_name': settings.STUDIO_SHORT_NAME,
'studio_request_email': settings.FEATURES.get('STUDIO_REQUEST_EMAIL', ''),
Expand Down
14 changes: 13 additions & 1 deletion cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ def setUp(self):
self.url = reverse("cms.djangoapps.contentstore:v1:home")
self.expected_response = {
"allow_course_reruns": True,
"allow_to_create_new_org": False,
"allow_to_create_new_org": True,
"allow_unicode_course_id": False,
"allowed_organizations": [],
"allowed_organizations_for_libraries": [],
"archived_courses": [],
"can_access_advanced_settings": True,
"can_create_organizations": True,
Expand Down Expand Up @@ -78,6 +79,17 @@ def test_home_page_studio_with_meilisearch_enabled(self):
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertDictEqual(expected_response, response.data)

@override_settings(ORGANIZATIONS_AUTOCREATE=False)
def test_home_page_studio_with_org_autocreate_disabled(self):
"""Check response content when Organization autocreate is disabled"""
response = self.client.get(self.url)

expected_response = self.expected_response
expected_response["allow_to_create_new_org"] = False

self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertDictEqual(expected_response, response.data)

def test_taxonomy_list_link(self):
response = self.client.get(self.url)
self.assertTrue(response.data['taxonomies_enabled'])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import ddt
from django.contrib.auth.models import Group
from django.test import override_settings
from django.test.client import Client
from freezegun import freeze_time
from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2
Expand Down Expand Up @@ -139,6 +140,63 @@ def test_library_validation(self):
'slug': ['Enter a valid “slug” consisting of Unicode letters, numbers, underscores, or hyphens.'],
}

def test_library_org_validation(self):
"""
Staff users can create libraries in any existing or auto-created organization.
"""
assert Organization.objects.filter(short_name='auto-created-org').count() == 0
self._create_library(slug="auto-created-org-1", title="Library in an auto-created org", org='auto-created-org')
assert Organization.objects.filter(short_name='auto-created-org').count() == 1
self._create_library(slug="existing-org-1", title="Library in an existing org", org="CL-TEST")

@patch(
"openedx.core.djangoapps.content_libraries.views.user_can_create_organizations",
)
@patch(
"openedx.core.djangoapps.content_libraries.views.get_allowed_organizations_for_libraries",
)
@override_settings(ORGANIZATIONS_AUTOCREATE=False)
def test_library_org_no_autocreate(self, mock_get_allowed_organizations, mock_can_create_organizations):
"""
When org auto-creation is disabled, user must use one of their allowed orgs.
"""
mock_can_create_organizations.return_value = False
mock_get_allowed_organizations.return_value = ["CL-TEST"]
assert Organization.objects.filter(short_name='auto-created-org').count() == 0
response = self._create_library(
slug="auto-created-org-2",
org="auto-created-org",
title="Library in an auto-created org",
expect_response=400,
)
assert response == {
'org': "No such organization 'auto-created-org' found.",
}

Organization.objects.get_or_create(
short_name="not-allowed-org",
defaults={"name": "Content Libraries Test Org Membership"},
)
response = self._create_library(
slug="not-allowed-org",
org="not-allowed-org",
title="Library in an not-allowed org",
expect_response=400,
)
assert response == {
'org': "User not allowed to create libraries in 'not-allowed-org'.",
}
assert mock_can_create_organizations.call_count == 1
assert mock_get_allowed_organizations.call_count == 1

self._create_library(
slug="allowed-org-2",
org="CL-TEST",
title="Library in an allowed org",
)
assert mock_can_create_organizations.call_count == 2
assert mock_get_allowed_organizations.call_count == 2

@skip("This endpoint shouldn't support num_blocks and has_unpublished_*.")
@patch("openedx.core.djangoapps.content_libraries.views.LibraryRootView.pagination_class.page_size", new=2)
def test_list_library(self):
Expand Down
12 changes: 12 additions & 0 deletions openedx/core/djangoapps/content_libraries/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@
from rest_framework.views import APIView
from rest_framework.viewsets import GenericViewSet

from cms.djangoapps.contentstore.views.course import (
get_allowed_organizations_for_libraries,
user_can_create_organizations,
)
from openedx.core.djangoapps.content_libraries import api, permissions
from openedx.core.djangoapps.content_libraries.serializers import (
ContentLibraryBlockImportTaskCreateSerializer,
Expand Down Expand Up @@ -269,6 +273,14 @@ def post(self, request):
raise ValidationError( # lint-amnesty, pylint: disable=raise-missing-from
detail={"org": f"No such organization '{org_name}' found."}
)
# Ensure the user is allowed to create libraries under this org
if not (
user_can_create_organizations(request.user) or
org_name in get_allowed_organizations_for_libraries(request.user)
):
raise ValidationError( # lint-amnesty, pylint: disable=raise-missing-from
detail={"org": f"User not allowed to create libraries in '{org_name}'."}
)
org = Organization.objects.get(short_name=org_name)

try:
Expand Down
Loading