fix: authenticate against the context's home realm, not the target realm - #32
Conversation
client.rs and user.rs authenticated against the resolved target realm (--realm or context default) instead of the context's configured home realm, where the OAuth client is actually registered. Any command whose target realm differed from the client's home realm failed: the token exchange (or the cached-credentials lookup) was attempted against the wrong realm. realm.rs already authenticated correctly (always via context.realm, independent of the operation's target realm) — client.rs and user.rs now follow the same auth_client(context) pattern. Root cause of #21.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughClient and user commands now authenticate against the selected context's home realm. New ChangesContext-based authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The CLI now authenticates in the context’s home realm while targeting resources in the selected realm, enabling intended cross-realm administration. Merge is reasonable with explicit owner awareness that the server must enforce authorization for these cross-realm client and user operations. Sequence Diagram(s)sequenceDiagram
participant CommandHandler
participant AuthClient as auth_client
participant SessionClient as session::authenticated_client
CommandHandler->>AuthClient: Request authenticated client
AuthClient->>AuthClient: Read context.realm
AuthClient->>SessionClient: Authenticate with home realm
SessionClient-->>CommandHandler: Return FerriskeyClient
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes meet issue Full details: Out of Scope Changes checkExplanation The changes are limited to authentication behavior for client and user commands, related error variants, and regression tests. These changes support the objectives in issue
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
* feat(user): add set-password, remove-role and roles commands Refs #23 * fix(user): correct set-password endpoint (verified against a live server) set_user_password guessed PUT realms/{realm}/users/{id}/password (404). The real endpoint is PUT realms/{realm}/users/{id}/reset-password - found via the 405 Allow header while probing against a running FerrisKey server. * fix(user): use auth_client after rebase reintroduced authenticate calls GitHub's branch-update rebase of this PR onto main (post-#32) replayed this branch's additive commit without semantic conflict: the new remove-role/roles/set-password functions still called the now-removed authenticate(context, realm) helper instead of #32's auth_client(context).
Bug
clientandusercommands authenticated against the resolved target realm (--realm, falling back to the context's default) instead of the context's home realm — the realm where the context'sclient_id/client_secretare actually registered.Repro: a context configured against
master(ferris-ctl context add local --url http://localhost:3333 --client-id cli --client-secret *** --realm master), then:Server logs show the CLI attempting the
client_credentialstoken exchange against/realms/mestier/protocol/openid-connect/tokeninstead of/realms/master/...— thecliclient doesn't exist inmestier, so the server 404s.realm.rsalready got this right (always authenticates viacontext.realm, independent of whatever realm a subcommand targets);client.rsanduser.rsdidn't.Fix
Added an
auth_client(context)helper toclient.rsanduser.rs, mirroring the one already inrealm.rs: it authenticates strictly viacontext.realm, never the command's resolved target realm. The target realm (resolve_realm) is still used, but only to build the resource path of each API call — authorization for cross-realm operations is left to the server, per #21's "Expected behaviour".This is the root cause of #21 (
--realmrefused / failing whenever it differs from the session realm) — the issue's literal repro (no credentials available) is one symptom of the same bug: the stored-credentials cache key and the client_credentials exchange were both keyed off the target realm instead of the home realm.Issue
Closes #21.
Verification
This repo shares a local FerrisKey instance (
http://localhost:3333, contextlocal, realmmaster) with amestierrealm and anathaelbuser in it — used it to reproduce and verify directly, not just unit tests:main):user list --realm mestier→ 404, as in the bug report.user list --realm mestierandclient list --realm mestierboth succeed, returningmestier's users/clients while authenticating againstmaster.user list(no--realm, same-realm case) still works.Test plan
cargo build --workspacecargo test --workspace(62 passed, +2 new regression tests:auth_client_requires_realm_on_contextinclient.rsanduser.rs)cargo clippy --workspace --all-targets --all-features -- -D warningsSummary by CodeRabbit