smolnar82 commented on PR #1325: URL: https://github.com/apache/knox/pull/1325#issuecomment-5066884987
Nice addition, @Raghav-Mah3shwari, the end-to-end path here (mint a token in knoxldap → present it to the JWTProvider-federated knoxtoken topology → assert x-knox-actor-username) is the highest-value scenario and it's covered cleanly. The file follows the suite conventions (common_utils, unittest, HTTPBasicAuth) and the issuance + negative auth cases all look correct. A couple of things before merge: **1. Missing token lifecyle operation test cases** Would you be up for extending the scope to cover the token lifecycle operations (`renew` / `revoke` / `enable` / `disable`) in a follow-up (or here, if you prefer)? Since [KNOX-3395 ](https://issues.apache.org/jira/browse/KNOX-3395) indicates `KNOXTOKEN` issuance, it's a natural next step and there's a config prerequisite worth capturing while it's fresh: These endpoints require server-managed token state, which neither `knoxtoken.xml` nor `knoxldap.xml` currently enables (both set only token TTL). So it's a topology-config change first, tests second. Against the current config: - enable / disable / revoke → tokenStateService == null → 400 CONFIGURATION_ERROR - renew → falls into the renewalDisabled branch and echoes the token's own expiry — no actual renewal To exercise them you'd need, on the `KNOXTOKEN` service: ``` <param><name>knox.token.exp.server-managed</name><value>true</value></param> <param><name>knox.token.renewer.whitelist</name><value>guest</value></param> ``` Without the whitelist, token renewal or revocation will return 403 (revoking your own token is the one exception). Cases I'd suggest for that round: - `Revoke` → enforcement (the key one): mint → revoke → re-present to the federated endpoint and assert it's now 401. The {"revoked":"true"} response alone doesn't prove revocation is enforced. - `Renew`: returns {"renewed":"true", ...} and the renewed token still federates. - `Authorization negative`: a non-whitelisted user gets 403 on renew/revoke. - `enable/disable`: disable a token, confirm it stops working; cover the "already enabled/disabled" 400 paths. **2. Two minor notes on the existing tests:** - `test_federated_topology_rejects_invalid_token` uses "not.a.valid.jwt" (structurally malformed). Consider also a well-formed-but-wrong-signature token so the RS256 signature check (`jwt.expected.sigalg`) is exercised, not just the parser. This is doable by removing the last character of the generated token's `access_key` field. - The topologies set `knoxsso.token.ttl`, but the documented `KNOXTOKEN` TTL param is `knox.token.ttl` if I were you, I'd change them in the scope of this PR. -- 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]
