-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
3d084c2
b103adb
ee00053
3a2815e
5f8974d
1fdd9ee
3db39d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -21,6 +21,30 @@ import Foundation | |||||||
/// | ||||||||
/// - SeeAlso: https://www.w3.org/TR/webauthn-2/#dictionary-makecredentialoptions | ||||||||
public struct PublicKeyCredentialCreationOptions: Encodable, Sendable { | ||||||||
|
||||||||
init(challenge: [UInt8], | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, note that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in |
||||||||
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. | ||||||||
/// | ||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove |
||||||||
public let timeout: Duration? | ||||||||
public let timeoutInMilliseconds: Int64? | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that is what I am thinking as well. |
||||||||
|
||||||||
/// Sets the Relying Party's preference for attestation conveyance. At the time of writing only `none` is | ||||||||
/// supported. | ||||||||
|
@@ -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 | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done as part of |
||||||||
{ | ||||||||
/// 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 | ||||||||
{ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, based on formatting in the rest of the repo:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Done as part of |
||||||||
let credProps: Bool | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||
} | ||||||||
|
||||||||
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" | ||||||||
} | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same for these, please use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done as part of |
||||||||
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 | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove.