Expanded `PublicKeyCredentialCreationOptions` #76

Joride · 2024-08-19T10:49:40Z

Hey folks,

This might be a change that brings changes to clients using the library, so please review carefully. Maybe this one requires a version-change? We could opt to maintin existing behaviour, although I would be careful with that option as well, as effectively clients are currently using an incomplete implementation of the spec? Open to hearing what direction to take and adjust as necessary.

I added some code so the options returned from beginRegistration now contain more fields from the spec.
Without these changes, I noticed that on Android, passkey-authentication could be used, however, after signup, the passkey did not seem to be stored anywhere. And so if a user was logged out, they could not sign in again with the created passkey.

With these changes, that issue is now resolved.

…m the spec - updated implementation of `beginRegistration(..)` supply milliseconds to a new initializer N.b. `beginRegistration(..)` now returns different fields (and so different JSON).

dimitribouniol · 2024-08-19T13:24:10Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

-    /// 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``.
-    public let timeout: Duration?
+    public let timeoutInMilliseconds: Int64?


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.

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

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?

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.

dimitribouniol · 2024-08-19T13:27:14Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

@@ -21,6 +21,30 @@ import Foundation
 ///
 /// - SeeAlso: https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions
 public struct PublicKeyCredentialCreationOptions: Encodable, Sendable {
+
+    init(challenge: [UInt8], 


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.

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.

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

Updated in 3a2815e1dd3bdd07bbbb57c311eac272da05bb75

dimitribouniol · 2024-08-19T13:29:26Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

+    }
+
+    let hints: [Hint]
+    enum Hint: String, Encodable


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

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.

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

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

Done as part of 5f8974df64c68188297a6841e77df797bb11ed70

dimitribouniol · 2024-08-19T13:30:24Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

+        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"
+        }


Same for these, please use UnreferencedStringEnumeration

Done as part of 5f8974df64c68188297a6841e77df797bb11ed70

dimitribouniol · 2024-08-19T13:31:05Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

+
+    struct Extensions: Encodable
+    {
+        let credProps: Bool


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.

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

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.

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)

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 } }

dimitribouniol · 2024-08-19T13:33:05Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

+    struct Extensions: Encodable
+    {


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

Suggested change

struct Extensions: Encodable

{

struct Extensions: Encodable {

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

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?

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.

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.

…ne with a suggestion from dimitribouniol: https://github.com/swift-server/webauthn-swift/pull/76/files/3d084c20a0a8753b8a6184490bff03810fc8a72d#r1721784750

dimitribouniol · 2024-08-19T15:03:11Z

Sources/WebAuthn/WebAuthnManager.swift

@@ -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,


This should still be unnecessary?

Yes, updated in commit ee000536baa6801d50b9c4a1242f43a9be1d71f1

…or timeout, of type `Duration` Follow up on this discussion: swift-server#76 (comment)

… moved defaults to `beginRegistration(..)` on `WebAuthnManager` see per suggestion: swift-server#76 (comment)

- removed some types that turned out to be redundant with existing ones - switched from using enums to using UnreferencedStringEnumeration addresses these discussion points swift-server#76 (comment) swift-server#76 (comment) swift-server#76 (comment)

Joride · 2024-10-17T05:44:15Z

Just checking in on this one, as I'm getting closer to going live using this package.
Any (rough) e.t.a. on when this will be merged in? Any pending adjustments maybe?

thoven87 · 2024-12-11T15:44:49Z

@dimitribouniol @Joride are they any particular reasons why this PR can't be merged? I wanted to build on this PR to expose aaguid, aaid so that relying party can filter passkey authenticators based on the fido metadata blob

Joride · 2024-12-11T15:49:33Z

From my perspective this is Ok from a functionality perspective. I've been using this change now for a few months, haven't run into any issues.

I think I addressed the various comments and suggestions, so I would be happy to see it merged.

dimitribouniol · 2024-12-11T15:50:33Z

I'll take a closer look reviewing it tomorrow morning when I get some time.

dimitribouniol

Changes notwithstanding, I'll need to first make a PR that fixes testing before this can go in.

dimitribouniol · 2024-12-11T16:57:23Z

Sources/WebAuthn/WebAuthnManager.swift

+            attestation: attestation,
+            hints: [],
+            extensions: .init(credProps: true),
+            excludeCredentials: [],
+            authenticatorSelection: .init(residentKey: .preferred,
+                                          requireResidentKey: false,
+                                          userVerification: .preferred)


I'll need to review the spec to make sure these won't cause issues in the default case, though at a glance, this could be breaking as it specifies more than what was there currently for all cases.

dimitribouniol · 2024-12-11T16:57:35Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

@@ -21,6 +21,7 @@ import Foundation
 ///
 /// - SeeAlso: https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions
 public struct PublicKeyCredentialCreationOptions: Encodable, Sendable {
+


Please remove.

dimitribouniol · 2024-12-11T16:57:42Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

-    ///
-    /// - Note: When encoded, this value is represented in milleseconds as a ``UInt32``.


Please remove

dimitribouniol · 2024-12-11T16:58:46Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

+struct Extensions: Encodable {
+    /// Indicate that this extension is requested by the Relying Party.
+    /// https://www.w3.org/TR/webauthn-3/#sctn-authenticator-credential-properties-extension
+    let credProps: Bool
+}
+
+struct  AuthenticatorSelectionCriteria: Encodable {
+    /// Specifies the extent to which the Relying Party desires to create a client-side discoverable credential.
+    /// https://www.w3.org/TR/webauthn-3/#dom-authenticatorselectioncriteria-residentkey
+    let residentKey: ResidentKeyRequirement
+
+    /// Relying Parties SHOULD set this to true if, and only if, `residentKey` is set to `required`.
+    /// https://www.w3.org/TR/webauthn-3/#dom-authenticatorselectioncriteria-requireresidentkey
+    let requireResidentKey: Bool
+
+    /// This member specifies the Relying Party's requirements regarding user verification for the create() operation.
+    /// https://www.w3.org/TR/webauthn-3/#dom-authenticatorselectioncriteria-userverification
+    let userVerification: UserVerificationRequirement
+}


These should be made public, and probably placed in their own files

dimitribouniol · 2024-12-11T16:59:35Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

+struct Extensions: Encodable {
+    /// Indicate that this extension is requested by the Relying Party.
+    /// https://www.w3.org/TR/webauthn-3/#sctn-authenticator-credential-properties-extension
+    let credProps: Bool
+}


This doesn't feel like the best Swift API for this, though not having looked deeper, I don't have a suggestion for improvement yet.

dimitribouniol · 2024-12-11T17:01:10Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

    public let timeout: Duration?

    /// Sets the Relying Party's preference for attestation conveyance. At the time of writing only `none` is
    /// supported.
    public let attestation: AttestationConveyancePreference
+
+    /// Use this enumeration to communicate hints to the user-agent about how a request may be best completed. These hints are not requirements, and do not bind the user-agent, but may guide it in providing the best experience by using contextual information that the Relying Party has about the request.
+    /// https://www.w3.org/TR/webauthn-3/#enum-hints


Minor nit, but would be good to follow this convention for references:

swift-webauthn/Sources/WebAuthn/Ceremonies/Shared/AuthenticatorAttestationGloballyUniqueID.swift

Line 21 in 0f31a51

/// - SeeAlso: [WebAuthn Leven 3 Editor's Draft §6. WebAuthn Authenticator Model](https://w3c.github.io/webauthn/#aaguid)

- expanded PublicKeyCredentialCreationOptions with more options fro…

3d084c2

…m the spec - updated implementation of `beginRegistration(..)` supply milliseconds to a new initializer N.b. `beginRegistration(..)` now returns different fields (and so different JSON).

dimitribouniol reviewed Aug 19, 2024

View reviewed changes

changed parameter from representing ms to representing seconds, in li…

b103adb

…ne with a suggestion from dimitribouniol: https://github.com/swift-server/webauthn-swift/pull/76/files/3d084c20a0a8753b8a6184490bff03810fc8a72d#r1721784750

dimitribouniol reviewed Aug 19, 2024

View reviewed changes

Joride added 3 commits August 22, 2024 15:43

reverting to using seconds on PublicKeyCredentialCreationOptions' f…

ee00053

…or timeout, of type `Duration` Follow up on this discussion: swift-server#76 (comment)

removed custom initializer from PublicKeyCredentialCreationOptions,…

3a2815e

… moved defaults to `beginRegistration(..)` on `WebAuthnManager` see per suggestion: swift-server#76 (comment)

Joride mentioned this pull request Aug 24, 2024

Roadmap to releasable/stable version? #77

Closed

Merge branch 'main' into PublicKeyCredentialCreationOptions-expansion

1fdd9ee

dimitribouniol requested changes Dec 11, 2024

View reviewed changes

		///
		/// - Note: When encoded, this value is represented in milleseconds as a ``UInt32``.

Expanded PublicKeyCredentialCreationOptions #76

Are you sure you want to change the base?

Expanded PublicKeyCredentialCreationOptions #76

Conversation

Joride commented Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitribouniol Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joride Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitribouniol Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

dimitribouniol Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joride commented Oct 17, 2024

thoven87 commented Dec 11, 2024

Joride commented Dec 11, 2024

dimitribouniol commented Dec 11, 2024

dimitribouniol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Expanded `PublicKeyCredentialCreationOptions` #76

Expanded `PublicKeyCredentialCreationOptions` #76

Joride commented Aug 19, 2024 •

edited

Loading

dimitribouniol Aug 19, 2024 •

edited

Loading

Joride Aug 19, 2024 •

edited

Loading

dimitribouniol Aug 19, 2024 •

edited

Loading

dimitribouniol Aug 19, 2024 •

edited

Loading