refactor(protocol): change *http.Request parameter #246
Conversation
|
Warning Rate limit exceeded@rgcruz010 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 55 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes focus on refactoring functions across multiple files to replace Changes
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: 0
Outside diff range and nitpick comments (4)
protocol/assertion.go (3)
Line range hint
84-84: Consider using existing constantsPublicKeyCredentialType,Platform, andCrossPlatforminstead of string literals to enhance maintainability and reduce errors.- if car.Type != "public-key" { + if car.Type != PublicKeyCredentialType {Also applies to: 91-91, 93-93
Line range hint
159-159: The append operation result should be assigned back to the slice to ensure the data is updated correctly.- sigData := append(p.Raw.AssertionResponse.AuthenticatorData, clientDataHash[:]...) + sigData = append(p.Raw.AssertionResponse.AuthenticatorData, clientDataHash[:]...)
Line range hint
125-125: Ensure there is no leading whitespace at the start of the block to maintain code cleanliness and readability.webauthn/login.go (1)
Line range hint
217-217: The cyclomatic complexity ofvalidateLoginfunction is high, which may impact maintainability. Consider refactoring to reduce complexity.- func (webauthn *WebAuthn) validateLogin(user User, session SessionData, parsedResponse *protocol.ParsedCredentialAssertionData) (*Credential, error) { + func (webauthn *WebAuthn) validateLogin(user User, session SessionData, parsedResponse *protocol.ParsedCredentialAssertionData) (*Credential, error) { + // Refactor to reduce complexity + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- protocol/assertion.go (2 hunks)
- protocol/assertion_test.go (2 hunks)
- protocol/credential.go (3 hunks)
- protocol/credential_test.go (2 hunks)
- webauthn/login.go (3 hunks)
- webauthn/registration.go (2 hunks)
Files skipped from review due to trivial changes (2)
- protocol/assertion_test.go
- protocol/credential_test.go
Additional context used
golangci-lint
protocol/assertion.go
91-91: string
platformhas 2 occurrences, but such constantPlatformalready exists (goconst)
93-93: string
cross-platformhas 2 occurrences, but such constantCrossPlatformalready exists (goconst)
84-84: string
public-keyhas 2 occurrences, but such constantPublicKeyCredentialTypealready exists (goconst)
159-159: appendAssign: append result not assigned to the same slice (gocritic)
125-125: block should not start with a whitespace (wsl)
webauthn/login.go
217-217: cyclomatic complexity 19 of func
(*WebAuthn).validateLoginis high (> 15) (gocyclo)
Additional comments not posted (10)
protocol/assertion.go (3)
50-55: Refactoring to accept[]byteinstead of*http.Requestaligns well with the PR's goal to simplify the function's responsibilities.
61-64: The refactoring of input type fromio.Readerto[]bytesimplifies the function interface and reduces overhead from handling different types of readers.
Line range hint
84-84: Reiterating the use of constants for repeated string literals would enhance code quality and consistency.Also applies to: 91-91, 93-93
webauthn/registration.go (1)
205-206: Refactoring to accept[]byteinstead of*http.Requestis consistent with the PR's objective to simplify input handling and delegate non-core responsibilities to the handler layer.protocol/credential.go (4)
62-67: Refactoring to accept[]byteinstead of*http.Requestaligns well with the PR's goal to simplify the function's responsibilities.
Line range hint
84-84: Consider using existing constantsPublicKeyCredentialType,Platform, andCrossPlatforminstead of string literals to enhance maintainability and reduce errors.Also applies to: 91-91, 93-93
72-75: The refactoring of input type fromio.Readerto[]bytesimplifies the function interface and reduces overhead from handling different types of readers.
Line range hint
84-84: Reiterating the use of constants for repeated string literals would enhance code quality and consistency.Also applies to: 91-91, 93-93
webauthn/login.go (2)
164-165: Refactoring to accept[]byteinstead of*http.Requestis consistent with the PR's objective to simplify input handling and delegate non-core responsibilities to the handler layer.
176-177: Refactoring to accept[]byteinstead of*http.Requestaligns well with the PR's goal to simplify the function's responsibilities.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (5)
protocol/assertion.go (3)
Line range hint
84-84: Use the existing constantPublicKeyCredentialTypeinstead of the string literal.- if car.Type != "public-key" { + if car.Type != PublicKeyCredentialType {
Line range hint
91-91: Replace string literals with existing constants for authenticator attachment types.- case "platform": + case Platform: - case "cross-platform": + case CrossPlatform:Also applies to: 93-93
Line range hint
159-159: Ensure that the result ofappendis assigned back to the slice to avoid losing data.- sigData := append(p.Raw.AssertionResponse.AuthenticatorData, clientDataHash[:]...) + sigData = append(p.Raw.AssertionResponse.AuthenticatorData, clientDataHash[:]...)README.md (1)
Line range hint
28-28: Consider adding a comma before 'and' to improve readability.- ...ement of security in a dependent project and we aim to avoid backwards + ...ement of security in a dependent project, and we aim to avoid backwardswebauthn/login.go (1)
Line range hint
217-217: Consider refactoringvalidateLoginto reduce cyclomatic complexity and improve maintainability.- func (webauthn *WebAuthn) validateLogin(user User, session SessionData, parsedResponse *protocol.ParsedCredentialAssertionData) (*Credential, error) { + func (webauthn *WebAuthn) validateLogin(user User, session SessionData, parsedResponse *protocol.ParsedCredentialAssertionData) (*Credential, error) { + // Possible refactor to split into smaller functions + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- README.md (2 hunks)
- protocol/assertion.go (2 hunks)
- protocol/assertion_test.go (2 hunks)
- protocol/credential.go (3 hunks)
- protocol/credential_test.go (2 hunks)
- webauthn/login.go (3 hunks)
- webauthn/registration.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/assertion_test.go
- protocol/credential.go
- protocol/credential_test.go
- webauthn/registration.go
Additional context used
golangci-lint
protocol/assertion.go
91-91: string
platformhas 2 occurrences, but such constantPlatformalready exists (goconst)
93-93: string
cross-platformhas 2 occurrences, but such constantCrossPlatformalready exists (goconst)
84-84: string
public-keyhas 2 occurrences, but such constantPublicKeyCredentialTypealready exists (goconst)
159-159: appendAssign: append result not assigned to the same slice (gocritic)
125-125: block should not start with a whitespace (wsl)
webauthn/login.go
217-217: cyclomatic complexity 19 of func
(*WebAuthn).validateLoginis high (> 15) (gocyclo)
LanguageTool
README.md
[grammar] ~25-~25: It seems that a pronoun is missing. (IF_VB)
Context: ...lly 1 patch release of go,
for example if go 1.21.0 is released, we will likely s...
[uncategorized] ~28-~28: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE)
Context: ...ement of security in a dependent project and we aim to avoid backwards
compatibilit...
[uncategorized] ~39-~39: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE)
Context: ...nately being used in dependent libraries and we'd opt for ensuring we can easily obt...
[uncategorized] ~40-~40: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... rather than backwards compatibility. A such we have lifted the version requirement ...
[typographical] ~45-~45: Consider adding a comma. (IF_THERE_COMMA)
Context: ... 0, as per Semantic Versioning 2.0 rules there may be breaking changes without warning...
[style] ~45-~45: ‘without warning’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_WITHOUT_WARNING)
Context: ...2.0 rules there may be breaking changes without warning.
While we strive to avoid such change...
[style] ~52-~52: As a shorter alternative for ‘able to’, consider using “can”. (BE_ABLE_TO)
Context: ...values.Make sure your
usermodel is able to handle the interface functions laid out...
[uncategorized] ~58-~58: It appears that in this idiom a possessive apostrophe is missing. (FOR_NOUN_SAKE)
Context: ...ow some basic use cases of the library. For consistency sake the following variables are used
to de...
[uncategorized] ~273-~273: A comma may be missing after the conjunctive/linking adverb ‘However’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...y by default does not enforce timeouts. However the default timeouts sent to the browse...
[typographical] ~274-~274: Consider adding a comma before the interrupter. (HOWEVER_COMMA)
Context: ...ou can override both of these behaviours however.package example import ( ... --- [uncategorized] ~316-~316: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) Context: ... implementation. Without their amazing work this library could not exist. [git... </blockquote></details> </blockquote></details> <details> <summary>Markdownlint</summary><blockquote> <details> <summary>README.md</summary><blockquote> 7-7: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 45-45: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 50-50: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 52-52: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 53-53: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 62-62: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 203-203: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 204-204: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 75-75: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 77-77: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 81-81: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 82-82: 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 --- 90-90: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 91-91: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 92-92: 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 --- 152-152: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 153-153: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 154-154: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 155-155: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 156-156: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 158-158: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 159-159: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 160-160: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 161-161: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 162-162: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 163-163: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 164-164: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 165-165: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 169-169: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 170-170: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 171-171: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 172-172: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 183-183: Column: 3 (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 --- 187-187: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 188-188: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 190-190: 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 --- 194-194: 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 --- 214-214: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 215-215: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 221-221: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 222-222: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 223-223: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 224-224: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 225-225: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 226-226: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 227-227: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 229-229: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 230-230: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 231-231: 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 --- 236-236: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 248-248: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 249-249: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 255-255: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 256-256: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 257-257: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 258-258: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 259-259: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 260-260: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 261-261: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 263-263: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 265-265: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 267-267: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 280-280: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 281-281: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 282-282: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 283-283: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 284-284: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 288-288: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 289-289: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 290-290: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 291-291: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 292-292: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 293-293: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 294-294: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 295-295: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 296-296: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 297-297: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 298-298: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 299-299: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 300-300: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 301-301: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 302-302: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 303-303: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 304-304: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 305-305: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 306-306: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 307-307: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 308-308: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 309-309: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 6-6: Expected: 1; Actual: 2 (MD012, no-multiple-blanks) Multiple consecutive blank lines --- 318-318: Expected: 1; Actual: 2 (MD012, no-multiple-blanks) Multiple consecutive blank lines </blockquote></details> </blockquote></details> </details> <details> <summary>Additional comments not posted (3)</summary><blockquote> <details> <summary>protocol/assertion.go (1)</summary><blockquote> `50-55`: Refactor to accept byte slice enhances encapsulation and reduces coupling. </blockquote></details> <details> <summary>README.md (1)</summary><blockquote> `104-142`: Update examples to reflect the new API usage correctly. </blockquote></details> <details> <summary>webauthn/login.go (1)</summary><blockquote> Line range hint `164-177`: Successfully adapted the login functions to handle byte slices, improving data handling. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
As it is a breaking change as you've already identified; and it's actually a sizable breaking change, and the effective functionality can be implemented without a breaking change it would require a sizable benefit over not doing it for me to accept this I think. I don't currently see it that way. Maybe you'd be interested in explaining that or in adjusting this potential enhancement to be backwards compatible via leaving the existing methods intact and refactoring them to use bytes methods. This would also add flexibility to the implementer. I'm not against the breaking change itself especially considering the state of the library, just questioning the necessity of it. For example it's simple to wrap a byte slice in an io.Reader which prevents the need of using a *http.Request entirely. |
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- README.md (3 hunks)
- protocol/assertion.go (2 hunks)
- protocol/assertion_test.go (2 hunks)
- protocol/credential.go (3 hunks)
- protocol/credential_test.go (2 hunks)
- webauthn/login.go (3 hunks)
- webauthn/registration.go (2 hunks)
Files skipped from review due to trivial changes (2)
- protocol/assertion_test.go
- protocol/credential_test.go
Additional context used
golangci-lint
protocol/assertion.go
90-90: string
public-keyhas 2 occurrences, but such constantPublicKeyCredentialTypealready exists (goconst)
97-97: string
platformhas 2 occurrences, but such constantPlatformalready exists (goconst)
99-99: string
cross-platformhas 2 occurrences, but such constantCrossPlatformalready exists (goconst)
165-165: appendAssign: append result not assigned to the same slice (gocritic)
131-131: block should not start with a whitespace (wsl)
webauthn/login.go
218-218: cyclomatic complexity 19 of func
(*WebAuthn).validateLoginis high (> 15) (gocyclo)
LanguageTool
README.md
[grammar] ~25-~25: It seems that a pronoun is missing. (IF_VB)
Context: ...lly 1 patch release of go,
for example if go 1.21.0 is released, we will likely s...
[uncategorized] ~28-~28: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE)
Context: ...ement of security in a dependent project and we aim to avoid backwards
compatibilit...
[uncategorized] ~29-~29: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...specially important in a language like
go where their backwards compatibility whe...
[uncategorized] ~39-~39: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE)
Context: ...nately being used in dependent libraries and we'd opt for ensuring we can easily obt...
[uncategorized] ~40-~40: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... rather than backwards compatibility. A such we have lifted the version requirement ...
[typographical] ~45-~45: Consider adding a comma. (IF_THERE_COMMA)
Context: ... 0, as per Semantic Versioning 2.0 rules there may be breaking changes without warning...
[style] ~45-~45: ‘without warning’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_WITHOUT_WARNING)
Context: ...2.0 rules there may be breaking changes without warning.
While we strive to avoid such change...
[style] ~52-~52: As a shorter alternative for ‘able to’, consider using “can”. (BE_ABLE_TO)
Context: ...values.Make sure your
usermodel is able to handle the interface functions laid out...
[uncategorized] ~58-~58: It appears that in this idiom a possessive apostrophe is missing. (FOR_NOUN_SAKE)
Context: ...ow some basic use cases of the library. For consistency sake the following variables are used
to de...
[uncategorized] ~274-~274: A comma may be missing after the conjunctive/linking adverb ‘However’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...y by default does not enforce timeouts. However the default timeouts sent to the browse...
[typographical] ~275-~275: Consider adding a comma before the interrupter. (HOWEVER_COMMA)
Context: ...ou can override both of these behaviours however.package example import ( ... --- [uncategorized] ~317-~317: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) Context: ... implementation. Without their amazing work this library could not exist. [git... </blockquote></details> </blockquote></details> <details> <summary>Markdownlint</summary><blockquote> <details> <summary>README.md</summary><blockquote> 7-7: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 45-45: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 50-50: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 52-52: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 53-53: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 62-62: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 204-204: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 205-205: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 75-75: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 77-77: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 81-81: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 82-82: 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 --- 90-90: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 91-91: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 92-92: 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 --- 152-152: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 153-153: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 154-154: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 155-155: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 156-156: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 158-158: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 159-159: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 160-160: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 161-161: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 162-162: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 163-163: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 164-164: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 165-165: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 169-169: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 170-170: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 171-171: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 172-172: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 183-183: Column: 3 (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 --- 187-187: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 188-188: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 190-190: 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 --- 194-194: 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 --- 215-215: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 216-216: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 222-222: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 223-223: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 224-224: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 225-225: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 226-226: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 227-227: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 228-228: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 230-230: 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 --- 234-234: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 235-235: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 237-237: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 249-249: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 250-250: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 256-256: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 257-257: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 258-258: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 259-259: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 260-260: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 261-261: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 262-262: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 264-264: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 266-266: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 268-268: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 281-281: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 282-282: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 283-283: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 284-284: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 285-285: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 289-289: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 290-290: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 291-291: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 292-292: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 293-293: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 294-294: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 295-295: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 296-296: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 297-297: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 298-298: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 299-299: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 300-300: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 301-301: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 302-302: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 303-303: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 304-304: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 305-305: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 306-306: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 307-307: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 308-308: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 309-309: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 310-310: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 6-6: Expected: 1; Actual: 2 (MD012, no-multiple-blanks) Multiple consecutive blank lines --- 319-319: Expected: 1; Actual: 2 (MD012, no-multiple-blanks) Multiple consecutive blank lines </blockquote></details> </blockquote></details> </details> <details> <summary>Additional comments not posted (4)</summary><blockquote> <details> <summary>protocol/assertion.go (1)</summary><blockquote> `51-56`: **Refactor to use `io.Reader` instead of `*http.Request`:** The change to use `io.Reader` directly for parsing the credential request response is aligned with the PR's objective to decouple HTTP handling from the parsing logic. This should allow for more flexible usage of the parsing functions. </blockquote></details> <details> <summary>webauthn/registration.go (1)</summary><blockquote> `206-207`: **Refactor to use `io.Reader` in `FinishRegistration`:** The transition to using `io.Reader` for the `FinishRegistration` function is consistent with the overall objective of the PR to handle request data more flexibly outside of HTTP contexts. </blockquote></details> <details> <summary>protocol/credential.go (1)</summary><blockquote> `63-68`: **Refactor to use `io.Reader` instead of `*http.Request`:** The modification to use `io.Reader` for parsing the credential creation response aligns with the PR's goal to separate HTTP handling from parsing logic. </blockquote></details> <details> <summary>README.md (1)</summary><blockquote> `116-142`: **Update README to reflect API changes:** The README has been updated appropriately to demonstrate the use of `io.Reader` in `FinishRegistration` and `FinishLogin` functions. This ensures that users of the library are aware of the breaking changes and how to handle them correctly. [APROVED] Also applies to: 174-199 </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
protocol/assertion.go
Outdated
| func ParseCredentialRequestResponseBody(credentialResponse io.Reader) (par *ParsedCredentialAssertionData, err error) { | ||
| bytes, err := io.ReadAll(credentialResponse) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var car CredentialAssertionResponse | ||
|
|
||
| if err = decodeBody(body, &car); err != nil { | ||
| if err = json.Unmarshal(bytes, &car); err != nil { |
There was a problem hiding this comment.
Error handling in JSON parsing: The error message "Parse error for Assertion" is clear, but consider using a predefined constant for consistency as previously suggested in another PR.
- return nil, ErrBadRequest.WithDetails("Parse error for Assertion").WithInfo(err.Error())
+ return nil, ErrBadRequest.WithDetails(ErrParseAssertion).WithInfo(err.Error())Committable suggestion was skipped due to low confidence.
webauthn/login.go
Outdated
| func (webauthn *WebAuthn) FinishLogin(user User, session SessionData, clientResponse io.Reader) (*Credential, error) { | ||
| parsedResponse, err := protocol.ParseCredentialRequestResponse(clientResponse) |
There was a problem hiding this comment.
Tip
Codebase Verification
Update Test Case: TestLogin_FinishLoginFailure
The test case TestLogin_FinishLoginFailure in webauthn/login_test.go has not been updated to match the new io.Reader parameter type for the FinishLogin function. The third argument is currently nil, which should be replaced with an appropriate io.Reader implementation, such as strings.NewReader("").
webauthn/login_test.goTestLogin_FinishLoginFailurefunction: Update theFinishLogincall to useio.Reader.
Analysis chain
Refactor Approved: Update to FinishLogin function signature.
The change from *http.Request to io.Reader in the FinishLogin function aligns with the PR's objective to reduce dependency on the http package and focus on processing the body bytes. Ensure that all usages of this function across the codebase are updated to pass io.Reader.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `FinishLogin` match the new signature.
# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go $'FinishLogin'
Length of output: 786
Script:
#!/bin/bash
# Description: Verify if the test cases in `webauthn/login_test.go` are correctly updated to use `io.Reader`.
# Test: Search for the implementation of `TestLogin_FinishLoginFailure` and check the usage of `FinishLogin`.
rg --type go --context 10 'TestLogin_FinishLoginFailure' webauthn/login_test.go
Length of output: 404
Script:
#!/bin/bash
# Description: Retrieve the full implementation of `TestLogin_FinishLoginFailure` to verify the usage of `io.Reader`.
# Test: Search for the complete implementation of `TestLogin_FinishLoginFailure`.
rg --type go --context 50 'func TestLogin_FinishLoginFailure' webauthn/login_test.go
Length of output: 1522
webauthn/login.go
Outdated
| func (webauthn *WebAuthn) FinishDiscoverableLogin(handler DiscoverableUserHandler, session SessionData, clientResponse io.Reader) (*Credential, error) { | ||
| parsedResponse, err := protocol.ParseCredentialRequestResponse(clientResponse) |
There was a problem hiding this comment.
Tip
Codebase Verification
Review the necessity of the FinishDiscoverableLogin function
There are no references or calls to the FinishDiscoverableLogin function in the codebase, indicating it is currently unused. Please review whether this function is intended for future use or if it can be removed to clean up the codebase.
webauthn/login.go:FinishDiscoverableLoginfunction definition
Analysis chain
Refactor Approved: Update to FinishDiscoverableLogin function signature.
The change from *http.Request to io.Reader in the FinishDiscoverableLogin function is consistent with the overall goal of simplifying input handling. Ensure that all usages of this function across the codebase are updated to pass io.Reader.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `FinishDiscoverableLogin` match the new signature.
# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go $'FinishDiscoverableLogin'
Length of output: 350
Script:
#!/bin/bash
# Description: Broaden the search to find all calls to `FinishDiscoverableLogin` and verify their signatures.
# Test: Search for calls to the function. Expect: Occurrences of the function calls.
rg 'FinishDiscoverableLogin\('
Length of output: 203
Script:
#!/bin/bash
# Description: Check for any references to `FinishDiscoverableLogin` in the codebase to verify its usage.
# Test: Search for any references to the function. Expect: Occurrences of references or indirect usage.
rg 'FinishDiscoverableLogin'
Length of output: 340
|
Thanks for the answer, it is more of a design decision so that the library has a simpler and more limited responsibility than handling the complete HTTP request. I will make another proposal as you suggest, seeing the possibility of backward compatibility. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (5)
protocol/assertion.go (3)
Line range hint
84-84: Use existing constant for credential type.The string
"public-key"is used multiple times and should be replaced with the existing constantPublicKeyCredentialTypefor better maintainability.- if car.Type != "public-key" { + if car.Type != PublicKeyCredentialType {
Line range hint
91-91: Replace string literals with constants for authenticator attachment types.The strings
"platform"and"cross-platform"are used directly in the code. These should be replaced with their respective constantsPlatformandCrossPlatformto avoid potential typos and facilitate changes in the future.- case "platform": + case Platform: - case "cross-platform": + case CrossPlatform:Also applies to: 93-93
Line range hint
159-159: Fix inefficient append operation.The result of the
appendoperation is not being assigned back to the slice, which could lead to logical errors or unexpected behavior.- append(p.Raw.AssertionResponse.AuthenticatorData, clientDataHash[:]...) + sigData = append(p.Raw.AssertionResponse.AuthenticatorData, clientDataHash[:]...)README.md (2)
Line range hint
25-25: Grammar correction in Go version support policy.There seems to be a missing pronoun in the sentence discussing the patch release of Go. Adding the pronoun would improve readability.
- for example if go 1.21.0 is released, we will likely still support go 1.17 until go 1.21.1 is released + for example, if go 1.21.0 is released, we will likely still support go 1.17 until go 1.21.1 is released
Line range hint
28-28: Improve sentence structure with commas.Adding commas before 'and' in compound sentences can improve readability and clarity.
- a dependent project and we aim to avoid backwards + a dependent project, and we aim to avoid backwards - dependent libraries and we'd opt for ensuring we can easily obtain potential fixes + dependent libraries, and we'd opt for ensuring we can easily obtain potential fixesAlso applies to: 39-40
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- README.md (3 hunks)
- protocol/assertion.go (2 hunks)
- protocol/assertion_test.go (2 hunks)
- protocol/credential.go (3 hunks)
- protocol/credential_test.go (2 hunks)
- webauthn/login.go (2 hunks)
- webauthn/registration.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/assertion_test.go
- protocol/credential.go
- webauthn/registration.go
Additional context used
golangci-lint
protocol/assertion.go
93-93: string
cross-platformhas 2 occurrences, but such constantCrossPlatformalready exists (goconst)
84-84: string
public-keyhas 2 occurrences, but such constantPublicKeyCredentialTypealready exists (goconst)
91-91: string
platformhas 2 occurrences, but such constantPlatformalready exists (goconst)
159-159: appendAssign: append result not assigned to the same slice (gocritic)
125-125: block should not start with a whitespace (wsl)
webauthn/login.go
249-249: cyclomatic complexity 19 of func
(*WebAuthn).validateLoginis high (> 15) (gocyclo)
167-167: ineffectual assignment to err (ineffassign)
LanguageTool
README.md
[grammar] ~25-~25: It seems that a pronoun is missing. (IF_VB)
Context: ...lly 1 patch release of go,
for example if go 1.21.0 is released, we will likely s...
[uncategorized] ~28-~28: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE)
Context: ...ement of security in a dependent project and we aim to avoid backwards
compatibilit...
[uncategorized] ~39-~39: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE)
Context: ...nately being used in dependent libraries and we'd opt for ensuring we can easily obt...
[uncategorized] ~40-~40: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... rather than backwards compatibility. A such we have lifted the version requirement ...
[typographical] ~45-~45: Consider adding a comma. (IF_THERE_COMMA)
Context: ... 0, as per Semantic Versioning 2.0 rules there may be breaking changes without warning...
[style] ~45-~45: ‘without warning’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_WITHOUT_WARNING)
Context: ...2.0 rules there may be breaking changes without warning.
While we strive to avoid such change...
[style] ~52-~52: As a shorter alternative for ‘able to’, consider using “can”. (BE_ABLE_TO)
Context: ...values.Make sure your
usermodel is able to handle the interface functions laid out...
[uncategorized] ~58-~58: It appears that in this idiom a possessive apostrophe is missing. (FOR_NOUN_SAKE)
Context: ...ow some basic use cases of the library. For consistency sake the following variables are used
to de...
[uncategorized] ~274-~274: A comma may be missing after the conjunctive/linking adverb ‘However’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...y by default does not enforce timeouts. However the default timeouts sent to the browse...
[typographical] ~275-~275: Consider adding a comma before the interrupter. (HOWEVER_COMMA)
Context: ...ou can override both of these behaviours however.package example import ( ... </blockquote></details> </blockquote></details> <details> <summary>Markdownlint</summary><blockquote> <details> <summary>README.md</summary><blockquote> 7-7: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 45-45: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 50-50: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 52-52: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 53-53: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 62-62: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 204-204: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 205-205: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 75-75: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 77-77: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 81-81: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 82-82: 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 --- 90-90: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 91-91: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 92-92: 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 --- 152-152: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 153-153: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 154-154: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 155-155: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 156-156: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 158-158: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 159-159: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 160-160: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 161-161: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 162-162: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 163-163: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 164-164: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 165-165: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 169-169: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 170-170: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 171-171: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 172-172: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 183-183: Column: 3 (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 --- 187-187: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 188-188: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 190-190: 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 --- 194-194: 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 --- 215-215: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 216-216: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 222-222: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 223-223: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 224-224: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 225-225: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 226-226: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 227-227: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 228-228: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 230-230: 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 --- 234-234: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 235-235: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 237-237: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 249-249: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 250-250: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 256-256: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 257-257: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 258-258: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 259-259: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 260-260: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 261-261: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 262-262: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 264-264: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 266-266: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 268-268: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 281-281: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 282-282: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 283-283: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 284-284: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 285-285: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 289-289: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 290-290: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 291-291: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 292-292: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 293-293: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 294-294: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 295-295: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 296-296: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 297-297: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 298-298: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 299-299: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 300-300: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 301-301: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 302-302: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 303-303: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 304-304: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 305-305: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 306-306: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 307-307: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 308-308: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 309-309: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 310-310: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 6-6: Expected: 1; Actual: 2 (MD012, no-multiple-blanks) Multiple consecutive blank lines --- 319-319: Expected: 1; Actual: 2 (MD012, no-multiple-blanks) Multiple consecutive blank lines </blockquote></details> </blockquote></details> </details> <details> <summary>Additional comments not posted (6)</summary><blockquote> <details> <summary>protocol/assertion.go (1)</summary><blockquote> `50-55`: **Refactor parameter type from `http.Request` to `[]byte` for better decoupling.** The change from `http.Request` to `[]byte` in `ParseCredentialRequestResponse` aligns with the PR's objective to decouple HTTP handling from parsing logic. This should make the function more versatile and easier to test. </blockquote></details> <details> <summary>README.md (2)</summary><blockquote> `104-142`: **Update example code to reflect new API changes.** The example code in the README has been updated to use `io.Reader` instead of `*http.Request`, which is consistent with the changes in the library. This should help users adapt to the new API. --- Line range hint `173-199`: **Ensure consistent error handling in example code.** The example code for login functionality uses `io.Reader` and includes error handling, which is good practice. However, ensure that error messages provide enough context to help diagnose issues. <details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 170-170: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 171-171: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 172-172: Column: 1 (MD010, no-hard-tabs) Hard tabs --- 183-183: Column: 3 (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 </blockquote></details> </details> </blockquote></details> <details> <summary>protocol/credential_test.go (1)</summary><blockquote> `99-99`: **Ensure comprehensive testing of credential parsing.** The test case `ShouldParseCredentialRequest` appears to be comprehensive, covering various aspects of the credential parsing process. Ensure that edge cases and error conditions are also tested to improve the robustness of the library. </blockquote></details> <details> <summary>webauthn/login.go (2)</summary><blockquote> `177-202`: **Refactor `processResponse` to simplify error handling and improve readability.** The `processResponse` method contains multiple cases for handling different types of `clientResponse`. Consider refactoring to separate private methods to improve readability and maintainability. [REFACTOR_Suggestion] ```diff func (webauthn *WebAuthn) processResponse(data any) ([]byte, error) { switch cl := data.(type) { case *http.Request: return processHTTPRequest(cl) case io.Reader: return processIOReader(cl) case []byte: return cl, nil default: return nil, protocol.ErrBadRequest.WithDetails("Invalid client response type") } } func (webauthn *WebAuthn) processHTTPRequest(request *http.Request) ([]byte, error) { body, err := io.ReadAll(request.Body) _ = request.Body.Close() return body, err } func (webauthn *WebAuthn) processIOReader(reader io.Reader) ([]byte, error) { return io.ReadAll(reader) }
Line range hint
249-249: Reduce cyclomatic complexity invalidateLogin.The function
validateLoginhas a high cyclomatic complexity. Consider breaking down the function into smaller, more focused methods to improve readability and maintainability.
[REFACTOR_Suggestion]func (webauthn *WebAuthn) validateLogin(user User, session SessionData, parsedResponse *protocol.ParsedCredentialAssertionData) (*Credential, error) { if err := validateSessionOwnership(user, session, parsedResponse); err != nil { return nil, err } if err := validateCredentialOwnership(user, session, parsedResponse); err != nil { return nil, err } return validateResponseIntegrity(session, parsedResponse) } func validateSessionOwnership(user User, session SessionData, parsedResponse *protocol.ParsedCredentialAssertionData) error { if !bytes.Equal(user.WebAuthnID(), session.UserID) { return protocol.ErrBadRequest.WithDetails("ID mismatch for User and Session") } if !session.Expires.IsZero() && session.Expires.Before(time.Now()) { return protocol.ErrBadRequest.WithDetails("Session has Expired") } return nil } func validateCredentialOwnership(user User, session SessionData, parsedResponse *protocol.ParsedCredentialAssertionData) error { // Steps to validate credential ownership... return nil } func validateResponseIntegrity(session SessionData, parsedResponse *protocol.ParsedCredentialAssertionData) (*Credential, error) { // Steps to validate the integrity of the response... return &loginCredential, nil }
webauthn/login.go
Outdated
| func (webauthn *WebAuthn) FinishLogin(user User, session SessionData, clientResponse any) (*Credential, error) { | ||
| body, err := webauthn.processResponse(clientResponse) | ||
|
|
||
| parsedResponse, err := protocol.ParseCredentialRequestResponse(body) |
There was a problem hiding this comment.
Handle error after processing the response in FinishLogin.
There is an ineffectual assignment to err after webauthn.processResponse(clientResponse). This can lead to ignoring errors that should stop the function execution.
- body, err := webauthn.processResponse(clientResponse)
+ body, err := webauthn.processResponse(clientResponse)
+ if err != nil {
+ return nil, err
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (webauthn *WebAuthn) FinishLogin(user User, session SessionData, clientResponse any) (*Credential, error) { | |
| body, err := webauthn.processResponse(clientResponse) | |
| parsedResponse, err := protocol.ParseCredentialRequestResponse(body) | |
| func (webauthn *WebAuthn) FinishLogin(user User, session SessionData, clientResponse any) (*Credential, error) { | |
| body, err := webauthn.processResponse(clientResponse) | |
| if err != nil { | |
| return nil, err | |
| } | |
| parsedResponse, err := protocol.ParseCredentialRequestResponse(body) |
Tools
golangci-lint
167-167: ineffectual assignment to err (ineffassign)
| func ParseCredentialRequestResponseBody(credentialResponse []byte) (par *ParsedCredentialAssertionData, err error) { | ||
| var car CredentialAssertionResponse | ||
|
|
||
| if err = decodeBody(body, &car); err != nil { | ||
| if err = json.Unmarshal(credentialResponse, &car); err != nil { |
There was a problem hiding this comment.
Consider using predefined constants for error details to maintain consistency.
The error handling code in ParseCredentialRequestResponseBody uses a hardcoded string for the error detail. Using a predefined constant would enhance maintainability and consistency across the codebase.
- return nil, ErrBadRequest.WithDetails("Parse error for Assertion").WithInfo(err.Error())
+ return nil, ErrBadRequest.WithDetails(ErrParseAssertion).WithInfo(err.Error())Committable suggestion was skipped due to low confidence.
This is a breaking API change, but I don't see it necessary to pass the entire request to the library. The responsibility lies only in processing the body bytes, parsing them, and performing the corresponding validations related to the protocol.
All boilerplate code like:
it's better off be part of the handler layer