Merge pull request #568 from ucb-rit/secure_dir_bug_fixes

Clean user-provided secure directory name

coldfront/core/allocation/forms_/secure_dir_forms.py

@@ -1,8 +1,32 @@
 from django import forms
 from django.core.validators import MinLengthValidator
 
-from coldfront.core.allocation.utils_.secure_dir_utils import \
-    sec_dir_name_available
+from coldfront.core.allocation.utils_.secure_dir_utils import is_secure_directory_name_suffix_available
+from coldfront.core.allocation.utils_.secure_dir_utils import SECURE_DIRECTORY_NAME_PREFIX
+
+
+class SecureDirNameField(forms.CharField):
+
+    def __init__(self, *args, **kwargs):
+        self.exclude_request_pk = kwargs.pop('exclude_request_pk', None)
+        super().__init__(*args, **kwargs)
+
+    def clean(self, directory_name):
+        # If the user does not provide the prefix, prepend it.
+        if not directory_name.startswith(SECURE_DIRECTORY_NAME_PREFIX):
+            directory_name = f'{SECURE_DIRECTORY_NAME_PREFIX}{directory_name}'
+        # If the directory name (sans the prefix) is not unique, raise an error.
+        # Optionally exclude the request the name is associated with.
+        directory_name_suffix = directory_name[
+            len(SECURE_DIRECTORY_NAME_PREFIX):]
+        if not is_secure_directory_name_suffix_available(
+                directory_name_suffix,
+                exclude_request_pk=self.exclude_request_pk):
+            raise forms.ValidationError(
+                f'The directory name {directory_name} is already taken. Please '
+                f'choose another.')
+        # Return the full, prefixed directory name.
+        return directory_name
 
 
 class SecureDirManageUsersForm(forms.Form):
@@ -86,9 +110,10 @@ def __init__(self, *args, **kwargs):
 
 class SecureDirDirectoryNamesForm(forms.Form):
 
-    directory_name = forms.CharField(
+    directory_name = SecureDirNameField(
         help_text=(
-            'Provide the name of the requested secure directory on the cluster.'),
+            'Provide the name of the requested secure directory on the '
+            'cluster.'),
         label='Subdirectory Name',
         required=True,
         widget=forms.Textarea(attrs={'rows': 1}))
@@ -98,16 +123,6 @@ def __init__(self, *args, **kwargs):
         kwargs.pop('breadcrumb_project', None)
         super().__init__(*args, **kwargs)
 
-    def clean(self):
-        cleaned_data = super().clean()
-        directory_name = cleaned_data.get('directory_name', None)
-
-        # Provided directory name must be unique.
-        if not sec_dir_name_available(directory_name):
-            raise forms.ValidationError(
-                'This directory name is already taken. Please choose another.')
-        return cleaned_data
-
 
 class SecureDirSetupForm(forms.Form):
 
@@ -122,12 +137,8 @@ class SecureDirSetupForm(forms.Form):
         label='Status',
         required=True)
 
-    directory_name = forms.CharField(
-        help_text=(
-            'Edit the provided directory name if necessary.'),
-        label='Subdirectory Name',
-        required=False,
-        widget=forms.Textarea(attrs={'rows': 1}))
+    # Overridden in __init__.
+    directory_name = forms.CharField()
 
     justification = forms.CharField(
         help_text=(
@@ -144,12 +155,17 @@ def __init__(self, *args, **kwargs):
         dir_name = kwargs.pop('dir_name', None)
         super().__init__(*args, **kwargs)
 
-        self.fields['directory_name'].initial = dir_name
+        self.fields['directory_name'] = SecureDirNameField(
+            help_text='Edit the provided directory name if necessary.',
+            initial=dir_name,
+            label='Subdirectory Name',
+            required=False,
+            widget=forms.Textarea(attrs={'rows': 1}),
+            exclude_request_pk=self.request_pk)
 
     def clean(self):
         cleaned_data = super().clean()
         status = cleaned_data.get('status', 'Pending')
-        directory_name = cleaned_data.get('directory_name', None)
 
         # Require justification for denials.
         if status == 'Denied':
@@ -158,13 +174,6 @@ def clean(self):
                 raise forms.ValidationError(
                     'Please provide a justification for your decision.')
 
-            return cleaned_data
-
-        # Provided directory name must be unique.
-        if not sec_dir_name_available(directory_name, self.request_pk):
-            raise forms.ValidationError(
-                'This directory name is already taken. Please choose another.')
-
         return cleaned_data
 
 

coldfront/core/allocation/templates/secure_dir/secure_dir_request/directory_name.html

@@ -31,8 +31,8 @@ <h1>Secure Directory: Directory Name</h1><hr>
   <p>Please provide the name of the secure directory you are requesting.</p>
 
   <p>
-    For example, to request the secure directories <em>{{ scratch_path }}/pl1_example</em> and
-    <em>{{ groups_path }}/pl1_example</em>, you would provide <strong>example</strong>.
+    For example, to request the secure directories <em>{{ scratch_path }}/{{ directory_name_prefix }}example</em> and
+    <em>{{ groups_path }}/{{ directory_name_prefix }}example</em>, you would provide <strong>example</strong>.
   </p>
   <p>
     Note that you will receive both a groups and a scratch secure directory upon approval of this request.

coldfront/core/allocation/templates/secure_dir/secure_dir_request/secure_dir_request_card.html

@@ -52,17 +52,17 @@ <h2>
 
     <p class="card-text text-justify">
       <strong>Proposed Directory Name: </strong>
-      {{ secure_dir_request.directory_name }}
+      {{ proposed_directory_name }}
     </p>
 
     <p class="card-text text-justify">
       <strong>Proposed Groups Path: </strong>
-      {{ groups_path }}
+      {{ proposed_groups_path }}
     </p>
 
     <p class="card-text text-justify">
       <strong>Proposed Scratch Path: </strong>
-      {{ scratch_path }}
+      {{ proposed_scratch_path }}
     </p>
   </div>
 </div>

coldfront/core/allocation/tests/test_utils/test_secure_dir_utils.py

@@ -30,7 +30,7 @@ def setUp(self):
 
         self.project1 = self.create_active_project_with_pi('project1', self.pi)
 
-        self.subdirectory_name = 'test_dir'
+        self.subdirectory_name = 'pl1_test_dir'
         call_command('add_directory_defaults')
         create_secure_dirs(self.project1, self.subdirectory_name, 'groups')
         create_secure_dirs(self.project1, self.subdirectory_name, 'scratch')

coldfront/core/allocation/tests/test_views/test_secure_dir_request_views.py

@@ -168,7 +168,7 @@ def test_post_creates_request(self):
             form_data[1]['1-rdm_consultants'])
         self.assertEqual(
             request.directory_name,
-            form_data[2]['2-directory_name'])
+            f'pl1_{form_data[2]["2-directory_name"]}')
         self.assertEqual(request.project, self.project1)
         self.assertTrue(
             pre_time < request.request_time < utc_now_offset_aware())
@@ -444,9 +444,9 @@ def test_post_request_emails_sent(self):
                       f'on the cluster is complete.',
                       f'The paths to your secure group and scratch directories '
                       f'are \'/global/home/groups/pl1data/'
-                      f'pl1_{self.request0.directory_name}\' and '
+                      f'{self.request0.directory_name}\' and '
                       f'\'/global/scratch/p2p3/'
-                      f'pl1_{self.request0.directory_name}\', '
+                      f'{self.request0.directory_name}\', '
                       f'respectively.']
 
         self.assertEqual(len(pi_emails), len(mail.outbox))
@@ -780,7 +780,8 @@ def test_post_updates_request(self):
 
         self.request0.refresh_from_db()
         self.assertRedirects(response, self.success_url)
-        self.assertEqual(self.request0.directory_name, data['directory_name'])
+        self.assertEqual(
+            self.request0.directory_name, f'pl1_{data["directory_name"]}')
         self.assertEqual(self.request0.status.name, 'Approved - Processing')
         self.assertEqual(self.request0.state['setup']['status'], 'Completed')
 
@@ -792,6 +793,7 @@ def test_denied_status_denies_request(self):
         """Tests that a Denied status denies the request."""
         self.client.login(username=self.admin.username, password=self.password)
         data = {'status': 'Denied',
+                'directory_name': self.request0.directory_name,
                 'justification': 'This is a test denial justification.'}
         response = self.client.post(self.url, data)
 

coldfront/core/allocation/utils_/secure_dir_utils.py

@@ -19,6 +19,10 @@
 logger = logging.getLogger(__name__)
 
 
+# All project-specific secure subdirectories begin with the following prefix.
+SECURE_DIRECTORY_NAME_PREFIX = 'pl1_'
+
+
 def create_secure_dirs(project, subdirectory_name, scratch_or_groups):
     """
     Creates one secure directory allocation: either a group directory or a
@@ -232,7 +236,7 @@ def call_create_secure_dir(self):
         """Creates the groups and scratch secure directories."""
 
         groups_alloc, scratch_alloc = None, None
-        subdirectory_name = f'pl1_{self.request_obj.directory_name}'
+        subdirectory_name = self.request_obj.directory_name
         try:
             groups_alloc = \
                 create_secure_dirs(self.request_obj.project,
@@ -400,29 +404,51 @@ def get_all_secure_dir_paths():
     return paths
 
 
-def sec_dir_name_available(directory_name, request_pk=None):
-    """Returns True if the proposed directory name is available
-    and False otherwise.
+def is_secure_directory_name_suffix_available(proposed_directory_name_suffix,
+                                              exclude_request_pk=None):
+    """Returns True if the proposed secure directory name suffix is
+    available and False otherwise. A name suffix is available if it is
+    neither in use by an existing secure directory nor in use by a
+    pending request for a new secure directory, with the possible
+    exception of the request with the given primary key from which it
+    came.
 
     Parameters:
-    - directory_name (str): the name of the proposed directory
-    - request_pk (int): the primary key of the request obj to exclude
+        - proposed_directory_name_suffix (str): The name of the proposed
+            directory, without SECURE_DIRECTORY_NAME_PREFIX
+        - exclude_request_pk (int): The primary key of a SecureDirRequest
+            object to exclude
 
     Returns:
-        - bool: True if the proposed directory name is available, False
-                otherwise
+        - bool: True if the proposed directory name suffix is available,
+            False otherwise
     """
 
-    paths = get_all_secure_dir_paths()
-    cleaned_dir_names = set([path.strip().split('_')[-1] for path in paths])
-
-    pending_request_dirs = \
-        set(SecureDirRequest.objects.exclude(
-            status__name='Denied').exclude(
-            pk=request_pk).values_list('directory_name', flat=True))
-    cleaned_dir_names.update(pending_request_dirs)
-
-    return directory_name not in cleaned_dir_names
+    def get_directory_name_suffix(_directory_name):
+        if _directory_name.startswith(SECURE_DIRECTORY_NAME_PREFIX):
+            _directory_name = _directory_name[
+                len(SECURE_DIRECTORY_NAME_PREFIX):]
+        return _directory_name
+
+    assert not proposed_directory_name_suffix.startswith(
+        SECURE_DIRECTORY_NAME_PREFIX)
+
+    unavailable_name_suffixes = set()
+    existing_secure_directory_paths = get_all_secure_dir_paths()
+    for directory_path in existing_secure_directory_paths:
+        directory_name = os.path.basename(directory_path)
+        directory_name_suffix = get_directory_name_suffix(directory_name)
+        unavailable_name_suffixes.add(directory_name_suffix)
+    pending_requested_directory_names = list(
+        SecureDirRequest.objects
+            .exclude(status__name='Denied')
+            .exclude(pk=exclude_request_pk)
+            .values_list('directory_name', flat=True))
+    for directory_name in pending_requested_directory_names:
+        directory_name_suffix = get_directory_name_suffix(directory_name)
+        unavailable_name_suffixes.add(directory_name_suffix)
+
+    return proposed_directory_name_suffix not in unavailable_name_suffixes
 
 
 def set_sec_dir_context(context_dict, request_obj):
@@ -445,8 +471,9 @@ def set_sec_dir_context(context_dict, request_obj):
         raise TypeError(f'Invalid SecureDirRequest {request_obj}.')
 
     context_dict['secure_dir_request'] = request_obj
+    context_dict['proposed_directory_name'] = request_obj.directory_name
     groups_path, scratch_path = get_default_secure_dir_paths()
-    context_dict['groups_path'] = \
-        os.path.join(groups_path, request_obj.directory_name)
-    context_dict['scratch_path'] = \
-        os.path.join(scratch_path, request_obj.directory_name)
+    context_dict['proposed_groups_path'] = \
+        os.path.join(groups_path, context_dict['proposed_directory_name'])
+    context_dict['proposed_scratch_path'] = \
+        os.path.join(scratch_path, context_dict['proposed_directory_name'])

coldfront/core/allocation/views_/secure_dir_views.py

@@ -36,14 +36,16 @@
     get_secure_dir_manage_user_request_objects, secure_dir_request_state_status, \
     SecureDirRequestDenialRunner, SecureDirRequestApprovalRunner, \
     get_secure_dir_allocations, get_default_secure_dir_paths, \
-    pi_eligible_to_request_secure_dir, set_sec_dir_context
+    pi_eligible_to_request_secure_dir, SECURE_DIRECTORY_NAME_PREFIX, \
+    set_sec_dir_context
 from coldfront.core.project.forms import ReviewStatusForm, ReviewDenyForm
 from coldfront.core.project.models import ProjectUser, Project
 from coldfront.core.user.utils import access_agreement_signed
 from coldfront.core.utils.common import utc_now_offset_aware, \
     session_wizard_all_form_data
 from coldfront.core.utils.mail import send_email_template
 
+
 logger = logging.getLogger(__name__)
 
 
@@ -875,6 +877,7 @@ def get_context_data(self, form, **kwargs):
         groups_path, scratch_path = get_default_secure_dir_paths()
         context['groups_path'] = groups_path
         context['scratch_path'] = scratch_path
+        context['directory_name_prefix'] = SECURE_DIRECTORY_NAME_PREFIX
 
         return context