Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(protocol): allow absent relying party id #166

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(protocol): allow absent relying party id #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f79567
feat(protocol): allow absent relying party id
james-d-elliott Oct 1, 2023
09686bd
test: add tests
james-d-elliott Oct 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion protocol/entities.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ type CredentialEntity struct {
type RelyingPartyEntity struct {
CredentialEntity
// A unique identifier for the Relying Party entity, which sets the RP ID.
ID string `json:"id"`
ID string `json:"id,omitempty"`
}

// The UserEntity represents the PublicKeyCredentialUserEntity IDL and is used to supply additional user account
Expand Down
27 changes: 18 additions & 9 deletions webauthn/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ type WebAuthn struct {
// Config represents the WebAuthn configuration.
type Config struct {
// RPID configures the Relying Party Server ID. This should generally be the origin without a scheme and port.
// If absent the browser will automatically determine this using standard conventions.
RPID string

// RPDisplayName configures the display name for the Relying Party Server. This can be any string.
Expand Down Expand Up @@ -101,20 +102,28 @@ func (config *Config) validate() error {
return fmt.Errorf(errFmtFieldEmpty, "RPDisplayName")
}

if len(config.RPID) == 0 {
return fmt.Errorf(errFmtFieldEmpty, "RPID")
}

var err error

if _, err = url.Parse(config.RPID); err != nil {
return fmt.Errorf(errFmtFieldNotValidURI, "RPID", err)
var uri *url.URL

if len(config.RPID) != 0 {
if uri, err = url.Parse(config.RPID); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if parsing RPID makes sense? This should be generally be a domain name? So parsing as URL has little sense. I see that it was done like this before.

Spec here mentions:

Though, the RP ID syntaxes MUST conform to either valid domain strings or URIs [RFC3986] [URL].

But the later, URIs is only allowed for "Other specifications mimicking the WebAuthn API to enable WebAuthn public key credentials on non-Web platforms".

So I am not sure, but there is no requirement that it should not be absolute?

return fmt.Errorf(errFmtFieldNotValidURI, "RPID", err)
}

if uri.IsAbs() {
return fmt.Errorf("field '%s' is an absolute URI but it must not be an absolute URI", "RPID")
}
}

if config.RPIcon != "" {
if _, err = url.Parse(config.RPIcon); err != nil {
if len(config.RPIcon) != 0 {
if uri, err = url.Parse(config.RPIcon); err != nil {
return fmt.Errorf(errFmtFieldNotValidURI, "RPIcon", err)
}

if !uri.IsAbs() {
return fmt.Errorf("field '%s' is not an absolute URI but it must be an absolute URI", "RPIcon")
}
}

defaultTimeoutConfig := defaultTimeout
Expand All @@ -141,7 +150,7 @@ func (config *Config) validate() error {
config.Timeouts.Registration.TimeoutUVD = defaultTimeoutUVDConfig
}

if len(config.RPOrigin) > 0 {
if len(config.RPOrigin) != 0 {
if len(config.RPOrigins) != 0 {
return fmt.Errorf("deprecated field 'RPOrigin' can't be defined at the same tme as the replacement field 'RPOrigins'")
}
Expand Down
182 changes: 182 additions & 0 deletions webauthn/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
package webauthn

import (
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestConfigValidateErr(t *testing.T) {
testCases := []struct {
name string
have *Config
err string
check func(t *testing.T, have *Config)
}{
{
"ShouldNotErrorOnStandardConfig",
&Config{
RPID: "example.com",
RPDisplayName: "example",
RPOrigins: []string{
"https://example.com",
},
},
"",
nil,
},
{
"ShouldErrorOnAbsoluteRPID",
&Config{
RPID: "https://example.com",
RPDisplayName: "example",
RPOrigins: []string{
"https://example.com",
},
},
"field 'RPID' is an absolute URI but it must not be an absolute URI",
nil,
},
{
"ShouldSkipValidation",
&Config{
validated: true,
},
"",
nil,
},
{
"ShouldErrorOnBadRPIcon",
&Config{
RPID: "example.com",
RPIcon: "exa$##$#@$@#%^@#mple.com",
RPDisplayName: "example",
RPOrigins: []string{
"https://example.com",
},
},
"field 'RPIcon' is not a valid URI: parse \"exa$##$#@$@#%^@#mple.com\": invalid URL escape \"%^@\"",
nil,
},
{
"ShouldErrorOnBadRPIconAbsolute",
&Config{
RPID: "example.com",
RPIcon: "example.com",
RPDisplayName: "example",
RPOrigins: []string{
"https://example.com",
},
},
"field 'RPIcon' is not an absolute URI but it must be an absolute URI",
nil,
},
{
"ShouldSetFallbackRPOriginAndNotErr",
&Config{
RPID: "example.com",
RPDisplayName: "example",
RPOrigin: "https://example.com",
RPOrigins: []string{},
},
"",
func(t *testing.T, have *Config) {
require.Len(t, have.RPOrigins, 1)
assert.Equal(t, "https://example.com", have.RPOrigins[0])
},
},
{
"ShouldNotErrorOnConfigWithoutRPID",
&Config{
RPDisplayName: "example",
RPOrigins: []string{
"https://example.com",
},
},
"",
nil,
},
{
"ShouldErrorOnNoDisplayName",
&Config{
RPID: "example.com",
RPOrigins: []string{
"https://example.com",
},
},
"the field 'RPDisplayName' must be configured but it is empty",
nil,
},
{
"ShouldErrorOnNoOrigins",
&Config{
RPID: "example.com",
RPDisplayName: "example",
RPOrigins: []string{},
},
"must provide at least one value to the 'RPOrigins' field",
nil,
},
{
"ShouldErrorOnInvalidRPID",
&Config{
RPID: "exa$##$#@$@#%^@#mple.com",
RPDisplayName: "example",
RPOrigins: []string{
"https://example.com",
},
},
"field 'RPID' is not a valid URI: parse \"exa$##$#@$@#%^@#mple.com\": invalid URL escape \"%^@\"",
nil,
},
{
"ShouldErrorOnDeprecatedAndNewRPOrigins",
&Config{
RPID: "example.com",
RPDisplayName: "example",
RPOrigin: "https://example.com",
RPOrigins: []string{
"https://example.com",
},
},
"deprecated field 'RPOrigin' can't be defined at the same tme as the replacement field 'RPOrigins'",
nil,
},
{
"ShouldSetDefaultTimeoutValues",
&Config{
RPID: "example.com",
RPDisplayName: "example",
RPOrigins: []string{
"https://example.com",
},
Timeout: int(time.Second.Milliseconds()),
},
"",
func(t *testing.T, have *Config) {
assert.Equal(t, time.Second, have.Timeouts.Login.Timeout)
assert.Equal(t, time.Second, have.Timeouts.Login.TimeoutUVD)
assert.Equal(t, time.Second, have.Timeouts.Registration.Timeout)
assert.Equal(t, time.Second, have.Timeouts.Registration.TimeoutUVD)
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := tc.have.validate()

if len(tc.err) == 0 {
assert.NoError(t, err)
} else {
assert.EqualError(t, err, tc.err)
}

if tc.check != nil {
tc.check(t, tc.have)
}
})
}
}