Add collision-safe KeyRing rename API

[erayack]

1. Purpose

KeyRing::rename_key returns a boolean, so callers cannot distinguish a missing source key from an occupied destination. Its legacy remove-then-insert behavior can also replace the destination before reporting failure.

This PR adds a fallible rename API that reports each failure explicitly while retaining the existing method for compatibility.

2. Implementation

crates/web-bot-auth/src/keyring.rs adds:

• OperationError::KeyNotPresent when the source identifier is absent.
• OperationError::KeyOccupied when the destination identifier is already present.
• try_rename_key, returning Result<(), OperationError>.

try_rename_key checks source absence first, then destination occupancy, then identifier equality. Otherwise it removes the existing KeyEntry and assigns it to the new identifier.

The existing rename_key implementation remains independent and is deprecated since 0.7.1, with a warning that it does not safely handle destination conflicts.

3. Behavior

The new API behaves as follows:

• A missing source returns Err(OperationError::KeyNotPresent).
• An occupied destination returns Err(OperationError::KeyOccupied) and preserves both keys.
• A free destination receives the existing KeyEntry, including its prepared verification state.

The legacy rename_key signature and behavior remain available during the deprecation period.

4. Tests

The Rust tests cover:

• Preserving both entries after a KeyOccupied result.
• KeyNotPresent taking precedence when the source is absent.
• Error precedence when both identifiers are the same.
• Successful rename verification with the prepared key state intact.

Internal test call sites use try_rename_key, avoiding deprecation warnings.

5. Review Guide

Review crates/web-bot-auth/src/keyring.rs:

• Confirm the requested error precedence in try_rename_key.
• Confirm destination collisions return before mutation.
• Confirm the final move relies on the preceding invariants without assertions.
• Confirm deprecated rename_key retains its legacy implementation and records 0.7.1.

Review crates/web-bot-auth/src/message_signatures.rs:

• Confirm the existing successful-rename verification test uses try_rename_key.

6. Risk

The new API is additive. The existing method remains callable with unchanged behavior, but now emits a deprecation warning.

This PR does not change key import, lookup, JWK handling, prepared-key construction, or signature verification.

7. Validation

The following commands passed:

• cargo test -p web-bot-auth --all-features (41 tests plus doc-tests)
• cargo clippy -p web-bot-auth --all-features --all-targets -- -D warnings
• cargo fmt --all -- --check
• git diff --check

[AkshatM]

Thank you for the contribution! Appreciate it.

I think the semantics mostly make sense, but the type output should really be a Result here. A boolean made sense when we expected the user to hold the interface right - if there's multiple ways to fail, however, then a Result communicates the full range of behaviours.

Here is my request for how to do this:

1. We should mark rename_key as #[deprecated].
2. We should have a new function try_rename_key with return type Result<(), OperationError> to use instead.
3. OperationError should be a new error enum, supporting the following variants:

• KeyNotPresent: Old key is not present.
• KeyOccupied: New key is already present.

If new key is same as old key, we return an Ok(()) without performing a reinsert.

Let me know what you think!

[erayack]

Makes sense, thanks! I’ll keep rename_key around as a deprecated boolean wrapper over try_rename_key for backward compatibility, and use OperationError to distinguish between a missing source and a taken destination.

Quick question on an edge case: if old_identifier == new_identifier, should we return Err(OperationError::KeyNotPresent) if the key doesn't exist? And if it does exist, just return Ok(()) as a no-op? Let me know if that matches what you're thinking.

[AkshatM]

Apply an order of precedence:

• If old key doesn't exist, return Err(OperationError::KeyNotPresent)
• If new exists, return Err(Operation::KeyOccupied)
• If both are the same, then it's safe to return Ok(()) without performing a rename
• Otherwise pop the old id and assign to new id

[AkshatM]

and I don't think we need to have rename_key as a wrapper around try_rename_key, especially since we're marking it deprecated. Just leave a comment explaining it doesn't guarantee renaming conflicts. I'll cut a new version later.

[erayack]

Just pushed the updates in 7314631!

• Added try_rename_key with the agreed error precedence.
• Kept and deprecated the legacy rename_key.
• Updated internal test call sites.
• All tests, Clippy, formatting, and diff checks are green.

[erayack]

🚀 @AkshatM