Refactor webhook method configuration and add CLI commands #1082
Conversation
Previously triggered for all methods by default. Modified code to require explicit method configuration.
WalkthroughThe changes in this pull request involve modifications to the authorization handling in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
api/types/project.go (1)
Line range hint
63-73: Consider enhancing documentation and validationThe method's behavior could be made more explicit and robust.
Consider these improvements:
// RequireAuth returns whether the given method requires authorization. +// If AuthWebhookURL is set but AuthWebhookMethods is empty, no authorization +// is required. To require authorization for all methods, explicitly add them +// to AuthWebhookMethods. func (p *Project) RequireAuth(method Method) bool { if len(p.AuthWebhookURL) == 0 { return false } if len(p.AuthWebhookMethods) == 0 { return false } + // Convert method list to a map for O(1) lookup + methodSet := make(map[Method]struct{}, len(p.AuthWebhookMethods)) + for _, m := range p.AuthWebhookMethods { + methodSet[Method(m)] = struct{}{} + } + + _, exists := methodSet[method] + return exists }cmd/yorkie/project/update.go (1)
38-39: Consider using typed constants for webhook methods.While the implementation is functional, using raw strings for method names could lead to runtime errors due to typos. Consider:
- Using typed constants or enums for webhook methods
- Adding validation for method names when they're added
Example improvement:
-var allAuthWebhookMethods = []string{ - string(types.ActivateClient), - string(types.DeactivateClient), - // ... -} +var allAuthWebhookMethods = []types.WebhookMethod{ + types.ActivateClient, + types.DeactivateClient, + // ... +} +func isValidWebhookMethod(method string) bool { + for _, m := range allAuthWebhookMethods { + if string(m) == method { + return true + } + } + return false +}Also applies to: 44-53
test/integration/auth_webhook_test.go (2)
44-53: Add documentation for the allWebhookMethods variableConsider adding a documentation comment explaining the purpose and usage of this variable, especially since it's used across multiple test cases.
+// allWebhookMethods contains all available webhook methods that can be configured +// for authorization. This is used across test cases to verify webhook behavior +// when all methods are enabled. var allWebhookMethods = &[]string{
Line range hint
215-260: LGTM! Comprehensive test coverage for selected methodsThe test case effectively verifies the new behavior where only explicitly configured methods require authorization. It tests both authorized (Activate) and unauthorized (Attach) methods.
Consider adding an assertion to verify that the project's
AuthWebhookMethodsfield is correctly set after the update:) assert.NoError(t, err) + + // Verify that only specified methods are configured + updatedProject, err := adminCli.GetProject(ctx, project.ID.String()) + assert.NoError(t, err) + assert.Equal(t, []string{ + string(types.AttachDocument), + string(types.WatchDocuments), + }, updatedProject.AuthWebhookMethods) projectInfoCacheTTL := 5 * time.Second
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
api/types/project.go(1 hunks)api/types/project_test.go(1 hunks)cmd/yorkie/project/update.go(4 hunks)test/integration/auth_webhook_test.go(12 hunks)
🔇 Additional comments (6)
api/types/project_test.go (2)
38-44: LGTM! Test correctly verifies the new webhook behavior.
The test case properly validates that when no methods are specified, authentication is not required for any method, which aligns with the new webhook behavior described in the PR objectives.
38-44: Verify impact on existing deployments.
This represents a significant behavioral change where webhooks will no longer trigger for all methods by default. This might affect existing deployments that rely on the previous behavior where an empty method list meant "allow all".
Let's check for potential impacts:
✅ Verification successful
Let me gather more information about the webhook behavior and its changes.
Let me check the changelog and previous behavior to understand if this is a documented change.
Based on the gathered information, I can now provide the final response:
The change in webhook behavior is safe and as designed
The current implementation is consistent with the documented behavior and previous changes. From the changelog entries, we can see that webhook authorization was intentionally designed with explicit method specification:
- The feature was initially added with basic behavior (v0.1.5)
- Authorization webhook methods were explicitly added as a configurable flag (pre-0.2.0)
- The behavior has been stable since these initial implementations
The test case correctly verifies that an empty method list means "deny all" rather than "allow all", which is a more secure default that requires explicit method specification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for existing webhook configurations that might be affected by this change
# Look for webhook-related configuration files
fd -e yaml -e json -e toml | xargs rg -l 'webhook|auth.*url'
# Look for webhook URL configurations in code
rg -A 3 'AuthWebhookURL.*=|SetAuthWebhookURL|authWebhookURL'
# Look for tests that might need updates
rg -A 5 'TestAuth.*Webhook|webhook.*test'
Length of output: 10306
Script:
#!/bin/bash
# Look for the implementation of RequireAuth and its changes
ast-grep --pattern 'func (p *Project) RequireAuth($_) bool {
$$$
}'
# Look for webhook-related configuration in the project
rg -l "AuthWebhookMethods.*empty"
# Check for documentation about webhook configuration
fd README.md | xargs rg -l "webhook"
# Look for any migration scripts or upgrade notes
fd -e md | xargs rg -l "upgrade|migration|breaking.change"
Length of output: 1165
Script:
#!/bin/bash
# Check the changelog for any mentions of webhook changes
rg -A 5 -B 5 "webhook" CHANGELOG.md
# Look at previous implementation through git
git log -p api/types/project.go
# Check if this is documented in API docs
rg "webhook" api/docs/yorkie/v1/resources.openapi.yaml
Length of output: 1419
api/types/project.go (1)
65-65:
This change reverses the default authorization behavior when webhook URL is set but methods are empty. Previously, this would require authorization for all methods, but now it requires no authorization. This is a breaking change that could have security implications.
Consider the following recommendations:
- Add a migration guide for existing projects
- Update documentation to clearly explain the new behavior
- Consider adding a deprecation period where you warn about the upcoming behavior change
- Add logging to help detect affected projects
Let's verify the impact and identify affected projects:
cmd/yorkie/project/update.go (2)
131-131: LGTM!
The addition of AuthWebhookMethods to the updatable fields follows the existing pattern and is implemented correctly.
196-207: LGTM! Verify command help text.
The flag definitions are clear and follow the Docker CLI patterns as intended in the PR objectives.
Let's verify the command help text is properly registered:
✅ Verification successful
Flag definitions are properly implemented and registered
The code inspection confirms that:
- The flags are correctly defined and registered in the
init()function - The flag descriptions are clear and consistent
- The flags are properly handled in the command's RunE function
- The 'ALL' keyword functionality is implemented as described
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the help text for the update command flags
# Build the help text and check for the new flags
go run ./cmd/yorkie/main.go project update --help 2>&1 | grep -A 2 "auth-webhook-method"
Length of output: 83
Script:
#!/bin/bash
# Let's verify the command structure and help text using different approaches
# Check if the update command is registered and its flags
rg -A 5 "project update" cmd/
# Look for the flag registration in the update command
ast-grep --pattern 'func NewUpdateCommand($$$) *cobra.Command {
$$$
}'
# Check for any test files that might verify the flags
fd -e go -E vendor test.*update
Length of output: 520
Script:
#!/bin/bash
# Let's examine the update command implementation more closely
# Look at the entire update.go file to understand the command structure
cat cmd/yorkie/project/update.go
# Search for any cobra.Command initialization patterns
ast-grep --pattern 'cmd := &cobra.Command{
$$$
}'
Length of output: 5686
test/integration/auth_webhook_test.go (1)
Line range hint 1-677: LGTM! Comprehensive test coverage for webhook method configuration
The test file has been thoroughly updated to verify the new webhook behavior where methods must be explicitly configured. It includes comprehensive test coverage for:
- Default behavior with all methods
- Selected method authorization
- Error handling
- Caching behavior
- Token management
The test cases are well-structured and effectively verify the changes described in the PR objectives.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1082 +/- ##
=======================================
Coverage 46.79% 46.79%
=======================================
Files 84 84
Lines 12219 12219
=======================================
Hits 5718 5718
Misses 5928 5928
Partials 573 573 ☔ View full report in Codecov by Sentry. |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Related yorkie-team/dashboard#175 (comment)
Special notes for your reviewer:
docker service updateALLkeyword: refStringArrayVarinstead ofStringSliceVarfor method flags: Idea: support repeated flags to allow lists spf13/cobra#661--auth-webhook-method-addfor each methodDoes this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests