🔥 Feature: Add support for custom KeyLookup functions in the Keyauth middleware #3028
Conversation
Signed-off-by: Dave Lee <[email protected]>
dave-gray101
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe key changes involved refactoring the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Config
participant KeySource
participant CustomKeyLookup
Client->>Middleware: Request with key
Middleware->>Config: Fetch configuration
Config-->>Middleware: Return configuration
alt Custom Key Lookup Enabled
Middleware->>CustomKeyLookup: Fetch key
CustomKeyLookup-->>Middleware: Return key
else
Middleware->>KeySource: Fetch key from source
KeySource-->>Middleware: Return key
end
Middleware-->>Client: Process request/Respond
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 2
Outside diff range and nitpick comments (1)
middleware/keyauth/config.go (1)
35-37: Ensure proper documentation forFallbackKeyLookupsto clarify its usage.Consider adding a comment explaining the expected format and use cases for
FallbackKeyLookupsto help developers understand how to use this new configuration option effectively.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- docs/middleware/keyauth.md (2 hunks)
- middleware/keyauth/config.go (3 hunks)
- middleware/keyauth/keyauth.go (3 hunks)
- middleware/keyauth/keyauth_test.go (1 hunks)
Additional context used
Learnings (1)
docs/middleware/keyauth.md (1)
User: dave-gray101 PR: gofiber/fiber#3027 File: docs/api/middleware/keyauth.md:222-222 Timestamp: 2024-06-09T18:50:02.075Z Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
golangci-lint
middleware/keyauth/keyauth.go
101-101: File is not
gofumpt-ed with-extra(gofumpt)
104-104: fmt.Errorf can be replaced with errors.New (perfsprint)
middleware/keyauth/keyauth_test.go
[warning] 144-144: unused-parameter: parameter 'c' seems to be unused, consider removing or renaming it as _ (revive)
LanguageTool
docs/middleware/keyauth.md
[uncategorized] ~7-~7: This expression is usually spelled with a hyphen. (BASED_HYPHEN)
Context: ...Keyauth Key auth middleware provides a key based authentication. ## Signatures ```go f...
[uncategorized] ~78-~78: The abbreviation “e.g.” (= for example) requires two periods. (E_G)
Context: ...of keyauth and apply a filter function (eg.authFilter) like so ```go package ma...
Markdownlint
docs/middleware/keyauth.md
22-22: Column: 1 (MD010, no-hard-tabs)
Hard tabs
23-23: Column: 1 (MD010, no-hard-tabs)
Hard tabs
24-24: Column: 1 (MD010, no-hard-tabs)
Hard tabs
25-25: Column: 1 (MD010, no-hard-tabs)
Hard tabs
29-29: Column: 1 (MD010, no-hard-tabs)
Hard tabs
33-33: Column: 1 (MD010, no-hard-tabs)
Hard tabs
34-34: Column: 1 (MD010, no-hard-tabs)
Hard tabs
36-36: Column: 1 (MD010, no-hard-tabs)
Hard tabs
37-37: Column: 1 (MD010, no-hard-tabs)
Hard tabs
38-38: Column: 1 (MD010, no-hard-tabs)
Hard tabs
39-39: Column: 1 (MD010, no-hard-tabs)
Hard tabs
43-43: Column: 1 (MD010, no-hard-tabs)
Hard tabs
45-45: Column: 1 (MD010, no-hard-tabs)
Hard tabs
46-46: Column: 1 (MD010, no-hard-tabs)
Hard tabs
47-47: Column: 1 (MD010, no-hard-tabs)
Hard tabs
48-48: Column: 1 (MD010, no-hard-tabs)
Hard tabs
49-49: Column: 1 (MD010, no-hard-tabs)
Hard tabs
51-51: Column: 1 (MD010, no-hard-tabs)
Hard tabs
52-52: Column: 1 (MD010, no-hard-tabs)
Hard tabs
53-53: Column: 1 (MD010, no-hard-tabs)
Hard tabs
55-55: Column: 1 (MD010, no-hard-tabs)
Hard tabs
84-84: Column: 1 (MD010, no-hard-tabs)
Hard tabs
85-85: Column: 1 (MD010, no-hard-tabs)
Hard tabs
86-86: Column: 1 (MD010, no-hard-tabs)
Hard tabs
87-87: Column: 1 (MD010, no-hard-tabs)
Hard tabs
88-88: Column: 1 (MD010, no-hard-tabs)
Hard tabs
89-89: Column: 1 (MD010, no-hard-tabs)
Hard tabs
93-93: Column: 1 (MD010, no-hard-tabs)
Hard tabs
94-94: Column: 1 (MD010, no-hard-tabs)
Hard tabs
95-95: Column: 1 (MD010, no-hard-tabs)
Hard tabs
96-96: Column: 1 (MD010, no-hard-tabs)
Hard tabs
97-97: Column: 1 (MD010, no-hard-tabs)
Hard tabs
101-101: Column: 1 (MD010, no-hard-tabs)
Hard tabs
102-102: Column: 1 (MD010, no-hard-tabs)
Hard tabs
104-104: Column: 1 (MD010, no-hard-tabs)
Hard tabs
105-105: Column: 1 (MD010, no-hard-tabs)
Hard tabs
106-106: Column: 1 (MD010, no-hard-tabs)
Hard tabs
107-107: Column: 1 (MD010, no-hard-tabs)
Hard tabs
111-111: Column: 1 (MD010, no-hard-tabs)
Hard tabs
113-113: Column: 1 (MD010, no-hard-tabs)
Hard tabs
114-114: Column: 1 (MD010, no-hard-tabs)
Hard tabs
115-115: Column: 1 (MD010, no-hard-tabs)
Hard tabs
116-116: Column: 1 (MD010, no-hard-tabs)
Hard tabs
117-117: Column: 1 (MD010, no-hard-tabs)
Hard tabs
118-118: Column: 1 (MD010, no-hard-tabs)
Hard tabs
122-122: Column: 1 (MD010, no-hard-tabs)
Hard tabs
124-124: Column: 1 (MD010, no-hard-tabs)
Hard tabs
125-125: Column: 1 (MD010, no-hard-tabs)
Hard tabs
126-126: Column: 1 (MD010, no-hard-tabs)
Hard tabs
127-127: Column: 1 (MD010, no-hard-tabs)
Hard tabs
128-128: Column: 1 (MD010, no-hard-tabs)
Hard tabs
130-130: Column: 1 (MD010, no-hard-tabs)
Hard tabs
131-131: Column: 1 (MD010, no-hard-tabs)
Hard tabs
132-132: Column: 1 (MD010, no-hard-tabs)
Hard tabs
133-133: Column: 1 (MD010, no-hard-tabs)
Hard tabs
134-134: Column: 1 (MD010, no-hard-tabs)
Hard tabs
135-135: Column: 1 (MD010, no-hard-tabs)
Hard tabs
136-136: Column: 1 (MD010, no-hard-tabs)
Hard tabs
137-137: Column: 1 (MD010, no-hard-tabs)
Hard tabs
138-138: Column: 1 (MD010, no-hard-tabs)
Hard tabs
140-140: Column: 1 (MD010, no-hard-tabs)
Hard tabs
166-166: Column: 1 (MD010, no-hard-tabs)
Hard tabs
167-167: Column: 1 (MD010, no-hard-tabs)
Hard tabs
168-168: Column: 1 (MD010, no-hard-tabs)
Hard tabs
169-169: Column: 1 (MD010, no-hard-tabs)
Hard tabs
177-177: Column: 1 (MD010, no-hard-tabs)
Hard tabs
179-179: Column: 1 (MD010, no-hard-tabs)
Hard tabs
180-180: Column: 1 (MD010, no-hard-tabs)
Hard tabs
181-181: Column: 1 (MD010, no-hard-tabs)
Hard tabs
182-182: Column: 1 (MD010, no-hard-tabs)
Hard tabs
184-184: Column: 1 (MD010, no-hard-tabs)
Hard tabs
185-185: Column: 1 (MD010, no-hard-tabs)
Hard tabs
186-186: Column: 1 (MD010, no-hard-tabs)
Hard tabs
187-187: Column: 1 (MD010, no-hard-tabs)
Hard tabs
188-188: Column: 1 (MD010, no-hard-tabs)
Hard tabs
189-189: Column: 1 (MD010, no-hard-tabs)
Hard tabs
191-191: Column: 1 (MD010, no-hard-tabs)
Hard tabs
192-192: Column: 1 (MD010, no-hard-tabs)
Hard tabs
193-193: Column: 1 (MD010, no-hard-tabs)
Hard tabs
195-195: Column: 1 (MD010, no-hard-tabs)
Hard tabs
196-196: Column: 1 (MD010, no-hard-tabs)
Hard tabs
197-197: Column: 1 (MD010, no-hard-tabs)
Hard tabs
199-199: Column: 1 (MD010, no-hard-tabs)
Hard tabs
231-231: Column: 1 (MD010, no-hard-tabs)
Hard tabs
232-232: Column: 1 (MD010, no-hard-tabs)
Hard tabs
233-233: Column: 1 (MD010, no-hard-tabs)
Hard tabs
234-234: Column: 1 (MD010, no-hard-tabs)
Hard tabs
235-235: Column: 1 (MD010, no-hard-tabs)
Hard tabs
236-236: Column: 1 (MD010, no-hard-tabs)
Hard tabs
237-237: Column: 1 (MD010, no-hard-tabs)
Hard tabs
238-238: Column: 1 (MD010, no-hard-tabs)
Hard tabs
239-239: Column: 1 (MD010, no-hard-tabs)
Hard tabs
240-240: Column: 1 (MD010, no-hard-tabs)
Hard tabs
241-241: Column: 1 (MD010, no-hard-tabs)
Hard tabs
242-242: Column: 1 (MD010, no-hard-tabs)
Hard tabs
75-75: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
Additional comments not posted (6)
middleware/keyauth/config.go (2)
58-60: Proper initialization ofFallbackKeyLookupsinConfigDefault.The initialization of
FallbackKeyLookupsas an empty slice in the default configuration is appropriate, ensuring that there are no null pointer exceptions when this field is accessed.
87-89: Good defensive programming practice inconfigDefault.Setting
FallbackKeyLookupsto an empty slice if it isnilin the provided configuration is a good defensive programming practice, ensuring stability and preventing runtime errors.middleware/keyauth/keyauth.go (2)
6-6: Import offmtpackage is justified.The import of the
fmtpackage is necessary for error formatting in the new implementation, which usesfmt.Errorfto wrap errors with additional context.
32-33: Introduction ofextractorFunctype.The introduction of the
extractorFunctype is a good design choice, encapsulating the key extraction logic into a type, which enhances modularity and readability.docs/middleware/keyauth.md (1)
223-223: Documentation update forFallbackKeyLookups.The documentation update to include
FallbackKeyLookupsin theConfigstruct description is clear and aligns with the code changes. This ensures that users are well-informed about the new configuration option.middleware/keyauth/keyauth_test.go (1)
133-177: Comprehensive testing for multiple key lookups.The new test function
TestMultipleKeyLookupis comprehensive and effectively tests the new multiple key lookup feature. It covers various scenarios and properly asserts the outcomes, ensuring the functionality works as expected.Tools
golangci-lint
[warning] 144-144: unused-parameter: parameter 'c' seems to be unused, consider removing or renaming it as _ (revive)
Signed-off-by: Dave Lee <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
middleware/keyauth/keyauth.go (1)
101-101: Format the file withgofumptto adhere to Go best practices.Tools
golangci-lint
101-101: File is not
gofumpt-ed with-extra(gofumpt)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/keyauth/keyauth.go (3 hunks)
Additional context used
golangci-lint
middleware/keyauth/keyauth.go
101-101: File is not
gofumpt-ed with-extra(gofumpt)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3028 +/- ##
==========================================
+ Coverage 82.85% 82.96% +0.11%
==========================================
Files 115 115
Lines 8224 8248 +24
==========================================
+ Hits 6814 6843 +29
+ Misses 1080 1075 -5
Partials 330 330
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@dave-gray101 The linter is more strict in v3. On the root of the repo there's a makefile that helps you run some of the tests locally. More info here: https://github.com/gofiber/fiber?tab=readme-ov-file#%EF%B8%8F-development |
|
I'm not entirely convinced that the current approach is right for a general framework. While I appreciate the effort behind the proposed changes, I'm concerned about the added complexity and potential performance impact. Before moving forward with this pull request, could we consider alternative solutions? One idea that comes to mind is implementing a custom @dave-gray101 what do you think about incorporating a custom |
Signed-off-by: Dave Lee <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
middleware/keyauth/keyauth.go (1)
6-6: Consider removing thefmtimport if it's not used elsewhere in the file.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- middleware/keyauth/keyauth.go (3 hunks)
- middleware/keyauth/keyauth_test.go (1 hunks)
Additional context used
golangci-lint
Additional comments not posted (2)
middleware/keyauth/keyauth.go (1)
32-32: TheextractorFunctype is well-defined and clear.middleware/keyauth/keyauth_test.go (1)
133-177: TheTestMultipleKeyLookupfunction effectively tests the new functionality of handling multiple key lookups. It's comprehensive and covers various scenarios. Good job on ensuring thorough testing!
I had considered that - initially, my plan was to change as little as possible, as this was targeted at the v2 branch at that time. Now that I'm targeting the v3 (which will have other, more pressing breaking changes for developers to worry about)... I feel more comfortable doing something like a custom Now that I've had a chance to test out the makefiles for local linting / testing on v3, I'll see what that might look like - I think it might be nice to have a utility function in there that can still parse the format and generate a handler like this. |
… as function, with utility functions to make creating these easy Signed-off-by: Dave Lee <[email protected]>
|
I've gone ahead and swapped this over to use I've exposed the existing functionality that parses |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- docs/middleware/keyauth.md (2 hunks)
- middleware/keyauth/config.go (3 hunks)
- middleware/keyauth/keyauth.go (4 hunks)
- middleware/keyauth/keyauth_test.go (1 hunks)
Additional context used
Learnings (1)
docs/middleware/keyauth.md (1)
User: dave-gray101 PR: gofiber/fiber#3027 File: docs/api/middleware/keyauth.md:222-222 Timestamp: 2024-06-09T18:50:02.075Z Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
golangci-lint
LanguageTool
docs/middleware/keyauth.md
[uncategorized] ~7-~7: This expression is usually spelled with a hyphen. (BASED_HYPHEN)
Context: ...Keyauth Key auth middleware provides a key based authentication. ## Signatures ```go f...
[uncategorized] ~78-~78: The abbreviation “e.g.” (= for example) requires two periods. (E_G)
Context: ...of keyauth and apply a filter function (eg.authFilter) like so ```go package ma...
[uncategorized] ~250-~250: Use a comma before “or” if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ... the above function until a key is found or the options are all exhausted
Markdownlint
docs/middleware/keyauth.md
22-22: Column: 1 (MD010, no-hard-tabs)
Hard tabs
23-23: Column: 1 (MD010, no-hard-tabs)
Hard tabs
24-24: Column: 1 (MD010, no-hard-tabs)
Hard tabs
25-25: Column: 1 (MD010, no-hard-tabs)
Hard tabs
29-29: Column: 1 (MD010, no-hard-tabs)
Hard tabs
33-33: Column: 1 (MD010, no-hard-tabs)
Hard tabs
34-34: Column: 1 (MD010, no-hard-tabs)
Hard tabs
36-36: Column: 1 (MD010, no-hard-tabs)
Hard tabs
37-37: Column: 1 (MD010, no-hard-tabs)
Hard tabs
38-38: Column: 1 (MD010, no-hard-tabs)
Hard tabs
39-39: Column: 1 (MD010, no-hard-tabs)
Hard tabs
43-43: Column: 1 (MD010, no-hard-tabs)
Hard tabs
45-45: Column: 1 (MD010, no-hard-tabs)
Hard tabs
46-46: Column: 1 (MD010, no-hard-tabs)
Hard tabs
47-47: Column: 1 (MD010, no-hard-tabs)
Hard tabs
48-48: Column: 1 (MD010, no-hard-tabs)
Hard tabs
49-49: Column: 1 (MD010, no-hard-tabs)
Hard tabs
51-51: Column: 1 (MD010, no-hard-tabs)
Hard tabs
52-52: Column: 1 (MD010, no-hard-tabs)
Hard tabs
53-53: Column: 1 (MD010, no-hard-tabs)
Hard tabs
55-55: Column: 1 (MD010, no-hard-tabs)
Hard tabs
84-84: Column: 1 (MD010, no-hard-tabs)
Hard tabs
85-85: Column: 1 (MD010, no-hard-tabs)
Hard tabs
86-86: Column: 1 (MD010, no-hard-tabs)
Hard tabs
87-87: Column: 1 (MD010, no-hard-tabs)
Hard tabs
88-88: Column: 1 (MD010, no-hard-tabs)
Hard tabs
89-89: Column: 1 (MD010, no-hard-tabs)
Hard tabs
93-93: Column: 1 (MD010, no-hard-tabs)
Hard tabs
94-94: Column: 1 (MD010, no-hard-tabs)
Hard tabs
95-95: Column: 1 (MD010, no-hard-tabs)
Hard tabs
96-96: Column: 1 (MD010, no-hard-tabs)
Hard tabs
97-97: Column: 1 (MD010, no-hard-tabs)
Hard tabs
101-101: Column: 1 (MD010, no-hard-tabs)
Hard tabs
102-102: Column: 1 (MD010, no-hard-tabs)
Hard tabs
104-104: Column: 1 (MD010, no-hard-tabs)
Hard tabs
105-105: Column: 1 (MD010, no-hard-tabs)
Hard tabs
106-106: Column: 1 (MD010, no-hard-tabs)
Hard tabs
107-107: Column: 1 (MD010, no-hard-tabs)
Hard tabs
111-111: Column: 1 (MD010, no-hard-tabs)
Hard tabs
113-113: Column: 1 (MD010, no-hard-tabs)
Hard tabs
114-114: Column: 1 (MD010, no-hard-tabs)
Hard tabs
115-115: Column: 1 (MD010, no-hard-tabs)
Hard tabs
116-116: Column: 1 (MD010, no-hard-tabs)
Hard tabs
117-117: Column: 1 (MD010, no-hard-tabs)
Hard tabs
118-118: Column: 1 (MD010, no-hard-tabs)
Hard tabs
122-122: Column: 1 (MD010, no-hard-tabs)
Hard tabs
124-124: Column: 1 (MD010, no-hard-tabs)
Hard tabs
125-125: Column: 1 (MD010, no-hard-tabs)
Hard tabs
126-126: Column: 1 (MD010, no-hard-tabs)
Hard tabs
127-127: Column: 1 (MD010, no-hard-tabs)
Hard tabs
128-128: Column: 1 (MD010, no-hard-tabs)
Hard tabs
130-130: Column: 1 (MD010, no-hard-tabs)
Hard tabs
131-131: Column: 1 (MD010, no-hard-tabs)
Hard tabs
132-132: Column: 1 (MD010, no-hard-tabs)
Hard tabs
133-133: Column: 1 (MD010, no-hard-tabs)
Hard tabs
134-134: Column: 1 (MD010, no-hard-tabs)
Hard tabs
135-135: Column: 1 (MD010, no-hard-tabs)
Hard tabs
136-136: Column: 1 (MD010, no-hard-tabs)
Hard tabs
137-137: Column: 1 (MD010, no-hard-tabs)
Hard tabs
138-138: Column: 1 (MD010, no-hard-tabs)
Hard tabs
140-140: Column: 1 (MD010, no-hard-tabs)
Hard tabs
166-166: Column: 1 (MD010, no-hard-tabs)
Hard tabs
167-167: Column: 1 (MD010, no-hard-tabs)
Hard tabs
168-168: Column: 1 (MD010, no-hard-tabs)
Hard tabs
169-169: Column: 1 (MD010, no-hard-tabs)
Hard tabs
177-177: Column: 1 (MD010, no-hard-tabs)
Hard tabs
179-179: Column: 1 (MD010, no-hard-tabs)
Hard tabs
180-180: Column: 1 (MD010, no-hard-tabs)
Hard tabs
181-181: Column: 1 (MD010, no-hard-tabs)
Hard tabs
182-182: Column: 1 (MD010, no-hard-tabs)
Hard tabs
184-184: Column: 1 (MD010, no-hard-tabs)
Hard tabs
185-185: Column: 1 (MD010, no-hard-tabs)
Hard tabs
186-186: Column: 1 (MD010, no-hard-tabs)
Hard tabs
187-187: Column: 1 (MD010, no-hard-tabs)
Hard tabs
188-188: Column: 1 (MD010, no-hard-tabs)
Hard tabs
189-189: Column: 1 (MD010, no-hard-tabs)
Hard tabs
191-191: Column: 1 (MD010, no-hard-tabs)
Hard tabs
192-192: Column: 1 (MD010, no-hard-tabs)
Hard tabs
193-193: Column: 1 (MD010, no-hard-tabs)
Hard tabs
195-195: Column: 1 (MD010, no-hard-tabs)
Hard tabs
196-196: Column: 1 (MD010, no-hard-tabs)
Hard tabs
197-197: Column: 1 (MD010, no-hard-tabs)
Hard tabs
199-199: Column: 1 (MD010, no-hard-tabs)
Hard tabs
231-231: Column: 1 (MD010, no-hard-tabs)
Hard tabs
232-232: Column: 1 (MD010, no-hard-tabs)
Hard tabs
233-233: Column: 1 (MD010, no-hard-tabs)
Hard tabs
234-234: Column: 1 (MD010, no-hard-tabs)
Hard tabs
235-235: Column: 1 (MD010, no-hard-tabs)
Hard tabs
236-236: Column: 1 (MD010, no-hard-tabs)
Hard tabs
237-237: Column: 1 (MD010, no-hard-tabs)
Hard tabs
238-238: Column: 1 (MD010, no-hard-tabs)
Hard tabs
239-239: Column: 1 (MD010, no-hard-tabs)
Hard tabs
240-240: Column: 1 (MD010, no-hard-tabs)
Hard tabs
241-241: Column: 1 (MD010, no-hard-tabs)
Hard tabs
242-242: Column: 1 (MD010, no-hard-tabs)
Hard tabs
75-75: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
249-249: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
Additional comments not posted (10)
middleware/keyauth/config.go (3)
9-9: Introduced a new typeKeyauthKeyLookupFuncfor custom key lookup functions.This is a good addition as it allows for more flexible and complex key extraction logic, aligning with the PR's goal to enhance key lookup capabilities.
37-37: AddedCustomKeyLookupfield to theConfigstruct.This change supports the new feature of custom key lookups, providing users the ability to specify their own key extraction logic. Ensure that the documentation is updated to reflect this new configuration option.
58-60: Updated default values forKeyLookupand introduced a defaultnilforCustomKeyLookup.The update to the default
KeyLookupto include thefiber.HeaderAuthorizationis a sensible default. SettingCustomKeyLookuptonilby default is appropriate as it maintains backward compatibility.middleware/keyauth/keyauth.go (5)
6-6: Imported thefmtpackage.This import is necessary for error formatting in the new functions introduced in this file.
38-43: Implemented a fallback mechanism forCustomKeyLookupusingSingleKeyLookup.This implementation ensures that if
CustomKeyLookupis not provided, the middleware defaults to a single key lookup based on the providedKeyLookupandAuthScheme. The use ofpanicfor error handling here is appropriate given that configuration errors should be caught early.
54-54: Enhanced key extraction and validation logic within the middleware handler.This change effectively integrates the
CustomKeyLookupfunctionality into the middleware's request handling, allowing for more flexible authentication mechanisms.
79-102: AddedMultipleKeySourceLookupfunction to handle multiple key sources.This function is a significant enhancement, allowing the middleware to attempt key extraction from multiple specified sources until one succeeds. This aligns with the PR's objective to support multiple key sources.
104-121: RefactoredSingleKeyLookupto support different types of key sources.This refactor makes the function more modular and easier to maintain. It also supports the new feature of handling multiple key sources by providing a consistent interface for key extraction.
docs/middleware/keyauth.md (1)
223-223: Updated documentation to include the newCustomKeyLookupfield in theConfigstruct.The documentation update is thorough and correctly describes the new functionality. It's crucial that the documentation keeps pace with the code changes to help users understand how to use the new features.
middleware/keyauth/keyauth_test.go (1)
133-181: AddedTestMultipleKeyLookupto validate the functionality ofMultipleKeySourceLookup.This test is well-structured and effectively checks the new functionality of handling multiple key sources. It's important to ensure that new features are accompanied by comprehensive tests to maintain code quality and functionality.
dave-gray101
changed the title
Signed-off-by: Dave Lee <[email protected]>
|
I added a few more tests to boost the coverage numbers, as CI flagged a few error paths I forgot to cover. Can someone please re-approve the workflow when they get a chance? Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
middleware/keyauth/keyauth_test.go (1)
144-155: Validate error messages for better test accuracy.In
TestCustomKeyUtilityFunctionErrors, the tests verify that an error occurs but do not check the content of the error message. It's beneficial to also validate the error message to ensure that the function fails for the expected reasons.require.EqualError(t, err, "expected error message")
Signed-off-by: Dave Lee <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
middleware/keyauth/keyauth.go (1)
6-6: Consider removing thefmtimport if it's not used elsewhere in the file.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- middleware/keyauth/keyauth.go (8 hunks)
- middleware/keyauth/keyauth_test.go (1 hunks)
Additional context used
golangci-lint
Additional comments not posted (3)
middleware/keyauth/keyauth.go (3)
79-102: The implementation ofMultipleKeySourceLookupis robust. However, consider handling errors more gracefully rather than usingpanic. This could improve the middleware's resilience and error reporting.- panic(fmt.Errorf("error creating middleware: invalid keyauth Config.KeyLookup: %w", err)) + return nil, fmt.Errorf("error creating middleware: invalid keyauth Config.KeyLookup: %w", err)Likely invalid or redundant comment.
124-124: Ensure that thekeyFromHeaderfunction correctly handles cases where theauthSchemeis not provided.Verification successful
The
keyFromHeaderfunction correctly handles cases where theauthSchemeis not provided by checking if the length ofauthSchemeis 0 and returning theauthheader directly.
middleware/keyauth/keyauth.go, lines 124-128Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `keyFromHeader` handles missing `authScheme`. # Test: Search for test cases covering missing `authScheme`. Expect: At least one test case. rg --type go $'keyFromHeader.*authScheme' tests/Length of output: 96
Script:
#!/bin/bash # Description: Verify that `keyFromHeader` handles missing `authScheme`. # Test: Search for test cases covering missing `authScheme`. Expect: At least one test case. rg --type go $'keyFromHeader' --glob '*_test.go'Length of output: 47
Script:
#!/bin/bash # Description: Find the implementation of `keyFromHeader` and analyze its logic. # Step 1: Search for the implementation of `keyFromHeader`. ast-grep --lang go --pattern 'func keyFromHeader($_, $_) $_' # Step 2: Search for any indirect references or usage of `keyFromHeader` in the codebase. rg 'keyFromHeader' --type goLength of output: 450
Script:
#!/bin/bash # Description: Extract and review the implementation of `keyFromHeader`. # Extract the implementation of `keyFromHeader` from `middleware/keyauth/keyauth.go`. sed -n '/func keyFromHeader(header, authScheme string) KeyauthKeyLookupFunc {/,/}/p' middleware/keyauth/keyauth.goLength of output: 331
54-54: Ensure that theCustomKeyLookupfunction is thoroughly tested to handle all possible error scenarios.
Signed-off-by: Dave Lee <[email protected]>
Signed-off-by: Dave Lee <[email protected]>
…101/fiber into feat-keyauth-fallback-keylookup
|
@gaby Would it be possible to get the workflow kicked off again? I punched in a few more tests for coverage today, so I think this time the bots will accept it. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/keyauth/keyauth_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/keyauth/keyauth_test.go
|
@coderabbitai full review |
Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- docs/middleware/keyauth.md (2 hunks)
- middleware/keyauth/config.go (3 hunks)
- middleware/keyauth/keyauth.go (8 hunks)
- middleware/keyauth/keyauth_test.go (2 hunks)
Additional context used
Learnings (1)
docs/middleware/keyauth.md (1)
User: dave-gray101 PR: gofiber/fiber#3027 File: docs/api/middleware/keyauth.md:222-222 Timestamp: 2024-06-09T18:50:02.075Z Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
GitHub Check: codecov/patch
middleware/keyauth/keyauth.go
[warning] 42-42: middleware/keyauth/keyauth.go#L42
Added line #L42 was not covered by tests
[warning] 97-97: middleware/keyauth/keyauth.go#L97
Added line #L97 was not covered by tests
LanguageTool
docs/middleware/keyauth.md
[uncategorized] ~7-~7: This expression is usually spelled with a hyphen. (BASED_HYPHEN)
Context: ...Keyauth Key auth middleware provides a key based authentication. ## Signatures ```go f...
[uncategorized] ~78-~78: The abbreviation “e.g.” (= for example) requires two periods. (E_G)
Context: ...of keyauth and apply a filter function (eg.authFilter) like so ```go package ma...
[uncategorized] ~250-~250: Use a comma before “or” if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ... the above function until a key is found or the options are all exhausted
Markdownlint
docs/middleware/keyauth.md
22-22: Column: 1 (MD010, no-hard-tabs)
Hard tabs
23-23: Column: 1 (MD010, no-hard-tabs)
Hard tabs
24-24: Column: 1 (MD010, no-hard-tabs)
Hard tabs
25-25: Column: 1 (MD010, no-hard-tabs)
Hard tabs
29-29: Column: 1 (MD010, no-hard-tabs)
Hard tabs
33-33: Column: 1 (MD010, no-hard-tabs)
Hard tabs
34-34: Column: 1 (MD010, no-hard-tabs)
Hard tabs
36-36: Column: 1 (MD010, no-hard-tabs)
Hard tabs
37-37: Column: 1 (MD010, no-hard-tabs)
Hard tabs
38-38: Column: 1 (MD010, no-hard-tabs)
Hard tabs
39-39: Column: 1 (MD010, no-hard-tabs)
Hard tabs
43-43: Column: 1 (MD010, no-hard-tabs)
Hard tabs
45-45: Column: 1 (MD010, no-hard-tabs)
Hard tabs
46-46: Column: 1 (MD010, no-hard-tabs)
Hard tabs
47-47: Column: 1 (MD010, no-hard-tabs)
Hard tabs
48-48: Column: 1 (MD010, no-hard-tabs)
Hard tabs
49-49: Column: 1 (MD010, no-hard-tabs)
Hard tabs
51-51: Column: 1 (MD010, no-hard-tabs)
Hard tabs
52-52: Column: 1 (MD010, no-hard-tabs)
Hard tabs
53-53: Column: 1 (MD010, no-hard-tabs)
Hard tabs
55-55: Column: 1 (MD010, no-hard-tabs)
Hard tabs
84-84: Column: 1 (MD010, no-hard-tabs)
Hard tabs
85-85: Column: 1 (MD010, no-hard-tabs)
Hard tabs
86-86: Column: 1 (MD010, no-hard-tabs)
Hard tabs
87-87: Column: 1 (MD010, no-hard-tabs)
Hard tabs
88-88: Column: 1 (MD010, no-hard-tabs)
Hard tabs
89-89: Column: 1 (MD010, no-hard-tabs)
Hard tabs
93-93: Column: 1 (MD010, no-hard-tabs)
Hard tabs
94-94: Column: 1 (MD010, no-hard-tabs)
Hard tabs
95-95: Column: 1 (MD010, no-hard-tabs)
Hard tabs
96-96: Column: 1 (MD010, no-hard-tabs)
Hard tabs
97-97: Column: 1 (MD010, no-hard-tabs)
Hard tabs
101-101: Column: 1 (MD010, no-hard-tabs)
Hard tabs
102-102: Column: 1 (MD010, no-hard-tabs)
Hard tabs
104-104: Column: 1 (MD010, no-hard-tabs)
Hard tabs
105-105: Column: 1 (MD010, no-hard-tabs)
Hard tabs
106-106: Column: 1 (MD010, no-hard-tabs)
Hard tabs
107-107: Column: 1 (MD010, no-hard-tabs)
Hard tabs
111-111: Column: 1 (MD010, no-hard-tabs)
Hard tabs
113-113: Column: 1 (MD010, no-hard-tabs)
Hard tabs
114-114: Column: 1 (MD010, no-hard-tabs)
Hard tabs
115-115: Column: 1 (MD010, no-hard-tabs)
Hard tabs
116-116: Column: 1 (MD010, no-hard-tabs)
Hard tabs
117-117: Column: 1 (MD010, no-hard-tabs)
Hard tabs
118-118: Column: 1 (MD010, no-hard-tabs)
Hard tabs
122-122: Column: 1 (MD010, no-hard-tabs)
Hard tabs
124-124: Column: 1 (MD010, no-hard-tabs)
Hard tabs
125-125: Column: 1 (MD010, no-hard-tabs)
Hard tabs
126-126: Column: 1 (MD010, no-hard-tabs)
Hard tabs
127-127: Column: 1 (MD010, no-hard-tabs)
Hard tabs
128-128: Column: 1 (MD010, no-hard-tabs)
Hard tabs
130-130: Column: 1 (MD010, no-hard-tabs)
Hard tabs
131-131: Column: 1 (MD010, no-hard-tabs)
Hard tabs
132-132: Column: 1 (MD010, no-hard-tabs)
Hard tabs
133-133: Column: 1 (MD010, no-hard-tabs)
Hard tabs
134-134: Column: 1 (MD010, no-hard-tabs)
Hard tabs
135-135: Column: 1 (MD010, no-hard-tabs)
Hard tabs
136-136: Column: 1 (MD010, no-hard-tabs)
Hard tabs
137-137: Column: 1 (MD010, no-hard-tabs)
Hard tabs
138-138: Column: 1 (MD010, no-hard-tabs)
Hard tabs
140-140: Column: 1 (MD010, no-hard-tabs)
Hard tabs
166-166: Column: 1 (MD010, no-hard-tabs)
Hard tabs
167-167: Column: 1 (MD010, no-hard-tabs)
Hard tabs
168-168: Column: 1 (MD010, no-hard-tabs)
Hard tabs
169-169: Column: 1 (MD010, no-hard-tabs)
Hard tabs
177-177: Column: 1 (MD010, no-hard-tabs)
Hard tabs
179-179: Column: 1 (MD010, no-hard-tabs)
Hard tabs
180-180: Column: 1 (MD010, no-hard-tabs)
Hard tabs
181-181: Column: 1 (MD010, no-hard-tabs)
Hard tabs
182-182: Column: 1 (MD010, no-hard-tabs)
Hard tabs
184-184: Column: 1 (MD010, no-hard-tabs)
Hard tabs
185-185: Column: 1 (MD010, no-hard-tabs)
Hard tabs
186-186: Column: 1 (MD010, no-hard-tabs)
Hard tabs
187-187: Column: 1 (MD010, no-hard-tabs)
Hard tabs
188-188: Column: 1 (MD010, no-hard-tabs)
Hard tabs
189-189: Column: 1 (MD010, no-hard-tabs)
Hard tabs
191-191: Column: 1 (MD010, no-hard-tabs)
Hard tabs
192-192: Column: 1 (MD010, no-hard-tabs)
Hard tabs
193-193: Column: 1 (MD010, no-hard-tabs)
Hard tabs
195-195: Column: 1 (MD010, no-hard-tabs)
Hard tabs
196-196: Column: 1 (MD010, no-hard-tabs)
Hard tabs
197-197: Column: 1 (MD010, no-hard-tabs)
Hard tabs
199-199: Column: 1 (MD010, no-hard-tabs)
Hard tabs
231-231: Column: 1 (MD010, no-hard-tabs)
Hard tabs
232-232: Column: 1 (MD010, no-hard-tabs)
Hard tabs
233-233: Column: 1 (MD010, no-hard-tabs)
Hard tabs
234-234: Column: 1 (MD010, no-hard-tabs)
Hard tabs
235-235: Column: 1 (MD010, no-hard-tabs)
Hard tabs
236-236: Column: 1 (MD010, no-hard-tabs)
Hard tabs
237-237: Column: 1 (MD010, no-hard-tabs)
Hard tabs
238-238: Column: 1 (MD010, no-hard-tabs)
Hard tabs
239-239: Column: 1 (MD010, no-hard-tabs)
Hard tabs
240-240: Column: 1 (MD010, no-hard-tabs)
Hard tabs
241-241: Column: 1 (MD010, no-hard-tabs)
Hard tabs
242-242: Column: 1 (MD010, no-hard-tabs)
Hard tabs
75-75: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
249-249: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
Additional comments not posted (10)
middleware/keyauth/config.go (3)
9-9: The new typeKeyauthKeyLookupFuncis well-defined and correctly encapsulates the expected functionality for a custom key lookup function. This type will facilitate the implementation of custom logic for key extraction from various parts of the request.
37-37: Adding theCustomKeyLookupfield to theConfigstruct is a crucial enhancement. It allows users to specify custom key extraction logic, which can be more flexible and powerful than predefined methods. Ensure that this new capability is well-documented and examples are provided to guide users on how to implement their custom functions effectively.
58-60: The default configuration is updated to include theCustomKeyLookupwith a default value ofnil. This change is consistent with the optional nature of the custom key lookup functionality. It's good practice to ensure that existing configurations without this field will still function as expected.middleware/keyauth/keyauth.go (3)
6-6: The import of thefmtpackage is appropriate given its usage in error formatting throughout the file. This is a necessary addition to support the new error handling strategies introduced in this update.
54-54: This line introduces a dynamic key lookup which enhances the flexibility of the middleware. However, ensure that this line is covered by unit tests to verify its functionality, especially in error scenarios.
97-97: The error handling withinMultipleKeySourceLookupis robust, but it lacks test coverage for the line where a formatted error is returned. It's important to test error conditions to ensure they are handled correctly.Verification successful
The test file
middleware/keyauth/keyauth_test.godoes include tests for error handling in theMultipleKeySourceLookupfunction, specifically in theTestCustomKeyUtilityFunctionErrorsandTestMultipleKeyLookupfunctions. These tests verify that errors are correctly handled and reported.
middleware/keyauth/keyauth_test.go:
TestCustomKeyUtilityFunctionErrorsTestMultipleKeyLookupScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for error handling in `MultipleKeySourceLookup`. # Test: Search for test coverage on line 97. Expect: Coverage report should include this line. codecov --file middleware/keyauth/keyauth.go --line 97Length of output: 131
Script:
#!/bin/bash # Locate the definition of the `MultipleKeySourceLookup` function in the codebase. ast-grep --lang go --pattern 'func MultipleKeySourceLookup($_) $_'Length of output: 68
Script:
#!/bin/bash # Refine the search to locate the definition of the `MultipleKeySourceLookup` function. rg 'func MultipleKeySourceLookup'Length of output: 166
Script:
#!/bin/bash # Search for test files that reference the `MultipleKeySourceLookup` function. rg 'MultipleKeySourceLookup' --glob '*_test.go'Length of output: 302
Script:
#!/bin/bash # Display the contents of the test file to verify error handling coverage. cat middleware/keyauth/keyauth_test.goLength of output: 16592
Tools
GitHub Check: codecov/patch
[warning] 97-97: middleware/keyauth/keyauth.go#L97
Added line #L97 was not covered by testsdocs/middleware/keyauth.md (1)
222-223: The documentation update correctly reflects the newCustomKeyLookupconfiguration option, providing clear information on its purpose and usage. It's essential to ensure that the documentation remains in sync with the code changes to help users understand how to utilize the new features effectively.middleware/keyauth/keyauth_test.go (3)
133-146: The test for handling invalid configurations correctly usesrequire.Panicsto ensure that the middleware behaves as expected in this scenario. Including a cleanup step in the deferred function is a good practice to ensure resources are properly released even when a test fails.
148-159: These tests effectively check for errors in custom key utility functions, which is crucial for ensuring the robustness of the middleware's error handling capabilities. It's good to see comprehensive testing around the new functionality.
161-218: The test forMultipleKeyLookupis well-implemented, covering various scenarios including success and failure cases. This thorough testing ensures that the middleware can handle multiple sources of authentication keys correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
docs/middleware/keyauth.md (1)
250-250: While the description ofMultipleKeySourceLookupis clear, the utility of this function could be enhanced by providing a practical example of its use in a real-world scenario.It would be helpful for users to see an example of `MultipleKeySourceLookup` in action. Consider adding a snippet that demonstrates its use, perhaps in combination with `SingleKeyLookup`, to handle multiple potential sources of authentication keys.Tools
LanguageTool
[uncategorized] ~250-~250: Use a comma before “or” if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ... the above function until a key is found or the options are all exhausted
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- docs/middleware/keyauth.md (2 hunks)
Additional context used
Learnings (1)
docs/middleware/keyauth.md (1)
User: dave-gray101 PR: gofiber/fiber#3027 File: docs/api/middleware/keyauth.md:222-222 Timestamp: 2024-06-09T18:50:02.075Z Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
LanguageTool
docs/middleware/keyauth.md
[uncategorized] ~7-~7: This expression is usually spelled with a hyphen. (BASED_HYPHEN)
Context: ...Keyauth Key auth middleware provides a key based authentication. ## Signatures ```go f...
[uncategorized] ~78-~78: The abbreviation “e.g.” (= for example) requires two periods. (E_G)
Context: ...of keyauth and apply a filter function (eg.authFilter) like so ```go package ma...
[uncategorized] ~250-~250: Use a comma before “or” if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ... the above function until a key is found or the options are all exhausted
Markdownlint
docs/middleware/keyauth.md
22-22: Column: 1 (MD010, no-hard-tabs)
Hard tabs
23-23: Column: 1 (MD010, no-hard-tabs)
Hard tabs
24-24: Column: 1 (MD010, no-hard-tabs)
Hard tabs
25-25: Column: 1 (MD010, no-hard-tabs)
Hard tabs
29-29: Column: 1 (MD010, no-hard-tabs)
Hard tabs
33-33: Column: 1 (MD010, no-hard-tabs)
Hard tabs
34-34: Column: 1 (MD010, no-hard-tabs)
Hard tabs
36-36: Column: 1 (MD010, no-hard-tabs)
Hard tabs
37-37: Column: 1 (MD010, no-hard-tabs)
Hard tabs
38-38: Column: 1 (MD010, no-hard-tabs)
Hard tabs
39-39: Column: 1 (MD010, no-hard-tabs)
Hard tabs
43-43: Column: 1 (MD010, no-hard-tabs)
Hard tabs
45-45: Column: 1 (MD010, no-hard-tabs)
Hard tabs
46-46: Column: 1 (MD010, no-hard-tabs)
Hard tabs
47-47: Column: 1 (MD010, no-hard-tabs)
Hard tabs
48-48: Column: 1 (MD010, no-hard-tabs)
Hard tabs
49-49: Column: 1 (MD010, no-hard-tabs)
Hard tabs
51-51: Column: 1 (MD010, no-hard-tabs)
Hard tabs
52-52: Column: 1 (MD010, no-hard-tabs)
Hard tabs
53-53: Column: 1 (MD010, no-hard-tabs)
Hard tabs
55-55: Column: 1 (MD010, no-hard-tabs)
Hard tabs
84-84: Column: 1 (MD010, no-hard-tabs)
Hard tabs
85-85: Column: 1 (MD010, no-hard-tabs)
Hard tabs
86-86: Column: 1 (MD010, no-hard-tabs)
Hard tabs
87-87: Column: 1 (MD010, no-hard-tabs)
Hard tabs
88-88: Column: 1 (MD010, no-hard-tabs)
Hard tabs
89-89: Column: 1 (MD010, no-hard-tabs)
Hard tabs
93-93: Column: 1 (MD010, no-hard-tabs)
Hard tabs
94-94: Column: 1 (MD010, no-hard-tabs)
Hard tabs
95-95: Column: 1 (MD010, no-hard-tabs)
Hard tabs
96-96: Column: 1 (MD010, no-hard-tabs)
Hard tabs
97-97: Column: 1 (MD010, no-hard-tabs)
Hard tabs
101-101: Column: 1 (MD010, no-hard-tabs)
Hard tabs
102-102: Column: 1 (MD010, no-hard-tabs)
Hard tabs
104-104: Column: 1 (MD010, no-hard-tabs)
Hard tabs
105-105: Column: 1 (MD010, no-hard-tabs)
Hard tabs
106-106: Column: 1 (MD010, no-hard-tabs)
Hard tabs
107-107: Column: 1 (MD010, no-hard-tabs)
Hard tabs
111-111: Column: 1 (MD010, no-hard-tabs)
Hard tabs
113-113: Column: 1 (MD010, no-hard-tabs)
Hard tabs
114-114: Column: 1 (MD010, no-hard-tabs)
Hard tabs
115-115: Column: 1 (MD010, no-hard-tabs)
Hard tabs
116-116: Column: 1 (MD010, no-hard-tabs)
Hard tabs
117-117: Column: 1 (MD010, no-hard-tabs)
Hard tabs
118-118: Column: 1 (MD010, no-hard-tabs)
Hard tabs
122-122: Column: 1 (MD010, no-hard-tabs)
Hard tabs
124-124: Column: 1 (MD010, no-hard-tabs)
Hard tabs
125-125: Column: 1 (MD010, no-hard-tabs)
Hard tabs
126-126: Column: 1 (MD010, no-hard-tabs)
Hard tabs
127-127: Column: 1 (MD010, no-hard-tabs)
Hard tabs
128-128: Column: 1 (MD010, no-hard-tabs)
Hard tabs
130-130: Column: 1 (MD010, no-hard-tabs)
Hard tabs
131-131: Column: 1 (MD010, no-hard-tabs)
Hard tabs
132-132: Column: 1 (MD010, no-hard-tabs)
Hard tabs
133-133: Column: 1 (MD010, no-hard-tabs)
Hard tabs
134-134: Column: 1 (MD010, no-hard-tabs)
Hard tabs
135-135: Column: 1 (MD010, no-hard-tabs)
Hard tabs
136-136: Column: 1 (MD010, no-hard-tabs)
Hard tabs
137-137: Column: 1 (MD010, no-hard-tabs)
Hard tabs
138-138: Column: 1 (MD010, no-hard-tabs)
Hard tabs
140-140: Column: 1 (MD010, no-hard-tabs)
Hard tabs
166-166: Column: 1 (MD010, no-hard-tabs)
Hard tabs
167-167: Column: 1 (MD010, no-hard-tabs)
Hard tabs
168-168: Column: 1 (MD010, no-hard-tabs)
Hard tabs
169-169: Column: 1 (MD010, no-hard-tabs)
Hard tabs
177-177: Column: 1 (MD010, no-hard-tabs)
Hard tabs
179-179: Column: 1 (MD010, no-hard-tabs)
Hard tabs
180-180: Column: 1 (MD010, no-hard-tabs)
Hard tabs
181-181: Column: 1 (MD010, no-hard-tabs)
Hard tabs
182-182: Column: 1 (MD010, no-hard-tabs)
Hard tabs
184-184: Column: 1 (MD010, no-hard-tabs)
Hard tabs
185-185: Column: 1 (MD010, no-hard-tabs)
Hard tabs
186-186: Column: 1 (MD010, no-hard-tabs)
Hard tabs
187-187: Column: 1 (MD010, no-hard-tabs)
Hard tabs
188-188: Column: 1 (MD010, no-hard-tabs)
Hard tabs
189-189: Column: 1 (MD010, no-hard-tabs)
Hard tabs
191-191: Column: 1 (MD010, no-hard-tabs)
Hard tabs
192-192: Column: 1 (MD010, no-hard-tabs)
Hard tabs
193-193: Column: 1 (MD010, no-hard-tabs)
Hard tabs
195-195: Column: 1 (MD010, no-hard-tabs)
Hard tabs
196-196: Column: 1 (MD010, no-hard-tabs)
Hard tabs
197-197: Column: 1 (MD010, no-hard-tabs)
Hard tabs
199-199: Column: 1 (MD010, no-hard-tabs)
Hard tabs
231-231: Column: 1 (MD010, no-hard-tabs)
Hard tabs
232-232: Column: 1 (MD010, no-hard-tabs)
Hard tabs
233-233: Column: 1 (MD010, no-hard-tabs)
Hard tabs
234-234: Column: 1 (MD010, no-hard-tabs)
Hard tabs
235-235: Column: 1 (MD010, no-hard-tabs)
Hard tabs
236-236: Column: 1 (MD010, no-hard-tabs)
Hard tabs
237-237: Column: 1 (MD010, no-hard-tabs)
Hard tabs
238-238: Column: 1 (MD010, no-hard-tabs)
Hard tabs
239-239: Column: 1 (MD010, no-hard-tabs)
Hard tabs
240-240: Column: 1 (MD010, no-hard-tabs)
Hard tabs
241-241: Column: 1 (MD010, no-hard-tabs)
Hard tabs
242-242: Column: 1 (MD010, no-hard-tabs)
Hard tabs
75-75: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
249-249: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
Signed-off-by: Dave Lee <[email protected]>
…101/fiber into feat-keyauth-fallback-keylookup
Signed-off-by: Dave Lee <[email protected]>
|
@sixcolors I've addressed your comments, thanks for the review! |
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
middleware/keyauth/keyauth.go (1)
[!TIP]
Codebase VerificationThe current implementation does not handle the scenario where
validis false buterris nil. This could indeed lead to unintended behavior where an invalid key is treated as valid.
- File:
middleware/keyauth/keyauth.go- Lines: 51-57
Category:
Analysis chain
Line range hint
54-61: Ensure that the key validation logic handles all error scenarios appropriately.The current implementation does not check for the scenario where
validis false buterris nil. This could lead to unintended behavior where an invalid key is treated as valid.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify all error handling paths in the key validation logic. grep -C 3 "cfg.Validator" middleware/keyauth/keyauth.goLength of output: 188
Script:
#!/bin/bash # Expand the context to gather more lines around the cfg.Validator call to see the complete error handling logic. grep -C 10 "cfg.Validator" middleware/keyauth/keyauth.goLength of output: 481
docs/middleware/keyauth.md (1)
223-223: Clarify the usage ofCustomKeyLookupin the documentation.The description of
CustomKeyLookupcould be enhanced by including a simple usage example directly in the documentation. This helps users understand how to implement custom key extraction functions.Consider adding a simple example of `CustomKeyLookup` here to illustrate its usage, which can help developers understand how to implement it effectively. For example: ```go func customKeyExtractor(c fiber.Ctx) (string, error) { // Custom logic to extract key return c.Cookies("custom_key"), nil }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- docs/middleware/keyauth.md (2 hunks)
- middleware/keyauth/config.go (3 hunks)
- middleware/keyauth/keyauth.go (8 hunks)
- middleware/keyauth/keyauth_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/keyauth/config.go
Additional context used
Learnings (1)
docs/middleware/keyauth.md (1)
User: dave-gray101 PR: gofiber/fiber#3027 File: docs/api/middleware/keyauth.md:222-222 Timestamp: 2024-06-09T18:50:02.075Z Learning: The `KeyLookup` field in the `keyauth` middleware does not support pipe-delimited keys. Instead, it specifies a single source and name for key extraction, with additional sources specified in the `FallbackKeyLookups` field.
LanguageTool
docs/middleware/keyauth.md
[uncategorized] ~7-~7: This expression is usually spelled with a hyphen. (BASED_HYPHEN)
Context: ...Keyauth Key auth middleware provides a key based authentication. ## Signatures ```go f...
[uncategorized] ~78-~78: The abbreviation “e.g.” (= for example) requires two periods. (E_G)
Context: ...of keyauth and apply a filter function (eg.authFilter) like so ```go package ma...
[uncategorized] ~250-~250: Use a comma before “or” if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ... the above function until a key is found or the options are all exhausted. For exam...
Markdownlint
docs/middleware/keyauth.md
22-22: Column: 1 (MD010, no-hard-tabs)
Hard tabs
23-23: Column: 1 (MD010, no-hard-tabs)
Hard tabs
24-24: Column: 1 (MD010, no-hard-tabs)
Hard tabs
25-25: Column: 1 (MD010, no-hard-tabs)
Hard tabs
29-29: Column: 1 (MD010, no-hard-tabs)
Hard tabs
33-33: Column: 1 (MD010, no-hard-tabs)
Hard tabs
34-34: Column: 1 (MD010, no-hard-tabs)
Hard tabs
36-36: Column: 1 (MD010, no-hard-tabs)
Hard tabs
37-37: Column: 1 (MD010, no-hard-tabs)
Hard tabs
38-38: Column: 1 (MD010, no-hard-tabs)
Hard tabs
39-39: Column: 1 (MD010, no-hard-tabs)
Hard tabs
43-43: Column: 1 (MD010, no-hard-tabs)
Hard tabs
45-45: Column: 1 (MD010, no-hard-tabs)
Hard tabs
46-46: Column: 1 (MD010, no-hard-tabs)
Hard tabs
47-47: Column: 1 (MD010, no-hard-tabs)
Hard tabs
48-48: Column: 1 (MD010, no-hard-tabs)
Hard tabs
49-49: Column: 1 (MD010, no-hard-tabs)
Hard tabs
51-51: Column: 1 (MD010, no-hard-tabs)
Hard tabs
52-52: Column: 1 (MD010, no-hard-tabs)
Hard tabs
53-53: Column: 1 (MD010, no-hard-tabs)
Hard tabs
55-55: Column: 1 (MD010, no-hard-tabs)
Hard tabs
84-84: Column: 1 (MD010, no-hard-tabs)
Hard tabs
85-85: Column: 1 (MD010, no-hard-tabs)
Hard tabs
86-86: Column: 1 (MD010, no-hard-tabs)
Hard tabs
87-87: Column: 1 (MD010, no-hard-tabs)
Hard tabs
88-88: Column: 1 (MD010, no-hard-tabs)
Hard tabs
89-89: Column: 1 (MD010, no-hard-tabs)
Hard tabs
93-93: Column: 1 (MD010, no-hard-tabs)
Hard tabs
94-94: Column: 1 (MD010, no-hard-tabs)
Hard tabs
95-95: Column: 1 (MD010, no-hard-tabs)
Hard tabs
96-96: Column: 1 (MD010, no-hard-tabs)
Hard tabs
97-97: Column: 1 (MD010, no-hard-tabs)
Hard tabs
101-101: Column: 1 (MD010, no-hard-tabs)
Hard tabs
102-102: Column: 1 (MD010, no-hard-tabs)
Hard tabs
104-104: Column: 1 (MD010, no-hard-tabs)
Hard tabs
105-105: Column: 1 (MD010, no-hard-tabs)
Hard tabs
106-106: Column: 1 (MD010, no-hard-tabs)
Hard tabs
107-107: Column: 1 (MD010, no-hard-tabs)
Hard tabs
111-111: Column: 1 (MD010, no-hard-tabs)
Hard tabs
113-113: Column: 1 (MD010, no-hard-tabs)
Hard tabs
114-114: Column: 1 (MD010, no-hard-tabs)
Hard tabs
115-115: Column: 1 (MD010, no-hard-tabs)
Hard tabs
116-116: Column: 1 (MD010, no-hard-tabs)
Hard tabs
117-117: Column: 1 (MD010, no-hard-tabs)
Hard tabs
118-118: Column: 1 (MD010, no-hard-tabs)
Hard tabs
122-122: Column: 1 (MD010, no-hard-tabs)
Hard tabs
124-124: Column: 1 (MD010, no-hard-tabs)
Hard tabs
125-125: Column: 1 (MD010, no-hard-tabs)
Hard tabs
126-126: Column: 1 (MD010, no-hard-tabs)
Hard tabs
127-127: Column: 1 (MD010, no-hard-tabs)
Hard tabs
128-128: Column: 1 (MD010, no-hard-tabs)
Hard tabs
130-130: Column: 1 (MD010, no-hard-tabs)
Hard tabs
131-131: Column: 1 (MD010, no-hard-tabs)
Hard tabs
132-132: Column: 1 (MD010, no-hard-tabs)
Hard tabs
133-133: Column: 1 (MD010, no-hard-tabs)
Hard tabs
134-134: Column: 1 (MD010, no-hard-tabs)
Hard tabs
135-135: Column: 1 (MD010, no-hard-tabs)
Hard tabs
136-136: Column: 1 (MD010, no-hard-tabs)
Hard tabs
137-137: Column: 1 (MD010, no-hard-tabs)
Hard tabs
138-138: Column: 1 (MD010, no-hard-tabs)
Hard tabs
140-140: Column: 1 (MD010, no-hard-tabs)
Hard tabs
166-166: Column: 1 (MD010, no-hard-tabs)
Hard tabs
167-167: Column: 1 (MD010, no-hard-tabs)
Hard tabs
168-168: Column: 1 (MD010, no-hard-tabs)
Hard tabs
169-169: Column: 1 (MD010, no-hard-tabs)
Hard tabs
177-177: Column: 1 (MD010, no-hard-tabs)
Hard tabs
179-179: Column: 1 (MD010, no-hard-tabs)
Hard tabs
180-180: Column: 1 (MD010, no-hard-tabs)
Hard tabs
181-181: Column: 1 (MD010, no-hard-tabs)
Hard tabs
182-182: Column: 1 (MD010, no-hard-tabs)
Hard tabs
184-184: Column: 1 (MD010, no-hard-tabs)
Hard tabs
185-185: Column: 1 (MD010, no-hard-tabs)
Hard tabs
186-186: Column: 1 (MD010, no-hard-tabs)
Hard tabs
187-187: Column: 1 (MD010, no-hard-tabs)
Hard tabs
188-188: Column: 1 (MD010, no-hard-tabs)
Hard tabs
189-189: Column: 1 (MD010, no-hard-tabs)
Hard tabs
191-191: Column: 1 (MD010, no-hard-tabs)
Hard tabs
192-192: Column: 1 (MD010, no-hard-tabs)
Hard tabs
193-193: Column: 1 (MD010, no-hard-tabs)
Hard tabs
195-195: Column: 1 (MD010, no-hard-tabs)
Hard tabs
196-196: Column: 1 (MD010, no-hard-tabs)
Hard tabs
197-197: Column: 1 (MD010, no-hard-tabs)
Hard tabs
199-199: Column: 1 (MD010, no-hard-tabs)
Hard tabs
231-231: Column: 1 (MD010, no-hard-tabs)
Hard tabs
232-232: Column: 1 (MD010, no-hard-tabs)
Hard tabs
233-233: Column: 1 (MD010, no-hard-tabs)
Hard tabs
234-234: Column: 1 (MD010, no-hard-tabs)
Hard tabs
235-235: Column: 1 (MD010, no-hard-tabs)
Hard tabs
236-236: Column: 1 (MD010, no-hard-tabs)
Hard tabs
237-237: Column: 1 (MD010, no-hard-tabs)
Hard tabs
238-238: Column: 1 (MD010, no-hard-tabs)
Hard tabs
239-239: Column: 1 (MD010, no-hard-tabs)
Hard tabs
240-240: Column: 1 (MD010, no-hard-tabs)
Hard tabs
241-241: Column: 1 (MD010, no-hard-tabs)
Hard tabs
242-242: Column: 1 (MD010, no-hard-tabs)
Hard tabs
75-75: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
249-249: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
…rk purely defensive code Signed-off-by: Dave Lee <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- middleware/keyauth/keyauth.go (8 hunks)
- middleware/keyauth/keyauth_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/keyauth/keyauth_test.go
Additional context used
GitHub Check: codecov/patch
middleware/keyauth/keyauth.go
[warning] 98-98: middleware/keyauth/keyauth.go#L98
Added line #L98 was not covered by tests
Additional comments not posted (10)
middleware/keyauth/keyauth.go (10)
6-6: Import offmtpackage to support error formatting.
54-54: Extraction and verification of key using the newly introducedCustomKeyLookup.
104-122: Refinement ofDefaultKeyLookupfunction to include more descriptive error messages and support various key sources.
125-125: Simple header extraction logic inKeyFromHeaderfunction.
140-140: Key extraction from query parameters inKeyFromQuery.
151-151: Key extraction from form data inKeyFromForm.
162-162: Key extraction from URL parameters inKeyFromParam.
173-173: Key extraction from cookies inKeyFromCookie.
79-102: Implementation ofMultipleKeySourceLookupfunction to support multiple key sources.Verification successful
The usage of
errors.Isin theMultipleKeySourceLookupfunction is consistent with other parts of the middleware directory.
middleware/timeout/timeout.gomiddleware/keyauth/keyauth.gomiddleware/keyauth/config.gomiddleware/csrf/csrf.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the error handling in `MultipleKeySourceLookup` is consistent with other parts of the project. rg --type go "errors.Is" src/middlewareLength of output: 94
Script:
#!/bin/bash # Find the directory containing the `keyauth.go` file fd keyauth.go # Find all Go files under the middleware directory to locate the correct path fd . --type f --extension go | grep middlewareLength of output: 3663
Script:
#!/bin/bash # Verify the usage of `errors.Is` within the middleware directory to check for consistency in error handling rg --type go "errors.Is" middlewareLength of output: 407
Tools
GitHub Check: codecov/patch
[warning] 98-98: middleware/keyauth/keyauth.go#L98
Added line #L98 was not covered by tests
38-43: Usingpanicfor error handling in the initialization of the middleware.Previous discussions suggest that using
panicmight be intentional to prevent the server from starting with invalid middleware configurations. However, consider providing a more graceful error handling mechanism that allows the caller to handle errors, especially in a library context.- panic(fmt.Errorf("unable to create lookup function: %w", err)) + return nil, fmt.Errorf("unable to create lookup function: %w", err)
|
@sixcolors when you get a chance, please let me know if there's any other changes you'd like me to make. Thanks! |
Will review tomorrow, thanks for your patience and efforts to make fiber better! |
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
I'm working on a project that must accept api keys from multiple possible sources, such as different header names. I've implemented this by slightly modifying the existing Keyauth middleware, and I'd like to contribute it upstream. These modifications have been refined through the PR process, and now allow the use of a custom key lookup function with the following signature to allow arbitrary key material to be extracted from the request context.
type KeyauthKeyLookupFunc func(c fiber.Ctx) (string, error)This was formerly PR #3027, but that was targeted at v2 instead of v3.
Changes introduced
CustomKeyLookupto keyauth.Config, allowing arbitrary key material to be extracted from the request context if more complex logic is required.SingleKeyLookup- this is still used by default, and can be used as a component of a custom implementationType of change
Please delete options that are not relevant.