Skip to content

Commit f71cf38

Browse files
Address Copilot review comments on WIF admin PR
- E2E teardown: let `ServerResponseError` from `auth_federation.delete()` propagate instead of swallowing it. A failed teardown left WIF enabled on the site, so the next run would hit the fixture's `enabled=True` guard and silently skip — the destructive paths stopped being exercised without anyone noticing. - Key-rotation guidance (endpoint docstring, CHANGELOG, and docs): the previous `create_key() -> activate_key(new) -> delete_key(old)` sequence omitted that the old key must stay in the JWKS until every JWT it signed has expired. Deleting it immediately removes it from the key set, and verifiers that fetch JWKS afterwards reject the still-valid tokens signed with it. New guidance keeps the old key inactive (but published) for up to `token_lifetime_seconds` before deleting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent b9db799 commit f71cf38

3 files changed

Lines changed: 25 additions & 13 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,13 @@
3939
or an `AuthFederationKeyItem` with `kid` set; `activate_key` is atomic
4040
and deactivates any previously active key. `delete_key` fails with 400
4141
if the key is currently active. Safe rotation is `create_key()` →
42-
`activate_key(new_kid)` → `delete_key(old_kid)`; `deactivate_key` on
43-
the active key with no replacement leaves the site unable to mint WIF
44-
tokens. The four key-management methods are bound to `NO_ACCESS`
42+
`activate_key(new_kid)`, then retain the old (now-inactive) key in the
43+
site's JWKS until every JWT it signed has expired (upper bound: the
44+
site's current `token_lifetime_seconds`), and only then
45+
`delete_key(old_kid)` — deleting it earlier removes it from the JWKS
46+
and any verifier fetching the key set afterwards will reject still-
47+
unexpired tokens signed with it. `deactivate_key` on the active key
48+
with no replacement leaves the site unable to mint WIF tokens. The four key-management methods are bound to `NO_ACCESS`
4549
server-side, which only rejects OAuth-scope-restricted sessions
4650
(e.g. connected-app JWT) with 401; password and personal-access-token
4751
(PAT) sessions do not carry scopes and are accepted.

‎tableauserverclient/server/endpoint/auth_federation_endpoint.py‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -311,12 +311,19 @@ def delete_key(self, key: KeyIdentifier) -> None:
311311
"""
312312
Delete a WIF signing key. Fails with 400 if the key is active.
313313
314-
For rotation, call ``create_key()`` → ``activate_key(new_kid)`` →
315-
``delete_key(old_kid)``: ``activate_key`` atomically deactivates the
316-
previously active key, so the site is never without an active signing
317-
key. Calling ``deactivate_key`` on the active key before another is
318-
activated leaves the site with no active key, and no new WIF tokens
319-
can be minted until a replacement is activated.
314+
For rotation, call ``create_key()`` → ``activate_key(new_kid)``, then
315+
**retain** the old (now-inactive) key in the site's JWKS until every
316+
JWT it signed has expired (upper bound: the site's current
317+
``token_lifetime_seconds``, default 7200 s / 2 h), and only then
318+
call ``delete_key(old_kid)``. ``activate_key`` atomically deactivates
319+
the previously active key, so the site is never without an active
320+
signing key — but the inactive key stays published in the JWKS so
321+
verifiers can keep validating tokens it already signed. Deleting it
322+
before those tokens have expired removes it from the JWKS, and any
323+
verifier that fetches the key set afterwards will reject the still-
324+
unexpired tokens. Calling ``deactivate_key`` on the active key
325+
before another is activated leaves the site with no active key, and
326+
no new WIF tokens can be minted until a replacement is activated.
320327
321328
Parameters
322329
----------

‎test_e2e/test_auth_federation.py‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,11 @@ def wif_site(server):
6060

6161
yield server
6262

63-
try:
64-
server.auth_federation.delete()
65-
except ServerResponseError:
66-
pass
63+
# Let any ServerResponseError propagate: a failed teardown delete must
64+
# surface as a pytest teardown error. Swallowing it would leave WIF
65+
# enabled on the site, so the next run would skip (fixture guard above)
66+
# and silently stop exercising the destructive paths.
67+
server.auth_federation.delete()
6768

6869

6970
def test_wif_round_trip_bare_body_wire_contract(wif_site):

0 commit comments

Comments
 (0)