Tighten AuthFederation tests + require update() lifetime arg

Three review nits from Wayne Xie's second pass on PR #1894:

- Realism in the "never-enabled" GET fixture: use the real server defaults
  — token lifetime 7200 (AuthFederationService.java:86) and the actual
  issuer URL shape %s/api/%s/sites/<luid>/auth-federation (:748-766),
  not the fabricated /sites/mySite/... URL and 3600 lifetime.
- UUID-guard matrix: add None to the bad-target list and parametrize over
  {raw, item} so the item-wrapped None path (connector.luid=None /
  key.kid=None) is also covered. Widens bad_target to object.
- update() token_lifetime_seconds is now required: there is no other
  mutable field on the config, so a no-arg call is not a meaningful
  operation. Tightens the matching request_factory signature and drops
  the test that exercised the empty-patch wire shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: sushmasreedharan-tableau

CHANGELOG.md

@@ -11,10 +11,10 @@
   - `server.auth_federation.enable(token_lifetime_seconds=None)` ->
     `AuthFederationItem` — idempotent PUT. Server auto-generates one active
     signing key on first enable.
-  - `server.auth_federation.update(token_lifetime_seconds=None)` ->
+  - `server.auth_federation.update(token_lifetime_seconds)` ->
     `AuthFederationItem` — merge-patch; only `token_lifetime_seconds` is
-    mutable. `enabled` is never sent on PATCH; use `enable()` / `delete()`
-    to change WIF state.
+    mutable (required), so `update()` always changes it. `enabled` is never
+    sent on PATCH; use `enable()` / `delete()` to change WIF state.
   - `server.auth_federation.delete()` — permanently deletes the WIF config
     along with every signing key and connector on the site. A subsequent
     `enable()` starts from a clean slate; prior connectors and keys are

tableauserverclient/server/endpoint/auth_federation_endpoint.py

@@ -130,7 +130,7 @@ def enable(self, token_lifetime_seconds: int | None = None) -> AuthFederationIte
         return AuthFederationItem.from_response(response.content, self.parent_srv.namespace)
 
     @api(version="3.30")
-    def update(self, token_lifetime_seconds: int | None = None) -> AuthFederationItem:
+    def update(self, token_lifetime_seconds: int) -> AuthFederationItem:
         """
         Merge-patch the mutable fields of the WIF admin config. Only
         ``token_lifetime_seconds`` is mutable; ``enabled``, ``issuer_url``,
@@ -141,9 +141,10 @@ def update(self, token_lifetime_seconds: int | None = None) -> AuthFederationIte
 
         Parameters
         ----------
-        token_lifetime_seconds : int | None
-            New JWT lifetime in seconds. ``None`` sends an empty patch (no
-            fields changed) — rarely useful, but accepted by the server.
+        token_lifetime_seconds : int
+            New JWT lifetime in seconds. Required — there is nothing else to
+            patch, so a call with no fields to change is not a meaningful
+            operation.
 
         Returns
         -------

tableauserverclient/server/request_factory.py

@@ -117,12 +117,11 @@ def enable_req(self, token_lifetime_seconds: int | None = None) -> bytes:
             elem.attrib["tokenLifetimeSeconds"] = str(token_lifetime_seconds)
         return ET.tostring(elem)
 
-    def update_req(self, token_lifetime_seconds: int | None = None) -> bytes:
+    def update_req(self, token_lifetime_seconds: int) -> bytes:
         # ``enabled`` must never be sent on PATCH — the server rejects a merge
         # patch with ``enabled`` present. Enable/disable goes through PUT/DELETE.
         elem = ET.Element("authFederationAdmin")
-        if token_lifetime_seconds is not None:
-            elem.attrib["tokenLifetimeSeconds"] = str(token_lifetime_seconds)
+        elem.attrib["tokenLifetimeSeconds"] = str(token_lifetime_seconds)
         return ET.tostring(elem)
 
     def create_connector_req(self, connector: "AuthFederationConnectorItem") -> bytes:

test/test_auth_federation.py

@@ -50,22 +50,27 @@ def test_auth_federation_get(server: TSC.Server) -> None:
 def test_auth_federation_get_defaults_when_never_enabled(server: TSC.Server) -> None:
     # Server GET on a never-enabled site still returns ``issuerUrl`` and the
     # default ``tokenLifetimeSeconds``; only ``activeKeyId`` and the
-    # ``<connectors>`` wrapper are absent.
+    # ``<connectors>`` wrapper are absent. Values mirror the real server:
+    # DEFAULT_TOKEN_LIFETIME_SECONDS=7200 (AuthFederationService.java:86) and
+    # ``<base>/api/<version>/sites/<siteLuid>/auth-federation`` (:748-766).
+    default_issuer_url = (
+        "https://10ax.online.tableau.com/api/3.30/sites/" "dad65087-b08b-4603-af4e-2887b8aafc67/auth-federation"
+    )
     default_body = (
         '<tsResponse xmlns="http://tableau.com/api">'
         '<authFederationAdmin enabled="false" '
-        'issuerUrl="https://10ax.online.tableau.com/sites/mySite/auth-federation" '
-        'tokenLifetimeSeconds="3600"/>'
+        f'issuerUrl="{default_issuer_url}" '
+        'tokenLifetimeSeconds="7200"/>'
         "</tsResponse>"
     )
     with requests_mock.mock() as m:
         m.get(server.auth_federation.baseurl, text=default_body)
         config = server.auth_federation.get()
 
     assert config.enabled is False
-    assert config.issuer_url == "https://10ax.online.tableau.com/sites/mySite/auth-federation"
+    assert config.issuer_url == default_issuer_url
     assert config.active_key_id is None
-    assert config.token_lifetime_seconds == 3600
+    assert config.token_lifetime_seconds == 7200
     assert config.connectors == []
 
 
@@ -119,22 +124,6 @@ def test_auth_federation_update(server: TSC.Server) -> None:
     assert config_out.token_lifetime_seconds == 7200
 
 
-def test_auth_federation_update_empty_patch(server: TSC.Server) -> None:
-    # This exercises the client wire shape only. Whether the server binds an
-    # empty ``<authFederationAdmin/>`` as a non-null ``AuthFederationConfigType``
-    # with every field null (a no-op merge-patch) or as a null body (400) must
-    # be covered by a server-side integration test — not here.
-    with requests_mock.mock() as m:
-        m.patch(server.auth_federation.baseurl, text=UPDATE.read_text())
-        server.auth_federation.update()
-        body = m.request_history[0].text
-
-    elem = fromstring(body)
-    assert elem.tag == "authFederationAdmin"
-    assert "enabled" not in elem.attrib
-    assert "tokenLifetimeSeconds" not in elem.attrib
-
-
 def test_auth_federation_delete(server: TSC.Server) -> None:
     with requests_mock.mock() as m:
         m.delete(server.auth_federation.baseurl, status_code=204)
@@ -330,16 +319,36 @@ def test_auth_federation_delete_key_by_item(server: TSC.Server) -> None:
     "deactivate_key",
     "delete_key",
 ]
+_KEY_METHODS = {"activate_key", "deactivate_key", "delete_key"}
 
 
+@pytest.mark.parametrize("wrap_as_item", [False, True], ids=["raw", "item"])
 @pytest.mark.parametrize("method_name", _UUID_METHODS)
-@pytest.mark.parametrize("bad_target", ["../../admin", "not-a-uuid", "a4b9c2d7", "", "d3a8e5b2-7c01-4e13-9c82"])
-def test_auth_federation_rejects_non_uuid_target(server: TSC.Server, method_name: str, bad_target: str) -> None:
+@pytest.mark.parametrize(
+    "bad_target",
+    [None, "../../admin", "not-a-uuid", "a4b9c2d7", "", "d3a8e5b2-7c01-4e13-9c82"],
+    ids=["None", "path", "noise", "short", "empty", "truncated"],
+)
+def test_auth_federation_rejects_non_uuid_target(
+    server: TSC.Server, method_name: str, bad_target: object, wrap_as_item: bool
+) -> None:
+    if wrap_as_item:
+        if method_name in _KEY_METHODS:
+            item = TSC.AuthFederationKeyItem()
+            item.kid = bad_target  # type: ignore[assignment]
+            target: object = item
+        else:
+            connector = TSC.AuthFederationConnectorItem()
+            connector.luid = bad_target  # type: ignore[assignment]
+            target = connector
+    else:
+        target = bad_target
+
     method = getattr(server.auth_federation, method_name)
     with requests_mock.mock() as m:
         m.register_uri(requests_mock.ANY, requests_mock.ANY)
         with pytest.raises(ValueError, match="Invalid UUID"):
-            method(bad_target)
+            method(target)
         assert not m.called  # must reject before any HTTP call