-
Notifications
You must be signed in to change notification settings - Fork 81
Signature: Make key management Actor-specific #1832
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
@pfefferle Is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request refactors key management functions by moving them from the Signature class into the Actors collection, deprecating the older public methods in Signature. Key changes include:
- Updating references from Signature to Actors across tests and core model files.
- Adding new key generation and retrieval methods in the Actors class.
- Removing deprecated key management functionality from the Signature class.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/includes/rest/class-test-signature-verification.php | Updated key lookups from Signature to Actors to use the new API. |
tests/includes/collection/class-test-actors.php | Added tear_down method and extended test coverage for key generation and legacy handling. |
tests/includes/class-test-signature.php | Removed tests for deprecated key management functions. |
includes/model/class-user.php, class-blog.php, class-application.php | Replaced references to Signature-based key fetching with Actors-based calls. |
includes/collection/class-actors.php | Introduced new methods for retrieving public/private keys as well as key pair generation. |
includes/class-signature.php | Deprecated key management functions now proxy to Actors with deprecation warnings. |
I would think so! Maybe we can unify the Either in the same function:
Or a dedicated |
includes/class-signature.php
Outdated
@@ -194,7 +81,7 @@ protected static function check_legacy_key_pair_for( $user_id ) { | |||
*/ | |||
public static function generate_signature( $user_id, $http_method, $url, $date, $digest = null ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be awesome to pass the private key as a param instead of the $user_id
, to be completely user agnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2a0373c. This turned out to be a bit more invasive than I hoped, since there are two data points dependent on the user.
Long term, I wonder if we should move Signature from being static to being an object that we'd instantiate with the user stuff and then call generate_signature()
with just the request information
I was wondering about that, too, but couldn't think of a use case. To verify signatures on incoming requests I think we should always make remote requests to the |
$user = Actors::get_by_id( $user_id ); | ||
$key = self::get_private_key_for( $user->get__id() ); | ||
|
||
public static function generate_signature( $key_id, $private_key, $http_method, $url, $date, $digest = null ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up that replacing $user_id
with the required values changes the function signature.
Appends '#main-key' to actor_id when generating signatures in HTTP requests and tests. Props @pfefferle.
Minor house keeping item in preparation for #1808, separating the key management bits out from the signature functionality itself. I'm hoping it'll make it easier to update the Signature class in the future.
Proposed changes:
Actors
class.Other information:
Testing instructions:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Management of public/private keys for Actors now lives in the Actors collection, in preparation for Signature improvements down the line.