AD interaction improvements and UI tweaks (#295)

* add ad group addition/removal checks
* restrict add/remove user settings, tweak UI

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']

coldfront/core/allocation/templates/allocation/allocation_detail.html

@@ -82,7 +82,7 @@ <h3><i class="fas fa-list" aria-hidden="true"></i> Allocation Information</h3>
             <td>
               {% 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 }} <a color="red">Please choose a resource.</a>
                   {% else %}
                     <a href="{% url 'resource-detail' resource.pk %}">{{ resource }}</a>
@@ -296,7 +296,7 @@ <h3><i class="fas fa-list" aria-hidden="true"></i> Allocation Information</h3>
       </div>
 
       <!-- New Allocation Action Form -->
-      {% 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' %}
         <div class="container mb-3 mt-3">
           <p class="text-justify"><i>
             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".

coldfront/core/project/templates/project/project_detail.html

@@ -234,20 +234,24 @@ <h3 class="d-inline"><i class="fas fa-list" aria-hidden="true"></i> Allocation H
         <tbody>
           {% for allocation, record in allocation_history_records %}
           <tr style="background-color:#FFFFFF">
-            <td>{{ allocation.get_parent_resource.name }}</td>
+            <td>
+              <a href="{% url 'allocation-detail' allocation.pk %}">
+		    {{ allocation.get_parent_resource.name }}
+              </a>
+	    </td>
             <td>{{ allocation.get_parent_resource.resource_type.name }}</td>
             <td>{{ allocation.path }}</td>
             <td>{{ allocation.allocationuser_set.count }}</td>
             <td>{{ allocation.size|floatformat:1 }}</td>
 
             {% if allocation.status.name in 'New,In Progress,On Hold' %}
-              <td class="text-info">{{ Requested }}</td>
+              <td class="text-success">Requested</td>
             {% elif allocation.status.name == 'Active' %}
               <td class="text-success">{{ allocation.status.name }}</td>
             {% elif allocation.status.name in 'Pending Deactivation,Denied,Inactive' %}
               <td class="text-danger">{{ allocation.status.name }}</td>
             {% else %}
-              <td class="text-info">{{ allocation.status.name }}</td>
+              <td class="text-success">{{ allocation.status.name }}</td>
             {% endif %}
             <td>
               {% if allocation.status.name not in 'New,Denied' %}
@@ -297,8 +301,10 @@ <h3 class="d-inline" id="users"><i class="fas fa-users" aria-hidden="true"></i>
     <div class="float-right">
       {% if project.status.name != 'Archived' and is_allowed_to_update_project %}
         <a class="btn btn-primary" href="{{mailto}}" role="button"><i class="far fa-envelope" aria-hidden="true"></i> Email Project Users</a>
-        <a class="btn btn-success" href="{% url 'project-add-users-search' project.id %}" role="button"><i class="fas fa-user-plus" aria-hidden="true"></i> Add Users</a>
-        <a class="btn btn-danger" href="{% url 'project-remove-users' project.id %}" role="button"><i class="fas fa-user-times" aria-hidden="true"></i> Remove Users</a>
+        {% if request.user.is_superuser or request.user == project.pi %}
+          <a class="btn btn-success" href="{% url 'project-add-users-search' project.id %}" role="button"><i class="fas fa-user-plus" aria-hidden="true"></i> Add Users</a>
+          <a class="btn btn-danger" href="{% url 'project-remove-users' project.id %}" role="button"><i class="fas fa-user-times" aria-hidden="true"></i> Remove Users</a>
+	{% endif %}
       {% endif %}
     </div>
   </div>

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
 

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):