Repository: learningequality/kolibri
Issue: Same "sync" user credentials across facilities cause sync to fail

<!--
Instructions:
 * Fill out the sections below, replace …'s with information about your issue
 * Use the 'preview' function above this text box to verify formatting before submitting
-->

### Observed behavior
<!--
Description of the behavior that was observed, including screenshots or other references when applicable
-->

Have two facilities with the same admin credentials on a server. Was attempting to import facilities on two different devices using `kolibri manage sync...`

The `sync` was always failing on the 2nd device with this error -

`requests.exceptions.HTTPError: 403 Client Error: Forbidden for url: http://192.168.1.69:8080/api/morango/v1/certificates/`

After a lot of permutations and combinations realised that setting different passwords for each of the `admin` users allowed the sync to complete successfully on each device. 

### Expected behavior
<!--
Description of what behavior was expected but did not occur
-->

The sync should have worked even with the same user credentials across facilities since the sync command differentiates the users with a `--facility <facilityid>`

### User-facing consequences
<!--
Implications and real-world consequences for learners, coaches, admins, and other users of the application
-->

Confusion and anxiety as user has no way to know what is wrong and what is the cause of the error.

### Errors and logs
<!--
Relevant logs from:
 * the command line
 * ~/.kolibri/logs/kolibri.txt
 * the browser console

Please wrap errors in triple backticks for clean formatting like this:
```
01:10 info: something happened
01:12 error: something bad happened
```
-->

Error message following a large exception dump -

`requests.exceptions.HTTPError: 403 Client Error: Forbidden for url: http://192.168.1.69:8080/api/morango/v1/certificates/`

### Steps to reproduce
<!--
Precise steps that someone else can follow in order to see this behavior
-->

1. Create two facilities.
2. Add a admin user in each of the facilities with exactly the same username and password - admin / password.
3. On another device try importing the two facilities using `kolibri manage sync --baseurl <source device ip/domain> --facility <facilityid[1,2]> --username admin --password password --no-push --noninteractive` command.
4. The import shall most likely fail for the second attempt.
5. Now change password of the admin user of the facility for which the sync is failing.
6. Stop Kolibri, remove the `KOLIBRI_HOME` folder and start Kolibri.
6. Reattempt the sync for both the facilities.
7. Now the sync shall work for both facilities.

### Context
<!--
Tell us about your environment, including:
 * Kolibri version
 * Operating system
 * Browser
-->

Kolibi 0.14.3


Changed files:
kolibri/core/auth/management/commands/sync.py, kolibri/core/tasks/test/test_api.py, kolibri/core/tasks/api.py, kolibri/core/auth/test/test_morango_integration.py, kolibri/core/device/test/test_syncqueue.py, kolibri/core/auth/management/utils.py

diff --git a/kolibri/core/auth/management/commands/sync.py b/kolibri/core/auth/management/commands/sync.py
index 53f760c65d..7e7af09a54 100644
--- a/kolibri/core/auth/management/commands/sync.py
+++ b/kolibri/core/auth/management/commands/sync.py
@@ -6,7 +6,7 @@ from django.core.management.base import CommandError
 from morango.models import ScopeDefinition
 
 from ..utils import get_client_and_server_certs
-from ..utils import get_dataset_id
+from ..utils import get_facility_dataset_id
 from kolibri.core.auth.constants.morango_sync import DATA_PORTAL_SYNCING_BASE_URL
 from kolibri.core.auth.constants.morango_sync import ScopeDefinitions
 from kolibri.core.auth.management.utils import get_facility
@@ -101,7 +101,7 @@ class Command(MorangoSyncCommand):
             if not re.match("[a-f0-9]{32}", user_id):
                 raise CommandError("User ID must be a 32-character UUID (no dashes)")
 
-            dataset_id = get_dataset_id(
+            facility_id, dataset_id = get_facility_dataset_id(
                 baseurl, identifier=facility_id, noninteractive=True
             )
 
@@ -160,7 +160,7 @@ class Command(MorangoSyncCommand):
             )
 
         else:  # do P2P setup
-            dataset_id = get_dataset_id(
+            facility_id, dataset_id = get_facility_dataset_id(
                 baseurl, identifier=facility_id, noninteractive=noninteractive
             )
 
diff --git a/kolibri/core/auth/management/utils.py b/kolibri/core/auth/management/utils.py
index e3a634dc8c..dbb520dcd0 100644
--- a/kolibri/core/auth/management/utils.py
+++ b/kolibri/core/auth/management/utils.py
@@ -85,7 +85,7 @@ def _interactive_server_facility_selection(facilities):
         message += "{}. {}\\n".format(idx + 1, f["name"])
     idx = input(message)
     try:
-        return facilities[int(idx) - 1]["dataset"]
+        return facilities[int(idx) - 1]
     except IndexError:
         raise CommandError(
             (
@@ -130,7 +130,7 @@ def get_facility(facility_id=None, noninteractive=False):
     return facility
 
 
-def get_dataset_id(baseurl, identifier=None, noninteractive=False):
+def get_facility_dataset_id(baseurl, identifier=None, noninteractive=False):
     # get list of facilities and if more than 1, display all choices to user
     facility_url = urljoin(baseurl, reverse("kolibri:core:publicfacility-list"))
     response = requests.get(facility_url)
@@ -142,7 +142,7 @@ def get_dataset_id(baseurl, identifier=None, noninteractive=False):
     if identifier:
         for obj in facilities:
             if identifier == obj["dataset"] or identifier == obj.get("id"):
-                return obj["dataset"]
+                return identifier, obj["dataset"]
         raise CommandError(
             "Facility with ID {} does not exist on server".format(identifier)
         )
@@ -154,12 +154,13 @@ def get_dataset_id(baseurl, identifier=None, noninteractive=False):
                 "Please pass in a facility ID by passing in --facility {ID} after the command."
             )
         )
-    else:
-        return (
-            _interactive_server_facility_selection(facilities)
-            if len(facilities) > 1
-            else facilities[0]["dataset"]
-        )
+
+    facility = (
+        _interactive_server_facility_selection(facilities)
+        if len(facilities) > 1
+        else facilities[0]
+    )
+    return facility["id"], facility["dataset"]
 
 
 def is_portal_sync(baseurl):
@@ -274,7 +275,7 @@ def get_client_and_server_certs(
                 password = getpass.getpass("Please enter password: ")
 
         userargs = username
-        if user_id:
+        if facility_id:
             # add facility so `FacilityUserBackend` can validate
             userargs = {
                 FacilityUser.USERNAME_FIELD: username,
diff --git a/kolibri/core/auth/test/test_morango_integration.py b/kolibri/core/auth/test/test_morango_integration.py
index c5b9779efd..bad7f9a72a 100644
--- a/kolibri/core/auth/test/test_morango_integration.py
+++ b/kolibri/core/auth/test/test_morango_integration.py
@@ -6,6 +6,7 @@ import os
 import unittest
 import uuid
 
+import requests
 from django.core.management import call_command
 from django.test import TestCase
 from django.utils import timezone
@@ -81,6 +82,96 @@ class DateTimeTZFieldTestCase(TestCase):
     "This test will only be run during integration testing.",
 )
 class CertificateAuthenticationTestCase(TestCase):
+    @multiple_kolibri_servers(1)
+    def test_multi_facility_authentication(self, servers):
+        """
+        Multiple facilities with a superuser of the same username on each to verify that cert
+        generation properly passes along the facility for authenticating the user with the correct
+        facility
+        """
+        # START: setup server
+        server = servers[0]
+        server.manage("loaddata", "scopedefinitions")
+        server.manage("loaddata", "content_test")
+        server.manage(
+            "generateuserdata",
+            "--no-onboarding",
+            "--num-content-items",
+            "1",
+            "--facilities",
+            "2",
+        )
+
+        facility_1 = Facility.objects.using(server.db_alias).first()
+        facility_2 = Facility.objects.using(server.db_alias).last()
+        self.assertNotEqual(facility_1.id, facility_2.id)
+
+        superuser_1 = FacilityUser.objects.using(server.db_alias).get(
+            username="superuser", facility=facility_1
+        )
+        superuser_2 = FacilityUser.objects.using(server.db_alias).get(
+            username="superuser", facility=facility_2
+        )
+        self.assertNotEqual(superuser_1.id, superuser_2.id)
+
+        superuser_1.set_password("superuser_1")
+        superuser_1.save(using=server.db_alias)
+        superuser_2.set_password("superuser_2")
+        superuser_2.save(using=server.db_alias)
+        # END: setup server
+
+        # START: local setup
+        if not ScopeDefinition.objects.filter():
+            call_command("loaddata", "scopedefinitions")
+
+        controller = MorangoProfileController(PROFILE_FACILITY_DATA)
+        network_connection = controller.create_network_connection(server.baseurl)
+
+        # facility mismatch for superuser 1
+        with self.assertRaises(requests.exceptions.HTTPError):
+            get_client_and_server_certs(
+                superuser_1.username,
+                "superuser_1",
+                facility_1.dataset_id,
+                network_connection,
+                facility_id=facility_2.id,
+                noninteractive=True,
+            )
+        # facility mismatch for superuser 2
+        with self.assertRaises(requests.exceptions.HTTPError):
+            get_client_and_server_certs(
+                superuser_2.username,
+                "superuser_2",
+                facility_2.dataset_id,
+                network_connection,
+                facility_id=facility_1.id,
+                noninteractive=True,
+            )
+
+        client_cert, server_cert, username = get_client_and_server_certs(
+            superuser_1.username,
+            "superuser_1",
+            facility_1.dataset_id,
+            network_connection,
+            facility_id=facility_1.id,
+            noninteractive=True,
+        )
+        self.assertIsNotNone(client_cert)
+        self.assertIsNotNone(server_cert)
+        self.assertIsNotNone(username)
+
+        client_cert, server_cert, username = get_client_and_server_certs(
+            superuser_2.username,
+            "superuser_2",
+            facility_2.dataset_id,
+            network_connection,
+            facility_id=facility_2.id,
+            noninteractive=True,
+        )
+        self.assertIsNotNone(client_cert)
+        self.assertIsNotNone(server_cert)
+        self.assertIsNotNone(username)
+
     @multiple_kolibri_servers(1)
     def test_learner_passwordless_authentication(self, servers):
         # START: setup server
diff --git a/kolibri/core/device/test/test_syncqueue.py b/kolibri/core/device/test/test_syncqueue.py
index 60360ef160..5c6f604d88 100644
--- a/kolibri/core/device/test/test_syncqueue.py
+++ b/kolibri/core/device/test/test_syncqueue.py
@@ -99,10 +99,10 @@ class TestRequestSoUDSync(TestCase):
     @mock.patch("kolibri.core.public.utils.requests")
     @mock.patch("kolibri.core.tasks.api.MorangoProfileController")
     @mock.patch("kolibri.core.tasks.api.get_client_and_server_certs")
-    @mock.patch("kolibri.core.tasks.api.get_dataset_id")
+    @mock.patch("kolibri.core.tasks.api.get_facility_dataset_id")
     def test_request_soud_sync(
         self,
-        get_dataset_id,
+        get_facility_dataset_id,
         get_client_and_server_certs,
         MorangoProfileController,
         requests_mock,
@@ -110,7 +110,10 @@ class TestRequestSoUDSync(TestCase):
     ):
 
         get_client_and_server_certs.return_value = None
-        get_dataset_id.return_value = self.facility.dataset_id
+        get_facility_dataset_id.return_value = (
+            self.facility.id,
+            self.facility.dataset_id,
+        )
 
         requests_mock.post.return_value.status_code = 200
         requests_mock.post.return_value.json.return_value = {"action": SYNC}
@@ -135,10 +138,10 @@ class TestRequestSoUDSync(TestCase):
     @mock.patch("kolibri.core.public.utils.requests")
     @mock.patch("kolibri.core.tasks.api.MorangoProfileController")
     @mock.patch("kolibri.core.tasks.api.get_client_and_server_certs")
-    @mock.patch("kolibri.core.tasks.api.get_dataset_id")
+    @mock.patch("kolibri.core.tasks.api.get_facility_dataset_id")
     def test_request_soud_sync_server_error(
         self,
-        get_dataset_id,
+        get_facility_dataset_id,
         get_client_and_server_certs,
         MorangoProfileController,
         requests_mock,
@@ -146,7 +149,10 @@ class TestRequestSoUDSync(TestCase):
     ):
 
         get_client_and_server_certs.return_value = None
-        get_dataset_id.return_value = self.facility.dataset_id
+        get_facility_dataset_id.return_value = (
+            self.facility.id,
+            self.facility.dataset_id,
+        )
 
         requests_mock.post.return_value.status_code = 500
 
@@ -162,10 +168,10 @@ class TestRequestSoUDSync(TestCase):
     @mock.patch("kolibri.core.public.utils.requests")
     @mock.patch("kolibri.core.tasks.api.MorangoProfileController")
     @mock.patch("kolibri.core.tasks.api.get_client_and_server_certs")
-    @mock.patch("kolibri.core.tasks.api.get_dataset_id")
+    @mock.patch("kolibri.core.tasks.api.get_facility_dataset_id")
     def test_request_soud_sync_connection_error(
         self,
-        get_dataset_id,
+        get_facility_dataset_id,
         get_client_and_server_certs,
         MorangoProfileController,
         requests_mock,
@@ -173,7 +179,10 @@ class TestRequestSoUDSync(TestCase):
     ):
 
         get_client_and_server_certs.return_value = None
-        get_dataset_id.return_value = self.facility.dataset_id
+        get_facility_dataset_id.return_value = (
+            self.facility.id,
+            self.facility.dataset_id,
+        )
 
         requests_mock.post.side_effect = ConnectionError
 
diff --git a/kolibri/core/tasks/api.py b/kolibri/core/tasks/api.py
index 2600d62b53..83708074d7 100644
--- a/kolibri/core/tasks/api.py
+++ b/kolibri/core/tasks/api.py
@@ -36,7 +36,7 @@ from .permissions import FacilitySyncPermissions
 from kolibri.core.auth.constants.morango_sync import PROFILE_FACILITY_DATA
 from kolibri.core.auth.constants.morango_sync import State as FacilitySyncState
 from kolibri.core.auth.management.utils import get_client_and_server_certs
-from kolibri.core.auth.management.utils import get_dataset_id
+from kolibri.core.auth.management.utils import get_facility_dataset_id
 from kolibri.core.auth.models import Facility
 from kolibri.core.content.models import ChannelMetadata
 from kolibri.core.content.permissions import CanExportLogs
@@ -1233,7 +1233,7 @@ def validate_and_create_sync_credentials(
     # try to get the certificate, which will save it if successful
     try:
         # make sure we get the dataset ID
-        dataset_id = get_dataset_id(
+        facility_id, dataset_id = get_facility_dataset_id(
             baseurl, identifier=facility_id, noninteractive=True
         )
 
diff --git a/kolibri/core/tasks/test/test_api.py b/kolibri/core/tasks/test/test_api.py
index db109f7ff1..ef86e7b282 100644
--- a/kolibri/core/tasks/test/test_api.py
+++ b/kolibri/core/tasks/test/test_api.py
@@ -955,10 +955,10 @@ class FacilityTaskHelperTestCase(TestCase):
     @patch("kolibri.core.tasks.api.MorangoProfileController")
     @patch("kolibri.core.tasks.api.NetworkClient")
     @patch("kolibri.core.tasks.api.get_client_and_server_certs")
-    @patch("kolibri.core.tasks.api.get_dataset_id")
+    @patch("kolibri.core.tasks.api.get_facility_dataset_id")
     def test_validate_peer_sync_job(
         self,
-        get_dataset_id,
+        get_facility_dataset_id,
         get_client_and_server_certs,
         NetworkClient,
         MorangoProfileController,
@@ -981,7 +981,7 @@ class FacilityTaskHelperTestCase(TestCase):
         controller = MorangoProfileController.return_value
         controller.create_network_connection.return_value = network_connection
 
-        get_dataset_id.return_value = dataset_id
+        get_facility_dataset_id.return_value = (123, dataset_id)
         get_client_and_server_certs.return_value = None
 
         expected = dict(
@@ -1005,7 +1005,7 @@ class FacilityTaskHelperTestCase(TestCase):
             "https://some.server.test/"
         )
 
-        get_dataset_id.assert_called_with(
+        get_facility_dataset_id.assert_called_with(
             "https://some.server.test/", identifier=123, noninteractive=True
         )
 
@@ -1046,9 +1046,9 @@ class FacilityTaskHelperTestCase(TestCase):
 
     @patch("kolibri.core.tasks.api.MorangoProfileController")
     @patch("kolibri.core.tasks.api.NetworkClient")
-    @patch("kolibri.core.tasks.api.get_dataset_id")
+    @patch("kolibri.core.tasks.api.get_facility_dataset_id")
     def test_validate_and_create_sync_credentials__unknown_facility(
-        self, get_dataset_id, NetworkClient, MorangoProfileController
+        self, get_facility_dataset_id, NetworkClient, MorangoProfileController
     ):
         req = Mock(
             spec=Request,
@@ -1067,7 +1067,7 @@ class FacilityTaskHelperTestCase(TestCase):
         controller = MorangoProfileController.return_value
         controller.create_network_connection.return_value = network_connection
 
-        get_dataset_id.side_effect = CommandError()
+        get_facility_dataset_id.side_effect = CommandError()
 
         with self.assertRaises(AuthenticationFailed):
             validate_and_create_sync_credentials(*validate_peer_sync_job(req))
@@ -1075,10 +1075,10 @@ class FacilityTaskHelperTestCase(TestCase):
     @patch("kolibri.core.tasks.api.MorangoProfileController")
     @patch("kolibri.core.tasks.api.NetworkClient")
     @patch("kolibri.core.tasks.api.get_client_and_server_certs")
-    @patch("kolibri.core.tasks.api.get_dataset_id")
+    @patch("kolibri.core.tasks.api.get_facility_dataset_id")
     def test_validate_and_create_sync_credentials__not_authenticated(
         self,
-        get_dataset_id,
+        get_facility_dataset_id,
         get_client_and_server_certs,
         NetworkClient,
         MorangoProfileController,
@@ -1095,7 +1095,7 @@ class FacilityTaskHelperTestCase(TestCase):
         controller = MorangoProfileController.return_value
         controller.create_network_connection.return_value = network_connection
 
-        get_dataset_id.return_value = 456
+        get_facility_dataset_id.return_value = (123, 456)
         get_client_and_server_certs.side_effect = CommandError()
 
         with self.assertRaises(PermissionDenied):
@@ -1104,10 +1104,10 @@ class FacilityTaskHelperTestCase(TestCase):
     @patch("kolibri.core.tasks.api.MorangoProfileController")
     @patch("kolibri.core.tasks.api.NetworkClient")
     @patch("kolibri.core.tasks.api.get_client_and_server_certs")
-    @patch("kolibri.core.tasks.api.get_dataset_id")
+    @patch("kolibri.core.tasks.api.get_facility_dataset_id")
     def test_validate_and_create_sync_credentials__authentication_failed(
         self,
-        get_dataset_id,
+        get_facility_dataset_id,
         get_client_and_server_certs,
         NetworkClient,
         MorangoProfileController,
@@ -1129,7 +1129,7 @@ class FacilityTaskHelperTestCase(TestCase):
         controller = MorangoProfileController.return_value
         controller.create_network_connection.return_value = network_connection
 
-        get_dataset_id.return_value = 456
+        get_facility_dataset_id.return_value = (123, 456)
         get_client_and_server_certs.side_effect = CommandError()
 
         with self.assertRaises(AuthenticationFailed):