implement start passkey enrollment- updated #15162
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback. |
Generated by 🚫 Danger |
| requestConfiguration: requestConfiguration | ||
| ) | ||
| let response = try await backend.call(with: request) | ||
| passkeyName = (name?.isEmpty ?? true) ? "Unnamed account (Apple)" : name! |
There was a problem hiding this comment.
Should we use guard or if let construct here to prevent force unwrapping similar to how we did in StartPasskeyEnrollmentRequest?
Also nit: We should either do it in the top of the method as a input validation and assignment or just before we use it so that we can look at it's value where it is being used. In this particular case, I would lean in to do it in the beginning of the method as this is a class level variable.
There was a problem hiding this comment.
actually force unwrapping is not neccesarily required here. we can safely remove it (also it wont go in that condition anyways, i added it just to be on the safe side). Removed force execution and used guard instead.
also Pavan, thanks for pointing that correctly but we are assigning the value just before we use it (in the just below registration request). I added it below the startEnrollment backend call because incase the backend call fails, we dont need to store the passkey name in that case.
| ) | ||
| let registrationRequest = provider.createCredentialRegistrationRequest( | ||
| challenge: challengeInData, | ||
| name: passkeyName ?? "Unnamed account (Apple)", |
There was a problem hiding this comment.
Is this required? We are just setting passkeyName above with default value. Is there a case where passkeyName would still be not populated?
There was a problem hiding this comment.
Thanks for pointing this out Pavan, removed the optional from here.
| name: passkeyName ?? "Unnamed account (Apple)", | ||
| userID: userIdInData | ||
| ) | ||
| return registrationRequest |
There was a problem hiding this comment.
I feel we can directly return from the previous statement, instead of storing.
| requestConfiguration: requestConfiguration | ||
| ) | ||
| let response = try await backend.call(with: request) | ||
| passkeyName = (name?.isEmpty ?? true) ? "Unnamed account (Apple)" : name! |
There was a problem hiding this comment.
We should store "Unnamed account (Apple)" as a constant.
There was a problem hiding this comment.
sure Pavan, created a constant defaultPasskeyName to store this. refactored.
| func testInitWithMissingRpThrowsError() { | ||
| var dict = makeValidDictionary() | ||
| if var options = dict["credentialCreationOptions"] as? [String: Any] { | ||
| options.removeValue(forKey: "rp") | ||
| dict["credentialCreationOptions"] = options as? AnyHashable | ||
| } | ||
| XCTAssertThrowsError(try StartPasskeyEnrollmentResponse(dictionary: dict)) | ||
| } | ||
|
|
||
| func testInitWithMissingRpIdThrowsError() { | ||
| var dict = makeValidDictionary() | ||
| if var options = dict["credentialCreationOptions"] as? [String: Any], | ||
| var rp = options["rp"] as? [String: Any] { | ||
| rp.removeValue(forKey: "id") | ||
| options["rp"] = rp | ||
| dict["credentialCreationOptions"] = options as? AnyHashable | ||
| } | ||
| XCTAssertThrowsError(try StartPasskeyEnrollmentResponse(dictionary: dict)) | ||
| } | ||
|
|
||
| func testInitWithMissingUserThrowsError() { | ||
| var dict = makeValidDictionary() | ||
| if var options = dict["credentialCreationOptions"] as? [String: Any] { | ||
| options.removeValue(forKey: "user") | ||
| dict["credentialCreationOptions"] = options as? AnyHashable | ||
| } | ||
| XCTAssertThrowsError(try StartPasskeyEnrollmentResponse(dictionary: dict)) | ||
| } | ||
|
|
||
| func testInitWithMissingUserIdThrowsError() { | ||
| var dict = makeValidDictionary() | ||
| if var options = dict["credentialCreationOptions"] as? [String: Any], | ||
| var user = options["user"] as? [String: Any] { | ||
| user.removeValue(forKey: "id") |
There was a problem hiding this comment.
Optional suggestion: Should we club all these test cases together and say testInitWithInvalid parameters and then loop them inside for each type of object as I see the testing as assertion part is same for all.
There was a problem hiding this comment.
I’ve added a new test case in this file to validate all these parameters together and assert them collectively. (I had tried it earlier to make these tests more effficient but was encountering some errors so for now I added this extra case.)
There was a problem hiding this comment.
Updated the tests as discussed.
| options.removeValue(forKey: "challenge") | ||
| dict["credentialCreationOptions"] = options as? AnyHashable | ||
| } | ||
| XCTAssertThrowsError(try StartPasskeyEnrollmentResponse(dictionary: dict)) |
There was a problem hiding this comment.
Should we also assert the error type to make sure code is not throwing error due to some other reason here? Similar for other testcases:
Something like below
XCTAssertThrowsError(try StartPasskeyEnrollmentResponse(dictionary: dict)) { error in
XCTAssertEqual(error.code, "expected code or message here")
}
There was a problem hiding this comment.
Sure Pavan, updated the tests to assert specific errors.
No description provided.