Santoshkumarpuppala commented on issue #11571:
URL: https://github.com/apache/gravitino/issues/11571#issuecomment-5521027588
A test worth adding here, because there is a case the current suite
structurally cannot reach.
`_extract_principal` (`mcp-server/mcp_server/core/audit.py:57`) returns
`f"{scheme}:{credential[:8]}"` for a bearer token. For a JWT, the first 8
characters are the base64url of the **header**, not anything caller-specific —
so every JWT issued by the same issuer with the same algorithm produces the
same principal.
Run against the shipped function, three different subjects:
```
sub=alice -> 'bearer:eyJhbGci'
sub=bob -> 'bearer:eyJhbGci'
sub=carol -> 'bearer:eyJhbGci'
distinct principals: 1 of 3
```
Controls from the same run, to show the function can discriminate and the
rig works: `Basic` for alice and bob give `'alice'` and `'bob'`; empty gives
`'anonymous'`; and this file's own `"Bearer abcdefghijklmnop"` gives
`'bearer:abcdefgh'`. It generalises past one header shape — `{"typ","alg"}`
collapses to `'bearer:eyJ0eXAi'` and a Keycloak-style `{"alg","typ","kid"}` to
`'bearer:eyJhbGci'`, 4 subjects to 1 principal in each case.
**This is not an argument against truncating.** Keeping token material out
of the log is right, and the docstring says plainly what it does. The point is
narrower: the epic's objective is that "every access produces an attributable
audit record", and #11572 (per-request token isolation, multi-principal) is
closed — so the multi-user configuration this was built for is exactly the one
where every JWT caller lands on a single principal. The record attributes to
the issuer configuration rather than to the caller.
**Why no test catches it.** `mcp-server/tests/unit/test_audit.py` contains
four bearer inputs — `"Bearer abcdefghijklmnop"`, `"Bearer abc"`, `"Bearer"`,
`"Bearer "` — all synthetic strings whose leading bytes differ by
construction, and **zero** `assertNotEqual` against 26 `assertEqual`. No test
compares two tokens, so "different callers produce different principals" is
never asserted. The suite pins the shape of one principal, not the
discrimination between two.
The two-line version:
```python
def test_two_jwt_callers_get_distinct_principals(self):
self.assertNotEqual(_extract_principal(f"Bearer {jwt_for('alice')}"),
_extract_principal(f"Bearer {jwt_for('bob')}"))
```
That fails today and is the property the epic actually needs. Whether the
fix is a salted hash of the credential, a window that isn't the header, or
decoding `sub` when the token parses as a JWT, is a separate call — but the
test states the requirement independently of which one you pick.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
