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.

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

Expanded PublicKeyCredentialCreationOptions #76

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,30 @@ import Foundation
///
/// - SeeAlso: https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions
public struct PublicKeyCredentialCreationOptions: Encodable, Sendable {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove.

init(challenge: [UInt8],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this was meant to be public, but the internal initializer should be synthesized automatically based on default values on members. If you means to make this public, it would be better in an extension.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, note that PublicKeyCredentialCreationOptions is not intended to be instanciated directly — you are expected to request one from the manager, so many of these options likely belong there instead.

Copy link
Author

Choose a reason for hiding this comment

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

Fair point, I'll adjust this one to be in line with your suggestions.

Copy link
Author

Choose a reason for hiding this comment

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

Updated in 3a2815e1dd3bdd07bbbb57c311eac272da05bb75

user: PublicKeyCredentialUserEntity,
relyingParty: PublicKeyCredentialRelyingPartyEntity,
publicKeyCredentialParameters: [PublicKeyCredentialParameters],
timeout: Int64?,
attestation: AttestationConveyancePreference,
hints: [Hint] = [],
extensions: Extensions = .init(credProps: true),
excludeCredentials: [Credentials] = [],
authenticatorSelection: AuthenticatorSelection = .init(residentKey: .preferred,
requireResidentKey: false,
userVerification: .preferred)){
self.challenge = challenge
self.user = user
self.relyingParty = relyingParty
self.publicKeyCredentialParameters = publicKeyCredentialParameters
self.timeout = timeout
self.attestation = attestation
self.hints = hints
self.extensions = extensions
self.excludeCredentials = excludeCredentials
self.authenticatorSelection = authenticatorSelection
}
/// A byte array randomly generated by the Relying Party. Should be at least 16 bytes long to ensure sufficient
/// entropy.
///
Expand All @@ -40,9 +64,7 @@ public struct PublicKeyCredentialCreationOptions: Encodable, Sendable {

/// A time, in seconds, that the caller is willing to wait for the call to complete. This is treated as a
/// hint, and may be overridden by the client.
///
/// - Note: When encoded, this value is represented in milleseconds as a ``UInt32``.
Comment on lines -42 to -43
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove

public let timeout: Duration?
public let timeout: Int64?

/// Sets the Relying Party's preference for attestation conveyance. At the time of writing only `none` is
/// supported.
Expand All @@ -55,17 +77,87 @@ public struct PublicKeyCredentialCreationOptions: Encodable, Sendable {
try container.encode(user, forKey: .user)
try container.encode(relyingParty, forKey: .relyingParty)
try container.encode(publicKeyCredentialParameters, forKey: .publicKeyCredentialParameters)
try container.encodeIfPresent(timeout?.milliseconds, forKey: .timeout)
try container.encodeIfPresent(timeout, forKey: .timeoutInMilliseconds)
try container.encode(attestation, forKey: .attestation)
try container.encode(authenticatorSelection, forKey: .authenticatorSelection)
try container.encode(hints, forKey: .hints)
try container.encode(extensions, forKey: .extensions)
try container.encode(excludeCredentials, forKey: .excludeCredentials)
}

let hints: [Hint]
enum Hint: String, Encodable
Copy link
Collaborator

@dimitribouniol dimitribouniol Aug 19, 2024

Choose a reason for hiding this comment

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

Rather than an enum, which can never take new values after the library gets a public release, please use UnreferencedStringEnumeration: https://github.com/swift-server/webauthn-swift/blob/75c32a7730188646dfae7c44adcba30242bdf220/Sources/WebAuthn/Ceremonies/Shared/AuthenticatorAttachment.swift#L21

Copy link
Author

Choose a reason for hiding this comment

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

Where can I find out more about enums being problematic after public releases? I would like to learn more about this.
Meanwhile, I'll adjust according to your suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is described in the motivation section here: https://forums.swift.org/t/pitch-non-frozen-enumerations/68373

Copy link
Author

Choose a reason for hiding this comment

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

Ok, thank you for sharing, I'll read up on that, never thought of enums being risky 😃

Copy link
Author

Choose a reason for hiding this comment

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

Done as part of 5f8974df64c68188297a6841e77df797bb11ed70

{
/// Hint to get the user to register their platform authenticator
case clientDevice = "client-device"

/// Hint to the user should be guided to register a security key. Iconography and text should emphasize the use of security keys
case securityKey = "security-key"

/// Hint to the user to to register a passkey using their mobile device by scanning a QR code that’s displayed on a computer
case hybrid = "hybrid"
}

let extensions: Extensions

struct Extensions: Encodable
{
Copy link
Collaborator

@dimitribouniol dimitribouniol Aug 19, 2024

Choose a reason for hiding this comment

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

nit, based on formatting in the rest of the repo:

Suggested change
struct Extensions: Encodable
{
struct Extensions: Encodable {

Copy link
Collaborator

@dimitribouniol dimitribouniol Aug 19, 2024

Choose a reason for hiding this comment

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

Also, it might make a lot more sense to make dedicated public types for all of these in their own files, so we can properly model them in the rest of the code

Copy link
Author

Choose a reason for hiding this comment

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

Ah yeah, I'm a bit of an outlier with how I format my braces, used to having a linter 'fix' that on a pre-commit hook 😃 .

About the public types: I was thinking that could be considered clutter, as those types are not really needed (yet?) anywhere outside of this struct?
Happy to make the change of course, but: are you sure?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a suggestion from my part, as I'm not an actual maintainer, but having seen the spec in its entirety, I think its warranted, plug it keeps this file highly focused.

Copy link
Author

Choose a reason for hiding this comment

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

Also, it might make a lot more sense to make dedicated public types for all of these in their own files, so we can properly model them in the rest of the code

Done as part of 5f8974df64c68188297a6841e77df797bb11ed70. It seems the rest of the repo keeps the types mostly in the same file, so in keeping with that, the types are no dedicated public, but still live in the same file.

let credProps: Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we use a CodingKeys declaration to give this type a nicer name in swift? Without looking into the spec, I have no clue what this refers to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, I might suggest linking directly to the spec as much as possible for all new types, like I did here, so others have an easy way back to understanding what these fields are: https://github.com/swift-server/webauthn-swift/blob/75c32a7730188646dfae7c44adcba30242bdf220/Sources/WebAuthn/Ceremonies/Shared/AuthenticatorAttachment.swift#L15-L20

Copy link
Author

@Joride Joride Aug 19, 2024

Choose a reason for hiding this comment

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

Yeah that is a good idea to use codingkeys to have a better name in the library for this. Currently I absolutely have not enough understanding of the spec to be able to convert the below statement into something more descriptive. Any suggestions?

WebAuthn Extension Identifiers defines the initial set of extensions to be registered in the IANA Registry "WebAuthn Extension Identifier" registry established by WebAuthn-Registries.

These MAY be implemented by User-agents targeting broad interoperability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://www.w3.org/TR/webauthn-2/#sctn-authenticator-credential-properties-extension

I would suggest credentialPropertiesExtension, set to a CredentialPropertiesExtension.RequestOptions type with .requested and .ignore options that render the appropriate value to the encoded output (either true, or the key is skipped)

Copy link
Author

Choose a reason for hiding this comment

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

Not sure I follow. Conceptually, do you mean something like the following (except instead of enum I would use UnreferencedStringEnumeration):

let credentialPropertiesExtension: CredentialPropertiesExtension
struct CredentialPropertiesExtension: Encodable
{
    let options: RequestOptions
    
    enum RequestOptions: Encodable
    {
        case requested
        case ignored
    }
}

}

let excludeCredentials: [Credentials]
struct Credentials: Encodable
{
let id: String
let type: [UInt8]

private enum CodingKeys: String, CodingKey
{
case id
case type
}

public func encode(to encoder: Encoder) throws
{
var container = encoder.container(keyedBy: CodingKeys.self)

try container.encode(type.base64URLEncodedString(), forKey: .id)
try container.encode(id, forKey: .id)
}
}

let authenticatorSelection: AuthenticatorSelection
struct AuthenticatorSelection: Encodable
{
enum ResidentKey: String, Encodable
{
case required = "required"
case preferred = "preferred"
case discouraged = "discouraged"
}

enum UserVerification: String, Encodable
{
case required = "required"
case preferred = "preferred"
case discouraged = "discouraged"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for these, please use UnreferencedStringEnumeration

Copy link
Author

Choose a reason for hiding this comment

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

Done as part of 5f8974df64c68188297a6841e77df797bb11ed70

let residentKey: ResidentKey
let requireResidentKey: Bool
let userVerification: UserVerification
}

private enum CodingKeys: String, CodingKey {
case challenge
case user
case relyingParty = "rp"
case publicKeyCredentialParameters = "pubKeyCredParams"
case timeout
case timeoutInMilliseconds = "timeout"
case attestation
case authenticatorSelection
case hints
case extensions
case excludeCredentials
}
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/WebAuthn/WebAuthnManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public struct WebAuthnManager: Sendable {
user: user,
relyingParty: .init(id: configuration.relyingPartyID, name: configuration.relyingPartyName),
publicKeyCredentialParameters: publicKeyCredentialParameters,
timeout: timeout,
timeout: (timeout?.milliseconds ?? 0) / 1000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should still be unnecessary?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, updated in commit ee000536baa6801d50b9c4a1242f43a9be1d71f1

attestation: attestation
)
}
Expand Down