-
Notifications
You must be signed in to change notification settings - Fork 26
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
SDKS-2995_Device_Binding_Block_Simulator #260
SDKS-2995_Device_Binding_Block_Simulator #260
Conversation
…lator env and throwing an Abort error
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.
@george-bafaloukas-forgerock There are bunch of tests failing due to this change.
We need to add checks to skip those tests if run on the simulator (we have other tests with this approach)
However, are we sure we want to completely disable the support on simulator for device binding/signing? Everything perfectly works and supported for authentication type NONE. I think supporting NONE will help the developers to test their binding/signing code on the simulator. What do you think?
@@ -98,15 +98,15 @@ public struct CryptoKey { | |||
query[String(kSecReturnRef)] = true | |||
query[String(kSecAttrApplicationTag)] = keyAlias | |||
|
|||
#if !targetEnvironment(simulator) | |||
|
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.
Update copyright
@@ -167,7 +166,11 @@ open class DeviceBindingCallback: MultipleValuesCallback, Binding { | |||
deviceId: String? = nil, | |||
deviceRepository: DeviceBindingRepository = LocalDeviceBindingRepository(), | |||
_ completion: @escaping DeviceBindingResultCallback) { | |||
|
|||
#if targetEnvironment(simulator) |
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.
Update copyright
@@ -174,7 +174,11 @@ open class DeviceSigningVerifierCallback: MultipleValuesCallback, Binding { | |||
authInterface: DeviceAuthenticator, | |||
customClaims: [String: Any] = [:], | |||
_ completion: @escaping DeviceSigningResultCallback) { | |||
|
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.
Update copyright
@george-bafaloukas-forgerock I have refactored your changes on tests to use |
Instead of skipping the tests, I changed them to check for Unsupported error when being run on the simulator. |
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.
Changes looks good to me
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.
Do we want to push these or sanitize?
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.
@george-bafaloukas-forgerock We want to include this test fix as well. I know it's not related to the existing fix, but as we were updating the failing tests as well, I included this one too.
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.
OK sounds good. Just wondering if we commit to dev/master the actual values. Happy if we are OK.
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.
LGTM!
(approving on behalf of George also)
Checking if the DeviceBinding/Signing callback is triggered in a simulator env and throwing an Abort error
JIRA Ticket
Please, link jira ticket here.
SDKS-2995
Description
Briefly describe the change and any information that would help speedup the review and testing process.
Definition of Done Checklist: