Fixes based on the code review (redmine-58907)

Author: Yusaku-Kitabatake

admin/templates/loa/list.html

@@ -89,21 +89,9 @@ <h2>{% trans "Level of Assurance" %}</h2>
 {% endblock content %}
 
 {% block bottom_js %}
-    <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.2.1/jquery.min.js"></script>
-    <script>window.jQuery || document.write('<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.2.1/jquery.min.js">\x3C/script>')</script>
     <script type="application/javascript">
-        function escapeHtml(unsafe) {
-            return unsafe
-                .replace(/&/g, "&amp;")
-                .replace(/</g, "&lt;")
-                .replace(/>/g, "&gt;")
-                .replace(/"/g, "&quot;")
-                .replace(/'/g, "&#039;");
-        }
-
         document.getElementById("institution_id").addEventListener("change", function change_institution() {
             window.location = window.location.origin + "{% url 'loa:list' %}" + '?institution_id=' + document.getElementById("institution_id").value;
         });
-
     </script>
 {% endblock %}

api/institutions/authentication.py

@@ -255,7 +255,17 @@ def get_next(obj, *args):
         if loa:
             if loa.aal == 2:
                 if not re.search(OSF_AAL2_STR, str(aal)):
-                    mfa_url = mfa_url_tmp
+                    if mfa_url_tmp:
+                        mfa_url = mfa_url_tmp
+                    else:
+                        # MFA URL cannot be generated (e.g. p_idp is not a string).
+                        # Without MFA redirect, allowing login would silently bypass
+                        # the AAL2 requirement — reject the login instead.
+                        message = (
+                            'Institution login failed: Does not meet the required AAL.'
+                            '<br />MFA redirect is not available for this institution.'
+                        )
+                        loa_flag = False
             elif loa.aal == 1:
                 if not aal:
                     message = (

api_tests/institutions/views/test_institution_auth.py

@@ -117,7 +117,7 @@ def test_new_user_created(self, app, url_auth_institution, institution):
 
         with capture_signals() as mock_signals:
             res = app.post(url_auth_institution, make_payload(institution, username))
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         assert mock_signals.signals_sent() == set([signals.user_confirmed])
 
         user = OSFUser.objects.filter(username=username).first()
@@ -134,7 +134,7 @@ def test_existing_user_found_but_not_affiliated(self, app, institution, url_auth
 
         with capture_signals() as mock_signals:
             res = app.post(url_auth_institution, make_payload(institution, username))
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         assert not mock_signals.signals_sent()
 
         user.reload()
@@ -150,7 +150,7 @@ def test_user_found_and_affiliated(self, app, institution, url_auth_institution)
 
         with capture_signals() as mock_signals:
             res = app.post(url_auth_institution, make_payload(institution, username))
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         assert not mock_signals.signals_sent()
 
         user.reload()
@@ -174,7 +174,7 @@ def test_new_user_names_guessed_if_not_provided(self, app, institution, url_auth
 
         username = 'user_created_with_fullname_only@osf.edu'
         res = app.post(url_auth_institution, make_payload(institution, username))
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username=username).first()
         assert user
         assert user.fullname == 'Fake User'
@@ -189,7 +189,7 @@ def test_new_user_names_used_when_provided(self, app, institution, url_auth_inst
             url_auth_institution,
             make_payload(institution, username, given_name='Foo', family_name='Bar')
         )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username=username).first()
         assert user
         assert user.fullname == 'Fake User'
@@ -216,7 +216,7 @@ def test_user_active(self, app, institution, url_auth_institution):
                     department='Fake Department',
                 )
             )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         assert not mock_signals.signals_sent()
 
         user = OSFUser.objects.filter(username=username).first()
@@ -255,7 +255,7 @@ def test_user_unclaimed(self, app, institution, url_auth_institution):
                     department='Fake Department',
                 )
             )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         assert mock_signals.signals_sent() == set([signals.user_confirmed])
 
         user = OSFUser.objects.filter(username=username).first()
@@ -290,7 +290,7 @@ def test_user_unconfirmed(self, app, institution, url_auth_institution):
                     fullname='Fake User'
                 )
             )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         assert mock_signals.signals_sent() == set([signals.user_confirmed])
 
         user = OSFUser.objects.filter(username=username).first()
@@ -412,7 +412,7 @@ def test_authenticate_jaSurname_and_jaGivenName_are_valid(
                          jaGivenName=jagivenname, jaSurname=jasurname),
             expect_errors=True
         )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username=username).first()
         assert user
         assert user.ext.data['idp_attr']['fullname_ja'] == jagivenname + ' ' + jasurname
@@ -426,7 +426,7 @@ def test_authenticate_jaGivenName_is_valid(
             make_payload(institution, username, jaGivenName=jagivenname),
             expect_errors=True
         )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username=username).first()
         assert user
         assert user.given_name_ja == jagivenname
@@ -440,7 +440,7 @@ def test_authenticate_jaSurname_is_valid(
             make_payload(institution, username, jaSurname=jasurname),
             expect_errors=True
         )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username=username).first()
         assert user
         assert user.family_name_ja == jasurname
@@ -454,7 +454,7 @@ def test_authenticate_jaMiddleNames_is_valid(
             make_payload(institution, username, jaMiddleNames=middlename),
             expect_errors=True
         )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username=username).first()
         assert user
         assert user.middle_names_ja == middlename
@@ -468,7 +468,7 @@ def test_authenticate_givenname_is_valid(
             make_payload(institution, username, given_name=given_name),
             expect_errors=True
         )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username=username).first()
         assert user
         assert user.given_name == given_name
@@ -482,7 +482,7 @@ def test_authenticate_familyname_is_valid(
             make_payload(institution, username, family_name=family_name),
             expect_errors=True
         )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username=username).first()
         assert user
         assert user.family_name == family_name
@@ -496,7 +496,7 @@ def test_authenticate_middlename_is_valid(
             make_payload(institution, username, middle_names=middle_names),
             expect_errors=True
         )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username=username).first()
         assert user
         assert user.middle_names == middle_names
@@ -515,7 +515,7 @@ def test_authenticate_jaOrganizationalUnitName_is_valid(
                          organizationName=organizationname),
             expect_errors=True
         )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username='tmp_eppn_' + username).first()
         assert user
         assert user.jobs[0]['department_ja'] == jaorganizationname
@@ -534,11 +534,37 @@ def test_authenticate_OrganizationalUnitName_is_valid(
                          organizationName=organizationname),
             expect_errors=True
         )
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username='tmp_eppn_' + username).first()
         assert user
         assert user.jobs[0]['department'] == organizationnameunit
 
+    def test_authenticate__check_user_extended_data(self, app, institution, url_auth_institution):
+        username, fullname, password = 'user_active@user.edu', 'Foo Bar', 'FuAsKeEr'
+        user = make_user(username, fullname)
+        user.set_password(password)
+        user.save()
+
+        with capture_signals() as mock_signals:
+            res = app.post(
+                url_auth_institution,
+                make_payload(
+                    institution,
+                    username,
+                    family_name='User',
+                    given_name='Fake',
+                    fullname='Fake User',
+                    department='Fake Department',
+                    login_availability='can login',
+                )
+            )
+        assert res.status_code == 200
+        assert not mock_signals.signals_sent()
+
+        # confirm login availability extended data
+        extended_data = UserExtendedData.objects.get(user=user)
+        assert extended_data.data.get('idp_attr', {}).get('login_availability') == 'can login'
+
     @mock.patch('api.institutions.authentication.login_by_eppn')
     def test_with_new_attribute(self, mock, app, institution, url_auth_institution):
         mock.return_value = True
@@ -569,7 +595,7 @@ def test_with_new_attribute(self, mock, app, institution, url_auth_institution):
                 gakunin_identity_assurance_method_reference=gakunin_identity_assurance_method_reference,)
         )
 
-        assert res.status_code == 204 or res.status_code == 200
+        assert res.status_code == 200
         user = OSFUser.objects.filter(username='tmp_eppn_' + username).first()
         assert user