[PM-26177] Implement device-bound PRF passkey without SDK #2009
Conversation
… it expects to be valid / not expired in order to be passed. So TODO to add validation before passing to SDK.
… so when it happens it just shows the general error message to avoid cryptic messages be shown to end users as they have the share sheet now.
|
Great job! No new security vulnerabilities introduced in this pull request |
| import Foundation | ||
|
|
||
| extension Data { | ||
| public func base64UrlEncodedString(trimPadding: Bool? = true) -> String { |
There was a problem hiding this comment.
🤔 Is this function really necessary when you can just .base64EncodedString().urlEncoded()? If we really care about whether or not we trim the padding, the current function can just be modified, I think?
|
|
||
| extension Data { | ||
| public func base64UrlEncodedString(trimPadding: Bool? = true) -> String { | ||
| let shouldTrim = if trimPadding != nil { trimPadding! } else { true } |
There was a problem hiding this comment.
📝 You can just say trimPadding == true, which covers the nil case already.
🤔 But why is trimPadding even optional in the first place? It seems to me like it shouldn't be, especially if we give it a default value.
| } | ||
| } | ||
|
|
||
| private func normalizeBase64Url(_ str: String) -> String { |
There was a problem hiding this comment.
🤔 I don't think we like having global functions if at all possible. This really feels like something that should be an extension on String to me
| let masterPasswordHash: String? | ||
| let otp: String? | ||
|
|
||
|
|
There was a problem hiding this comment.
⛏️ Missing a MARK for initializers
|
|
||
| // MARK: Properties | ||
|
|
||
| let authRequestAccessCode: String? |
There was a problem hiding this comment.
🎨 I think these properties should all have documentation comments
| } | ||
| guard let prf else { return nil } | ||
|
|
||
| let encoder = JSONEncoder() |
There was a problem hiding this comment.
🎨 Why not use our standard encoder?
| guard let prf else { return nil } | ||
|
|
||
| let encoder = JSONEncoder() | ||
| let salt2 = if let salt2 = prf.1 { |
There was a problem hiding this comment.
🎨 let salt2 = if let salt2 feels really weird to me
| let record = try decoder.decode(DevicePasskeyRecord.self, from: json.data(using: .utf8)!) | ||
| identities.append(ASPasskeyCredentialIdentity( | ||
| relyingPartyIdentifier: record.rpId, | ||
| userName: record.userName!, |
There was a problem hiding this comment.
🎨 We try to avoid force unwrapping if possible, as that crashes the app
| passkeyRequest: passkeyRequest, credentialIdentity: credentialIdentity | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
🎨 Blank lines shouldn't have spaces in them
| ) | ||
| .getAssertion(request: request) | ||
| let devicePasskeyResult = try await devicePasskeyService.useDevicePasskey(for: request) | ||
| let (assertionResult, prfResult): (GetAssertionResult, Data?) = if let devicePasskeyResult { |
There was a problem hiding this comment.
🤔 I feel like this would be better as a struct than a tuple, personally
|
Re: the above—I can't speak to the specifics of the implementation, but it looks fine. The comments mostly a bunch of papercut stuff where things are out of line with our preferred code style and documentation, along with some force-unwrapping that would probably be best to not have. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2009 +/- ##
===========================================
- Coverage 81.32% 48.91% -32.42%
===========================================
Files 832 289 -543
Lines 52457 12993 -39464
===========================================
- Hits 42663 6355 -36308
+ Misses 9794 6638 -3156 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you for the review @KatherineInCode! This is my first time writing Swift, so your comments are very helpful. I still need to do the companion PR to compare an alternative implementation before the Auth team can consider taking on this feature at all, so I'll come back to address these comments after I've written the other PR. |
For sure. And I also apologize a little—I thought this was further along than it was when I dropped all my comments. For the next one I'll wait a bit until it's in a more-final state. |
iinuwa
force-pushed
the
auth/PM-26177/ios-device-passkey
branch
from
58ccc38 to
192e272
Compare
🎟️ Tracking
PM-26177
📔 Objective
This is a demo implementation for discussion of a device-bound PRF passkey implementation. This implements the WebAuthn create and get operations manually inside of Swift rather than using functionality from the SDK. This is because the current FIDO authenticator API in the SDK does not allow easily injecting passkey key material from outside of the vault. Another PR showing an alternative implementation using the SDK will follow for comparison.
Note: This depends on the following changes to be merged before this can function:
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes