Merge pull request #37 from geoadmin/feat-PB-1109-update-user-endpoint

PB-1109: Add PUT endpoint to update user

app/access/api.py

@@ -1,5 +1,8 @@
+from http import HTTPStatus
+
 from cognito.utils.user import create_cognito_user
 from cognito.utils.user import delete_cognito_user
+from cognito.utils.user import update_cognito_user
 from ninja import Router
 from ninja.errors import HttpError
 from provider.models import Provider
@@ -112,3 +115,34 @@ def delete(request: HttpRequest, username: str) -> HttpResponse:
             raise HttpError(500, "Internal Server Error")
         user_to_delete.delete()
         return HttpResponse(status=204)
+
+
+@router.put("users/{username}", auth=PermissionAuth('access.change_user'))
+def update_user(
+    request: HttpRequest, username: str, user_in: UserSchema
+) -> HttpResponse | UserSchema:
+    """Update the given user with the given user data and return it.
+
+    Return HTTP status code
+    - 200 (OK) if the user has been updated successfully
+    - 400 (Bad Request) if there is no provider with the given ID
+    - 404 (Not Found) if there is no user with the given username
+    - 500 (Internal Server Error) if there is an inconsistency with Cognito
+    - 503 (Service Unavailable) if Cognito cannot be reached
+    """
+    with transaction.atomic():
+        user_object = User.objects.select_for_update().filter(username=username).first()
+        if not user_object:
+            raise Http404()
+
+        if not Provider.objects.filter(id=user_in.provider_id).exists():
+            raise HttpError(HTTPStatus.BAD_REQUEST, "Provider does not exist")
+
+        for attr, value in user_in.dict(exclude_unset=True).items():
+            setattr(user_object, attr, value)
+        user_object.save()
+
+        updated = update_cognito_user(user_object)
+        if not updated:
+            raise HttpError(HTTPStatus.INTERNAL_SERVER_ERROR, "Internal Server Error")
+        return user_to_response(user_object)

app/access/tests/test_api.py

@@ -413,3 +413,140 @@ def test_delete_user_returns_403_if_no_permission(self, delete_cognito_user):
         assert response.json() == {"code": 403, "description": "Forbidden"}
         assert User.objects.count() == 1
         assert not delete_cognito_user.called
+
+    def test_update_user_returns_401_if_not_logged_in(self):
+        response = self.client.put("/api/users/dude")
+
+        assert response.status_code == 401
+        assert response.json() == {"code": 401, "description": "Unauthorized"}
+
+    def test_update_user_returns_403_if_no_permission(self):
+        create_user_with_permissions('test', 'test', [])
+        self.client.login(username='test', password='test')
+
+        response = self.client.put("/api/users/dude")
+
+        assert response.status_code == 403
+        assert response.json() == {"code": 403, "description": "Forbidden"}
+
+    @patch('access.api.update_cognito_user')
+    def test_update_user_updates_existing_user_as_expected(self, update_cognito_user):
+        update_cognito_user.return_value = True
+
+        create_user_with_permissions('test', 'test', [('access', 'user', 'change_user')])
+        self.client.login(username='test', password='test')
+
+        payload = {
+            "username": "dude",
+            "first_name": "Jeff",
+            "last_name": "Bridges",
+            "email": "[email protected]",
+            "provider_id": Provider.objects.last().id,
+        }
+
+        response = self.client.put("/api/users/dude", data=payload, content_type='application/json')
+
+        assert response.status_code == 200
+        assert response.json() == payload
+        user = User.objects.filter(username="dude").first()
+        for key, value in payload.items():
+            assert getattr(user, key) == value
+        assert update_cognito_user.called
+
+    def test_update_user_returns_404_and_leaves_user_as_is_if_user_nonexistent(self):
+
+        create_user_with_permissions('test', 'test', [('access', 'user', 'change_user')])
+        self.client.login(username='test', password='test')
+
+        user_before = User.objects.filter(username="dude").first()
+        payload = {
+            "username": "dude",
+            "first_name": "Jeff",
+            "last_name": "Bridges",
+            "email": "[email protected]",
+            "provider_id": Provider.objects.last().id,
+        }
+
+        nonexistent_username = "maude"
+        response = self.client.put(
+            f"/api/users/{nonexistent_username}", data=payload, content_type='application/json'
+        )
+
+        assert response.status_code == 404
+        assert response.json() == {"code": 404, "description": "Resource not found"}
+        user_after = User.objects.filter(username="dude").first()
+        assert user_after == user_before
+
+    def test_update_user_returns_400_and_leaves_user_as_is_if_provider_nonexistent(self):
+
+        create_user_with_permissions('test', 'test', [('access', 'user', 'change_user')])
+        self.client.login(username='test', password='test')
+
+        user_before = User.objects.filter(username="dude").first()
+        nonexistent_id = Provider.objects.last().id + 1234
+        payload = {
+            "username": "dude",
+            "first_name": "Jeff",
+            "last_name": "Bridges",
+            "email": "[email protected]",
+            "provider_id": nonexistent_id,
+        }
+
+        response = self.client.put("/api/users/dude", data=payload, content_type="application/json")
+
+        assert response.status_code == 400
+        assert response.json() == {"code": 400, "description": "Provider does not exist"}
+        user_after = User.objects.filter(username="dude").first()
+        assert user_after == user_before
+
+    @patch('access.api.update_cognito_user')
+    def test_update_user_returns_500_and_leaves_user_as_is_if_cognito_inconsistent(
+        self, update_cognito_user
+    ):
+        update_cognito_user.return_value = False
+
+        create_user_with_permissions('test', 'test', [('access', 'user', 'change_user')])
+        self.client.login(username='test', password='test')
+
+        user_before = User.objects.filter(username="dude").first()
+        payload = {
+            "username": "dude",
+            "first_name": "Jeff",
+            "last_name": "Bridges",
+            "email": "[email protected]",
+            "provider_id": Provider.objects.last().id,
+        }
+
+        response = self.client.put("/api/users/dude", data=payload, content_type="application/json")
+
+        assert response.status_code == 500
+        assert response.json() == {"code": 500, "description": "Internal Server Error"}
+        user_after = User.objects.filter(username="dude").first()
+        assert user_after == user_before
+        assert update_cognito_user.called
+
+    @patch('access.api.update_cognito_user')
+    def test_update_user_returns_503_and_leaves_user_as_is_if_cognito_down(
+        self, update_cognito_user
+    ):
+        update_cognito_user.side_effect = EndpointConnectionError(endpoint_url='http://localhost')
+
+        create_user_with_permissions('test', 'test', [('access', 'user', 'change_user')])
+        self.client.login(username='test', password='test')
+
+        user_before = User.objects.filter(username="dude").first()
+        payload = {
+            "username": "dude",
+            "first_name": "Jeff",
+            "last_name": "Bridges",
+            "email": "[email protected]",
+            "provider_id": Provider.objects.last().id,
+        }
+
+        response = self.client.put("/api/users/dude", data=payload, content_type="application/json")
+
+        assert response.status_code == 503
+        assert response.json() == {"code": 503, "description": "Service Unavailable"}
+        user_after = User.objects.filter(username="dude").first()
+        assert user_after == user_before
+        assert update_cognito_user.called

app/cognito/tests/test_user_utils.py

@@ -3,7 +3,7 @@
 
 from cognito.utils.user import create_cognito_user
 from cognito.utils.user import delete_cognito_user
-from cognito.utils.user import update_user
+from cognito.utils.user import update_cognito_user
 
 from django.test import TestCase
 
@@ -19,7 +19,7 @@ class ClientTestCase(TestCase):
 
     @patch('cognito.utils.user.Client')
     @patch('cognito.utils.user.logger')
-    def test_create_user_creates_user(self, logger, client):
+    def test_create_cognito_user_creates_user(self, logger, client):
         client.return_value.create_user.return_value = True
 
         created = create_cognito_user(DummyUser('123', '[email protected]'))
@@ -29,7 +29,7 @@ def test_create_user_creates_user(self, logger, client):
 
     @patch('cognito.utils.user.Client')
     @patch('cognito.utils.user.logger')
-    def test_create_user_does_not_create_existing_user(self, logger, client):
+    def test_create_cognito_user_does_not_create_existing_user(self, logger, client):
         client.return_value.create_user.return_value = False
 
         created = create_cognito_user(DummyUser('123', '[email protected]'))
@@ -41,7 +41,7 @@ def test_create_user_does_not_create_existing_user(self, logger, client):
 
     @patch('cognito.utils.user.Client')
     @patch('cognito.utils.user.logger')
-    def test_delete_user_deletes_user(self, logger, client):
+    def test_delete_cognito_user_deletes_user(self, logger, client):
         client.return_value.delete_user.return_value = True
 
         deleted = delete_cognito_user(DummyUser('123', '[email protected]'))
@@ -51,7 +51,7 @@ def test_delete_user_deletes_user(self, logger, client):
 
     @patch('cognito.utils.user.Client')
     @patch('cognito.utils.user.logger')
-    def test_delete_user_does_not_delete_nonexisting_user(self, logger, client):
+    def test_delete_cognito_user_does_not_delete_nonexisting_user(self, logger, client):
         client.return_value.delete_user.return_value = False
 
         deleted = delete_cognito_user(DummyUser('123', '[email protected]'))
@@ -63,20 +63,22 @@ def test_delete_user_does_not_delete_nonexisting_user(self, logger, client):
 
     @patch('cognito.utils.user.Client')
     @patch('cognito.utils.user.logger')
-    def test_update_user_updates_user(self, logger, client):
+    def test_update_cognito_user_updates_user(self, logger, client):
         client.return_value.update_user.return_value = True
 
-        updated = update_user(DummyUser('123', '[email protected]'))
+        updated = update_cognito_user(DummyUser('123', '[email protected]'))
 
         self.assertEqual(updated, True)
         self.assertIn(call.info('User %s updated', '123'), logger.mock_calls)
 
     @patch('cognito.utils.user.Client')
     @patch('cognito.utils.user.logger')
-    def test_update_user_does_not_update_nonexisting_user(self, logger, client):
+    def test_update_cognito_user_does_not_update_nonexisting_user(self, logger, client):
         client.return_value.update_user.return_value = False
 
-        updated = update_user(DummyUser('123', '[email protected]'))
+        updated = update_cognito_user(DummyUser('123', '[email protected]'))
 
         self.assertEqual(updated, False)
-        self.assertIn(call.warning('User %s does not exist, not updated', '123'), logger.mock_calls)
+        self.assertIn(
+            call.critical('User %s does not exist, not updated', '123'), logger.mock_calls
+        )

app/cognito/utils/user.py

@@ -37,7 +37,7 @@ def delete_cognito_user(user: User) -> bool:
     return deleted
 
 
-def update_user(user: User) -> bool:
+def update_cognito_user(user: User) -> bool:
     """ Update the given user in cognito.
 
     Returns True, if the user has been updated.
@@ -48,5 +48,5 @@ def update_user(user: User) -> bool:
     if updated:
         logger.info("User %s updated", user.username)
     else:
-        logger.warning("User %s does not exist, not updated", user.username)
+        logger.critical("User %s does not exist, not updated", user.username)
     return updated