-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PM-9473] Add messaging for macOS passkey extension and desktop #10768
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #10768 +/- ##
==========================================
- Coverage 33.49% 33.47% -0.03%
==========================================
Files 2897 2916 +19
Lines 90351 91246 +895
Branches 17165 17357 +192
==========================================
+ Hits 30262 30541 +279
- Misses 57698 58299 +601
- Partials 2391 2406 +15 ☔ View full report in Codecov by Sentry. |
New Issues
Fixed Issues
|
// | ||
// If instead I make this a static, the deinit gets called correctly after each request. | ||
// I think we still might want a static regardless, to be able to reuse the connection if possible. | ||
static let client: MacOsProviderClient = { |
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.
TODO: Investigate why deinit
wasn't always called. Maybe we leave some request unanswered?
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.
I remember having similar issues when I was working on Passkey Provider PoCs. I got the feeling that swift was really bad at keeping track of references and/or really eager to garbage collect objects that are still in use. If that is the issue here then it makes sense that putting it in a static
variable solves it.
Long time ago though, so takes this with a pinch of salt
import { Command } from "../platform/main/autofill/command"; | ||
import { RunCommandParams, RunCommandResult } from "../platform/main/autofill/native-autofill.main"; | ||
|
||
export default { | ||
runCommand: <C extends Command>(params: RunCommandParams<C>): Promise<RunCommandResult<C>> => | ||
ipcRenderer.invoke("autofill.runCommand", params), | ||
|
||
listenPasskeyRegistration: ( |
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.
This is API just proxies the requests from NAPI to the renderer process, and we also pass a callback so the renderer can respond.
}: PickCredentialParams): Promise<{ cipherId: string; userVerified: boolean }> { | ||
this.logService.warning("pickCredential", cipherIds, userVerification); | ||
|
||
return { cipherId: cipherIds[0], userVerified: userVerification }; |
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.
When doing assertion, the user already selected a cipher from the quicktype bar, so we've already set allowCredentialDescriptorList
to that credential ID, which means cipherIds should only contain the correct value.
|
||
// TODO: For some reason the credentialId is passed as an empty array in the request, so we need to | ||
// get it from the cipher. For that we use the recordIdentifier, which is the cipherId. | ||
if (request.recordIdentifier && request.credentialId.length === 0) { |
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.
TODO: Check why the provideCredentialWithoutUserInteraction
on the extension doesn't provide the credentialId. It's not a big deal because we have the cipher ID, but there's no reason why it's not returning it.
}); | ||
} | ||
|
||
private convertRegistrationRequest( |
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.
I'd appreciate a second look at these four conversion methods, there's a few fields that are left undefined because the swift extension doesn't have related values, and some values I had to set myself.
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.
Added some minor comments but other than that I think it looks good!
libs/common/src/platform/services/fido2/fido2-autofill-utils.ts
Outdated
Show resolved
Hide resolved
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.
Quality work! 💯
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.
question: would it have been easier to compile into a regular C compatible library and use C <> ObjC <> Swift
interop? Mostly I'm just wondering if you considered that approach and if/why you decided against it
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.
Oh ok nevermind, we get uniffi support. Do you think it would be worth to try and use this for the objective-c part?
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.
In this case I mostly used it because we needed to deal with callbacks and holding a reference to the object from the swift side, so I avoided using C-interop because I was worried about representing those callbacks in plain C, and dealing with memory leaks and undefined behavior.
The objective-c part is simpler as it's just calling a couple of functions, so for that I think all the boilerplate from uniffi might be too much.
info!( | ||
"Time to process request: {:?}", | ||
request_start_time.elapsed() | ||
); |
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.
question: are you thinking we should keep this in production or was it just something we added do debug the slow requests?
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.
Yeah this is mostly for debug purposes, I was waiting until the PR removing sync was merged to test it again.
That said it woulnd't hurt to leave it, either.
apps/desktop/src/platform/services/native-autofill.renderer.service.ts
Outdated
Show resolved
Hide resolved
alg, | ||
type: "public-key", | ||
})), | ||
// TODO: Marked as optional but needs to be defined? |
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.
optional in the client but apparently not optional in the authenticator, can't remember why 🤷, we can just leave it like this for now. Future macOS versions will provides this though
requireUserVerification: | ||
request.userVerification === "required" || request.userVerification === "preferred", |
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.
question: can this ever be undefined? If yes, then that should be treated as preferred
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.
It shouldn't ever be, this struct is coming from Rust and there UserVerification is not an optional field
}); | ||
} | ||
|
||
private convertRegistrationRequest( |
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.
Added some minor comments but other than that I think it looks good!
libs/common/src/platform/services/fido2/fido2-autofill-utils.ts
Outdated
Show resolved
Hide resolved
ed0cf93
to
0a0c3ef
Compare
4c91e50
to
212519a
Compare
9836079
to
030e138
Compare
212519a
to
fe2c6cd
Compare
030e138
to
a4594f6
Compare
fe2c6cd
to
0c8e074
Compare
0c8e074
to
6262a59
Compare
a4594f6
to
d20ded9
Compare
6262a59
to
6c50198
Compare
d20ded9
to
cb2f7c3
Compare
6c50198
to
646c0bc
Compare
646c0bc
to
52a6760
Compare
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.
@dani-garcia @coroiu thanks for all the hard work on this! I am still very new to many concepts being introduced here 😅
Mostly looks good, just a couple of questions.
apps/browser/src/autofill/fido2/services/browser-fido2-user-interface.service.ts
Show resolved
Hide resolved
Co-authored-by: Colton Hurst <[email protected]>
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.
Approving my own changes, good job Andreas!
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-9473
📔 Objective
📸 Screenshots
⏰ 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