Repository: openwisp/openwisp-radius (language: python)

Issue:
[bug/regression] Signing-in with SAML changes user's username
If a user signs up with email or phone number and then tries to log-in with SAML, then the user's username will get updated if the email address of the user is same in both IdP and OpenWISP. This is because, `djangosaml2` uses email addresses 

We should prevent username field to get changed in this scenario. 


[bug/regression] Signing-in with SAML changes user's username
If a user signs up with email or phone number and then tries to log-in with SAML, then the user's username will get updated if the email address of the user is same in both IdP and OpenWISP. This is because, `djangosaml2` uses email addresses 

We should prevent username field to get changed in this scenario. 




Discussion / hints:

Fix patch:
diff --git a/docs/source/developer/how_to_extend.rst b/docs/source/developer/how_to_extend.rst
index e4538a26..50b74d79 100644
--- a/docs/source/developer/how_to_extend.rst
+++ b/docs/source/developer/how_to_extend.rst
@@ -86,7 +86,7 @@ ensuring also that ``openwisp_radius`` has been removed:
 
     AUTHENTICATION_BACKENDS = (
         'openwisp_users.backends.UsersAuthenticationBackend',
-        'djangosaml2.backends.Saml2Backend', # optional, can be removed if SAML login is not needed
+        'openwisp_radius.saml.backends.OpenwispRadiusSaml2Backend', # optional, can be removed if SAML login is not needed
     )
 
 .. important::
diff --git a/docs/source/user/saml.rst b/docs/source/user/saml.rst
index c7abf6e5..f18ac5a8 100644
--- a/docs/source/user/saml.rst
+++ b/docs/source/user/saml.rst
@@ -76,7 +76,7 @@ Ensure your ``settings.py`` looks like the following:
     # Update AUTHENTICATION_BACKENDS
     AUTHENTICATION_BACKENDS = (
         'openwisp_users.backends.UsersAuthenticationBackend',
-        'djangosaml2.backends.Saml2Backend', # <- add for SAML login
+        'openwisp_radius.saml.backends.OpenwispRadiusSaml2Backend', # <- add for SAML login
     )
 
     # Update MIDDLEWARE
@@ -142,3 +142,27 @@ Settings
 --------
 
 See :ref:`SAML related settings <saml_settings>`.
+
+FAQs
+----
+
+.. _preventing_change_in_username_of_registered_user:
+
+Preventing change in username of a registered user
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The ``djangosaml2`` library requires configuring ``SAML_DJANGO_USER_MAIN_ATTRIBUTE``
+setting which serves as the primary lookup value for User objects.
+Whenever a user logs in or registers through the SAML method,
+a database query is made to check whether such a user already exists.
+This lookup is done using the value of ``SAML_DJANGO_USER_MAIN_ATTRIBUTE`` setting.
+If a match is found, the details of the user are updated with the
+information received from SAML Identity Provider.
+
+If a user (who has registered on OpenWISP with a different method from SAML)
+logs into OpenWISP with SAML, then the default behaviour of OpenWISP RADIUS
+prevents updating username of this user. Because, this operation could
+render the user's old credentials useless. If you want to update the username
+in such scenarios with details received from Identity Provider, set
+:ref:`OPENWISP_RADIUS_SAML_UPDATES_PRE_EXISTING_USERNAME <openwisp_radius_saml_updates_pre_existing_username>`
+to ``True``.
diff --git a/docs/source/user/settings.rst b/docs/source/user/settings.rst
index 28cb9656..b770acd9 100644
--- a/docs/source/user/settings.rst
+++ b/docs/source/user/settings.rst
@@ -776,3 +776,15 @@ created during SAML sign-in as verified users (``RegisteredUser.is_verified=True
 
 This is useful when SAML identity providers can be trusted
 to be legally valid identity verifiers.
+
+.. _openwisp_radius_saml_updates_pre_existing_username:
+
+``OPENWISP_RADIUS_SAML_UPDATES_PRE_EXISTING_USERNAME``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+**Default**: ``False``
+
+Allows updating username of a registered user with the value
+received from SAML Identity Provider. Read the
+:ref:`FAQs in SAML integration documentation <preventing_change_in_username_of_registered_user>`
+for details.
diff --git a/openwisp_radius/saml/backends.py b/openwisp_radius/saml/backends.py
new file mode 100644
index 00000000..bc0d3d87
--- /dev/null
+++ b/openwisp_radius/saml/backends.py
@@ -0,0 +1,31 @@
+from django.core.exceptions import ObjectDoesNotExist
+from djangosaml2.backends import Saml2Backend
+
+from .. import settings as app_settings
+
+
+class OpenwispRadiusSaml2Backend(Saml2Backend):
+    def _update_user(self, user, attributes, attribute_mapping, force_save=False):
+        if (
+            not app_settings.SAML_UPDATES_PRE_EXISTING_USERNAME
+            and not user._state.adding
+        ):
+            # Skip updating user's username if the user didn't signed up
+            # with SAML registration method.
+            try:
+                attribute_mapping = attribute_mapping.copy()
+                if user.registered_user.method != 'saml':
+                    for key, value in attribute_mapping.items():
+                        if 'username' in value:
+                            break
+                    if len(value) == 1:
+                        attribute_mapping.pop(key, None)
+                    else:
+                        attribute_mapping[key] = []
+                        for attr in value:
+                            if attr != 'username':
+                                attribute_mapping[key].append(attr)
+
+            except ObjectDoesNotExist:
+                pass
+        return super()._update_user(user, attributes, attribute_mapping, force_save)
diff --git a/openwisp_radius/settings.py b/openwisp_radius/settings.py
index bd897659..f0479ec3 100644
--- a/openwisp_radius/settings.py
+++ b/openwisp_radius/settings.py
@@ -50,6 +50,9 @@ def get_settings_value(option, default):
     'SAML_REGISTRATION_METHOD_LABEL', _('Single Sign-On (SAML)')
 )
 SAML_IS_VERIFIED = get_settings_value('SAML_IS_VERIFIED', False)
+SAML_UPDATES_PRE_EXISTING_USERNAME = get_settings_value(
+    'SAML_UPDATES_PRE_EXISTING_USERNAME', False
+)
 DISPOSABLE_RADIUS_USER_TOKEN = get_settings_value('DISPOSABLE_RADIUS_USER_TOKEN', True)
 API_ACCOUNTING_AUTO_GROUP = get_settings_value('API_ACCOUNTING_AUTO_GROUP', True)
 FREERADIUS_ALLOWED_HOSTS = get_settings_value('FREERADIUS_ALLOWED_HOSTS', [])


Test patch:
diff --git a/openwisp_radius/tests/test_saml/test_views.py b/openwisp_radius/tests/test_saml/test_views.py
index a6b36243..32a56a02 100644
--- a/openwisp_radius/tests/test_saml/test_views.py
+++ b/openwisp_radius/tests/test_saml/test_views.py
@@ -19,6 +19,7 @@
 OrganizationUser = swapper.load_model('openwisp_users', 'OrganizationUser')
 Organization = swapper.load_model('openwisp_users', 'Organization')
 RadiusToken = swapper.load_model('openwisp_radius', 'RadiusToken')
+RegisteredUser = swapper.load_model('openwisp_radius', 'RegisteredUser')
 OrganizationRadiusSettings = swapper.load_model(
     'openwisp_radius', 'OrganizationRadiusSettings'
 )
@@ -38,8 +39,9 @@
         idp_hosts=['idp.example.com'],
         metadata_file=METADATA_PATH,
     ),
-    SAML_ATTRIBUTE_MAPPING={'uid': ('username',)},
+    SAML_ATTRIBUTE_MAPPING={'uid': ('email', 'username')},
     SAML_USE_NAME_ID_AS_USERNAME=False,
+    SAML_DJANGO_USER_MAIN_ATTRIBUTE='email',
 )
 @patch('openwisp_radius.settings.SAML_REGISTRATION_ENABLED', True)
 class TestAssertionConsumerServiceView(TestOrganizationMixin, TestSamlMixins, TestCase):
@@ -53,18 +55,18 @@ def _get_saml_response_for_acs_view(self, relay_state):
         saml2_req = saml2_from_httpredirect_request(response.url)
         session_id = get_session_id_from_saml2(saml2_req)
         self.add_outstanding_query(session_id, relay_state)
-        return auth_response(session_id, 'org_user'), relay_state
+        return auth_response(session_id, 'org_user@example.com'), relay_state
 
     def _post_successful_auth_assertions(self, query_params, org_slug):
         self.assertEqual(User.objects.count(), 1)
         user_id = self.client.session[SESSION_KEY]
         user = User.objects.get(id=user_id)
-        self.assertEqual(user.username, 'org_user')
+        self.assertEqual(user.username, 'org_user@example.com')
         self.assertEqual(OrganizationUser.objects.count(), 1)
         org_user = OrganizationUser.objects.get(user_id=user_id)
         self.assertEqual(org_user.organization.slug, org_slug)
         expected_query_params = {
-            'username': ['org_user'],
+            'username': ['org_user@example.com'],
             'token': [Token.objects.get(user_id=user_id).key],
             'login_method': ['saml'],
         }
@@ -110,6 +112,50 @@ def test_relay_state_relative_path(self):
         query_params = parse_qs(urlparse(response.url).query)
         self._post_successful_auth_assertions(query_params, org_slug)
 
+    @capture_any_output()
+    def test_user_registered_with_non_saml_method(self):
+        user = self._create_user(username='test-user', email='org_user@example.com')
+        RegisteredUser.objects.create(user=user, method='manual')
+        relay_state = self._get_relay_state(
+            redirect_url='https://captive-portal.example.com', org_slug='default'
+        )
+
+        with self.subTest('Test username remains unchanged'):
+            with patch(
+                'openwisp_radius.settings.SAML_UPDATES_PRE_EXISTING_USERNAME', False
+            ):
+                saml_response, relay_state = self._get_saml_response_for_acs_view(
+                    relay_state
+                )
+                response = self.client.post(
+                    reverse('radius:saml2_acs'),
+                    {
+                        'SAMLResponse': self.b64_for_post(saml_response),
+                        'RelayState': relay_state,
+                    },
+                )
+                self.assertEqual(response.status_code, 302)
+                user.refresh_from_db()
+                self.assertEqual(user.username, 'test-user')
+
+        with self.subTest('Test username gets updated'):
+            with patch(
+                'openwisp_radius.settings.SAML_UPDATES_PRE_EXISTING_USERNAME', True
+            ):
+                saml_response, relay_state = self._get_saml_response_for_acs_view(
+                    relay_state
+                )
+                response = self.client.post(
+                    reverse('radius:saml2_acs'),
+                    {
+                        'SAMLResponse': self.b64_for_post(saml_response),
+                        'RelayState': relay_state,
+                    },
+                )
+                self.assertEqual(response.status_code, 302)
+                user.refresh_from_db()
+                self.assertEqual(user.username, 'org_user@example.com')
+
 
 @override_settings(
     SAML_CONFIG=conf.create_conf(
diff --git a/tests/openwisp2/settings.py b/tests/openwisp2/settings.py
index 87ece92f..5f750300 100644
--- a/tests/openwisp2/settings.py
+++ b/tests/openwisp2/settings.py
@@ -56,7 +56,7 @@
 
 AUTHENTICATION_BACKENDS = (
     'openwisp_users.backends.UsersAuthenticationBackend',
-    'djangosaml2.backends.Saml2Backend',
+    'openwisp_radius.saml.backends.OpenwispRadiusSaml2Backend',
     'sesame.backends.ModelBackend',
 )