-
Notifications
You must be signed in to change notification settings - Fork 0
task: Refactor HMAC authentication plugin for better maintainability and cleaner tests #34
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
Conversation
Co-authored-by: GrantBirki <[email protected]>
Co-authored-by: GrantBirki <[email protected]>
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 PR refactors the HMAC authentication plugin by extracting timestamp logic, simplifying HMAC validation, and cleaning up the test suite.
- Extracts
TimestampValidator
into its own class to handle parsing and validation - Updates
HMAC
to delegate timestamp checks toTimestampValidator
- Refactors specs with helper methods to reduce duplication and improve clarity
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
spec/unit/lib/hooks/plugins/auth/timestamp_validator_spec.rb | Adds comprehensive specs for TimestampValidator covering valid/invalid inputs |
spec/unit/lib/hooks/plugins/auth/hmac_spec.rb | Refactors HMAC tests to use new helper methods and removes repetitive code |
lib/hooks/plugins/auth/timestamp_validator.rb | Introduces TimestampValidator class encapsulating timestamp parsing/validation |
lib/hooks/plugins/auth/hmac.rb | Delegates timestamp checks to TimestampValidator and cleans up inline logic |
.bundle/config | Updates bundle path and deployment settings, but may include environment-specifics |
Comments suppressed due to low confidence (3)
spec/unit/lib/hooks/plugins/auth/timestamp_validator_spec.rb:3
- Use
require \"spec_helper\"
instead of a deep relative path to leverage standard load paths and simplify requires.
require_relative "../../../../spec_helper"
spec/unit/lib/hooks/plugins/auth/hmac_spec.rb:42
- [nitpick] The helper name
create_version_prefixed_signature
is a bit verbose and unclear. Consider renaming tocreate_versioned_signature
to better express its purpose.
def create_version_prefixed_signature(signing_payload, version = "v0")
.bundle/config:3
- Committing an absolute, CI-specific
BUNDLE_PATH
can break local setups. Consider removing.bundle/config
from version control or using a relative path.
BUNDLE_PATH: "/home/runner/work/hooks/hooks/vendor/bundle"
Co-authored-by: Copilot <[email protected]>
Refactored the HMAC authentication plugin to follow Ruby best practices and improve maintainability, addressing the unwieldy size and complex timestamp parsing logic identified in the issue.
Key Improvements
1. Extracted TimestampValidator class
TimestampValidator
class (133 lines) to encapsulate all timestamp-related logicvalid?(timestamp, tolerance)
andparse(timestamp)
2. Simplified HMAC class
TimestampValidator
3. Improved test maintainability
create_signature(payload, algorithm)
create_algorithm_prefixed_signature(payload, algorithm)
create_timestamped_signature(timestamp, version)
TimestampValidator
(127 lines)TimestampValidator
4. Enhanced code quality
Validation
Total impact: Removed 223 lines of code while significantly improving structure, maintainability, and testability of the timestamp parsing logic.
Fixes #31.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.