From 259dbf605c86ef3e112277db055629ffa41b3a5e Mon Sep 17 00:00:00 2001
From: Navin Karkera <navin@opencraft.com>
Date: Tue, 13 Jun 2023 16:06:25 +0530
Subject: [PATCH] fix: allow only enrolled users in course forum roles

The course team management section under Instructor > Membership tab
allows users to be added a role even if are not enrolled in the course.
This is not the expected behaviour based on the help text displayed in
the section.

This PR updates update_forum_role_membership api to check whether user
is enrolled before adding them to a role.

Cherry pick from https://github.com/openedx/edx-platform/pull/32436

(cherry picked from commit 10377ea7d0b1182f5d0b6d389161fab2bac1722a)
---
 lms/djangoapps/instructor/tests/test_api.py   | 33 +++++++++++++++++++
 lms/djangoapps/instructor/views/api.py        |  6 ++++
 .../js/instructor_dashboard/membership.js     |  5 +++
 3 files changed, 44 insertions(+)

diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py
index eab63e2b927a..f34cb560e2d0 100644
--- a/lms/djangoapps/instructor/tests/test_api.py
+++ b/lms/djangoapps/instructor/tests/test_api.py
@@ -2212,11 +2212,16 @@ def setUp(self):
         super().setUp()
 
         self.instructor = InstructorFactory(course_key=self.course.id)
+        CourseEnrollment.enroll(self.instructor, self.course.id)
         self.client.login(username=self.instructor.username, password='test')
 
         self.other_instructor = InstructorFactory(course_key=self.course.id)
+        CourseEnrollment.enroll(self.other_instructor, self.course.id)
         self.other_staff = StaffFactory(course_key=self.course.id)
+        CourseEnrollment.enroll(self.other_staff, self.course.id)
         self.other_user = UserFactory()
+        CourseEnrollment.enroll(self.other_user, self.course.id)
+        self.unenrolled_user = UserFactory()
 
     def test_modify_access_noparams(self):
         """ Test missing all query parameters. """
@@ -2432,6 +2437,34 @@ def assert_update_forum_role_membership(self, current_user, identifier, rolename
         elif action == 'revoke':
             assert rolename not in user_roles
 
+    def test_update_forum_role_with_unenrolled_user(self):
+        """
+        Check response of update_forum_role_membership api with unenrolled user.
+        """
+
+        def _check_allow_access_with_unenrolled_user(url, role):
+            """
+            Helper function for testing api.
+            """
+            response = self.client.post(url, {
+                'unique_student_identifier': self.unenrolled_user.username,
+                'rolename': role,
+                'action': 'allow',
+            })
+            assert response.status_code == 200
+            expected = {
+                'unique_student_identifier': self.unenrolled_user.username,
+                'userNotEnrolled': True,
+            }
+            res_json = json.loads(response.content.decode('utf-8'))
+            assert res_json == expected
+            user_roles = self.unenrolled_user.roles.filter(course_id=self.course.id).values_list("name", flat=True)
+            assert role not in user_roles
+
+        forum_role_url = reverse('update_forum_role_membership', kwargs={'course_id': str(self.course.id)})
+        _check_allow_access_with_unenrolled_user(forum_role_url, 'Community TA')
+        _check_allow_access_with_unenrolled_user(forum_role_url, 'Administrator')
+
 
 @ddt.ddt
 class TestInstructorAPILevelsDataDump(SharedModuleStoreTestCase, LoginEnrollmentTestCase):
diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py
index 39ec25b703f6..f332105a5ce2 100644
--- a/lms/djangoapps/instructor/views/api.py
+++ b/lms/djangoapps/instructor/views/api.py
@@ -2807,6 +2807,12 @@ def update_forum_role_membership(request, course_id):
         ))
 
     user = get_student_from_identifier(unique_student_identifier)
+    if action == 'allow' and not is_user_enrolled_in_course(user, course_id):
+        response_payload = {
+            'unique_student_identifier': unique_student_identifier,
+            'userNotEnrolled': True,
+        }
+        return JsonResponse(response_payload)
 
     try:
         update_forum_role(course_id, user, rolename, action)
diff --git a/lms/static/js/instructor_dashboard/membership.js b/lms/static/js/instructor_dashboard/membership.js
index b29fd85b3daa..81f54a35dbc1 100644
--- a/lms/static/js/instructor_dashboard/membership.js
+++ b/lms/static/js/instructor_dashboard/membership.js
@@ -281,6 +281,11 @@ such that the value can be defined later than this assignment (file load order).
                 return this.show_errors(
                     gettext('Error: You cannot remove yourself from the Instructor group!')
                 );
+            } else if (data.userNotEnrolled) {
+                msg = gettext("Error: User '<%- username %>' is not enrolled in this course.")
+                return this.show_errors(_.template(msg)({
+                    username: data.unique_student_identifier
+                }));
             } else {
                 return this.reload_list();
             }