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.timeoutInMilliseconds = 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 @@ -38,11 +62,9 @@ public struct PublicKeyCredentialCreationOptions: Encodable, Sendable {
/// preferred.
public let publicKeyCredentialParameters: [PublicKeyCredentialParameters]

/// A time, in seconds, that the caller is willing to wait for the call to complete. This is treated as a
/// A time, in milliseconds, 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 timeoutInMilliseconds: Int64?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this was intentional so we expose a more Swift-native API for timing, and the struct encodes with milliseconds for easy integration with JS.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, makes sense. I'll adjust that one.

Any thoughts on how to introduce this change, given that it might change behaviour for those who are relying on the current implementation? My thoughts:

  • A version change, I think this is quite a critical part of the lib, so might be the proper course. This would require immediate action from clients when they update the package.
  • I could come up with some layer of abstraction (maybe a protocol or a class-cluster/factory like approach) that provides current behaviour by default, and the extended behaviour when some option is specified or a specific initialiser is called. This would require zero effort from clients, but complicates the code base a little bit and keeps essentially an incompletely implemented spec alive.
  • A combination of both above, where the new default is the extended version, but current version can still be requested as well. This would require little, but non-zero effort of clients using the current version when updating the library. This could be mitigated by changing some function signature (beginRegistration comes to mind) to take an option so that when the library gets updated, clients will have to specify which behaviour the want

Copy link
Collaborator

Choose a reason for hiding this comment

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

The package is in prerelease, so breaking changes can still be made (and probably should if they improve the API). That said, not sure what about these changes (unless there are more) would incur breaking changes — it mostly seems like a collection of added properties more than anything, all of which could have sensible defaults?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that is what I am thinking as well.
However: maybe, some implementations were relaying for example on the effect on android that I found undesirable: users being able to authenticate once, but unable to re-use that credential on subsequent logins.


/// 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(timeoutInMilliseconds, 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,
attestation: attestation
)
}
Expand Down