[34253] - Add AuthorizationControllerWrapper & Unit Test for TwilioPasskey iOS. #21
Conversation
AlejandroOrozco
force-pushed
the
aorozcobuiles/ACCSEC-34253/PasskeysWrappersForUnitTests
branch
from
e80eb42
to
de93987
Compare
| // Then | ||
| when (result) { | ||
| is CreatePasskeyResult.Success -> { | ||
| fail("It shouldn't succeed") |
There was a problem hiding this comment.
| Line detected, which is longer than the defined maximum line length in the code style. |
There was a problem hiding this comment.
looks like a false positive
…wilioPasskey iOS.
AlejandroOrozco
force-pushed
the
aorozcobuiles/ACCSEC-34253/PasskeysWrappersForUnitTests
branch
from
de93987
to
375997d
Compare
| @@ -84,7 +84,7 @@ class AuthenticationManager: NSObject, ObservableObject { | |||
| ) { | |||
| Task { | |||
| do { | |||
| let result = try await worker.registrationVerification(request: .init( | |||
| _ = try await worker.registrationVerification(request: .init( | |||
There was a problem hiding this comment.
Is there something that can be checked in the verification response?
There was a problem hiding this comment.
Yeah definitely makes sense, I added a validation to check if the response was verified by the backend. Thank you.
| "name": "Example" | ||
| }, | ||
| "user": { | ||
| "id": "WUU0ZmQzYWFmNGU0NTMyNGQwZjNlMTM0NjA3YjIxOTEyYg", |
There was a problem hiding this comment.
This can be flaged as a secret, could you please change them to a placeholder?
There was a problem hiding this comment.
Definitely 😅, added Placeholder prefix just to avoid any confusion. But apart from that the length must be exact. Thank you
| * @property deviceUtils The utility class for device-related operations. | ||
| */ | ||
| class AuthorizationControllerWrapper : IAuthorizationControllerWrapper { | ||
| private var authController: ASAuthorizationController? = null |
There was a problem hiding this comment.
Could this be a lateinit var?
There was a problem hiding this comment.
Done, seems way clear with that one. Thank you
Generated by 🚫 Danger |
Generated by 🚫 Danger |
🧛 shared module Code Coverage:
|
| File | Coverage |
|---|---|
TwilioPasskey.kt |
97.38% |
Modified Files Not Found In Coverage Report:
AuthenticationManager.swift
AuthorizationControllerWrapper.kt
AuthorizationControllerWrapperMock.kt
PayloadMocks.kt
TwilioPasskeyTest.kt
Codebase cunningly covered by count Shroud 🧛
Generated by 🚫 Danger
Generated by 🚫 Danger |
| */ | ||
| actual class TwilioPasskey private constructor( | ||
| actual class TwilioPasskey internal constructor( |
There was a problem hiding this comment.
Can mapToTwilioException be part of this class instead of the wrapper? As it is a TwilioException, this could be more related to this class and the wrapper can pass the original error
| "name": "Example" | ||
| }, | ||
| "user": { | ||
| "id": "PLACEHOLDERIDWUU0ZmQzYWFmNGU0NTMyNGQwZjNlMTM0N", |
There was a problem hiding this comment.
Can this be something like PLACEHOLDERID without the letters? Or can the letters be ABCD.... instead of WUU0ZmQzY.....
There was a problem hiding this comment.
The size of the userId is being validated, so it requires to have the same size, maybe we can try with PLACEHOLDERID and spaces wdyt?
There was a problem hiding this comment.
So, we can complete the values with ABCDEFG......., so it will be added as the alphabet (known order)
| "name": "user1", | ||
| "displayName": "User One" | ||
| }, | ||
| "challenge": "WUYwNDhkMWE3ZWMzYTJhNjk3MDA1OWMyNzY2YmJjN2UwZg", |
There was a problem hiding this comment.
Same as above, this can be flagged as a secret
| const val TIMEOUT = 600000L | ||
| val keyCredential = | ||
| KeyCredential( | ||
| id = "6ySmhJd6qGUMCthiqszyb4Od4U6TFn0v3DLz-1EZrNQ", |
There was a problem hiding this comment.
This can be flagged as a secret
There was a problem hiding this comment.
Same for values below RAW_ID, ATTESTATION_OBJECT, AUTHENTICATE_CHALLENGE, CLIENT_DATA_JSON_CREATE, SIGNATURE, USER_HANDLE. If we can use placeholder without letters will be better, if we need the values encoded or following a format, we should use dummy values so they are not considered secrets
| controller: ASAuthorizationController, | ||
| didCompleteWithAuthorization: ASAuthorization, | ||
| ) { | ||
| val credentialRegistration = didCompleteWithAuthorization.credential as ASAuthorizationPlatformPublicKeyCredentialRegistration |
There was a problem hiding this comment.
Is there a way to pass the credentialRegistration to TwilioPasskey and applying the logic below in TwilioPasskey? AuthorizationControllerWrapper could be in charge of completing the interaction / needed code for iOS authorization, but some logic can be kept in TwilioPasskey, so it can be tested
There was a problem hiding this comment.
No unfortunately that ASAuthorizationPlatformPublicKeyCredentialRegistration contains a couple of Objects that can't be initialized outside of Apple private logic. So It won't be possible to create that object from Scratch to use it as a mock.
Ticket
Github Issue
Description
Commit message
Screenshot
Testing