-
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-2761 #241
SDKS-2761 #241
Conversation
54f4d8e
to
f3709f1
Compare
d7cbd6b
to
d206c3f
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.
Looks good, a couple of questions nothing major
self.setVerification(result.assertKey) | ||
let result = try await FRAppAttestDomainModal.shared.requestIntegrityToken(challenge: challenge) | ||
self.setAttestation(result.appAttestKey) | ||
self.setAssertion(result.assertKey ?? "") |
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.
Should we through if you have no key? Or is there a valid reason that would happen?
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.
yes, that value can be either empty or have a value ..AM needs that information
but i can modify that logic like this. good find
if let assertkey = result.assertKey {
self.setAssertion(assertkey)
}
return FRAppIntegrityKeys(attestKey: attest.base64EncodedString(), | ||
assertKey: assert.base64EncodedString(), | ||
keyIdentifier: keyIdentifier, | ||
let assertion = try await withRetry { |
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.
Out of curiosity the withRetry
how many times does it retry? And why are we using it here? Is there a chance of failure?
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 will retry twice, and if its failed , we have a catch block to handle a special scenario
public private(set) var assertKey: String? = nil | ||
public private(set) var keyIdentifier: String | ||
public private(set) var clientDataHash: String | ||
private let key = "com.forgerock.ios.appattest.keychainservice" |
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.
Would it be useful to allow to customise this?
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.
users has the access to just to delete the key stored in keychain , i am wondering whats the advantage of customizing this . lets discuss this
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.
since we dont share the keys between apps , we dont let users customize this
@@ -174,4 +191,8 @@ public class FRAppIntegrityCallback: MultipleValuesCallback { | |||
} | |||
} | |||
} | |||
|
|||
public func isAttestationCompleted() -> Bool { |
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.
Missing doc
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.
still missing :-)
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.
done sorry, that one commit didnt pushed
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.
Overall implementation looks good to me. Some missing documentation
d206c3f
to
602082e
Compare
addressed most of the comments |
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
602082e
to
cc60ce5
Compare
self.setkeyId(result.keyIdentifier) | ||
self.setClientData(result.clientDataHash) | ||
self.appIntegritykeys = result | ||
} | ||
catch { | ||
FRLog.e("Error: \(error.localizedDescription)") |
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 setting the error to the callback, it set to the actual error or the string "ClientDeviceErrors"? For Server to route to the correct outcome, it should set it to "ClientDeviceErrors", and allow developers to customize the error.
The failure will throw to developer, and if they want to have a custom outcome, they can set using FRAppIntegrityCallback.setClientError()
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 addressed already.
edb799e
to
467df3e
Compare
467df3e
to
3b737d6
Compare
JIRA Ticket
Please, link jira ticket here.
Description
Briefly describe the change and any information that would help speedup the review and testing process.
Definition of Done Checklist: