feat(protocol): signals structs#574
Conversation
WalkthroughIntroduces WebAuthn signaling types and constructors, adds helpers to emit unknown-credential signals from descriptors and credentials, expands unit tests (including credential parsing subtests), and reformats a comment. Several new exported types, methods, and tests were added; no exported APIs were removed. Changes
Sequence Diagram(s)(Skipped — changes do not introduce a multi-component sequential control flow suitable for diagramming.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #574 +/- ##
==========================================
+ Coverage 52.71% 54.09% +1.37%
==========================================
Files 40 41 +1
Lines 2464 2481 +17
==========================================
+ Hits 1299 1342 +43
+ Misses 951 923 -28
- Partials 214 216 +2
🚀 New features to boost your workflow:
|
james-d-elliott
force-pushed
the
feat-signals-struct
branch
from
856de8d to
9b6cc60
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces WebAuthn protocol signal structures to support signaling various credential states. The changes add new types and helper methods for creating signal responses that can be marshaled to JSON.
- Adds new signal structs (
SignalAllAcceptedCredentials,SignalCurrentUserDetails,SignalUnknownCredential) with appropriate JSON tags - Implements helper methods for creating signal responses from credentials and descriptors
- Refactors
TestParseCredentialCreationResponseto test three different parsing entry points (Response, ResponseBody, Bytes)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| protocol/signals.go | New file defining signal structs and NewSignalAllAcceptedCredentials constructor function |
| protocol/signals_test.go | Tests for the new signal functionality |
| protocol/options.go | Adds SignalUnknownCredential method to CredentialDescriptor |
| protocol/options_test.go | Tests for the new SignalUnknownCredential method |
| webauthn/credential.go | Adds SignalUnknownCredential method to Credential type |
| webauthn/credential_test.go | Tests for credential signal methods and descriptors |
| protocol/credential_test.go | Refactors test to validate three parsing functions with the same test cases |
| webauthn/types.go | Reformats multi-line comment for better readability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
protocol/signals_test.go (1)
45-49: Consider marshalingactualinstead oftc.expectedfor complete verification.Line 47 marshals
tc.expectedrather thanactual. While theassert.Equalon line 45 ensures structural equality, marshalingactualwould provide end-to-end validation that the constructor's output serializes correctly.🔎 Proposed fix
assert.Equal(t, tc.expected, actual) - data, err := json.Marshal(tc.expected) + data, err := json.Marshal(actual) assert.NoError(t, err) assert.Equal(t, tc.expectedJSON, string(data))webauthn/credential_test.go (1)
84-129: Remove unusedrpidfield from test case struct.The
rpidfield defined on line 87 is never used in the test execution (lines 117-128). This appears to be leftover from copy-paste.🔎 Proposed fix
func TestCredentials_CredentialDescriptors(t *testing.T) { testCases := []struct { name string - rpid string have Credentials expected []protocol.CredentialDescriptor expectedJSON string }{ { "ShouldHandleStandard", - "example.com", Credentials{ Credential{ ID: []byte("1234"), }, }, []protocol.CredentialDescriptor{ { Type: protocol.PublicKeyCredentialType, CredentialID: protocol.URLEncodedBase64("1234"), }, }, `[{"type":"public-key","id":"MTIzNA"}]`, }, { "ShouldHandleEmpty", - "example.com", Credentials{}, []protocol.CredentialDescriptor{}, `[]`, }, }protocol/credential_test.go (1)
150-151: Remove duplicate assertion.Line 151 duplicates the assertion from line 150 - both assert
ParsedPublicKeyCredentialequality.🔎 Proposed fix
assert.Equal(t, tc.expected.ParsedCredential, actual.ParsedCredential) assert.Equal(t, tc.expected.ParsedPublicKeyCredential, actual.ParsedPublicKeyCredential) - assert.Equal(t, tc.expected.ParsedPublicKeyCredential, actual.ParsedPublicKeyCredential) assert.Equal(t, tc.expected.Raw, actual.Raw)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
protocol/credential_test.goprotocol/options.goprotocol/options_test.goprotocol/signals.goprotocol/signals_test.gowebauthn/credential.gowebauthn/credential_test.gowebauthn/types.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T05:23:05.652Z
Learnt from: james-d-elliott
Repo: go-webauthn/webauthn PR: 497
File: webauthn/login.go:385-387
Timestamp: 2025-08-26T05:23:05.652Z
Learning: The `AttestationType` field in the webauthn.Credential struct actually contains the attestation format (like "none", "packed", "fido-u2f") rather than the attestation type. This is evident from the comment "The attestation format used (if any) by the authenticator when creating the credential" and the assignment from c.Response.AttestationObject.Format during credential creation.
Applied to files:
protocol/options.gowebauthn/types.go
🧬 Code graph analysis (7)
protocol/options.go (1)
protocol/signals.go (1)
SignalUnknownCredential(36-39)
protocol/credential_test.go (2)
protocol/credential.go (6)
ParsedCredentialCreationData(56-61)ParseCredentialCreationResponse(65-76)ParseCredentialCreationResponseBody(80-88)ParseCredentialCreationResponseBytes(92-100)ParsedCredential(29-32)ParsedPublicKeyCredential(42-48)protocol/func_test.go (1)
AssertIsProtocolError(12-43)
webauthn/credential_test.go (5)
webauthn/credential.go (2)
Credential(23-48)Credentials(59-59)protocol/signals.go (1)
SignalUnknownCredential(36-39)protocol/base64.go (1)
URLEncodedBase64(12-12)protocol/options.go (2)
CredentialDescriptor(62-74)PublicKeyCredentialType(105-105)webauthn/types_test.go (1)
id(11-14)
protocol/signals.go (2)
protocol/base64.go (1)
URLEncodedBase64(12-12)protocol/credential.go (3)
ID(16-25)Credential(34-40)ID(29-32)
protocol/signals_test.go (3)
protocol/signals.go (3)
AllAcceptedCredentialsUser(41-44)SignalAllAcceptedCredentials(23-27)NewSignalAllAcceptedCredentials(3-21)protocol/base64.go (1)
URLEncodedBase64(12-12)protocol/webauthncbor/webauthncbor.go (1)
Marshal(31-33)
protocol/options_test.go (3)
protocol/options.go (1)
CredentialDescriptor(62-74)protocol/signals.go (1)
SignalUnknownCredential(36-39)protocol/base64.go (1)
URLEncodedBase64(12-12)
webauthn/credential.go (2)
protocol/credential.go (1)
Credential(16-25)protocol/signals.go (1)
SignalUnknownCredential(36-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (6)
webauthn/types.go (1)
184-186: LGTM!Minor documentation formatting adjustment - no functional changes.
protocol/options_test.go (1)
65-107: LGTM!Well-structured table-driven test with good coverage of both the standard case and the edge case with no credential ID. The JSON marshaling assertions correctly verify the base64url encoding behavior.
protocol/options.go (1)
76-81: LGTM!Clean factory method that correctly constructs the signal struct. The value receiver is consistent with the struct's usage pattern.
webauthn/credential.go (1)
50-54: LGTM!Clean delegation pattern that reuses the existing
Descriptor()method, avoiding code duplication. The documentation clearly explains the method's purpose.webauthn/credential_test.go (1)
39-82: LGTM!Comprehensive test coverage for
SignalUnknownCredentialwith proper handling of both standard and edge cases. JSON marshaling verification ensures correct serialization behavior.protocol/credential_test.go (1)
114-168: Good test refactoring!The subtest structure effectively covers all three parsing entry points (
ParseCredentialCreationResponse,ParseCredentialCreationResponseBody,ParseCredentialCreationResponseBytes) without duplicating test data. The CBOR unmarshaling verification for public keys adds valuable round-trip validation.
james-d-elliott
force-pushed
the
feat-signals-struct
branch
from
568b0bb to
04b5fef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@protocol/credential_test.go`:
- Around line 150-151: There is a duplicate assertion comparing
tc.expected.ParsedPublicKeyCredential to actual.ParsedPublicKeyCredential;
remove the redundant assert.Equal so the test only asserts this once (leave a
single assert.Equal(t, tc.expected.ParsedPublicKeyCredential,
actual.ParsedPublicKeyCredential) and delete the duplicate line).
🧹 Nitpick comments (1)
protocol/signals_test.go (1)
47-49: Consider usingrequire.NoErrorfor marshal error.If
json.Marshalfails, usingassert.NoErrorallows the test to continue and compare against potentially invalid data. The other test file in this PR (webauthn/credential_test.go) usesrequire.NoErrorfor the same pattern.♻️ Suggested fix for consistency
+ "github.com/stretchr/testify/require" ... data, err := json.Marshal(actual) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, tc.expectedJSON, string(data))
james-d-elliott
force-pushed
the
feat-signals-struct
branch
from
04b5fef to
8bdcacc
Compare
james-d-elliott
force-pushed
the
feat-signals-struct
branch
from
8bdcacc to
827db4c
Compare
No description provided.