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 a distinct destination identifier is already present.
• try_rename_key, returning Result<(), OperationError>.

try_rename_key applies source absence first, treats an existing same-identifier rename as a successful no-op, rejects occupied destinations before mutation, and otherwise moves the existing KeyEntry to the new identifier.

The existing rename_key implementation remains independent and is marked deprecated 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 existing same-identifier rename returns Ok(()) without reinsertion.
• A distinct 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.
• Same-identifier behavior for present and absent keys.
• Successful rename verification with the prepared key state intact.

Internal test call sites now use try_rename_key, avoiding deprecation warnings.

5. Review Guide

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

• Confirm the error variants and precedence in try_rename_key.
• Confirm the same-identifier path returns before removal.
• Confirm the destination collision path returns before mutation.
• Confirm deprecated rename_key retains its legacy implementation.

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

• Confirm the existing successful-rename verification test now 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.

[AkshatM]

This needs to be swapped with the check on line 247 - that is, checking if the new identifier is already present should happen before checking if the two identifiers are the same.

[erayack]

Should an existing same-identifier rename now return KeyOccupied, or should it remain Ok(()) as previously requested?

[AkshatM]

Let's not use debug_assert! - this is going to be exposed to end users. We can remove this line altogether.

[AkshatM]

slightly simpler to just:

if let Some(value) = self.ring.remove(&old_identifier) {
    self.ring.insert(new_identifier, value);
}

Ok(())

The preceding checks will ensure our invariant of old existing / new key not existing.

[AkshatM]

You can set since to be 0.7.1