feat(acl): added ext/acl to implement Access Control List with hosts/ipnets/countries
#48
Conversation
Reviewer's Guide by SourceryThis pull request introduces an Access Control List (ACL) middleware, enhances request logging, refactors text viewer and template initialization, updates context to use Updated class diagram for TextTemplate and TextViewerclassDiagram
class TextTemplate {
-template *template.Template
}
class TextViewer {
-template *TextTemplate
}
TextViewer -- TextTemplate : has
note for TextTemplate "Represents a text template that can be loaded from a file system and executed with data."
note for TextViewer "Holds an TextTemplate and is used to render text content."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding some unit tests for the new
ext/aclpackage to ensure its functionality and correctness. - The
WithConfigfunction inext/acl/option.gocould benefit from error handling when parsing the config file.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Here's the code health analysis summary for commits Analysis Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
==========================================
+ Coverage 93.10% 93.98% +0.88%
==========================================
Files 49 54 +5
Lines 1899 2196 +297
==========================================
+ Hits 1768 2064 +296
- Misses 97 98 +1
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding comments to the
aclpackage to explain the purpose and usage of its functions and types. - The
aclpackage introduces a config file, so it would be helpful to provide an example config file in the documentation.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more comprehensive error handling, especially around IP parsing and country code lookups.
- The ACL middleware could benefit from a mechanism to disable it entirely via a configuration option.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }) | ||
| } | ||
|
|
||
| func TestIPNets(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Edge case tests for IPNets with invalid formats.
Add tests to handle scenarios with invalid IP network formats to ensure proper error handling.
| }) | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Test case for countries with empty lookup function.
It would be beneficial to add a test case where the lookup function is nil or returns an empty string to ensure the middleware handles such scenarios gracefully.
| }) | |
| } | |
| }) | |
| t.Run("empty_lookup", func(t *testing.T) { | |
| emptyLookup := func(ip string) string { | |
| return "" | |
| } | |
| m := New(WithLookupFunc(emptyLookup), AllowCountries("cn")) | |
| ctx := createContext(nil) | |
| ctx.Request.RemoteAddr = "172.0.0.2:2222" | |
| err := m(nop)(ctx) | |
| require.Error(t, err) | |
| require.Equal(t, http.StatusForbidden, ctx.Response.StatusCode()) | |
| }) | |
| t.Run("nil_lookup", func(t *testing.T) { | |
| // Passing a nil lookup function to ensure the middleware handles it gracefully. | |
| m := New(WithLookupFunc(nil), AllowCountries("cn")) | |
| ctx := createContext(nil) | |
| ctx.Request.RemoteAddr = "172.0.0.2:2222" | |
| err := m(nop)(ctx) | |
| require.Error(t, err) | |
| require.Equal(t, http.StatusForbidden, ctx.Response.StatusCode()) | |
| }) | |
| } |
README.md
Outdated
| @@ -698,7 +695,95 @@ func main(){ | |||
| ``` | |||
|
|
|||
|
|
|||
| #### Access Control List(ACL ) | |||
There was a problem hiding this comment.
issue (typo): Typo: Remove space before parenthesis in "Access Control List(ACL )"
It should be "Access Control List (ACL)".
| #### Access Control List(ACL ) | |
| #### Access Control List (ACL) |
Changed
Fixed
Added
ext/aclto implement Access Control List with hosts/ipnets/countriesTests
Tasks to complete before merging PR:
make unit-testto check for any regressions 📋make lintto check for any issuesSummary by Sourcery
Implement ACL middleware to filter requests based on host, IP, or country. Add a new log format
VCombinedand support loading rules from a config file. Escape values from the client in request logs.New Features:
Bug Fixes:
Enhancements:
VCombinedlog format which includes virtual hostDocumentation:
Tests: