Split the update_user method to 3 methods (check, update, persist)

Author: brichet

jupyter_server/auth/identity.py

@@ -296,22 +296,30 @@ async def _get_user(self, handler: web.RequestHandler) -> User | None:
     def update_user(
         self, handler: web.RequestHandler, user_data: dict[UpdatableField, str]
     ) -> User:
-        """Update user information."""
+        """Update user information and persist the user model."""
+        self.check_update(user_data)
         current_user = t.cast(User, handler.current_user)
+        updated_user = self.update_user_model(current_user, user_data)
+        self.persist_user_model(handler)
+        return updated_user
 
+    def check_update(self, user_data: dict[UpdatableField, str]) -> None:
+        """Raises if some fields to update are not updatable."""
         for field in user_data:
             if field not in self.updatable_fields:
                 msg = f"Field {field} is not updatable"
                 raise ValueError(msg)
 
-        # Update fields
-        for field in self.updatable_fields:
-            if field in user_data:
-                setattr(current_user, field, user_data[field])
+    def update_user_model(self, current_user: User, user_data: dict[UpdatableField, str]) -> User:
+        """Update user information."""
+        raise NotImplementedError
 
-        # Persist changes (if applicable)
-        self.set_login_cookie(handler, current_user)  # Save updated user to cookie/session
-        return current_user
+    def persist_user_model(self, handler: web.RequestHandler) -> None:
+        """Persist the user model (i.e. a cookie).
+
+        This is a no-op for IdentityProvider.
+        """
+        raise NotImplementedError
 
     def identity_model(self, user: User) -> dict[str, t.Any]:
         """Return a User as an Identity model"""
@@ -681,6 +689,21 @@ def auth_enabled(self) -> bool:
         """Return whether any auth is enabled"""
         return bool(self.hashed_password or self.token)
 
+    def update_user_model(self, current_user: User, user_data: dict[UpdatableField, str]) -> User:
+        """Update user information."""
+        for field in self.updatable_fields:
+            if field in user_data:
+                setattr(current_user, field, user_data[field])
+        return current_user
+
+    def persist_user_model(self, handler: web.RequestHandler) -> None:
+        """Persist the user model to a cookie.
+
+        This is a no-op for PasswordIdentityProvider, but subclasses may
+        want to implement this.
+        """
+        self.set_login_cookie(handler, handler.current_user)
+
     def passwd_check(self, password):
         """Check password against our stored hashed password"""
         return passwd_check(self.hashed_password, password)

jupyter_server/services/api/handlers.py

@@ -11,7 +11,7 @@
 
 from jupyter_server._tz import isoformat, utcfromtimestamp
 from jupyter_server.auth.decorator import authorized
-from jupyter_server.auth.identity import IdentityProvider, UpdatableField
+from jupyter_server.auth.identity import IdentityProvider, UpdatableField, User
 
 from ...base.handlers import APIHandler, JupyterHandler
 
@@ -134,6 +134,8 @@ async def patch(self):
             )
         except ValueError as e:
             raise web.HTTPError(400, str(e)) from e
+        except NotImplementedError as e:
+            raise web.HTTPError(501, str(e)) from e
 
 
 default_handlers = [

tests/services/api/test_api.py

@@ -6,6 +6,7 @@
 from tornado.httpclient import HTTPError
 
 from jupyter_server.auth import Authorizer, IdentityProvider, User
+from jupyter_server.auth.identity import PasswordIdentityProvider
 
 
 async def test_get_spec(jp_fetch):
@@ -50,6 +51,22 @@ async def get_user(self, handler):
             return self.mock_user
 
 
+class MockPasswordIdentityProvider(PasswordIdentityProvider):
+    mock_user: MockUser
+
+    async def get_user(self, handler):
+        # super returns a UUID
+        # return our mock user instead, as long as the request is authorized
+        _authenticated = super().get_user(handler)
+        if isinstance(_authenticated, Awaitable):
+            _authenticated = await _authenticated
+        authenticated = _authenticated
+        if isinstance(self.mock_user, dict):
+            self.mock_user = MockUser(**self.mock_user)
+        if authenticated:
+            return self.mock_user
+
+
 class MockAuthorizer(Authorizer):
     def is_authorized(self, handler, user, action, resource):
         permissions = user.permissions
@@ -70,6 +87,17 @@ def identity_provider(jp_serverapp):
         yield idp
 
 
+@pytest.fixture
+def password_identity_provider(jp_serverapp):
+    idp = MockPasswordIdentityProvider(parent=jp_serverapp)
+    authorizer = MockAuthorizer(parent=jp_serverapp)
+    with mock.patch.dict(
+        jp_serverapp.web_app.settings,
+        {"identity_provider": idp, "authorizer": authorizer},
+    ):
+        yield idp
+
+
 @pytest.mark.parametrize(
     "identity, expected",
     [
@@ -118,9 +146,44 @@ async def test_identity(jp_fetch, identity, expected, identity_provider):
 
 
 @pytest.mark.parametrize("identity", [{"username": "user.username"}])
-async def test_update_user_success(jp_fetch, identity, identity_provider):
+async def test_update_user_not_implemented_update(jp_fetch, identity, identity_provider):
+    """Test successful user update."""
+    identity_provider.mock_user = MockUser(**identity)
+    payload = {
+        "color": "#000000",
+    }
+    with pytest.raises(HTTPError) as exc:
+        await jp_fetch(
+            "/api/me",
+            method="PATCH",
+            body=json.dumps(payload),
+            headers={"Content-Type": "application/json"},
+        )
+    assert exc.value.code == 501
+
+
+@pytest.mark.parametrize("identity", [{"username": "user.username"}])
+async def test_update_user_not_implemented_persist(jp_fetch, identity, identity_provider):
     """Test successful user update."""
     identity_provider.mock_user = MockUser(**identity)
+    identity_provider.update_user_model = lambda *args, **kwargs: identity_provider.mock_user
+    payload = {
+        "color": "#000000",
+    }
+    with pytest.raises(HTTPError) as exc:
+        await jp_fetch(
+            "/api/me",
+            method="PATCH",
+            body=json.dumps(payload),
+            headers={"Content-Type": "application/json"},
+        )
+    assert exc.value.code == 501
+
+
+@pytest.mark.parametrize("identity", [{"username": "user.username"}])
+async def test_update_user_success(jp_fetch, identity, password_identity_provider):
+    """Test successful user update."""
+    password_identity_provider.mock_user = MockUser(**identity)
     payload = {
         "color": "#000000",
     }
@@ -137,12 +200,12 @@ async def test_update_user_success(jp_fetch, identity, identity_provider):
 
 
 @pytest.mark.parametrize("identity", [{"username": "user.username"}])
-async def test_update_user_raise(jp_fetch, identity, identity_provider):
+async def test_update_user_raise(jp_fetch, identity, password_identity_provider):
     """Test failing user update."""
-    identity_provider.mock_user = MockUser(**identity)
+    password_identity_provider.mock_user = MockUser(**identity)
     payload = {
         "name": "Updated Name",
-        "color": "#000000",
+        "fake_prop": "anything",
     }
     with pytest.raises(HTTPError) as exc:
         await jp_fetch(
@@ -151,6 +214,7 @@ async def test_update_user_raise(jp_fetch, identity, identity_provider):
             body=json.dumps(payload),
             headers={"Content-Type": "application/json"},
         )
+    assert exc.value.code == 400
 
 
 @pytest.mark.parametrize(
@@ -168,10 +232,10 @@ async def test_update_user_raise(jp_fetch, identity, identity_provider):
     ],
 )
 async def test_update_user_success_custom_updatable_fields(
-    jp_fetch, identity, expected, identity_provider
+    jp_fetch, identity, expected, password_identity_provider
 ):
     """Test successful user update."""
-    identity_provider.mock_user = MockUser(**identity)
+    password_identity_provider.mock_user = MockUser(**identity)
     identity_provider.updatable_fields = ["name", "display_name", "color"]
     payload = {
         "name": expected["name"],