addressing comments #2

ocadotechnology · Jun 27, 2023 · ab7aad0 · ab7aad0
1 parent 7ef5636
commit ab7aad0
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 56 deletions.
diff --git a/portal/helpers/password.py b/portal/helpers/password.py
@@ -21,7 +21,7 @@ def is_password_pwned(password):
     response = requests.get(url)
 
     if response.status_code != 200:
-        return True  # backend ignore this and frontend tells the user
+        return False  # backend ignore this and frontend tells the user
         # that we cannot verify this at the moment
 
     # Check if the password's hash is found in the response body
@@ -31,8 +31,8 @@ def is_password_pwned(password):
         # api response is using the format of suffix:count hence
         # we need to get rid of the ending count number
         if sha1_hash[5:].upper() == suffix[:35].upper():
-            return True
-    return False
+            return False
+    return True
 
 
 class PasswordStrength(Enum):
@@ -56,7 +56,7 @@ def password_test(self, password):
                 raise forms.ValidationError(
                     f"Password not strong enough, consider using at least {minimum_password_length} characters and making it hard to guess."
                 )
-            if is_password_pwned(password):
+            if not is_password_pwned(password):
                 raise forms.ValidationError("Password is too common, consider using a different password.")
 
         elif self is PasswordStrength.INDEPENDENT:
@@ -73,7 +73,7 @@ def password_test(self, password):
                     f"Password not strong enough, consider using at least {minimum_password_length} characters, "
                     "upper and lower case letters, and numbers and making it hard to guess."
                 )
-            if is_password_pwned(password):
+            if not is_password_pwned(password):
                 raise forms.ValidationError("Password is too common, consider using a different password.")
         else:
             minimum_password_length = 10
@@ -89,7 +89,7 @@ def password_test(self, password):
                     f"Password not strong enough, consider using at least {minimum_password_length} characters, "
                     "upper and lower case letters, numbers, special characters and making it hard to guess."
                 )
-            if is_password_pwned(password):
+            if not is_password_pwned(password):
                 raise forms.ValidationError("Password is too common, consider using a different password.")
 
         return password

diff --git a/portal/static/portal/js/passwordStrength.js b/portal/static/portal/js/passwordStrength.js
@@ -6,43 +6,49 @@ let password_strengths = [
 ];
 
 async function handlePasswordStrength() {
-  const tPwd = $('#id_teacher_signup-teacher_password').val();
-  const sPwd = $('#id_independent_student_signup-password').val();
+  const teacherPwd = $('#id_teacher_signup-teacher_password').val();
+  const studentPwd = $('#id_independent_student_signup-password').val();
 
-  const isTPwdEmpty = tPwd.length > 0;
-  const isSPwdEmpty = sPwd.length > 0;
+  const isTeacherPwdTyped = teacherPwd.length > 0;
+  const isStudentPwdTyped = studentPwd.length > 0;
 
-  const isTPwdStrong = isTPwdEmpty && isPasswordStrong(tPwd, true);
-  const isSPwdStrong = isSPwdEmpty && isPasswordStrong(sPwd, false);
+  const isTeacherPwdStrong =
+    isTeacherPwdTyped && isPasswordStrong(teacherPwd, true);
+  const isStudentPwdStrong =
+    isStudentPwdTyped && isPasswordStrong(studentPwd, false);
 
-  const isTPwdSafe = isTPwdStrong && (await getPwnedStatus(tPwd));
-  const isSPwdSafe = isSPwdStrong && (await getPwnedStatus(sPwd));
+  const isTeacherPwdSafe =
+    isTeacherPwdStrong && (await !isPasswordPwned(teacherPwd));
+  const isStudentPwdSafe =
+    isStudentPwdStrong && (await !isPasswordPwned(studentPwd));
 
-  const tPwdStr = [isTPwdEmpty, isTPwdStrong, isTPwdSafe].filter(
-    Boolean
-  ).length;
-  const sPwdStr = [isSPwdEmpty, isSPwdStrong, isSPwdSafe].filter(
-    Boolean
-  ).length;
+  const teacherPwdStr = [
+    isTeacherPwdTyped,
+    isTeacherPwdStrong,
+    isTeacherPwdSafe
+  ].filter(Boolean).length;
+  const studentPwdStrength = [
+    isStudentPwdTyped,
+    isStudentPwdStrong,
+    isStudentPwdSafe
+  ].filter(Boolean).length;
 
   $('#teacher-password-sign').css(
     'background-color',
-    password_strengths[tPwdStr].colour
+    password_strengths[teacherPwdStr].colour
   );
-  $('#teacher-password-text').html(password_strengths[tPwdStr].name);
+  $('#teacher-password-text').html(password_strengths[teacherPwdStr].name);
   $('#student-password-sign').css(
     'background-color',
-    password_strengths[sPwdStr].colour
+    password_strengths[studentPwdStrength].colour
   );
-  $('#student-password-text').html(password_strengths[sPwdStr].name);
+  $('#student-password-text').html(password_strengths[studentPwdStrength].name);
 }
 
-const getPwnedStatus = async (password) => {
+const isPasswordPwned = async (password) => {
   const computeSHA1Hash = (password) =>
     CryptoJS.SHA1(password).toString().toUpperCase();
 
-  const doesSuffixExist = (data, suffix) => data.includes(suffix);
-
   try {
     const hashedPassword = computeSHA1Hash(password);
     const prefix = hashedPassword.substring(0, 5);
@@ -52,19 +58,19 @@ const getPwnedStatus = async (password) => {
     const response = await fetch(apiUrl);
 
     if (!response.ok) {
-      return true; // ignore the check if the server is down as the popup warns
+      return false; // ignore the check if the server is down as the popup warns
       // the user that we cannot check the password
     }
 
     const data = await response.text();
 
-    if (doesSuffixExist(data, suffix)) {
-      return false;
+    if (data.includes(suffix)) {
+      return true;
     }
-    return true;
+    return false;
   } catch (error) {
     console.error(`Request failed with error: ${error.message}`);
-    return false;
+    return true;
   }
 };
 
@@ -90,3 +96,23 @@ function isPasswordStrong(password, isTeacher) {
     );
   }
 }
+
+async function isPwnedPasswordApiAvailable(url) {
+  try {
+    const response = await fetch(url, { metheod: 'GET' });
+    return response.ok;
+  } catch (error) {
+    console.error(error);
+    return false;
+  }
+}
+async function handlePwnedPasswordApiAvailability() {
+  const url = 'https://api.pwnedpasswords.com/range/00000';
+  const isAvailable = await isPwnedPasswordApiAvailable(url);
+  const errorTitle = 'Password Vulnerability Check Unavailable';
+  const errorMessage =
+    'We are currently unable to check your password vulnerability. Please ensure that you are using a strong password. If you are happy to continue, please confirm.';
+  if (!isAvailable) {
+    showServiceUnavailable(errorTitle, errorMessage);
+  }
+}
diff --git a/portal/templates/portal/register.html b/portal/templates/portal/register.html
@@ -194,28 +194,9 @@ <h4>Independent learner</h4>
     <script>
     var TEACHER_PASSWORD_FIELD_ID = '{{ teacher_signup_form.teacher_password.auto_id }}';
     var INDEP_STUDENT_PASSWORD_FIELD_ID = '{{ independent_student_signup_form.password.auto_id }}';
-    async function isPwnedPasswordApiAvailable(url) {
-        try {
-            const response = await fetch(url, {metheod: 'GET'})
-            return response.ok;
-        }
-        catch (error) {
-            console.error(error)
-            return false;
-        }
-    }
-    async function handlePwnedPasswordApiAvailability() {
-        const url = 'https://api.pwnedpasswords.com/range/00000';
-        const isAvailable = await isPwnedPasswordApiAvailable(url);
-        const errorTitle = 'Password Vulnerability Check Unavailable';
-        const errorMessage = "We are currently unable to check your password vulnerability. Please ensure that you are using a strong password.If you are happy to continue, please confirm.";
-        if (!isAvailable) {
-            showServiceUnavailable(errorTitle, errorMessage)
-        }
-    }
-    handlePwnedPasswordApiAvailability()
-    handlePasswordStrength(); // the password strength text is updated dynamically hence this is the initial first call
     $(document).ready(function() {
+    handlePasswordStrength(); // the password strength text is updated dynamically hence this is the initial first call
+    handlePwnedPasswordApiAvailability();
     $('#id_teacher_signup-teacher_password, #id_independent_student_signup-password').on('input change focus blur', handlePasswordStrength);
 
 });

diff --git a/portal/tests/test_helper_methods.py b/portal/tests/test_helper_methods.py
@@ -30,7 +30,7 @@ def test_is_password_pwned(mock_get):
 
     # Assert
     mock_get.assert_called_once_with(f"https://api.pwnedpasswords.com/range/{sha1_hash[:5]}")
-    assert result == True
+    assert result == False
 
 
 @patch("requests.get")
@@ -49,4 +49,4 @@ def test_is_password_pwned_status_code_not_200(mock_get):
 
     # Assert
     mock_get.assert_called_once_with(f"https://api.pwnedpasswords.com/range/{sha1_hash[:5]}")
-    assert result == True
+    assert result == False
diff --git a/portal/tests/test_teacher.py b/portal/tests/test_teacher.py
@@ -533,7 +533,6 @@ def test_edit_details_non_admin(self):
         self.selenium.get(self.live_server_url)
         page = HomePage(self.selenium).go_to_teacher_login_page().login(email_2, password_2).open_account_tab()
 
-        strong_password = "$RFV`bgt%6yhn"
         page = page.change_teacher_details(
             {"first_name": "Florian", "last_name": "Aucomte", "current_password": password_2}
         )