From 0b54e837660e5f8147050ce945cb6028f9ea24ca Mon Sep 17 00:00:00 2001 From: claire-peters <34081638+claire-peters@users.noreply.github.com> Date: Mon, 17 Jun 2024 08:40:43 -0700 Subject: [PATCH] AD interaction improvements and UI tweaks (#295) * add ad group addition/removal checks * restrict add/remove user settings, tweak UI --- coldfront/config/core.py | 2 +- .../allocation/allocation_detail.html | 4 +-- .../templates/project/project_detail.html | 16 ++++++--- coldfront/core/project/views.py | 24 +++++++++---- coldfront/plugins/ldap/utils.py | 34 +++++++++++++++---- 5 files changed, 59 insertions(+), 21 deletions(-) diff --git a/coldfront/config/core.py b/coldfront/config/core.py index 0a8862cb1..34dc95f06 100644 --- a/coldfront/config/core.py +++ b/coldfront/config/core.py @@ -30,7 +30,7 @@ ALLOCATION_DEFAULT_ALLOCATION_LENGTH = ENV.int('ALLOCATION_DEFAULT_ALLOCATION_LENGTH', default=365) # Categorization of allocation statuses -PENDING_ALLOCATION_STATUSES = ['New', 'In Progress', 'On Hold'] +PENDING_ALLOCATION_STATUSES = ['New', 'In Progress', 'On Hold', 'Pending Activation'] ACTIVE_ALLOCATION_STATUSES = ['Active'] PENDING_ACTIVE_ALLOCATION_STATUSES = PENDING_ALLOCATION_STATUSES + ACTIVE_ALLOCATION_STATUSES INACTIVE_ALLOCATION_STATUSES = ['Denied', 'Expired', 'Inactive', 'Pending Deactivation'] diff --git a/coldfront/core/allocation/templates/allocation/allocation_detail.html b/coldfront/core/allocation/templates/allocation/allocation_detail.html index 30f0e6e2e..5178ddfb2 100644 --- a/coldfront/core/allocation/templates/allocation/allocation_detail.html +++ b/coldfront/core/allocation/templates/allocation/allocation_detail.html @@ -82,7 +82,7 @@

Allocation Information

{% if allocation.get_resources_as_list %} {% for resource in allocation.get_resources_as_list %} - {% if request.user.is_superuser and allocation.status.name == "New" and "Tier" in allocation.get_resources_as_string %} + {% if request.user.is_superuser and allocation.status.name in "New,In Progress,On Hold" and "Tier" in allocation.get_resources_as_string %} {{ form.resource }} Please choose a resource. {% else %} {{ resource }} @@ -296,7 +296,7 @@

Allocation Information

- {% if request.user.is_superuser or request.user.is_staff and allocation.status.name == 'New' %} + {% if request.user.is_superuser or request.user.is_staff and allocation.status.name in 'New,In Progress,On Hold' %}

To approve the request, select a resource volume in the field above and fill out the form below before clicking "Approve". To deny the request, simply click "Deny". diff --git a/coldfront/core/project/templates/project/project_detail.html b/coldfront/core/project/templates/project/project_detail.html index 2bb301567..dddd49016 100644 --- a/coldfront/core/project/templates/project/project_detail.html +++ b/coldfront/core/project/templates/project/project_detail.html @@ -234,20 +234,24 @@

Allocation H {% for allocation, record in allocation_history_records %} - {{ allocation.get_parent_resource.name }} + + + {{ allocation.get_parent_resource.name }} + + {{ allocation.get_parent_resource.resource_type.name }} {{ allocation.path }} {{ allocation.allocationuser_set.count }} {{ allocation.size|floatformat:1 }} {% if allocation.status.name in 'New,In Progress,On Hold' %} - {{ Requested }} + Requested {% elif allocation.status.name == 'Active' %} {{ allocation.status.name }} {% elif allocation.status.name in 'Pending Deactivation,Denied,Inactive' %} {{ allocation.status.name }} {% else %} - {{ allocation.status.name }} + {{ allocation.status.name }} {% endif %} {% if allocation.status.name not in 'New,Denied' %} @@ -297,8 +301,10 @@

{% if project.status.name != 'Archived' and is_allowed_to_update_project %} Email Project Users - Add Users - Remove Users + {% if request.user.is_superuser or request.user == project.pi %} + Add Users + Remove Users + {% endif %} {% endif %}

diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py index 0467e385a..510cfce88 100644 --- a/coldfront/core/project/views.py +++ b/coldfront/core/project/views.py @@ -460,7 +460,8 @@ class ProjectAddUsersSearchView(LoginRequiredMixin, UserPassesTestMixin, Templat def test_func(self): """UserPassesTestMixin Tests""" project_obj = get_object_or_404(Project, pk=self.kwargs.get('pk')) - if project_obj.has_perm(self.request.user, ProjectPermission.UPDATE): + # if project_obj.has_perm(self.request.user, ProjectPermission.UPDATE): + if self.request.user.is_superuser or self.request.user == project.pi: return True return False @@ -488,7 +489,8 @@ class ProjectAddUsersSearchResultsView( def test_func(self): """UserPassesTestMixin Tests""" project_obj = get_object_or_404(Project, pk=self.kwargs.get('pk')) - if project_obj.has_perm(self.request.user, ProjectPermission.UPDATE): + # if project_obj.has_perm(self.request.user, ProjectPermission.UPDATE): + if self.request.user.is_superuser or self.request.user == project.pi: return True return False @@ -560,7 +562,8 @@ class ProjectAddUsersView(LoginRequiredMixin, UserPassesTestMixin, View): def test_func(self): """UserPassesTestMixin Tests""" project_obj = get_object_or_404(Project, pk=self.kwargs.get('pk')) - if project_obj.has_perm(self.request.user, ProjectPermission.UPDATE): + # if project_obj.has_perm(self.request.user, ProjectPermission.UPDATE): + if self.request.user.is_superuser or self.request.user == project.pi: return True err = 'You do not have permission to add users to the project.' messages.error(self.request, err) @@ -616,6 +619,8 @@ def post(self, request, *args, **kwargs): allocation_form_data = allocation_form.cleaned_data['allocation'] if '__select_all__' in allocation_form_data: allocation_form_data.remove('__select_all__') + errors = [] + successes = [] for form in formset: user_form_data = form.cleaned_data if user_form_data['selected']: @@ -636,7 +641,7 @@ def post(self, request, *args, **kwargs): except Exception as e: error = f"Could not locate user for {user_username}: {e}" logger.error('P665: %s', error) - messages.error(request, error) + errors.append(error) continue role_choice = user_form_data.get('role') @@ -653,13 +658,14 @@ def post(self, request, *args, **kwargs): project_obj.title, ) except Exception as e: - error = f"Could not add user {user_obj} to AD Group for {project_obj.title}: {e}" + error = f"Could not add user {user_obj} to AD Group for {project_obj.title}: {e}\nPlease contact Coldfront administration for further assistance." logger.error( "P685: user %s could not add AD user of %s to AD Group of %s: %s", self.request.user, user_obj, project_obj.title, e ) - messages.error(request, error) + errors.append(error) continue + successes.append(f"User {user_obj} added to AD Group for {project_obj.title}") # Is the user already in the project? project_obj.projectuser_set.update_or_create( @@ -690,6 +696,9 @@ def post(self, request, *args, **kwargs): sender=self.__class__, allocation_user_pk=allocation_user_obj.pk, ) + if errors: + for error in errors: + messages.error(request, error) messages.success(request, f'Added {added_users_count} users to project.') else: @@ -710,7 +719,8 @@ class ProjectRemoveUsersView(LoginRequiredMixin, UserPassesTestMixin, TemplateVi def test_func(self): """UserPassesTestMixin Tests""" project_obj = get_object_or_404(Project, pk=self.kwargs.get('pk')) - if project_obj.has_perm(self.request.user, ProjectPermission.UPDATE): + # if project_obj.has_perm(self.request.user, ProjectPermission.UPDATE): + if self.request.user.is_superuser or self.request.user == project.pi: return True return False diff --git a/coldfront/plugins/ldap/utils.py b/coldfront/plugins/ldap/utils.py index 5e1834e5b..299d77383 100644 --- a/coldfront/plugins/ldap/utils.py +++ b/coldfront/plugins/ldap/utils.py @@ -36,6 +36,15 @@ username_ignore_list = import_from_settings('username_ignore_list', []) +class LDAPException(Exception): + """The base exception class for all LDAPExceptions""" + +class LDAPUserAdditionError(LDAPException): + """An exception raised when a user cannot be added to an LDAP Group""" + +class LDAPUserRemovalError(LDAPException): + """An exception raised when a user cannot be removed from an LDAP Group""" + class LDAPConn: """LDAP connection object """ @@ -71,6 +80,11 @@ def __init__(self, test=False): self.server, self.LDAP_BIND_DN, self.LDAP_BIND_PASSWORD, auto_bind=True ) + def member_in_group(self, member_dn, group_dn): + """Check if a member is in a group""" + group = self.search_groups({'distinguishedName': group_dn, 'member': member_dn}) + return bool(group) + def search(self, attr_search_dict, search_base, attributes=ALL_ATTRIBUTES): """Run an LDAP search. @@ -176,13 +190,16 @@ def add_group_to_group(self, group_name, parent_group_name): self.add_member_to_group(group, parent_group) def add_member_to_group(self, member, group): - group_dn = group['distinguishedName'] member_dn = member['distinguishedName'] + group_dn = group['distinguishedName'] try: result = ad_add_members_to_groups( - self.conn, [member_dn], group_dn, fix=True) + self.conn, [member_dn], group_dn, fix=True, raise_error=True) except Exception as e: - raise e + logger.exception("Error encountered while adding user to group: %s", e) + raise LDAPUserAdditionError("Error adding user to group.") + if not self.member_in_group(member_dn, group_dn): + raise LDAPUserAdditionError("Member not successfully added to group.") return result def remove_member_from_group(self, user_name, group_name): @@ -192,13 +209,18 @@ def remove_member_from_group(self, user_name, group_name): user = self.return_user_by_name(user_name) if user['gidNumber'] == group['gidNumber']: raise ValueError( - "Group is user's primary group. Please contact FASRC support to remove this user from your group.") + "Group is member's primary group. Please contact FASRC support to remove this member from your group.") group_dn = group['distinguishedName'] user_dn = user['distinguishedName'] try: - result = ad_remove_members_from_groups(self.conn, [user_dn], group_dn, fix=True) + result = ad_remove_members_from_groups( + self.conn, [user_dn], group_dn, fix=True, raise_error=True + ) except Exception as e: - raise e + logger.exception("Error encountered while removing user from group: %s", e) + raise LDAPUserRemovalError("Error removing user from group.") + if self.member_in_group(user_dn, group_dn): + raise LDAPUserRemovalError("Member not successfully removed from group.") return result def users_in_primary_group(self, usernames, groupname):