Skip to content

fix: correct client-role create/delete (verified against a live server) - #33

Merged
NathaelB merged 1 commit into
mainfrom
fix/client-role-create-and-delete
Sep 2, 2026
Merged

fix: correct client-role create/delete (verified against a live server)#33
NathaelB merged 1 commit into
mainfrom
fix/client-role-create-and-delete

Conversation

@NathaelB

@NathaelB NathaelB commented Sep 2, 2026

Copy link
Copy Markdown
Member

Bug

#30 (merged as 01c681d, before I got a chance to verify it live) shipped two wrong assumptions about client-role endpoints:

  1. create_client_role's response was decoded as an enveloped {"data": ...} payload, same as create_role. Against a real server this fails with error decoding response body — the endpoint actually returns the created role as a bare JSON object, not wrapped.
  2. delete_client_role posted to a guessed DELETE realms/{realm}/clients/{client_uuid}/roles/{role_id} path, which doesn't exist (404). There's no client-scoped delete endpoint at all — role ids are unique across realm and client scopes, so the existing DELETE realms/{realm}/roles/{role_id} deletes a client role too.

Fix

  • create_client_role now decodes a bare CreatedRole, no envelope.
  • Dropped delete_client_role entirely; realm::delete_role now always calls the flat client.delete_role(realm, role_id), regardless of --client.

Verification

Ran the full role lifecycle against a local FerrisKey server: role create/get/list/delete for both a realm role and a client role (on a throwaway test client), confirmed each step against role list, then cleaned up all test fixtures. Caught both bugs this way before writing the fix.

Test plan

  • cargo build --workspace
  • cargo test --workspace (60 passed)
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • Manual full lifecycle test against a running FerrisKey server (see above)

@NathaelB NathaelB self-assigned this Sep 2, 2026
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 7 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 9b82bb76-5dd5-40bd-8184-d8b3c41bfed4

📥 Commits

Reviewing files that changed from the base of the PR and between 15eaaae and 1ca0056.

📒 Files selected for processing (2)
  • libs/ferriskey-cli-client/src/lib.rs
  • libs/ferriskey-cli-core/src/realm.rs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Verified role get/create/list/delete (realm and client-scoped) against
a running FerrisKey server:
- create_client_role's response isn't enveloped in {"data": ...} like
  create_role's is — decode it as a bare CreatedRole instead.
- There is no client-scoped delete endpoint; role ids are unique across
  realm and client scopes, so plain delete_role handles both. Dropped
  delete_client_role and simplified realm::delete_role accordingly.
@NathaelB
NathaelB force-pushed the fix/client-role-create-and-delete branch from 485f21c to 1ca0056 Compare September 2, 2026 22:20
@NathaelB
NathaelB merged commit 410ab62 into main Sep 2, 2026
4 checks passed
@NathaelB
NathaelB deleted the fix/client-role-create-and-delete branch September 2, 2026 22:42
NathaelB added a commit that referenced this pull request Sep 3, 2026
UserBlueprint.roles only resolved realm roles: the role_ids table was
populated by create_role alone, so client roles created by
ClientBlueprint.roles could never be assigned to a user — an import
described an intent it couldn't carry out.

A role entry can now be qualified as `client_id:role_name` to reference
a client role; unqualified entries keep resolving as realm roles. An
unresolved client role reference fails the import loudly
(ImportError::UnresolvedClientRole) rather than being silently skipped
with a warning, per the issue's expected behaviour — this is
deliberately different from an unresolved realm role, which keeps its
existing warn-and-skip behavior to avoid changing established
semantics for existing files.

Verified live: assign_user_role accepts a client role id through the
same endpoint as a realm role id (no separate client-scoped assignment
endpoint needed) — role ids are unique across both scopes, consistent
with the delete_role finding in #33.
NathaelB added a commit that referenced this pull request Sep 3, 2026
Closes the last gap in #23: a role assigned to or removed from a user
could only be a realm role. --client resolves the role among that
client's roles instead, mirroring realm role's --client support.

assign_user_role/remove_user_role need no client-scoped variant: role
ids are unique across realm and client scopes, so the existing
endpoints accept a client role id directly (verified live) — same
finding as delete_role in #33.
NathaelB added a commit that referenced this pull request Sep 3, 2026
* feat(import): extend ClientBlueprint with PKCE, device auth, origins, lifetimes

post_logout_redirect_uris, web_origins, PKCE requirement, the Device
Authorization grant toggle, and per-client token lifetimes were
missing from ClientBlueprint, so an import couldn't reproduce them and
they were lost on the next realm.

Adds the corresponding client endpoints (POST .../post-logout-redirects,
POST .../web-origins, PATCH .../clients/{id} for PKCE/lifetimes — the
latter only settable after creation, unlike the rest of a client's
fields) and applies them in apply.rs. All five stay optional so
existing blueprint files keep importing unchanged.

Keycloak/Zitadel sources default these fields rather than extracting
them: Keycloak carries the equivalent settings under a free-form
`attributes` map with Keycloak-specific keys, which is a separate
scope of work.

* feat(import): resolve client roles in UserBlueprint.roles

UserBlueprint.roles only resolved realm roles: the role_ids table was
populated by create_role alone, so client roles created by
ClientBlueprint.roles could never be assigned to a user — an import
described an intent it couldn't carry out.

A role entry can now be qualified as `client_id:role_name` to reference
a client role; unqualified entries keep resolving as realm roles. An
unresolved client role reference fails the import loudly
(ImportError::UnresolvedClientRole) rather than being silently skipped
with a warning, per the issue's expected behaviour — this is
deliberately different from an unresolved realm role, which keeps its
existing warn-and-skip behavior to avoid changing established
semantics for existing files.

Verified live: assign_user_role accepts a client role id through the
same endpoint as a realm role id (no separate client-scoped assignment
endpoint needed) — role ids are unique across both scopes, consistent
with the delete_role finding in #33.
NathaelB added a commit that referenced this pull request Sep 3, 2026
Closes the last gap in #23: a role assigned to or removed from a user
could only be a realm role. --client resolves the role among that
client's roles instead, mirroring realm role's --client support.

assign_user_role/remove_user_role need no client-scoped variant: role
ids are unique across realm and client scopes, so the existing
endpoints accept a client role id directly (verified live) — same
finding as delete_role in #33.
NathaelB added a commit that referenced this pull request Sep 3, 2026
* feat(client): add secret command, carry client secrets in import report

Add `ferris-ctl client secret <client_id> [--realm <realm>]`, reading
a confidential client's secret via GET .../clients/{uuid}/client-secret
(the endpoint cited in the issue, confirmed to exist server-side).
Prints the raw secret to stdout only, so it can be piped/captured; all
other output stays on stderr, ignoring --output deliberately.

realm import now also reads and reports the secret of every
confidential client it touches, so an import is self-sufficient
without a follow-up `client secret` call per client.

The endpoint's response shape isn't documented; ClientSecretPayload
accepts the plausible variants (bare string, {secret}, and either
enveloped in {data: ...}) rather than assuming one — see PR
description for what could and couldn't be verified live.

* feat(user): add --client to assign-role and remove-role

Closes the last gap in #23: a role assigned to or removed from a user
could only be a realm role. --client resolves the role among that
client's roles instead, mirroring realm role's --client support.

assign_user_role/remove_user_role need no client-scoped variant: role
ids are unique across realm and client scopes, so the existing
endpoints accept a client role id directly (verified live) — same
finding as delete_role in #33.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant